test(hotreload): drop caddytest's 5s Client.Timeout#2431
Merged
Conversation
caddytest's NewTester sets http.Client.Timeout to 5s by default. On slow CI runners (notably emulated linux/arm/v7) the SSE roundtrip "watcher fires -> mercure publishes -> client receives" can exceed that budget and the test dies with "context deadline exceeded (Client.Timeout or context cancellation while reading body)". The test already terminates the streaming read via context.WithCancel once "index.php" appears in the buffer, and the Go test harness provides the outer deadline. Setting Client.Timeout to 0 removes the redundant inner deadline. Flaky run that prompted this: #2430 arm/v7 job 76318473726
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses flaky CI failures in TestHotReload caused by caddytest.NewTester configuring an http.Client.Timeout of 5s, which can be too short for the SSE-based hot-reload roundtrip on slower/emulated runners.
Changes:
- Disable
caddytest’s defaulthttp.Client.TimeoutwithinTestHotReloadby settingtester.Client.Timeout = 0. - Add an explanatory comment documenting why the timeout is disabled for this test.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses Copilot's review on #2431: setting Client.Timeout to 0 leaves the test relying on Go's outer harness timeout if the SSE marker never arrives, making real regressions slow to surface. 30s is generous enough for emulated armv7 (the actual roundtrip there is ~5-6s) while still bounding the failure mode.
Member
Author
|
Switched to a bounded 30s timeout per #2431 (comment) — keeps the test fast-failing on real regression while leaving enough headroom for emulated armv7 (~5-6s actual roundtrip). |
dunglas
added a commit
that referenced
this pull request
May 16, 2026
caddytest's default 5s http.Client.Timeout covers the whole roundtrip, including body reads. On emulated armv7 the SSE chain (WriteFile -> e-dant/watcher -> mercure -> client) doesn't fit in 5s and the test dies with "Client.Timeout or context cancellation while reading body". Bump to 30s — generous enough for the slow runners (~5-6s actual) while keeping a bounded failure mode if the marker never arrives. Same fix is in #2431; carrying it here so this PR's CI does not trip on the unrelated flake.
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.
Root cause
TestHotReloadwas failing on https://github.com/php/frankenphp/actions/runs/25961840516/job/76318473726 (linux/arm/v7, dispatched from #2430) with:caddytest.NewTesterinitialisestester.Client.Timeout = 5s(Default.TestRequestTimeout). That budget covers the entire roundtrip including body reads. On an emulated armv7 runner the chainos.WriteFile→ e-dant/watcher event →mercurepublish → SSE chunk delivery doesn't complete in 5 s, so the streamingresp.Body.Readis killed before the markerindex.phpis seen. The job died at exactly 5.03 s.Fix
Set
tester.Client.Timeout = 0for this test. The test already cancels the read viacontext.WithCancelonce the marker appears in the buffer, and Go's-timeoutprovides the outer deadline — the 5 s inner cap was redundant.The other failing job on that run (
linux/amd64, e-dant/watcher cmake build) is unrelated to this PR.