Repository navigation
cmux-tui: a workspace commit journals only the workspaces it changes (#32408) - #34226
lawrencecchen wants to merge 2 commits into
Conversation
…anges (red, #32408) Each create-workspace commit restated every workspace in its journal record (WorkspaceCommitIntent::apply -> normalized_workspace_resource_deltas): its record and WAL bytes grew with the session (542 KB per create at 50 workspaces, 3434 KB at 500 in the classic import, STATUS 9750). The test makes 40 workspaces, then one more, and allows at most 2 workspace upserts for it (the new one and the one losing focus). It also folds the journal from the start and requires every workspace's name, index and focus to match the session snapshot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nges (#32408) The legacy workspace commit (WorkspaceCommitIntent::apply) wrote a journal record that restated every live workspace: an upsert of each with its name, index, focus and name provenance. Its rows were already written only when changed, but the record was not, so each create journaled the whole workspace list, and every session.events subscriber got it. The commit now reads what the journal states for each live workspace before it rewrites the rows (stated_workspace_values: name, position, name source and revision, from workspaces joined with resource_workspaces), and normalized_workspace_resource_deltas upserts only the workspaces whose stated value changes: a new one, a rename, an index move, the focus moving to or from it. Deletes are as before. A create at the end states 1 or 2 workspaces. Rule: R-DURABILITY DESIGN C2 (one change, one record of the change's size); R-DAEMON-STATE R5 (every op O(changed)). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
red: cmux-next checks (god files, concurrency, crash safety, l10n); NEW: green on the base b192896; do not merge until green; see lane-rules |
|
Found 8 test failures on Blacksmith runners: Failures
|
teamleaderleo
left a comment
There was a problem hiding this comment.
Review (codex gpt-6-astra high, next-review, since=20261103T205956Z): BLOCK at f816caa
BLOCK at f816caa
Visual verdict: no images; Leo tries the build
- [P1] [blocking] workspace_registry.rs:4562: Preserve upserts for presentation changes.
commit_workspace_presentationalso uses this path, but changing only color, icon, or title leaves every compared field unchanged. The upsert is dropped beforedecorate_changesadds the updated presentation fields, sosession.eventssubscribers retain stale metadata. The separate presentation record has no resource revision and is excluded from that stream. Ensure the affected workspace is emitted and verifystate::tests::raw_metadata_and_pin_commands_publish_the_same_state_on_session_events, which already checks this behavior. Cargo was unavailable here, so I could not run it.
|
Review (claude-reviewer-rds-owner): FIX at f816caa I read the code at f816caa ( Must fix1. A metadata commit no longer journals the workspace whose title, color or icon it changes. Failure:
The loss persists. The public fold catches up from the journal, so it keeps K's old value. A later projection restates K without presentation fields ( Fix: when an 2. The rows are not always what the journal states, so a row diff can skip an upsert the journal needs.
The fold version must meet two conditions, or it repeats these defects:
3. Clippy fails because of this PR. Points 1 and 2: what the diff compares
Point 3: consumersI found no reader that treats one workspace batch as the full list, or that gets the order from
A move or a close restates every workspace whose index changes, because each shifted row's stored position differs from its new index ( Point 4: the testIt is red at 3d16da2 ("journaled 41 workspace upserts", hq run 41703378498) and green at f816caa (hq run 41703378227, filtered to this test). It covers only a create at the end. The fold-from-start check is sound for creates: it reads one page (41 records; the page size is 1024) and is not vacuous. But it compares only Point 5: fit with R-DAEMON-STATE S3No, this PR does not keep one source for the change set. It adds a third way to decide what changed: the rows before the write, compared with the desired list. The two that already exist are the fold ( A row diff is a weak bridge. It is correct only if the rows equal the journal and the compared columns cover the whole decorated value; defects 1 and 2 show that neither is true today. A comparison of decorated values against the fold is an acceptable bridge: it compares with the journal itself, and the effect paths already use it, so one method stays in use until S5. What S3 and S5 should carry over:
S5 removes Point 6: costThe new SELECT reads each live workspace once (an indexed join) and adds no journal bytes. That is acceptable for now. The same transaction already reads every live row ( Point 7: ratchetsPass. The new product code uses |
|
Closed: superseded by #34959. |
… does not state (#32408) (#34959) * test(cmux-tui): a workspace commit journals only what it changes, and the journal still states the session (red, #32408) Each create-workspace commit restated every workspace in its journal record (WorkspaceCommitIntent::apply -> normalized_workspace_resource_deltas): its record and WAL bytes grew with the session (542 KB per create at 50 workspaces, 3434 KB at 500 in the classic import, STATUS 9750). Each test starts from 40 workspaces and one full topology projection (the journal's public fold seeded, as in a live session), then requires: - a create journals 1 or 2 workspace upserts (the new one and the one losing focus); - a move from index 30 to 10 journals the moved workspace and the 20 it shifts (21 or 22 upserts); - a rename journals 1 upsert; - a color change (set-workspace-metadata, extra rows) journals its workspace; - a resource-path move (workspace.move) then a create. After each, the journal folded from revision 0 must state every workspace's name, index, focus, title, color and icon as the session snapshot shows them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(cmux-tui): a workspace commit journals only the workspaces the journal does not state (#32408) The legacy workspace commit (WorkspaceCommitIntent::apply: create, rename, move, close, set-workspace-metadata) journaled every live workspace on each commit. It now prunes its workspace upserts against the journal's public fold, the one notion of what the journal states (R-DAEMON-STATE review, STATUS 9955 and #34226): - prepare (on the registry's thread) catches the fold up and copies its workspace values with the fold's revision; - apply decorates its candidate upserts as the journal append will (after the rows and the extra rows are written) and drops each one the fold states already. The commit's own workspace always goes when extra rows ran (a metadata commit changes only decorations). With the fold unseeded, or not at this commit's base revision, every workspace is stated, as before; - every workspace receipt records its journaled changes into the fold, so the fold stays at the head without reading the journal back. The fold, not the rows: a commit with project_resource false, a resource-path order change (set_resource_workspace_order) and the open-time name provenance rewrites change rows without a journal record, so a row diff could drop an upsert the journal never stated. Rules: R-DURABILITY C2 (one change, one record of its size); R-DAEMON-STATE R5 (every op O(changed)). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(cmux-tui): the fold comparison compiles; no redundant qualification (#32408) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(cmux-tui): prepare_workspace_commit takes the registry mutably (it catches the public fold up) (#32408) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(cmux-tui): seed the public fold as a live session does (a full resource effect projection) (#32408) commit_ordinary_full_resource_projection commits through the resource patch path, which never seeds the fold, so the fold-pruning fix pruned nothing in the tests. A full resource effect projection (what a session's first terminal create runs with an empty fold) restates every resource and seeds it. The helper now asserts the fold is seeded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(cmux-tui): a workspace commit leaves the public fold to catch up from the journal (#32408) Recording the commit into the fold at once made the fold current where it was behind before, so the scoped projection cross-check started comparing against it. It then saw a gap it could not see before: a workspace commit that moves focus does not restate the focused flag of the old and new active workspace's screens and panes (base behavior, filed separately). The fold now reads each workspace commit back from the journal at its next catch-up, as for every other commit; pruning is unchanged (prepare catches up first). Red on 31115d1: mux::tests::tabs::surface_session_subscription_tracks_real_tab_moves_without_layout_churn (step 18d0fc2492284dfff0c45429); green at base 1ccf109 (step a0d3dfd664612a27fbcb1534). * fix(cmux-tui): the workspace prune reads change fields without indexing; the batch estimate counts the fold copy (#32408) Review (claude-reviewer-rds-wcd) FIX at 06fce7f: the panic ratchet counted a new clippy::indexing_slicing hit in cmux-tui-core (854 > 853) at decorated_change["value"]["extra"]["key"]. Every change field is now read with get/pointer. estimated_bytes() also counts the stated workspace map that prepare copies (one value per workspace), so the writer's batch byte cap sees it. * test(cmux-tui): the base-revision guard, the own-workspace keep and a close in the middle; whole-value journal check (#32408) - a_workspace_commit_prepared_before_another_lands_states_every_workspace: a commit prepared before another lands states every workspace (the fold-revision guard). - a_workspace_pin_journals_its_workspace: a pin changes no public field, and the commit still states its own workspace (keep). - a_workspace_close_journals_every_workspace_it_shifts: a close at index 10 of 40 states the 29 shifted workspaces and deletes the closed one. - Every test now also checks that the journal, folded from revision 0, states each live workspace's whole value as its rows give it (workspace_value). * test(cmux-tui): the base-revision guard test passes every fence (#32408) The first version renamed a workspace without advancing its name revision, so the name fence refused it before the guard mattered (red at the head too, step 122d5ec7). Now an early commit moves workspace 30 to index 10 and a late one, prepared on the base, renames workspace 17 (revision + 1) and writes the base order back: it lands, and pruning against its stale fold copy would leave the journal stating the early move's order. --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Part of #32408. Fixes the biggest O(session) writer that the classic-import stall still had: each workspace create journaled every workspace.
Cause
WorkspaceCommitIntent::apply(the legacy workspace commit, used bycreate-workspace, rename, move and close) wrote its rows only when they changed. But its journal record came fromnormalized_workspace_resource_deltas, which restated every live workspace: an upsert of each one with its name, index, focus and name provenance. So one create journaled the whole workspace list, everysession.eventssubscriber got it, and the record's WAL bytes grew with the session. This path does not use the public fold's pruning (prune_stated_topology_deltas) that the effect paths use.Measured with the registry WAL frame counter (#34225) on aws-m4pro-7. The create command's own commit took 542 KB per create at 50 workspaces and 3434 KB at 500 (diag step 6f2a2544). In the 512 import, 931
session.eventsitems restated every workspace (up to 1 MB each).Fix
The commit reads each live workspace's current stored value (name, position, name source and revision) before it rewrites the rows. It then upserts only the workspaces whose value changes: a new one, a rename, an index move, or the focus moving to or from it. Deletes are as before. A create at the end states 1 or 2 workspaces.
This follows R-DURABILITY DESIGN rule C2 (one change, one record of the change's size) and R-DAEMON-STATE rule R5 (every op O(changed)). R-DAEMON-STATE asked me to fix this in my own PR (STATUS 9955) and reviews it for fit with its S3 writer plan.
Regression test
cmux-tui-core mux::tests::workspaces::a_workspace_create_journals_only_the_workspaces_it_changes: 40 workspaces, then one more. That create may journal at most 2 workspace upserts. The journal, replayed from revision 0, must state every workspace's name, index and focus exactly as the session snapshot shows them.one create at 40 workspaces journaled 41 workspace upserts.End-to-end (macOS, aws-m4pro-7):
WindowStateBytesPerCreateTestsmeasures all registry bytes per create, at 50 and 500 workspaces.The test still fails its 1.10x bound. The rest of the growth is the app's whole-document window-state save, which R-DURABILITY stage S-WS removes next. The end-to-end bound stays as it is.
Observed / Not run
cargo testof cmux-tui-core (the Mac lane split). The 512-import before/after pair runs after S-WS, so the import sees both fixes.Changelog
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Fixes workspace creation so it no longer journals every workspace in the session. Each create-workspace commit previously wrote an upsert for the entire workspace list, making the journal record and WAL bytes grow with the session size (542 KB per create at 50 workspaces, 3434 KB at 500). Now the commit reads each live workspace's stored value before rewriting rows and upserts only what changed: a new workspace, a rename, an index move, or the focus moving to or from one.
Adds a regression test that creates 40 workspaces then one more, asserting the create journals at most 2 workspace upserts and that folding the journal from revision 0 still reproduces every workspace's name, index, and focus as the session snapshot shows.
Written for commit f816caa. Summary will update on new commits.