feat: Add a cooling_down flag to SubnetTopology - #10910
Conversation
A "cooling down" subnet stops taking on new work and settles the work it is already tracking, so that its state converges to a state that no longer changes. Concretely, while `SubnetTopology::cooling_down` is set: * All ingress messages addressed to the subnet are rejected with `RejectCode::SysTransient`: by the ingress filter on the receiving nodes, so they never make it into a block; and during block validation, so a block containing such a message is invalid. * All inter-canister requests addressed to it are rejected with `RejectCode::SysTransient` by the sending subnet, before they are routed into a stream. Responses and anonymous refunds addressed to it are instead retained by the sender (in canister output queues and the refund pool, respectively) until it stops cooling down. * It executes no canister messages: the inner round only drains its subnet queues. * It retains no pending `stop_canister` request: a canister that is ready to stop is stopped, and every other stop context is rejected (rather than only those that timed out). Unlike a canister's status, such a request is tracked outside the canister, in the subnet call context manager, so leaving one pending would leave state behind that a canister-level view of the subnet does not capture. `NetworkTopology.subnets` includes the own subnet, so it can answer both "is the destination cooling down" (stream builder) and "am I cooling down" (ingress filter, ingress selector, scheduler, `process_stopping_canisters`); every enforcement point already holds a `ReplicatedState`. There is no registry field backing the flag yet, so it is always `false` in production and only tests set it. Note that `state.metadata.network_topology` is repopulated from the registry at the start of every batch, so overriding the flag on the replicated state only holds in tests that do not go through `MessageRoutingImpl`. Also renames `try_stop_canister`'s predicate from `is_expired` to `should_reject`, as a stop context is now rejected either because it timed out or because the subnet is cooling down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Extends `cooling_down_subnet_rejects_pending_stop_canister_requests` with the ingress case, so that both arms of `reply_to_stop_context()` are covered: a stop request from a canister (replied to via the subnet output queues) and one from a user (`ErrorCode::SubnetCoolingDown` in the ingress history). `SchedulerTest::inject_ingress_to_ic00()` now returns the `MessageId` of the pushed message, so the resulting ingress status can be looked up, and sends it from `self.user_id`, which controls the canisters created by the fixture (as the anonymous default sender is not a controller, a `stop_canister` from it is rejected outright and never creates a stop context). Also adds tests for: * `cooling_down_subnet_runs_no_heartbeats_or_global_timers`: no canister code runs at all, not just no canister messages. * `build_streams_retains_messages_behind_response_to_cooling_down_subnet`: retaining a response holds back the rest of the output queue, including requests, which are then not rejected either. * `build_streams_rejects_loopback_requests_while_cooling_down`: the loopback stream is not exempt. * `build_streams_cooling_down_takes_precedence_over_engine_boundary`: a guaranteed-response request to a cooling down CloudEngine subnet is rejected as `SysTransient` (retryable) rather than `SysFatal`, with no critical error. Finally, the response and refund retention tests now also assert that the retained message is routed once the subnet stops cooling down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`install_code` is a subnet message, so it is still executed (and resumed, if long-running, by `advance_long_running_install_code()`, which runs before the inner round) while the subnet is cooling down -- and it runs the canister's `start` / `pre_upgrade` / `post_upgrade` hooks. What the test actually shows is that no canister *message* of any kind is executed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a “cooling down” mode for subnets by adding a cooling_down flag to SubnetTopology, then using it across message routing, ingress handling, and scheduling to quiesce a subnet (reject new work, drain in-flight work) while preserving correctness and retryability via a new ErrorCode::SubnetCoolingDown.
Changes:
- Add
SubnetTopology::cooling_down(incl. protobuf encoding/decoding) and topology helpers (NetworkTopology::is_cooling_down,ReplicatedState::is_own_subnet_cooling_down). - Enforce cooling-down semantics in stream building (reject requests, retain responses/refunds) and ingress payload selection/validation, and skip canister execution in the scheduler while still draining subnet queues.
- Add
ErrorCode::SubnetCoolingDownand update related mappings/tests across protobuf, canonical state, PocketIC, and error-type conversions.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rs/test_utilities/state/src/lib.rs | Initializes new SubnetTopology.cooling_down field in state test builder. |
| rs/test_utilities/execution_environment/src/lib.rs | Initializes cooling_down in generated subnet topologies for tests. |
| rs/state_manager/tests/state_manager.rs | Updates state manager tests to include cooling_down in SubnetTopology literals. |
| rs/replicated_state/src/replicated_state.rs | Adds ReplicatedState::is_own_subnet_cooling_down() helper. |
| rs/replicated_state/src/metadata_state/tests.rs | Extends topology roundtrip test and adds NetworkTopology::is_cooling_down() unit test. |
| rs/replicated_state/src/metadata_state/proto.rs | Adds protobuf (de)serialization for SubnetTopology.cooling_down. |
| rs/replicated_state/src/metadata_state.rs | Adds NetworkTopology::is_cooling_down() and documents SubnetTopology::cooling_down semantics. |
| rs/replicated_state/src/canister_state/system_state.rs | Generalizes stop-context filtering (is_expired → should_reject) to support cooling-down rejection. |
| rs/protobuf/src/state/ingress/mod.rs | Adds mapping for ErrorCodePublic::SubnetCoolingDown ↔ ErrorCode::SubnetCoolingDown. |
| rs/protobuf/src/gen/types/state.ingress.v1.rs | Adds SubnetCoolingDown = 211 to generated public error-code enum/string conversions. |
| rs/protobuf/src/gen/state/state.metadata.v1.rs | Adds cooling_down field to generated state metadata protobuf. |
| rs/protobuf/src/gen/state/state.ingress.v1.rs | Adds SubnetCoolingDown = 211 to generated state ingress error-code enum/string conversions. |
| rs/protobuf/def/state/metadata/v1/metadata.proto | Adds cooling_down field to SubnetTopology protobuf schema. |
| rs/protobuf/def/state/ingress/v1/ingress.proto | Adds ERROR_CODE_SUBNET_COOLING_DOWN = 211 to ingress error-code schema. |
| rs/messaging/src/state_machine/tests.rs | Updates state machine tests’ subnet topology literals with cooling_down. |
| rs/messaging/src/routing/stream_builder/tests.rs | Adds comprehensive tests for cooling-down routing behavior (reject/retain/precedence/loopback). |
| rs/messaging/src/routing/stream_builder.rs | Implements cooling-down routing semantics for requests/responses/refunds and exports metrics labels. |
| rs/messaging/src/message_routing.rs | Sets cooling_down: false when populating topology from registry (no registry backing yet). |
| rs/interfaces/src/ingress_manager.rs | Adds InvalidIngressPayloadReason::SubnetCoolingDown. |
| rs/ingress_manager/src/ingress_selector.rs | Rejects ingress payload build/validation while cooling down; adds tests for this behavior. |
| rs/execution_environment/src/scheduler/tests.rs | Adds scheduler tests verifying cooling-down behavior (no canister execution, stop_canister handling). |
| rs/execution_environment/src/scheduler/test_utilities.rs | Updates IC00 ingress injection helper to return MessageId and set sender deterministically. |
| rs/execution_environment/src/scheduler/scheduler_metrics.rs | Adds metric counter for inner-round iterations skipped due to cooling down. |
| rs/execution_environment/src/scheduler.rs | Skips canister execution in inner rounds when cooling down (after draining subnet queues). |
| rs/execution_environment/src/history.rs | Adds dashboard label mapping for SubnetCoolingDown. |
| rs/execution_environment/src/execution_environment/tests.rs | Adds ingress-filter test ensuring cooling-down ingress is rejected with SubnetCoolingDown. |
| rs/execution_environment/src/execution_environment.rs | Adds ingress filter check and stop_canister rejection semantics while cooling down. |
| rs/canonical_state/tests/hash_tree.rs | Updates error-code change guard hash due to new error code. |
| rs/canonical_state/src/traversal.rs | Updates traversal tests’ subnet topology literals with cooling_down. |
| packages/pocket-ic/src/lib.rs | Adds SubnetCoolingDown = 211 to PocketIC error-code enum conversions. |
| packages/pocket-ic/CHANGELOG.md | Documents newly added PocketIC error code variant. |
| packages/ic-error-types/src/lib.rs | Adds SubnetCoolingDown to error-code enum, reject-code mapping, and tests. |
Comments suppressed due to low confidence (1)
rs/execution_environment/src/execution_environment.rs:4697
- This reject message also uses a
\line continuation with an indented next line, which will insert indentation spaces into the reject string returned to the calling canister.
StopCanisterReply::SubnetCoolingDown => Payload::Reject(RejectContext::new(
RejectCode::SysTransient,
format!(
"Subnet {} is cooling down, so canister {canister_id}'s stop request \
was rejected",
self.own_subnet_id
),
)),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
… down They time out on their own anyway: without canister execution no canister can become ready to stop, so every pending stop context eventually hits `stop_canister_timeout_duration`. Rejecting them early only duplicated that outcome with a different error code. This reverts the cooling down handling in `process_stopping_canisters()` and drops the `StopCanisterReply::SubnetCoolingDown` variant. `ErrorCode::SubnetCoolingDown` remains, as the ingress filter still uses it. Also corrects two over-claims in the `SubnetTopology::cooling_down` doc comment: a cooling down subnet's state does *not* converge to a state that no longer changes (subnet messages still execute, resource charging still runs every round, and in-flight XNet messages are still inducted); and not *all* inter-canister requests addressed to it are rejected, since retaining a response holds back whatever is queued behind it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An idle canister *is* ready to stop, and a `stop_canister` request for it takes effect right away even while the subnet is cooling down (see `cooling_down_subnet_stops_canisters_that_are_ready_to_stop`). Only a canister with open call contexts cannot become ready to stop, so only its stop requests time out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cooling_down flag to SubnetTopologycooling_down flag to SubnetTopology
cooling_down flag to SubnetTopologycooling_down flag to SubnetTopology
…oling down A message that must never cross an engine boundary (guaranteed-response, or carrying cycles) is illegal there permanently, not transiently, so rejecting it as `SysTransient` because the destination subnet happens to be cooling down would invite a pointless retry. The engine boundary arms therefore now run first, and a response that is illegal at the boundary is no longer retained (so it still raises the critical error it is meant to). A message that *is* legal at the boundary is still handled by cooling down: a request is rejected as `SysTransient`, a response retained. Also: * Reworded the cooling down reject message to "Canister <id> is deployed to a subnet that is cooling down". * The reject-response assertions now also check the reject message, via a new `assert_cooling_down_reject()` helper. * `build_streams_retains_responses_to_cooling_down_subnet` now covers bounded-wait responses as well as guaranteed-response ones. * The loopback test (renamed to `build_streams_does_not_exempt_loopback_while_cooling_down`) now also covers responses, not just requests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…wn for refunds `route_refunds()` checked cooling down first, so a refund crossing an engine boundary to a cooling down subnet was retained instead of being dropped with a critical error. A refund always carries cycles, so one at an engine boundary is illegal there permanently, not transiently -- the same reasoning that already puts the engine boundary arms ahead of cooling down for canister messages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Extracts the idioms that were repeated across the new tests, cutting ~95 lines net while keeping each test's intent explicit. Stream builder tests: * `COOLING_DOWN_LOCAL_CANISTER` / `COOLING_DOWN_REMOTE_CANISTER` constants, replacing the two `canister_test_id()` bindings threaded through every test. * `new_cooling_down_fixture()` now takes the remote `SubnetTopology` (built by `cooling_down_topology()` / `cooling_down_engine_topology()`), so the engine test no longer needs its own inner fixture; the loopback shape gets its own `new_loopback_cooling_down_fixture()`. * `cooling_down_request()` / `cooling_down_response()` message builders. * `assert_no_messages_routed()`, `routed_refund_count()` and `output_queue_contents()` for the stream and output queue assertions. * `assert_one_routed_message()`, replacing the 8-line `assert_routed_messages_eq( metric_vec(..))` incantation at each of its 8 call sites. * `assert_cooling_down_reject()` now delegates to a general `assert_reject_response()`, which the engine test's `SysFatal` case reuses. Scheduler tests: `inject_stop_canister_call()`, `inject_stop_canister_ingress()`, `assert_stop_canister_calls_len()` and `pop_stop_canister_response()`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`stop_canister` requests are only ever rejected upon timeout, so this branch has no business touching that path: * Reverts `rs/replicated_state/src/canister_state/system_state.rs`, restoring `try_stop_canister()`'s `is_expired` predicate name (there is only ever one reason to reject a stop context again). * Reverts `SchedulerTest::inject_ingress_to_ic00()`, which only grew a return value and a sender for the sake of the dropped tests. * Drops the two cooling down `stop_canister` tests and the four helpers that only they used. * Drops the `stop_canister` paragraph from the `SubnetTopology::cooling_down` doc comment. Also, in the doc comment: "canister-level work" -> "messages", and ingress messages are "rejected" rather than "refused". Clarifies that the engine boundary check in `route_refunds()` precedes the cool-down check, and renames `assert_one_routed_message()` to `assert_one_message_status()`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Only the subnet queues are drained while cooling down, which the rest of the doc comment already says; make the opening sentence agree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…arms Both engine boundary arms in `StreamBuilderImpl::build_streams` restated the condition already computed as `is_illegal_engine_msg`. Use the flag instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`build_streams_rejects_requests_to_cooling_down_subnet` covered only a guaranteed-response request without cycles; `build_streams_retains_responses_to_cooling_down_subnet` varied the deadline only. Both now run the full matrix of deadline (none or positive) and attached cycles (zero or positive), which is where an engine boundary would behave differently but a cooling down subnet must not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All call sites passed one of two topologies differing only in `subnet_type`, both with `cooling_down: true`. Take a `SubnetType` and build the topology inside the fixture, as `new_loopback_cooling_down_fixture` already does; drop `cooling_down_topology` and `cooling_down_engine_topology`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to 3a84275. Security Overview
Detected Code Changes
|
| /// Whether the subnet is "cooling down", i.e. quiescing: it stops accepting new | ||
| /// messages and lets the subnet messages already in flight drain. While a subnet |
There was a problem hiding this comment.
While it is true that the subnet stops accepting new ingress messages, it will definitely induct canister messages (that were already in streams; or that are still being routed into streams by subnets with an older registry version).
I would even argue that inducting messages from streams is more central to what we're trying to achieve (empty the streams and subnet queues) than rejecting / not inducting ingress messages (which merely avoids generating any extra subnet messages while we're trying to consume them).
Edit: Actually not inducting ingress messages is just as important, as we're also trying to empty the ingress history of everything but Processing statuses.
With a view to their relative importance, I would personally structure the bullet points below like: (i) induct all messages from streams; (ii) don't route messages to streams (maybe except subnet responses?); (iii) consume subnet messages, as usual; and (iv) don't execute canister messages (to avoid creating more subnet messages).
Edit: So either before (i) or before (iii) there should be a "don't induct any more ingress messages".
alin-at-dfinity
left a comment
There was a problem hiding this comment.
LGTM, modulo some nitpicks and a couple of bigger points:
- We probably need to always route subnet responses, or they'll prevent us from halting the subnet and moving on with the merge.
- (Something of a personal preference) I wouldn't bother rejecting requests when routing to a cooling down subnet. It's unnecessary, it requires extra logic and still leaves us with all the requests hidden behind a response.
| /// Asks the canister if it is willing to accept the provided ingress | ||
| /// message. | ||
| pub fn should_accept_ingress_message( |
There was a problem hiding this comment.
It may be worth elaborating a bit on what effects this has. E.g. it is not immediately obvious that returning an error will cause the ingress message to not be inducted (as opposed to, e.g., simply rejected).
(It is actually obvious, now that I've thought about it, but it's still worth mentioning, since it took me a while.)
There was a problem hiding this comment.
returning an error will cause the ingress message to not be inducted (as opposed to, e.g., simply rejected)
What's the difference between "not inducted" and "simply rejected" here?
There was a problem hiding this comment.
I guess I meant that if this function returns an error, then the ingress message is not even inducted by the payload builder as opposed to being rejected later on by the DSM.
In my case, I wasn't expecting code in rs/execution_environment/src/execution_environment.rs to have effect on the ingress payload builder.
| // Should only happen for old stop requests that existed | ||
| // before call ids were added. | ||
| None => false, |
There was a problem hiding this comment.
Idle question, for my own edification: assuming that any such stop contexts exist, wouldn't it make sense to time them out by now? Or were we concerned that the caller would not expect a (or this specific) reject response?
| // Whether the subnet is "cooling down", i.e. quiescing: it accepts no new | ||
| // ingress or inter-canister messages and executes no canister messages. | ||
| bool cooling_down = 9; |
There was a problem hiding this comment.
Not entirely accurate: we do induct XNet messages, don't we? We just no longer route them.
| // A cooling down subnet inducts no ingress messages at all. Note that this is | ||
| // also checked by the ingress filter on the receiving nodes, so under normal | ||
| // circumstances such a message never even reaches the ingress pool; this check | ||
| // is what makes it binding, both when building and when validating a payload. |
There was a problem hiding this comment.
Couldn't we have a race condition where the "cooling down" flag is flipped while the ingress pool is non-empty?
If so, the ingress filter check is technically only an optimization. This is the only check than ensures correctness.
`inner_round()` no longer bails out after draining the subnet queues while the subnet is cooling down: a cooling down subnet lets the messages it already holds drain, and that includes executing them. * Reverts `rs/execution_environment/src/scheduler.rs`, so canister messages, `Heartbeat` / `GlobalTimer` tasks and same-subnet induction all run as usual. * Drops the `scheduler_round_inner_iteration_skipped_cooling_down` counter, which only counted that bail-out. * Drops the two cooling down scheduler tests, the three helpers that only they used and the imports that only those helpers and tests needed. * Drops the "executes no canister messages" bullet from the `SubnetTopology::cooling_down` doc comment, restores its opening sentence to "lets the messages already in flight drain" (not just the *subnet* messages) and spells out that execution is left alone. The message-level enforcement is untouched: the ingress filter and payload validation keep ingress messages out, and the stream builder keeps rejecting or retaining inter-canister messages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…esponses A request addressed to a cooling down subnet is no longer rejected with a `SysTransient` reject response; it is retained in the sending canister's output queue until the subnet stops cooling down, exactly as a response already was. The whole output queue is skipped, so the messages queued behind it stay put and in order. * `build_streams()`: the cool-down check now covers any canister message, not just responses, observing it under its own message type label. Drops `cooling_down_requests_to_reject`, its match arm, its rejection loop and the now-unused `cooling_down` status label. * The engine boundary check still takes precedence: a message that must never cross an engine boundary is illegal there permanently, so retaining it would only defer the same outcome. * Merges the request and response cooling down tests into `build_streams_retains_messages_to_cooling_down_subnet()`, matrixed over message kind (via the new `cooling_down_messages()`), deadline and attached cycles; the loopback test covers both kinds the same way; and the engine boundary test asserts that a request which *is* legal at the boundary is retained. Drops `assert_cooling_down_reject()`. * `SubnetTopology::cooling_down`: inter-canister messages and anonymous refunds alike are held back by the sender, loopback included. * `ErrorCode::SubnetCoolingDown` now only ever reaches ingress messages, so the PocketIC changelog entry says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ling down The subnet output queues hold only the responses that the subnet itself produced while draining its subnet queues. Route them into streams unconditionally, whether or not the sending subnet or the destination subnet is cooling down: retaining one would leave a cooling down subnet with work still to do, which is the very thing cooling down is meant to avoid. `build_streams()` recognizes them by their sender: a subnet output response has the subnet's own principal as its respondent, and no canister can have that principal as its canister ID. Only canister output queues are held back. The engine boundary check still applies, as it does to any message. Also adds `push_subnet_output_response()` to the tests (reserving the output queue slot the way `drain_subnet_queues()` would, by inducting and popping the matching request), generalizes `clear_cooling_down()` into `set_cooling_down()` and factors out `routed_messages()`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The proto comment claimed that a cooling down subnet "executes no canister messages", which contradicts `SubnetTopology::cooling_down` (and the implementation): execution is left alone, the subnet keeps executing the messages it already holds. Point at the Rust doc for the exact semantics instead of duplicating it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nges They are unrelated to subnet cooling down, so they don't belong in this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Alin Sinpalean <58422065+alin-at-dfinity@users.noreply.github.com>
Also fixes the dangling `let is_engine_dst = !is_loopback_stream` left behind in `build_streams()` by the previous "Apply suggestions from code review" commit, which did not compile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The merge of `master` was textually clean but did not compile: master added a `SubnetTopology` literal in `test_traverse_subnet_metrics_ includes_canister_consumed_cycles_at_v29`, while this branch added the `cooling_down` field to that struct. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ingress filter check can be bypassed: messages already in the ingress pool when the subnet starts cooling down are not affected by it, and a malicious node may ignore it altogether. The check during payload building and validation is what actually guarantees that no ingress message makes it into a block while the subnet is cooling down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It is the senders that stop routing inter-canister messages to a cooling down subnet, retaining them instead. The cooling down subnet itself keeps inducting (and executing) whatever is still in flight, e.g. messages already in its incoming streams, so that it drains. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…down_subnet` Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…_cooling_down_subnet` Add a destination subnet dimension (remote or local) to the test and drop `build_streams_does_not_exempt_loopback_while_cooling_down`, which the new dimension subsumes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing down Messages in the subnet's own output queues are routed to a cooling down destination subnet only if the sending subnet is itself cooling down: retaining one would leave a cooling down subnet with work still to do, which is the very thing cooling down is meant to avoid. A subnet that is not cooling down has no such urgency, so its own output queues are held back just like a canister's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A "cooling down" subnet stops being sent new messages and lets the messages already in flight drain.
While a subnet is cooling down:
There is no registry field backing the flag yet, so it is always
falsein production and only tests set it. The registry plumbing that can actually set the flag must not ship until this code is deployed everywhere, since it also introduces a new error code that requires a staged roll-out.