Skip to content

Keep a pack-observer Scanner error from aborting the upload#84

Merged
Soph merged 1 commit into
mainfrom
fix/pack-observer-non-fatal
Jun 18, 2026
Merged

Keep a pack-observer Scanner error from aborting the upload#84
Soph merged 1 commit into
mainfrom
fix/pack-observer-non-fatal

Conversation

@Soph

@Soph Soph commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Problem

packStreamObserver tees the pack upload stream through a goroutine running packfile.Scanner. When that Scanner stopped early — a malformed pack, or content it couldn't parse — consume's defer pr.Close() broke the pipe. The next io.TeeReader write then failed with io.ErrClosedPipe, surfaced from Read, and aborted the live push — directly contradicting the documented "non-fatal for the upload" contract. The resulting spurious error also wasn't classified as subdividable by executeBatched.

Fix

Tee through a small bestEffortWriter that absorbs write failures and always reports success, so a stopped observer can never turn into a failed upload. The bytes keep flowing to the server; the Scanner error stays available via ScannerError() for debugging.

Why the writer side, not the pipe lifecycle

observer.Close() is not reliably called in the bootstrap push flow (the error and success paths close the underlying packReader, not the observer), so the obvious alternative — keep draining the pipe with io.Copy(io.Discard, pr) until pw closes — would block forever and leak the goroutine. Absorbing failures at the writer boundary works regardless of whether Close() runs, and removes the spurious push failure entirely (so there's no longer an unclassified error for executeBatched to mishandle).

Tests

TestPackStreamObserverScannerErrorDoesNotAbortUpload feeds non-pack bytes (Scanner fails parsing the header mid-stream) and asserts every byte still comes back out of Read with no error, while ScannerError() records the failure.

🤖 Generated with Claude Code


Note

Low Risk
Localized change to bootstrap pack streaming with a clear contract and regression test; no auth or data-model changes.

Overview
Fixes pack uploads failing when the side-channel packfile.Scanner stops early (malformed pack or parse error). The observer tee used to write directly to the pipe; when consume closed the reader, io.TeeReader surfaced io.ErrClosedPipe from Read and killed the push despite the documented rule that scanner failures are non-fatal for the upload.

The tee now writes through a bestEffortWriter that records the first write failure, drops later observer bytes, and always reports a successful write so the upload stream keeps flowing. Failures remain visible via ScannerError() for debugging.

Adds TestPackStreamObserverScannerErrorDoesNotAbortUpload, which streams non-pack garbage, expects a full byte-for-byte ReadAll with no error, and asserts a scanner error was stored.

Reviewed by Cursor Bugbot for commit f6bc49f. Configure here.

The observer tees the pack stream through a goroutine running packfile.Scanner.
When that Scanner stopped early (a malformed pack, or content it couldn't
parse), its deferred pr.Close() broke the pipe, so the next TeeReader write
failed with io.ErrClosedPipe and surfaced from Read — aborting the live push,
despite the documented "non-fatal for the upload" contract.

Tee through a bestEffortWriter that absorbs write failures and always reports
success, so a stopped observer can never turn into a failed upload. The bytes
keep flowing to the server and the Scanner error remains available via
ScannerError() for debugging. (This removes the spurious push failure, so
there is no longer an unclassified error for executeBatched to mishandle.)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: b7dda726e331
@Soph Soph merged commit 146fcd5 into main Jun 18, 2026
4 checks passed
@Soph Soph deleted the fix/pack-observer-non-fatal branch June 18, 2026 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants