Skip to content

fix(protocol): peer loss and shutdown order sessions by registration, not creation time - #4904

Open
Apollon77 wants to merge 1 commit into
fix/subscription-report-sessionfrom
fix/session-registration-order
Open

Apollon77 wants to merge 1 commit into
fix/subscription-report-sessionfrom
fix/session-registration-order

Conversation

@Apollon77

Copy link
Copy Markdown
Collaborator

When a peer is lost or shut down, SessionManager closes the peer's older sessions and keeps the newer ones, because the newer ones carry the peer's data after the event. Until now, "newer" meant a Session.createdAt at or after a given instant (PeerLossContext.asOf). That rule has two weak points:

  • Two sessions can have the same creation time. A session from before the event then counts as newer and stays open. Under MockTime this happens in every test that does not advance the clock. On real clocks it needs a coarse clock, such as performance.now() in browsers, which is rounded to 100 µs or more, or the Time.nowMs fallback.
  • A session we initiate is created before its handshake but registered only when the handshake completes. If the handshake completes after an exchange on the peer started, and that exchange then reports the peer lost, the new session is closed: its creation time is before the start of the exchange.

The reboot detection (RebootResubscribeArmer) already uses registration order for its evidence since #4882, but its close of the older sessions still used createdAt. The two rules can disagree: with equal creation times, a pre-reboot session no longer counts as evidence but stays open, and maybeSessionFor() may pick it.

Design

SessionManager gives each session a registration position when it joins registeredSessions. Peer loss and peer shutdown compare positions instead of times. The three callers that pass a position use one rule:

  • ExchangeManager takes registrationCount when an exchange starts. A peer loss reported by that exchange keeps the sessions registered after the exchange started.
  • RebootResubscribeArmer passes the returning session to handlePeerShutdown() and builds its evidence set with sessionsRegisteredSince() from the same positions, so evidence and close agree.
  • The StartUp event handling in Peers passes the current session to handlePeerShutdown().

Without a position, peer loss and peer shutdown affect every session registered when they are called. A session that registers while the peer loss runs is not affected. Before, such a session was affected if it was created before the call.

PeerLossContext.registeredBefore is a new field next to asOf, which stays and is deprecated. handlePeerShutdown() takes a session or, deprecated, a timestamp. When asOf is given, both rules apply. This is internal API, so no break is marked.

This PR builds on #4882.

Verification

  • SessionManager tests: peer shutdown keeps the given session and the sessions registered after it, also with equal creation times; peer loss keeps a session that registers after the mark although it was created before; a session registered while the peer loss runs is kept; the deprecated timestamp still works; a session that never registered keeps nothing; sessionsRegisteredSince() returns the sessions registered at or after a session, including one created earlier that registered last.
  • ExchangeManager test: a session of the peer registered after the failing exchange started is kept, with equal creation times. The helper no longer advances the clock to separate the sessions.
  • RebootResubscribeArmer test: an older session created in the same instant as the returning session is closed.
  • Peers test: on StartUp, a session created after the current session but registered before it is closed.
  • Mutants, each followed by a failing test: no position filter in the peer loss, the position of the kept session ignored, the armer passing createdAt, the sessionsRegisteredSince() filter removed, <= instead of <, the exchange mark dropped, no default position, the deprecated timestamp ignored, Peers passing createdAt, a session that never registered keeping every session or returning every session from sessionsRegisteredSince(). Two mutants survive and cannot be observed with the current callers: setting the position right after super.add() instead of before it, because no observer of registeredSessions.added reads positions; and no default position on the deprecated timestamp path, because no caller uses that path.
  • npm run build-clean, npm run format, npm run lint and npm test pass.

🤖 Generated with Claude Code

… not creation time

Two sessions can have the same creation time, so a session from before a
peer loss or shutdown could count as newer and stay open. A session we
initiate registers only after its handshake, so its creation time is before
an exchange that started while the handshake ran.

SessionManager now gives each session a registration position. Peer loss
and peer shutdown affect only sessions registered before
PeerLossContext.registeredBefore, by default every session registered at
call time. ExchangeManager, RebootResubscribeArmer and the StartUp handling
in Peers use this order. PeerLossContext.asOf and the Timestamp argument of
handlePeerShutdown() stay, deprecated.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings November 4, 2026 07:49
@mergify

mergify Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 remain for the registration-ordered session cleanup changes.

No open issues 🎉

What changed in this PR

This PR makes peer-loss and shutdown cleanup use session registration order rather than creation time. Building on #4882, it aligns reboot evidence with session cleanup and preserves newer sessions even when timestamps match or handshakes complete out of creation order.

Changes:

  • Adds registration positions and cutoffs while retaining deprecated timestamp support.
  • Updates exchange failures, reboot detection, and StartUp handling to use registration order.
  • Adds regression coverage for equal timestamps, delayed registration, and registration during cleanup.

Verification: The PR reports successful clean build, formatting, lint, and tests; these were not rerun here.

File Description
packages/​protocol/​test/​session/​SessionManagerTest.ts Tests registration cutoffs, concurrent registration, and timestamp compatibility.
packages/​protocol/​test/​protocol/​ExchangeManagerTest.ts Tests preservation of sessions registered after an exchange starts.
packages/​protocol/​test/​peer/​RebootResubscribeArmerTest.ts Tests cleanup of older sessions with equal creation times.
packages/​protocol/​src/​session/​SessionManager.ts Tracks registration positions and applies cleanup cutoffs.
packages/​protocol/​src/​session/​Session.ts Removes the creation-time documentation link to peer-loss cutoffs.
packages/​protocol/​src/​protocol/​ExchangeManager.ts Captures a registration cutoff for exchange-triggered peer loss.
packages/​protocol/​src/​peer/​RebootResubscribeArmer.ts Aligns reboot evidence and cleanup by registration order.
packages/​protocol/​src/​peer/​PeerLossContext.ts Adds registeredBefore and deprecates asOf.
packages/​node/​test/​node/​SessionViewsTest.ts Removes clock advancement previously needed for peer-loss cleanup.
packages/​node/​test/​node/​ClientStartUpTest.ts Tests StartUp cleanup when creation and registration order differ.
packages/​node/​test/​node/​client/​PeersInstrumentBasicInformationTest.ts Updates a comment about preserving the live session.
packages/​node/​test/​behaviors/​ota/​OtaTest.ts Checks reboot cleanup using session identity instead of timestamps.
packages/​node/​src/​node/​client/​Peers.ts Passes the current session to StartUp cleanup.
packages/​general/​src/​util/​Set.ts Removes the insertion-order statement from documentation.
CHANGELOG.md Records registration-based cleanup and timestamp deprecation.

Review effort: Balanced

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.

2 participants