Skip to content

fix(search): skip unreadable blobs with a warning instead of aborting - #11

Merged
hammadmajid merged 2 commits into
mainfrom
fix/skip-unreadable-blobs
Sep 5, 2026
Merged

hammadmajid merged 2 commits into
mainfrom
fix/skip-unreadable-blobs

Conversation

@hammadmajid

Copy link
Copy Markdown
Contributor

Description

Since 6393c33 a single blob that cannot be read (zero-byte or corrupt loose object, bad zlib stream, bad pack entry) aborted the entire search: workers pushed the read error onto errCh, Pipeline.Execute returned it, and Aggregator.AggregateChannel bailed on the first res.Error. One missing blob in git fsck meant exit 2 and no output at all. v0.1.0 tolerated this by accident; git grep and ripgrep both warn on unreadable inputs and keep going.

A per-blob read failure is now a soft error: the blob is skipped with a warning on stderr and the search continues, emitting results for every other blob. Context cancellation, matcher failures, history-walk and repository-discovery errors are unchanged and remain fatal.

Fixes #10

Type of Change

  • Bug fix (non-breaking change fixing an issue)
  • New feature (non-breaking change adding functionality)
  • Performance improvement
  • Refactoring or code cleanup
  • Documentation update
  • CI/CD or build workflow change

Key Changes

  • internal/search: new BlobReadError{OID, Path, Err} (Path = first occurrence path). processTask produces it for reader.ReadObject failures; workers still deliver that result on resultsCh (so callers can warn) but no longer push it onto errCh. All other errors keep the existing fatal path.
  • internal/aggregator: AggregateChannel skips BlobReadError results instead of returning; any other res.Error still aborts.
  • cmd/grg: prints grg: warning: skipping blob <oid> (<path>): <err> per skipped blob to stderr before any match output; a new skippedBlobsError (exit 2, no extra message since each blob was already reported) folds the count into the exit code. The pre-existing exit-code error types were moved into cmd/grg/errors.go in a separate pure-move commit (main.go was over the 350-line target).
  • README.md: exit-code section updated to the ripgrep semantics below. It also previously claimed 128 for repository errors, which the code never returned.

Exit-code semantics

Outcome Exit
Match found, no blob skipped 0
No match, no blob skipped 1
Any blob skipped (matches may still have been printed) 2
-q and a match found (even with skipped blobs) 0
-q, no match, blob skipped 2
Fatal error (CLI, repo discovery, cancellation, ...) 2, unchanged

Warnings are printed to stderr even under -q, matching ripgrep (which has a separate --no-messages for suppressing them).

Verification & Testing

  • Ran go test -v -count=1 ./...
  • Ran go test -race -count=1 ./...
  • Ran go vet ./...
  • Added or updated unit/integration tests
  • Tested manually against sample Git repository histories (covered by the integration tests below, which drive the built binary against a real repo)

Also ran golangci-lint run ./... (0 issues), go mod verify, and govulncheck ./... (0 vulnerabilities in reachable code).

Tests:

  • internal/search: TestPipelineExecuteContext_ReaderError previously asserted that a read failure reached errCh, i.e. the exact fatal behaviour this fixes; it now asserts the soft BlobReadError path. New TestPipelineSkipsUnreadableBlob uses a fake ObjectReader where one OID returns ErrObjectNotFound / ErrCorruptObject and asserts the other blobs' matches are returned, the failing OID and first path are reported, and no fatal error is returned.
  • internal/aggregator: TestAggregator_AggregateChannel_SkipsBlobReadError; the existing test asserting a generic res.Error is fatal is kept.
  • test/integration/corrupt_object_test.go: creates a real repo, commits and later deletes a file (so the blob is only reachable through history, as in the issue), truncates its loose object to zero bytes, and asserts: matches from the healthy blob are printed, stderr has exactly one warning naming the OID and path, exit 2; exit 2 with no stdout when only the corrupt blob could match; -q semantics above; and a healthy repo still exits 0/1 with empty stderr.

Checklist

  • My code adheres to the project's coding style and architecture guidelines (modular, files under 350 lines).
  • No Cgo or external runtime dependencies have been introduced.
  • Existing tests pass and new tests cover added functionality.
  • Documentation / README has been updated where appropriate.

🤖 Generated with Claude Code

https://claude.ai/code/session_016ut4mEjEnEXdJZejxFm8gT

hammadmajid and others added 2 commits September 5, 2026 12:13
Pure move, no behavior change. main.go had grown past the 350-line target in
CONTRIBUTING.md and the exit-code mapping is about to gain a case for
skipped blobs, so give it its own file.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016ut4mEjEnEXdJZejxFm8gT
Since 6393c33 a single blob that cannot be read (missing or corrupt loose
object, bad zlib stream, bad pack entry) aborted the whole search: workers
pushed the read error onto errCh, Pipeline.Execute returned it, and
AggregateChannel bailed on the first res.Error. One zero-byte object under
.git/objects therefore meant exit 2 with no output. v0.1.0 silently
tolerated it, and both git grep and ripgrep warn and keep going.

processTask now wraps a reader.ReadObject failure in search.BlobReadError
{OID, Path, Err}. The pipeline still delivers that result on resultsCh (so
callers can warn) but no longer reports it on errCh, and AggregateChannel
skips it instead of returning. Every other error (context cancellation,
matcher failures, history walk and discovery errors) is unchanged and stays
fatal.

The CLI prints one line per skipped blob to stderr before any match output:

  grg: warning: skipping blob <oid> (<first path>): <err>

Exit codes follow ripgrep: 0 when matches were found and nothing was
skipped, 1 when nothing matched and nothing was skipped, 2 when any blob
was skipped (matches may still have been printed). --quiet keeps its
contract: a match found still exits 0, no match with a skipped blob exits 2.
The README exit-code table is updated accordingly (it also claimed 128 for
repository errors, which the code never returned).

Tests: TestPipelineExecuteContext_ReaderError previously asserted that a
read failure reached errCh, i.e. the fatal behaviour this fixes; it now
asserts the soft BlobReadError path. New unit tests cover the pipeline and
aggregator with a fake ObjectReader, and test/integration exercises a real
repo whose loose blob is truncated to zero bytes.

Fixes #10

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016ut4mEjEnEXdJZejxFm8gT
@hammadmajid
hammadmajid merged commit 0fac95d into main Sep 5, 2026
3 checks passed
@hammadmajid
hammadmajid deleted the fix/skip-unreadable-blobs branch September 5, 2026 07:23
hammadmajid added a commit that referenced this pull request Sep 5, 2026
gofmt -l was failing on nine files untouched by #11, so the CONTRIBUTING
format check was red on main. Whitespace and alignment changes only.

The bare `grg` pattern in .gitignore also matched the cmd/grg directory,
so new files there needed `git add -f`. Anchor it to the repo root.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016ut4mEjEnEXdJZejxFm8gT
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Regression: a single unreadable blob aborts the entire search since v0.2.0

1 participant