Skip to content

fix(grpc): return errors instead of panicking for unservable FollowTip/WatchTx intersect - #1226

Open
AmbientTea wants to merge 1 commit into
txpipe:mainfrom
AmbientTea:fix/grpc-stream-intersect-not-found
Open

fix(grpc): return errors instead of panicking for unservable FollowTip/WatchTx intersect#1226
AmbientTea wants to merge 1 commit into
txpipe:mainfrom
AmbientTea:fix/grpc-stream-intersect-not-found

Conversation

@AmbientTea

@AmbientTea AmbientTea commented Aug 18, 2026

Copy link
Copy Markdown

Hello, I'm submitting a fix for a panic that bit me when subscribing to the block events stream:

Issue

ChainStream::start calls unwrap twice: when creating the crawler and when searching for the intersect point. This caused panics in Dolos when the client tried to connect with cursor that resulted in an empty intersection, which left the client hanging on the open connection.

Solution

Change ChainStream::start to return Result (crawler creation errors) of an Option (no intersection point found), and turn those two cases into distinguishable errors in the callers.

Summary by CodeRabbit

  • Bug Fixes
    • Improved error handling when starting chain synchronization and watch streams.
    • Requests with no matching local history now return a clear “not found” response.
    • Startup failures now return an internal service error instead of causing unexpected failures.
    • Added coverage for requests involving unknown intersection points.

ChainStream::start ran ChainCrawler::start inside an async_stream::stream!
block and unwrapped both its Result and its Option. A client resuming
FollowTip or WatchTx from a point this node no longer holds (e.g. after a
snapshot restore) hit the None case on every subscription, panicking a
tokio worker while the client's stream just hung open with no status.

Move the fallible crawler start ahead of the stream! block and have
ChainStream::start return Result<Option<Stream>, DomainError>: None means
the intersect wasn't found, Err means the crawler itself failed to start.
Callers in the v1alpha/v1beta sync and watch services map None to
Status::not_found (naming the requested points) and Err to
Status::internal.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@AmbientTea
AmbientTea requested a review from scarmuega as a code owner August 18, 2026 12:35
@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4e91d26f-a630-4d19-92fe-44db4c25a369

📥 Commits

Reviewing files that changed from the base of the PR and between 81f7ba7 and aafcd89.

📒 Files selected for processing (5)
  • src/serve/grpc/stream.rs
  • src/serve/grpc/v1alpha/sync.rs
  • src/serve/grpc/v1alpha/watch.rs
  • src/serve/grpc/v1beta/sync.rs
  • src/serve/grpc/v1beta/watch.rs

Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

ChainStream::start now reports crawler errors and missing intersections through a fallible optional result. gRPC sync and watch handlers map these outcomes to internal or not-found statuses. Tests cover successful startup and unknown intersections.

Changes

Chain stream startup handling

Layer / File(s) Summary
Fallible stream contract and tests
src/serve/grpc/stream.rs
ChainStream::start now returns Result<Option<Stream>, DomainError>. It propagates crawler-start errors and returns Ok(None) when no intersection exists. Tests cover both outcomes.
gRPC status mapping
src/serve/grpc/v1alpha/sync.rs, src/serve/grpc/v1alpha/watch.rs, src/serve/grpc/v1beta/sync.rs, src/serve/grpc/v1beta/watch.rs
Sync and watch handlers map startup errors to internal status and missing streams to not-found status. Watch handlers retain the requested intersection points for error reporting.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to aafcd

This localized change replaces panic paths with returned errors for unservable stream intersections; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: scarmuega

Sequence Diagram(s)

sequenceDiagram
  participant GRPCHandler
  participant ChainStream
  participant ChainCrawler
  GRPCHandler->>ChainStream: start with intersection
  ChainStream->>ChainCrawler: start crawler
  ChainCrawler-->>ChainStream: startup result
  alt startup error
    ChainStream-->>GRPCHandler: internal status
  else no matching intersection
    ChainStream-->>GRPCHandler: not-found status
  else stream available
    ChainStream-->>GRPCHandler: continue stream setup
  end
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: gRPC FollowTip and WatchTx requests now return errors instead of panicking for unavailable intersections.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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.

1 participant