fix(engine): stop search deliveries double-collecting ETB/landfall triggers - #7058
Merged
Merged
Conversation
…iggers
A library-search delivery (fetchlands, Cultivate, Harrow, every
SearchLibrary -> ChangeZone card) ran TWO trigger collectors over the same
GameEvent::ZoneChanged occurrence:
1. the logical zone-change owner -- change_zone.rs (targeted) or
zone_pipeline.rs (partition) -> complete_logical_zone_trigger_collection
-> state.deferred_triggers
2. park_search_observer_triggers, which re-scanned the raw action slice and
called collect_triggers_into_deferred again with no filter but PhaseChanged
triggers.rs's per-event dedup (registered_this_event) is a HashSet allocated
inside the event loop, so it cannot see across the two passes. A cracked
fetchland fired a landfall observer twice and an Undercity Sewers ETB twice; the
same land merely PLAYED fired once, because handle_play_land calls
zone_pipeline::deliver directly and allocates no logical group -- one collector,
not two.
LogicalZoneChangeGroup::append_delivery_events retains EVERY ZoneChanged in the
slice it is handed, so ownership is per-SLICE: a collector whose slice is
exactly one owner's completion slice may blanket-drop ZoneChanged, and that is
what batch_or_drain_observer_triggers (zone_changes_are_logically_owned) and the
resumed ChangeZone drain in effects/mod.rs correctly do. park's slice spans a
whole continuation drain and can hold zone changes no owner allocated a group
for, so it cannot. It must consult the authority instead.
Extract that authority -- the consumed-occurrence ledger plus the ZoneChanged
values already queued in deferred_triggers -- into
triggers::filter_already_collected_trigger_events_from and route BOTH the
generic priority scan and park through it.
The queued witness is consumed one-for-one rather than by set membership. That
is a BOUND, not a proof: deferred_triggers holds one context per matching
observer, not one entry per occurrence, so N observers of one occurrence
contribute N copies of its value. The new filter therefore removes at most
min(queued_copies, slice_copies), and it is NOT occurrence-exact -- it does not
by itself discharge CR 603.2c's second sentence. No occurrence-exact witness is
reachable here: the only exact record is
LogicalZoneChangeGroup::all_origin_occurrences, and a completed owner's group is
a caller-owned local that is gone before the collector runs (GameState holds a
group only inside the two paused frames).
The HEAD comparison is per-caller. At the generic priority scan the new filter
consumes at most what the set-membership filter it replaces consumed, so that
caller can never newly hide a trigger. At park there was no ZoneChanged filter
at all, so park becoming more suppressive IS the fix -- and the bound above is
therefore also a new residual there: a byte-identical ZoneChanged that no owner
grouped, sharing a slice with an owner-collected copy two observers saw, is
suppressed where HEAD fired it. That residual is bounded by byte-identical
ZoneChanged duplicates being unreachable inside one collector slice, and is
pinned executably by a unit row.
Deliberately does NOT pair mark_ with the four unpaired complete_ owners
(change_zone.rs x2, engine_resolution_choices.rs x2). Claiming an occurrence
also hides it from check_delayed_triggers (engine_priority.rs), which would
silently kill the CR 603.7b leaves-the-battlefield delayed family -- the
reanimation-aura sacrifice from the Animate Dead class and the earthbend
WhenDiesOrExiled gate. A new regression guard pins it, and was watched go red
under the rejected shape.
Park intentionally defers its observers to the next priority checkpoint
(issue #5336), so the new tests pass priority before asserting -- asserting at
the end of the parked action measures nothing, because
WaitingForWithParkedObservers sets skip_deferred_trigger_drain and both test
drivers break on an empty stack.
CR 603.2 / CR 603.2c / CR 603.3b / CR 603.7b / CR 704.5b
(verified in docs/MagicCompRules.txt at lines 2561 / 2567 / 2586 / 2616 / 5494).
matthewevans
enabled auto-merge
August 6, 2026 12:25
Contributor
📝 WalkthroughWalkthroughThe engine adds occurrence-aware filtering for collected trigger events. Priority scans and search observer parking use the shared filter. Unit and integration tests cover duplicate zone changes, search deliveries, delayed triggers, and fail-to-find searches. ChangesTrigger deduplication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SearchEffect
participant SearchObserverParking
participant EventFilter
participant DeferredTriggers
participant TriggerResolution
SearchEffect->>SearchObserverParking: deliver continuation events
SearchObserverParking->>EventFilter: filter collected events
EventFilter->>DeferredTriggers: inspect consumed occurrences and queued witnesses
DeferredTriggers-->>EventFilter: return matching collection state
EventFilter-->>SearchObserverParking: return remaining events
SearchObserverParking->>DeferredTriggers: queue observer triggers
DeferredTriggers->>TriggerResolution: drain deferred triggers
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/game/triggers_dedup_regression_tests.rs`:
- Around line 3803-3838: Update the trigger occurrence model so distinct
equal-valued ZoneChanged occurrences remain distinguishable: carry an
action-scoped occurrence identifier through PendingTriggerContext and
ConsumedTriggerEventOccurrence, and have
filter_already_collected_trigger_events_from consume each occurrence only once
regardless of observer count. Change
owner_collected_filter_counts_contexts_not_occurrences to expect one survivor,
and add a production-pipeline regression covering separate equal-valued
occurrences and separate trigger processing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 13565a15-b37b-4594-8381-e8a86e246f6a
📒 Files selected for processing (7)
crates/engine/src/game/engine_priority.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/triggers.rscrates/engine/src/game/triggers_dedup_regression_tests.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/search_delivery_observer_dedup.rs
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
This was referenced Aug 6, 2026
matthewevans
added a commit
to dan-blanchard/phase
that referenced
this pull request
Aug 7, 2026
…hase-rs#7069) Follow-up to phase-rs#7058. A review comment on that PR asked to carry an action-scoped occurrence identifier through PendingTriggerContext and ConsumedTriggerEventOccurrence so that two equal-valued ZoneChanged entries in one collector slice are consumed as separate occurrences. That remedy is rejected, on evidence: * The identity already exists. ZoneChangeRecord::turn_zone_change_index is a per-turn monotonic index assigned by restrictions::record_zone_change (its sole grower), and it participates in GameEvent's derived PartialEq. Two ZoneChanged values from distinct occurrences therefore cannot be byte-identical. Two that ARE byte-identical denote one occurrence emitted twice, which is exactly what the filter should collapse. * The finding's evidence was a #[cfg(test)] fixture. test_minimal pins the index at 0 for both events; that is a property of the fixture, not a production contract. * The remedy would regress CR 104.4b. deferred_triggers IS compared inside GameState::eq, so injecting a fresh per-instance id there makes two otherwise-identical loop iterations compare unequal: the mandatory-loop draw never fires, the engine burns auto_pass_loop_max_iterations and downgrades to a CR 732.2 halt. This is the same shape that pip_id on ManaUnit caused. The sibling ledger is safe only because consumed_before_priority_trigger_events is excluded from equality -- an exclusion deferred_triggers does not have. So the deliverable is to make the shipped doc comment's asserted premise an enforced one. No type changes; GameState::eq, normalize_for_loop and loop_states_equal are untouched, and ZoneChangeRecord keeps its derive -- keeping it is the mechanism. CR 104.4b is discharged vacuously. Three regression rows pin the invariant, each verified to fail for its stated mechanism when the corresponding authority is falsified: * U5 pins the equality link at the filter authority itself; red when ZoneChangeRecord's PartialEq is hand-written to skip the index. * U6 pins the within-Library reposition family (CR 400.7 zero-bump, per CR 701.20b a reveal is not a zone change) at the real production mover; red when the zones.rs:1820 stamp is zeroed. * N5 pins allocator-to-event fidelity across the search-delivery park path, asserting pre-drain by design; red when the zones.rs:826 stamp is zeroed. It must not pass priority -- a pass runs the drain and makes the assertion vacuous. Also corrects the harness note in search_delivery_observer_dedup.rs, whose universal was already inaccurate before this change: it is scoped to park-path rows asserting what the parked observers did, and N4/N5 assert pre-drain deliberately. Four unrelated engine defects surfaced while establishing the above and are filed rather than fixed here: phase-rs#7063, phase-rs#7064, phase-rs#7065, phase-rs#7066. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
This was referenced Aug 7, 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
1 participant
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.
A library-search delivery (fetchlands, Cultivate, Harrow, every
SearchLibrary -> ChangeZone card) ran TWO trigger collectors over the same
GameEvent::ZoneChanged occurrence:
zone_pipeline.rs (partition) -> complete_logical_zone_trigger_collection
-> state.deferred_triggers
called collect_triggers_into_deferred again with no filter but PhaseChanged
triggers.rs's per-event dedup (registered_this_event) is a HashSet allocated
inside the event loop, so it cannot see across the two passes. A cracked
fetchland fired a landfall observer twice and an Undercity Sewers ETB twice; the
same land merely PLAYED fired once, because handle_play_land calls
zone_pipeline::deliver directly and allocates no logical group -- one collector,
not two.
LogicalZoneChangeGroup::append_delivery_events retains EVERY ZoneChanged in the
slice it is handed, so ownership is per-SLICE: a collector whose slice is
exactly one owner's completion slice may blanket-drop ZoneChanged, and that is
what batch_or_drain_observer_triggers (zone_changes_are_logically_owned) and the
resumed ChangeZone drain in effects/mod.rs correctly do. park's slice spans a
whole continuation drain and can hold zone changes no owner allocated a group
for, so it cannot. It must consult the authority instead.
Extract that authority -- the consumed-occurrence ledger plus the ZoneChanged
values already queued in deferred_triggers -- into
triggers::filter_already_collected_trigger_events_from and route BOTH the
generic priority scan and park through it.
The queued witness is consumed one-for-one rather than by set membership. That
is a BOUND, not a proof: deferred_triggers holds one context per matching
observer, not one entry per occurrence, so N observers of one occurrence
contribute N copies of its value. The new filter therefore removes at most
min(queued_copies, slice_copies), and it is NOT occurrence-exact -- it does not
by itself discharge CR 603.2c's second sentence. No occurrence-exact witness is
reachable here: the only exact record is
LogicalZoneChangeGroup::all_origin_occurrences, and a completed owner's group is
a caller-owned local that is gone before the collector runs (GameState holds a
group only inside the two paused frames).
The HEAD comparison is per-caller. At the generic priority scan the new filter
consumes at most what the set-membership filter it replaces consumed, so that
caller can never newly hide a trigger. At park there was no ZoneChanged filter
at all, so park becoming more suppressive IS the fix -- and the bound above is
therefore also a new residual there: a byte-identical ZoneChanged that no owner
grouped, sharing a slice with an owner-collected copy two observers saw, is
suppressed where HEAD fired it. That residual is bounded by byte-identical
ZoneChanged duplicates being unreachable inside one collector slice, and is
pinned executably by a unit row.
Deliberately does NOT pair mark_ with the four unpaired complete_ owners
(change_zone.rs x2, engine_resolution_choices.rs x2). Claiming an occurrence
also hides it from check_delayed_triggers (engine_priority.rs), which would
silently kill the CR 603.7b leaves-the-battlefield delayed family -- the
reanimation-aura sacrifice from the Animate Dead class and the earthbend
WhenDiesOrExiled gate. A new regression guard pins it, and was watched go red
under the rejected shape.
Park intentionally defers its observers to the next priority checkpoint
(issue #5336), so the new tests pass priority before asserting -- asserting at
the end of the parked action measures nothing, because
WaitingForWithParkedObservers sets skip_deferred_trigger_drain and both test
drivers break on an empty stack.
CR 603.2 / CR 603.2c / CR 603.3b / CR 603.7b / CR 704.5b
(verified in docs/MagicCompRules.txt at lines 2561 / 2567 / 2586 / 2616 / 5494).
Summary by CodeRabbit
Bug Fixes
Tests
Documentation