Repository navigation
feat(sdk): make consumption-paced decrypt prefetch the default (DSPX-4654) - #1024
dmihalcik-virtru wants to merge 1 commit into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
d6a1ecd to
a8ce43f
Compare
aab50c7 to
1ab0150
Compare
1ab0150 to
01383d5
Compare
a8ce43f to
2f8c03d
Compare
X-Test Failure Reportopentdf-ctl |
01383d5 to
27fb733
Compare
2f8c03d to
54f59eb
Compare
X-Test Failure Reportcoverage-web-app-browser |
54f59eb to
6fc06b1
Compare
27fb733 to
aedec68
Compare
X-Test Failure Reportcoverage-web-app-browser |
6fc06b1 to
7dece30
Compare
aedec68 to
bc3b4c7
Compare
X-Test Failure Reportopentdf-ctl |
…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>
7dece30 to
a803a9f
Compare
bc3b4c7 to
a23f972
Compare
X-Test Failure Reportcoverage-web-app |
|
|
Superseded by #1091, which is the same change rebased directly onto |
1 participant
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.