Repository navigation
fix(gitread): decline a tree walk whose cat-file ended before the request - #774
Conversation
…uest TestLsTreePageCursorWhenTheWalksGitFails/silent_end failed in #764's merge-group run: "resumeTreePage = write |1: broken pipe". Its stand-in git exits 0 on cat-file --batch without reading. When the walk's first request reached the pipe before Run had reaped that git, the walk read the end of the output and declined. When it came after, the write failed with EPIPE, and the walk returned that error instead of declining. treeSource.request now declines on a failed write when git ended cleanly, as readErr does for an output that ends. A git that failed still gives its own error. LsTreePage answered with the stream in both cases, so no page changes. TestTreeSourceRequestAfterGitEnded waits for the stand-in to end before the request, which makes the late write certain: with the old code it fails with the merge group's broken pipe. Change-Id: Id9621239573398e698d520dea0681f309f68110b 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. 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
TestLsTreePageCursorWhenTheWalksGitFails/silent_endfailed in #764's merge-group run (actions run 42416712375, jobtest):#764 does not touch
internal/gitread. The test's stand-in git exits 0 oncat-file --batchwithout reading its input. A cat-file child that ends before it answers must make the walk decline (errDeclined), so the page falls back to the stream. The walk did that only when its first request reached the pipe before the child was reaped. Then it read the end of the output, andreadErrdeclined. When the request came later, the write failed with EPIPE, andtreeSource.requestreturned that error instead of declining.The page was the same in both cases:
leftToStreamsends a walk's git error to the stream too. Only the walk's own outcome changed with timing, and the test checks that outcome.What changes
treeSource.requestdeclines when the write fails and git ended cleanly. A git that failed (an exit status, a crash, a heap cap) still gives its own error, as before.Verification
The race needs the walk's write to land after the child has been reaped. Plain runs did not hit it: 0 failures in 200 runs, and 0 in 240 runs with the test process stopped 80% of the time (SIGSTOP 20 ms, SIGCONT 5 ms). So I widened the window: an overlay (not in this PR) sleeps 500 ms before each request write. With the same overlay and N on both sides:
With the fix and the overlay, the other stand-ins (heap cap, exit, crash) still give git's error, 3 of 3 runs each.
TestTreeSourceRequestAfterGitEndedmakes the late write certain without the overlay. It waits for the stand-in to end before it sends the request. Against main:The fixed test passes, and so does an exit-status stand-in, which keeps git's error.
Lanes on this head, all with
-race:internal/gitlimit,internal/gitread,internal/apiandcmd/openstoragepass on PostgreSQL and S3-WAL (-tags=openstorage_viewcheck) and on SQLite. Build, vet, golangci-lint (0 issues), the doc-link check, both--fingerprints-onlychecks and the gitread cross-compiles (linux and darwin, amd64 and arm64) pass. No file that the verify-lane-schedule or verify-orphan-sweep fingerprints cover changes.Size
35 changed lines: 6 of production Go and 29 of tests.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh