Conversation
lightspeed-core bounds concurrent file uploads and vector store attaches with
per-endpoint semaphores (max_concurrent_file_uploads /
max_concurrent_vector_store_attaches) and rejects excess requests with 429 Too
Many Requests rather than queuing them. Bursty notebook uploads therefore
failed intermittently.
Add a fetchWithRetry helper in VectorStoresOperator that retries on 429,
honoring the Retry-After header when present and otherwise backing off
exponentially (capped, up to 8 attempts). Use it for the file upload
(POST /v1/files) and vector store attach (POST /v1/vector-stores/{id}/files)
calls, the two endpoints subject to those limits.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Changed Packages
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4786 +/- ##
==========================================
- Coverage 63.60% 63.13% -0.47%
==========================================
Files 2685 2655 -30
Lines 107077 105862 -1215
Branches 30010 29646 -364
==========================================
- Hits 68101 66841 -1260
- Misses 37165 37212 +47
+ Partials 1811 1809 -2
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 2:40 PM UTC · Completed 2:56 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Cost: $2.88 |
ReviewFindingsMedium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
the
requires-manual-review
Jdubrick
left a comment
Jdubrick
left a comment
There was a problem hiding this comment.
Can we add some tests for this as well? Simulating the 429
| let attempt = 0; | ||
| for (;;) { |
There was a problem hiding this comment.
Why not have a traditional for loop over attempt instead of infinitely loop and do extra work?
There was a problem hiding this comment.
Good call. Reworked it into a traditional for (let attempt = 0; attempt <= maxRetries; attempt++) loop that returns as soon as the request succeeds or is non-429, so there's no infinite loop or extra work. Done in 5c3faf5.
Add a VectorStoresOperator.test.ts suite exercising fetchWithRetry through the public vector store attach method: first-try success, retry-then-succeed, Retry-After precedence, exponential fallback, jitter, the 5s cap, retry exhaustion, and non-429 responses not being retried. Fetch is mocked and fake timers skip the real backoff sleeps. Also harden fetchWithRetry: - Cancel the 429 response body before backing off so the undici socket returns to the connection pool immediately instead of waiting for GC, which otherwise compounds across retries under the bursty concurrent uploads this targets. - Add random jitter to the exponential backoff so several requests rejected at once don't recompute the same delay and retry in lockstep. Assisted-by: Claude Opus 4.8 Co-Authored-By: Claude Opus 4.8 <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Replace the `for (;;)` retry loop in fetchWithRetry with a bounded `for (let attempt = 0; attempt <= maxRetries; attempt++)` so the iteration limit is visible in the loop header rather than relying solely on the in-body guard. Behaviour is unchanged: same backoff delays, same number of attempts, same log text. A trailing throw after the loop is unreachable and present only to satisfy the Promise<Response> return type. Assisted-by: Claude Opus 4.8 Co-Authored-By: Claude Opus 4.8 <[email protected]>
…n in 429 test The "does not retry a non-429 error response" test used `await expect(attach()).rejects.toThrow()`. That matcher formats the rejected error's stack, which source-map-support remaps by quick-sorting the compiled bundle's mappings. Under CI's tighter stack limit that quick-sort recurses past the maximum call stack and the test fails with `RangeError: Maximum call stack size exceeded`, even though the code under test is fine (the test passes locally). Capture the rejection with `.catch(err => err)` and assert `toBeInstanceOf(Error)` instead, matching the existing pattern in the "gives up after maxRetries" test. This asserts the same behavior without forcing stack symbolication. Signed-off-by: Lucas <[email protected]> Assisted-by: Claude Opus 4.8 Co-Authored-By: Claude Opus 4.8 <[email protected]>
|
3 participants
Description
lightspeed-core bounds concurrent file uploads and vector store attaches with per-endpoint semaphores (
max_concurrent_file_uploads/max_concurrent_vector_store_attaches) and rejects excess requests with429 Too Many Requestsrather than queuing them. Bursty notebook uploads therefore failed intermittently — observed as repeated:Changes
fetchWithRetryhelper inVectorStoresOperatorthat retries on429, honoring theRetry-Afterheader when present and otherwise backing off exponentially (capped, up to 8 attempts).POST /v1/files) and vector store attach (POST /v1/vector-stores/{id}/files). Other calls (GET/PUT/DELETE) are unchanged.Testing
Verified against a live lightspeed-core deployment that was returning 429s on concurrent attaches: with the retry in place, bursty uploads complete instead of failing.
🤖 Generated with Claude Code