From 66ff417278ac7ffc9f838b95366e913d4ac11d0e Mon Sep 17 00:00:00 2001 From: Khaliq Date: Sat, 15 Aug 2026 22:21:13 +0200 Subject: [PATCH] fix(engine): tombstone on node-completed release instead of deleting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `relaycast#309` gave the local release path (`dispatchRelease` -> `completeLocally`) a tombstone, because four FKs reference `agents.id` with no ON DELETE action — `messages.agent_id`, `channels.created_by`, `files.uploaded_by`, `webhooks.created_by` — so a bare DELETE is refused for any agent that has ever spoken. The node-completed path (`applyReleaseCompletionEffect`) kept the bare DELETE. Because it runs inside the completion's atomic unit, that refusal aborts the invocation completion along with it: the invocation stays `dispatched` forever, the seat and the name stay claimed, and the caller gets a plausible receipt for work that never happened. Observed in production 2026-08-15 on a live workspace. Across five agents the split was exactly "has authored messages / has not": agent channel posts remove relay-e2e 8 FAILS relay-terminal 8 FAILS relay-terminal2 2 FAILS relay-dmresolve 0 SUCCEEDS relay-e2epr 0 SUCCEEDS Three seats are still stuck on that workspace as a result. Operationally this is backwards: only agents that never spoke can be reclaimed, while the ones worth reclaiming are exactly the ones that did work, so a node whose agents were productive cannot be fully recovered after a restart. This applies the same tombstone the local path already proved: rename to `releasedAgentName` (freeing the unique `(workspace_id, name)`), rotate `token_hash` so the surviving row's credential stops authenticating, set `RELEASED_AGENT_STATUS`, and clear the routing location. Test: `nodeCompletedRelease.test.ts`, must-fire / must-not-fire, driven over the node control channel (`action.result`) because the engine refuses off-channel completion of node-owned invocations. Verified both directions: without this change the has-history case fails and the no-history case still passes; with it both pass. Note: `a2aFederation` and `providerAttachRace` fail in the full suite both with and without this change, and the failing set varies between runs — pre-existing flakiness in the concurrency tests, unrelated to this fix. Co-Authored-By: Claude Opus 5 --- .../conformance/nodeCompletedRelease.test.ts | 137 ++++++++++++++++++ packages/engine/src/engine/action.ts | 26 +++- 2 files changed, 162 insertions(+), 1 deletion(-) create mode 100644 packages/engine/src/__tests__/conformance/nodeCompletedRelease.test.ts diff --git a/packages/engine/src/__tests__/conformance/nodeCompletedRelease.test.ts b/packages/engine/src/__tests__/conformance/nodeCompletedRelease.test.ts new file mode 100644 index 00000000..e5a5f2f3 --- /dev/null +++ b/packages/engine/src/__tests__/conformance/nodeCompletedRelease.test.ts @@ -0,0 +1,137 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { and, eq } from 'drizzle-orm'; +import { attachDirectNodeSocket, createWorkspace, makeNodeStack, registerAgent, type TestStack } from './harness.js'; +import { actionInvocations, agents, messages } from '../../db/schema.js'; + +/** + * `relaycast#309` gave the LOCAL release path (`dispatchRelease` -> + * `completeLocally`) a tombstone, because four FKs reference `agents.id` with + * no ON DELETE action — `messages.agent_id`, `channels.created_by`, + * `files.uploaded_by`, `webhooks.created_by` — so a bare DELETE is refused for + * any agent that has ever spoken. + * + * The NODE-COMPLETED path (`applyReleaseCompletionEffect`) kept the bare + * DELETE. Because it runs inside the completion's atomic unit, the FK refusal + * aborts the invocation completion along with it: the invocation is stuck at + * `dispatched` forever, the seat and the name stay claimed, and the caller sees + * a plausible-looking receipt. Observed in production on 2026-08-15, where the + * split was exactly "agent has authored messages / has not" across five agents. + */ +describe('node-completed release preserves attributed history', () => { + let stack: TestStack; + + beforeEach(() => { + stack = makeNodeStack(); + }); + + afterEach(() => stack.close()); + + async function post(token: string, text: string) { + const res = await stack.app.request('/v1/channels/general/messages', { + method: 'POST', + headers: { 'content-type': 'application/json', authorization: `Bearer ${token}` }, + body: JSON.stringify({ text }), + }); + expect(res.status).toBe(201); + } + + async function release(workspaceKey: string, name: string) { + const res = await stack.app.request('/v1/agents/release', { + method: 'POST', + headers: { 'content-type': 'application/json', authorization: `Bearer ${workspaceKey}` }, + body: JSON.stringify({ name, delete_agent: true }), + }); + expect(res.status).toBe(201); + return (await res.json()) as { data: { status: string; invocation_id: string } }; + } + + // MUST-FIRE: fails before the fix — the completion aborts on the FK and the + // agent is never released. + it('tombstones an agent that has spoken when a NODE completes the release', async () => { + const ws = await createWorkspace(stack.app, 'node-release-with-history'); + const target = await registerAgent(stack.app, ws.workspaceKey, 'spoke-then-released'); + await post(target.token, 'this message must keep its author'); + // A LIVE node binding is what routes the release through + // `applyReleaseCompletionEffect` instead of the local tombstone path. + const { handle } = await attachDirectNodeSocket(stack, ws.workspaceId, target); + + const { data } = await release(ws.workspaceKey, target.name); + expect(data.status).toBe('dispatched'); + + // Drive the node side of the handshake: this is what the broker does. + await handle.handleMessage(JSON.stringify({ + v: 1, + type: 'action.result', + invocation_id: data.invocation_id, + output: { released: true }, + })); + + const [invocation] = await stack.runtime.deps.db + .select({ status: actionInvocations.status }) + .from(actionInvocations) + .where(eq(actionInvocations.id, data.invocation_id)); + expect(invocation.status).toBe('completed'); + + // The name — the scarce resource — must be free for immediate reuse. + expect( + await stack.runtime.deps.db + .select() + .from(agents) + .where(and(eq(agents.workspaceId, ws.workspaceId), eq(agents.name, target.name))), + ).toHaveLength(0); + + // The row survives as a tombstone so history keeps its author. + const [tombstone] = await stack.runtime.deps.db + .select({ name: agents.name, status: agents.status, tokenHash: agents.tokenHash }) + .from(agents) + .where(eq(agents.id, target.agentId)); + expect(tombstone).toMatchObject({ + name: `${target.name}#released-${target.agentId}`, + status: 'released', + }); + + // Attribution intact, and the old credential is dead. + expect( + await stack.runtime.deps.db.select().from(messages).where(eq(messages.agentId, target.agentId)), + ).toHaveLength(1); + const reuse = await stack.app.request('/v1/channels/general/messages', { + method: 'POST', + headers: { 'content-type': 'application/json', authorization: `Bearer ${target.token}` }, + body: JSON.stringify({ text: 'should be rejected' }), + }); + expect(reuse.status).toBeGreaterThanOrEqual(400); + + // And the freed name is genuinely reusable. + const successor = await registerAgent(stack.app, ws.workspaceKey, target.name); + expect(successor.agentId).not.toBe(target.agentId); + }); + + // MUST-NOT-FIRE: an agent with no history must release identically, so the + // fix cannot pass by making the silent case behave differently. + it('releases an agent that never spoke through the same node-completed path', async () => { + const ws = await createWorkspace(stack.app, 'node-release-no-history'); + const target = await registerAgent(stack.app, ws.workspaceKey, 'never-spoke'); + const { handle } = await attachDirectNodeSocket(stack, ws.workspaceId, target); + + const { data } = await release(ws.workspaceKey, target.name); + expect(data.status).toBe('dispatched'); + await handle.handleMessage(JSON.stringify({ + v: 1, + type: 'action.result', + invocation_id: data.invocation_id, + output: { released: true }, + })); + + const [invocation] = await stack.runtime.deps.db + .select({ status: actionInvocations.status }) + .from(actionInvocations) + .where(eq(actionInvocations.id, data.invocation_id)); + expect(invocation.status).toBe('completed'); + expect( + await stack.runtime.deps.db + .select() + .from(agents) + .where(and(eq(agents.workspaceId, ws.workspaceId), eq(agents.name, target.name))), + ).toHaveLength(0); + }); +}); diff --git a/packages/engine/src/engine/action.ts b/packages/engine/src/engine/action.ts index dad8982e..2bbe7897 100644 --- a/packages/engine/src/engine/action.ts +++ b/packages/engine/src/engine/action.ts @@ -1358,7 +1358,31 @@ async function applyReleaseCompletionEffect( } if (input.delete_agent === true) { - await db.delete(agents).where(and(eq(agents.workspaceId, workspaceId), eq(agents.id, agent.id))); + // Tombstone rather than DELETE, matching `dispatchRelease`'s + // `completeLocally`. Four FKs reference `agents.id` without an ON DELETE + // action (`messages.agent_id`, `channels.created_by`, `files.uploaded_by`, + // `webhooks.created_by`), so a bare delete is refused for any agent that + // has ever spoken — and this runs inside the completion's atomic unit, so + // that refusal aborts the invocation completion too. The seat and the name + // then stay claimed forever and the caller only ever sees `dispatched`. + // Renaming frees the unique `(workspace_id, name)` immediately while every + // FK target stays valid and every message keeps its sender. + const releasedName = releasedAgentName(agent.name, agent.id); + // The row survives, so its credential must not. `token_hash` is NOT NULL + // UNIQUE and cannot be cleared, so rotate it to a value nobody holds. + const releasedTokenHash = await sha256Hex(`released:${agent.id}:${randomHex(16)}`); + await db + .update(agents) + .set({ + name: releasedName, + handle: `@${releasedName}`, + status: RELEASED_AGENT_STATUS, + tokenHash: releasedTokenHash, + locationType: 'self_connected', + locationNodeId: null, + lastSeen: new Date(), + }) + .where(and(eq(agents.workspaceId, workspaceId), eq(agents.id, agent.id))); const implicitNodeId = `node_direct_${agent.id}`; await db.delete(nodes).where(and(eq(nodes.workspaceId, workspaceId), eq(nodes.id, implicitNodeId))); } else {