Repository navigation
fix(codex): give each revert rollout its own observation source - #4366
ScriptedAlchemy wants to merge 4 commits into
Conversation
Codex thread/revert keeps the thread id and writes a new rollout beside the original, so one session has two live rollout files. The new suite module ingests both files and then rewrites the revert rollout. On master both tests fail: the revert rollout reads as a rewrite of the original, and messages from one or both files drop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The Codex source identity came from the session id alone. A thread that Codex reverted has a second live rollout with the same id, so ingest read each file as a rewrite of the other and retired the other file's messages. CodexRolloutFile reads the rollout id from the file name. An ordinary rollout keeps the shipped v2 source byte for byte. A revert rollout gets a source hashed from the session id and its rollout id, and it leaves the session transcript_path to the thread rollout so that projection does not refuse it as a collision. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
64a9cc5 to
6f0302e
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let rollout = CodexRolloutFile::of(path, &meta.session_id); | ||
| let canonical_source = rollout.source(&meta.session_id)?; |
There was a problem hiding this comment.
🔴 Shared messages lose their original rendering
When a shared Codex record moves within a revert rollout, codex_rollout_siblings accepts its changed rendering as lineage. The projector uses each record's byte offset for the message ordinal and source offset. The later rollout overwrites the original message, leaving its live observation inconsistent with the stored row.
Learn more
A native record ID hashes the record's JSON, not its place in the rollout. derive_canonical_projection_for derives the message ordinal and source offset from its byte range. classify_message_transition allows a newer observation in another generation of the same lineage to supersede the stored row. The other rollout's still-live projection can then fail verify_projection_rows_from_records.
Example: The thread holds an agent message at byte 150. A revert copies the exact JSON after an extra context record, placing it at byte 250. Both share the native record ID, but the later projection rewrites the shared message's ordinal and source offset from 150 to 250 while the thread still offers its observation.
Recommended fix: Define sibling output sharing using verified projection compatibility or select one canonical rendering without mutating it while both observations remain live. Keep alias sharing and message lineage consistent with the chosen policy; test copied records at different offsets and with different inherited contexts.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Real. A revert rollout a shipped binary had already ingested under v2 produced new observation ids with the same message ids, and the native alias plus same_projection_lineage refused them at admission and projection. The fix adds tracedecay_domain::codex_rollout_siblings (same provider, session id, scope and native record id across two keyed Codex sources) and consults it at the preflight alias check, the runtime anchor verification and the projection lineage, so both observations own the message and a retirement promotes the remaining owner; codex_rollout_sources.rs now seeds master's v2 rows through production admission for each upgrade order and the shared-record case.
A store the shipped binary wrote holds a revert rollout under the session's v2 source. A test-helpers entry point admits a rollout exactly as that binary did, so the suite seeds master's rows through production admission for each order the shipped binary could have produced: both rollouts imported together, the thread rollout first and then both, and a hook that admitted the revert rollout last. On the previous head the both-together upgrade shows only the thread rollout's replies, the thread-then-both upgrade shows none, and a record both rollouts hold disappears when the revert rollout is rewritten. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Message ids and native record ids derive from the session id and the record bytes, not from the source, so a revert rollout a shipped binary ingested under v2, or a record both rollout files carry, yields two observations for one record. The native alias and the projection lineage refused the second one. codex_rollout_siblings names the relation: same provider, same session id, two keyed Codex sources, same scope, same native record id. The three checks that already carry the Cline cross-source rule consult it: the global-db preflight alias check, which now preloads the alias owner's observation, the runtime anchor verification, and same_projection_lineage. Both observations own the projected message and a retirement promotes the remaining owner. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| for owner_id in alias_owners.values() { | ||
| if !refused_observation_ids.contains(owner_id.as_str()) | ||
| && seen_observation_ids.insert(owner_id.as_str().to_owned()) | ||
| { | ||
| observation_ids.push(owner_id.clone()); | ||
| } | ||
| } | ||
| let stored_observations = | ||
| read_stored_observations_from_snapshot(&snapshot, &observation_ids, OPERATION).await?; |
There was a problem hiding this comment.
🔴 Released alias owners block ingestion
When an alias points to retention-released evidence, alias_owners adds its tombstoned observation to the preflight decode. Retention keeps aliases and bindings after releasing their JSON. The entire admission batch fails, including otherwise valid observations.
Learn more
The retention pass replaces an eligible observation's JSON and its anchor JSON with __retention_released markers but retains their alias and binding rows; see retention markers and bindings. The new alias-owner lookup includes such IDs even when no write refers directly to them. read_stored_observations_from_snapshot decodes those tombstones as full observations and anchors and returns a storage error. A normal collision involving a released alias therefore becomes a batch-wide read failure.
Example: A month after a deleted record's evidence is released, an incoming observation requests its retained native alias. Preflight decodes the tombstone and rejects the entire batch before evaluating that alias or any unrelated writes.
Recommended fix: Exclude released alias owners from full-object hydration while retaining their alias IDs for collision decisions. Hydrate only live Codex owners that can legitimately share the alias, and handle a released owner as an ordinary non-shareable collision.
Was this helpful? React with 👍 or 👎 to provide feedback.
| #[cfg(any(test, feature = "test-helpers"))] | ||
| pub async fn try_admit_codex_rollout_under_shipped_v2_source_for_test( | ||
| path: &Path, | ||
| project_root: &Path, | ||
| project_id: ProjectId, | ||
| admission: &dyn HostAdmission, | ||
| ) -> TranscriptIngestResult<CodexJsonlAdmissionProgress> { | ||
| try_admit_codex_jsonl_observations_as( | ||
| |_, _| CodexRolloutFile::Thread, | ||
| path, | ||
| CodexObservationAdmission::Project { | ||
| root: project_root, | ||
| project_id, | ||
| }, | ||
| admission, | ||
| None, | ||
| None, | ||
| &ObservationCancellation::default(), | ||
| ) |
There was a problem hiding this comment.
🟡 Test-only Codex ingestion port
try_admit_codex_rollout_under_shipped_v2_source_for_test exposes a second ingestion route solely under test-helpers. Repository rules forbid test-only production ports; the upgrade fixture bypasses the shipped source-selection path.
Learn more
The helper injects CodexRolloutFile::Thread to force the historical source for every rollout. It is exported from the production sessions crate behind the test-helpers feature and used by the new integration tests. AGENTS.md explicitly disallows test-only production ports and requires real production journeys rather than gates built only for acceptance.
Example: A revert rollout normally selects its own source, but a test calling this helper directly ingests it through the thread source, a route no shipped current binary uses.
Recommended fix: Construct upgrade evidence through an actual released binary or an isolated persisted fixture created from its real storage authority, without adding a second ingestion entry point to the sessions crate.
Was this helpful? React with 👍 or 👎 to provide feedback.
This branch has not been deployed
1 participant
Fixes #4328
Why
Codex
thread/revertkeeps the thread id and writes a new rollout,rollout-<ts>-<thread>_<rollout>.jsonl, next to the original. TraceDecay built the observation source from the session id alone. Both files therefore shared one source cursor, ingest read each file as a rewrite of the other, and each pass retired the other file's messages.What changed
CodexRolloutFilereads the rollout id from the file name, the same way Codex'sRolloutFileNamedoes. An ordinary rollout keeps the v2 source byte for byte. A revert rollout gets its own source, hashed from the session id and its rollout id, and leaves the sessiontranscript_pathto the thread rollout.tracedecay_domain::codex_rollout_siblingsnames that relation (same providercodex, same session id, two keyed sources, same scope, same native record id). The three places that already carry the Cline cross-source rule now consult it too: the global-db preflight alias check (accepts_codex_rollout_sibling_alias, which preloads the alias owner's observation), the rusqlite runtime anchor verification (shared_alias_is_valid), andsame_projection_lineage. The first anchor keeps the native alias; both observations own the projected message, andretire_source_recordpromotes the remaining owner when one file's rewrite drops its copy.test-helpersentry point,try_admit_codex_rollout_under_shipped_v2_source_for_test, admits a rollout exactly as the shipped v2 binary did, so the upgrade tests seed master's rows through production admission.Scope
SourceRecordRetired), and a re-offered exact duplicate is never queued again, so neither this PR nor a per-file cursor inside one source can revive those rows. The upgrade tests assert that bound. Reviving re-offered retired records needs a re-projection path below the projection checkpoint and affects every provider; it is tracked separately.transcript_path.Design
prove_cline_native_source_transitionsits behind the same three checks), and an observation id isdigest(source, scope, native_record_id), so a shared record is two observations that can co-own one message.observation_source_presenceandobservation_source_rewritesare keyed by(source, scope); a file component changes shipped keys and needs a migration of the existing v2 rows, and under one source a record both files carry is one observation id, so presence cannot say which file still holds it.Blast Radius
Only rollouts named
rollout-<ts>-<thread>_<rollout>.jsonlwith a rollout id other than the thread id get a new source; a unit test pins the shipped v2 bytes for every other name. The sibling rule fires only for providercodex, the same session id, two distinct keyed sources, the same scope and the same native record id; the global-db preflight additionally requires the two anchors to share owner, aliases, payload access, retention, durability and authorization. Cline behaviour is unchanged; its field comparison moved intoanchors_share_accessand is reused as is.Verification
transcript_ingest_suite,codex_rollout_sourceson head 6f0302e plus the new tests: 3 passed, 4 failed. Both-together upgrade showed only the thread rollout's 2 of 4 replies; the shared-record case lost the record the thread rollout still held; the thread-then-both order ended with no messages before and after; the hook-last order kept the revert rollout's replies. Fixed: 36 passed, 0 failed on the fullcodexfilter (onecodex_compactiondaemon-effect failure on the first run passed on the identical binary).tracedecay-sessions:unit_test(codexfilter): 86 plus 1 passed, including the shipped v2 byte pin.tracedecay-global-db:unit_test(observation_filter): 75 passed; the first sweep failed threeobservation_collision_testsbecause the alias-owner preload decoded a refused write's own corrupt row, and the preload now skips refused ids.tracedecay-rusqlite-runtime:unit_test(observation): 21 passed.tracedecay-domain:unit_test(observation): 3 passed.codex_compaction::codex_post_compact_hook_commits_app_server_summary_through_daemon_effectfailed on 2 of 4 suite runs atsummary_node_count0 vs 1 afterdrop(daemon), withraw_message_countpassing on the same status read. Alone (--runs_per_test) it fails 4 of 4 on this head and 2 of 2 on the previous head 6f0302e in the same worktree, so it predates this change and is not covered by it.--config=clippy) clean on the domain, global-db, rusqlite-runtime and sessions libraries, the sessions unit test and the transcript suite.🤖 Generated with Claude Code