Skip to content

fix(protocol): apply the peer's MRP margin to inbound exchanges - #4166

Open
Apollon77 wants to merge 5 commits into
mainfrom
fix/mrp-margin-inbound-exchanges
Open

fix(protocol): apply the peer's MRP margin to inbound exchanges#4166
Apollon77 wants to merge 5 commits into
mainfrom
fix/mrp-margin-inbound-exchanges

Conversation

@Apollon77

@Apollon77 Apollon77 commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Problem

The medium-specific MRP retransmission margin (NetworkProfile.additionalMrpDelay — 1.5s for thread, 1s for WiFi) was a per-exchange constructor option, and PeerExchangeProvider was its only populator. Exchanges the peer initiates go through MessageExchange.fromInitialMessage(), which ExchangeManager calls with no options at all, so #sendAdditionalDelay collapsed to localAdditionalMrpDelay — the local node's own medium.

On a WiFi/ethernet controller talking to a Thread device that is 0 instead of 1.5s, and it affects all three timing paths that read the margin:

  • MRP retransmission backoff (the margin joins the base interval, so it is amplified by the backoff multiplier)
  • nextMessage() response timeout — too tight, so a slow Thread hop yields a premature PeerMessageMissingError
  • the exchange close/drain timer

Our responder-side messages do use MRP: requiresAck defaults true on MRP sessions. So the StatusResponse to a subscription data report, BDX BlockQuery, and ICD check-in responses all retransmitted on unpadded timing.

Fix

The margin now originates from the peer end of the session, so every exchange inherits it regardless of creation path. Peer installs a lazy resolver on each of its sessions — a resolver rather than a value because the medium is often only known (or changes, e.g. a new thread channel) after the session exists.

An explicit per-exchange option still wins. That matters: CASE establishment runs on an unsecured NO_FABRIC session which never gets a resolver, so PeerExchangeProvider's explicit margin remains the only thing padding Sigma retransmits.

The resolver pads only when the peer's medium is actually known. The conservative unknown fallback is deliberately not applied to inbound exchanges — it would inflate responder timing on nodes that never characterize their peers (controllers, aggregator-only nodes). Device nodes are unaffected either way: their auto-detected unknown profile already equals their own ownNetworkProfileId.

Also: a PeerSet adoption hole

Both of PeerSet's sessions.added observers only saw future sessions, so a session established before PeerSet was instantiated was never associated with a peer — leaving peer.sessions empty, so no newestSession(), no disconnected event, and no MRP margin.

This is reachable: in ServerNetworkRuntime.start() the first unconditional env.get(PeerSet) comes after addTransports() (already listening) and after env.get(SecureChannelProtocol) (CASE handler installed), with awaits in between. An inbound Sigma1 can land in that window.

The two observers are merged into one #adoptSession() path. They had disagreed on whether closed sessions qualify — the second lacked the isClosed check, so it could add an already-closed session to peer.sessions where the closing hook has already fired, leaving a permanent stale entry that newestSession() can return. The merged guard is the stricter one, registration goes through the observer group so it unregisters on close, and existing sessions are seeded at construction.

Tests

  • MessageExchangeTest: peer-initiated exchange inherits the session margin; initiated exchange without an explicit margin inherits it; explicit margin wins over the session's
  • ClientTuningTest: a WiFi peer's 1s margin reaches the session; a PeerSet constructed after commissioning adopts the pre-existing session

Clean build, format-verify, lint and the full suite pass.

🤖 Generated with Claude Code

Checklist

  • Tests added or updated to cover the change
  • npm test passes (full suite, all packages)
  • npm run format-verify and npm run lint pass
  • npm run build -- --clean passes
  • CHANGELOG updated

No log file attached: this is a timing-correctness fix derived from the code paths, not from a reported failure.

The medium-specific MRP retransmission margin was a per-exchange constructor
option populated only by PeerExchangeProvider, so peer-initiated exchanges
(subscription data reports, BDX transfers, ICD check-ins) fell back to the local
node's own margin. On a WiFi/ethernet controller talking to a Thread device that
means no pad at all: responder retransmissions fire too eagerly and
MessageExchange.nextMessage() times out before a slow Thread hop can answer.

The margin now originates from the peer end of the session, so every exchange
inherits it regardless of creation path. Peer installs a lazy resolver on each of
its sessions because the medium often becomes known only after the session
exists. An explicit per-exchange option still wins, which keeps CASE
establishment padded - that runs on an unsecured NO_FABRIC session, which has no
resolver.

The resolver pads only when the peer's medium is actually known. The conservative
"unknown" fallback is deliberately not applied to inbound exchanges because it
would inflate responder timing on nodes that never characterize their peers.

Also closes a hole in PeerSet: both session observers only saw future sessions,
so a session established before PeerSet was instantiated was never associated
with a peer. ServerNetworkRuntime.start() first touches PeerSet after transports
listen and SecureChannelProtocol is installed, so an inbound Sigma1 can land in
that window. The observers are merged into one adoption path with a single guard
(the previous two disagreed on whether closed sessions qualify), registered
through the observer group so it unregisters on close, and existing sessions are
seeded at construction.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 29, 2026 16:21

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.

Pull request overview

This PR fixes MRP timing on responder/inbound exchanges by sourcing the medium-specific retransmission margin from the peer side of the session (rather than only from per-exchange construction options), ensuring peer-initiated exchanges and other exchanges created without peer context get correct padding.

Changes:

  • Add a lazy Session.peerAdditionalMrpDelay resolver and have MessageExchange fall back to it when no per-exchange peerAdditionalMrpDelay is provided.
  • Fix a PeerSet session-adoption gap by seeding and adopting pre-existing operational sessions (and centralizing adoption logic with stricter closed-session guards).
  • Add protocol/node tests covering session margin inheritance and late PeerSet construction, and document the fix in the changelog.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/protocol/test/protocol/MessageExchangeTest.ts Adds coverage for peer-initiated and initiated exchanges inheriting the session’s peer MRP margin (and precedence of explicit per-exchange margin).
packages/protocol/src/session/Session.ts Introduces a lazy resolver-backed peerAdditionalMrpDelay on sessions to represent peer-medium timing margin.
packages/protocol/src/protocol/MessageExchange.ts Uses per-exchange peer margin if provided, otherwise falls back to session.peerAdditionalMrpDelay for send timing.
packages/protocol/src/protocol/ExchangeProvider.ts Removes now-stale comment about missing peer/medium context.
packages/protocol/src/peer/PeerSet.ts Adopts pre-existing sessions at construction and consolidates session adoption through a single guarded path.
packages/protocol/src/peer/Peer.ts Installs a per-session resolver to expose the peer’s medium-specific MRP margin when the peer medium is known.
packages/node/test/node/ClientTuningTest.ts Adds integration tests ensuring the peer margin reaches the session and late PeerSet construction adopts existing sessions.
CHANGELOG.md Adds a WIP changelog entry describing the behavioral fix in @matter/protocol.

@mergify

mergify Bot commented Jul 29, 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

Apollon77 and others added 2 commits July 30, 2026 12:27
BDX exchanges bypass both the local and the peer's medium-specific
retransmission margin in favor of a fixed value, in either direction.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Network profiles now carry bdxAdditionalMrpDelay beside additionalMrpDelay.
BDX sustains many round trips over one path, so it warrants a margin
independent of normal messaging. It defaults to the profile's messaging margin,
so a profile that does not distinguish traffic classes behaves as before, and
connect/probeAddress sub-profiles inherit both. Thread sets it explicitly to
zero.

MRP.marginFor() is the single place that maps a protocol to its traffic class.
Session carries the peer's margins as an MRP.Margins pair, installed by Peer,
so both directions and every exchange creation path select the right one.

This replaces the previous fixed BDX margin, which ignored the medium
entirely.

Also exposes the whole of PeerTimingParameters and the new profile field
through NetworkServer, and adds a compile-time guard so a source type that
grows a field breaks the build until the config class declares it. The guard
found six timing parameters that were silently unconfigurable: kick throttle,
retransmission and restart-saving thresholds, restart cooldowns, and the
address-change stabilization delay and probe cooldowns.

Exposing the two grouped parameters required fixing their merge:
PeerTimingParameters merged only top-level keys, so overriding one member of
kickRestartCooldown or addressChangeProbeCooldown dropped its siblings to
undefined despite the type declaring them required. Groups now merge
field-wise, and the config types widen from Partial to DeepPartial.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

packages/protocol/src/protocol/MessageExchange.ts:1120

  • JSDoc references Session.peerAdditionalMrpDelay, but the session-level margin is now exposed via Session.peerMrpMargins. The current link appears stale and could mislead API consumers.
         * Overrides {@link Session.peerAdditionalMrpDelay}, which supplies the margin for exchanges created without
         * this option (notably peer-initiated ones).

packages/node/src/behavior/system/network/NetworkServer.ts:122

  • TimingConfig still exposes minimumTimeBetweenMrpKicks, but that key no longer exists in PeerTimingParameters and is not used anywhere else. Keeping it makes the config surface an option that will be silently ignored.
        @field(duration)
        minimumTimeBetweenMrpKicks?: Duration;

        @field(duration)
        kickThrottleInterval?: Duration;

Apollon77 and others added 2 commits July 30, 2026 20:57
Existing coverage overrode both members of kickRestartCooldown at once, which
passes whether the merge is shallow or field-wise. These fail against the
shallow merge with the sibling dropped to undefined.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
minimumTimeBetweenMrpKicks left PeerTimingParameters when the MRP kick logic
was redesigned but stayed on TimingConfig, so setting it was silently ignored.
The completeness guard only looked for source fields the config omits, which is
why it did not catch this; it now rejects divergence in either direction.

Also repoints a stale jsdoc link at Session.peerMrpMargins.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

packages/protocol/src/peer/NetworkProfile.ts:151

  • bdxAdditionalMrpDelay is documented as defaulting to additionalMrpDelay, but in configure() the current precedence (limits.bdxAdditionalMrpDelay ?? parent?.bdx ?? additionalMrpDelay) causes sub-profiles (e.g. connect/probeAddress) that override additionalMrpDelay without also setting bdxAdditionalMrpDelay to inherit the parent BDX margin instead of following their overridden messaging margin. This can yield surprising defaults and violates the stated defaulting behavior for profiles that don’t distinguish traffic classes.
    configure(id: string, limits: NetworkProfiles.Limits, parent?: MRP.Margins) {
        const additionalMrpDelay = limits.additionalMrpDelay ?? parent?.messaging ?? Millis(0);
        const bdxAdditionalMrpDelay = limits.bdxAdditionalMrpDelay ?? parent?.bdx ?? additionalMrpDelay;

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants