fix(fetch): a hard deadline that holds even when undici drops the abort (PHARN-09) - #203
Merged
Merged
Conversation
…rt (PHARN-09) Every fetch used an abort-only timer: setTimeout -> controller.abort(). On Node 20/22 (undici 6) the caller's signal is held through a WeakRef, and after one full GC the abort no longer reaches the response body: with a server dripping a byte every 250 ms, a 3 s-capped read was still running at 10 s (reproduced on 22.22.2 and 20.20.2; Node 24 unaffected). undici's bodyTimeout measures the gap between chunks, so init/add/update/status could hang indefinitely — add/update while holding the project lock. New lib/deadline.ts `withDeadline` races the whole fetch-and-read against a timer that aborts AND rejects on its own; downloadArchive, fetchCommitSha and fetchRemoteSkillsVersion use it, and each body reader is cancelled from our own abort listener so the socket is released too. With the same GC repro the new shape rejects at 3.0 s. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TvcuVhk8hTeDskp5pAJhnc
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
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 this changes
Every network request used the same timeout: a
setTimeoutthat callscontroller.abort(). On Node 20 and 22, the bundled HTTP client (undici 6) keeps only a weak reference to that abort signal. After a full garbage collection, the abort no longer reaches the response body. Reproduced on Node 22.22.2 and 20.20.2 with a server that sends one byte every 250 ms and a 3 s cap: after one forcedgc(), the read was still running at 10 s. Node 24 is not affected, which is why CI (Node 24 only) never showed it. undici's ownbodyTimeoutonly measures the gap between chunks, so a slow trickle never triggers it. As a resultinit,add,updateandstatuscould hang indefinitely, andadd/updatewould hold the project lock the whole time.The fix:
src/lib/deadline.ts(withDeadline): runs the whole request, including reading the body, against a timer. When the timer fires it aborts the request and fails the call itself, so control comes back at the deadline whether or not the abort reaches the body stream.downloadArchive, which downloads the pharn-oss tarball (60 s limit);fetchCommitSha, which looks up the upstream commit (8 s limit, and still returnsnullon failure); andfetchRemoteSkillsVersion, which fetchesSKILLS_VERSION(8 s limit). Timeout errors keep the existing "Could not reach " wording.Evidence, using the review's original repro script (one
gc()after 1 s):Built with
/pharn-dev-ship; stage artifacts are in.dev/features/fetch-hard-deadline/. Results:no-regressionsPASSType of change
feat— new stack option, wizard step, or command capabilityfix— bug fixdocs— docs-only changechore/refactor— tooling or internal restructure, no behavior changeArea(s) touched
lib/deadline (new) | lib/repo | lib/skills-version
Checklist
.js-extension import convention.tests/deadline.test.ts(4 cases), plus one test per request type in which the body never responds to the abort. All three of those hang on the old code.repo.test.tsandrepo-signals.test.ts: the fake download body is now a real webReadableStream, the shapefetchactually returns.redirect: 'error', the byte caps and the timeout values are unchanged.Quality gates
npm run checkpasses locally (1345/1345; non-root user, node 22).npm run build/npm run test:coverage(left to CI).🤖 Generated with Claude Code
https://claude.ai/code/session_01TvcuVhk8hTeDskp5pAJhnc
Generated by Claude Code