Repository navigation
Commit 997f5d8
fix(webview): detect dead webview renderer via heartbeat and auto-reload (#1715)
* fix(webview): detect dead webview renderer via heartbeat and auto-reload
* fix(webview): scope recovery reload to the provider's own webview
* fix(webview): stop recovery watchdog when sidebar webview is disposed
* test(webview): capture sidebar dispose callback in task history spec
* fix(webview): invalidate in-flight recovery reload on dispose or replace
The watchdog fires reloadWebviewForRecovery() without tracking it, so the
recovery could reassign webview.html after the provider was disposed or the
watched view was disposed/replaced while the HTML was being generated.
Track the operation with an epoch token: capture it before the await and
bail out before assigning webview.html when the provider is disposed, the
epoch changed, or this.view no longer references the captured view.
clearWebviewResources() bumps the epoch so sidebar view disposal and
provider disposal invalidate any in-flight recovery.
Also add fake-timer watchdog coverage for the tab-panel branch
(WebviewPanel shape): a hidden tab does not reload, the
onDidChangeViewState callback resets the heartbeat grace window when the
tab becomes visible, and a stale heartbeat reloads a visible tab.
* test(webview): cover recovery rejection and watchdog precondition
Address two minor review threads on the watchdog spec:
- Cover the reload rejection path: stub the recovery HTML regeneration to
reject and assert the failure is logged and webview.html is untouched.
- Assert the watchdog interval was actually scheduled before the sidebar
disposal test disposes the view, so the post-disposal null check proves
the watchdog was stopped rather than never having started.
* test(webview): assert global webview reload command is not used in provider isolation
What: spy on vscode.commands.executeCommand in the multi-provider watchdog
isolation test and assert workbench.action.webview.reloadWebviewAction was
never invoked while the stale-heartbeat watchdog fires.
Why: the pre-PR recovery path fired that global command, which resets every
webview. The html-only assertions still pass under a regression back to the
global command because the vscode mock swallows executeCommand without
touching any view, so the test could not fail for the exact bug class it
guards (code-reviewer P2 F-01 on PR #1715).
* fix(webview): harden recovery reload and scope sidebar disposal cleanup
What: The recovery reload captures lastWebviewHeartbeatAt when recovery
starts and, after the awaited HTML generation, additionally requires the
heartbeat timestamp to be unchanged and the captured view to still be
visible before reassigning webview.html. The sidebar onDidDispose handler
now runs clearWebviewResources() and the codeIndexManager reset only when
the disposed view is the currently watched one. Cancellation specs defer
getWebviewHtml() itself (instead of getState() with a fabricated state)
so they prove disposal and replacement land while recovery is in flight,
and new specs cover heartbeat-during-recovery and hide-during-recovery
skips.
Why: A heartbeat arriving during recovery means the renderer is alive
again, and a view that hides mid-recovery no longer proves a dead
renderer (hidden webviews throttle timers), so reloading in either case
needlessly reloads a healthy or throttled webview. This mitigates the
security audit finding LOW-1 (self-reload loop when the postMessage
bridge is half-broken): a renderer that resumes heartbeating during the
recovery window is no longer reloaded on every watchdog tick.
Disposing an outdated (replaced) sidebar view previously wiped the
replacement view's resources and code index manager; scoping the cleanup
to the watched view keeps the replacement intact.
* test(e2e): poll restart task-history presence before asserting
What: runVerify now gates the Task should be present after restart
assertion on a bounded waitForTaskInHistory poll that uses the same
api.isTaskInHistory read as the assertion, instead of a single
unguarded read immediately after isReady.
Why: the e2e-mock lane failed on da8d752 with exactly this
assertion (false !== true) while the same commit passed 6/6 local
runs and the changed production paths cannot execute during the
scenario (watchdog tick 60s vs ~2s phases; the sidebar dispose
scoping is behavior-identical for the single watched view). The
read races the persisted state becoming observable on a fresh host,
the same swap-window failure class documented in #1641 for this
scenario, which #1663 fixed for the conversation-history reads but
left the isTaskInHistory read single-shot. Polling with the same
read removes the false negative without touching production code,
keeping the CodeRabbit-requested recovery and dispose guarantees
intact.
* fix(webview): guard recovery with heartbeat revision and in-flight flag
What: The recovery reload now compares a webviewHeartbeatRevision counter
(captured before the awaited HTML generation against the value after)
instead of comparing heartbeat timestamps, and reloadWebviewForRecovery
skips starting when webviewRecoveryInFlight is already set, clearing that
flag in a finally block so a failed generation cannot block future
recoveries. updateWebviewHeartbeat bumps the revision on every accepted
heartbeat. New specs cover the same-millisecond double-heartbeat case and
the no-second-recovery-while-in-flight case including the reject path.
Why: Two heartbeats landing in the same millisecond advanced the old
timestamp not at all, so a recovery that started before them could still
replace a page whose renderer had reported back twice; a monotonic counter
catches every accepted heartbeat. Without the in-flight flag, every 60s
watchdog tick during a slow HTML generation started another recovery for
the same view, piling forced reloads onto it; the flag limits the view to
one recovery at a time while the finally block keeps one failed attempt
from disabling recovery permanently. The existing webviewRecoveryEpoch
checks stay in place: the epoch invalidates completions after dispose or
view replacement, while the in-flight flag only serializes generation
starts, so the two guards address different races and both remain.
* fix(webview): scope recovery in-flight guard to epoch
What: The in-flight guard for recovery reloads now records which
webviewRecoveryEpoch owns the pending recovery
(webviewRecoveryInFlightEpoch) instead of a plain boolean. A new recovery
is only blocked when the pending one belongs to the same epoch, and the
finally block releases ownership only if the completing recovery still
holds it. Specs: the same-millisecond heartbeat spec captures the
pre-spy timestamp, feeds it to the Date.now spy, and asserts the stamp is
unchanged after each heartbeat; the retry spec resolves the second
getWebviewHtml call with distinct HTML and asserts it lands in the
webview; a new spec covers recovery A pending, view disposed/replaced
(epoch bump), recovery B starting immediately, and A's finally not
releasing B's ownership.
Why: With a boolean flag, a recovery pending on a disposed or replaced
view blocked the replacement view's recovery until the stale generation
settled, delaying recovery of the live view by a whole generation window.
Scoping ownership to the epoch keeps the pile-up protection for the same
view while letting the replacement's recovery start immediately; the
conditional finally clear stops a stale completion from releasing
ownership the replacement recovery took. The per-heartbeat timestamp
assertions prove both same-millisecond heartbeats leave the recovery-start
stamp unchanged, which is exactly the case the revision counter exists
for, and the retry HTML assertion proves a post-failure recovery actually
replaces the page content.
* fix(webview): bump recovery epoch when resolveWebviewView replaces the view
What: resolveWebviewView now advances webviewRecoveryEpoch (the same
increment clearWebviewResources uses) when the incoming view is a
different object than the currently watched one, before assigning
this.view. Re-resolving the same view and the first resolve leave the
epoch unchanged. New specs: a replacement resolve without an intervening
disposal advances the epoch, lets the replacement view's recovery start
while the stale one is still pending, keeps the stale completion from
reassigning the old view's html or releasing the replacement's in-flight
ownership, and lands the replacement's HTML; a same-view re-resolve
asserts no epoch bump and that the pending recovery's ownership still
blocks a second recovery until it settles.
Why: Without the bump, a recovery pending on the old view kept the
epoch-scoped in-flight ownership, so a replacement view resolved without
an intervening disposal (a path the dispose handler never saw) could not
start its own recovery until the stale generation settled, delaying the
live view's recovery by a whole generation window. The bump closes that
interleaving while leaving the normal dispose-then-resolve path
unchanged: disposal already bumps via clearWebviewResources, and by the
time the replacement resolves this.view is undefined, so the two bump
sites never double-count one replacement. Same-view re-resolves (a known
VS Code re-resolve pattern) must not invalidate an in-flight recovery,
so they are excluded from the bump.
* chore(e2e): restore restart-persistence test to merge-base state
What: Restores apps/vscode-e2e/src/suite/restart-persistence.test.ts to the
merge-base (08d05eb), removing the waitForTaskInHistory polling helper and
its call in runVerify that commit 148b641 added.
Why: The restart-persistence polling change is unrelated to webview renderer
heartbeat recovery and was flagged as an out-of-scope change in the PR's
pre-merge review; it will be submitted separately.
Impact: The PR diff no longer touches apps/vscode-e2e; no runtime behavior
changes.
* fix(webview): gate obsolete-view messages and dispose replaced-view subscriptions
What: setWebviewMessageListener now takes the resolved view/panel and skips
dispatch when this.view no longer references it, so a replaced view's live
renderer cannot post into the provider. resolveWebviewView now tracks the
view-specific subscriptions (message, visibility, active-editor, and
configuration listeners plus the view's onDidDispose registration) in a
dedicated resolvedViewDisposables array; switching to a different view
disposes the previous entries before installing the replacement's, and both
visibility callbacks (onDidChangeViewState and onDidChangeVisibility) also
guard on this.view === webviewView before refreshing the provider-wide
heartbeat. clearWebviewResources disposes the per-view subscriptions too, so
provider dispose() and sidebar disposal still clean up everything. New specs:
a replaced view's message is ignored while the current view's dispatches; the
replaced view's subscriptions are each disposed exactly once and only the
replacement's remain; a replaced sidebar view's late dispose callback neither
stops the replacement's watchdog nor blocks its stale-heartbeat recovery; a
replaced tab panel's view state changes and messages are ignored while the
current panel's still dispatch; and a racing stale visibility event cannot
refresh the heartbeat.
Why: A live old renderer kept the provider-wide heartbeat timestamp fresh
after its view was replaced, so the dead-renderer watchdog could skip
recovery and leave the current webview gray, and every resolution
accumulated duplicate listeners in the shared webviewDisposables array.
Impact: Only ClineProvider and its spec change; same-view re-resolves keep
today's behavior (no disposal, no epoch bump), and the existing sidebar
onDidDispose guard semantics are unchanged.
* fix(webview): bail out of a resolve that went stale during its awaits
What: resolveWebviewView now checks _disposed and this.view === webviewView
after its getWebviewHtml/getState awaits and before installing the message
listener, restarting the watchdog, or registering any other view
subscription; a stale resolve returns without installing anything. New
specs: a provider disposed mid-resolve ends with no watchdog interval and
zero view subscriptions; a view that replaces a still-pending resolve ends
with only the replacement's four subscriptions and the replacement keeping
the watchdog.
Why: Disposal (or a view replacement) during the awaited HTML/state
generation used to stop the watchdog and clear resources, after which the
pending resolve resumed and installed a fresh watchdog interval plus
listeners for a disposed provider or a replaced view. That interval
retained the provider and kept attempting recovery forever, and a replaced
pending resolve duplicated the replacement's subscriptions on top of its
own.
Impact: Only the pending-resolve race is closed; normal resolves,
same-view re-resolves, and dispose-then-revive via a new provider instance
are unchanged. Both the sidebar (WebviewView) and tab (WebviewPanel)
shapes share this code path, so both are covered by the single guard.
* fix(webview): skip the initial html assignment for a stale resolve
What: resolveWebviewView now checks _disposed and this.view === webviewView
after the getWebviewHtml await and before assigning webview.html; a stale
resolve returns without touching the view. The existing post-awaits guard
before listener installation is unchanged. The two mid-resolve regression
specs now also assert no html landed on the disposed/replaced view.
Why: Disposal or replacement during html generation used to let the pending
resolve assign html afterwards. The VS Code API throws when assigning to a
destroyed webview, so the rejection surfaced from resolveWebviewView instead
of returning cleanly, and a replaced resolve pushed html into the obsolete
view it no longer owned.
Impact: Live resolves assign exactly as before (a single combined guard
after both awaits was rejected: it would leave the webview blank when the
subsequent getState read fails, where today the html stays assigned). Same-
view re-resolves, replacements, and revive flows are unaffected.
* chore: retrigger review-state reconciliation
* chore: retrigger review-state reconciliation
* fix(webview): ignore suspend gap in webview heartbeat watchdog
What: record the wall-clock reading of the previous watchdog tick; a tick
arriving more than 1.5x the interval late restarts the heartbeat grace
window instead of counting the gap against the renderer.
Why: during system sleep both the extension-host interval and the
webview heartbeat timer pause, but Date.now() keeps advancing. The first
post-wake tick then saw a stale heartbeat on a live renderer and
reloaded it, discarding unsent composer text and pending attachments.
Impact: no false recovery reload after wake from sleep; the watchdog
still reloads when the heartbeat genuinely goes stale (regression test
advances the clock past the stale threshold between ticks and asserts
no HTML reassignment, then asserts a real stale period still reloads).
* fix(webview): stop stale-heartbeat logging after retry limit
* test(webview): verify sidebar disposal cleanup and recovery invalidation
* fix(webview): reset recovery budget on view replacement and strengthen watchdog tests
* test: cover webview heartbeat recovery and reproduce renderer crash failure
* fix: recreate unresponsive webview hosts and preserve active tasks
* fix: address heartbeat recovery review feedback
* fix(webview): keep recovery warnings and probe results retryable
* fix(webview): distinguish HMR probe timeouts from unavailable dev server
* fix(webview): preserve recovery state for delayed sidebar replacements
* fix(webview): retry failed tab initialization and clear failed sidebar recovery
* fix(webview): warn on tab initialization failures without stopping retries
* fix(webview): restore bounded recovery for failed sidebar initialization
* test(webview): add bounded model checking for recovery protocols
* fix(webview): log terminal recovery stops after retry warnings
* feat(webview): add explicit interface recovery retry with generation guards
---------
Co-authored-by: Zoo (VP) <zoo@emebala.dev>
Co-authored-by: Elliott de Launay <edelauna@gmail.com>1 parent 2368339 commit 997f5d8
60 files changed
Lines changed: 4011 additions & 209 deletions
File tree
- .github/workflows
- apps/vscode-e2e
- src/visual
- docs/architecture
- packages/types/src
- scripts
- src
- activate
- __tests__
- core/webview
- __tests__
- i18n/locales
- ca
- de
- en
- es
- fr
- hi
- id
- it
- ja
- ko
- nl
- pl
- pt-BR
- ru
- tr
- vi
- zh-CN
- zh-TW
- webview-ui/src
- __tests__
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
116 | 116 | | |
117 | 117 | | |
118 | 118 | | |
| 119 | + | |
| 120 | + | |
119 | 121 | | |
120 | 122 | | |
121 | 123 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
85 | 85 | | |
86 | 86 | | |
87 | 87 | | |
88 | | - | |
| 88 | + | |
| 89 | + | |
89 | 90 | | |
90 | 91 | | |
91 | 92 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
7 | | - | |
8 | 7 | | |
9 | 8 | | |
10 | 9 | | |
11 | 10 | | |
12 | | - | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
13 | 15 | | |
14 | 16 | | |
15 | 17 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
2 | | - | |
3 | | - | |
4 | | - | |
5 | | - | |
6 | | - | |
7 | 1 | | |
8 | | - | |
9 | | - | |
10 | | - | |
11 | | - | |
12 | | - | |
13 | | - | |
14 | | - | |
15 | | - | |
16 | | - | |
17 | | - | |
18 | | - | |
19 | | - | |
20 | | - | |
21 | | - | |
22 | | - | |
23 | | - | |
24 | | - | |
25 | | - | |
26 | | - | |
27 | | - | |
28 | | - | |
29 | | - | |
30 | | - | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
31 | 13 | | |
32 | 14 | | |
33 | 15 | | |
| |||
52 | 34 | | |
53 | 35 | | |
54 | 36 | | |
55 | | - | |
56 | | - | |
57 | | - | |
58 | | - | |
59 | | - | |
60 | | - | |
61 | | - | |
62 | | - | |
63 | | - | |
64 | | - | |
65 | | - | |
66 | | - | |
67 | | - | |
68 | | - | |
69 | | - | |
70 | | - | |
71 | | - | |
72 | | - | |
73 | | - | |
74 | | - | |
75 | | - | |
76 | | - | |
77 | | - | |
78 | | - | |
79 | | - | |
80 | | - | |
81 | | - | |
82 | | - | |
83 | | - | |
84 | | - | |
85 | | - | |
86 | | - | |
87 | | - | |
88 | | - | |
89 | | - | |
90 | | - | |
91 | | - | |
92 | | - | |
93 | | - | |
94 | | - | |
95 | | - | |
96 | | - | |
97 | | - | |
98 | | - | |
99 | | - | |
100 | | - | |
101 | | - | |
102 | | - | |
103 | | - | |
104 | | - | |
105 | | - | |
106 | | - | |
107 | | - | |
108 | | - | |
109 | | - | |
110 | | - | |
111 | | - | |
112 | | - | |
113 | | - | |
114 | | - | |
115 | | - | |
116 | | - | |
117 | | - | |
118 | | - | |
119 | | - | |
120 | | - | |
121 | | - | |
122 | | - | |
123 | | - | |
124 | | - | |
125 | | - | |
126 | | - | |
127 | | - | |
128 | | - | |
129 | | - | |
130 | | - | |
131 | | - | |
132 | | - | |
133 | | - | |
134 | | - | |
135 | | - | |
136 | | - | |
137 | | - | |
138 | | - | |
139 | | - | |
140 | | - | |
141 | | - | |
142 | | - | |
143 | | - | |
144 | | - | |
145 | | - | |
146 | | - | |
147 | | - | |
148 | | - | |
149 | | - | |
150 | | - | |
151 | | - | |
152 | | - | |
153 | | - | |
154 | | - | |
155 | | - | |
156 | | - | |
157 | | - | |
158 | | - | |
159 | | - | |
160 | | - | |
161 | | - | |
162 | | - | |
163 | | - | |
164 | | - | |
165 | | - | |
166 | | - | |
167 | | - | |
168 | | - | |
169 | | - | |
170 | | - | |
171 | | - | |
172 | | - | |
173 | | - | |
174 | | - | |
175 | | - | |
176 | | - | |
177 | | - | |
178 | | - | |
179 | | - | |
180 | | - | |
181 | | - | |
182 | | - | |
183 | | - | |
184 | | - | |
185 | | - | |
186 | | - | |
187 | | - | |
188 | | - | |
189 | | - | |
190 | | - | |
191 | | - | |
192 | | - | |
193 | | - | |
194 | | - | |
195 | | - | |
196 | | - | |
197 | | - | |
198 | | - | |
199 | | - | |
200 | | - | |
201 | | - | |
202 | | - | |
203 | | - | |
204 | 37 | | |
205 | 38 | | |
206 | 39 | | |
| |||
0 commit comments