Skip to content

fix(daemon): reclaim run directories when a project is deleted - #6118

Open
kobbyDaUXer wants to merge 1 commit into
nexu-io:mainfrom
kobbyDaUXer:fix/orphaned-run-dirs
Open

fix(daemon): reclaim run directories when a project is deleted#6118
kobbyDaUXer wants to merge 1 commit into
nexu-io:mainfrom
kobbyDaUXer:fix/orphaned-run-dirs

Conversation

@kobbyDaUXer

Copy link
Copy Markdown

Fixes #6117

Why

My use case: I created a throwaway project to smoke-test an agent adapter, deleted it afterwards, and noticed the run directory was still on disk.

The pain: run event logs live at <dataDir>/runs/<runId>/{events.jsonl,state.json} — keyed by run id, not nested under the project — so removeProjectDir never reaches them. Deleting a project stranded every run it had ever produced, permanently: with the project row gone there is nothing left that can list or reclaim them. It is unbounded, and it accumulates fastest for exactly the people who use throwaway projects to try things out.

There is a privacy dimension too. events.jsonl contains the prompts and the agent's output. Someone deleting a project reasonably expects that content gone; today it stays.

What users will see

Deleting a project now also removes that project's run logs. Nothing new in the UI.

Implementation notes

Two things here are non-obvious enough to be worth calling out, because the naive version of this fix is wrong:

1. Ownership comes from state.json, not from runs.list(). The obvious approach is to reuse the existing cancelRunsOwnedBy path, which already enumerates runs by projectId. That does not work for this: runs is an in-memory Map and terminal runs are evicted from it (runtimes/runs.ts:229 and :674), so by the time a user deletes a project its finished runs — the ones actually left on disk — are long gone from that view. runs.list({ projectId }) would return only whatever happened to be in flight, i.e. approximately nothing, and the leak would look fixed while still leaking. Each run's state.json records projectId and survives eviction, which makes it the only durable attribution for a historical run.

2. The sweep is deliberately conservative. A directory is removed only when its state.json parses and names this project. Missing, unparseable, or projectId-less state files are left alone — stranding a few KB is strictly better than deleting a run belonging to a project the user is keeping. Per-entry failures are swallowed so one undeletable run cannot block the delete the user actually asked for, matching the existing .catch(() => {}) treatment of removeProjectDir in the same handler.

The runs root is derived from ctx.paths.RUNTIME_DATA_DIR per the daemon data-directory contract in AGENTS.md, not recomputed.

Scope

Deletes on project-delete only. Not included: a startup sweep for directories orphaned by earlier versions. That would be the natural companion, but it means deleting user data on upgrade based on a heuristic, and I would rather a maintainer decide that separately than smuggle it into a bug fix. Anyone upgrading keeps their existing orphans until they clear them by hand.

I also did not add a retention window. #6117 raised the question of whether run diagnostics should survive a project deletion briefly; immediate deletion is what the privacy argument points to, and it matches what the user asked for. Happy to change this if you would rather have a grace period.

Surface area

  • UI
  • Keyboard shortcut
  • CLI / env var
  • API / contract
  • Extension point
  • i18n keys
  • New top-level dependency
  • Default behavior changeDELETE /api/projects/:id now also removes the project's run directories. Previously they were retained indefinitely (unintentionally).
  • None

Bug fix verification

  • Test path: apps/daemon/tests/routes/remove-owned-run-dirs.test.ts
  • Red on main? The helper does not exist on main, so the spec cannot compile there — the pre-fix behaviour is instead demonstrated by the reproduction in Deleting a project leaves its run directories orphaned under .od/runs/ #6117 (create project → run → delete project → ls .od/runs/<runId> still lists events.jsonl and state.json).

Five cases covered: only-owned directories are removed; unattributable directories (no state file, corrupt JSON, no projectId) survive; loose files in the runs root are ignored; a missing runs root is a no-op; empty arguments are a no-op.

Validation

  • pnpm guard — pass
  • pnpm --filter @open-design/daemon typecheck — pass (exit 0, includes tsconfig.tests.json)
  • pnpm exec vitest run -c vitest.config.ts tests/routes/ tests/project-routes.test.ts — 11 files, 136 passed, 0 failed
  • New spec in isolation — 5 passed

Note for anyone running the full apps/daemon suite locally: three tests fail on main in my environment (langfuse-trace.test.ts ×2, plugins-headless-run.test.ts ×1). They look environment-sensitive rather than real, and they are unrelated to this change — flagging so the numbers are not a surprise.

Run event logs live at <dataDir>/runs/<runId>/{events.jsonl,state.json},
keyed by run id rather than nested under the project, so
removeProjectDir never reached them. Deleting a project left every run
it ever produced on disk permanently — with no project row left to list
them from, nothing could ever reclaim them. events.jsonl holds the
prompts and the agent's output, which a user deleting a project
reasonably expects to be gone.

Ownership is read from each run's own state.json rather than from
runs.list(): the run service is backed by an in-memory Map that evicts a
run the moment it reaches a terminal status, so a project's finished
runs are already absent from that view by the time it is deleted.
state.json records projectId and survives, making it the only durable
attribution for a historical run.

The sweep is deliberately conservative — a directory is removed only
when its state.json parses AND names this project. Missing, malformed,
or project-less state files are left alone, because stranding a few KB
beats deleting a run belonging to a project the user is keeping.
Per-entry failures are swallowed so one undeletable run cannot block the
delete the user asked for.

Refs nexu-io#6117
@lefarcen
lefarcen requested a review from nettee July 27, 2026 12:08
@lefarcen lefarcen added size/M PR changes 100-300 lines risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps type/bugfix Bug fix labels Jul 27, 2026
@lefarcen lefarcen added the needs-validation Runtime change detected; needs human or /explore agent validation. label Jul 27, 2026
@lefarcen

Copy link
Copy Markdown
Contributor

🧪 Queued for QA validation — this PR changes project-delete behavior in the daemon, so we want a manual QA pass before it merges. Nothing needed from you right now; we’ll update here once that validation is done. Thanks for the contribution! 🙏

@nettee nettee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kobbyDaUXer I reviewed the changed ranges in the project delete path and the new daemon helper. The ownership check against durable state.json, the conservative skip behavior for unattributable entries, and the route wiring against RUNTIME_DATA_DIR/runs all look consistent with the existing run lifecycle and restart-reconciliation code. I also checked the surrounding delete/cancel flow to make sure active runs are canceled before the sweep and that unrelated run directories are not touched. Nice fix on a real privacy and disk-leak issue.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

@lefarcen

Copy link
Copy Markdown
Contributor

Heads-up: #6202 is also open against this same cleanup path. Both PRs address #6117 and touch apps/daemon/src/routes/project/index.ts, with the same general approach of reading each run's state.json to decide which run directories belong to the deleted project. Sharing the overlap so review effort doesn’t get split accidentally.

@xxiaoxiong

Copy link
Copy Markdown
Contributor

Hi @kobbyDaUXer — I'm the author of #6202, the other PR touching this same #6117 path. I want to flag the gap I closed there so you (and reviewers) can decide whether to fold it into your PR or stick with the simpler scope here.

Your #6118 does the disk sweep correctly — durable state.json ownership, conservative skips, RUNTIME_DATA_DIR/runs per the daemon data contract. That part is identical in spirit to mine. The thing #6202 additionally closes is the lifecycle race that the Looper reviewer raised as a blocking inline note on #6202 (apps/daemon/src/routes/project/index.ts L2291):

terminal telemetry can persist the run again after this sweep. In routes/runs.ts, the runs.wait(run) callback awaits telemetry and later calls markAnalyticsCompleted; the finalized-message reporter similarly calls markLangfuseCompleted after an async report. Both methods call persistState, whose atomic writer recreates the parent directory.

So a project deleted while a run is still in its late-async telemetry-flush window can have its <runsDir> recreated after your sweep completes. I proved this with a regression case where the terminal analytics capture is held until after cancelRunsOwnedBy resolves and the project run sweeper has run. #6118 as currently scoped does not have this race closed — deleting a project while a run is active can still leave exactly the orphaned directory #6111 promises to eliminate.

What #6202 adds:

  • A "tombstone" flag on each run, owned by the run-service layer, that disables durable writes (both persistState and ensureLogStream's parent-dir mkdir) for every run attributed to a deleted project, before the sweep starts.
  • The tombstone survives terminal telemetry flushes and event-log reopens, so the orphaned directory cannot be recreated.
  • A regression test that holds the terminal analytics capture, deletes the project, runs the sweeper (directory is absent), releases the capture, and asserts the directory stays absent after the callback settles.

I'd suggest, whichever direction reviewers prefer:

Either way, no work wasted — both PRs end up at the same fix. Happy to hand the tombstone patch over if Option A is cleaner. /cc @lefarcen

@lefarcen

Copy link
Copy Markdown
Contributor

Thanks @xxiaoxiong — this is very useful context.

The disk sweep in #6118 and the lifecycle-race closure you described in #6202 are not equivalent fixes, so we should treat the tombstone + delayed-telemetry regression case as real review context rather than assuming the current head here already covers it. @kobbyDaUXer, if you want #6118 to stay the landing PR, folding that run-service-owned tombstone path and the late-callback regression into this branch would make the comparison straightforward; otherwise maintainers can choose #6202 as the fuller fix for #6117.

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

Labels

needs-validation Runtime change detected; needs human or /explore agent validation. risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps size/M PR changes 100-300 lines type/bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deleting a project leaves its run directories orphaned under .od/runs/

4 participants