Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,21 @@ because it turns other people's test suites red.

### Fixed

- **`verify` no longer calls a half-written manifest a file it knows nothing
about.** A run killed outright can leave `<manifest>.tfg-writing` behind.
`verify` reported it as `extra`, the word it uses for a file somebody else put
in the directory, so the reader was told their fixtures were polluted by
something the tool had written itself.

It is now reported as a leftover, with the sentence that case needs: the run
was saving the list of what it produced, so the directory may hold finished
files that nothing lists - and `cleanup` cannot remove those, because it
removes only what a manifest names. That is the one case worth looking at
rather than just deleting.

Half-written files from the same run were already reported this way. This was
the second marker, and nothing on the reading side had been told about it.

- **`verify` and `cleanup` read each file in larger pieces**, which takes about
a fifth off the time spent hashing.

Expand Down
19 changes: 18 additions & 1 deletion internal/audit/audit.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,18 @@ func (d Difference) String() string {
case Unreadable:
return fmt.Sprintf("unreadable %s - %s", d.Path, d.Got)
case Leftover:
// Two markers reach this, and one sentence cannot serve both. The
// first is a file that was being produced, so nothing is lost. The
// second is a RECORD that was being saved, so the useful thing to say
// is that the directory may hold files nothing lists - which is the
// one case where a person has to look rather than just delete.
if core.IsWritingName(filepath.Base(d.Path)) {
return fmt.Sprintf(
"leftover %s\n a run's record that was not finished being saved, from a run that was "+
"stopped before it could tidy up. The directory may hold files that nothing lists, and cleanup "+
"cannot remove those - check what is here against what you expected before deleting this by hand",
d.Path)
}
return fmt.Sprintf(
"leftover %s\n an unfinished file from a run that was stopped before it could tidy up. "+
"Nothing described by this manifest is missing because of it. "+
Expand Down Expand Up @@ -250,7 +262,12 @@ func Verify(ctx context.Context, dir string, m *manifest.Manifest, skip string)
kind := Extra
want := ""
switch {
case core.IsPartialName(filepath.Base(p)):
case core.IsPartialName(filepath.Base(p)), core.IsWritingName(filepath.Base(p)):
// Both markers, because both name a file this tool started and did
// not finish. Only the first was recognised until 2026-09-06, so a
// half written manifest was reported as "extra" - the word that
// means somebody else put it here. They get different sentences in
// String, because what a reader should do about them differs.
kind = Leftover
default:
// One file under two spellings reads as a polluted directory
Expand Down
34 changes: 34 additions & 0 deletions internal/core/limits.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,40 @@ func IsPartialName(name string) bool {
return strings.Contains(name, PartialMarker)
}

// WritingMarker is what a file being REPLACED is called while it is filled.
//
// A different job from PartialMarker, and the two are not interchangeable. That
// one marks a file this run is producing, under a name nobody held before. This
// one marks the half written copy of a file that already exists and belongs to
// somebody - the manifest being rewritten over an earlier one, or the recipe
// that "recipe fmt -w" is formatting in place. The full name is
// "<final>.tfg-writing", with no process id, because the name is claimed
// exclusively rather than made unique.
//
// Declared here beside PartialMarker on 2026-09-06, and the argument for it is
// the one already written above: two parts of the tool have to agree on the
// spelling, the writing side and the reading side. Until that day this marker
// had TWO spellings and no reader at all - an unexported constant in
// core/replace.go and a bare literal in manifest.go - so verify reported our
// own half written manifest as "extra", the word that means somebody else put
// it here. That is exactly the failure the comment on PartialMarker describes
// as fixed in 2026-08-03, arriving a second time through the other marker.
const WritingMarker = ".tfg-writing"

// IsWritingName says whether a file name is one of ours, left behind by a
// replacement that did not finish.
//
// A suffix rather than a contained string, which is the difference from
// IsPartialName: that one has a process id after it, this one ends the name.
//
// Reaching one of these needs the process to die between the create and the
// rename, because every error path in the writers removes it. That is a hard
// kill, a CI timeout that outruns the grace period, or power loss - the same
// conditions PartialMarker exists for.
func IsWritingName(name string) bool {
return strings.HasSuffix(name, WritingMarker)
}

// AddSizes adds one file size to a running total and says when the total has
// left the range it is measured in.
//
Expand Down
6 changes: 5 additions & 1 deletion internal/core/replace.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ import (
// Beside the target rather than in the system temporary directory, because a
// rename across volumes is not one operation and the whole point of this is to
// have one.
const writingSuffix = ".tfg-writing"
//
// Reads WritingMarker rather than spelling it again. It was spelled again here
// until 2026-09-06, and a second copy of it sat in manifest.go as a bare
// literal, so nothing on the reading side knew either of them.
const writingSuffix = WritingMarker

// ReplaceFile puts new content in place of a file somebody else owns.
//
Expand Down
115 changes: 115 additions & 0 deletions internal/guard/leftovers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,3 +98,118 @@ func TestAFileNobodyAskedForIsStillReportedAsExtra(t *testing.T) {
// reach the behaviour makes cleanup walk more manifest entries, and a leftover
// is in no entry at all, so the runner reported NOT CAUGHT: the guard was
// asserting against a capability the code does not have.

// The other marker, and it went unrecognised for as long as it existed.
//
// A file being produced is called "<final>.tfg-partial-<pid>". A file being
// REPLACED - the manifest written over an earlier one, or the recipe that
// "recipe fmt -w" formats in place - is called "<final>.tfg-writing". Both are
// ours and both outlive a hard kill, and until 2026-09-06 the reading side knew
// only the first. So verify reported our own half written manifest as "extra",
// which is the word that means somebody else put it here.
//
// That is the same defect the comment on core.PartialMarker describes as fixed
// on 2026-08-03, arriving a second time through the other marker - which is the
// argument for the source level guard below rather than for this one alone.
//
// The sentence differs from the one above it deliberately. A half written
// generated file costs nothing: the manifest does not describe it and nothing
// is missing. A half written RECORD is the case where a person has to look,
// because the run was saving the list of what it produced, so the directory can
// hold finished files that nothing lists and that cleanup therefore cannot
// remove - untouchable rule 7.
func TestAHalfWrittenRecordIsNamedForWhatItIs(t *testing.T) {
out, mf := generated(t)

leftover := filepath.Join(out, filepath.Base(mf)+".tfg-writing")
if err := os.WriteFile(leftover, []byte("{\"manifest_v"), 0o644); err != nil {
t.Fatalf("writing the leftover: %v", err)
}

code, _, errOut := run(t, "verify", mf)

if code != cli.ExitVerify {
t.Errorf("exit %d, expected %d - a leftover is still a difference:\n%s", code, cli.ExitVerify, errOut)
}
if strings.Contains(errOut, "extra "+filepath.Base(leftover)) {
t.Errorf("our own half written manifest is reported as an ordinary extra file, which "+
"tells the reader nothing and reads as a polluted directory:\n%s", errOut)
}
// Not the other marker's sentence. Both reach the same kind, so a build
// that ran them through one branch would pass every check above while
// telling somebody their record is an ordinary unfinished file.
if strings.Contains(errOut, "an unfinished file") {
t.Errorf("the half written record got the sentence written for a half written "+
"GENERATED file, which does not mention the files that may be unlisted:\n%s", errOut)
}
if !strings.Contains(errOut, "record") {
t.Errorf("the report does not say the file is a run's record:\n%s", errOut)
}
if !strings.Contains(errOut, "nothing lists") {
t.Errorf("the report does not say the directory may hold files nothing lists, which is "+
"the whole reason this case is worse than the one above:\n%s", errOut)
}
}

// Each marker is spelled in exactly one place.
//
// This is the guard that would have caught the defect above, and the one above
// would not have caught it coming back. core.PartialMarker carries the argument
// in its own comment - "two parts of the tool have to agree on it: the engine
// writes it, and the reading side has to recognise one that outlived its run. A
// second spelling would mean verify reports our own leftovers as files it knows
// nothing about" - and that argument was written down, applied to one marker,
// and then the other marker was introduced with the literal duplicated and no
// reader taught about it.
//
// A written reason is a claim until something can turn red on it.
//
// Asked of quoted Go literals rather than of the text, so the prose in
// core/createnew.go that names these files while explaining what it defends
// against is not a hit.
func TestEachLeftoverMarkerIsSpelledInOnePlace(t *testing.T) {
root := repoRoot(t)
home := filepath.Join("internal", "core", "limits.go")

for _, marker := range []string{".tfg-partial-", ".tfg-writing"} {
literal := "\"" + marker + "\""
var found []string
for _, dir := range []string{"internal", "cmd"} {
err := filepath.WalkDir(filepath.Join(root, dir), func(path string, d os.DirEntry, err error) error {
if err != nil || d.IsDir() || !strings.HasSuffix(path, ".go") {
return err
}
if strings.HasSuffix(path, "_test.go") {
return nil
}
body, err := os.ReadFile(path)
if err != nil {
return err
}
if strings.Contains(string(body), literal) {
rel, _ := filepath.Rel(root, path)
found = append(found, rel)
}
return nil
})
if err != nil {
t.Fatalf("walking %s: %v", dir, err)
}
}

// Asserted rather than assumed. Nought occurrences would mean the
// constant was renamed and this guard is now checking a marker nothing
// uses, which is a green that proves nothing.
if len(found) == 0 {
t.Fatalf("%s is spelled nowhere in the shipped code, so this guard checked nothing. "+
"If the marker was renamed, rename it here too.", marker)
}
if len(found) == 1 && found[0] == home {
continue
}
t.Errorf("%s is spelled in %d place(s): %v\n"+
"What to do: declare it once in %s and read it everywhere else. A second spelling "+
"is how the reading side came to not recognise a file this tool wrote itself.",
marker, len(found), found, home)
}
}
6 changes: 5 additions & 1 deletion internal/manifest/manifest.go
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,11 @@ func holdsACredential(props map[string]any) bool {
// "the claim goes when the write does", and a rule spelled once cannot be
// half applied.
func (m *Manifest) writeOver(path string) error {
tmp := path + ".tfg-writing"
// The marker comes from core rather than being spelled here. It was a bare
// literal until 2026-09-06, which is how verify came to report our own half
// written manifest as "extra" - the reading side recognised the other
// marker and had never been told about this one.
tmp := path + core.WritingMarker
// Claimed rather than created, and core.CreateNew says why: this name sits
// in a directory the run does not own, nothing else in the tool checks it,
// and a create that is not exclusive follows whatever is at the name.
Expand Down
Loading