Bound the control-mode wait off the descriptor - #733
Open
tony wants to merge 2 commits into
Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #733 +/- ##
=======================================
Coverage 52.45% 52.45%
=======================================
Files 26 26
Lines 3729 3729
Branches 747 747
=======================================
Hits 1956 1956
Misses 1469 1469
Partials 304 304 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
tony
force-pushed
the
fix/731-control-mode-select
branch
from
August 1, 2026 01:04
9f3e1c4 to
23592e0
Compare
Member
Author
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
why: The test asked select whether the descriptor had a line and then read with readline, which serves from the buffer above it. ControlMode runs its subprocess with text=True, and tmux writes a whole %begin/%end block in one burst, so the first readline routinely drains every remaining line off the descriptor -- select then reports nothing ready while the answer is already in hand. Passing at all depended on unrelated %output from the pane's shell re-arming select. Closes #731. what: - Read lines on a thread and poll a queue with a monotonic deadline, so the wait is bounded without consulting the descriptor - Explain in the helper why the descriptor cannot answer the question - Leave ControlMode alone: text=True and encoding="utf-8" are the behaviour this test guards
tony
force-pushed
the
fix/731-control-mode-select
branch
from
August 1, 2026 10:40
23592e0 to
25226cc
Compare
why: A test that fails on loaded runners and passes on re-run costs every contributor a re-run, so the entry belongs in the release notes even though no library code changed. what: - Record the bounded control-mode wait under `Development`, where a test-suite-only change belongs.
tony
force-pushed
the
fix/731-control-mode-select
branch
from
August 1, 2026 11:41
25226cc to
8c54de1
Compare
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
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.
Closes #731.
Summary
test_control_mode_stdout_preserves_non_ascii_outputtiming out while the line it wants sits in the buffer. It bounded its wait withselecton the descriptor and read withreadline, which serves from theTextIOWrapperabove it.selectwith a reader thread and a queue polled against a monotonic deadline, so the wait is bounded without asking the descriptor a question it cannot answer.ControlModeuntouched.text=True, encoding="utf-8"is exactly the behaviour this test exists to guard.Why the descriptor cannot answer
tmux writes a whole
%begin/%endblock in one burst, so the reply arrives together. The firstreadlinedrains up to 8192 bytes off the descriptor into userspace and returns one line; the rest sits in the decoder, and the descriptor is now empty.selectreports not-ready while the answer is microseconds away:What made the old test pass was unrelated
%outputfrom the pane's shell painting its prompt, arriving ~460 ms later and re-armingselect. The test passed for a reason unconnected to the data it asserts on, which is why it failed on loaded CI runners: once the read loop starts after that burst, failure is certain rather than unlikely.Before / After
Injecting a 300 ms stall before the read loop, everything else verbatim:
The coverage is preserved
The risk in rewriting this test is silently retiring what it guards. Verified by removing
encoding="utf-8"fromControlModeand re-running:Still caught. Restored, it passes.
Design notes
A thread rather than
selectonstdout.buffer.raw. Both work. Selecting on the raw descriptor means the test owns partial-line assembly and decoding — real logic, in a test whose subject is decoding. The thread keepsreadlinedoing the line splitting and adds only a bound.Monotonic deadline, not a per-iteration timeout. A fixed timeout per line lets a slow-but-progressing stream run for
limit × timeout. The deadline bounds the whole read.Fails, never hangs. Stream exhaustion is reported distinctly from expiry, so a closed pipe does not present as a timeout.
Test plan
tests/test_control_mode.pygreen, 3 consecutive runs at--reruns 0encoding="utf-8"removed fromControlModeuv run ruff check ./uv run ruff format ./uv run mypycleanuv run py.test --reruns 0cleanjust build-docscleanNote for the merger
CHANGESwill conflict with the sibling PRs for #723, #724, #725 and #726, which all add to the same block — keep both sides.