fix(zip): observe writer promises so ZIP errors surface to the pipeline#114
Open
dobby-coder[bot] wants to merge 1 commit into
Open
fix(zip): observe writer promises so ZIP errors surface to the pipeline#114dobby-coder[bot] wants to merge 1 commit into
dobby-coder[bot] wants to merge 1 commit into
Conversation
createZipReadable fired writer.write()/writer.close() without observing their promises. When a source file read failed or conflux rejected, those promises rejected with no handler and surfaced as unhandled rejections instead of reaching the consumer of the returned readable. Accumulate the write promises plus the close promise and observe them with Promise.all; on failure, abort the writer, which errors the linked readable so anything piping it (e.g. sealStream in encrypt.ts) sees the failure rather than a silently truncated archive. Adds a failing-source regression test that asserts no unhandled rejection leaks while the returned readable still rejects on drain. Closes #107 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
There was a problem hiding this comment.
VERDICT: approve
Rules Dobby 2 sign-off — PR #114 (fixes #107)
Consolidated review (rule check + Review Dobby 2 findings): no blocking issues.
- Correctness:
Promise.all(pending).catch(...)observes everywriter.write()promise pluswriter.close(), so no rejection escapes unhandled;Promise.allattaches a handler to each input, so late-rejecting losers are observed too. On failurewriter.abort(err)errors the linkedreadable, so a consumer (e.g.sealStream) sees the failure instead of a truncated archive. Happy path and backpressure are unchanged (writes still enqueued without awaiting, matching the original). - Test: the added
tests/zip.test.tsregression feeds aFilewhoseslice().arrayBuffer()rejects;createFileReadablepropagates that throughpull, erroring the source → conflux errors the writer → the observed promise rejects. It asserts the drained readable rejects and that nounhandledRejectionleaks. Verified the error path is real. - Rules: conventional-commit title (
fix(zip): ...) satisfies the pr-title workflow; regression test present (tests-required-on-fixes);Closes #107correct for a full fix; prose clean.
Review Dobby 2: 0 findings, full suite 206 passing, tsc clean.
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
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.
Closes #107
Problem
In
createZipReadable(src/util/zip.ts) the writer promises were fire-and-forget:When a source file read fails (or conflux rejects), those promises reject with no handler and surface as unhandled rejections instead of reaching the consumer of the returned
readable. This contradicts the deliberate stream-error handling elsewhere in the pipeline (awaitAllOrAbortinsrc/crypto/encrypt.ts).Fix
writer.write(...)promise plus thewriter.close()promise.Promise.all(...).catch(...), so no rejection escapes unhandled (Promise.allattaches a handler to each input, so late-rejecting losers are observed too).writer.abort(err)errors the linkedreadable, so anything piping it (e.g.sealStream) sees the failure rather than a silently truncated archive.The happy path is unchanged: the same
readableis returned and writes are still enqueued without blocking, so backpressure behaviour is identical.Test
Added a failing-source regression test in
tests/zip.test.tsthat feeds aFilewhose read rejects. It asserts:unhandledRejectionleaks to the process.Verified it fails before the fix (two unhandled rejections captured) and passes after. Full suite: 206 passing;
tsc --noEmitclean.