Skip to content

fix(runner): write the exit statuses to an open descriptor and fix the wording - #568

Merged
ThomasK33 merged 6 commits into
mainfrom
test/runner-status-fd
Nov 12, 2026
Merged

ThomasK33 merged 6 commits into
mainfrom
test/runner-status-fd

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented Nov 12, 2026 •

Copy link
Copy Markdown
Member

Why

#554 merged with four follow-ups from its readiness review. This PR makes three of them and records why the fourth stays open.

The first push of this PR opened the status file with exec {status_fd}>..., a Bash 4.1 redirection. Bash 3.2, the macOS /bin/bash, does not know it. A review found that, and a verifier showed the effect: under bash 3.2 the exec failed, and because the runner sets an EXIT trap, bash then exited 0 after the first Running: line, without running a spec. That was a false pass, and I had declined the review finding wrongly. The runner now uses descriptor 9, a fixed number that bash 3.2 can open.

What changed

  1. The status file opens before the subshell starts, on descriptor 9. In fix(runner): name why a file failed when nvim exits non-zero without a reason #554, the file's subshell opened exit_statuses in the state folder by path when its file ended. A subshell that ended while the cleanup's rm walked that folder made the file during the walk, so rm could not remove the folder: Directory not empty, and the folder stayed in TMPDIR. The runner now opens the file as descriptor 9 before it starts the subshell, and the subshell writes to it. After the cleanup starts, the subshell opens no path. The subshell closes descriptor 9 for Neovim and tee, which do not need it. The runner closes descriptor 9 when the subshell has ended, and then reads the file back by path. When it cannot read the file, it uses the exit status of the subshell.
    I chose a fixed descriptor and not a BASH_VERSINFO refusal like scripts/tla/check-lib.sh has, because descriptor 9 works on bash 3.2. The runner runs on bash 3.2 and on bash 5.
    Under strace -f, which slows every traced call, I sent SIGTERM to a runner whose file had made 2000 files in the state folder. Main left exit_statuses in TMPDIR and printed one rm error in 5 of 5 runs. This PR left nothing and printed no error in 5 of 5 runs.
  2. A test for the fallback. A stub Neovim deletes the state folder, so the runner cannot read the statuses back, and a stub tee exits 3. The runner must fail the file and print FAILED: <file>: nvim exit 3: the fallback cannot tell the stages apart, so it names the status as Neovim's. A variant whose subshell exits with the status of Neovim only is now red.
  3. A check that would have caught the first push, and one for a failed open. CI runs bash 5, which accepts {status_fd}, so no CI run could see it. A test now fails when the runner source holds a {name}> redirection. Only this test fails on the first push. A second test makes mktemp -d return a folder that does not exist, so descriptor 9 cannot open, and expects a non-zero exit and no "All integration tests passed". On bash 3.2 and on bash 5 a failed exec 9> exits 1 under set -e.
  4. README and comments say what is true.
    • A file with no summary and a status of 124, 137 or 143 gets two lines, the no-summary line and the TIMEOUT: line. A file with no summary and another status, for example a crash with 139 or 134, gets only the FAILED: line.
    • An outside SIGTERM that reaches timeout gives 124 with uutils. With GNU it gives 143 only when the child dies of the signal, and the child's own status when the child handles it. Real Neovim exits 1, so there is no TIMEOUT: line.
    • "Because nvim did not exit in time" no longer contradicts the hedged TIMEOUT: text.
    • A signal name appears for a status from 129 to 192, except 160 and 161. Bash on Linux names signals 1 to 31 and 34 to 64, so 160 and 161 get no name, and neither does a status from 193 to 255.
    • The cleanup sentences describe the open status file and the fallback. A runner comment no longer says "as on main".

Declined: the process group id can be reused

The runner kills the process group of timeout after a file ends with a non-zero Neovim status. A process group id that another process reused in the few milliseconds after timeout ended would get SIGKILL. This is a limit, and I do not change it.

Tests

Changed or new runner spec tests (the names are the columns of the table):

  • no_named_fd: the runner source holds no {name}> redirection.
  • open_fails: descriptor 9 cannot open, so the runner stops with a non-zero exit and no pass.
  • fd_closed: the stub Neovim, the stub tee and the stub rm that the runner calls after it read the statuses look at their own descriptors and find none that points to the status file.
  • n1_open_after_cleanup: SIGTERM while a stub rm waits between its files and its folders, and a stub tee ends half a second after its input. Nothing is left in TMPDIR, and the runner prints no Directory not empty. It reproduces the strace result without strace.
  • n5_fallback: the test of item 2.
  • tee_stage, both_stages, cleanup (tests of fix(runner): name why a file failed when nvim exits non-zero without a reason #554) and window (a test of fix(runner): stop the running file at once on Ctrl-C and close the SIGTERM window #532): run again because the code they cover changed.

Each test ran against each mutant of the runner, with uutils timeout 0.10.0 and with GNU timeout 9.7. The two builds gave the same table. Red means the test fails.

Variant no_named_fd open_fails fd_closed n1_open_after_cleanup n5_fallback tee_stage both_stages cleanup window
this PR green green green green green green green green green
main's runner green red green red green green green green green
nvim keeps descriptor 9 green green red green green green green green green
tee keeps descriptor 9 green green red green green green green green green
the subshell writes by path green green green red green green green green green
the subshell exits with the status of Neovim only green green green green red green green green green
the statuses are never read green green green green green red red green green
the runner keeps descriptor 9 open after it read the statuses green green red green green green green green green
the runner never opens the status file green red green green green red red green green
first push of this PR ({status_fd}, a Bash 4.1 redirection) red green green green green green green green green
a failed open is ignored green red green green green green green green green

bash 3.2

GNU bash 3.2.57 built from the GNU source (checksum checked), as the interpreter of the runner, with real Neovim 0.12.3 and GNU timeout. Two specs: a_spec.lua passes and b_spec.lua fails. bash on PATH is 3.2, as on a stock Mac, or 5.3, as with Homebrew bash first on PATH.

Runner Exit Specs that ran Output
main, with either bash on PATH 1 both Total Success: 1 Total Failed: 1, b_spec.lua listed as failed
first push of this PR, with either bash on PATH 0 none (it printed Running: a_spec.lua, then stopped) line 72: exec: {status_fd}: not found, no totals
this PR, with either bash on PATH 1 both the same output as main, nothing left in TMPDIR

Real Neovim, strace race

Real Neovim 0.12.3. The spec makes 2000 files in the state folder and then waits. A stub tee sleeps 0.05 s after its input ends, and strace -f traces openat, unlinkat, rmdir, kill and execve. SIGTERM goes to the runner. The probe is a copy of the verifier's, with paths changed.

Runner Items left in TMPDIR one second later rm error lines
main, 5 runs the state folder with exit_statuses, in 5 of 5 1 in each of the 5
this PR, 5 runs none, in 5 of 5 0 in each of the 5

Whole runner, main versus this PR

Stdin is /dev/null and TMPDIR is unset. I ran this on the tree before the last merge of main. The rows are identical: 135 per-file rows each (33 files with a Running line and three count lines, plus three totals), no difference, and no FAILED, TIMEOUT, E886, Killed or No such file line.

Neovim main this PR
0.12.3 631 passed, 0 failed, 0 errors 631 passed, 0 failed, 0 errors
v0.8.0 613 passed, 0 failed, 0 errors 613 passed, 0 failed, 0 errors

With TMPDIR set to a folder with a space and a % (what the CI guard uses), this PR on 0.12.3 also gives 631 passed, 0 failed, 0 errors.

Gate

mise run clean && mise run all on the same tree: the formatter changed no file, luacheck reports 0 warnings and 0 errors in 191 files, and busted reports 1837 successes, 0 failures, 0 errors, 31 pending. shellcheck on scripts/*.sh and actionlint pass.

A merge of main came after that run: 9 commits, none in scripts/ or in the runner spec. They add three integration specs (diff_legacy_cleanup, mention_immediate_send, selection_demotion_in_terminal) and delete diff_accept_trailing_newlines, so 33 files become 35. An independent rerun of the whole runner on the exact head gave 669 passed on Neovim 0.12.3 and 651 on v0.8.0, 0 failed, 143 rows equal to main's. On the merged tree I ran the static checks (mise run check: exit 0, luacheck 0 warnings and 0 errors in 193 files) and the whole runner spec (73 tests, all passed).

Not covered

  • Declined from the round-2 review: the source check looks only for {name}> and {name}< redirections, not every bash 4 construct (the whole runner spec passes 73 of 73 under bash 3.2, so none is in the runner today). The spec Neovims no longer inherit a descriptor 9 that the caller passed in (neither version writes to it, and results are equal). On a host without /proc, fd_closed checks nothing. A follow-up PR in this run, with the fix for the flaky dies by SIGINT on Ctrl-C test, makes it report pending there instead of passing.

Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh

…e wording

- The runner opens the status file in the state folder before it starts the
  file's subshell, and the subshell writes to the open descriptor. A subshell
  that ends while the cleanup removes the folder opens no path, so it cannot
  make a file in a folder that is being removed. Under `strace -f`, main left
  the state folder with `exit_statuses` and printed `Directory not empty` in
  every run. nvim and `tee` close the descriptor.
- A test with a slow stub `rm` and a late stub `tee` reproduces that without
  strace and is red on main. A test makes the runner unable to read the
  statuses back (the stub nvim deletes the state folder) and checks that it
  uses the exit status of the subshell. A test checks that nvim and `tee` do
  not hold the status file.
- scripts/README.md and two runner comments say what is true: a file with no
  summary and a status of 124, 137 or 143 gets two lines and a crash with 139
  or 134 gets one, an outside SIGTERM gives 143 with GNU `timeout` only when
  the child dies of it, "did not exit in time" no longer contradicts the
  hedged TIMEOUT text, a status from 193 to 255 gets no signal name, and the
  cleanup sentences describe the open descriptor.

_Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
_Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
_Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
@ThomasK33
ThomasK33 marked this pull request as ready for review November 12, 2026 16:44
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-12T19:10:03.575238Z eaedba1 Manual request
🔒 Security Review ✅ Completed 2026-11-12T19:10:51.743328Z eaedba1 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: a53dff7919

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a53dff7919

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

_Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
… comments

- `exec {status_fd}>...` is a Bash 4.1 redirection. Under bash 3.2 (the macOS
  /bin/bash) it fails, and with an EXIT trap set the runner then exits 0
  without running a spec: a false pass. The runner now opens descriptor 9 and
  the subshell closes it for nvim and `tee`. This is the fixed descriptor
  that bash 3.2 runs: both specs run and the runner exits 1 for the failing
  one, as it does on main.
- A test fails when the runner source holds a `{name}>` redirection, which CI
  (bash 5) runs without complaint. A test makes `mktemp -d` return a folder
  that does not exist, so the descriptor cannot open, and expects a non-zero
  exit and no "All integration tests passed". The descriptor test also checks
  that the runner closes its own descriptor after it read the statuses.
- scripts/README.md names the exception to the signal names: bash on Linux
  names signals 1 to 31 and 34 to 64, so 160 and 161 get no name. The runner
  comment on descriptor 9 names the stage that closes it, and a comment no
  longer says "as on main".

_Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
_Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: eaedba1a3d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: eaedba1a3d

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@ThomasK33
ThomasK33 added this pull request to the merge queue Nov 12, 2026
Merged via the queue into main with commit c7e2eb4 Nov 12, 2026
7 checks passed
@ThomasK33
ThomasK33 deleted the test/runner-status-fd branch November 12, 2026 21:09
hareki pushed a commit to hareki/claudecode.nvim that referenced this pull request Nov 14, 2026
…hing after a signal (coder#592)

## Why

`scripts/run_integration_tests_individually.sh` runs each integration
spec in its own Neovim. When a developer presses Ctrl-C, or CI sends
SIGTERM or SIGHUP, the runner must stop at once, remove what it made,
and leave no Neovim running. A calling shell loop must also stop. This
PR fixes four defects in that path and updates the README. The first
defect made a test fail on main. The others were gaps that
`scripts/README.md` listed, or that review of earlier heads of this PR
found.

## What changed

### 1. A failing cleanup no longer swallows Ctrl-C

The INT trap ran `cleanup_current_file` under `set -e`. When its `rm
-rf` exited non-zero, bash ended the trap there. The runner then exited
with status 1 and never re-raised SIGINT. A calling shell goes on after
a child that exited, so its loop ran the runner again and an interrupted
run looked green.

This is the failure of main's post-merge Tests run 43299040669, where
"dies by SIGINT on Ctrl-C, so a shell that calls it in a loop stops the
loop" got status 0 instead of 130. The handler now runs
`cleanup_current_file || true`.

Before coder#568 the file's subshell could create `exit_statuses` again while
the cleanup removed the state folder. That made `rm -rf` fail with
"Directory not empty", and it is the most likely trigger of the CI
failure. coder#568 closed that trigger. This change makes the handler safe
for any other `rm` failure too.

### 2. No temp item is left when a signal comes while the runner makes
them

The runner made four items with four `mktemp` calls and stored their
names afterwards. A signal in between left up to four items that no
cleanup knew. Now the runner picks the name of one folder with `mktemp
-u`, stores it, and only then makes the folder with `mkdir`. All items
live in that folder. `mkdir` fails when the name exists. The runner then
drops the name again.

INT, TERM and HUP now share one handler. Bash runs a trap after the
foreground command that it waits for, so a signal that comes while
`mkdir` makes the folder finds the folder. Before, bash ended on SIGTERM
and SIGHUP at once, ran the EXIT cleanup, and a `mkdir` that was still
running made the folder afterwards.

### 3. The cleanup renames the folder first, so the shell that starts
the Neovim sees it

The shell that execs `timeout` writes its process id to a file that the
cleanup reads. A signal before that write left the cleanup with nothing
to kill, and the shell then started a Neovim that ran on after the
runner died.

Now the cleanup moves the work folder into a fresh folder next to it,
made with `mktemp -d`, so the old path is gone at once. It then reads
the process id file in the moved folder, kills `timeout` and its process
group, and removes the fresh folder and the old path. A Neovim that
still runs between the move and the kill can make its state folder at
the old path again, so the old path goes last, after the kill. When the
grave or the move fails, the folder keeps its old name and the cleanup
works there. The shell writes its id, then looks for the folder at the
old path, and it exits when it finds none. So one of the two sees the
other. An earlier version of this PR used a `cancel` file inside the
folder. Codex showed that `rm -rf` removes that file before the folder,
which left a gap. The move has no such gap. The previous head of this PR
renamed the folder to `<work>.gone`. When a folder of that name existed,
`mv` moved the work folder into it and `rm` then deleted the content of
that folder. A name from `mktemp -d` cannot exist yet. The move also
closes a second gap: `tee` opened its output file inside the folder
while `rm` removed it, and `rmdir` then failed.

The handler ignores INT, TERM and HUP from its first line on, and so
does its `rm`. A second Ctrl-C reaches the `rm` too. With bash 3.2 (the
macOS `/bin/bash`) it stopped the `rm` half way, and the runner died
with 130 and left the renamed folder.

### 4. A Neovim that the cleanup kills no longer leaves its socket
folder in TMPDIR

Neovim makes `$TMPDIR/nvim.<user>/XXXXXX` for its server socket and
removes it when it exits by itself. The cleanup kills it with SIGKILL,
so every Ctrl-C left one. Main does the same. I ran the real runner with
a real Neovim and pressed Ctrl-C: main left `nvim.<user>/…` in `TMPDIR`,
and this head leaves nothing in `TMPDIR`. Each file now gets `TMPDIR`
and `XDG_RUNTIME_DIR` that point at a `tmp/` folder in its work folder,
so the socket folder goes away with the work folder. A work folder with
a name over 80 characters goes to `/tmp`. The runner counts characters,
not bytes, so a multibyte `TMPDIR` can still pass the limit (see "Not
proven"). Without that, a `TMPDIR` of about 92 to 105 characters made
the socket path pass the 107 bytes that libuv allows. libuv cut the
path, and a socket file named `tmp.XXXXXXXX` landed in the `TMPDIR` of
the developer (v0.8.0 left 38 in one run). This needs a writable `/tmp`.
Without one the folder stays in `TMPDIR`.

### README

`scripts/README.md` no longer lists the closed windows as open. It lists
the ones that remain (see "Not proven").

## Tests

Each new test fails on a runner without its change. I checked that with
the runner of main, with the runner of the earlier head of this PR
(c4e00f8), and with copies of the final runner that each lack one
change. A stub `rm`, `mv`, `cat`, `mktemp`, `mkdir`, `tee` or a
`BASH_ENV` file creates each moment, and no test depends on sleeps for
its verdict. One exception, the timing of the `during_rm` tests, is
under "Not proven".

- Defect 1: a stub `rm` removes everything and then exits with 1. The
calling shell's loop must stop with status 130.
- Defect 2: a `mktemp` stub and a `mkdir` stub send SIGINT right after
they made something. A `mkdir` stub also sends SIGTERM or SIGHUP to the
runner alone. In the test for the second `mkdir`, the stub `rm` of the
cleanup waits half way until that `mkdir` ends, so `mkdir` makes its
folder while `rm` runs. The runner must leave nothing in `TMPDIR`.
Another test checks that the runner removes no folder that it did not
make.
- Defect 3: a `BASH_ENV` file stops the shell that execs `timeout`
before its first command, or between the redirection that makes the
process id file and the write, until the test lets it go. The tests let
it go after the runner died, and while the stub `rm` has deleted the
files and not yet the folders. Two more tests let `tee` open its output
file at that moment. A test makes a folder where the process id file
belongs, so the write fails, and expects no Neovim. A test makes the
rename fail and expects the folder removed under its old name. A stub
`mv` waits while the shell of the file starts its Neovim, and the test
expects that Neovim to die: a cleanup that reads the process id and
kills before it moves the folder, or that reads, moves and then kills,
leaves it running. Nine tests send a second signal, INT, TERM or HUP
after each of the three, while the `rm` of the cleanup runs, and expect
that `rm` to live on and the first signal's status. Two tests check that
a runner that gets SIGTERM or SIGHUP dies of it. Bash stops a calling
loop only after a child that died of SIGINT, so for SIGTERM and SIGHUP
the calling shell prints the report that bash prints for a killed child
("Terminated", "Hangup") and the status, and it prints no report for an
exit with 143 or 129. A test puts a folder named `<work>.gone` next to
the work folder and expects the cleanup to leave it alone. A `cat` stub
holds the cleanup after the move and before the kill while the stub
Neovim writes into the old path, and the test expects nothing left.
- Defect 4: one test checks that `TMPDIR` and `XDG_RUNTIME_DIR` of the
file's Neovims are the `tmp/` folder of the work folder. A second test
runs the runner under a `TMPDIR` of 100 characters and checks that the
work folder is in `/tmp`, that a socket path in it stays under 107
bytes, that the folder is gone afterwards, and that nothing stays in the
long `TMPDIR`.
- Each signal test puts the status, the leftovers and the output of the
runner and of the calling shell into its failure message. The CI failure
would have explained itself.
- The stubs that hold a process wait 20 seconds at most. Against the
runner of main, which never calls `mkdir`, a wait without a limit hung
the spec for 28 minutes.
- The SIGHUP tests report pending where SIGHUP is ignored when the test
process starts, as under `nohup`. The test that reads which files the
stubs hold open reports pending on a host without `/proc/<pid>/fd`.

Failures of the new spec (105 tests) against other runners:

| Runner | Failing tests of 105 | Which |
|---|---|---|
| main (bcf1f08) | 31 | The CI test (cleanup fails), the status-file
test, the `TMPDIR` and long-`TMPDIR` tests, the nine second-signal
tests, the two death tests, the `mv` test, and the 15 signal-timing
tests |
| c4e00f8, the earlier head with the `cancel` file | 20 | The `TMPDIR`
test, the long-`TMPDIR` test, all nine second-signal tests, the two
death tests, SIGTERM and SIGHUP while `mkdir` 1 and 2 runs (4 tests),
SIGINT while `rm` is half way, SIGTERM and SIGINT with `tee` during `rm`
(2 tests) |
| 3ee7698, the previous head | 2 | The long-`TMPDIR` test and the
`<work>.gone` test |
| cleanup reads the process id and kills, then moves the folder | 1 |
The Neovim started while `mv` waits |
| cleanup reads the process id, moves the folder, then kills | 2 | The
Neovim started while `mv` waits, and SIGINT with `tee` during `rm` |
| handler ignores INT only | 6 | TERM and HUP as the second signal,
after each of the three |
| handler resets only EXIT and INT | 2 | The SIGTERM and SIGHUP death
tests |
| cleanup moves the folder to `<work>.gone` | 1 | The `<work>.gone` test
|
| no move to /tmp for a long `TMPDIR` | 1 | The long-`TMPDIR` test |
| cleanup removes only the old path, not the grave | 35 | Every test
that finds the grave left in `TMPDIR` |
| cleanup removes only the grave, not the old path | 2 | The live-write
test and the move fails |
| no move (cleanup works on the old name) | 3 | SIGINT while `rm` is
half way, SIGTERM and SIGINT with `tee` during `rm` |
| no `[ -d ]` check of the launcher | 1 | SIGINT between the redirection
and the write of the process id |
| `;` instead of `&&` in front of that check | 1 | The write of the
process id fails |
| no `\|\| true` on the cleanup in the handler | 1 | The cleanup fails |
| no TERM and HUP traps | 10 | SIGTERM and SIGHUP while `mkdir` 1 runs,
six second-signal tests, two death tests |
| name stored after `mkdir` | 5 | SIGINT right after `mkdir` 1, SIGTERM
and SIGHUP while `mkdir` 1 runs, two death tests |
| no guard on a failing `mkdir` | 1 | The name exists |
| no private `TMPDIR` | 2 | The `TMPDIR` test and the long-`TMPDIR` test
|
| no `trap '' INT TERM HUP` in the handler | 9 | All nine second-signal
tests |

Each row is one run of the whole spec, started in a session where SIGHUP
is not ignored, so every SIGHUP test ran.

## Evidence

- Final head cde2e40: `MISE_LOCKFILE=false mise run all` with GNU
timeout first on `PATH` exits 0 with 1936 successes, 0 failures, 0
errors, 36 pending, luacheck 0 warnings and 0 errors in 200 files, and 0
files changed by format. `mise run format-check`, `shellcheck` and
`shfmt` on the runner exit 0.
- The runner spec (105 tests) passed 20 of 20 times on the final head,
two runs at a time under the host's load: 15 with timeout from uutils
coreutils, 3 with GNU bash 3.2.57 as `bash` on `PATH` and as the
interpreter, and 2 with GNU timeout. These runs did not ignore SIGHUP,
so no test was pending.
- The whole runner with real Neovim gives the same counts as main
(f28eba7): 856 successes on 0.12.3 and 852 on v0.8.0. That holds with
`TMPDIR` unset, with a space and a `%` in it, and on v0.8.0 with a
`TMPDIR` of 92, 100 and 105 characters (0.12.3: 100). After each run
`TMPDIR` is empty and has no socket, and `/tmp` has no new folder of the
runner. No file got a TIMEOUT line. The previous head left 38 socket
files in a `TMPDIR` of 100 characters on v0.8.0.
- A real Ctrl-C after 7 to 10 seconds of the real runner and a real
Neovim, 15 times (5 of them with a `TMPDIR` of 100 characters): every
run exits 130, and nothing is left in `TMPDIR` after 1 second or after 5
seconds. Main leaves `nvim.<user>/…` socket folders in `TMPDIR`. A first
version of the move, which did not remove the old path, left
`tmp.X/state/nvim/…` in 7 of 15 such runs on my host (an independent
verifier saw 1 of 10). A live Neovim made that folder again between the
move and the kill. The `cat` stub test covers it.

## Not proven

- I did not prove that the status-file race caused the CI failure. The
deterministic repro uses a slow `rm` that reports failure. Main had 0
failures in 440 runs of the single test, in 2300 runs of the runner
under a calling shell, and in 3200 runs of the runner alone, so the
natural race did not reproduce.
- Windows that remain: SIGKILL gives the runner no time to clean up, so
the folder stays. A runner that leads its own session on a terminal
hangs up the foreground process group of the terminal when it dies, so
the shell of the file dies too and starts no Neovim. Without a terminal
that shell goes on, also when it has not written its process id yet, and
its Neovim runs until its own timeout. Bash ignores SIGQUIT, so the file
runs to its end. If the name from `mktemp -u` already exists and a
signal comes before the runner drops the name, the cleanup can remove
that folder. A second signal that comes before the first line of the
handler runs is not ignored.
- When the grave or the move fails, the folder keeps its old name. A
shell that acts while the delete runs can then still start a Neovim, and
the folder stays. Main has the same race. One test covers a failing
move, and it has no shell that acts at that moment.
- A work folder moves to /tmp only when `/tmp` is a writable folder.
Without one it stays in `TMPDIR`, and a socket path over 107 bytes is
cut by libuv and can name a file there. The limit of 80 characters
leaves room for a socket name of 20 characters in the folder
(`/tmp/nvim.<7 digits>.<n>`).
- The limit counts characters, not bytes. A `TMPDIR` with multibyte
characters can pass the 107 bytes without passing 80 characters, and
then leaves socket files in `TMPDIR` (a verifier saw 38 with a
48-character, 94-byte `TMPDIR` on v0.8.0, where main left 54).
- The death tests for TERM and HUP read bash's English "Terminated" and
"Hangup". In a translated locale they fail on a correct runner.
- An error exit (for example a failing `exec 9>`) followed by a signal
during its cleanup can leave the first cleanup's grave folder, with
empty `work/config` and `work/tmp`, in `TMPDIR`. The exit status stays
right and a calling loop stops. A verifier found this in 4 of 6 such
orders.
- A follow-up PR fixes the three items above.
- The `during_rm` tests let the held process go, wait one second, and
then let the `rm` go on. On a host so slow that the held process needs
more than one second for its next command, such a test can pass on a
runner that has the defect. A slow host cannot make it fail on a correct
runner.
- I tested bash 3.2 with a GNU build of 3.2.57, not with Apple's
`/bin/bash`.

---
_Generated with [`xum`](https://github.com/coder/xum) • Model:
`anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
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