Repository navigation
fix(tui): prevent reducer snapshot cursor regression - #11374
lawrencecchen wants to merge 3 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmux-tui/crates/cmux-tui-core/src/workspace_registry/session_journal.rs">
<violation number="1" location="cmux-tui/crates/cmux-tui-core/src/workspace_registry/session_journal.rs:935">
P1: When a fold and terminal retirement produce snapshots at the same cursor, `<=` still lets the late fold overwrite the newer retirement snapshot. Serialize snapshot mutation with persistence or add an ordering token; a nondecreasing cursor alone does not protect equal-cursor writes.</violation>
<violation number="2" location="cmux-tui/crates/cmux-tui-core/src/workspace_registry/session_journal.rs:935">
P1: The monotonic guard breaks the intentional reducer-state reset path. In `mux.rs:1172` the agent-roster loader writes `put_journal_reducer_state(AGENT_ROSTER_REDUCER_ID, VERSION, 0, &empty_snapshot)` specifically to clear a persisted snapshot when the journal has an unreplayable pruned gap (fail closed with an empty roster). Because the stored cursor is > 0, the new `WHERE old_cursor <= new_cursor` (0) evaluates false, the UPDATE is skipped, and the stale non-empty snapshot at the higher cursor stays persisted. `execute` still returns Ok (0 rows changed), so the "clearing the ... snapshot failed" handler never runs and the reset is silently lost. Exempt the explicit reset (new cursor == 0) from the guard, or route resets through a separate delete/clear that bypasses the monotonic check.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "INSERT INTO meta(key, value) VALUES(?1, ?2) | ||
| ON CONFLICT(key) DO UPDATE SET value = excluded.value", | ||
| ON CONFLICT(key) DO UPDATE SET value = excluded.value | ||
| WHERE COALESCE(CAST(json_extract(meta.value, '$.cursor') AS INTEGER), 0) |
There was a problem hiding this comment.
P1: When a fold and terminal retirement produce snapshots at the same cursor, <= still lets the late fold overwrite the newer retirement snapshot. Serialize snapshot mutation with persistence or add an ordering token; a nondecreasing cursor alone does not protect equal-cursor writes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmux-tui/crates/cmux-tui-core/src/workspace_registry/session_journal.rs, line 935:
<comment>When a fold and terminal retirement produce snapshots at the same cursor, `<=` still lets the late fold overwrite the newer retirement snapshot. Serialize snapshot mutation with persistence or add an ordering token; a nondecreasing cursor alone does not protect equal-cursor writes.</comment>
<file context>
@@ -931,7 +931,9 @@ impl WorkspaceRegistry {
"INSERT INTO meta(key, value) VALUES(?1, ?2)
- ON CONFLICT(key) DO UPDATE SET value = excluded.value",
+ ON CONFLICT(key) DO UPDATE SET value = excluded.value
+ WHERE COALESCE(CAST(json_extract(meta.value, '$.cursor') AS INTEGER), 0)
+ <= CAST(json_extract(excluded.value, '$.cursor') AS INTEGER)",
params![format!("journal_reducer.{reducer_id}"), value.to_string()],
</file context>
| "INSERT INTO meta(key, value) VALUES(?1, ?2) | ||
| ON CONFLICT(key) DO UPDATE SET value = excluded.value", | ||
| ON CONFLICT(key) DO UPDATE SET value = excluded.value | ||
| WHERE COALESCE(CAST(json_extract(meta.value, '$.cursor') AS INTEGER), 0) |
There was a problem hiding this comment.
P1: The monotonic guard breaks the intentional reducer-state reset path. In mux.rs:1172 the agent-roster loader writes put_journal_reducer_state(AGENT_ROSTER_REDUCER_ID, VERSION, 0, &empty_snapshot) specifically to clear a persisted snapshot when the journal has an unreplayable pruned gap (fail closed with an empty roster). Because the stored cursor is > 0, the new WHERE old_cursor <= new_cursor (0) evaluates false, the UPDATE is skipped, and the stale non-empty snapshot at the higher cursor stays persisted. execute still returns Ok (0 rows changed), so the "clearing the ... snapshot failed" handler never runs and the reset is silently lost. Exempt the explicit reset (new cursor == 0) from the guard, or route resets through a separate delete/clear that bypasses the monotonic check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmux-tui/crates/cmux-tui-core/src/workspace_registry/session_journal.rs, line 935:
<comment>The monotonic guard breaks the intentional reducer-state reset path. In `mux.rs:1172` the agent-roster loader writes `put_journal_reducer_state(AGENT_ROSTER_REDUCER_ID, VERSION, 0, &empty_snapshot)` specifically to clear a persisted snapshot when the journal has an unreplayable pruned gap (fail closed with an empty roster). Because the stored cursor is > 0, the new `WHERE old_cursor <= new_cursor` (0) evaluates false, the UPDATE is skipped, and the stale non-empty snapshot at the higher cursor stays persisted. `execute` still returns Ok (0 rows changed), so the "clearing the ... snapshot failed" handler never runs and the reset is silently lost. Exempt the explicit reset (new cursor == 0) from the guard, or route resets through a separate delete/clear that bypasses the monotonic check.</comment>
<file context>
@@ -931,7 +931,9 @@ impl WorkspaceRegistry {
"INSERT INTO meta(key, value) VALUES(?1, ?2)
- ON CONFLICT(key) DO UPDATE SET value = excluded.value",
+ ON CONFLICT(key) DO UPDATE SET value = excluded.value
+ WHERE COALESCE(CAST(json_extract(meta.value, '$.cursor') AS INTEGER), 0)
+ <= CAST(json_extract(excluded.value, '$.cursor') AS INTEGER)",
params![format!("journal_reducer.{reducer_id}"), value.to_string()],
</file context>
|
Closing as superseded by #11383. The successor explicitly includes this CAS snapshot work and fixes equal-cursor overwrite and cursor-zero reset behavior. No unique current-main change remains in this intermediate stack. |
This branch was successfully deployed
1 participant
Stacked on #11364.
Adds a regression test and a SQLite monotonic cursor guard so late reducer writes cannot overwrite newer snapshots. Hosted focused tests are required; no local cargo tests were run.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes reducer snapshot persistence in the TUI journal so a late write with an older cursor can no longer overwrite a newer snapshot.
ON CONFLICTguard that only updates when the incoming cursor is equal to or newer than the stored cursor.Written for commit 251a795. Summary will update on new commits.