Repository navigation
feat(cloud-agent-next): coalesce adjacent part deltas in outbox - #6274
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryFull re-review after a force-push (previous head Files Reviewed (11 files)
Previous Review Summary (commit ee9f854)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit ee9f854)Status: No Issues Found | Recommendation: Merge Executive SummaryReviewed the outbox delta-merge rule and the new control-event diagnostics end to end; the merge predicate compares the full non- Files Reviewed (5 files)
Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0 Review guidance: REVIEW.md from base branch |
…ed batch items
Three rejection paths in applySandboxControlEvent returned a bare
{ applied: false } and emitted no diagnostic, and a caught exception while
applying a batch item emitted nothing, so an event that did not apply could
not be attributed from logs alone.
Return the id-mismatch and child-lineage rejections through the existing
result() helper so they emit session_event_result, and emit one warn-level
session_event_batch_item_failed per caught batch item with its identity and
index.
A caught item records that item as unknown and the batch continues, so one
bad event can no longer discard the rest of the batch. Terminal events such
as session.turn.close and session.idle are therefore still applied when an
earlier item throws.
The item diagnostic never reads the thrown value and is built inside a
containing try/catch, so it cannot leak payload content or change the batch
result. Applied, rejected and pre-RPC unattempted outcomes and retry
semantics are unchanged.
…outbox The outbox had one compaction rule, and it was a supersede rule keyed on an entity id. message.part.delta carries an increment rather than a snapshot, so it produced no key and could never be compacted: a delta burst grew the lane one entry per delta until admission failed and the event was dropped with queue_overflow at MAX_CONTROL_EVENT_OUTBOX_EVENTS. Add an append rule beside the supersede rule. Adjacent same-part deltas for field 'text' merge by concatenating delta, keeping the tail entry's receiptId, sequence and preparedAt so the existing receipt identity still describes the published frame. Both rules share the in-flight tail guard, so an entry that a pending attempt will remove is never replaced. A merged entry takes the newer member's deadlineAt rather than the older one. Retaining the older deadline would expire a still-growing group and lose its fresh text, which survives today because the newer delta would carry its own deadline. The refresh is deliberate: an actively-merged group is bounded by the outbox entry and byte limits instead of the 30s clock. Snapshot squash keeps its previous deadline, so supersede semantics are unchanged. Merging is declined when the merged payload would exceed MAX_SANDBOX_CONTROL_FRAME_BYTES, and the delta is appended as its own entry instead. That fallback is not unconditionally lossless: at the entry cap the append can still be dropped as queue_overflow, which is correct because an over-frame replacement was never sendable. Merging is restricted to field 'text'; no wire protocol, DO, SDK or schema change is involved.
…ation deadline The per-session forward queue admitted at most 32 in-flight operations, so a burst of batch frames, each carrying up to SANDBOX_EVENT_BATCH_MAX_ITEMS events, could exhaust it and drop later frames. Bound the forward queue with SANDBOX_CONTROL_FORWARD_OPERATION_LIMIT = 2048. The existing frame budget (MAX_SESSION_FORWARD_BYTES) remains the memory bound, and SANDBOX_CONTROL_OPERATION_LIMIT stays at 32 where it still guards stop receipts and recovery arrays. Raise MAX_CONTROL_EVENT_OUTBOX_EVENTS from 256 to 2048 so the wrapper outbox and its receipt tracking share one entry budget, and raise PUBLICATION_DEADLINE_MS from 30s to 60s so an in-flight publication is not expired by a short reconnect.
ee9f854 to
134561a
Compare
| outcomes.push({ receiptId: item.receiptId, status: 'unattempted' }); | ||
| continue; | ||
| } | ||
| for (const [index, item] of input.items.entries()) { |
There was a problem hiding this comment.
bot: Continuing after an unknown item can apply same-session events out of order.
Suggested fix: Restore the halt guard after the catch: leave the throwing item as unknown, mark every remaining item unattempted, and emit one bounded session_event_batch_halted diagnostic for the failed item plus suffix shape. This keeps later deltas/snapshots/completions from being applied after a predecessor whose mutation outcome is indeterminate. The changed test at session-message-queue.test.ts:2226-2275 should again assert that the suffix is unattempted rather than applied.
2 participants
Summary
The wrapper control-event outbox could only compact by superseding a snapshot entry keyed on an entity id.
message.part.deltacarries an increment, not a snapshot, so it had no key and could never be compacted: a delta burst grew the lane one entry per delta until admission failed and the event was dropped asqueue_overflowatMAX_CONTROL_EVENT_OUTBOX_EVENTS.textmerge by concatenatingdelta, keeping the tail entry'sreceiptId,sequenceandpreparedAt, so the existing receipt identity still describes the published frame.deadlineAt, so an actively-growing group is not expired mid-burst; snapshot squash keeps its previous deadline, so supersede semantics are unchanged.MAX_SANDBOX_CONTROL_FRAME_BYTES; the delta is appended as its own entry instead. At the entry cap that append can still be dropped asqueue_overflow, which is correct because an over-frame replacement was never sendable.result()and emitsession_event_result, and a caught batch halt emits one warn-levelsession_event_batch_haltedwith the halting item's identity and the skipped suffix shape. The halt diagnostic never reads the thrown value.text; no wire protocol, DO, SDK or schema change is involved.Verification
No manual testing performed. Automated checks run on the branch:
bun test src/control/control-event-outbox.test.ts(wrapper): 76 pass, 0 failpnpm exec vitest run src/sandbox-session/session-message-queue.test.ts src/sandbox-control/diagnostics.test.ts: 362 pass, 0 failVisual Changes
N/A
Reviewer Notes
The deadline refresh for merged groups is deliberate: an actively-merged group is bounded by the outbox entry and byte limits instead of the 30s clock.