fix(harness): complete short writes - #4
Conversation
|
@onmax is attempting to deploy a commit to the unjs Team on Vercel. A member of the Team first needs to authorize it. |
pi0x
left a comment
There was a problem hiding this comment.
Approving. Verified independently rather than from the description: applied the branch locally and ran the suite on this host — test/index.test.ts 18 passed, full pnpm test green (lint, typecheck, 900 passed / 185 skipped). The count differs from the 888/197 in the description only because 12 Tier-2 tests gate on fusermount3 and ran here instead of skipping; the 1085 total matches.
What I checked in the change itself:
- Offset and position advance together.
handle.write(bytes, written, remaining, written)is the correct shape, and the test asserts it rather than only asserting the final content —[[0,2,0],[2,2,2],[4,1,4]]pins both the buffer offset and the explicit position, which is the part that would silently regress otherwise. - The guard covers every way the loop could go wrong.
!Number.isIntegercatchesNaN/Infinityas well as fractions,<= 0prevents the infinite loop,> remainingprevents skipping past the end.EIOis the right errno for a driver that violated the contract. - Zero-length is unchanged —
byteLength === 0never enters the loop, andopen(path, "w")still creates and truncates, so empty-file behaviour is identical to before. - Symmetry with
readFileis the point. The existingreadFilealready loops on short reads; this makes the pair consistent, which is exactly what the issue asked for.
Coverage is better than the diff suggests: test/conformance.ts calls fs.writeFile 45 times across every driver column, so the non-short path of this loop is exercised broadly by Tier 0 — the new tests only need to cover the short and invalid cases, which they do.
Two non-blocking notes, neither worth holding the merge for:
- Destructuring
const { bytesWritten } = await handle.write(...)throws a rawTypeErrorif a driver returns nothing, where the old code silently tolerated it. That is out of contract (FileHandleLike.writeis typedPromise<WriteResult>) andreadFilehas the identical exposure one line up — so matching it is the right call, not a defect. Worth knowing only because this helper is what driver authors debug against. - The description's note that FUSE and NFS sessions never call
Loopback.writeFile()checks out — no match anywhere insrc/fuse/orsrc/nfs/. So this is a driver-authoring and conformance-surface fix, not a mount-path one, and the scoping to G1 alone is right.
Tracking issue updated.
Summary
createLoopback().writeFile()when a Node-compatible handle reports a legitimate short writeEIOinstead of hanging or returning false successAddresses Group G item 1 in #1.
Scope
This is only the
src/harness.tswhole-file helper fix from G1. It does not include G2-G5, transport write paths, driver performance changes, public-surface changes, or work from other pull requests.Original intent
FsDriveris deliberately a structural subset ofnode:fs/promises, andFileHandleLike.write()returnsbytesWrittenbecause a successful write can be short. Issue #1 asks this helper to loop like its existingreadFile()counterpart. Node's own whole-file implementation also awaits and advances across repeated writes.The guard accepts every positive integer count within the requested remainder. A zero, negative, fractional, or oversized result violates the driver contract; failing it promptly prevents an infinite loop or skipped bytes.
Before and after
Against base
ffc0f66022f53a20ac57edb8c54af4ba4948f49d, an external custom driver limited to two bytes per call produced"ab"from"abcdefgh"and received one call:After this change, the packed package installed into a fresh external project produces all bytes through advancing calls:
That packed external consumer is the relevant E2E boundary. FUSE and NFS sessions call file handles directly and do not invoke
Loopback.writeFile(), so a real mount would not exercise this fix.Verification
pnpm exec vitest run test/index.test.ts— 18 passedpnpm test— lint, typecheck, 888 passed and 197 skippedpnpm build