websocket: Make unsubscribe helpers accept the request ID returned by subscribe

[kartsan03]

Problem

Every *_unsubscribe() helper is unusable with the value returned by its matching *_subscribe() on any real RPC endpoint.

Since #647, subscribe helpers return the client request ID used for the subscription request. But the server assigns subscription IDs independently of request IDs, and the unsubscribe helpers forward the given value verbatim as the subscription ID. On devnet today:

slot_subscribe() returned request id: 1
server confirmed subscription id 9710270 for request id 1

So await ws.slot_unsubscribe(await ws.slot_subscribe()) sends slotUnsubscribe(1), which the node rejects — verified against wss://api.devnet.solana.com with raw websockets frames:

slotUnsubscribe(request_id=1)      -> {"error": {"code": -32602, "message": "Invalid subscription id."}, "id": 2}
# notifications keep arriving - the subscription leaks

slotUnsubscribe(server_sub_id=9710270) -> {"result": true, "id": 3}
# stream stops

Through the client itself, the wrong ID also crashes local bookkeeping (del self.subscriptions[subscription]), since subscriptions is keyed by server-assigned IDs:

async with connect("wss://api.devnet.solana.com") as ws:
    sub = await ws.slot_subscribe()
    ...
    await ws.slot_unsubscribe(sub)
    # KeyError: 1  at websocket_api.py:351

A fresh local validator happens to assign small subscription IDs that coincide with request IDs, which is why this only surfaces on public endpoints.

Fix

Record the mapping from request ID to server-assigned subscription ID when a SubscriptionResult confirmation is processed, and resolve it in all nine unsubscribe helpers via _pop_server_subscription(). Resolution passes the value through unchanged when no confirmation has been seen, so callers that track server-assigned IDs themselves (as the integration tests do) behave exactly as before. Bookkeeping cleanup now uses pop(..., None) instead of del, so unsubscribing an unconfirmed or already-cancelled subscription no longer raises KeyError.

Verification

Against https://api.devnet.solana.com / wss://api.devnet.solana.com (Python 3.13):

Before this change:

$ python verify_ws_unsub.py
slot_subscribe() returned request id: 1
server confirmed subscription id 9710270 for request id 1
Traceback (most recent call last):
  ...
  File "src/solana/rpc/websocket_api.py", line 351, in slot_unsubscribe
    del self.subscriptions[subscription]
KeyError: 1

After this change:

$ python verify_ws_unsub.py
slot_subscribe() returned request id: 1
server confirmed subscription id 9710270 for request id 1
slot_unsubscribe(request_id) sent without error
unsubscribe ack from server: result=True
confirmed: no more notifications after unsubscribe

Local checks:

• ruff format --check src tests — clean
• ruff check src tests — clean
• mypy src — Success: no issues found in 31 source files
• pytest -m "not integration" --doctest-modules src tests/unit — 155 passed (includes two new unit tests covering the translation and the unconfirmed pass-through)

Notes

• Integration tests that extract server-assigned IDs from SubscriptionResult.result and pass them to the unsubscribe helpers continue to work unchanged (pass-through path).
• Unsubscribing before the server confirmation has been received still cannot cancel anything server-side; it now degrades to sending the given ID instead of raising KeyError.

[kingsznhone]

LGTM

[codecov]

Codecov Report

❌ Patch coverage is 97.56098% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.99%. Comparing base (62df6f2) to head (5f8c1fb).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #701      +/-   ##
==========================================
+ Coverage   94.82%   94.99%   +0.17%     
==========================================
  Files          25       25              
  Lines        1739     1739              
==========================================
+ Hits         1649     1652       +3     
+ Misses         90       87       -3     

🚀 New features to boost your workflow:

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

[michaelhly]

Thanks for the detailed report — the bug is real and the direction of the fix (record the request ID → server subscription ID mapping on the SubscriptionResult confirmation) is right.

My concern is how the two ID spaces are reconciled: both are plain int and the request-ID map is consulted first, so a caller passing a server ID that numerically collides with a live request ID gets silently mistranslated and cancels the wrong subscription. That's the path README and every integration fixture use, so the "behaves exactly as before" claim doesn't hold in general. Details inline.

I'd like the first comment resolved before merge; the next two are cheap to fix alongside it.

[michaelhly]

Request IDs and server subscription IDs collide here, cancelling the wrong subscription.

Both spaces are plain int and the request-ID map is consulted first, so the "pass through unchanged if unconfirmed" rule can't distinguish an unconfirmed request ID from a server-assigned ID that happens to equal a live request ID. Reproduced on a fresh protocol object (small server IDs, as a local validator assigns):

slot_subscribe()  -> req 1, server sub 1;  slot_unsubscribe(1) consumes req id 2
root_subscribe()  -> req 3, server sub 2      # map {3: 2}
vote_subscribe()  -> req 4, server sub 3      # map {3: 2, 4: 3}
vote_unsubscribe(3)   # 3 = server id of the vote sub, the README form
wire: {"method":"voteUnsubscribe","id":5,"params":[2]}   # cancels the ROOT subscription
remaining: {3: VoteSubscribe...}                          # vote sub leaks

The PR description says callers that track server-assigned IDs "behave exactly as before"; that only holds while no live request ID equals the server ID being passed. README.md:94/:105 and every integration fixture use the server-ID form, so this is a real backward-compat break rather than a theoretical one.

Suggestion: disambiguate instead of guessing. Checking self.subscriptions first keeps the older contract authoritative:

if subscription in self.subscriptions:          # already a server-assigned ID
    self.request_ids_to_subscriptions = {k: v for k, v in self.request_ids_to_subscriptions.items() if v != subscription}
    self.subscriptions.pop(subscription, None)
    return subscription

Or, better long term, expose the mapping through a distinct return type/API so the two ID spaces are never interchangeable ints.

[michaelhly]

Minor: swapping del for pop(..., None) removes the only local signal that the unsubscribe target isn't a live subscription. Unsubscribing before confirmation (or twice) now silently sends an invalid subscription ID; the server's -32602 comes back as a SoldersSubscriptionError and is raised as SubscriptionError out of an unrelated later recv(), while the real subscription keeps streaming. Dropping the KeyError is the right call, but a returned bool or a logging.warning would let the caller learn the unsubscribe was a no-op at the call site.

[michaelhly]

Local bookkeeping is now torn down before the request is sent.

_pop_server_subscription(...) is evaluated while building the request object, so the mapping and the self.subscriptions entry are gone before await self.send_request(req) runs. Previously the del ran only after a successful send.

If send_request raises (e.g. ConnectionClosed), a reconnect-and-resubscribe routine reading self.subscriptions sees no record of the subscription, and a retried slot_unsubscribe(req_id) now falls through to the pass-through path and sends the raw request ID as the subscription ID. Please resolve the ID, send, then clean up on success.

Same pattern at lines 192, 238, 294, 325, 345, 365, 385 and 405.

[michaelhly]

These docstrings (all nine) now document the request-ID form, but README.md:94 and the docs page mirroring it still pass first_resp[0].result — the server-assigned ID — and so do all the integration fixtures. Since the two forms are silently interchangeable and can collide, the public contract ends up documented one way and exercised another. Worth updating README/docs to whichever form is authoritative.

[kartsan03]

Thanks — the collision is real, and I overstated the pass-through compatibility.

I'll treat self.subscriptions as authoritative (server ID first, as you suggested), then fall back to the request-ID map. I'll also:

• drop map entries when unsubscribing by server ID (and on the other leak paths)
• resolve the ID, send, then clean up only after a successful send
• restore a call-site signal for unknown/unconfirmed IDs (warning or bool)
• align README/docs with whichever form is canonical

Pushing a follow-up shortly.

[kartsan03]

Pushed in b8a3e79:

• self.subscriptions is checked first, so a server-assigned ID is never rewritten through the request-ID map (covers the voteUnsubscribe(3) collision).
• Map entries are dropped on helper unsubscribe, raw send_request unsubscribes, and after a signatureNotification.
• Resolve → send → forget, so a failed send leaves local bookkeeping intact.
• Unknown/unconfirmed IDs still pass through, with a logging.warning at the call site.
• README now documents both forms.

The original unit tests plus the collision / leak / failed-send cases are green (pytest -m "not integration": 160 passed).

[kingsznhone]

I am attempting to propose a plan here.

The request_id is only needed to increment at local. Once the response is received, it can be discarded.

Active subscriptions can be stored as a new strong type in a simple list[Subscription], avoiding any long-lived mapping between request IDs and subscription IDs.

The general idea is something like this:

@dataclass
class Subscription:
    subscription_id: int
    unsubscribe_method: str


subscriptions: list[Subscription] = []


# Subscribe
request_id = send_subscribe_request(...)
response = await wait_for_response(request_id)

sub = Subscription(
    subscription_id=response.result,
    unsubscribe_method="logsUnsubscribe",
)

subscriptions.append(sub)

# Unsubscribe
subscriptions.remove(sub)
send_request(sub.unsubscribe_method, [sub.subscription_id])

Key points

• request_id is temporary and discarded after the subscribe response is received.
• subscription_id is the persistent identity of the subscription.
• Store active subscriptions as a simple list[Subscription].
• Each Subscription contains its server-assigned ID and unsubscribe method.
• No request_id → subscription_id mapping is required.
• This completely avoids request ID / subscription ID collisions.

[kartsan03]

@kingsznhone agreed that a distinct Subscription type is the clean long-term shape — it matches what Michael already flagged as the better API.

I'd keep that out of this PR though. Today *_subscribe() is fire-and-forget and returns the request id immediately; callers recv() the confirmation separately (README + every integration fixture). Waiting for the subscribe response and returning a Subscription object would be a breaking API change, not a bugfix.

This PR is the compatibility shim: keep accepting both ints, treat self.subscriptions as the server-id space, and drop the request-id map after a successful unsubscribe. Happy to follow up with a typed handle in a separate PR if that's the direction you want.

[michaelhly]

Thanks!

[michaelhly]

CI is green across the matrix on a3dabda, including the full integration suite against a live validator on ubuntu — that exercises the server-ID contract (first_resp[0].result plus the raw batched send_request unsubscribes) end to end, so the backward-compat story holds. All the review points are addressed; this is good to merge as-is.

Recording the residual caveats so they're findable later. None of these block this PR:

1. The int overload stays ambiguous in one direction. self.subscriptions wins, so a request ID that happens to equal a live server subscription ID resolves to the server subscription and cancels the wrong feed. The priority order is the right trade — it keeps the documented form authoritative and confines the ambiguity to the new convenience form — but it can't be eliminated while both spaces are bare ints. This is @kingsznhone's Subscription-type point, and the main reason I'd like to see that follow-up.

2. Unsubscribing before the confirmation still can't cancel anything server-side. Logging a warning and passing the ID through is the right degradation from KeyError, but the subscription keeps streaming.

3. request_ids_to_subscriptions still grows for subscriptions that are never unsubscribed — the same shape as the existing subscriptions / sent_subscriptions growth, so not a regression here, but the typed-handle refactor should clean up all three together.

4. hasattr(req, "subscription_id") in send_request is a structural check. solders exposes no shared base class and no unsubscribe-only union, so an isinstance against a tuple of the nine already-imported classes is the only nominal alternative; the upside is mypy narrowing subscription_id to int rather than Any | int. Style only — fine either way.

Doc nit for whoever picks up the follow-up: the README now says both forms are accepted, but given (1) the server-assigned ID is the canonical one and the request ID is the convenience. Worth stating that explicitly.

Thanks @kartsan03 for the repro and the quick turnarounds.

[kartsan03]

Thanks for the careful review on the first pass — the collision catch was the right one, and the notes made the follow-up straightforward. Glad this landed.