test(xnet): exercise silently dropped responses on subnet deletion#10793
Open
mraszyk wants to merge 16 commits into
Open
test(xnet): exercise silently dropped responses on subnet deletion#10793mraszyk wants to merge 16 commits into
mraszyk wants to merge 16 commits into
Conversation
The subnet deletion XNet test only exercised that requests towards a
deleted subnet are rejected, not that responses towards it are silently
dropped (a response can never be rejected).
Add a universal-canister op `LoopUntilGlobalDataSet` that loops making a
self `canister_status` inter-canister call until the canister's global
data equals a trigger blob, then replies with a reply blob. This delays a
reply across message boundaries until an external message sets the global
data.
Use it in `subnet_delete_test` so UC (on the deleted subnet C) makes two
fire-and-forget calls to two canisters on the surviving sender subnet S,
whose replies are released at different times:
- one before US's 2 MB requests, so its response is inducted into the
S->C stream;
- one after the S->C stream is full, so its response stays in the sender's
output queue.
Both responses must be silently dropped when C is deleted. The queued
response is verified via the
`mr_routed_message_count{type="response",status="canister_not_found"}`
metric; the streamed response is dropped together with the whole stream
(intentionally metric-silent), so it is covered indirectly by the
unallowed-`panicked` log guard plus a post-drop health check.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t-delete test
Port the response-dropping extension of the system test
`rs/tests/message_routing/xnet/subnet_delete_test.rs` to its StateMachine
analogue.
UC (on the deleted subnet C) makes two fire-and-forget looping calls (the
new `LoopUntilGlobalDataSet` op) to two canisters on the surviving sender
subnet S; their replies are released at different times so that one response
ends up in the S->C stream and the other stays in the sender's output queue.
After C is deleted both must be silently dropped.
Thanks to direct `ReplicatedState` access, the drops are asserted precisely:
the queued response via
`mr_routed_message_count{type="response",status="canister_not_found"} == 1`,
and the streamed response via S's stream towards C being discarded (the
metric-silent bulk drop). Bump the best-effort call timeout so the
early-created responses survive until C is deleted.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-delete-response-drop # Conflicts: # Cargo.lock
…ister change Adding the LoopUntilGlobalDataSet op to the universal canister changes its wasm size and per-message instruction counts, which shifts the split-finder's near-symmetric partition and flips the source/destination assignment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands the subnet-deletion XNet test coverage to validate that in-flight responses destined for a deleted subnet are silently dropped (since responses can’t be rejected), in addition to the existing checks for in-flight request rejection. It introduces a new universal-canister operation to deterministically delay responses until a trigger condition is met, enabling the tests to place responses precisely in either an output queue or an XNet stream at deletion time.
Changes:
- Add a universal-canister op
LoopUntilGlobalDataSetto delay replies across message boundaries via repeatedcanister_statuscalls. - Extend the system test and its
StateMachineanalogue to create and observe both a streamed response and a queued response towards a soon-to-be-deleted subnet, then verify silent drops. - Update test dependencies/build metadata and refresh some recovery integration-test metric expectations.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rs/universal_canister/lib/src/lib.rs | Adds a PayloadBuilder helper for the new looping op and extends opcode conversion tests. |
| rs/universal_canister/impl/src/main.rs | Implements LoopUntilGlobalDataSet execution logic using repeated canister_status calls and callbacks. |
| rs/universal_canister/impl/src/lib.rs | Assigns a new opcode value (100) for LoopUntilGlobalDataSet. |
| rs/tests/message_routing/xnet/subnet_delete_test.rs | Extends the subnet-deletion system test to exercise and verify silent dropping of responses. |
| rs/tests/message_routing/xnet/Cargo.toml | Adds ic-config dependency for stream-size constants used by the test. |
| rs/tests/message_routing/xnet/BUILD.bazel | Adds Bazel dep on //rs/config to match the Rust dependency. |
| rs/state_machine_tests/tests/subnet_delete.rs | Extends StateMachine test scenario to validate response drops (queue + stream) and stream disappearance. |
| rs/recovery/subnet_splitting/tests/integration_tests.rs | Updates expected metric values in an integration test. |
| Cargo.lock | Records the new ic-config dependency in the lockfile. |
💡 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>
Matching on the bare subnet id substring could select a different per-stream series whose subnet id happens to contain it as a substring. Match on the full `remote="<id>"` label instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The self `canister_status` call targets the management canister (with canister_id set to self), not this canister directly. Update the comment to match, consistent with the lib.rs doc-comment wording. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-delete-response-drop # Conflicts: # rs/recovery/subnet_splitting/tests/integration_tests.rs
Address review feedback on the subnet-delete response-dropping tests: - Flatten the sub-lettered runbook steps into a single 1..11 numbering and keep it identical between the StateMachine test and the XNet system test, updating every inline step cross-reference accordingly. - Merge the two fire-and-forget call groups into Step 3, promote the former 4b/5a/5b into their own steps, and drop the redundant US2 health-check step and the trigger-processed confirmation. - Reword "looping on self `canister_status` calls" to "looping on `canister_status` calls" and simplify the `loop_until_global_data_set` doc comment. - Revert the unnecessary `try_from_macro_works` assertion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reword the looping-callee, call-timeout, reply-release, and Step 5 comments for accuracy and clarity; the call-timeout note no longer implies the bounded completion-wait loop can hang. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Spell out the stream builder's soft byte limit (count_bytes() >= TARGET_STREAM_SIZE_BYTES, checked before each message) in both subnet-delete tests' Step 7 comments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Relabel the StateMachine test's Step 9 substeps to 9a/9b/9d/9e so shared letters mean the same thing across both subnet-delete tests, with a documented 9c gap (T observes the deletion registry version synchronously here, so there is no separate wait). Also align the runbook Step 9 prose. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Step 11 check on
mr_routed_message_count{type="response",status="canister_not_found"} used an
absolute `>= 1` threshold. Both matrix entries share a single IC (one
with_setup) and the same sender subnet S, so this cumulative counter carries
over: the second entry could pass off the first entry's leftover count even if
its own queued response was not dropped.
Capture a baseline just before deleting subnet C (Step 9a) and assert the
counter grows by at least 1 relative to it. Split the helper into
read_dropped_response_count (plain fetch) and
wait_for_dropped_response_count_at_least.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Use Principal::management_canister().as_slice() instead of a bare &[] for the canister_status callee in the LoopUntilGlobalDataSet op. Byte-identical (the management canister id is the empty blob), just clearer intent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
The subnet-deletion XNet tests only checked that in-flight requests towards a deleted subnet are rejected. This PR also exercises that in-flight responses towards a deleted subnet are silently dropped (a response can never be turned into a reject).
LoopUntilGlobalDataSetthat loops oncanister_statuscalls (requesting its own status) until its global data is set to a fixed blob, then replies — used to delay a response until a chosen moment.rs/tests/message_routing/xnet/subnet_delete_test.rs) and itsStateMachineanalogue (rs/state_machine_tests/tests/subnet_delete.rs): the deleted subnet's canister calls two canisters on a surviving subnet whose replies are released so that one response ends up in the stream and the other in the sender's output queue when the subnet is deleted.mr_routed_message_count{type="response",status="canister_not_found"}; the streamed response via the stream being discarded (metric-silent bulk drop).