feat(sdk): add segment batch size and max concurrent segment batches options - #920
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 38 minutes and 51 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded configurable segment batching and bounded prefetch scheduling to the decryption pipeline by introducing Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Reader as "ZTDF Reader"
participant Scheduler as "SegmentBatchScheduler"
participant Fetcher as "Fetch/Decrypt Worker"
participant Stream as "Readable Stream"
Client->>Reader: read(request, opts with batch settings)
Reader->>Scheduler: createBoundedSegmentScheduler(config)
Reader->>Scheduler: fillWindow()
Scheduler->>Fetcher: schedule fetchAndDecryptBatch(batchRange)
Fetcher-->>Scheduler: resolve(batchSegments)
Scheduler->>Reader: mark batch scheduled/resolved
Reader->>Stream: push next ready chunks
Stream->>Client: consumer reads chunk
Client->>Reader: acknowledge/consume(n)
Reader->>Scheduler: markConsumed(n)
Scheduler->>Scheduler: update inFlight / scheduled counters
Note over Scheduler,Fetcher: on failure -> reject affected segment promises -> propagate error to Reader/Stream
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
bnrobinson93
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lib/tests/web/roundtrip.test.ts (1)
149-179: Test exercises the scheduler path but does not assert the bounded behavior.
trackingChunkeris named as if it tracks fetches, yet nothing is recorded/asserted about call counts or windowing. As a result this test would still pass ifsegmentBatchSize/maxConcurrentSegmentBatcheswere silently dropped in the pipeline — it only validates the plaintext roundtrip. Consider capturing chunker invocations (e.g., ranges/timestamps) and asserting at least that the chunker was called more than once and batch sizes don't exceedsegmentBatchSize * encryptedSegmentSize, to guard against future regressions in the plumbing fromReadOptions→decryptStreamFrom.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tests/web/roundtrip.test.ts` around lines 149 - 179, Instrument the trackingChunker used in the test to record each invocation's byteStart/byteEnd (e.g., push ranges into an array inside trackingChunker) and then after the read completes assert that the recorded calls show the bounded scheduler behavior: (1) there was more than one invocation (to ensure batching/scheduling happened), and (2) for each recorded range verify (end - start) does not exceed segmentBatchSize * encryptedSegmentSize (or the equivalent max bytes expected per batch), and optionally assert no more than maxConcurrentSegmentBatches overlapping batch windows; update references to trackingChunker, client.read, segmentBatchSize and maxConcurrentSegmentBatches in the test to include these assertions.lib/tests/mocha/unit/tdf.spec.ts (1)
170-246: Tests validate the happy path; consider adding coverage for edges of the new scheduler.The two scenarios correctly pin the window semantics (
remainingWindow < nextBatchSize⇒ break). To harden the contract exposed bycreateBoundedSegmentScheduler, please consider adding:
- A scenario where
totalSegments < segmentBatchSize(partial-only batch) so the last-batch sizing viaMath.min(segmentBatchSize, totalSegments - startIndex)is covered.- A scenario where
scheduleBatchrejects, verifying thatonErrorfires with the correctstartIndex/endIndexand thatstoppedprevents further scheduling.- Validation coverage via
decryptStreamFrom/ the normalizer (segmentBatchSize: 0or non-integer should throwConfigurationError).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tests/mocha/unit/tdf.spec.ts` around lines 170 - 246, Add three unit tests exercising edge cases for createBoundedSegmentScheduler: (1) a test where totalSegments < segmentBatchSize to assert the final scheduled batch uses Math.min(...) sizing (referencing createBoundedSegmentScheduler and scheduleBatch to verify the single partial batch start/end), (2) a test where scheduleBatch returns a rejected promise to assert onError is invoked with the failing startIndex/endIndex and that the scheduler transitions to a stopped state preventing further scheduling (reference scheduleBatch, onError, and stopped), and (3) validation tests invoking the public entry used by the normalizer (e.g. decryptStreamFrom or the normalizer path) with segmentBatchSize: 0 and a non-integer value to assert a ConfigurationError is thrown (reference ConfigurationError and segmentBatchSize). Ensure each test uses deferred promises or rejections and assertions on scheduler.snapshot() or started batches to confirm behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/tdf3/src/tdf.ts`:
- Around line 74-77: The current bounded-scheduler defaults silently reduce
throughput when callers set only one of the knobs; update the defaults to match
the legacy values and add a short JSDoc note about the coupling: change
DEFAULT_BOUND_SEGMENT_BATCH_SIZE to 500 and
DEFAULT_BOUND_MAX_CONCURRENT_SEGMENT_BATCHES to 3 so that setting only
segmentBatchSize or maxConcurrentSegmentBatches preserves legacy throughput, and
add/augment JSDoc on getBoundedSegmentSchedulerOptions (and ReadOptions/builder
docs) to state that both options should be adjusted together for expected
performance.
- Around line 1058-1111: fetchAndDecryptChunkSlice currently swallows
fetch/decrypt failures by calling rejectChunks and returning a resolved promise,
which prevents scheduleBatch's .catch handler from flipping stopped and invoking
onError; either make the scheduler fail-fast or remove the dead stopped/onError
path. Fix by modifying fetchAndDecryptChunkSlice (and the similar block around
the other occurrence) to rethrow (or throw a new aggregated error) after calling
rejectChunks (use asDecryptError for decrypt failures and wrap fetch failures
similarly) so scheduleBatch receives a rejected promise, or alternatively remove
the stopped and onError logic (and any .catch that sets stopped) from wherever
scheduleBatch is used (keeping rejectChunks behavior) to make the code
consistent.
- Around line 1045-1056: The TypeScript error occurs in asDecryptError where
DecryptError's second parameter expects Error|undefined but we pass unknown;
update asDecryptError to avoid passing the unknown as the cause — include the
unknown value in the fallback message and pass undefined as the cause.
Concretely, change the return in asDecryptError to something like new
DecryptError(`${fallbackMessage}: ${String(error)}`, undefined) so the message
contains the unknown details while satisfying DecryptError's constructor type;
keep function name asDecryptError and ensure rejectChunks remains unchanged.
---
Nitpick comments:
In `@lib/tests/mocha/unit/tdf.spec.ts`:
- Around line 170-246: Add three unit tests exercising edge cases for
createBoundedSegmentScheduler: (1) a test where totalSegments < segmentBatchSize
to assert the final scheduled batch uses Math.min(...) sizing (referencing
createBoundedSegmentScheduler and scheduleBatch to verify the single partial
batch start/end), (2) a test where scheduleBatch returns a rejected promise to
assert onError is invoked with the failing startIndex/endIndex and that the
scheduler transitions to a stopped state preventing further scheduling
(reference scheduleBatch, onError, and stopped), and (3) validation tests
invoking the public entry used by the normalizer (e.g. decryptStreamFrom or the
normalizer path) with segmentBatchSize: 0 and a non-integer value to assert a
ConfigurationError is thrown (reference ConfigurationError and
segmentBatchSize). Ensure each test uses deferred promises or rejections and
assertions on scheduler.snapshot() or started batches to confirm behavior.
In `@lib/tests/web/roundtrip.test.ts`:
- Around line 149-179: Instrument the trackingChunker used in the test to record
each invocation's byteStart/byteEnd (e.g., push ranges into an array inside
trackingChunker) and then after the read completes assert that the recorded
calls show the bounded scheduler behavior: (1) there was more than one
invocation (to ensure batching/scheduling happened), and (2) for each recorded
range verify (end - start) does not exceed segmentBatchSize *
encryptedSegmentSize (or the equivalent max bytes expected per batch), and
optionally assert no more than maxConcurrentSegmentBatches overlapping batch
windows; update references to trackingChunker, client.read, segmentBatchSize and
maxConcurrentSegmentBatches in the test to include these assertions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6fd778d-2eeb-457d-831f-1d683faf6018
📒 Files selected for processing (7)
lib/src/opentdf.tslib/tdf3/src/client/builders.tslib/tdf3/src/client/index.tslib/tdf3/src/tdf.tslib/tests/mocha/unit/builders.spec.tslib/tests/mocha/unit/tdf.spec.tslib/tests/web/roundtrip.test.ts
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a bounded segment scheduler to manage the fetching and decryption of payload segments in batches. It adds configuration options segmentBatchSize and maxConcurrentSegmentBatches to the decryption process, allowing for better control over concurrency and memory usage. The implementation includes a new SegmentBatchScheduler, refactored chunk processing logic in tdf.ts, and corresponding updates to the client builders and read options. Additionally, memory efficiency is improved by using Uint8Array.prototype.subarray instead of slice during segment processing. Unit and integration tests have been added to verify the scheduler's behavior and the end-to-end decryption flow. I have no feedback to provide.
This comment was marked as resolved.
This comment was marked as resolved.
14f48fc to
c678f88
Compare
eugenioenko
left a comment
eugenioenko
left a comment
There was a problem hiding this comment.
Code changes look solid.
There is one test we can update to test this e2e here:
web-sdk/web-app/tests/tests/huge.spec.ts
Line 14 in f77cb3d
The test would be to download same generated file but with modified segmentBatchSize.
It can be done in a follow up PR
X-Test Failure Report |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-app/tests/tests/huge.spec.ts (1)
47-50:⚠️ Potential issue | 🟠 MajorRemove the
awaitfrom line 45 to prevent timeout.Line 45 awaits the download event before clicking
#decryptButton, which prevents the click from executing and causes the event to never trigger. This blocks indefinitely (or until the 60-second timeout) since the action that emits the event hasn't run yet. Match the correct pattern used on line 30: store the promise first, execute the click, then await it.🧪 Proposed fix
- const plainDownloadPromise = await page.waitForEvent('download', { timeout: 60000 }); + const plainDownloadPromise = page.waitForEvent('download', { timeout: 60000 }); await page.locator('#fileSink').click(); await page.locator('#decryptButton').click(); const download2 = await plainDownloadPromise;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-app/tests/tests/huge.spec.ts` around lines 47 - 50, The download promise is being awaited before the action that triggers it, so remove the premature await when assigning plainDownloadPromise (use page.waitForEvent('download') without awaiting), then perform the clicks on '#fileSink' and '#decryptButton', and only after those actions await the stored promise (download2 = await plainDownloadPromise); reference the plainDownloadPromise variable, page.waitForEvent('download'), and the '#decryptButton'/'#fileSink' locators to locate and fix the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web-app/src/App.tsx`:
- Around line 90-115: The readPositiveIntSearchParam function is too permissive
(e.g., accepts "2abc" or "2.5") and URL values need upper bounds before being
merged into decrypt tuning; update readPositiveIntSearchParam to strictly
validate the raw string (e.g., only digits via /^\d+$/), then parse and enforce
a positive integer, and add caps in getDecryptReadTuningFromLocation by applying
Math.min(parsedValue, <UPPER_BOUND>) for segmentBatchSize and
maxConcurrentSegmentBatches (introduce constants like MAX_SEGMENT_BATCH_SIZE and
MAX_CONCURRENT_SEGMENT_BATCHES), keeping the same conditional spreading logic
for segmentBatchSize and maxConcurrentSegmentBatches so undefined/invalid params
are omitted.
---
Outside diff comments:
In `@web-app/tests/tests/huge.spec.ts`:
- Around line 47-50: The download promise is being awaited before the action
that triggers it, so remove the premature await when assigning
plainDownloadPromise (use page.waitForEvent('download') without awaiting), then
perform the clicks on '#fileSink' and '#decryptButton', and only after those
actions await the stored promise (download2 = await plainDownloadPromise);
reference the plainDownloadPromise variable, page.waitForEvent('download'), and
the '#decryptButton'/'#fileSink' locators to locate and fix the code.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d5d838a-c914-4012-b05d-9f30e0d01ab8
📒 Files selected for processing (6)
lib/src/opentdf.tslib/tdf3/src/client/builders.tslib/tdf3/src/tdf.tslib/tests/mocha/unit/tdf.spec.tsweb-app/src/App.tsxweb-app/tests/tests/huge.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/tdf3/src/tdf.ts
X-Test Failure Report✅ java-main |
appUrl was never defined — introduced in #920. Use page.url() and append query params to navigate to the current origin instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
This PR introduces new configuration options for segment processing within the TDF (Trusted Data Format) library. Specifically, the following enhancements have been made:
Added Configuration Options:
segmentBatchSize: This option defines the maximum number of payload segments that can be fetched and decrypted per batch.maxConcurrentSegmentBatches: This option limits the number of segment batches that may be fetched concurrently.Affected Files:
DecryptParamsBuilderto allow setting the new options.This change enhances the performance and control over how segments are fetched and processed, improving efficiency in handling larger data sets.
You can adjust any part of the title or body to better fit your style or the project's conventions!
Summary by CodeRabbit
New Features
Tests