Skip to content

Commit 2e889ff

Browse files
committed
fix(daemon): skip on-disk sweep for runs still live in the in-memory map (#6202 @mrcfps follow-up)
@mrcfps 7-30 non-blocking follow-up评审指出:purgeRunsForProject 先抓 runs.values() snapshot,再 await per-run cancel。在 await 窗口期间为本 项目新建的 run 不在 snapshot,不会被 tombstone;它的 state.json 仍然 指向该项目,后续的 removeProjectRunDirs 只看 state.projectId === target 就会把这个活 run 的目录删掉——活 run 还在跑,但 state.json 没 了,后续 persistState mkdirs 又把目录写回,正好重复我们刚消除的 #6117 orphan 场景。 修复: 1. apps/daemon/src/runtimes/runs.ts - purgeRunsForProject: 返回值改为 { tombstoned, protectedRunIds }, 让 caller 可以拿到后续 in-memory 仍然 live 的 run ids 而不必再查 一次 map。protectedRunIds 在 await cancel 之后重新扫一次 runs.values() 得到(persistsDisabled === false 的本项目 run), 捕获 await 窗口期间新建的 late arrival。 - 新增 isLiveRun(runId): 同步查 in-memory map,返回 run 存在且未被 tombstone。暴露给 sweep caller 用作 shouldSkip 谓词。 2. apps/daemon/src/projects.ts - removeProjectRunDirs: 新增第三个可选参数 { shouldSkip?: (runId) => boolean }。在 read state.json 之前先调 shouldSkip(runId), 返回 true 就跳过该目录,避免误删活 run 的 dir。注释里点明这是为 了保护 race 期间新建的 mid-flight run。 3. apps/daemon/src/routes/project/index.ts - DELETE handler 把 shouldSkip: (runId) => design.runs.isLiveRun(runId) 传给 removeProjectRunDirs。这样 purge 之后 sweep 之前的窗口里出现 的新 run,只要它还在 in-memory map 里就能被识别并跳过。 - 注释里点明race motivation + #6202 follow-up 来源。 4. apps/daemon/tests/delete-sweeps-orphaned-run-dirs.test.ts - 新增 regression:验证 removeProjectRunDirs 的 shouldSkip 行为: 先手动 purgeRunsForProject tombstone 掉一个 run,再造一个 late arrival run,再调 removeProjectRunDirs 传 production 的 shouldSkip = (id) => runs.isLiveRun(id) 谓词。断言: * orphan dirs (state.json projectId 匹配但 in-memory 没有该 run) 被正常清扫(2 个) * late arrival run 的目录完好无损 * isLiveRun 对 late arrival true,对空字符串/orphan/已 tombstone 的 run 都 false * 第二次 purgeRunsForProject 返回 { tombstoned: [lateRun.id], protectedRunIds: [] } * 空/undefined projectId 返回 { tombstoned: [], protectedRunIds: [] } 而非旧的 bare array - 这条 regression锁住,mrcfps 后续 refactor 不会再让 race 期间的 新建 run 被静默清掉。 CHANGELOG:更新 #6117 fixed 条目,把 #6202 follow-up 一并交代清楚。 验证: - pnpm vitest run tests/delete-sweeps-orphaned-run-dirs.test.ts → 5/5 pass - pnpm vitest run tests/cancel-owned-runs.test.ts tests/delete-cancels-active-runs.test.ts → 6/6 pass - NODE_OPTIONS=--max-old-space-size=1536 npx tsc -p tsconfig.json --noEmit → 0 - NODE_OPTIONS=--max-old-space-size=1536 npx tsc -p tsconfig.tests.json --noEmit → 0
1 parent bd54ec0 commit 2e889ff

5 files changed

Lines changed: 188 additions & 7 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
99

1010
### Fixed
1111

12-
- [fix] 删除项目时清理 `<RUNTIME_DATA_DIR>/runs/<runId>/` 下归属于该项目的孤立 run 目录,而非仅取消活跃 run。Run service 在 ~30 分钟 TTL 后从内存 map 移除已终止 run,但磁盘上的 `state.json`/`events.jsonl` 超出该窗口仍然残留;此前 delete project 会泄漏该项目所有已结束 run 的目录。新 helper `removeProjectRunDirs(runsDir, projectId)` 在 cancel + removeProjectDir 之后以 best-effort 方式扫描并清理。(#6117)
12+
- [fix] 删除项目时清理 `<RUNTIME_DATA_DIR>/runs/<runId>/` 下归属于该项目的孤立 run 目录,而非仅取消活跃 run。Run service 在 ~30 分钟 TTL 后从内存 map 移除已终止 run,但磁盘上的 `state.json`/`events.jsonl` 超出该窗口仍然残留;此前 delete project 会泄漏该项目所有已结束 run 的目录。新 helper `removeProjectRunDirs(runsDir, projectId)` 在 cancel + removeProjectDir 之后以 best-effort 方式扫描并清理。`purgeRunsForProject` 现在返回 `{ tombstoned, protectedRunIds }` 结构化结果,`removeProjectRunDirs` 接受可选的 `shouldSkip(runId)` 回调,DELETE handler 通过 `design.runs.isLiveRun(runId)` 跳过仍在内存 map 中的活跃 run,避免在 purge 之后、sweep 之前出现的窗口里被创建的新 run 被误删;修复 PR #6202 @mrcfps 评审指出的第二个 race condition。(#6117, #6202)
1313

1414
## [0.9.0] - 2026-05-29
1515

apps/daemon/src/projects.ts

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1358,11 +1358,23 @@ export async function removeProjectDir(projectsRoot, projectId) {
13581358
*
13591359
* @param {string} runsDir Absolute path to the runtime `runs` directory.
13601360
* @param {string} projectId The project id whose run directories to remove.
1361+
* @param {{ shouldSkip?: (runId: string) => boolean }} [opts] Optional
1362+
* per-entry guard. When `shouldSkip(runId)` returns true, the directory is
1363+
* left alone — used to protect runs that legitimately still own their
1364+
* on-disk directory at sweep time (e.g. a run created for this project
1365+
* during the cancel-await window of a project-delete — its state.json
1366+
* references the deleted project, but it's mid-flight and would otherwise
1367+
* be wiped out from under itself). See #6202 @mrcfps follow-up.
13611368
* @returns {Promise<number>} Number of run directories removed. Resolves to 0
13621369
* when `runsDir` does not exist (fresh install, no runs yet).
13631370
*/
1364-
export async function removeProjectRunDirs(runsDir, projectId) {
1371+
export async function removeProjectRunDirs(
1372+
runsDir: string,
1373+
projectId: string,
1374+
opts: { shouldSkip?: (runId: string) => boolean } = {},
1375+
): Promise<number> {
13651376
if (!isSafeId(projectId)) return 0;
1377+
const shouldSkip = opts.shouldSkip;
13661378
let entries: import('node:fs').Dirent[];
13671379
try {
13681380
entries = await readdir(runsDir, { withFileTypes: true });
@@ -1376,6 +1388,13 @@ export async function removeProjectRunDirs(runsDir, projectId) {
13761388
.filter((entry) => entry.isDirectory())
13771389
.map(async (entry) => {
13781390
const runDir = path.join(runsDir, entry.name);
1391+
const runId = entry.name;
1392+
// Consult the live-run guard before touching the disk so a
1393+
// mid-flight run created during a concurrent delete-project can't
1394+
// have its state.json wiped out from under itself. Should be a
1395+
// cheap synchronous check (in-memory map lookup); see
1396+
// `runService.isLiveRun` in runs.ts.
1397+
if (shouldSkip && shouldSkip(runId)) return;
13791398
try {
13801399
const stateRaw = await readFile(path.join(runDir, 'state.json'), 'utf8');
13811400
const state = JSON.parse(stateRaw);

apps/daemon/src/routes/project/index.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2279,6 +2279,13 @@ export function registerProjectRoutes(app: Express, ctx: RegisterProjectRoutesDe
22792279
// while it was still alive would leak its dir back onto disk the moment
22802280
// the analytics/langfuse completion resolved (#6117 race @mrcfps on #6202).
22812281
// Also cancels live runs (#5468 supersedes the bare cancelRunsOwnedBy).
2282+
// The returned `protectedRunIds` are runs that entered the in-memory
2283+
// registry during the cancellation await — their state.json still
2284+
// references this project but the runs are mid-flight and must be left
2285+
// alone. The `isLiveRun` guard re-checks the in-memory map at sweep
2286+
// time so even later concurrent creates (after `purgeRunsForProject`
2287+
// returns but before `removeProjectRunDirs` runs) are protected
2288+
// (#6202 @mrcfps non-blocking follow-up).
22822289
await design.runs.purgeRunsForProject(req.params.id).catch(() => {});
22832290
dbDeleteProject(db, req.params.id);
22842291
await removeProjectDir(PROJECTS_DIR, req.params.id).catch(() => {});
@@ -2287,7 +2294,11 @@ export function registerProjectRoutes(app: Express, ctx: RegisterProjectRoutesDe
22872294
// map; this catches runs whose TTL has already expired out of the map
22882295
// (their state.json lingers on disk until now). Best-effort — same
22892296
// posture as removeProjectDir above.
2290-
await removeProjectRunDirs(path.join(ctx.paths.RUNTIME_DATA_DIR, 'runs'), req.params.id).catch(() => {});
2297+
await removeProjectRunDirs(
2298+
path.join(ctx.paths.RUNTIME_DATA_DIR, 'runs'),
2299+
req.params.id,
2300+
{ shouldSkip: (runId) => design.runs.isLiveRun(runId) },
2301+
).catch(() => {});
22912302
/** @type {import('@open-design/contracts').OkResponse} */
22922303
const body = { ok: true };
22932304
res.json(body);

apps/daemon/src/runtimes/runs.ts

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1076,14 +1076,23 @@ export function createChatRunService({
10761076
* child is reaped) — terminal runs are left alone status-wise, only
10771077
* tombstoned on disk.
10781078
*
1079-
* Returns the list of run ids that were tombstoned, so the caller can
1080-
* additionally remove their on-disk run directories after this resolves.
1079+
* Returns `{ tombstoned, protectedRunIds }`:
1080+
* - `tombstoned` lists the run ids that were tombstoned (already disabled
1081+
* and cancel-awaited) so the caller can additionally remove their
1082+
* on-disk run directories after this resolves.
1083+
* - `protectedRunIds` lists runs that *entered the in-memory registry
1084+
* after the initial snapshot* — i.e. were created for this project
1085+
* during the await-cancellation window. These are live runs the sweep
1086+
* must NOT delete (their state.json still references the deleted
1087+
* project, but the run is mid-flight and owns that dir legitimately).
1088+
* See #6202 @mrcfps non-blocking follow-up.
1089+
*
10811090
* Cancellation failures are swallowed per-run so a single bad run never
10821091
* blocks the project delete the user asked for — same posture as
10831092
* `cancelRunsOwnedBy` for #5468.
10841093
*/
10851094
const purgeRunsForProject = async (projectId) => {
1086-
if (typeof projectId !== 'string' || !projectId) return [];
1095+
if (typeof projectId !== 'string' || !projectId) return { tombstoned: [], protectedRunIds: [] };
10871096
const owned = Array.from(runs.values()).filter((run) => run.projectId === projectId);
10881097
const tombstoned = [];
10891098
// Order matters (#6117 race): disablePersist MUST run before cancel,
@@ -1101,7 +1110,33 @@ export function createChatRunService({
11011110
}
11021111
}),
11031112
);
1104-
return tombstoned;
1113+
// Re-snapshot after the await window. A run created for this project
1114+
// during the cancellation await is *not* in `owned` and therefore not
1115+
// tombstoned above; its state.json still references the soon-to-be
1116+
// deleted project, so the on-disk sweep would otherwise remove it
1117+
// mid-flight. Track it as protected so the caller can skip its dir.
1118+
// (Live runs whose project is gone will eventually go terminal + TTL out
1119+
// of the in-memory map; the next delete-project call's sweep will catch
1120+
// their then-orphaned state.json. That's the same backstop posture
1121+
// already used for runs that TTL-expire out before delete-project.)
1122+
const protectedRunIds = Array.from(runs.values())
1123+
.filter((run) => run.projectId === projectId && !run.persistsDisabled)
1124+
.map((run) => run.id);
1125+
return { tombstoned, protectedRunIds };
1126+
};
1127+
1128+
/**
1129+
* Live-run guard for the on-disk sweep. Returns true if `runId` is currently
1130+
* registered in the in-memory run map AND has not been tombstoned (i.e. its
1131+
* `persistsDisabled` flag is still false). Callers that walk `runsDir` to
1132+
* remove orphaned run directories use this to skip directories owned by runs
1133+
* that legitimately still own them — e.g. a run created for the project
1134+
* during the cancel-await window of a project-delete (#6202 @mrcfps follow-up).
1135+
*/
1136+
const isLiveRun = (runId) => {
1137+
if (typeof runId !== 'string' || !runId) return false;
1138+
const run = runs.get(runId);
1139+
return !!run && !run.persistsDisabled;
11051140
};
11061141

11071142
// Drop a run from the in-memory registry without emitting any terminal
@@ -1175,6 +1210,7 @@ export function createChatRunService({
11751210
fail,
11761211
drop,
11771212
purgeRunsForProject,
1213+
isLiveRun,
11781214
signalChild: killChild,
11791215
reapProcessGroup,
11801216
signalProcessGroup,

apps/daemon/tests/delete-sweeps-orphaned-run-dirs.test.ts

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -277,4 +277,119 @@ describe('DELETE project sweeps orphaned run dirs (#6117)', () => {
277277

278278
expect(existsSync(runDir)).toBe(false);
279279
});
280+
281+
it('does not sweep a run dir for a run still live in the in-memory map (#6202 @mrcfps follow-up)', async () => {
282+
// Reproduces the second race @mrcfps flagged on PR #6202 (non-blocking
283+
// follow-up): purgeRunsForProject snapshots the in-memory runs once, then
284+
// awaits per-run cancellations. A run that is created for the same project
285+
// during that await window is NOT in the snapshot, so it is not tombstoned;
286+
// its state.json still references the soon-to-be-deleted project. If the
287+
// on-disk sweep (removeProjectRunDirs) only filters by `state.projectId`,
288+
// it wipes the new run's dir out from under itself — the run keeps running
289+
// but its state file is gone, and any later persistState mkdirs the dir
290+
// back, recreating the orphan we just swept.
291+
//
292+
// After the fix:
293+
// - removeProjectRunDirs takes a `shouldSkip(runId)` callback that
294+
// consults the in-memory run map.
295+
// - The DELETE handler passes `(runId) => design.runs.isLiveRun(runId)`.
296+
// - A run that is currently in the in-memory map AND not tombstoned is
297+
// left alone even if its state.json projectId matches the sweep target.
298+
//
299+
// This test exercises the sweep with a controlled shouldSkip that mirrors
300+
// the production wiring (isLiveRun) and asserts the live run's dir survives
301+
// the sweep even though its state.json projectId matches the sweep target.
302+
const runsDir = path.join(tempDir, 'runs');
303+
mkdirSync(runsDir, { recursive: true });
304+
const runs = createChatRunService({
305+
createSseResponse: () => ({ send: vi.fn(() => true), end: vi.fn(), cleanup: vi.fn() }),
306+
createSseErrorPayload: (code: string, message: string) => ({ error: { code, message } }),
307+
shutdownGraceMs: 10,
308+
ttlMs: 60_000,
309+
runsLogDir: runsDir,
310+
});
311+
312+
// Seed one orphaned run dir that DOES belong to p1 and is NOT in the
313+
// in-memory map (simulates a TTL-expired prior run). The sweep should
314+
// catch this one.
315+
const orphanRunId = 'orphan-' + Math.random().toString(36).slice(2);
316+
const orphanDir = path.join(runsDir, orphanRunId);
317+
mkdirSync(orphanDir, { recursive: true });
318+
writeFileSync(path.join(orphanDir, 'state.json'), JSON.stringify({ projectId: 'p1' }));
319+
320+
// And create a live run for p1 that IS in the in-memory map. The sweep
321+
// must skip its directory because the production shouldSkip guard
322+
// (design.runs.isLiveRun) returns true for it.
323+
const liveRun = runs.create({ projectId: 'p1' });
324+
const liveRunDir = path.join(runsDir, liveRun.id);
325+
expect(existsSync(path.join(liveRunDir, 'state.json'))).toBe(true);
326+
327+
// Mount and DELETE the project. The DELETE handler will:
328+
// 1. purgeRunsForProject('p1') — tombstones liveRun (it is in the
329+
// snapshot, because we created it before the request). liveRun is
330+
// now persistsDisabled=true and its dir is removed by disablePersist.
331+
// 2. removeProjectRunDirs with shouldSkip = (id) => isLiveRun(id).
332+
// liveRun is no longer "live" (persistsDisabled=true), so its dir
333+
// isn't expected to survive — but we want to verify the production
334+
// path end-to-end against a "truly live" run.
335+
//
336+
// To exercise the race scenario end-to-end (a run created DURING the
337+
// cancel await window, which is NOT in the snapshot), we mount the app
338+
// but hold the DELETE back: we pre-await a manual `purgeRunsForProject`
339+
// that tombstones liveRun, then create a second live run that is NOT
340+
// tombstoned, then invoke the on-disk sweep directly with the production
341+
// shouldSkip guard.
342+
//
343+
// Step 1: tombstone the first liveRun via purgeRunsForProject.
344+
const firstPurge = await runs.purgeRunsForProject('p1');
345+
expect(firstPurge.tombstoned).toEqual(expect.arrayContaining([liveRun.id]));
346+
expect(existsSync(liveRunDir)).toBe(false);
347+
expect(runs.isLiveRun(liveRun.id)).toBe(false);
348+
349+
// Step 2: create a new run for p1 — this one is "mid-flight" relative to
350+
// the original DELETE, i.e. not in any snapshot yet. Its state.json
351+
// projectId matches p1.
352+
const lateRun = runs.create({ projectId: 'p1' });
353+
const lateRunDir = path.join(runsDir, lateRun.id);
354+
expect(existsSync(path.join(lateRunDir, 'state.json'))).toBe(true);
355+
expect(runs.isLiveRun(lateRun.id)).toBe(true);
356+
357+
// Also re-seed an orphan dir with state.json projectId=p1 (the sweep's
358+
// primary target).
359+
const orphan2Dir = path.join(runsDir, 'orphan2-' + Math.random().toString(36).slice(2));
360+
mkdirSync(orphan2Dir, { recursive: true });
361+
writeFileSync(path.join(orphan2Dir, 'state.json'), JSON.stringify({ projectId: 'p1' }));
362+
363+
// Step 3: invoke the on-disk sweep with the production shouldSkip guard,
364+
// exactly as the DELETE handler does.
365+
const { removeProjectRunDirs } = await import('../src/projects.js');
366+
const removed = await removeProjectRunDirs(runsDir, 'p1', {
367+
shouldSkip: (runId) => runs.isLiveRun(runId),
368+
});
369+
// The orphan dirs (no in-memory run) are swept.
370+
expect(removed).toBe(2);
371+
expect(existsSync(orphanDir)).toBe(false);
372+
expect(existsSync(orphan2Dir)).toBe(false);
373+
// The late run's dir is untouched — the shouldSkip guard caught it.
374+
expect(existsSync(lateRunDir)).toBe(true);
375+
expect(existsSync(path.join(lateRunDir, 'state.json'))).toBe(true);
376+
377+
// The new helper is exposed for callers that want to query the live-run
378+
// state directly (e.g. future sweep callers).
379+
expect(runs.isLiveRun(lateRun.id)).toBe(true);
380+
expect(runs.isLiveRun('orphanRunId-not-in-map')).toBe(false);
381+
expect(runs.isLiveRun('')).toBe(false);
382+
383+
// And purgeRunsForProject now returns a structured result so callers can
384+
// destructure safely instead of treating the return as a bare array.
385+
const result = await runs.purgeRunsForProject('p1');
386+
expect(result).toEqual({ tombstoned: expect.arrayContaining([lateRun.id]), protectedRunIds: [] });
387+
expect(existsSync(lateRunDir)).toBe(false);
388+
expect(runs.isLiveRun(lateRun.id)).toBe(false);
389+
390+
// Empty/invalid project id is a no-op and returns the empty structured
391+
// shape (not a bare array, so callers can destructure safely).
392+
await expect(runs.purgeRunsForProject('')).resolves.toEqual({ tombstoned: [], protectedRunIds: [] });
393+
await expect(runs.purgeRunsForProject(undefined as unknown as string)).resolves.toEqual({ tombstoned: [], protectedRunIds: [] });
394+
});
280395
});

0 commit comments

Comments
 (0)