fix: one directory holds one run at a time - #79
Merged
Conversation
Two runs writing into one directory used to write over each other's files and
both report that they had succeeded.
Measured with two processes started on the same wall clock instant, sixty files
of 200 kB each, eight times:
both ended 0 2 of 8 120 files reported, 60 on the disk
both ended 8 5 of 8 partial, and only because Windows refuses to
rename onto a file another process holds open
one ended 5 1 of 8 the preflight happened to see the other run
In the first case verify against the first run's manifest reported sixty wrong
hashes about a run that had been told it succeeded. The middle case is not a
defence: it is a property of one system, and Linux renames onto a file another
process has open without complaint.
The protection already existed and was keyed to the wrong thing. A run claims
its manifest name before the first file and keeps it until it ends, so two runs
both writing manifest.json into one directory have always been refused -
measured the same day, four times out of four. output.manifest is the one way
out of that claim, and it was never meant to be a way out of this.
So this widens the key of the claim that is already there rather than adding a
second mechanism. A run takes core.RunLockName in the output directory, holds
it for its whole length, and gives it back however it ends, interruption
included. The preflight asks about it before it asks about the manifest name,
which is what a dry run needs, and is also the better sentence: a second run
used to be told that manifest.json already exists and is the only record of
what an earlier run wrote, about a run that was still going.
Eight of eight refused after the change.
verify names the lock as ours rather than as a file somebody else left, which
makes it the third marker to need that repair after the partial and the writing
ones. Its sentence holds both endings open, because it is the only one of the
three that may belong to a run still in flight.
The cost is stated in the refusal, in the changelog and in the document. A run
killed outright cannot give the name back, so the next run into that directory
is refused until a person deletes the file. And two runs can no longer fill one
directory at the same time, even when the names they write do not collide - for
every run that does not set output.manifest, that was already true.
Six guards, six mutations. Two of the mutations found faults in the guards
rather than in the code: the stopped-run guard cancelled BEFORE the run, so the
lock was never taken and "it is gone afterwards" was true of nothing, and a
seventh guard proved exactly what the first one proves, because the lock never
sees the manifest name at all.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Two runs writing into one directory could write over each other's files, and
both report that they had succeeded.
What was measured
Two processes started on the same wall clock instant, sixty files of 200 kB
each, the same name template, different
output.manifest, eight times:085In the first case,
tfg verifyagainst the first run's manifest reportedsixty wrong hashes about a run that had been told it succeeded.
The middle row is not protection. Windows refuses to rename onto a file another
process holds open, and Linux does not, so the same eight attempts on Linux
would be silent seven times rather than twice.
Starting the two runs with
&in a shell does not reproduce this. Thesecond process is milliseconds behind and its preflight sees the first run's
files. Both have to be released on a shared timestamp.
Why the fix is small
The protection already existed and was attached to the wrong thing. A run claims
its manifest name before the first file and keeps it until it ends, so two runs
both writing
manifest.jsoninto one directory have always been refused -measured the same day, four times out of four.
output.manifestis the oneway out of that claim, and it was never meant to be a way out of this.
So this widens the key of the claim that is already there:
core.RunLockNamein the output directory, holds it for its wholelength, and gives it back however it ends, interruption included,
run needs, and it is also the truer sentence: a second run used to be told
that
manifest.jsonalready exists and is the only record of what an earlierrun wrote, about a run that was still going,
verifynames the lock as ours rather than as a file somebody else left. Thatmakes it the third marker to need this repair, after the partial and the
writing ones, and its sentence holds both endings open because it is the only
one of the three that may belong to a run still in flight.
Eight of eight refused after the change.
The cost, stated in the refusal and in the changelog
directory is refused until a person deletes
.tfg-run-lock. The refusal namesthe file.
cleanupwill not remove it, because it removes only what a manifestlists.
names they write do not collide. For every run that does not set
output.manifest, that was already true.Checking whether the process id in a stale lock is still alive was considered
and refused: process ids are reused, and on a network share the question is
meaningless. A wrong "this lock is dead" reproduces exactly the data loss this
closes.
Guards
Six guards in
internal/guard/runlock_test.go, six mutations, all caught. Twoof the mutations found faults in the guards rather than in the code:
preflight refused, the lock was never taken, and "it is gone afterwards" was
true of nothing. It now cancels from inside the run and asserts the lock was
held at that moment.
never sees the manifest name at all. Folded into one rather than given an
excuse on the not-proven list.
staleness.pythen found three existing mutations this change hadinvalidated - two patterns in
audit.goand one that stopped being unique inengine.go. Repaired, and every pattern occurs exactly once again.Full suite green,
preflight --quickgreen on all twelve checks.🤖 Generated with Claude Code