Debounce persistence instead of writing on every command - #27
Open
Chirag6722 wants to merge 1 commit into
Open
Conversation
Fixes #1 persist() serializes the entire history, and every entry carries its full captured output, so with the defaults a single write is up to roughly 5 MB (maxEntries 50 x maxOutputBytes 102400). Doing that on the synchronous path of every recorded command means the cost scales with the whole history rather than with the one new entry, on a path that runs constantly while you work. Per the direction on #1, full output is still persisted. Only the timing changes: a write is queued and happens at most once per second. schedulePersist() deliberately does not restart the timer when a write is already queued. A trailing debounce that resets on every change would starve persistence during a burst of commands, which is exactly when there is most to lose. This way a burst costs one write, and no change waits longer than the interval. clear() still writes immediately: it is rare, deliberate and user-initiated, and "I cleared it" should be true on disk at once. flush() cancels any queued write and persists now, and dispose() runs it, registered in activate()'s subscriptions so a queued write is not lost to a window closing in the second after a command finished. Two existing tests asserted the old synchronous behaviour by reading workspaceState immediately after a command, so both now await store.flush() first. That is the same guarantee a real reload gets, since flush() is what dispose() runs on shutdown, and it avoids putting a sleep in the suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Fixes #1, taking the option you picked: keep full output persisted, just get the write off the synchronous path.
persist()serializes the entire history, and every entry carries its full captured output, so with the defaults one write is up to roughly 5 MB (maxEntries50 xmaxOutputBytes102400). Doing that after every recorded command means the cost scales with the whole history rather than with the new entry, on a path that runs constantly while you work.Now a write is queued and happens at most once per second. Nothing about what survives a reload changes.
The one design decision worth reviewing.
schedulePersist()does not restart the timer when a write is already queued. The obvious implementation is a trailing debounce that resets on every change, and that one starves persistence during a burst of commands, which is exactly when there is most to lose: keep running commands and the write keeps getting pushed out. This way a burst costs one write, and no change ever waits longer than the interval. There is a test for precisely that, since it is the failure mode you would never notice until you needed the data.clear()still writes immediately. It is rare, deliberate and user-initiated, and "I cleared it" should be true on disk at once rather than a second later.flush()cancels a queued write and persists now;dispose()runs it and is registered inactivate()'s subscriptions, so a queued write is not lost to a window closing right after a command finished.Two existing tests had to change, which is the part to look at
checklist.test.ts's reload test andrecording.test.ts's "step 5: persists to workspaceState" both readworkspaceStateimmediately after a command, so both encoded the synchronous-write behaviour. Both nowawait api.store.flush()first.I want to be explicit that this is a real semantic change and not just test churn: "persisted synchronously" becomes "persisted within a second, and flushed on shutdown." The property those tests exist to protect, that history survives a reload, is unchanged, and awaiting
flush()is the same guarantee a real reload gets, sinceflush()is whatdispose()runs when the window closes. But if you would rather the synchronous guarantee stay exactly as it was, this is the PR to say so on, and the answer would be to shrink the interval rather than to reshape the tests.Verification
New
persistDebounce.test.tscovers the burst case, the sustained-activity case that a resetting debounce would fail, the immediate write onclear(), and the flush ondispose(), all against a counting in-memoryMementoso it asserts write counts without touching real workspace state.I could not get a full local Windows run for this one: the runner refuses to start while another VS Code instance is open, and running it against an isolated
--user-data-dirnever brings shell integration up, so the terminal-driven suites cannot run that way. Rather than report a number I do not have, I am leaning on CI here, which runs the whole suite under xvfb. Happy to post a clean local run if you want one before merging.