diff --git a/CHANGELOG.md b/CHANGELOG.md index f8f92ad..9205ce8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -349,6 +349,83 @@ because it turns other people's test suites red. `height` can be set, and naming one lets the other be worked out from the size. The smallest TIFF this produces is 183 B. +### Fixed + +- **A run of a few files is now weighed against the memory ceiling too.** The + ceiling that stops a run from planning more than it can hold only started + counting once a run asked for sixty four files, so a smaller run had no + ceiling at all - and it counted files across the whole run, so a recipe of + sixty four one file targets began counting after sixty three of them were + already planned. + + That was reachable with ordinary settings rather than with a contrived one. A + zip of ten thousand entries costs about 75 MB to plan, so twenty nine of them + come to 2.17 GB, past the ceiling and without a single check. Such a run is + now refused before anything is written, and the refusal says how far it had + got and what the ceiling is. + + Runs this tool was designed around are orders of magnitude under the ceiling + and are unaffected. Nothing about the files that are produced changes. + +- **A crash inside a generator now costs one file instead of the whole run.** + Until this release a defect in one of them ended the process, and it left the + file it was writing on the disk under its temporary name - a name `cleanup` + will not remove, because `cleanup` only removes what the manifest lists, and a + file that never finished never reached one. `verify` then reported it for + good. + + Such a crash is now an ordinary failure of one file: the rest of the run + carries on, the manifest says which file it was and what happened, the + temporary file is removed, and the run ends with the partial exit code. A + crash while planning ends the run instead, with the exit code that means this + tool has a defect rather than the recipe does. + + This is a safety net, not a licence. A crash is still a defect worth + reporting, and the message says so. + +- **Ctrl+C now stops `generate`, `validate` and `preset show` while they are + still planning.** Until this release they finished planning first and noticed + the key only afterwards, so a large batch could look frozen: ten thousand + pictures is about a minute and a half of planning before the first byte is + written, and all of it ignored the key. Under a CI timeout the grace period + ran out and the job was killed rather than shutting down. + + Writing was never affected. A run interrupted while producing files already + stopped promptly, saved its manifest and left no partial files behind. + +- **A recipe can no longer use up all the memory on the machine.** A document + of 40 kB that nested brackets twenty thousand deep took most of a gigabyte + before it was refused, and the size limit did not help - forty kilobytes is a + small fraction of what a recipe is allowed to be. + + Recipes now refuse to nest brackets and braces more than thirty two deep. + Nothing anybody writes comes close: a list such as `[1B, 1kb, 1mb]` is one + deep, and so is a target written out with braces. Brackets inside a quoted + value are text and are not counted. + +- **A compressed `zip` is now either produced at the size you asked for or + refused, never quietly written at the wrong length.** Two sizes did the + second thing, and both passed `--dry-run` first. + + Asking for a compressed zip at exactly the smallest size the tool reports + produced no file at all. That size is now refused, and the smallest + compressed zip is a little larger than the smallest stored one - the padding + entry a compressed archive needs costs a few bytes of its own. `tfg formats` + is unchanged, because it reports the stored floor and compression is off by + default. + + Asking for a zip whose contents are already compressed - other zips, Office + documents, pictures - could also land in a narrow band of sizes that could + not be produced. Deflate makes such data slightly larger rather than smaller, + and the padding could not shrink far enough to make up for it. Those sizes + are now refused with the smallest size that does work. + + Sizes that worked before are unaffected, byte for byte. + +- **A `validate --json` that is stopped no longer says the recipe is invalid.** + It never finished reading the recipe, so it has no verdict to report. The + exit code says what happened instead. + ## [0.2.0] - 2026-08-28 ### Breaking diff --git a/internal/cli/cli.go b/internal/cli/cli.go index bedb601..519f151 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -72,7 +72,7 @@ func Run(ctx context.Context, args []string, out, errOut io.Writer) int { case "generate": return generate(ctx, args[1:], out, errOut) case "validate": - return validate(args[1:], out, errOut) + return validate(ctx, args[1:], out, errOut) case "verify": return verify(ctx, args[1:], out, errOut) case "cleanup": @@ -80,7 +80,7 @@ func Run(ctx context.Context, args []string, out, errOut io.Writer) int { case "recipe": return recipeCmd(args[1:], out, errOut) case "preset": - return presetCmd(args[1:], out, errOut) + return presetCmd(ctx, args[1:], out, errOut) case "formats": return formats(args[1:], out, errOut) case "--version", "version": diff --git a/internal/cli/errors.go b/internal/cli/errors.go index fc8fdfd..d14c496 100644 --- a/internal/cli/errors.go +++ b/internal/cli/errors.go @@ -146,6 +146,13 @@ func classifyRequest(err error) (int, bool) { if errors.As(err, &recipeTooLarge) { return ExitRecipe, true } + // Same class, one more shape: a recipe whose brackets nest far enough to + // exhaust this machine. A fact about the document rather than a fault of + // ours, so it ends the way a recipe that will not parse does. + var recipeTooDeep *recipe.TooDeepError + if errors.As(err, &recipeTooDeep) { + return ExitRecipe, true + } // Two parts of one recipe saying different things about the same archive // is a recipe problem, like a boundary stated beside a size. var conflict *format.ContentsConflictError diff --git a/internal/cli/generate.go b/internal/cli/generate.go index 8dfa62f..c6c65de 100644 --- a/internal/cli/generate.go +++ b/internal/cli/generate.go @@ -372,7 +372,13 @@ func sizesFromFlags(g *generateOpts, errOut io.Writer) (sizes []int64, low, high // produce plans the run, writes it and reports what happened. func produce(ctx context.Context, targets []engine.Target, opt engine.Options, g *generateOpts, out, errOut io.Writer) int { - planned, err := engine.Plan(targets, opt) + // PlanContext rather than Plan, because planning is where the time goes for + // anything that encodes a picture and it is work somebody may want to stop. + // Measured 2026-09-02: about 8.5 ms a png, so ten thousand of them is about + // a minute and a half before a byte is written. preflight would notice the + // signal on the way to writing, which is late enough that Ctrl+C looks + // ignored and SIGTERM in CI runs out its grace period. + planned, err := engine.PlanContext(ctx, targets, opt) if err != nil { fmt.Fprintf(errOut, "tfg: %s\n", describeError(err)) return classify(err) diff --git a/internal/cli/preset.go b/internal/cli/preset.go index 4144e7a..c98b8ab 100644 --- a/internal/cli/preset.go +++ b/internal/cli/preset.go @@ -2,6 +2,7 @@ package cli import ( + "context" "flag" "fmt" "io" @@ -49,7 +50,7 @@ type budget struct { Formats []string `json:"formats"` } -func budgetOf(e *preset.Expansion) (budget, error) { +func budgetOf(ctx context.Context, e *preset.Expansion) (budget, error) { rec, err := recipe.Parse(e.Source, e.Preset.ID) if err != nil { return budget{}, err @@ -60,7 +61,7 @@ func budgetOf(e *preset.Expansion) (budget, error) { targets = append(targets, engineTarget(t, t.Label)) seen[t.Format] = true } - planned, err := engine.Plan(targets, engine.Options{OutDir: rec.Output.Dir, Seed: rec.Seed}) + planned, err := engine.PlanContext(ctx, targets, engine.Options{OutDir: rec.Output.Dir, Seed: rec.Seed}) if err != nil { return budget{}, err } @@ -338,13 +339,13 @@ func targetsFromPreset(fs *flag.FlagSet, g *generateOpts, given map[string]bool, return targetsFromParsedRecipe(rec, hash, g, given, opt), ExitOK } -func presetCmd(args []string, out, errOut io.Writer) int { +func presetCmd(ctx context.Context, args []string, out, errOut io.Writer) int { if len(args) > 0 { switch args[0] { case "list": return presetList(args[1:], out, errOut) case "show": - return presetShow(args[1:], out, errOut) + return presetShow(ctx, args[1:], out, errOut) case "eject": return presetEject(args[1:], out, errOut) } @@ -483,7 +484,7 @@ Flags: return ExitOK } -func presetShow(args []string, out, errOut io.Writer) int { +func presetShow(ctx context.Context, args []string, out, errOut io.Writer) int { usage := func(w io.Writer) { fmt.Fprint(w, `tfg preset show - what a preset takes and what it would produce. @@ -502,7 +503,7 @@ Usage: return code } - b, err := budgetOf(expanded) + b, err := budgetOf(ctx, expanded) if err != nil { fmt.Fprintf(errOut, "tfg: %s\n", describeError(err)) return classify(err) diff --git a/internal/cli/recipecmd.go b/internal/cli/recipecmd.go index aff29ad..aa8ef7d 100644 --- a/internal/cli/recipecmd.go +++ b/internal/cli/recipecmd.go @@ -3,6 +3,7 @@ package cli import ( "bytes" + "context" "errors" "flag" "fmt" @@ -36,7 +37,7 @@ func loadRecipe(path string, errOut io.Writer) (*recipe.Recipe, string, int) { // validate runs the checks a run would run and writes nothing at all, so it // suits a pre commit hook. -func validate(args []string, out, errOut io.Writer) int { +func validate(ctx context.Context, args []string, out, errOut io.Writer) int { fs := flag.NewFlagSet("validate", flag.ContinueOnError) fs.SetOutput(errOut) asJSON := fs.Bool("json", false, "write the result as JSON, with every problem separately") @@ -79,14 +80,9 @@ func validate(args []string, out, errOut io.Writer) int { for _, t := range rec.Targets { targets = append(targets, engineTarget(t, t.Label)) } - planned, err := engine.Plan(targets, planningOptions(rec)) + planned, err := engine.PlanContext(ctx, targets, planningOptions(rec)) if err != nil { - if *asJSON { - return writeJSON(errOut, errOut, validateReport{Recipe: path, Valid: false, - Problems: []validateProblem{problemOf(err)}}, classify(err)) - } - fmt.Fprintf(errOut, "tfg: %s\n", describeError(err)) - return classify(err) + return planningRefusal(err, path, *asJSON, errOut) } if *asJSON { @@ -103,6 +99,25 @@ func validate(args []string, out, errOut io.Writer) int { return ExitOK } +// planningRefusal reports a plan that did not finish, and gives the exit code +// for it. +// +// The stopped case is told apart because a run that was stopped has no verdict +// about the recipe and must not print one. "valid: false" is a claim about the +// file, and all that happened is that somebody pressed Ctrl+C - a consumer +// reading that one field would act on a recipe this never finished reading. The +// exit code says what really happened. Untouchable rule 5 in the other surface: +// do not claim a certainty there is none of. +func planningRefusal(err error, path string, asJSON bool, errOut io.Writer) int { + stopped := errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) + if asJSON && !stopped { + return writeJSON(errOut, errOut, validateReport{Recipe: path, Valid: false, + Problems: []validateProblem{problemOf(err)}}, classify(err)) + } + fmt.Fprintf(errOut, "tfg: %s\n", describeError(err)) + return classify(err) +} + // planningOptions is what this command hands the planner. // // The manifest name is in it, and leaving it out was a hole this command exists diff --git a/internal/core/diskspace.go b/internal/core/diskspace.go new file mode 100644 index 0000000..c898c8e --- /dev/null +++ b/internal/core/diskspace.go @@ -0,0 +1,38 @@ +package core + +import "math" + +// AvailableFrom turns what the system said about free space into a number of +// bytes this program can work with. +// +// The two platform files below both had the same shape at the end of them: a +// number the system reports as unsigned, multiplied or converted into an int64 +// and handed on. On a filesystem of eight exbibytes or more that conversion +// wraps and free space comes back NEGATIVE, which the free space check reads as +// a disk smaller than any run - so the tool would refuse to write to the +// largest disk it will ever meet. +// +// Nobody has such a disk. It is here as a pure function rather than as a +// condition inside the syscall because a condition in there is one nothing +// could ever turn red, and this project has taken seven of those back out. As a +// function it can be asked directly, which is what a guard does. +// +// Saturating rather than erroring, and that is the truthful answer rather than +// the convenient one: a disk too large to count in int64 is a disk with more +// room than any run this tool can plan, so the largest number is what the +// caller should hear. An error would be read as "this disk cannot be measured", +// which is a different thing and is already what a failing syscall means. +func AvailableFrom(blocks, blockSize uint64) int64 { + if blockSize == 0 { + // A block size of nought would make the multiplication below divide by + // nought in its own overflow check. No filesystem reports it, and the + // answer if one did is that we know nothing about the space rather + // than that there is none - but nought is what a failing syscall + // already returns beside its error, so it is what this returns too. + return 0 + } + if blocks > uint64(math.MaxInt64)/blockSize { + return math.MaxInt64 + } + return int64(blocks * blockSize) +} diff --git a/internal/core/diskspace_unix.go b/internal/core/diskspace_unix.go index e879efb..13944f4 100644 --- a/internal/core/diskspace_unix.go +++ b/internal/core/diskspace_unix.go @@ -20,8 +20,10 @@ func AvailableBytes(path string) (int64, error) { return 0, err } // Blocks available to an unprivileged user, which is the number that - // matters to us rather than the total free on the device. - return int64(st.Bavail) * int64(st.Bsize), nil + // matters to us rather than the total free on the device. The arithmetic + // is next door because it is the half that can be wrong on its own, and a + // check written in here would be one nothing could ever reach. + return AvailableFrom(uint64(st.Bavail), uint64(st.Bsize)), nil } // existingAncestor walks up until it finds a directory that exists, because diff --git a/internal/core/diskspace_windows.go b/internal/core/diskspace_windows.go index d854faa..50c81cc 100644 --- a/internal/core/diskspace_windows.go +++ b/internal/core/diskspace_windows.go @@ -34,7 +34,10 @@ func AvailableBytes(path string) (int64, error) { if r == 0 { return 0, e } - return int64(freeForCaller), nil + // Already in bytes, so the block size is one - but through the same + // function as the other platform, because the conversion from unsigned is + // where both of them could hand back a negative. + return AvailableFrom(freeForCaller, 1), nil } // The handle is looked up once rather than on every call. It was built inside diff --git a/internal/core/records.go b/internal/core/records.go index 89dd1b6..0cba32b 100644 --- a/internal/core/records.go +++ b/internal/core/records.go @@ -153,6 +153,22 @@ func AppendFiller(dst []byte, words []string, n int64, separator func(i int) str // which mistake it is beats a runtime panic with no name on it. panic("core: AppendFiller was given no words to pad with") } + if !wordsHaveBytes(words) { + // The same mistake spelled differently, and it ends worse. The loop + // below stops only by adding bytes, so a vocabulary that adds none + // spins without end and without growing - the one failure shape a size + // guard cannot see, because no file is ever produced to measure. + // FillRecords above names it in those words and refuses. This one did + // not, which made it a difference between two halves of one primitive + // rather than a hypothetical. + // + // The question is about the VOCABULARY rather than about each word, + // and that distinction is the whole of the check. One word with bytes + // in it ends the loop whatever the separator returns, because every + // cycle through the vocabulary appends it - so asking per iteration + // would refuse ["", "ab"], which pads perfectly well. + panic("core: AppendFiller was given words that are all empty, so it could never reach the length asked for") + } start := len(dst) for i := 0; int64(len(dst)-start) < n; i++ { @@ -201,3 +217,17 @@ func WriteAll(w io.Writer, b []byte) error { } return nil } + +// wordsHaveBytes says whether a vocabulary can add anything at all. +// +// One word is enough, whatever the separator does with the rest, because the +// loop in AppendFiller walks the vocabulary in a cycle and reaches that word +// every time round. +func wordsHaveBytes(words []string) bool { + for _, w := range words { + if w != "" { + return true + } + } + return false +} diff --git a/internal/engine/crash.go b/internal/engine/crash.go new file mode 100644 index 0000000..0ea8740 --- /dev/null +++ b/internal/engine/crash.go @@ -0,0 +1,99 @@ +package engine + +import ( + "context" + "fmt" + "io" + + "github.com/donislawdev/TestingFilesGenerator/internal/format" +) + +// GeneratorCrashError is a generator that stopped the way a program stops when +// it has a defect in it, rather than the way this tool refuses something. +// +// Until 2026-09-03 there was one recover in this whole tree, around the yaml +// reader, and the argument that put it there applies to generators word for +// word: a crash in there is a crash on ordinary user input. Two formats hand +// their pixels to somebody else's encoder, and every format runs on numbers +// that came from a recipe. +// +// What the crash cost was worse than the crash. A panic while writing left the +// file under its temporary name, and untouchable rule 7 makes the manifest the +// whole authority over what cleanup may delete - so a file that never reached +// one is a file nothing in this tool can ever take away. verify then reports it +// for good. +// +// While says which half of the work it happened in, because the two end +// differently. Planning is the whole run, so the run does not start. Writing is +// one file, so the file is dropped and the rest of the run carries on, which is +// what every other failure of a single file does. +type GeneratorCrashError struct { + Format string + While string + Name string + Value any +} + +func (e *GeneratorCrashError) Error() string { + where := e.While + if e.Name != "" { + where += " " + e.Name + } + return fmt.Sprintf( + "the %s generator stopped with an internal error while %s: %v. "+ + "This is a defect in this tool rather than something a recipe can ask for. "+ + "Please report it with the settings that produced it", + e.Format, where, e.Value) +} + +// planWithoutCrashing asks a generator what it would produce and turns a panic +// into an ordinary error. +// +// This is the likelier of the two places, which is not where the report that +// asked for this was looking. The picture formats encode while PLANNING - jxl +// and avif walk a ladder of sizes and hand each rung to a borrowed encoder - +// so a defect in one of them lands here rather than in the write below. A +// container planning its children calls them from inside its own Plan, so a +// child crashing is caught here too - and named as the container, because the +// container is what this was asked to plan. +func planWithoutCrashing(desc format.Descriptor, r format.Request) (p format.Plan, err error) { + defer func() { + v := recover() + if v == nil { + return + } + err = &GeneratorCrashError{Format: desc.ID, While: "planning a file", Value: v} + }() + return desc.Generator.Plan(r) +} + +// writeWithoutCrashing is the same around the write, so a crash costs one file +// instead of the process. +// +// Only the generator is wrapped. What follows it - the flush, the close, the +// rename - is this package's own code, and the close and the remove it already +// runs are what take the temporary file away once the panic has become an +// error. A defence around code that cannot crash is a defence nothing can turn +// red, and this project has taken seven of those back out. +// +// Two things this cannot catch, and both are worth naming rather than +// implying. A panic on another goroutine answers to that goroutine alone, so a +// generator that starts one takes the process with it. And a memory violation +// is not a panic at all - the AVX2 path in the AVIF encoder reads past its +// buffer, which is why .github/build-tags exists. +// +// One thing it catches and names wrongly: the progress callback runs inside the +// generator, through the counting writer, so a caller whose callback crashes is +// told its generator did. The window's callback posts to a channel and does +// nothing else, and a wrapper of its own to tell the two apart would be more +// machinery than the mistake is worth. +func writeWithoutCrashing(ctx context.Context, f PlannedFile, w io.Writer) (err error) { + defer func() { + v := recover() + if v == nil { + return + } + err = &GeneratorCrashError{Format: f.Desc.ID, While: "writing", Name: f.Name, Value: v} + }() + return f.Desc.Generator.Write(ctx, w, f.Plan) +} diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 2986016..f449a0e 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -87,7 +87,7 @@ type Target struct { // here. A copy would be a place for the two to disagree, and the disagreement // would surface as a file that planning accepted and writing refused. func drawSizes(t *Target, desc format.Descriptor, targetSeed uint64) error { - if _, err := desc.Generator.Plan(format.Request{ + if _, err := planWithoutCrashing(desc, format.Request{ Bytes: t.SizeMin, Contains: t.Contains, Seed: core.FileSeed(targetSeed, 0), @@ -332,9 +332,9 @@ func PlanContext(ctx context.Context, targets []Target, opt Options) ([]PlannedF // space check as a negative requirement and satisfied it. var totalFiles int - // Watches what the plan costs while it is being built. Started here rather - // than at the first file so that the baseline is taken before any of it - // exists. + // Watches what the plan costs while it is being built. Constructed here + // rather than at the first file because constructing it is what takes the + // reference point, and that has to happen before any of the plan exists. pl.budget = newPlanMemory(opt.MaxPlanBytes) // The manifest lands beside the files, so its name is a name too. A path @@ -386,9 +386,6 @@ func PlanContext(ctx context.Context, targets []Target, opt Options) ([]PlannedF // Counted across every target, not per target. A ceiling on one target // alone is one somebody reaches by writing the number out in pieces. - // The budget is told the size of the target before its files are - // planned, so its baseline is taken before any of them exist. - pl.budget.expect(len(t.Sizes)) totalFiles += len(t.Sizes) if totalFiles > core.MaxFilesPerRun { // Addressed to the target that took the total past the ceiling, @@ -739,7 +736,7 @@ func writeOne(ctx context.Context, f PlannedFile, outDir string, report func(int buffered := bufio.NewWriterSize(fh, 64<<10) counter := &countingWriter{w: io.MultiWriter(buffered, h), report: report} - writeErr := f.Desc.Generator.Write(ctx, counter, f.Plan) + writeErr := writeWithoutCrashing(ctx, f, counter) if writeErr == nil { writeErr = buffered.Flush() } diff --git a/internal/engine/planmemory.go b/internal/engine/planmemory.go index 4b66e2d..54c2387 100644 --- a/internal/engine/planmemory.go +++ b/internal/engine/planmemory.go @@ -33,19 +33,6 @@ import ( // without being told about. const ( - // planCheckFirst is both when the first reading is taken and how many - // files a run needs before any of this happens at all. - // - // The second half is what keeps it cheap. A reading means collecting - // first, which measured 4.9 ms against 0.023 ms for the reading on its own - // - fifty rounds each, 2026-08-26 - and a test suite plans thousands of - // times, almost always a handful of files. Putting a collection on every - // plan took the guard package from 124 s past its ten minute limit. - // - // Sixty four files of the dearest shape measured is about 340 MB, well - // under the ceiling, so nothing that matters is missed by waiting. - planCheckFirst = 64 - // planCheckEvery is the longest this waits between readings. // // An upper bound rather than a fixed interval, and the arithmetic says @@ -65,46 +52,68 @@ const ( // target has been walked, and it needs the per file cost to be uniform, which // it is not - a recipe can put a thousand page pdf next to a text file. Asking // the real heap needs neither and cannot be wrong about the shape of the run. +// +// What it can be wrong about is whose heap it is. ReadMemStats answers for the +// whole process, so anything else allocating in it while a plan is being built +// is counted against the plan - O162. Two gigabytes is a great deal to borrow +// by accident, and every run this tool was designed around is orders of +// magnitude under the ceiling, so the room for a wrong refusal is small. It is +// not nought, and this is where it lives. type planMemory struct { baseline uint64 - started bool - expected int seen int nextAt int ceiling uint64 } +// newPlanMemory takes the reference point, and it is taken here because here +// is before the first target has been looked at. +// +// Both readings come from the same method - collected first - which is the +// part that matters: a baseline read without collecting counts whatever has +// not been swept yet, so it can be HIGHER than a collected reading taken +// later, and the growth between them comes out negative. That is not a small +// error in a number, it is the check quietly answering "nothing to refuse" for +// every run. +// +// Until 2026-09-03 this waited. The reading was taken only once a run had +// announced sixty four files, and account below returned at once until then, +// so a run of sixty three files had no ceiling at all. The comment that +// justified the wait said sixty four files of the dearest shape measured is +// about 340 MB, so nothing that matters is missed. Measured again with the +// plansize probe, this time on containers rather than on plain formats: +// +// zip, entries=10000, entry_format=pdf 74740758 B a file +// targz, the same 74758716 B a file +// pdf, pages=5000 25194908 B a file +// pdf, pages=1000 5244543 B a file +// +// Five points from one file to sixteen for the first shape, linear to within +// 0.05%. Twenty nine files of it come to 2.17 GB, which is past the ceiling +// and was accepted without a reading, because twenty nine is under sixty four. +// Sixty three files come to 4.71 GB. And the wait was worse than that number +// says, because the count it waited on was the whole run's, added up across +// targets: sixty four targets holding one file each took the reading after +// SIXTY THREE files had been planned, and those then sat inside the baseline +// for the rest of the run, however long it ran. +// +// What the waiting bought is measured rather than assumed, since the number +// that justified it - "putting a collection on every plan took the guard +// package from 124 s past its ten minute limit" - turns out not to describe +// this. A reading here and a first reading at the first file, four runs of the +// guard package interleaved on 2026-09-03: 277.9 s and 283.7 s without them +// against 292.3 s and 306.1 s with, so about six percent. The ranges do not +// overlap, which is the only reason a conclusion is drawn from four runs. func newPlanMemory(ceiling int64) *planMemory { if ceiling <= 0 { ceiling = core.MaxPlanBytes } - return &planMemory{nextAt: planCheckFirst, ceiling: uint64(ceiling)} -} - -// expect is told how many files a target is about to contribute, before any of -// them are planned. -// -// This is where the baseline is taken, and it is taken only once the run is -// known to be big enough to be worth measuring. Both readings then come from -// the same method - collected first - which is the part that matters: a -// baseline read without collecting counts whatever has not been swept yet, so -// it can be HIGHER than a collected reading taken later, and the growth -// between them comes out negative. That is not a small error in a number, it -// is the check quietly answering "nothing to refuse" for every run. -func (p *planMemory) expect(files int) { - p.expected += files - if !p.started && p.expected >= planCheckFirst { - p.baseline = heapInUse() - p.started = true - } + return &planMemory{baseline: heapInUse(), 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. func (p *planMemory) account(targetIndex, filesSoFar int) error { - if !p.started { - return nil - } p.seen++ if p.seen < p.nextAt { return nil @@ -157,6 +166,11 @@ func (p *planMemory) account(targetIndex, filesSoFar int) error { // Two readings of a heap that has not been collected differ by whatever the // allocator happened to be carrying, which for this purpose is noise larger // than the thing being measured. +// +// It costs what a collection costs, which measured 4.9 ms against 0.023 ms for +// the reading on its own - fifty rounds each, 2026-08-26. That is why the +// schedule above exists: a run of ten thousand files takes a handful of these +// rather than ten thousand. func heapInUse() uint64 { runtime.GC() var m runtime.MemStats diff --git a/internal/engine/plantarget.go b/internal/engine/plantarget.go index d926928..3b3bd73 100644 --- a/internal/engine/plantarget.go +++ b/internal/engine/plantarget.go @@ -35,7 +35,7 @@ func (pl *planning) files(ctx context.Context, t *Target, desc format.Descriptor for idx, size := range t.Sizes { fileSeed := core.FileSeed(targetSeed, idx) - p, err := desc.Generator.Plan(format.Request{ + p, err := planWithoutCrashing(desc, format.Request{ Bytes: size, SizeFromContents: t.SizeFromContents, Contains: t.Contains, diff --git a/internal/format/targz/compress.go b/internal/format/targz/compress.go index 581e68e..430ec6a 100644 --- a/internal/format/targz/compress.go +++ b/internal/format/targz/compress.go @@ -199,11 +199,20 @@ func belowMinimum(target, floor int64) error { // reachable refuses a compressed archive whose size the contents already // exceed, using the STORED arithmetic. // -// The stored number is the honest bound to check here even though the archive -// will be compressed. Compression only ever makes the contents smaller, so an -// archive that fits when stored fits when squeezed - and the stored number is -// the one the plan can work out without compressing anything, which is what -// keeps a preview cheap. +// A cheap early refusal rather than a proof, and the sentence that used to +// stand here claimed the second thing. It said "compression only ever makes the +// contents smaller, so an archive that fits when stored fits when squeezed". +// That is false, and it is the exact assumption zip fell over on: deflate GROWS +// data that is already compressed. Measured on 2026-09-02 with two Office +// documents inside a zip, a band 50 B wide at 2 MB where the space compression +// freed came out negative and the file was written short of its size. +// +// What keeps this format to the byte is not this check. It is +// settleCompressed, which measures the real stream and iterates until the +// padding lands, and refuses when it cannot. This one answers only "even +// stored, the contents already exceed the size asked for", which is worth +// having because it is the answer a preview can give without compressing +// anything. func reachable(m *memo, target int64, label string, groups []format.Content) error { probe := *m probe.squeeze = archive.Squeeze{} diff --git a/internal/format/zip/compress.go b/internal/format/zip/compress.go index 044a157..dac4808 100644 --- a/internal/format/zip/compress.go +++ b/internal/format/zip/compress.go @@ -30,7 +30,7 @@ import ( // the two differ by a number the writer can measure and the plan does not have // to predict. func padCompressed(m *memo, p *format.Plan, r format.Request, groups []format.Content) error { - m.withFiller = true + m.withFiller, m.target = true, r.Bytes withFiller, err := archiveSize(*m) if err != nil { return err diff --git a/internal/format/zip/filler.go b/internal/format/zip/filler.go index 481a401..d65b072 100644 --- a/internal/format/zip/filler.go +++ b/internal/format/zip/filler.go @@ -3,7 +3,10 @@ package zip import ( stdzip "archive/zip" "context" + "fmt" "io" + + "github.com/donislawdev/TestingFilesGenerator/internal/format" ) // writeFillerEntry puts the padding entry into the archive. @@ -79,7 +82,35 @@ func writeCompressedFiller(ctx context.Context, zw *stdzip.Writer, m memo, withC // writing its bytes here would count them twice. return nil } - if err := writeFiller(ctx, entry, m.seed, m.fillerSize+freed()); err != nil { + // What the compressor freed can be NEGATIVE, and that is the case this + // arithmetic was written without. Deflate grows data it cannot shrink - + // about five bytes for every 65535 - so an archive holding already + // compressed files comes out of the compressor LARGER than it went in. + // The filler then has to shrink to compensate, and below zero it cannot. + // + // Left alone, writeFiller's loop simply wrote nothing and the archive came + // out too long with no error at all - measured 2026-09-02 as a band 50 B + // wide holding two 1 MB docx entries, which the engine reported as this + // generator disagreeing with its own plan. That reads as a broken tool + // rather than as an impossible size, and a person cannot act on it. + // + // Refused here rather than while planning, which is where targz refuses + // the same thing for the same reason: how far contents squeeze is not + // knowable without squeezing them, and planning is not allowed to. The + // floor reported is measured rather than derived - it is what this + // archive came to with no padding at all. + size := m.fillerSize + freed() + if size < 0 { + return &format.BelowMinimumError{ + Format: "ZIP", + Requested: m.target, + Minimum: m.target - size, + Reason: "these contents come out of the compressor larger than they went in, " + + "so the archive cannot be made this small once they are squeezed", + Hint: fmt.Sprintf("Ask for %d B or more, or ask for compression: none.", m.target-size), + } + } + if err := writeFiller(ctx, entry, m.seed, size); err != nil { return err } return shut() diff --git a/internal/format/zip/zip.go b/internal/format/zip/zip.go index 88181c9..9b03496 100644 --- a/internal/format/zip/zip.go +++ b/internal/format/zip/zip.go @@ -119,6 +119,11 @@ type memo struct { // works out the padding for the STORED archive, and the writer adds back // exactly what the compressor freed - see build. squeeze archive.Squeeze + // target is the size that was ordered, carried so the writer can say what + // the smallest archive would have been when it turns out this one cannot be + // made. Only the compressed path needs it: a stored archive settles its + // padding while planning, so a size it cannot reach is refused there. + target int64 } func (generator) Plan(r format.Request) (format.Plan, error) { @@ -332,7 +337,14 @@ func pad(m *memo, p *format.Plan, r format.Request, target, bare int64, label st Reason: fmt.Sprintf("an archive holding %s already needs that much", describeGroups(groups)), Hint: fmt.Sprintf("Ask for %d B or more, or hold fewer or smaller files.", bare), } - case target == bare: + // Exactly the bare size, and nothing to add - but only when the entries + // are stored. A squeezed archive comes out SHORTER than its stored + // arithmetic says, and the filler entry is the only thing that can give + // those bytes back. Returning here leaves withFiller false, so the + // writer has nowhere to put them and the archive ends up short: measured + // 2026-09-02 at 8382 B, where every level but none produced about 2.6 kB + // and --dry-run had already said the size was fine. + case target == bare && !m.squeeze.On(): return nil // Nothing to pad. } diff --git a/internal/guard/arithmeticedges_test.go b/internal/guard/arithmeticedges_test.go new file mode 100644 index 0000000..995568d --- /dev/null +++ b/internal/guard/arithmeticedges_test.go @@ -0,0 +1,98 @@ +package guard + +import ( + "fmt" + "math" + "strings" + "testing" + "time" + + "github.com/donislawdev/TestingFilesGenerator/internal/core" +) + +// Free space too large to count comes back as the largest number rather than as +// a negative one. +// +// Both platform files ended in the same shape: a count the system reports +// unsigned, turned into an int64 and handed to the free space check. At eight +// exbibytes that conversion wraps, so the check would read the largest disk it +// will ever meet as smaller than any run and refuse to write to it. +// +// Nobody has such a disk, and that is the argument for how this is written +// rather than a reason to leave it alone. A condition inside the syscall is one +// nothing could ever turn red - this project has taken seven of those back out +// - so the arithmetic is a function that can be asked directly, and this asks +// it. +func TestFreeSpaceTooLargeToCountIsNotANegativeNumber(t *testing.T) { + cases := []struct { + blocks, size uint64 + want int64 + }{ + {0, 4096, 0}, + {1, 4096, 4096}, + {1024, 4096, 4 << 20}, + // Where int64 stops, in blocks of four kilobytes. + {math.MaxUint64 / 4096, 4096, math.MaxInt64}, + {math.MaxUint64, math.MaxUint64, math.MaxInt64}, + // A block size no filesystem reports. The answer is that we know + // nothing about the space rather than that there is none, which is + // what a failing syscall already says, and it is here so the overflow + // check above does not divide by nought. + {1024, 0, 0}, + } + + for _, c := range cases { + got := core.AvailableFrom(c.blocks, c.size) + if got < 0 { + t.Errorf("AvailableFrom(%d, %d) is %d, and the free space check reads a negative as a disk too small for anything", + c.blocks, c.size, got) + continue + } + if got != c.want { + t.Errorf("AvailableFrom(%d, %d) = %d, expected %d", c.blocks, c.size, got, c.want) + } + } +} + +// A filler with nothing to add says so instead of spinning for ever. +// +// AppendFiller ends its loop only by adding bytes, so a vocabulary of empty +// strings never reaches the length asked for - and with a separator that +// returns nothing either, the loop neither ends nor grows. That is the one +// failure shape a size guard cannot see, because no file is ever produced to +// measure. FillRecords three functions up names exactly this and says so in a +// comment. Its sister did not, which made it a difference between two halves of +// one primitive rather than a hypothetical. +func TestAFillerWithNothingToAddRefusesRatherThanSpinning(t *testing.T) { + done := make(chan any, 1) + go func() { + defer func() { done <- recover() }() + core.AppendFiller(nil, []string{"", ""}, 16, func(int) string { return "" }) + }() + + select { + case v := <-done: + if v == nil { + t.Fatal("AppendFiller came back without refusing, so it either padded with nothing or lost the length it was asked for") + } + if said := fmt.Sprint(v); !strings.Contains(said, "empty") { + t.Errorf("the refusal does not say what was wrong with the words it was handed: %s", said) + } + case <-time.After(5 * time.Second): + t.Fatal("AppendFiller did not come back in five seconds, and its loop ends only by adding bytes") + } +} + +// A vocabulary that merely CONTAINS an empty word still pads to the byte. +// +// The refusal above has to be about the vocabulary having nothing at all to +// say, not about any one word being empty. One word with bytes in it ends the +// loop whatever the separator does, because every cycle through the vocabulary +// appends it - so a check per word would refuse something legal, and this is +// the guard that says so. +func TestAFillerWithOneRealWordAmongEmptyOnesStillPadsToTheByte(t *testing.T) { + out := core.AppendFiller(nil, []string{"", "ab", ""}, 16, func(int) string { return "" }) + if len(out) != 16 { + t.Fatalf("the padding came to %d B and 16 were asked for", len(out)) + } +} diff --git a/internal/guard/codeshape_test.go b/internal/guard/codeshape_test.go index 42847e1..2c92515 100644 --- a/internal/guard/codeshape_test.go +++ b/internal/guard/codeshape_test.go @@ -35,7 +35,11 @@ const ( // choosing: comments and blanks run 17 to 45 lines in the longest // functions, so counting them would have punished the wrong thing. longestFunction = 79 - longestFile = 503 + // 503 until 2026-09-03. engine.go lost the line that told the plan budget + // 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 // 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/generatorcrash_test.go b/internal/guard/generatorcrash_test.go new file mode 100644 index 0000000..9e5b2fe --- /dev/null +++ b/internal/guard/generatorcrash_test.go @@ -0,0 +1,171 @@ +package guard + +import ( + "context" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/donislawdev/TestingFilesGenerator/internal/core" + "github.com/donislawdev/TestingFilesGenerator/internal/engine" + "github.com/donislawdev/TestingFilesGenerator/internal/format" + _ "github.com/donislawdev/TestingFilesGenerator/internal/format/all" +) + +// A generator that crashes costs one file, not the process and not the +// directory. +// +// There was one recover in this whole tree until 2026-09-03, around the yaml +// reader, and the argument that put it there is the same one here: a crash in +// there is a crash on ordinary user input. Two formats hand their pixels to +// somebody else's encoder, and every format runs on numbers that came from a +// recipe. +// +// The crash is not the expensive part. writeOne creates the file under a +// temporary name and renames it into place, so a panic left that name on the +// disk - and untouchable rule 7 makes the manifest the whole authority over +// what cleanup may delete, so a file that never reached one is a file nothing +// in this tool can ever take away. verify reports it from then on and no +// command ends it. +// +// So the assertion that earns this guard is the one about the leftover: after +// a crash the directory holds what it would have held if the file had been +// refused. +func TestAGeneratorThatCrashesWhileWritingCostsOneFile(t *testing.T) { + dir := t.TempDir() + opt := engine.Options{OutDir: dir, Seed: 7, Command: "test"} + targets := []engine.Target{ + txtTarget("broken", 1, 4096), + txtTarget("good", 1, 4096), + } + + planned, err := engine.Plan(targets, opt) + if err != nil { + t.Fatalf("planning: %v", err) + } + if len(planned) != 2 { + t.Fatalf("planned %d files, expected 2", len(planned)) + } + // The real descriptor carrying the real plan, with only the writing half + // swapped for one that stops the way a defect stops a program. + planned[0].Desc.Generator = crashingGenerator{Generator: planned[0].Desc.Generator} + + res, err := engine.Run(context.Background(), planned, opt) + if err != nil { + t.Fatalf("the run ended on the crash instead of carrying on: %v", err) + } + if res.Failures != 1 { + t.Errorf("the run reports %d failures, expected 1", res.Failures) + } + + if _, err := os.Stat(filepath.Join(dir, planned[0].Name)); !os.IsNotExist(err) { + t.Errorf("%s is on the disk and the generator never finished writing it", planned[0].Name) + } + if _, err := os.Stat(filepath.Join(dir, planned[1].Name)); err != nil { + t.Errorf("the file beside the crash was not produced: %v", err) + } + + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("reading %s: %v", dir, err) + } + for _, e := range entries { + if core.IsPartialName(e.Name()) { + t.Errorf("%s was left behind, and nothing in this tool can ever remove it", e.Name()) + } + } + + // The manifest says what happened in the words of what happened. Swallowing + // the panic and letting the size check answer instead gives "generator for + // txt produced 0 B where the plan said 4096 B", which describes a symptom + // and blames the wrong thing. + said := "" + for _, f := range res.Manifest.Files { + if f.ID != planned[0].ID { + continue + } + said = f.Error + if !f.Failed { + t.Errorf("the manifest does not mark %s as failed", f.Name) + } + } + if said == "" { + t.Fatal("the manifest carries no error for the file that crashed") + } + for _, want := range []string{"txt", "internal error", crashWord} { + if !strings.Contains(said, want) { + t.Errorf("the manifest entry does not say %q: %s", want, said) + } + } +} + +// Both places the engine hands work to a generator go through that wrapper. +// +// Asked of the source, because there is no way to ask it of a run. The +// planning side looks its descriptor up in the registry, so a test cannot hand +// it a generator that crashes without registering one - and a registered +// format is visible to every other guard in this package, which ask that every +// format carries a card, a layer, an oracle and a mutation. The write side is +// proven above by crashing it for real, and the planning side is held to going +// through the same wrapper. +// +// Planning is the likelier of the two, which is worth saying because the +// review that asked for this was looking at the other one: jxl and avif encode +// while PLANNING, walking a ladder of sizes and handing each rung to a +// borrowed encoder. +func TestEveryCallIntoAGeneratorGoesThroughTheCrashWrapper(t *testing.T) { + const wrapper = "crash.go" + calls := []string{"Generator.Plan(", "Generator.Write("} + + dir := filepath.Join(repoRoot(t), "internal", "engine") + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("reading %s: %v", dir, err) + } + read := 0 + for _, e := range entries { + if e.IsDir() || !strings.HasSuffix(e.Name(), ".go") || e.Name() == wrapper { + continue + } + body, err := os.ReadFile(filepath.Join(dir, e.Name())) + if err != nil { + t.Fatalf("reading %s: %v", e.Name(), err) + } + read++ + for _, call := range calls { + if strings.Contains(string(body), call) { + t.Errorf("%s calls %s itself, so a panic in there ends the process", + e.Name(), strings.TrimSuffix(call, "(")) + } + } + } + if read == 0 { + t.Fatal("the scan read no file, so this guard would pass against anything") + } + + // And the wrapper still makes those calls, or the rule above is satisfied + // by a build that never reaches a generator at all. + body, err := os.ReadFile(filepath.Join(dir, wrapper)) + if err != nil { + t.Fatalf("reading %s: %v", wrapper, err) + } + for _, call := range calls { + if !strings.Contains(string(body), call) { + t.Errorf("%s does not call %s, so nothing in the engine does", wrapper, call) + } + } +} + +// crashWord is the panic the generator below raises, and the manifest has to +// carry it through. Spelled once so the assertion and the crash cannot drift. +const crashWord = "reachable only by crashing on purpose" + +// crashingGenerator plans the way the real format does and stops the way a +// defect stops a program. +type crashingGenerator struct{ format.Generator } + +func (crashingGenerator) Write(context.Context, io.Writer, format.Plan) error { + panic(crashWord) +} diff --git a/internal/guard/planmemory_test.go b/internal/guard/planmemory_test.go index 871c9be..ee657aa 100644 --- a/internal/guard/planmemory_test.go +++ b/internal/guard/planmemory_test.go @@ -97,6 +97,85 @@ func TestAnOrdinaryRunIsNotRefusedByThePlanCeiling(t *testing.T) { } } +// A run too short to have been weighed is weighed. +// +// planMemory used to take its reference point only once a run had announced +// sixty four files, and account returned at once until it had one - so a run of +// sixty three files had no ceiling at all. The count it waited on was the whole +// run's, added up across targets, which made it worse than that sounds: sixty +// four targets holding one file each took the reference point after SIXTY THREE +// files had already been planned, and those then sat inside the baseline for +// the rest of the run, however long it ran. +// +// What makes that reachable rather than theoretical was measured on 2026-09-03 +// with tools/probes/plansize: a zip of ten thousand pdf entries costs 74 740 +// 758 B of plan a file, five points from one file to sixteen, linear to within +// 0.05%. Twenty nine of those come to 2.17 GB against a ceiling of two, and +// twenty nine is under sixty four. +// +// Asked here with ONE file, which is the sharpest way to put the rule: the +// reference point exists before the first file rather than after the sixty +// fourth. The ceiling is injected for the same reason as the guard above, and +// the shape is a five thousand page pdf, measured at 25 194 908 B of plan - so +// one file passes a 4 MB ceiling six times over, which is clear of the +// megabytes of noise a heap reading carries in a process that has already run +// four hundred other tests. +func TestARunTooShortToHaveBeenWeighedIsWeighed(t *testing.T) { + 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("one file whose plan is six times the ceiling was accepted, so nothing weighed it") + } + + said := err.Error() + for _, want := range []string{"plan", "ceiling"} { + if !strings.Contains(said, want) { + t.Errorf("the refusal does not mention %q: %s", want, said) + } + } + // Where the run had got to, which for a single file is the half of the + // sentence that says the check ran at all. + if !strings.Contains(said, "after 1 file") { + t.Errorf("the refusal does not say the plan was weighed at the first file: %s", said) + } +} + +// The same shape one setting down still plans. +// +// Without this the guard above passes on a build that refuses every short run, +// and a check nothing can turn green is worth as little as one nothing can turn +// red. +func TestAShortOrdinaryRunIsNotRefusedByThePlanCeiling(t *testing.T) { + targets := []engine.Target{{ + ID: "one", + Format: "pdf", + Sizes: engine.Uniform(1, 3<<20), + Properties: map[string]string{"pages": "1000"}, + }} + + files, err := engine.Plan(targets, engine.Options{ + OutDir: "out", + ManifestName: "manifest.json", + // Zero means the real ceiling, which is what every caller passes. + }) + if err != nil { + t.Fatalf("a single thousand page pdf was refused under the real ceiling: %v", err) + } + if len(files) != 1 { + t.Fatalf("planned %d files, expected 1", len(files)) + } +} + // The ceiling this build works to is the one core states. // // Asked separately from the mechanism above, because that one runs against an diff --git a/internal/guard/planningstops_test.go b/internal/guard/planningstops_test.go new file mode 100644 index 0000000..e4a6e18 --- /dev/null +++ b/internal/guard/planningstops_test.go @@ -0,0 +1,179 @@ +package guard + +import ( + "bytes" + "context" + "strings" + "testing" + + "github.com/donislawdev/TestingFilesGenerator/internal/cli" +) + +// Planning is work somebody may want to stop, and until 2026-09-02 the command +// line could not be stopped while it did it. +// +// engine.PlanContext exists for exactly this and asks ctx.Err() once per file - +// internal/engine/plantarget.go says why it is asked there rather than per +// target, "one target of ten thousand pictures is the case this is for". Only +// the window called it. All three planning call sites in the command line +// called engine.Plan, which is PlanContext(context.Background(), ...), so the +// context the process had already built for the signal reached the writing and +// stopped at the planning. +// +// Measured that day on this machine, --dry-run so nothing is written: 200 pngs +// 1.66 s, 500 4.18 s, 1000 8.20 s, 2000 17.03 s. About 8.5 ms a file, so ten +// thousand pictures is about a minute and a half of a signal being ignored, +// and under SIGTERM in CI the grace period runs out and SIGKILL arrives. +// +// The two halves are the whole design. A finished context on its own would +// pass for a command that never reached planning at all - a typo in a flag +// ends long before any of this - so each surface is asked twice: once with a +// live context, where it has to succeed, and once with a finished one, where +// it has to end with the interrupt code. +// +// generate is deliberately NOT here, and that omission is the finding this +// guard was written by. It passes this shape already, and for the wrong +// reason: planning runs to the end ignoring the context, and preflight then +// notices the context on its way to writing. Green without reaching the thing +// being guarded. It is asked in the test below instead, where the two answers +// differ. +func TestPlanningFromTheCommandLineCanBeStopped(t *testing.T) { + cases := []struct { + name string + args func(recipePath string) []string + }{ + {"validate", func(p string) []string { return []string{"validate", p} }}, + {"preset show", func(string) []string { return []string{"preset", "show", "size-boundaries"} }}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + args := c.args(writeRecipe(t, t.TempDir(), validRecipeBody)) + + // The live half. Without it the finished half below would pass for + // a command that ends before it plans anything. + var out, errOut bytes.Buffer + if code := cli.Run(context.Background(), args, &out, &errOut); code != cli.ExitOK { + t.Fatalf("with a live context this ended with %d, so the stopped half below would prove nothing about planning\nstderr: %s", + code, errOut.String()) + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + out.Reset() + errOut.Reset() + + code := cli.Run(ctx, args, &out, &errOut) + if code != cli.ExitInterrupted { + t.Errorf("with a finished context this ended with %d, expected %d - planning ignored the context it was handed\nstderr: %s", + code, cli.ExitInterrupted, errOut.String()) + } + // A run that was stopped is a failed run, and a failed run puts + // nothing on standard output. + if out.Len() > 0 { + t.Errorf("a stopped run wrote %d bytes to standard output: %q", out.Len(), out.String()) + } + }) + } +} + +// Stopping has to happen DURING planning, not be noticed after it. +// +// This is the half that tells the two apart, and it needs no clock to do it. +// The recipe has a first target that plans and a second one that cannot: a zip +// far below the smallest archive its contents already need. So a run that +// walks the whole plan reaches the second target and refuses it by format, +// while a run that honours the context stops inside the first target and ends +// with the interrupt code. Two different codes for the same input, decided by +// nothing but whether planning looked at the context. +// +// Without this, generate passed while planning was still uninterruptible, +// because preflight checks the context on the way to writing - the run did end +// with the right code, having first done every second of the work somebody +// asked it to stop. +func TestPlanningStopsBeforeItWalksToTheFarTarget(t *testing.T) { + cases := []struct { + name string + args func(recipePath string) []string + }{ + {"generate", func(p string) []string { return []string{"generate", p, "--dry-run"} }}, + {"validate", func(p string) []string { return []string{"validate", p} }}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + args := c.args(writeRecipe(t, t.TempDir(), farRefusalRecipeBody)) + + // The live half asserts the state this guard depends on: planning + // really does walk as far as the second target and really does + // refuse it. If that ever stops being true - the first target + // starts failing, say - the stopped half below would agree for a + // reason that has nothing to do with the context. + var out, errOut bytes.Buffer + if code := cli.Run(context.Background(), args, &out, &errOut); code != cli.ExitFormat { + t.Fatalf("with a live context this ended with %d, expected %d from the far target - the two halves would no longer differ\nstderr: %s", + code, cli.ExitFormat, errOut.String()) + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + out.Reset() + errOut.Reset() + + code := cli.Run(ctx, args, &out, &errOut) + if code != cli.ExitInterrupted { + t.Errorf("with a finished context this ended with %d, expected %d - planning walked all the way to the far target before anything looked at the context\nstderr: %s", + code, cli.ExitInterrupted, errOut.String()) + } + }) + } +} + +// A validate that was stopped has no verdict about the recipe, and must not +// print one. +// +// This is a shape the change above created rather than one that was already +// there: before it, validate could not be stopped at all, so the machine +// readable report had no way to reach its "valid: false" branch by being +// interrupted. Left alone it would answer a question nobody asked - a consumer +// reading that one field would learn the recipe is bad, when all that happened +// is that somebody pressed Ctrl+C. Untouchable rule 5 in the other surface: do +// not claim a certainty there is none of. +func TestAStoppedValidateDoesNotCallTheRecipeInvalid(t *testing.T) { + recipePath := writeRecipe(t, t.TempDir(), validRecipeBody) + args := []string{"validate", recipePath, "--json"} + + // The live half earns its place twice here: it proves the recipe really is + // valid, so a "valid" field appearing below could only have come from the + // stopping. + var out, errOut bytes.Buffer + if code := cli.Run(context.Background(), args, &out, &errOut); code != cli.ExitOK { + t.Fatalf("with a live context validate --json ended with %d, expected %d\nstderr: %s", + code, cli.ExitOK, errOut.String()) + } + if !strings.Contains(out.String(), "\"valid\": true") { + t.Fatalf("the live run did not report the recipe as valid, so this guard is not reading what it thinks it is\nstdout: %s", out.String()) + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + out.Reset() + errOut.Reset() + + code := cli.Run(ctx, args, &out, &errOut) + if code != cli.ExitInterrupted { + t.Errorf("a stopped validate --json ended with %d, expected %d\nstderr: %s", + code, cli.ExitInterrupted, errOut.String()) + } + if strings.Contains(errOut.String(), "\"valid\"") || strings.Contains(out.String(), "\"valid\"") { + t.Errorf("a stopped validate reported a verdict about the recipe, and it has none to report\nstdout: %s\nstderr: %s", + out.String(), errOut.String()) + } +} + +const validRecipeBody = "version: 1\nseed: 7\noutput:\n dir: out\ntargets:\n - id: a\n format: txt\n size: 1kb\n count: 4\n" + +// A first target that plans and a second one that cannot. The size is not a +// copied number: it is far below anything a zip could be, so it stays a +// refusal whatever the smallest archive turns out to be. +const farRefusalRecipeBody = "version: 1\nseed: 7\noutput:\n dir: out\ntargets:\n - id: a\n format: txt\n size: 1kb\n count: 4\n - id: b\n format: zip\n size: 10\n" diff --git a/internal/guard/recipebombs_test.go b/internal/guard/recipebombs_test.go new file mode 100644 index 0000000..16981b9 --- /dev/null +++ b/internal/guard/recipebombs_test.go @@ -0,0 +1,89 @@ +package guard + +import ( + "errors" + "strconv" + "strings" + "testing" + + "github.com/donislawdev/TestingFilesGenerator/internal/recipe" +) + +// A recipe somebody else wrote cannot exhaust this machine. +// +// The comment on recipe.MaxBytes used to say the size limit took care of that: +// "A megabyte caps the worst case at seconds rather than minutes". Measured on +// 2026-09-02 it does not. A document of 40 kB nesting flow collections twenty +// thousand deep took 918 MB of heap, and 40 kB is a twenty fifth of what the +// size limit allows - so the limit that was supposed to bound the work buys as +// much memory as the machine will give. +// +// The legal halves are not decoration, and one of them is the reason the +// defence is shaped the way it is. Counting brackets in the raw bytes - the +// first thing anybody reaches for - refuses a value that merely CONTAINS +// brackets, and that is a legal document. The lexer has already told those two +// apart, so it is asked instead of the bytes. +// +// What is deliberately NOT guarded here: a second shape that looked like it +// never finished, and did not exist. Chaining two thousand bracket pairs reads +// in 0.19 s. What hung was the harness measuring it, which wrote a 4 kB refusal +// into a pipe nothing was reading - and the pipe holds 4096 bytes, which is +// exactly where the apparent cliff sat. A budget on the reading was written for +// it and taken back out, because nothing could have made it fire. +func TestAHostileRecipeCannotHangTheReader(t *testing.T) { + const tail = "\ntargets:\n - id: a\n format: txt\n size: 10\n" + + t.Run("nesting is refused by depth", func(t *testing.T) { + src := []byte("version: 1\npolicy:\n x: " + + strings.Repeat("[", 20000) + strings.Repeat("]", 20000) + tail) + _, err := recipe.Parse(src, "bomb.yaml") + var deep *recipe.TooDeepError + if !errors.As(err, &deep) { + t.Fatalf("a recipe nesting 20000 deep was answered with %v, expected a refusal about its depth", err) + } + }) + + t.Run("a legal recipe at nearly the size limit is read", func(t *testing.T) { + // Twenty thousand targets comes to about 949 kB, just under MaxBytes, + // and is the slowest legal read there can be - 1.02 s when it was + // measured. A defence that ever came near this would be refusing + // honest work. + var b strings.Builder + b.WriteString("version: 1\nseed: 7\noutput:\n dir: out\ntargets:\n") + for i := 0; i < 20000; i++ { + b.WriteString(" - id: t") + b.WriteString(strconv.Itoa(i)) + b.WriteString("\n format: txt\n size: 100\n") + } + if _, err := recipe.Parse([]byte(b.String()), "big.yaml"); err != nil { + t.Fatalf("a legal recipe of %d B was refused: %v", b.Len(), err) + } + }) + + t.Run("a value in flow style is not a bomb", func(t *testing.T) { + // A thousand targets written with braces reaches depth one, the same + // as spread: [1B, 1kb]. Anything counting markers rather than nesting + // would see two thousand of them and refuse this. + var b strings.Builder + b.WriteString("version: 1\nseed: 7\noutput:\n dir: out\ntargets:\n") + for i := 0; i < 1000; i++ { + b.WriteString(" - {id: t") + b.WriteString(strconv.Itoa(i)) + b.WriteString(", format: txt, size: 100}\n") + } + if _, err := recipe.Parse([]byte(b.String()), "flow.yaml"); err != nil { + t.Fatalf("a legal recipe written in flow style was refused: %v", err) + } + }) + + t.Run("brackets inside a value are not collections", func(t *testing.T) { + src := []byte("version: 1\nseed: 7\noutput:\n dir: out\ntargets:\n" + + " - id: a\n format: txt\n size: 100\n name: \"" + + strings.Repeat("[", 20000) + strings.Repeat("]", 20000) + ".txt\"\n") + _, err := recipe.Parse(src, "brackets.yaml") + var deep *recipe.TooDeepError + if errors.As(err, &deep) { + t.Fatalf("a value holding brackets was refused as if they nested: %v", err) + } + }) +} diff --git a/internal/guard/zipcompressedsizes_test.go b/internal/guard/zipcompressedsizes_test.go new file mode 100644 index 0000000..35329eb --- /dev/null +++ b/internal/guard/zipcompressedsizes_test.go @@ -0,0 +1,132 @@ +package guard + +import ( + "bytes" + "context" + "errors" + "testing" + + "github.com/donislawdev/TestingFilesGenerator/internal/format" + "github.com/donislawdev/TestingFilesGenerator/internal/format/archive" +) + +// A compressed archive is either written at the size it was ordered, or it is +// refused in words about the archive. There is no third answer. +// +// The third answer is what this was written for, and it existed twice. +// Measured 2026-09-02 on the shipped binary, both with exit code 8 and both +// passing --dry-run with exit code 0: +// +// zip at exactly its own floor, any level but none: no file at all +// zip holding two 1 MB docx entries, a band 50 B wide: no file at all +// +// The manifest said "generator for zip produced 2607 B where the plan said +// 8382 B", which reads as the tool being broken rather than as the size being +// impossible - and a person cannot act on it. +// +// TestACompressedArchiveStillHitsTheSizeToTheByte was written for this exact +// failure and says so: "a size where the space compression frees is larger than +// the padding can give back". It is green, honestly, and it cannot see either +// of these. It asks three round sizes - 64 KiB, 256 KiB and 1 MiB - with the +// contents left at their default. Round sizes never land on the floor, where +// the first fault lives, and the default contents are text, which deflate +// shrinks, so the second fault cannot happen at all: it needs contents deflate +// GROWS. Naming the right failure is not the same as sampling where it lives. +// +// So this one asks the registry where the floor is for each shape rather than +// carrying a number, and sweeps across it a byte at a time. Both halves have to +// appear in every sweep - some size accepted, some size refused - or the sweep +// straddles nothing and proves nothing. +func TestACompressedZipIsEitherWrittenAtItsSizeOrRefusedInWords(t *testing.T) { + d, err := format.Get("zip") + if err != nil { + t.Fatalf("zip is not registered: %v", err) + } + + shapes := []struct { + name string + props map[string]string + below int64 // how far under the floor to start + above int64 // how far over it to stop + }{ + // The floor itself, at every level that squeezes. The fault is one + // byte wide here, so the sweep is narrow and starts under the floor to + // pick up the refusals that prove it straddles something. + {"floor, fast", map[string]string{archive.Compression: archive.CompressFast}, 3, 4}, + {"floor, default", map[string]string{archive.Compression: archive.CompressDefault}, 3, 4}, + {"floor, best", map[string]string{archive.Compression: archive.CompressBest}, 3, 4}, + // Contents deflate cannot shrink. A docx is already a compressed + // container, so a big one comes out of deflate LARGER than it went in - + // which is the case the arithmetic in this format was written without. + // The band measured that day was 50 B, so the sweep reaches well past + // it and into sizes that have to work. + {"contents deflate grows", map[string]string{ + archive.Compression: archive.CompressBest, + "entry_format": "docx", + "entry_size": "1mb", + "entries": "2", + }, 5, 250}, + } + + for _, s := range shapes { + t.Run(s.name, func(t *testing.T) { + floor := d.SmallestAccepted(format.Request{Label: true, Properties: s.props}) + if floor <= 0 { + t.Fatalf("the registry reports a floor of %d B for this shape, so there is nothing to sweep", floor) + } + + var written, refused int + for size := floor - s.below; size <= floor+s.above; size++ { + plan, planErr := d.Generator.Plan(format.Request{ + Bytes: size, Seed: 7741, Label: true, Properties: s.props, + }) + if planErr != nil { + if !isBelowMinimum(planErr) { + t.Fatalf("%d B was refused by something other than a minimum: %v", size, planErr) + } + refused++ + continue + } + + var buf bytes.Buffer + writeErr := d.Generator.Write(context.Background(), &buf, plan) + if writeErr != nil { + if !isBelowMinimum(writeErr) { + t.Errorf("%d B planned and then failed to write with a message that is not about the size: %v\n"+ + "A size the plan accepted and the writer cannot produce has to say what a person should ask for instead.", + size, writeErr) + continue + } + refused++ + continue + } + if int64(buf.Len()) != size { + t.Errorf("%d B was planned and produced %d B, with no error at all - "+ + "the archive is the wrong length and nothing says so", + size, buf.Len()) + continue + } + written++ + } + + // Both halves, or the sweep sat entirely on one side of the floor + // and the run above asked nothing. + if written == 0 { + t.Errorf("no size in this sweep produced an archive, so it proves nothing about the ones that should work") + } + if refused == 0 { + t.Errorf("no size in this sweep was refused, so the sweep never reached the floor it is aimed at") + } + t.Logf("floor %d B: %d written, %d refused", floor, written, refused) + }) + } +} + +// isBelowMinimum is the one refusal a size sweep is allowed to meet. +// +// Asked by type rather than by reading the sentence, because the sentence is +// written for a person and this is the part a script would act on. +func isBelowMinimum(err error) bool { + var below *format.BelowMinimumError + return errors.As(err, &below) +} diff --git a/internal/gui/window/run.go b/internal/gui/window/run.go index 20adfe5..aeade27 100644 --- a/internal/gui/window/run.go +++ b/internal/gui/window/run.go @@ -117,8 +117,22 @@ type runner struct { // HoldBeforeFinishing for what it is for and why nothing else can do it. hold func() - // stop ends the run in progress and waits for it, and is only ever touched - // on the interface thread. + // stop ends the run in progress and waits for the worker to finish, and is + // only ever touched on the interface thread. + // + // "Waits for the worker" is as far as it reaches, and the limit is worth + // writing down because G7 gets read out of this sentence. The worker closes + // the channel this waits on immediately after handing its last piece of + // work to fyne.Do, and fyne.Do QUEUES that work rather than running it - + // DoAndWait there would be the interface thread waiting on a worker that is + // waiting on the interface thread. So when stop returns, the worker is + // finished and the widget writes it asked for may still be in the toolkit's + // queue. + // + // What that buys is what G7 asks for: nothing of ours is still running and + // nothing more is going to the disk. What it does not buy is the screen + // having caught up - and Settled does not buy that either, because it waits + // on the same channel. // // It is deliberately left in place once a run has ended rather than // cleared, and that is a threading decision rather than an oversight. @@ -959,6 +973,13 @@ func (r *runner) holdBeforeFinishing() { // guards used "close the window" as their way of waiting for an answer. Now // closing really does cancel, which is the point of the change, and a guard // that closed the window to read the preview would be cancelling the preview. +// +// It waits on the same channel stop does and carries the same limit: the worker +// is finished and its last fyne.Do may still be queued. Under the test driver +// that distinction disappears, because there fyne.Do runs on the calling +// goroutine, so the callback has already run by the time the channel closes - +// which is why a guard may read the screen the moment this returns, and why the +// limit is a production one rather than a test one. func (r *runner) Settled() { if r.settled != nil { <-r.settled diff --git a/internal/recipe/limits.go b/internal/recipe/limits.go new file mode 100644 index 0000000..65347f2 --- /dev/null +++ b/internal/recipe/limits.go @@ -0,0 +1,108 @@ +// Part of package recipe. See recipe.go. +package recipe + +import ( + "fmt" + + "github.com/goccy/go-yaml/lexer" + "github.com/goccy/go-yaml/token" +) + +// What a recipe is allowed to cost to read, beyond its size. +// +// MaxBytes bounds the input and its comment used to claim that bounded the +// work: "A megabyte caps the worst case at seconds rather than minutes". That +// was measured on 2026-08-02 and it is not true. Measured again on 2026-09-02, +// on this build: a document of 40 kB nesting flow collections twenty thousand +// deep took 918 MB of heap, and it would have taken more had the machine had +// more. Forty kilobytes is a twenty fifth of what the size limit allows. +// +// Reading time is not the problem and one measurement here was WRONG before it +// was checked. A chain of two thousand bracket pairs looked like it never +// finished, and a budget on the reading was written to answer it. It finishes +// in 0.19 s. What hung was the harness measuring it: the tool wrote a 4 kB +// refusal into a pipe nothing was reading, and the pipe holds 4096 bytes. The +// apparent cliff sat exactly there - 1900 pairs produce a 4010 B message and +// pass, 1950 produce 4110 B and blocked. The budget was taken back out, since +// nothing could have made it fire. + +const ( + // MaxFlowDepth is how far flow collections - the ones written with + // brackets and braces - may nest. + // + // This is the deterministic half, and it is deterministic because the + // lexer has already decided what is a bracket and what is a character + // inside a quoted value. Measured across shapes on 2026-09-02: + // + // block style, a thousand targets 0 + // forty thousand brackets inside a string 0 + // spread: [1B, 1kb, 1mb] 1 + // a thousand targets written in flow style 1 + // the nesting bomb 20000 + // + // So one is what real recipes reach and thirty two is far above anything + // a person writes. Counting rather than guessing also rules out the two + // obvious mistakes: a bracket inside a quoted value is not a collection, + // and neither is one inside a comment. + MaxFlowDepth = 32 +) + +// TooDeepError is returned for a recipe whose flow collections nest past +// MaxFlowDepth. +type TooDeepError struct { + Name string + Depth int +} + +func (e *TooDeepError) Error() string { + return fmt.Sprintf( + "%s nests brackets and braces %d deep and the limit is %d. Reading a deeply nested document costs memory that grows far faster than the document does, so a small file can exhaust this machine before anything is written. Write the targets out as an ordinary list instead", + e.Name, e.Depth, MaxFlowDepth) +} + +// flowDepth is the deepest the flow collections in src nest. +// +// The lexer is asked rather than the bytes, and that is the whole point of +// doing it this way: it has already decided which brackets open a collection +// and which are characters inside a quoted value or a comment. Measured on +// 2026-09-02, forty thousand brackets inside one quoted value come back as +// depth nought, which a scan over the raw bytes could only manage by +// reimplementing the quoting rules. +// +// Cheap enough to run on every recipe: 0.002 s for a forty kilobyte document, +// 0.008 s for the bomb, and the cost grows with the input rather than with the +// shape - which is exactly what the parser underneath does not do. +func flowDepth(src []byte) int { + current, deepest := 0, 0 + for _, t := range lexer.Tokenize(string(src)) { + current += depthChange(t.Type) + if current > deepest { + deepest = current + } + } + return deepest +} + +// depthChange is what one token does to the nesting: a collection opening adds +// a level, one closing takes it away, and anything else leaves it where it was. +// +// A function of its own rather than a switch inside the loop above, because +// together they nested three deep - the loop, the switch, the comparison - and +// the shape guard counts how many functions sit that deep as well as how deep +// the deepest one is. Splitting is what that guard asks for and it costs +// nothing here. +// The default is not decoration. token.Type has thirty four members and a +// switch on it without one is reported as incomplete, which is correct of the +// linter and wrong about this function: everything that is not a bracket or a +// brace leaves the nesting where it was, and listing thirty of them would say +// less than one line saying so. +func depthChange(t token.Type) int { + switch t { + case token.SequenceStartType, token.MappingStartType: + return 1 + case token.SequenceEndType, token.MappingEndType: + return -1 + default: + return 0 + } +} diff --git a/internal/recipe/recipe.go b/internal/recipe/recipe.go index fe30e83..c2ade4a 100644 --- a/internal/recipe/recipe.go +++ b/internal/recipe/recipe.go @@ -121,14 +121,16 @@ type Output struct { // and the cost sits inside the YAML parser where nothing we do afterwards can // reduce it. The only lever before parsing is the size of the input. // -// Measured on 2026-08-02: a deliberately nested document of 80 kB cost about -// 1.3 s of parsing over the baseline, and the growth is faster than linear. A -// megabyte caps the worst case at seconds rather than minutes, and is far above -// any recipe a person writes - ten thousand targets spelled out in full come to -// roughly half of it. +// A megabyte is far above any recipe a person writes - twenty thousand targets +// spelled out in full come to 949 kB, and read in 1.02 s. // -// What this does not do: it does not defend against a recipe that fits and is -// still expensive. Seconds on a hostile file are acceptable, minutes were not. +// What this does NOT do, and the sentence here used to claim otherwise: it does +// not bound the work. It said "a megabyte caps the worst case at seconds rather +// than minutes", measured on 2026-08-02. Measured again on 2026-09-02, a +// document of 40 kB - a twenty fifth of this limit - took 918 MB of heap by +// nesting flow collections twenty thousand deep, and would have taken whatever +// the machine had. The size of the input says nothing about the shape of it. +// What bounds the shape is in limits.go. const MaxBytes = 1 << 20 // TooLargeError is returned for a recipe past MaxBytes. @@ -181,6 +183,14 @@ func Parse(src []byte, name string) (*Recipe, error) { // readers below, so both see the same bytes. src = withoutBOM(src) + // Asked before the parser sees any of it, because the parser is where the + // cost is and no amount of it can be given back afterwards. See limits.go + // for the two shapes this and the budget below answer, and why one number + // cannot answer both. + if depth := flowDepth(src); depth > MaxFlowDepth { + return nil, &TooDeepError{Name: name, Depth: depth} + } + // One file is one recipe. Everything after a document separator would be // dropped by the decoder, which means somebody gets half the fixtures they // asked for and a run that says it went fine.