fix(fleet): accept an unnamed placement instead of killing the worker - #407
Conversation
@agent-relay/sdk 11.8.5 (relay#1619) made `RelaySpawnPlacementAck.node` and `placement.node` OPTIONAL, documenting the latter as "absent when acknowledgment metadata is missing or not yet visible". That was deliberate: an accepted placement must not become a failure merely because the roster could not be read. Factory had not caught up. `spawn()` passed only `ack.placement?.node` into `assertNamedRemotePlacement`, whose `if (!node || node === 'self')` treated an absent name identically to an explicit self-placement — and the catch does not merely reject, it calls `release(name, 'unverified-placement')`. Under `requireNode` (set whenever placementLocality === 'remote', i.e. production), a successful remote spawn whose roster metadata was not yet visible would therefore TEAR DOWN the worker it had just launched. Split the two cases: - absent/empty name -> accepted, worker retained, no node name claimed - explicit 'self' -> still refused, worker still released When the acknowledgement carries no name, the output-derived node is stripped rather than inherited. Action output can name the node running the spawn handler, which is exactly the untrusted source the `self` guard exists to defeat; letting an unnamed placement adopt it would launder an unverified name into a trusted result. An accepted-but-unidentified placement is tracked without a node. Also narrows `dispatchedNodeId` (`string | null`) to `undefined` at the preview call site rather than letting a null masquerade as a node name. Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus 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 |
|
@coderabbitai review Requested for exact head |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Review raised a P1: that a worker tracked without a node would be skipped by
the registration probe and torn down after the registration timeout. It is not.
The liveness probe keys on AGENT NAME (`onlineAgentNames.has(name)`), so an
unnamed worker is checked exactly like a named one. The `!entry.node` skip
bypasses only the node-offline check, which is meaningless without a node name
and is safer skipped than guessed. Nothing on that path releases: after the
grace an absent agent reaches `#emitExit`, which deletes tracking and notifies
listeners.
Two tests rather than an argument:
- an unnamed worker that stays online survives both grace windows, is never
exited and never released;
- an unnamed worker that actually leaves the roster still exits, so skipping
the node check does not make it immortal.
Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7
|
@coderabbitai review Requested for exact head |
2 participants
The bug
@agent-relay/sdk11.8.5 (relay#1619) madeRelaySpawnPlacementAck.nodeandplacement.nodeoptional — verified by diffing the published tarballs 11.8.4 → 11.8.5. The SDK documents the latter as "absent when acknowledgment metadata is missing or not yet visible". That was deliberate and correct: #1619 exists so an accepted placement never becomes a failure just because the roster could not be read.Factory had not caught up.
spawn()passes onlyack.placement?.nodeintoassertNamedRemotePlacement— no fallback at that call site — and the guard was:so an absent name was treated identically to an explicit self-placement. The catch does not merely reject:
It tears the worker down.
factory.ts:13860setsrequireNodewheneverplacementLocality === 'remote', which is the production configuration. So a successful remote spawn whose roster metadata was not yet visible would cause Factory to kill the agent it had just launched — reproducing the dispatch outage under a new stack trace, and looking exactly like "the placement fix did not work".Before #1619 this was safe:
placement.nodewas required, so!nodecould only mean something had gone wrong. #1619 removed the only non-nullable link in that chain (dispatchedNodeIdwas alreadystring | nullin 11.8.4).The fix
Split the two cases that had been sharing one condition:
'self''self'is a positive assertion that the work did not go remote and must still fail closed. "No name yet" is not that.On not inheriting the output-derived node.
spawnResultFromInvocationbuilds a node from action output, and the existing comment explains why that is untrusted: "Action output may name the node executing the spawn handler even when Relay acknowledgedself". The old code deliberately discarded it in favour of the authoritative acknowledgement. So when the acknowledgement is silent, this strips the name rather than falling back to the guess — otherwise "accepted but unidentified" would launder an unverified name into a trusted result. An unidentified placement is tracked without a node; roster reconciliation attributes it later.Also narrows
dispatchedNodeId(string | null) toundefinedat the preview call site, so anullcannot masquerade as a node name.Ablation
The three failures are exactly the three new acceptance tests, and they fail for the predicted reason:
All 8 self-guard tests still PASS on reverted source, which is what proves the
selfguard was not weakened in the process. The error message is deliberately left unchanged so the ablation isolates behaviour rather than a string rename.Checks
tsconfig.build.json: 0 errors.tsconfig.json: 207 errors on this branch, 207 on unmodified main — no delta, none in the changed file. (Note: the "14 pre-existing errors" figure circulating yesterday was wrong against both configs.)src/cli/fleet.test.ts: 1 failed | 169 passed — the failure iskeeps relay dispatch ownership until the remote PR is published and the issue is parked(5037 ms timeout), which reproduces identically on unmodified origin/main. Pre-existing flake, not from this change.Note on the changed test
The previous
it.eachasserted thatself, empty, and absent all "fail closed". That was right under the old required-field contract and is wrong under the new one. It is split rather than deleted:selfkeeps its assertion verbatim, and empty/absent become explicit acceptance cases with a comment recording why the contract changed.Context
This is the last blocker before a Factory release that also carries #402 (the
candidates: 0fix) and the completegh→ GitHub App migration (#396–#401), all currently unreleased on main. No dep bump is needed:^11.6.9already resolves to 11.8.5.