Repository navigation
fix: a run of many files no longer runs out of memory while it saves its manifest - #212
Conversation
Saving the manifest built the whole document in memory, and an indented copy of it, while every entry was still held. At 100 000 text files the save peaked at 9.3 KB a file against 4.0 KB for writing every file, and a run of 200 000 in a container of 1 GB was killed after its last file with no manifest written. Encode now writes the document with no entries cut where its list is, and the entries between, one at a time. A guard holds the bytes to encoding/json's own rendering and another fails when the document reaches its writer in one piece. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Grown by append, a plan of a million text files took 1.9-2.2 GB from the system for 0.94 GB of plan, because every doubling leaves the last array to the collector. Reserved, it took 1.2 GB with five collections instead of fourteen. The reservation never passes the memory setting's ceiling for the plan and is not made for a run past the file ceiling, which the loop refuses as before. A guard holds the plan's capacity to its length. Mutation entries for this and for the manifest written an entry at a time, and the changelog entry for both. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
| Layer / File(s) | Summary |
|---|---|
Stream manifest encoding internal/manifest/encode.go, internal/manifest/manifest.go, internal/manifest/size.go, internal/guard/manifeststream_test.go, internal/guard/manifestreach_test.go, internal/guard/mutation/entries.py, CHANGELOG.md |
Manifest.Encode writes nonempty manifests incrementally and retains whole-document encoding for empty manifests. Tests compare encoded bytes with indented JSON, check write sizes, and verify the manifest-size calculation. |
Reserve planned-file capacity internal/engine/planmemory.go, internal/engine/engine.go, internal/guard/planreserve_test.go, internal/guard/mutation/entries.py |
planCapacity calculates reservation capacity from target sizes and the memory budget. PlanContext uses that capacity, and a test checks the resulting plan length and slice capacity. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant Caller
participant Encode as Manifest.Encode
participant Shell as shell
participant Whole as encodeWhole
participant JSON as json.Marshal
participant Buffer as bufio.Writer
participant Writer as io.Writer
Caller->>Encode: Encode(writer)
Encode->>Shell: Build empty-files document shell
Shell->>Whole: Encode manifest copy with empty files
Whole-->>Shell: Indented JSON bytes
Shell-->>Encode: Document head and tail
loop Each file entry
Encode->>JSON: Marshal entry
JSON-->>Encode: Entry bytes
Encode->>Buffer: Write head or entry bytes
end
Encode->>Buffer: Write document tail and flush
Buffer->>Writer: Write buffered bytes
Writer-->>Buffer: Write result
Buffer-->>Encode: Flush result
Encode-->>Caller: Return encoding or write error
Possibly related PRs
- donislawdev/TestingFilesGenerator#208: Establishes the memory-budget contract that
planCapacityuses when calculating its reservation. - donislawdev/TestingFilesGenerator#204: Adds the manifest-size calculations whose expected byte layout is checked against the streaming encoder.
Suggested labels: bug, performance
Merge Risk: 
5947b
-
An invalid large run can consume substantial memory before reporting why it cannot proceed, and may exhaust a constrained container. Defer or bound the reservation before merging.
Pre-merge checks: 
-
Check name Status Explanation Resolution Tests For Changed Behavior The PR adds tests for manifest byte preservation, streaming writes, and exact plan capacity under the normal budget. It does not test the new memory-bounded reservation branch in planCapacity(`plan…Add deterministic tests for planCapacityor an equivalent engine-level behavior. Cover a budget whoseceiling / sizeof(PlannedFile)is below the target count and assert that the initial plan capacity is capped. Also cover the zero-budge…Desktop Robustness The new non-empty Manifest.Encodepath performs one marshal/write loop per manifest entry, but it accepts onlyio.Writerand has no progress or cancellation hook. In the GUI,startRunreports pr…Thread a context and manifest-save progress callback through SaveRecord,Reservation.Save, andManifest.Encode. Check cancellation between entries and report entry/byte progress to the GUI. Preserve the temporary-file and publish sequ…Clear User-Facing Text The PR adds a user-visible defect message in internal/manifest/encode.go:a manifest with no entries holds %d lists of files one level in, not one. The CLI exposes this through `cannot write the m…Replace the defect text with: manifest encoding found %d lists for "files" instead of one; this is an internal error. Report it with the command output.-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly describes the user-visible fix: large runs no longer exhaust memory while saving the manifest. It is specific and within the length limit. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. No Secrets Or Debug Leftovers The pull-request diff adds no CLAUDE.md, CLAUDE.local.md, AGENTS.md, .claude/, or .env files. Added lines contain no detected credentials, private keys, URLs, absolute user paths, IP addresses, person… No Hardcoded Ui Styling The pull request does not add or change GUI code. The authoritative diff contains only CHANGELOG.md and internal Go/Python engine, manifest, and test files. No XAML, Slint, Fyne, Tkinter, or WPF UI co… No Obvious Performance Problems No clear performance problem matches the check. Manifest.Encodeemits each file entry through a 64 KiB buffered writer, so it avoids whole-document accumulation; each entry is processed once. `planC…Safe File Parsing No unsafe file parsing was introduced. The PR exports only manifest JSON through json.NewEncoderand per-entryjson.MarshalIndentinManifest.Encode; it does not parse XML/CSV/XLSX/YAML, evaluat…System Changes Are Reversible PASS: The pull request changes manifest JSON encoding, plan-slice capacity, tests, and changelog text. The reviewed diff contains no changes to network filters or rules, proxies, firewalls, system tim… No Resource Leaks No resource leak is introduced. Manifest.Encodeuses a bounded 64 KiBbufio.Writerand temporarybytes.Bufferdata; it does not own or retain the caller's writer.saveReservedcloses the manif…Scope, Duplication And Docs The PR scope matches the title and description: it streams manifest entries and reserves plan capacity. The added guards and mutation cases support those two changes. `Manifest.Encode(io.Writer) error… Full details: No Secrets Or Debug Leftovers
-
Explanation
The pull-request diff adds no CLAUDE.md, CLAUDE.local.md, AGENTS.md, .claude/, or .env files. Added lines contain no detected credentials, private keys, URLs, absolute user paths, IP addresses, personal email addresses, or debug output/calls. The fmt usage is limited to test formatting and Sprintf construction.
Full details: No Hardcoded Ui Styling
-
Explanation
The pull request does not add or change GUI code. The authoritative diff contains only CHANGELOG.md and internal Go/Python engine, manifest, and test files. No XAML, Slint, Fyne, Tkinter, or WPF UI code is changed, so the hardcoded UI styling condition does not apply.
Full details: No Obvious Performance Problems
-
Explanation
No clear performance problem matches the check.
Manifest.Encodeemits each file entry through a 64 KiB buffered writer, so it avoids whole-document accumulation; each entry is processed once.planCapacitymakes one linear pass over target sizes and reserves the plan once, while the planning loop remains linear. No UI-thread I/O, per-item external call, or O(n²) collection processing was introduced.
Full details: Safe File Parsing
-
Explanation
No unsafe file parsing was introduced. The PR exports only manifest JSON through
json.NewEncoderand per-entryjson.MarshalIndentinManifest.Encode; it does not parse XML/CSV/XLSX/YAML, evaluate formulas, resolve entities, deserialize arbitrary types, or add path handling. JSON string escaping remains provided byencoding/json, and the diff preserves existing manifest path validation.
Full details: System Changes Are Reversible
-
Explanation
PASS: The pull request changes manifest JSON encoding, plan-slice capacity, tests, and changelog text. The reviewed diff contains no changes to network filters or rules, proxies, firewalls, system time, process hooking or injection, Windows services, registry, or drivers. Reversibility safeguards are therefore not applicable.
Full details: No Resource Leaks
-
Explanation
No resource leak is introduced.
Manifest.Encodeuses a bounded 64 KiBbufio.Writerand temporarybytes.Bufferdata; it does not own or retain the caller's writer.saveReservedcloses the manifest file throughcore.Finishon encode, sync, and close-error paths.planCapacitycreates one bounded plan slice, and failed planning releases it with the local context. No handlers, timers, goroutines, processes, or unbounded caches were added.
Full details: Scope, Duplication And Docs
-
Explanation
The PR scope matches the title and description: it streams manifest entries and reserves plan capacity. The added guards and mutation cases support those two changes.
Manifest.Encode(io.Writer) errorkeeps the same signature, and the manifest format remains unchanged through byte-for-byte tests againstencoding/json; no CLI, configuration, build, or requirement changes were introduced.CHANGELOG.mddocuments the user-visible memory fix. No duplicate implementation or bypassed architecture layer was found.
-
Full details: Tests For Changed Behavior
-
Explanation
The PR adds tests for manifest byte preservation, streaming writes, and exact plan capacity under the normal budget. It does not test the new memory-bounded reservation branch in
planCapacity(planmemory.go:38-40).TestAPlanIsReservedRatherThanGrownplans only 1,000 files with default settings, so it exercises the uncappedreturn npath. Existing memory tests check refusal, not the reserved capacity. The PR description also explicitly states that the memory-setting cap has no guard. No test assertions were deleted or weakened; the existing manifest-size test was refactored with its assertion preserved.Resolution
Add deterministic tests for
planCapacityor an equivalent engine-level behavior. Cover a budget whoseceiling / sizeof(PlannedFile)is below the target count and assert that the initial plan capacity is capped. Also cover the zero-budget and over-MaxFilesPerRunpaths if those branches remain part of the implementation.
Full details: Desktop Robustness
-
Explanation
The new non-empty
Manifest.Encodepath performs one marshal/write loop per manifest entry, but it accepts onlyio.Writerand has no progress or cancellation hook. In the GUI,startRunreports progress only fromengine.Run; it then callssaveRecord/engine.SaveRecord, which runs this new loop. Closing the window cancels the run context, butEncodenever observes it, so a 200,000-entry save can continue without feedback whileStopwaits. The reservation still writes to a temporary file, syncs it, and publishes it atomically.Resolution
Thread a context and manifest-save progress callback through
SaveRecord,Reservation.Save, andManifest.Encode. Check cancellation between entries and report entry/byte progress to the GUI. Preserve the temporary-file and publish sequence. If cancellation occurs after files exist, either finish and publish the manifest before returning or retain a valid partial-run manifest; do not leave written files without their record.
Full details: Clear User-Facing Text
-
Explanation
The PR adds a user-visible defect message in
internal/manifest/encode.go:a manifest with no entries holds %d lists of files one level in, not one. The CLI exposes this throughcannot write the manifestorcannot render the manifest. It describes the internal state but gives the user no action. The changelog text is otherwise clear.Resolution
Replace the defect text with:
manifest encoding found %d lists for "files" instead of one; this is an internal error. Report it with the command output.
-
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Reserved before the first target, a million files asking for a size their format cannot make were refused at the first file after taking 441-450 MB, against 27-30 MB before. The size of the reservation is still worked out before the targets are walked, and the list is now taken when the first file has passed every check, with Ctrl+C asked just before. That refusal is back to 26-28 MB, and a plan of a million text files still peaks at 1.2 GB rather than 2. A guard holds what planning allocates when the first file is refused. A mutation entry for the context check is narrowed to its own place, since the new check made its pattern appear twice. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…loop The block that takes the list at the first file was nested three deep inside the loop over files, which made it the fiftieth function at that depth against a ceiling of 49. The ceiling is a ratchet, so the block moved into startList instead of the number moving up. Behaviour is the same: the list is taken once, at the first file that passed its checks, with Ctrl+C asked first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
Encode(internal/manifest/encode.go) takes the document with no entries and cuts it where the list of files is. It writes that head, then each entry, then the tail, through a buffered writer. This is the layoutsize.goalready counts. A manifest with no entries is still encoded whole, so a list that was never made staysnull.engine.planCapacity). The reservation never goes past what the memory setting allows the plan. A run past the file ceiling gets no reservation, and the planning loop refuses it as before.Why
Saving the manifest put the whole document together in memory, plus an indented copy, after the last file and while every entry was still held. Measured with a probe that splits a run into planning, writing the files and saving the manifest, at 100 000 text files:
In a Docker container limited to 1 GB,
tfg generate --format txt --size 4kb --count 200000was killed for lack of memory after writing its last file. It left 200 000 files and no manifest, sotfg cleanuphad nothing to remove them by. With this change the same run exits 0 and writes its manifest.Guards
TestTheManifestIsWrittenAsEncodingJSONWritesItchecks that the bytes fromEncodeequal whatencoding/jsonproduces for the same value: no entries, a list that was never made, one entry, many unlike entries, and two thousand entries with escaped text.TestTheManifestIsNotHeldWholeToBeWrittenfails if the document reaches its writer in one piece. Going back tojson.NewEncoderturns it red while the bytes guard stays green.TestAPlanIsReservedRatherThanGrownchecks that a plan's capacity equals its length. Without the reservation, a plan of 1000 files had room for 1359.🤖 Generated with Claude Code
Summary by CodeRabbit