diff --git a/CHANGELOG.md b/CHANGELOG.md index 17a85a3..3ce7936 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 `.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. diff --git a/internal/audit/audit.go b/internal/audit/audit.go index 27fca96..0930872 100644 --- a/internal/audit/audit.go +++ b/internal/audit/audit.go @@ -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. "+ @@ -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 diff --git a/internal/core/limits.go b/internal/core/limits.go index 4e7c08d..5ac3e88 100644 --- a/internal/core/limits.go +++ b/internal/core/limits.go @@ -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 +// ".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. // diff --git a/internal/core/replace.go b/internal/core/replace.go index afacace..195d294 100644 --- a/internal/core/replace.go +++ b/internal/core/replace.go @@ -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. // diff --git a/internal/guard/leftovers_test.go b/internal/guard/leftovers_test.go index 33fc15a..a672d03 100644 --- a/internal/guard/leftovers_test.go +++ b/internal/guard/leftovers_test.go @@ -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 ".tfg-partial-". A file being +// REPLACED - the manifest written over an earlier one, or the recipe that +// "recipe fmt -w" formats in place - is called ".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) + } +} diff --git a/internal/manifest/manifest.go b/internal/manifest/manifest.go index 45604b8..b7dc856 100644 --- a/internal/manifest/manifest.go +++ b/internal/manifest/manifest.go @@ -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.