Repository navigation
feat(orchestration): make a recorded heartbeat state the coordinator reads, not mail that wakes it - #15173
feat(orchestration): make a recorded heartbeat state the coordinator reads, not mail that wakes it#15173Tauri-EPO wants to merge 2 commits into
Conversation
b0a97e6 to
cf04fcb
Compare
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughAdded Fleet Echo snapshots for orchestration runs. The snapshots report active lanes, delivery state, process state, heartbeat age, quiet duration, run identity, and truncation. RPC methods and CLI commands support fleet data by default and allow 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/main/runtime/orchestration/db-dispatch-liveness.test.ts (1)
32-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest the oldest-first contract with multiple active rows.
The first test has only one active row. It cannot detect loss of
dc.rowid ASCordering. Create multiple active rows and assert their ordered IDs. Assert the selected IDs in the limit test too.src/main/runtime/orchestration/mailbox-pointer-eligibility.ts (1)
7-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce these comments to one concise sentence each.
Keep the delivery invariant and remove the detailed rationale and implementation walkthrough.
As per coding guidelines: “Comments must be concise, non-obvious, and brief—prefer one line.”
Also applies to: 65-67
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d162d69e-e7ab-4471-93f6-6c64f4cf96cf
📥 Commits
Reviewing files that changed from the base of the PR and between 2aebcfe and b0a97e6da9f7386f48b574afba3819424924524b.
📒 Files selected for processing (21)
skill-guides/orchestration.mdsrc/cli/bundled-skill-guides.tssrc/cli/handlers/orchestration-fleet-echo-format.test.tssrc/cli/handlers/orchestration-fleet-echo-format.tssrc/cli/handlers/orchestration.tssrc/cli/specs/orchestration.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/orchestration-heartbeat-push-silence.test.tssrc/main/runtime/orchestration/db-dispatch-liveness.test.tssrc/main/runtime/orchestration/db.test.tssrc/main/runtime/orchestration/db/dispatch-context/dispatch-lookup.tssrc/main/runtime/orchestration/mailbox-pointer-delivery.tssrc/main/runtime/orchestration/mailbox-pointer-eligibility.tssrc/main/runtime/rpc/methods/orchestration-check.test.tssrc/main/runtime/rpc/methods/orchestration-fleet-echo-sources.test.tssrc/main/runtime/rpc/methods/orchestration-fleet-echo-sources.tssrc/main/runtime/rpc/methods/orchestration-send-fleet-echo.test.tssrc/main/runtime/rpc/methods/orchestration-tasks-dispatch.test.tssrc/main/runtime/rpc/methods/orchestration.tssrc/shared/orchestration-fleet-echo.test.tssrc/shared/orchestration-fleet-echo.ts
💤 Files with no reviewable changes (1)
- src/main/runtime/orchestration/db.test.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 8 remain after this review.
53984ac to
e04b8b1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/shared/orchestration-fleet-echo.ts (1)
53-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the multi-line rationale comments.
The comments state useful policy, but each is four lines long. Reduce each comment to one concise, non-obvious sentence.
src/shared/orchestration-fleet-echo.ts#L53-L56: Replace the scan-limit rationale with one short sentence.src/shared/orchestration-fleet-echo.ts#L99-L102: Replace the ranking rationale with one short sentence.As per coding guidelines: “Comments must be concise, non-obvious, and brief—prefer one line; do not explain obvious behavior or walk through code.”
Source: Coding guidelines
src/cli/handlers/orchestration-fleet-echo-format.ts (1)
32-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce explanatory comments. The code and test names already express most of this behavior. Keep only short comments that explain a non-obvious constraint.
src/cli/handlers/orchestration-fleet-echo-format.ts#L32-L34: reduce this to one short rationale, or remove it.src/cli/handlers/orchestration-fleet-echo-format.ts#L62-L63: reduce this to one short rationale.src/cli/handlers/orchestration-fleet-echo-format.ts#L66-L68: reduce this to one short rationale.src/cli/handlers/orchestration-fleet-echo-format.test.ts#L17-L18: remove or reduce this setup explanation.src/cli/handlers/orchestration-fleet-echo-format.test.ts#L73-L74: remove or reduce this assertion explanation.src/cli/handlers/orchestration-fleet-echo-format.test.ts#L108-L109: remove or reduce this assertion explanation.src/cli/handlers/orchestration-fleet-echo-format.test.ts#L198-L199: remove or reduce this setup explanation.As per coding guidelines, comments must be concise, non-obvious, and brief—prefer one line.
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8db926b7-6d90-42ea-81b2-16df4a94f952
📥 Commits
Reviewing files that changed from the base of the PR and between 53984acb88a876e2e1f4a80529bcff9374ae9540 and e04b8b1f257997051632646b5b7687bff61fcfd7.
📒 Files selected for processing (7)
skill-guides/orchestration.mdsrc/cli/bundled-skill-guides.tssrc/cli/handlers/orchestration-fleet-echo-format.test.tssrc/cli/handlers/orchestration-fleet-echo-format.tssrc/main/runtime/rpc/methods/orchestration-fleet-echo-sources.tssrc/shared/orchestration-fleet-echo.test.tssrc/shared/orchestration-fleet-echo.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- skill-guides/orchestration.md
- src/main/runtime/rpc/methods/orchestration-fleet-echo-sources.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 8 remain after this review.
e04b8b1 to
e31740b
Compare
Why it was worse than "some rows get dropped". The gate made a valid heartbeat never deliver, so That is self-reinforcing in the wrong direction: the queue only drains on an explicit The fix moves the rule into the read. NOT (type = 'heartbeat' AND instr(COALESCE(payload, ''), '_orcaLifecycleRejection') = 0)
The test I should have written. The existing case queued one heartbeat against a limit of 50 — right in intent, wrong in dose. The new one queues a full page plus five, then a Zero pointers, which is the bug exactly. Two smaller things that came out of the same pass:
Verification on |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d4dde344-43b9-4bd5-89cf-f8c5e4e669e1
📥 Commits
Reviewing files that changed from the base of the PR and between e04b8b1f257997051632646b5b7687bff61fcfd7 and e31740b621c4a2358688e4c6079a64c6f9769666.
📒 Files selected for processing (8)
src/cli/handlers/orchestration-fleet-echo-cli.test.tssrc/cli/handlers/orchestration.test.tssrc/cli/handlers/orchestration.tssrc/main/runtime/orchestration-heartbeat-push-silence.test.tssrc/main/runtime/orchestration/db/messages/message-inbox.tssrc/main/runtime/orchestration/mailbox-pointer-eligibility.tssrc/main/runtime/rpc/methods/orchestration-send-fleet-echo.test.tssrc/main/runtime/rpc/methods/orchestration-tasks-dispatch.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/runtime/rpc/methods/orchestration-send-fleet-echo.test.ts
- src/main/runtime/rpc/methods/orchestration-tasks-dispatch.test.ts
- src/main/runtime/orchestration-heartbeat-push-silence.test.ts
- src/cli/handlers/orchestration.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review.
|
Holding this branch rather than pushing the standalone re-cut, and recording why, because the reason is the interesting part. The re-cut works, and it is cleanly separableThis PR turned out to be one commit on top of the fleet-echo stack, not an entangled diff. The heartbeat commit is Verified on the standalone branch: Regression injection, two mutations with disjoint failures: That second one pins the property that matters: the filter has to narrow the SQL read, not the result, or a page that is entirely heartbeats starves the I also re-checked the interaction with #14961 on current Why I am not opening itStandalone, this removes the wake without adding anywhere to read. I checked what exists on That is the same objection this PR's own notes raise against #13235 — "removes the wake and leaves the coordinator with nowhere to see freshness" — and it would be inconsistent of me to ship it and not mind. It is an ordering problem, not a code problem: exposing heartbeat freshness on a read has to come first, and it is a small change of its own since the column already exists. So the sequence is two isolated PRs, and this is the second one. The branch is ready and will keep. One thing I left as the original wrote itThe comment claims Tracked as REC-04 on #15190, where it now sits behind the read step rather than in front of it. |
7544a32 to
558af98
Compare
|
Rewritten from scratch. The branch was force-pushed to a single commit on current What changed about this PR The previous version was 13 commits on a 17 Aug base and carried roughly 1,500 lines that no longer Three things were dropped:
Why it was reimplemented rather than rebased #16904 rewrote I re-checked the current behavior before rewriting rather than assuming the old diagnosis still held: Verification
I have not exercised this in a live multi-agent run, so the manual-testing box stays unchecked. |
…reads, not mail that wakes it A heartbeat's whole content is "nothing changed", and the liveness it carries is already written to dispatch_contexts.last_heartbeat_at on the send path, before any pointer runs. Pushing it a second time as mail spends a coordinator turn to convey state that worker-list and worker-show can read at any time. Exclude a recorded heartbeat from the pointer-delivery batch, in SQL rather than after the read: the rows are never delivered and never read, so they accumulate at the head of `sequence` and a post-LIMIT filter would eventually return a page that is entirely heartbeats while a worker_done waits behind them. A rejected heartbeat still delivers. It is not liveness -- it means someone other than the assignee claimed it, which would otherwise mask a hung assignee. Nothing is hidden either way: both rows stay unread for check, inbox, and worker-show. Both delivery lanes share selectOrchestrationPointerBatch, so the PTY lane and the structured-session lane are covered by the same change.
…ession lane A structured coordinator owns its `run:` mailbox through the structured pointer lane, which selects its batch with the same selectOrchestrationPointerBatch. Drive that lane against a real database so the shared filter is proven there too, not only on the PTY lane.
558af98 to
f156e48
Compare
|
A data point from real runs, in case it helps prioritise this. Setup: Orca 1.4.218, one orchestrator, one coordinator, several worktree workers per run, multi-hour unattended runs (issue -> worker -> review -> merge).
Each of those 95 deliveries woke an idle session just to say nothing changed. Run B finished normally with zero, so in our workflow heartbeats carried no information the coordinator needed; worker completion and escalation messages were enough. The workaround has costs that this PR would remove: it relies on a prompt instruction (not enforced), it also gives up the hung-worker signal, and a filtered |
ELI5
A dispatched worker sends a heartbeat on a fixed cadence to say "still alive". Today that heartbeat
is pushed to the coordinator like any other mail, so the coordinator spends a turn reading a message
whose entire content is "nothing changed".
The liveness it carries was already written to the database before the push ever ran. So this stops
pushing it and leaves it where the coordinator can read it on demand.
What Changed
getUndeliveredUnreadMessagestakes a newexcludeRecordedHeartbeatsoption that narrows theread to exclude a recorded heartbeat.
selectOrchestrationPointerBatchpasses it. That function is the one batch selector both deliverylanes share — the PTY lane in
mailbox-pointer-delivery.tsand the structured-session lane instructured-mailbox-pointer-delivery.ts— so a single flag covers both, including a worker runningover SSH or imported through federation, because all of those paths land rows in the same
messagestable and push them through the same selector.assignee claimed it, which would otherwise mask a hung assignee. The exclusion tests for the
_orcaLifecycleRejectionmarker thatconvertLifecycleMessageToRejectionwrites, and a false hiton that test only over-delivers.
check,inbox, andworker-showread through different queries; the rowstays
read = 0and keeps showing up there.in the always-loaded kernel and one in the worker-contract reference.
src/cli/bundled-skill-guides.tsis regenerated, not hand-edited.orchestration-heartbeat-push-silence.test.ts, five cases against a real SQLite database.The filter is applied in SQL rather than after the read on purpose. A recorded heartbeat is never
delivered and never read, so heartbeats accumulate at the head of
sequence; a filter applied afterLIMITwould eventually return a page that is entirely heartbeats and push nothing while aworker_donewaited behind them. One of the five tests is exactly that case.Why
#14910.
HEARTBEAT_INTERVAL_MINinpreamble.tsis 5, so each worker sends 12 heartbeats an hour; a six-lane fleet is 72 pushes an hour that convey
nothing the coordinator could not already read. Each one costs a coordinator turn and displaces
attention from mail that does carry a decision.
The heartbeat is not lost by not being pushed.
reconcileLifecycleMessagewritesdispatch_contexts.last_heartbeat_aton the send path, before any pointer runs, andgetStaleDispatchesreads that column. Stale-worker detection is untouched — a test asserts thetimestamp is still written when the push is silenced.
Prior art, for the record: #13235 proposed the same
wake half and was closed as a draft on 31 Aug without review. This is written against current
mainrather than derived from it.
Linked Issue
Fixes #14910
Visual Proof
N/A — no UI surface. The change is which rows a mailbox pointer batch contains; nothing renders
differently.
Testing
Windows 11, Node 24.19.0, pnpm 12.0.0, rebased onto
upstream/mainat9d29e6878e.New test, red then green — with
excludeRecordedHeartbeatsleft off, 3 of the 5 cases fail:Suites around the change:
Gates:
config/scripts/orchestration-skill-guidance.test.mjsis 28 passed / 1 failed here. That one failure("keeps heartbeat and worker_done recipes bound to the injected capability") reproduces unmodified on
upstream/mainon this machine: its fenced-block regex requires a bare LF after the fence marker, soit cannot match a Windows checkout with
core.autocrlf=true. Unrelated to this change and untouched.pnpm buildand the fullpnpm testwere not run locally; CI covers them.The manual box is unchecked because I did not exercise this in a live multi-agent run. The evidence
above is the test suites, not an in-app session.
AI Disclosure
Written with Claude Code (Claude Opus). I reviewed the diff and ran every command quoted above.
Review
Cross-platform. No platform-dependent code. The predicate uses the SQLite core scalar functions
instrandCOALESCE; no path, shell, or process handling is involved.SSH and remote. The execution host owns the mailbox and the pointer push, and nothing here reads
or reports liveness across a connection. A heartbeat that arrives from a peer through
federation-relay-importis inserted into the samemessagestable and selected by the samefunction, so remote workers get the same behavior rather than a second code path. No wire format,
RPC parameter, or stream opcode changes, so a mixed-version client/host pair is unaffected — this is
entirely host-side row selection.
Agent and integration compatibility. Both delivery lanes go through the shared selector, so PTY
agents and structured-session agents behave identically and no agent-specific branch is added.
Nothing here touches source control, so it is provider-neutral by construction.
Performance. One extra constant predicate on an already-filtered query, and the read returns
fewer rows than before. The starvation test covers the pathological case the SQL placement exists to
prevent.
Security. No privilege boundary is crossed and no new data reaches a caller. The rejected
heartbeat — what a wrong sender produces — is precisely the case that still delivers, so this cannot
be used to hide a spoofed liveness claim.
UI quality. No renderer change.
Folder workspaces. Not workspace-shaped; mailboxes are addressed by run or dispatch handle, not
by worktree.
Considered and left out. The e2e fake in
orca-runtime-test-orchestration-messages.spec.tsimplements its own
getUndeliveredUnreadMessagesand does not model the new option. No spec seeds aheartbeat row into it, so the difference is unobservable there; I left the fake alone rather than
grow the diff, and I am happy to mirror the option if you would rather it stay in step.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
Scope note: this is only the wake half of
#14910. The read half — reporting heartbeat
freshness so a coordinator can see the age it is no longer being told — is
#15963 and is deliberately not here.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred) — see Testing for exactly what ran and what did notAuthor