From 3fa3f04ecd807565843bb6bce75635dcbb3e02c3 Mon Sep 17 00:00:00 2001 From: Miya Date: Thu, 27 Aug 2026 14:33:43 +0200 Subject: [PATCH 1/2] fix(fleet): accept an unnamed placement instead of killing the worker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @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 --- src/fleet/relay-fleet-client.test.ts | 80 +++++++++++++++++++++++++--- src/fleet/relay-fleet-client.ts | 64 +++++++++++++++++----- 2 files changed, 125 insertions(+), 19 deletions(-) diff --git a/src/fleet/relay-fleet-client.test.ts b/src/fleet/relay-fleet-client.test.ts index 417e3d4c..6160384b 100644 --- a/src/fleet/relay-fleet-client.test.ts +++ b/src/fleet/relay-fleet-client.test.ts @@ -325,13 +325,11 @@ describe('RelayFleetClient', () => { }) }) - it.each([ - ['self', { node: 'self' }], - ['an empty node', { node: '' }], - ['an absent node', {}], - ])('fails closed when placement resolves to %s', async (_label, placement) => { + // `self` is a positive assertion that the work did NOT go remote, so it must + // still fail closed and tear the worker down. + it('fails closed when placement resolves to self', async () => { const messaging = new FakeMessaging() - messaging.placementAck = { placement } + messaging.placementAck = { placement: { node: 'self' } } const fleet = createClient(messaging) await expect(fleet.spawn({ @@ -353,6 +351,76 @@ describe('RelayFleetClient', () => { }) }) + // CONTRACT CHANGE, deliberate. These two cases previously shared the `self` + // assertion above and were expected to fail closed. That was correct while + // `placement.node` was REQUIRED: an absent name could only mean something had + // gone wrong. @agent-relay/sdk 11.8.5 (relay#1619) made it OPTIONAL and + // documents it as "absent when acknowledgment metadata is missing or not yet + // visible", so an unnamed placement is now an ordinary SUCCESSFUL spawn. + // + // Keeping the old expectation would make Factory release — i.e. kill — a + // worker it had just launched, every time the roster lagged. That is the + // production incident this change exists to prevent, so the guard must + // distinguish "no name yet" from "explicitly not remote". + it.each([ + ['an empty node', { node: '' }], + ['an absent node', {}], + ])('accepts an unnamed placement (%s) without releasing the worker', async (_label, placement) => { + const messaging = new FakeMessaging() + messaging.placementAck = { placement } + const fleet = createClient(messaging) + + const result = await fleet.spawn({ + name: 'ar-1-impl', + capability: 'spawn:codex', + node: 'self', + repo: 'AgentWorkforce/factory', + task: 'do work', + }) + + expect(result.name).toBe('ar-1-impl') + // Accepted and tracked — the worker keeps running. + expect(fleet.trackedAgents().size).toBe(1) + // Never torn down. + expect(messaging.invokes).not.toContainEqual( + expect.objectContaining({ name: 'release' }), + ) + // And no node name is invented for it: only the acknowledgement may name + // the node, and it did not. + expect(result.node).toBeUndefined() + }) + + it('does not inherit an action-output node when the acknowledgement is unnamed', async () => { + // The `self` guard exists because action output can name the node running + // the spawn handler. That output is equally untrustworthy when the + // acknowledgement is merely silent, so an unnamed placement must not quietly + // adopt it — otherwise "accepted but unidentified" would launder an + // unverified name into a trusted result. + const messaging = new FakeMessaging() + messaging.placementAck = { invocationId: 'unnamed-placement', status: 'pending', placement: {} } + messaging.invocations.set('unnamed-placement', [{ + invocationId: 'unnamed-placement', + actionName: 'spawn', + status: 'completed', + output: { name: 'ar-1-impl', node: 'mac-mini' }, + }]) + const fleet = createClient(messaging) + + const result = await fleet.spawn({ + name: 'ar-1-impl', + capability: 'spawn:codex', + node: 'self', + repo: 'AgentWorkforce/factory', + task: 'do work', + }) + + expect(result.node).toBeUndefined() + expect(fleet.trackedAgents().get('ar-1-impl')?.node).toBeUndefined() + expect(messaging.invokes).not.toContainEqual( + expect.objectContaining({ name: 'release' }), + ) + }) + it('rejects the acknowledgement node even when action output synthesizes a named node', async () => { const messaging = new FakeMessaging() messaging.placementAck = { diff --git a/src/fleet/relay-fleet-client.ts b/src/fleet/relay-fleet-client.ts index 24d39bf6..3b968bbd 100644 --- a/src/fleet/relay-fleet-client.ts +++ b/src/fleet/relay-fleet-client.ts @@ -373,14 +373,16 @@ export class RelayFleetClient implements FleetClient { throw error } const result = spawnResultFromInvocation(input.name, input.sessionRef, invocation, ack) - let acknowledgedNode: string + let acknowledgedNode: string | undefined try { - acknowledgedNode = assertNamedRemotePlacement(result, ack.placement?.node) + acknowledgedNode = assertRemotePlacement(result, ack.placement?.node) } catch (error) { // A completed placement invocation has already launched the worker. If - // Relay cannot prove that it ran on a named remote node, tear that worker - // down before refusing the result so a rejected spawn cannot keep acting - // outside Factory's lifecycle tracking. + // Relay positively acknowledged `self`, tear that worker down before + // refusing the result so a rejected spawn cannot keep acting outside + // Factory's lifecycle tracking. Reached only for `self` — an accepted + // placement whose node name is merely absent is NOT a failure and must + // never land here, or Factory would kill workers it just started. try { await this.release(result.name, 'unverified-placement') } catch (releaseError) { @@ -398,8 +400,20 @@ export class RelayFleetClient implements FleetClient { } throw error } - const trustedResult = { ...result, node: acknowledgedNode } - this.#track(trustedResult.name, { invocationId: ack.invocationId, node: acknowledgedNode }) + // Only the acknowledgement may name the node. `spawnResultFromInvocation` + // derives a node from action output, which is exactly the untrusted source + // the `self` guard above exists to defeat — so when the acknowledgement + // carries no name, strip it rather than inheriting that guess. An accepted + // placement with an unknown node is tracked without one; the roster + // reconciliation loop is what later attributes it. + const { node: _untrustedNode, ...resultWithoutNode } = result + const trustedResult: SpawnResult = acknowledgedNode + ? { ...result, node: acknowledgedNode } + : resultWithoutNode + this.#track(trustedResult.name, { + invocationId: ack.invocationId, + ...(acknowledgedNode ? { node: acknowledgedNode } : {}), + }) return trustedResult } @@ -469,7 +483,12 @@ export class RelayFleetClient implements FleetClient { log: this.#log, })) const invocation = await this.#awaitInvocation(ack.actionName || 'preview:tailscale-serve', ack, deadlineAtMs) - return previewReferenceFromInvocation(invocation, ack.placement?.node ?? ack.dispatchedNodeId) + // `dispatchedNodeId` is `string | null`, and `placement.node` became + // optional in @agent-relay/sdk 11.8.5, so this chain can yield `null`. + // Collapse it to `undefined` explicitly — the callee distinguishes only + // "have a node" from "do not", and a `null` masquerading as a value here + // is how an absent node turns into a bad preview reference. + return previewReferenceFromInvocation(invocation, ack.placement?.node ?? ack.dispatchedNodeId ?? undefined) } async removePreview(preview: PreviewReference): Promise { @@ -1690,12 +1709,31 @@ function spawnResultFromInvocation( } } -function assertNamedRemotePlacement(result: SpawnResult, acknowledgedNode: string | undefined): string { - // The placement acknowledgement is authoritative. Action output may name - // the node executing the spawn handler even when Relay acknowledged `self`, - // so accepting the synthesized SpawnResult would let self-placement pass. +/** + * Decide whether an accepted placement may be trusted as remote. + * + * Two outcomes that used to be one. `@agent-relay/sdk` 11.8.5 (relay#1619) made + * `placement.node` OPTIONAL, documenting it 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. Before that change `placement.node` was required, so `!node` could only + * mean something was wrong; now it is an ordinary outcome of a SUCCESSFUL spawn. + * + * `'self'`, by contrast, is a positive assertion that the work did NOT go + * remote, and it must still be refused. Action output may name the node running + * the spawn handler even when Relay acknowledged `self`, so the acknowledgement + * — not the synthesized SpawnResult — remains the authority here. + * + * Returns the proven remote node name, or `undefined` when the placement was + * accepted but no node name is available yet. Throws only for `'self'`. + */ +function assertRemotePlacement(result: SpawnResult, acknowledgedNode: string | undefined): string | undefined { const node = acknowledgedNode?.trim() - if (!node || node === 'self') { + if (!node) { + // Accepted, but unidentified. The caller must not invent a name for it. + return undefined + } + if (node === 'self') { throw new Error( `Relay placement did not prove a named remote node for ${result.name}; ` + `refusing to accept the spawn result`, From 679aaa04591771997e4fd7afc6489d8f027744a9 Mon Sep 17 00:00:00 2001 From: Miya Date: Thu, 27 Aug 2026 18:28:49 +0200 Subject: [PATCH 2/2] test(fleet): prove unnamed workers survive reconciliation 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 --- src/fleet/relay-fleet-client.test.ts | 56 ++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/src/fleet/relay-fleet-client.test.ts b/src/fleet/relay-fleet-client.test.ts index 6160384b..244055d2 100644 --- a/src/fleet/relay-fleet-client.test.ts +++ b/src/fleet/relay-fleet-client.test.ts @@ -1165,6 +1165,62 @@ describe('RelayFleetClient', () => { expect(fleet.trackedAgents().has('ar-2-review')).toBe(true) }) + // Guards the downstream half of the unnamed-placement change. A review raised + // the concern that a worker tracked WITHOUT a node would be skipped by the + // registration probe and eventually torn down. It is not: the 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. + it('keeps reconciling a tracked worker that has no node, by name', async () => { + const messaging = new FakeMessaging() + // Online under its own name, and there is a live node it is NOT attributed to. + messaging.agentRows = [{ name: 'ar-1-impl', status: 'online' }] + messaging.nodeRows = [{ name: 'mac-mini', status: 'online', capabilities: [] }] + let nowMs = 1_000_000 + const fleet = createClient(messaging, { now: () => nowMs }) + const exits: Array<{ name: string; reason?: string }> = [] + fleet.onAgentExit((name, reason) => exits.push({ name, reason })) + + // An accepted placement whose acknowledgement carried no node name. + messaging.placementAck = { placement: {} } + const result = await fleet.spawn({ name: 'ar-1-impl', capability: 'spawn:codex' }) + expect(result.node).toBeUndefined() + expect(fleet.trackedAgents().get('ar-1-impl')?.node).toBeUndefined() + + // Well past the registration grace AND past the node-offline grace. + nowMs += 600_000 + await fleet.reconcileTrackedAgents() + await fleet.reconcileTrackedAgents() + + // Still tracked, never exited, never released. Being unnamed is not death. + expect(exits).toEqual([]) + expect(fleet.trackedAgents().has('ar-1-impl')).toBe(true) + expect(messaging.invokes).not.toContainEqual( + expect.objectContaining({ name: 'release' }), + ) + }) + + it('still exits an unnamed tracked worker when it actually leaves the roster', async () => { + // The complement of the test above: skipping the node check must not make + // an unnamed worker immortal. Name-based liveness still applies. + const messaging = new FakeMessaging() + messaging.agentRows = [] + messaging.nodeRows = [{ name: 'mac-mini', status: 'online', capabilities: [] }] + let nowMs = 1_000_000 + const fleet = createClient(messaging, { now: () => nowMs }) + const exits: Array<{ name: string; reason?: string }> = [] + fleet.onAgentExit((name, reason) => exits.push({ name, reason })) + + messaging.placementAck = { placement: {} } + await fleet.spawn({ name: 'ar-1-impl', capability: 'spawn:codex' }) + + nowMs += 600_000 + await fleet.reconcileTrackedAgents() + + expect(exits).toEqual([{ name: 'ar-1-impl', reason: 'exited' }]) + }) + it('synthesizes exits for offline roster rows and dead nodes after their grace windows', async () => { const messaging = new FakeMessaging() messaging.agentRows = [