Skip to content

Stop close, flush and remove from discarding their libc status - #16

Merged
hellerve merged 1 commit into
masterfrom
claude/report-close-and-remove-errors
Aug 23, 2026
Merged

Stop close, flush and remove from discarding their libc status#16
hellerve merged 1 commit into
masterfrom
claude/report-close-and-remove-errors

Conversation

@carpentry-agent

Copy link
Copy Markdown

File.close was (ignore (IO.Raw.fclose @(file &f))) and File.remove was
(ignore (IO.Raw.unlink (name f))). Both fclose and unlink are registered
in Carp core returning Int, and both returns went on the floor.

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 / EDQUOT failure surfaces only when fclose flushes — and that was
exactly the value being dropped. remove hid unlink's -1 the same way, so a
failed delete was indistinguishable from a successful one.

The downstream case

filelog/main.carp states a crash-on-failure contract:

; crashing is the contract: a logger that cannot open its file must not
; carry on as though it logged
(let-do [f (Result.unsafe-from-success (File.open-with FILENAME "a+"))]
  (ignore
    (Result.unsafe-from-success
      ; this forces a crash if we can’t write
      (File.write &f ...)))
  (File.close f))

That contract does not hold for write failures today. The unsafe-from-success
only sees the buffered write succeed; the flush error dies inside File.close
one line later, and the logger returns normally having logged nothing.

Design, and the constraint on close

close keeps its (Fn [File] ()) signature. using/main.carp declares
(definterface close (Fn [a] ())) and using/examples/file.carp does
(implements close File.close). I checked this rather than assuming it: making
close return a Result and re-running that example's shape against this branch
gives

[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(Fn [&File] (Result () String)) over IO.Raw.fflush. Forces
    the buffer out and reports whether it landed, while the caller still holds the
    file.
  • close-checked(Fn [File] (Result () String)). Takes ownership like
    close and returns the closing flush's status. close is now
    (ignore (close-checked f)).
  • remove — now returns (Result () String). It implements no interface and
    had exactly one call site org-wide.

Error messages follow open-with, read-all and write: a Result.Error
naming the file.

Call sites

close's signature is unchanged, so nothing that calls it is affected. I ran the
using RAII macro and the (do (File.close f) true) shape against this branch
and both compile and run:

Call site Affected?
using/examples/file.carp:4(implements close File.close) no
using/main.carpusing / using-do macros no
web/web.carp:508, 517, 3371File.close in do position no
filelog/main.carp:17File.close in do position no
test/file.carp — 9 × File.close no
test/file.carp:261File.remove yes, updated here

remove is the one signature change, and it is the only File.remove call site
in the whole org. It also cannot break anyone silently: Carp rejects a discarded
non-unit value in statement position outright —

I can’t match the types `(Result d String)` and `()`
Statement in do-expression : ()

— 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 fails
the flush. That pins the actual bug rather than the happy path: File.write
returns Result.Success and flush / close-checked return an error on the
same file, in the same test.

  • write succeeds into the buffer but flush reports the failed write
  • write succeeds into the buffer but close-checked reports the failure
  • remove reports an error for a file that is already gone
  • a failed remove names the file

I mutation-checked all four — reverting each function to discard its status
(i.e. the behaviour on master) fails exactly the corresponding test and nothing
else.

Coverage caveat: /dev/full is Linux-only, so the two flush/close tests are
guarded by a probe and skip on the macOS CI leg, printing
(skipped, no unwritable device available). That follows the guards already in
this 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 remove tests are unguarded and run everywhere.

Also in here

  • Module doc string and README.md gained the buffering caveat; the existing
    claim that these calls "return a Result.Error if they can’t" was true of the
    readability check but read as a guarantee that a Success from write meant
    the data landed.
  • Doc strings for close and remove updated; flush and close-checked
    documented.
  • docs/ regenerated, with index.html kept as the byte-identical copy of
    File.html.
  • Typo non-existantnon-existent on test/file.carp:135 and :139.

Not touched, and worth a separate pass if you want it: nonexistant appears 8
more times in file.carp's walk/open doc strings. I left those alone to keep
this 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.carp 39/39 pass; carp-fmt --check and
angler clean over the CI file glob; carp -x gendocs.carp clean. Angler was
rebuilt from its current master for this, since it picked up
unused-defn-parameter and the leading-- discard marker after my local binary
was built — the -first binding in remove-again uses that new spelling.


Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.

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.

@carpentry-reviewer carpentry-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build & Tests

carp -x test/file.carp39 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.carp and src/byte-writer.carp both do check fwrite/fputc, so the hole really was only the buffered case. Confirmed on /dev/full: a 20 000-byte write fails in fwrite and File.write reports it today; a 5-byte write returns Success and the failure only appears at flush/close. That is exactly the gap being closed.
  • close's signature really is unaffected. I compiled using/main.carp's using-do RAII macro plus (implements close File.close) against this branch and ran it — (Success @"hi"). The interface still matches.
  • File.remove has no other call site. Swept all 47 clones: zero File.remove outside this repo, and the five File.close sites (web ×3, using/examples, filelog) are all in do position or an implements, so none are touched.
  • docs/index.html is byte-identical to docs/File.html, per this org's convention.
  • -first is the right discard spelling. angler's HEAD is bb48b50 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.

@hellerve
hellerve merged commit 141da0d into master Aug 23, 2026
2 checks passed
@hellerve
hellerve deleted the claude/report-close-and-remove-errors branch August 23, 2026 01:28
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