Repository navigation
Conversation
A federated worker host holds a remote_dispatch_attachments row and no dispatch_contexts row, so checkWorkerMailbox and importFederatedControlMessage had no Run to file the worker's dispatch:<id> mailbox under and fell back to run_legacy_local. Live rows there make hasConsistentLegacyAdoption false, so the next open replays v2->v39, and the legacy-adoption pass fences the outstanding Delivery and reclassifies the coordinator's instruction as legacy_direct, which every mailbox read filters out. The worker's re-check then returns nothing and its ack throws stale_delivery: an ordinary restart destroyed live worker mail. The Run that owns the Dispatch lives on the home and is not knowable here, so these rows get a dedicated non-legacy local Run instead of a wire field the home would have to learn to send. Both writers resolve it through one function, which prefers a local Dispatch's Run so a loopback home stays consistent. Databases the pre-fix build already misfiled are repaired on open, ahead of the version-skew probe. Adoption may have swept the rows into the recovered Run and fenced the Delivery on the way; the fence is undone only where the attachment still carries the generation the Delivery was minted at and no message in it was ever read, which is provable evidence that nothing consumed or superseded it.
projectFleetNextAction returned inspect = worker-show --dispatch <id> for every unverifiable row, and worker-show republishes this same projection. An agent following the guide's rule to run a row's literal nextAction argv whenever attention.requiresAction is true therefore re-derived the identical command forever, and the escape clause only covered live rows. Absence cannot be settled by any command, so an unverifiable row with nothing pending now recommends none and surfaces its reason on liveness. recover stays reserved for a proven exit; a pending question or approval still points at worker-show, where the row can actually change. The kernel sentence changes with it, so an agent reading it literally cannot loop, and the bundled copy is regenerated from the source guide.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds federated attachment mailbox Run resolution and registers it with the orchestration database. Schema version 40 migrates misplaced mailbox messages and deliveries from legacy Runs. Federated mailbox tests cover migration, adoption, fencing, restart, replay, and acknowledgment behavior. Worker projections now return no action for unverifiable rows without pending input or approval. Orchestration guidance and tests now describe this behavior and the Merge Risk: ⚪ Minimal · up to Federated mailbox records are assigned durable Runs and repaired during upgrade, while unverifiable idle workers no longer loop on inspection. The supplied coverage supports the intended delivery and projection behavior with no current merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives detailed scope, rationale, implementation, compatibility notes, and testing results. However, it does not follow several required template items, including an explicit ELI5 section, a linked issue in the required Resolution Add the missing required template sections and information. Provide the issue reference as
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: ad36e6ac-3f34-4930-8369-826efed6981d
📒 Files selected for processing (13)
config/scripts/orchestration-skill-guidance.test.mjsskill-guides/orchestration.mdsrc/cli/bundled-skill-guides.tssrc/main/runtime/orchestration/db/attach-orchestration-db-methods.tssrc/main/runtime/orchestration/db/federation/federated-attachment-mailbox-run.test.tssrc/main/runtime/orchestration/db/federation/federated-attachment-mailbox-run.tssrc/main/runtime/orchestration/db/orchestration-db-methods.tssrc/main/runtime/orchestration/db/orchestration-db.tssrc/main/runtime/orchestration/federation-control-message.tssrc/main/runtime/rpc/methods/orchestration/messaging/check-worker.tssrc/main/runtime/rpc/methods/orchestration/worker/legacy-dispatch-projection.test.tssrc/shared/orchestration-fleet-projection.test.tssrc/shared/orchestration-fleet-worker-projection.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This PR fixes two #16904 audit findings: it gives federated worker hosts a dedicated non-legacy mailbox Run and stops the fleet projection from recommending a self-looping worker-show for unverifiable rows.
- Non-legacy mailbox Run for federated hosts — a new
run_federated_localRun, created lazily per host, scopesdispatch:<id>mailboxes whose host has aremote_dispatch_attachmentsrow but nodispatch_contextsrow.resolveFederatedMailboxRunIdprefers a loopback home's local Dispatch Run; both writers (importFederatedControlMessageandcheckWorkerMailbox) now resolve through it. - Constructor-time repair —
repairFederatedAttachmentMailboxRunsruns betweencreateTablesandmigrate(before the version-skew probe), moving pre-fix rows off the legacy/adopted Runs and un-fencing adoption-swept Deliveries only where the attachment generation still matches and no message was read. - Fleet projection anti-loop —
unverifiablewith nothing pending now reportsnextAction: noneinstead ofinspect; the guide sentence and bundled copy are regenerated.
The load-bearing invariants check out: the repair's table/column probes are safe against the createTables-then-migrate ordering (federated tables sit in createGraphTablesSql; hasColumn guards the legacy_adoptions/mailbox_handle/consumer_generation columns so pre-v36 databases return early); the only runtime fence of a remote attachment mailbox bumps consumer_generation and fences in the same statement, so the generation-equality guard cleanly separates adoption fences from real authority handoffs; and the host's sole dispatch:<id> writer is importFederatedControlMessage. The new tests carry exact assertions (Run id, delivery_contract, delivery status, getLegacyAdoption()) rather than loose ones, so they genuinely fail on regressions.
ℹ️ Adoption tombstone left behind after the repair
When a pre-fix build already ran the adoption pass (classifyLegacyMessageContracts + adoptLegacyRunIfNeeded), the repair correctly moves the misfiled rows back and un-fences the restored Delivery, but it leaves the legacy_adoptions row, the now-empty adopted Run, and the legacy Run's "adopted; inspect only" objective in place. For a host that only ever held misfiled federated rows, this leaves two empty stem Runs visible in run-list alongside the real federated Run.
This is inert today: the empty adopted Run has no coordinator_handle, so getLegacyAdoptedRunMailboxOwner returns null, and hasConsistentLegacyAdoption still returns true so no migration replays. Worth confirming whether that empty-toned run-list artifact is acceptable or whether the repair should also revert the legacy Run heading when the adoption it undid left nothing genuinely legacy behind.
Technical details
# Adoption tombstone left after repair
## Affected sites
- src/main/runtime/orchestration/db/federation/federated-attachment-mailbox-run.ts — `repairFederatedAttachmentMailboxRuns` moves `messages`/`deliveries` and calls `restoreAdoptionFencedDeliveries` but never touches `legacy_adoptions`, the adopted Run row, or the legacy Run objective.
- src/main/runtime/orchestration/db/schema/adopt-legacy-run.ts:79 — stamps the legacy Run with `Legacy orchestration state (adopted; inspect only)` during adoption.
- src/main/runtime/orchestration/db/schema/adopt-legacy-run.ts:34-39 — inserts the `legacy_adoptions` row and the orphan adopted Run.
## Required outcome
- Decide whether a host that only held misfiled federated rows should clean up the orphan adoption + adopted Run (and revert the legacy Run objective) so `run-list` reflects reality, or accept the tombstone deliberately.
## Open questions for the human (optional)
- Is leaving a stale `legacy_adoptions` row intentionally conservative (safer to keep a tombstone than delete state), or should the repair delete it when `LEGACY_RUN_ID` and the adopted Run are both empty of remaining graph rows?DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
…try point Two review findings against the first pass, both reproduced. The relocation required v36's consumer-generation columns on the claim that pre-v36 databases predate federated mailboxes. They do not: role mailboxes shipped at v34 and v36 only added counters, so a populated v35 host holds the same misfiled rows. The guard made the first upgraded open skip relocation, the skew probe then read a live legacy graph, and the replayed adoption fenced the instruction for that whole launch. Relocation now degrades to whatever columns the stored shape has, and runs a second time inside the legacy-contract migration so no replay start version can classify or fence these rows. Both call sites are load-bearing and cover different populations: the constructor is the only pass that runs when migrate returns early, which is every already-adopted database, and the migration pass is the only one that reaches a database too old to carry messages.delivery_contract. The adoption-fence restore is removed rather than made safer. It inferred provenance from current-generation equality, and resetTasks keeps deliveries while dropping attachments, so recreating a Dispatch restarts the counter and a real authority fence matched every predicate. Nothing durable records which incarnation minted a Delivery or why it was fenced, and a fence is a safety state, so an ambiguous one stays fenced. The instruction is still recovered: its message returns to the federated Run unread, and the worker's next check mints a fresh Delivery carrying it. The check-worker Run resolver had no test that exercised it, so a revert stayed green. The new RPC regression drives attach, check, restart, replay, and ack through orchestration.check against a file-backed database.
… rule The kernel claimed every unverifiable row reports a `none` nextAction, but projectFleetNextAction deliberately keeps `inspect` when input or approval is pending, because that is work a command can actually move. An agent following the guide literally would have treated a waiting question as another wait-only row. The guidance test asserted the sentence verbatim, so it agreed with the wrong text instead of the behavior; it now checks the rule's two halves as fragments, and the projection suite covers both pending flags directly.
…egacy Replaces the every-open repair pass with a correction at the layer that held the wrong belief, plus one numbered migration for the damage already on disk. hasConsistentLegacyAdoption reads live rows under the legacy Run as proof of a pre-Run database. A federated attachment mailbox misfiled there by an earlier build is damage, not proof, and counting it is what forced the replay from v6 that let adoption fence a live worker. The probe now discounts exactly those rows, which removes the reason the old pass had to run before migrate at all. Both Codex P1s fall out of that: a populated pre-v36 host no longer replays on its first upgraded open, and there is no fence-restore predicate left to guess provenance from a consumer generation that a task reset can rewind. The one-time re-home is v40, a plain migration that moves run_id and restores delivery_contract. With the probe corrected it runs on a start version above the v19 adoption step, so it prevents rather than undoes. It is guarded so a host that never federated gains no Run, and it self-retires with the stamp instead of running forever on every open. No Delivery is un-fenced. Nothing durable records why a Delivery was fenced, and the mail is not lost by leaving one alone: the message returns unread under the right Run and the next check mints a fresh Delivery, which a test now pins. Membership is the attachment row alone, not the absence of a local Dispatch. A loopback home holds both rows, which is why the resolver prefers the local Dispatch's Run, and excluding that shape left the one configuration the resolver exists for unrepaired. The target is that same preference in SQL, and the comparison is against the Run the row sits in, so mail already in the right place, including a pre-Run Dispatch's own legacy mail, is never claimed.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This round reworked the round-1 constructor-time repair into a boundary-correct, self-retiring migration after the Codex/Opus findings (loopback gap, boundary analysis). Reviewed the three new commits against the prior Pullfrog review.
- Schema-v40 repair replaces the constructor repair — the every-open
repairFederatedAttachmentMailboxRuns/restoreAdoptionFencedDeliveriespair is gone; a one-timemigrateV40re-homes misfiled messages/deliveries offrun_legacy_local∪ adopted runs onto the correct Run (loopback → local Dispatch Run, elserun_federated_local) and stamps messagescurrent_delivery. - Skew probe fixed at its own layer —
hasConsistentLegacyAdoptionnow discounts misfiled federated messages/deliveries viamisplacedFederatedMailboxSql, so a poisoned DB no longer reads as version-skewed and no longer replays from v6; adoption functions carry no carve-outs. - No un-fencing — an ambiguous fence is preserved; the unread instruction returns under the right Run and the worker's next
checkmints a fresh Delivery (the one-outstanding partial index only coversstatus='outstanding', so a fenced + freshly-minted Delivery coexist). - Shared SQL builders —
federatedMailboxRunIdSql/misplacedFederatedMailboxSqlare used by both the probe and the migration; membership is the attachment alone, and the Run comparison keeps genuinely-legacy mail (a local Dispatch whose own Run is the legacy Run) untouched. - Named the
noneexception —unverifiablewith no pending input/approval now reportsnextAction: none; the pending-input/approval case keepsinspect, and the guide sentence + bundled copy and its kernel test were updated.
The new tests carry exact assertions (Run id, delivery_contract, delivery status, getLegacyAdoption(), minted-vs-original delivery id), plus a real-RPC attach→check→restart→replay→ack regression, so they genuinely fail on regression rather than absorbing whatever they are handed.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
"An unverifiable row reports it unless input or approval waits" still overstated the projection. Cleanup outranks the liveness verdict: a worker that reported success and still owns a reclaimable terminal is unverifiable with neither pending flag set, and projectFleetNextAction returns `release` for it well before the unverifiable branch is reached. An agent reading the old sentence would treat that row as wait-only and leave the terminal held. The sentence now says what `none` means without claiming absence produces it, and the projection test that covers the reclaimable row asserts its unverifiable verdict so the counterexample is pinned rather than described.
The redesign lets adoption sweep a genuinely version-skewed database and has v40 re-home the rows afterwards, so the case no longer runs before that pass.
v40 runs once for every upgrading database, so the guard that skips a host with no misplaced mail is the only thing between the whole install base and an extra Run in run-list. Nothing asserted it: deleting the early return left the suite green. The new case opens an ordinary single-Run database stamped at the pre-fix version and requires that run_federated_local exists neither as a row nor in the listing, and it fails with the guard removed. Also records the one shape v40 deliberately does not repair: a loopback Dispatch whose own Run is the legacy one, which adoption sweeps together with its mail so the row ends up under the right Run still stamped legacy_direct. No caller can build it, because resolveRunScope refuses an explicit Run whose legacy flag is set, so re-classifying by contract would guard an unreachable state.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This round addresses the round-2 documentation nit — the guide sentence that still implied none follows from liveness alone, even though a settled worker owning a reclaimable terminal is unverifiable and owes release. No production path changed; the projection source is untouched.
- Corrected the
noneguidance sentence —skill-guides/orchestration.mdnow statesnonehas no argv (readliveness.reason, keep waiting), and that absence never earns an argv while settlement and pending work still do. - Regenerated the bundle and kernel assertions —
src/cli/bundled-skill-guides.tsis regenerated, and the kernel fragments inorchestration-skill-guidance.test.mjstrack the new wording. - Pinned the reclaimable-terminal counterexample — the
suggests release only for reclaimable ownershipprojection test now also asserts theunverifiableliveness verdict, sononeprovably does not follow from absence alone. - Renamed the pre-contract migration test — it now names what it asserts: the instruction is redelivered after the replayed adoption sweep, not merely relocated beforehand.
I verified the ordering this guidance depends on: projectFleetNextAction checks terminalState === 'reclaimable' (returns release) and the pending input/approval branches before the unverifiable → none fallthrough, so the new sentence and its test agree with the code.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
One new commit since the prior review, closing a coverage gap on the v40 guard rather than changing any production path.
- Pinned the v40 guard with a regression test —
gives a host that never federated no Run of its ownopens an ordinary single-Run database stamped at the pre-fix version and assertsrun_federated_localappears neither as a row nor inlistRuns, so the early return that keeps the whole install base from gaining a phantom Run now fails the suite if removed. - Documented the one deliberately un-repaired shape —
misplacedFederatedMailboxSqlnow records that a loopback Dispatch whose own Run is the legacy Run is left to adoption, sinceresolveRunScopemakes that state unreachable and re-classifying by contract would guard a no-caller path.
I verified the test is a genuine regression guard: with the misplaced early return deleted, ensureFederatedAttachmentRun runs unconditionally and the assertions fail. The comment matches the SQL — membership is the attachment alone, and the run_id <> correctRun comparison is what excludes a legacy-owned Dispatch from re-homing.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Superseded by #19542: the coordinator's real Run is filed on the attachment row and the legacy-Run write fallbacks are deleted, so no synthetic Run or resolver is needed. |
1 participant
Summary
Two findings from the #16904 audit (migration-wire P0, fleet-projection P1).
P0. A federated worker host has a
remote_dispatch_attachmentsrow and nodispatch_contextsrow, so both writers of itsdispatch:<id>mailbox (checkWorkerMailboxandimportFederatedControlMessage) fell back torun_legacy_local. That broke the legacy Run's one premise, whichhasConsistentLegacyAdoptiondepends on: live rows there prove a pre-Run database. So every open replayed migrations from v6, and legacy adoption fenced the worker's outstanding Delivery and stamped the coordinator's instructionlegacy_direct, which every mailbox read filters out. After an ordinary app restart the worker's mail was unreachable and its ack threw.P1.
projectFleetNextActionrecommendedworker-showfor anunverifiablerow.worker-showrepublishes the same projection, so an agent following the guide's literal-argv rule re-derived the same command forever.Fix
Three layers, each corrected where it owns the invariant "a federated attachment mailbox is never legacy mail".
run_federated_localscopes these mailboxes. Both writers go throughresolveFederatedMailboxRunId, which prefers a local Dispatch's Run so a loopback home (both rows in one database) cannot diverge. The Run id never crosses the wire, so mixed-version peers are unaffected.hasConsistentLegacyAdoptionno longer counts a federated attachment mailbox sitting in the wrong Run as evidence of a pre-Run database. A poisoned database therefore no longer replays from v6, and legacy adoption never reaches these rows. No carve-out was added toadoptLegacyRunIfNeededorclassifyLegacyMessageContracts.run_idand restoringdelivery_contract. Because the probe is corrected, it runs at a start version above the v19 adoption step, so it prevents rather than undoes. It is guarded so a host that never federated gains no Run, and it self-retires with the stamp instead of running on every open.No Delivery is un-fenced. Nothing durable records why a Delivery was fenced, and a fence is a safety state, so an ambiguous one is left alone. The instruction is not lost: its message returns unread under the correct Run, and the worker's next check mints a fresh Delivery, because the one-outstanding index is scoped to outstanding rows.
P1:
unverifiablewith nothing else owed now yieldsnextAction: none.recoverstays reserved for a proven exit, and settlement, cleanup, and pending input or approval still name a command. One guide sentence updated inskill-guides/orchestration.md; the bundle is regenerated by the generator, never hand-edited.SCHEMA_VERSIONgoes 39 to 40. An older binary reading a v40 stamp returns early from the migration chain rather than replaying it.Behavior note:
run-liston a federated worker host shows one extra Run, "Federated worker attachments hosted on this Orca server".Tests
federated-attachment-mailbox-run.test.ts, 9 cases: restart survival; pre-fix database re-homed; adoption-swept instruction redelivered through a fresh Delivery; populated pre-v36 first open; pre-contract (v18) database swept then re-homed; loopback mailbox re-homed onto the local Dispatch Run; mail whose Dispatch Run is the legacy Run never claimed; genuine legacy mailbox left to adoption; a real authority fence survivingresetTasksplus attachment recreation.check-worker-federated-attachment.test.tsdrives attach, check, restart, replay, and ack through the realorchestration.checkRPC against a file-backed database, so reverting the check-worker resolver cannot stay green. Projection tests cover both pending flags and the reclaimable-terminal counterexample.Mutation sweep, 42 tests over 4 files, 9 modes, no survivors: skew-probe exclusion for messages and for deliveries, the v40 step, the loopback target and its membership predicate, the wrong-Run comparison, the
delivery_contractrestore, the deliveries update, and the resolver's loopback preference.Gates: orchestration plus rpc/orchestration plus the projection and guidance suites, 164 files and 1430 tests;
tc:node,tc:cli,tc:web; changed-code quality, 0 new findings;verify:bundled-skill-guides;verify:skill-bundle-manifest.Part of the fix-forward set for #16904. Not to be merged without explicit sign-off.
Review history
Round 1 (Codex, 2×P1; Opus, loopback gap and boundary analysis) produced the design above. The originally shipped every-open constructor repair and its generation-equality fence restore are both deleted: the repair existed only because the skew probe believed something false, and the restore inferred provenance that
resetTaskscan rewind by restarting a recreated Dispatch's consumer generation. Round 2 confirmed every round-1 finding closed, with one documentation nit (a guide sentence that still impliednonefollows from liveness alone, since a settled worker owning a reclaimable terminal isunverifiableand owesrelease), now fixed and pinned by a test.Compat: the upgrade path is covered for populated v18, v35, and v39 databases. Downgrade is out of scope by decision and documented in the module: an older binary addresses the legacy Run, so it can replay these Deliveries but never acknowledge them, and a host that downgrades, is re-poisoned, then upgrades again is not repaired a second time because v40 is already stamped.