fix(engine): persistence-robustness cluster — pure reads stop flipping the live pointer (F08-2/3/4/5) - #853
Conversation
…g the live pointer (F08-2/3/4/5) 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
|
Warning Review limit reached
More reviews will be available in 20 minutes and 51 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ 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 (3)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8771f9260e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # is NEVER dropped). Note (F08-3 composition): after a TOLERANT load the in-memory dump differs | ||
| # from the on-disk bytes (dropped keys are gone), so the candidate != disk and we DO write — | ||
| # which is exactly why F08-3 stashes the original bytes independently BEFORE the drop. | ||
| candidate = campaign.model_dump_json(indent=2) |
There was a problem hiding this comment.
When a campaign was last written by a previous engine SHA, save_campaign overwrites campaign.engine_sha before building this byte-identity candidate. A no-op load/save then differs only by the SHA, so the new dirty-skip path still rewrites the inspected campaign, bumps updated_at, and can flip active_campaign_id after an engine update—the same pure-read pointer-stealing class this change is trying to remove.
Useful? React with 👍 / 👎.
1 participant
What
Fixes the P2 persistence-robustness cluster (#804) from the WorldOS full-engine adversarial audit (2026-06-11). Four skeptic-verified defects in the single-writer persistence layer where a pure read silently corrupted recency / dropped data / bricked resume.
Source:
docs/audits/ENGINE-AUDIT-2026-06-11.md(Part C, unit 08). Closes #804. Refs #640, #165, #741.Findings fixed
F08-2 — pure reads flipped the #640 live-campaign pointer
save_campaignsaved unconditionally.check_consequences/check_companion_arc/check_faction_arcs/world_tick— andscene_context, which delegates tocheck_companion_arcevery beat — allload → maybe-mutate → save, so a call that mutated nothing still bumpedupdated_atand flipped theactive_campaign_idresolver ("most-recently-updated == live") onto the inspected campaign. This is 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 the on-disk snapshot; byte-identical ⇒ return without writing or re-stamping. A genuine mutation differs ⇒ still stamps + writes (a real write is never dropped). Fixes all 92 call sites at the chokepoint for free.world_ticklegitimately mutateslast_tick_dayon its first per-day call (so it saves once), then no-ops on a same-day repeat — exactly why the fix is a byte-compare, not a blanket "never save".F08-3 — tolerant load dropped unknown keys, next save destroyed them
The tolerant load drops unknown top-level keys then the next save rewrites current-schema-only, destroying them permanently. Now the original bytes are stashed write-once into
campaigns/<id>/snapshot.pre-tolerant.json(fixed name + skip-if-exists — first-skew bytes are the valuable ones;_atomic_write; degrade-not-abort on backup failure), and the dropped key names are surfaced on the resume path (start_session/start_worldresume return aschema_driftblock) instead of being log-only.F08-4 — enumerators used the STRICT parse only
list_campaigns/campaigns_for_world/active_campaign_idused a bare strict parse, so a tolerant-loadable campaign was invisible to listings and to the #640 resolver (which then silently resolved 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 bricked recap/resume
A truncated final line (OOM-killed half-write) raised out of
read_log → read_log_all → recap_from_store → start_session's recap, hand-repair the only fix.read_lognow skips-and-warns the unparseable line;append_lognewline-heals when the file's last byte isn't a newline, so the poison can't grow by concatenating onto the torn fragment.Invariants upheld
os.replace); pure reads do not save / bumpupdated_at(the whole point of this wave).schema_driftis an additive optional output field; thepre-tolerantsibling file is inert to every glob (list_slotsglobsslots/*.json; listings readsnapshot.jsonby name). No schema or wire-contract change.scene_context).Tests
New
servers/engine/tests/test_store_persistence_robustness.py(24, red→green):check_*/ same-dayworld_tickrepeat does NOT changeupdated_atand does NOT flip the live pointer (two-campaign end-to-end); a firingcheck_consequences/ firstworld_tick/ any real mutation DOES still save + bump.start_sessionreturnsschema_drift; clean loads carry none.read_log/read_log_all;append_lognewline-heals without injecting a blank line on a clean append.Verification
test_store.py+test_slots.py(existing persistence): no regressions.qa/fast_gate.sh(Tier-0): PASS (188 deterministic).DO NOT MERGE — awaiting review.