Repository navigation
fix(local-disk): handle partial file writes - #19
Conversation
Finish every chunk before committing the temporary file. Reject writes that make no progress or persist a different size, preserving the previous file and metadata on failure. Closes nestjs#18
micalevisk
left a comment
micalevisk
left a comment
There was a problem hiding this comment.
Thanks @rafaself — tight fix with a real root cause behind it.
Verified locally against the PR head (full suite, Node 26): npm run build, npx tsc -p tsconfig.json --noEmit, npm run lint and npx vitest run all pass (900 passed / 16 skipped). I also ran the new specs against master's implementation: 4 of the 6 fail, so the regression coverage is genuine.
Correctness. Retrying with position: null is the right choice here — the descriptor's offset advances by the bytes actually written, so each retry resumes exactly where the short write stopped; an explicit position would have needed separate bookkeeping. Skipping zero-length chunks preserves the previous behaviour minus a pointless syscall, and the reported size and the stat feeding the etag are now provably the same number. The abort path still rejects with signal.reason, consistent with abortable() upstream.
Worth recording why this matters, since the repro in #18 is synthetic: on Unix, libuv's uv__fs_write_all already retries ordinary short writes before they reach JS, so the case that actually surfaces as bytesWritten < length with no error is partial progress followed by a failing syscall (ENOSPC / EDQUOT / EFBIG / EIO mid-chunk), plus genuinely short-writing filesystems such as FUSE and network mounts. In those cases master commits a truncated file and reports the full size, which is silent corruption. The shape this PR adopts — loop on bytesWritten, abort check between writes, reject zero progress — is the same one Node core uses in its own write-all helpers; the low-level FileHandle.write() is deliberately a single syscall.
No scope creep: no stray logging, no .only / .skip, no commented-out code, no lockfile or dependency churn, no reformatting of untouched regions. The commit message follows the convention. The "no breaking change" answer is honest — a put() that previously succeeded while corrupting data now rejects, and no export, interface, decorator signature or option default moves.
Approving. A few non-blocking notes inline, plus two things that aren't yours to fix:
- Prettier reports both changed files as unformatted, but
masteris already unformatted and CI runs no formatter (thelint-stagedglob is empty), so there is nothing to do here. - #17 edits the same two files and its spec hunk overlaps this one's region, so whichever lands second will need a rebase.
If the storage package picks up a page on docs.nestjs.com, the new failure mode deserves a line there as a follow-up.
Identify the object whose write stopped making progress so callers can trace a failed upload. Verify that the error message includes its key.
Include the object key when persisted size differs from the written body. Cover the message while preserving the existing file on failure.
Check stored content, reported size, and cleanup without requiring a particular buffer offset sequence. Allow equivalent write-all loops.
Abort on the second write and require cancellation before a third call. Distinguish the retry loop from master while checking target cleanup.
Yield empty buffers through an async iterable so they reach the disk write loop. Verify an empty file is stored without issuing any writes.
|
Thanks for the review, @micalevisk |
* docs(file-storage): explain local write failures Describe zero-progress and persisted-size errors, temporary-file cleanup, and preservation of existing files when a local write fails. Follow up nestjs/storage#19. * docs(file-storage): keep the local write paragraph on one line --------- Co-authored-by: Kamil Myśliwiec <mail@kamilmysliwiec.com>
3 participants
PR Checklist
PR Type
What is the current behavior?
LocalDisk.writeObject()ignoresFileHandle.write()'sbytesWrittenand assumes each chunk was written completely. A short write can commit
corrupted content successfully and report a larger size than the file.
For example, two six-byte chunks can produce a six-byte file while
put()reports 12 bytes.Closes #18.
What is the new behavior?
Finish each chunk by writing its remaining bytes sequentially and count
the bytes actually written. Reject zero progress, check cancellation
between writes, and verify the persisted size before committing.
The existing temporary-file cleanup preserves the previous file and
metadata when these checks fail. Update the LocalDisk documentation and
add six regression tests covering short writes across sliced buffers and
multiple chunks, zero progress, write errors, cancellation, empty bodies,
and persisted-size mismatches.
Does this PR introduce a breaking change?
Other information
MinIO tests require its unavailable binary; one test requires macOS.
stores the complete content and reports the correct 12-byte size.