feat(sdk): make consumption-paced decrypt prefetch the default (DSPX-4654) - #1024
Open
dmihalcik-virtru wants to merge 1 commit into
Open
feat(sdk): make consumption-paced decrypt prefetch the default (DSPX-4654)#1024dmihalcik-virtru wants to merge 1 commit into
dmihalcik-virtru wants to merge 1 commit into
Conversation
…4654) Bounded segment scheduling was opt-in, selected only when a caller set segmentBatchSize or maxConcurrentSegmentBatches. Everyone else got updateChunkQueue, which fired 500-segment batches three at a time without waiting for the consumer -- 1.5 GiB of payload in flight at the 1 MiB segment size, and 24 GiB once chooseSegmentSize picks 16 MiB. Route every decrypt through createBoundedSegmentScheduler and derive its parameters from a byte budget rather than a segment count, so peak memory stays flat as the segment size grows: 1 MiB -> 42 x 3 = 126 MiB 16 MiB -> 2 x 3 = 96 MiB 4 MiB -> 10 x 3 = 120 MiB 64 MiB -> 1 x 2 = 128 MiB Concurrency yields before batch size, since overlapping reads are a better use of a tight budget. MAX_SEGMENT_BATCH_SIZE caps the batch at the legacy 500 so a tiny-segment file is not one absurd ranged read. A segment larger than the budget gets a window of one and knowingly blows it; there is no smaller unit. Deletes updateChunkQueue and the LEGACY_* / DEFAULT_BOUND_* constants outright rather than leaving a second path to rot. Behaviour change: the legacy path did `.catch(() => undefined)`, leaving the consumer awaiting a mailbox that never settled. Batch failures now surface through onError and reject the affected chunks -- a clean failure instead of a hang. No existing test depended on the silent behaviour. This does not bound decrypt memory on its own; readStream still materializes one Chunk per segment up front. Documented in the spec as outstanding. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
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 |
|
dmihalcik-virtru
added this pull request to stack #1033
September 10, 2026 20:52
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
1 participant
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.
Item 4 of
spec/DSPX-4648-web-sdk-large-files.md, sub-taskDSPX-4654. Final PR in the stack.Stacked on #1023 (
DSPX-4653). Review only the last commit; the base is the rest of the stack.What
Bounded segment scheduling was opt-in, selected only when a caller set
segmentBatchSizeormaxConcurrentSegmentBatches. Everyone else gotupdateChunkQueue, which fired 500-segment batches three at a time without waiting for the consumer — 1.5 GiB of payload in flight at the 1 MiB segment size, and 24 GiB oncechooseSegmentSize(PR A) picks 16 MiB. Neither number was chosen; they fell out of a segment count that nothing tied to memory.Every decrypt now goes through
createBoundedSegmentScheduler, with its parameters derived from a byte budget rather than a segment count.Why a byte budget
derivePrefetchWindowdividesDEFAULT_PREFETCH_BYTE_BUDGET(128 MiB) by the segment size, so peak memory stays roughly flat as the segment size grows:Two deliberate asymmetries:
MAX_SEGMENT_BATCH_SIZE = 500(the legacy value) caps the batch below what the budget would allow. At a 1 KiB segment size the budget permits 43,690 segments per batch — inside 128 MiB, but the consumer would wait on one absurd ranged read for the first byte.The one case that exceeds the budget is a segment larger than the budget itself. There is no smaller unit to schedule, and refusing to decrypt a validly-formed TDF would be worse than spending the memory.
The budget is charged against
encryptedSegmentSizeDefault, not the plaintext size — ciphertext segments are what the window actually holds.Explicit knobs still win, and each falls back to its derived value rather than to the other, so setting one does not silently rescale the other.
Behaviour change (intentional)
The legacy path swallowed batch failures with
.catch(() => undefined), leaving the consumer awaiting a mailbox that would never settle — a hang, not a clean failure. Failures now surface through the scheduler'sonError, which rejects the affected chunks and stops scheduling. Arguably a bug fix, but it is observable, so it is called out here and in the spec. No existing test depended on the silent behaviour.updateChunkQueueand theLEGACY_*/DEFAULT_BOUND_*constants are deleted rather than bypassed, so there is no second path left to rot.What this does not do
It does not bound decrypt memory on its own.
readStreamstill materializes oneChunk(hash string + mailbox promise) per segment before the first byte is read — at 16 MiB segments a 50 TiB file is 3.28 M of them. That is bounded by PR A's manifest cap but not by this budget; fixing it means streaming the segment list rather thansegments.map(...), which is a separate change. Recorded as outstanding in the spec.Also deferred: adaptive sizing from
navigator.deviceMemory(a fixed number is predictable across browsers and testable), andprogressHandler-driven backpressure.How to test
cd lib && npm testNew coverage:
scale-limits.spec.ts—derivePrefetchWindow: window stays inside the budget at every rung of the ladder; concurrency yields before batch size; the batch cap binds at small segments; the legacy 1 MiB window is preserved; single-oversized-segment fallback; caller-supplied budgets; input validation.tdf.spec.ts—getBoundedSegmentSchedulerOptions: derived defaults with nothing configured, explicit settings winning without rescaling the other, non-positive settings rejected. Plus'surfaces a failing batch instead of stalling, and stops scheduling', which pins the error-behaviour change.Verified green locally across all three tiers: mocha 486 passing / 6 pending, karma
TOTAL: 486 SUCCESS, wtr 253 passed,npm testexit 0,npm run lintexit 0.Risk
Touches the decrypt hot path for every caller, not just large files. The scheduler itself is unchanged and was already covered; the change is which path reaches it and with what parameters. The error-surfacing change is the one to watch — a caller that previously saw a hang on a failed segment fetch will now see a rejection.