[alecgibson]

Fixes #740
Fixes #748

At the moment, Agent._cleanup() only destroys streams that are still in each subscription path's own map, so any bookkeeping slip in one of those paths leaves a stream subscribed after the client disconnects.

This change tracks every stream handed to the agent in one map, and destroys them all on cleanup. So that presence streams can be replaced like doc streams, the p: null "client left" broadcast moves off the presence stream's 'end' and into _unsubscribePresence() and _cleanup(). A broadcast still in middleware at that point goes out as p: null instead.

That also fixes presence outliving its client when it was never subscribed, or still in flight, which receivePresence middleware now sees as p: null:

1. Client A submits presence on a channel it hasn't subscribed to
2. Client B, subscribed to the channel, receives it
3. Client A disconnects, and B keeps A's presence forever

🤖 Generated with Claude Code

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

[coveralls]

coverage: 97.99% (+0.03%) from 97.963% — destroy-every-agent-stream into main

[Copilot]

🟡 Changes recommended

In-flight presence broadcasts can survive cleanup, and synthetic disconnect messages include an invalid sequence value.

2 open findings

• Cancel pending presence broadcasts before unsubscribe completes · New
• Remove invalid seq increment from synthetic disconnect messages · New

What changed in this PR

Tracks Agent-owned streams for reliable cleanup and broadcasts null presence on unsubscribe or disconnect.

Changes:

• Adds centralized stream tracking and cleanup.
• Moves presence-disconnect broadcasts into explicit cleanup paths.
• Adds tests and documents null presence behavior.

File	Description
lib/​agent.js	Implements stream tracking and presence cleanup.
test/​agent.js	Tests cleanup of all stream types.
test/​client/​presence/​presence.js	Tests null presence broadcasts.
docs/​api/​presence.md	Documents unsubscribe behavior.
docs/​middleware/​actions.md	Documents null middleware presence.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Asynchronous ordering gaps can still publish non-null presence after unsubscribe or disconnect, and failed submissions retain empty channel maps.

2 open findings

• Presence request maps leak on rejected broadcasts · New
• Unsubscribe does not null presence without an active subscription · New

2 resolved since last review

• Cancel pending presence broadcasts before unsubscribe completes
• Remove invalid seq increment from synthetic disconnect messages

🧠 Review effort: Balanced

[alecgibson]

Migration note

Not breaking. This can ship as a 7.x 🐛 fix.

On main, receivePresence middleware already sees p: null leaves: when a client destroys its presence, and when a subscribed client unsubscribes or disconnects. This PR sends the same leave in two more cases:

• Presence submitted without subscribing, when its client disconnects. Before, remote clients kept it forever.
• Presence still in the doc-version lookup or middleware when its client unsubscribes or disconnects. It goes through receivePresence a second time, as the same context.presence object with p: null and any changes from its first pass kept. Before, it was published after the leave and never cleared (A presence submitted just before disconnect can outlive its client #748).

On main, middleware that reads context.presence.p without a null check already throws when a client leaves, and it will now also throw in these two cases. Handle the leave explicitly:

backend.use('receivePresence', function(context, next) {
  if (context.presence.p === null) return next();
  // ...
});

Two other changes that middleware with side effects (metering, auditing, "last known presence") may notice:

• A presence can pass receivePresence and still not be broadcast. If a newer presence with the same ID from the same client, or its leave, replaces it before it's published, its client gets an acknowledgement but nobody else receives it. Before, it was published, and could outlive the leave (A presence that finishes after a newer one can still outlive its client #763).
• backend.close() no longer sends leaves for clients that are still connected. On main those were a side effect of the presence streams ending: best-effort, and racing the same pubsub and DB shutdown. Leaves now follow the client connection closing.

[Copilot]

🟢 Approval recommended

The implementation addresses both reported lifecycle races with comprehensive regression coverage and consistent documentation.

0 open findings

2 resolved since last review

• Presence request maps leak on rejected broadcasts
• Unsubscribe does not null presence without an active subscription

🧠 Review effort: Balanced

[dawidreedsy]

I don't have comments, but my review panel has few comments :P

[dawidreedsy]

[🤖 Operability + Intent + Correctness · should] The pending-broadcast guard ends here, but the transform (getOps) and publish still run after it. A presence in that window that isn't the stored requests[id] can still publish live after the leave.

1. The client sends pv1 and pv2 for the same id without waiting for acks (local-presence.js:30-54).
2. pv1 finishes middleware second, through async middleware or two concurrent backend.subscribes in _subscribeDocVersion. Line 863 doesn't store it, because pv2 is newer.
3. The client disconnects while pv1 is in transformPresenceToLatestVersion. _retractPresences nulls only the stored pv2.
4. pv1 publishes its real p after the null. On receivers, the null already ran _removeRemotePresence (client/presence/presence.js:136-137), which dropped the pv high-water mark. pv1 lands on a fresh RemotePresence with presenceVersion 0, is accepted, and leaves a ghost.

The stored-request case is only safe because the transform shares the object and mutates it in place. #748 lists the transform as one of its windows, but none of the new tests holds a broadcast inside it.

Suggestion: keep the pending entry until just before pubsub.publish, removing it in a single exit wrapper around callback. Re-check agent.closed || pendingBroadcast.retracted at that point, then publish p: null or drop the stale one. To test it: make the presence stale (as in doc-presence.js:253), hold agent1's op middleware (fired via getOps → _sanitizeOp), unsubscribe inside it, release it, and assert the last receive is null.

Dissent (Operability): base had the same hole, and a wider one, so this narrows #748 rather than regressing it. It could be a follow-up.

[alecgibson]

Fixed, but with a publish-time check rather than keeping the pending entry: once the transform finishes, a presence that's no longer the stored presenceRequests[ch][id] (because a newer one from the same client, or its leave, has replaced it) is acknowledged but not published. That needs no extra bookkeeping, and it also covers #763, so the commit now closes that too. Leaves are exempt, because _cleanup() deletes the channel's map while one may still be in its transform.

A presence re-created with the same id starts again at pv 0, so any presence now replaces a stored leave whatever its pv. Otherwise the check above would drop it. That would also let an older presence still in flight replace its own client's later leave, which happens without any middleware, because a doc presence's first submit waits on a DB subscribe and its leave doesn't. So a leave from the client now flags its older pending broadcasts with the same id, and they're dropped as they leave middleware, releasing the doc version they subscribed to after the leave had already released it. _pendingPresenceBroadcasts is already in arrival order, so that needs no new bookkeeping. Async receive middleware can still reorder a presence and its own leave before either reaches this point. That's no worse than 7.0.0, and I've filed #767 for it.

Tests:

• "does not broadcast an older presence that finishes transforming after its client unsubscribes" (holds agent1's op middleware, as you suggested)
• "broadcasts a null presence still being transformed when the connection closes"
• "does not broadcast presence still looking up its doc version when it is destroyed"
• "does not broadcast presence that gets through middleware after its own leave"
• the two "re-created with the same id" tests

[dawidreedsy]

[🤖 Operability · should] When the agent is closed, errors from this rewritten p: null pass are dropped without a trace. That covers middleware rejecting the null, the doc-version unsubscribe, the transform and the publish.

callback goes to _reply and then send, which returns early once this.closed (lines 287-289). The other synthetic-leave path, _retractPresences, logs with logger.error (line 201). This path exists to prevent ghosts, so a silent failure here leaves a ghost with nothing logged.

Suggestion:

return agent._broadcastPresence(presence, function(error, result) {
  if (error) logger.error('Error broadcasting disconnect presence', presence.ch, presence.id, error);
  callback(error, result);
});

[alecgibson]

Done. Both synthetic leaves now go through _broadcastLeave(), which logs failures with the channel, presence id and client src. Test: "logs an error when the leave for presence in flight fails".

[dawidreedsy]

[🤖 Readability · should] This condition carries the PR's key new rule (an in-flight presence whose client has left goes out as a leave), but the rule has no name, and the recursion's stopping condition is hidden.

To follow it, a reader has to know three things:

• retracted is set far away, in _retractPresences.
• agent.closed is needed too, because _cleanup only retracts channels that already have a presenceRequests entry.
• The recursion stops only because leaving is recomputed as true on the second pass.

Suggestion:

var clientHasLeft = agent.closed || pendingBroadcast.retracted;
if (clientHasLeft && !leaving) {
  // Re-broadcast once as a leave: `leaving` is true on that pass, so it can't loop
  presence.p = null;
  return agent._broadcastPresence(presence, callback);
}

Alternatively, flag every pending broadcast in _cleanup, so this becomes pendingBroadcast.retracted && !leaving. Correctness checked that it behaves the same, because only leaving broadcasts can start once the agent is closed.

[alecgibson]

Named it clientHasLeft, and moved the rewrite into _broadcastLeave(). The !leaving beside it is what stops it recursing, the same way it does on the replaced check further down.

[dawidreedsy]

[🤖 Tests & conventions · should] No test checks that _trackStream drops a stream from _streams when the stream closes. A long-lived connection could build up a map of dead streams and CI would stay green.

Mutation test: deleting the stream.once('close', … delete streams[key]) listener still gives 902 passing, 0 failing. The new cleanup tests assert backend.pubsub.streamsCount, which drops on destroy whether or not the agent's map is pruned. No test reads agent._streams. A script that subscribes and unsubscribes shows it does drain today; the tests just don't pin that.

Suggestion: in the cleanup describe block, subscribe and unsubscribe a doc and a presence channel, and submit and then destroy a doc presence (doc-version stream). Then assert expect(connection.agent._streams).to.be.empty before closing. Existing tests already read agent internals such as presenceRequests.

[alecgibson]

Added "stops tracking streams once they close". It covers doc subscribe/unsubscribe, presence subscribe/unsubscribe, and a doc presence submit then destroy, and expects agent._streams to be empty. Deleting the 'close' listener now fails it.

[dawidreedsy]

[🤖 Intent + Compatibility + Readability · should] This sentence understates when the second pass happens and what it receives. The author's migration note says more than these docs do.

• When: the pending broadcast is registered at lib/agent.js:838, before the doc-version lookup at line 845. A leave during that lookup also triggers the second pass, not only a leave "while a presence is still in this middleware".
• What: it's the same presence object, already changed by the first pass, with p set to null. Middleware with side effects (metering, auditing, counting) will see one submitted presence twice.

This is a documentation caveat, not a semver break: base already ran p: null leaves through receivePresence.

Suggestion: "If its client leaves while a presence is still being processed (its doc version lookup or this middleware), the same presence object passes through this middleware a second time, with p set to null and any changes from the first pass kept. Middleware with side effects should expect this." Add the same line to the migration note.

[alecgibson]

Reworded. Any leave from an unsubscribe or disconnect reuses the client's latest presence object, not only one still in flight, so the docs now say that. They also say that a presence which passes this middleware may still not be broadcast if a newer one replaces it first. Both are in the migration note too.

[dawidreedsy]

[🤖 Readability · nit] This skip has no explanation. The old code re-broadcast every request, so a reader can't tell why null requests are now skipped: the presence has already left, and its null has already gone out.

Suggestion:

var alreadyLeft = request.p === null;
if (alreadyLeft) continue;

[alecgibson]

Done, as you suggested.

[dawidreedsy]

[🤖 Readability · nit] _streams and _nextStreamKey are the only new fields with no comment. _streams is also easy to confuse with this.stream (the client connection) and with the per-path maps above it, and a reader can't tell that it leaves out query emitters.

Suggestion:

// Every pub/sub stream this agent holds (doc, doc version and presence
// subscriptions). Streams remove themselves on 'close', and _cleanup()
// destroys the rest, even ones no longer in their own map
this._streams = Object.create(null);

[alecgibson]

Done. Every other field in that constructor documents its map, so these do too.

[dawidreedsy]

[🤖 Readability · nit] The PR removed the // Clean up doc subscription streams comment, and the two new loops have none, while the query loop below keeps its comment. So _cleanup is now half documented, and a reader comparing it with base will wonder whether the map resets were dropped on purpose.

Suggestion:

// Tell remote subscribers this client's presence has left
for (var channel in this.presenceRequests) { … }

// Destroy doc, doc version and presence streams. Their maps empty
// out through each stream's 'end' handler
for (var key in this._streams) { … }

[alecgibson]

I'd rather leave these out. They'd restate _retractPresences() and destroy(), and the resets are back, so there's nothing left for a reader comparing it with base to wonder about.

[dawidreedsy]

[🤖 Performance + Readability · praise] _trackStream follows the same counter-key plus once('close') self-removal pattern as PubSub#_createStream, so it reads as familiar. It also holds up under scrutiny:

• keys can't collide;
• destroying twice does nothing, because of OpStream's open guard;
• deleting inside _cleanup's for-in loop is defined behaviour;
• streams that arrive after close are destroyed before they are tracked.

The cost is about 150-200 B per stream, which is roughly 2 MB for a client holding 10k doc subscriptions.

[dawidreedsy]

[🤖 Intent · praise] This fixes #748's stale-pv problem for the doc-version lookup and middleware windows. The newer in-flight presence is never published live. It goes out as p: null with its own newer pv, so whichever null arrives last, a leave has the last word. leaving stops a second rewrite, and the "doesn't rewrite a null presence again" test pins that.

[alecgibson]

Heads-up from #752, which now also carries #749 (reject presence ahead of the doc) and is due to land first. This PR reworks the same presenceRequests lifecycle, so it seems the right place to settle this.

#749 caches a presence before its transform and rolls it back if the transform rejects it. If the client disconnects while that transform is still running, the leave never reaches subscribers:

1. Submit presence, so subscribers see it
2. Send presence with v: doc.version + 1, while its getOps is still pending
3. Disconnect, then let getOps finish
4. Subscribers never receive null for that presence ID, so they keep showing the earlier presence

The leave replay nulls the in-flight cached request in place, and #749 rejects that request as ahead of the doc. The rollback then writes the previous request into a map the 'end' handler has already deleted. Honest clients only send ahead-of-doc presence when the database reads from a replica that lags behind, so the window is narrow.

Repro, as a drop-in for test/client/presence/doc-presence.js (it passes with v: doc1.version - 1):

it('broadcasts null presence on disconnect while presence ahead of the doc is in its transform', function(done) {
  var localPresence1 = presence1.create('presence-1');
  var releaseGetOps;

  async.series([
    presence1.subscribe.bind(presence1),
    presence2.subscribe.bind(presence2),
    function(next) {
      localPresence1.submit({index: 1}, errorHandler(done));
      presence2.once('receive', function() {
        next();
      });
    },
    function(next) {
      var getOps = backend.db.getOps;
      sinon.stub(backend.db, 'getOps').callsFake(function() {
        var args = arguments;
        releaseGetOps = function() {
          getOps.apply(backend.db, args);
        };
        backend.db.getOps.restore();
        next();
      });

      var message = localPresence1._message();
      message.v = doc1.version + 1;
      message.t = presenceTestType.type.uri;
      connection1.send(message);
    },
    function(next) {
      presence2.once('receive', function(id, presence) {
        expect(id).to.equal('presence-1');
        expect(presence).to.be.null;
        next();
      });
      connection1.close();
      setTimeout(releaseGetOps, 20);
    }
  ], done);
});

[Copilot]

🟡 Changes recommended

Pending-presence cancellation can suppress valid updates when a leave is rejected or processed out of order.

2 open issues

• Rejected leave permanently cancels pending live updates · New
• Leave supersedes newer same-ID updates · New

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Failed transforms can suppress valid updates, and already-null presence requests can leave document-version streams subscribed after unsubscribe.

2 open issues

• Prevent failed presence transforms from suppressing valid older updates · New
• Release subscriptions when skipping duplicate null presence broadcasts · New

2 resolved since last review

• Leave supersedes newer same-ID updates
• Rejected leave permanently cancels pending live updates

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

The interactions between asynchronous middleware, presence transformations, and disconnect cleanup warrant final human validation despite the added regression coverage.

No open issues 🎉

2 resolved since last review

• Prevent failed presence transforms from suppressing valid older updates
• Release subscriptions when skipping duplicate null presence broadcasts

_{Review effort: Balanced}

[alecgibson]

A heads-up for rebasing this over #752, which a Copilot fix has since changed (c3be55e). When a newer presence for the same id is still in flight, or gets rejected, #752 publishes an older presence once its transform succeeds, and caches it so the disconnect leave goes out at its pv. Its four "keeps older accepted presence cached when a newer one is rejected" tests would fail under this branch's rules:

• replaced skips publishing as soon as a newer request is cached, before that request's transform. If the newer one is then rejected, subscribers never see the client's actual presence (Copilot's "Prevent failed presence transforms from suppressing valid older updates" here).
• _withdrawPresenceRequest leaves a p: null marker at the rejected pv. That marker outranks the accepted older presence, so no leave goes out for it on disconnect.

To keep what replaced does for #763, it could mean "a newer request for this id has already been accepted" rather than "is cached", and withdraw could roll back as #752 does instead of marking. While rebasing, folding _pendingPresenceBroadcasts and #752's _pendingPresenceTransforms into one record per in-flight request (along the lines of #765) would let a rejection work out its rollback target directly, instead of patching stored pointers.