Repository navigation
Conversation
… 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>
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
Contributor
There was a problem hiding this comment.
🟢 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a peer is lost or shut down,
SessionManagercloses 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 aSession.createdAtat or after a given instant (PeerLossContext.asOf). That rule has two weak points:performance.now()in browsers, which is rounded to 100 µs or more, or theTime.nowMsfallback.The reboot detection (
RebootResubscribeArmer) already uses registration order for its evidence since #4882, but its close of the older sessions still usedcreatedAt. The two rules can disagree: with equal creation times, a pre-reboot session no longer counts as evidence but stays open, andmaybeSessionFor()may pick it.Design
SessionManagergives each session a registration position when it joinsregisteredSessions. Peer loss and peer shutdown compare positions instead of times. The three callers that pass a position use one rule:ExchangeManagertakesregistrationCountwhen an exchange starts. A peer loss reported by that exchange keeps the sessions registered after the exchange started.RebootResubscribeArmerpasses the returning session tohandlePeerShutdown()and builds its evidence set withsessionsRegisteredSince()from the same positions, so evidence and close agree.Peerspasses the current session tohandlePeerShutdown().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.registeredBeforeis a new field next toasOf, which stays and is deprecated.handlePeerShutdown()takes a session or, deprecated, a timestamp. WhenasOfis given, both rules apply. This is internal API, so no break is marked.This PR builds on #4882.
Verification
SessionManagertests: 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.ExchangeManagertest: 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.RebootResubscribeArmertest: an older session created in the same instant as the returning session is closed.Peerstest: on StartUp, a session created after the current session but registered before it is closed.createdAt, thesessionsRegisteredSince()filter removed,<=instead of<, the exchange mark dropped, no default position, the deprecated timestamp ignored,PeerspassingcreatedAt, a session that never registered keeping every session or returning every session fromsessionsRegisteredSince(). Two mutants survive and cannot be observed with the current callers: setting the position right aftersuper.add()instead of before it, because no observer ofregisteredSessions.addedreads 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 lintandnpm testpass.🤖 Generated with Claude Code