From 9ce6fb9de7591c4f1a244eaa945e92027e80c58d Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Sat, 5 Sep 2026 22:59:33 +0200 Subject: [PATCH] perf: one listing instead of two questions a file, and one collection instead of two Three findings from the performance report, and two of them came back with the report's own suggestion measured and refused. preflight asked the filesystem twice for every planned file. It reads the output directory once. A run of 100 000 files with --dry-run goes from 15.7-19.7 s to 0.49-0.56 s, order alternated - the check was 97% of it. Ten thousand names measured on their own: 1.937 s of stat calls against 8.9 ms for one listing. That is also a fix rather than only a speedup, and the guard for it says so. os.Stat follows a link, so a link pointing at nothing answered "no name here" and the run replaced it without a word. A directory ENTRY is what a taken name is, whatever it points at. With the old question put back, the new guard reports that the run went ahead over a name somebody else's link was holding. A directory that cannot be LISTED is still asked about file by file. Both systems allow write permission without read, a run into such a directory has always worked, and reading nothing there and calling it empty would let the run write over what is inside. That fallback has its own guard, which skips on Windows because denying a listing there needs an ACL. The plan ceiling forced a collection to take every reading, so a run of one kilobyte paid for two of them - measured with GODEBUG=gctrace=1, exactly two on every run however small. It asks /gc/heap/allocs:bytes first, at 251 ns against 519 us, and only collects when that says it might be over. The shortcut is sound by an inequality rather than by an estimate: the live heap cannot have grown by more than has been allocated. The report asked for /gc/heap/live:bytes and that metric is WRONG here. It reports the heap as of the last collection, and measured on 2026-09-05 all four existing ceiling guards stay green with it, because 25 MB of allocation makes the collector run on its own and the lagging reading catches up by luck. With the collector switched off the luck goes: a plan six times the ceiling is accepted. The new guard turns the collector off for exactly that reason. hashFile reads in 256 KiB pieces. The report asked for a 1 MB buffer through io.CopyBuffer and that does nothing at all: os.File implements io.WriterTo, so CopyBuffer hands it the whole job and throws the buffer away - 128 KiB, 256 KiB and 1 MiB with a plain file all take the same 167-172 ms that io.Copy takes. Hidden behind a reader that offers only Read, 256 KiB takes 161 ms against 199. The size is measured too: 64 KiB is 182 ms and nothing above a quarter of a megabyte can be told apart, so sixteen workers cost four megabytes. planChildren is sized up front, since the total is known from the groups. TotalBytes being walked twice is NOT done, and that is a measurement rather than an oversight: one walk over 100 000 planned files has a median of 0 s and a maximum of 541 us, so two of them cost half a millisecond of a nineteen second run. Widening a signature for that would be a change nothing can see. engine.go went past the length ceiling, and the guard asks for a split by what the parts do rather than for a bigger number - so preflight and the questions it asks about names are their own file now. Two ceilings came down with it. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 21 +++ internal/audit/audit.go | 42 +++++- internal/audit/cleanup.go | 8 +- internal/audit/parallel.go | 19 ++- internal/engine/engine.go | 122 ------------------ internal/engine/planmemory.go | 107 ++++++++++++--- internal/engine/preflight.go | 207 ++++++++++++++++++++++++++++++ internal/format/zip/children.go | 10 +- internal/guard/branching_test.go | 4 +- internal/guard/codeshape_test.go | 4 +- internal/guard/planmemory_test.go | 48 +++++++ internal/guard/safety_test.go | 115 +++++++++++++++++ 12 files changed, 549 insertions(+), 158 deletions(-) create mode 100644 internal/engine/preflight.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 72b7aeb..64e687d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,27 @@ because it turns other people's test suites red. Small runs are unaffected either way. A handful of files was already instant and still is. +- **Starting a run of many files no longer spends most of its time deciding + whether it may start.** Before writing anything, `tfg` checks that it is not + about to write over something of yours. That check asked the filesystem two + questions for every file it planned. It reads the folder once instead. + + Measured on a run of 100 000 files: `--dry-run` went from 15.7-19.7 seconds to + 0.49-0.56 seconds. The check was 97% of it. + +- **A name held by a shortcut that points at nothing now counts as taken.** The + check followed the shortcut, found nothing at the other end, and replaced it + without a word. It asks what the folder holds now, so anything already sitting + under a name your run wants is refused, whatever it points at. + +### Fixed + +- **`verify` and `cleanup` read each file in larger pieces**, which takes about + a fifth off the time spent hashing. + +- **Every run used to pause twice to tidy memory**, however small it was. It + pauses once. Nothing about what a run produces changes. + ## [0.3.0-rc1] - 2026-09-03 ### Breaking diff --git a/internal/audit/audit.go b/internal/audit/audit.go index 78a36cd..27fca96 100644 --- a/internal/audit/audit.go +++ b/internal/audit/audit.go @@ -219,8 +219,8 @@ func Verify(ctx context.Context, dir string, m *manifest.Manifest, skip string) return nil, err } - found, stopped := inOrder(ctx, len(claimed), func(i int) Difference { - return compare(claimed[i], full[i]) + found, stopped := inOrder(ctx, len(claimed), func(i int, scratch []byte) Difference { + return compare(claimed[i], full[i], scratch) }) for _, d := range found { if d.Kind != "" { @@ -392,7 +392,7 @@ func claimedPaths(b core.Boundary, files []manifest.File) ([]string, error) { // agreement on its own. No sentinel anybody has to remember, and no pointer // per file - a run of ten thousand would allocate ten thousand of them to say // "nothing to report" ten thousand times. -func compare(f manifest.File, full string) Difference { +func compare(f manifest.File, full string, scratch []byte) Difference { info, statErr := os.Stat(full) if statErr != nil { return Difference{Kind: Missing, Path: f.Path} @@ -408,7 +408,7 @@ func compare(f manifest.File, full string) Difference { } } - sum, hashErr := hashFile(full) + sum, hashErr := hashFile(full, scratch) if hashErr != nil { return Difference{Kind: Unreadable, Path: f.Path, Got: hashErr.Error()} } @@ -422,18 +422,48 @@ func compare(f manifest.File, full string) Difference { return Difference{} } +// hashScratch is how much of a file is read at a time. +// +// Measured 2026-09-05 on a 256 MB file, median of five interleaved runs: +// io.Copy 199 ms, 64 KiB 182 ms, 256 KiB 161 ms, 1 MiB 162 ms, 4 MiB 160 ms. +// The whole win is in by a quarter of a megabyte and nothing above it can be +// told apart, so this is the size that buys it without asking every worker for +// a megabyte. +const hashScratch = 256 << 10 + +// onlyRead hides everything but Read from io.CopyBuffer. +// +// This is not a wrapper for its own sake and without it the buffer below does +// nothing at all. os.File implements io.WriterTo, so io.CopyBuffer hands the +// whole job to the file and THROWS THE BUFFER AWAY - measured on 2026-09-05, +// passing 128 KiB, 256 KiB or 1 MiB with a plain file on the other end all take +// the same 167-172 ms that io.Copy takes, because all four are the same 32 KiB +// path inside. Hidden behind this, a 256 KiB buffer takes 150-161 ms. +// +// The report that suggested the buffer said it would "cut the syscall count 32 +// fold" and would have changed nothing as written. That is the whole reason +// this type exists rather than a comment saying the buffer is bigger. +type onlyRead struct{ io.Reader } + // hashFile streams the file rather than reading it in. A run of this tool // produces files measured in gigabytes, and verify has to survive its own // output. -func hashFile(path string) (string, error) { +// +// scratch belongs to the caller and is reused across every file that caller +// answers for. It may be nil, and then this asks for one of its own - which is +// what a sequential caller wants and what every test that reaches here gets. +func hashFile(path string, scratch []byte) (string, error) { f, err := os.Open(path) if err != nil { return "", err } defer func() { _ = f.Close() }() + if scratch == nil { + scratch = make([]byte, hashScratch) + } h := sha256.New() - if _, err := io.Copy(h, f); err != nil { + if _, err := io.CopyBuffer(h, onlyRead{f}, scratch); err != nil { return "", err } return hex.EncodeToString(h.Sum(nil)), nil diff --git a/internal/audit/cleanup.go b/internal/audit/cleanup.go index 84249e6..7e659db 100644 --- a/internal/audit/cleanup.go +++ b/internal/audit/cleanup.go @@ -79,14 +79,14 @@ func Inspect(ctx context.Context, dir string, m *manifest.Manifest) ([]Candidate // In order, because this list is what cleanup removes from and what it // printed to a person beforehand. - return inOrder(ctx, len(claimed), func(i int) Candidate { - return look(claimed[i], full[i]) + return inOrder(ctx, len(claimed), func(i int, scratch []byte) Candidate { + return look(claimed[i], full[i], scratch) }) } // look is what one claimed file comes to for cleanup: whether it may be // removed, and if not, why not in the words a person needs. -func look(f manifest.File, full string) Candidate { +func look(f manifest.File, full string, scratch []byte) Candidate { info, err := os.Stat(full) if errors.Is(err, fs.ErrNotExist) { return Candidate{Path: f.Path, Disposition: Absent} @@ -101,7 +101,7 @@ func look(f manifest.File, full string) Candidate { return Candidate{Path: f.Path, Disposition: Changed, Detail: fmt.Sprintf("it is %d B and the manifest recorded %d B", info.Size(), f.Bytes)} } - sum, err := hashFile(full) + sum, err := hashFile(full, scratch) if err != nil { return Candidate{Path: f.Path, Disposition: Unreachable, Detail: err.Error()} } diff --git a/internal/audit/parallel.go b/internal/audit/parallel.go index e0c8726..ea264b4 100644 --- a/internal/audit/parallel.go +++ b/internal/audit/parallel.go @@ -75,7 +75,7 @@ func widthFor(n int) int { // own sake: if a refusal could arrive here, stopping the other goroutines // would mean a LOWER index never got asked, and the same manifest would // name a different file on different days. -func inOrder[T any](ctx context.Context, n int, one func(i int) T) ([]T, error) { +func inOrder[T any](ctx context.Context, n int, one func(i int, scratch []byte) T) ([]T, error) { out := make([]T, n) done := make([]bool, n) @@ -111,7 +111,20 @@ func inOrder[T any](ctx context.Context, n int, one func(i int) T) ([]T, error) // out and done are written at indices no other goroutine will take, because // the counter hands each index out exactly once. They are the only things // written at all. -func drain[T any](ctx context.Context, next *atomic.Int64, out []T, done []bool, one func(i int) T) { +func drain[T any](ctx context.Context, next *atomic.Int64, out []T, done []bool, one func(i int, scratch []byte) T) { + // One buffer per goroutine, handed to every item that goroutine answers. + // + // Per worker rather than per item, because a run of a hundred thousand + // files would otherwise allocate a hundred thousand of these. And per + // worker rather than one shared, because two goroutines reading files into + // the same bytes is the data race this whole file is written to avoid. + // + // The size is measured rather than picked. Hashing a 256 MB file, median of + // five: io.Copy 199 ms, 64 KiB 182 ms, 256 KiB 161 ms, 1 MiB 162 ms, 4 MiB + // 160 ms. The win is all in by a quarter of a megabyte and nothing above it + // is distinguishable, so sixteen workers cost four megabytes rather than + // sixty four. + scratch := make([]byte, hashScratch) for { i := int(next.Add(1)) - 1 // Cancellation is asked per item rather than per pass, so a Ctrl+C @@ -120,7 +133,7 @@ func drain[T any](ctx context.Context, next *atomic.Int64, out []T, done []bool, if i >= len(out) || ctx.Err() != nil { return } - out[i] = one(i) + out[i] = one(i, scratch) done[i] = true } } diff --git a/internal/engine/engine.go b/internal/engine/engine.go index f449a0e..4c557e9 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -433,128 +433,6 @@ type Progress struct { BytesTotal int64 } -// DefaultManifestName is where the manifest lands when nothing says otherwise. -// -// It lives here rather than in the caller because the engine has to know the -// name to protect it from being written over, and two copies of a file name -// are two things to keep in step. -const DefaultManifestName = "manifest.json" - -// preflight answers whether this run may start, without writing anything. -// -// Both checks used to sit after the dry run had already returned, so -// --dry-run reported success for runs that would refuse to start. -func preflight(ctx context.Context, files []PlannedFile, opt Options) error { - // Free space first. Finding out at file five thousand of ten thousand - // leaves a half written set and a full disk on a machine somebody works on. - needed := TotalBytes(files) - if available, err := opt.availableBytes(opt.OutDir); err == nil { - if available < needed { - return &SpaceError{Needed: needed, Available: available, Path: opt.OutDir} - } - } - // A failure to read the free space is not a reason to refuse. A disk we - // cannot measure is not the same as a disk that is full. - - // Pointing --out at a file rather than a directory is a mistake somebody - // makes, and it used to arrive as two messages about one fault, the first - // of them saying "there is nothing at that path" about a path that has - // something at it. The system reports ENOTDIR and our mapping only knew - // "missing", "no permission" and "already there". - if info, err := os.Stat(opt.OutDir); err == nil && !info.IsDir() { - return &RecipeError{Setting: SettingOutDir, - Detail: fmt.Sprintf("the output directory %s is a file, not a directory", opt.OutDir), - Remedy: "Point the output directory at a directory, or at one that does not exist yet and it will be created"} - } - - // The manifest is checked with the files it would describe, and leaving it - // out cost exactly what it protects. A second run into the same directory - // wrote a fresh manifest over the old one, so every file the old one listed - // stopped being anybody's - cleanup reported nothing to remove and those - // files could never be cleaned up by this tool again. It happened on a - // successful run as readily as on a refused one, and on the successful one - // it happened in silence. - if path := ManifestPath(opt); exists(path) { - return &CollisionError{Path: path, Manifest: true} - } - - // Nothing else is written over either. This tool runs in directories that - // belong to the user, so destroying their work is the one failure that - // cannot be undone by running again. - // - // The temporary name is checked as well as the final one. It used to be - // created with os.Create, which truncates, so a file already sitting under - // that name lost its contents without a word while the collision check - // looked only at the name the file ends up with. Asking the filesystem - // instead, with O_EXCL at the write, was tried on 2026-08-25 and taken - // back out - see writeOne for what it costs on Windows. So this check is - // the whole of the protection rather than the friendlier half of it. - // - // Which run leaves such a file behind is worth being accurate about, - // because the comment here used to get it wrong. The name carries the - // process id, so a run killed outright leaves one that no later run can - // meet: the next run builds a different name and walks past it. Nor is it - // lost - verify names it as one of ours rather than as something nobody - // asked for, and cleanup leaves it alone because untouchable rule 7 makes - // the manifest the whole authority over what may be deleted, and a file - // that never finished never reached one. - for _, f := range files { - // Two questions of the filesystem per planned file, so a large run on a - // slow share spends real time in here. Until 2026-08-26 that time could - // not be interrupted: preflight took no context, so a preview had - // nothing to cancel, the window offered no button for it, and closing - // the window waited for the whole loop on the interface thread. The - // same reasoning that put a context into the directory walk in - // internal/audit on 2026-08-25. - if err := ctx.Err(); err != nil { - return err - } - if path := filepath.Join(opt.OutDir, f.Name); exists(path) { - return &CollisionError{Path: path} - } - if path := tempPathFor(opt.OutDir, f.Name); exists(path) { - return &CollisionError{Path: path} - } - } - return nil -} - -// tempPathFor is the name a file is written under before it is renamed into -// place. One definition, used by the check above and by the write below, so -// the two cannot disagree about what is being protected. -func tempPathFor(outDir, name string) string { - return fmt.Sprintf("%s%s%d", filepath.Join(outDir, name), core.PartialMarker, os.Getpid()) -} - -func exists(path string) bool { - _, err := os.Stat(path) - return err == nil -} - -// manifestNameOf is where this run's record lands. One definition, because the -// preflight check, the claim and the save all have to mean the same file. -func manifestNameOf(opt Options) string { - if opt.ManifestName == "" { - return DefaultManifestName - } - return opt.ManifestName -} - -// ManifestPath is the file this run's record lands in, for the callers that -// save it. The engine claims that name before the first file and refuses the -// run if it is taken, so a saver joining the path its own way is answering a -// question this package has already answered. -// -// Exported on 2026-08-27 because both savers did join it their own way. The -// window's did not handle an empty name at all, so a caller that left it blank -// would have asked the manifest to be written to the output directory itself - -// a rename of a file onto a directory. Nothing reached that, because all three -// screens fill the field in, and "nothing reaches it today" is the description -// of a fault waiting for a fourth screen rather than of a safe piece of code. -func ManifestPath(opt Options) string { - return filepath.Join(opt.OutDir, manifestNameOf(opt)) -} - // Run writes a planned set of files. // // Each file is written under a temporary name and only then renamed, so the diff --git a/internal/engine/planmemory.go b/internal/engine/planmemory.go index 54c2387..e9afd64 100644 --- a/internal/engine/planmemory.go +++ b/internal/engine/planmemory.go @@ -3,6 +3,7 @@ package engine import ( "fmt" "runtime" + "runtime/metrics" "github.com/donislawdev/TestingFilesGenerator/internal/core" ) @@ -61,9 +62,19 @@ const ( // not nought, and this is where it lives. type planMemory struct { baseline uint64 - seen int - nextAt int - ceiling uint64 + // allocsBaseline is the running total of bytes this process has ever + // allocated, taken beside the baseline above. + // + // It is what makes the cheap question in account possible, and the reason + // it works is an inequality rather than an estimate: every byte the live + // heap grows by has to have been allocated, so the growth this guard cares + // about can never exceed the total allocated since the baseline. Under the + // ceiling on that total means under the ceiling full stop, and no + // collection has to be paid for to find out. + allocsBaseline uint64 + seen int + nextAt int + ceiling uint64 } // newPlanMemory takes the reference point, and it is taken here because here @@ -108,17 +119,51 @@ func newPlanMemory(ceiling int64) *planMemory { if ceiling <= 0 { ceiling = core.MaxPlanBytes } - return &planMemory{baseline: heapInUse(), nextAt: 1, ceiling: uint64(ceiling)} + return &planMemory{ + baseline: heapInUse(), + allocsBaseline: allocatedSoFar(), + nextAt: 1, + ceiling: uint64(ceiling), + } } // account is called once per planned file. It returns an error only when the // plan has already passed the ceiling. +// +// The cheap question is asked first and it is the one nearly every run gets to +// stop at. Until 2026-09-05 there was no cheap question: every reading was a +// forced collection, so a run of one file of one kilobyte paid for TWO of them +// - measured with GODEBUG=gctrace=1, exactly two on every run however small. +// The cost is 519 us against 251 ns for the counter, and it grows with the live +// heap, which is precisely the run this guard exists for. +// +// What the counter cannot do is answer the question this refuses on. It counts +// everything ever allocated, including what has already been thrown away, so it +// says "no" with certainty and "maybe" otherwise. The certain half is enough, +// because it is the half every real run lands in: MaxPlanBytes is two +// gigabytes and planning ten thousand files allocates tens of megabytes. +// +// /gc/heap/live:bytes was the obvious other candidate and it is the wrong one. +// It reports the live heap as of the LAST COLLECTION, so a burst of allocation +// between two collections is invisible to it - which would turn a refusal into +// silence exactly when the plan is growing fastest. The counter has no such +// gap. func (p *planMemory) account(targetIndex, filesSoFar int) error { p.seen++ if p.seen < p.nextAt { return nil } + if allocated := allocatedSoFar() - p.allocsBaseline; allocated <= p.ceiling { + // Bring the next reading forward to land inside the headroom that is + // left, for the reason written out below - and on this figure rather + // than on the swept one, which makes the step SHORTER than it needs to + // be rather than longer, since what has been allocated is never less + // than what is still held. + p.nextAt = p.seen + stepFor(p.ceiling, allocated, p.seen) + return nil + } + grown := heapInUse() if grown <= p.baseline { // The collector handed memory back between the two readings. That is @@ -130,22 +175,7 @@ func (p *planMemory) account(targetIndex, filesSoFar int) error { } used := grown - p.baseline if used <= p.ceiling { - // Bring the next reading forward to land inside the headroom that is - // left, so an expensive file cannot carry the plan far past the - // ceiling between two readings. Half the headroom rather than all of - // it, because the average is an average - a run whose later files are - // dearer than its earlier ones would otherwise step straight over. - perFile := used / uint64(p.seen) - step := planCheckEvery - if perFile > 0 { - if room := int((p.ceiling - used) / perFile / 2); room < step { - step = room - } - } - if step < 1 { - step = 1 - } - p.nextAt = p.seen + step + p.nextAt = p.seen + stepFor(p.ceiling, used, p.seen) return nil } @@ -160,6 +190,43 @@ func (p *planMemory) account(targetIndex, filesSoFar int) error { } } +// stepFor is how many more files may go by before the next reading. +// +// It brings the reading forward to land inside the headroom that is left, so +// an expensive file cannot carry the plan far past the ceiling between two of +// them. Half the headroom rather than all of it, because the average is an +// average - a run whose later files are dearer than its earlier ones would +// otherwise step straight over. +// +// One function rather than the same arithmetic twice, since 2026-09-05 there +// are two readings it has to serve: the cheap counter and the swept heap. They +// disagree about the number they are given and agree about what to do with it. +func stepFor(ceiling, used uint64, seen int) int { + step := planCheckEvery + if perFile := used / uint64(seen); perFile > 0 { + if room := int((ceiling - used) / perFile / 2); room < step { + step = room + } + } + if step < 1 { + step = 1 + } + return step +} + +// allocatedSoFar is every byte this process has ever allocated, thrown away or +// not. +// +// Cumulative and exact - unlike the live heap, it does not wait for a +// collection to catch up, so a burst of allocation is visible the moment it +// happens. Measured 2026-09-05: 251 ns a call against 519 us for a forced +// collection and its reading. +func allocatedSoFar() uint64 { + sample := [1]metrics.Sample{{Name: "/gc/heap/allocs:bytes"}} + metrics.Read(sample[:]) + return sample[0].Value.Uint64() +} + // heapInUse is the live heap, after a collection so that the number is about // what the plan is holding rather than about what has not been swept yet. // diff --git a/internal/engine/preflight.go b/internal/engine/preflight.go new file mode 100644 index 0000000..ca85e4a --- /dev/null +++ b/internal/engine/preflight.go @@ -0,0 +1,207 @@ +package engine + +import ( + "context" + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + + "github.com/donislawdev/TestingFilesGenerator/internal/core" +) + +// What has to be true before a run may start, and where the things it writes +// land. +// +// Split out of engine.go on 2026-09-05, when reading the output directory once +// instead of asking about every planned file twice took that file past the +// length ceiling. The guard's own message asks for a split by what the parts +// do rather than for a bigger number, and this is that part: no bytes are +// written from here, every function answers a question about a name. + +// DefaultManifestName is where the manifest lands when nothing says otherwise. +// +// It lives here rather than in the caller because the engine has to know the +// name to protect it from being written over, and two copies of a file name +// are two things to keep in step. +const DefaultManifestName = "manifest.json" + +// preflight answers whether this run may start, without writing anything. +// +// Both checks used to sit after the dry run had already returned, so +// --dry-run reported success for runs that would refuse to start. +func preflight(ctx context.Context, files []PlannedFile, opt Options) error { + // Free space first. Finding out at file five thousand of ten thousand + // leaves a half written set and a full disk on a machine somebody works on. + needed := TotalBytes(files) + if available, err := opt.availableBytes(opt.OutDir); err == nil { + if available < needed { + return &SpaceError{Needed: needed, Available: available, Path: opt.OutDir} + } + } + // A failure to read the free space is not a reason to refuse. A disk we + // cannot measure is not the same as a disk that is full. + + // Pointing --out at a file rather than a directory is a mistake somebody + // makes, and it used to arrive as two messages about one fault, the first + // of them saying "there is nothing at that path" about a path that has + // something at it. The system reports ENOTDIR and our mapping only knew + // "missing", "no permission" and "already there". + if info, err := os.Stat(opt.OutDir); err == nil && !info.IsDir() { + return &RecipeError{Setting: SettingOutDir, + Detail: fmt.Sprintf("the output directory %s is a file, not a directory", opt.OutDir), + Remedy: "Point the output directory at a directory, or at one that does not exist yet and it will be created"} + } + + // The manifest is checked with the files it would describe, and leaving it + // out cost exactly what it protects. A second run into the same directory + // wrote a fresh manifest over the old one, so every file the old one listed + // stopped being anybody's - cleanup reported nothing to remove and those + // files could never be cleaned up by this tool again. It happened on a + // successful run as readily as on a refused one, and on the successful one + // it happened in silence. + if path := ManifestPath(opt); exists(path) { + return &CollisionError{Path: path, Manifest: true} + } + + // Nothing else is written over either. This tool runs in directories that + // belong to the user, so destroying their work is the one failure that + // cannot be undone by running again. + // + // The temporary name is checked as well as the final one. It used to be + // created with os.Create, which truncates, so a file already sitting under + // that name lost its contents without a word while the collision check + // looked only at the name the file ends up with. Asking the filesystem + // instead, with O_EXCL at the write, was tried on 2026-08-25 and taken + // back out - see writeOne for what it costs on Windows. So this check is + // the whole of the protection rather than the friendlier half of it. + // + // Which run leaves such a file behind is worth being accurate about, + // because the comment here used to get it wrong. The name carries the + // process id, so a run killed outright leaves one that no later run can + // meet: the next run builds a different name and walks past it. Nor is it + // lost - verify names it as one of ours rather than as something nobody + // asked for, and cleanup leaves it alone because untouchable rule 7 makes + // the manifest the whole authority over what may be deleted, and a file + // that never finished never reached one. + // One listing of the directory rather than two questions of the filesystem + // for every planned file. + // + // The comment that stood here named the cost - "a large run on a slow share + // spends real time in here" - and until 2026-08-26 that time could not even + // be interrupted, which is why the context arrived. Measured on 2026-09-05, + // ten thousand planned names with half of them present: 1.937 s of stat + // calls against 8.9 ms for one listing, ranges disjoint over eight + // repetitions with a steady canary. That is a local disk. The share the old + // comment was written for pays a round trip for each of those twenty + // thousand questions. + // + // Names cannot hold a separator - checkFileName refuses both on every + // system - so one listing of the output directory covers every planned file + // and every temporary name beside it. + taken := namesIn(opt.OutDir) + + for _, f := range files { + // Asked per file rather than once, so a preview of a hundred thousand + // files can still be cancelled. That reasoning is unchanged by the + // listing above, it is only much cheaper to be interrupted now. + if err := ctx.Err(); err != nil { + return err + } + if path := filepath.Join(opt.OutDir, f.Name); isTaken(taken, path, f.Name) { + return &CollisionError{Path: path} + } + if path := tempPathFor(opt.OutDir, f.Name); isTaken(taken, path, tempNameFor(f.Name)) { + return &CollisionError{Path: path} + } + } + return nil +} + +// namesIn is every name the output directory holds, or nil when it could not be +// listed at all. +// +// nil is not the same answer as empty, and the difference is the one failure +// this check exists to prevent. A directory that cannot be READ can still be +// one a file may be created in - both systems allow that combination - and +// treating it as empty would let a run write over somebody's work. So nil means +// "ask file by file" rather than "there is nothing there". +// +// A directory that does not exist yet is a different answer and a correct one: +// it holds no names, nothing can collide, and that is the ordinary case for a +// fresh run. +func namesIn(dir string) map[string]struct{} { + entries, err := os.ReadDir(dir) + if errors.Is(err, fs.ErrNotExist) { + return map[string]struct{}{} + } + if err != nil { + return nil + } + taken := make(map[string]struct{}, len(entries)) + for _, e := range entries { + taken[e.Name()] = struct{}{} + } + return taken +} + +// isTaken answers whether a name is already in use - from the listing when +// there is one, and from the filesystem when there is not. +// +// The two are not quite the same question and the listing asks the better one. +// os.Stat follows a link, so a link pointing at nothing answered "no name +// here" and the run went on to replace it. A directory ENTRY is what a name +// being taken means, whatever it points at, and the rule this serves is that +// nothing already there is written over. So the listing refuses a little more +// than the stat did, in the direction that cannot lose somebody's work. +func isTaken(taken map[string]struct{}, path, name string) bool { + if taken == nil { + return exists(path) + } + _, ok := taken[name] + return ok +} + +// tempPathFor is the name a file is written under before it is renamed into +// place. One definition, used by the check above and by the write below, so +// the two cannot disagree about what is being protected. +func tempPathFor(outDir, name string) string { + return filepath.Join(outDir, tempNameFor(name)) +} + +// tempNameFor is that name without the directory in front of it, which is what +// a directory listing gives back. Split out so the listing and the path cannot +// disagree about how the temporary name is spelt. +func tempNameFor(name string) string { + return fmt.Sprintf("%s%s%d", name, core.PartialMarker, os.Getpid()) +} + +func exists(path string) bool { + _, err := os.Stat(path) + return err == nil +} + +// manifestNameOf is where this run's record lands. One definition, because the +// preflight check, the claim and the save all have to mean the same file. +func manifestNameOf(opt Options) string { + if opt.ManifestName == "" { + return DefaultManifestName + } + return opt.ManifestName +} + +// ManifestPath is the file this run's record lands in, for the callers that +// save it. The engine claims that name before the first file and refuses the +// run if it is taken, so a saver joining the path its own way is answering a +// question this package has already answered. +// +// Exported on 2026-08-27 because both savers did join it their own way. The +// window's did not handle an empty name at all, so a caller that left it blank +// would have asked the manifest to be written to the output directory itself - +// a rename of a file onto a directory. Nothing reached that, because all three +// screens fill the field in, and "nothing reaches it today" is the description +// of a fault waiting for a fourth screen rather than of a safe piece of code. +func ManifestPath(opt Options) string { + return filepath.Join(opt.OutDir, manifestNameOf(opt)) +} diff --git a/internal/format/zip/children.go b/internal/format/zip/children.go index df3447c..466e155 100644 --- a/internal/format/zip/children.go +++ b/internal/format/zip/children.go @@ -24,7 +24,15 @@ import ( // seed of a member does not move when a group above it changes count. That is // untouchable rule 2 applied one level down. func planChildren(r format.Request, groups []format.Content, layout archive.Layout) ([]child, error) { - var out []child + // Sized up front, because the total is known before the walk starts: it is + // what the groups add up to. Growing by append instead reallocates and + // copies the whole slice fourteen times on the way to ten thousand entries, + // which is the ceiling this format declares. + total := 0 + for _, g := range groups { + total += g.Count + } + out := make([]child, 0, total) index := 0 // Numbering runs per format rather than per group, so two groups of the // same format do not both start at 0001 and collide inside the archive. diff --git a/internal/guard/branching_test.go b/internal/guard/branching_test.go index 7735c16..9a9fcd7 100644 --- a/internal/guard/branching_test.go +++ b/internal/guard/branching_test.go @@ -45,7 +45,9 @@ const ( // either, and one handles neither. Flattening it in TIFF alone would make // two functions that answer the same question look different, which costs // more than the depth does. - crowdedDepthFunctions = 52 + // Lowered from 52 on 2026-09-05: splitting preflight out took one function + // out of the band. The ratchet only tightens. + crowdedDepthFunctions = 51 // An axis this set does not watch. crowding() asks n >= band, so nothing // reaches it. diff --git a/internal/guard/codeshape_test.go b/internal/guard/codeshape_test.go index 2c92515..14a2c0c 100644 --- a/internal/guard/codeshape_test.go +++ b/internal/guard/codeshape_test.go @@ -39,7 +39,9 @@ const ( // how big a target was, because the budget stopped needing to be told - it // takes its reference point when it is built. A ratchet goes down when work // makes it lowerable. - longestFile = 502 + // Lowered from 502 on 2026-09-05: preflight and the questions it asks about + // names moved into their own file. The ratchet only tightens. + longestFile = 457 // Depth answers a different question than length, and it is the better // question of the two. A hundred line function that is flat reads top to diff --git a/internal/guard/planmemory_test.go b/internal/guard/planmemory_test.go index ee657aa..9e6315c 100644 --- a/internal/guard/planmemory_test.go +++ b/internal/guard/planmemory_test.go @@ -1,6 +1,7 @@ package guard import ( + "runtime/debug" "strings" "testing" @@ -207,3 +208,50 @@ func asAddressed(err error, target *interface{ AboutSetting() string }) bool { } return false } + +// The cheap question the ceiling asks first is about ALLOCATION, not about the +// last collection, and the difference only shows when the collector is idle. +// +// Since 2026-09-05 account reads /gc/heap/allocs:bytes before it forces a +// collection, because that costs 251 ns against 519 us and a run of one file +// used to pay for two collections however small it was. What makes the +// shortcut safe is an inequality rather than an estimate: the live heap cannot +// have grown by more than has been allocated, so under the ceiling on the +// total is under the ceiling full stop. +// +// /gc/heap/live:bytes was the other candidate - the report that asked for this +// named it - and it is unsound here. It reports the heap as of the LAST +// COLLECTION, so a plan growing between two of them is invisible. +// +// Measured on 2026-09-05: swapping the metric leaves all four guards above +// GREEN, because twenty five megabytes of allocation makes the collector run on +// its own and the lagging reading catches up by luck. Turning the collector off +// takes the luck away - the live reading then never moves at all, a build +// asking that question accepts a plan of any size, and this is the only thing +// here that would say so. +func TestThePlanCeilingAsksAboutAllocationRatherThanTheLastCollection(t *testing.T) { + was := debug.SetGCPercent(-1) + t.Cleanup(func() { debug.SetGCPercent(was) }) + + // The shape the short run guard uses, which is one file measured at + // 25 194 908 B of plan against a ceiling of four megabytes. + targets := []engine.Target{{ + ID: "one", + Format: "pdf", + Sizes: engine.Uniform(1, 20<<20), + Properties: map[string]string{"pages": "5000"}, + }} + + _, err := engine.Plan(targets, engine.Options{ + OutDir: "out", + ManifestName: "manifest.json", + MaxPlanBytes: 4 << 20, + }) + if err == nil { + t.Fatal("with the collector switched off a plan six times the ceiling was accepted, " + + "so the cheap reading is answering about the last collection rather than about what was allocated") + } + if !strings.Contains(err.Error(), "ceiling") { + t.Errorf("the refusal does not mention the ceiling: %s", err) + } +} diff --git a/internal/guard/safety_test.go b/internal/guard/safety_test.go index b245b6b..ef66c28 100644 --- a/internal/guard/safety_test.go +++ b/internal/guard/safety_test.go @@ -5,6 +5,7 @@ import ( "errors" "os" "path/filepath" + "runtime" "strings" "testing" @@ -174,3 +175,117 @@ func TestAFreshRunIntoAnEmptyDirectoryStillWorks(t *testing.T) { t.Errorf("produced %d files, expected 3", res.Manifest.Summary.Materialized) } } + +// A name is taken by whatever holds it, including a link that points nowhere. +// +// Until 2026-09-05 preflight asked os.Stat whether the path existed, and +// os.Stat follows a link - so a link pointing at nothing answered "no name +// here" and the run replaced the entry without a word. It reads one listing of +// the directory now, and a directory ENTRY is what a taken name is, whatever +// it points at. +// +// That difference is why the change is not only about speed. Replacing a link +// somebody put there loses their work exactly as replacing a file does, and it +// happened on the quiet path rather than the loud one. +func TestANameTakenByALinkPointingNowhereIsStillTaken(t *testing.T) { + dir := t.TempDir() + dangling := filepath.Join(dir, "files_0001.txt") + if err := os.Symlink(filepath.Join(dir, "nothing-is-here"), dangling); err != nil { + t.Skipf("this system will not create a link here, so the case cannot be built: %v", err) + } + + // Asserted rather than assumed, because the whole case is a name that IS + // there and that os.Stat cannot see. A fixture that quietly resolved would + // leave this guard green about the old behaviour (O118). + if _, err := os.Stat(dangling); err == nil { + t.Fatal("the link resolves, so this is not the state being guarded") + } + if _, err := os.Lstat(dangling); err != nil { + t.Fatalf("there is no entry at all, so there is nothing for a run to collide with: %v", err) + } + + opt := engine.Options{OutDir: dir, Seed: 7741, Command: "test"} + planned, err := engine.Plan([]engine.Target{txtTarget("files", 1, 4096)}, opt) + if err != nil { + t.Fatalf("planning: %v", err) + } + + _, runErr := engine.Run(context.Background(), planned, opt) + if runErr == nil { + t.Fatal("the run went ahead over a name somebody else's link was holding") + } + var collision *engine.CollisionError + if !errors.As(runErr, &collision) { + t.Errorf("refused with %T, expected a CollisionError so the caller answers with the right exit code", runErr) + } +} + +// A directory that cannot be LISTED is still asked about every name, one at a +// time. +// +// The listing that made preflight cheap has an answer it cannot give: a +// directory with permission to write and none to read. Both systems allow that +// combination, and a run into one has always worked. Reading nothing there and +// calling it empty would let the run write over whatever is inside - the one +// failure here that running again cannot undo - so a listing that fails means +// "ask file by file" rather than "there is nothing there". +// +// Without this the fallback is a branch nothing can turn red, and this project +// has removed seven of those. +func TestADirectoryThatCannotBeListedIsStillAskedAboutEveryName(t *testing.T) { + if runtime.GOOS == "windows" { + // The same reason environment_test.go gives for the sibling of this + // guard: os.Chmod on Windows moves the read only bit and nothing else, + // and denying a listing needs an ACL, which is not something a test + // should be installing. + t.Skip("a directory that refuses a listing needs an ACL on Windows") + } + + dir := t.TempDir() + out := filepath.Join(dir, "writeonly") + if err := os.Mkdir(out, 0o755); err != nil { + t.Fatalf("making the directory: %v", err) + } + const precious = "work that took an afternoon" + victim := filepath.Join(out, "files_0001.txt") + if err := os.WriteFile(victim, []byte(precious), 0o644); err != nil { + t.Fatalf("preparing the file: %v", err) + } + if err := os.Chmod(out, 0o300); err != nil { + t.Fatalf("taking the read permission away: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(out, 0o755) }) + + // Asserted rather than assumed. Root ignores the permission bits, so on a + // container running as root this state does not exist and the guard would + // otherwise pass while testing the fast path twice. + if _, err := os.ReadDir(out); err == nil { + t.Skip("this process can list a directory with no read permission, so the fallback cannot be reached") + } + + opt := engine.Options{OutDir: out, Seed: 7741, Command: "test"} + planned, err := engine.Plan([]engine.Target{txtTarget("files", 1, 4096)}, opt) + if err != nil { + t.Fatalf("planning: %v", err) + } + + _, runErr := engine.Run(context.Background(), planned, opt) + if runErr == nil { + t.Fatal("a run into a directory it could not list went ahead over what was inside") + } + var collision *engine.CollisionError + if !errors.As(runErr, &collision) { + t.Errorf("refused with %T, expected a CollisionError", runErr) + } + + if err := os.Chmod(out, 0o755); err != nil { + t.Fatalf("putting the permission back to read the file: %v", err) + } + after, err := os.ReadFile(victim) + if err != nil { + t.Fatalf("reading the file back: %v", err) + } + if string(after) != precious { + t.Error("the existing file was destroyed - this is the one failure that cannot be undone by running again") + } +}