Fix marketplace fetch crash loop: consume bodyless responses and cancel the request timeout - #3962
Merged
SawyerHood merged 1 commit intoSep 21, 2026
Conversation
refreshMarketplaces arms each marketplace request with AbortSignal.timeout(10_000), which cannot be cancelled. When the manifest fetch returns a bodyless status (304 on a warm cache, 204, or 205), publicMarketplaceFetch returned `new Response(null)` without consuming the IncomingMessage, so the socket stayed bound to the request. About 10s later the timeout fired, Node destroyed that socket, and its unhandled 'error' exited the server; the launcher restarted it into the same 304, producing a ~12s crash loop on Node 26.9.0. - Consume bodyless responses so the request completes and the socket returns to the agent pool. - Own a cancellable timeout in publicMarketplaceFetch and clear it when the response settles, so no abort outlives the request. - Drop the caller-side AbortSignal.timeout from the four publicMarketplaceFetch call sites; the fetch owns the deadline now. - Add an injected-request regression test against a local server.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved blocking issues were identified.
Review effort: Lite
Findings: None
What changed in this PR
Fixes marketplace crash loops by draining bodyless responses and centralizing cancellable request timeouts.
Changes:
- Consumes 204/205/304 response streams.
- Centralizes timeout and abort handling.
- Removes redundant caller timeouts.
- Adds regression coverage.
| File | Description |
|---|---|
apps/server/test/services/plugin-catalog/marketplace-http.test.ts |
Adds bodyless-response regression coverage. |
apps/server/src/services/plugins/managed-plugin-artifacts.ts |
Removes redundant timeout handling. |
apps/server/src/services/plugin-catalog/marketplace-stats.ts |
Removes redundant timeout handling. |
apps/server/src/services/plugin-catalog/marketplace-source.ts |
Removes redundant timeout handling. |
apps/server/src/services/plugin-catalog/marketplace-icons.ts |
Removes redundant timeout handling. |
apps/server/src/services/plugin-catalog/marketplace-http.ts |
Implements response draining and cancellable timeouts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
What was wrong
refreshMarketplacesarms each marketplace request withsignal: AbortSignal.timeout(10_000), which cannot be cancelled. When the manifest fetch returns a bodyless status (304 on a warm cache, 204, or 205),publicMarketplaceFetchreturnednew Response(null)without consuming the underlyingIncomingMessage, so the socket stayed bound to the request. About 10s later the timeout fired, Node destroyed that socket, and its unhandled'error'exited the server; the launcher restarted it into the same 304, producing a ~12s crash loop on Node 26.9.0.The issue inferred a 200 from a bare
curl, butrequestManifestsendsif-none-matchwhenever a stored catalog has an etag, so every boot after the first one exercises the empty-body (304) path. Verified onmainatc1a64f4b4; see #3961 for the instrumentation.What changed
publicMarketplaceFetch(apps/server/src/services/plugin-catalog/marketplace-http.ts) consumes bodyless responses (incoming.resume()), so the request completes and the socket returns to the agent pool.AbortController+setTimeout, cleared when the response closes) instead of forwarding an uncancellableAbortSignal.timeout. A caller-providedinit.signalis still honored throughAbortSignal.any.signal: AbortSignal.timeout(MARKETPLACE_FETCH_TIMEOUT_MS)(marketplace-source.ts,marketplace-icons.ts,marketplace-stats.ts,managed-plugin-artifacts.ts) no longer pass their own signal;publicMarketplaceFetchowns the deadline for its callers.plugin-updates.tskeeps its own timeout because it uses globalfetch.createPublicMarketplaceFetch({ request, lookup })with a production-injectedpublicMarketplaceFetch, so the request path can be exercised against a local server without weakening the public-address policy.apps/server/test/services/plugin-catalog/marketplace-http.test.ts.No
HOST_DAEMON_PROTOCOL_VERSIONchange and no CLI/config/wire changes.How you verified
settles a bodyless response so a late timeout cannot abort the socketfails before the fix (the socket receivesAbortErroronce the caller timeout fires) and passes after; confirmed by temporarily removing only theincoming.resume()line.pnpm exec turbo run test --filter=@bb/server -- test/services/plugin-catalog/marketplace-http.test.ts→ 21 passed.pnpm exec turbo run typecheck --filter=@bb/server→ passed.pnpm exec turbo run test --filter=@bb/server -- test/services/plugin-catalog→ 148 passed. The one failure (third-party-marketplaces.test.ts"resolves a third-party git range to its current tag and commit", a 5s git timeout) reproduces on basec1a64f4b4with this change stashed.c1a64f4b4with anAbortSignal.timeoutprobe: with the old bodyless handling the reported stack appears (stream.<computed>.AbortError.cause (node:internal/streams/add-abort-signal:47:22)); with the fix the timeout fires with no unhandled error and the process stays up.Fixes #3961