fix(viewer): deterministic + sticky live-campaign resolution — active PC no longer flips between beats (#735) - #741
Merged
Conversation
… PC no longer flips between beats (#735) The focused/active player character silently switched between DM beats with no UI handoff (newbie: Liara<->Rolan every beat; adversarial: Florrick->Rolan; narrative: Rolan->Liara). Root cause = a NON-DETERMINISTIC live-campaign pick. PRODUCT FIX (viewer/server.py): - `_pick_campaign` resolved the attached campaign with `max(snaps, key=(has_player, _campaign_recency))` where `_campaign_recency` is the jittery *filesystem* mtime and there was NO stable tiebreak. When two campaigns are BOTH seated (each has a kind=="player") AND tie on recency, `max` returned whichever `glob("*/snapshot.json")` yielded first — filesystem-readdir-order-dependent (APFS returns sorted so it hid locally; the QA VM's ext4/tmpfs returns hash order, which produced the flip). Downstream `_action_actor`/`_lead_pc` then returned the single seated player in whatever snapshot won -> the per-beat PC flip. - New `_snapshot_updated_at` reads the snapshot BODY `updated_at` (the engine's sole-writer clock, re-stamped on every save), falling back to `_campaign_recency` only for legacy snapshots that predate the body clock. The pick is now `(has_player, body updated_at)` with the remaining tie broken on the lexicographically-SMALLEST id — mirroring `store.active_campaign_id` EXACTLY, so the viewer's auto-follow and the engine resolve to the SAME live campaign every read. The deterministic key IS the stickiness: on a recency tie the same id wins every call, so the attached campaign (and the active PC) moves only when a genuinely strictly-newer campaign is written — never on jitter. HARNESS FIX (qa/ui_playtest_app.sh, qa/vm/sweep_v2.sh): - Each persona REUSES its play-state store and never cleaned `campaigns/` (the `: >` truncations reset only the sidecars), so a re-run minted a 2nd seated campaign and manufactured the equal-recency precondition. Wipe both the solo and Part-B `-b` stores for the run prefix before launch -> exactly one seated campaign. bash 3.2-clean, guarded so the glob can never widen to all of play-state/. TESTS (TDD; red-verified against the pre-fix code): - viewer/tests/test_campaign_resolution_stability.py: pins order-independence across injected glob/readdir permutations, actor stability across beats, agreement with the engine resolver, strictly-newer-wins, and legacy round-trip. - servers/engine/tests/test_lean_reground_contamination.py: pins the smallest-id tiebreak on an `updated_at` tie. Additive; engine stays the sole writer; old snapshots round-trip.
|
Warning Review limit reached
More reviews will be available in 2 minutes and 34 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Comment |
This was referenced Jun 9, 2026
100yenadmin
added a commit
that referenced
this pull request
Jun 14, 2026
…g the live pointer (F08-2/3/4/5) (#853) P2 persistence-robustness cluster (issue #804) from the WorldOS full-engine adversarial audit. Four skeptic-verified defects in the single-writer layer: F08-2 — save_campaign saved UNCONDITIONALLY, so a pure read (check_consequences / check_companion_arc / check_faction_arcs / world_tick, and scene_context which delegates to check_companion_arc every beat) bumped updated_at and flipped the #640 "most-recently-updated == live" pointer onto the inspected campaign (the silent Rolan<->Liara character-switch class). Fix as filed: a dirty-skip chokepoint in save_campaign — serialize with the CURRENT (pre-stamp) updated_at, byte-compare to disk; identical => return without write/stamp. A genuine mutation still differs => still saves (a real write is never dropped). This fixes all 92 call sites at the chokepoint, including world_tick (which legitimately mutates last_tick_day on its first per-day call, so it saves once then no-ops on repeat). F08-3 — the tolerant load dropped unknown top-level keys then the next save destroyed them permanently. Now stash the ORIGINAL bytes write-once into campaigns/<id>/snapshot.pre-tolerant.json (fixed name + skip-if-exists: first-skew bytes are the valuable ones; degrade-not-abort on backup failure) and surface the dropped key names on the resume path (start_session / start_world resume return a schema_drift block) instead of only logging. F08-4 — list_campaigns / campaigns_for_world / active_campaign_id used the STRICT parse only, so a tolerant-loadable campaign was invisible to listings and to the #640 resolver (which would silently pick the OTHER, strict-only campaign). They now share the same tolerant loader (_load_summary) — strict first, tolerant retry on failure — while staying PURE reads (no backup write, no module-state mutation). F08-5 — one torn session-log line (an OOM-killed half-write) raised out of read_log -> read_log_all -> recap_from_store -> start_session's recap, bricking resume until hand-repair. read_log now skips-and-warns the unparseable line; append_log newline-heals when the file's last byte isn't a newline so the poison can't grow by concatenation onto the torn fragment. Invariants upheld: engine stays sole writer; pure reads do NOT save/bump updated_at (the whole point); additive-by-default (old snapshots round-trip byte-identical through the chokepoint; schema_drift is an additive optional output field; no schema or wire-contract change); the pre-tolerant sibling file is inert to every glob. Red->green tests (servers/engine/tests/test_store_persistence_robustness.py, 24): a pure check_*/world_tick-repeat does NOT change updated_at and does NOT flip the live pointer; a firing check_consequences / first world_tick / any real mutation DOES still save; tolerant load writes the backup once and surfaces dropped keys; enumerators see the tolerant campaign while staying read-only; torn log lines are skipped not fatal; append newline-heals. Closes #804 Refs #640, #165, #741 Findings: F08-2, F08-3, F08-4, F08-5 Source: docs/audits/ENGINE-AUDIT-2026-06-11.md Co-authored-by: Eva <arncalso@gmail.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
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.
Summary
Fixes #735 — the keystone
zero_criticalblocker where the focused/active player character silently switches between DM beats with no UI handoff (newbie: Liara↔Rolan every beat; adversarial: Florrick→Rolan; narrative: Rolan→Liara). Root cause: a non-deterministic live-campaign pick.Two stacked bugs (both fixed)
1. PRODUCT — non-deterministic live-campaign resolution (the release-blocking half)
viewer/server.py_pick_campaignresolved the attached campaign withmax(snaps, key=(has_player, _campaign_recency)), where_campaign_recencyis the jittery filesystem mtime and there was no stable tiebreak. When two campaigns are BOTH seated (each has akind=="player") AND tie on recency,maxreturns whichevercdir.glob("*/snapshot.json")yielded first — filesystem-readdir-order-dependent. macOS APFS returns sorted order so it hid locally; the QA VM's ext4/tmpfs returns hash order, which produced the observed flip. Downstream_action_actor/_lead_pcdeterministically return the single seated player in whatever snapshot won → the visible per-beat PC flip. A real player with two saves of equal recency hits this.The fix makes the pick deterministic + sticky and agrees with the engine's authoritative resolver:
_snapshot_updated_at(snap, path)reads the snapshot BODYupdated_at(the engine's sole-writer clock, re-stamped on everystore.save_campaign), falling back to_campaign_recencyonly for a legacy snapshot that predates the body clock (so old saves round-trip).(has_player, body updated_at)with the remaining tie broken on the lexicographically-smallest campaign id — mirroringstore.active_campaign_idexactly (largestupdated_at, smallest-id tiebreak). The viewer's auto-follow and the engine now resolve to the same live campaign every read.2. HARNESS — manufactured the equal-recency precondition
qa/ui_playtest_app.shandqa/vm/sweep_v2.shreuseplay-state/vm2-<persona>(+ the Part-B-bstore) and never cleancampaigns/— the: >truncations reset only the sidecars. Each re-run minted a 2nd seated campaign in the same store → the equal-recency precondition. Fix: wipe both the solo and-bstores for the run prefix before each persona launch → exactly one seated campaign. bash 3.2-clean, guarded so the glob can never widen to all ofplay-state/.Tests (TDD; red-verified against the pre-fix code)
viewer/tests/test_campaign_resolution_stability.py(6 tests): order-independence across injected glob/readdir permutations (reproduces the flip on any host filesystem), actor stability across beats, agreement with the engine resolver, strictly-newer-wins, legacy round-trip. 4 of 6 confirmed RED on the original code (the flip{camp_aaa…, camp_zzz…}is reproduced); all GREEN with the fix.servers/engine/tests/test_lean_reground_contamination.py(+1 test): pinsactive_campaign_idreturns the lexicographically-smallest id on anupdated_attie.Verification
test_live_view_recovery+test_readmodel_surfaces: 50 passed, 4 subtests passedtest_lean_reground_contamination: 7 passedviewer/testssuite: 457 passed, 6 skipped, 48 subtests passed (no regression)qa/fast_gate.sh: PASS — 188 passedScope discipline
Additive; engine stays the sole writer; old snapshots round-trip. Does NOT touch the
engine_logged/dup-narration dedup (#727), the chronicle render (#731/#732), or wire contracts.