h2origin autest: honor the 30s squid.log wait budget - #13450
Open
mandarwagh9 wants to merge 1 commit into
Open
Conversation
The hand-rolled watcher relied on StartBefore's default 10 second readiness gate, so the intended 30 second budget was never reached and the test failed intermittently on loaded CI workers. Use the existing AddAwaitFileContainsTestRun helper, which allows 30 seconds, and flush log buffers every second so the entry appears sooner. Fixes: apache#13445 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tests/gold_tests/h2/h2origin.test.py:52
- All neighboring
ts.Disk.records_config.update()values are strings; setting this one as anintis inconsistent and can be problematic if the config writer expects string values. Use'1'(string) for consistency with the rest of the records config values in this file.
# Flush log buffers promptly so that the squid.log entry for the last
# transaction appears while the test is still waiting for it.
'proxy.config.log.max_secs_per_buffer': 1,
Contributor
|
[approve ci] |
Contributor
|
Actually just ran into this on the 10.2.x branch. The h2origin autest is flaky waiting for the log update. Thanks so much for the fix! |
Contributor
|
Can you please rebase, @mandarwagh9 ? I've also add you to the CI allow list, so CI should run for your PR on a future push. |
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.
Fixes #13445.
Problem
tests/gold_tests/h2/h2origin.test.pyhand-rolled its "wait forsquid.log" step:
The intent is a 30-second budget, but neither
watcher.TimeOutnortr.TimeOutgoverns the readiness gate. That gate isStartupTimeout,which
Processes.Process()defaults to 10 seconds:autest/testenities/processes.py:startup_timeout=10, # default to 10 second as most things should be ready by this timeautest/runlogic/runlogic.py:if ready_item.object.StartupTimeout < curr_temp: ... "Checking that {typename} is ready within {ready_item.object.StartupTimeout} seconds ..."So the run failed at 10.0s while the watcher was still waiting at 9.5s,
exactly as reported in the issue. On a loaded CI worker the squid.log
entry for the last transaction had not been flushed yet.
Worth noting: simply raising
StartupTimeoutto 30 would still race,because the watcher's own
sleep 30would exit at about the samemoment, tripping autest's "Process finished before it was ready" path.
Fix
Two complementary changes:
Use the existing
AddAwaitFileContainsTestRunhelper(
tests/gold_tests/autest-site/when.test.ext) instead of thehand-rolled watcher. It sets
StartupTimeout = 30against asleep 60, so the 30-second budget is actually honored with nosleep/gate race. This helper is already used at 20+ call sites;
logging/log-milestone-fields.test.py::_waitForLogis the sameshape,
StillRunningAfterlines included. h2origin was simply aleftover from before the helper existed.
Set
proxy.config.log.max_secs_per_buffer: 1so the log entryappears promptly rather than waiting out the default 5-second buffer
flush. This addresses the underlying latency the issue describes
rather than only widening the window. 13 other gold tests that assert
on log contents already do this.
The needle, test-run name, and
StillRunningAfterassertions areunchanged, so coverage is identical.
Testing
Verified locally:
yapf0.43.0 with.style.yapfreports noformatting changes, and the file compiles. I develop on Windows and
cannot build or run ATS there, so I have not executed the autest
myself — relying on Jenkins for that. The change is confined to test
configuration and contains no production code.
🤖 Generated with Claude Code