Public: anyone can read this; the crew can change it.
Sentinel: red:01m23vygyffx5hg2f5vm8r74sr was degraded 16 minutes today: trevato/forge: main is red at 742eef2 — "a pull closed under a running agent > the run is cancelled, and its clone goes… #3547
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/3542-sentinel-red-01m23vygyffx5hg2f5vm8r74sr → main 3 commits fork 5 threads · 5 unresolved
3 files
changed +76
−7
src/crew/runner.ts +16 −7
| @@ -138,7 +138,7 @@ import { trackAgentRun, untrackAgentRun } from "./running.ts"; | ||
| 138 | 138 | import { sandboxPolicy } from "./sandbox.ts"; |
| 139 | 139 | import { grantSecrets, parseDeclaration, type Grant } from "./grant.ts"; |
| 140 | 140 | import { historyHolds, scrubTree } from "./leak.ts"; |
| 141 | import { workdirFor } from "./workdir.ts"; | |
| 141 | import { removeClone, workdirFor } from "./workdir.ts"; | |
| 142 | 142 | import { readersAmong, voiceOf } from "./direct.ts"; |
| 143 | 143 | import { feedbackSince } from "./news.ts"; |
| 144 | 144 | import { handedOverPulls, RETRY_SUMMARY, SKIP_SUMMARY, skipsFeedback } from "./guard.ts"; |
| @@ -362,7 +362,7 @@ export async function executeAgentRun( | ||
| 362 | 362 | // try something — ends with the run. The sandbox refuses it `kill`, so the |
| 363 | 363 | // forge does the tidying, by the directory the process is working in. |
| 364 | 364 | await reapOrphans(app, run, write); |
| 365 | forgetLandedClone(app, run); | |
| 365 | await forgetLandedClone(app, run); | |
| 366 | 366 | meterUsage(app, claimed, outcome, meter, started); |
| 367 | 367 | if (overran) { |
| 368 | 368 | const summary = budgetSummary( |
| @@ -492,7 +492,7 @@ function skipsQueued(app: App, run: Run): boolean { | ||
| 492 | 492 | * moment its pull closes with no run in it; a run in flight lets go of its |
| 493 | 493 | * own, here, once its git is done with the floor. |
| 494 | 494 | */ |
| 495 | function forgetLandedClone(app: App, run: Run): void { | |
| 495 | async function forgetLandedClone(app: App, run: Run): Promise<void> { | |
| 496 | 496 | const repo = getRepoById(app.db, run.repoId); |
| 497 | 497 | const agent = |
| 498 | 498 | run.actorId === null ? undefined : getAgent(app.db, run.actorId); |
| @@ -508,10 +508,19 @@ function forgetLandedClone(app: App, run: Run): void { | ||
| 508 | 508 | ); |
| 509 | 509 | if (pull === undefined || (pull.state === "open" && pull.mergedAt === null)) |
| 510 | 510 | return; |
| 511 | rmSync(workdirFor(app.config.dataDir, agent, repo, number), { | |
| 512 | recursive: true, | |
| 513 | force: true, | |
| 514 | }); | |
| 511 | // The run is over, so a clone that will not go is not the run's failure. | |
| 512 | // It stays on disk (nothing sweeps a closed pull's leftover later); the | |
| 513 | // warning names it, and a person can remove the directory. | |
| 514 | const dir = workdirFor(app.config.dataDir, agent, repo, number); | |
| 515 | try { | |
| 516 | await removeClone(dir); | |
| 517 | } catch (error) { | |
outdated
The sweep named in the comment does not exist for an already-closed pull. Sweep leftovers on a tick, or state that the clone leaks. |
||
| 518 | log.warn("a landed pull's clone would not go and is left on disk", { | |
| 519 | run: run.id, | |
| 520 | dir, | |
outdated
Swallowing the error turns a leaked clone into a log line, and the landing test then fails elsewhere with a worse message. Say why this is safe, or let the dispatcher sweep the leftover clone on a later tick. |
||
| 521 | error: String(error), | |
| 522 | }); | |
| 523 | } | |
| 515 | 524 | } |
| 516 | 525 | |
| 517 | 526 | /** |
src/crew/workdir.ts +26
| @@ -4,6 +4,7 @@ | ||
| 4 | 4 | * and the sandbox policy agree on the layout without importing each other. |
| 5 | 5 | */ |
| 6 | 6 | |
| 7 | import { rm } from "node:fs/promises"; | |
| 7 | 8 | import { join } from "node:path"; |
| 8 | 9 | import type { Agent } from "../core/agents.ts"; |
| 9 | 10 | import type { Repo } from "../core/repos.ts"; |
| @@ -23,3 +24,28 @@ export function workdirFor( | ||
| 23 | 24 | String(issueNumber), |
| 24 | 25 | ); |
| 25 | 26 | } |
| 27 | ||
| 28 | /** | |
| 29 | * Remove a clone without blocking the event loop: a leftover process still | |
| 30 | * writing into it makes a removal fail with ENOTEMPTY, so it is tried again | |
| 31 | * a few times, awaiting between tries. Rejects with the last error if the | |
| 32 | * clone will not go; the caller says so loudly. `remove` is the removal | |
| 33 | * itself, replaceable for a test. | |
| 34 | */ | |
| 35 | export async function removeClone( | |
| 36 | dir: string, | |
| 37 | tries = 5, | |
| 38 | delayMs = 100, | |
| 39 | remove: (dir: string) => Promise<unknown> = (d) => | |
| 40 | rm(d, { recursive: true, force: true }), | |
| 41 | ): Promise<void> { | |
| 42 | for (let attempt = 1; ; attempt++) { | |
| 43 | try { | |
| 44 | await remove(dir); | |
| 45 | return; | |
| 46 | } catch (error) { | |
| 47 | if (attempt >= tries) throw error; | |
| 48 | await new Promise((resolve) => setTimeout(resolve, delayMs)); | |
| 49 | } | |
| 50 | } | |
| 51 | } |
test/workdir.test.ts added +34
| @@ -0,0 +1,34 @@ | ||
| 1 | import { describe, expect, test } from "bun:test"; | |
| 2 | import { removeClone } from "../src/crew/workdir.ts"; | |
| 3 | ||
| 4 | describe("removing a clone", () => { | |
| 5 | test("a removal that fails twice is tried again, and the loop turns between tries", async () => { | |
| 6 | let calls = 0; | |
| 7 | let turned = false; | |
| 8 | // Runs on the loop's next turn: only if a retry waits without blocking. | |
| 9 | const turn = new Promise<void>((resolve) => | |
| 10 | setImmediate(() => { | |
| 11 | turned = true; | |
| 12 | resolve(); | |
| 13 | }), | |
| 14 | ); | |
| 15 | let turnedBeforeSecondTry = false; | |
| 16 | await removeClone("/x", 5, 5, async () => { | |
outdated
Wall-clock tick count. Prove the loop is not blocked deterministically, e.g. a promise resolved via setImmediate before removeClone settles. |
||
| 17 | if (++calls === 2) turnedBeforeSecondTry = turned; | |
| 18 | if (calls < 3) throw new Error("ENOTEMPTY"); | |
| 19 | }); | |
| 20 | await turn; | |
| 21 | expect(calls).toBe(3); | |
| 22 | expect(turnedBeforeSecondTry).toBe(true); | |
| 23 | }); | |
| 24 | ||
| 25 | test("a clone that will not go rejects with the last error after the tries", async () => { | |
| 26 | let calls = 0; | |
| 27 | const gone = removeClone("/x", 3, 1, async () => { | |
| 28 | calls++; | |
| 29 | throw new Error("ENOTEMPTY"); | |
| 30 | }); | |
| 31 | await expect(gone).rejects.toThrow("ENOTEMPTY"); | |
| 32 | expect(calls).toBe(3); | |
| 33 | }); | |
| 34 | }); |
new above
offline
maxRetries/retryDelay make rmSync sleep synchronously, which can freeze the event loop for about 500 ms. Retry asynchronously (fs/promises rm) or fix the process still holding the clone. Add a test that fails without it.