the reader retries a race, and the last in-place producer stops making one - #87
Merged
Merged
Conversation
Eight tasks against `docs/superpowers/specs/2026-08-18-daemon-reader-retryable-verdict-design.md`, each ending in an independently testable deliverable and a commit. The producer half is tasks 1-4: share the staging vocabulary, teach the sweeper that there are two stagers now, rewrite `retire_held_record` to stage-rename- truncate with `validate_retired_tombstone` checking two identities, and prove the window is closed with a concurrent observer over 64 reclaim cycles. Task 4 step 3 is the point of that task: restore the old syscall order and the test must fail, because a test that has not been made to fail has proven nothing. The reader half is tasks 5-7. Transience rides inside `io::Error` as a typed payload rather than changing every validator's signature, which also makes the default the safe one -- anything not built by `raced()` stays terminal. The retry wraps the whole observation sequence and keeps `inspect_with`'s signature, so none of its 23 call sites change. Task 8 makes the three-face rule true in the shared module's documentation rather than aspirational, which is what the producer fix earns. Two assumptions were checked against the tree rather than assumed while reviewing the plan: `open_interrupted_tombstone` returns `io::Result<Option<InterruptedTombstone>>`, which is what the task 3 and 4 tests rely on, and `Inspection`'s five fields are what task 6 extends. One self-review note was wrong and is corrected in this commit rather than left to mislead an implementer. It claimed the compiler would not force the new `Inspection` field to be set at every construction site. It does -- a struct literal missing a field does not compile. The real hazard is the opposite and quieter: setting `raced: None` at a site that should carry the marker compiles cleanly and silently disables the retry for that path, which looks exactly like the bug the plan exists to fix.
…s what makes it safe
… in place retire_held_record fchmod-ed a live, still-linked record to 0200 and only then truncated it, so between two syscalls the daemon instance path showed a reader (0200, content) -- "written but never published", which the reader treats as a terminal fault. It was the last in-tree producer of that fourth face. Retirement now stages an inert 0200 empty file under a .reclaim- name, renames it over the destination, and empties the old record only once the rename has taken that inode out of the namespace. The order is the mirror of the publisher's, which truncates before it chmods: there the torn lstat must not read (0200, content); here the destructive step must not run on an inode anyone can still reach by name, because truncating first would put (0600, 0) at the live name and validate_file_stat accepts that as a published record of size zero. That ordering is not observable from the end state, so no end-state test can hold it: hoisting the truncate above the rename left the whole suite green. empty_unlinked_inode carries the precondition with the syscall it guards and refuses unless st_nlink is 0, which makes the same mutation fail. validate_retired_tombstone is replaced rather than adjusted. Its old post-condition -- the held inode is still the inode at the name -- is unsatisfiable once a rename unlinks it. The replacement proves the record is gone from the namespace and empty, that the name holds the inert tombstone this retirement staged, and that the staged inode did not change under the rename. validate_file_stat's comment claimed retirement was unfixed and cited the lines this commit deletes; corrected in place.
…aces it The rename introduced in 6d040c2 was unconditional. It is not NOREPLACE -- retirement must replace -- so it overwrote whatever was at the name at that instant, and nothing downstream could tell that it had: validate_retired_tombstone compares the destination against the inode retirement staged, not against the record, and empty_unlinked_inode accepts st_nlink == 0, which is just as true when somebody else did the unlinking. A replacement arriving after the caller's last validate_held_* was therefore clobbered, its bytes destroyed, and Ok(()) returned. MEASURED by deleting the new check and re-running `cargo test -p gascan --lib -- tombstone_recovery_ stale_record_recovery_ retirement_`: the new test failed with "retirement renamed over a file it had never validated: ()" -- the value returned was Ok. The pre-existing tombstone_recovery_never_mutates_a_pathname_replacement passed under that same mutation, because it injects during a probe, inside the window the caller still covers. A raw statat compared against record.identity now runs immediately before the rename, mirroring the identity check guarding the non-NOREPLACE rename in crates/gascand/src/socket.rs. Raw, not file_identity_at: that helper runs validate_file_stat, which refuses (0200, content) -- the destination this path exists to retire. Inode numbers are sound to compare here because record.file is still open, so the kernel cannot recycle that number underneath us. validate_retired_tombstone's doc claimed the new post-condition was "strictly stronger". It is not. It is stronger in two dimensions the old form never checked -- the record's bytes are destroyed, and it is out of the namespace -- and weaker in one: the old form compared the inode at the name against record.identity, so reaching Ok meant the name still held the inode the recovery validated. Comparing against staged_identity cannot say that, because retirement's own rename put that inode there either way. The pre-rename check is what restores the causal link, and the comment now says so. Staging cleanup moves to ReclaimStagingGuard, an RAII guard modelled on gascand's StagingGuard, and the hand-written unlinkat at the rename site is removed so the guard is the only cleanup between staging and commit. The sync_all before the rename previously returned without unlinking. MEASURED by deleting the guard and re-running the same filter: the new test failed with "the refusal left reclaim staging behind: [\".reclaim-3ifdBUte3w\"]". crates/gascand/src/socket.rs argued its own check-then-act partly from "gascan never creates or renames a node at this path". 6d040c2 made both halves false. The replacement argues the interleaving harmless rather than impossible: both writers commit an inert 0200-and-empty tombstone, so the destination is legal in every ordering and never the fourth face; neither touches the destination after its own rename; and with the check above, gascand's tombstone landing first makes the CLI refuse, the CLI's landing before gascand's identity_at makes gascand not rename at all, and in the remaining order gascand replaces one inert tombstone with another. The CLI detects the loss and reports TombstoneChanged; gascand does not check, which is stated rather than glossed.
…faces An observer thread samples the instance path across 64 reclaim cycles while the main thread drives retirement over both shapes it is reachable with: a published record, 0600 with content, through open_published_record, which is what recover_stale_published_record hands it and which had no end-to-end success coverage; and an interrupted tombstone, 0200 with content, through open_interrupted_tombstone. MEASURED: restoring the old two-syscall order in retire_held_record -- fchmod to 0200 then ftruncate on the live record, no staging, no rename -- fails the test on its first cycle with Some((128, 341)): mode 0200 holding the whole 341-byte published record, read at the destination with st_nlink == 1. That is the illegal fourth face gascan_core::daemon_protocol names. Restoring the staged rename passes, and `cargo test -p gascan --lib` reports 299 passed. The published shape is what carries that evidence. On the interrupted shape the mutation's fchmod is a no-op, because the record already wears 0200, so that shape cannot distinguish the two orders and is in the loop for coverage rather than for proof. Its own residue is consequently in the legal set, at exactly the length the test commits, with the two payload lengths asserted different so that admitting it cannot also admit the published record wearing 0200. The setup commits each state with a rename rather than fs::write followed by fs::set_permissions. MEASURED: that setup showed the observer Some((420, 0)) and Some((420, 21)) of its own making, beside the residue it meant to arrange, before reclaim had run at all -- a test that shows a reader a half-built file cannot then claim the reader only ever saw whole ones.
…t says what it changes
…get there The standing exception in gascan_core::daemon_protocol named retire_held_record as an in-tree producer that still violated the three-face rule. This branch fixed that producer, so the paragraph is replaced rather than softened: every producer in production code obeys the rule, two test fixtures fabricate the illegal face on purpose because a reader that refuses it has to be shown it, and the one producer left in the field is a gascand from a release older than the staged publication -- which is the reason the reader keeps (0200, content) terminal instead of retrying it. Nothing is coming along to finish that record. validate_file_stat's comment already named only the older gascand; it now says why that one producer cannot be fixed from here, which is the argument for the verdict staying terminal. Its reasoning about reporting size in every case is untouched. Three durable-text defects a review found in the observer test are fixed. "The last in-tree producer" was false -- DelayedPublicationSpawner in this same file still writes b"publication-in-progress" and chmods 0200 -- so it now reads "in production code" and names the fixtures. "Only ever showed a face a reader may legally see" overstated the assertion, whose legal set admits the interrupted residue the test itself commits. The 47,124,057-sample claim is no longer restated without its anchor; it points at the comment in retire_instance_record_with_hook that holds it. Three mutations, run and reverted at beb05f4: - retire_held_record replaced by the old two syscalls (fchmod to INSTANCE_TOMBSTONE_MODE then ftruncate on the held record, no staging, no rename): cargo test -p gascan --lib no_reader_ever_sees_an_illegal_state_across_reclaim FAILED with "a reader saw [Some((128, 341))]". 341 is the serialised record's length at this path, not a constant of the protocol -- the record embeds std::env::current_exe(). The doc comment now says so. - ftruncate hoisted above the renameat in retire_held_record: same test FAILED with "a reader saw [Some((384, 0))]". empty_unlinked_inode's doc said this window was one "which no test in this tree opens"; that stopped being true when the observer landed, and it now names the test and the measurement. - the ENOENT split in validate_instance_tombstone widened to mark every errno raced: a_symlink_swapped_over_the_tombstone_is_a_fault_not_a_race FAILED. The design spec's two false claims are corrected. "A strictly stronger post-condition" was wrong in one direction: the new form proves the record is out of the namespace and its bytes destroyed, but the old form proved the inode at the name was the one the recovery validated and the new form cannot, because the rename is not NOREPLACE and must not be. What restores it is the check-then-act immediately before the rename, not the post-condition. And the ENOENT was called reachable-but-inert in production; that is backwards. MEASURED at beb05f4 with a temporary probe, not in the tree, over read_instance_record_for_inspection_with_hook with a hook that unlinks the tombstone: classification present -> Err(kind=PermissionDenied, raced=true); the raced() arm reverted to errno(error) -> Ok(None). raced() builds a PermissionDenied, so the NotFound => Ok(None) arm stops swallowing it. No end-to-end verdict flip was tested and none is claimed. START-HERE records that open item 1's residual is implemented on this unmerged branch, that item 9's "the two adjacent constants did not move" is now half false -- INSTANCE_STAGING_PURPOSE moved when the CLI became a second stager -- and that item 2c remains decided but unbuilt. It also records the one follow-up this branch knowingly left: the recheck statat in validate_instance_tombstone still carries an unmarked, terminal ENOENT. The window is characterised rather than guessed -- openat->fstat is already covered, because an unlink there leaves st_nlink == 0 and the existing raced mark fires, leaving fstat->recheck-statat and within it only the sub-window between a successor's unlinkat and its renameat_with. Narrow, real, fail-closed today.
…rifted START-HERE said STARTUP_DIAGNOSTIC_NAME became pub(crate) "so the four hard-coded literals in crates/gascan/src/client.rs name it through the constant". Four is that file's count, not this constant's. VERIFIED at the pre-item-9 base: git grep -c daemon-startup-error 3cf98b5 -- crates/gascan/src/client.rs returns 2, and the same command for daemon-instance returns 2. Two of the four were the startup diagnostic and two were the instance record, which now goes through gascan_core::daemon_protocol::INSTANCE_NAME. The sentence says two, says where the other two went, and points at a grep instead of a line number. START-HERE also said the reader is "retried up to three times". const OBSERVATIONS: u32 = 3 feeds for observation in 0..observations, so that is three observations and two retries -- which is what the verdict's own message means by "still changing after 3 observations". The spec's 4.2 already had it right; both now say the observation count is the number to quote. Three line anchors in the design spec had drifted, one paragraph away from an anchor already replaced for being drift-prone: - clear_inert_destination cited as crates/gascand/src/socket.rs:530; it is at :590, and :530 now lands inside a comment about the sweep. - the openat in validate_instance_tombstone cited as crates/gascan/src/daemon.rs:2855; it is at :3282, and :2855 now lands inside an Inspection struct literal in observe_once. - crates/gascan/src/daemon.rs:1366 cited for the for probe_index in 0..2 shape in recover_interrupted_tombstone; that loop is at :1498, and :1366 is a call site of recover_stale_published_record. Each was re-derived against the tree and replaced with the function name alone, the way the corrections in 3.4 and 4.3 already do it. Every one of these three is uniquely greppable, so the line number was carrying no information the name does not.
DaemonPaths (crates/gascan/src/daemon.rs:42) has directory, socket, instance, lifecycle_lock and expected_uid. The struct with instance_path and startup_diagnostic_path is DaemonLaunch (:491), and both literals in crates/gascan/src/client.rs are DaemonLaunch.
…ot among them START-HERE called the recheck `statat`'s unmarked `ENOENT` "the only thing from this branch still open". It is one of six unmarked race-shaped failures, and the completeness claim hid the part that matters: the published->inert transition -- what a legitimate stop does -- is still a terminal `Unsafe` in the common case. Walked against the tree rather than argued: a reader whose first `statat` lands after the rename sees a plain tombstone and answers `Stopped`, so the failures come from tearing across the rename, and those tears reach `validate_file_stat`'s "mode is 0200 and the file is empty" fault or the `EACCES` an `O_RDONLY` `openat` returns against a 0200 file. `open_interrupted_tombstone` answers `Ok(None)` on the way past, because `is_interrupted_tombstone` requires `st_size > 0` and an inert tombstone is empty -- so the verdict is built from the record read's unmarked failure. `open_published_record` is never called on that path at all, which is why its two `raced()` marks fire only on a record-for-record substitution. The block now names the full residue and says plainly which case is uncovered. It does NOT mark the missing sites: `validate_file_stat`'s "0200 and empty" is a genuine fault for the socket and the lifecycle lock, so reclassifying it is a per-call-site change, and widening a fail-closed default at the end of a branch is how that default gets weakened by accident. The design spec kept a sentence the code deliberately dropped -- "Under `start_with` the lifecycle lock makes a race impossible". `gascand` only names the lock path (`crates/gascand/src/socket.rs`); its only two `flock` calls are in `ssh.rs` and lock the managed SSH directory. Deleted rather than replaced: the rule already lives on `inspect_with`. The same paragraph said the retry waits `SupervisorTimeouts::poll`; it waits `DEFAULT_POLL`, because `inspect_with` takes no timeouts argument. The plan is a snapshot, so its copy is annotated instead of rewritten. The shared protocol doc's two-fixture sentence read as an enumeration where `crates/gascan/src/daemon.rs` has twelve `from_mode(0o200)` fixture sites. Hedged.
`inspect_with`'s body was the whole reader half -- `retry_while_raced` applied to `observe_once` -- and nothing reached it. The three retry tests call `retry_while_raced` directly with canned observations; both marker tests call `observe_once_with_hook` directly; no test injected a race into `inspect_with`. Each half was proved and the join was not. `inspect_with` is now a one-line delegation to `inspect_with_hook`, which holds the composition and exposes the observation's tombstone window -- the same `_with_hook` shape `observe_once_with_hook`, `read_instance_record_with_hook` and `open_private_directory_with_create_hook` already use. The hook is `Fn` rather than `FnOnce` because the retry rebuilds the observation on each attempt, which is what lets it fire on the first and stand aside afterwards. The `observe_once` wrapper had no callers left and was deleted rather than kept alive by the file-level `dead_code` allow; its references in nearby comments were repointed. `a_raced_observation_is_looked_at_again_through_inspect_with` renames a second inert tombstone over the name on observation one only, then asserts the hook ran twice and the verdict is `Stopped` with no marker. The substitution is sequenced inside the observation's own hook in one thread, so no clock decides the outcome. MEASURED 2026-08-19, `cargo test -p gascan --lib`: collapsing `inspect_with_hook` to a single `observe_once_with_hook` gives 307 passed, 1 failed -- that test and nothing else -- and `OBSERVATIONS = 1` gives the same. Rewriting `inspect_with`'s delegation to bypass `inspect_with_hook` gives 308 passed, 0 failed; that residual is recorded in START-HERE rather than closed, because closing it means threading a test-shaped parameter through every production caller. The doc comment claimed "every such disagreement used to be a terminal `DaemonState::Unsafe`". Five became retryable and the rest did not, so it now names the five and points at the residue. No site was newly marked; `(0200, content)` stays terminal and unmarked.
…empty
The CI section names three standing root causes and budgets sessions to them.
This is a fourth, found while verifying the reader-retry fix wave and diagnosed
rather than merely observed.
`doctor_recovers_a_legacy_daemon_through_double_attested_sigterm` failed a local
`cargo test --workspace` on this branch with `ParseIntError { kind: Empty }`.
Three lines explain it: `crates/gascand/src/api.rs:504` writes the pid with
`std::fs::write`, a create-truncate then a write; `wait_for_socket` at
`crates/gascan-e2e/tests/doctor.rs:263` gates readiness on `pid.exists()` alone;
`UpgradeEnvironment::pid` at `:346-349` parses the file's trimmed contents. A
read landing between the create and the write sees an existing empty file.
It is a fixture-startup race in the e2e harness widened by load, not the
`ssh-keygen` `/dev/fd` descriptor flake recorded as mechanism 1. The branch
touches no file under `crates/gascan-e2e/`, and the test passed 4 runs out of 4
alone. The cure is the discipline the instance record gained in `025b922` --
non-empty gate, or a staged rename -- applied to a file the tests depend on.
Documentation only; no code or test changed.
`sweep_abandoned_staging` is `write_instance_record`'s first action and now
matches `.reclaim-` with no age or liveness filter, and the lifecycle lock does
not fence every `gascand` -- the sweeper's own doc comment names which ones it
misses. Either of those, publishing while the CLI holds a reclaim staging file
open across stage, `sync_all`, the identity check and `renameat`, deletes that
file. The rename then failed `ENOENT`, mapped to `SupervisorError::Io`, and
reached the user as `gascan start` failing with "No such file or directory (os
error 2)" and nothing to attribute it to.
That interaction is new on this branch: on the merge-base the CLI staged nothing.
It fails closed, so this is diagnosability, not safety.
`ENOENT` alone now maps to `SupervisorError::TombstoneChanged` naming the sweep,
the same narrow split `validate_instance_tombstone` already makes. Every other
errno stays `Io`.
`retire_held_record` delegates to a new `retire_held_record_with_hook`, which
exposes the window between staging and the commit; the staging name carries a
random token no test can predict, so the hook receives it. The test unlinks
through that seam and pins the variant. Reverting the errno split fails it with
`Err(Io(Os { code: 2, kind: NotFound, ... }))`.
`sweep_abandoned_staging`'s doc comment stated the cost as a failed rename of an
empty file. It is a failed CLI lifecycle command, and it says so now.
… once `ensure_started_locked`'s readiness loop has a `timeouts.poll` arm for every transient state it knows and none for the verdict `retry_while_raced` gives up with. A raced inspection therefore fell into the catch-all and returned `SupervisorError::Readiness` immediately, with almost the whole 15-second deadline unspent. `retry_while_raced` stops after three observations because it has no budget of its own; this caller has one, and a path that is still moving is transient by definition. The give-up `Inspection` now carries `raced: Some(detail)` and the loop matches `inspected.raced.is_some()`. Keyed on structure, not prose: the existing `ENDPOINT_CHANGED_DURING_PROBE` arm may match a message because that message is a constant, and the give-up message is formatted with a runtime observation count. The marker cannot make that verdict retryable. It is built after the `for` loop and never passed to `observe`, `inspect_with_hook` is `retry_while_raced`'s only production caller, and `observe_once_with_hook` does not call it -- so no retry loop ever reads it. The readiness loop that does read it is bounded by `deadline` both on the arm and in the `timeout_at` around the inspection. `ensure_started_locked` delegates to `ensure_started_locked_with_hook`, which forwards to `inspect_with_hook`; that is the only way to hold the path raced across the loop without a second thread and a clock. The hook took the signature to eight arguments, so `inspect_with`'s four parameters travel as an `InspectionSubject` rather than as an `allow`. The test drives it under paused time and asserts readiness ended on its own deadline after more than one inspection; removing either half of the fix returns the give-up verdict at once.
CI's `rust` job failed PR #87 on `provision_and_health_kill_point_phase_matrix_has_exact_recovery_status` at `crates/gascand/tests/reconcile.rs:965`, `during-health, left: Completed, right: Failed`. Run `32214820420` shows the same test failing the same way at the same line on `main` at `61f1b3c`, which is that branch's own merge-base, so `main` is red for this reason independent of any branch. Recorded beside the four mechanisms already named, with its anchors and with the diff and isolation evidence. Not diagnosed: nobody has traced why a kill during the health phase leaves the operation `Completed`, and the entry says so rather than guessing.
…aper than the file implied An independent review of PR #87 on 2026-08-19 raised two Importants, both now fixed on this branch, and five Minors that lived only in a session scratchpad. Four are recorded as open items under the branch's entry: the reclaim stager's missing name-recheck against the mirror it claims, DEFAULT_POLL severing SupervisorTimeouts::poll from the retry, the terminal endpoint detail the give-up path discards, and two defensive branches that are unverified rather than covered. The fifth amends the delegation-gap passage rather than sitting beside it: module privacy closes that gap as a compile error at zero call-site cost, which the passage's 'residual every _with_hook pair carries' framing did not consider.
…assignment The cold-start block said the branch was unpushed and that merging it was the assignment. Both are now wrong. PR #87 is open, green at head d5ea334, and left unmerged deliberately so the reader half is finished on that same branch rather than split across a second PR. What the block now records instead: the verified branch state (22 commits off merge-base 61f1b3c, four checks passing with engine skipping), why it is unmerged, what "finish the reader half" means and why it is a design task, and the queue after it -- the two unfixed CI flake mechanisms, then item 2c. Two numbers this file carried were wrong and are corrected against the tree. The branch's base is 61f1b3c, not 3cf98b5: `git log -1 --format=%p 3e2cc9e`, its first commit, returns 61f1b3c. And validate_file_stat is not reached from the socket -- inspect_endpoint_path checks mode inline and never calls it -- so the "genuine fault for the socket and the lifecycle lock" sentence now names the five production call sites the tree actually has and the lifecycle lock as what validate_open_file also guards. The two CI flake mechanisms added on 2026-08-19 sit below the "Where the work is" heading, which the header declares to be history. The header now says they are the exception.
…h no code in it CI run 32281948905's rust job failed on automatic_ssh_port_reservation_is_loopback_unprivileged_and_exclusive with AddrInUse out of -p gascand --test lifecycle. An ephemeral-port collision on a shared runner is as far as it is diagnosed, and the entry says so. What makes it worth recording is the exoneration: the run's head b1ef129 changes only docs/status/START-HERE.md, and the preceding run on d5ea334 passed rust. The code was byte-identical, so no crate-scoped diff and no isolation run were needed. Beside the existing 38%-on-main measurement, records three consecutive completed rust runs on this one branch -- two different failing tests with a green run between them -- and adds the cheap check to the exoneration rule: look first at whether the failing commit changed any code at all.
The retry covered five race-shaped failures and the design's own motivating case was not among them. A `gascan status` sampling the instance path while a daemon retires its record tore onto faults nobody had classified, so a daemon that was merely stopping read as `Unsafe`. MEASURED at `bf107a1` with a temporary probe not retained in the tree, one reader spinning on `read_instance_record_for_inspection` against a publish-and-retire loop, over five seconds: 2426 `link count is not one (mode 0600, links 0)`, 1138 `mode is 0200 and the file is empty`, and 11 bare `EACCES`, all three terminal, beside 2662 and 438 already-marked tombstone substitutions. The `nlink == 0` tear was the most common of the three and was not in the residue the previous session enumerated. `validate_file_stat` could not be marked in place: it has five production call sites, two of them inside the generic helpers `validate_open_file` and `file_identity_at`, and `validate_open_file` is what the lifecycle lock is validated through as well as the instance record. So the file being guarded is now a parameter, `GuardedFile`, and the faults are values rather than message strings. `StatFault::is_transitional_for` holds the entire widening in one place: `Unlinked` and `InertTombstone`, for `InstanceRecord` only. `UnpublishedRecord` -- 0200 with content -- stays terminal, which is the line this change exists to hold: per the three-face rule the only producer left is a `gascand` from an older release, so retrying it would report a permanently stuck record as a passing squall. Ownership, file type, extra links and wrong modes stay terminal everywhere, for the instance record too. The lifecycle lock treats all of them as faults, including the two that are transitions for the record. `EACCES` from the record read's `O_RDONLY` `openat` is classified by evidence rather than by declaration, because no validator sees it: the name is looked at again, and only an inert tombstone or an absence makes it retryable. A record chmod-ed to 0200 and left there still reaches the caller as `Unsafe`. Also classified, each with a test that fails without it: the record read's two identity comparisons, `open_interrupted_tombstone`'s two, and the `ENOENT` on `file_identity_at`'s `statat` and on `validate_instance_tombstone`'s recheck. The `file_identity_at` one is not cosmetic -- unmarked it is `NotFound`, which `read_instance_record_for_inspection` maps to `Ok(None)`, so the window used to read out as a confident "stopped" for a daemon mid-transition. The retry composition is sealed. `observe::sealed` holds `inspect_with_hook`, `retry_while_raced` and `observe_once_with_hook`; only the first leaves the module. Rewriting `inspect_with`'s delegation to call the observation directly was a silently green mutation and is now a compile error -- VERIFIED, `cargo build -p gascan` fails with E0425 `cannot find function `observe_once_with_hook` in module `observe``. Three review minors, each now held by a test that fails when reverted: `stage_inert_reclaim_file` performs the staging-name check its doc comment already claimed its `gascand` mirror shared; `inspect_with_hook` takes the retry delay as a parameter, so `SupervisorTimeouts::poll` reaches the one delay in the supervisor it could not; and the give-up verdict's marker composes the terminal endpoint fault with the race instead of dropping the actionable half. Verified at this commit: `cargo fmt --all --check` exit 0, `cargo clippy --workspace --all-targets -- -D warnings` clean, `cargo test --workspace` 1532 passed, 0 failed, 49 ignored. The three race loops were additionally run twice at 30000 cycles each with no unmarked failure; they are committed at 4096, where a mutation of either dominant classification is caught 3 of 3.
…tself incomplete Rewrites the cold-start block and open item 1 against `ae03597`. The three blocks that stood in open item 1 — `KNOWINGLY LEFT`, `ALSO LEFT` and `FIVE MINORS` — are all discharged, so they are replaced rather than amended. The correction worth carrying forward is that this file's own enumeration of the reader's residue was wrong by omission, and the omitted item was the largest one. It named the "0200 and empty" fault and the `EACCES` as the stop transition's tears; the `nlink == 0` tear — the reader still holding the inode a rename unlinked — was in neither the residue list nor the design, and MEASURED at `bf107a1` it produced 2426 of the 3575 unmarked faults in a five-second window. It was found by writing the end-to-end test this file admitted did not exist, which is the transferable lesson: the enumeration in a handoff doc is a hypothesis, not an inventory. Two smaller corrections to claims this file carried. The three identity-comparison messages it listed as residue produce zero samples in the stop transition — `validate_file_stat` reaches a verdict first on every path that transition takes — so they are classified now on their own evidence rather than as part of the motivating case. And the review's premise that `inspect_with_hook` has exactly one legitimate caller is off by one: it has two, `inspect_with` and `ensure_started_locked_with_hook`'s readiness loop. The module seal works either way. The queue below the block is now the assignment rather than what follows it.
… on the production path A review of `bf107a1..8613f22` found a Critical the previous commit had already declared closed. `open_published_record`'s `openat` was unclassified, and `observe_once_with_hook` calls that function on every observation where the record read *succeeded* — so an ordinary stop transition still produced terminal `Unsafe` verdicts, with no retry at all. MEASURED by the review at `8613f22`, driving `observe_once_with_hook` against a publish-and-retire producer over 20,000 observations: 4456 terminal `Unsafe` verdicts, 100% of them from that one `openat`, about 55% of every `Unsafe` observed. Reproduced here before fixing, by the test that should have existed first: `every_unsafe_observation_across_a_real_stop_transition_is_marked_raced` fails at `8613f22` with `{"Permission denied (os error 13)"}`. The cause is methodological and it is the second time on this branch. `every_reader_failure_across_a_real_stop_transition_is_marked_raced` drives `read_instance_record_for_inspection`, which is a strict subset of one production observation. A test one layer below the production path proves nothing about the production path. The first instance is recorded in `every_tombstone_failure_across_a_concurrent_unlink_is_marked_raced`, where a reader placed above the `NotFound => Ok(None)` mapping could not see the errors it was testing and a whole classification could be deleted with the mutation caught 0/3. Fixing it took three passes, each driven by a measurement rather than by reasoning, because the evidence-based classifier's re-stat is itself racing: - admitting only the inert tombstone missed a re-stat landing after the *next* publication (`mode=0600 size=341 nlink=1`) - adding the published face missed a re-stat tearing across the retirement itself (`mode=0200 size=0 nlink=0`) - deferring to `validate_file_stat` — `Ok` or an `is_raced` fault — covers all three and keeps the terminal set defined in exactly one place `classify_unreadable_instance_record` now also splits `ENOENT`, at both call sites, on the argument `file_identity_at` already makes: the name resolved moments earlier, so its absence is a successor's commit rather than an absent daemon. `open_published_record`'s recheck `statat` gets the same split. Four further findings from the same review: - All three loop tests asserted an empty failure set behind a guard that counted only *successes*, so a reader that never tore would pass while exercising no classification. Each now asserts `marked > 0`. - `inspect_with`'s doc comment enumerated "five" race-shaped failures and named the wrong two functions. Replaced with the invariant rather than a list, since the list is what went stale. - `read_instance_record_with_hook*` took three positionally-distinguished no-op closures; swapping two in a test still compiled and still passed while testing the same window twice. They are now named fields of `ReadHooks`. - `stage_inert_reclaim_file`'s new staging-name check unlinked by bare name on failure — the weaker route `ReclaimStagingGuard` exists to avoid, reintroduced on the cleanup path of the check added to be careful. It now unlinks only an inode it still owns. The terminal arms of the classifier now carry mode, size, links and uid, for the reason `validate_file_stat`'s doc already gives: a bare "Permission denied" made a CI failure unattributable, and this is the arm that survives to a human. The review's I2 is fixed in the docs rather than the code: this file claimed all four remaining minors were held by a test, and MEASURED the staging-name check is not — deleting it leaves `cargo test -p gascan --lib` green. It is now listed only among the uncovered defensive branches. Verified at this commit: `cargo fmt --all --check` exit 0, `cargo clippy --workspace --all-targets -- -D warnings` clean, `cargo test --workspace` 85 suites, **1534 passed, 0 failed**. `cargo test -p gascan --lib` run five times consecutively, 324 passed each. The observation loop was additionally swept three times at 40,000 observations with no unmarked verdict; it is committed at 4096, where the unclassified `openat` is caught 3/3. Every classifier branch is held by `the_unreadable_record_classifier_admits_only_faces_in_motion`, whose four mutations were each confirmed to fail it.
…n the same target `retained_ssh_host_key_failure_removes_prior_alias_before_stop` in `gascand`'s `lifecycle` suite asserted a rejected `up` had left the container `Stopped` and sampled `Running`. Run `32314592435`, on PR #87 at `93c77fe`, `crates/gascand/tests/lifecycle.rs:1228`. Not diagnosed, and deliberately not guessed at. Worth separating from the sixth mechanism rather than filing beside it: that one is `automatic_ssh_port_reservation_is_loopback_unprivileged_and_exclusive` failing on `AddrInUse`, this one is a different test asserting a different thing. "The `lifecycle` flake" is already at least two mechanisms, and treating them as one is how a real failure gets waved through. Exonerated on both halves of this repo's rule. By diff: `crates/gascand/Cargo.toml`'s `[dev-dependencies]` does not list `gascan`, so the only crate `93c77fe` changed is not linked into that test binary, and the assertion is on `ContainerState` from a `FakeRuntime` rather than on `DaemonState`. By isolation: the test ran six consecutive times locally, 1 passed 0 failed each, and `cargo test --workspace` at that commit reported 85 suites, 1534 passed, 0 failed. `gh run rerun 32314592435 --failed` then settled green at the same commit — `gate`, `rust`, `contracts` and `changes` all SUCCESS, `engine` skipping. The queue at the top of this file now names four flake mechanisms, not three.
…e in it Run `32315397045` at `cfcfb62` on PR #87 failed `rust` with `provision_and_health_kill_point_phase_matrix_has_exact_recovery_status` at `crates/gascand/tests/reconcile.rs:965:9`, `left: Completed, right: Failed` — the same test, line and values the fifth mechanism already records, and one this file documents as red on `main` itself at `61f1b3c`. `git diff --name-only 93c77fe cfcfb62` returns `docs/status/START-HERE.md` and nothing else, and `93c77fe` had just settled `rust` SUCCESS on a re-run, so the code under test was byte-identical to code CI had run green minutes earlier. Two of the seven mechanisms have now been caught failing on Markdown-only commits. That is worth stating plainly: it is the cheapest exoneration available and it has now applied twice, so run `git diff --name-only` before reaching for isolation, let alone before bisecting against code.
…d rather than a race `pty_signal_driver_does_not_wait_for_inherited_slave_descriptor` failed on PR #87 at `ec47492` with "signal helper waited 420.525083ms for an inherited PTY slave descriptor". The assertion at `crates/gascan-e2e/tests/apple_common/mod.rs:4562` is `started.elapsed() < Duration::from_millis(250)`. That the bound is wall-clock is a fact readable in the source, not an inference, and it makes this the only entry in the list whose fix does not require finding a race first: a test whose pass condition is "the machine was fast enough" will fail on a shared runner eventually. Why this particular run took 420ms is not diagnosed and is not written down as if it were. Exonerated by diff: `git diff --name-only cfcfb62 ec47492` returns `docs/status/START-HERE.md` and nothing else. Also records the pattern, which is worth more than any single entry: across `93c77fe`, `cfcfb62` and `ec47492` the `rust` job failed on three different tests in three different crates, and the last two commits contain no code at all. The local `cargo test --workspace` — 85 suites, 1534 passed, 0 failed at `93c77fe` — is the signal that means something on this branch right now.
…it made and did not keep A review of `8613f22..93c77fe` found no Critical and confirmed the previous one fixed and held — reverting `open_published_record`'s `openat` fails `every_unsafe_observation_across_a_real_stop_transition_is_marked_raced` 3 runs out of 3. Six other findings, three of which are `93c77fe` claiming something it did not deliver. **The `ReadHooks` refactor did not close the gap it was made for.** That commit said "fields cannot be swapped silently". True of the call site; the destructure one layer in is still positional, and both window tests asserted only `is_raced(&error)` with the *same* message, so neither pinned its own window. MEASURED here before fixing: swapping the two field bindings — relocating both injection points so each test exercises the other's window — left `cargo test -p gascan --lib` green, 3 runs out of 3. Each test now asserts its own detail string, and the same swap fails both, 3 out of 3. **`classify_unreadable_instance_record`'s doc comment contradicted its body three ways**: it claimed only `EACCES` is split (`ENOENT` is split above it), that the read opens `O_RDONLY` (one of two callers does; `open_published_record` opens `O_RDWR`), and it described the pre-widening admission set. The widening moved three times in `93c77fe` and the comment above it moved zero. That is the defect class that commit fixed one screen up, in `inspect_with`, and reintroduced in the function that fix points at. Rewritten to state the predicate the code holds. **The diagnosability argument was backwards.** `93c77fe` said the terminal arms carry the evidence because they are what survives to a human. They are not: a race that settles produces no message at all, so the raced detail is what `retry_while_raced` builds its give-up verdict from, and both raced arms carried no errno. The review reproduced a persistent `EACCES` over a stat that reads as a legal published record — a same-uid deny ACL — which now reports "a publication committed over the record", naming a transition that never happened, with the `EACCES` discarded. Both raced arms carry the errno and the observed face now, and `the_unreadable_record_classifier_admits_only_faces_in_motion` fails if either stops. Also: `classify_unreadable_instance_record` takes `GuardedFile` instead of hardcoding `InstanceRecord`, so a lock-guarding caller added later cannot inherit the record's retry classification by saying nothing — the accidental widening that parameter exists to prevent. `commit_at_instance` names which of its three steps failed, because one full-suite run produced a bare `NotFound` from it that did not reproduce in 25 isolated runs or 20 instrumented ones; the cause is unattributed and the messages stay so a second sighting is free to place. Two further defensive branches are recorded as uncovered rather than tested — the staging cleanup guard added by `93c77fe` (MEASURED: deleting it, and inverting it so it unlinks precisely a stranger's file, both leave the suite green) and `open_published_record`'s recheck `ENOENT`. The doc's count goes from three to five. **The docs also correct a claim of mine that a reviewer could not reproduce.** `93c77fe` said `cargo test -p gascan --lib` ran five times at 324 passed. Over 43 runs here it failed 12 times, about 28%, across four distinct tests, none reproducible in isolation and none of them new. 28 of those runs turned out to have been taken while four `yes` processes left by a review subagent span the CPU — found at 68 minutes each and killed by PID. The natural reading is that they caused it; they did not. The 15 runs on the quiet machine were the worst set, 6 failures in 15. Recorded as a ninth mechanism, and the claim that the local suite is the trustworthy signal is withdrawn. Verified at this commit on a quiet machine: `cargo fmt --all --check` exit 0, `cargo clippy --workspace --all-targets -- -D warnings` clean, `cargo test -p gascand` **445 passed, 0 failed**, `cargo test -p gascan-e2e` all green, and a `cargo test --workspace` that reached 69 of 85 suites with **1414 passed, 0 failed** before a 40-minute local timeout cut it — no failure in any of them. No single complete `--workspace` run is claimed at this tree.
…nd CI green Refreshes the cold-start block for a session picking this up cold. The decision in front of the next agent is "merge or not", not "what is left to build": nothing on the reader half is outstanding, two independent reviews ran — the first found a Critical on the production path, the second found none and confirmed the fix held — and every finding from both is fixed with a test that fails when the fix is reverted, bar five defensive branches recorded as uncovered on purpose. Records the verification that the previous commit could not: a **complete** `cargo test --workspace` at `2a622f0`, 85 suites, 1534 passed, 0 failed, exit 0. `93c77fe`'s message said no complete run was claimed, which was true when it was written; two earlier attempts had been cut by a local timeout while the machine carried four leaked CPU burners. CI at `2a622f0`: `gate`, `rust`, `contracts` and `changes` all SUCCESS, `engine` skipping — the first fully green run on this branch in five commits. The block says plainly that this is one run rather than a property of the branch, and points at the ninth mechanism, so nobody reads it as a guarantee. The commit count in that block is 31 at head `2a622f0` and this commit makes it 32, which is noted inline rather than left to rot — the staleness this file has warned about twice, arriving inside the warning again.
…ft stale The two reviews existed only in a scratchpad that does not survive the session, and they carry measurements this file only summarises — round one's attribution of the Critical to a single `openat` (4456 of 4456 unmarked verdicts in 20,000 observations), round two's mutation table, and its "Checked and found sound" section recording what was attacked and survived. That last is the part a successor needs before re-opening a settled question. Committed as `docs/status/review-daemon-reader-half.md` and `docs/status/review-daemon-reader-fixes.md`, and pointed at from the cold-start block. Two counts in this file had gone stale within the same session that wrote them: open item 1 still said three uncovered defensive branches where the cold-start block already said five, and the header still said three current exceptions below the history line where there are now six mechanisms. Both fixed, and the five are now enumerated with the measurement that put each on the list rather than named in a sentence.
…/CLEAN Head 903ef05, 33 commits off main, all four CI checks SUCCESS with engine skipping, and gh pr view reporting mergeable=MERGEABLE mergeStateStatus=CLEAN and not a draft. This commit makes it 34 and moves the head, which is stated inline rather than left to rot.
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
1 participant
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.
Open item 1's residual, implemented from
docs/superpowers/specs/2026-08-18-daemon-reader-retryable-verdict-design.mdacross 18 commits.Two programs share one file.
gascandwrites the daemon instance record; thegascanCLI reads it and classifies what it finds. The path is meant to show a reader exactly three faces — absent, inert(0200, empty), published(0600, content)— and a fourth,(0200, content), is a terminal fault.The producer half is complete
retire_held_recordused tofchmoda live record to0200and thenftruncateit in place, so between two syscalls the destination wore the illegal fourth face. It now stages a fresh inert file, renames it over the destination, and truncates the old inode only after the rename has unlinked it.The ordering is forced in the opposite direction from the publish-race fix that merged earlier: there, truncate had to precede chmod because
lstattears; here the destructive step must follow the rename, because truncating first would put(0600, 0)at the live name — whichvalidate_file_stataccepts as a published record of size zero, so the reader would take it and then fail parsing an empty file.Proven by a concurrent observer over 64 reclaim cycles. Restore the old syscall order and it fails with the exact illegal pair
Some((128, 341))—0o200with the whole 341-byte record still in the file.A review found that the rewrite had quietly traded away a property the old code had: proof that it destroyed the inode it validated. A raw
statatcompared againstrecord.identityimmediately before the rename restores it, mirroringretire_instance_record_with_hookingascand, which already solved the same problem. Its own test fails without the check, panickingretirement renamed over a file it had never validated: ()with the return valueOk(()).The reader half is a PARTIAL fix, and the PR says so because the branch says so
Race-shaped failures now carry a typed payload inside the existing
io::Errorplumbing, so no validator signature changed and the classification defaults to terminal — a validator added next year staysUnsafeuntil a human classifies it.inspect_withretries the whole observation sequence three times with no clock in any test, and falls back toUnsafewhen the path never settles.What it does not yet fix. Ten sites produce race-shaped failures; five are marked and five remain unmarked. (An earlier version of this description said nine and four; an independent review counted the tree —
raced(appears at five production call sites, anddocs/status/START-HERE.mdlists five unmarked. START-HERE and theinspect_withdoc comment were right; only this description was wrong.) The design's own motivating example —gascan statusagainst a legitimately stopping daemon, the published→inert transition — is still largely uncovered, because the tears land onvalidate_file_stat's "0200 and empty" fault, onEACCESfrom anO_RDONLYopen of a0200file, and onvalidate_open_file, all of which fire before the tworaced()marks inopen_published_recordare reached. Those two marks only fire on a record→record substitution, which is rarer.This is not a regression — every one of those was terminal on
maintoo, and every one fails closed. It is not fixable by marking, either:validate_file_stat's fault is genuine for the socket and the lifecycle lock and could only be reclassified per call site. It is recorded in START-HERE'sKNOWINGLY LEFTblock rather than described as solved.Verification
Every step of CI's
rustjob, run locally and alone on the integrated tree:cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspace./scripts/ci-check-ignored-tests.shscripts/ci-classify-paths.shon the branch diff, re-derived after the last commit:rust=true contracts=true engine=false.Known limits, all recorded rather than left silent
raced:wiring sites are traced and reviewed but unpinned; one is pinned end to end, so the mechanism cannot be removed wholesale in silence.inspect_with's delegation to bypass its_with_hookwrapper is not caught by any test. Closing it would mean threading a hook parameter through 23 production call sites for a mutation nobody plausibly writes.statatinvalidate_instance_tombstonestill carries an unmarked terminalENOENT, with the exposed window characterised in START-HERE.gascandwrites the pid file withstd::fs::write(create-truncate, then write) atcrates/gascand/src/api.rs:504, while the e2e harness gates readiness onpid.exists()alone (crates/gascan-e2e/tests/doctor.rs:263) and then parses it — so a read landing in the empty window yieldsParseIntError { kind: Empty }. Same defect class this PR fixes for the instance record, in a file the tests depend on.Post-review fixes
An independent review of this PR found two Importants that eight per-task reviews and one whole-branch review had missed, both now fixed:
gascandsweep could delete the CLI's live staging file. The sweeper runs aswrite_instance_record's first action and now matches.reclaim-with no liveness filter, while the CLI holds that file open across an fsync-length window — so an unfenced daemon publishing there swept it andgascan startfailed with a bare "No such file or directory". New to this branch: onmainthe CLI staged nothing. Therenameat'sENOENTnow names its own cause.ensure_started_locked's readiness loop had a poll arm for every transient state it knew and none for "never settled", so a raced inspection burned the deadline instantly. It now polls, keyed on structure rather than on a verdict string.The review's direct answer on the partial reader fix: sound, because it is monotone — no verdict here is less trustworthy than
main's in the same state. It also found theENOENTchange fixes a real bug rather than merely improving a message: onmainthat window answeredStopped, which madeensure_started_lockedspawn a second daemon that then died onAddrInUse.Process note
Eight tasks, each implemented by a fresh agent and reviewed independently, with a whole-branch review at the end. Every Critical and Important finding in the run was a defect in the plan or spec rather than in an implementation — including an evidence test that would have passed while proving nothing, and a "strictly stronger" post-condition that was weaker in one dimension. Those corrections are in the commit messages and in START-HERE.