From f54463a8c48a8079c187422b495b1bfedfd9befc Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 2 Sep 2026 22:55:01 +0200 Subject: [PATCH 1/8] cli: planning can be stopped, and stopping it says nothing about the recipe engine.PlanContext was written so planning could be stopped and its own comment says why - ten thousand pngs is a minute and a half before a byte is written. 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 builds for the signal reached the writing and stopped at the planning. Measured on Linux with a real SIGINT, 4000 pngs, --dry-run, signal sent at t=2.0 s: main ended at 51.99 s, this ends at 2.010 s. Both exit 130. A stopped validate --json no longer reports "valid": false. It never finished reading the recipe, so it has no verdict to give, and a consumer reading that one field would act on a claim about the file rather than on what happened. validate was split into planningRefusal to stay under the crowding threshold, which the guard asks for rather than raising the cap. Three guards, all proven by mutation. The second exists because the first passed for generate while planning was still uninterruptible: preflight noticed the context on the way to writing, so the exit code was right and the work had all been done anyway. It tells them apart with two exit codes on one input rather than with a clock. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 16 +++ internal/cli/cli.go | 4 +- internal/cli/generate.go | 8 +- internal/cli/preset.go | 13 +- internal/cli/recipecmd.go | 31 +++-- internal/guard/planningstops_test.go | 179 +++++++++++++++++++++++++++ 6 files changed, 234 insertions(+), 17 deletions(-) create mode 100644 internal/guard/planningstops_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f8f92ad..76fcdff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -349,6 +349,22 @@ 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 + +- **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 `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/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/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" From ecc74751e6ba7cdb4225f17c8ec6b7c5addf4caa Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 2 Sep 2026 23:12:33 +0200 Subject: [PATCH 2/8] zip: a compressed archive is either its ordered size or a refusal in words Two shapes produced a file of the wrong length with no error, and both passed --dry-run first. At exactly the bare size, pad() took its "nothing to pad" early return before reaching padCompressed, so withFiller stayed false - and the filler entry is the only thing that can give back what the compressor freed. Measured: 8382 B produced about 2.6 kB at every level but none. The floor for a compressed zip moves 8382 -> 8522, which is where sizes started working anyway. freed() can be NEGATIVE. Deflate grows data it cannot shrink, so an archive holding already compressed files comes out larger than it went in, and the filler cannot shrink below zero to compensate. writeFiller simply wrote nothing. Measured as a band 50 B wide holding two 1 MB docx entries. It is now a BelowMinimumError naming the measured floor. Refused at write time rather than at planning, which is where targz refuses the same thing for the same reason: how far contents squeeze is not knowable without squeezing them. Refusing at plan time would need the worst case, about 165 B at 2 MB against a measured band of 50 B, so it would refuse sizes that work. The report also asked for an n < 0 check inside writeFiller. Not done, and the reason is recorded: after this, nothing could redden it. A guard for this already existed and was honestly green. TestACompressedArchiveStillHitsTheSizeToTheByte names this exact failure and samples three round sizes with the contents left at their default. A round size never lands on the floor, and the default contents are text, which deflate shrinks - so neither shape can occur where it looks. Two mutations, both caught. A third, pre-existing entry was retargeted after gofmt moved the spaces in the line it named. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 19 ++++ internal/format/zip/compress.go | 2 +- internal/format/zip/filler.go | 33 +++++- internal/format/zip/zip.go | 14 ++- internal/guard/zipcompressedsizes_test.go | 132 ++++++++++++++++++++++ 5 files changed, 197 insertions(+), 3 deletions(-) create mode 100644 internal/guard/zipcompressedsizes_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 76fcdff..8467ac1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -361,6 +361,25 @@ because it turns other people's test suites red. Writing was never affected. A run interrupted while producing files already stopped promptly, saved its manifest and left no partial files behind. +- **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. 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/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) +} From 3a32c9d3ef9932ed6c5f93b35da21498141c2c5b Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 2 Sep 2026 23:49:43 +0200 Subject: [PATCH 3/8] recipe: a document of 40 kB can no longer buy every byte of memory the machine has Nesting flow collections twenty thousand deep took 918 MB of heap on a 40 kB file, which is a twenty fifth of what MaxBytes allows. The comment on that limit claimed it bounded the work - "a megabyte caps the worst case at seconds rather than minutes" - and the size of a document says nothing about its shape. After this: 22.7 MB and exit 3, in our own words. The ceiling is on DEPTH, and that is measured rather than preferred. Three other measures were tried and each let a legal recipe look like the bomb: token count (a legal thousand-target recipe has 10005, the bomb 3919 - the legal one has MORE), flow marker count (2000 against 3900), and the number of collections. Depth separates cleanly: legal recipes reach one, the bomb reaches twenty thousand. The lexer is asked rather than the bytes. Forty thousand brackets inside one quoted value come back as depth nought, which a byte scan could only manage by reimplementing YAML's quoting rules - a second parser beside the one being defended against. A budget on reading time was written for a second shape and taken back out, because that shape does not exist. A chain of two thousand bracket pairs looked like it never finished and reads in 0.19 s. What hung was the harness measuring it: it 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, between a 4010 B message and a 4110 B one. Two mutations, both caught. The legal halves of the guard are load bearing: without them it would pass for a limit that refuses everything. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 10 ++++ internal/cli/errors.go | 7 +++ internal/guard/recipebombs_test.go | 89 ++++++++++++++++++++++++++++++ internal/recipe/limits.go | 89 ++++++++++++++++++++++++++++++ internal/recipe/recipe.go | 24 +++++--- 5 files changed, 212 insertions(+), 7 deletions(-) create mode 100644 internal/guard/recipebombs_test.go create mode 100644 internal/recipe/limits.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 8467ac1..9fb4ad4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -361,6 +361,16 @@ because it turns other people's test suites red. 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. 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/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/recipe/limits.go b/internal/recipe/limits.go new file mode 100644 index 0000000..545d262 --- /dev/null +++ b/internal/recipe/limits.go @@ -0,0 +1,89 @@ +// 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)) { + switch t.Type { + case token.SequenceStartType, token.MappingStartType: + current++ + if current > deepest { + deepest = current + } + case token.SequenceEndType, token.MappingEndType: + current-- + } + } + return deepest +} 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. From c8f0d44d04275cd8b4ce653c2fc6406da97bbe4a Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Thu, 3 Sep 2026 10:53:00 +0200 Subject: [PATCH 4/8] recipe: counting how deep a document nests is two functions, not one nested three deep flowDepth held a loop, a switch and a comparison inside one another, which is three levels, and the shape guard counts how many functions sit that deep as well as how deep the deepest one is. The count had reached fifty three against a cap of fifty two, so the whole suite was red on a branch whose targeted runs were all green. The switch moves into depthChange, which answers what one token does to the nesting and nothing else. Behaviour is unchanged - the deepest value is only ever written where it was written before, on a token that opens a collection. Co-Authored-By: Claude Opus 5 --- internal/recipe/limits.go | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/internal/recipe/limits.go b/internal/recipe/limits.go index 545d262..5cc2c21 100644 --- a/internal/recipe/limits.go +++ b/internal/recipe/limits.go @@ -75,15 +75,28 @@ func (e *TooDeepError) Error() string { func flowDepth(src []byte) int { current, deepest := 0, 0 for _, t := range lexer.Tokenize(string(src)) { - switch t.Type { - case token.SequenceStartType, token.MappingStartType: - current++ - if current > deepest { - deepest = current - } - case token.SequenceEndType, token.MappingEndType: - current-- + 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. +func depthChange(t token.Type) int { + switch t { + case token.SequenceStartType, token.MappingStartType: + return 1 + case token.SequenceEndType, token.MappingEndType: + return -1 + } + return 0 +} From ee136a73e78253c5fb0ccf5e44057cf1ba29fec4 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Thu, 3 Sep 2026 10:53:16 +0200 Subject: [PATCH 5/8] engine: a plan of one file is weighed the way a plan of ten thousand is, and a generator that crashes costs one file Two findings from the stability review, both in the engine. The memory ceiling on a plan only started counting once a run had announced sixty four files, so a shorter run had no ceiling at all - and the count was the whole run's, added up across targets, so sixty four targets of one file each took their reference point after sixty three files were already planned, and those sat inside it for the rest of the run. Measured rather than reasoned about: 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 them come to 2.17 GB against a ceiling of two, and nothing weighed them. The reference point is now taken when the budget is built and the first reading is at the first file, so expect, expected, started and planCheckFirst all go. What waiting bought was measured too, because the number in the comment that justified it turned out to be about something else: the guard package runs 277.9 s and 283.7 s without the change against 292.3 s and 306.1 s with it, so about six percent. The second is that a panic inside a generator ended the process and 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. Such a crash is now an ordinary failure of one file. Planning is wrapped as well as writing, because the picture formats encode while planning and that is the likelier of the two, and a crash there ends the run with the code that means this tool has a defect. defer fh.Close() was asked for by the review and left out on purpose: once the panic is an error the existing close and remove run, and nothing could turn that defer red. Five mutations, all caught. The file ceiling drops from 503 to 502 because engine.go lost a line. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 32 +++++ internal/engine/crash.go | 99 +++++++++++++++ internal/engine/engine.go | 13 +- internal/engine/planmemory.go | 88 +++++++------ internal/engine/plantarget.go | 2 +- internal/guard/codeshape_test.go | 6 +- internal/guard/generatorcrash_test.go | 171 ++++++++++++++++++++++++++ internal/guard/planmemory_test.go | 79 ++++++++++++ 8 files changed, 443 insertions(+), 47 deletions(-) create mode 100644 internal/engine/crash.go create mode 100644 internal/guard/generatorcrash_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 9fb4ad4..9205ce8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -351,6 +351,38 @@ because it turns other people's test suites red. ### 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 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/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 From 48ee1f03ce9ebfc610e7114d2bf25014b57ca2c8 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Thu, 3 Sep 2026 11:04:31 +0200 Subject: [PATCH 6/8] core: the two numbers that could only go wrong at the edge of their range now have a guard that can reach them Three small findings from the stability review, and two of them turned on being able to turn the fix red. Free space came back from both platform files as a number the system reports unsigned, converted to int64. At eight exbibytes that wraps into a negative, and the free space check reads a negative as a disk too small for anything - so the tool would refuse to write to the largest disk it will ever meet. Nobody has such a disk, which is the argument for where the fix lives rather than a reason to skip it: a condition inside the syscall is one nothing could ever reach, so the arithmetic is now core.AvailableFrom in a file with no build tag, and the guard asks it directly on every platform. The review only named the unix side. Windows had the same conversion, one multiplication shorter. AppendFiller ends its loop only by adding bytes, so a vocabulary of empty strings with a separator that returns nothing spins without end and without growing - the one failure a size guard cannot see, because no file is produced to measure. FillRecords three functions up names exactly that and refuses. The obvious fix was wrong: checking progress every iteration refuses ["", "ab"], which pads perfectly well. The check is about the vocabulary having anything to say at all, and one word with bytes in it ends the loop whatever the separator does. The third is two sentences. The comment on stop said it waits for the run, and what it waits on is the worker - the last widget writes may still be queued, because fyne.Do queues rather than runs. Settled carries the same limit, since it waits on the same channel, and the first draft of this correction said otherwise. Four mutations, all caught. Co-Authored-By: Claude Opus 5 --- internal/core/diskspace.go | 38 ++++++++++ internal/core/diskspace_unix.go | 6 +- internal/core/diskspace_windows.go | 5 +- internal/core/records.go | 30 ++++++++ internal/guard/arithmeticedges_test.go | 98 ++++++++++++++++++++++++++ internal/gui/window/run.go | 25 ++++++- 6 files changed, 197 insertions(+), 5 deletions(-) create mode 100644 internal/core/diskspace.go create mode 100644 internal/guard/arithmeticedges_test.go 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/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/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 From 68eea8b7fb7d49e6fcdf40a10245ffd0b1e9f18d Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Thu, 3 Sep 2026 11:31:31 +0200 Subject: [PATCH 7/8] recipe: the depth switch says what it does about every other token golangci-lint reads a switch on token.Type without a default as incomplete, and it is right about the shape while being wrong about this function: everything that is not a bracket or a brace leaves the nesting where it was, and listing the other thirty members would say less than one line saying so. Measured rather than assumed, because the natural conclusion was that flattening the function had introduced this. It had not. The version before the flattening, restored from 3a32c9d and run through the same pinned linter, reports the same thing at line 78 - the branch had simply never been through preflight, which is also how a shape cap sat one over its measurement for three commits. Co-Authored-By: Claude Opus 5 --- internal/recipe/limits.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/recipe/limits.go b/internal/recipe/limits.go index 5cc2c21..65347f2 100644 --- a/internal/recipe/limits.go +++ b/internal/recipe/limits.go @@ -91,12 +91,18 @@ func flowDepth(src []byte) int { // 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 } - return 0 } From 0d3868ffb70dfe42716c5fe5492ccd9d32c65259 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Thu, 3 Sep 2026 12:37:58 +0200 Subject: [PATCH 8/8] targz: the sentence beside the stored bound stops claiming what it cannot The comment on reachable 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 settleCompressed, which measures the real stream and iterates. reachable is a cheap early refusal, and now says so. This was on the list of the piece that fixed zip and was missed there - internal/format/targz was never touched by this branch until now. Co-Authored-By: Claude Opus 5 --- internal/format/targz/compress.go | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) 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{}