fix(mothership): end refused replay turns cleanly and bound oversized stream events - #8450
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 36 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 37 files
Confidence score: 4/5
- In
apps/sim/lib/mothership/request/session/replay-budget.integration.ts, the tests could miss a regression that removes replay-batch splitting, allowing oversized combined batches to go unnoticed; add a case where individually writable frames exceed the batch limit when serialized together.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/mothership/request/session/replay-budget.integration.ts">
<violation number="1" location="apps/sim/lib/mothership/request/session/replay-budget.integration.ts:222">
P2: A regression that removes replay-batch splitting would still pass these tests because they never exercise an oversized batch. Add a case with multiple individually writable frames whose combined serialized size exceeds one write, and verify all frames persist.
(Based on your team's feedback about preview frame and replay batch bounds.) .</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Fix all with cubic | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 40 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 40 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
|
@cubic-dev-ai review this PR |
… handing them off
A leased Chat stream persists every event to its Redis replay buffer before
delivering or dispatching it. When the buffer refused a write (a frame over
the 1 MiB single-write ceiling, or a stream past its 32 MiB budget), the
writer threw a generic error, the controller classified it as a takeover and
released its lock without finalizing, and every reconnect started a recovery
controller that re-received and re-refused the same event until the run
deadline. The refused tool call was never dispatched, so the worker waited.
Refusals:
- The writer throws StreamReplayBudgetExhaustedError and rejects later
publishes the same way, but still delivers the turn's terminal error and
complete events unpersisted; flush no longer rethrows the refusal.
- The controller aborts with that error rather than a supersession, the
lifecycle classifies it as an error (not a user cancel), the run is
finalized as an error with code replay_budget_exhausted, and the worker is
told to stop. Reconnects then see a terminal run and start no controller.
- Only a genuine StreamControllerSupersededError still hands the run off.
Compaction, applied only to the copy the writer delivers and persists:
- Payloads over 256 KiB have long string leaves cut to an 8 KiB head in
place, then medium strings to 500 characters, and only then the largest
unprotected subtrees replaced by { omitted, bytes }, targeting 128 KiB.
A payload-level streamTruncation marker lists every changed field.
- Identity fields, UI-read keys, arguments of client-executable calls, file
preview events, and generate_api_key results are never compacted; such a
frame that stays too large is refused and ends the turn cleanly.
- Oversized assistant text splits into contiguous exact pieces, and each tool
call forwards only the head of its argument deltas.
A call frame whose whole argument object was omitted by stream compaction no longer replaces a tool node's arguments, and the permission card and generic tool output render omitted values as "Too large to show (N KB)" instead of raw marker JSON.
…ation pass Compaction is now a single linear walk: when a payload's strings could serialize past 256 KiB, string leaves longer than 8K units are cut to their head with an inline "…[truncated, N total]" note, copying only what changes. Identity and UI-read keys, file previews, generate_api_key results, and the arguments of calls the browser executes are never cut; anything still over the write ceiling is refused and ends the turn. The subtree omission, second cut pass, truncation marker, text splitting, argument-delta cap, and the UI stub handling are removed. One predicate, isClientExecutedToolCall, now names the tool calls the browser starts from the call frame; both the stream compaction and the client dispatch use it, and the workflow tool names move beside it. Also fixes three defects in the refusal path: - A superseded controller that is refused an oversized frame no longer expires its successor's stream or clears its abort marker; teardown keys on whether this controller handed off, not on the abort reason. - A refusal after a user Stop stays a cancellation: the abort reason decides. - The per-user hourly ceiling gets its own message instead of telling the user to continue immediately.
…rrored-turn spinners - A replayed call frame is history and never gates anything, so Sim now clears the worker's awaiting_approval stamp on it as it already does for live frames. Replays no longer render Allow/Skip cards when approvals are off. - Stop settles every unfinished tool row (pending, executing, or awaiting approval) through one shared predicate, isUnsettledToolState, on the persisted, live, and pending-message paths. - An errored turn settles its unfinished tool rows as errored when it is persisted, so they no longer reload as spinners. - A failed backend response no longer puts the upstream body in the error shown to users: 5xx and non-JSON bodies say the service is temporarily unavailable, 4xx uses the worker's displayMessage or a generic message. The status and body stay on the error for logs.
…ot be gated The worker stamps awaiting_approval on resolved integration calls and relies on Sim to keep or clear it. Sim cleared it on live complete frames that were not gated, but a partial frame returned before that decision and kept the stamp. Only a live, complete call frame can be held behind a prompt, so every replayed or partial frame now drops the stamp at the one point each frame passes through before it is persisted and delivered. With approvals off, no frame, replay, snapshot, or persisted row can carry the stamp to the client.
…n up only ended turns - An unreachable worker and a reachable one now have independent retry budgets. Any answer from the worker restarts only the two-minute unreachable window; the three-retry, 30 s budget for failures of a worker that answered keeps its original semantics, so a failure that repeats after every reattach still stops, and a long outage no longer spends the replacement's retries. - Intermediate file preview content is capped at 8M characters per edit; past it the preview holds until the final snapshot, which is always sent. - A controller cleans up its stream only when it finalized the turn and still holds the lock, so a run it leaves recoverable keeps its buffer and any pending Stop. - Remove the unreachable generate_api_key compaction exemption, avoid a second copy of trimmed arrays, and correct the size-estimate comment.
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
… previews per turn in bytes - A controller that ended its turn but could not publish the terminal events still cleans up its stream: the run is settled either way, so the buffer no longer lingers for its full TTL. Only a superseded controller skips cleanup, and the lease check still protects a successor. - The intermediate file preview cap is now 8 MiB of UTF-8 per stream loop, across all of its edits, instead of characters per edit. Each edit's final snapshot is still always sent.
…row unreachable retries - A file preview frame whose content would not fit one replay write, or that would exceed the turn's preview budget, is skipped instead of sent: the preview holds its last content and the client loads the stored file on completion. A completion too large to send whole drops its tool output. The per-frame limit derives from the replay buffer's single-write ceiling, and the 8 MiB budget now covers every preview frame in the turn, finals included, across all legs. A large file preview no longer ends the turn. - Only a request that fails before any response headers counts as an unreachable worker. A TypeError raised after the response began, including one from handling delivered events, keeps the three-retry budget, so a deterministic failure can no longer retry until the run deadline. - A worker that cannot be reached is reported with the generic "temporarily unavailable" message; the network error stays on the error's cause. - Retry logs and spans count retries from both budgets.
… and persist closed lanes - An event whose many short values survive every string and array cut, such as an object with thousands of keys, now has its largest bulk field replaced by a size note when it would still exceed one replay write, instead of ending the turn. Identity fields and client-executed arguments are never replaced; such an event is still refused cleanly. - finalizeResidualToolCalls reports closing an open subagent lane as a change, so the error finalizer persists it instead of leaving a stale lane.
…che a skipped preview as file content - The last-resort omission for an event no cut can bound now walks down from the largest field while one child holds most of its parent's bytes and replaces only that node, so the ids, resources, and status fields beside the bulk keep driving the UI's resource updates. - On completion the client seeds the file content cache from preview text only when it received the completed version. When the server skipped the final content frame, the text is an earlier draft, so the stored file loads instead.
A result with more than one large object of short fields left the second one over the write limit after the first was omitted, so the event was refused and the turn ended. Omission now repeats until the event fits or nothing large enough is left; each pass removes at least 64 KiB, so it always terminates.
…id-body stream cuts - Replace the replay-compaction omit loop with one bounded pass over memoized exact serialized sizes: recurse into the smallest child that covers the overage, otherwise drop the largest children whole, keeping small siblings - Map a non-abort body-read failure from the worker stream to a retryable WorkerStreamInterruptedError on the reachable budget, with the generic message and the original error on cause - Log the cause message on retry warnings and orchestration failures - Count replayed file preview content toward a restored turn's preview budget
70129f6 to
7aa6002
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 41 files
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Fix all with cubic | Re-trigger cubic
…eview - Share the replay write ceiling between compaction and file previews - Scan for the smallest sufficient child only once one can cover the need, and reuse one size memo across every compaction step - Stop retrying a raw TypeError; fetch and body-read failures are already wrapped as WorkerUnreachableError and WorkerStreamInterruptedError - Use absolute imports, a private retry counter, a typed unsettled-state set, and hasUnsettledTool naming - Cover a timed-out body read and the preview completion edge cases
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 41 files
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Fix all with cubic | Re-trigger cubic
…s on every save - Compact every preview phase except content and completion, which the preview adapter already bounds; a model-written patch search string could otherwise exceed one replay write in an edit_meta frame - Settle unfinished tool rows as stopped in buildPersistedAssistantMessage for a cancelled turn, so background and API callers that persist the result directly never save pending, executing, or awaiting-approval rows
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 41 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
This branch was previously deployed
1 participant
Summary
Long Chat turns could stall until the 60-minute run limit, or break on large tasks. The root cause is the replay buffer fence added in Mothership v1.0.0. Several related failures surfaced around it; this PR fixes all of them. The worker side is in simstudioai/mothership#552.
Replay buffer refusals no longer stall a turn
StreamWriter.publishpersists every event to the Redis replay buffer before delivering it or dispatching its tool. A budget refusal (1 MiB per write, 32 MiB per stream, 128 MiB per user per hour) threw a generic error thatstart.tstreated as a controller handoff. Nothing was delivered, the tool never ran, the run stayed recoverable, and every reconnect started a new controller that was refused again until the run deadline.StreamReplayBudgetExhaustedError. The turn ends as an error with an honest message, the run is marked terminal before the lock is released, and the worker is told to stop. Only a genuine takeover still hands off.Oversized events are bounded before the fence
replay-compaction.tsworks on the copy that is both delivered and persisted, so live and replay stay identical. Dispatch still uses the full event.…[truncated, N total]note.isClientExecutedToolCall, also used by client dispatch), file-preview events, and assistant text, whose length is part of the text receipt.…[omitted, N total]note in one pass over exact, memoized sizes. Fields beside it (ids, resources, status) are kept.Related fixes
awaiting_approvalis cleared on replayed and partial frames. Only a live, complete frame can be gated, so Allow/Skip cards never appear while tool permissions are off. The gate itself is unchanged.isUnsettledToolState. Stored messages render any leftover rows as interrupted instead of spinning.messageId, which the worker handles as a duplicate send.Testing
replay-budget.integration.tsagainst real Redis and Postgres covers:bun run lint,bun run type-check,bun run check:auditsand the fullapps/simsuite pass.