Repository navigation
fix(lab): preserve CL-03 live transport failure classes #1710
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
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
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
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,117 @@ | ||
| import { createServer, type IncomingMessage, type Server, type ServerResponse } from "node:http"; | ||
| import { afterEach, describe, expect, test } from "bun:test"; | ||
| import { createLabAuthorizedPinnedSender } from "../src/lib/lab-live-pinned-sender"; | ||
| import type { LabCredentialLeaseV1, LabDestinationV1, LiveRunConfig } from "../src/lab/live/types"; | ||
|
|
||
| const SERVERS: Server[] = []; | ||
|
|
||
| afterEach(async () => { | ||
| for (const server of SERVERS.splice(0)) { | ||
| server.closeAllConnections?.(); | ||
| await new Promise<void>((resolve) => server.close(() => resolve())); | ||
| } | ||
| }); | ||
|
|
||
| async function listen(handler: (req: IncomingMessage, res: ServerResponse) => void): Promise<number> { | ||
| return await new Promise<number>((resolve, reject) => { | ||
| const server = createServer(handler); | ||
| SERVERS.push(server); | ||
| const onError = (error: Error) => reject(error); | ||
| server.once("error", onError); | ||
| server.listen(0, "127.0.0.1", () => { | ||
| server.off("error", onError); | ||
| const address = server.address(); | ||
| if (!address || typeof address === "string") { | ||
| reject(new Error("loopback test server did not expose a TCP port")); | ||
| return; | ||
| } | ||
| resolve(address.port); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| const BASE_LIMITS: LiveRunConfig = { | ||
| totalTimeoutMs: 1_000, | ||
| connectTimeoutMs: 250, | ||
| firstByteTimeoutMs: 30, | ||
| inactivityTimeoutMs: 30, | ||
| maxRequests: 2, | ||
| maxInputBytes: 1024, | ||
| maxOutputBytes: 1024, | ||
| maxOutputTokens: 1024, | ||
| maxToolCalls: 8, | ||
| maxMemoryBytes: 64 * 1024 * 1024, | ||
| maxChildProcesses: 0, | ||
| maxArtifacts: 4, | ||
| perArtifactBytes: 64 * 1024, | ||
| aggregateArtifactBytes: 256 * 1024, | ||
| }; | ||
|
|
||
| function destination(port: number): LabDestinationV1 { | ||
| return { | ||
| scheme: "http", | ||
| host: "lab-timeout.invalid", | ||
| port, | ||
| basePath: "", | ||
| sniHost: "lab-timeout.invalid", | ||
| addresses: [{ address: "127.0.0.1", family: 4 }], | ||
| privateNetwork: true, | ||
| fingerprint: "a".repeat(64), | ||
| }; | ||
| } | ||
|
|
||
| async function send(port: number, limitOverrides: Partial<LiveRunConfig> = {}) { | ||
| const sender = createLabAuthorizedPinnedSender(() => ({})); | ||
| return await sender( | ||
| {} as LabCredentialLeaseV1, | ||
| destination(port), | ||
| { address: "127.0.0.1", family: 4 }, | ||
| { method: "POST", path: "/", body: "{}" }, | ||
| new AbortController().signal, | ||
| { ...BASE_LIMITS, ...limitOverrides }, | ||
| ); | ||
| } | ||
|
|
||
| describe("CL-03 pinned live transport failure classification", () => { | ||
| test("preserves first-byte timeout as a transport timeout", async () => { | ||
| const port = await listen((_req, res) => { | ||
| setTimeout(() => { | ||
| if (res.destroyed) return; | ||
| res.writeHead(200, { "content-type": "application/json" }); | ||
| res.end("{}"); | ||
| }, 150); | ||
| }); | ||
|
|
||
| await expect(send(port, { firstByteTimeoutMs: 30, inactivityTimeoutMs: 250 })).rejects.toMatchObject({ | ||
| name: "TransportError", | ||
| code: "first_byte_timeout", | ||
| }); | ||
| }); | ||
|
|
||
| test("preserves response inactivity as inactivity_timeout", async () => { | ||
| const port = await listen((_req, res) => { | ||
| res.writeHead(200, { "content-type": "application/json" }); | ||
| res.write("{\"ok\":"); | ||
| setTimeout(() => { | ||
| if (!res.destroyed) res.end("true}"); | ||
| }, 150); | ||
| }); | ||
|
|
||
| await expect(send(port, { firstByteTimeoutMs: 250, inactivityTimeoutMs: 30 })).rejects.toMatchObject({ | ||
|
Check failure on line 100 in tests/lab-live-pinned-timeouts.test.ts
|
||
| name: "TransportError", | ||
| code: "inactivity_timeout", | ||
| }); | ||
| }); | ||
|
|
||
| test("preserves the output byte ceiling as output_byte_limit", async () => { | ||
| const port = await listen((_req, res) => { | ||
| res.writeHead(200, { "content-type": "text/plain" }); | ||
| res.end("x".repeat(128)); | ||
| }); | ||
|
|
||
| await expect(send(port, { maxOutputBytes: 16 })).rejects.toMatchObject({ | ||
|
Check failure on line 112 in tests/lab-live-pinned-timeouts.test.ts
|
||
| name: "TransportError", | ||
| code: "output_byte_limit", | ||
| }); | ||
| }); | ||
| }); | ||
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.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 28880
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 50376
🌐 Web query:
Node.js http IncomingMessage setTimeout callback socket timeout listener removal keep-alive official documentation💡 Result:
In Node.js, the behavior of
message.setTimeout(msecs, callback)on anhttp.IncomingMessageinstance has historically been a subject of community confusion regarding callback execution [1][2]. According to official documentation and technical context, when you callmessage.setTimeout(msecs, callback), thecallbackis intended to be invoked when a 'timeout' event occurs [3][4]. However, historically, developers have reported that the callback provided tomessage.setTimeoutmight not be triggered as expected because theIncomingMessageobject itself did not reliably emit the 'timeout' event in certain versions [2]. ThesetTimeoutmethod on anIncomingMessagetypically delegates to the underlying socket'ssetTimeoutmethod [3]. To ensure a timeout listener is handled correctly, it is standard practice to bind directly to the socket or monitor the 'timeout' event on the request/response object explicitly, while keeping in mind thatsetTimeoutonly notifies that the socket has been idle and does not automatically abort the request [5][6]. Regarding listener removal and keep-alive: 1. Listener Removal: Node.js does not automatically remove timeout listeners attached viamessage.setTimeoutwhen a socket is reused for keep-alive connections. If you attach listeners manually, you must manage their lifecycle (e.g., usingremoveListeneroronce) to prevent memory leaks or unexpected behavior during socket reuse. 2. Keep-Alive Interaction: Socket timeouts are heavily influenced by the server'skeepAliveTimeoutconfiguration [7][8]. When using keep-alive, Node.js manages idle sockets to allow for reuse. If a socket is idle and exceeds thekeepAliveTimeout, the server will close it [7]. Developers should prefer usingserver.keepAliveTimeoutto control idle connection duration rather than relying on manual socket timeouts, as the latter can be preempted or overridden by internal server logic during the keep-alive lifecycle [7][8]. For robust timeout management, rely on the server-level configuration options such asheadersTimeout,requestTimeout, andkeepAliveTimeoutprovided in thehttp.createServer()orhttp.request()options [9][8].Citations:
headersTimeoutandrequestTimeoutnot restarted on keep-alive HTTP connections nodejs/node#58140🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 20816
🌐 Web query:
site:nodejs.org/api/http.html IncomingMessage setTimeout socket timeout callback💡 Result:
In Node.js, the http.IncomingMessage class does not possess a setTimeout method [1]. The setTimeout method is a feature of http.ClientRequest (for outgoing requests) or part of the underlying socket handling for server-side connections [1]. When you are working with an http.IncomingMessage object (which represents the response in a client request or the request in a server listener), you can monitor inactivity by accessing the underlying socket directly [1]. Key points regarding timeout handling: 1. Timeout Event: The http.IncomingMessage object inherits from stream.Readable and emits a 'timeout' event when the underlying socket times out due to inactivity [1]. 2. Manual Destruction: Receiving the 'timeout' event only signals that the socket has been idle; it does not automatically close or destroy the stream. You must manually call request.destroy or response.destroy to terminate the connection [1]. 3. Socket Access: If you need to set a timeout on the underlying socket for an incoming message, you can generally access the socket via the message.socket property (e.g., message.socket.setTimeout(msecs, callback)) [1]. If you are dealing with a client-side request, use request.setTimeout(timeout, callback) on the http.ClientRequest object before the request is sent to manage timeouts effectively [1].
Citations:
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 2126
🌐 Web query:
site:nodejs.org/api/http.html "message.setTimeout(msecs" "IncomingMessage"💡 Result:
In Node.js, there is no direct method named message.setTimeout(msecs) for the http.IncomingMessage class [1]. The http.IncomingMessage object, which represents the incoming message in an HTTP server or the response in an HTTP client request, does not provide a built-in timeout method [1]. If you are looking to manage timeouts for HTTP connections in Node.js, consider these alternatives: 1. Server-level Timeouts: For an HTTP server, you can set a timeout using server.setTimeout([msecs][, callback]), which applies to all sockets connected to that server [1]. 2. Request-level Timeouts: For client-side requests, you use request.setTimeout(timeout[, callback]) on the http.ClientRequest object [1]. 3. Socket-level Timeouts: Since http.IncomingMessage is associated with an underlying socket, you can access that socket via message.socket and call socket.setTimeout(msecs) to manage inactivity timeouts directly on the connection [1]. Note that when a socket times out, the 'timeout' event is emitted, but the connection is not automatically destroyed; you must manually call request.destroy or socket.destroy to terminate the connection [1].
Citations:
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 184
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 271
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 23058
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 4292
🌐 Web query:
Bun node:http IncomingMessage setTimeout socket timeout compatibility documentation💡 Result:
In Bun, the node:http IncomingMessage.setTimeout method is implemented to provide compatibility with Node.js [1][2]. This method delegates the timeout logic to the underlying socket (or the request's native handle), maintaining behavioral parity with Node.js by invoking the socket's timeout mechanism [1][3]. Key details regarding Bun's implementation include: Mechanism: Calling message.setTimeout(msecs, callback) essentially maps to the underlying socket's timeout functionality [1][3]. In Bun's node:http module, this is wired to native per-connection inactivity timers [3][4]. When the specified duration (msecs) passes without activity, a timeout event is emitted [3][4]. Compatibility Progress: - Bun has actively iterated to ensure these methods behave like Node.js, including fixes to properly unref timers so they do not unnecessarily keep the event loop alive [5][4]. - Recent updates have ensured that server-side timeout listeners (on the server, request, or response) can correctly veto the default socket destruction behavior, matching the established Node.js contract [4]. - Improvements have also been made to ensure timeouts correctly fire during the connection phase (e.g., against dead or firewalled hosts), addressing past gaps in timeout event emission [6]. While Bun aims for high compatibility, users should be aware that implementation details for networking primitives are subject to ongoing refinement as part of the broader effort to support the full Node.js API surface [7][8]. If you encounter specific issues with socket timeout behavior, ensure your Bun version is up to date, as many historical limitations related to socket.setTimeout stubs or silent failures have been resolved in recent releases [3][9][10].
Citations:
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 1726
Reset the socket timeout when the response stream terminates.
response.setTimeout()atsrc/lib/pinned-http.ts:152attaches its callback toIncomingMessage, not toresponse.socket. Remove the proposedsocket.off()cleanup. Capture the socket before the"end"handler runs, then callsocket.setTimeout(0)on"end","error", output-limit failure, and cancellation. The current code leaves a one-shot timeout armed on pooled keep-alive sockets.🤖 Prompt for AI Agents
Source: Path instructions