Stop close, flush and remove from discarding their libc status - #16
Conversation
File.close was (ignore (IO.Raw.fclose ...)) and File.remove was (ignore (IO.Raw.unlink ...)), so both threw away the only status the operating system ever reports. close is the one that costs data. File.write hands its bytes to stdio's buffer and returns Result.Success long before anything reaches the disk, so an ENOSPC, EIO or EDQUOT failure surfaces only when fclose flushes -- and that was exactly the value being dropped. A caller could write, get a Success, close, and never learn the write did not land. remove hid unlink's -1 the same way, so a failed delete was indistinguishable from a successful one. close keeps its (Fn [File] ()) signature: carpentry-org/using declares (definterface close (Fn [a] ())) and its examples/file.carp does (implements close File.close) for RAII-style scoped closing. Returning a Result there fails to compile with [INTERFACE ERROR] File.close : (Fn [File] (Result () String)) doesn't match the interface signature (Fn [a] ()) so the failure is made reachable additively instead: - flush forces the buffer out and reports whether it landed, so a caller can check while it still holds the file - close-checked takes ownership like close and returns the closing flush's status; close is now (ignore (close-checked f)) - remove returns (Result () String); it implements no interface and had one call site org-wide, this repo's own test Error messages follow open-with, read-all and write: a Result.Error naming the file. The new tests write to /dev/full, which accepts a buffered write and then fails the flush, pinning that File.write reports success while flush and close-checked report the failure. /dev/full is Linux-only, so they are guarded and skip on the macOS CI leg, following the existing guards for unreadable directories and named pipes.
There was a problem hiding this comment.
Build & Tests
carp -x test/file.carp — 39 passed, 0 failed on this armhf Pi, and both CI legs are green. Branch b7a30d7, based on 433f9db (docs: bump to 0.3.0), which is origin/master's head — so it sits after the release commit and nothing is mis-filed.
I checked the load-bearing claims rather than taking them from the body:
- The diagnosis is right.
src/string-writer.carpandsrc/byte-writer.carpboth do checkfwrite/fputc, so the hole really was only the buffered case. Confirmed on/dev/full: a 20 000-byte write fails infwriteandFile.writereports it today; a 5-byte write returnsSuccessand the failure only appears at flush/close. That is exactly the gap being closed. close's signature really is unaffected. I compiledusing/main.carp'susing-doRAII macro plus(implements close File.close)against this branch and ran it —(Success @"hi"). The interface still matches.File.removehas no other call site. Swept all 47 clones: zeroFile.removeoutside this repo, and the fiveFile.closesites (web×3,using/examples,filelog) are all indoposition or animplements, so none are touched.docs/index.htmlis byte-identical todocs/File.html, per this org's convention.-firstis the right discard spelling. angler's HEAD isbb48b50 Mark a deliberate discard with a leading dash, not an underscore(2026-08-21), so this matches the current rule, not the old_one.
Findings
1. The sticky stream error is not consulted, so close-checked can report success on a stream that lost data
file.carp:269-287. Both flush and close-checked look only at the return of their own fflush/fclose call. fflush's failure is not sticky — the stream's error indicator is. So once a flush has failed, the next check comes back clean:
write 5 bytes to /dev/full : Success (buffered)
ferror(fp) : 0
flush : Error(The file "/dev/full" could not be flushed)
ferror(fp) : 1 <- stream is in an error state, data is gone
close-checked : Success <- reports OK on a stream that lost data
The tests don't catch it because close-dev-full goes write → close-checked with no flush in between, which is the path that works.
Worth being precise about how reachable this is: flush returns a (Result () String), and Carp refuses to discard a non-unit value in statement position, so a caller has to write (ignore (File.flush &f)) deliberately to get here. But that is a normal shape — flush periodically without handling each result, check once at close — and it is the shape filelog would grow into. A function whose whole job is reporting whether the data landed giving the wrong answer there is worth closing.
The fix is two lines, and folding ferror in works; I tried it (reading ferror before fclose, since the stream is invalid afterwards):
flush-then-close on /dev/full : flush Error, close-checked Error (was: Success)
close only on /dev/full : close-checked Error (unchanged)
happy path on a real file : flush Success, close-checked Success
read-only file : flush Success, close-checked Success
No regression on the happy path or on a read-only handle.
2. The two /dev/full tests are vacuous on the macOS leg
test/file.carp:300-315. The skip branch returns the expected value, so on macOS the assertion becomes [false true] against [false true] and passes for free. I confirmed from the CI log that this is what happens — the macOS job prints (skipped, no unwritable device available) twice and still reports Passed: 39. Locally both tests genuinely run (no skip message printed), so the coverage does exist on Linux.
This is disclosed in the PR body and it follows the guard convention already in this file, so I am not asking for a change — just flagging that "39/39 on both legs" reads as more coverage than the macOS leg actually has.
3. Minor: flush doesn't guard on mode
file.carp:269. write and read both check writable?/readable? first; flush doesn't. Not a bug — fflush on a read handle returns 0 here and close-checked stays Success (I checked) — just an inconsistency with the module's own convention.
Verdict: merge
Everything this PR claims, it does, and I could not find a case where the new error paths report the wrong thing on the sequences the tests and README describe. Finding 1 is a real wrong answer from a new public function, but it needs a deliberate (ignore …) to reach, it is a pre-existing stdio characteristic rather than anything this PR introduced, and the branch is a strict improvement over master with or without it — so I'd take this as-is and treat the ferror fold as a follow-up. If you'd rather have it in this PR, the change above is validated and small.
File.closewas(ignore (IO.Raw.fclose @(file &f)))andFile.removewas(ignore (IO.Raw.unlink (name f))). Bothfcloseandunlinkare registeredin Carp core returning
Int, and both returns went on the floor.closeis the one that costs data.File.writehands its bytes to stdio'sbuffer and returns
Result.Successlong before anything reaches the disk, so anENOSPC / EIO / EDQUOT failure surfaces only when
fcloseflushes — and that wasexactly the value being dropped.
removehidunlink's-1the same way, so afailed delete was indistinguishable from a successful one.
The downstream case
filelog/main.carpstates a crash-on-failure contract:That contract does not hold for write failures today. The
unsafe-from-successonly sees the buffered write succeed; the flush error dies inside
File.closeone line later, and the logger returns normally having logged nothing.
Design, and the constraint on
closeclosekeeps its(Fn [File] ())signature.using/main.carpdeclares(definterface close (Fn [a] ()))andusing/examples/file.carpdoes(implements close File.close). I checked this rather than assuming it: makingclosereturn aResultand re-running that example's shape against this branchgives
So the failure is made reachable additively instead:
flush—(Fn [&File] (Result () String))overIO.Raw.fflush. Forcesthe buffer out and reports whether it landed, while the caller still holds the
file.
close-checked—(Fn [File] (Result () String)). Takes ownership likecloseand returns the closing flush's status.closeis now(ignore (close-checked f)).remove— now returns(Result () String). It implements no interface andhad exactly one call site org-wide.
Error messages follow
open-with,read-allandwrite: aResult.Errornaming the file.
Call sites
close's signature is unchanged, so nothing that calls it is affected. I ran theusingRAII macro and the(do (File.close f) true)shape against this branchand both compile and run:
using/examples/file.carp:4—(implements close File.close)using/main.carp—using/using-domacrosweb/web.carp:508, 517, 3371—File.closeindopositionfilelog/main.carp:17—File.closeindopositiontest/file.carp— 9 ×File.closetest/file.carp:261—File.removeremoveis the one signature change, and it is the onlyFile.removecall sitein the whole org. It also cannot break anyone silently: Carp rejects a discarded
non-unit value in statement position outright —
— so any out-of-tree caller gets a compile error, not a wrong result.
Tests
The new tests write to
/dev/full, which accepts a buffered write and then failsthe flush. That pins the actual bug rather than the happy path:
File.writereturns
Result.Successandflush/close-checkedreturn an error on thesame file, in the same test.
write succeeds into the buffer but flush reports the failed writewrite succeeds into the buffer but close-checked reports the failureremove reports an error for a file that is already gonea failed remove names the fileI mutation-checked all four — reverting each function to discard its status
(i.e. the behaviour on
master) fails exactly the corresponding test and nothingelse.
Coverage caveat:
/dev/fullis Linux-only, so the two flush/close tests areguarded by a probe and skip on the macOS CI leg, printing
(skipped, no unwritable device available). That follows the guards already inthis file for unreadable directories and named pipes. On this box the guard does
not fire and both tests genuinely run — I verified no skip message is printed.
The
removetests are unguarded and run everywhere.Also in here
README.mdgained the buffering caveat; the existingclaim that these calls "return a
Result.Errorif they can’t" was true of thereadability check but read as a guarantee that a
Successfromwritemeantthe data landed.
closeandremoveupdated;flushandclose-checkeddocumented.
docs/regenerated, withindex.htmlkept as the byte-identical copy ofFile.html.non-existant→non-existentontest/file.carp:135and:139.Not touched, and worth a separate pass if you want it:
nonexistantappears 8more times in
file.carp's walk/open doc strings. I left those alone to keepthis diff about the error propagation.
No CHANGELOG in this repo, so none added. I also did not bump the version —
that has been a separate commit of yours on the previous three.
Verification:
carp -x test/file.carp39/39 pass;carp-fmt --checkandanglerclean over the CI file glob;carp -x gendocs.carpclean. Angler wasrebuilt from its current
masterfor this, since it picked upunused-defn-parameterand the leading--discard marker after my local binarywas built — the
-firstbinding inremove-againuses that new spelling.Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.