Repository navigation
fix(ci): stop the Node 24.21 healthcheck hang and cap job runtimes - #267
Merged
Merged
Conversation
The "Node 24" job resolves to v24.21.0, whose http2 client defers RST_STREAM while receiving (nodejs/node#65093): a stream closed from its 'response' handler before the body is read is never destroyed, so the client keeps its TCP connection open, the server's graceful stop() never completes and the healthcheck integration suite hangs until GitHub's 6-hour limit. - The worker-components httpStatus helper drains the body before closing the session (16/16, exits in ~1 s on Node 24.20.0, 24.21.0 and 26.10.0). - Every workflow job gets timeout-minutes (15; 30 for the npm release job; the slowest job measured over 224 successful runs took 116 s), so a hang fails in minutes instead of holding a runner for hours. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
The latest Buf updates on your PR. Results from workflow Buf Proto / lint (pull_request).
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (11)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughWorkflow jobs now have execution timeouts. The healthcheck integration test helper now waits for the HTTP/2 response stream to end before closing its session and returning the captured status. ChangesWorkflow Job Timeouts
HTTP/2 Response Test Handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The workflow limits bound job runtimes, and the healthcheck helper waits for complete responses before closing its session. Available evidence establishes no ordinary-path regression or concrete merge-blocking impact. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change does not show a new production entry point or a verified security issue. The main design risk is that a release could be stopped after publication starts but before its accompanying release and registry updates finish. No evidence shows that normal releases approach the new deadline. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
type:bug
@connectum/auth
@connectum/cli
@connectum/core
@connectum/events
@connectum/events-amqp
@connectum/events-kafka
@connectum/events-nats
@connectum/events-redis
@connectum/healthcheck
@connectum/interceptors
@connectum/otel
@connectum/protoc-gen-catalog
@connectum/reflection
@connectum/test-fixtures
@connectum/testing
commit: |
9 of 13 tasks
intech
added a commit
that referenced
this pull request
Sep 28, 2026
## Summary `shutdown.forceCloseOnTimeout` (default `true`) is documented to make `server.stop()` finish within `shutdown.timeout` "even if clients hold connections open", but it only destroyed HTTP/2 sessions. The following cases were measured in Docker on Node 22.13.0, 24.21.0 and 26.10.0 with a 2 s timeout. In each of them a client kept the server — and the process — alive **forever**: - **Default transport** (plaintext, `allowHTTP1: true`), which is `http.createServer` and therefore HTTP/1.1 only. There are no sessions, so force-close did nothing against a long, unfinished or idle request. - **TLS**: a connection that never completed the handshake or sent an unfinished request. - **h2c with a client ignoring GOAWAY** on Node 24+. The session is already destroyed and its half-closed socket waits for the peer's FIN. Two more gaps: - **Node 22** `server.close()` sends no GOAWAY (added in Node 24, nodejs/node#57586), so every idle HTTP/2 client stretched `stop()` to the full timeout. - A **TLS session completing its handshake after `close()`** got no GOAWAY on any version. What changes in `TransportManager`: - It tracks every accepted TCP connection (`'connection'`, all modes) and destroys them on the force-close path. For TLS the raw socket also covers pre-handshake connections. - `close()` sends GOAWAY (`session.close()`) to every HTTP/2 session itself, and closes sessions created while it is closing. - `forceCloseOnTimeout: false` still destroys nothing. Its JSDoc now says precisely what happens. - Connectum never calls `process.exit()`. Alternatives measured and rejected: `http.Server#closeAllConnections` (does not exist on `Http2Server`/`Http2SecureServer`), destroying via `session.socket` (throws on Node 22, no-op on 24/26), `unref`, and `process.exit`. ## Type of change - [x] Bug fix (non-breaking) - [ ] New feature (non-breaking) - [ ] Breaking change (documented in migration guide) - [ ] Documentation / chore / internal Observable change with the default config: connections that outlive the shutdown timeout are now actually terminated on HTTP/1.1 and TLS, as the docs promised. ## Test plan - [x] `pnpm build && pnpm typecheck && pnpm test` pass locally (33/33 turbo tasks) - [x] `pnpm lint` passes locally. The only warning, `packages/events/src/broadcast.ts:18`, pre-exists on `main`. - [x] New `packages/core/tests/unit/TransportManager.forceClose.test.ts`, 5 scenarios: - three transports × a raw half-open client that never closes: the graceful phase keeps it, the force path closes it; - an idle HTTP/2 client drains without the force path; - a late TLS session receives GOAWAY. Results: 5/5 under node, bun and esbuild. On Docker `node:22.13.0` (esbuild) it also passes 5/5. The same test on `main` code gives 1/5. - [x] The TLS pair is generated per run with `openssl`, so no key material is committed (the repo ignores `*.key`/`*.crt`). - [x] Consumer floor: two shutdown checks were added to the release-gate behavioral smoke. It runs on Node 22.13.0 against the packed artifacts, as the runtime-matrix spec requires. On `node:22.13.0`, `main` fails both (`stop()` took 5004 ms; the connection outlived the timeout); with this PR the smoke is 34/34. Local `pnpm release:gate`: PASS. - [x] End-to-end: the healthcheck suite with the old non-draining client helper (the Node 24.21 trigger) completes in 31 s on `node:24.21.0` via the force path, instead of hanging. - [x] `test:bun` (14 suites) and `test:esbuild` (226/226) for core. ## Parity coverage - [ ] Parity coverage added - [x] Parity N/A: server shutdown and transport connection lifecycle only. RPC semantics over HTTP and in-process are unchanged, and `scripts/parity-suite.sh` is unaffected. ## Related issues / changes - Follows #267, which fixed the Node 24.21 CI hang from the test side; this PR fixes the framework-side gap. - Companion docs PR: graceful-shutdown guide (options table, shutdown sequence, force-close section). - Out of scope, measured: - HTTP/1.1 keep-alive idling after the last in-flight response (bounded by `keepAliveTimeout` or the timeout); - Bun's `http.Server#close()` callback firing before connections close. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Server shutdown now sends GOAWAY to idle HTTP/2 clients when draining begins. * By default, connections still open when the shutdown timeout expires are closed across HTTP/1.1, HTTP/2, and TLS transports, including connections with unfinished requests. * With `forceCloseOnTimeout` set to `false`, shutdown completes after the timeout without forcibly closing open connections. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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
1 participant
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.
Summary
The Node 24 CI job hangs on every PR until GitHub's 6-hour limit.
node-version: "24"now resolves to v24.21.0, whosenode:http2client defersRST_STREAMwhile receiving (nodejs/node#65093, a v24-only adaptation of #64166). A stream closed withreq.close()from its'response'handler before its body is read is never destroyed, so the client keeps its TCP connection open. The healthcheck test helperhttpStatususes exactly that pattern. As a result, the server's gracefulstop()never completes and the integration suite never exits.The regression was reproduced on plain
node:http2without Connectum: 24.20.0 exits, 24.21.0 hangs, 26.10.0 exits. It is a client-side issue; the server's Node version is irrelevant.worker-components.test.tshttpStatusdrains the response body before closing the session.timeout-minutes: 15, and 30 for "Release & Publish". The slowest job over 224 successful runs took 116 s, so a hang now fails in minutes instead of hours.A related server-side reliability gap is not addressed here and will get its own PR: after
shutdown.timeout, Connectum destroys HTTP/2 sessions but not raw sockets, so the process does not exit while a client holds TCP.Type of change
Test plan
node:24.20.0,node:24.21.0andnode:26.10.0. Before the fix, 24.21.0 hung.pnpm build && pnpm typecheck && pnpm testpass locally (33/33 turbo tasks)pnpm lintpasses locally. The only warning,packages/events/src/broadcast.ts:18, pre-exists onmain.pnpm release:contractpasses (release.ymlgains onlytimeout-minutes)timeout-minutes, checked withyqover all 10 workflow files.Parity coverage
Related issues / changes
🤖 Generated with Claude Code
Summary by CodeRabbit