diff --git a/CHANGELOG.md b/CHANGELOG.md index 72b7aeb..843c0a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,29 @@ because it turns other people's test suites red. ## [Unreleased] +### Breaking + +- **ZIP, TAR.GZ and WAV files have different bytes.** The padding these three + formats write is now drawn from the random generator eight bytes at a time + instead of one byte at a time. For a ZIP or a TAR.GZ that padding is almost + the whole file, so almost every byte changes. + + Nothing a reader can see is different. The size is the same to the byte, the + structure is the same, the same tools open the same files. What changes is the + content of the padding, so a hash you recorded from an earlier version will + not match one you generate now. + + This is what makes large archives faster to produce. Measured on 64 MB files, + runs interleaved: ZIP 2.8 times faster, TAR.GZ 2.7 to 3.3 times faster. + + WAV is on this list for consistency rather than for speed. Its padding is at + most a few bytes per file, so only those bytes differ and the time to produce + one is unchanged. + + Every other format keeps exactly the bytes it had. Twelve of them share the + same new code and were checked against their recorded hashes and across every + format at five sizes and two seeds. + ### Changed - **`verify` and `cleanup` read the files over several threads, so checking a @@ -29,6 +52,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/core/fill.go b/internal/core/fill.go new file mode 100644 index 0000000..00fd815 --- /dev/null +++ b/internal/core/fill.go @@ -0,0 +1,59 @@ +package core + +import ( + "encoding/binary" + // D11 promises the same bytes from the same seed, so a deliberate, + // reproducible generator is the product rather than a weakness. Nothing + // here ever makes a secret. + // nosemgrep: go.lang.security.audit.crypto.math_random.math-random-used + "math/rand/v2" +) + +// Bulk random bytes are drawn eight at a time, and the two byte orders are two +// functions rather than one function with a flag. +// +// Eight at a time rather than one is the whole point. Measured 2026-09-06 over +// 64 MiB through a 32 KiB buffer, repetitions interleaved with the order +// reversed: one draw per byte runs at 182 MB/s, eight bytes per draw at +// 2499 MB/s. The write path of an archive is slower than the disk underneath it +// when it draws a byte at a time (P2 in PERFORMANCE-REVIEW-2026-09-05.md). +// +// The orders stay separate because picking the wrong one is not a style +// mistake - it silently rewrites every file a format has ever produced, which +// is exactly what D11 forbids. A boolean argument would put that mistake one +// typo away and would read the same in review either way. Two names cannot be +// confused by accident, and the compiler cannot help with a flag. +// +// Neither function draws anything for an empty slice, and both spend exactly +// one draw on a trailing group shorter than eight bytes. That is what the ten +// call sites did before they were folded into here, so folding them in moved no +// bytes except where the owner asked for them to move. + +// FillRandomBE fills b with random bytes, most significant byte of each draw +// first. This is the order bmp, gif, ico, opc, png, tiff and - since 0.3.0 - +// targz, wav and zip write. +func FillRandomBE(b []byte, rng *rand.Rand) { + i := 0 + for ; i+8 <= len(b); i += 8 { + binary.BigEndian.PutUint64(b[i:], rng.Uint64()) + } + if i < len(b) { + var eight [8]byte + binary.BigEndian.PutUint64(eight[:], rng.Uint64()) + copy(b[i:], eight[:]) + } +} + +// FillRandomLE fills b with random bytes, least significant byte of each draw +// first. This is the order avif, jpg, jxl and webp write. +func FillRandomLE(b []byte, rng *rand.Rand) { + i := 0 + for ; i+8 <= len(b); i += 8 { + binary.LittleEndian.PutUint64(b[i:], rng.Uint64()) + } + if i < len(b) { + var eight [8]byte + binary.LittleEndian.PutUint64(eight[:], rng.Uint64()) + copy(b[i:], eight[:]) + } +} 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/avif/avif.go b/internal/format/avif/avif.go index 5077e86..aa740dd 100644 --- a/internal/format/avif/avif.go +++ b/internal/format/avif/avif.go @@ -515,7 +515,7 @@ func writeFiller(ctx context.Context, w io.Writer, buf []byte, rng *rand.Rand, s return ctx.Err() default: } - fillBytes(buf[:n], rng) + core.FillRandomLE(buf[:n], rng) if _, err := w.Write(buf[:n]); err != nil { return err } @@ -523,12 +523,3 @@ func writeFiller(ctx context.Context, w io.Writer, buf []byte, rng *rand.Rand, s } return nil } - -func fillBytes(b []byte, rng *rand.Rand) { - for i := 0; i < len(b); i += 8 { - v := rng.Uint64() - for j := 0; j < 8 && i+j < len(b); j++ { - b[i+j] = byte(v >> (8 * uint(j))) - } - } -} diff --git a/internal/format/bmp/bmp.go b/internal/format/bmp/bmp.go index 612af04..153c3ee 100644 --- a/internal/format/bmp/bmp.go +++ b/internal/format/bmp/bmp.go @@ -372,11 +372,7 @@ func writeGap(ctx context.Context, w io.Writer, seed uint64, n int64) error { take = remaining } chunk := buf[:take] - for i := 0; i < len(chunk); i += 8 { - var eight [8]byte - binary.BigEndian.PutUint64(eight[:], rng.Uint64()) - copy(chunk[i:], eight[:]) - } + core.FillRandomBE(chunk, rng) if _, err := w.Write(chunk); err != nil { return err } diff --git a/internal/format/gif/gif.go b/internal/format/gif/gif.go index 0703482..ee62db6 100644 --- a/internal/format/gif/gif.go +++ b/internal/format/gif/gif.go @@ -358,11 +358,7 @@ func writeComment(ctx context.Context, w io.Writer, seed uint64, blocks, payload } buf[0] = byte(size) chunk := buf[1 : 1+size] - for j := 0; j < len(chunk); j += 8 { - var eight [8]byte - putUint64(eight[:], rng.Uint64()) - copy(chunk[j:], eight[:]) - } + core.FillRandomBE(chunk, rng) if _, err := w.Write(buf[:1+size]); err != nil { return err } @@ -372,17 +368,6 @@ func writeComment(ctx context.Context, w io.Writer, seed uint64, blocks, payload return err } -func putUint64(b []byte, v uint64) { - b[0] = byte(v >> 56) - b[1] = byte(v >> 48) - b[2] = byte(v >> 40) - b[3] = byte(v >> 32) - b[4] = byte(v >> 24) - b[5] = byte(v >> 16) - b[6] = byte(v >> 8) - b[7] = byte(v) -} - // sizeLadder is tried from the largest down when the recipe names no picture // size, exactly as PNG does. The first rung that leaves a reachable remainder // wins, so a small file gets a small picture instead of being refused. diff --git a/internal/format/ico/ico.go b/internal/format/ico/ico.go index f6b469c..813ffd8 100644 --- a/internal/format/ico/ico.go +++ b/internal/format/ico/ico.go @@ -333,11 +333,7 @@ func writeGap(ctx context.Context, w io.Writer, seed uint64, n int64) error { take = remaining } chunk := buf[:take] - for i := 0; i < len(chunk); i += 8 { - var eight [8]byte - binary.BigEndian.PutUint64(eight[:], rng.Uint64()) - copy(chunk[i:], eight[:]) - } + core.FillRandomBE(chunk, rng) if _, err := w.Write(chunk); err != nil { return err } diff --git a/internal/format/jpg/jpg.go b/internal/format/jpg/jpg.go index cd9b652..7c1a094 100644 --- a/internal/format/jpg/jpg.go +++ b/internal/format/jpg/jpg.go @@ -543,14 +543,7 @@ func writeComments(ctx context.Context, w io.Writer, seed uint64, segments int, size = remaining } chunk := buf[:size] - for j := 0; j < len(chunk); j += 8 { - var eight [8]byte - v := rng.Uint64() - for k := 0; k < 8; k++ { - eight[k] = byte(v >> (8 * k)) - } - copy(chunk[j:], eight[:]) - } + core.FillRandomLE(chunk, rng) if _, err := w.Write(chunk); err != nil { return err } diff --git a/internal/format/jxl/jxl.go b/internal/format/jxl/jxl.go index 83b4a9c..8b5ed0b 100644 --- a/internal/format/jxl/jxl.go +++ b/internal/format/jxl/jxl.go @@ -566,7 +566,7 @@ func writeFiller(ctx context.Context, w io.Writer, buf []byte, rng *rand.Rand, s return ctx.Err() default: } - fillBytes(buf[:n], rng) + core.FillRandomLE(buf[:n], rng) if _, err := w.Write(buf[:n]); err != nil { return err } @@ -574,12 +574,3 @@ func writeFiller(ctx context.Context, w io.Writer, buf []byte, rng *rand.Rand, s } return nil } - -func fillBytes(b []byte, rng *rand.Rand) { - for i := 0; i < len(b); i += 8 { - v := rng.Uint64() - for j := 0; j < 8 && i+j < len(b); j++ { - b[i+j] = byte(v >> (8 * uint(j))) - } - } -} diff --git a/internal/format/opc/opc.go b/internal/format/opc/opc.go index 5dced06..097b2e0 100644 --- a/internal/format/opc/opc.go +++ b/internal/format/opc/opc.go @@ -24,7 +24,6 @@ import ( "bytes" "compress/flate" "context" - "encoding/binary" "fmt" "hash/crc32" "io" @@ -412,11 +411,7 @@ func writeFiller(ctx context.Context, w io.Writer, seed uint64, n int64) error { take = remaining } chunk := buf[:take] - for i := 0; i < len(chunk); i += 8 { - var eight [8]byte - binary.BigEndian.PutUint64(eight[:], rng.Uint64()) - copy(chunk[i:], eight[:]) - } + core.FillRandomBE(chunk, rng) if _, err := w.Write(chunk); err != nil { return err } diff --git a/internal/format/png/png.go b/internal/format/png/png.go index 42b8db6..5fde3a7 100644 --- a/internal/format/png/png.go +++ b/internal/format/png/png.go @@ -470,11 +470,7 @@ func writePaddingChunk(ctx context.Context, w io.Writer, kind string, seed uint6 size = remaining } chunk := buf[:size] - for i := 0; i < len(chunk); i += 8 { - var eight [8]byte - binary.BigEndian.PutUint64(eight[:], rng.Uint64()) - copy(chunk[i:], eight[:]) - } + core.FillRandomBE(chunk, rng) crc.Write(chunk) if _, err := w.Write(chunk); err != nil { diff --git a/internal/format/targz/size.go b/internal/format/targz/size.go index 9cfd9ba..b07ff10 100644 --- a/internal/format/targz/size.go +++ b/internal/format/targz/size.go @@ -418,9 +418,7 @@ func writeFiller(ctx context.Context, w io.Writer, seed uint64, n int64) error { size = remaining } chunk := buf[:size] - for i := range chunk { - chunk[i] = byte(rng.UintN(256)) - } + core.FillRandomBE(chunk, rng) if _, err := w.Write(chunk); err != nil { return err } diff --git a/internal/format/tiff/tiff.go b/internal/format/tiff/tiff.go index f36f98a..220733b 100644 --- a/internal/format/tiff/tiff.go +++ b/internal/format/tiff/tiff.go @@ -471,11 +471,7 @@ func writeGap(ctx context.Context, w io.Writer, seed uint64, n int64) error { take = remaining } chunk := buf[:take] - for i := 0; i < len(chunk); i += 8 { - var eight [8]byte - binary.BigEndian.PutUint64(eight[:], rng.Uint64()) - copy(chunk[i:], eight[:]) - } + core.FillRandomBE(chunk, rng) if _, err := w.Write(chunk); err != nil { return err } diff --git a/internal/format/wav/wav.go b/internal/format/wav/wav.go index a584c30..340b400 100644 --- a/internal/format/wav/wav.go +++ b/internal/format/wav/wav.go @@ -481,9 +481,7 @@ func fill(ctx context.Context, w io.Writer, seed uint64, n int64) error { size = remaining } chunk := buf[:size] - for i := range chunk { - chunk[i] = byte(rng.UintN(256)) - } + core.FillRandomBE(chunk, rng) if err := writeAll(w, chunk); err != nil { return err } diff --git a/internal/format/webp/webp.go b/internal/format/webp/webp.go index 0c75318..8ca881c 100644 --- a/internal/format/webp/webp.go +++ b/internal/format/webp/webp.go @@ -37,7 +37,6 @@ import ( "encoding/binary" "fmt" "io" - "math/rand/v2" "strconv" "github.com/donislawdev/TestingFilesGenerator/internal/core" @@ -433,7 +432,7 @@ func writeFiller(ctx context.Context, w io.Writer, name string, seed uint64, siz return ctx.Err() default: } - fillBytes(buf[:n], rng) + core.FillRandomLE(buf[:n], rng) if err := writeAll(w, buf[:n]); err != nil { return err } @@ -447,19 +446,10 @@ func writeFiller(ctx context.Context, w io.Writer, name string, seed uint64, siz func filler(seed uint64, size int64) []byte { out := make([]byte, size) - fillBytes(out, core.NewRand(seed)) + core.FillRandomLE(out, core.NewRand(seed)) return out } -func fillBytes(b []byte, rng *rand.Rand) { - for i := 0; i < len(b); i += 8 { - v := rng.Uint64() - for j := 0; j < 8 && i+j < len(b); j++ { - b[i+j] = byte(v >> (8 * uint(j))) - } - } -} - func writeUint32(w io.Writer, v uint32) error { var b [4]byte binary.LittleEndian.PutUint32(b[:], v) 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/format/zip/zip.go b/internal/format/zip/zip.go index 9b03496..423ad73 100644 --- a/internal/format/zip/zip.go +++ b/internal/format/zip/zip.go @@ -626,9 +626,7 @@ func writeFiller(ctx context.Context, w io.Writer, seed uint64, n int64) error { size = remaining } chunk := buf[:size] - for i := range chunk { - chunk[i] = byte(rng.UintN(256)) - } + core.FillRandomBE(chunk, rng) if _, err := w.Write(chunk); err != nil { return err } 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/fillerprimitive_test.go b/internal/guard/fillerprimitive_test.go new file mode 100644 index 0000000..11938cf --- /dev/null +++ b/internal/guard/fillerprimitive_test.go @@ -0,0 +1,113 @@ +package guard + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" +) + +// Bulk random bytes come from core, and no format keeps its own copy of the +// loop. +// +// Until 2026-09-06 thirteen packages each carried their own version of the same +// filler, in four different shapes, and the drift between them was not +// cosmetic. Three of them - zip, targz and wav - drew one byte per call to the +// generator and ran at 182 MB/s, which is slower than the disk they were +// feeding. Six drew eight bytes but through a temporary array. Four wrote the +// same eight bytes with a hand rolled shift loop. Measured over 64 MiB: the +// slow shape 182 MB/s, the temporary array 1468 MB/s, the shift loop 845 MB/s, +// and one store straight into the buffer 2499 MB/s. +// +// The performance report that started this named three of the thirteen. The +// other ten were found by grep, and two of the four shapes were nobody's +// finding at all. That is the argument for this guard rather than for a note in +// a document: the copies were not written by one person on one day, they +// accumulated one format at a time, and each one looked reasonable beside the +// format next to it. +// +// Uint64 is the bulk draw. Every one of the thirteen reached for it, so asking +// that no format calls it directly is asking the question the copies actually +// answer. UintN is here for the one legitimate caller, which picks a word out +// of a list rather than filling a buffer. +// +// What this does NOT see, stated because a guard that hides its edges is worse +// than none: a filler written with IntN, Uint32 or Float64 would walk straight +// past it. Two such loops already exist and are deliberately left alone - the +// salt and header fills in internal/format/archive, twelve and sixteen bytes +// once per archive, where the shape costs nothing and changing it would move +// bytes for no gain. +func TestBulkRandomBytesComeFromOnePlace(t *testing.T) { + // Every call to UintN allowed to live outside core, and why. An entry that + // stops naming real code is a failure below, not a comment nobody reads. + allowed := map[string]string{ + "internal/format/opc/opc.go": "picks a word from a list, it does not fill a buffer", + } + + seen := map[string]bool{} + + root := filepath.Join("..", "format") + err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() || !strings.HasSuffix(path, ".go") { + return nil + } + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, path, nil, 0) + if err != nil { + return err + } + rel := filepath.ToSlash(filepath.Join("internal", "format", + strings.TrimPrefix(filepath.ToSlash(path), filepath.ToSlash(root)+"/"))) + + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + where := fset.Position(call.Pos()) + switch sel.Sel.Name { + case "Uint64": + t.Errorf("%s:%d calls Uint64 directly.\n"+ + "Bulk random bytes come from core.FillRandomBE or core.FillRandomLE, so that "+ + "every format draws them the same way and at the same speed. Thirteen packages "+ + "each had their own copy of this loop until 2026-09-06, in four shapes, the "+ + "slowest of them thirteen times slower than the fastest. Call the core helper "+ + "whose byte order this format already writes - and if the order is genuinely "+ + "new, add it there rather than here.", rel, where.Line) + case "UintN": + if _, ok := allowed[rel]; !ok { + t.Errorf("%s:%d calls UintN directly.\n"+ + "If this fills a buffer, use core.FillRandomBE or core.FillRandomLE. If it "+ + "draws a single value for something else, add it to the allowed map above "+ + "with the reason.", rel, where.Line) + return true + } + seen[rel] = true + } + return true + }) + return nil + }) + if err != nil { + t.Fatalf("walking the format packages: %v", err) + } + + // An exception that outlived its code is an exception nobody granted. + for rel, why := range allowed { + if !seen[rel] { + t.Errorf("%s is listed as allowed to call UintN (%s), but it does not call it.\n"+ + "Delete the entry. A standing exception for code that has gone will quietly "+ + "cover the next thing that lands in that file.", rel, why) + } + } +} diff --git a/internal/guard/generatorbytes_test.go b/internal/guard/generatorbytes_test.go index 5b575b7..f79e752 100644 --- a/internal/guard/generatorbytes_test.go +++ b/internal/guard/generatorbytes_test.go @@ -191,6 +191,17 @@ func goldenCases() map[string]engine.Target { "jpg_192kib_many_segments": {ID: "g", Format: "jpg", Sizes: engine.Uniform(1, 196608), Label: true, Properties: map[string]string{"width": "64", "height": "64", "quality": "90"}}, "wav_32kib": {ID: "g", Format: "wav", Sizes: engine.Uniform(1, 32768), Label: true}, + + // WAV only pads when the audio does not land on the size exactly, and + // the padding is audio modulo the frame size - two bytes here, never a + // frame's worth. wav_32kib happens to be a size the audio fills + // exactly, so it walks past the filler without touching it, and until + // 2026-09-06 no case pinned that path at all. + // + // Found by measurement rather than by reading: making the filler emit a + // constant moved 33 of the 54 cases and not one of them was a WAV. + "wav_with_the_padding_chunk": {ID: "g", Format: "wav", Sizes: engine.Uniform(1, 102400), Label: true}, + "zip_16kib": {ID: "g", Format: "zip", Sizes: engine.Uniform(1, 16384), Label: true}, "md_8kib": {ID: "g", Format: "md", Sizes: engine.Uniform(1, 8192), Label: true}, "log_8kib": {ID: "g", Format: "log", Sizes: engine.Uniform(1, 8192), Label: true}, 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") + } +} diff --git a/internal/guard/testdata/generator-golden.json b/internal/guard/testdata/generator-golden.json index 504af8b..8c2ad46 100644 --- a/internal/guard/testdata/generator-golden.json +++ b/internal/guard/testdata/generator-golden.json @@ -164,8 +164,8 @@ }, "targz_past_the_comment_limit": { "bytes": 262144, - "sha256": "e0ea316d5fd0d3dee866a551660cd7626fc1902d5d36dfdb2e1ca8e240395e13", - "measured_on": "2026-09-01" + "sha256": "34dae598e18f6ab8f0abf07358353b014feb7b6e1f831dc6b0f3e0f84c4e6b1a", + "measured_on": "2026-09-06" }, "targz_with_three_pdfs": { "bytes": 65536, @@ -199,6 +199,11 @@ "bytes": 32768, "sha256": "7f1de29ead6fd51cad8675133054f77f760c9f867cc26351260e5340e5201b3a" }, + "wav_with_the_padding_chunk": { + "bytes": 102400, + "sha256": "55f453ce6cc46a6d856d62825327169683283b22b98b0a4e06effe3a3aaafffe", + "measured_on": "2026-09-06" + }, "webp_64kib": { "bytes": 65536, "sha256": "de9034bd6315f0a51fd8fe1b99adfa5719597f7c3d7cdaba1d3b8624754bf89e", @@ -234,15 +239,18 @@ }, "zip_16kib": { "bytes": 16384, - "sha256": "480b35575b9cec5bcdcdf02d066f4ad8b4b967969aecbeb2e49d7a451a14fdaf" + "sha256": "4e02a3eb7c2cad250ec98d4b03e1393b503c344cda1fc1d16c0a3296d9eed9a5", + "measured_on": "2026-09-06" }, "zip_past_the_comment_limit": { "bytes": 262144, - "sha256": "fae918bc961420dd675dd9a90005d319c93856d56b5c3f9aeeb1150baf41a6df" + "sha256": "19f8a63a28a0cd930caa95300bc4b70b164d35befde65bc919a3a9dbbbc37e7e", + "measured_on": "2026-09-06" }, "zip_with_three_pdfs": { "bytes": 65536, - "sha256": "2f45c46662ab65d2f598d65446ba0476067fb5c6297dc497d580a0fcd1752fbe" + "sha256": "fa73baa3d6fa5a3cdd9ee024c027c0b34e95664d2a99c3b6c15deea10a2a3751", + "measured_on": "2026-09-06" }, "csv_8kib_quote_all": { "bytes": 8192, @@ -315,6 +323,17 @@ "files": [ "csv_8kib_seventeen_columns" ] + }, + { + "on": "2026-09-06", + "why": "The three formats whose filler drew one byte per RNG call - zip, targz and wav - now draw eight, which is where their padding bytes come from and so is a breaking change under D11. Measured 2026-09-06, interleaved with the order reversed and the exit code checked on every run: zip 64 MB 2.81x, targz 64 MB 2.74-3.32x, ranges disjoint in both. The filler loop itself goes from 182 MB/s to 2499 MB/s over 64 MiB. Ten further packages moved onto the same core.FillRandomBE/LE and did NOT move a byte - checked across 24 formats by 5 sizes by 2 seeds, twenty formats identical. wav gains no speed at all and the owner took it anyway for uniformity: its padding is audio modulo the frame size, which is two bytes, so only two bytes of a WAV differ. That is also why wav_with_the_padding_chunk had to be added - wav_32kib lands on a size the audio fills exactly and never reaches the filler, so the path had no golden value at all.", + "files": [ + "targz_past_the_comment_limit", + "wav_with_the_padding_chunk", + "zip_16kib", + "zip_past_the_comment_limit", + "zip_with_three_pdfs" + ] } ] }