fix: debug_assert against recording IngressStatus::Unknown - #11015
Merged
Merged
Conversation
`IngressStatus::Unknown` stands for the absence of an ingress history entry, so it must never be recorded as the status of a message. The `IngressStatus::is_valid_state_transition()` checks done by the callers of `IngressHistoryState::insert()` only catch this if an entry already exists, as any transition away from `Unknown` (i.e. from "no entry") is allowed. Assert it at the bottom of the stack instead, where the entry is actually recorded. Three test fixtures did record one: * `test_traverse_ingress_history` covered the canonical state encoding of `Unknown`, which is unreachable for the same reason. Drop the entry along with the corresponding expected traversal. * `valid_transitions()` listed `Unknown` both as an origin and as a target status. Represent the origin as an `Option<IngressStatus>`, where `None` stands for "no ingress history entry" (which is what `Unknown` means), and drop it from the targets. `test_invalid_transitions` thus still covers `Unknown` as an invalid target from every origin, including from an empty ingress history, which is exactly what the `debug_assert` catches. * the `test_backward_compatibility` state fixture recorded five of them. Record the five ingress states that actually can be recorded instead, which also covers the `reply` and `Completed(Reject)` canonical encodings that the fixture was missing. The partial state hashes change accordingly: the fixture changed, the hashing did not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a defensive debug_assert! to prevent recording IngressStatus::Unknown (which semantically represents “no ingress history entry”), and updates test fixtures to stop encoding/expecting Unknown as a persisted status.
Changes:
- Add
debug_assert!inIngressHistoryState::insert()to catch attempts to recordIngressStatus::Unknown. - Update multiple tests/fixtures to avoid persisting
Unknownand to model “no entry” asNonewhere appropriate. - Refresh backward-compatibility fixture ingress states and update expected partial state hashes accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rs/state_manager/src/tree_hash.rs | Updates the backward-compatibility state fixture ingress statuses and expected partial state hashes. |
| rs/replicated_state/src/metadata_state/tests.rs | Adds a regression test for inserting Unknown and removes Unknown from an existing fixture’s status list. |
| rs/replicated_state/src/metadata_state.rs | Adds a debug_assert! preventing IngressStatus::Unknown from being recorded. |
| rs/execution_environment/tests/history.rs | Reworks transition fixtures/tests to treat “no entry” as None and make Unknown invalid as a target. |
| rs/canonical_state/src/traversal.rs | Removes an unreachable traversal fixture/expectation for encoding Unknown in canonical state traversal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
✅ No security or compliance issues detected. Reviewed everything up to 2bcaef2. Security Overview
Detected Code Changes
|
schneiderstefan
approved these changes
Aug 5, 2026
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.
IngressStatus::Unknownstands for the absence of an ingress history entry, so it must never be recorded as the status of a message. Hence, this PR adds adebug_assertfor preventingIngressStatus::Unknownto be inserted into the ingress history.Two test fixtures did record one:
test_traverse_ingress_historycovered the canonical state encoding ofUnknown, which is unreachable for the same reason. Drop the entry along with the corresponding expected traversal.valid_transitions()listedUnknownboth as an origin and as a target status. Represent the origin as anOption<IngressStatus>, whereNonestands for "no ingress history entry" (which is whatUnknownmeans), and drop it from the targets.test_invalid_transitions(testing all butvalid_transitions()) thus now coversUnknownas an invalid target from every origin, including from an empty ingress history.Note. The
IngressStatus::is_valid_state_transition()checks done by the callers ofIngressHistoryState::insert()only catch this if an entry already exists, as any transition away fromUnknown(i.e. from "no entry") is allowed. This PR doesn't tighten the validation as it could crash a subnet in production (unlikedebug_assert).