Repository navigation
fix(sdk): cancel the source stream when a TDF write fails or is cancelled (DSPX-5146) - #1084
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: canceling the source stream when a TDF write fails or is canceled.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks the stream’s way through,
When errors come, the source gets told.
The first error stays in view,
While cancel reasons travel bold.
The rabbit hops, the tests turn gold.
Comment @coderabbitai help to get the list of available commands.
dmihalcik-virtru
changed the title
…lled writeStream locked cfg.contentStream for its whole lifetime but never released it. When pull threw (byte limit, cipher failure, and soon IvExhaustionError from DSPX-4492), or the consumer cancelled the output, the input stayed locked and open: file handles, sockets, and downloads leaked, and callers could not reuse the stream. Cancel the source with the failing error (best effort, so a cancel failure cannot mask the original error), and forward consumer cancellation to the source. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
aa01257 to
e9002ad
Compare
|
2 participants
Fixes DSPX-5146.
What
writeStream(lib/tdf3/src/tdf.ts) callsgetReader()oncfg.contentStream, which locks it, and never releases the lock. This PR does two things:pullthrows, cancel the source with the error that caused the failure. Examples: the byte limit is exceeded, the cipher fails or gets its size wrong, or the upcomingIvExhaustionErrorfrom feat(sdk): throw IvExhaustionError when IV collision risk exceeds 2^-32 (DSPX-4492) #1077 is thrown. The cancel is best effort and not awaited, so a slow or failing cancel can't delay or replace the error the consumer sees. If the cancel fails with a different error, it's logged withconsole.warn. A source that errored on its own rejectscancelwith that same error, so that case isn't logged twice.cancelhandler to the underlying source, so a consumer that cancels the encrypted output also cancels the input.Most of the diff is re-indentation from wrapping
pullintry/catch. With whitespace ignored it's about 20 lines.Why
Before this change, a failed or cancelled write left the input locked and open. File handles, sockets, and in-progress fetch downloads leaked, and the caller couldn't reuse the stream. Found while reviewing #1077, whose new error fires after a very long write and tells users to "split the input". This PR is independent of #1077 and is based on
main.How to test
cd lib && npm test. The new tests are intests/mocha/encrypt-decrypt.spec.ts, under "releases the source when the write stops early":toBuffer()rejects with it.toBuffer()still rejects with the original error, and the cancel error is logged once withconsole.warn.toBuffer()rejects with the source's error and nothing is logged.Without the fix, the first two tests fail. The other two cover the logging.
Risk
Small. This touches only the write stream's error path and cancel path. Nothing changes on the wire or on the read side.
Summary by CodeRabbit