-
Notifications
You must be signed in to change notification settings - Fork 13.9k
ci: fix dynamic-import response truncation flake (proxy transport race) + socket instrumentation #41399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
ci: fix dynamic-import response truncation flake (proxy transport race) + socket instrumentation #41399
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
fecf51a
ci: instrument CI for dynamic-import response truncation forensics
KevLehman 6ec6373
ci: timestamp socket-forensics events for correlation with test failures
KevLehman 3b99b80
ci: enable traefik debug logs to surface upstream connection errors
KevLehman 6a1d21c
ci: capture socket end() call sites and 5s-timeout firings in forensics
KevLehman 690d261
ci: capture headers-only pcap at traefik to attribute first FIN/RST o…
KevLehman d414f3e
test: prove node truncates in-flight responses when the client half-c…
KevLehman 299e50e
ci: neutralize dynamic-import truncation at the proxy, config only
KevLehman 4f84833
test: keep half-close truncation spec enabled as red/green gate
KevLehman a42ec17
test: hammer dynamic-import through traefik to catch response truncation
KevLehman b678863
ci: neutralize dynamic-import truncation at the proxy, config only
KevLehman 0b6ea1c
ci: fix traefik idleConnTimeout flag path (forwardingtimeouts)
KevLehman 5791a25
ci: make reaped-conn reuse retryable instead of racing the reaper
KevLehman 7962a37
ci: raise server socket timeout above the proxy idle window, opt-in v…
KevLehman 704cbaf
ci: buffer dynamic-import requests to close the go-transport early-re…
KevLehman 3a6cff2
ci: remove diagnostic instrumentation, keep fixes and regression test
KevLehman File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import { WebApp } from 'meteor/webapp'; | ||
|
|
||
| // meteor's webapp arms a 5s idle-socket timeout (and re-arms it after every | ||
| // response), which kills keep-alive conns a fronting proxy may be about to | ||
| // reuse — the root of the CI dynamic-import truncation flake (PR #41399). | ||
| // Overriding it is opt-in: only CI sets HTTP_SOCKET_TIMEOUT_MS, pointing it | ||
| // above traefik's idleConnTimeout so the proxy always closes conns first. | ||
| const timeout = parseInt(process.env.HTTP_SOCKET_TIMEOUT_MS ?? '', 10); | ||
| if (timeout > 0) { | ||
| const { httpServer, _timeoutAdjustmentRequestCallback } = WebApp as typeof WebApp & { | ||
| _timeoutAdjustmentRequestCallback: (req: unknown, res: unknown) => void; | ||
| }; | ||
| httpServer.removeListener('request', _timeoutAdjustmentRequestCallback); | ||
| httpServer.setTimeout(timeout); | ||
| httpServer.keepAliveTimeout = timeout; | ||
| httpServer.headersTimeout = timeout + 1000; | ||
| } |
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
86 changes: 86 additions & 0 deletions
86
apps/meteor/tests/end-to-end/api/http-response-truncation.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| import { expect } from 'chai'; | ||
| import { describe, it } from 'mocha'; | ||
|
|
||
| import { apiUrl } from '../../data/api-data'; | ||
|
|
||
| // Regression test for the dynamic-import response truncation flake (PR #41399). | ||
| // | ||
| // Through traefik with `serverstransport.maxidleconnsperhost=-1` (upstream | ||
| // keep-alive disabled), Go's transport rarely closes its upstream connection | ||
| // while the ReverseProxy is still copying a chunked gzip response body | ||
| // ("use of closed network connection"), truncating the response mid-stream: | ||
| // the browser gets 200 headers and then ERR_INCOMPLETE_CHUNKED_ENCODING, and | ||
| // the meteor module loader hangs the page. Rate is ~1.6e-4 per request under | ||
| // load, so this test replays the exact request shape (real dynamic-import | ||
| // bodies captured from a failing CI run — the fatal one is the same | ||
| // AppLayoutThemeWrapper.tsx request seen in every reproduction) at high | ||
| // volume and concurrency: red on the broken proxy config, green once | ||
| // upstream keep-alive is restored. | ||
| // | ||
| // The bodies are build-dependent fixtures: if the app tree renames these | ||
| // modules the responses shrink below the gzip threshold and lose the | ||
| // chunked+gzip shape the race needs, so the test asserts that shape first. | ||
|
|
||
| const FETCH_URL = `${apiUrl}/__meteor__/dynamic-import/fetch`; | ||
|
|
||
| const BODIES = [ | ||
| '{"client":{"components":{"AppLayoutThemeWrapper.tsx":1}}}', | ||
| '{"client":{"meteor":{"login":{"index.ts":1,"cas.ts":1,"crowd.ts":1,"facebook.ts":1,"oauth.ts":1,"LoginCancelledError.ts":1,"google.ts":1,"ldap.ts":1,"meteorDeveloperAccount.ts":1,"password.ts":1,"saml.ts":1,"twitter.ts":1}},"lib":{"2fa":{"overrideLoginMethod.ts":1},"wrapRequestCredentialFn.ts":1,"loginServices.ts":1}},"node_modules":{"@rocket.chat":{"string-helpers":{"package.json":1,"dist":{"esm":{"index.js":1}}}}}}', | ||
| ]; | ||
|
|
||
| const TOTAL_REQUESTS = 30000; | ||
| const CONCURRENCY = 64; | ||
|
|
||
| const fetchModuleTree = async (body: string): Promise<{ error?: string }> => { | ||
| try { | ||
| const response = await fetch(FETCH_URL, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'text/plain;charset=UTF-8' }, | ||
| body, | ||
| }); | ||
| if (response.status !== 200) { | ||
| return { error: `unexpected status ${response.status}` }; | ||
| } | ||
| await response.text(); | ||
| return {}; | ||
| } catch (error) { | ||
| return { error: `${error} (cause: ${(error as { cause?: unknown }).cause})` }; | ||
| } | ||
| }; | ||
|
|
||
| describe('dynamic-import response delivery through the proxy', () => { | ||
| it('should keep the chunked gzip response shape the race depends on', async () => { | ||
| const response = await fetch(FETCH_URL, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'text/plain;charset=UTF-8' }, | ||
| body: BODIES[1], | ||
| }); | ||
| const text = await response.text(); | ||
| expect(response.status).to.equal(200); | ||
| expect( | ||
| text.length, | ||
| 'fixture went stale: response too small to be gzip-chunked, update BODIES from a fresh browser trace', | ||
| ).to.be.greaterThan(10000); | ||
| }); | ||
|
|
||
| it(`should deliver ${TOTAL_REQUESTS} concurrent dynamic-import responses without a single truncation`, async function () { | ||
| this.timeout(10 * 60 * 1000); | ||
|
|
||
| const failures: string[] = []; | ||
| let issued = 0; | ||
|
|
||
| const worker = async (): Promise<void> => { | ||
| while (issued < TOTAL_REQUESTS && failures.length === 0) { | ||
| const current = issued++; | ||
| const { error } = await fetchModuleTree(BODIES[current % BODIES.length]); | ||
| if (error) { | ||
| failures.push(`request #${current}: ${error}`); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| await Promise.all(Array.from({ length: CONCURRENCY }, worker)); | ||
|
|
||
| expect(failures).to.deep.equal([]); | ||
| }); | ||
| }); |
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3: Since the dynamic-import route only receives POST requests, attaching
test-retryhere is effectively a no-op: Traefik's retry middleware skips non-idempotent methods unlessretryNonIdempotentMethod: trueis set, so all the actual retry protection for this fix comes fromtest-buffer'sretryExpression. Consider droppingtest-retryfrom this router (to avoid implying it does work here) or explicitly settingretryNonIdempotentMethod: trueon it if retrying the POST via this middleware is actually intended.Prompt for AI agents