fix(core): Return partial response for headers if one fails

[renaynay]

Fixes an issue where we would discard all headers in case one failed. Now we can at least return the ones we synced successfully (as long as they're contiguous from from) so we don't need to do duplicate work.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 37.22%. Comparing base (2469e7a) to head (f61d92c).
⚠️ Report is 868 commits behind head on main.

Files with missing lines	Patch %	Lines
core/exchange.go	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4738      +/-   ##
==========================================
- Coverage   44.83%   37.22%   -7.61%     
==========================================
  Files         265      311      +46     
  Lines       14620    21588    +6968     
==========================================
+ Hits         6555     8037    +1482     
- Misses       7313    12541    +5228     
- Partials      752     1010     +258     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[walldiss]

A few concerns:

Syncer assumes full range on nil error. In go-header's sync/syncer.go:373, requestHeaders does amount -= size (the requested size, not len(headers)) and advances fromHead to the last returned header. If this returns fewer headers with nil error, the syncer will silently skip the gap between what it got and what it thinks it got. There's also a panic if the partial result is empty (headers[len(headers)-1]).

Concurrent fill makes nil-scan unreliable. errgroup.WithContext cancels remaining goroutines on first error. Earlier slots may still be nil if those goroutines hadn't written yet. The contiguous prefix assumption only holds reliably when failures are at the tail (e.g. beyond chain tip), not for mid-range transient errors.

h.IsZero() is redundant — it's just eh == nil, same as the nil check before it.

Suggestion: either return the error alongside partial results (new sentinel like ErrPartialRange so callers can opt in), or fix the syncer in go-header to use amount -= uint64(len(headers)) before landing this.

[vgonkivs]

Moving it to draft until go-header pr merged and tested. @renaynay do you want to finish it then or should I take it over ?

[greptile-apps]

Greptile Summary

This PR fixes a regression where a single failed height would discard all successfully-fetched headers in getRangeByHeight. It switches from errgroup.WithContext to a plain errgroup.Group so sibling goroutines are not cancelled when one fails, then returns the largest contiguous prefix (headers[:firstNil]) with a nil error instead of propagating the failure.

• core/exchange.go: Replace errgroup.WithContext with a plain errgroup.Group and add a post-Wait scan that returns headers[:i] when the first nil is found at index i > 0.
• core/exchange_partial_test.go (new): Adds gappyBlockAPIClient to simulate a mid-range failure and verifies the contiguous prefix is returned without cancelling lower in-flight fetches.
• core/exchange_test.go: Adds TestExchange_ReturnsPartialRange testing the past-tip case; verifies at least the known-good prefix is returned.
• go.mod / go.sum: Bumps celestiaorg/go-header from v0.8.5 → v0.8.6 to accommodate partial-response semantics in the syncer.

Confidence Score: 5/5

Safe to merge; the contiguous-prefix logic is correct and the errgroup switch achieves its stated goal without introducing data races or incorrect header ordering.

The core logic is straightforward and all edge cases are handled correctly. Go 1.22+ per-iteration variable capture prevents closure bugs, and errgroup.Wait() provides the happens-before guarantee for safe slice reads after goroutines finish. The two flagged observations are efficiency and style concerns, not correctness bugs.

core/exchange.go — the partial-return branch and its interaction with the go-header syncer retry loop is the most nuanced part of the change and worth a second read.

Important Files Changed

Filename	Overview
core/exchange.go	Switches errgroup to a non-cancelling variant and adds contiguous-prefix partial return; logic is correct but all error types (including context cancellation) are silently dropped when any prefix exists.
core/exchange_partial_test.go	New test verifies that a mid-range failure does not cancel lower in-flight fetches; deterministic because errGroup.Wait blocks until all goroutines finish.
core/exchange_test.go	Adds TestExchange_ReturnsPartialRange for the past-tip case; the GreaterOrEqual assertion could be flaky on slow CI if valid-height goroutines do not complete before the 5s timeout.
go.mod	Bumps go-header v0.8.5 to v0.8.6 to pair with the partial-response change in the syncer.

Sequence Diagram

sequenceDiagram
    participant Syncer as go-header Syncer
    participant GRH as GetRangeByHeight
    participant GRP as getRangeByHeight
    participant EG as errgroup.Group
    participant Core as Core gRPC

    Syncer->>GRH: GetRangeByHeight(ctx, from, to)
    GRH->>GRP: getRangeByHeight(ctx, from+1, amount)
    GRP->>EG: spawn goroutines up to 16 concurrent
    par heights from+1 to X-1
        EG->>Core: GetByHeight(ctx, h)
        Core-->>EG: extHeader ok
        EG->>EG: "headers[i] = extHeader"
    and height X fails
        EG->>Core: GetByHeight(ctx, X)
        Core-->>EG: error
        Note over EG: No sibling cancellation plain errgroup
    and heights X+1 to end
        EG->>Core: GetByHeight(ctx, h)
        Core-->>EG: extHeader ok
        EG->>EG: "headers[j] = extHeader"
    end
    EG-->>GRP: Wait returns error
    Note over GRP: Scan headers for first nil at index i
    alt i greater than 0
        GRP-->>GRH: headers up to i nil
        GRH->>GRH: verify contiguous prefix
        GRH-->>Syncer: partial headers nil
    else i equals 0
        GRP-->>GRH: nil err
        GRH-->>Syncer: nil err
    end

Loading

_{Reviews (3): Last reviewed commit: "lint" | Re-trigger Greptile}

[vgonkivs]

LGTM. I'd suggest to mark it as breaking.

@renaynay do you want me to add a test case to tastora ?

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[renaynay]

Yes please add a test case to tastora @vgonkivs but how is this PR breaking?