Repository navigation
fix(run): clear a terminal worker signal on every worker-launch replay - #2238
Open
abretonc7s wants to merge 3 commits into
Open
abretonc7s wants to merge 3 commits into
abretonc7s wants to merge 3 commits into
Conversation
`mark start` refuses to re-open a `complete` or `no-change` signal, on the grounds that a legitimate re-dispatch clears it first. That clearing only ran for a native fresh restart, so on tmux transport a replay of prepare or dispatch left the finished signal in place and the replayed worker met a refusal it had no way past. The comment in mark-checklist-step.cjs asserting otherwise was wrong, and is corrected here. Only a terminal signal goes. A `running` signal belongs to a worker that may still be alive and is preserved until admission replaces it — the behaviour replay-signal-preservation.test.ts already pins — and `blocked` stays because `start` resumes it and keeps the checked boxes. So a non-native replay reads each signal and removes only the two statuses the start guard refuses. A native fresh restart has already established that no previous worker survives, so it still clears whatever is there. Two tests, one per terminal status, both red before the change: the signal survived the replay with its `complete`/`no-change` status intact.
Review r1. The gate demanded a prepare step, so `update-branch` — a flow with none, whose worker is required to call `mark start` — kept the strand this change exists to remove. The rule is now any replay that ends in a worker launch: a target at or before dispatch. `replaysWorkerLaunch` keeps its own meaning for the metrics reset rather than being widened under it. `mark no-change` writes status `complete` with a disposition (resolveTerminalPreset), never a `no-change` status, so the filter tested a value no worker writes. It now matches on `complete` alone, through `isStartRefusedWorkerSignalStatus` beside the terminal predicate it is deliberately narrower than, and the same dead value is gone from the #2232 start guard — whose refusal now names the disposition, so a no-change run says why it finished. The read goes through the slot-files service, which already reads a missing, unreadable or directory path as empty, instead of a second copy of the cat and parse beside `readAdoptableTaskSignal`. Deciding and deleting are now separate: a numbered `./mark N` can rewrite a finished signal back to `running` in between, so the delete re-reads each file and unlinks only while it still says `complete`. It runs after the ownership assertion, because an `rm` that lands before a thrown assertion leaves a run short of a signal with no step reset behind it. A replay at or before write-task was the reviewer's other example and does not strand: it clears `taskFile` and the task regenerates into its own timestamped directory, so the replayed worker never sees the old signal. Pinned as such.
Review r2. The delete read a file and then unlinked its path, and `mark`
publishes a fresh signal over that path by rename (mark-io.cjs), so an unlink
by name could take the `running` signal of a worker that marked a step in
between. Rename is the one atomic step available: the candidate is moved
aside, only the bytes that moved are judged, and a snapshot that turns out to
be live goes back — or is dropped when the worker has already written its own
over the original path. The delete reports what it removed, so a failed unlink
is no longer logged as cleared.
The status lives in one place as `START_REFUSED_WORKER_SIGNAL_STATUS` and is
passed to the remote script, which cannot import it, so a later widening
cannot leave the script behind.
The fixture helpers are typed off the real signatures: they were implicit
`any` and widened the domain literals, so this file ran green while the
gateway typecheck failed on it — now 0 errors. The gate is a single narrowed
value, `{ slotId, taskFile }` or null, rather than two consts the block had to
re-derive.
The comment and the changelog fragment both claimed every worker-launch replay
clears a `complete` or `no-change` signal. The code clears one status, and on
native only as part of a fresh restart; both now say so. Protocol and
agent-runtime get the fragments their hygiene CI requires.
The new test covers the gap between the decision and the delete. It does not
prove the narrower window inside the delete — restoring the unlink by name
leaves it green — and says so rather than implying coverage it lacks.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
mark startrefuses to re-open acompleteorno-changesignal (#2232), on the stated grounds that a legitimate re-dispatch clears it first. That clearing only ran for a native fresh restart:So on tmux transport a replay of prepare or dispatch left the finished signal in place, and the replayed worker met
signal already terminal: status=complete; start does not re-open itwith nothing having cleared it. The comment inmark-checklist-step.cjsasserting the opposite was mine and wrong; it is corrected here. A grep forWORKER_SIGNAL_FILEremoval across the gateway finds exactly one site, so nothing else was covering it.Found while closing out #2232's live proof: a
run.replayStepofmonitorleftSIGNAL.jsonuntouched, which sent me to the guard rather than the other way round.What
Every worker-launch replay (prepare..dispatch) now clears, on any transport. Only a terminal signal goes:
runningis preserved until admission replaces it —replay-signal-preservation.test.tsalready pins this, because a refused retry must not abandon a proven live worker. Clearing unconditionally would have broken it.blockedstays:startresumes it and keeps the checked boxes, which tonight's live run on mini-core-3 confirmed (./mark start→signal started, step-1 tick intact,./mark 2accepted).completeandno-change— the two the start guard refuses — are removed.A native fresh restart has already established that no previous worker survives, so it still clears whatever is there, unchanged.
Tests
Two new cases in
replay-signal-preservation.test.ts, one per terminal status, both red before the change (actual: '{"attemptId":"finished-attempt","status":"complete",…}', expected: null). The existing preservation test still passes, which is the distinction this change turns on.src/methods/runthrough the repo's runner:discovered=41 assigned=41 failed=0(3m 48s). An earlier batch run of five files in one process showed failures that were my invocation, not the code — two wereTypeError: mock.module is not a functionfrom a missing--experimental-test-module-mocks, and two were cross-file state from sharing one process.Note for review
The new helper calls
execOnSlotdirectly, matching the existing reader in this file (replay-step.ts:750). That import is flagged byno-restricted-imports("run commands through a service in src/host-services") as a pre-existing violation on the import line; the ratchet reports 0 new. Say if you want the helper routed through a host service instead.Live proof after deploy, on mini-core-2.