Skip to content

fix: skip writes to destroyed file streams - #1115

Open
neallin917 wants to merge 2 commits into
node-formidable:masterfrom
neallin917:codex/skip-writes-to-destroyed-files
Open

fix: skip writes to destroyed file streams#1115
neallin917 wants to merge 2 commits into
node-formidable:masterfrom
neallin917:codex/skip-writes-to-destroyed-files

Conversation

@neallin917

@neallin917 neallin917 commented Jul 29, 2026

Copy link
Copy Markdown

Summary

  • skip writes when a persistent file stream has already been destroyed
  • align PersistentFile.write() with the existing VolatileFile.write() guard
  • add a deterministic regression test for the destroyed-but-not-yet-closed stream state

Why

A destroyed fs.WriteStream is not necessarily marked closed immediately. During that interval, PersistentFile.write() currently calls .write() and records the buffer as successfully written even though the stream has already been destroyed. Depending on timing and the underlying filesystem callback, this is also the race behind Cannot call write after a stream was destroyed failures reported after aborted uploads.

The new test demonstrates the exact state before the fix: destroyed === true, closed === false, and the ignored buffer was still added to file.size.

This addresses the destroyed-stream race discussed in #958 without claiming to reproduce every environment-specific crash from that report.

Validation

  • pnpm test: 12 node:test tests passed; 91 Jest tests passed and 3 existing tests skipped
  • pnpm exec prettier --check src/PersistentFile.js test-node/files/persistent-file.test.js
  • pnpm exec eslint --no-cache --quiet --format codeframe src/PersistentFile.js test-node/files/persistent-file.test.js
  • pnpm run build-package
  • git diff --check

Greptile Summary

This PR prevents writes to destroyed persistent file streams.

  • Extends the existing closed-stream guard to include destroyed streams.
  • Adds a deterministic regression test confirming ignored writes do not increase file size.

Confidence Score: 4/5

The PR is not yet safe to merge because ignored writes still alter the hash even though their bytes are not persisted.

The destroyed-stream check prevents the write and size update, but hash.update(buffer) still runs first, producing a hash that can disagree with the uploaded file.

Files Needing Attention: src/PersistentFile.js

Important Files Changed

Filename Overview
src/PersistentFile.js Adds the destroyed-stream write guard, but hashing still occurs before that guard and remains inconsistent with persisted content.
test-node/files/persistent-file.test.js Adds a focused regression test covering the destroyed-but-not-yet-closed stream state and file-size behavior.

Reviews (2): Last reviewed commit: "Merge branch 'master' into codex/skip-wr..." | Re-trigger Greptile

@neallin917
neallin917 marked this pull request as ready for review July 29, 2026 05:12
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