Harden KclManager against multiple writers#10918
Open
franknoirot wants to merge 4 commits intomainfrom
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
991afaf to
38c74c6
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
38c74c6 to
0000e44
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Codex-assisted audit and hardening meant to close #10809 and other bugs reported in Discord and Slack around code loss when the app is open with multiple instances. Stacked on top of #10750 because that PR augments the editor history and change flows already, so I wanted to build upon that. I'm gonna do a bit of an info dump here from my Codex session.
Prompt
We have a number of issues with shaky editor state in KclManager.ts. #10809 is a related bug, but we've also got reports that typing in the code editor, which triggers a number of effects that asynchronously execute the code, can result in a jittery typing and selecting experience. When users run multiple instances of the app, which they sometimes do to run multiple projects or multiple AI agents with it, they have reported suddenly losing their work with no way to recover it.
I suspect that a lot of this is due to the many ways that we can update the canonical CodeMirror editor state, which drives so much of our other systems. We have
updateCodeEditor, andupdateEditorWithAstAndWriteToFile, and probably more ways I'm not aware of.Could you please analyze KclManager.ts, write integration tests that exercise it for these problematic "multiple competing updates" code paths within KclManager.spec.ts, and write me a report on how we might harden this system to make unrecoverable loss of code impossible?
Initial response and report
Added integration coverage in
KclManager.spec.ts#L203for the main competing-update paths:updateEditorWithAstAndWriteToFilestill refuses to replace code with an empty ASTI also made a narrow hardening change in KclManager.ts#L673, KclManager.ts#L1118, KclManager.ts#L1185, and KclManager.ts#L2328:
KclManagernow tracks the last disk-synced buffer and skips automatic watcher reloads when the editor has unsaved local changes, which closes the most direct “second app instance overwrote my work” path.Verified with:
npx vitest run --mode=development src/lang/KclManager.spec.tsnpx eslint src/lang/KclManager.ts src/lang/KclManager.spec.tsHardening Report
The current architecture still has one structural weakness: CodeMirror document updates, execution, and file persistence are coupled by side-effect listeners rather than one serialized mutation pipeline. That means the canonical buffer can still be affected by multiple producers with different clocks: user typing, AST recasts, watcher reloads, undo/redo effects, and delayed writes/executions.
To make unrecoverable loss impossible, I’d harden it in this order:
documentVersionand require every async path to carry it. Execution results, watcher reads, AST recasts, and writes should only commit if their version still matches the live buffer.updateCodeEditor,updateEditorWithAstAndWriteToFile, file-watch reloads, and undo/redo should all funnel through the same provenance-aware transaction API.The related GitHub issue, #10809, opened April 3, 2026, describes sketch-edit flicker followed by code-pane deletion. That matches the broader pattern here: multiple async systems are allowed to treat the current editor buffer as writable state instead of treating it as an owned versioned document.
Final response after some additional prompts to stack this work on top of #10750
Implemented the three hardening steps in KclManager.ts.
KclManagernow maintains a monotonic document revision at KclManager.ts#L689 and uses it to drop stale async completions. That now guards:updateEditorWithAstAndWriteToFile()at KclManager.ts#L3019Disk writes are now CAS-style at KclManager.ts#L2927: before writing,
KclManagerre-reads the file, skips the write if disk changed since the last sync, and avoids clobbering newer external contents.There is now a local recovery journal keyed by file path. Unsynced edits are persisted as the user types at KclManager.ts#L1578, and
fromFile()restores that recovery buffer on reopen at KclManager.ts#L1474, while still treating the actual disk contents as the sync base.I added focused regressions in KclManager.spec.ts#L400, KclManager.spec.ts#L428, and KclManager.spec.ts#L454 for:
Verified with:
npx vitest run --mode=development src/lang/KclManager.spec.tsnpx eslint src/lang/KclManager.ts src/lang/KclManager.spec.tsThis materially changes the risk profile: late async work can no longer blindly win, external disk changes won’t be overwritten silently, and unsaved text now survives process loss.