eviction logic and migrations and trajectories - #46
khaliqgant wants to merge 1 commit into
Conversation
| for (const { id } of candidates) { | ||
| await purgeWorkspaceR2(env.FILES_BUCKET, id); | ||
| await db.delete(workspaces).where(eq(workspaces.id, id)); | ||
| evicted.push(id); | ||
| } |
There was a problem hiding this comment.
🔴 Race condition in evictSuspendedWorkspaces: revived workspace can be hard-deleted
The evictSuspendedWorkspaces function selects candidate workspaces in one query, then iterates over them to purge R2 and delete the DB record. The DELETE at eviction.ts:59 only filters by eq(workspaces.id, id) and does not re-check that suspendedAt is still set.
Root Cause: TOCTOU race between SELECT and DELETE
Between the initial SELECT (eviction.ts:50-54) and the per-workspace DELETE (eviction.ts:59), a user could authenticate, triggering touchWorkspaceActivity (eviction.ts:96-102) which clears suspendedAt and updates lastActivityAt — effectively "reviving" the workspace.
However, the eviction loop doesn't re-check and proceeds to:
purgeWorkspaceR2— permanently deletes all R2 files for the workspacedb.delete(workspaces).where(eq(workspaces.id, id))— cascade-deletes the workspace and all agents, channels, messages, etc.
The time window is the duration of the R2 purge for each workspace (potentially many seconds for workspaces with numerous files). During this window, if a returning user authenticates, the auth middleware (auth.ts:51-52, auth.ts:117, auth.ts:171) calls touchWorkspaceActivity which revives the workspace, but the eviction loop still deletes it.
Impact: Permanent data loss for a workspace that a user just revived. Although the probability is low (cron runs once daily at 2 AM against workspaces inactive for 60+ days), the consequence is severe — all workspace data is irrecoverably destroyed.
Prompt for agents
In packages/server/src/engine/eviction.ts, the evictSuspendedWorkspaces function (lines 43-64) has a TOCTOU race condition. The fix should ensure the workspace is still suspended before deletion. The safest approach is to reverse the order: attempt the DELETE first with a WHERE condition that checks suspendedAt IS NOT NULL, and only purge R2 if the delete actually removed a row. This avoids deleting R2 files for a workspace that was revived between the SELECT and the processing loop.
Specifically, in the for loop at lines 57-61, change:
await purgeWorkspaceR2(env.FILES_BUCKET, id);
await db.delete(workspaces).where(eq(workspaces.id, id));
evicted.push(id);
To something like:
import { isNotNull } from 'drizzle-orm'; (add to imports at line 1)
const deleted = await db.delete(workspaces)
.where(and(eq(workspaces.id, id), isNotNull(workspaces.suspendedAt)))
.returning({ id: workspaces.id });
if (deleted.length > 0) {
await purgeWorkspaceR2(env.FILES_BUCKET, id);
evicted.push(id);
}
This way, the cascade delete only proceeds if suspendedAt is still set (not revived), and R2 cleanup happens after the atomic DB delete succeeds. Note this means R2 orphans could remain if purgeWorkspaceR2 fails, but that is much safer than deleting a revived workspace's files.
Was this helpful? React with 👍 or 👎 to provide feedback.
Uh oh!
There was an error while loading. Please reload this page.