feat: Do not route canister messages to a cooling down subnet - #11129
feat: Do not route canister messages to a cooling down subnet#11129mraszyk wants to merge 6 commits into
Conversation
Messages (requests and responses) in canister output queues destined for a subnet that is cooling down are no longer routed into the stream to it. They are retained in the output queue they came from (along with everything behind them in the same queue, so ordering is preserved) until the destination subnet stops cooling down, rather than being rejected or dropped. This includes the loopback stream, i.e. a cooling down subnet does not route its canisters' messages to its own canisters either. The subnet's own output queues are not affected; neither are refunds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates message routing so that canister requests/responses destined for a cooling down subnet are retained in canister output queues (including for loopback) and routed once the destination subnet is no longer cooling down.
Changes:
- Extend
SubnetTopology::cooling_downdocumentation to specify stream-routing behavior for canister output queues. - Update stream building logic to skip routing canister-output-queue messages to cooling-down destination subnets and emit a new
retained_cooling_downmetric status. - Add targeted routing tests covering retention, later routing after cooldown ends, and unaffected routing to other subnets.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rs/replicated_state/src/metadata_state.rs | Documents expanded semantics of cooling_down related to stream routing. |
| rs/messaging/src/routing/stream_builder.rs | Implements retention of canister messages to cooling-down subnets (incl. loopback) and adds a new metrics status label. |
| rs/messaging/src/routing/stream_builder/tests.rs | Adds coverage ensuring messages are retained during cooldown and routed immediately after cooldown clears, plus a control test for other subnets. |
Suppressed comments (1)
rs/messaging/src/routing/stream_builder/tests.rs:1377
- This inline comment also calls
CanisterId::from(cooling_down_subnet)“its management canister”. The management canister isIC_00; here the receiver is the subnet’s own principal-as-canister-id (subnet message endpoint).
// A canister hosted by the cooling down subnet; and the subnet itself, i.e.
// its management canister.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
✅ No security or compliance issues detected. Reviewed everything up to 7bd6d6e. Security Overview
Detected Code Changes
|
alin-at-dfinity
left a comment
alin-at-dfinity
left a comment
There was a problem hiding this comment.
Haven't looked at the tests yet.
| // Cheap to clone, `RequestOrResponse` wraps `Arcs`. | ||
| let msg = msg.clone(); | ||
|
|
||
| let is_from_subnet_queues = msg.sender() == own_subnet_as_canister_id; |
There was a problem hiding this comment.
is_subnet_response?
It might also make the intent clearer: we must still route/deliver subnet responses, just not other messages.
There was a problem hiding this comment.
is_subnet_response?
That would be correct because subnet output queues can only contain responses. But I planned to leave subnet output queues for a follow-up PR.
we must still route/deliver subnet responses, just not other messages.
I now put this into the design doc: messages in subnet output queues (responses as the mgmt canister never makes requests itself) on subnets that are not “cooling down” are not routed to streams to subnets that are “cooling down” (i.e., they stay in the subnet output queues)—this is enough because the (source) subnet that is “cooling down” is not being routed any new input subnet messages and thus it stops “bombarding” the target subnet which is also “cooling down” by its subnet output queue messages eventually;
I.e., we should still not route responses from subnets that are not cooling down. Otherwise, canisters on the subnet that is cooling down could keep calling the mgmt canister on a remote subnet that is not cooling down and if those responses are not held back, then the stream to the cooling down subnet never fully empties.
There was a problem hiding this comment.
Right, my bad. I totally forgot that this was only an issue for the cooling down subnet itself and its loopback stream.
But then what's with the if !is_from_subnet_queues && is_cooling_down(&dst_subnet_id) below? Why are we letting messages from subnet queues through? Or is this change intended to be strictly about "messages from canister output queues", with "messages from subnet output queues" intended to be handled in the follow-up PR?
If so, I suppose I found it somewhat confusing that you chose to explicitly deal with subnet queues in this PR. If you were going to split it into 2 PRs, I would have found it a lot more natural to ignore subnet queues in this PR altogether. I.e. simply say if is_cooling_down(&dst_subnet_id) { exclude_queue() }. Then, in the follow-up refine it to "except for routing subnet responses into the loopback stream"
There was a problem hiding this comment.
Or is this change intended to be strictly about "messages from canister output queues", with "messages from subnet output queues" intended to be handled in the follow-up PR?
Exactly, this PR should not change behavior for subnet output queues at all.
this was only an issue for the cooling down subnet itself and its loopback stream
not sure what you mean by this; if you refer to the case when we need to route subnet responses, then we need to do so unless source is not cooling down and target is cooling down, i.e., whenever source is cooling down or target is not cooling down
…metric `observe_message_status()` only counts messages that were consumed (routed, rejected or dropped), so messages retained because their destination subnet is cooling down do not belong there. Count the skipped output queues in a dedicated `mr_cooling_down_skipped_queues` counter instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Alin Sinpalean <58422065+alin-at-dfinity@users.noreply.github.com>
Wrap the subnet cooling down tests into an inline `cooling_down` module and rename `DESTINATION_CANISTER` to `COOLING_DOWN_CANISTER`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // Cheap to clone, `RequestOrResponse` wraps `Arcs`. | ||
| let msg = msg.clone(); | ||
|
|
||
| let is_from_subnet_queues = msg.sender() == own_subnet_as_canister_id; |
There was a problem hiding this comment.
Right, my bad. I totally forgot that this was only an issue for the cooling down subnet itself and its loopback stream.
But then what's with the if !is_from_subnet_queues && is_cooling_down(&dst_subnet_id) below? Why are we letting messages from subnet queues through? Or is this change intended to be strictly about "messages from canister output queues", with "messages from subnet output queues" intended to be handled in the follow-up PR?
If so, I suppose I found it somewhat confusing that you chose to explicitly deal with subnet queues in this PR. If you were going to split it into 2 PRs, I would have found it a lot more natural to ignore subnet queues in this PR altogether. I.e. simply say if is_cooling_down(&dst_subnet_id) { exclude_queue() }. Then, in the follow-up refine it to "except for routing subnet responses into the loopback stream"
Messages (requests and responses) in canister output queues destined for a subnet that is cooling down are no longer routed into the stream to it. They are retained in the output queue until the destination subnet stops cooling down, rather than being rejected or dropped. This includes the loopback stream, i.e. a cooling down subnet does not route its canisters' messages to its loopback stream either.