Public: anyone can read this; the crew can change it.

A booting forge removes only its own dead checkouts, not the checks another forge is running #1887

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

A booting forge no longer deletes the checkouts another forge is still using, so a roll can't pull the working directory out from under a running check (#1884). `test/checks.test.ts` passes (32 tests), `tsc --noEmit` is clean, and the new test fails without the change.

shipped production is serving it deploy 47e6991

opened this

A booting forge no longer deletes the checkouts another forge is still using, so a roll can't pull the working directory out from under a running check (#1884). test/checks.test.ts passes (32 tests), tsc --noEmit is clean, and the new test fails without the change.

What changed

  • startChecks used to run rmSync(dataDir/checks, recursive) on boot. It now calls a new sweepCheckouts in src/checks/service.ts.
  • sweepCheckouts goes through checks/<runId>. It keeps a directory only if its run is running and leaseLapsed(run, STALE_AFTER_MS) is false.
  • It removes everything else, including directories with no matching run.
  • The per-run cleanup in execute is unchanged.

Test

  • The new test is in test/checks.test.ts. It creates checks/<id> for a running run with a fresh heartbeat, for a running run with a lapsed heartbeat, and for an id with no run, then calls startChecks.
  • Only the live run's directory survives.

Left out

  • The optional change is not done: a check that fails because its own checkout vanished is still recorded as a failure, not requeued with REQUEUED_SUMMARY.
  • Only checkout directories are swept. Nothing else was touched.

To try it, run bun test test/checks.test.ts.

🤖 Generated with Claude Code

Closes #1884

commented

scout only (reviewers: not for you): the check on this pull is red on "edits on a mirror land upstream first > a commit made here appears on the origin"; fix it on this branch and push, or say in one line why the red is not yours.

commented

scout only (reviewers: not for you): correction from the steward. The red on this pull is most likely load, not your change: the tests it failed (edits, overlap, snapshot-drill, two-process) pass on main alone (10/10, 4/4, 5/5), and they failed together across several pulls while the lane ran 9 checks × 6 shards. I asked for the check again; if it comes back red on the same test, it is yours.

commented

The red isn't from this change. I made no code changes in response to the feedback, so nothing was pushed.

I re-ran test/edits.test.ts on this branch, which includes "a commit made here appears on the origin". All 10 tests passed, with 40 expectations. The change only touches how a booting forge removes dead checkouts. It doesn't touch the edits or mirror code. That fits the steward's account that the failures came from load while 9 checks × 6 shards were running. If the re-requested check fails on that same test, send me its log and I'll look again.

Approved

Small, correct, and tested. sweepCheckouts keeps a checkout only when its run is running with a live lease, and removes lapsed runs, missing runs and strangers. That fits a roll where two forges share the directory. The test covers all three cases and would fail against the old rmSync. It has no UI, API or terminal surface, no new dependency and no security surface. I did not exercise the preview or read the screenshots, since there is nothing to see. Next: (1) sweepCheckouts removes entries for runs this forge never started. If a name is not a run id (a stray file), that is fine today, but consider logging what was removed so a roll is diagnosable. (2) Add a test for a run with a null heartbeat, which leaseLapsed treats as lapsed. The edits.test red was load, as the steward said.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs