Skip to content

🐛 Broadcast a leave before presence re-created with its ID - #772

Draft
alecgibson wants to merge 1 commit into
mainfrom
presence-leave-order
Draft

alecgibson wants to merge 1 commit into
mainfrom
presence-leave-order

Conversation

@alecgibson

Copy link
Copy Markdown
Collaborator

Refs #771

At the moment, a presence re-created with the ID of one that's leaving can be transformed and published before the leave:

  1. Destroy a presence, and hold its leave's transform in op middleware
  2. Re-create it with the same ID, at the doc's version
  3. Release the leave: subscribers drop the re-created presence, since its pv restarted, and then clear it with the leave

This change has any presence that gets through middleware wait until the newest leave of its ID has been handed to pubsub or rejected, so the leave goes out first. Each leave waits only for the one before it, and releases its waiters on the next tick, so a long run of leaves for one ID drains in order without recursing. Since presence can now sit waiting, a disconnect's leave is a copy, rather than the waiting presence turned into a second leave.

Waiters are released once the leave's publish() is called, not when it calls back, so we rely on pubsub delivering in call order, as both the memory and Redis adapters do. That way an adapter that never calls back can't hold up the ID.

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

Refs #771

At the moment, a presence re-created with the ID of one that's leaving
can be transformed and published before the leave:

 1. Destroy a presence, and hold its leave's transform in `op`
    middleware
 2. Re-create it with the same ID, at the doc's version
 3. Release the leave: subscribers drop the re-created presence, since
    its `pv` restarted, and then clear it with the leave

This change has any presence that gets through middleware wait until
the newest leave of its ID has been handed to pubsub or rejected, so
the leave goes out first. Each leave waits only for the one before it,
and releases its waiters on the next tick, so a long run of leaves for
one ID drains in order without recursing. Since presence can now sit
waiting, a disconnect's leave is a copy, rather than the waiting
presence turned into a second leave.

Waiters are released once the leave's `publish()` is called, not when
it calls back, so we rely on pubsub delivering in call order, as both
the [memory][1] and [Redis][2] adapters do. That way an adapter that
never calls back can't hold up the ID.

[1]: https://github.com/share/sharedb/blob/2aa0345e6865b20aa743ab107c86c3bd210160c2/lib/pubsub/memory.js#L28-L39
[2]: https://github.com/share/sharedb-redis-pubsub/blob/4909f88651d2d91086857218ea2f94fde107b156/index.js#L79-L85

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 98.067% (+0.01%) from 98.057% — presence-leave-order into main

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

A critical queue-handling bug can acknowledge a valid presence update without publishing it when a newer queued update fails transformation, leaving subscribers without replacement presence.

1 open issue
What changed in this PR

This PR addresses part of #771 by ordering server-side presence broadcasts so a re-created presence waits for an earlier leave with the same ID.

Changes:

  • Queue same-ID broadcasts until the preceding leave is handed to pubsub or rejected, releasing waiters without recursion.
  • Shallow-copy disconnect leaves to avoid mutating pending presence updates.
  • Add regression tests for ordering, failures, disconnects, and long leave queues.
File Description
test/​client/​presence/​doc-presence.js Adds helpers and regression cases for queued presence broadcasts.
lib/​agent.js Tracks in-flight leaves, orders broadcasts, and copies disconnect leaves.
docs/​middleware/​actions.md Documents shallow-copy behavior for disconnect and unsubscribe leaves.

Review effort: Balanced

Comment thread lib/agent.js Resolved

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues are recorded for the presence-ordering fix.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants