fix(agent): stop one oversized model call context disabling a run's mirror - #3496
Conversation
…irror An oversized context threw before anything was written, and the sink's catch disposes the mirror on any error. Because the mirror's own handleChunk and appendEvents return early when disabled rather than throwing, every later event in that run was then dropped in silence — no throw, no log, no metric — while the run carried on and could still report completed. Keep the gate. The model must not be dispatched when its context cannot be recorded faithfully, and the pre-existing dispatches === 0 contract still holds. What changes is what an operator is left with: - the context is reduced to fit and persisted, stamped truncated with originalByteLength and omittedMessageCount, so the attempt is on the record and can never be mistaken for a faithful one - the mirror survives, so the run's later events, including its failure, persist - the refusal names the actual size, the limit, that the model was not called, and what to reduce; a warn carries the same fields Newest messages are kept: text is clamped in decreasing passes, then the oldest messages are dropped, then a guaranteed-fit fallback keeps only the envelope. Refs veryfront/veryfront-issue-inbox#421
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughOversized durable run events are reduced to fit the append limit, persisted with truncation metadata, and then refused for model dispatch. Tests verify persistence, error details, mirror usability, and fail-closed dispatch behavior. The resolver test narrows request header access. ChangesDurable event handling
Request mock typing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DurableRunEventSink
participant Append
participant agentLogger
participant ModelDispatch
DurableRunEventSink->>Append: persist truncated audit record
Append-->>DurableRunEventSink: persistence succeeds
DurableRunEventSink->>agentLogger: log size and omission details
DurableRunEventSink--x ModelDispatch: refuse oversized dispatch
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0fd860ff3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| truncated: true, | ||
| originalByteLength, | ||
| omittedMessageCount, |
There was a problem hiding this comment.
Whenever a model-call context exceeds the append limit, these added fields make the audit event invalid in the production mirror path. ConversationRunChunkMirror.appendEvents passes it through prepareConversationRunExternalEvents, where isPrivateConversationRunEvent permits only type, messages, and tools; normalization therefore throws Invalid private run event shape before enqueueing anything, and this sink catches that error and disposes the mirror. The new tests miss the regression because their stub appendEvents bypasses normalization. Update the private-event schema and its consumers to accept the audit metadata, or encode the truncation without adding rejected fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and severe — fixed in 3e65e69.
private-run-event.ts:23 is a strict allowlist:
return Object.keys(value).every((key) => key === "type" || key === "messages" || key === "tools")So the three fields I added would have failed isPrivateConversationRunEvent, normalizeConversationRunEvent would have thrown Invalid private run event shape inside appendEvents, this sink would have caught that as a persistence failure, and the mirror would have been disposed. The fix would have caused the exact loss it exists to prevent, on every oversized context.
Your point about the tests is the reason it got through: the stub appendEvents never normalizes, so nothing exercised the real path.
The marker now lives in a leading system message instead, so the event keeps exactly the three allowed keys. It still states the original size, the limit, how many messages were omitted, and that the record is an excerpt rather than the context that was sent.
Guarded two ways now: a test that pushes the persisted record through prepareConversationRunExternalEvents — the real production path — and direct isPrivateConversationRunEvent assertions. Verified load-bearing: adding a single stray top-level key fails three assertions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/agent/hosted/durable-run-event-sink.ts (2)
102-103: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFix the comment and drop the duplicate size measurement.
Two small points in this function:
- Line 116 states that each pass halves the budget. Line 120 divides by four. Correct the comment.
- Line 103 recomputes the append byte length that
resolvePersistableEventalready computed at line 181. Each call serializes the whole oversized event, which is at least 10 MiB. Pass the known value in.♻️ Proposed refactor
-function truncatePrivateRunEventToLimit(event: Record<string, unknown>): Record<string, unknown> { - const originalByteLength = getPrivateRunEventAppendRequestByteLength(event); +function truncatePrivateRunEventToLimit( + event: Record<string, unknown>, + originalByteLength: number, +): Record<string, unknown> { const messages = Array.isArray(event.messages) ? event.messages : [];- // Clamp text progressively; each pass halves the per-part budget. + // Clamp text progressively; each pass quarters the per-part budget.Update the call site:
- const truncated = truncatePrivateRunEventToLimit(event); + const truncated = truncatePrivateRunEventToLimit(event, requestByteLength);Also applies to: 116-121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent/hosted/durable-run-event-sink.ts` around lines 102 - 103, Update truncatePrivateRunEventToLimit to accept the already computed append-request byte length from resolvePersistableEvent, and use that value instead of calling getPrivateRunEventAppendRequestByteLength again. Correct the truncation-loop comment to describe division by four rather than halving the budget, while preserving the existing truncation behavior.
79-89: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueText clamping skips string
contentand non-text parts.
truncateMessageTextPartsonly reduces parts that carry a stringtextfield inside an arraycontent. Two common shapes escape clamping:
contentas a plain string.- Non-text parts, such as base64 media or tool-result payloads.
Correctness still holds, because the message-removal loop and the envelope fallback guarantee a fit. The cost is wasted work: when the oversize comes from those shapes, all five clamping passes run and serialize the full event without reducing anything. Handling string
contentis a small addition that covers a frequent case.♻️ Optional: clamp string `content`
function truncateMessageTextParts(message: unknown, maxTextBytes: number): unknown { - if (!isRecord(message) || !Array.isArray(message.content)) return message; + if (!isRecord(message)) return message; + if (typeof message.content === "string") { + return { ...message, content: truncateTextToBytes(message.content, maxTextBytes) }; + } + if (!Array.isArray(message.content)) return message; return {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent/hosted/durable-run-event-sink.ts` around lines 79 - 89, Update truncateMessageTextParts to also truncate message.content when it is a string, using truncateTextToBytes with maxTextBytes. Preserve the existing array-of-parts handling and leave non-text parts unchanged.src/agent/hosted/durable-run-event-sink.test.ts (1)
210-224: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThis test is expensive to run.
The event holds 120,000 messages. The reducer maps all of them and serializes the full event once per clamping pass, five times, before message removal starts. That is several hundred megabytes of transient allocation and a large CPU cost for one unit test.
The assertion needs a message count that exceeds the budget on its own, so the count must stay large. You can still cut the cost by lowering the per-message text to a few characters and raising the count only as far as the budget requires. Measure the current runtime first, and keep the test as is if it stays fast.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent/hosted/durable-run-event-sink.test.ts` around lines 210 - 224, Reduce the expensive fixture in the “guarantees a fit when message count alone exceeds the budget” test while preserving its purpose: use only a few characters per message and the minimum message count needed to exceed the persistence budget. Measure the test runtime first and leave it unchanged if it remains fast; otherwise adjust the existing Array.from message fixture without changing the rejection assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/agent/hosted/durable-run-event-sink.ts`:
- Around line 152-160: Make the fallback in the event-clamping logic truly fit
the byte limit: after constructing the current fallback, measure it and, if
still oversized, return a minimal envelope containing only the required event
identity fields, replacement system message, and empty tools when applicable.
Add a test that makes a non-messages field such as providerOptions or metadata
oversized and verifies the final result is within
MAX_CONVERSATION_RUN_EVENT_APPEND_REQUEST_BYTES.
- Around line 106-114: Update the private-event validation used by
prepareConversationRunExternalEvents and isPrivateConversationRunEvent to accept
the truncation metadata fields truncated, originalByteLength, and
omittedMessageCount, while preserving validation of all existing event fields so
stamped audit records pass normalization and persist.
---
Nitpick comments:
In `@src/agent/hosted/durable-run-event-sink.test.ts`:
- Around line 210-224: Reduce the expensive fixture in the “guarantees a fit
when message count alone exceeds the budget” test while preserving its purpose:
use only a few characters per message and the minimum message count needed to
exceed the persistence budget. Measure the test runtime first and leave it
unchanged if it remains fast; otherwise adjust the existing Array.from message
fixture without changing the rejection assertion.
In `@src/agent/hosted/durable-run-event-sink.ts`:
- Around line 102-103: Update truncatePrivateRunEventToLimit to accept the
already computed append-request byte length from resolvePersistableEvent, and
use that value instead of calling getPrivateRunEventAppendRequestByteLength
again. Correct the truncation-loop comment to describe division by four rather
than halving the budget, while preserving the existing truncation behavior.
- Around line 79-89: Update truncateMessageTextParts to also truncate
message.content when it is a string, using truncateTextToBytes with
maxTextBytes. Preserve the existing array-of-parts handling and leave non-text
parts unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a85e4bb-caee-4112-a3ca-76fd7740bab0
📒 Files selected for processing (2)
src/agent/hosted/durable-run-event-sink.test.tssrc/agent/hosted/durable-run-event-sink.ts
…ields isPrivateConversationRunEvent allows only type, messages and tools (private-run-event.ts:23). The truncated, originalByteLength and omittedMessageCount fields the previous commit added made the audit record fail that check, so normalizeConversationRunEvent would have thrown "Invalid private run event shape" inside appendEvents, the sink would have caught it as a persistence failure, and the mirror would have been disposed — reintroducing the exact loss this change exists to prevent. The stub appendEvents in the tests never normalizes, so nothing caught it. Move the marker into a leading system message instead: it states the original size, the limit, how many messages were omitted, and that the record is an excerpt rather than the context that was sent. The event keeps exactly the three allowed keys. Also: the last-resort fallback now drops tools and asserts it fits rather than spreading the original event and hoping, and the reused append-request byte length is passed in instead of recomputed over a 10 MiB payload. Guarded by a test that runs the persisted record through prepareConversationRunExternalEvents — the real production path — and by direct isPrivateConversationRunEvent assertions. Adding any stray top-level key fails them.
Three problems the ci (lint) chain caught that local --no-check runs could not: - appended[0][0] and the notice-message walk violated noUncheckedIndexedAccess; both go through small helpers that fail loudly instead of indexing blind - the sink returns void | Promise<void>, so .then() is not available; the normalization test is async/await now - assertRejects yields unknown, so error.message needed assertInstanceOf first Importing the real private-event validator into this test widened the type graph enough to surface a latent error in project-reference-resolver.test.ts, where init is a union of RequestInit variants and .headers is not on every branch. Narrowed with an `in` check rather than dropping the import: asserting against a hand-copied allowlist instead of the real validator is what let the shape bug through in the first place. Full ci (lint) chain and verify:quick both clean.
Fixes veryfront/veryfront-issue-inbox#421.
Problem
assertSupportedEventSizethrew before anything was written, and the sink's catch disposes the mirror on any error:The damage is what happens next. The mirror's own methods return early when disabled rather than throwing (
run-chunk-mirror.ts:180-199), and the main streaming path goes throughhandleChunk. So after one oversized context the run kept executing and every later event was dropped in silence — no throw, no log, no metric — while the run could still reportcompleted.That is the "run finishes with a hole in its history and the UI waits forever" shape, with nothing anywhere recording that it happened.
What is deliberately unchanged
The gate stays. This sink is the persist-context-before-dispatch mechanism from #3309. Its oversize rejection is not only a size check — it is what stops a model being called when the call cannot be recorded.
rejects append failures and a request over the general body limitassertsdispatches === 0, and that assertion is untouched and still passing.An earlier draft of this change let the truncated context through and allowed the dispatch. That quietly traded a fail-closed audit guarantee for run completion, which is a bigger decision than this issue. Reverted.
What changes
Only what an operator is left holding after a refusal:
truncated: truewithoriginalByteLengthandomittedMessageCount. It cannot be mistaken for a faithful record — that is the whole point of the stamps, since a truncated model call context still parses as a valid one.warncarries the same fields structured.Reduction keeps the newest messages, which is what someone reading a failed run wants: text is clamped in decreasing passes, then the oldest messages are dropped, then a guaranteed-fit fallback keeps only the envelope.
Tests
11 passing. Updated two that encoded the old behaviour, and added:
The pre-existing
dispatches === 0assertion is kept verbatim and extended to also assert the mirror was not disposed.deno task verify:quickexit 0.Note on this issue's history
#421's description was written from too shallow a read and has been corrected three times on the issue: the abort path is narrower than I first claimed, truncation machinery already existed, and private run events are exempt from it by design. The defect that survived all of that is the one fixed here — the silent loss of everything after one oversized event.
Summary by CodeRabbit