Conversation
There was a problem hiding this comment.
Pull request overview
Fixes log fanout behavior in the private API logs subscription so that a single subscriber send failure does not prevent healthy subscribers from receiving the same log.
Changes:
- Update
distributeLogsto continue broadcasting a log to remaining subscribers even if oneSendfails. - Add a regression test that simulates failing subscribers and asserts healthy subscribers still receive all logs and failing subscribers are removed.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| node/privateapi/logsfilter.go | Adjusts send-failure control flow so one broken subscriber doesn’t abort per-log fanout. |
| node/privateapi/logsfilter_test.go | Adds a failing server stub and a regression test covering send-failure fanout behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err := filter.sender.Send(proto); err != nil { | ||
| filtersToDelete[filterId] = filter | ||
| continue outerLoop | ||
| continue | ||
| } |
There was a problem hiding this comment.
On Send error, this code schedules the filter for deletion inside distributeLogs, but the corresponding subscribeLogs handler still has a deferred removeLogsFilter(filterId, filter). If distributeLogs deletes/subtracts first, the deferred removeLogsFilter will later subtract the same filter again, potentially driving aggLogsFilter counters negative and leaving EmptyLogSubscription state incorrect. Consider making removal idempotent (e.g., in removeLogsFilter: check that logsFilters[filterId] still exists and matches the same filter pointer before subtracting/deleting), or avoid subtracting/deleting here and instead mark/close the subscriber so the deferred cleanup is the single removal path.
| if err := filter.sender.Send(proto); err != nil { | ||
| filtersToDelete[filterId] = filter | ||
| continue outerLoop | ||
| continue | ||
| } |
There was a problem hiding this comment.
Once a subscriber Send fails, the filter stays in a.logsFilters until the end of distributeLogs, so subsequent logs in the same batch will keep attempting Send (and failing) for that subscriber. Since you already track filtersToDelete, you can skip Send for filterIds already marked to reduce repeated failures/overhead when logs contains many entries.
A failed
SendinSubscribeLogscurrently skips the rest of subscribers for the same log (continue outerLoop), which causes observable log loss for healthy subscribers. This patch keeps broadcasting the current log to remaining subscribers, still removes broken subscribers after dispatch, and addsTestLogsFilter_SendFailure_DoesNotSkipHealthySubscribersto reproduce the bug and lock in the correct behavior.