Stop reporting two environmental faults as red builds - #734
Merged
Conversation
github-actions
Bot
added
ruby
There was a problem hiding this comment.
Pull request overview
Improves CI reliability by distinguishing transient environmental faults from genuine build failures.
Changes:
- Adds retry configuration to both Gradle wrappers and preserves it during regeneration.
- Removes live DNS dependencies from proxy transport tests.
- Consolidates proxy environment setup and cleanup.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
kotlin/build.gradle.kts |
Preserves wrapper retry settings during regeneration. |
kotlin/gradle/wrapper/gradle-wrapper.properties |
Enables three distribution-download retries. |
spec/smithy-bare-arrays/build.gradle.kts |
Preserves retry settings for the Smithy wrapper. |
spec/smithy-bare-arrays/gradle/wrapper/gradle-wrapper.properties |
Enables three distribution-download retries. |
ruby/test/basecamp/oauth_transport_test.rb |
Makes proxy tests independent of live DNS resolution. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Both wrappers carried the Wrapper task's default retries=0, so the download loop got exactly one attempt: a transient services.gradle.org fault printed "Attempt 1/1 failed" and failed the job. Six of those across four unrelated PRs in one afternoon, on both a 503 and an "Unexpected end of file from server" — each costing a log read to classify a fault whose classification is always the same, and training the reflex of re-running red Gradle jobs without reading them. retries=3 gives the loop four attempts with 500/1000/2000ms of backoff. This is the DISTRIBUTION DOWNLOAD, not the build: it cannot mask a flaky test or a real compile error. networkTimeout stays at 10s. Every observed fault was immediate — a 503 or an EOF, never a SocketTimeoutException — so raising it treats a case we have not seen, while multiplying the dead time a genuinely hung endpoint costs by the new attempt count. Revisit if a timeout ever actually shows up. The properties files are generator output, so setting the line is not enough: configure tasks.wrapper in both builds, or the next `./gradlew wrapper` resets retries to 0 with no diff to notice. Verified both ways — with the block, both files regenerate byte-identically (gradlew and the jar too); with it stripped, regeneration silently rewrites retries=3 back to 0. Dependabot's gradle-wrapper bumps rewrite only distributionUrl, so they preserve it either way. Closes #729
#720 recorded two failures in this file and read them as two mechanisms. They are one, and it is not timing. stream_http resolves the proxy with URI#find_proxy INSIDE the advertised deadline, and find_proxy calls IPSocket.getaddress(hostname) to evaluate its loopback rule. Every proxy test here targets a `.test` name, which RFC 6761 reserves as never resolvable — so that call is a real round trip to a resolver that has to answer NXDOMAIN, sitting inside a 0.5-1s budget. Four tests do it: test_proxy_credentials_are_percent_decoded, test_https_proxy_refused_without_ p_use_ssl_support, test_https_proxy_scheme_gets_tls_on_the_proxy_connection, and test_proxy_connect_drip_is_bounded_by_the_deadline. Slow that one resolver down and both recorded failures reproduce verbatim from it: the validation error arrives as Faraday::TimeoutError, and — for the drip test — the request dies in find_proxy, the proxy never receives a CONNECT, and the mechanism queue stays empty. That is why #708's hardening did not hold. It correctly moved the drip test off wall-clock assertions, but an empty queue is indistinguishable from a late one, so no size of wait bound could have covered a request that never reached the proxy. So this widens nothing. with_proxy_env scrubs the proxy environment and stubs getaddress to a fixed non-loopback address, which is the only thing find_proxy asks of it, and the resolver leaves the deadline entirely. The bounds #708 set stay exactly as they were, doing the hang-guard job they were meant for. The five open-coded ENV save/restore blocks collapse into the helper. test_proxy_resolution_dns_is_inside_the_deadline is about resolution, so it opts out with resolves_to: nil and keeps its own stub — now returning a fixed address instead of falling through to a live lookup on the un-cut path. Not addressed, deliberately: the sub-second `assert_operator seconds, :<, TIMEOUT` bounds elsewhere in the file are a different property. There the margin between correct (~10ms) and broken (the deadline itself) IS the deadline, so widening the bound would delete the discrimination rather than protect it. Those stay coupled to real timing until someone finds a mechanism to assert instead; none has been observed failing. Closes #720
jeremy
force-pushed
the
fix/ci-flake-reduction
branch
from
August 13, 2026 01:40
f98c501 to
f57a753
Compare
Member
Author
Rebased onto
|
| proof | REAL_EXIT |
tree diff |
|---|---|---|
REBASED-regen-kotlin-rebased.log |
0 | 0 — entire tracked tree byte-identical |
REBASED-regen-smithy-rebased.log |
0 | 0 — entire tracked tree byte-identical |
REBASED-regen-smithy-rebased-NEGATIVE.log |
0 | 1 — -retries=3 / +retries=0 |
The encoding pin survives regeneration, and by the strongest available argument: the Wrapper task rewrote nothing in the tracked tree, build.gradle.kts included. It only ever writes gradlew, gradlew.bat, the jar and the properties file, and all four came back identical.
The negative control still discriminates, and it discriminates on my block specifically: stripping only tasks.wrapper — with #724's encoding pin verified still present (encoding_pin_still_there=1) — makes regeneration rewrite retries=3 back to 0.
One pre-existing thing the whole-tree diff surfaced
./gradlew wrapper drops an untracked gradlew.bat in both modules. Neither module tracks or gitignores it. This predates both PRs and is unrelated to either change — it is simply what the Wrapper task always emits — but since this PR makes wrapper regeneration a documented path, it is worth knowing that regenerating leaves an untracked file behind. Whether to commit gradlew.bat is a Windows-support decision, not this PR's, so I have left it alone.
Ruby proofs, re-run (unaffected, but confirmed rather than assumed)
| proof | REAL_EXIT |
|---|---|
| DNS instrumentation on the fixed tests | 0 — DNSPROBE_TOTAL_CALLS=0 |
| green under the 1.2s slow resolver | 0 — resolver never consulted |
| whole file (44 runs) | 0 |
| rubocop | 0 |
| mutation matrix, all 5 | 1 each, all RESTORED=OK |
DNSPROBE_TOTAL_CALLS=0 is the same instrumentation that counted 4 real lookups on unmodified main.
Full make on the rebased tree
REAL_EXIT=0, ==> All checks passed, working tree clean afterwards. This matters more than a normal re-run: #724 reworked the Makefile's Gradle-backed check targets with new order-only serialization edges, so this is the first time those edges and the wrapper config have run together.
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
jeremy
added a commit
that referenced
this pull request
Aug 19, 2026
…a String test_proxy_credentials_are_percent_decoded filled a captured = +"" on the proxy thread and asserted on it from the main thread with no join, Queue or condvar. The ordering was incidental — the socket close — and the client's own timeout: 1 can raise while the proxy is still mid-gets on a loaded runner, which reads as absent credentials and points at percent-decoding, which is not the bug. Observed red on CI twice, on a branch touching only Go files. Distinct from #720: that was a find_proxy resolver stall, closed by #734 removing the resolver from these tests. This survives that fix. Queue rather than Thread#join because join(timeout) returning nil leaves you reading the shared String with no edge at all. Here the value's existence is the evidence — it can only be popped because the producer finished reading the preamble — so a wedged producer is an explicitly reported failure rather than an empty match. pop(timeout: 15) is a wait bound, not a timing assertion: this assertion is about content, so a generous wait costs nothing. The sub-second bounds #734 kept on the siblings, where the margin is the deadline, are untouched.
jeremy
added a commit
that referenced
this pull request
Aug 21, 2026
…produced two of them (#794) * Abort from a seam, not a timer, in the two caller-abort lifecycle tests Both landed in #538 and survived #655's sweep because its selector required a no-argument abort() and both pass a reason. The custom-reason test raced a 50ms timer against a 1000ms mocked response — #655's own numbers. It now aborts from inside the MSW handler, so the request has provably reached the transport, and rests on err === reason: the caller's own Error object, which the request timeout (a TimeoutError DOMException) and a completed request (undefined) cannot produce. The backoff test raced a 100ms timer against a 2s Retry-After window and paired it with a redundant Date.now() ceiling. It now aborts from hooks.onRetry, the seam executeWithRetry fires immediately before it sleeps, and rests on starts == [1]: a sleep that ignored the signal would run the delay out and then begin attempt 2, which is what the deleted ceiling was standing in for. The abort is queued as a microtask rather than called synchronously so it lands inside the sleep, exercising the abort listener rather than the already-aborted fast path. * Hand the captured CONNECT preamble across a Queue instead of sharing a String test_proxy_credentials_are_percent_decoded filled a captured = +"" on the proxy thread and asserted on it from the main thread with no join, Queue or condvar. The ordering was incidental — the socket close — and the client's own timeout: 1 can raise while the proxy is still mid-gets on a loaded runner, which reads as absent credentials and points at percent-decoding, which is not the bug. Observed red on CI twice, on a branch touching only Go files. Distinct from #720: that was a find_proxy resolver stall, closed by #734 removing the resolver from these tests. This survives that fix. Queue rather than Thread#join because join(timeout) returning nil leaves you reading the shared String with no edge at all. Here the value's existence is the evidence — it can only be popped because the producer finished reading the preamble — so a wedged producer is an explicitly reported failure rather than an empty match. pop(timeout: 15) is a wait bound, not a timing assertion: this assertion is about content, so a generous wait costs nothing. The sub-second bounds #734 kept on the siblings, where the margin is the deadline, are untouched. * Record the corrected abort-on-a-timer selector where the next sweeper stands #655 scoped its fix with an rg typed into an issue body, requiring a no-argument abort(); #715 shipped no script, lint rule or make target, so there is no durable selector to widen. The corrected form goes in CONTRIBUTING.md under Testing, stated with its coverage bound rather than as a claim to cover every call site, alongside the rule it enforces and the Ruby reading of the same rule. Whether this deserves an actual gate is a separate decision, deliberately not taken here. * Gate the timer-scheduled abort with an oxlint rule, not another selector #655 declared this class contained on the strength of an rg typed into an issue body; two survivors shipped anyway and one went red in CI two months later. The finding that there is no durable instrument is the reason to build the first one, and this is the accident class — a colleague adding a timer, no adversary to route around it — so a syntactic invariant is the right shape. An oxlint JS plugin rather than a grep in a make target, and rather than introducing eslint: oxlint is already the repo's TS linter and supports local JS plugins, so this adds no dependency and no lockfile churn. It reads the AST, which is what lets it tell a timer that SCHEDULES an abort from one the abort is merely racing — the proximity selector I wrote first flags device.test.ts:1632 and the rule does not, so no suppression is needed there. The rule's header states what it cannot see rather than claiming every call site. The self-test runs beside the rule, not only when the rule is edited. oxlint's JS plugin API is alpha and un-semvered behind a caret range: a bump that stopped dispatching the visitor leaves the gate exiting 0 while matching nothing. Demonstrated by mutating create() to return a visitor that never fires — the gate stayed green, the self-test went red. Wired into make ts-check AND into the TypeScript CI job as its own steps, since no CI job in this repo runs make check. * Read the rule's findings as JSON, and run the self-test in both environments The gate's first CI run went red, and the rule was not the reason: oxlint detects GITHUB_ACTIONS and switches to the ##[error] annotation format, which the line-scraping parser could not match. The rule fired on all three positive fixtures; the harness could not see it. It failed closed, which is the behaviour it was built for, but a harness that reads differently on the machine that matters is a harness that will one day be wrong in the other direction. Two changes: parse --format json and match on the rule CODE rather than message text, so a finding also has to come from this rule and not merely exist; and run every case twice, once with GITHUB_ACTIONS unset and once with it set, so the environment is part of the matrix rather than something the local run happens to get right. Invoke the oxlint binary from node_modules/.bin directly instead of through npx, so resolution is the same everywhere too. Re-proved the disarmed-visitor mutation against the rewritten harness: the gate stays green under a simulated CI environment, and the self-test reds in both environments. * Assert the promptness the backoff-abort test is named for The attempt ledger only catches a sleep that ignores cancellation entirely. A sleep that noticed the abort but deferred its rejection to the timer's expiry never starts attempt 2 and still rejects with the caller's reason, so every assertion in the test passed — two seconds late — while the behavior in its name was broken. Demonstrated: that mutant runs green against the previous version in 2.77s and red against this one in 17ms. Fake timers close it by construction rather than by measurement. With setTimeout frozen, the 2s backoff timer fires only if the test advances the clock, and it never does, so "the request settled" and "it settled before the backoff elapsed" are one statement. The barrier is a count of event-loop turns via setImmediate, not an elapsed-time bound: under load a turn takes longer, but how many turns a settlement needs does not change. No wall-clock threshold comes back (#783). Also from review: the lint rule's ancestor walk compared the callback function against the timer's first argument, which is the wrapper node when TypeScript-only syntax sits in between — `setTimeout((() => c.abort()) as () => void, 50)` and the `satisfies` and `!` spellings all slipped a gate whose header claimed inline callbacks were covered. The walk now erases those wrappers first, with a self-test case that reds without the fix. CONTRIBUTING's population bound was dot-member only while the rule also recognizes bare `abort()` and `c["abort"]()`; it now covers all three spellings, which return the same 15 sites today. * Restore the frozen clock where a timed-out test can still reach it A `finally` inside the test does not run when vitest times the test out — the suspended async function is never resumed — and a hung test is exactly what a broken abort produces, so the one failure the cleanup existed for was the one it could not cover. Measured: on a timed-out test the finally does not run, the suite's afterEach does, and it sees vi.isFakeTimers() still true. The restore moves there, and the try/finally goes rather than sitting beside it covering strictly fewer paths. CONTRIBUTING said to drop the elapsed-time ceiling because identity and the attempt ledger already discriminate. That is true except when the named behavior is promptness, which is the case this PR just had to fix, so as written it told contributors to delete the only promptness check and put nothing back. It now says to replace it, and how: freeze the clock, count event-loop turns, never advance. * Make the population sweep as quote-blind as the rule it bounds The rule's isAbortCall reads the computed key's VALUE, so controller['abort']() is an abort to it exactly as controller["abort"]() is. The documented population bound — in the rule header and in CONTRIBUTING — matched only the double-quoted spelling, so a sweep typed from the docs could report one site fewer than the rule recognizes, which is the same drift between stated and actual coverage the bound exists to prevent. Widen it to both quote styles in both places, kept byte-identical. Both the old and the new command return the same 15 lines under typescript/tests today, so no classification changes. The self-test's computed-abort case now carries the single-quoted spelling too: a rule mutated to be quote-sensitive reports [2,3,4] where [2,3,4,5] is expected, in both environments, so the claim that the quote style is invisible to the rule is asserted rather than read. * Recognize a computed-literal timer property, as the rule already does for abort
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.
Two CI-reliability fixes. They share a harm, not a mechanism: today CI reports
an environmental fault with the same red X as a real failure, which trains the
reflex of re-running red jobs without reading them.
Closes #729
Closes #720
#729 — both Gradle wrappers set
retries=0kotlin/andspec/smithy-bare-arrays/both carried the Wrapper task'sdefault
retries=0, so the wrapper's download loop got exactly one attempt:Attempt 1/1 failedand the job was red. Set to 3 in both — four attempts,500/1000/2000ms backoff.
The files are generator output, so setting the line was not enough. There
is no
gradle wrapperinvocation anywhere in the repo today, but the Wrappertask exposes
getRetries(), so both root builds now configuretasks.wrapperand the setting survives regeneration.
networkTimeoutstays at 10s, deliberately. Every observed fault wasimmediate — a 503 or an EOF, never a
SocketTimeoutException— so raising ittreats a case we have not seen, while multiplying what a genuinely hung
endpoint costs (4 attempts × 10s + 3.5s backoff instead of one 10s wait).
This retries the distribution download, not the build. It cannot mask a
flaky test or a real compile error.
What is proven
Read out of the shipped
gradle-wrapper.jarbytecode:WrapperExecutorreadsretries/retryBackOffMsfrom the properties file;WrapperConfiguration's defaults are0and500.Install.forceFetchloopsretries + 1times, doubling the backoff, andlogs
Attempt %d/%d failed. Reason: %s— the exact string in Both Gradle wrappers set retries=0, so a transient services.gradle.org 503 fails the job #729's logs.catch (IOException). The 503 surfaces fromDownload.download'sgetResponseCode()inside that region, which is whyBoth Gradle wrappers set retries=0, so a transient services.gradle.org 503 fails the job #729's log shows the loop's own message before the rethrow.
validateDistributionUrl=trueadds no separate un-retried network call(checked empirically,
gradle-retry-validate-on.log).A/B against a blackholed distribution URL (
gradle-retry-ab.log):Regeneration survival, with a negative control:
gradle-regen-smithy.log./gradlew wrapperreproduces the committed file byte-identically,REAL_EXIT=0gradle-regen-kotlin.loggradlew+gradle-wrapper.jarbyte-identical toogradle-regen-smithy-NEGATIVE.logtasks.wrapperblock stripped, regeneration silently rewritesretries=3→retries=0What is NOT proven
That this absorbs a real
services.gradle.org503. A 503 cannot besummoned on demand, and I did not fabricate one. The chain above shows the
loop's ceiling changed and that a 503 lands inside the retried region; what
remains unverified is whether a real outage clears inside 500/1000/2000ms of
backoff. If services.gradle.org is down for minutes, four attempts still fail —
just more slowly.
Dependabot's
gradle-wrapperbumps have rewritten onlydistributionUrlinevery historical instance (three checked), so they preserve the setting; that
is evidence, not a guarantee about a tool we do not control.
#720 —
OAuthTransportTesttiming/network fragilityThe issue records two failures and reads them as two mechanisms. They are
one, and it is not timing.
stream_httpresolves the proxy withURI#find_proxyinside the advertiseddeadline (
fetcher.rb:363), andfind_proxycallsIPSocket.getaddress(hostname)to evaluate its loopback rule. Every proxy testin the file targets a
.testname — which RFC 6761 reserves as neverresolvable — so that is a real round trip to a resolver that has to answer
NXDOMAIN, sitting inside a 0.5–1s budget.
The sweep found four such tests, not two:
test_proxy_credentials_are_percent_decodedtest_https_proxy_refused_without_p_use_ssl_supporttest_https_proxy_scheme_gets_tls_on_the_proxy_connectiontest_proxy_connect_drip_is_bounded_by_the_deadlineInstrumenting
IPSocket.getaddresson unmodifiedmainshows all four makingreal lookups (
ruby-dns-instrument-BEFORE.log,DNSPROBE_TOTAL_CALLS=4).Red proof
Slow that one resolver to 1.2s and both recorded failures reproduce
verbatim, from a single injected cause (
ruby-RED-slow-resolver-BEFORE.log,REAL_EXIT=1):The first is character-for-character the failure recorded in #720's comment.
This also explains why #708 did not hold. #708 correctly moved the drip
test off wall-clock assertions to a mechanism assertion, and argued its 15s
wait bound was far outside any plausible scheduling delay. That argument was
right; the bound was never the problem. With a slow resolver the request dies
in
find_proxy, the proxy never receives a CONNECT at all, and an emptyqueue is indistinguishable from a late one no matter how long you wait. No size
of wait bound could have covered it.
The fix widens nothing
with_proxy_envscrubs the proxy environment and stubsgetaddressto a fixednon-loopback address — the only thing
find_proxyasks of it — so the resolverleaves the deadline entirely. Every bound #708 set stays exactly as it was,
doing the hang-guard job it was meant for. Five open-coded ENV save/restore
blocks collapse into the helper.
test_proxy_resolution_dns_is_inside_the_deadlineis about resolution, so itopts out with
resolves_to: niland keeps its own stub.Green under the same 1.2s resolver (
ruby-GREEN-slow-resolver-AFTER.log,REAL_EXIT=0): the resolver is never called at all, and the set runs in 1.34sinstead of 16.5s.
The tests still catch their bugs
Deflaking must not defang. Each changed test was run against a mutated
fetcher.rb(ruby-MUTATION-matrix.log); all five die, each at its guardingassertion, and the file was restored by copy and verified with
diff -qafterevery run:
REAL_EXIThttps://proxy refusal..._refused_without_p_use_ssl_supportassert_raises(OauthError)— gotConnectionFailed..._connect_drip_is_bounded_by_the_deadlineassert cut.pop— the mechanism assertion..._credentials_are_percent_decodedProxy-Authorization(p%40s+svsp@s+s)p_use_sslpassthrough..._gets_tls_on_the_proxy_connectionExpected: 22)find_proxybound..._dns_is_inside_the_deadlineassert_raises— gotReadDeadlineExceededThat last one contradicted a comment claiming the error class does not
discriminate there. The comment is corrected rather than left standing.
What is NOT proven, and what I did not do
That the recorded CI failures were caused by resolver latency. The injected
cause reproduces both messages exactly, and it is by far the most economical
explanation, but no DNS timing was captured on those runs. If the real cause
was something else, this fix does not address it. What is proven is that
these tests no longer depend on the resolver at all — one whole class of
environmental input is gone from them.
Repetition is weak evidence, reported as such. 12/12 green under 2× CPU
saturation (
ruby-repeat-under-parallel-load.log). That is not a proof and isnot offered as one; the mutation matrix and the red/green pair are the
argument.
Deliberately not addressed — the sub-second
assert_operator seconds, :<, TIMEOUTbounds elsewhere in the file (test_device_auth_non_2xx_...,test_discovery_non_2xx_..., and siblings). These have the same shape but adifferent property: the margin between correct (~10ms) and broken (the
deadline itself) is the deadline, so widening the bound deletes the
discrimination rather than protecting it, and no mechanism assertion is
available to replace it. They stay coupled to real timing. None has been
observed failing. Recording the reasoning here so it is not re-litigated.
Also left alone:
test_proxy_credentials_are_percent_decodedreadscapturedacross threads without explicit synchronization. It is ordered in practice —
the proxy appends every header line before the
closethat makes the clientraise — so there is a happens-before edge; flagging it rather than churning it.
Summary by cubic
Stops two environmental faults from showing as red builds. Gradle wrapper now retries transient distribution downloads; OAuth proxy tests no longer depend on live DNS inside their deadlines.
Gradle wrapper retries
retries=0→retries=3withretryBackOffMs=500(four attempts).networkTimeoutstays 10s.tasks.wrapperin bothkotlin/andspec/smithy-bare-arrays/so./gradlew wrapperre-emits the setting.tasks.wrapperblocks in both builds.OAuth proxy tests
with_proxy_envhelper that scrubs proxy env and stubsIPSocket.getaddressto a fixed non-loopback address, removing DNS from proxy deadlines.test_proxy_resolution_dns_is_inside_the_deadlineopting out with its own resolver stub.IPSocket.getaddressstub/restore and env handling. No runtime code changes or migrations.Written for commit f57a753. Summary will update on new commits.