fix(EXC): Time out stop_canister requests without a call ID - #11430
Open
mraszyk wants to merge 2 commits into
Open
fix(EXC): Time out stop_canister requests without a call ID#11430mraszyk wants to merge 2 commits into
mraszyk wants to merge 2 commits into
Conversation
Stop contexts predating call IDs (introduced in 6260fc5, 2023-08) can never be timed out: `process_stopping_canisters()` looks up the request time via `get_time_for_stop_canister_call(call_id)`, so the `call_id: None` arm of its `is_expired` predicate returns `false` unconditionally, no matter how much time has passed. Since no such stop request can be younger than two years, and all of them are therefore long past `stop_canister_timeout_duration`, expire them unconditionally instead. There are still such stop contexts on mainnet, e.g. 6 of them on subnet yinp6-35cfo-wgcd2-oc4ty-2kqpf-t4dul-rfk33-fsq3r-mfmua-m2ngh-jqe, as reported by `scheduler_stop_canister_calls_without_call_id`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Coverage should verify replies for both ingress- and canister-originated legacy contexts.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Expires legacy stop_canister contexts without call IDs to unblock permanently pending requests.
Changes:
- Immediately times out call-ID-less stop contexts.
- Adds regression coverage for legacy ingress contexts.
File summaries
| File | Description |
|---|---|
execution_environment.rs |
Marks legacy contexts as expired. |
scheduler/tests.rs |
Tests expiration without advancing time. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address review feedback: the test only asserted that the stop contexts without a call ID were dropped, and covered only the ingress originated one. Assert that the user is told their request failed with `StopCanisterRequestTimeout`; and cover the canister originated case, checking that the queued response is a `SysTransient` reject on the callback of the original stop request, with its cycles refunded. The canister stop context is obtained by clearing the call ID of an actual stop request, rather than by adding a synthetic stop context: responding to it requires the output queue slot that was reserved when that request was inducted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to 9af914a. Security Overview
Detected Code Changes
|
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.
Problem
A
stop_canisterrequest that predates stop-canister call IDs can never time out.ExecutionEnvironment::process_stopping_canisters()determines whether a stop context has expired by looking its creation time up in the subnet call context manager, keyed by call ID. A stop context whosecall_idisNonehas no such entry, so the predicate returnsfalseunconditionally:Call IDs were introduced in 6260fc5 (2023-08-18) and the timeout itself in 4ee1803 (2023-11-22), so every such stop context has been pending for over two years — far beyond
stop_canister_timeout_duration(5 minutes) — with no way to ever be responded to.These still exist on mainnet. On subnet
yinp6-35cfo-wgcd2-oc4ty-2kqpf-t4dul-rfk33-fsq3r-mfmua-m2ngh-jqe, a replica reports:i.e. all six pending stops on that subnet are call-ID-less ones, tracked only on the canisters themselves.
Change
Expire stop contexts without a call ID unconditionally. Nothing else was needed:
try_stop_canister()andreply_to_stop_context()already handle bothStopCanisterContextvariants, so an ingress-originated request getsErrorCode::StopCanisterRequestTimeoutand a canister-originated one gets aSysTransientreject, exactly as for a request with a call ID.Also adds
can_timeout_stop_canister_requests_without_call_id, which pins that a call-ID-less stop context expires in the next round without advancing the time, while a stop context with a call ID that has not yet reached the timeout is left alone.Effect on mainnet
In the first round after this is rolled out, every dormant stop context reaches its originator. For ingress-originated ones the reply goes to an ingress history entry nobody is waiting on. For canister-originated ones, a reject is delivered to a callback that has been dormant for years, but it is a behavior change for canisters whose controllers are not expecting one.