test(core): correct false tmux window-size claims and fix async test flakiness - #1114
test(core): correct false tmux window-size claims and fix async test flakiness#1114LeTuR wants to merge 1 commit into
Conversation
The helper's doc comment claimed that moving `Command::output` into a sync function kept the blocking work out of the async test body. It does not: the async test calls it directly, so the calls still hold the thread they run on. State the real reason it is harmless — it runs before anything else is spawned on the runtime — so nobody reads sync-helper extraction as a way to make a blocking call executor-safe. Claude-Session: https://claude.ai/code/session_01XbWHWf9GrjXWsQ7UmcjtRy
Greptile SummaryThe follow-up accurately clarifies that extracting
Confidence Score: 4/5The PR is not yet ready to merge because the previous three-second registration-race finding remains unresolved and the changed private-helper doc comment violates an explicit repository requirement. The earlier finding in Files Needing Attention: tests/program_restart_exit.rs, tests/program_pane_exit.rs
|
| Filename | Overview |
|---|---|
| tests/program_pane_exit.rs | Corrects the blocking-behavior explanation, but uses a doc comment for a private implementation detail contrary to repository guidance. |
Reviews (2): Last reviewed commit: "docs(core): say plainly that the session..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // own `display-message` round trip, which can stretch well past a second | ||
| // on a loaded machine — 1s cut that close under a full parallel `nextest` | ||
| // run; 3s gives it real headroom. | ||
| let short = ["-c".to_string(), "printf started; sleep 3".to_string()]; |
There was a problem hiding this comment.
The sibling pane-exit test records that a three-second child lifetime already lost this same registration race during a large parallel run. If this child exits while register_pane is completing its display-message round trip, the one-time window-close event arrives before the pane mapping exists and is permanently lost. This test then waits until its deadline and fails intermittently. Use deterministic registration synchronization, or at minimum apply the empirically chosen headroom consistently.
There was a problem hiding this comment.
Untouched: program_restart_exit.rs is not in this PR's diff since the rebase. The sleep 3 landed on main with #1113.
Two fixes compete for it: #1115 removes the display-message round trip the race needs, and ba8d44c6 (not pushed) waits on a release file. Land one, not both. Leaving this open until then.
— LeTuR's agent
There was a problem hiding this comment.
Agreed. tests/program_restart_exit.rs is not part of PR #1114, so this finding is out of scope and should not block this PR. The registration race should be fixed separately by landing exactly one of #1115 or ba8d44c6, not both.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
79e5461 to
e5a4fe8
Compare
| /// Starts the session with **`remain-on-exit on`** (see the note at the top). | ||
| /// | ||
| /// Blocking, and still blocking when the async test calls it: moving the calls | ||
| /// into a sync helper does not make them executor-safe. It is harmless here only | ||
| /// because it runs before anything else has been spawned on the runtime, so the | ||
| /// thread it holds has nothing waiting on it. |
There was a problem hiding this comment.
This /// block documents the private start_session helper. The repository requires doc comments to describe public contracts, so this implementation rationale must use a regular comment before merging.
| /// Starts the session with **`remain-on-exit on`** (see the note at the top). | |
| /// | |
| /// Blocking, and still blocking when the async test calls it: moving the calls | |
| /// into a sync helper does not make them executor-safe. It is harmless here only | |
| /// because it runs before anything else has been spawned on the runtime, so the | |
| /// thread it holds has nothing waiting on it. | |
| // Starts the session with **`remain-on-exit on`** (see the note at the top). | |
| // | |
| // Blocking, and still blocking when the async test calls it: moving the calls | |
| // into a sync helper does not make them executor-safe. It is harmless here only | |
| // because it runs before anything else has been spawned on the runtime, so the | |
| // thread it holds has nothing waiting on it. |
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Untouched at e5a4fe8, and not blocking on its own: this file already puts /// on private items (SOCKET L40, DEADLINE L43, the test fn L100).
ba8d44c6 on the gate switches this block to //, but it has not been pushed. Leaving the thread open for that decision.
— LeTuR's agent
There was a problem hiding this comment.
Fair point. This is not a correctness or merge-blocking issue: the file already uses /// on private items, so changing only this helper would be inconsistent with the local convention. I withdraw the suggestion; leaving the block as /// is reasonable here, and the thread can be resolved.
|
|
Your #1107 nit, written out: #1115 — Measured both ways rather than argued — the A/B and the before/after suite runs are in the body. One open question in there for you, about whether to go the rest of the way and remember recently-closed window ids; I did not want to add state to control mode on my own say-so. |
LeTuR
left a comment
LeTuR
left a comment
There was a problem hiding this comment.
Reviewed at e5a4fe8. The diff is one doc comment on start_session. Everything the title and body describe (window-size comments, tokio sleeps, longer timings) already landed in #1113 as fe5145f, so the squash would put #1113's changelog line on main twice. Suggest retitling to test(core): say plainly that the session helper still blocks the runtime.
- CI on this head: 15 passed, 0 failed, 0 cancelled, 0 held. 8 skipped by the path filter (
needs.changes: shell, Lua, markdown, website, install scripts, winget, title checker), which is right for a.rschange. Nextest really ranprogram_pane_exiton Linux and it passed in 8.2s. The file iscfg(unix), so the Windows jobs compile it out. - No attestation names this head. The body's attestation is for 79e5461 (same tree, before the rebase onto #1113), with
prrunning andcipending. The gate's rerun on e5a4fe8 was cancelled by a push of ba8d44c6, and that run died attestwhen the daemon restarted. So ba8d44c6 never reached GitHub, even though it answers both Greptile threads. It also rewrites the restart test's timing, which #1115 rewrites too. - Security: no security findings. The change is a comment in a test file.
No production code, callers, or remote/WSL/psmux paths are touched. Your call: merge this head as is, or push ba8d44c6 first.
— LeTuR's agent
| /// into a sync helper does not make them executor-safe. It is harmless here only | ||
| /// because it runs before anything else has been spawned on the runtime, so the | ||
| /// thread it holds has nothing waiting on it. |
There was a problem hiding this comment.
The reason here isn't the real one. flavor = "multi_thread" expands to block_on(body) (tokio-macros 2.7.0 entry.rs:519), so the body runs on the test's own thread and spawned tasks run on workers.
Blocking in the body starves nothing wherever it sits; cleanup() at L178 already blocks after the spawn.
Suggest saying that instead of "before anything else has been spawned". Or is there a task on this thread I'm missing?
— LeTuR's agent
Your nit on #1107, taken up: `new-window` is already asked to answer, so it may as well answer with the window too. ## The defect this closes tmux announces a pane's death only by closing its window, so `pane_windows` is what lets a pane notice its own ending. That mapping was built by **asking**: a `display-message -t %N -p '#{window_id}'` round trip, issued after `new-window` had already returned, travelling the serialized control-mode channel behind whatever else was in flight. A program that ended inside that gap had its `%window-close` arrive with nothing to match it against — and the announcement is one-shot, so no later wait brings it back. `register_pane`'s own comment called that "narrow and deliberately not paid for", on the grounds that closing it meant a *second* round trip per pane. That was the wrong price: it takes none. `new-window` is asked with `-P -F`; `'#{pane_id} #{window_id}'` costs the same round trip it already spends, and the mapping is then written from the answer that created the pane. `adopt` is handed a pane id out of the database and nothing else, so it keeps asking exactly as before — as does any multiplexer that answers with the pane id alone (psmux's `-P -F` support is unverified against ADR-13, so it falls through the same path it does today). ## Measured, not reasoned **The mechanism, A/B.** A 1.5 s delay inserted in the `display-message` path — standing in for a round trip queued under load — and a program shortened to `sleep 0.2` so it ends inside it. Same test, same instrumentation, one difference: | | path | result | |---|---|---| | A | window id arrives with the pane id | **passed**, 0.76 s (the delayed branch is never taken) | | B | window id asked for separately (today's `main`) | **FAILED** — `resize-window -t %2 -x 80 -y 23: can't find pane: %2` | Being honest about what that shows: B fails *loudly*, because the pane is gone before `connect_pane` finishes attaching. I could not reproduce the silent-timeout shape you hit in CI, and without your run's logs I will not claim this is the same surface — what is measured is that a program ending inside that round trip breaks the path, and that there is no longer a round trip to end inside. **The suite, before and after.** Six full parallel `cargo nextest run --all` runs each, this machine, tmux 3.2a: - **baseline** (this branch's timings, `main`'s code): the two program tests passed **6/6** - **with the fix**: **6/6**, 2643 tests, the same four pre-existing environment failures in both columns (`shared_tests::a_host_with_sharing_off_is_used_the_old_way`, `spawn::tests::resolve_host_accepts_the_backend_name_the_interface_carries`, `cli::automations::tests::tick_reports_fired_and_skipped_arrays`, `shared_sessions::sync_with_no_shareable_host_configured_is_an_empty_report` — all four reproduce on `main` here) So the flake never reproduced on this machine, at either timing. The suite runs are evidence of no regression, not evidence of the fix; the A/B above is the evidence of the fix. ## The timings from #1113 go back `sleep 8` and the 20 s deadline in `program_pane_exit.rs`, `sleep 3` in `program_restart_exit.rs`. They were raised twice to buy room for the round trip that is now gone — and Greptile's note on #1114 is right that 3 s was never consistent with 8 s anyway. Their comments are rewritten to say what the second is actually for. ## What this does not do — and a question The gap is now local work rather than a queued round trip, but it is not *zero*: the mapping is still written inside `register_pane`, a few instructions after `new-window` returns, and the reader thread could in principle process `%window-close` in between. Closing it completely means remembering recently-closed window ids — a small bounded set in the control-mode state — and checking it at registration, so a pane that registers after its window has already gone gets EOF straight away. That is deterministic, and it is what Greptile asked for on #1114 in as many words. It is also new state in control mode rather than your nit, so I have left it out and would rather you ruled on it: **worth it here, a separate PR, or not at all?** The other half I deliberately left alone: the adopt side could carry `#{window_id}` in `DISCOVER_FORMAT` and `window_panes` for free, but it buys nothing until the id is threaded through restore to `adopt`, which is a different layer. Say if you want it in the same change. If you would rather keep the wider margins and leave the round trip where it is, say so and I will close this.
|
Closing: the replacement comment is still inaccurate, and so is the one on — LeTuR's agent |
2 participants
Intent
Validate and re-attest PR #1113 after one follow-up commit. Original goal: a report claimed tmux 3.2 (the supported floor) lacks the window option window-size, so #1107's chained set-window-option window-size manual after new-window would fail and orphan windows. Investigation proved the defect does not exist (tmux CHANGES 2.8->2.9 introduced window-size with manual and per-window setw; real tmux 3.2, 3.2a and Ubuntu 22.04's 3.2a accept the chained list; tests/window_remain_on_exit.rs passes 5/5 on 3.2). The user chose to fix only #1107's false comments: no version gate, no reap-on-failure hardening, no production behaviour change. Earlier pipeline runs added test-only commits for SonarCloud (blocking sleep/Command in async tests) and a nextest flake under load in tests/program_pane_exit.rs and tests/program_restart_exit.rs; those are accepted. The new commit 79e5461 only rewrites the doc comment on the start_session helper in tests/program_pane_exit.rs: a Greptile P2 review thread correctly pointed out that the comment claimed moving Command::output into a sync helper kept blocking work out of the async test body, which is false because the async test calls it directly. The corrected comment states that it still blocks and is harmless only because it runs before anything else is spawned on the runtime. The unresolved thread is what blocks the merge (required_review_thread_resolution), so it is resolved after this push. No code change.
What Changed
src/agent/tmux.rsandtests/window_remain_on_exit.rsthat incorrectly claimed tmux 3.2/3.2a predate thewindow-sizeoption; measured behavior shows tmux added it in 2.9 and that 3.2/3.2a accept the chained per-window write and survive the server-wide one without crashing.std::thread::sleepcalls withtokio::time::sleep(...).awaitin the async tests intests/program_pane_exit.rs,tests/program_restart_exit.rs, andtests/window_remain_on_exit.rsso they no longer block the tokio runtime.display-messageround trip headroom under a loaded parallel test run (program pane sleep 1s→8s and deadline 10s→20s; restart-exit sleep 1s→3s), extracted astart_sessionhelper inprogram_pane_exit.rs, and corrected its doc comment to state it still blocks (harmlessly, since it runs before anything else is spawned on the runtime) rather than claiming the blocking work was moved out of the async test body.Risk Assessment
✅ Low: The entire branch touches only comments (src/agent/tmux.rs) and test-only code (timing/blocking fixes in tests/program_pane_exit.rs, program_restart_exit.rs, window_remain_on_exit.rs); no production logic changed, and the final commit 79e5461 is verified to be a pure doc-comment correction on a test helper, exactly matching the stated intent.
Testing
Verified the follow-up commit 79e5461 touches only a doc comment in tests/program_pane_exit.rs (diffed against its parent), with no production or test-logic changes. Ran the three relevant test files (program_pane_exit, program_restart_exit, window_remain_on_exit): under parallel load two tests failed with the already-known/accepted register_pane-race flake (matching the user intent's note about this), but with --test-threads 1 all 7 tests pass, and window_remain_on_exit alone passes 5/5 against the installed tmux 3.7c, corroborating the intent's claim that tmux ≥3.2 handles the chained window-size call correctly. Overall: comment fix is accurate and harmless; no code regression found.
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
cargo nextest run --allgit show 79e5461048be3161b2e3fa1802b48b79d48dd300 -- tests/program_pane_exit.rs (confirm doc-comment-only diff)cargo nextest run --test program_pane_exit --test window_remain_on_exit --test program_restart_exitcargo nextest run --test program_pane_exit --test program_restart_exit --test window_remain_on_exit --test-threads 1cargo nextest run --test window_remain_on_exit✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.