From fea7ec33809057f0d00c8fe0e63cbd843e01cdaf Mon Sep 17 00:00:00 2001 From: Khaliq Date: Sat, 15 Aug 2026 22:38:06 +0200 Subject: [PATCH 1/2] fix(engine): tombstone on the DELETE /v1/agents/:name route too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the set. `relaycast#309` fixed the local release path and `relaycast#330` fixed the node-completed one; `agentEngine.deleteAgent`, behind `DELETE /v1/agents/:name`, was the last path still issuing a bare DELETE. Four FKs reference `agents.id` with no ON DELETE action (`messages.agent_id`, `channels.created_by`, `files.uploaded_by`, `webhooks.created_by`), so that delete is refused for any agent that has ever spoken, and the refusal reaches the operator as raw SQL carrying the row id: Failed query: delete from "agents" where "agents"."id" = ? params: 2144… This route is what older CLI builds call, so it stays reachable after the other two fixes ship. It also matters operationally: it is the only SYNCHRONOUS removal path. The release action is dispatched to a node and only tombstones when that node reports completion, so a seat whose worker no longer exists — the common case after a node restart — cannot be reclaimed through it at all. Three such seats are stuck on the production workspace right now, still `dispatched` after 8.0.2 deployed. Also fixes a regression this change surfaced. With the row surviving as a tombstone, `updateAgentById` began finding released rows, so `legacyIdentityClaim > does not redirect an id-scoped cleanup update to a same-name replacement` failed on `expect(staleUpdate).toBeNull()`. The assertion was right and the semantics were wrong: a released row is a tombstone kept only so history stays attributable, and must not be updatable. `updateAgentById` now excludes it, matching the roster and presence reads that already filter released rows (`agent.ts:272`, `presence.ts:22`). Test: `deleteAgentRoute.test.ts`, must-fire / must-not-fire through the HTTP route. Verified both directions — without the change the has-history case fails with a 500 and the no-history case still passes; with it both pass. Co-Authored-By: Claude Opus 5 --- .../conformance/deleteAgentRoute.test.ts | 103 ++++++++++++++++++ packages/engine/src/engine/agent.ts | 35 +++++- 2 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 packages/engine/src/__tests__/conformance/deleteAgentRoute.test.ts diff --git a/packages/engine/src/__tests__/conformance/deleteAgentRoute.test.ts b/packages/engine/src/__tests__/conformance/deleteAgentRoute.test.ts new file mode 100644 index 00000000..6350bbb0 --- /dev/null +++ b/packages/engine/src/__tests__/conformance/deleteAgentRoute.test.ts @@ -0,0 +1,103 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { and, eq } from 'drizzle-orm'; +import { createWorkspace, makeNodeStack, registerAgent, type TestStack } from './harness.js'; +import { agents, messages } from '../../db/schema.js'; + +/** + * `DELETE /v1/agents/:name` -> `agentEngine.deleteAgent` was the last release + * path still doing a bare DELETE, after `relaycast#309` fixed the local release + * path and `relaycast#330` fixed the node-completed one. + * + * Four FKs reference `agents.id` with no ON DELETE action + * (`messages.agent_id`, `channels.created_by`, `files.uploaded_by`, + * `webhooks.created_by`), so the delete is refused for any agent that has ever + * spoken — and the refusal reaches the operator as raw SQL with the row id in + * it, observed in production as: + * + * Failed query: delete from "agents" where "agents"."id" = ? params: 2144… + * + * This route is what older CLIs call, so it stays reachable after the other two + * fixes ship. + */ +describe('DELETE /v1/agents/:name 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); + } + + function removeAgent(workspaceKey: string, name: string) { + return stack.app.request(`/v1/agents/${name}`, { + method: 'DELETE', + headers: { authorization: `Bearer ${workspaceKey}` }, + }); + } + + // MUST-FIRE: fails before the fix — the bare DELETE is refused by the FK. + it('removes an agent that has authored messages, keeping the attribution', async () => { + const ws = await createWorkspace(stack.app, 'route-delete-with-history'); + const target = await registerAgent(stack.app, ws.workspaceKey, 'talkative'); + await post(target.token, 'this message must keep its author'); + + const res = await removeAgent(ws.workspaceKey, target.name); + expect(res.status).toBeLessThan(300); + + // The name is the scarce resource and must be free immediately. + 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 }) + .from(agents) + .where(eq(agents.id, target.agentId)); + expect(tombstone).toMatchObject({ + name: `${target.name}#released-${target.agentId}`, + status: 'released', + }); + + // Attribution intact; the old credential is dead; the name is reusable. + 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); + const successor = await registerAgent(stack.app, ws.workspaceKey, target.name); + expect(successor.agentId).not.toBe(target.agentId); + }); + + // MUST-NOT-FIRE: the silent case must behave identically, so the fix cannot + // pass by treating one class of agent specially. + it('removes an agent that never spoke through the same route', async () => { + const ws = await createWorkspace(stack.app, 'route-delete-no-history'); + const target = await registerAgent(stack.app, ws.workspaceKey, 'silent'); + + const res = await removeAgent(ws.workspaceKey, target.name); + expect(res.status).toBeLessThan(300); + 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/agent.ts b/packages/engine/src/engine/agent.ts index 781f3926..1f46cf9f 100644 --- a/packages/engine/src/engine/agent.ts +++ b/packages/engine/src/engine/agent.ts @@ -451,7 +451,15 @@ export async function updateAgentById( const [updated] = await db .update(agents) .set(setClause) - .where(and(eq(agents.workspaceId, workspaceId), eq(agents.id, agentId))) + // A released row is a tombstone kept only so history stays attributable — + // it is not a live agent and must not be updatable, matching the roster and + // presence reads that already exclude it. Without this, an id-scoped write + // against a released agent silently succeeds against the tombstone. + .where(and( + eq(agents.workspaceId, workspaceId), + eq(agents.id, agentId), + ne(agents.status, RELEASED_AGENT_STATUS), + )) .returning(); if (!updated) return null; @@ -527,7 +535,30 @@ export async function deleteAgent(db: Db, workspaceId: string, name: string) { if (!agent) return false; - await db.delete(agents).where(eq(agents.id, agent.id)); + // Tombstone rather than DELETE, matching both release paths + // (`dispatchRelease` -> `completeLocally`, and `applyReleaseCompletionEffect`). + // 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, and the caller sees the raw SQL failure with the row id in it. + // 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(eq(agents.id, agent.id)); await db.delete(nodes).where(eq(nodes.id, directNodeIdForAgent(agent.id))); return true; } From 4959e1959ea014c7bd907950223f569439d9ac3a Mon Sep 17 00:00:00 2001 From: Khaliq Date: Sat, 15 Aug 2026 22:48:19 +0200 Subject: [PATCH 2/2] fix(engine): drop subscriptions and block tombstone revival MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review on #331. Both findings were real and both are broader than this PR — they apply to the already-merged #309 and #330 paths too, so all three are fixed here rather than only the new one. P1 — a tombstoned agent stayed a delivery target. `channel_members` and `dm_participants` reference `agents.id` ON DELETE CASCADE, so the bare DELETE cleared them implicitly. Replacing it with an UPDATE does not fire the cascade, and `delivery.ts` fans out over channel members, so a released agent kept receiving. Now dropped explicitly in `deleteAgent` and in `applyReleaseCompletionEffect`. P2 — a late heartbeat could revive a tombstone. Auth schedules `touchLastSeen` without awaiting it, and it set `status: 'active'` unconditionally, so a heartbeat landing after a release flipped the row back to a live roster member. A tombstone is terminal; it now excludes released rows. P1 — changelog entries added to both the root and engine `[Unreleased]` sections, since this changes user-visible API behaviour. The subscription assertion discriminates: removing the `channel_members` cleanup fails it, restoring passes. Full engine suite: 292 passed, 0 failed. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 17 +++++++++++++++++ packages/engine/CHANGELOG.md | 17 +++++++++++++++++ .../conformance/deleteAgentRoute.test.ts | 12 +++++++++++- packages/engine/src/engine/action.ts | 7 ++++++- packages/engine/src/engine/agent.ts | 15 +++++++++++++-- 5 files changed, 64 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f16f5898..4917d838 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,23 @@ Packages without a separate changelog are covered by the cross-package notes bel ## [Unreleased] +### Fixed + +- `DELETE /v1/agents/:name` now tombstones an agent instead of issuing a bare + `DELETE`. Four foreign keys reference `agents.id` with no `ON DELETE` action + (`messages.agent_id`, `channels.created_by`, `files.uploaded_by`, + `webhooks.created_by`), so removing an agent that had ever spoken failed and + surfaced the raw SQL to the caller. The name is freed, the credential is + rotated, and message attribution is preserved. Completes the set alongside + the local (#309) and node-completed (#330) release paths. +- Releasing an agent now clears its `channel_members` and `dm_participants` + rows. Those cascade on `DELETE`, and the tombstone's `UPDATE` does not fire + the cascade, so a released agent could remain a delivery target. +- A late `touchLastSeen` can no longer revive a released tombstone back to + `active`, and id-scoped `updateAgentById` writes no longer resolve to a + released row. + + ## [8.0.1] - 2026-08-14 ### Changed diff --git a/packages/engine/CHANGELOG.md b/packages/engine/CHANGELOG.md index 35bcd28d..a633efcf 100644 --- a/packages/engine/CHANGELOG.md +++ b/packages/engine/CHANGELOG.md @@ -9,6 +9,23 @@ and this project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.ht ## [Unreleased] +### Fixed + +- `DELETE /v1/agents/:name` now tombstones an agent instead of issuing a bare + `DELETE`. Four foreign keys reference `agents.id` with no `ON DELETE` action + (`messages.agent_id`, `channels.created_by`, `files.uploaded_by`, + `webhooks.created_by`), so removing an agent that had ever spoken failed and + surfaced the raw SQL to the caller. The name is freed, the credential is + rotated, and message attribution is preserved. Completes the set alongside + the local (#309) and node-completed (#330) release paths. +- Releasing an agent now clears its `channel_members` and `dm_participants` + rows. Those cascade on `DELETE`, and the tombstone's `UPDATE` does not fire + the cascade, so a released agent could remain a delivery target. +- A late `touchLastSeen` can no longer revive a released tombstone back to + `active`, and id-scoped `updateAgentById` writes no longer resolve to a + released row. + + ## [8.0.1] - 2026-08-14 ### Fixed diff --git a/packages/engine/src/__tests__/conformance/deleteAgentRoute.test.ts b/packages/engine/src/__tests__/conformance/deleteAgentRoute.test.ts index 6350bbb0..86240e9b 100644 --- a/packages/engine/src/__tests__/conformance/deleteAgentRoute.test.ts +++ b/packages/engine/src/__tests__/conformance/deleteAgentRoute.test.ts @@ -1,7 +1,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { and, eq } from 'drizzle-orm'; import { createWorkspace, makeNodeStack, registerAgent, type TestStack } from './harness.js'; -import { agents, messages } from '../../db/schema.js'; +import { agents, channelMembers, messages } from '../../db/schema.js'; /** * `DELETE /v1/agents/:name` -> `agentEngine.deleteAgent` was the last release @@ -71,6 +71,16 @@ describe('DELETE /v1/agents/:name preserves attributed history', () => { status: 'released', }); + // The tombstone must not stay subscribed: `channel_members` cascades on + // DELETE, and an UPDATE does not fire that cascade, so a released agent + // would otherwise remain a delivery target. + expect( + await stack.runtime.deps.db + .select() + .from(channelMembers) + .where(eq(channelMembers.agentId, target.agentId)), + ).toHaveLength(0); + // Attribution intact; the old credential is dead; the name is reusable. expect( await stack.runtime.deps.db.select().from(messages).where(eq(messages.agentId, target.agentId)), diff --git a/packages/engine/src/engine/action.ts b/packages/engine/src/engine/action.ts index 2bbe7897..ad08fe76 100644 --- a/packages/engine/src/engine/action.ts +++ b/packages/engine/src/engine/action.ts @@ -1,6 +1,6 @@ import { and, asc, eq, inArray, isNotNull, isNull, lte, or, sql } from 'drizzle-orm'; import type { getDb } from '../db/index.js'; -import { actions, actionInvocations, agents, agentNodeBindings, nodes } from '../db/schema.js'; +import { actions, actionInvocations, agents, agentNodeBindings, channelMembers, dmParticipants, nodes } from '../db/schema.js'; import { generateId } from './snowflake.js'; import { RELEASED_AGENT_STATUS, releasedAgentName } from './agent.js'; import { randomHex, sha256Hex } from '../lib/crypto.js'; @@ -1383,6 +1383,11 @@ async function applyReleaseCompletionEffect( lastSeen: new Date(), }) .where(and(eq(agents.workspaceId, workspaceId), eq(agents.id, agent.id))); + // `channel_members` and `dm_participants` reference `agents.id` ON DELETE + // CASCADE; an UPDATE does not fire that cascade, so drop the memberships + // explicitly or the released agent stays a delivery target. + await db.delete(channelMembers).where(eq(channelMembers.agentId, agent.id)); + await db.delete(dmParticipants).where(eq(dmParticipants.agentId, agent.id)); const implicitNodeId = `node_direct_${agent.id}`; await db.delete(nodes).where(and(eq(nodes.workspaceId, workspaceId), eq(nodes.id, implicitNodeId))); } else { diff --git a/packages/engine/src/engine/agent.ts b/packages/engine/src/engine/agent.ts index 1f46cf9f..29b2feb6 100644 --- a/packages/engine/src/engine/agent.ts +++ b/packages/engine/src/engine/agent.ts @@ -1,6 +1,6 @@ import { eq, and, gt, lt, ne, inArray, sql } from 'drizzle-orm'; import type { getDb } from '../db/index.js'; -import { agents, agentNodeBindings, channels, channelMembers, actions, deliveries, nodes } from '../db/schema.js'; +import { agents, agentNodeBindings, channels, channelMembers, dmParticipants, actions, deliveries, nodes } from '../db/schema.js'; import { randomHex, sha256Hex } from '../lib/crypto.js'; import { generateId } from './snowflake.js'; import { codedError } from '../lib/httpError.js'; @@ -559,6 +559,14 @@ export async function deleteAgent(db: Db, workspaceId: string, name: string) { lastSeen: new Date(), }) .where(eq(agents.id, agent.id)); + // `channel_members` and `dm_participants` reference `agents.id` ON DELETE + // CASCADE, so the bare DELETE used to clear them implicitly. An UPDATE does + // not fire that cascade, which would leave a released agent subscribed and + // therefore still selected as a delivery target (`delivery.ts` fans out over + // channel members). Drop the memberships explicitly to keep the tombstone + // equivalent to the delete it replaces. + await db.delete(channelMembers).where(eq(channelMembers.agentId, agent.id)); + await db.delete(dmParticipants).where(eq(dmParticipants.agentId, agent.id)); await db.delete(nodes).where(eq(nodes.id, directNodeIdForAgent(agent.id))); return true; } @@ -567,7 +575,10 @@ export async function touchLastSeen(db: Db, agentId: string): Promise { await db .update(agents) .set({ lastSeen: new Date(), status: 'active' }) - .where(eq(agents.id, agentId)); + // Auth schedules this without awaiting it, so a heartbeat racing a release + // can land after the tombstone and flip the row back to `active` — reviving + // a released agent as a live roster member. A tombstone is terminal. + .where(and(eq(agents.id, agentId), ne(agents.status, RELEASED_AGENT_STATUS))); } export async function sweepStaleAgents(db: Db, workspaceId?: string): Promise {