fix(client-typescript): implement documented maxConcurrent for sendFiles() (#1381)#1382
fix(client-typescript): implement documented maxConcurrent for sendFiles() (#1381)#1382anshvermadev wants to merge 12 commits into
Conversation
sendFiles() advertised a maxConcurrent parameter (default 5) in its JSDoc and example, but the signature accepted only (files, token) and the implementation opened one data pipe per file with no cap. Large batches could exhaust pipes/sockets/memory, and the documented 3-argument call failed to type-check (TS2554). Add the maxConcurrent = 5 parameter and run uploads through a bounded worker pool that keeps at most maxConcurrent uploads in flight. Results are still written by index, so output order is preserved. Closes rocketride-org#1381
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe TypeScript client now caps concurrent file uploads through a maxConcurrent argument, and the CLI forwards its concurrency option into that upload path. ChangessendFiles concurrency fix
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant sendFiles
participant Worker
participant uploadFile
CLI->>sendFiles: sendFiles(files, token, maxConcurrent)
sendFiles->>sendFiles: start bounded workers
loop until next file index is exhausted
sendFiles->>Worker: assign next file index
Worker->>uploadFile: uploadFile(fileData, index)
uploadFile-->>Worker: result
Worker->>sendFiles: store results[index]
end
sendFiles-->>CLI: resolve with ordered results
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/client-typescript/src/client/client.ts (1)
1347-1355: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPublic SDK signature change missing docs update.
sendFilesnow takes a new public parametermaxConcurrent, but the linked docs snippet (packages/client-typescript/docs/guide/index.md) still shows the old(files, token)signature withoutmaxConcurrent.As per path instructions,
packages/client-typescript/src/**/*.ts: "Update public TypeScript SDK signatures inpackages/client-typescript/docs/when changing the signature in code." Also per coding guidelines,packages/client-typescript/src/**/*.{ts,tsx}: "Public TypeScript SDK signature changes must be documented inpackages/client-typescript/docs/with reference documentation auto-generated byclient-typescript:docs-generate."Please regenerate/update the docs (e.g. run
client-typescript:docs-generate) to reflect the newmaxConcurrentparameter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/client-typescript/src/client/client.ts` around lines 1347 - 1355, The public `sendFiles` signature in `client.ts` now includes the new `maxConcurrent` parameter, but the generated docs still show the old `files, token` form. Update the TypeScript SDK reference docs under `packages/client-typescript/docs/` by regenerating them (for example via `client-typescript:docs-generate`) so the `sendFiles` documentation matches the current signature and includes `maxConcurrent`.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/client-typescript/src/client/client.ts`:
- Around line 1466-1486: The bounded upload worker in client.ts’s sendFiles path
does not validate maxConcurrent before using it, so NaN or other non-finite
values can turn the pool length into zero and silently skip all uploads. Add an
explicit sanity check near the limit calculation in sendFiles (and/or before
Promise.all) to normalize only finite positive integers, or throw a clear error
when maxConcurrent is invalid, so uploadFile still runs for every file or fails
fast instead of returning empty results.
- Around line 1466-1486: Forward the parsed --max-concurrent value from the
upload command into sendFiles so the bounded pool in client.ts uses the
requested concurrency instead of always falling back to the default. Update
cmdUpload() in rocketride.ts to pass the third argument through to
sendFiles(fileObjects, taskToken!, maxConcurrent), matching the sendFiles worker
pool logic and its maxConcurrent parameter.
---
Outside diff comments:
In `@packages/client-typescript/src/client/client.ts`:
- Around line 1347-1355: The public `sendFiles` signature in `client.ts` now
includes the new `maxConcurrent` parameter, but the generated docs still show
the old `files, token` form. Update the TypeScript SDK reference docs under
`packages/client-typescript/docs/` by regenerating them (for example via
`client-typescript:docs-generate`) so the `sendFiles` documentation matches the
current signature and includes `maxConcurrent`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7513a56-3fdf-4574-95a0-1bcff69d0e59
📒 Files selected for processing (1)
packages/client-typescript/src/client/client.ts
Address CodeRabbit review on rocketride-org#1381: - sendFiles() now guards against non-finite/invalid maxConcurrent (e.g. NaN from an upstream parseInt on a malformed --max-concurrent flag). Previously NaN propagated through Math.min/Math.max, produced a zero-length worker array, and sendFiles resolved immediately without uploading anything. Invalid values now fall back to the documented default of 5; non-integers are floored. - The upload CLI command now forwards --max-concurrent into sendFiles() instead of always using the default 5.
Address CodeRabbit outside-diff comment on rocketride-org#1381: the SDK guide still showed the old (files, token) sendFiles() signature. Add the optional maxConcurrent parameter (default 5) to the TypeScript signature in the method reference table and to the send_files/sendFiles parameters table.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/client-typescript/docs/guide/methods/send.md`:
- Line 143: The `maxConcurrent` documentation in the `sendFiles()` guide does
not match the actual fallback behavior. Update the wording in the `sendFiles()`
docs table to reflect that only non-finite values fall back to `5`, while finite
out-of-range values are handled differently by the implementation, or adjust
`sendFiles()` so its concurrency logic matches the documented fallback. Use the
`sendFiles` symbol to locate the implementation and keep the docs aligned with
the runtime behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b7e7a54-f3aa-4857-abef-5e82db1ca1df
📒 Files selected for processing (2)
packages/client-typescript/docs/guide/index.mdpackages/client-typescript/docs/guide/methods/send.md
Address CodeRabbit comment on rocketride-org#1381: the parameter table claimed all invalid values fall back to 5, but sendFiles() only falls back to 5 for non-finite input; finite values are floored and clamped to at least 1. Narrow the wording to describe the actual behavior.
dsapandora
left a comment
dsapandora
left a comment
There was a problem hiding this comment.
Nice, tight fix — good bug to catch too, since an undocumented unbounded-concurrency upload path is exactly the kind of thing that looks fine in dev and then falls over on a real batch of files.
The worker-pool implementation is clean: nextIndex++ dispatch, results written by index so ordering survives out-of-order completion, and per-file failures caught so one bad upload doesn't sink the batch. I like that you also fixed the CLI call site to pass --max-concurrent through instead of leaving it disconnected.
On CodeRabbit's NaN concern (client.ts:1490) — that's already handled here: Number.isFinite(maxConcurrent) ? Math.floor(maxConcurrent) : 5 catches NaN before it ever reaches Math.min/Array.from, so the "zero uploads, no error" scenario it describes doesn't actually happen in this diff. Their second comment (docs wording for finite-but-invalid values like 0/-1) is correctly marked addressed — the clamp-to-1 behavior matches what the docs now say.
Two minor things, neither blocking:
- No test exercises the concurrency cap itself (e.g. asserting no more than
maxConcurrentuploads are in flight at once) — worth a quick unit test if there's an existing harness forsendFiles, since this is the actual bug being fixed. packages/client-typescript/docs/guide/index.mdtable row is now quite long (163+ chars) — pure formatting nit, not worth blocking on.
Looks good to merge.
|
@dsapandora or @asclearuc merge when ready! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/client-typescript/src/client/client.ts (2)
1484-1484: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
this.debugMessagefor consistency with the rest of the class.Every other error/log path in this class routes through
this.debugMessage(e.g. Lines 1174, 1203, 1226); this is the only spot using rawconsole.error, which bypasses any consumer-side debug/trace hooks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/client-typescript/src/client/client.ts` at line 1484, Replace the raw console.error in the upload failure path of client.ts with this.debugMessage so it matches the rest of the Client class logging behavior and preserves consumer-side debug/trace hooks. Locate the upload error handling around the file upload flow in Client and route the failure message plus err through this.debugMessage, following the pattern used by the other error paths in the class.
1399-1448: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose the pipe on upload errors
Failed uploads can leave the data pipe open on the server.send()already does best-effort cleanup, butuploadFileonly records the error; ifopen()succeeds andwrite()orclose()throws, the pipe stays allocated until server cleanup. Add a best-effortpipe.close()in the catch path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/client-typescript/src/client/client.ts` around lines 1399 - 1448, The uploadFile flow records errors but does not clean up an opened pipe when pipe.open(), pipe.write(), or pipe.close() fails. Update the catch path in uploadFile to perform a best-effort pipe.close() whenever pipe was created/opened, and ignore any cleanup failure so the original error is preserved. Use the existing pipe variable and the surrounding try/catch/finally logic in uploadFile to ensure server-side resources are released on failed uploads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/client-typescript/src/client/client.ts`:
- Line 1484: Replace the raw console.error in the upload failure path of
client.ts with this.debugMessage so it matches the rest of the Client class
logging behavior and preserves consumer-side debug/trace hooks. Locate the
upload error handling around the file upload flow in Client and route the
failure message plus err through this.debugMessage, following the pattern used
by the other error paths in the class.
- Around line 1399-1448: The uploadFile flow records errors but does not clean
up an opened pipe when pipe.open(), pipe.write(), or pipe.close() fails. Update
the catch path in uploadFile to perform a best-effort pipe.close() whenever pipe
was created/opened, and ignore any cleanup failure so the original error is
preserved. Use the existing pipe variable and the surrounding try/catch/finally
logic in uploadFile to ensure server-side resources are released on failed
uploads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67a48f9a-63d8-4f5f-a36e-9d34fa95cbe4
📒 Files selected for processing (2)
packages/client-typescript/docs/guide/index.mdpackages/client-typescript/src/client/client.ts
… non-torch packages
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
2 participants
Summary
sendFiles()advertised amaxConcurrentparameter (default5) in its JSDoc and example, but the actual signature accepted only(files, token)and the implementation opened one data pipe per file with no concurrency cap.This fixes the mismatch by implementing the documented behavior.
Closes #1381
Problem
Two concrete defects:
Unbounded concurrency.
Uploading
Nfiles openedNserver-side data pipes simultaneously (files.map(...)+Promise.all), contrary to the documented default of5. Large batches could exhaust pipes, sockets, or memory on both the client and server.Documented example didn't compile.
Under the package's strict TypeScript configuration, the JSDoc example:
failed with:
Fix
maxConcurrent = 5parameter to thesendFiles()signature, matching the JSDoc.files.map(...) + Promise.alldispatch with a bounded worker pool that keeps at mostmaxConcurrentuploads (and therefore data pipes) in flight at a time.index, so output order matches input order regardless of completion order.Testing
npx tsc --noEmitpasses forpackages/client-typescript(the documented 3-argument call now type-checks).maxConcurrent.Notes
docs/reference/is produced byclient-typescript:docs-generate.Summary by CodeRabbit
New Features
--max-concurrent/maxConcurrent), defaulting to 5 simultaneous uploads.Bug Fixes
Documentation
maxConcurrent.