Stability review: nine findings closed, one rejected, one observation overturned - #45
Merged
Merged
Conversation
…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 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
…e 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 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
…ange 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…nnot 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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An outside session reviewed this tool for stability and reported ten findings
and three observations. This branch is the answer to all of them: a verdict with
a measurement for each, and the work for the ones that survived it.
What changed for somebody running
tfggenerate,validateandpreset showwhile they are stillplanning. Ten thousand pictures is about a minute and a half of planning
before the first byte, and all of it used to ignore the key.
zipis either produced at the size you asked for or refused.Two sizes did neither, and both passed
--dry-runfirst.kilobytes nesting brackets twenty thousand deep took most of a gigabyte.
started counting at sixty four files, so a shorter run had no ceiling at all.
longer leaves a temporary file that
cleanupwill never remove.Runs this tool was designed around are unaffected, and no generated file changes
its bytes.
What the measurements said
Every fix here started from a number rather than from the report's reasoning,
and three of the report's conclusions were right for the wrong reason.
summed across targets, so it was the first up to sixty three files of every
run, however long. A zip of ten thousand pdf entries costs 74 740 758 B of plan
a file - five points, linear to within 0.05% - so twenty nine of them are
2.17 GB against a ceiling of two, unweighed.
something else. Four interleaved runs of the guard package: 277.9 s and 283.7 s
before, 292.3 s and 306.1 s after, so about six percent.
conversion.
cases.Caserinstead of buildingone per call changes neither the allocation count nor the byte count.
does not get reopened without a new argument.
Two measurements were deferred for want of machine memory and then done once it
came back, rather than left as estimates:
the twenty ninth file, which is exactly where five points said it would be.
signal, 2.01 s with one, exit 130.
And one comment was corrected that had been on this work's own list and missed:
targzsaid compression only ever makes contents smaller. It does not, and thatis the assumption
zipfell over on. What keepstargzto the byte is that itmeasures rather than that the sentence was true.
What was deliberately not done
defer fh.Close()beside the newrecover. Once the panic is an error, theclose and the remove that already exist run - nothing could turn that defer
red, and this project has taken seven such pieces back out.
deflate growth is about 165 B at 2 MB and the measured band is 50 B, so a
refusal with that margin would reject sizes that work today.
arithmetic could be asked directly, which is why one of them moved into a pure
function first.
Guards
Every changed behaviour has a guard, and every guard is proven by a mutation
that turns it red. The mutation list went from 735 entries to 750, and four
existing entries were retargeted or replaced after the refactors moved code out
from under them - the staleness check names those in no time at all, which is
why it runs after every refactor rather than at the end.
🤖 Generated with Claude Code