Fix BadResource errors in subprocess HTTP/2 cleanup#82
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes BadResource errors occurring during subprocess HTTP/2 cleanup by implementing proper resource lifecycle management and error suppression. The fix involves making IPC connection cleanup async to flush pending writes, reordering cleanup operations to wait for subprocess exit before closing connections, and adding an unhandledrejection handler to suppress unavoidable node:http2 cleanup errors.
Key changes:
- Made
IpcConnection.close()andcloseIpc()async to properly await writer flushing - Reordered cleanup in parent process to wait for subprocess exit before closing IPC
- Added unhandledrejection handler in run.ts subprocess to suppress node:http2 BadResource errors
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/cli/commands/run.ts | Reordered cleanup to await subprocess exit before closing IPC connection |
| src/cli/commands/list.ts | Reordered cleanup to await subprocess exit before closing IPC connection |
| src/cli/_templates/utils.ts | Made IpcConnection.close() and closeIpc() async to properly flush pending writes |
| src/cli/_templates/run.ts | Added unhandledrejection handler for node:http2 errors and updated closeIpc() call to await |
| src/cli/_templates/list.ts | Updated closeIpc() call to await the async operation |
Critical Issues Found:
There are two critical bugs where the async ipc.close() method is not being awaited in the parent process cleanup code. This defeats the purpose of making the close operation async, as pending writes may not be properly flushed before cleanup continues. The subprocess templates correctly await the close operation, but the parent process files (run.ts and list.ts in commands/) need to be updated to also await this call.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The subprocess IPC connection was being closed before the subprocess finished writing, causing "Bad resource ID" errors when the subprocess tried to write to an already-closed TCP stream. This also caused benign unhandled rejections from Deno's node:http2 compatibility layer during HTTP/2 stream cleanup. Changes: - Made closeIpc() and IpcConnection.close() async to ensure writer is properly flushed before closing the underlying TCP connection - Reordered cleanup sequence in run/list commands: wait for subprocess exit first, then close IPC, then close listener - Added unhandled rejection handler in subprocess to gracefully suppress benign "Bad resource ID" errors from node:http2 cleanup This ensures all pending IPC writes complete before resource cleanup.
6ab6d0c to
68b0807
Compare
Summary
Why
After migrating from Worker to subprocess execution, HTTP/2-based scenarios
(HTTP client, GraphQL, etc.) were failing with "BadResource: Bad resource ID"
errors from node:http2. These errors occurred during HTTP/2 stream cleanup when
the subprocess terminated.
The root causes were:
subprocess finished writing results
leaving pending writes in limbo
This PR implements a two-layer solution:
reorder cleanup to ensure subprocess completes before IPC closes
matching the Worker-based approach, as HTTP/2 cleanup errors don't affect
test correctness
The combination ensures both correct resource lifecycle management and graceful
handling of unavoidable node:http2 cleanup errors.
Test Plan
deno task verify- all tests pass