Skip to content

Presence re-created with a leaving presence's ID can still be lost #771

Open

Description

@alecgibson

#772 makes a presence wait behind a leave with the same ID that's already being transformed, so the leave is published first. Reviewing it turned up a few related gaps it doesn't cover:

  1. A re-created presence that gets through receivePresence before its leave is still lost. It's acked but never published, and the leave then clears it on subscribers. For example, for presence without a doc, _subscribeDocVersion calls back synchronously but _unsubscribeDocVersion waits a tick, so destroy() then create(sameId).submit() in one tick hits this. Async receivePresence middleware that finishes out of order does too. This happens on main as well. Simply waiting before the cache decision would break the rejected-leave rollback: a re-created presence that isn't cached yet when its leave is rejected would have the old presence restored over it.
  2. The root cause is that pv resets to 0 on re-creation, so subscribers treat the new presence as stale until they've applied the leave, and every async step between publisher and subscriber has to keep the order for each ID. sendPresence middleware runs separately for each subscriber, so an async check there can still reorder a leave and its re-creation. A pv that never goes down for an ID would fix this at the source. The Agent could offset a re-creation's pv past the last leave it published for that ID, or the client could carry pv across LocalPresence instances. Two more cases bear on that choice:
    • Even when the leave arrives first, a subscriber's RemoteDocPresence holds it as _pending until whenNothingPending applies it, at least a tick later. A re-creation arriving in that window fails message.pv < this._pending.pv and is dropped.
    • 🐛 Broadcast a leave before presence re-created with its ID #772's waiters only cover one Agent. If a client destroys a presence and re-creates it on a new connection while the old Agent is still transforming the leave, or before the server notices the old connection dropped and retracts it, the new Agent publishes the re-creation with nothing to wait for. An Agent-side offset wouldn't cover that, since the new Agent never saw the leave; carrying pv over on the client would.
  3. create(sameId) while destroy() is in flight sends the leave's ack to the wrong presence. create replaces localPresences[id] straight away, and the ack is routed by ID, so the old instance's destroy callback never fires, and if the new instance has reached the same pv, its callback fires instead. Either this needs a client fix, or the docs should say to wait for destroy's callback.
  4. Pubsub adapters must deliver one server's publishes on one channel in call order, which 🐛 Broadcast a leave before presence re-created with its ID #772 now relies on: it releases a waiting presence once the leave's publish() has been called. MemoryPubSub and sharedb-redis-pubsub both deliver in call order, but test/pubsub.js, which sharedb-redis-pubsub also runs in its CI, doesn't check it. Only 🐛 Broadcast a leave before presence re-created with its ID #772's waiters need this, and fixing (2) removes them, so this is only worth adding as an adapter-agnostic case there if (2) doesn't land. It shouldn't go into the adapter docs as a permanent contract.

Activity

  1. alecgibson commented on Oct 30, 2026

    @alecgibson
    CollaboratorAuthor

    One more for whoever picks up point 2: once a presence's pv never goes down for an ID, most of the leave handling #772 adds to lib/agent.js can go again.

    • _leavesInFlight, and its _addLeaveInFlight, _afterLeaveHandedOver and _handOverLeaveInFlight helpers, only exist to hold a re-created presence back until its leave has been published. With a higher pv, subscribers would just drop the stale leave if it arrived second, since remote presence already ignores anything below the pv it has seen.
    • The previousRequest.p === null check in _broadcastPresence only exists because a re-creation's pv can't be compared with its old incarnation's leave. A plain pv comparison would do.

    It's worth deleting those in the same change, rather than leaving them to guard against something that can no longer happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions