Repository navigation
fix(scripts): remove the grave of an interrupted cleanup and pin the runner's limits - #605
Conversation
…ests in the C locale Codex threads on cde2e40. Bash counts characters with ${#var} in a UTF-8 locale, but a socket path limit is in bytes. A TMPDIR of 50 multibyte characters stayed below 80 characters, kept its work folder, and libuv cut the socket path into the TMPDIR of the developer. The runner now counts bytes with LC_ALL=C. A test with a TMPDIR of 30 multibyte characters fails without it. The two death tests read the English reports Terminated and Hangup, which a translated libc prints in another language. The calling shell of those tests now runs with LC_ALL=C. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
Verifier finding N1 on cde2e40. The cleanup of an error exit (the EXIT trap) makes a grave for the work folder. A signal that comes while it runs starts a second cleanup through the signal handler. The second one made its own grave, found the old path gone, and removed only its own grave, so the first grave stayed in TMPDIR with config/ and tmp/ in it. The cleanup now stores the name of its grave in a global before it makes the folder: mktemp -u, then mkdir -m 700. A second cleanup finds that name, moves nothing again, reads the process id file in the grave when it exists, and removes the grave. The EXIT path does not ignore signals, so Ctrl-C still ends with 130 and stops a calling loop. The mktemp stub now gives its fixed name only to a call without a template, so the grave name stays random in the test of a work folder with a .gone neighbor. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…mv and at rm The mkdir stub drops the state folder, so opening the status file fails and the runner takes an error exit. Its EXIT-trap cleanup waits at the mv stub or at the rm stub. Six tests send INT, TERM or HUP then and expect 130, 143 or 129 and no folder left. The INT tests run in a calling loop and expect it to stop. On cde2e40 four of the six fail: INT at mv and at rm, TERM and HUP at mv. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…r leftover checks see the work folder Verifier finding M2. The runner moves a work folder with a name over 80 bytes to /tmp. The signal tests made their TMPDIR under the temp folder of the test, so a test process with a TMPDIR of about 42 characters or more sent every work folder to /tmp, where the tests did not look. Mutants that leave folders then passed. The tests now ask short_temp() for the TMPDIR of the runner. It is dir/tmp when the work folder fits, and a short /tmp/ccr.XXXXXX folder otherwise, removed after the test. An assert checks the length. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…m both sides Verifier finding M1. The long-TMPDIR test used one TMPDIR of 100 characters, so a runner with a limit of 92 or 100 passed the whole spec although its socket paths passed 107 bytes. Two tests now give the runner a work folder name of exactly 80 and 81 bytes through the mktemp stub, and expect the first to stay and the second to move to /tmp. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
… deletes the folder Verifier finding M3. A runner that deleted the folders first and killed the Neovim afterwards passed the whole spec. The stub Neovim now notes its process id. The rm stub waits half way, and the test checks then that the Neovim is gone or a zombie. The test fails on a runner that reads the process id, deletes, and then kills. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…e its grave The grave of the cleanup is made with mkdir, and its name is stored before that. The test sends SIGINT from the mkdir stub right after the third mkdir, the one that makes the grave of the cleanup of an error exit, and expects no folder left. A cleanup that stores the name after it made the folder fails the test. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…under a short parent Found while running the spec under a TMPDIR of about 87 characters. The test that names the work folder through the mktemp stub, the .gone test and the two boundary tests built that folder under the temp folder of the test. With a long TMPDIR the name passed 80 bytes, the runner moved the work folder to /tmp, and the first of them failed on the unchanged runner while the others tested nothing. The multibyte test had the same problem: its name stayed above 80 characters. short_temp() now returns the same folder for the whole test, and the multibyte test builds its folder in a new /tmp/ccr.XXXXXX. The boundary tests no longer skip under a long TMPDIR. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
The README said that the grave comes from mktemp -d. The grave is made with mkdir under a name from mktemp -u, and the runner stores the name first, so a second cleanup removes the grave of the first. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
|
@codex review |
|
@codex security review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Why
#592 makes
scripts/run_integration_tests_individually.shstop on Ctrl-C, SIGTERM and SIGHUP and leave nothing behind. A verifier and two Codex threads found six gaps on its head cde2e40. None of them changes the status that the runner returns for a signal. This PR closes them and adds the tests that were missing. It follows #592, which merged as 0e10d2a. The record comment of #592 is #592 (comment).What changed
Fixes
TMPDIRwithconfig/andtmp/in it. The cleanup now stores the name of its grave in a global before it makes the folder (mktemp -u, thenmkdir -m 700). A second cleanup finds that name and removes the grave. The EXIT path still ignores no signal, so under bash 5 Ctrl-C ends with 130 and stops a calling loop (for bash 3.2 see "Not proven").${#var}in a UTF-8 locale, but libuv cuts a socket path at 107 bytes. ATMPDIRof 64 characters and 99 bytes kept its work folder, and v0.8.0 then left 38 socket files inTMPDIR. The runner now counts bytes withLC_ALL=C.scripts/README.mdsays bytes.LC_ALL=C.Tests
mvand at itsrm. The INT tests run in a calling loop and expect it to stop. A seventh test sends SIGINT right after themkdirthat makes the grave.mktempstub and expect the first to stay and the second to move to/tmp(finding M1).rmstub wait half way and checks then that the file's Neovim is already dead, so the cleanup kills before it deletes (finding M3).TMPDIRshort throughshort_temp()(finding M2). Before, a test process with aTMPDIRof about 42 characters or more sent every work folder to/tmp, where the leftover checks did not look. Writing the helper showed that four more tests had the same flaw: the test of a name that exists, the.gonetest and the two boundary tests built a folder under the test folder. The first of them failed on the unchanged runner under a longTMPDIR. The multibyte test needed a short parent too.Evidence
All runs use GNU timeout first on
PATHand a stubclaude. Every run below passed on its first try. No test passed only on a retry.mise run clean && mise run allexits 0: 1970 successes, 0 failures, 0 errors, 38 pending, luacheck 0 warnings and 0 errors in 206 files, and 0 files changed by format.TMPDIRof 85 characters and with a multibyteTMPDIR, and bash 3.2.57 (GNU build, asbashonPATHand as interpreter) with the same two folders: all four runs pass 116 of 116.TMPDIR(3 baseline runs plus 3 plain runs), and with bash 3.2.57 under all three. In a German libc sandbox (a compiled de_DE.UTF-8 locale and a hand-madelibc.mo, run withbwrap, no root) it gave 116 ok, 0 failures, 6 pending. The 6 are the wide-digit tests, which need other digits in[0-9]. The live runs and the mutants below come from that build.TMPDIRof 64 characters and 99 bytes. On the build of these commits on cde2e40, cde2e40 left 38 socket files and the build left none, with 852 successes each (0.12.3: nothing left, 856). On this head an independent verifier measured maind21f310aat 40 socket files and this head at 0, with 865 successes on v0.8.0 and 869 on 0.12.3.LC_ALL=Cprefix and pass 2 of 2 with it.mv, INT atrm, and the test right after the grave. TERM and HUP atrmpass on both runners, because the interruptedrmhad finished by then.Mutants of the final runner, each run against the whole spec:
TMPDIR)mkdirmv, and three tests wherermis half wayReverting the M2 change shows what it fixes. Under a
TMPDIRof 85 characters, the spec before this change failed the mutant "rm only the grave" only in the test of a name that exists, which fails on the unchanged runner too. The live-write test and the failed-move test passed, because the folders were in/tmp. With this change the same mutant fails those two tests under a long and under a shortTMPDIR.Not proven
mktemp -uandmkdir. In that state the grave name is stored and the folder does not exist, so the move fails and the cleanup works on the old path, as in the failed-move test. An independent verifier sent INT, TERM and HUP there under bash 5 and bash 3.2: nothing was left and the status was right.mkdiropens a name race thatmktemp -ddid not have: if another process makes a folder at that exact name and a signal comes before the runner drops the name, the cleanup removes that folder. In the same forced sequence, a nested cleanup could also read a process id from that folder and kill that process group. No process can know the name in advance, the window is microseconds, and main has the same race for its work folder. The README line ("no name that exists is touched") and the runner comment overstate this.$(mktemp ...)after an error exit makes the runner exit with status 130 instead of dying from the signal, so a calling loop goes on. Main does the same.ps./bin/bash./tmponly when/tmpis a writable folder. Without one it stays inTMPDIR, as in fix(scripts): make the integration runner die by SIGINT and leave nothing after a signal #592.Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh