revert(engine): hold #360 from the next release - #364
Merged
Conversation
PR #360 merged with unresolved functional findings: enrollment can race registration and lose authoritative tags; legacy client-supplied cloud tags become trusted indefinitely; and reserving the entire cloud namespace breaks client tag round-tripping. Keep #355 and #358 releasable while the tag-authority contract is hardened separately. Session-Id: 01a060f2-87c5-7d21-bf8f-515f3e9cbfdd
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 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 |
khaliqgant
added a commit
that referenced
this pull request
Sep 2, 2026
Two from cubic review on #363, plus a composition guard for the post-#364 tree. Serialization was keyed on `machine_id` alone, so a direct-node enrollment carrying one queued behind every other enrollment on that machine even though it never consults the machine lookup. A box running many direct node-of-one delivery hosts would serialize enrollments that are genuinely independent. Gate the queue on `requestedNodeRole(...) === 'broker'`, matching the condition that actually reaches the lookup. The concurrency test asserted only the roster length, so it would have passed if one enrollment had failed outright. Both enrollments now have to return 201 before the row count means anything; same for the two direct hosts. Also pin the one real interaction between this feature and the node.register path, now that #364 has removed server-authoritative tag preservation: a broker that enrolls with a `machine_id` and then registers WITHOUT one must keep the stored value. `recomputeNodeAggregate` writes `machineId` only when non-null, and this feature depends on that — if a register could null it, the node's next boot under a fresh name would mint a new row and the dedupe would silently stop working. The test asserts the value survives and that dedupe still holds after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QgtVrPUzGDZwa89Bg4deaL Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978
khaliqgant
added a commit
that referenced
this pull request
Sep 3, 2026
* feat(engine): dedupe node enrollment on machine_id `nodes` rows were only ever keyed on `node_id` then `name`. A fleet host that persists no `node_id` enrolls under a fresh name on every boot, so each boot minted a brand-new roster row and nothing ever reclaimed the old one. That is why roster cleanup does not stick: the roster refills from enrollment. `machine_id` already existed as a column but was written only on the WebSocket `node.register` path — `POST /v1/nodes` did not even accept it, so zod stripped it and every enrolled row stored NULL. Enrollment now resolves `node_id`, then `name`, then `machine_id`, and persists the value it was given. Re-enrollment under a fresh name rotates the machine's existing node instead of inserting another. The `machine_id` step is scoped to `broker` nodes. A broker is the node-of-many fleet host and a machine runs one; a `direct` node is a node-of-one delivery host and a single machine legitimately runs many, so collapsing those would strand agents. An explicit `node_id` still wins, which is how to run two brokers on one machine. The lookup resolves oldest-first so a roster already holding several rows for one machine converges on its earliest rather than picking arbitrarily. Migration 0043 indexes `nodes(workspace_id, machine_id)` — the lookup runs on every enrollment. Non-unique: the existing roster already holds many rows per machine, and direct nodes are meant to repeat. Verified on a local dev server (SQLite), before and after, with the same probe: two enrollments, same `machine_id`, different names. before: roster rows for one machine_id: 2, machine_id NULL on both after: roster rows for one machine_id: 1, same node id, machine_id recorded Reverting the two source files makes 3 of the 8 new conformance tests fail. Complements relaycast-cloud#91, which reaps existing rows; this stops the refill. Rollout to the hosted gateway needs the companion relaycast-cloud change: the D1 index migration, the `@relaycast/engine` bump, and the bundled engine-version marker bump. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QgtVrPUzGDZwa89Bg4deaL Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 * fix(engine): infer the requested role before matching on machine_id Codex review on #363 found a P1 in the machine_id dedupe, and it reproduces. The machine lookup skipped only an *explicit* `role: "direct"`. But the role default depends on `kind`: `http_push` and `poll` nodes default to `direct`, and a caller that omits `role` left `data.role` undefined. So an http_push node enrolling on a machine that already had a broker matched that broker, and the route then inherited the broker's role, rotated its token and rewrote its transport to http_push. Silently moving a live node's identity is a worse failure than the roster growth this dedupe exists to stop. `requestedNodeRole` now derives the role from the request alone, mirroring the route's own default, and only a broker request reaches the machine lookup. Two tests cover it — http_push and poll — and both fail without the change: before: rows: 1, broker-host rotated to kind=http_push after: rows: 2, broker-host ws/broker, push-host http_push/direct Also from that review: - Enrollments keyed on one machine are serialized in-process (`serializeMachineEnroll`, alongside the existing `serializeNodeOp`). Resolve and insert are not one step and the index is deliberately non-unique, so concurrent first-enrollments of one machine could each miss and each insert. This closes the race within an isolate, not across them; a duplicate that still slips through self-corrects, since the lookup resolves oldest-first. - `machine_id` added to the `NodeRosterEntry` response schema in `openapi.yaml`. The endpoint description already promised it and `publicNode` already emitted it, so OpenAPI-generated clients were missing a field the TypeScript SDK had. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QgtVrPUzGDZwa89Bg4deaL Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 * docs: align changelog heading link and tighten the engine entry Both from cubic review on #363. Raising the root heading to `[Unreleased - Minor]` left the reference-style link definition as `[Unreleased - Patch]:`, matching no in-text label — `cut-changelog.mjs` does not rewrite it. The engine entry carried design backstory that AGENTS.md asks changelogs to omit. Trimmed to the API and migration detail a package changelog is for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QgtVrPUzGDZwa89Bg4deaL Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 * fix(engine): only broker enrollments take the machine queue Two from cubic review on #363, plus a composition guard for the post-#364 tree. Serialization was keyed on `machine_id` alone, so a direct-node enrollment carrying one queued behind every other enrollment on that machine even though it never consults the machine lookup. A box running many direct node-of-one delivery hosts would serialize enrollments that are genuinely independent. Gate the queue on `requestedNodeRole(...) === 'broker'`, matching the condition that actually reaches the lookup. The concurrency test asserted only the roster length, so it would have passed if one enrollment had failed outright. Both enrollments now have to return 201 before the row count means anything; same for the two direct hosts. Also pin the one real interaction between this feature and the node.register path, now that #364 has removed server-authoritative tag preservation: a broker that enrolls with a `machine_id` and then registers WITHOUT one must keep the stored value. `recomputeNodeAggregate` writes `machineId` only when non-null, and this feature depends on that — if a register could null it, the node's next boot under a fresh name would mint a new row and the dedupe would silently stop working. The test asserts the value survives and that dedupe still holds after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QgtVrPUzGDZwa89Bg4deaL Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 * fix(engine): never adopt a live broker on a machine_id match CodeRabbit, Major, on the current head: the dedupe tests covered a renamed reboot but not a second broker presenting a live incumbent's machine_id. That case was a silent hijack. Enrollment matched the incumbent, renamed it to the claimant's name and rotated its token — a running broker losing its identity and its credential with no error raised, the same shape as burning an agent name. Reproduced before fixing: expected 'node_live' not to be 'node_live' The match now skips any node that `isNodeLive`. The reasoning is that a live incumbent cannot be the host re-enrolling, because that host has not left; only a node that is gone can be the one coming back. Liveness is read through `isNodeLive` rather than reproduced in SQL so it cannot drift from the definition placement and the roster already use. Declining the match rather than rejecting the enrollment is deliberate. A shared machine_id is not automatically hostile: a VM cloned from a snapshot, or containers baked from a single image, carry the same id and run concurrently — they are genuinely separate nodes and each deserves its own row. Rejecting would break an entire fleet booted from one image; declining costs one extra roster row, which is exactly the pre-existing behaviour. The roster-refill fix is untouched, because a rebooting host's incumbent is offline by then. Three tests, all failing without the change or pinning its edges: - a live incumbent keeps its id, name AND token hash while the claimant gets its own row - the same claim against a stale incumbent still collapses, so the guard is liveness and not identity - the register/enroll composition test now takes the node offline first, which is the real sequence a re-enroll follows Full engine suite 711/711. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QgtVrPUzGDZwa89Bg4deaL Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 * docs: document that a live broker is never adopted on machine_id AGENTS.md keeps README.md and openapi.yaml aligned with behavior, and the liveness gate narrowed what enrollment does. Both now state that only an offline broker is reused, and that a caller presenting a live broker's machine_id gets its own node with the incumbent's id, name and token left untouched — including why that is the right answer for cloned VMs and containers baked from one image rather than a rejection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QgtVrPUzGDZwa89Bg4deaL Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 * fix(engine): filter live brokers before bounding the machine scan Two from cubic on the liveness gate. The candidate scan bounded before the liveness filter, so a machine with more live brokers than the limit could hide its one offline row past the bound and enrollment would insert another — the roster growth this feature exists to stop, reappearing precisely on the busiest machines. Live rows are now excluded in SQL, so the bound counts only reusable candidates. `isNodeLive` stays authoritative; the SQL predicate is a pre-filter. The one case where they can disagree is a heartbeat timestamped in the future, which `isNodeLive` treats as not live and SQL treats as live. SQL is the stricter side, so the row is skipped and a new node is created — an extra roster row, never an adopted live broker. New test builds 25 live brokers on one machine ahead of a single offline row and asserts the offline row is reused. It fails without the pre-filter: expected 'node_220881473274068992' to be 'node_reusable' Also: the docs said a "running" broker is never adopted, which overstates the guard. It checks liveness — `status: online` with a heartbeat inside the node liveness TTL — so a broker whose process is up but whose heartbeat has lapsed is reusable. Reworded across CHANGELOG, engine CHANGELOG, README and openapi. Full engine suite 712/712. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QgtVrPUzGDZwa89Bg4deaL Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 * refactor(engine): name the machine-match reusability rule cubic's second-round P2 on the future-heartbeat case. The behaviour it flags is intended, but it was only expressed as a `lte` in SQL plus a code comment, with the TS side still checking the looser `!isNodeLive`. Two predicates that disagree is a drift hazard whatever the intent, and the intent was not written anywhere a reviewer would read as a decision. `isReusableForMachineMatch` is now the single definition, sitting beside `isNodeLive` and composing it. SQL mirrors it exactly, and the TS re-check runs the same rule rather than a more permissive one. The rule keeps treating an `online` row with a future heartbeat as live. `lastHeartbeatAt` is always stamped server-side, so a future value means the server clock moved backwards; `isNodeLive` requires `age >= 0` and therefore reports such a node as not live while it may still be heartbeating normally. Deferring to it there would hand a running broker's row and token to the next caller — the hijack the liveness gate exists to prevent, in the one window where `isNodeLive` cannot see it. The cost is a duplicate row bounded at one per machine per rollback, not one per enrollment: the row enrollment creates instead carries a null heartbeat, so the next enrollment reuses that one. It also self-heals — once the clock passes the stale timestamp the original becomes reusable and oldest-first convergence collapses back onto it. The new test asserts both halves. Full engine suite 713 tests; two http_push redrive sweep tests flaked under load and pass in isolation (unrelated to enrollment). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QgtVrPUzGDZwa89Bg4deaL Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 * fix(engine): require a prior heartbeat before reusing a machine_id row P1 from independent review at 40866bf, reproduced here before fixing. Two hosts cold-booting from one snapshot or baked image enroll moments apart. At the instant the second enrolls, the first has enrolled but not yet registered, so its row is `offline` with a null heartbeat — which the old predicate called reusable. The second adopted it, overwriting `tokenHash`, and the first host was left holding a 201 and a credential that silently stopped authenticating. Neither side saw an error. Reproduced at head: expected 'node_2209...' not to be 'node_2209...' The liveness guard did not help: it protects a broker that is heartbeating, and clones have not heartbeated yet. This was also a regression against main, where zod stripped machine_id and both clones kept their own row and token. The deeper problem is that "one host enrolling twice" and "two clones enrolling once each" are the same request sequence — enrollment has no information that separates them. So reuse now requires proof that a host held the row and left: the row must have heartbeated at least once, and that heartbeat must be older than the liveness TTL. Live rows and future-dated heartbeats stay excluded for the reasons already documented. This narrows the feature, and the trade should be explicit: a host that enrolls and never connects is no longer deduped, so this bounds the roster for hosts that actually join the fleet. Never-connected rows have no working credential to protect and are reclaimed by the roster reaper (relaycast-cloud#91) instead. The probe changed with it. Its old scenario — enroll twice, never connect — was demonstrating the unsafe path, so it now covers both a reboot that had connected and a clone cold-boot, and asserts the first host's row and name survive: Scenario 1 reused_same_row: true roster_rows: 1 PASS Scenario 2 vmB_took_vmA_row: false roster_rows: 2 vmA_row_name_now: clone-a PASS Tests rewritten against the safe semantics; the clone case fails without this change. Full engine suite 711/711. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QgtVrPUzGDZwa89Bg4deaL Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 * fix(engine): gate machine_id reuse on proven liveness, not lastHeartbeatAt Both P1s from the 17:50 review. They share one root, so they are fixed together rather than patched separately. `lastHeartbeatAt` does not mean "this node proved it was alive". Enumerating every writer of the column on `nodes`: PROOF OF LIFE (a frame arrived) heartbeatNode node.ts:598 node.heartbeat, direct branch heartbeatProvider nodeProvider.ts:162 node.heartbeat, broker branch REGISTRATION (the node connected; it never sent a heartbeat) registerNode node.ts:523 upsertProvider nodeProvider.ts:114,133 recomputeNodeAggregate nodeProvider.ts:352 (`lastHeartbeatAt ?? new Date()`) DISCONNECT CLEANUP (proves the opposite) markNodeOffline node.ts:675,684 markProviderOffline nodeProvider.ts:177 markDirectNodeOfflineForAgent node.ts:1103 IMPLICIT DIRECT-NODE LIFECYCLE ensureDirectNodeForAgent node.ts:1003,1034,1078,1086 agent.ts:225 (null) So a node that registered and never heartbeated satisfied the old gate once the TTL passed — P1 (confidence 9). And `createNodeToken` rotated `tokenHash` while preserving the stale timestamp, so a row reused once still looked proven and could be taken again immediately, revoking the credential just issued — P1 (confidence 10). Migration 0044 adds `proven_live_at`, written ONLY by the two heartbeat paths and cleared in `createNodeToken`'s update branch when a row's token is re-issued. `isReusableForMachineMatch` reads that column; the SQL pre-filter mirrors it. Three tests, each failing against the specific thing it guards: - reverting only the clear-on-rotation fails the double-reuse test - reverting only the predicate to lastHeartbeatAt fails the registered-only test and the double-reuse test - an end-to-end test asserts registration leaves provenLiveAt null while a heartbeat frame sets it Probe unchanged in shape, re-run on this build: Scenario 1 reused_same_row true, roster_rows 1 PASS Scenario 2 vmB_took_vmA_row false, roster_rows 2, vmA name clone-a PASS Full engine suite 714/714. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QgtVrPUzGDZwa89Bg4deaL Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 * fix(engine): mirror the full reusability predicate in SQL Six threads from review run a9cf7c6a. The three substantive ones are one defect, and it is a regression I introduced in 20cbb08: moving the gate from lastHeartbeatAt to provenLiveAt, I REPLACED the SQL predicate instead of extending it, dropping two guards I had added earlier for exactly these cases. CodeRabbit (Major) and cubic (P2), node.ts:402 — `provenLiveAt < liveCutoff` does not mean "not live". A broker that registered a moment ago has a fresh lastHeartbeatAt and is live, while its last real proof can be hours old. Those rows were admitted, filled the scan window ahead of the reusable row, and enrollment created another. The TS re-check rejected them, but only after the LIMIT had spent the window. cubic (P1), placement.ts:111 — a future lastHeartbeatAt makes isNodeLive return false on the negative age, so a stale proof read as reusable while the broker was still running. The future check moved to provenLiveAt and was not kept on lastHeartbeatAt. The SQL now mirrors isReusableForMachineMatch completely — stale proof, not live, and no future-dated heartbeat — so the bound counts only reusable rows. cubic (P2) on the fixture was the sharpest of the six: the crowded-machine test had no provenLiveAt on its 25 live rows, so they never survived the proof filter and never reached the scan window. The test was passing for the wrong reason and had never exercised the regression it was named for. They now carry a stale proof with a fresh heartbeat, the exact shape that must be admitted by the proof filter and rejected as live. Reverting each fix alone fails its own test: SQL liveness clauses -> × finds the reusable row even when many live brokers… future-heartbeat check -> × does not reuse a row with a stale proof but a future heartbeat Two minor, both fair: the engine changelog entry had become a design note against the AGENTS.md concision rule, and the README stated the no-double-reuse guarantee unqualified when serializeMachineEnroll is isolate-local. The README now scopes it to one process and names the boundary. Machine-id suite 18/18. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QgtVrPUzGDZwa89Bg4deaL Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978 * test(engine): pin the SQL future-heartbeat clause with a crowding case cubic P3: the future-heartbeat test used an `online` row, which the SQL not-live clause already excludes, so the separate future-heartbeat clause could be deleted without failing anything. Their suggested fix — switch the row to `offline` — does not pin it either, and I checked before applying it. An offline row with a future heartbeat is admitted by SQL and then rejected by `isReusableForMachineMatch`, so the test still passes with the SQL clause removed. For a single row the JS check is the real guard and the SQL clause is only a pre-filter. What the clause actually buys is scan-window headroom, so only crowding shows its absence: 25 future-dated rows ahead of one reusable row. Without the clause they fill the window and the reusable row is never seen. removed SQL future clause -> × finds the reusable row behind many future-dated rows Kept the `offline` status change as well, since it exercises the JS future check on a non-live row rather than duplicating the live case. Machine-id suite 19/19. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QgtVrPUzGDZwa89Bg4deaL Session-Id: c39a612d-f08b-44de-837c-dfcd1819a978
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release gate
Hold #360 out of the next engine cut while keeping merged fixes #355 and #358 releasable.
#360 merged with four unresolved functional review threads and no author response:
cloud:*tags can become trusted and preserved indefinitely;cloud:*namespace breaks the documented non-repo:client-tag round trip.Source inspection confirms
POST /v1/nodescallscreateNodeTokenoutside the per-nodeserializeNodeOpused by node-control registration. Publishing the current main would therefore ship a known stale-write race as the cure for tag churn.This is the reasoned exclusion allowed by the release brief. Follow-up #365 restores the behavior with explicit tag authority/provenance, shared node mutation ordering that survives the hosted runtime, and deterministic interleaving controls.
#363 interaction
#363 does not close this hole. Its machine enrollment queue uses
['machine', workspaceId, machineId]; registration uses the deliberately separate['node', workspaceId, nodeId]queue, and enrollment withoutmachine_idtakes no machine queue. Both queues are process-local. A synthetic three-way composition check against current main produced the same final tree in either merge order: #360's authority marker/test are absent while #363's machine lock and migration 0043 remain present.Verification
git diff --check: exit 0SERVER_AUTHORITATIVE_TAG_PREFIX; this branch does notgit diff --checkNo merge, release, or deploy has been performed.