Skip to content

Keep orchestration notices flowing after an unacknowledged mail batch - #26351

Closed
brennanb2025 wants to merge 4 commits into
mainfrom
brennanb2025/mail-notice-unacked-batch
Closed

brennanb2025 wants to merge 4 commits into
mainfrom
brennanb2025/mail-notice-unacked-batch

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

ELI5

A worker could silently stop hearing from its coordinator after reading one mail batch without acknowledging it. That batch locked notice delivery for the whole mailbox, so later messages piled up while the worker sat idle.

New mail now gets the normal notice even when an earlier fetched batch still needs acknowledgement. A repeated check explains which batch is being replayed, how much unread mail waits behind it, and the exact acknowledgement command that reaches the next batch.

What Changed

  • Both terminal and native chat notices derive eligibility from the particular messages already fetched, rather than a mailbox-wide outstanding flag. Selection applies those exclusions before the existing batch limit.
  • Terminal submission rechecks its original rows after asynchronous writes and liveness checks, preserving an unfetched tail without submitting the fetched rows twice.
  • Native chat derives original notice ownership from the existing queue and linked journal submissions before selecting new mail. That owner survives a fresh handoff, carry or withdrawal; failed delivery bookkeeping cannot create a second card or provider turn. Independent later mail still proceeds.
  • Explicit acknowledgement, original-batch replay and coordinator/worker read ordering remain intact. Replacing a worker still prevents it from acknowledging its replacement's mail. Replay recovery appears in text and JSON; older hosts can omit the unread count.

Why

Acknowledgement records that a particular batch was processed; it cannot safely decide whether the mailbox may announce unrelated new work. Deriving exact membership fixes both lanes without another stored flag, expiry timer, implicit acknowledgement or repeated idle reminders.

The common pattern separates waking a consumer from ownership of particular work. The alignment is partial. Intended differences: Orca keeps explicit whole-batch acknowledgement and its existing host queue instead of adding expiring execution claims; existing arrival, restart and idle events drive recovery rather than a periodic sweep. A legacy notice whose original message IDs are missing can be replaced only after explicit withdrawal or its own current turn ends, so unresolved work cannot start twice. A terminal notice already written retains its original count after partial fetching, preserving the remaining mail and composer ownership. Recovery guidance uses the existing check receipt because explicit acknowledgement needs an actionable command. These choices reuse authoritative state and add no stored obligation. There is no temporary deviation remaining.

Linked Issue

Maintainer fix for the verified idle-worker incident; no new issue created.

Visual Proof

BEFORE: the worker received one notice, fetched without acknowledging it, and finished its turn. After later mail arrived, it stayed idle; the later message remained undelivered. This is an actual hidden native-chat window with an isolated database and an offline agent stub.

Before: worker remains idle after later mail

AFTER: later mail receives a second normal notice while the first delivery is still outstanding. The worker completes the new turn. A repeated check reports one waiting message, and its acknowledgement command returns that later message in a new delivery.

After: later mail receives its own notice

Captured on this laptop through CDP with hidden windows, isolated home/profile/agent configuration, hooks disabled before first launch, and an absolute-path offline Codex stub. The actual runtime, database and native-chat host delivered the notices. BEFORE used 31a5b42; AFTER used 3d0d2d2. The final main sync changed none of this fix's production files. During QA, the live screenshot driver also fetched a coordination batch and ended without acknowledging it, stranding the next instruction; its retained transcript reproduced the same failure on the current live app.

Testing

  • I manually tested these changes locally — hidden native-chat before/after and explicit acknowledgement recovery verified on macOS.
  • Automated regressions written first for both lanes: notice → check without acknowledgement → idle → new mail → notice.
  • 332 tests in 28 explicit files pass, including working consumers, accepted queue recovery after repeated failed stamps, carry/withdrawal/receipt pruning, current-handoff ordering, a lost terminal Enter followed by one idle edge, asynchronous changes, FIFO/fences, unlimited replay counts and old-host CLI recovery.
  • Node, CLI and renderer typechecks passed sequentially through the machine queue on the final main-synced head. All 22 repository static checks passed before each push.

The initial CI red was an unchanged main-branch compatibility manifest exceeding its line limit, plus the dependent required-job verification failure. Main fix #26342 resolved both; final-head CI passed with no failures.

macOS tests use real database, orchestration runtime and native-chat host, with only the provider process faked. Linux, Windows and SSH execution are not manually verified. Selection and recovery remain on the owning host; no new remote opcode or required field is introduced, and folder workspaces require no Git assumption.

Review

Two independent plan reviews converged before implementation. The first implementation review found queue-ownership and idle-edge recovery gaps; tests were written first and both are fixed. Two independent final reviewers found no P0/P1. Their remaining query-size finding was reproduced and fixed using one JSON parameter; 93 affected tests and Node typecheck pass, and all 22 static checks passed before its push. Hidden before/after QA and explicit acknowledgement recovery passed. Final local checks and final-head CI after the one main sync passed.

Agent skill upstream boundary

  • Not applicable; no agent skill sharing source or artifacts changed.

Checklist

  • Focused on notice delivery and acknowledgement recovery.
  • Explained the problem, user-facing change, mechanism and alternatives.
  • Before/after screenshots attached.
  • Self-reviewed for correctness, security and performance.
  • Cross-platform, SSH and backwards compatibility considered.
  • Final-head CI has no failures; all local checks pass.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

A worker that fetched a mail batch without acknowledging it could silently stop hearing from its coordinator. The outstanding batch blocked notices for the entire mailbox, so later mail piled up while the worker sat idle.

Both terminal and native-chat workers now receive the normal notice for later mail while the old batch remains outstanding. Notice ownership is derived for particular messages from existing deliveries and conversation history, preserving live work without blocking independent mail. Repeated checks explain the replay, waiting mail and exact acknowledgement command; explicit acknowledgement still advances to the next batch.

Fixed: the mailbox-wide latch, duplicate native-chat turns after failed bookkeeping, lost terminal idle-edge recovery, and query argument growth. Deferred: no remaining findings in this lane. Two independent final reviews found no P0/P1; their remaining query-size finding was reproduced and fixed.

Verified on final head 3025436e8fd: 332 tests in 28 explicit files, sequential Node/CLI/renderer typechecks, all 22 static checks, and final CI. Hidden macOS before/after QA used an isolated folder workspace, actual runtime/database/native-chat host and an offline Codex stub. The old batch stayed unacknowledged when the second notice arrived; replay reported one waiting message, and its acknowledgement command returned that later message. Both screenshots are embedded above; protected-config checks passed and owned rig processes exited.

Not manually verified: real provider execution, SSH or mixed-version pairing, and Linux/Windows UI. Compatibility was reviewed in source; platform smoke checks and the automated CI suites cover their reported environments. Ready for review; no merge or auto-merge was performed.

@brennanb2025
brennanb2025 marked this pull request as ready for review October 8, 2026 00:33
@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

📝 Walkthrough

Walkthrough

Mailbox pointer delivery now selects and revalidates messages by mailbox attention eligibility. Structured-session delivery reconciles queued sends and journal submissions with message provenance. Eligible consuming replay responses include acknowledgement guidance and may include a waiting-message count; CLI output renders this recovery information.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 30254

Later orchestration mail now gets notices even when an earlier fetched batch has not been acknowledged. Repeated checks also show acknowledgement guidance. No concrete defect was found in the reviewed changes. The one open suggestion is optional hardening of a database query that is safe for its current callers.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 15.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 66 functions across 39 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary change: preserving orchestration notices after a mail batch remains unacknowledged.
Description check ✅ Passed The description is complete and focused. It explains the user impact, implementation, rationale, linked-issue context, visual proof, testing, review findings, compatibility considerations, and checkli…
Linked Issues check ✅ Passed No active directly linked coding target remains. Linked issue #26342 is closed and marked historical context only, so its compatibility-manifest acceptance criteria do not apply to this pull request. …
Out of Scope Changes check ✅ Passed The reported changes stay within notice delivery and acknowledgement recovery. Production changes update mailbox eligibility, terminal submission revalidation, native-chat ownership recovery, replay o…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/main/runtime/orchestration/db/messages/mailbox-pointer-attention.ts (1)

61-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Batch the id IN (...) list in getMailboxPointerAttentionIds.

The query binds one SQLite parameter for each ID. getUndeliveredUnreadMessages uses json_each(?) for exclusions so it does not hit the parameter limit. areUnreadMessages processes IDs in batches of 500. Pointer batches are capped by ORCHESTRATION_DELIVERY_BATCH_LIMIT, so current callers are probably safe. However, this helper is a public DB method with no size cap. To keep it safe for any caller, use json_each(?) here as well.

♻️ Proposed change
-         AND id IN (${ids.map(() => '?').join(',')}) AND ${OUTSIDE_FETCHED_BATCH_SQL}`
+         AND id IN (SELECT value FROM json_each(?)) AND ${OUTSIDE_FETCHED_BATCH_SQL}`
     )
-    .all(mailboxHandle, ...ids)
+    .all(mailboxHandle, JSON.stringify(ids))

Before you apply this change, check the INDEXED BY idx_messages_id plan assertion in orchestration-mailbox-notification-consistency.test.ts. That test checks for the id IN SQL text.


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6dda97a9-bab3-4efa-a478-913a9e1807fc
📥 Commits

Reviewing files that changed from the base of the PR and between 4f60f32 and 3025436.

⛔ Files ignored due to path filters (1)
  • src/main/runtime/orchestration/__snapshots__/preamble.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (39)
  • config/scripts/vitest-sqlite-database-runtime-files.mjs
  • src/cli/handlers/orchestration-check-replay-recovery.test.ts
  • src/cli/handlers/orchestration/message-check-handler.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-host.ts
  • src/main/runtime/orchestration-dispatch-mailbox-delivery.test.ts
  • src/main/runtime/orchestration-mailbox-notification-consistency.test.ts
  • src/main/runtime/orchestration-mailbox-unacked-notice.test.ts
  • src/main/runtime/orchestration/agent-facing-parity.test.ts
  • src/main/runtime/orchestration/db-messages.test.ts
  • src/main/runtime/orchestration/db/messages/mailbox-pointer-attention.ts
  • src/main/runtime/orchestration/db/messages/message-inbox.ts
  • src/main/runtime/orchestration/db/messages/replay-waiting-count.test.ts
  • src/main/runtime/orchestration/db/messages/role-mailbox-delivery.ts
  • src/main/runtime/orchestration/mailbox-pointer-delivery.ts
  • src/main/runtime/orchestration/mailbox-pointer-eligibility.test.ts
  • src/main/runtime/orchestration/mailbox-pointer-eligibility.ts
  • src/main/runtime/orchestration/mailbox-pointer-partial-fetch.test.ts
  • src/main/runtime/orchestration/mailbox-pointer-stage.ts
  • src/main/runtime/orchestration/mailbox-pointer-submit.ts
  • src/main/runtime/orchestration/preamble.test.ts
  • src/main/runtime/orchestration/preamble.ts
  • src/main/runtime/orchestration/structured-mailbox-pointer-delivery.test.ts
  • src/main/runtime/orchestration/structured-mailbox-pointer-delivery.ts
  • src/main/runtime/orchestration/structured-mailbox-pointer-host.test.ts
  • src/main/runtime/orchestration/structured-mailbox-pointer-host.ts
  • src/main/runtime/orchestration/structured-pointer-operation-id.test.ts
  • src/main/runtime/orchestration/structured-pointer-operation-id.ts
  • src/main/runtime/rpc/methods/orchestration/messaging/check-delivery-result.ts
  • src/main/runtime/rpc/methods/orchestration/messaging/check-replay-recovery.test.ts
  • src/main/runtime/rpc/methods/orchestration/messaging/check-replay-recovery.ts
  • src/main/runtime/rpc/methods/orchestration/messaging/check-run-pending-mail.ts
  • src/main/runtime/rpc/methods/orchestration/messaging/check-run.ts
  • src/main/runtime/rpc/methods/orchestration/messaging/check-worker.ts
  • src/main/runtime/structured-chat-coordinator-mail.test.ts
  • src/main/runtime/structured-chat-coordinator-observation-clock.test-fixture.ts
  • src/main/runtime/structured-chat-mail-queue-recovery.test.ts
  • src/main/runtime/structured-chat-worker-unacked-notice.test.ts
  • src/shared/orchestration-check-output.test.ts
  • src/shared/orchestration-check-output.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Closing: the unacknowledged-batch notice fix moved into #29006 (one lane for this mechanism), and this branch had gone stale against main.

@brennanb2025
brennanb2025 deleted the brennanb2025/mail-notice-unacked-batch branch October 19, 2026 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant