ci: fix dynamic-import response truncation flake (proxy transport race) + socket instrumentation - #41399
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📜 Recent review details⏰ Context from checks skipped due to timeout. (13)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (3)📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-02-26T19:25:44.063ZApplied to files:
📚 Learning: 2026-05-06T12:21:44.083ZApplied to files:
🔇 Additional comments (3)
WalkthroughThe PR configures Rocket.Chat socket timeouts and Traefik dynamic-import routing, buffering, and retries. It adds a high-concurrency truncation regression test and collects Traefik logs when E2E tests fail. ChangesDynamic import transport handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #41399 +/- ##
===========================================
- Coverage 68.52% 68.51% -0.02%
===========================================
Files 4114 4129 +15
Lines 159688 160180 +492
Branches 28938 29230 +292
===========================================
+ Hits 109432 109744 +312
- Misses 45213 45339 +126
- Partials 5043 5097 +54
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Closing: the premise didn't survive verification. CI already fronts the server with traefik ( |
Recurring e2e flake: /__meteor__/dynamic-import/fetch dies with ERR_INCOMPLETE_CHUNKED_ENCODING (headers sent, zero body bytes) and the killer leg was never attributed. Instead of another speculative fix, instrument CI so the next occurrence names it: - traefik JSON access logs enabled in docker-compose-ci.yml and dumped on e2e failure (traefik was missing from the log dump step), telling apart upstream truncation from proxy/client aborts - SOCKET_FORENSICS env flag: logs any response whose socket closes after headers but before the body finishes, response stream errors, and the stack of whoever destroys a socket mid-response
2ce367d to
fecf51a
Compare
KevLehman
changed the title
First reproduction attributed the truncation to the traefik->node leg: traefik aborts the response copy (net/http: abort Handler, OriginStatus 0) while node either finished cleanly or ended the socket mid-stream via the destroySoon path (destroy at writable finish, exactly 48KiB of body sent). The destroy stack cannot name the original end() caller, so: - patch Socket#end like Socket#destroy (shared in-flight teardown logger) - log socket timeout events (webapp reaps sockets 5s idle by default) with socket age and pending bytes, to catch the reaper racing a reused conn - include pendingBytes/ageMs on truncation events
…n both legs Forensics v2 named node kill path: socketOnEnd — the peer half-closes first and node then terminates the in-flight response. The remaining question is which side breaks the connection and why; a headers-only (-s 96) tcpdump sidecar sharing the traefik netns sees both the docker-proxy<->traefik and traefik<->node legs. Uploaded only on e2e failure.
…loses Deterministic reproduction of the server-side link of the dynamic-import truncation flake: a client that FINs after fully sending its request is entitled to the complete response, but node (httpAllowHalfOpen=false, the shape meteor webapp configures) aborts the in-flight chunked response via socketOnEnd -> socket.end(). Two red cases (FIN before response, FIN after headers) plus a green control. Red by design until the server tolerates half-closing clients.
Root cause found (2nd instrumented reproduction, run 29560562870,
|
- drop serverstransport.maxidleconnsperhost=-1: its keep-alives-disabled mode is where Go transport rarely closes the upstream conn mid body copy, truncating chunked responses (RCA on the PR) - set serverstransport.idleconntimeout=3s instead: traefik drops idle upstream conns before the meteor webapp 5s reaper (the reason the -1 flag existed) can kill one it would reuse - actually attach the test-retry middleware to the rocketchat router; it was defined but never referenced, so RetryAttempts was always 0 - skip the half-close truncation spec: red by design, documents the server-side behavior the config fix routes around
Replaces the half-close unit spec (tested node directly, so the proxy config fix could never turn it green) with a proxy-level regression test: 30k concurrent POSTs of real dynamic-import bodies captured from a failing CI run (including the recurring AppLayoutThemeWrapper.tsx request), zero truncations allowed. Traefik fix temporarily reverted on this commit to confirm the test goes red against the broken config; the fix commit follows.
- drop serverstransport.maxidleconnsperhost=-1: its one-conn-per-request mode both races the ReverseProxy body copy (use of closed network connection -> truncated chunked responses) and exhausts ephemeral ports under load (dial tcp: cannot assign requested address -> 502 bursts, reproduced by the new truncation hammer test) - set serverstransport.idleconntimeout=3s instead: traefik drops idle upstream conns before the meteor webapp 5s reaper (the reason the -1 flag existed) can kill one it would reuse - attach the test-retry middleware to the rocketchat router; it was defined but never referenced, so nothing ever retried
The 3s idleConnTimeout traded the -1 body-copy race for the Go idle-expiry-vs-checkout race (reproduced on CE UI shards: same "use of closed network connection" on dynamic-import). No timeout value wins the timing game against the webapp 5s reaper, so stop playing it: default 90s idle pool, and buffering+retry middlewares so a request that lands on a server-reaped conn (fails before response headers) is replayed transparently, POST bodies included.
…ia env Every proxy-side config permutation loses a timing game (measured on the PR): -1 races the body copy and exhausts ports, a short idleConnTimeout races idle expiry against checkout, and buffering+retry breaks response semantics suite-wide. The only race-free arrangement is the server outliving the proxy pool: HTTP_SOCKET_TIMEOUT_MS (CI compose sets 120s, above traefik 90s idleConnTimeout) detaches webapp 5s reaper re-arming and raises socket/keepAlive/headers timeouts, so traefik always closes pooled conns first and can never reuse one node already killed. Unset (prod) nothing changes.
…sponse race pcap from the v3 run (pooled conn, reaper disabled) shows the same signature as every reproduction: request written in two segments, server responds within ~1ms, traefik FINs 0.3ms after the first response bytes and RSTs the rest — go net/http treats the fast response as arriving before the request write completed and kills the conn mid body copy. Config knobs only modulated the frequency; the fix is removing the window: a dedicated dynamic-import router with buffering makes the tiny POST body memory-resident so headers+body coalesce into one upstream write, and retry replays any pre-header failure. Scoped to that route because suite-wide buffering broke 404/CORS semantics.
KevLehman
changed the title
|
/jira ARCH-2228 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docker-compose-ci.yml">
<violation number="1" location="docker-compose-ci.yml:52">
P3: Since the dynamic-import route only receives POST requests, attaching `test-retry` here is effectively a no-op: Traefik's retry middleware skips non-idempotent methods unless `retryNonIdempotentMethod: true` is set, so all the actual retry protection for this fix comes from `test-buffer`'s `retryExpression`. Consider dropping `test-retry` from this router (to avoid implying it does work here) or explicitly setting `retryNonIdempotentMethod: true` on it if retrying the POST via this middleware is actually intended.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| # 404/CORS response semantics | ||
| traefik.http.routers.rocketchat-dynimport.rule: PathPrefix(`/__meteor__/dynamic-import`) | ||
| traefik.http.routers.rocketchat-dynimport.service: rocketchat | ||
| traefik.http.routers.rocketchat-dynimport.middlewares: test-buffer,test-retry |
There was a problem hiding this comment.
P3: Since the dynamic-import route only receives POST requests, attaching test-retry here is effectively a no-op: Traefik's retry middleware skips non-idempotent methods unless retryNonIdempotentMethod: true is set, so all the actual retry protection for this fix comes from test-buffer's retryExpression. Consider dropping test-retry from this router (to avoid implying it does work here) or explicitly setting retryNonIdempotentMethod: true on it if retrying the POST via this middleware is actually intended.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docker-compose-ci.yml, line 52:
<comment>Since the dynamic-import route only receives POST requests, attaching `test-retry` here is effectively a no-op: Traefik's retry middleware skips non-idempotent methods unless `retryNonIdempotentMethod: true` is set, so all the actual retry protection for this fix comes from `test-buffer`'s `retryExpression`. Consider dropping `test-retry` from this router (to avoid implying it does work here) or explicitly setting `retryNonIdempotentMethod: true` on it if retrying the POST via this middleware is actually intended.</comment>
<file context>
@@ -37,7 +40,17 @@ services:
+ # 404/CORS response semantics
+ traefik.http.routers.rocketchat-dynimport.rule: PathPrefix(`/__meteor__/dynamic-import`)
+ traefik.http.routers.rocketchat-dynimport.service: rocketchat
+ traefik.http.routers.rocketchat-dynimport.middlewares: test-buffer,test-retry
+ traefik.http.middlewares.test-buffer.buffering.retryExpression: IsNetworkError() && Attempts() < 4
healthcheck:
</file context>
2 participants
Proposed changes (including videos or screenshots)
Fixes the years-old CI flake where e2e tests died with
Target page, context or browser has been closed/Application Error/ stuck boot spinners — all one bug:POST /__meteor__/dynamic-import/fetchresponses truncated mid-body (ERR_INCOMPLETE_CHUNKED_ENCODING), poisoning the client module loader.Root cause (established via pcap + socket forensics + traefik debug logs on two instrumented reproductions)
A race in Go's
net/httptransport inside traefik, independent of every pooling config: when the tiny dynamic-import POST is written in two TCP segments (headers, then ~57-107B body) and node answers within ~1ms (cached module tree), the transport treats the response as arriving before the request write finished, marks the connection dead, and closes it 0.3ms after the first response bytes — mid body copy (httputil: ReverseProxy read error during body copy: use of closed network connection, thennet/http: abort Handler). The browser has already received the 200 headers, so nothing can retry, and the truncated body kills the module fetch. Packet captures showed byte-identical kill signatures on fresh conns (maxidleconnsperhost=-1) and healthy pooled conns — config knobs only modulated frequency.Two secondary defects found en route:
test-retrymiddleware was defined but never attached to the router (RetryAttemptswas always 0);maxidleconnsperhost=-1(2024 workaround for meteor webapp's 5s idle-socket reaper) additionally exhausts ephemeral ports under load (dial tcp: cannot assign requested address→ 502 bursts).The fix
/__meteor__/dynamic-importwithbuffering+retry: buffering makes the request body memory-resident so headers+body coalesce into a single upstream write — no write-in-progress window, no early-response race; retry replays any pre-header failure. Scoped to this route because suite-wide buffering breaks 404/CORS response semantics.HTTP_SOCKET_TIMEOUT_MS(opt-in env, CI sets 120s): detaches webapp's 5s reaper re-arming and raises socket/keepAlive/headers timeouts above traefik's 90s idle pool, so the proxy always closes pooled conns first and never reuses one node killed. Unset (prod) = no behavior change.test-retryactually attached;-1flag removed (keep-alive pool restored); traefik added to the e2e failure log dump (it was missing).Regression test
tests/end-to-end/api/http-response-truncation.ts: 30k concurrent POSTs of real dynamic-import bodies (captured from a failing run, incl. the recurringAppLayoutThemeWrapper.tsxrequest) through the CI traefik; zero truncations allowed, plus a fixture-staleness canary. Red on the broken config (502 burst in both API jobs), green with the fix.Validation
Broken-config states each reproduced their failure signature on camera; fix state: 3 consecutive fully green CI runs (hammer + ~1350 e2e tests per run), zero truncation signatures in server/traefik logs. The diagnostic instrumentation used for the investigation (socket forensics hooks, traefik access/debug logs, tcpdump sidecar) was removed once the root cause was proven — this PR carries only the fixes and the regression test.
Issue(s)
Steps to test or reproduce
Revert the traefik labels in
docker-compose-ci.yml(drop therocketchat-dynimportrouter) and the API jobs fail the truncation hammer; restore them and it passes.Further comments
Prod is unaffected: changes are CI compose/workflow config plus one env-gated code path (
HTTP_SOCKET_TIMEOUT_MS) that is inert unless the env var is set.Task: ARCH-2272
Summary by CodeRabbit