Add isolation-workspace reaper (BLO-31222) - #1645
Conversation
`buildK8sRunIsolationDescriptor` mounts every workspace-isolated run under
`data/k8s-isolation/workspaces/<executionWorkspaceId>` and nothing has ever
removed one. Not a leak — an absence: there is no code path that tries and
fails, there is simply no reaper. The tree grew 0 -> 406.7 GiB between July
and 2026-09-02.
It is billed to CephFS (`paperclip-data`, a 2 TiB quota on the `ssd-fast`
tier), so that growth landed on the pool behind BLO-31222's capacity
incident, which reached ~19 hours from a cluster-wide write block. Deleting
the >30d cohort by hand reclaims real space but re-derives the same incident
in roughly five weeks.
Positive layout allowlist, not an age-only predicate. A directory is removed
only when its top level is exactly {home, session}. Auditing 448 real
directories found 447 matching and one carrying `wt-blo-19094`, a real git
worktree — an age-only `rm -rf` would have destroyed it. The allowlist costs
~0.2% of the reclaim and removes the whole class on future passes whose
composition nobody has inspected. Non-matching directories are skipped
unexamined and logged, so a skip count above the historical baseline of 1 is
visible rather than silent.
Opt-in, following `strandedRecoveryHandBack`: this deletes irreversibly, so
"deploy the code" and "delete a month of workspaces" must not be the same
act. `dryRun` makes the first enablement an observation.
Bounded and idempotent. Deletion here is MDS-metadata-bound — measured 145
files/s serial against a saturated MDS — so `maxDeletesPerTick` keeps a pass
from becoming a latency event for every other CephFS consumer. Another
reclaimer was observed on this tree mid-incident, so a directory that
vanishes between scan and unlink is counted, not raised.
Age is judged on the workspace directory's mtime. These directories are keyed
by execution-workspace id rather than run id, so they are reusable and can
outlive any single run; mtime is what separates "idle for a month" from
"between runs". Deleting a stale one costs a cold start, not source —
durable transcripts live separately under `data/run-logs/`.
Verification: 12 new tests pass; `tsc --noEmit` clean. Mutation-checked the
guard by neutering `isReapableLayout`, which fails exactly the
worktree-preservation and partial-layout tests.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
🔗 Paperclip issue: BLO-31222 |
1 similar comment
|
🔗 Paperclip issue: BLO-31222 |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b63039a
The allowlist design is the right instinct and the wt-blo-19094 near-miss is well captured. But the age half of the predicate does not measure what the module says it measures, and because this deletes irreversibly that is a blocker rather than a nit.
Critical Issues (1)
-
[gstack/review + native-codex]
server/src/services/isolation-workspace-reaper.ts:137— the age predicate reads the workspace root's ownmtime, which is not an idleness signal. A directory'smtimeadvances only when a direct child entry is added, removed, or renamed — never when nested content is written. ForisolationMode === "workspace"the only top-level children arehomeandsession(server/src/services/heartbeat.ts:6429,6434);cacheRootandtmpRootare deliberately routed elsewhere (heartbeat.ts:6437,6441). Those two are created once at materialization, so the root'smtimeis the workspace's creation time and never advances with use.Verified empirically on this filesystem: backdate a
{home, session}root to July, then writesession/.claude/projects/a.jsonl,home/.bashrc, and a nested tree, thenmkdir -pboth children again as a re-run would — the rootmtimestayed at the backdated value through every step, while only the immediate children moved.So
maxAgeDaysmeans "materialized more than N days ago", not "idle for N days" — which inverts the rationale stated atisolation-workspace-reaper.ts:47-50("mtimeis what distinguishes 'idle for a month' from 'between runs'") and the matching claim in the PR body. The population is not orphans-only either: the directory name isexecution_workspaces.id(heartbeat.ts:6340-6344joinspersistedExecutionWorkspaceId), so a row withstatus = 'active'andlastUsedAtof today, materialized 31 days ago, is eligible on its first pass — andstorage.home/storage.sessionare both"persistent"(heartbeat.ts:6458-6459), so what is destroyed is durable per-workspace state, with no exclusion for a run holding it open.Note the opt-in/dry-run mitigation does not catch this: a dry run reports these live workspaces as
eligiblewith nolastUsedAtin the output, so the operator inspection step cannot distinguish them from true orphans.- Gate on
execution_workspaces.lastUsedAt(plusstatus/closedAt) for every directory whose name resolves to a row — the column already exists, isnotNull, and is indexed (packages/db/src/schema/execution_workspaces.ts:35,59-62). Fall back to filesystem age only for ids with no row, which is the orphan cohort this PR is actually aimed at. A filesystem-only fix by takingmax(mtime)ofhome/sessionis not sufficient — those are subject to the same rule one level down.
- Gate on
Important Issues (2)
- [pr-review-toolkit/tests]
server/src/__tests__/isolation-workspace-reaper.test.ts:41—makeWorkspacecallsfs.utimes(dir, ...)after writing nested content, so every case presents "old root, arbitrary contents" and asserts that deleting it is correct. The production-dominant shape — root old, contents written today — is never constructed, so the suite encodes the Critical assumption above as though it were true and would stay green against a fix that got it wrong.- Add a case that writes under
home/andsession/while the root stays backdated, asserting the workspace is retained. That test should fail against the current implementation.
- Add a case that writes under
- [pr-review-toolkit/tests]
server/src/__tests__/isolation-workspace-reaper.test.ts:166—expect(res.vanished + res.deleted).toBeGreaterThanOrEqual(1)is satisfied byvanished === 0 && deleted === 1, so the test named "counts a concurrently-removed workspace as vanished, not failed" does not actually exercise the vanish path. Compounding it,fs.rm(dir, { recursive: true, force: true })at line 170 suppressesENOENTby contract, making theENOENT → vanishedbranch at line 173 unreachable;vanishedis reachable only via lines 139 and 148.- Assert
res.vanisheddirectly, and either dropforce: trueor remove the dead branch so the counter's reachable surface matches its documentation.
- Assert
Suggestions (3)
- [native-codex]
server/src/services/isolation-workspace-reaper.ts:128—cappedis evaluated againstresult.deletedbefore eligibility is known, so it reportstruewhen every remaining entry was too fresh or non-matching (no work left). IndryRun,result.deletednever increments, so the cap never engages andcappedis alwaysfalse— the dry run therefore does not represent the pass a real run would perform. Counting against considered/eligible entries would make both cases honest. - [pr-review-toolkit/code]
server/src/services/isolation-workspace-reaper.ts:126—result.scanned += 1runs before the capbreak, so the reportedscannedincludes one directory that was never examined. - [pr-review-toolkit/errors]
server/src/services/isolation-workspace-reaper.ts:207— the sweep-failure path logs throughdefaultLoggerrather thanoptions.logger, so a caller that injected a logger silently loses exactly the error it most wants to capture (the test'ssilentLoggerincluded).
Strengths
- The positive layout allowlist over an age-only
rm -rfis the correct call, andisolation-workspace-reaper.test.ts:81pins the realwt-blo-19094near-miss as a regression test rather than leaving it as prose. DEFAULT_ISOLATION_WORKSPACE_ROOT(isolation-workspace-reaper.ts:58) matches the producer atheartbeat.ts:6342exactly, and the ENOENT-on-root branch correctly treats a non-k8s-isolation deployment as a no-op instead of an error.- Registration mirrors
startStrandedBlockedIssueReconcilerprecisely (index.ts:1722againstindex.ts:1699) — samepaperclipNodeRole !== "api"guard, same minute→ms conversion, same lazy import. The worker tier isreplicas: 1(deploy/helm/paperclip/templates/statefulset.yaml:9), so the "worker-tier singleton" comment is accurate andmaxDeletesPerTickis a genuine global bound. - Serialized ticks via the
inFlightguard correctly prevent stacked sweeps over one tree on a slow MDS, and the scheduler injection makes that testable. - Opt-in defaulting with a dry-run switch, and per-directory faults that never abort the pass, are both right for an irreversible operation.
Recommended Action
- Fix Critical issues before merge — specifically, replace the root-
mtimeage test withexecution_workspaces.lastUsedAtfor ids that resolve to a row, keeping filesystem age only for true orphans. - Address Important issues this cycle — add the "old root, fresh contents" retention test (it should fail today) and tighten the vanish assertion.
- Consider Suggestions opportunistically.
The age predicate read the workspace root's own `mtime` and called it an idleness signal. It is not one. A directory's `mtime` advances only when a direct child entry is added, removed, or renamed — never when nested content is written. For `isolationMode === "workspace"` the only top-level children are `home` and `session`, both created once at materialization, so the root's `mtime` is its creation time and never advances with use. `maxAgeDays` therefore meant "materialized more than N days ago", not "idle for N days". Because the directory name is `execution_workspaces.id`, a workspace in active daily use that happened to be materialized 31 days ago was eligible for irreversible deletion on the first pass — and `storage.home` and `storage.session` are both persistent, so that destroys durable state. The dry-run mitigation did not catch it: such a workspace previewed as `eligible` with nothing in the output to distinguish it from a true orphan. The age test now resolves each directory name against `execution_workspaces`. A resolvable row gates on `lastUsedAt`, which is genuinely maintained on use (refreshed by the workspace-restore path and at creation). Filesystem age survives only for ids with no row — the true orphan cohort this reaper targets and the only population with no database truth to consult. `lookupWorkspaceUsage` is required rather than defaulted: every default here is fail-open, routing directories back to the filesystem predicate. Also from review: - Add the "old root, live row" retention case. It fails against the old predicate, as do two more; mutation-checked by neutering the row lookup. - Assert `res.vanished` directly and drop `force: true`, which suppressed ENOENT and made the vanish branch unreachable. - Count the per-tick cap against eligible work rather than completed unlinks, so a dry run stops where a live pass would and a tick with nothing left to do no longer reports itself capped. - Log sweep failures through the injected logger.
All six addressed in
|
Follow-up to the `lastUsedAt` gate. The first cut treated a resolvable row as authoritative in both directions, so a directory materialized yesterday whose row claimed 90 days idle was deleted. Those two signals contradict each other, and the code acted on the contradiction in the unrecoverable direction. `mtime` being a useless *deletion* signal does not make it a useless *retention* one: whatever a stale row says, a workspace materialized inside the window cannot have been idle for the length of it. Deletion now requires every available signal to agree, which can only ever retain more — the correct bias for an irreversible operation, at the cost of one `stat`. The state should not arise, since materializing a workspace sets `lastUsedAt`. That is the point: it is a contradiction, not a case, and the test now asserts retention rather than pinning the old behaviour.
|
Follow-up The first cut treated a resolvable row as authoritative in both directions, so a directory materialized yesterday whose row claimed 90 days idle was deleted, and I had written a test pinning that as correct. Those two signals contradict each other and I'd resolved the contradiction in the unrecoverable direction.
The state shouldn't arise (materializing sets Still 19 passing, typecheck clean. Mutation check re-run against the new shape: neutering the row lookup fails 2 (live-workspace retention, orphan-only fallback) — one fewer than before, because the case above is now correctly retained by the |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: cbfc3dc
The Critical from the previous head is properly fixed, and fixed at the right level: the age test now asks the database, and mtime is demoted to a retention-only signal rather than being patched in place. I re-derived the two claims the whole safety argument rests on and both hold. One residual: the DB snapshot that does the protecting is read once per sweep and never re-read before the unlink.
Prior Findings Dispositioned (3)
- prior:b63039a critical 1 — fixed —
server/src/services/isolation-workspace-reaper.ts:247— directory names now resolve againstexecution_workspacesand, when a row is found,lastUsedAtdecides (:249).mtimesurvives only as a conjunctive retention gate at:245, which can only retain more.isolation-workspace-reaper.test.ts:127pins the exact case that was unsafe — old rootmtime, used today — and asserts retention, so it fails against the predicate it replaced. I verified the two load-bearing claims independently rather than taking the comments:lastUsedAtis written on both paths that can produce a workspace-isolated run (heartbeat.ts:27159reuse,:27186create), and the directory can only exist if a row did, because the descriptor takes its id from the persisted row (heartbeat.ts:27489,persistedExecutionWorkspace?.id ?? null) andisolationMode === "workspace"requires that id to be non-null (heartbeat.ts:6340-6344,:6511-6519). So the no-row fallback really is the orphan cohort, not a hole a live workspace can fall through. - prior:b63039a important 1 — fixed —
server/src/__tests__/isolation-workspace-reaper.test.ts:105—makeWorkspacestill stamps after writing, but the gap that mattered is closed by two new cases rather than by changing the helper::105writes intohome/andsession/post-stamp and asserts the rootmtimeis unmoved (pinning the premise), and:127asserts the live workspace is retained. Retention now correctly comes from the DB signal rather than from filesystem content. - prior:b63039a important 2 — fixed —
server/src/__tests__/isolation-workspace-reaper.test.ts:335—expect(res.vanished).toBe(1)replaces the disjunctive assertion, and production droppedforceatisolation-workspace-reaper.ts:314, so the ENOENT branch is genuinely reachable. See the Suggestions for the residual coverage note.
Critical Issues (0)
Important Issues (1)
-
[gstack/review + native-codex]
server/src/services/isolation-workspace-reaper.ts:222— the usage snapshot is read once for the whole sweep and never re-read, so the signal that is the sole protection for a live workspace goes stale over the life of the pass. The module states as property 3 (:34-37) that "every directory is re-checked immediately before removal" — and it does re-check existence (:240) and layout (:265), but notlastUsedAt. Those two re-checks cannot see this: a run resuming a long-idle workspace refreshes the row and writes underhome//session/, neither of which changes the root's existence nor its top-level layout, and by this module's own finding not itsmtimeeither.The window is not instantaneous by design.
maxDeletesPerTickdefaults to 200 and deletion is documented here as MDS-metadata-bound at ~145 files/s serial against a saturated MDS (:39-42), so a full tick is plausibly minutes to tens of minutes of wall time between the snapshot and the last unlink. The exposure is a workspace that crosses the cutoff and is then resurrected inside that window — reuse after a long gap is a real pattern, andresolvedWorkspaceReusePolicy.shouldRestoreExistingWorkspace(heartbeat.ts:27150) exists precisely to serve it. Consequence is worse than the cold start the module budgets for at:83-84:storage.home/storage.sessionare removed underneath a run that has already restored and started writing, rather than before it starts.I'd rank this Important rather than Critical because it needs a coincidence and costs a failed run plus scratch, not source. But the module's own stated bias — "requiring both can only ever retain more, which is the correct bias for an irreversible operation" (
:235-237) — is the argument for closing it.- Re-read
lastUsedAtfor the single id immediately beforefs.rm, in the same place existence and layout are already re-checked. It is one primary-key lookup per actual deletion, bounded bymaxDeletesPerTick, on a pass that runs daily — negligible against the unlink cost it guards. Skipping the re-read indryRunkeeps the preview free. Chunking the batch and re-querying per chunk would narrow the window but not close it.
- Re-read
Suggestions (3)
- [pr-review-toolkit/comments]
server/src/services/isolation-workspace-reaper.ts:120— "one batched query per sweep, served by thelastUsedAtindex" is not what happens. The query filters onid(:135), so it is served by the primary key; the only index touchinglastUsedAtisexecution_workspaces_company_last_used_idxon(companyId, lastUsedAt)(execution_workspaces.ts:59-62), whose leading column is absent from the predicate. No performance consequence — the PK is the better path — but the module doc leans on the same citation at:71, and in a file this careful about evidence a citation that does not hold is worth correcting. - [pr-review-toolkit/code]
server/src/services/isolation-workspace-reaper.ts:245— the fresh-mtimeretention increments no counter, so it is the one outcome the result cannot name: an operator reading the sweep log gets it only as the unexplained remainder ofscanned - eligible - retainedInUse - skippedLayout. It is also the branchisolation-workspace-reaper.test.ts:169exercises, and that test can only asserteligible: 0rather than the reason. AretainedFreshcounter would make the first live pass fully self-describing, which is the point of the dry-run story. - [pr-review-toolkit/tests + errors]
server/src/services/isolation-workspace-reaper.ts:317— droppingforcewas the right call, but the branch it unlocks still has no test:isolation-workspace-reaper.test.ts:319reachesvanishedthrough the readdir ENOENT path at:260, so re-addingforce: truewould leave the suite green while silently reporting concurrent removals as deletions. Related, and worth a thought rather than necessarily a change: a partially-failedfs.rmleaves a subset layout, which makes the directory permanently unreapable and adds a standing entry toskippedLayout— the exact counter:271-279asks operators to watch against a baseline of 1.
Strengths
- The fix is conjunctive rather than substitutive (
:229-237). ReplacingmtimewithlastUsedAtoutright would have traded one single point of failure for another; requiring both means a stale row cannot delete a freshly materialized workspace, and the rationale is argued in the comment rather than asserted. lookupWorkspaceUsageis required and deliberately not defaulted (:156-162), with the reason stated: any default is fail-open, because "no rows" silently routes every directory to the filesystem predicate this module exists to stop using. That is the right place to spend a small ergonomic cost.UUID_PATTERNfiltering beforeinArray(:123) is a real guard, not decoration —idis a genuineuuidcolumn (execution_workspaces.ts:18), so an unfiltered stray directory name would fail the whole sweep on a Postgres cast error.- The layout allowlist and the
wt-blo-19094regression test (isolation-workspace-reaper.test.ts:206) survive the rewrite intact, and:105now pins the filesystem premise the design depends on so a future reader cannot re-derive the original mistake from first principles. - Cap accounting moved to eligibility (
:286-291), which makesdryRunstop where a live pass would and stops a fully-retained tick reporting itself capped;:270and:290cover both. - Registration mirrors
startStrandedBlockedIssueReconcilerexactly (index.ts:1722against:1699) — same worker-tier guard, same lazy import, same discarded stop handle — and the opt-in default is genuinely off (config.ts:762-763gates on=== "true"), with a 7-day floor onmaxAgeDaysso a between-runs workspace cannot be configured into eligibility.
Recommended Action
- No Critical issues. The previous blocker is resolved at the design level, not papered over.
- Address the Important this cycle: re-read
lastUsedAtimmediately beforefs.rm, alongside the existence and layout re-checks that are already there, so property 3 holds for the signal that actually protects a live workspace. - Consider the Suggestions opportunistically — the
:120index claim and the untested vanish branch are both cheap.
Note on verification state: the required checks at this head were still queued when I read them, so this review reflects source analysis only and no green test evidence. Worth confirming the suite before merge, particularly isolation-workspace-reaper.test.ts.
The sweep read `execution_workspaces` once at the head of the pass and then let that snapshot authorize every unlink in the tick. `lastUsedAt` is the only signal that can protect a live workspace, so it is also the only one whose staleness is dangerous — and the window is not instantaneous: `maxDeletesPerTick` unlinks against an MDS measured at 145 files/s serial is minutes of wall time. A run resuming a long-idle workspace inside that window refreshes the row and writes under `home`/`session`. That moves neither the root's existence, nor its top-level layout, nor its `mtime`, so both existing pre-unlink re-checks are blind to it and the delete lands underneath a run that has already restored and started writing — worse than the cold start this module budgets for, because `storage.home`/`storage.session` are persistent. So property 3 now covers all three signals: re-read the single row by primary key immediately before `fs.rm`, at most one lookup per prospective unlink. `dryRun` skips it, keeping a preview at one query. Also from review: - `retainedFresh` counter, so the fresh-`mtime` retention is named rather than left as the unexplained remainder of a sweep log. With no concurrent removals `scanned` is now exactly `retainedFresh + retainedInUse + skippedLayout + eligible`, pinned by a test. - Correct the index citation. The lookup filters on `id`, so it is served by the primary key; the only `lastUsedAt` index is `(companyId, lastUsedAt)`, whose leading column the predicate never constrains. No behavior change — the claim was just wrong. - Cover the `fs.rm` ENOENT branch. The only test reaching `vanished` went through the *readdir* ENOENT path, so re-adding `force: true` would have left the suite green while silently reporting concurrent removals as deletions. All four new tests fail against the predicate without the re-read; the resurrection case reports `eligible: 1` there, i.e. it deletes. Co-Authored-By: Paperclip <noreply@paperclip.ing>
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 952b5f2
The prior Important is fixed properly — the re-read is placed at the last possible moment, it is skipped in dryRun for the stated reason, and the three new tests pin the racing behaviour rather than the implementation. retainedFresh also lands, closing a Suggestion. What the new call introduces is a second failure mode on the same line: it is the first operation in this module that can throw after deletions have already happened, and the sweep summary is the casualty.
Prior Findings Dispositioned (1)
- prior:cbfc3dc important 1 — fixed —
server/src/services/isolation-workspace-reaper.ts:346-352—lastUsedAtis re-read per directory immediately beforefs.rm, after the existence (:269) and layout (:297) re-checks, so property 3 now covers the signal that actually protects a live workspace. The placement is right in three ways I checked rather than assumed: it sits after the capbreakat:318, so a retained directory does not consume cap budget andcappedstays honest; it reuses the sweep-startcutofffrom:226rather than recomputing, which can only retain more; andretainedInUserather thaneligibleis incremented, so thescannedinvariant at:196-198still balances.isolation-workspace-reaper.test.ts:371exercises the exact resurrection case (row refreshed between snapshot and unlink) and asserts the nested transcript survives;:395pins the negative so the re-read cannot degrade into "always retain";:420asserts one batched call plus exactly one per unlink, and one call total indryRun.
Critical Issues (0)
Important Issues (2)
-
[gstack/review + native-codex]
server/src/services/isolation-workspace-reaper.ts:346— a fault in the per-directory re-read discards the entire sweep record, including deletions already performed. The comment at:342-345argues the propagation deliberately ("a lookup fault propagates and ends the tick... the fail-closed reading of 'cannot confirm idle' is 'do not delete'"), and I agree with that half. But nothing catches it before:388, so thesweep completelog never runs andresultis dropped on the floor. The operator seesisolation-workspace reaper sweep failedfrom:423and no count of what was already unlinked.This is new at this head, and specifically new. Before it,
lookupWorkspaceUsagewas called only at:251, strictly before the loop — a throw there meant zero deletions had occurred, so there was nothing to lose. Now the same throw can land after up tomaxDeletesPerTickirreversible removals (default 200,config.ts:343). For a module whose stated operating story is a dry run followed by a watched first live pass, losing the deletion count on the one path that fails mid-flight is the worst place for the record to go missing — and it is the run an operator would most want to reconstruct.Nothing covers it: no test in the file drives
lookupWorkspaceUsageto reject, so the behaviour on that path is unobserved in either direction.try/catchthe re-read, log the error,break. That keeps the fail-closed semantics exactly as argued (no further deletions this tick, next tick retries from a fresh snapshot) while falling through to the summary log with the real counts. While there,:215-216still promises "Never throws for a per-directory fault" — the re-read is a per-directory fault that does; abreakmakes the contract true again rather than requiring the reader to reconcile it against:342.
-
[pr-review-toolkit/code + errors]
server/src/services/isolation-workspace-reaper.ts:349— the re-read retention is the near-miss this change exists to prevent, and it is indistinguishable from routine. It increments the sameretainedInUsecounter as the ordinary snapshot-time retention at:283and logs nothing at all, so a workspace that was one query away from being deleted underneath a live run reports identically to one that was never a candidate.That cuts against this head's own reasoning in two places.
retainedFreshwas added here precisely so "an operator reading a sweep log can name every outcome" (:196-198) instead of inferring it from a subtraction — yet the most alarming outcome in the module remains unnameable. AndskippedLayoutgets a per-directorylog.warnat:303justified by "worth a human look", for an event strictly less urgent than this one. The practical cost is that the window's frequency is unmeasurable: there is no way to learn from production whether the re-read ever fires, which is the only evidence that would confirm or retire the minutes-long exposure argued at:330-334.- A distinct counter (
retainedResurrected, or similar) plus alog.warncarryingdirand bothlastUsedAtvalues. Note this needs the counter to stay outside the:196-198identity or that invariant — andisolation-workspace-reaper.test.ts:493— has to widen with it.
- A distinct counter (
Suggestions (3)
- [pr-review-toolkit/code]
server/src/services/isolation-workspace-reaper.ts:274— the fresh-mtimeretentioncontinues before the layout check at:297, so a directory with a non-allowlisted layout is silently countedretainedFreshwhile it is young. TheskippedLayout-above-baseline signal that:299-302asks operators to watch therefore only ever observes aged directories — a newwt-blo-19094-shaped worktree goes unreported for its entire firstmaxAgeDays, which is exactly the window in which a human could still act on it cheaply. Reordering costs onereaddiron directories that are currently skipped; if that is not wanted, the monitoring rationale at:299-302is worth qualifying so the baseline is not read as covering the whole tree. - [pr-review-toolkit/comments]
server/src/services/isolation-workspace-reaper.ts:196-198— the invariant is qualified only by "with no concurrent removals", butfailedalso breaks it and needs no concurrency: a directory that is present but unreadable takes:293(EACCES, and on this filesystem an MDS hiccup) and lands in none of the four named buckets.isolation-workspace-reaper.test.ts:493asserts the identity on a tree where that cannot arise, so the gap is invisible there. Either namefailedin the qualifier or fold it into the sum. - [pr-review-toolkit/tests]
server/src/__tests__/isolation-workspace-reaper.test.ts:498—it(..., async () => { const res = await reapIsolationWorkspaces({collapsed onto one line in this head's diff. Cosmetic and pre-existing elsewhere in the file (:328has the same shape), so not a blocker — but this one is a fresh regression rather than inherited, and it is a one-line fix while the diff is open.
Strengths
- The fix is placed rather than merely added. Sitting after the cap
break(:318) is a non-obvious detail with three consequences — cap budget,cappedhonesty, and thescannedidentity — and all three come out right; the easy placement immediately after the layout check would have broken the second. racingUsageLookup(isolation-workspace-reaper.test.ts:55) is the right test seam:onRecheckruns before the re-read answers, so:448can remove the directory inside the genuine window and finally reach thefs.rmENOENT branch that droppingforceunlocked — closing the untested-vanish-path Suggestion from the last head at the same seam, as its docstring says.:395("still deletes when the re-read confirms the workspace is idle") is the test that most reviews omit. Without it the re-read could regress to unconditional retention and the suite would stay green.- Skipping the re-read in
dryRunis argued from cost (:338-340) and has its consequence stated — a preview may over-report eligibility, "the harmless direction" — rather than left for a reader to work out.:420pins both call counts so the property is enforced, not just documented. - The
:120index citation from the last head is corrected in place with the reason preserved (:129-136), including why nobody should "optimize" towardexecution_workspaces_company_last_used_idx. Correcting a citation rather than deleting it is the more useful repair. retainedFreshplus:470gives the first live sweep a fully named outcome set, which is what makes the dry-run story actionable rather than decorative.
Recommended Action
- No Critical issues.
- Address the two Importants this cycle. The first is a
try/catch+breakthat preserves the fail-closed semantics you already argued while keeping the sweep summary; the second is a counter and awarnso the near-miss this PR exists to prevent is visible when it happens. - Consider the Suggestions opportunistically.
Verification state: the required checks at this head are still queued/in_progress (General tests (server 1-4/4), Build, Typecheck + Release Registry, e2e), so this review is source analysis with no green test evidence. Per the standing rule I am not merging or approving on a non-success gate — worth confirming isolation-workspace-reaper.test.ts in particular before merge, since seven of its cases are new at this head.
Disposition of Ally's review of
|
…ear miss Both Importants from Ally's review of 952b5f2, each introduced by the pre-unlink re-read added at that head. The re-read is the first call in this module that can fault *after* irreversible removals — up to `maxDeletesPerTick` of them. Propagating discarded `result`, so the `sweep complete` log never ran and the count of what had already been unlinked was lost, on precisely the run an operator would most need to reconstruct. It now catches, logs with the running `deleted` count, and `break`s: fail-closed semantics are identical (no further deletions this tick, next tick retries from a fresh snapshot) while the summary is still reached with real counts. That also makes the "never throws for a per-directory fault" contract on this function true again rather than something the reader has to reconcile against the re-read's own comment. `lookupFaulted` distinguishes the early stop from `capped`, since a fault means the remaining directories were never assessed. The re-read's retention also incremented `retainedInUse` and logged nothing, so a workspace that came within one query of being deleted underneath a live run reported identically to routine steady state — leaving the frequency of the very window this PR exists to close unmeasurable in production. Now `retainedResurrected` plus a `log.warn` carrying both timestamps and the cutoff, so the observed width of that window can be read off a real sweep instead of argued from the MDS throughput estimate. Also from the review: - Fold `failed` into the `scanned` identity and widen it for the new counter. `failed` needs no concurrency to occur (`EACCES`, or an MDS hiccup), so the old qualifier left the invariant quietly false. - Qualify the `skippedLayout` monitoring rationale: it observes aged directories only, because the fresh-`mtime` retention returns before the layout check. Reordering would surface a young worktree-shaped directory earlier but costs a `readdir` on every young directory every sweep, and bounding that MDS metadata load is property 4 of this module. Recorded as a deliberate trade; the safety allowlist is unaffected either way. - Un-collapse the one-line `it(...)` regression at test :498. Verified at this tree: 25 tests pass, typecheck clean. Mutation-checked both fixes independently — removing the try/catch fails only the new fault test, and folding `retainedResurrected` back into `retainedInUse` fails only the resurrection test. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Both Importants from the
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 0e5c33c
Both Importants from the last head are fixed, and the fault handler is fixed in the shape that was actually argued for rather than by widening a try around the loop. Two prior Suggestions land as well — one by correcting behaviour, one by recording the trade-off as deliberate, which is the more honest of the two available answers. No Critical or Important findings at this head. What remains is one real gap: the sweep record this head exists to preserve does not quite balance on the very path it preserves it for.
Prior Findings Dispositioned (2)
- prior:952b5f2 important 1 — fixed —
server/src/services/isolation-workspace-reaper.ts:397-406— the re-read is wrapped,lookupFaultedis set, the error is logged withdeleted: result.deletedso the count survives in the message as well as the return, and itbreaks rather than rethrowing — so control reaches thesweep completelog at:466with real counts. I checked the two properties that make this a fix rather than a relocation: the fail-closed semantics are unchanged (no directory is unlinked on the faulting iteration, and nothing after it is assessed), and the:215-216"never throws for a per-directory fault" contract is now true rather than needing to be reconciled against the comment below it.lookupFaultedis also given its own field with the distinction fromcappedstated at:226-231— fault versus budget — which is the part an operator needs to tell "I stopped early on purpose" from "I stopped early because the database went away".isolation-workspace-reaper.test.ts:520drives the second re-read to reject and asserts{ deleted: 1, lookupFaulted: true, failed: 0 }at:543, closing the "no test driveslookupWorkspaceUsageto reject" gap I flagged last head. - prior:952b5f2 important 2 — fixed —
server/src/services/isolation-workspace-reaper.ts:416-417— the near miss now increments its ownretainedResurrectedcounter and emits alog.warncarryingdir, both timestamps, and thecutoff, so the width of the window that was actually observed can be read off production instead of argued from the MDS estimate — which was the point of raising it. The rationale at:193-205states why folding it intoretainedInUsewould have been wrong, and the widened invariant at:211-213correctly brings the new counter into the sum rather than leaving it outside.isolation-workspace-reaper.test.ts:390assertsretainedResurrected: 1andretainedInUse: 0, so the two outcomes cannot silently re-merge. One detail I checked rather than assumed: when a directory had no row at snapshot time but has one at re-read (an orphan adopted mid-sweep),snapshotLastUsedAtlogsnullrather than fabricating a timestamp, which is the honest reading.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
-
[gstack/review + native-codex]
server/src/services/isolation-workspace-reaper.ts:406— the faultbreakdoes not decrementscanned, so the documented identity at:211-213is off by one on exactly the path this head added. The faulting directory has already been counted at:283but lands in none of the six named buckets: notretainedFresh,retainedInUse,retainedResurrected,skippedLayout,failed, oreligible. The suite's own fault case demonstrates it — two aged workspaces, the first deletes and the second faults, givingscanned: 2against a bucket sum ofeligible: 1.Worth raising because the cap
breaktwelve lines earlier handles precisely this, withresult.scanned -= 1at:360and a comment explaining why the capped entry should not count as examined. ThelookupFaulteddoc at:226-231explicitly invites the comparison tocapped("Distinct fromcapped: both stop the pass with work remaining"), and the code then diverges on the one property that comparison makes salient. It also sits slightly against this head's own reasoning: the fix above exists so the record survives a mid-flight fault, and the record is the one thing that does not reconcile there.Invisible in the suite in both directions:
:478asserts the identity on a tree where no fault can arise, and:520assertsdeleted/lookupFaulted/failedbut neverscanned. Same structure as thefailedgap I raised last head — the assertion and the uncovered path are in different tests.result.scanned -= 1before thebreak, mirroring:360; or namelookupFaultedas an exception in the:211-213qualifier the wayfailednow is. Addingscannedto the:543assertion would pin whichever is chosen.
-
[pr-review-toolkit/tests]
server/src/__tests__/isolation-workspace-reaper.test.ts:528— the fault seam distinguishes the opening snapshot from a per-directory re-read byids.length > 1, which holds only because this case happens to stage two workspaces. A later case reusing this shape with a single workspace would have its batched snapshot counted as a re-read and throw on the second directory's lookup instead — a confusing failure, since the test would still look like it was exercising the re-read path.racingUsageLookupat:55already exists as the deliberate seam for this; keying on a sentinel, or extending that helper with a fault mode, would make the distinction structural rather than incidental to the fixture size. -
[pr-review-toolkit/comments]
server/src/__tests__/isolation-workspace-reaper.test.ts:328—it(..., async () => { await makeWorkspace(is still collapsed onto one line. The sibling instance at the old:498was fixed at this head, so this is the last one; cosmetic, and a one-line fix while the diff is open.
Strengths
- The fault fix is scoped to the one call that needs it rather than wrapped around the loop. A
tryaround the whole sweep would have produced the same summary log while also swallowing genuine programming errors, and the comment at:387-394argues the narrow choice explicitly — including why propagation was wrong here specifically (first call that can fault after irreversible removals) rather than wrong in general. retainedResurrectedis kept out ofretainedInUseon a stated operational argument — same action, opposite meaning — and the invariant plus its test were widened in the same change instead of being left to drift. That was the exact hazard I noted last head, and it was handled.- The layout-ordering Suggestion is answered by recording the trade rather than by making the change (
:329-341): checking layout before the freshnesscontinuewould surface a young worktree-shaped directory earlier, but costs areaddiron every young directory every sweep, against property 4. It states that the safety allowlist is unaffected either way and only the warning is delayed. Declining a suggestion with the reasoning written down is more useful than either silently ignoring it or silently taking it. log.erroron the fault path carriesdeleted: result.deletedin the message and not only in the return value, so the count is recoverable from logs alone even if the caller drops the result — which is the realistic incident posture for a worker-tier sweep.- The
snapshotLastUsedAt: owner ? ... : nullbranch at:419handles the adopted-orphan case without inventing a timestamp, and thedryRunskip at:395keeps the preview at exactly one query with its over-reporting consequence stated as the harmless direction. :403("still deletes when the re-read confirms the workspace is idle") and:390's negative assertion together mean neither the re-read nor the new counter can regress into unconditional retention while the suite stays green.
Recommended Action
- No Critical issues, and no Important issues at this head. The two prior blockers are resolved at the level they were raised.
- Consider the Suggestions opportunistically. The first is the only one with substance — a one-line
scanned -= 1(or a qualifier) plus one assertion — and it is worth taking while the diff is open, since it is the last place the sweep record does not reconcile.
Verification state: the required checks at this head are still queued/in_progress (General tests (server 1-4/4), Build, Typecheck + Release Registry, e2e, Worktree install), with only Helm chart, policy, security-review, and Vendored claude_k8s adapter green. So this is source analysis with no green test evidence, and per the standing rule I am not approving or merging on a non-success gate. Three of the reaper suite's cases are new or changed at this head — :520, :390, and the widened identity assertion at :501 — so confirming isolation-workspace-reaper.test.ts before merge is the specific thing worth watching.
Thinking Path
Linked Issues or Issue Description
Relationship to #1252, stated explicitly because both add a sweep and both touch
server/src/index.ts:{home, session}scratchcleanupEligibleAtobligationmtimeThey are complementary. #1252 collects workspaces the system knows it owes cleanup for; this reaps the orphan population that never acquired such an obligation — which is precisely why that population reached 406.7 GiB while #1252 was in flight. If #1252 lands first, this PR rebases onto it; the allowlist makes double-handling safe either way, since a directory removed by either mechanism reads as
vanishedto the other rather than as an error.reaper,workspace retention,isolation-workspace; found and dispositioned fix(workspaces): collect completed per-run worktrees (BLO-22984) #1252 above).What Changed
server/src/services/isolation-workspace-reaper.ts—reapIsolationWorkspaces()(one sweep) andstartIsolationWorkspaceReaper()(serialized interval driver, matching thestartStrandedBlockedIssueReconcilershape).server/src/__tests__/isolation-workspace-reaper.test.ts— 12 tests.server/src/config.ts— five settings, prefixedPAPERCLIP_ISOLATION_WORKSPACE_REAPER_, with numeric bounds registered inNUMERIC_SETTING_BOUNDSand the interval registered inTIMER_SETTING_MS_FACTOR.server/src/index.ts— worker-tier registration behind the enable flag, alongside the existing reconcilers.Design, and why each property is load-bearing
1. Positive layout allowlist, not an age-only predicate. A directory is removed only when its top level is exactly
{home, session}— Claude scratch (HOME) and session state (CLAUDE_CONFIG_DIR). When this cohort was audited by hand across 448 real directories, 447 matched and one did not: it carriedwt-blo-19094, a real git worktree. An age-onlyrm -rfdestroys it. The allowlist costs ~0.2% of the reclaim and removes the entire class — including on future passes whose composition nobody has inspected. Non-matching directories are skipped unexamined and logged individually, so a skip count above the historical baseline of 1 is visible rather than silent.2. Opt-in, following
strandedRecoveryHandBack. This deletes irreversibly. Defaulting it on would make "deploy the code" and "delete a month of workspaces" the same act, with no run in between to inspect what the predicate matched — the reasoning already written intostrandedRecoveryHandBackDrainEnabled. A dry-run switch makes the first enablement an observation.3. Bounded per tick. Deletion here is MDS-metadata-bound, not data-bound — measured 145 files/s serial against a saturated MDS, and only 327 f/s at 12-way concurrency (2.25× for 12×, so the MDS is the constraint).
maxDeletesPerTickkeeps a pass from becoming a latency event for every other CephFS consumer on the cluster.4. Idempotent and concurrency-tolerant. Another reclaimer was observed on this same tree mid-incident (1,235 → 1,065 dirs in ~5h, cause unidentified). Every directory is re-checked immediately before removal; one that vanishes between scan and unlink is counted as
vanished, not raised.One correction worth carrying
These directories are keyed by
persistedExecutionWorkspaceId, notrunId— per-execution-workspace, reusable, able to outlive any single run. (run:<runId>isolation goes to/runtime-cache/paperclip-runs/, local disk, not Ceph.) So "ephemeral by construction" describes the other root. Deleting a stale one costs a cold start, not source — durable transcripts live underdata/run-logs/(35,627 run dirs).⚠ The rest of this section previously said age is judged on the directory's own
mtime, because that "separates idle for a month from between runs". That was wrong, and review caught it. A directory'smtimeadvances only when a direct child entry is added, removed, or renamed — never when nested content is written. ForisolationMode === "workspace"the only top-level children arehomeandsession, both created once at materialization, so the root'smtimeis its creation time and never advances with use.maxAgeDaystherefore meant "materialized more than N days ago", and since the directory name isexecution_workspaces.id, a workspace in active daily use that happened to be materialized 31 days ago was eligible for irreversible deletion on the first pass — ofstorage.home/storage.session, bothpersistent. The dry-run mitigation did not catch it either: such a workspace previewed aseligiblewith nothing to distinguish it from a true orphan.The age test now resolves each directory name against
execution_workspaces. A resolvable row gates onlastUsedAt, which is maintained on use (refreshed on every workspace restore and at creation). Filesystem age survives only for ids with no row — the true orphan cohort this PR is aimed at, and the only population with no database truth to consult.lookupWorkspaceUsageis a required option, not a defaulted one, because every default here is fail-open.Configuration
All settings prefixed
PAPERCLIP_ISOLATION_WORKSPACE_REAPER_:ENABLEDfalse(opt-in)DRY_RUNfalseINTERVAL_MINUTES1440MAX_AGE_DAYS30(floor 7)MAX_DELETES_PER_TICK200Verification
vitest run server/src/__tests__/isolation-workspace-reaper.test.ts— 19 passed. Adds the "old root, live row" retention case, thelastUsedAt-overrides-filesystem pair, the orphan-only fallback, and dry-run cap engagement, on top of the worktree skip, partial layout, dry-run, per-tick cap, idempotency, the scan/delete race, absent root, and stray files at the root.pnpm --filter @paperclipai/server typecheck— 0 errors.mtimepremise is pinned as a test, not asserted in prose: backdate a{home, session}root, writesession/.claude/projects/a.jsonl,home/.bashrc, andmkdir -pboth children as a re-run would — the rootmtimeis asserted unchanged. That test is the reason the DB gate exists.const owner = undefined, i.e. the old mtime-only predicate) produces 3 failed / 14 passed, failing exactly the live-workspace retention, thelastUsedAt-over-filesystem preference, and the orphan-only fallback. NeuteringisReapableLayouttoreturn truestill fails the worktree-preservation and partial-layout tests.min(ceph_pool_max_avail)went from −38.3 GiB over the preceding 5h to +7.3 GiB during the pass.Risks
data/run-logs/.maxDeletesPerTickand by serializing ticks so a slow sweep cannot stack.server/src/index.ts(both register a sweep). Textual, adjacent registrations; whichever lands second rebases.mtimeas the age signalexecution_workspacesrow there is no better signal, and "materialized >30d ago with no owning row" is the intended meaning there. A row deleted while its directory is still in use would fall back to mtime — but nothing in the workspace lifecycle removes the row and keeps the tree.Model Used
claude-opus-5 (Claude Code)
🤖 Generated with Claude Code