fix(fs): propagate write errors#6415
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSynchronous and callback-based filesystem string and buffer writes now preserve syscall failures. Synchronous APIs throw filesystem errors, while callback APIs receive errors. Unix errno mappings and read-only descriptor coverage are also expanded. ChangesFilesystem write error propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test-files/test_gap_fs_write_error_propagation.ts (1)
6-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove file setup into the
try...finallyblock to ensure robust cleanup.Currently, if
fs.writeFileSyncorfs.openSyncthrows an error during setup, the script will exit beforemain()is executed, leaving the temporary directory uncleaned on disk. Additionally, prependingos.tmpdir()to the prefix prevents the temporary directory from being created in the current working directory.Consider refactoring the setup and cleanup flow:
♻️ Proposed refactor
-import * as fs from "node:fs"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; -const dir = fs.mkdtempSync("perry-fs-write-error-"); -const file = dir + "/file.txt"; -fs.writeFileSync(file, "seed"); -const fd = fs.openSync(file, "r"); +const dir = fs.mkdtempSync(path.join(os.tmpdir(), "perry-fs-write-error-")); +let fd: number | undefined; function syncCode(label: string, fn: () => void): void { try { fn(); console.log(label + ": OK"); } catch (err: any) { console.log(label + ": " + err.code + " " + err.syscall); } } function callbackCode( label: string, invoke: (callback: (err: any) => void) => void, ): Promise<void> { return new Promise((resolve) => { invoke((err: any) => { console.log(label + ": " + (err ? err.code + " " + err.syscall : "OK")); resolve(); }); }); } async function main(): Promise<void> { try { + const file = path.join(dir, "file.txt"); + fs.writeFileSync(file, "seed"); + fd = fs.openSync(file, "r"); + - syncCode("write string sync", () => fs.writeSync(fd, "x")); - syncCode("write buffer sync", () => fs.writeSync(fd, Buffer.from("x"), 0, 1, null)); - await callbackCode("write string callback", (callback) => fs.write(fd, "x", callback)); + syncCode("write string sync", () => fs.writeSync(fd!, "x")); + syncCode("write buffer sync", () => fs.writeSync(fd!, Buffer.from("x"), 0, 1, null)); + await callbackCode("write string callback", (callback) => fs.write(fd!, "x", callback)); await callbackCode("write buffer callback", (callback) => - fs.write(fd, Buffer.from("x"), 0, 1, null, callback), + fs.write(fd!, Buffer.from("x"), 0, 1, null, callback), ); } finally { - fs.closeSync(fd); + if (fd !== undefined) { + try { fs.closeSync(fd); } catch {} + } fs.rmSync(dir, { recursive: true, force: true }); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-files/test_gap_fs_write_error_propagation.ts` around lines 6 - 44, Move temporary directory, file, and descriptor setup into main’s try block so failures during fs.writeFileSync or fs.openSync still reach finally; initialize the descriptor safely and close it only when opened, then remove the directory during cleanup. Create the temporary directory with an os.tmpdir()-based prefix rather than relying on the current working directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test-files/test_gap_fs_write_error_propagation.ts`:
- Around line 6-44: Move temporary directory, file, and descriptor setup into
main’s try block so failures during fs.writeFileSync or fs.openSync still reach
finally; initialize the descriptor safely and close it only when opened, then
remove the directory during cleanup. Create the temporary directory with an
os.tmpdir()-based prefix rather than relying on the current working directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff1c0284-5193-4fa1-be2b-469d83785c8a
📒 Files selected for processing (3)
crates/perry-runtime/src/fs/callbacks.rscrates/perry-runtime/src/fs/fd_ops.rstest-files/test_gap_fs_write_error_propagation.ts
The propagation this PR adds worked, but the CODE was wrong: a write to a
closed descriptor reported
write string sync: EIO write (node: EBADF write)
`io_error_code` matches the raw errno first, but its table had no EBADF arm —
and Rust has no `ErrorKind` for it either, so it fell through to the catch-all
`_ => "EIO"`. `io_error_errno` already returned the raw errno, so only the code
string was lost.
Add EBADF plus the other descriptor/write-side errnos that hit the same hole
(EPIPE, EROFS, EFBIG, ESPIPE, EBUSY, EMFILE, ENFILE, EXDEV). Each only replaces
a wrong "EIO" with the correct code.
test_gap_fs_write_error_propagation is now byte-identical to node, and
test_gap_fs_fd_2749 / test_gap_fs_errprop_2735plus / test_gap_fs_errprop2_2745plus
/ test_gap_node_fs still pass.
|
Nice catch on the propagation — that was a real hole. CI surfaced one more thing on top of it, so I pushed a fix to your branch ( What failedYour new So the error did reach the callback — your change works. It just arrived wearing the wrong name. Why
code if code == libc::ETIMEDOUT => return "ETIMEDOUT",
code if code == libc::EAGAIN => return "EAGAIN",
_ => {} // <- EBADF falls through here…and Rust has no The fixAdded Verified locally against node:
Also merged |
2 participants
Summary
fs.writeSyncand callbackfs.writeforms.Root cause
The write cores converted every
std::io::Errorinto a zero-byte success. The synchronous and callback APIs therefore could not distinguish a failed write from a genuine zero-byte result.Checks
node --experimental-strip-types test-files/test_gap_fs_write_error_propagation.tsrustfmt --edition 2021 --check crates/perry-runtime/src/fs/fd_ops.rs crates/perry-runtime/src/fs/callbacks.rsgit diff --checkSummary by CodeRabbit
Bug Fixes
codemappings.Tests