fix(workspaces): reduce repeated save work without losing pending changes#486
fix(workspaces): reduce repeated save work without losing pending changes#486morluto wants to merge 16 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
baron
left a comment
baron
left a comment
There was a problem hiding this comment.
Intent Analysis
Problem: Workspace autosaves performed full-workspace preparation (decode/merge/encode) per request before any coalescing, amplifying CPU during edit bursts (issue #469).
Before → After: Before, each save request independently prepared and encoded a payload; only encoded writes were coalesced at the disk writer. After, requests are debounced and coalesced per workspace before serialization (200 ms quiet period, 1 s max delay), payloads receive per-payload outcome receipts from the shared WorkspaceDiskWriter, and window-close/app-termination paths flush pending saves as durability barriers.
Evidence: Coordinator/debounce logic at WorkspaceManagerViewModel.swift:7287-7688; disk receipts at :6835-6891; close barriers at WindowState.swift:1326-1331, WindowStateManager.swift:917-925, AppDelegate.swift:159-175; tests at WorkspaceSaveCoordinatorTests.swift:12-206. Two independent code-first reviews converge on the same structure at head 357b1b6.
Confidence: High on intent and direction; medium on durability semantics — the reviews conflict there, and the conflict resolves in favor of the fully line-cited failure chain (Finding 1), which the merge-leaning review never examined.
Red Flags: .superseded receipts resolve at in-memory displacement time, not on the replacement's durable outcome. App test shards 1–3 and the Sentry-enabled build are unconcluded at head. Unresolved Sentry comment on combinedWorkspaceSaveCompletion (:7499-7512) is valid but non-blocking.
Findings
1. HIGH (blocking) — Premature .superseded resolution can certify unsaved state as durable. WorkspaceDiskWriter.enqueue resolves a displaced pending payload's receipt as .superseded immediately during coalescing (WorkspaceManagerViewModel.swift:6845-6869), before the replacement reaches disk. The outcome propagates (:7643-7647, :7675-7680); the coordinator treats same-version .superseded as saved (:7426-7449), and the close flush treats it as terminal success and stops retrying (:7480-7494). If the replacement write then fails (:7202-7214), a close barrier reports durability for state that never reached disk — concrete under cross-window or direct-save contention on the same workspace URL. Writer tests cover failed→success and success→failed but not a pending payload superseded by a payload whose write fails (WorkspaceSaveCoordinatorTests.swift:10-63).
2. MEDIUM (verify before merge) — Close flush may not revalidate versions mutated during the disk-write await. Stale-version checking is pre-enqueue (:7612-7625); post-commit, lastSavedVersionByWorkspaceID advances using the captured version without re-comparison (:7412-7425), and flushPendingWorkspaceSavesBeforeClose does not re-scan dirty versions before returning (:7467-7496). Blocking only if a mutation path bumps the state version without enqueueing a save request — unproven; verify or add a post-await re-check. The existing mutation test pauses before serialization, not during the writer await (WorkspaceSaveCoordinatorTests.swift:136-175).
3. LOW (non-blocking) — Coverage/observability gaps: no natural quiet-period/max-delay timer test (:7350-7401; all tests force .flushBeforeBoundary); lifecycle wiring (WindowState.swift:1326-1331, AppDelegate.swift:164-175) only indirectly covered; WorkspaceSaveDiagnostics.swift:69-97 omits the atomic-write duration requested by issue #469.
4. LOW (non-blocking) — Sentry's combinedWorkspaceSaveCompletion comment (:7499-7512): default case discards an earlier .committed; valid, but no current caller distinguishes it.
Maintainer-guidance check
- User impact/invariant: A save or close/termination barrier must not report success unless the captured-or-newer workspace state is durably on disk; Finding 1 violates this on a traced path.
- Root-cause confidence: Confirmed for the performance root cause (issue #469); confirmed-by-citation for Finding 1's chain.
- Authority:
WorkspaceManagerViewModelremains the save-preparation authority;WorkspaceDiskWriterremains the sole durable-write authority — no parallel persistence path introduced. - State-safety: Premature superseded receipts can lose final workspace state at close/quit — the highest-severity state class for this feature.
- Scale/observability: Coalescing is bounded (200 ms quiet/1 s max); write-duration diagnostics missing.
- Recommended scope: Fix in this PR — tie
.supersededresolution to the durable outcome of the draining replacement (or re-mark the version dirty on replacement failure); add the three-payload successor-failure test; verify or close the post-await revalidation window. Finding 3 items may be follow-ups. - Validation boundary: Focused
WorkspaceSaveCoordinatorTestsplus the new cases; multi-window close/quit smoke; all app shards and the Sentry-enabled build green at head.
Verdict
REQUEST_CHANGES. Two independent grounds: (1) Finding 1 is a fully line-cited, unrebutted durability defect on the exact invariant this PR hardens — the merge-leaning review never analyzed the superseded path, so the conflict resolves against it on evidence quality; (2) required checks (app shards 1–3, Sentry-enabled build) are unconcluded at head, so even the merge recommendation's own precondition is unmet. The optimization is well-targeted and should merge quickly once superseded-receipt semantics are tied to durable completion and the successor-failure case is tested.
Ensure combinedWorkspaceSaveCompletion keeps a successful commit when later workspace flushes return superseded or other non-failure states.
WorkspaceDiskWriter was resolving .superseded receipts immediately during enqueue coalescing, which let a failed replacement write appear as a durable success to callers. Keep superseded receipts in Pending.supersededReceiptIDs and resolve them when the replacement write actually finishes, using .failed if the replacement write failed and .superseded otherwise. Add a regression test where a pending payload is discarded in favor of a newer payload, and the newer payload's write fails; the discarded payload now reports .failed instead of .superseded.
|
Addressed the remaining Baron durability findings in pushed commits |
|
Fresh exact-head shard 3 exposed an unrelated slice-rebase registration race in |
|
Fixed the exact-head Style failure in pushed commit |
`lastSavedVersion` was only updated when no newer pending version was re-armed, so a close-boundary waiter could resume with `.committed(version)` while `lastSavedVersion` still lagged behind. Record the just-completed version at the start of the `.committed` case so an interrupted loop (cancelled worker or guard exit) always leaves `lastSavedVersion` consistent with what was actually persisted.
2 participants
Problem
Workspace saves were coalesced inside
WorkspaceDiskWriter, after disk loading and JSON encoding had already happened. Rapid updates could therefore prepare the same workspace repeatedly even when the writer later superseded most results.The save pipeline also did not preserve every writer failure through its drain path. Window close or app termination could finish while the newest state remained unwritten, or mark a workspace clean before the latest request was durably committed.
Fixes #469.
Approach
Coalesce before serialization, where the repeated work occurs, while retaining the shared writer as the authority for conflicts between windows.
A short quiet period absorbs ordinary editing bursts, a maximum dirty age prevents continuous activity from postponing saves indefinitely, and lifecycle boundaries drain immediately. Per-payload writer outcomes ensure that failed or cancelled work cannot be reported as saved.
Changes
Performance effect
Coalescing now happens before disk loading and JSON encoding.
The burst regression submits 100 save requests and verifies that they produce one initial preparation. If another request arrives while preparation is running, the coordinator performs one follow-up using the latest state instead of preparing every intermediate snapshot.
Ordinary saves wait for a 200 ms quiet period, with a maximum dirty age of one second. Switch, close, and termination flushes bypass that delay.
Testing
Coverage includes burst coalescing, requests arriving during preparation, same-version ordering, failed follow-up writes, drain outcomes, window close, app termination, and coordinator cleanup.
WorkspaceSaveCoordinatorTests: passedWindowCloseCoordinatorLifecycleTests: passedRepoPromptproduct build: passedNot included