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

A roll does not throw away running checks: drain or hand them over before the forge restarts #1820

Merged opened this in answer to

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

Merged

A roll now waits for running checks before it cuts them off, and a pull that merges mid-check ends that check as superseded rather than failed. `bun test test/checks.test.ts` passes (29 tests). `bun x tsc --noEmit` could not run here: it stops on a missing `bun` type definition, which comes from the sandbox, not from this change.

shipped production is serving it deploy 8c46a8d

opened this

A roll now waits for running checks before it cuts them off, and a pull that merges mid-check ends that check as superseded rather than failed. bun test test/checks.test.ts passes (29 tests). bun x tsc --noEmit could not run here: it stops on a missing bun type definition, which comes from the sandbox, not from this change.

What changed

  • Draining (src/checks/service.ts, src/main.ts): ChecksOptions has a new drainMs. On SIGTERM, stop() starts nothing new and waits up to drainMs for checks already running to finish. Only the ones still running after that are aborted and queued again, as before.
  • Setting: main.ts sets the budget from FORGE_CHECKS_DRAIN_S, default 300 seconds. drainMs itself defaults to 0, so the existing "queued again when the forge stops" test still holds. The new variable is documented in docs/APPS.md.
  • Merge mid-check: when a pull merges or closes, pull.merged and pull.closed already cancelled the run row and aborted its check. The check's own log then still ended with failed: …, and that is what the audit counted as failed. Now execute sees the row is already cancelled and writes superseded: <reason> instead of the failure line. The row stays cancelled with its summary ("pull landed" or "pull closed").
  • Tests:

- A check that sleeps 2 s is running when stop() is called with drainMs 30 s. It finishes ok and nothing is requeued. - A running check has its pull cancelled with "pull landed". Its log ends with superseded: pull landed, has no failed: line, and the row stays cancelled.

What I left out

  • Handing checks to the new process (#1432): I did not build it. Draining covers the roll case without it.
  • Wait vs. platform kill: the 300-second wait is a guess. If the platform kills the old process sooner, the wait never completes and the cut-off checks are requeued as before. Raise or lower it with FORGE_CHECKS_DRAIN_S.
  • Merge-gate runs: a full-tier gate run for a merge in progress is still aborted immediately on stop, after the drain wait. It is not waited on separately.
  • Merge-mid-check test: the test cancels the run row directly instead of going through a real mergePull. That is the same state the event handler produces, but it does not exercise the event path.

Try it

  • Run bun test test/checks.test.ts.
  • On a deployed forge, send SIGTERM while a check runs. The check completes, and the process exits after it.

Closes #1796

Approved

Approved. A roll now lets running checks finish (FORGE_CHECKS_DRAIN_S, default 300 s, nothing new claimed once stopped) before cutting off and requeueing the rest. Two tests cover it, and the stop path is still reached from shutdown() in main.ts. There is no UI surface, so nothing to look at on the preview. Two things I would do next: (1) confirm the platform's termination grace period is at least 300 s plus the linger, otherwise the process is SIGKILLed mid-drain and the drain does nothing. Consider logging 'draining N checks' so the wait is visible in the logs. (2) The 'superseded' branch (a cancelled run is no longer reported as failed) is a separate behaviour from the drain. It is tested, but it belongs in its own pull next time.

commented

Parked by the crew: the check failed ("the look > a green deploy is pictured at all three widths, once per head — phone and desktop as the owner, the thumb as a stranger, reduced from the desktop page; a newer head replaces") and the decider saw no point in another try

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs