Repository navigation
Conversation
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for measuring the marks so carefully. As written, though, this change can lose migrated settings for users who still have legacy .dat files, so I don't think it can go in as is. I couldn't run the desktop app here (headless Linux), so this comes from reading the code. The storage tests (29 of 29) and bun run typecheck in packages/desktop pass.
The early snapshot's legacy check only protects the window until the layer is up. Once importLegacyStores is forked, the layer finishes straight away, installs setStorageSnapshotProvider and opens the IPC port, and nothing waits for the import to commit. So the snapshot and StorageItems can serve empty namespaces, which the renderer then keeps for its lifetime. Worse, any write the renderer flushes before the import's transaction creates a row that onConflictDoNothing keeps, and the import then deletes the legacy file, so the user's old value is lost. Details are inline.
Public API: unchanged.
| : Effect.logInfo("imported legacy store files", { imported: result.imported, files: result.removed }), | ||
| ), | ||
| Effect.catch((error) => Effect.logWarning("failed to import legacy store files", { error })), | ||
| Effect.forkScoped, |
There was a problem hiding this comment.
Forking here lets the rest of this layer run before the import commits. setStorageSnapshotProvider is installed a few lines below, and from then on the snapshot handler no longer goes through readWindowSnapshot's "legacy files present → empty storage" guard. The IPC port, and with it StorageItems/StorageUpdate, is also handed out after the layers. So on the first launch with legacy .dat files:
StorageItems/ snapshots can return empty namespaces, and the renderer loads each namespace only once, so it keeps that empty state.- Any
StorageUpdatethat flushes before the import's transaction creates rows that the import'sonConflictDoNothingkeeps.importLegacyStoresthen deletes the.datfile, so the user's old value is lost for good.
If the import has to leave the layer's critical path, the storage handlers and the snapshot provider need to wait for it, e.g. by awaiting a Deferred before answering StorageItems/StorageUpdate and before swapping the snapshot provider. Another option is a cheap synchronous check, so the import only blocks when legacy files actually exist. When there are none, which is the common case, the cost is just the readDirectory.
Te-River
changed the title
|
Fixed — you were right, and the race was exactly as you described. The import stays forked; a
Regression test: It needs an Electron stub, so it adds
|
The first window is shown as soon as Electron is ready, and the layers are built on the same thread afterwards. The legacy store import reads and decodes the whole userData directory inside that build, holding the layers (and the IPC port) back. The import is a one-time migration of files the renderer no longer writes, and the early snapshot already handles the pre-import state (window-snapshot.ts checks for legacy files, covered by window-snapshot.test.ts). Fork it so the layers and the port are handed out first. Measured on the dev startup benchmark: the storage interval drops from 72ms to 42-47ms (median of 4 baseline runs vs 2 fixed runs).
Forking the import let the layer finish before it committed: a snapshot or StorageItems could serve an empty namespace the renderer keeps for its lifetime, and a write before the import's transaction would be kept by onConflictDoNothing while the import deleted the legacy file.
Add a Deferred the import completes (through ensuring, so a failure or interrupt still releases it). The snapshot provider and the state handlers await it; the layers and the port are still handed out first.
Regression test: with a legacy .dat present, the snapshot handler must wait for the import. It fails before this change (items: {} instead of the stored value).
d512bb2 to
77fc686
Compare
This branch has not been deployed
1 participant
Issue for this PR
Closes #54296
Type of change
What does this PR do?
The first window is shown as soon as Electron is ready (
windows/early.ts), and the main bundle and the Effect layers are built on the same thread afterwards. The legacy store import reads and decodes the whole userData directory inside that build, holding the layers — and the IPC port — back.The import is a one-time migration of files the renderer no longer writes, and the early snapshot already handles the pre-import state (
storage/window-snapshot.tschecks for legacy files;window-snapshot.test.tscovers it). This forks it, so the layers and the port are handed out first.How did you verify your code works?
Measured with the repo's dev startup benchmark (
bun run bench:devex), reading the main-process marks (lifecycle/marks.ts).The window→layers block is 280ms (median of 4 runs). Split: main bundle evaluation 152ms, storage 72ms, logging 34ms, onboarding 4ms. Inside storage: sqlite open + migrate 29ms, legacy import 21ms.
Forking the import takes the storage interval from 72ms to 42-47ms (4 baseline runs vs 2 fixed runs, reproducible in the marks).
The benchmark's headline metric (
visibleWindowToHome) does not resolve this: it is dominated by the dev-mode renderer (9.0-14.1s) and the isolated dev service (5.9-14.1s), and itswindowVisiblemilestone is the adoption log after the layers, so the 280ms block sits outside it. Paired deltas were +101ms / −1459ms. The marks are the evidence here.Two assumptions from the issue were measured and did not hold: theme resolution is 1.3ms (not "tens of milliseconds"), and onboarding is 4ms.
Machine (for comparability): AMD Ryzen 5 5600U (6C/12T), 13.8 GB RAM, AMD Radeon integrated graphics (driver 30.0.13044.0) plus a virtual display adapter, 1920×1080 @60Hz, Windows 11 Pro for Workstations build 26300, balanced power plan.
bun test src/main/storage(packages/desktop): 29 passed, 0 failed.Screenshots / recordings
n/a
Checklist