Repository navigation
Fix flaky StorageMainService tests by awaiting storage warm-up - #341305
Merged
Ulugbek Abdullaev (ulugbekna) merged 1 commit intoOct 13, 2026
Merged
Ulugbek Abdullaev (ulugbekna) merged 1 commit into
Ulugbek Abdullaev (ulugbekna) merged 1 commit into
Conversation
StorageMainService starts initializing application and application-shared storage in the background once a window has opened, which TestLifecycleMainService reports immediately. Tests that never awaited this warm-up could dispose the service while ApplicationSharedStorageMain was still awaiting application storage init. It then registered a MigratingStorage and a change listener on the disposed store during the next test, failing that test's leak check. Await the in-flight warm-up in the suite teardown before disposing. Fixes #341273 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused teardown change correctly joins the background initialization race without altering production behavior.
0 open findings
What changed in this PR
Ensures storage warm-up completes before test teardown disposes services, preventing cross-test disposable leaks.
Changes:
- Tracks all created test storage services.
- Awaits application and shared-storage initialization before cleanup.
| File | Description |
|---|---|
src/vs/platform/storage/test/electron-main/storageMainService.test.ts |
Coordinates asynchronous storage warm-up with teardown. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Screenshot ChangesBase: Changed (1)
|
| Before | After |
|---|---|
 |
 |
Aiday Marlen Kyzy (aiday-mar)
approved these changes
Oct 13, 2026
Ulugbek Abdullaev (ulugbekna)
deleted the
ulugbekna/agents/investigate-vscode-issue-341273
branch
October 13, 2026 15:55
Don Jayamanne (DonJayamanne)
added a commit
that referenced
this pull request
Oct 13, 2026
…xploration-vscode * origin/main: (40 commits) MSRC fixes (#341342) chat: render input attachments for the input model's session (#341350) chat: center subagent tool activity hooks (#341258) Manage Copilot plugin marketplaces through Agent Host (#341301) fix: linux setuid sandbox helper permissions (#341292) agentHost: resolve #chat chips on the host of the chat that shows them (#341332) [Perf] agentHost: limit archived sessions over Mission Control (#341327) Remember expanded state for "Other Models" in model picker (#341335) Offer Stop for copilot detached background shells (#341310) agentHost: allow inherited peer chats to move workspaces (#341317) [Perf] agentHost: reduce Mission Control relay overhead (#341223) Fix Customizations modal spacing (#341144) Fix dependency cycle in QuickInputService and EditorResolverService (#341321) Fix Explorer refresh on restoring deleted workspace root folder (#341323) chat: Share and rate-limit core utility model requests (#340100) agentHost: open #chat chips on the connection that owns the chat (#341313) Fix flaky StorageMainService tests by awaiting storage warm-up (#341305) agentHost: record the default chat role instead of reading the default chat URI shape (#341293) welcome: preserve tab icon color with Modern Icons Fix flaky test in diskFileSystemProvider.test.ts (#341304) ... # Conflicts: # src/vs/platform/agentHost/node/agentService.ts # src/vs/platform/agentHost/node/protocolServerHandler.ts # src/vs/platform/agentHost/test/node/protocolServerHandler.test.ts # src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostSessionHandler.ts
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 #341273
Problem
StorageMainServicestarts initializing application and application-shared storage in the background once a window has opened, andTestLifecycleMainService.when()resolves immediately, so every service this suite creates kicks off that warm-up. Some tests, such asbasics (profile)andbasics (workspace), never await it. When teardown disposes the service first,ApplicationSharedStorageMain.doCreate()later resumes afterawait this.applicationStorage.init()and registers aMigratingStorageand its change listener on an already-disposed store, during whichever test runs next.That next test then fails both
after eachhooks: the leak check (There are 2 undisposed disposables!) and the console-output check, which trips on theDisposableStorewarning. In the CI run linked from the issue, the victim wasstorage channel compareAndSwap isolates keys and storage scopesbecause it runs right afterbasics (workspace). The race has existed since application-shared storage was added in #311317.Fix
This is a test-only change. The suite now tracks every
StorageMainServiceit creates, and itsteardownawaits the in-flightapplicationStorage.init()andapplicationSharedStorage.init()before disposing. It does not usewhenInit, because that never settles when storage is closed before init (as instorage closed before init works).How to test
./scripts/test.sh --run src/vs/platform/storage/test/electron-main/storageMainService.test.ts: passed (10 passing).npm run eslint -- src/vs/platform/storage/test/electron-main/storageMainService.test.ts: passed.The race did not reproduce naturally on macOS in 9,500 local suite iterations. To reproduce it deterministically, I used a temporary local patch (not part of this PR) that delays
ApplicationStorageMain.doInitbyREPRO_APP_INIT_DELAYms and repeats the suiteREPRO_REPEATtimes:REPRO_APP_INIT_DELAY=30 ./scripts/test.sh --run src/vs/platform/storage/test/electron-main/storageMainService.test.ts --grep "basics \(workspace\)|storage channel compareAndSwap": failed before the fix (2 failing, with the same leak and console-output failures as CI); passed after (2 passing).REPRO_APP_INIT_DELAY=30 ./scripts/test.sh --run src/vs/platform/storage/test/electron-main/storageMainService.test.ts: failed before the fix (basics (application shared)hooks, afterbasics (profile)); passed after (10 passing).REPRO_APP_INIT_DELAY=10 REPRO_REPEAT=50 ./scripts/test.sh --run src/vs/platform/storage/test/electron-main/storageMainService.test.ts: passed with the fix (451 passing).REPRO_REPEAT=1000 ./scripts/test.sh --run src/vs/platform/storage/test/electron-main/storageMainService.test.ts: passed with the fix (9,001 passing).Not run:
npm run typecheck-client. Validation ran before rebasing ontomain; the 3 commits picked up by the rebase do not touch storage or test infrastructure.