An Abandoned sub keeps its entry but reports failure, so a retrying caller gets two entries for one stream

Open
  • Is something wrong?
#307 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
48/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Quiet
Tech stack
typescript
Domain
api

Research direction

Start by tracing subscribe's AbandonedRequest catch, the subscriptions bookkeeping, and subscribeAll's reconnect path; compare their behavior with ADR-0006. Reproduce an abandoned sub followed by a retry, then verify the chosen behavior leaves one retained entry and one live DDP subscription after reconnect, with unsubscribe affecting the expected entry.

Written by the indexing model from the issue text.

Description

What

subscribe writes the subscription entry for an Abandoned sub and then tells the caller the request failed:

.catch((err) => {
  this.logger.error(`[ddp] Subscribe error: ${err.message}`)
  if (err instanceof AbandonedRequest) {
    this.rememberSubscription(err.id, name, params, callback)
  }
  return undefined
})

The entry survives, which is what ADR-0006 intends. But the caller receives undefined, which is indistinguishable from the failure modes that leave nothing behind. A caller that responds the obvious way — by subscribing again — gets a second entry for the same stream under a new id, because subscribe mints a fresh id when none is passed.

Both entries then live in subscriptions, and subscribeAll establishes both at the next Login. The result is two live DDP subscriptions on one stream where the app asked for one: duplicate events delivered to the callback, and an unsubscribe that only ends one of them.

Steps to reproduce

  • Subscribe to a stream and let the sub frame be written.
  • End the connection before the DDP response arrives, so the wait is abandoned and the entry is kept.
  • Observe subscribe resolves undefined.
  • Subscribe to the same stream and params again, as a caller seeing a failure would.
  • Both entries are present in subscriptions under different ids.
  • Reconnect. subscribeAll establishes both.

Worth deciding

Whether the caller should be able to tell "kept, will be re-established" apart from "nothing left behind" — or whether subscribe should recognise an existing entry for the same name and params instead of adding a second. ADR-0006 states that what a caller receives is unchanged and the bookkeeping is its only concern, so the second option may fit the decision better than changing the return.

Why now

This is reachable on its own, but a send on a Socket that is Transport open while its Liveness chain has lapsed now writes the frame instead of failing before the write, which makes an abandoned-after-write sub considerably more common.

Dominant language
TypeScript
Stars
146
Forks
101
No merged PRs in 30d

Getting set up

This project ships no dev container, Dockerfile or contributing guide, so setting up is up to you: start from its README, and see our first-contribution guide for the general steps.

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from RocketChat/Rocket.Chat.js.SDK

All issues in RocketChat/Rocket.Chat.js.SDK

Similar issues

More TypeScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.