Skip to content

fix(workspace): deletion must take the agents' data with it - #187

Open
WhichPaths wants to merge 1 commit into
yetone:mainfrom
WhichPaths:fix/workspace-deletion-orphans
Open

fix(workspace): deletion must take the agents' data with it#187
WhichPaths wants to merge 1 commit into
yetone:mainfrom
WhichPaths:fix/workspace-deletion-orphans

Conversation

@WhichPaths

Copy link
Copy Markdown
Collaborator

Deleting a workspace leaves the agents' memory and their written notes about people behind.

The purge sweeps its soft-scoped tables with

for (const table of softScopedTables) {
  await client.query(`DELETE FROM ${table} WHERE company_id = $1`, [companyId])
}

which is exactly as good as the company_id the writers put there. Two of them do not put one.

agent_workspace — the agent's own filesystem endpoint writes no tenant:

// server/src/agents/runtime/fs-endpoints.ts:130
`INSERT INTO agent_workspace (agent_id, path, body, meta, updated_at)
   VALUES ($1, $2, $3, $4::jsonb, NOW())
 ON CONFLICT (agent_id, path) DO UPDATE
   SET body = EXCLUDED.body, meta = …, updated_at = NOW()`

Every other writer of that table does — skills.ts:262, cli.ts:2713/4116/4122/4363, router.ts:2962, and the baseline backfill at migrate.ts:647 — and they refresh it on conflict too. This is the one path that doesn't, and it is the path an agent uses to write its own memory.

agent_climate — the column is TEXT NOT NULL DEFAULT 'personal' (migrate.ts:822) and neither of its two INSERT sites (climate.ts:57, cli.ts:4250) names it. So every climate row in every workspace is labelled 'personal', and DELETE FROM agent_climate WHERE company_id = 'co-acme' can never match one.

Demonstrated

Postgres 16, one agent in co-tenant, one row written the FUSE way and one written the way a peer writer does:

     path       | company_id
----------------+------------
 memory/note.md | <NULL>
 skills/x.md    | co-tenant

 deleted_by_company_scope | 1
 survives_deletion        | 1

Same agent, same table, same deletion — one row goes, one stays. The one that stays is the memory file.

The fix, in two halves

Stop producing them. fs-endpoints fills the tenant from the agent's participants row with a scalar subquery, so it stays a single round trip and still inserts when the lookup finds nothing — behaviour is unchanged where it was already correct. The ON CONFLICT arm uses COALESCE(EXCLUDED.company_id, agent_workspace.company_id), so a row already written without one heals on the agent's next write and a known tenant is never overwritten with NULL.

Reach the ones already written. The purge also sweeps the agent-owned subset by agent_id:

if (agentIds.length > 0) {
  for (const table of agentOwnedTables) {
    await client.query(`DELETE FROM ${table} WHERE agent_id = ANY($1::text[])`, [agentIds])
  }
}

agentIds is already built earlier in the same transaction for board_mention_reads. Deleting by owner needs no backfill migration to reach rows written before this, which matters because those rows exist in every deployment today. computers is deliberately not in the subset — it is keyed by the machine, not an agent. I confirmed against the live schema that all eight tables in the subset really do have an agent_id column.

Why the existing test didn't catch it

workspace-management.test.ts's purge test seeds every row with a correct company_id:

`INSERT INTO agent_runs (id, agent_id, company_id) VALUES ('run-managed', 'agent-managed', 'co-managed')`

That is the one shape the broken writers never produce, so the test passes on the broken code. The new tests seed the way the real writers do.

Verification

  • Five integration tests, and each half goes red on its own:
    • reverting only the purge change → tests 1 and 2 fail (memory and climate outlive the workspace)
    • reverting only the fs-endpoints change → test 5 fails (the row is written without a workspace)
  • Test 5 drives the real PUT /runtime/fs/write with a minted agent token rather than issuing the SQL itself, so it pins the writer, not my restatement of it.
  • Tests 3 and 4 pin the other direction: a row the company_id sweep already reached is still removed, and another workspace's agent keeps its data — deleting by owner must not reach past the workspace being deleted.
  • The existing workspace-management.test.ts stays green, 12/12.
  • Full unit suite against a live Postgres: 1095 pass, 0 fail. tsc --noEmit, biome lint ., all three source guards clean.

Related, not fixed here

agent_climate's writers should probably name company_id rather than relying on a 'personal' default that is wrong for every non-personal workspace — the column and its index (idx_agent_climate_company) exist as if it were meaningful, and AGENT_ID_CASCADE_TABLES scopes on it. But climate reads are per-agent and global by design (ADR 0004 says so explicitly), so changing what the column holds is a separate decision with its own blast radius. This PR makes deletion correct without taking that on.

@yetone

yetone commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Thanks — the bug is real and the integration tests here are the best coverage anyone has written for the purge path. I'm not merging this one as-is, though, because it now collides with #207, which I merged a few minutes ago.

#207 fixes the same two touch points:

  • server/src/agents/runtime/fs-endpoints.ts — the INSERT INTO agent_workspace now carries company_id. It takes it from c.companyId (the authenticated JWT claim, which the endpoints already 403 on when empty) rather than from a (SELECT company_id FROM participants WHERE id = $1) subquery. Same result, one fewer round trip, and it can't silently write NULL if the participant row is missing.
  • server/src/api/router.tsDELETE /api/companies/:id gained the agent-owned table purge.

#207 also covered ground this PR doesn't: agent_log, agent_tasks and agent_climate writes in server/src/agents/cli.ts, and bumpClimate in climate.ts, all of which were writing NULL tenants too.

What this PR still has that #207 does not, and what I'd like to keep:

  1. The wider table list. fix(runtime): record company_id on FUSE workspace and CLI agent state #207 purges agent_workspace, agent_memory, agent_log, agent_tasks, agent_climate. Yours also takes agent_events, agent_runs, agent_triages. Those three are the ones that carry per-run history and cost, so leaving them behind is the version of this bug that shows up on a billing report months later.
  2. server/src/__integration__/workspace-deletion-orphans.test.ts. Five tests against a real database, asserting no orphans survive. Nothing in fix(runtime): record company_id on FUSE workspace and CLI agent state #207 replaces that.

Could you rebase onto main and reduce this to those two things — drop the fs-endpoints.ts change (already landed) and keep the purge widened to the full eight tables plus the integration tests? That lands cleanly and keeps the part of this PR that #207 genuinely missed.

Rebased onto main and reduced to what yetone#207 did not cover, per review.

Dropped: the fs-endpoints.ts change. yetone#207 landed it, and its version is
better — the tenant comes from c.companyId (the JWT claim the endpoints
already 403 on when empty) rather than a subquery against participants,
so it is one round trip fewer and cannot write NULL when the participant
row is missing. The test that exercised that endpoint went with it;
yetone#207 asserts the same property in runtime-server.test.ts.

Kept, and the only behaviour change here: agent_events, agent_runs and
agent_triages join the by-owner sweep, taking it from five tables to
eight.

Worth being precise about why, because it is not the same reason as the
five. Those five were leaking: their writers were dropping company_id,
so the tenant sweep could not see the rows. These three have writers
that all pass a tenant today — I checked every createRun, recordEvent
and recordTriage call site — so nothing is leaking through them right
now. They are here so the sweep stops depending on writer discipline at
all, which is the property that failed for the other five. And they are
the tables carrying per-run history and cost, so a row that does slip
through resurfaces on a billing report rather than in a UI.

Six of the seven integration tests pass against main's five-table list;
the seventh is the one that pins the three, and it seeds them the way a
writer that forgot the tenant would. Two are guards in the other
direction: a correctly-tenanted row is still removed, and another
workspace keeps its own run history.
@WhichPaths
WhichPaths force-pushed the fix/workspace-deletion-orphans branch from d506929 to f638290 Compare September 5, 2026 14:38
@WhichPaths

Copy link
Copy Markdown
Collaborator Author

Rebased onto main and reduced to the two things you asked for.

Dropped: the fs-endpoints.ts change. #207's version is better than mine and I'd rather it stayed — taking the tenant from c.companyId avoids a round trip and, more importantly, cannot write NULL when the participant row is missing, which my (SELECT company_id FROM participants …) subquery silently could. I removed the test that drove PUT /runtime/fs/write along with it, since #207 asserts the same property in runtime-server.test.ts; keeping mine would just have been a second copy.

Kept: the widened sweep. agent_events, agent_runs, agent_triages join the by-owner purge, taking agentScopedTables from five to eight. Verified against the live schema that all eight really do carry an agent_id.

One correction to the framing, because I'd rather you merged this knowing it:

These three are not leaking today. The five in #207 were — their writers were dropping company_id, so the tenant sweep could not see the rows. I went through every createRun, recordEvent and recordTriage call site and they all pass a tenant, so the existing company_id = $1 delete already reaches these three. So this is not a second instance of the bug you fixed; it is making the sweep independent of writer discipline, which is precisely the property that failed for the other five. Your billing-report argument is what makes that worth doing rather than merely tidy — but "currently leaking" would have been the wrong reason and I didn't want to leave it implied.

That shows up honestly in the tests: six of the seven pass against main's five-table list. Only one fails, and it is the one that seeds those three the way a writer that forgot the tenant would:

not ok 5 - [integration] deletion takes the run history and its cost with it
# pass 6  # fail 1

Two of the seven are guards in the other direction — a correctly-tenanted row must still be removed (the tenant sweep is not being replaced), and another workspace must keep its own run history (deleting by owner must not reach past the workspace being deleted).

Checks: workspace-management.test.ts 12/12 and the unit suite 1160/0 with the reduced diff. runtime-server.test.ts has one failure here — /cli still sends text-only agent email in mock mode — but it fails the same way on clean main in my environment, so it looks like missing email config on my side rather than anything from this branch.

Final diff is 7 lines in router.ts plus the test file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants