Repository navigation
VC-831: release page-scoped sign-ins and file/dir/worktree watches when the renderer page reloads or crashes - #922
Open
hussainph wants to merge 3 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…-reload-or-crash-strands-per-window-main
…-reload-or-crash-strands-per-window-main
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.
Defect
A reload (View → Reload / Force Reload, and the only recovery from a renderer crash) keeps the same
WebContents. Three main-side owners released their per-window resources only ondestroyed, so they outlived the page that asked for them:main/model-access/ipc.ts): the per-windowSignInOwnerwas abandoned only ondestroyed. If the page reloaded mid sign-in, the attempt kept the provider's single attempt slot. The reloaded page no longer knew the attempt id, so every further Sign in answered "This provider is already signing in.", and the flow's loopback callback server (fixed ports 53692 for Anthropic and 1455 for ChatGPT in pi-ai 1.0.0) stayed bound until the window closed.main/volli-fs-ipc.ts→ host-coreFileWatchManager/DirWatchManager): subscriptions are refcounted per${client.id}:…. After a reload the new page re-watched the same key, so refcount went to 2. Its own unwatch left 1, and thefs.watchhandle never closed. A watch the new page never re-opened was never released.main/data-ipc.ts→WorktreeChangeWatchManager): every ticket rail opened before the reload kept its recursive watcher and git-ignore refreshes while the window lived.Fix
main/document-lifetime.ts:onDocumentGone(contents, listener)fires once when the page goes away, then detaches all three hooks. "Gone" meansdid-navigate(committed main-frame navigation),render-process-gone, ordestroyed.main/client-event-sink.ts: newpageClientEventSink(contents). It is the same id and channel asclientEventSink, butonceClosed/removeCloseListenerfollow the page rather than the window.isClosedstill means the window is destroyed, because a reloaded page is a live client. The file, folder and worktree-change watch channels now use it. Nothing else does.main/model-access/ipc.ts: the owner map entry and its attempts are abandoned throughonDocumentGone. The abandoned flow aborts, which closes its loopback server, and the provider slot frees. A gone owner also stops sending. Otherwise the cancelled attempt'ssettledupdate would land on the reloaded page, which never began it and would hold it unclaimed forever.How the regression tests prove it
These tests use fake
WebContents(realEventEmitters) that emit the page events. They fail onmain: 7 failures with the four production files reverted toorigin/main, and the tests pass with this change.model-access/ipc.test.ts: realModelAccessSignInServicewith a scripted flow shaped like pi-ai's OAuth flow. It listens on a real loopback server, parks on a pasted-code prompt, and closes the server infinally. Fordid-navigateandrender-process-gone, the test checks that the old server stops listening, that a newbeginfor the same provider succeeds, and that no update for the old attempt reaches the page. An in-page navigation or a started-but-refused navigation (did-start-navigation) keeps the attempt. Window destruction still frees the slot for the next window.volli-fs-ipc.test.ts: dir watch → reload (did-navigate/render-process-gone) → watch again → unwatch closes everyfs.watchhandle. Same for a file watch. Adid-navigate-in-pageordid-start-navigationreleases nothing.data-ipc.test.ts: the worktree-change-watch door hands the manager a page-scoped client:did-navigatecloses it, an in-page navigation doesn't.client-event-sink.test.ts:pageClientEventSinksemantics, plus the realWorktreeChangeWatchManager. A rail opened before a reload and never re-opened has its recursive watcher closed. Re-open then release after a reload closes both watchers. An in-page navigation keeps it.document-lifetime.test.ts: fires once on each of the three events and detaches all hooks. A started or in-page navigation fires nothing. Dispose is idempotent.Decisions
did-navigate, notdid-start-navigation. The ticket proposed Session RPC'speerOfsemantics. As the concurrency review on the ticket points out,did-start-navigationfires before main'swill-navigatecan refuse a navigation, and main refuses every non-internal main-frame navigation. Copying it would release a live page's sign-in and watches on a refused navigation. This PR uses thedid-navigate+render-process-gone+destroyedtrio thatbrowser/plane-reset.tsalready uses.did-navigatearrives at commit, before the new document's scripts run, so it cannot sweep a fresh page's own begin or watch.HostClientEventSinkmember. Host-core and its port contract are unchanged. Only the watch channels opt in. PTY IPC still usesclientEventSink(window lifetime), per VC-425.isClosedunchanged. It still reports window destruction, so host-core's "closed client" short-circuits behave as before for a live, reloaded page.Not in this PR
peerOfandnotifications.forgetRendereronto the same helper, which the ticket comment suggests. That is outside this ticket's Proposed fix, and thedid-start-navigationhazard there is latent and unreproduced.Tests run locally
vp test run src/main/model-access/ipc.test.ts src/main/client-event-sink.test.ts src/main/document-lifetime.test.ts src/main/volli-fs-ipc.test.ts src/main/data-ipc.test.ts src/main/pty.test.ts src/main/browser/plane-reset.test.ts(apps/desktop): 7 files, 548 tests passed.origin/main'smodel-access/ipc.ts,volli-fs-ipc.tsanddata-ipc.ts: 7 failed.vp checkon the 10 touched files: clean.vp run --filter @volli/desktop typecheckand rootvp exec tsc --noEmit -p tsconfig.json: clean.CI history
Check + Build. That failure was a lint error main itself had (react(purity)inupdates-pane.tsx), fixed on main by VC-294: compute the Updates status clock outside render (unbreaks main: react/purity) #923.Check + Buildfailed on a TS2353 type error main itself had in host-core tests, fixed on main by VC-786: drop the removed git port from tests merged beside #908 (unbreaks main typecheck) #925.Test (packages)also had two failures in host-core tests this PR does not touch. One was a timeout inworktree/watch.test.ts(VC-836 poll test); the other wascrash-recovery.integration.test.tsreturning arejectedreceipt. The same job passed on main at the time, and both pass in the next run with no change to them, so they were flaky.CI gategreen. That was a fresh run on a new head, not a rerun of failed jobs.