Repository navigation
Write JSON, CSV and the sidecar atomically - #16
Merged
Merged
Conversation
The audit's second finding, and real. open(path, "w") truncates before a single byte is written, so a crash, a kill or a full disk partway through json.dump leaves a SHORTER file where a complete one was. Reproduced here: a 204,292-byte out.json came back 28 bytes and invalid. That breaks the same promise save keeps when it refuses to overwrite good output with an empty result, by a different route — the previous good run destroyed by the ATTEMPT to replace it rather than by its outcome. The sidecar is the half the audit does not mention and the one that matters most: <out>.meta.json is the file a consumer branches on, so a truncated one beside good rows reads as a broken run over data that is fine. All three writers go through one _atomic: temp file in the target's OWN directory (os.replace is atomic only within one filesystem), flush + fsync before the rename, and the temp file unlinked on any exception including KeyboardInterrupt. newline="" is threaded through for the csv module. Lifted from a sibling rather than written here, because the part that is easy to get wrong was got wrong across the family: NamedTemporaryFile creates its file 0600 and a rename keeps that. Measured 2026-10-09 by CALLING each sibling's writer and stat-ing the result — 8 of 11 leave the output 0600, one hardcodes 0644, two derive it from the umask. This takes the third: a new file gets what open() would have given it, and a target somebody tightened keeps its own mode. Five faults planted, suite red on a named check for each. The suite also carries its own control inline: it asserts a truncating writer really does destroy the file before asserting this one does not. One control stayed green first time and is reported rather than smoothed over: dropping newline="" changes nothing on Linux, so that behavioural check cannot fail on CI. The byte assertion is kept and labelled as a sanity check; the guard that can fail is the wiring one beside it. Not included, with the reasoning in the PR: a run identifier for the JSON/CSV/sidecar set (a failed run already writes no sidecar at all), and CSV formula neutralisation (measured 0 of 3,620 string cells on a real run, and a separate decision). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The audit's second finding, worked. Real, and the half it does not mention is the one that matters most.
The defect, reproduced
open(path, "w")truncates before a single byte is written, so a crash, a kill or a full disk partway throughjson.dumpleaves a shorter file where a complete one was. Measured here, by writing a good run and then interrupting the next one mid-dump:That breaks the same promise
savealready keeps when it refuses to overwrite good output with an empty result — by a different route, with the previous good run destroyed by the attempt to replace it rather than by its outcome.The sidecar is the part the audit did not mention and the part that matters most.
<out>.meta.jsonis the file a consumer branches on, so a truncated one beside good rows reads as a broken run over data that is fine.Lifted, not written
All three writers now go through one
_atomic: temp file in the target's own directory (os.replaceis atomic only within one filesystem — a temp file under/tmpcan be on another, where the rename degrades to an interruptible copy),flush+fsyncbefore the rename, and the temp file unlinked on any exception includingKeyboardInterrupt.It is taken from a sibling rather than written here, because the one part that is easy to get wrong has been got wrong across the family.
NamedTemporaryFilecreates its file0600and a rename keeps that, so an output nobody else can read is the silent default.Measured 2026-10-09 by calling each sibling's writer and stat-ing the file it produced, not by grepping for a umask line:
06000644This takes the third kind: a new file gets what
open()would have given it, and a target somebody tightened deliberately keeps its own mode.Verification
Five faults planted, suite required to go RED on a named check:
write_jsonback to a truncating openthe previous good file survives an interrupted write(+1)write_run_meta() writes through _atomicNamedTemporaryFile's 0600a new output is readable, not 0600 (got 0o600 under umask 022)...and no half-written temp file is left beside itnewline=""dropped on the CSVThe suite also carries its own control inline: it asserts that a truncating writer really does destroy the file before asserting that this one does not, so "the file survived" cannot quietly become a property of the test.
The fifth control stayed green, and that is reported rather than smoothed over. Dropping
newline=""changes nothing on Linux — the doubled carriage return only appears whereos.linesepis\r\n— so the behavioural check cannot fail on CI however it is written. That is a hole in the suite, not a pass. The byte-level assertion is kept as a sanity check and labelled as one, and the guard that can actually fail is now the wiring:write_csv()must handnewline=""to_atomic.What is NOT changed, and why
No run identifier for the JSON/CSV/sidecar set. The atomic rename is right and is taken. The transaction id is solving a problem this contract already solves by construction: a failed run writes no sidecar at all, and
saverefuses to overwrite good output with an empty result. Adding an id would add a mechanism to enforce something that is enforced by not writing — and a second thing to keep consistent is a second thing that can disagree.What the audit is right about underneath that recommendation is the failure window, and
os.replacecloses it: each file is either the previous run's or this one's, never half of either.CSV formula neutralisation is not included. It is a known family TODO and the audit did not raise it. Measured on a real 213-row run before deciding: 0 of 3,620 string cells are formula-shaped. It is worth shipping on its own merits and is not worth bundling into a fix for a different problem.
🤖 Generated with Claude Code