Repository navigation
fix(cmux-tui-core): a tab order change validates its pane, not every tab of it - #36235
Conversation
…nged (red: it validates every tab of its pane)
…tab of it SetTabOrder writes only positions: set_resource_tab_order requires the exact live set and the contiguous-position check covers the order, so no tab's own rows change, as for SetWorkspaceOrder (validated the same way since the workspace-order fix). A chat tab create in a pane of N tabs ran validate_touched_tab N times, about 5 queries each, under the registry lock.
|
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)
Shipped in:
|
|
Found 3 test failures on Blacksmith runners: Failures
|
|
Review (independent review subagent, Claude): SHIP at 39fb532 A |
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Summary
Second cause of the base red
cmux-tui-core state::group_update_latency_tests::a_persons_requests_answer_while_busy_sessions_saturate_the_registry(a person's chat tab over its 200 ms p95 while busy sessions write; red in 9 of 31 full runs I have logs of). The first cause (the public fold never pruned restated chat tabs) is #36212.Cause (product): every tab create, move or close sends its pane's new order (
SetTabOrder), andvalidate_touched_resource_invariants(workspace_registry/resource_store.rs) put every tab of that order into the validation set:validate_touched_tabran once per tab of the pane, about 5 queries each, inside the writer transaction under the registry lock a person's next click waits on. A tab order change writes only positions:set_resource_tab_orderrequires the exact live set, and the contiguous-position check below covers the order, so no tab's own rows change.SetWorkspaceOrderwas already exempted for the same reason ("validating every workspace per create cost O(n)").Fix:
SetTabOrderadds only its pane to the validation set. Tabs the patch changes are still validated through their own changes (UpsertTab,TombstoneTab, their content).Commits: (1) red test
state::group_update_latency_tests::a_chat_tab_create_validates_only_the_tabs_it_changedwith a test-only counter onvalidate_touched_tab(a create in a 31-tab pane validated 31 tabs on the base); (2) the fix.Testing
42a31ec88055136f1de61666) and macOS (job1b0a8d844d2f77a79e413e2a): "one chat tab create in a 31-tab pane validated 31 tabs".allfilterresource(job90677b103a2f7c679c2b003d): 339 of 339, fmt pass, clippy pass; macOS filtertab(job87511a2fde88bde618fdf1fa): 779 of 779; panic ratchet (job1de28e5641b7f95052901e0a) pass; full Linux (job1276a059d6d618160a09ffcf): 17258 of 17258 pass, the latency test included.python3 scripts/cmux-next/prepush-gate.py: no new violations.Observed / Not run
SetTabOrderalso makes every tab of the pane a journal subject (session_journal.rscollect_patch_subjects: onejournal_subject_indexrow per tab per create).SetWorkspaceOrderdropped the same; changing it changes what a subject reader returns, so it is the owner's call.Changelog
none
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
A tab order change (
SetTabOrder) now validates only the pane it changed, not every tab in it. Previously each tab of the pane ran about 5 validation queries inside the writer transaction under the registry lock a person's next click waits on — a second cause of the redgroup_update_latency_testslatency test. An order change writes only positions, covered by the exact-live-set and contiguous-position checks, so no tab's own rows change; the same rationale already exemptedSetWorkspaceOrder.Adds a regression test with a test-only counter on
validate_touched_tab, asserting one create in a 31-tab pane validates only the changed tabs.SetTabOrderstill makes every tab of the pane a journal subject; changing that is left to the owner ofsession_journal.rs.Written for commit 39fb532. Summary will update on new commits.