CI only: wide-directory memory work (do not merge) - #5
Open
msaggiorato wants to merge 8 commits into
Open
msaggiorato wants to merge 8 commits into
msaggiorato wants to merge 8 commits into
Conversation
saveDir submitted every entry of a directory and only folded the results into the directory's tree once the whole directory had been walked, so a finished entry was held -- its node, the two paths naming it and its channel, about a kilobyte in all -- until then. A directory of a million entries therefore needed roughly a gigabyte on top of the tree itself, which is enough for a host with a memory cap to kill the backup. The entries of a directory are folded into its tree in the order they appear, so a finished entry need not be kept. Fold them in as the walk goes, holding only enough to let the entries ahead of a slow one finish out of order. The file saver's queue is unbuffered, so the walk was already paced by the file readers and consuming results earlier costs no concurrency. Directories no wider than the limit behave exactly as before. Two smaller costs went with it: each entry's name is released once the paths have been built from it, and the buffer holding the tree is sized from the directory's entry count rather than being grown and copied repeatedly, which held two copies of a large tree at once. Backing up one directory of 600000 entries under a 600 MiB cap failed before and now succeeds; the same backup uncapped uses 39% less memory at a million entries. The tree written is byte for byte unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017d5y4KNmm2N5ySPPH1H49Z
verifyCiphertext decrypted and decompressed the blob it had just sealed and rehashed it, holding the whole plaintext a second time to do so. For the tree of a very wide directory -- several hundred megabytes -- that doubled the cost of saving it at exactly the moment the first copy was still live. Hash the plaintext as it decompresses instead, for blobs large enough that setting up a streaming decoder is worth it. What gets verified is unchanged; only the copy is gone. Measured on a backup of one directory of a million entries, peak RSS falls from 773 MB to 603 MB. Disabling verification entirely reaches 592 MB, so almost all of the difference was the copy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017d5y4KNmm2N5ySPPH1H49Z
A directory's tree is one blob listing every entry, a few hundred bytes each, and it was built in memory: held while the directory was walked, then compressed and encrypted from that same copy. For a directory of a million entries that is several hundred megabytes, and it was by then the largest single cost of the backup -- enough that a host which caps process memory kills the backup outright. Write the tree of a wide directory to a temp file as the walk goes, and hand the repository the file rather than a buffer. saveBlobFromReader hashes and compresses in one pass as it reads, declaring the plaintext size in the zstd frame header exactly as EncodeAll does, so whoever reads the blob back can still size its buffer in one go. The plaintext never exists in one piece anywhere. The temp file comes from fs.TempFile, which returns a file already unlinked, so closing it is the whole of the cleanup. Creating it is a nicety rather than a requirement: if it fails, the tree is built in memory as before. Directories below the threshold are untouched. This is what rsync does with its file list -- stream the metadata rather than materialise it -- adapted to a format that needs each directory's listing addressed as one blob. One directory of a million entries: peak RSS 208 MB, against 1248 MB before any of this work and 603 MB after bounding the pending nodes. 1.2M entries succeeds under a 400 MiB cap. The tree written is byte for byte unchanged, a restore of it matches the source exactly, and check --read-data passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017d5y4KNmm2N5ySPPH1H49Z
The temp file holding a wide directory's tree was only closed once the tree had been handed to the repository, so four paths leaked the descriptor: either error return in treeSaver.save, a cancellation that made Save drop the job before a worker took it, and any error out of saveDir once the builder existed. os.File's finalizer closes an abandoned descriptor eventually, but an error affecting many wide directories could exhaust the table before the garbage collector got to them. Release in a defer as soon as ownership is known, hand ownership over explicitly where it passes to the tree saver, and make release safe to call twice and on a builder that never spilled. Also add the corruption tests the streaming verification was missing. It is the last check before a blob is uploaded, so a version of it that always passed would be worse than no check at all: prove it rejects a wrong id, a bit flipped at three different offsets, a truncated stream and input that was never compressed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017d5y4KNmm2N5ySPPH1H49Z
msaggiorato
force-pushed
the
ci-check/wide-directory-memory
branch
from
September 17, 2026 23:28
657b106 to
97d813c
Compare
A cancelled backup returns from treeSaver.save while the repository is still reading the temp file holding a wide directory's tree, so releasing that file in a defer there closed it underneath the upload, which then failed with "file already closed". The failure was confined to a path that was already failing, and the size check in saveBlobFromReader means no short blob could result, but the error was one we invented rather than one reported by anything real. Hold the file until the repository is finished with it, which the callback signals, and keep the defer for the paths where it was never handed over. TestTreeSaverSpilledTreeSurvivesCancel reproduces it: it reaches the spilling path by asking for a directory as wide as the threshold rather than by feeding the builder that many nodes, and holds the mock repository's read until after the cancellation, so the ordering is deterministic rather than raced. Without this change it fails with exactly the error above. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017d5y4KNmm2N5ySPPH1H49Z
msaggiorato
force-pushed
the
ci-check/wide-directory-memory
branch
from
September 18, 2026 01:59
65edf3e to
871c545
Compare
saveBlob hashes a blob, asks the index whether the repository already has
it, and compresses only if it does not. saveBlobFromReader could not follow
that order, because it hashed and compressed in a single pass, so the tree
of an unchanged wide directory was compressed on every backup and then
thrown away when the index said the blob was known.
Read the stream twice instead: once to hash, and again to compress only
when the blob turns out to be new. That restores saveBlob's order, and
costs a second pass over a temp file whose pages the kernel has just
written and still holds.
Measured on a 47 MB tree shaped like a real WordPress uploads directory --
varied names and times, and a content id per node, so it compresses like
the real thing rather than like a repeated string:
hash only 43 ms
compress only 230 ms
hash + compress 275 ms
So recognising a known tree cost six times what it needed to. On the
committed benchmark, whose tree is more repetitive and therefore kinder to
the compressor, saving a known blob goes from 33 ms to 15 ms and saving a
new one from 70 ms to 59 ms. For a directory of a million entries this is
seconds per backup rather than a bounded cost, since the tree grows with
the directory.
The stream therefore has to be seekable, which the interface now says. Its
one caller hands over a temp file. Also stop asking the encoder to close a
stream it has already closed, and size the compressed buffer at a quarter
of the plaintext rather than an eighth, which is nearer what a real tree
achieves and so avoids a resize that would hold two buffers at once.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017d5y4KNmm2N5ySPPH1H49Z
The test jobs no longer get past "Get programs": the open source MinIO server is archived and dl.minio.io answers every download with "410 Gone", so the wget fails and the whole matrix stops before a single test runs. Install it with go install from the last release instead, which also drops the per-OS branch and picks up the runner's own architecture rather than the amd64 build. The version is pinned next to latest_go because the repository is archived and its master branch has moved on since. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LhQ5vniAMEFx1V4PtwEuWD
A same-repo pull request satisfies the condition guarding those tests, so they run and then fail for want of secrets. This branch exists only to run the matrix, so drop them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017d5y4KNmm2N5ySPPH1H49Z
msaggiorato
force-pushed
the
ci-check/wide-directory-memory
branch
from
September 18, 2026 02:14
871c545 to
0680b40
Compare
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.
Exists only to run the full test matrix against
upstream/bound-wide-directory-memory, which cannot get CI on its own (the workflow runs on pull requests, and pushes to non-master branches are ignored).Carries the minio-from-source fix on top so the
Get programsstep does not die ondl.minio.ioreturning 410 Gone. That extra commit is why this is a throwaway branch rather than the real one.Do not merge. The reviewable branch is
upstream/bound-wide-directory-memory.🤖 Generated with Claude Code
https://claude.ai/code/session_017d5y4KNmm2N5ySPPH1H49Z