Repository navigation
fix: recover orchestration mail after interrupted chats - #32108
brennanb2025 wants to merge 17 commits into
Conversation
…rrupted consumers
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
Priority: Merge Risk:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments: 1
src/main/runtime/orchestration/structured-mailbox-pointer-delivery.test.ts: 1
97-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the "no second send" assertion able to fail.
onJournalActivitystartsdeliver(), which awaitsreadSessionFacts,readHandoffEvidence, andselectStructuredMailHandoffbefore it reachessend. A singleawait Promise.resolve()returns before those awaits finish.The first delivery loop can also still be in flight at this point. In that case,
changedDuringFlightdefers the redelivery to a microtask afterfinally. The checkexpect(send).toHaveBeenCalledTimes(1)therefore passes even if a second send occurs. Wait for the delivery to settle before the assertion.♻️ Proposed fix
delivery.onJournalActivity('session') - await Promise.resolve() + await new Promise((resolve) => setTimeout(resolve, 0)) + await new Promise((resolve) => setTimeout(resolve, 0)) expect(send).toHaveBeenCalledTimes(1)
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
c6b77453-4339-46e6-82bd-5244caa52103
📒 Files selected for processing: 55
src/main/agent-hooks/agent-hook-runtime-deps.tssrc/main/agent-hooks/server-agent-presence.test.tssrc/main/agent-hooks/server/retained-provider-session-row.tssrc/main/agent-hooks/server/server-cleanup.tssrc/main/agent-hooks/server/terminal-mail-target.tssrc/main/native-chat/agent-session-journal/journal-dispatch-reducer.tssrc/main/native-chat/agent-session-journal/journal-reducer.tssrc/main/native-chat/agent-session-journal/journal-rewind-submission-retirement.tssrc/main/native-chat/agent-session-journal/journal-store.tssrc/main/native-chat/agent-session-journal/journal-submission-fold.tssrc/main/native-chat/agent-session-journal/journal-submission-handoff.test.tssrc/main/native-chat/agent-session-journal/journal-submission-handoff.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host.tssrc/main/native-chat/agent-session-wire/structured-mail-handoff-evidence.tssrc/main/runtime/construction/runtime-construction-dependencies.tssrc/main/runtime/construction/runtime-construction.tssrc/main/runtime/construction/runtime-service-state.tssrc/main/runtime/orchestration/agent-facing-parity.test.tssrc/main/runtime/orchestration/db/attach-orchestration-db-methods.tssrc/main/runtime/orchestration/db/contract-constants.tssrc/main/runtime/orchestration/db/messages/mail-handoff-store.tssrc/main/runtime/orchestration/db/messages/mailbox-pointer-enter-state.tssrc/main/runtime/orchestration/db/messages/message-inbox.tssrc/main/runtime/orchestration/db/messages/role-mailbox-delivery.tssrc/main/runtime/orchestration/db/orchestration-db-methods.tssrc/main/runtime/orchestration/db/schema/create-core-tables-sql.tssrc/main/runtime/orchestration/db/schema/migrate-v44.tssrc/main/runtime/orchestration/db/schema/migrate.tssrc/main/runtime/orchestration/legacy-mail-handoff.test.tssrc/main/runtime/orchestration/legacy-mail-handoff.tssrc/main/runtime/orchestration/mail-handoff.tssrc/main/runtime/orchestration/mailbox-pointer-delivery-contract.tssrc/main/runtime/orchestration/mailbox-pointer-delivery.tssrc/main/runtime/orchestration/mailbox-pointer-eligibility.tssrc/main/runtime/orchestration/mailbox-pointer-resume.tssrc/main/runtime/orchestration/mailbox-pointer-stage.tssrc/main/runtime/orchestration/mailbox-pointer-submit.test.tssrc/main/runtime/orchestration/mailbox-pointer-submit.tssrc/main/runtime/orchestration/mailbox-settlement.test.tssrc/main/runtime/orchestration/mailbox-settlement.tssrc/main/runtime/orchestration/orchestration-schema-version-skew.tssrc/main/runtime/orchestration/structured-mailbox-pointer-delivery.test.tssrc/main/runtime/orchestration/structured-mailbox-pointer-delivery.tssrc/main/runtime/orchestration/structured-mailbox-pointer-host.test.tssrc/main/runtime/orchestration/structured-mailbox-pointer-host.tssrc/main/runtime/orchestration/structured-pointer-operation-id.test.tssrc/main/runtime/orchestration/structured-pointer-operation-id.tssrc/main/runtime/orchestration/terminal-mail-settlement.tssrc/main/runtime/orchestration/terminal-pane-mail-routing.tssrc/main/runtime/orchestration/types.tssrc/main/runtime/rpc/methods/orchestration/messaging/mailbox-message-receipt.tssrc/main/runtime/structured-chat-coordinator-mail-rig.test-fixture.tssrc/main/runtime/structured-chat-coordinator-mail.test.tssrc/main/runtime/structured-chat-dispatch-worker-mail.test.tssrc/main/runtime/structured-chat-mail-recovery.test.ts
💤 Files with no reviewable changes: 1
- src/main/runtime/orchestration/structured-pointer-operation-id.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
-
⚙️ Run configuration
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
a1ad62f8-7746-4f43-89c8-53b5948a6431
📒 Files selected for processing: 38
-
src/main/acp/acp-structured-steer.test.tssrc/main/agent-hooks/server-agent-presence.test.tssrc/main/agent-hooks/server/server-cleanup.tssrc/main/agent-hooks/server/server-row-ownership.tssrc/main/agent-hooks/server/terminal-mail-target.tssrc/main/native-chat/agent-session-journal/journal-dispatch-reducer.tssrc/main/native-chat/agent-session-journal/journal-item-fold.tssrc/main/native-chat/agent-session-journal/journal-reducer.tssrc/main/native-chat/agent-session-journal/journal-rewind-submission-retirement.tssrc/main/native-chat/agent-session-journal/journal-store.tssrc/main/native-chat/agent-session-journal/journal-submission-fold.tssrc/main/native-chat/agent-session-journal/journal-submission-handoff.tssrc/main/native-chat/agent-session-wire/structured-agent-session-current-work.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-current-work.tssrc/main/native-chat/agent-session-wire/structured-mail-handoff-evidence.tssrc/main/runtime/orchestration/agent-facing-parity.test.tssrc/main/runtime/orchestration/db/messages/mail-handoff-store.tssrc/main/runtime/orchestration/db/messages/mailbox-pointer-enter-state.tssrc/main/runtime/orchestration/legacy-mail-handoff.test.tssrc/main/runtime/orchestration/legacy-mail-handoff.tssrc/main/runtime/orchestration/mail-handoff.tssrc/main/runtime/orchestration/mailbox-owner.test.tssrc/main/runtime/orchestration/mailbox-pointer-delivery.tssrc/main/runtime/orchestration/mailbox-pointer-release-query.test.tssrc/main/runtime/orchestration/mailbox-pointer-resume.tssrc/main/runtime/orchestration/mailbox-pointer-stage.test.tssrc/main/runtime/orchestration/mailbox-pointer-stage.tssrc/main/runtime/orchestration/mailbox-pointer-submit.test.tssrc/main/runtime/orchestration/mailbox-pointer-submit.tssrc/main/runtime/orchestration/mailbox-settlement.test.tssrc/main/runtime/orchestration/mailbox-settlement.tssrc/main/runtime/orchestration/structured-mailbox-pointer-delivery.test.tssrc/main/runtime/orchestration/structured-mailbox-pointer-delivery.tssrc/main/runtime/orchestration/structured-mailbox-pointer-host.test.tssrc/main/runtime/orchestration/structured-mailbox-pointer-host.tssrc/main/runtime/orchestration/structured-mailbox-pointer-receipt.tssrc/main/runtime/orchestration/terminal-mail-settlement.tssrc/main/runtime/structured-chat-coordinator-observation-clock.test-fixture.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
-
There was a problem hiding this comment.
🧹 Nitpick comments: 1
src/main/runtime/orchestration/legacy-mail-handoff.test.ts: 1
231-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the VM-step measurement skip when
python3is unavailable.The test calls
execFileSync('python3', ...)without a fallback. On a machine wherepython3is not onPATH,execFileSyncthrows and the test fails. The cause is a missing tool, not a migration defect. This can happen on a Windows developer machine, wherepython3is often absent or a Store stub.Probe for the interpreter once. Skip the step-count assertions when the probe fails. Keep the migration-state assertions in a separate case so they still run.
The
ast-grepwarning aboutchild_processis a false positive here. The call passes an argument array, andpath,migrationSqlandUNKNOWN_MAIL_HANDOFFare not shell-interpolated.Source: Linters/SAST tools
ℹ️ Review info
-
⚙️ Run configuration
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
87621856-010f-4e25-9268-714a8bfdebd1
📒 Files selected for processing: 12
-
src/main/native-chat/agent-session-journal/journal-store.tssrc/main/native-chat/agent-session-journal/journal-submission-handoff.test.tssrc/main/native-chat/agent-session-journal/journal-submission-handoff.tssrc/main/runtime/orchestration/db/messages/mail-handoff-store.tssrc/main/runtime/orchestration/db/schema/migrate-v44.tssrc/main/runtime/orchestration/legacy-mail-handoff.test.tssrc/main/runtime/orchestration/mailbox-owner.test.tssrc/main/runtime/orchestration/mailbox-pointer-delivery.tssrc/main/runtime/orchestration/structured-mailbox-pointer-receipt.tssrc/main/runtime/orchestration/terminal-mail-settlement.tssrc/main/runtime/rpc/methods/orchestration/messaging/check.test.tssrc/main/runtime/rpc/methods/orchestration/messaging/message-methods.ts
💤 Files with no reviewable changes: 1
-
- src/main/native-chat/agent-session-journal/journal-submission-handoff.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
-
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments: 1
src/main/runtime/orchestration/structured-mailbox-pointer-delivery.test.ts: 1
97-99: 📐 Maintainability & Code Quality |Trivial | ⚡ Quick win The no-redelivery assertion cannot fail.
await Promise.resolve()waits for one microtask only.deliverawaitsreadSessionFacts,reconcileLegacyMailHandoff, andselectStructuredMailHandoffbefore it reachessend. A wrongful redelivery therefore cannot callsendbefore thetoHaveBeenCalledTimes(1)assertion runs, so the test passes even if a redelivery happens. Use thesetImmediateflush that the other tests in this file use.Proposed fix
delivery.onJournalActivity('session') - await Promise.resolve() + await new Promise<void>((resolve) => setImmediate(resolve)) expect(send).toHaveBeenCalledTimes(1)

Review info
-
Run configuration-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
62dbf678-6bbb-47c1-9c98-d35efa80ea91
Files selected for processing: 17-
src/main/acp/acp-structured-steer.test.tssrc/main/agent-hooks/agent-hook-runtime-deps.tssrc/main/agent-hooks/server/server-cleanup.tssrc/main/agent-hooks/server/server-row-ownership.tssrc/main/native-chat/agent-session-journal/journal-reducer.tssrc/main/native-chat/agent-session-journal/journal-store.tssrc/main/native-chat/agent-session-journal/journal-submission-fold.tssrc/main/runtime/construction/runtime-construction-dependencies.tssrc/main/runtime/construction/runtime-construction.tssrc/main/runtime/construction/runtime-service-state.tssrc/main/runtime/orchestration/db/messages/message-inbox.tssrc/main/runtime/orchestration/mailbox-pointer-stage.test.tssrc/main/runtime/orchestration/mailbox-settlement.test.tssrc/main/runtime/orchestration/structured-mailbox-pointer-delivery.test.tssrc/main/runtime/orchestration/structured-mailbox-pointer-delivery.tssrc/main/runtime/orchestration/terminal-pane-mail-routing.tssrc/main/runtime/rpc/methods/orchestration/messaging/message-methods.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
-
|
A connection drop left the review chat holding an unacknowledged mailbox batch. Later reports were saved but could not wake it. This change lets independent fresh mail progress and ties recovery to the actual receiving input, instead of letting an old delivery block the mailbox. Eligible messages share one normal input while retaining separate immutable authorizations and recovery budgets. Original receipts are reconciled before replay. Stop or a consuming read invalidates only the affected old obligation. Held mail retains its reservation/budget and releases in eligible FIFO order. Actual routed authority and terminal recipients are revalidated; skipped Enter is not accepted, and accepted lifecycle changes permanently retire old terminal proof. Legacy missing ownership remains explicitly readable. Existing helper ownership transfer retires the former recipient's transport without resetting recovery. Review: three completed functional rounds, with all proven P0/P1 findings fixed and the final independent reference/performance conclusions carried across checked integration/type/test corrections. No new functional round was counted for CI execution turns. Production churn is 2228 (1787 additions, 441 deletions), tests/fixtures 3952, config 12 against merged main Verified: exact-head PR Checks 40211745671 passes. The conflict-blocking main integration passed the required direct merge hook and 104 targeted test executions (103 distinct cases in 11 named files). Actual host/scripted ACP/RPC integration exercises five-message check/replay/exact ack/completion (two prompts, zero cancellations, recovery Real native QA uses Claude and Codex, with saved state made by the previous build: upgrade/restart, actual HTTP200 SSE cuts, Stop/fresh mail, two real replies, held release, rewind/fork and real CLI FIFO receipts. Attachment integration passed 3/3. Native evidence head remains Terminal limit: positive exact offered-target abandonment, retained-proof restart and permanent retirement are CONTRACT TEST ONLY; REAL POSITIVE HISTORY UNEXERCISED. Current conservative UNKNOWN/explicit-read histories do not prove that positive branch or a universal impossibility. The written owner technical disposition changes only the original separate real-demonstration acceptance requirement, without changing any shipped proof, behavior or budget. No Brennan waiver is claimed. Accepted targeted terminal lifecycle: Targeted real Claude/Codex terminal lifecycle acceptance is complete after an independent raw-receipt audit. Each original notice was captured before check as unread/offered with Limits are retained: Codex's installed TUI exited while its backend daemon survived until separate cleanup, so this is not daemon-death proof. Claude tool receipts have no numeric exit field; its fresh ack/empty calls were submitted together, so invocation order is not guaranteed. The real restart screenshots retain their onboarding/session-search overlays, corroborated by read-only state; the resumed Claude PID was not separately recorded. Codex's initial sandbox EPERM and subsequent ordinary approval are retained. Neither target was retrospectively reassigned after Enter. The original port-only FAILED report is preserved with an appended factual correction: all configured ports were9500/9501 inside the assigned9500–9519 range; stock hook listeners request port0, permitted by the existing OS-assigned-listener rule. This was neither a waiver nor a rerun. The audit confirms all42 recorded process identities, keepers and four driver sessions stopped, four graceful app stops, configured/recorded hook ports free and seven protected hashes unchanged. Ticket2202 is released and the lead terminal is closed. Deferred / limits: the ACP test teardown Final dispositions of the five automated review threads (recorded here before resolution; no source changes):
Later commits and evidence carry:
For Brennan's review Three source locations where judgment matters:
Existing QA screenshots — original outcomes and qualifications retained (30)codex / upgrade-baseline — BASELINE_NO_WAKE codex / upgrade-native-reply-fifo — PASS_WITH_RETAINED_SETUP_CORRECTION codex / native-stop — PASS codex / actual-sse-cut-recovery — PASS codex / bounded-recovery-exhaustion — PASS_HTTP400_ONLY codex / graceful-restart-two-person-replies — PASS codex / graceful-restart-fresh-mail — PASS codex / manual-terminal-fifo — PASS claude / upgrade-baseline — BASELINE_NO_WAKE claude / upgrade-native-reply-fifo — PASS_WITH_RETAINED_SETUP_CORRECTION claude / native-stop — PASS claude / actual-sse-cut-recovery — PASS claude / bounded-recovery-exhaustion — PASS_HTTP400_ONLY claude / graceful-restart-two-person-replies — PASS claude / graceful-restart-fresh-mail — PASS claude / manual-terminal-fifo — PASS claude / native-held-prompt — PASS claude / native-held-release — PASS_WITH_SAME_ROOT_QUALIFICATION codex / native-held-prompt — PASS codex / native-held-release — PASS claude / rendered-rewind-confirm — PASS claude / rendered-rewind-edited-reply — PASS codex / rendered-continue-with-terminal-fork — PASS claude / native-clear — PASS claude / prompt-cancel-clear-unacked-preservation — PASS_ADMITTED_UNACKED_ONLY codex / manual-terminal-stop — PASS codex / manual-terminal-fresh-after-stop — PASS claude / manual-terminal-fresh — PASS_FRESH_ONLY_STOP_UNKNOWN claude / supported-terminal-exact-proof-checkpoint — NOT_PASSED_MISSING_OFFERED_TARGET claude / terminal-normal-mail-after-root-completed — PASS_ORDINARY_ONLY |
|
Closing: superseded. The original problem (an unacknowledged batch blocking every later report from waking the chat) is fixed on main by #33639. Native-chat report delivery is moving to the ordinary chat message path in #28567, where a delivered report is the chat's message like any other, so the per-report handoff tracking and recovery allowance here are no longer needed. Turns that die on a temporary provider failure will be handled by automatic retry in a separate PR. The branch is kept for now because #28567 currently targets it; it will be retargeted to main before the branch is deleted. |












































A review chat stopped after a connection drop while holding an unacknowledged orchestration batch. Later reports were saved but could not wake it: the previous batch blocked delivery, and an accepted notice no longer identified the turn responsible for it.
Fresh mail now wakes chats independently of earlier unacknowledged batches. Eligible messages share one ordinary chat input, while each keeps its own immutable authorization and recovery allowance. An unread notice can receive one automatic recovery only when the exact original receiving turn or terminal process supplies the required failure evidence. Stop and consuming reads invalidate that old permission; independent fresh mail can still progress. Existing FIFO delivery IDs, replay and exact acknowledgements remain intact.
The implementation reconciles the original receipt before replay, validates the actual local or attached mailbox authority, and rechecks the terminal recipient before writing and Enter. Held mail keeps its reservation and recovery budget, then redrives in original eligible order on the real release edge. Skipped Enter is never reported as accepted. Accepted lifecycle changes permanently retire old terminal proof. Legacy mail with missing ownership remains explicitly readable and is never guessed into a fresh automatic send. Helper settlement also clears only the former recipient's handoff when existing ownership transfers unread mail back to its Run; the recovery allowance is preserved.
Only two private message columns are added; no new wire operation, journal/history field or minimum paired-server version is introduced. The common pattern settles the actual ordinary input. Intended differences preserve external FIFO consumers, independently versioned hosts and exact terminal-process evidence. Pointer content and timing work remains tracked in #28567.
Validation on reviewed head
6b9ce8b650fda17d6ed3989fcb740d2a9c04de4f:09873c1442914d8d78c7ca03e8a2b6090e502eee, within the binding 2300 ceiling; existing.specpaths remain production.367904836c0preserves upstream composer/dialog holds, Cursor Enter submission and relocated sources alongside the reviewed receipt/authorization/authority contracts. All 13 conflicts and narrow automatic overlaps checked; 104 targeted test executions (103 distinct cases in 11 named files) and the required direct merge hook passed. Main integration carries accepted QA; no broken run-row flow was established. Follow-up6b9ce8b650fcompletes only the incoming dialog test fixture’s numeric pane identity; its 12-case named suite and normal pre-commit hook passed, with runtime source unchanged.[1,0,0,0,0]. N/10N contracts count underlying SQLite work and journal operations. These establish stated population-dependent bounds, not constant cost or a latency benchmark.6342786b7cde9f4ec917b7e2f9999c5922bf40d3carries: previous-build state upgrade/restart, actual HTTP200 SSE cuts and recovery, Stop with independent fresh mail, two real replies after restart, held release, rewind/fork, compiled CLI receipts and attachment integration (3/3). Later main integration, equivalent/type-only corrections and non-shipped changes do not stale those flows. The helper-recipient transfer is a CLI/orchestration tests-row change with actual DB/RPC reopen coverage.Terminal coverage is deliberately precise: positive exact-target abandonment, retained restart proof and permanent retirement have contract-test coverage only; a positive real history remains unexercised. Current CLI idle-start or unavailable-proof cases stay conservative and explicitly readable. This is not a real positive pass or a claim of impossibility. The owner has supplied a written technical acceptance disposition without changing shipped behavior or the proof contract.
Targeted real Claude/Codex terminal lifecycle acceptance is complete after an independent raw-receipt audit. Each original notice was captured before check as unread/offered with
target:nulland recovery use0. The same original identity, delivery time and budget survived exact installed CLI exit and graceful same-state app restart. Each real agent then used the compiled CLI for original check, same-Delivery/member replay, exact acknowledgement, empty result and a content reply; each independently handled one later automatic fresh notice through the same protocol and a real reply. No external consuming check was used.Limits are retained: Codex's installed TUI exited while its backend daemon survived until separate cleanup, so this is not daemon-death proof. Claude tool receipts have no numeric exit field; its fresh ack/empty calls were submitted together, so invocation order is not guaranteed. The real restart screenshots retain their onboarding/session-search overlays, corroborated by read-only state; the resumed Claude PID was not separately recorded. Codex's initial sandbox EPERM and subsequent ordinary approval are retained. Neither target was retrospectively reassigned after Enter.
The original port-only FAILED report is preserved with an appended factual correction: all configured ports were9500/9501 inside the assigned9500–9519 range; stock hook listeners request port0, permitted by the existing OS-assigned-listener rule. This was neither a waiver nor a rerun. The audit confirms all42 recorded process identities, keepers and four driver sessions stopped, four graceful app stops, configured/recorded hook ports free and seven protected hashes unchanged. Ticket2202 is released and the lead terminal is closed.