fix(rpc): more fixes in filecoin chain notify API

[akaladarshi]

Summary of changes

Changes introduced in this pull request:

• Removing the closed subscription from the registry

Reference issue to close (if applicable)

Partially Closes #5795

Other information and links

Change checklist

• I have performed a self-review of my own code,
• I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
• I have added tests that prove my fix is effective or that my feature works (if possible),
• I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Outside contributions

• This pull request is based on an issue that a maintainer has accepted (see Before Opening a Pull Request).
• I have read and agree to the CONTRIBUTING document.
• I have read and agree to the AI Policy document. I understand that failure to comply with the guidelines will lead to rejection of the pull request.

Summary by CodeRabbit

• Bug Fixes

  • Improved RPC subscription cleanup when channels close or sources shut down.
  • Reusing a request ID now properly closes the previous subscription.
  • Subscription cancellation works immediately, including before acceptance and when authorization filters are enabled.
  • Enforced per-connection subscription limits.
  • Serialization failures no longer stop later notifications from being delivered.
  • Improved handling of lagged subscriptions and added warnings for slow chain-notification subscribers.

• Documentation

  • Updated the unreleased changelog with RPC subscription fixes.

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f3ac82d3-8d45-483e-bbfd-d1ae11b47bf1

📥 Commits

Reviewing files that changed from the base of the PR and between 0fd098f and 7af7842.

📒 Files selected for processing (1)

• src/rpc/channel.rs

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• filecoin-project/lotus (manual)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/rpc/channel.rs

Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.

---

Walkthrough

The PR updates RPC subscription registration, cancellation, cleanup, notification serialization, allow-list handling, request-ID replacement, and ChainNotify backlog monitoring. It adds focused tests and changelog entries.

Changes

RPC subscription behavior

Layer / File(s)	Summary
Subscription registration and ownership CHANGELOG.md, src/rpc/channel.rs	ChannelRegistration now owns registry cleanup. Reused request IDs close displaced channels. Pending and accepted sinks retain registration and permit state. Tests cover registration, cancellation, source closure, and request-ID replacement.
Channel pump lifecycle src/rpc/channel.rs	Serialization failures are skipped without stopping later notifications. Close handling and shutdown now clean up channel state. Lagged-channel tests verify stream termination.
Cancellation authorization src/rpc/filter_layer.rs	xrpc.cancel bypasses the configured allow-list. Tests cover cancellation, allowed methods, and rejected methods.
Chain notification backlog monitoring src/rpc/methods/chain.rs	ChainNotify increases broadcast capacity and logs when buffered head-change batches exceed five entries.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to 7af78

This change removes closed subscriptions from the registry, and no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: lesnyrumcajs, eclesiomelojunior

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The changes address subscription cleanup, channel closure, cancellation, request ID reuse, and subscription limits. However, issue #5795 requires direct WebSocket integration tests for channel behavio…	Add the required direct WebSocket integration tests for subscription creation and closure, simultaneous active channels, and current, apply, and revert ChainNotify notifications. Ensure all integration and unit tests pass.
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies RPC and Filecoin chain notification fixes. It is concise and related to the primary change.
Out of Scope Changes check	✅ Passed	The changes remain within RPC subscription and Filecoin chain notification scope. The changelog update and notification backlog warning support the documented fixes and do not introduce unrelated func…

Full details: Linked Issues check

Explanation

The changes address subscription cleanup, channel closure, cancellation, request ID reuse, and subscription limits. However, issue #5795 requires direct WebSocket integration tests for channel behavior and ChainNotify notifications, and the summary lists only unit tests and implementation changes.

Full details: Out of Scope Changes check

Explanation

The changes remain within RPC subscription and Filecoin chain notification scope. The changelog update and notification backlog warning support the documented fixes and do not introduce unrelated functionality.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch akaladarshi/fix-more-chain-notify-issues

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch akaladarshi/fix-more-chain-notify-issues

---

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

[codecov]

Codecov Report

❌ Patch coverage is 95.95960% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.88%. Comparing base (d231c4f) to head (93d5d1d).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/rpc/channel.rs	95.71%	1 Missing and 2 partials ⚠️
src/rpc/methods/chain.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
src/rpc/filter_layer.rs	79.41% <100.00%> (+18.18%)	⬆️
src/rpc/filter_list.rs	97.56% <100.00%> (+0.12%)	⬆️
src/rpc/methods/chain.rs	60.69% <75.00%> (+0.03%)	⬆️
src/rpc/channel.rs	93.48% <95.71%> (+0.48%)	⬆️

... and 10 files with indirect coverage changes

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8099e3b...93d5d1d. Read the comment docs.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[LesnyRumcajs]

I understand this is an existing behavior, but crashing the node when the response is a bit too big is a bit dramatic, isn't it?

[LesnyRumcajs]

I'm not really an expert in the JSON-RPC subscription mechanism and WS, so I employed AI to check some things, especially compatibility with go-jsonrpc (which majority of the Filecoin ecosystem uses). Please check.

Headline: xrpc.cancel still does not work for any go-jsonrpc client, three independent reasons

(a) id-less form never dispatched. go-jsonrpc builds the cancel with ID unset (client.go:379-384, websocket.go:381-385) and request.ID is json:"id,omitempty" (handler.go:56), so the wire frame is a JSON-RPC notification. jsonrpsee RpcService::notification (jsonrpsee-server-0.26.0/src/middleware/rpc.rs:190-195) returns MethodResponse::notification() and never reaches a MethodCallback. No Forest layer converts notifications to calls (src/rpc/mod.rs:665-672). The module doc admits this ("the id-less notification form is currently ignored") while the CHANGELOG reads as if cancel now works. The filter exemption and the register-before-accept race fix are both unreachable for real clients: the dispatch gap, not the filter list, is what blocks cancels.

(b) the reply is the wrong frame kind. close_channel_response (channel.rs:311) emits {"jsonrpc":"2.0","id":null,"result":{"jsonrpc":"2.0","method":"xrpc.ch.close","params":[N]}}, a response with a notification-shaped object nested in result. go-jsonrpc's handleFrame switches on frame.Method; that frame has none, so it lands in handleResponse, never handleChanClose. The client's chanHandlers[N] entry and its Go channel are never released for the life of the websocket. Lotus always sends the bare notification, including after a cancel: cancelling the handler ctx closes the handler's channel, and the select loop then writes request{ID: nil, // notification, Method: chClose} (websocket.go:303-315). Forest's pump instead breaks on the sink.closed() arm and sends nothing. The PR's new "Close path (a)" diagram documents this divergence as if it were the protocol.

(c) id convention is inconsistent. Success uses Id::Null, the error path echoes the request id (channel.rs:379). A spec-compliant client keying pending calls by id never resolves the success case. Tracked as #4453, but this PR rewrote exactly that doc block and added assertions locking it in.

Fixing (b) is cheap and is the one that actually unbreaks Lotus/Curio teardown: send the bare xrpc.ch.close from the pump's unsubscribe arm. (a) needs a notification hook in a layer.

Medium

Reject list also bypassed. filter_layer.rs:50: method_name == CANCEL_METHOD_NAME || self.filter_list.authorize(..) short-circuits before the reject half. FilterList::authorize is "allow-empty-or-matched AND not rejected", with substring matching (filter_list.rs:22-26), so !xrpc.cancel (and even !xrpc) is silently ignored, no log. Keep the reject check in the condition; there is no is_rejected accessor yet, so one has to be added.

Serialization-failure skip contradicts the #5795 sibling entry. channel.rs:429: a batch that fails to serialize is now dropped and the channel stays open, 12 lines above a RecvError::Lagged arm that closes the channel specifically to avoid undetectable gaps, and one CHANGELOG bullet above "Filecoin.ChainNotify now closes the subscription channel ... instead of silently dropping head changes". go-jsonrpc's continue (websocket.go:320-323) is faithful parity, but for ChainNotify a lost apply/revert batch is exactly the failure #5795 set out to kill. A persistently unserializable payload also produces one tracing::error! per head change forever. Was skip-vs-close a deliberate choice for ChainNotify, or just parity copied from a different payload class? Also missing CHANGELOG entries for this flip, for 10 -> 16, and for the new slow-subscriber warning.

Cancel-after-server-close now errors. Previously the registry entry outlived the pump, so a client tearing down after xrpc.ch.close got a success. Now ChannelRegistration::drop removes it and the same cancel gets channel not found, codified by the new source_closed_sends_bare_close assertion. go-jsonrpc's cancelCtx (websocket.go:416-441) ignores an unknown id silently and never responds. Unconditional-cancel-on-teardown is a normal pattern, so this surfaces a new spurious error.

Low

- Displaced close races its own pump (channel.rs:512). insert drops the old unsubscribe receiver and a detached send_close runs concurrently with the old pump, whose tokio::select! picks randomly among ready arms. With queued broadcast messages, xrpc.ch.val for channel N can be written after xrpc.ch.close for N on the same sink; go-jsonrpc has already deleted the handler and logs handler N not found per stray frame. Send the close from the pump's unsubscribe arm instead.
- Double close window (channel.rs:504). Pump does send_close, break, debug!, then drops the registration. A same-id subscribe in that window displaces the stale entry and spawns a second send_close for the already-dead channel id.
- Forwarder task outlives the pump (chain.rs:1739). It parks on head_changes_rx.recv_async() and only notices the dead receiver on the next sender.send. On a stalled or header-syncing node, every connect/disconnect cycle accumulates a task plus a flume receiver the publisher must fan out to. Pre-existing, but the CHANGELOG says "no longer leak node resources".
- Duplicate mechanism (chain.rs:61). subscribe_head_changes_bounded exists for precisely "consumers driven by an untrusted RPC client's read rate" (chain_store.rs:349-355) and eth newHeads uses it (pubsub.rs:119). ChainNotify keeps the unbounded subscription and stacks a second bespoke scheme on top (16-slot broadcast, lag close, hand-rolled len() > 5 warn). Two things to keep in sync, and the bounded of the new warning.
- Permit fix untested (channel.rs:158). Methods::raw_json_request injects mock_subscription_permit() (jsonrpsee-core-0.26.0/src/server/rpc_module.rs:358), so nothing in the module asserts the max_subscriptions_per_connection claim. Only _permit plus a comment stand between a future cleanup and silently reverting it.
- PendingSubscriptionSink.id duplicates registration.request_id (lines 499 and 522 both clone the same Id). One source of truth would do.
- ChannelRegistration::drop does get then remove (two hashes; entry/Occupied does it in one) and drops the removed tuple while ke the subscribe path (504) and cancel handler (360) which both release first.
- v1.36.2 lotus link is likely dead (chain.rs:60). No such tag in the local clone (latest v1.36.1), and the repo pins v1.36.0 / release/v1.36.1 elsewhere. Content is right: at v1.36.1, store.go:295 is make(chan []*api.HeadChange, 16), warn at :329 with > 5 and byte-identical text.
- CHANGELOG: stray blank line inserted between the two ### Added bullets; five separate #5795 bullets could collapse.
- SLOW_SUBSCRIBER_BACKLOG doc: "forest" -> "Forest".

Style, your personal rules rather than repo rules, so raise only if you want to: the new comments include several 6-7 line narrative paragraphs that explain the alternative and the decision (lines 489-494, 505-511), the _permit rationale is duplicated verbatim at 156-158 and 232-234, and four added comment lines use em-dashes (78, 107, 739, 867).