Repository navigation
fix(api): cap diff-commit's patch, and read git apply's stderr as it streams - #778
Conversation
…streams diff-commit capped each decoded chunk at 4 MiB but not their sum, and applyPatch kept all of git apply's stderr to classify it. git apply prints a line per file that fails, so that buffer grew with the patch (850 KB of stderr for a 1.7 MB patch of 20,000 files). diff-commit now takes at most 262144000 bytes of decoded patch, the commit-pack limit, and answers 400 invalid past it, as for a chunk over 4 MiB. applyPatch reads stderr with wcApplyStderr, which keeps only the facts that the status comes from, the head that the client sees and the tail that the log sees: about 128 KiB, whatever git prints. wcApplyStatus and wcApplyServerFailure read a whole stderr through it. Change-Id: I344ac48019953424ec485f3fd139a85feeca331a Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I31c306e28e212d72c9c1eb2cc70617e3033f1078 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…ument the diff cap The head stands for a longer stderr, so one path builds the message. docs/explanation/streaming-commit-protocols.md and the HTTP API status table state diff-commit's 262144000-byte patch limit. Change-Id: Iaefe7c73e5e4cebfe796c84e25cbdb0289321881 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Change-Id: I8a357c561f87f5ca89f520f1f76f49ba8a658992 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I5f9a6e6a747d5cceedfa50c7f337f4c76939eff9 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…me RunIn for hydrate.go TestTailBufferStartsAtACharacter cut only one continuation byte, so a String that dropped a single byte passed. A 4-byte-rune case now leaves three. The lease audit's reason for hydrate.go names RunIn, which updateRefStdin uses since the stderr-tail change. Change-Id: I324e027199616c999fd32b89bb7be3bb4f4f4194 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…survived TailBuffer: a cut that small writes bring, as exec's 32 KiB writes do; a cut that leaves three continuation bytes; a Write of exactly 64 KiB, alone and after earlier bytes; and a Write whose array the caller changes afterwards. HeadBuffer: the memory it holds after 64 MiB, and a Write that crosses the limit with varied bytes. Change-Id: Ie4a1e59dbe945da38a0cba4e2dc6766fdcb120ea Signed-off-by: Thomas Kosiewski <tk@coder.com>
CombinedOutputIn has no caller, but it buffered all of the child's output, so a caller that quoted it in an error would reopen the unbounded stderr that TailBuffer closes for the other runners. It keeps its signature and returns the TailBuffer's tail as written. Change-Id: I42d7740d57287e674189363149d535abd7d2e444 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Each small write past 64 KiB moved the whole kept tail to the front, so 64 MiB of 50-byte writes copied about 87 GB. The buffer now takes up to 2 x 64 KiB and then moves the last 64 KiB to the front once, and it grows its array to at most 2 x 64 KiB. String reads the last 64 KiB. Tests pin the bytes copied, the memory after every write, and the tail after every write of a mixed-rune stream. Change-Id: Id2701798c02c04934eb05690d0369f18c43549e2 Signed-off-by: Thomas Kosiewski <tk@coder.com>
A 64 MiB stderr line with no newline keeps only its start for the line checks. The longest server path counts at every split position of two writes, and in one-byte writes. Change-Id: If4df323324e4894b56aa04793ace781c4edc9998 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Why
diff-commit capped each decoded chunk at 4 MiB but not their sum.
applyPatchkept all ofgit apply's stderr in memory to classify it (#759), and git apply prints a line per file that fails. So that buffer grew with the patch: with git 2.55.0, a 1,735,560-byte patch of 20,000 files that the base does not have gave 848,890 bytes of stderr. #776 named this as the one runner that keeps all of git's stderr.What changes
A total patch cap. diff-commit takes at most 262144000 bytes of decoded patch, summed over its chunks: the commit-pack limit. Past that, the server stops reading and answers 400
invalidwithdiff exceeds 262144000 bytes, as it answers a chunk over 4 MiB. Upstream's diff-commit lists 400, 409 and 412, and no 413. No ref moves.git apply's stderr as it streams.
wcApplyStderrreads git apply's stderr as git writes it, and keeps only whatapplyPatchneeds:wcApplyPayloadLines(400), withwcApplyClashLine(409) or withfatal:, and whether stderr names the scratch directory or the cached repository (500, as before)gitlimit.TailBuffer, which the log's error quotes, as beforeIt keeps about 128 KiB of content (a 64 KiB head and a 64 KiB tail) and uses at most about 192 KiB of memory, whatever git prints. A line counts by its first bytes, and a server path that two writes split still counts.
wcApplyStatusreads a whole stderr through it, andwcApplyServerFailure, which onlyapplyPatchcalled, goes.Lines are read as git wrote them. The old code read them after
strings.TrimSpaceof the whole stderr, which differs only for white space before git's first line, or a last line that ends in a space that a known line head ends with.docs/explanation/streaming-commit-protocols.mdand the HTTP API status table state the patch limit.gitlimit.TailBuffer's small writes cost linear time. Each small write past 64 KiB moved the whole kept tail to the front, so 64 MiB of 50-byte writes copied about 87 GB. git writes stderr a message at a time, and a slow git gives one pipe read per line. The buffer now takes up to 2 × 64 KiB, then moves the last 64 KiB to the front once, and it grows its array to at most 2 × 64 KiB.Stringreads the last 64 KiB, from its first character when more was written.gitlimit.CombinedOutputInkeeps only the last 64 KiB of the output in aTailBuffer, with the same signature. It has no caller, so nothing changes today, but a future caller cannot quote an unbounded output.The lease audit's reason for
hydrate.gonamesRunIn, whichupdateRefStdinuses since fix: keep only the tail of git's stderr in every git runner's error #776.Verification
TestDiffCommitTotalSizeLimitlowers the cap to one patch's size: one byte over it answers 400 with the message and moves no ref, and exactly at it the commit lands.TestDiffCommitManyFailingFilessends the 20,000-file patch on every backend: 409conflict, the message starts with git's first line and is cut to 4096 bytes, and no ref moves.TestApplyStderrKeepsABoundedPartwrites that patch's stderr in 4 KiB pieces: about 128 KiB is kept, and the status, the server-failure answer and the message equal the old whole-stderr reading.FuzzApplyStderrChunksfeeds a stderr in random pieces and compares each answer with the old whole-stderr reading (kept in the test as the oracle). It ran 2 minutes (26,306 inputs on 4 workers), andFuzzApplyRejection1 minute (146,364 inputs), with no failure.TestApplyStderrLongLineKeepsItsStartwrites a 64 MiB line with no newline: the line buffer holds at most 2 ×wcApplyLineKeepbytes, and the status is 400.TestApplyStderrFindsASplitServerPathsplits the longest server path at every byte, and writes it a byte at a time.TestTailBufferSmallWritesCopyLittlecounts the bytes that 4 MiB of 50-byte writes copy (at most twice the bytes written).TestTailBufferFollowsEveryWritechecks the tail after every write of a stream of 4-, 3-, 2- and 1-byte runes. Other tests check the memory after every write, an exact 64 KiB write alone and after one byte, a caller that reuses its array, and, forHeadBuffer, its memory after 64 MiB and a write that crosses the limit with varied bytes.TestCombinedOutputInKeepsTheLast64KiBruns a child that prints 1 MiB. Each of these mutants fails a test: the old quadratic move, a move that does not set the cut flag, a view that does not set it, growth past 2 × 64 KiB, a move one byte short, a view of 64 KiB + 1, the verifier's TailBuffer and HeadBuffer mutants (a cut flag not set on small writes, a strip of at most 2 bytes, a large write that keeps the caller's array, a cut on an exact fit, a large write after earlier bytes without the cut, a HeadBuffer that keeps all or keeps a write's tail), and an unboundedCombinedOutputIn.Lanes
On this head, with
-racefor every test run:go build ./...,go vet -tags=openstorage_viewcheck ./..., golangci-lint (0 issues),scripts/check-doc-links.py(0 broken links), and both fingerprint checks (check.sh --fingerprints-onlyof verify-lane-schedule and verify-orphan-sweep: PASS, no re-record).internal/gitlimit/...passes.internal/apipasses on PostgreSQL (with-tags=openstorage_viewcheck), local and S3-WAL.internal/githttp,internal/jobs,internal/lfsscan,internal/syncandinternal/simtestpass, because they readTailBuffer.Size
683 changed lines: 249 of production Go (204 in
internal/api/handlers_write_commit.go, 45 ininternal/gitlimit), 426 of tests and 8 of docs.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh