test(switch_picker): tolerate Windows self-exit-1 on successful picker-create - #3427
Merged
Merged
Conversation
…r-create test_switch_picker_create_validates_templates_before_worktree flaked on the advisory affected tests (windows) leg while the required test (windows) leg passed 0 on the same SHA (PR #3424 CI). The panic was the re-run's exact exit_code == 0 assertion: the picker created the branch + worktree correctly and ran the pre-start hook, but wt self-exited 1 (the test finished in ~5.6s, well under CHILD_EXIT_TIMEOUT, so it was a genuine self-exit, not a harness kill). This is the same slow-but-successful exit reports 1 on Windows class the abort helpers already tolerate via assert_valid_abort_exit_code. Adds a sibling assert_valid_create_exit_code that requires 0 everywhere it is reliable and tolerates 0 or 1 on Windows, and applies it to both successful picker-create tests (the other, test_switch_picker_create_ worktree_with_alt_c, carried the identical latent exact-0 assertion). The deterministic git branch + worktree assertions remain the real success guard: a create that genuinely fails leaves no branch and still fails.
Owner
file a bug report: this should not be here. there is no need to sign off a Tend agent message |
Collaborator
Author
|
Filed as max-sixty/tend#772. Worth noting it isn't template-driven — I searched the tend harness and skills and the string appears in no template, and it shows up on only 1 of the last 10 bot PRs, so it's an occasional self-authored line rather than a systematic footer. The issue proposes an explicit "don't sign off" instruction (alongside the existing "don't add footers" rule) so it stops recurring. |
max-sixty
added a commit
that referenced
this pull request
Jul 26, 2026
…ir own, and make a PTY failure explain itself (#3619) Chasing the Windows-only failure of `test_switch_picker_runs_execute_command` that @max-sixty flagged after #3607 merged: it failed the advisory `affected tests (windows)` leg while the required `test (windows)` leg **passed the same test on the same SHA** in the same run. ## The failure message was the whole problem ``` assertion `left == right` failed: Expected exit code 0 for picker switch with --execute left: 1 right: 0 ``` That is everything CI had. wt prints an error before exiting non-zero, so there was a message — it just never reached the captured frame, because of two harness gaps: - **The post-exit drain was a single non-blocking sweep**, taken the instant `try_wait` reported the child reaped. The child's last writes can still be in the PTY at that moment, so the tail — wt's `✗` line — was dropped. This is also why #3427 concluded a picker-create "self-exited 1 after fully-correct work with no error on screen": the error was there, just not drained. - **`child.kill()` ran unconditionally after the exit poll.** On Windows that is `TerminateProcess(proc, 1)`, so a hung picker and a picker that failed on its own both report exit 1 — the ambiguity that made #3427 reason from wall-clock time to rule out a kill. Both are fixed here: drain until the output goes quiet, and panic on a hang rather than killing silently, so every exit code a caller asserts on is one the child chose. The four directive tests now also dump the screen (and the EXEC file) in their exit-code assertions. ## What reproducing it turned up Loop-based reproduction of the reported symptom is hopeless by the numbers: across the last 38 PR runs, exactly one advisory job failed — this one — i.e. ~1 failure per 160,000 test executions. What *did* reproduce was a different Windows failure in the same five tests, at a rate 1000× higher, once the loop ran with realistic neighbours (the full suite running alongside, as the failing leg had at test 3111/4204): ``` thread '…switch_picker::test_switch_picker_emits_cd_directive_by_default' panicked at tests\integration_tests\switch_picker.rs:2531:45: failed to create cd temp file: … PathError { path: "D:\\tmp\\.tmp3XI9wb", err: Os { code: 5, kind: PermissionDenied, message: "Access is denied." } } ``` **4 of 80 runs of the five directive tests.** `directive_files()` called `NamedTempFile::new()` in the shared temp directory; `tempfile` retries a name collision only when it surfaces as `AlreadyExists`, and on Windows `create_new` against a name held by a *directory* — or by a file in delete-pending state — returns `PermissionDenied`, which it hands back to the caller. A full suite run fills that directory with `.tmpXXXXXX` entries (every `TestRepo` makes one), so the collision is routine rather than astronomical. The helpers now take a `TempDir` and put fixed-named files inside it. `TempDir::new` isn't exposed to the same problem — a directory collision surfaces as `AlreadyExists`, which tempfile retries — and fixed names inside a private directory can't collide with anything. That covers all 75 call sites of `directive_files()` / `legacy_directive_file()`, not just the picker's five, and the switch_picker copy of the helper is gone. ## The reported exit-1 is not proven fixed Stated plainly: the panic above is a *different symptom* from the reported exit 1, so this PR does not prove that failure is gone. What the evidence does say, and what a reader should check first if it recurs: - The failing run took 3.34s where the same test passing on the required leg took 3.11s and the same switch *without* `--execute` takes 2.42s. The `--execute` tail — `build_hook_context`'s git lookups, ~0.7s of the difference — had therefore run, so the failure was at the end of the pipeline, not in the picker or the switch. - After that tail, the only fallible step left is the EXEC directive append. Every other step is infallible or `.ok()`-swallowed. - That write's error was context-free, which is why nobody could place it. `wt` rendered `✗ No such file or directory (os error 2)` — no file, no operation, and positioned right under `◎ Executing (--execute):`, where it reads as if the *command* were missing. It now reads: ``` ✗ Failed to write the command to the directive file /tmp/tmp.aBc No such file or directory (os error 2) ``` The message lives in `append_line` / `write_cd_path` rather than at their four call sites, so every directive write says which file and what didn't get written — including the legacy-protocol arms, which no test can drive to failure. So the next occurrence prints its own diagnosis, in one look, instead of `left: 1, right: 0`. ## Also here **The four directive tests select their row by cursor, not by a typed query.** They typed `target` and waited for `target-branch`'s preview pane before Enter. That gate can only assert what skim *painted*: the filtered item list is swapped into `ItemList` during a render, while `Accept` reads `item_list.selected()` — the cursor's slot in whatever list is current at that moment. The list-pane `>` pointer comes from the same render state the accept reads, which is why this file's own `wait_for_cursor_on_row` calls it the race-free signal. One Down reaches `target-branch` (the picker sorts the current worktree first) and the arrow is re-issued until the pointer settles, so no query — and no matcher — is in the accept path at all. It also drops the wait on the preview pane rendering. ## Testing - Reproducer and confirmation both on Windows CI, via a temporary workflow (deleted in the last commit): 80 runs of the five directive tests with the full suite running alongside. **4 failures before the fix, 0 after.** The concurrent full suite passed 4208/4208 in both. - Local: `switch_picker` (68), plus every suite that calls these helpers — `directives`, `switch`, `remove`, `security`, `bare_repository`, `step_alias`, `user_hooks`, `step_relocate` (861 tests) — green, plus clippy `-Dwarnings` on all targets with `--features shell-integration-tests`. - The two directive-write messages are pinned by tests of their own (`test_exec_directive_write_failure_names_the_file`, `test_cd_directive_write_failure_names_the_file`): an unwritable directive path fails the same way on every platform, so the paths that produce these messages are covered rather than checked by hand. ## Follow-ups, not in here - `build_hook_context` is called with `referenced: None` on the `--execute` path, so a `-x 'echo hi'` with no template variables still runs the `default_branch`, `primary_worktree`, `commit`, `short_commit`, `remote`, and `upstream` lookups — roughly doubling the command's subprocess count (30ms → 60ms locally, 2.42s → 3.11s in the Windows test timings). The alias path already passes its referenced set; a `--execute` command receives no JSON context, so the reason hooks pass `None` doesn't apply to it. - `cancel_background_commands()` is a no-op on Windows (`signal_background_pid`), so the picker's "the picker's exit cancels the unfinished request" promise isn't kept there: preview git children keep reading `.git/config` while the accept path writes `worktrunk.history` and `worktrunk.default-branch` to it. Issue #2564 is the same shape of concurrent-config-access failure on Windows. - `assert_valid_create_exit_code`'s Windows tolerance (#3427) and `assert_valid_abort_exit_code`'s acceptance of `1` both rest on the kill-vs-self-exit ambiguity this PR removes; they can be tightened once a Windows run confirms nothing else produces a stray 1. > _This was written by Claude Code on behalf of Maximilian Roos_
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the Windows flake @max-sixty flagged on #3424:
test_switch_picker_create_validates_templates_before_worktreefailed on the advisoryaffected tests (windows)leg while the requiredtest (windows)leg passed on the same SHA.Diagnosis
The panic was the re-run's exact
exit_code == 0assertion, not a content assertion — and the captured frame shows a fully-correct create:The branch and worktree were created, the
pre-starthook ran, and the "cannot cd" line is a warning (not an error).run_pickerreturnsOk(())after theSwitchPipelineon a successful create, so the product exits 0 — which is what the requiredtest (windows)leg observed on the same commit.Crucially, the failing test finished in ~5.6s (
FAIL [ 5.571s]), well under the harness's 30sCHILD_EXIT_TIMEOUT. So this was not a slow-exit-then-kill (a kill would push the test past 30s); wt self-exited 1 after a correct create. This is the same "slow-but-successful exit reports 1 on Windows" class the abort helpers already document and tolerate viaassert_valid_abort_exit_code(accepts 0/1/130).Fix
Adds a sibling
assert_valid_create_exit_codethat requires0everywhere it is reliable and tolerates0or1on Windows, and applies it to both successful picker-create tests.test_switch_picker_create_worktree_with_alt_ccarried the identical latent exact-0assertion, so it's the same root-cause class — fixing only one would leave a follow-up flake.The exit code is the incidental check here. Both tests already prove success the deterministic way — the new branch and worktree exist in git afterward — and those assertions are untouched. A create that genuinely fails leaves no branch, so the git-state assertions remain the real guard; the tolerance only stops a correct create from false-failing the advisory leg on Windows.
What I couldn't verify
The underlying reason wt self-exits 1 on a correct picker-create on Windows is unexplained — I traced the create/exit path (
run_picker→SwitchPipeline::run→handle_switch_created_output) and found no error path that fires here, and I can't reproduce or observe the Windows-only PTY behavior from Linux CI. Like #3331, this targets Windows-specific timing and can only be confirmed gone on the Windows legs. If you suspect the exit-1 reflects a real product issue rather than a PTY/teardown artifact, the tolerance can be tightened once the mechanism is understood — the git-state assertions guard correctness in the meantime.