Skip to content

fix(modal): a rejected deterministic archive closes its output descriptor once - #1215

Merged
aviggiano merged 3 commits into
mainfrom
claude/fup-archive-double-close
Sep 29, 2026
Merged

aviggiano merged 3 commits into
mainfrom
claude/fup-archive-double-close

Conversation

@aviggiano

@aviggiano aviggiano commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

packages/modal/test/deterministic-archive.test.ts › "rejects a caller-visible output parent replaced after descriptor normalization" fails intermittently. It failed in #1209's package gates lane:

AssertionError: expected [Function] to throw error including 'deterministic archive output changed while it was written' but got 'EBADF: bad file descriptor, close'

Root cause

writeDeterministicTarGzip passes its output descriptor to fs.createWriteStream(..., { fd, autoClose: false }). On failure, its catch calls output.destroy(), and its finally closes the descriptor.

  • Destroying an fs.WriteStream closes its descriptor even with autoClose: false (checked on Node 24.21), and the close is asynchronous.
  • When the output check fails after the pipeline has already finished, await completion returns at once. The synchronous close in finally then runs before the stream's own close.
  • The stream's close then targets a freed descriptor number. The result is EBADF, or, if the number was reused in the meantime, the close of an unrelated file.

Change

In the catch, the function waits for the destroyed stream's close event. It then leaves the descriptor to the stream and does not close it again. The success path is unchanged: there the stream does not close the descriptor, and finally closes it once.

The writer's only production caller is the Modal node provider, which draft #1197 deletes along with this file. This fix removes the CI flake whatever the decision on #1197.

Verification

  • New test "closes each descriptor once when the finished output is rejected". It records every fs.close and fs.closeSync and fails when a descriptor is closed twice. It fails on origin/main 3 of 3 times and passes here 6 of 6 times.
  • deterministic-archive.test.ts: 9/9.
  • modal tsc --noEmit, prettier, eslint and lint:strict:ci pass.

Greptile follow-up

  • Fixed: waiting for the destroyed stream's close could reject (comment). events.once rejects when error comes before close. The catch now listens for close directly and absorbs the stream's error. So the created output is still removed, the descriptor is never closed twice, and the original error is the one reported.
  • Fixed: the test now identifies the output descriptor (from fs.openSync) and requires exactly one close, so a leak fails it too (comment). On origin/main it fails with expected [ 24, 24 ] to have a length of 1 but got 2.

🤖 Generated with Claude Code

RetriggerConfidence Score: 5/5

The PR appears safe to merge; no new issue or outstanding previous finding was identified.

Summary

The PR waits for a rejected archive’s output stream to close before relinquishing its descriptor, and adds a test requiring that descriptor to close exactly once. The change since the previous review simplifies the close-event callback without changing its behavior.

Reviews (3) · Last reviewed commit: "style(modal): pass the close listener wi..."

…ptor once

When the output check failed after the gzip pipeline had finished, the
error path destroyed the write stream and then closed the output
descriptor again in finally. Destroying an fs.WriteStream closes its
descriptor even with autoClose off, asynchronously, and the finished
pipeline no longer waited for it. So the synchronous close ran first and
the stream's close then hit a freed descriptor number: EBADF, or the
close of whatever file had reused that number. CI saw it as 'EBADF: bad
file descriptor, close' instead of the expected output-changed error in
'rejects a caller-visible output parent replaced after descriptor
normalization'.

Wait for the stream's close and do not close the descriptor again.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Comment thread packages/modal/src/deterministic-archive.ts
Comment thread packages/modal/test/deterministic-archive.test.ts
…rows

events.once rejects when the stream emits 'error' before 'close'. A
write error raised while an earlier failure was being handled then
skipped the cleanup of the created output, left the descriptor to be
closed again in finally, and replaced the original error. Listen for
'close' directly and absorb the stream's error, so the original error is
the one reported. The test now identifies the output descriptor and
requires exactly one close, so a leak fails it too (Greptile, on #1215).

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@aviggiano aviggiano closed this Sep 29, 2026
@aviggiano aviggiano reopened this Sep 29, 2026
@aviggiano
aviggiano requested a review from a team as a code owner September 29, 2026 15:22
The strict type-aware lint (no-confusing-void-expression) rejects an
arrow shorthand that returns resolve()'s void result.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant