fix: Reliably garbage collect reject signals - #11263
Open
alin-at-dfinity wants to merge 3 commits into
Open
Conversation
`XNetPayloadBuilder` currently pools and selects slices that have either messages or new signals. This ensures that we can always make progress (new messages are reliably inducted; messages already inducted by the remote subnet are reliably garbage collected); but does not ensure any time bound on garbage collecting reject signals. Streams don't have explicit accept signals, only a `signals_end` index (pointing beyond the last inducted message). But we do have explicit reject signals (for missing or migrating canisters, full queues, etc.). These reject signals must be (and are) garbage collected when the remote subnet indicates that it has acted on them (by garbage collecting messages and advancing its stream begin index). But currently this only happens when a stream slice is inducted for other reasons (new messages or signals). In order to ensure that reject signals don't remain alive indefinitely in an otherwise idle stream, we must also actively induct stream slices with no messages and no new signals iff the stream begin they advertise points past at least one of our reject signals. We do this by adding a `gced_message_index` field to `ExpectedIndices` (next to the existing `message_index` and `stream_index`). This field points to the next reject signal, which may not be the first reject if the validation context includes some payloads on top of the certified state (i.e. if we already included slices that GC-ed some reject signals). Based on this field, we can decide (when selecting, validating or garbage collecting slices) whether a slice with no messages and no signals should still be retained and/or inducted because its induction will result in garbage collecting at least one reject signal. This change also drops the `signals_begin` field (only used for metrics and extra validation). And ensuring that it always converges to `signals_end` on an idle stream would be the equivalent of having explicit reject signals (requiring otherwise unnecessary extra roundtrips). Dropping the field altogether means that rolling back to an older replica version would simply result in that replica starting with `signals_begin = 0`, which is entirely benign as it respects all invariants.
|
✅ No security or compliance issues detected. Reviewed everything up to 80cf5a3. Security Overview
Detected Code Changes
|
Contributor
There was a problem hiding this comment.
Pull request overview
Updates XNet payload handling to reliably garbage collect reject signals on idle streams.
Changes:
- Tracks reject-signal GC progress during slice selection and validation.
- Removes persisted
signals_beginstate. - Updates stream metrics and adds regression tests.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
rs/xnet/payload_builder/tests/xnet_payload_builder.rs |
Tests header-only GC slices. |
rs/xnet/payload_builder/tests/certified_slice_pool.rs |
Tests reject-signal-aware pool GC. |
rs/xnet/payload_builder/src/test_fixtures.rs |
Updates expected stream indices. |
rs/xnet/payload_builder/src/lib.rs |
Implements reject-signal GC tracking. |
rs/xnet/payload_builder/src/impl_tests.rs |
Adds index and validation tests. |
rs/xnet/payload_builder/src/certified_slice_pool.rs |
Retains useful header-only slices. |
rs/test_utilities/state/src/lib.rs |
Removes generated signals_begin. |
rs/replicated_state/src/metadata_state/tests.rs |
Updates stream-state tests. |
rs/replicated_state/src/metadata_state/proto.rs |
Removes field serialization. |
rs/replicated_state/src/metadata_state.rs |
Removes field and adds reject lookup. |
rs/protobuf/src/gen/types/state.queues.v1.rs |
Updates generated protobuf type. |
rs/protobuf/src/gen/state/state.queues.v1.rs |
Updates generated state protobuf type. |
rs/protobuf/def/state/queues/v1/queues.proto |
Removes and reserves protobuf tag. |
rs/messaging/src/routing/stream_handler/tests.rs |
Updates stream-handler fixtures. |
rs/messaging/src/routing/stream_handler.rs |
Removes obsolete signal-begin validation. |
rs/messaging/src/routing/stream_builder/tests.rs |
Updates signal metric expectations. |
rs/messaging/src/routing/stream_builder.rs |
Reports explicit reject-signal counts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+486
to
+488
| gced_message_index: stream.map_or(StreamIndex::from(0), |s| { | ||
| s.next_reject_signal_index(max_header_begin) | ||
| }), |
Contributor
Author
There was a problem hiding this comment.
While this could happen (block maker validates based on an older certified state; the intervening payloads have resulted in us inducting more messages in subsequent states; remote subnet has seen more recent certified states and its header.begin() has advanced past our outdated signals_end), the worst outcome is inducting multiple empty slices with monotonically increasing header.begin() values, none of which result in any reject signals getting garbage collected. A couple hundred bytes of wasted block space each.
Still, we can easily avoid this waste if we make gced_message_index an Option<StreamIndex> and past the last reject signal set it to None. As in "no header.begin() will result in GC-ing any reject signals, so don't bother". This condition may change in the future (e.g. when we induct the messages in the intervening payloads, we may generate reject signals for some of them); but the condition will be re-evaluated once those future certified states become available to the payload builder; and we can decide to induct the previously rejected otherwise empty header then.
Contributor
Author
There was a problem hiding this comment.
Done. gced_message_index is now an Option and we only return Some when it refers to an actual reject signal to be garbage collected. Past the last reject signal, we return None.
Comment on lines
+847
to
+852
| gced_message_index: state | ||
| .streams() | ||
| .get(&subnet_id) | ||
| .map_or(StreamIndex::from(0), |stream| { | ||
| stream.next_reject_signal_index(slice.header().begin()) | ||
| }), |
…n() beyond it will definitely result in GC-ing a reject signal. Past the last reject signal, return None.
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.
XNetPayloadBuildercurrently pools and selects slices that have either messages or new signals. This ensures that we can always make progress (new messages are reliably inducted; messages already inducted by the remote subnet are reliably garbage collected); but does not ensure any time bound on garbage collecting reject signals.Streams don't have explicit accept signals, only a
signals_endindex (pointing beyond the last inducted message). But we do have explicit reject signals (for missing or migrating canisters, full queues, etc.). These reject signals must be (and are) garbage collected when the remote subnet indicates that it has acted on them (by garbage collecting messages and advancing its stream begin index). But currently this only happens when a stream slice is inducted for other reasons (new messages or signals).In order to ensure that reject signals don't remain alive indefinitely in an otherwise idle stream, we must also actively induct stream slices with no messages and no new signals iff the stream begin they advertise points past at least one of our reject signals.
We do this by adding a
gced_message_indexfield toExpectedIndices(next to the existingmessage_indexandstream_index). This field points to the next reject signal, which may not be the first if the validation context includes some payloads on top of the certified state (i.e. if we already included slices that garbage collected some reject signals). Based on this field, we can decide (while selecting, validating or garbage collecting slices) whether a slice with no messages and no signals should be considered "non-empty" because its induction would result in garbage collecting at least one reject signal.This change also drops the
signals_beginfield (only used for metrics and extra validation). And ensuring that it always converges tosignals_endon an idle stream would be the equivalent of having explicit reject signals (requiring otherwise unnecessary extra roundtrips). Dropping the field altogether means that rolling back to an older replica version would simply result in that replica starting withsignals_begin = 0, which is entirely benign as it respects all invariants.