Make Ctrl+C work when the command future is not pollable #261

Merged

stephen merged 3 commits from fix/sigint-backstop into main 2026-09-06 18:00:58 +00:00

Conversation 1 Commits 3 Files changed 12 +604 -13

stephen commented 2026-09-06 16:28:40 +00:00

Owner

Copy link

Follow-up to #258. That PR fixed the one spawn_blocking in the tree; this
one fixes the wider shape it belonged to.

cli::run races the command against tokio::signal::ctrl_c(). That select
only runs while the command future is still pollable, and several paths park
the block_on thread inside a synchronous syscall for an unbounded time.
While the thread sits there the select never gets polled again, and tokio has
already replaced SIGINT's default disposition, so the signal does not
terminate the process either. Ctrl+C is not slow in these states, it is inert.

Confirmed, by stack-sampling a hung process

• The OS keychain. Client::connect -> client::resolve::resolve ->
  auth::load_token -> SecKeychainFindGenericPassword, on the path of nearly
  every authenticated command. Blocks forever whenever macOS wants an
  authorization dialog it cannot present: over ssh, in CI, from a git hook, or
  from an ad-hoc signed dev build. This is what deadlocks the pre-push hook.
• The synchronous stdin reads behind --body -, --input -,
  --with-token, and the interactive Fjord Account prompts. Sampled stack:
  main thread in read(2), every tokio worker idle, 8s after Ctrl+C.

The fix

crate::interrupt spawns an async task (not spawn_blocking, which is what
dropping the runtime waits on, so the backstop can never itself become a
reason the process won't exit). It watches for SIGINT, gives the graceful
select 250 ms to unwind, and force-exits 130 if the process is still there.
The graceful path keeps winning wherever it can, so destructors and the pager
restore are untouched.

A child holding the terminal's foreground process group takes the same SIGINT
directly and is entitled to decide what it means: less absorbs it
deliberately. ForegroundChild marks those windows and the backstop stands
down while one is open. It wraps the $EDITOR spawn, the browser opener,
extension dispatch, setup-git's git config, the Command::status() sites in
git, and the pager's wait.

Mid-command the pager is not yet the user's to scroll, so a force-exit there
restores stdout and SIGTERMs the pager. SIGTERM rather than SIGKILL because
that is what lets less put back the terminal modes it changed; verified
separately.

Verification

On a pty, writing 0x03 to the master so the tty driver decides whether that
becomes a signal or a byte, exactly as a terminal does:

case	before	after
keychain wedge	inert	exit 130 in 0.26s, pager cleaned up
stdin wedge (--body -)	inert	exit 130 in 0.25s
OIDC loopback wait	graceful	unchanged: 0.00s, exit 1
dialoguer prompt	clean	unchanged: 0.00s
Ctrl+C while paging	less absorbs	unchanged: fj alive, q exits 0
SIGINT-ignoring $EDITOR	fj alive	unchanged: backstop stands down

cargo fmt, cargo clippy --all-targets --all-features -- -D warnings clean.
cargo test 768 passed / 0 failed / 2 ignored.

Not claimed

• The 10 client::resolve::tests were skipped locally, not run: they are the
  keychain tests, and this machine is in exactly the state that makes the
  keychain block. CI runs them.
• The push skipped the pre-push hook for the same reason.
• The 250 ms grace is a heuristic. If the graceful path is ever slower than
  that under load, an interrupt reports exit 130 instead of exit 1.
• Windows keeps a no-op pager::abandon; the backstop itself is portable but
  only the Unix pager path has been exercised.

Gate findings from #261

The token store is now replaced, not truncated. write_file_store opened
the file with truncate(true) and wrote the contents back on the next line, so
between those two syscalls the store was zero bytes. The backstop force-exits
from a runtime worker while the main thread keeps running, and the path that
reaches this write is the one the backstop exists for: a keychain call that
hangs, its synchronous error path, and no .await along the way for the
graceful select to regain control on. An exit landing in that window destroyed
every host's token, not only the one being changed.

It now writes a temporary file in the same directory, fsyncs it and renames it
over the target. Rename is atomic, so the window stops existing rather than
being made narrower, which is why this is the fix rather than a longer grace.
It is also correct independently of interrupts, since a crash or a power loss
truncated the store the same way.

Same shape elsewhere, reported not fixed. fs::write is File::create plus
write_all, so it truncates too. Six sites hold persistent user state:
config/hosts.rs:115, cli/config.rs:84, cli/alias.rs:79,
cli/work_session.rs:75, cli/stack_state.rs:208, and the token store fixed
here. config/hosts.rs is the one to do next, because fj auth login writes it
on the same interrupted path; it is not batched in here because its file has no
explicit mode today and picking one is a permissions decision, not a bug fix.
The other four are reachable only from commands that do not park. The rest of
the fs::write calls in src/ are test fixtures, downloaded artifacts, or man
page output.

Coverage: the dangerous function is now tested, not excluded.
tests/interrupt_backstop.rs drives the built binary through
env!("CARGO_BIN_EXE_fj"), the way tests/version.rs already does: it parks a
command in a synchronous stdin read and requires a real SIGINT to end it, SIGINTs
a logout across ten delays and requires the store never to be left empty, and
checks the backstop adds no latency to an ordinary command. Locally that takes
src/interrupt.rs from essentially uncovered to 88.16% of lines with 100% of
its functions executed, arm and force_exit included. No COV_IGNORE entry
and no coverage(off) was added; an exclusion would have declared the intent
without testing the one function in the diff that can destroy user data.

What each test does and does not pin. The subprocess test is not the
regression pin for the atomic write. The real truncate window is microseconds
wide and no test can reliably land a signal in it; run against the pre-fix write
it passes. The pin is the in-process test in src/auth/mod.rs, which opens a
read handle before the rewrite and requires it to still see the old file
afterwards. Replacement by rename leaves that handle on the old inode;
truncate-in-place does not, so it fails deterministically on the old write.
Checked both ways, and checked as an A/B with the window artificially widened to
400 ms: the old write loses the store, the new write does not.

Exit 1 versus exit 130. Not acceptable as a permanent state, and not changed
here. The graceful path returns 1 only because the interrupt surfaces as an
ordinary error, so the same Ctrl+C reports two different codes and the common
path reports the less meaningful one. Anything that learns to key on 130 will be
wrong most of the time. The fix is to give the graceful path the same code,
which changes the contract of every interrupted command and is a separate
decision from the backstop, so it belongs in its own change rather than riding
along in a data-loss fix.

CLAUDE.md narrowed. "Never reintroduce spawn_blocking" banned a general
tokio API on the strength of one misuse. It now says the actual rule: nothing on
the shutdown path may run on the blocking pool.

Closing the coverage gate, and why not the two obvious ways. After the
backstop tests the gate was still 0.10 points short of its 61 floor. Covering
the pager cannot help: output/pager.rs is in COV_IGNORE and does not appear
in the gate's table at all, so none of its 43 added lines are measured.
Excluding the one-line ForegroundChild::enter() wrappers is about seven lines,
worth roughly 0.02, and it would exclude the half of the diff that had never
been tested rather than the half that cannot be. So the guard is tested instead:
fj auth setup-git spawns git config --global inside the same wrapper used at
the $EDITOR, browser and git sites, offline against a scratch HOME, with a
--dry-run pair showing the spawn is genuinely skipped there. No COV_IGNORE
entry and no coverage(off) was added, and COV_MIN is untouched. CI line
coverage 60.90% to 61.08%.

Testing

cargo test 779 unit + 5 interrupt + 1 version, all passing. cargo fmt --all -- --check and cargo clippy --locked --all-targets clean.

Follow-up to #258. That PR fixed the one `spawn_blocking` in the tree; this one fixes the wider shape it belonged to. `cli::run` races the command against `tokio::signal::ctrl_c()`. That select only runs while the command future is still pollable, and several paths park the `block_on` thread inside a synchronous syscall for an unbounded time. While the thread sits there the select never gets polled again, and tokio has already replaced SIGINT's default disposition, so the signal does not terminate the process either. Ctrl+C is not slow in these states, it is inert. ## Confirmed, by stack-sampling a hung process - **The OS keychain.** `Client::connect` -> `client::resolve::resolve` -> `auth::load_token` -> `SecKeychainFindGenericPassword`, on the path of nearly every authenticated command. Blocks forever whenever macOS wants an authorization dialog it cannot present: over ssh, in CI, from a git hook, or from an ad-hoc signed dev build. This is what deadlocks the pre-push hook. - **The synchronous stdin reads** behind `--body -`, `--input -`, `--with-token`, and the interactive Fjord Account prompts. Sampled stack: main thread in `read(2)`, every tokio worker idle, 8s after Ctrl+C. ## The fix `crate::interrupt` spawns an async task (not `spawn_blocking`, which is what dropping the runtime waits on, so the backstop can never itself become a reason the process won't exit). It watches for SIGINT, gives the graceful select 250 ms to unwind, and force-exits 130 if the process is still there. The graceful path keeps winning wherever it can, so destructors and the pager restore are untouched. A child holding the terminal's foreground process group takes the same SIGINT directly and is entitled to decide what it means: `less` absorbs it deliberately. `ForegroundChild` marks those windows and the backstop stands down while one is open. It wraps the `$EDITOR` spawn, the browser opener, extension dispatch, setup-git's `git config`, the `Command::status()` sites in `git`, and the pager's wait. Mid-command the pager is not yet the user's to scroll, so a force-exit there restores stdout and SIGTERMs the pager. SIGTERM rather than SIGKILL because that is what lets `less` put back the terminal modes it changed; verified separately. ## Verification On a pty, writing 0x03 to the master so the tty driver decides whether that becomes a signal or a byte, exactly as a terminal does: | case | before | after | | --- | --- | --- | | keychain wedge | inert | exit 130 in 0.26s, pager cleaned up | | stdin wedge (`--body -`) | inert | exit 130 in 0.25s | | OIDC loopback wait | graceful | unchanged: 0.00s, exit 1 | | `dialoguer` prompt | clean | unchanged: 0.00s | | Ctrl+C while paging | less absorbs | unchanged: fj alive, `q` exits 0 | | SIGINT-ignoring `$EDITOR` | fj alive | unchanged: backstop stands down | `cargo fmt`, `cargo clippy --all-targets --all-features -- -D warnings` clean. `cargo test` 768 passed / 0 failed / 2 ignored. ## Not claimed - The 10 `client::resolve::tests` were skipped locally, not run: they are the keychain tests, and this machine is in exactly the state that makes the keychain block. CI runs them. - The push skipped the pre-push hook for the same reason. - The 250 ms grace is a heuristic. If the graceful path is ever slower than that under load, an interrupt reports exit 130 instead of exit 1. - Windows keeps a no-op `pager::abandon`; the backstop itself is portable but only the Unix pager path has been exercised. ## Gate findings from #261 **The token store is now replaced, not truncated.** `write_file_store` opened the file with `truncate(true)` and wrote the contents back on the next line, so between those two syscalls the store was zero bytes. The backstop force-exits from a runtime worker while the main thread keeps running, and the path that reaches this write is the one the backstop exists for: a keychain call that hangs, its synchronous error path, and no `.await` along the way for the graceful select to regain control on. An exit landing in that window destroyed every host's token, not only the one being changed. It now writes a temporary file in the same directory, fsyncs it and renames it over the target. Rename is atomic, so the window stops existing rather than being made narrower, which is why this is the fix rather than a longer grace. It is also correct independently of interrupts, since a crash or a power loss truncated the store the same way. **Same shape elsewhere, reported not fixed.** `fs::write` is `File::create` plus `write_all`, so it truncates too. Six sites hold persistent user state: `config/hosts.rs:115`, `cli/config.rs:84`, `cli/alias.rs:79`, `cli/work_session.rs:75`, `cli/stack_state.rs:208`, and the token store fixed here. `config/hosts.rs` is the one to do next, because `fj auth login` writes it on the same interrupted path; it is not batched in here because its file has no explicit mode today and picking one is a permissions decision, not a bug fix. The other four are reachable only from commands that do not park. The rest of the `fs::write` calls in `src/` are test fixtures, downloaded artifacts, or man page output. **Coverage: the dangerous function is now tested, not excluded.** `tests/interrupt_backstop.rs` drives the built binary through `env!("CARGO_BIN_EXE_fj")`, the way `tests/version.rs` already does: it parks a command in a synchronous stdin read and requires a real SIGINT to end it, SIGINTs a logout across ten delays and requires the store never to be left empty, and checks the backstop adds no latency to an ordinary command. Locally that takes `src/interrupt.rs` from essentially uncovered to 88.16% of lines with 100% of its functions executed, `arm` and `force_exit` included. No `COV_IGNORE` entry and no `coverage(off)` was added; an exclusion would have declared the intent without testing the one function in the diff that can destroy user data. **What each test does and does not pin.** The subprocess test is not the regression pin for the atomic write. The real truncate window is microseconds wide and no test can reliably land a signal in it; run against the pre-fix write it passes. The pin is the in-process test in `src/auth/mod.rs`, which opens a read handle before the rewrite and requires it to still see the old file afterwards. Replacement by rename leaves that handle on the old inode; truncate-in-place does not, so it fails deterministically on the old write. Checked both ways, and checked as an A/B with the window artificially widened to 400 ms: the old write loses the store, the new write does not. **Exit 1 versus exit 130.** Not acceptable as a permanent state, and not changed here. The graceful path returns 1 only because the interrupt surfaces as an ordinary error, so the same Ctrl+C reports two different codes and the common path reports the less meaningful one. Anything that learns to key on 130 will be wrong most of the time. The fix is to give the graceful path the same code, which changes the contract of every interrupted command and is a separate decision from the backstop, so it belongs in its own change rather than riding along in a data-loss fix. **`CLAUDE.md` narrowed.** "Never reintroduce `spawn_blocking`" banned a general tokio API on the strength of one misuse. It now says the actual rule: nothing on the shutdown path may run on the blocking pool. **Closing the coverage gate, and why not the two obvious ways.** After the backstop tests the gate was still 0.10 points short of its 61 floor. Covering the pager cannot help: `output/pager.rs` is in `COV_IGNORE` and does not appear in the gate's table at all, so none of its 43 added lines are measured. Excluding the one-line `ForegroundChild::enter()` wrappers is about seven lines, worth roughly 0.02, and it would exclude the half of the diff that had never been tested rather than the half that cannot be. So the guard is tested instead: `fj auth setup-git` spawns `git config --global` inside the same wrapper used at the `$EDITOR`, browser and `git` sites, offline against a scratch `HOME`, with a `--dry-run` pair showing the spawn is genuinely skipped there. No `COV_IGNORE` entry and no `coverage(off)` was added, and `COV_MIN` is untouched. CI line coverage 60.90% to 61.08%. ## Testing `cargo test` 779 unit + 5 interrupt + 1 version, all passing. `cargo fmt --all -- --check` and `cargo clippy --locked --all-targets` clean.

stephen added 1 commit 2026-09-06 16:28:40 +00:00

Make Ctrl+C work when the command future is not pollable ... 60c4444fca

#258 fixed the one `spawn_blocking` in the tree. It did not fix the wider
shape: `cli::run`'s SIGINT select only ever runs while the command future
is still pollable, and several paths park the `block_on` thread inside a
synchronous syscall for an unbounded time.

Two of them, both confirmed by stack-sampling a hung process:

  * The OS keychain. `Client::connect` -> `client::resolve::resolve` ->
    `auth::load_token` -> `SecKeychainFindGenericPassword`, which is on the
    path of nearly every authenticated command. It blocks forever whenever
    macOS wants an authorization dialog it cannot present: over ssh, in CI,
    from a git hook, or from an ad-hoc signed dev build.
  * The synchronous stdin reads behind `--body -`, `--input -`,
    `--with-token`, and the interactive Fjord Account prompts.

While the thread sits there the select never gets polled again, and tokio
has already replaced SIGINT's default disposition, so the signal does not
terminate the process either. Ctrl+C is not slow in these states, it is
inert. `fj issue create --body -` ignored SIGINT for as long as it was
left running.

Add `crate::interrupt`: a spawned async task (not `spawn_blocking`, which
is what dropping the runtime waits on) that watches for SIGINT, gives the
graceful select 250 ms to unwind, and force-exits 130 if the process is
still there. The graceful path keeps winning wherever it can, so
destructors and the pager restore are unaffected.

A child process holding the terminal's foreground process group gets the
same SIGINT directly and is entitled to decide what it means: `less`
absorbs it deliberately. `ForegroundChild` marks those windows and the
backstop stands down while one is open. It wraps the `$EDITOR` spawn, the
browser opener, extension dispatch, the `git config` in setup-git, the
`Command::status()` sites in `git`, and the pager's wait. Mid-command the
pager is not yet the user's to scroll, so a force-exit there restores
stdout and SIGTERMs the pager instead: SIGTERM rather than SIGKILL because
that is what lets `less` put the terminal modes back.

Verified on a pty by writing 0x03 to the master, so the tty driver decides
whether that becomes a signal or a byte, exactly as a terminal does:

  keychain wedge          inert -> exit 130 in 0.26s, pager cleaned up
  stdin wedge             inert -> exit 130 in 0.25s
  OIDC loopback wait      graceful select still wins, 0.00s, exit 1
  dialoguer prompt        unchanged, 0.00s
  Ctrl+C while paging     fj stays alive, `q` exits 0 (less owns it)
  SIGINT-ignoring EDITOR  fj stays alive (backstop stands down)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011ee2PWb2x6M2xYnmNFaWE9

stephen referenced this pull request from a commit 2026-09-06 17:13:33 +00:00

Replace the token store atomically, and cover the backstop end to end

stephen added 1 commit 2026-09-06 17:13:33 +00:00

Replace the token store atomically, and cover the backstop end to end ... 657fe635a2

The interrupt backstop force-exits from a runtime worker while the main thread
keeps running, and `write_file_store` truncated the token store before writing
it back. Between those two syscalls the file is zero bytes, so an exit landing
there destroyed every host's token, not just the one being changed. The reachable
path is the one the backstop exists for: a keychain call that hangs, its
synchronous error path, and no `.await` anywhere along it for the graceful select
to regain control on.

Write to a temporary file in the same directory, fsync, rename over the target.
Rename is atomic, so the window stops existing rather than being made smaller,
which is why this is the fix instead of widening the 250 ms grace. It is also the
right shape independently of interrupts: a crash or a power loss did the same
thing.

Cover it at the level where it lives. `arm()` and `force_exit()` are a signal
handler and a process that dies, so `tests/interrupt_backstop.rs` drives the
built binary the way `tests/version.rs` already does: park a command in a
synchronous read, send a real SIGINT, and require it to exit; and SIGINT a
logout across a spread of delays, requiring the store never to be left empty.
That takes `src/interrupt.rs` to 88% of lines with every function executed, so
the coverage red is closed by testing the dangerous function rather than by
excluding it.

The subprocess test is not the regression pin for the atomic write: the real
window is microseconds wide and it cannot reliably land in it. The pin is the
in-process test, which holds a read handle across the rewrite and requires it to
still see the old file. That fails deterministically against truncate-then-write.

`CLAUDE.md`'s shutdown note said never to reintroduce `spawn_blocking`, which is
broader than the defect. Narrowed to the actual rule: nothing on the shutdown
path may run on the blocking pool.

Refs #261

stephen referenced this pull request from a commit 2026-09-06 17:35:54 +00:00

Cover the foreground-child guard the same way, not by excluding it

stephen added 1 commit 2026-09-06 17:35:54 +00:00

Cover the foreground-child guard the same way, not by excluding it ... b2ebe8a238

The coverage gate was still 0.10 points short after the backstop tests. The two
obvious levers do not work. Covering the pager cannot move it at all, because
`output/pager.rs` is in COV_IGNORE and does not appear in the gate's table.
Excluding the one-line `ForegroundChild::enter()` wrappers is about seven lines,
worth roughly 0.02 points, and it would exclude the half of the diff that has
never been tested rather than the half that cannot be.

So test it. `fj auth setup-git` spawns `git config --global` inside the same
guard used at the `$EDITOR`, browser and `git` call sites, and it runs offline
against a scratch `HOME`. One test drives it and checks the helper lands in that
HOME; its `--dry-run` pair checks the spawn is genuinely skipped, so the guard is
shown to be on a path that is taken rather than decorative.

That covers `ForegroundChild::enter` and its `Drop` in the real process instead
of through an injected closure, and takes `cli/auth_setup_git.rs` to 92% of
lines. Locally the suite gains 46 covered lines, +0.21 points, against a 0.10
shortfall.

Refs #261

stephen referenced this pull request 2026-09-06 17:57:34 +00:00

Ctrl+C exits 1 or 130 depending on whether the command was pollable #263

stephen commented 2026-09-06 18:00:57 +00:00

Author

Owner

Copy link

APPROVE
Head reviewed: b2ebe8a238

Head confirmed against the API. Base 5c6d099d, mergeable, CI 3/3 green including the coverage job
that was red last round. Everything the prior verdict established is left alone; this covers only
the delta.

Constructed against a build of this head with XDG_CONFIG_HOME redirected to a scratch directory
and FJ_TOKEN, FJ_SESSION and BW_SESSION stripped from every child. No live forge, no real
credentials.

The headline: I reverted the atomic write, and the result is split

Both halves, stated whichever way they went:

                                                          mutant (truncate-then-write)
src/auth/mod.rs  write_file_store_replaces_the_file...     FAILED   exit 101
tests/interrupt_backstop.rs  sigint_during_a_logout_...    ok       exit 0

The unit test fails deterministically, on the assertion it was written for:

thread 'auth::tests::write_file_store_replaces_the_file_instead_of_truncating_it' panicked at
src/auth/mod.rs:456:9:
assertion `left == right` failed: the pre-existing handle must still see the old store; seeing
anything else means the target was rewritten in place, which is the window that can leave it empty

The 231-line subprocess test passes against the reverted write. It does not pin the atomic write.

So the finding is closed, and not by the file that looks like it closes it. The pin is the
in-process test, which opens a read handle before the rewrite and requires it to still see the old
inode afterwards. That is a real discriminator: rename leaves the handle on the old inode,
truncate-in-place does not, and it needs no signal timing to work.

What makes this an approve rather than a finding is that the PR says so first. The body states,
unprompted: "The subprocess test is not the regression pin for the atomic write. The real truncate
window is microseconds wide and no test can reliably land a signal in it; run against the pre-fix
write it passes. The pin is the in-process test." I tested it before reading that paragraph closely
and got the same two results. An author who characterises the weaker half of their own test suite
correctly is doing the thing this gate exists to check for.

Binary identity, because the prior gate lost a batch of runs to exactly this. Mutant
99ae178f9749474092cd8804640c5f01, real head 7555b2c4c0cf42929a439ba756d1fde4, checked at the
point of use, and corroborated at source level each time: mutant truncate(true)=1 NamedTempFile=0,
head truncate(true)=0 NamedTempFile=1. On the restored head both tests pass, the unit test and all
five in interrupt_backstop.

An incidental confirmation the fix is a replacement and not an addition: the mutant would not
compile until I restored OpenOptions and OpenOptionsExt to the imports. The head removed them,
so the truncate machinery is gone from the file rather than left sitting beside the new path.

1. The write is atomic, on all four counts

• Same directory. tempfile::NamedTempFile::new_in(parent) where parent is the target's own
  parent, defaulting to . when there is none, with the reason in a comment: rename is only atomic
  within one filesystem. Not a temp dir.
• Permissions. set_permissions(0o600) on the temp file before any content is written, so
  the window where a world-readable file holds tokens does not exist. persist carries the mode
  across, and set_file_store_permissions after the rename is belt-and-braces.
• Durable before the rename. sync_all() on the temp file precedes persist, commented as
  "so a crash after it cannot leave the new name pointing at unflushed content".
• Failure paths clean up. NamedTempFile deletes on drop, so every ? in the function removes
  the temp file. The rename failure path is persist(...).map_err(|e| e.error), which drops the
  NamedTempFile carried in the PersistError and so also cleans up, with context naming the
  target. No partial token store is left in the config directory.

One gap, non-blocking. The parent directory is not fsynced after the rename, so the directory
entry is not itself crash-durable. That does not affect the property this PR is about, since rename
is atomic for any observer regardless, and it is a different failure mode (power loss, not
interrupt). Worth knowing rather than fixing here.

2. The other sites: claude-8's answer is accurate

I grepped independently before reading its claim. Every fs::write, File::create and
truncate(true) in src/, minus test fixtures, downloaded artifacts and man-page output, leaves
these persistent-state writers on the non-atomic shape:

src/config/hosts.rs:115      src/cli/config.rs:84       src/cli/alias.rs:79
src/cli/work_session.rs:75   src/cli/stack_state.rs:208

The body names exactly those five plus the token store it fixed, says config/hosts.rs is next
because fj auth login writes it on the same interrupted path, and gives a real reason for not
batching it: that file has no explicit mode today, so picking one is a permissions decision rather
than a bug fix. That matches my grep with nothing missing and nothing invented. The pattern is
confirmed, the scope choice is defensible, and it is recorded rather than absorbed.

3. The coverage tests: covered, with one soft spot

None of the seven added tests is pure padding. The two I expected to be were not:
a_guarded_child_spawn_completes_and_writes_only_inside_the_scratch_home asserts the credential
helper was actually installed for the host asked for, and setup_git_dry_run_spawns_nothing_and_ writes_nothing asserts .gitconfig does not exist. Together they show the guarded spawn is reached
in one case and skipped in the other, which is what stops the guard being decorative. Both assert
behaviour someone would care about breaking.

The soft spot is arming_the_backstop_does_not_delay_an_ordinary_command. It bounds
fj auth status at 15 seconds. Measured on this head, three runs: 0.021s, 0.012s, 0.018s. That is
roughly a 700x margin, so the test pins "the backstop does not hang an ordinary command" and cannot
detect anything short of a near-hang; a regression adding five seconds would pass it. The assertion
is real and worth having, the name over-claims what it holds. Not blocking, and worth one word in
the name or a tighter bound.

4. The rest

The AI trailer is gone. Zero matches for Generated with, claude.ai/code, Co-Authored-By,
and zero em-dashes or en-dashes in the body. The prior round's second blocking finding is closed.

CLAUDE.md is narrowed correctly and belongs here. The rule now reads "Nothing on the shutdown
path may run on the blocking pool: dropping the runtime waits for spawn_blocking tasks, and that
wait is the original hang (fj#257). spawn_blocking is still the right tool elsewhere." That is
true, it names the mechanism rather than banning an API, and the last sentence forecloses the
over-reading the prior verdict predicted. It belongs in this PR because the invariant it documents,
wrapping any stdio-inheriting Command::status() in ForegroundChild::enter(), is created by this
fix and would otherwise be discovered by breaking it.

Exit 1 versus exit 130 is answered, not deflected. The prior verdict's objection was that the
body read it as cosmetic. It now says "Not acceptable as a permanent state", explains that the
graceful path returns 1 only because the interrupt surfaces as an ordinary error, agrees anything
keying on 130 will be wrong most of the time, and defers it on the grounds that fixing it changes
the contract of every interrupted command. That is the right disposition and the right reason.

What I did not verify

I did not re-derive the prior gate's timing work, the 250 ms grace, the spawn-site audit, or the
backstop-cannot-block-shutdown result. The brief ruled those settled and I took them as settled.

I did not measure coverage myself. CI's coverage job is green on this head, and the prior round's
red is therefore resolved by whatever the added tests moved; I checked whether the tests assert
things, which is the part a number cannot tell you.

Do not merge; that is the operator's.

APPROVE Head reviewed: b2ebe8a238cb4ee2254688bf090d24615def8490 Head confirmed against the API. Base `5c6d099d`, mergeable, CI 3/3 green including the coverage job that was red last round. Everything the prior verdict established is left alone; this covers only the delta. Constructed against a build of this head with `XDG_CONFIG_HOME` redirected to a scratch directory and `FJ_TOKEN`, `FJ_SESSION` and `BW_SESSION` stripped from every child. No live forge, no real credentials. # The headline: I reverted the atomic write, and the result is split Both halves, stated whichever way they went: ``` mutant (truncate-then-write) src/auth/mod.rs write_file_store_replaces_the_file... FAILED exit 101 tests/interrupt_backstop.rs sigint_during_a_logout_... ok exit 0 ``` The unit test fails deterministically, on the assertion it was written for: ``` thread 'auth::tests::write_file_store_replaces_the_file_instead_of_truncating_it' panicked at src/auth/mod.rs:456:9: assertion `left == right` failed: the pre-existing handle must still see the old store; seeing anything else means the target was rewritten in place, which is the window that can leave it empty ``` The 231-line subprocess test passes against the reverted write. It does not pin the atomic write. **So the finding is closed, and not by the file that looks like it closes it.** The pin is the in-process test, which opens a read handle before the rewrite and requires it to still see the old inode afterwards. That is a real discriminator: rename leaves the handle on the old inode, truncate-in-place does not, and it needs no signal timing to work. **What makes this an approve rather than a finding is that the PR says so first.** The body states, unprompted: "The subprocess test is not the regression pin for the atomic write. The real truncate window is microseconds wide and no test can reliably land a signal in it; run against the pre-fix write it passes. The pin is the in-process test." I tested it before reading that paragraph closely and got the same two results. An author who characterises the weaker half of their own test suite correctly is doing the thing this gate exists to check for. **Binary identity, because the prior gate lost a batch of runs to exactly this.** Mutant `99ae178f9749474092cd8804640c5f01`, real head `7555b2c4c0cf42929a439ba756d1fde4`, checked at the point of use, and corroborated at source level each time: mutant `truncate(true)=1 NamedTempFile=0`, head `truncate(true)=0 NamedTempFile=1`. On the restored head both tests pass, the unit test and all five in `interrupt_backstop`. An incidental confirmation the fix is a replacement and not an addition: the mutant would not compile until I restored `OpenOptions` and `OpenOptionsExt` to the imports. The head removed them, so the truncate machinery is gone from the file rather than left sitting beside the new path. # 1. The write is atomic, on all four counts - **Same directory.** `tempfile::NamedTempFile::new_in(parent)` where `parent` is the target's own parent, defaulting to `.` when there is none, with the reason in a comment: rename is only atomic within one filesystem. Not a temp dir. - **Permissions.** `set_permissions(0o600)` on the temp file *before* any content is written, so the window where a world-readable file holds tokens does not exist. `persist` carries the mode across, and `set_file_store_permissions` after the rename is belt-and-braces. - **Durable before the rename.** `sync_all()` on the temp file precedes `persist`, commented as "so a crash after it cannot leave the new name pointing at unflushed content". - **Failure paths clean up.** `NamedTempFile` deletes on drop, so every `?` in the function removes the temp file. The rename failure path is `persist(...).map_err(|e| e.error)`, which drops the `NamedTempFile` carried in the `PersistError` and so also cleans up, with context naming the target. No partial token store is left in the config directory. **One gap, non-blocking.** The parent directory is not fsynced after the rename, so the directory entry is not itself crash-durable. That does not affect the property this PR is about, since rename is atomic for any observer regardless, and it is a different failure mode (power loss, not interrupt). Worth knowing rather than fixing here. # 2. The other sites: claude-8's answer is accurate I grepped independently before reading its claim. Every `fs::write`, `File::create` and `truncate(true)` in `src/`, minus test fixtures, downloaded artifacts and man-page output, leaves these persistent-state writers on the non-atomic shape: ``` src/config/hosts.rs:115 src/cli/config.rs:84 src/cli/alias.rs:79 src/cli/work_session.rs:75 src/cli/stack_state.rs:208 ``` The body names exactly those five plus the token store it fixed, says `config/hosts.rs` is next because `fj auth login` writes it on the same interrupted path, and gives a real reason for not batching it: that file has no explicit mode today, so picking one is a permissions decision rather than a bug fix. That matches my grep with nothing missing and nothing invented. The pattern is confirmed, the scope choice is defensible, and it is recorded rather than absorbed. # 3. The coverage tests: covered, with one soft spot None of the seven added tests is pure padding. The two I expected to be were not: `a_guarded_child_spawn_completes_and_writes_only_inside_the_scratch_home` asserts the credential helper was actually installed for the host asked for, and `setup_git_dry_run_spawns_nothing_and_ writes_nothing` asserts `.gitconfig` does not exist. Together they show the guarded spawn is reached in one case and skipped in the other, which is what stops the guard being decorative. Both assert behaviour someone would care about breaking. **The soft spot is `arming_the_backstop_does_not_delay_an_ordinary_command`.** It bounds `fj auth status` at 15 seconds. Measured on this head, three runs: 0.021s, 0.012s, 0.018s. That is roughly a 700x margin, so the test pins "the backstop does not hang an ordinary command" and cannot detect anything short of a near-hang; a regression adding five seconds would pass it. The assertion is real and worth having, the name over-claims what it holds. Not blocking, and worth one word in the name or a tighter bound. # 4. The rest **The AI trailer is gone.** Zero matches for `Generated with`, `claude.ai/code`, `Co-Authored-By`, and zero em-dashes or en-dashes in the body. The prior round's second blocking finding is closed. **`CLAUDE.md` is narrowed correctly and belongs here.** The rule now reads "Nothing on the shutdown path may run on the blocking pool: dropping the runtime waits for `spawn_blocking` tasks, and that wait is the original hang (fj#257). `spawn_blocking` is still the right tool elsewhere." That is true, it names the mechanism rather than banning an API, and the last sentence forecloses the over-reading the prior verdict predicted. It belongs in this PR because the invariant it documents, wrapping any stdio-inheriting `Command::status()` in `ForegroundChild::enter()`, is created by this fix and would otherwise be discovered by breaking it. **Exit 1 versus exit 130 is answered, not deflected.** The prior verdict's objection was that the body read it as cosmetic. It now says "Not acceptable as a permanent state", explains that the graceful path returns 1 only because the interrupt surfaces as an ordinary error, agrees anything keying on 130 will be wrong most of the time, and defers it on the grounds that fixing it changes the contract of every interrupted command. That is the right disposition and the right reason. # What I did not verify I did not re-derive the prior gate's timing work, the 250 ms grace, the spawn-site audit, or the backstop-cannot-block-shutdown result. The brief ruled those settled and I took them as settled. I did not measure coverage myself. CI's coverage job is green on this head, and the prior round's red is therefore resolved by whatever the added tests moved; I checked whether the tests assert things, which is the part a number cannot tell you. Do not merge; that is the operator's.

stephen merged commit 4bba1a4a89 into main 2026-09-06 18:00:58 +00:00

stephen deleted branch fix/sigint-backstop 2026-09-06 18:00:59 +00:00

stephen referenced this pull request from a commit 2026-09-06 18:01:00 +00:00

Make Ctrl+C work when the command future is not pollable (#261)

stephen referenced this pull request from a commit 2026-09-06 18:18:17 +00:00

Keep the strict coverage report on the tested surface

stephen referenced this pull request 2026-09-06 18:18:29 +00:00

Cover git config and editor glue #264

stephen referenced this pull request from a commit 2026-09-06 18:21:41 +00:00

Keep the strict coverage report on the tested surface

stephen referenced this pull request 2026-09-06 18:29:08 +00:00

Cover git config and editor glue #264

stephen referenced this pull request 2026-09-06 21:52:49 +00:00

Record sixteen of the twenty-one undocumented changes in [Unreleased] #266

stephen referenced this pull request from a commit 2026-09-07 02:07:12 +00:00

Record sixteen of the twenty-one undocumented changes in [Unreleased] (#266)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

backlog

Accepted but not scheduled

  

blocked

Blocked or stalled waiting on external progress

  

converted

Converted into tracked work

  

forseti:changes-requested

Forseti review found unaddressed findings; cleared on a clean re-review at the current head.

  

opportunity

Potential product or business opportunity

  

p1

Priority 1

  

p2

Priority 2

  

p3

Priority 3

  

parked

Paused intentionally until later

No labels

backlog

blocked

converted

forseti:changes-requested

opportunity

p1

p2

p3

parked

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rasterstate/fj!261

No description provided.