Repository navigation
fix(cli): give up on a request that stops making progress - #2186
Merged
Merged
Conversation
Found while auditing #2185. A server that accepts the connection and then says nothing left the command open indefinitely: [no response] outcome=NEVER SETTLED There was no timeout anywhere in the CLI's HTTP layer, so this applied to every request rather than only to downloads - `get`, `verify`, `publish` and the OIDC token exchange alike. Node's `timeout` option alone does not fix it: it raises an event and leaves the request open, which is presumably how this went unnoticed. Confirmed: timeout event fired; is the request still live? destroyed = false So each request destroys itself with an error when the event arrives, which routes through the error handling every call site already has - including download's, which removes the partial file on the way out. Inactivity rather than a deadline, since any byte read or written resets the socket timer: a large extension downloading slowly over a poor connection is not cut off, while a connection that has gone quiet does not hold the command open. There is a test for each half, the second being the one that would fail if this were ever changed to a total-duration limit. Thirty seconds by default, `OVSX_TIMEOUT` in milliseconds to change it and zero to disable. The OIDC token service is not the registry, so it does not read the registry's option and has its own constant. Also swaps the deprecated `request.abort()` in postFile for `destroy()`, the last one left in the file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Timeout errors can leak PAT credentials through request URLs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds configurable inactivity timeouts to prevent stalled CLI HTTP requests from hanging indefinitely.
Changes:
- Applies timeouts to registry and OIDC requests.
- Adds stalled and slow-progress download tests.
- Documents
OVSX_TIMEOUTand replaces deprecatedabort()usage.
Review comments:
- Critical —
cli/src/registry.ts:330: Timeout errors expose full URLs, potentially leaking PAT query parameters. Strip user information and query strings. - Nit —
cli/src/oidc.ts:91: Describe the error as inactivity rather than “No response,” since partial responses may have arrived.
File summaries
| File | Description |
|---|---|
cli/test/unit/registry-download.spec.ts |
Tests stalled and progressing downloads. |
cli/src/util.ts |
Parses OVSX_TIMEOUT. |
cli/src/registry.ts |
Implements registry request timeouts. |
cli/src/registry-options.ts |
Exposes timeout configuration. |
cli/src/oidc.ts |
Adds an OIDC request timeout. |
cli/README.md |
Documents timeout configuration. |
cli/CHANGELOG.md |
Records the timeout fix. |
Review details
Suppressed comments (1)
cli/src/oidc.ts:91
- This timeout can also fire after headers or partial body chunks have arrived, so “No response” is inaccurate for a stalled response body. Describe the actual inactivity condition instead.
request.destroy(new Error(`No response from ${url} for ${TOKEN_SERVICE_TIMEOUT} ms.`));
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review feedback, and it was already true of a message on main before this
branch added another. createNamespace, verifyPat, publish and delete all put
the personal access token in a `token` query parameter, and handleError writes
an error's message straight to stderr - and so into CI logs:
No response from http://.../api/some-namespace/verify-pat?token=super-secret-pat-value for 200 ms.
Three messages interpolated a URL: this branch's timeout message in registry
and oidc, and `The connection closed before the whole of ${url} was received`,
which merged with #2185. That last one is reachable with any URL a caller hands
to download, so all three are redacted rather than only the reported one.
redactUrl keeps scheme, host and path and drops the query whole rather than
filtering it key by key, since a filter has to be kept in step with every
parameter anyone adds later. User info goes too, being another place a
credential can sit.
The test drives it through verifyPat rather than asserting on the helper, so it
covers the path a real command takes, and asserts the message contains neither
the token nor a '?' at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical response-error handling gaps and missing partial-response timeout coverage must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
cli/test/unit/registry-download.spec.ts:161
- The timeout coverage never verifies the key failure mode after a response has started. A client that times out only while waiting for response headers would pass both the silent-server test and this slow-success test, yet still hang forever if a download sends one chunk and then stalls. Add a partial-response test that waits beyond the inactivity timeout and asserts rejection plus
.partcleanup.
cli/CHANGELOG.md:21
OVSX_TIMEOUTdoes not override the timeout for all requests: the OIDC token exchange added in this PR always uses its separate 30-second constant. Qualify this changelog entry as registry requests to match the implemented configuration semantics.
- Requests now give up after 30 seconds without progress instead of hanging indefinitely when a server accepts a connection and then says nothing. `OVSX_TIMEOUT` overrides the duration in milliseconds and `OVSX_TIMEOUT=0` disables it; it measures inactivity, so a large extension downloading slowly is unaffected ([#2186](https://github.com/eclipse-openvsx/openvsx/pull/2186))
cli/README.md:13
- This wording makes
OVSX_TIMEOUT=0sound as though it disables every CLI request timeout, but the OIDC token-service request always usesTOKEN_SERVICE_TIMEOUTand ignores this setting. Scope the documented behavior to registry requests so users relying on OIDC are not given an incorrect configuration guarantee.
Requests give up if they stop making progress for 30 seconds, so a registry that accepts a connection and then goes quiet does not leave the command hanging. `OVSX_TIMEOUT` changes that, in milliseconds; `OVSX_TIMEOUT=0` disables it. It measures inactivity rather than total time, so a large extension downloading slowly is not cut off.
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
Review feedback. The JSON paths attached no error handler to the response,
unlike download, where pipeline installs one. Both getJsonResponse and the OIDC
token exchange are affected, so getJson, post, postFile and getIdToken all are.
The reported symptom does not occur: destroying the request does not produce an
uncaught ECONNRESET, because the request's own error is what settles the
promise and the response's error goes nowhere. Verified with listeners on
uncaughtException and unhandledRejection - a timeout mid-body rejects cleanly
with the timeout message and nothing escapes.
What does happen is a hang, which is worse for being quiet. With the timeout
switched off, a server that starts answering and then loses the connection
never fires 'end' on the response and never rejects:
[socket killed mid-json, no timeout] outcome=never settled uncaught=[]
The timeout added earlier on this branch bounds that to thirty seconds, but it
reports "no response" for a connection that answered and then broke, which is
not what happened. The response's error now settles the promise, so it fails
immediately as ECONNRESET:
[socket killed mid-json] outcome=rejected: ECONNRESET
A timeout firing after part of the body has arrived still reports the timeout,
since the request's error wins that race. There is a test for each, so neither
message can quietly become the other, and one in oidc.spec.ts for the token
service dying mid-answer.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The OIDC inactivity-timeout path needs a regression test for a connection that remains silent indefinitely.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
…n one clock Review feedback, and the gap was real: removing the OIDC timeout handler left the whole suite green. The test I had added there destroys its socket after a partial response, which the response's error handler settles either way, so it never exercised the timeout. The OIDC request is a second HTTP implementation, so the registry's timeout tests cannot cover it - a silent token service would have hung trusted publishing with nothing to notice. Testing it needed the duration to be reachable, and a hardcoded thirty seconds is not. Rather than inject a timeout only tests would use, the token exchange now reads OVSX_TIMEOUT like every other request: one knob to document, and a CI system slow enough to need a longer registry timeout probably needs a longer one here too. The constant and the environment read move to util, which both modules already import; DEFAULT_TIMEOUT is still exported from registry, beside the other defaults. The new test stands up a service that accepts the connection and says nothing, with OVSX_TIMEOUT at 200ms. It fails with the timeout handler removed, and so does the mid-response test with the response handler removed - so the two failure modes are covered separately rather than one masking the other. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Download timeout failures can race, causing duplicate cleanup and surfacing the wrong error.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
…hat is reported
Review feedback, and the race is real and frequent. A timeout firing once the
body has started drives both the request's error handler and pipeline's
callback. Each called fail, which removed the partial file before rejecting, so
whichever fs.rm finished first decided what the caller was told - and the
cleanup ran twice. Over 36 stalled downloads:
{"timeout message":7,"ECONNRESET":5}
{"timeout message":9,"ECONNRESET":3}
{"timeout message":8,"ECONNRESET":4}
A third of them reported a connection reset for a server that had answered and
then gone quiet.
fail now claims the failure synchronously, before the cleanup that has to
precede the rejection, so the first error settles the operation and the partial
is removed once. That is the fix: the new test fails without it and passes with
it, across eight stalled downloads per run.
The timeout also reports itself before tearing the request down, rather than
destroying the request with the error and letting the resulting stream errors
compete. Worth being straight about: with the claim in place, destroying with
the error passed 56 stalled downloads without once reporting the wrong one, so
this is belt and braces against an ordering node does not promise rather than
something demonstrably needed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
Perspective on this PR, since it has taken a lot of roundsSix review rounds across #2185 and this one, all inside the same twenty lines. That looks like flailing from the outside, so it is worth setting out what each round actually was. Eight distinct defects, in the order they were found:
The pattern is the useful part: every one of these is a promise that fails to settle, or settles with the wrong thing. Not one is a logic error in the download itself. The original implementation wired up the success path and left every failure edge to chance - no error listener on the response, no timeout, resolution on the wrong event, cleanup racing itself. Each fix exposed the next edge rather than breaking anything a previous fix had established, which is why the rounds kept coming and also why none of them was a regression. Two of the rounds were my fault in a way worth naming, because both were tests that looked like coverage and were not:
Given that, the thing I would want a reviewer to weigh is not the diff but the coverage. Each failure mode now has a test that fails on its own: I removed each handler in turn - the timeout handler, the request error listener, the response error listener, the synchronous claim - and confirmed exactly one test fails each time. Where a failure is a race rather than a certainty, the test repeats it (eight stalled downloads, fifty small ones) rather than passing on one lucky attempt. Two things I deliberately left out, both noted in the description rather than fixed here:
|
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.
Follow-up to #2185. While auditing that after it merged, I checked what happens when a server accepts the connection and then says nothing:
ovsx getsits there forever. There is no timeout anywhere in the CLI's HTTP layer, so this is not specific to downloads —getJson,post,postFileand the OIDC token exchange are all the same.Node's
timeoutoption is not enough on its ownWhich is presumably why this went unnoticed: setting
timeoutlooks like it should do the job, but it only raises an event and leaves the request open.So each request destroys itself with an error when the event arrives. That routes through the error handling every call site already has, including
download's, which removes the.partfile on the way out — no new failure path to get wrong.Inactivity, not a deadline
The socket timer is reset by any byte read or written, which is the behaviour we want: a 200MB extension crawling over a bad connection must not be killed, while a connection that has gone silent must not hold the command open.
Two tests, one for each half:
rejects when the server accepts the connection and never respondsdoes not time out a download that is slow but still progressing— five chunks 120ms apart against a 300ms timeout, so 600ms in total, twice the timeout, but never 300ms without a byteThe second is the one that would fail if this were ever changed to a total-duration limit, which is why it is there rather than only the obvious one.
Configuration
Thirty seconds by default.
OVSX_TIMEOUTsets it in milliseconds,OVSX_TIMEOUT=0disables it, and it is also aRegistryOptionsfield for library callers. A mistyped value falls back to the default rather than failing the command.The OIDC token service is not the registry — it deliberately carries no registry credentials — so it does not read the registry's option and has its own constant.
Also
postFilestill used the deprecatedrequest.abort(); that is nowdestroy(), the last one in the file.Verified
main, by timing outOne thing left from the same audit
The nine scenarios I ran against #2185's merged code all behave correctly — large bodies, rename over an existing target, 404 and mid-body drops with a pre-existing file, chunked truncation, connection refused, redirects. The one other weakness is that
<file>.partis a fixed name, so two concurrent downloads to the same target share it and one fails with a spuriousENOENTon rename. It does not arise today (getis one download per process,verifyuses two distinct temp paths) and the previous code had the same hazard writing straight to the target, so I have left it out of this PR. Happy to close it with a unique suffix if you would rather not leave it.