test: make TEST_TAP_TIMEOUT actually fire, and enable it by default - #6011
Conversation
TEST_TAP_TIMEOUT could not catch the failure it exists for. The read loop
was:
line = fop.stdout.readline() # blocks
...
if tap_timeout > 0 and (time.time() - start_time) > tap_timeout:
readline() blocks until a full line arrives, so a test that hangs while
producing no output never reached the deadline check at all. Verified
directly: with tap_timeout=3 against 'sleep 300', the old loop was still
blocked after 25 seconds; the new one raises at 3.0s.
That is the exact profile of the CI-mysql84-g9 stall on #5991, where
test_ssl_fast_forward-3_libmariadb-t ran for hours.
Replaces the blocking readline() with select() on a bounded wait plus raw
os.read() chunking. select() guarantees the deadline check runs even when
the child is silent; chunking rather than line-reading means a test that
stops mid-line cannot wedge the loop either. Output order and content are
unchanged, a trailing partial line is now flushed instead of dropped, and
decoding uses errors='replace' so a stray non-UTF-8 byte no longer throws.
Verified against four cases: normal chatty test (all lines, in order),
silent hang (timeout fires), partial-line-then-hang (timeout fires), and
clean exit.
Also flips the default from 0 (disabled) to 1800s. 1800 is ~2.4x the
slowest single test measured across 47 groups:
reg_test_3765_ssl_pollout-t 12.5 min
test_cluster_sync-t 10.5 min
set_testing-240-t 7.8 min
test_auth_methods-t 7.7 min
Only 4 of 401 tests exceed 5 minutes, so this cannot fire on a merely slow
test, while still stopping a hang well inside the 90-minute step budget
added in the companion CI PRs -- and, unlike a step or job timeout, it
identifies WHICH test hung and lets the run continue to its archive steps.
|
Warning Review limit reached
Next review available in: 14 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe test harness now uses a 1,800-second default TAP timeout. The tester reads subprocess output with readiness checks and chunk buffering, so timeouts remain active during silent execution and partial output. ChangesTest timeout monitoring
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ProxySQLTester
participant select
participant TAPProcess
ProxySQLTester->>select: wait for TAP output with timeout
select->>TAPProcess: check output readiness
TAPProcess-->>ProxySQLTester: return output chunk or EOF
ProxySQLTester->>ProxySQLTester: buffer and log complete lines
ProxySQLTester->>ProxySQLTester: enforce timeout when no output arrives
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/scripts/bin/proxysql-tester.py (1)
869-889: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve the timeout after EOF and reap killed processes.
At lines 888-889, EOF closes stdout, but the test process may still hang. Use
fop.wait(timeout=remaining_timeout)instead of unconditionalfop.wait()soTEST_TAP_TIMEOUTstill applies. Afterfop.kill(), callfop.wait()before loggingfop.returncode;returncoderemainsNoneuntil reap. Also keep thedecode('utf-8', 'replace')fallback in the timeout path and avoid blockingfor line in fop.stdout; drain only immediately available output instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/scripts/bin/proxysql-tester.py` around lines 869 - 889, Update the TAP process handling around fop.wait() and the subprocess.TimeoutExpired handler: preserve the remaining TEST_TAP_TIMEOUT after stdout EOF by using a bounded wait, reap the process after fop.kill() before inspecting or logging its return code, and replace the blocking fop.stdout iteration with a non-blocking drain of immediately available output while retaining decode('utf-8', 'replace').
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/scripts/bin/proxysql-tester.py`:
- Around line 869-889: Update the TAP process handling around fop.wait() and the
subprocess.TimeoutExpired handler: preserve the remaining TEST_TAP_TIMEOUT after
stdout EOF by using a bounded wait, reap the process after fop.kill() before
inspecting or logging its return code, and replace the blocking fop.stdout
iteration with a non-blocking drain of immediately available output while
retaining decode('utf-8', 'replace').
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5645c09a-d7a0-42c8-8abd-df1e27ef1ba5
📒 Files selected for processing (2)
test/infra/control/env-isolated.bashtest/scripts/bin/proxysql-tester.py
📜 Review details
🔇 Additional comments (2)
test/infra/control/env-isolated.bash (1)
107-114: LGTM!test/scripts/bin/proxysql-tester.py (1)
8-8: LGTM!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81c52909cb
ℹ️ 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".
| wait = 1.0 | ||
| if tap_timeout > 0: | ||
| wait = max(0.0, min(1.0, tap_timeout - (time.time() - start_time))) | ||
| ready, _, _ = select.select([fop.stdout], [], [], wait) |
There was a problem hiding this comment.
When this new non-blocking wait reaches the deadline for the intended silent-hang case, the TimeoutExpired handler only calls kill() and drains stdout; it never wait()s or poll()s the child. In Python, Popen.kill() does not populate returncode, so the later rc += abs(int(fop.returncode)) raises TypeError with returncode is None after the first timeout, causing the workdir to be reported as a Python exception and aborting the remaining TAP tests instead of recording a timed-out failure.
Useful? React with 👍 / 👎.
Review catch. The TimeoutExpired handler called kill() and drained stdout
but never wait()ed, so fop.returncode stayed None -- and the shared exit
path below evaluates:
rc += abs(int(fop.returncode))
which raises TypeError on None. The first timed-out test would therefore
abort the whole group as a Python exception instead of failing one test.
Latent until now: the timeout could never fire while the read loop blocked
in readline(), so this path was unreachable. Enabling the timeout in the
previous commit makes it reachable, so it has to be fixed in the same PR.
wait() after the drain yields -SIGKILL, so abs(int(...)) contributes 9 and
the timed-out test is correctly scored as failed. Verified directly:
returncode None before wait(), -9 after.
Also switches the drain's decode to errors='replace', matching the main
read loop -- a hung test is exactly the case likely to emit a truncated
multi-byte sequence, which would otherwise throw inside the handler.
|
1 participant
TEST_TAP_TIMEOUTcould not catch the failure it exists for.readline()blocks until a full line arrives, so a test that hangs while producing no output never reached the deadline check. Verified directly: withtap_timeout=3againstsleep 300, the old loop was still blocked after 25 s; the new one raises at 3.0 s. That is the profile of theCI-mysql84-g9stall on #5991.Fix:
select()on a bounded wait + rawos.read()chunking.select()guarantees the deadline check runs while the child is silent; chunking rather than line-reading means a test that stops mid-line can't wedge the loop either. Output order/content unchanged; a trailing partial line is now flushed instead of dropped; decoding useserrors='replace'so a stray non-UTF-8 byte no longer throws.Verified against 4 cases: normal chatty test (all lines in order), silent hang (fires), partial-line-then-hang (fires), clean exit.
Also flips the default from 0 (disabled) to 1800s. Measured across 47 groups / 401 tests:
reg_test_3765_ssl_pollout-ttest_cluster_sync-tset_testing-240-ttest_auth_methods-tOnly 4 of 401 tests exceed 5 minutes, so 1800s (~2.4× the max) can't fire on a merely slow test.
Complements #6005/#6006: a step timeout bounds cost, but only this identifies which test hung and lets the run continue to its archive/coverage steps.
Summary by CodeRabbit