Skip to content

fix(cli): a closed reader never panics a command - #190

Draft
medazizktata25 wants to merge 5 commits into
Guepard-Corp:mainfrom
medazizktata25:fix/closed-reader-never-panics-a-command
Draft

medazizktata25 wants to merge 5 commits into
Guepard-Corp:mainfrom
medazizktata25:fix/closed-reader-never-panics-a-command

Conversation

@medazizktata25

@medazizktata25 medazizktata25 commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #189. #179 is merged, so this branch is #189's commit plus four of its own: a72a9d9 (the println! sweep), 391c85a (status's copy-on-write notice), f20ba9f (--help and --version) and 9de7298 (branch -d's detached-workspace notice). When #189 merges, this needs one more rebase. println_safe! landed in #179.

The defect

Every bare println! panics when stdout's reader has gone away (failed printing to stdout: Broken pipe). The process then exits 101, the exit code of a crash. So gfs status | head -1 "crashed".

Worse, gfs schema diff ... | head turned its own verdict, 2 for breaking changes, into that same 101.

Measured on a sqlite repository, with the read end closed before gfs writes, 10 runs per command: 10 commands exited 101 on every run.

  • version
  • providers and providers sqlite
  • status and status --json
  • branch and branch --json
  • schema extract
  • schema diff
  • proxy status

The fix

All println! calls in the CLI crate now go through println_safe! (338 when this was written, 339 after the later cmd_branch.rs addition):

  • 335 were converted by a structural rewrite (ast-grep);
  • 3 were added by main since and are converted in the follow-up commits, plus one more in cmd_branch.rs that main gained later (9de7298).

println_safe! records the closed reader and leaves the exit code to the command.

  • Helpers that returned nothing: nine printing helpers returned (), so ? could not propagate from them. They now return Result, so a write error other than a closed pipe (EIO, a full disk) is reported as an error instead of panicking.

  • How the callers were found: by compiling with -D unused_must_use, which makes every caller that ignored the new Result a hard error. All 14 now propagate with ?.

  • The one deliberate discard: the --json error object in main.rs, written just before the process exits with that error's own code.

  • println_safe!(): gains an empty arm for the 54 bare newlines.

  • No tokens lost: a check confirmed every rewritten file is token-identical to the original apart from whitespace, the macro name and the added ?. The check itself catches a deleted argument.

  • --help and --version: clap printed these with e.print().expect(..), before any command runs, so gfs --help | head -1 exited 101. f20ba9f treats BrokenPipe there the way println_safe! does.

Verification

  • Closed-pipe harness: 15 commands, 10 runs each: the 10 above plus log, log --json, config user.name, fsck and fsck --json. A non-zero verdict is included (schema diff = 2).
    • Before: 10 commands exit 101.
    • After: 0 failing, and schema diff still exits 2 with its reader closed.
    • Guard: each command must first produce its real output with an open reader, so pointing the harness at /bin/echo fails 15 of 15 rather than passing.
  • Regression test: a_closed_reader_keeps_the_verdict_of_every_command in tests/json_output.rs runs 7 non-fsck commands on a sqlite repository, so it needs no container runtime. It was checked against two deliberate breaks:
    1. one bare println! restored in gfs branch → the test fails with 101;
    2. the macro changed back to exit(0) on a broken pipe, main's old behavior → schema diff gets 0 instead of 2.
  • Full gfs-cli suite, Docker suites included (on colima): 108 passed across 25 test binaries. Two fail: postgres_checkout_lifecycle and clone_range_and_temporal_hydration_elide, both failing identically on main at e51fc61:
    • postgres_checkout_lifecycle refuses on the system catalog base/5/1249, documented as left failing in 5683650;
    • clone_range_and_temporal_hydration_elide reports t0=0 t1=0.
  • Lint: cargo fmt and cargo clippy --all-targets -D warnings are clean for gfs-cli, also with -D unused_must_use.

Not verified

  • Remaining print! calls (5): not converted; this change covers println! only.
    • Four are in cmd_config and write no newline, so their failing flush happens at exit and is ignored; config already survived the harness.
    • The fifth, cmd_query.rs:91, prints a query's output and probably panics on a closed reader too. Not reproduced.
  • Commands not covered by the harness: commands that need a running database or a container runtime were not exercised with a closed reader: query, compute, user, clone, import and export. They go through the same macro, but no test proves it.

After the rebase

cargo test --workspace --no-fail-fast on the rebased branch: json_output passes (26/26). The 18 failures are the Docker-backed e2e suites, which could not run (no Docker daemon on the test machine); main at b10dc37 fails the same 18 there. The closed-pipe harness and the Docker suites above were last run before the rebase and were not re-run. cargo clippy -D warnings fails on 54 must_use errors in gfs-domain both with and without this branch's changes (55 total either way), so it could not be used as a gate here.

@medazizktata25

Copy link
Copy Markdown
Contributor Author

Draft until #179 and #189 merge: this repo can't base a PR on a fork branch, so the diff here currently includes their commits. Once they're in, this branch gets rebased onto main so only its own commits remain, then it comes out of draft.

std::env::args() panics on an argument that is not UTF-8, so gfs exited
101 -- the code of any crash -- with the undecodable bytes echoed in the
panic message. Read args_os instead and turn the first bad argument into
a clap usage error: exit 3 like any value that will not parse, the
position named, the bytes never repeated. --json still gets its error
object. argv[0] is converted lossily rather than refused, since it is the
launch path and not something the user typed.
Every bare println! panicked on BrokenPipe, so `gfs status | head -1`
exited 101 -- the code of a crash -- and `gfs schema diff | head` turned
"breaking changes" (2) into the same 101. All 335 now go through
println_safe!, which records the closed reader and leaves the exit status
to the command.

Nine printing helpers returned () and now return Result, so a write
error other than a closed pipe propagates instead of panicking; every
caller was found by compiling with unused_must_use denied. The one
deliberate discard is main's --json error object, written just before
the process exits with that error's own code. println_safe!() gains an
empty arm for the 54 bare newlines.
Main added three bare println! calls for the full-copy snapshot warning;
on a closed reader they panicked into 101 like the ones this branch
already converted.
clap's help and version output was printed with
`e.print().expect(..)`, so a reader that had gone away turned them into a
panic and exit 101:

  gfs --help | head -1                 -> PIPESTATUS 101 0
  (sleep 1; gfs --version) | true      -> PIPESTATUS 101 0

with "panicked at crates/applications/cli/src/lib.rs:847 ... BrokenPipe"
on stderr. They are printed before any command runs, so println_safe!
never saw them.

BrokenPipe is now treated the way println_safe! treats it: output stops,
the exit status (0) stands. Any other write error is returned rather than
panicking. --help and --version join the closed-reader verdict table.
…ntln_safe!

A bare println! landed on main after this branch's sweep (1b26050, the reserved
'detached' branch name guard), so 'gfs branch -d detached' on a repository that
already carries such a branch still panicked into exit 101 on a closed reader.
delete_branch already returns Result, so the call takes a ?.
@medazizktata25

Copy link
Copy Markdown
Contributor Author

Update 2026-10-26: rebased after #179 merged, plus one new commit

  1. gfs fsck: report unreachable and dangling objects without removing anything #179 merged into main as b10dc37.
  2. Rebased onto the rebased fix(cli): a non-UTF-8 argument is a usage error, not a crash #189 (ef55f24), keeping only this branch's own commits. Tip went from f19a755 to f20ba9f: a72a9d9 (the println! sweep), 391c85a (status's copy-on-write notice), f20ba9f (--help and --version). No conflicts.
  3. Found one println! that main gained after the sweep, in cmd_branch.rs (the reserved-detached guard from 1b26050). It would still panic with exit 101 on a closed reader. Converted it to println_safe! in 9de7298; delete_branch already returns Result.
  4. Description updated: stacked on fix(cli): a non-UTF-8 argument is a usage error, not a crash #189 only, commit list corrected, the --help/--version commit described, and the converted-call count changed from 338 to 339.
  5. Tests: json_output passes (26/26). The same 18 Docker-backed e2e tests fail here as on main (no Docker daemon). Not re-run after the rebase: the closed-pipe harness and the Docker suites. No test exercises the cmd_branch.rs path from step 3. cargo clippy -D warnings fails on 54 must_use errors in gfs-domain, with and without this branch, so it could not be used as a gate.
  6. When fix(cli): a non-UTF-8 argument is a usage error, not a crash #189 merges, this needs one more rebase.

This branch is waiting to be deployed

1 waiting deployment
pr-validation — 9de72986 Waiting Oct 26, 2026 by medazizktata25 via build-and-test (ubuntu-latest) #294
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant