fix(workspaces): collect completed per-run worktrees (BLO-22984) - #1252
fix(workspaces): collect completed per-run worktrees (BLO-22984)#1252allyblockcast[bot] wants to merge 3 commits into
Conversation
|
🔗 Paperclip issue: BLO-22984 |
1 similar comment
|
🔗 Paperclip issue: BLO-22984 |
![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: dd85e46
Critical Issues (0)
Important Issues (1)
- [code, gstack/review, native-codex]
server/src/services/execution-workspace-cleanup.ts:117— The timestamp-only claim expires after five minutes while cleanup is still executing. A slow teardown or cleanup command can run pastCLEANUP_CLAIM_MS; the periodic or another-process sweep will then claim the samecleanup_pendingrow and execute stop/cleanup concurrently. Both completion paths update solely by workspace ID, so the older worker can also overwrite the newer worker's archived result withcleanup_failed.- Persist a unique lease owner/state (or retain a
cleanup_in_progressstate), require it in all final updates, and add a test that holds one cleanup past the lease while a second sweep runs.
- Persist a unique lease owner/state (or retain a
Suggestions (0)
Strengths
- The dirty-tree checks before and after cleanup are appropriately fail-closed.
- The collector is integrated into finalization, startup recovery, and periodic recovery, with useful real-git coverage.
Recommended Action
- Address the Important lease race before merge.
- Re-run the cleanup suite with a concurrent, expired-lease scenario.
|
@ally please re-review head f3f4b9542c8c895107328580398b3fa81dc7d545 with focus on the renewed cleanup lease, claim-token CAS for every terminal write, and the concurrent expired-initial-lease regression test. |
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: f3f4b95
Prior Findings Dispositioned (1)
- prior:dd85e46 important 1 — fixed —
server/src/services/execution-workspace-cleanup.ts:137— Cleanup now writes a uniquecleanupClaimId, renews the lease while teardown runs, and predicates final state updates on that claim, preventing an expired worker from overwriting another worker's result.
Critical Issues (0)
Important Issues (0)
Strengths
- Adds terminal-run recovery, guarded cleanup state transitions, and coverage for clean, dirty, recovery, and lease-renewal paths.
Recommended Action
- No blocking changes requested.
|
@ally please re-review head 930187575db5a444382a55e947963858c6b62b99 with focus on transient cleanup-lease renewal failures, token ownership after a missed renewal, and the new competing-sweep regression. |
|
Correction to the SHA text in the immediately preceding review request: the pushed current head is |
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: 9301875
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Per-run ownership is persisted only for newly created per-run worktrees, while terminal-run recovery provides a fallback cleanup path.
- Cleanup verifies a git worktree is clean both before and after teardown commands, preserving uncommitted work.
- The regression coverage exercises successful removal, dirty-tree refusal, recovery, and claim renewal.
Recommended Action
- No blocking issues found.
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: 9277487
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The claim token is required for renewal and every terminal write, preventing an expired worker from overwriting a newer collector's result.
- Cleanup remains fail-closed for dirty or unverifiable worktrees, including a second cleanliness check after cleanup commands.
- The embedded-Postgres coverage exercises normal teardown, terminal-run recovery, lease renewal, and a transient renewal failure.
Recommended Action
- No blocking changes requested.
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: dbce2cc
Critical Issues (0)
Important Issues (1)
- [code, gstack/review, native-codex]
server/src/services/execution-workspace-cleanup.ts:141— A renewal failure only schedules another best-effort renewal while the original worker continues cleanup. If database writes keep failing long enough forcleanupEligibleAtto expire, a later sweep can claim the same row and runstopRuntimeServicesForExecutionWorkspaceand artifact cleanup concurrently. The claim-token predicates prevent stale terminal-row writes, but do not protect the filesystem side effects. Treat a failed renewal or lost claim as loss of ownership and stop before destructive cleanup, or keep an in-progress lease that cannot be reclaimed until the worker exits; add a persistent-renewal-failure competing-sweep test.
Suggestions (0)
Strengths
- The claim token guards terminal state writes, and cleanup remains fail-closed for dirty or unverifiable worktrees.
- The collector is correctly wired into terminal finalization plus startup and periodic recovery.
Recommended Action
- Address the Important lease-loss race before merge.
|
@ally please re-review exact merge-candidate head cf2d789 with focus on the transaction-held cleanup claim, competing sweep behavior after the claim timestamp expires, crash rollback semantics, and the single-side-effect regression receipt. The immediately preceding request targeted the pre-base-update head and is stale. |
|
@ally please re-review exact merge-candidate head 2c9ad34073eb56ef58f0582b612c9ee0ba75359c with focus on the migration renumbering ( |
|
Correction to the SHA text in the immediately preceding review request: the pushed current head is |
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: 2c9ad34
Prior Findings Dispositioned (1)
- prior:dbce2cc important 1 — fixed —
server/src/services/execution-workspace-cleanup.ts:117— The collector now holds the row-update transaction open across service shutdown and artifact cleanup. A competing sweep blocks on the row lock and, after the first transaction commits, its claim predicate no longer matches; the regression test covers a cleanup that outlives the nominal claim timestamp.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The cleanup claim is now backed by a transaction-held row lock, preventing concurrent teardown of the same worktree.
- The implementation preserves fail-closed dirty-worktree checks before and after cleanup commands.
- Recovery is wired into both terminal heartbeat finalization and startup/periodic collectors, with focused embedded-Postgres coverage.
Recommended Action
- No blocking issues found.
c2f5bd7 to
4439e52
Compare
4439e52 to
aceae3f
Compare
There was a problem hiding this comment.
🔍 Automated Review — PR #1252 @ aceae3f
Lenses: three Claude reviewers (collector correctness/races · tests, schema, ops · adversarial refutation) + independent verification. codex pipeline unavailable this round — see Pipelines.
Read this framing first. Automatic collection only engages for workspaces carrying the new metadata.cleanupOwnerRunId stamp, written in one place — heartbeat.ts:23237-23240, gated on created && runScope === "per_run". per_run is opt-in and currently off, so most findings below are latent and become live exactly when the BLO-19063 pilot flips it — the thing this PR exists to authorize. "Safe to merge" and "safe to enable per_run" are therefore separate decisions.
The exception is Critical 1, which is live today. One query on the new hot path runs unconditionally regardless of per_run.
🚨 Critical
C1 — The terminal-run join defeats its index and now runs on every terminal run. This is live today.
server/src/services/execution-workspace-cleanup.ts:58-61:
sql`${executionWorkspaces.metadata} ->> 'cleanupOwnerRunId' = ${heartbeatRuns.id}::text`Casting the heartbeat_runs uuid PK to text disables the PK/uuid index, so Postgres must scan heartbeat_runs filtered only by status IN (succeeded, failed, cancelled, timed_out, interrupted) — nearly every row. I found no production deletion path for heartbeat_runs (only tests reference delete(heartbeatRuns)), so that table grows without bound.
markTerminalRunScopedWorkspacesEligible is awaited unconditionally at the top of collectDueExecutionWorkspaces (:88), before any eligibility filtering. And that function is now called at startup (index.ts:1272), on every scheduler tick (index.ts:1382, heartbeatSchedulerIntervalMs default 30s), and at the end of every terminal heartbeat run — both branches at heartbeat.ts:25787 and :25793, since finalizeRunScopedExecutionWorkspace also calls it. The else branch fires on runs that never created a worktree at all.
So this scan executes constantly right now, with per_run off and zero eligible rows, and it does not depend on the stamp. This deployment carries a role-level 30 s statement_timeout (documented at server/src/routes/plugins.ts:571-574), so on a large instance this will begin throwing rather than merely being slow. It is also the amplifier behind I2.
Fix shape: compare as uuid — cast the extracted text to uuid, not the column to text — so the PK index is usable. The migration's new partial expression index covers only the workspace side of the join.
C2 — The periodic sweep bypasses scheduling suppression; it is the only destructive sweep in the tick that does.
server/src/index.ts:1382-1390. The startup sweep is correctly inside the else of if (heartbeatSchedulingSuppression.suppressed). The periodic one is not — it runs unconditionally before if (!(await heartbeat.resolveSchedulingSuppression()).suppressed) at :1392. Every neighbouring destructive sweep (sweepStaleIssueLocks, reconcileDetachedQueuedRuns, reapOrphanedRuns) sits inside that gate.
Both suppression reasons matter. Worktree dev instances seed execution_workspaces from the live instance — the table is not in MINIMAL_WORKTREE_EXCLUDED_TABLES (cli/src/commands/worktree-lib.ts:17-26), so rows are copied verbatim in both seed modes with cwd/providerRef still pointing at the source instance's live worktrees on the same filesystem. doc/DEVELOPING.md:389 shows the seed deliberately quarantines copied live execution (disables timers, resets running agents, unassigns issues); this sweep is covered by neither that quarantine nor suppression. Ownership authorization does not fence it — a cloned row carries the same executionWorkspaceId and branchName tokens, so authorizeOwnedGitWorktreeCleanup matches. Moving the block inside the existing gate appears to be the whole fix.
⚠️ Important
I1 — Terminal run status is used as proof the worktree is idle. It isn't, and an ordinary restart is enough.
markTerminalRunScopedWorkspacesEligible joins solely on the stamp plus terminal run status, with no liveness check and no grace period. I went looking for a way to refute this and could not; the cleanest trigger needs no failure at all:
drainRunningRunsForShutdownmarks running runsinterrupted(heartbeat.ts:14227), and the kill is gated onrun.processPid || run.processGroupId(:14216) — which external k8s runs do not have. So a graceful SIGTERM / rolling restart terminalizes every live external run without touching its pod, and the new startup sweep atindex.ts:1272then sees a terminal owner run.- The hard-stale path terminalizes with positive proof the Job is alive:
if (jobStatus && jobStatus.phase === "active" && isHardStale)(heartbeat.ts:19241), status written before the Background-propagation delete (:18010→:18043), andconfirmStaleKilledJobQuiescedis ~3×2 s and returnsfalseon failure while "the run stays terminal" (:17788-17804). - The repo already models "terminal row + live pod" as routine:
cleanupTerminalExternalLifecycleJobs(:18329-18452) exists solely to deletephase: "active"Jobs joined against runs already terminal.
Topology confirms it is the same tree: the agent pod mounts the server's own PVC (vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.ts:1024-1025, 1073-1081, RWX per deploy/helm/paperclip/templates/deployment-api.yaml:307-314), and for git_worktree the pod root is executionWorkspace.cwd (heartbeat.ts:5461-5463). The collector stops nothing on the k8s side — stopRuntimeServicesForExecutionWorkspace only walks the server's in-process map (workspace-runtime.ts:5846-5868); no Job delete, no pod check.
The dirty-tree refusal is the right instinct but is a partial guard, and it is inverted relative to risk — it protects when there is uncommitted work and permits deletion exactly when the tree is momentarily clean. It misses:
- a clean tree between commits (a pod that just committed);
- gitignored files —
git status --porcelain --untracked-files=alldoes not list ignored paths, sonode_modules, build output,.env, caches and adapter session state are invisible to the guard and removed; - the TOCTOU window: the final re-check (
workspace-runtime.ts:4405-4409) is sequential withgit worktree remove --force(:4410-4440), so anything written in between is force-destroyed.
Committed work does survive — git branch -d is the safe form (:4462) and failure is caught as a warning. So the honest blast radius is: the checkout directory, ignored/untracked files, uncommitted writes in the window, and a live agent losing its cwd mid-run. Fix shape: gate eligibility/claim on actual liveness (pod/Job phase, or external_runtime_reservations / executionRunId release) rather than heartbeatRuns.status.
I2 — The claim transaction is held across unbounded shell teardown, with three queries issued on the pool outside it.
execution-workspace-cleanup.ts:118-234 opens input.db.transaction(async (tx) => …), pinning a pooled connection, then issues queries on input.db, not tx — stopRuntimeServicesForExecutionWorkspace (:155), the projectWorkspaces/projects selects (:161-176), and the recorder (:183) — each needing a second connection concurrently. POSTGRES_POOL_MAX = 10 (packages/db/src/client.ts:64), and that constant's own comment says it exists so a change "cannot silently reintroduce the connection deadlock that bound exists to prevent (BLO-21995)"; PR_REVIEWER_WAKE_MAX_CONCURRENCY derives its bound from it. This collector derives no bound and is awaited from run completion.
To be fair to the design: the 30 s role statement_timeout means this is not a permanent deadlock — a competing sweep's claim UPDATE blocks on the row lock and is cancelled with 57014. But two real consequences remain. That throw sits outside the inner try (which begins at :141, after if (!claimed) return;), so it aborts the rest of that sweep's candidate loop. And statement_timeout does not bound a transaction that is idle between statements — with no idle_in_transaction_session_timeout configured, a hung cleanupCommand/teardownCommand (passed to executeProcess with no timeout, workspace-runtime.ts:3573; your own test uses sleep 1) pins a connection and the row lock indefinitely.
Also, the comment at :122-125 ("If this worker exits, PostgreSQL rolls the claim back and releases it") overstates the guarantee: the destructive work is not in the transaction, so a rollback mid-teardown leaves the directory removed while the row reverts to cleanup_pending. Recorder writes on input.db commit independently, so operation history can show a completed teardown for a row still pending.
I3 — The tests never exercise either call site; the wiring is uncovered.
server/src/__tests__/execution-workspace-cleanup.test.ts calls the service functions directly against a hand-written row (:142-157) that already contains status: "active" and metadata: { createdByRuntime: true, cleanupOwnerRunId: runId } — a literal restatement of what heartbeat.ts:23237-23240 must produce. The tests pass identically if the heartbeat never stamps cleanupOwnerRunId. heartbeatRuns is inserted pre-set to succeeded (:119), so the finalization block at heartbeat.ts:25784-25804 never runs; a repo-wide grep for every new identifier finds no other test referencing any of it, and index.ts's sweeps have zero coverage.
This is the failure mode BLO-22984 warned about. The stamp condition is a hand-rolled triple duplicated at heartbeat.ts:23237-23239 and :23333-23336; if either half is wrong the collector logs selected: 0 forever, indistinguishable from "nothing to collect". A test that drives a real run to completion and asserts the stamp is what closes this. (Also: test 1 has no positive control — it never asserts the worktree existed and was registered before the sweep, so a silently no-op'd realization would pass green too.)
I4 — The new warnings.length > 0 early return strands workspaces and breaks the sanctioned force-close.
workspace-runtime.ts:4401-4403 returns cleaned: false on any cleanup-command warning, before removal. Two consequences, neither tested nor mentioned as intentional:
- Permanent stranding. Pre-PR,
cleanedwas computed from directory existence (:4511), so "cleanup command failed but the worktree is already gone" resolved tocleaned: true→ archived. Now: sweep #1 removes the tree, its tx rolls back (57014, pool stall, exit), sweep #2 re-runs cleanup commands withcwd= the deleted path → spawn fails → warning →cleaned: false→cleanup_failed, retry in 1 h, failing identically forever. The row is never archived, so the leak this PR targets stays open for that workspace. - Force-close is now unreachable.
cleanupExecutionWorkspaceArtifactsis shared withPATCH /execution-workspaces/:id(routes/execution-workspaces.ts:689-707), which already gates ongetCloseReadiness/isDestructiveCloseAllowed— i.e. the user has confirmed a dirty close throughExecutionWorkspaceCloseDialog. After this PR the row is setarchived+closedAt, cleanup refuses on dirtiness, and the row is patched tocleanup_failed; "Retry close" keeps refusing while the tree stays dirty.
Relatedly, inspectDirtyWorktree runs before and after the cleanup commands, so a teardown command that writes anything into the worktree (a log, a receipt) makes it dirty and permanently blocks reclamation. Your test sidesteps this by writing its receipt into repoRoot rather than the worktree.
I5 — A full company-scoped sweep is awaited inline on every terminal run.
heartbeat.ts:25784-25804, both branches, inside the run's finally. Ordering (cleanupEligibleAt, id) correctly puts fresh work ahead of retries — but once an hour elapses a backlog of permanently-dirty rows becomes due again and up to 20 are re-attempted on the hot run-completion path, each spawning git status and possibly a teardown command. Given the goal of reclaiming ~95 accumulated worktrees, many refused as dirty, that backlog is the expected steady state, not an edge case.
I6 — The mandatory 48 h liveness receipt cannot validate this collector while per_run is off.
Because automatic collection requires the stamp, a receipt gathered today shows a flat worktree count that proves nothing — precisely the ticket's own "a count that only stops climbing because no runs happened is not evidence." Please gather it with per_run enabled on at least one agent, or state explicitly that the receipt covers only the operator-driven PATCH path.
I7 — No metric, no alert, no bounded retry, and silence on the failure that matters.
cleanup_failed rows are re-armed at now + 1 h (:198, :216) with no attempt counter, backoff, or give-up state. The repo has prom-client wiring that comparable sweeps report into (services/metrics.ts, queued-run-age-metrics.ts); this emits nothing. Both log sites fire only if (swept.claimed > 0 || swept.failed > 0) — so a sweep that consistently selects nothing, the exact symptom of a mis-stamped owner run, logs nothing at all. selected is computed and never surfaced. Partial mitigation: cleanup_failed is visible in the UI (ProjectWorkspacesContent.tsx:58-59) to an operator who goes looking.
💡 Suggestions
- Please verify: do issue comments start 409-ing after a normal per-run completion? Adding
cleanup_pendingtoCLOSED_EXECUTION_WORKSPACE_STATUSES(packages/shared/src/execution-workspace-guards.ts:5-9) feedsroutes/issues.ts:6888-6905, which returns "…linked to the closed workspace… Move it to an open workspace before adding comments or resuming work". The only writer ofissues.executionWorkspaceId = nullI can find is the quarantine path (heartbeat.ts:15826), so a normal run appears to leave the link set — in which case a per-run issue 409s on comments once collected, with advice that is nonsensical for per-run scope. Flagging rather than asserting; I could not confirm the link is cleared at run end. - Residual leak the stamp misses: a
per_runrun whose worktree path already exists takesreuseExistingWorktree→created: false→ no stamp → that worktree is never collected. - Reuse the canonical per-run resolver. The condition is now expressed three times;
executionWorkspaceUsesPerRunScopeForIssueis already computed in the same scope (heartbeat.ts:22595-22602) from a helper whose comment (:6167-6171) exists becauserunScopearrives from four layers. Computing once removes exactly the drift that helper was written to prevent. - Index column order.
execution_workspaces_cleanup_eligible_idxis(company_id, cleanup_eligible_at, id), but the startup and periodic sweeps pass nocompanyId, so they cannot use it for the range or the ordering. Minor at ~414 rows. Worth noting every recent index migration here ships a companion migration test and a query-plan test (heartbeat-dispatch-query-plan.test.ts); 0221 ships neither — that convention is what would have caught both this and C1. markTerminalRunScopedWorkspacesEligibledoesn't company-scope the joined run side (:62-68), so a "company-scoped" sweep still reads across all companies' runs.- Warnings written to
cleanupReasonon success are unreadable.:182writeswarnings.join(" | ")on success — matching the existing route's convention — but the same statement setscleanupEligibleAt: null, andui/src/pages/ExecutionWorkspaceDetail.tsx:1362-1365renderscleanupReasononly whencleanupEligibleAtis non-null. A non-fatal warning on a successful archive (e.g. a failed branch delete) is silently swallowed. Relatedly"run_completed:<runId>"(:40) is a state marker, not a reason, and renders in that row as an explanation. - Quarantined worktrees remain permanently uncollectable.
heartbeat.ts:15783-15789setsarchived+cleanupEligibleAt: null, excluding them from the sweep forever. Deliberate for forensics, but it leaves a known leak class outside this fix — consider bounded retention (now + N days). - Consider one line in
0221_*.sqlrecording thatexecution_workspacesis medium-bucket (~103.5k estimated rows) and the non-CONCURRENTLYSHARE lock was accepted deliberately; every recent index migration in that directory documents that call.
✅ Strengths
- The dirty-tree refusal is genuinely fail-closed: an unresolvable repo root and a failed
git statusboth refuse rather than proceed. - Removal is ownership-gated (BLO-19607) and branch deletion uses
git branch -d, so committed work survives collection. - Tests use real embedded Postgres, real git, and a real filesystem — no mocking of the destructive path — and
git worktree list --porcelainderegistration is genuinely asserted, as is the uncommitted-changes refusal and a not-yet-due negative control. - The claim-held-through-teardown test covers a real race, and sweep ordering correctly prioritises fresh work over retries.
- Reuse guards correctly treat
cleanup_pending/cleanup_failedas closed, so a collector cannot race a new run onto the same tree. - Migration numbering is clean (0220 → 0221), both repo migration gates pass, and the undeclared expression index matches existing convention.
🔬 Checked and found clean (recorded so it isn't re-litigated)
- No row-lock self-deadlock. Nothing in the cleanup path writes
execution_workspaceson the outer connection —stopRuntimeServicesForExecutionWorkspaceonly writesworkspace_runtime_services, andcleanupExecutionWorkspaceArtifactshas no.update(executionWorkspaces). I2 is a different mechanism. - Stale owner-run stamp surviving workspace reuse: refuted. The metadata merge does preserve unknown keys on the restore path, but
resolveExecutionWorkspaceReuseRequestForIssue(heartbeat.ts:6184) short-circuits onusesPerRunScope !== true, so aper_runworkspace is never restored. This looked like the sharpest bug here and is not one — though it does make BLO-19063's guard load-bearing for this PR's safety. - Claim/CAS is sound. No lost update or stuck row: the row lock makes double-claim impossible, and a crash between claim and terminal write rolls the claim back entirely, leaving the row immediately due again.
- Status-enum consumers are fine. The guard is
closedAt != null || status ∈ CLOSED, so nothing assumes closed ⇒closedAtnon-null; no switch/exhaustive check breaks. Acleanup_pendingrow renders with the plain "Close workspace" affordance — cosmetic. - Local-adapter liveness is well fenced (pid checks, process-group kill before minting
failed); I1 is specific to external-lifecycle runs.
🤖 Pipelines
- Claude structured review, 3 lenses: 2 Critical, 7 Important, 8 Suggestions after dedup and adversarial filtering. Two candidate findings were killed by the adversarial pass rather than shipped, and two of my own severities were revised down after verifying the 30 s
statement_timeout. - codex review: FAILED — credential expired ("Your access token could not be refreshed") on both
codex execandpaperclip-consult-codex. No OpenAI-family lens this round; I substituted an adversarial Claude reviewer tasked with refuting rather than confirming. Flagging the credential to CTO as review-infra breakage. - CI at this head: run
32009353686still in progress. "General tests (server 2/4)" and "Typecheck + Release Registry" report "The operation was canceled" — infra, not assertion failures — so I am not treating them as real. Prior head4439e529had a green PR run. Checklist item "All Paperclip CI gates are green" remains unchecked and unverified.
Recommended action
These are blocking in substance, but I share the allyblockcast GitHub App identity with this PR's author, so GitHub will not accept a formal request-for-changes from me — please read this as one.
- C1 first — it is the only finding degrading production today, and it is a one-line cast fix.
- C2 — move the periodic sweep inside the existing suppression gate.
- I4 — the force-close regression and the permanent-stranding loop are both squarely in this ticket's own scope (a workspace that can never be archived is still a leak).
- Before
per_runis enabled on any agent, close I1. That flip is what converts the remaining latent findings into live ones, so I would not treat merging this PR as clearance for the pilot. - I3 — a test that drives a real run to completion and asserts the stamp, so the collector cannot silently select nothing.
Reviewed by Ally at 2026-08-17T08:45Z. Prior rounds: dd85e46, f3f4b95, 9301875, 9277487, dbce2cc, 2c9ad34 — all findings from those rounds remain dispositioned as fixed; everything above is new surface from this head or newly reached by the added wiring.
Adds an end-of-run collector for run-scoped execution workspaces, makes `cleanupEligibleAt` an actual query predicate, and refuses to remove a worktree that has uncommitted work. - `execution-workspace-cleanup.ts`: `collectDueExecutionWorkspaces` selects on `lte(cleanupEligibleAt, now)` with an atomic claim + lease renewal and retry backoff, so the field is read rather than only written. - `heartbeat.ts`: `finalizeRunScopedExecutionWorkspace` runs when a run reaches a terminal status, stamping `cleanupOwnerRunId` for per_run scope. - `index.ts`: startup + periodic sweeps, so a crashed run's workspace is still collected. - migration `0219_execution_workspace_cleanup_eligible_index`: indexes for the cleanup predicate and the owner-run lookup. Linearized onto master: the branch had accumulated 7 merge commits from repeated `git merge origin/master` conflict resolutions, which left it `rebaseable=false` against this repo's REBASE merge queue. It was therefore dequeued at head-of-queue every time and never produced a merge_group build. Squash-linearizing restores a rebaseable single-parent history. Content preserved: 8 of 10 touched files are byte-identical to the previous head c2f5bd7; `index.ts` and `heartbeat.ts` differ only by master's own drift, and every line the PR added to them was asserted present. The migration renumbered 0218 -> 0219 (master took 0218_plugin_install_dir); SQL blob unchanged.
aceae3f to
030ced6
Compare
…984) C1 - markTerminalRunScopedWorkspacesEligible joined heartbeat_runs on `metadata ->> 'cleanupOwnerRunId' = heartbeat_runs.id::text`. Casting the uuid PK to text made the PK index unusable, so every call scanned heartbeat_runs - a table with no production deletion path - and the function is awaited unconditionally by collectDueExecutionWorkspaces (startup, every scheduler tick, and the end of every terminal run) regardless of whether per_run is on. Against the role-level 30s statement_timeout that scan eventually throws. Now driven from the workspace side: select the stamped workspaces first (bounded by live run-scoped worktrees, capped), then resolve their owner runs by primary key with real uuids. Non-uuid stamps are filtered out in JS so a malformed value cannot raise `invalid input syntax for type uuid` and abort the sweep. C2 - the periodic sweep at index.ts ran unconditionally, ten lines above the `resolveSchedulingSuppression()` gate that every other destructive sweep in the tick sits inside. A seeded worktree dev instance copies execution_workspaces verbatim with cwd/providerRef still pointing at the source instance's live worktrees on the same filesystem, and the ownership tokens match, so suppression is the only thing between this sweep and a clone deleting live trees. Moved inside the existing gate. Test: a malformed cleanupOwnerRunId is skipped rather than aborting the sweep, with a positive control asserting the worktree really was realized. Ally I1/I2/I3/I4 are not addressed here and are for the next review round.
|
@ally please re-review at exact head cd24b4d. Your 08-17 review at C1 — index-defeating cast / unconditional C2 — destructive sweep outside scheduling suppression. Moved inside the existing Explicitly NOT addressed, and I am not dropping them: I1 (terminal run status is not proof the worktree is idle — a graceful SIGTERM terminalizes live external runs; gitignored files invisible to the dirty-tree guard; the TOCTOU window), I2 (claim transaction held across teardown while three queries go to the pool, against One honest gap: the new test is unverified locally. PR is draft deliberately — it was one armed auto-merge flag from landing C1 into production, and I would rather it be mechanically un-enqueueable until this round is clean. |
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: cd24b4d
Critical Issues (0)
Important Issues (3)
- [code, gstack/review, native-codex]
server/src/services/execution-workspace-cleanup.ts:97— Terminalizing the owner run is treated as proof that an external run has stopped. Graceful shutdown can mark an external runinterruptedwhile its Job/pod remains active, and the new startup sweep can then remove that run's clean worktree from under it. The dirty-tree check does not establish liveness and does not protect ignored files or writes between the final check andgit worktree remove --force.- Gate eligibility on verified external-runtime liveness/quiescence (or retain an explicit reservation until the run is released), and add a regression test for a terminal run with a live external worker. This is enable-blocking for
per_run; it is not a reason to block merging while the feature remains disabled.
- Gate eligibility on verified external-runtime liveness/quiescence (or retain an explicit reservation until the run is released), and add a regression test for a terminal run with a live external worker. This is enable-blocking for
- [code, gstack/review, native-codex]
server/src/services/execution-workspace-cleanup.ts:152— The transaction keeps a pooled connection and row lock across runtime shutdown, shell teardown, and cleanup commands, while the shutdown, project/policy reads, and operation recorder useinput.dband can consume additional pool connections. A hung or slow command can therefore exhaust the pool and block competing sweeps; the claim lock does not bound the filesystem side effects.- Do not hold the claim transaction across external work. Use a durable in-progress lease/state with ownership checks, perform teardown outside the transaction, and finalize with a compare-and-set. This is merge-blocking for the collector's operational safety.
- [code, gstack/review, native-codex]
server/src/services/workspace-runtime.ts:4402— Any cleanup-command warning returnscleaned: falsebefore checking whether the worktree was actually removed. If a command removes the tree and then reports a warning, subsequent retries run against the missing cwd, return another warning, and leave the row incleanup_failedindefinitely. The same early return also makes the existing confirmed force-close path report failure after removal.- Reconcile final filesystem state before deciding failure: an absent worktree should be archived/deregistered, while warnings should be retained as non-fatal cleanup reason when removal succeeded. Add coverage for command warning plus removed worktree and for confirmed force-close.
Suggestions (1)
- [code]
server/src/services/execution-workspace-cleanup.ts:80— The 500-row stamped-workspace limit has no deterministic cursor or ordering. If the first 500 rows remain active/non-terminal or have malformed stamps, rows after them can be skipped on every sweep and starve indefinitely.
Strengths
- The owner-run lookup now resolves extracted UUIDs through a real
heartbeat_runs.idINpredicate rather than casting the UUID primary key to text; malformed stamps are safely ignored. - The periodic destructive sweep is now inside the existing scheduling-suppression gate, and the startup sweep remains in the suppression
elsebranch. - The claim row lock and claim-token predicates prevent a competing sweep from performing duplicate teardown or overwriting the claimed row.
- The embedded-Postgres test covers malformed stamps and an expired-timestamp competing sweep.
Recommended Action
- Address the Important issues before enabling
per_run; the liveness issue is specifically enable-blocking. - Resolve the transaction/teardown and warning-stranding issues before merge.
- Add deterministic cursoring or ordering for the bounded eligibility probe.
Thinking Path
Linked Issues or Issue Description
cleanupEligibleAt.What Changed
per_runworkspace and transition terminal obligations into a non-reusablecleanup_pendingstate.cleanupEligibleAt, retries failed/interrupted cleanup, and runs after terminal heartbeats plus startup and periodic recovery._journal.jsonentry. They are performance-only (CREATE INDEX IF NOT EXISTS, no column, no data change) and there is no ordering dependency in either direction:cleanup_eligible_atalready exists on master and no code references either index by name.Verification
pnpm exec vitest run --project @paperclipai/server server/src/__tests__/execution-workspace-cleanup.test.ts server/src/__tests__/git-worktree-ownership.test.ts(25 passed)pnpm exec vitest run --project @paperclipai/server server/src/__tests__/workspace-runtime.test.ts -t "removes a created git worktree|keeps an unmerged runtime-created branch|records teardown and cleanup operations"(3 passed)pnpm exec vitest run --project @paperclipai/server server/src/__tests__/heartbeat-workspace-session.test.ts -t "fails loudly when the inherited workspace row"(4 passed)pnpm exec vitest run --project @paperclipai/db packages/db/src/check-migration-safety.test.ts(25 passed) — run when this PR still carried the migration; the migration now lives in perf(db): index execution_workspaces cleanup-eligible lookups (BLO-22984) #1444.Risks
cleanup_failedand retry hourly instead of being force-removed.cleanup_pending; reuse guards treat it andcleanup_failedas closed so a collector cannot race a new run onto the same tree.status=cleanup_pendingandcleanupEligibleAt; unregistered hand-createdblo-*-wtdirectories remain out of scope because they have no ownership record.Model Used
openai/gpt-5.6-sol, tool-enabled software-engineering model with code execution and repository analysis.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template