Repository navigation
test(e2e): poll restart conversation history - #1663
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📜 Recent review details🧰 Additional context used📓 Path-based instructions (5)Require regression coverage at the lowest valid harness with behavior-focused assertions, including relevant negative, error, false/unset, and boundary cases.⚙️ CodeRabbit configuration file Files:
Check strict typing and exhaustive behavior across normal, boundary, error, cancellation, retry, and compatibility paths.⚙️ CodeRabbit configuration file Files:
Reserve end-to-end coverage for behavior that requires the real VS Code host, workspace APIs, extension activation, webview messaging, file watchers, or a full workflow.⚙️ CodeRabbit configuration file Files:
Act as an adversarial second-opinion reviewer.⚙️ CodeRabbit configuration file Files:
Use short, stable, unique text in the task prompt.📄 CodeRabbit inference engine (apps/vscode-e2e/AGENTS.md) Files:
🔇 Additional comments (1)
📝 SummarySummary by CodeRabbit
WalkthroughThe restart-persistence end-to-end test now polls task history for the expected completion sequence. The restored-task and reopened-task checks use this polling instead of immediate boolean assertions. ChangesRestart persistence validation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The restart-persistence checks now wait for the expected persisted completion sequence, avoiding transient atomic-rename read failures without adding fixed delays. 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Review statusThanks for contributing. This comment tracks the review sequence and the next action. Current step: Awaiting fresh human maintainer or CODEOWNER approval. Automated review is complete for the latest commit but does not replace human approval. Review-state labels are managed by this workflow; do not edit them manually. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
edelauna
left a comment
There was a problem hiding this comment.
Thanks for addressing this.
Resolve the 22-file / 92-hunk conflict surface: - adopt-main (17 files): the four add/add Zoo-Code-Org#1505 duplicates (effective-tool-policy.ts, effective-tool-policy.spec.ts, build-tools.spec.ts, generateSystemPrompt.spec.ts), the P-theme sections/{capabilities,objective,rules,skills}.ts + system.ts + their specs + ask-mode-prompt.snap, filter-tools-for-mode.ts + its spec, zoo-gateway.spec.ts, restart-persistence.test.ts (main's Zoo-Code-Org#1663 polling supersedes the branch poll rewrite). - generateSystemPrompt.ts: adopt-main; the branch D-adjacent provider-state snapshot call-site (disabledTools + modelInfo args to SYSTEM_PROMPT) is identical to main's — no re-apply needed. - presentAssistantMessage.ts: keep main's buildToolRequirements(disabledTools, modelInfo?.info) call (matches the adopted two-arg policy API); the branch AutoApprovalContext threading survived the auto-merge. - presentAssistantMessage-custom-tool.spec.ts, Task.ts, Task.spec.ts: take main's side of every conflicted hunk (branch hunks were P/L-theme duplicates of main's Zoo-Code-Org#1505/Zoo-Code-Org#1625 rework); branch D-theme glue outside the conflict regions is preserved by the auto-merge. - Delete src/core/prompts/tools/__tests__/effective-tool-policy-warn.spec.ts: mechanical orphan of the adopt-main resolution — main's effective-tool-policy.ts never exported warnProtocolToolOverrides, so the branch-only spec cannot compile at the merged head. - presentAssistantMessage-auto-deny.spec.ts (branch-only, non-conflicting): add the getTaskMode mock the merged impl now requires (main Zoo-Code-Org#1625 reads the task-local mode); test-double drift invisible to the textual merge.
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 Zoo-Code-Org#1641 for this scenario, which Zoo-Code-Org#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.
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 Zoo-Code-Org#1641 for this scenario, which Zoo-Code-Org#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.
…oad (Zoo-Code-Org#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 Zoo-Code-Org#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 Zoo-Code-Org#1641 for this scenario, which Zoo-Code-Org#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) <[email protected]> Co-authored-by: Elliott de Launay <[email protected]>
…oad (#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) <[email protected]> Co-authored-by: Elliott de Launay <[email protected]>
Related GitHub Issue
Closes: #1641
Description
The restart-persistence E2E previously checked persisted API conversation history with one immediate read after extension-host restart and another after reopening the task. Those reads could land in
safeWriteJson's atomic-rename swap window, where the destination path is briefly absent, and report a false negative.This change routes both checks through a shared
waitForMarkedCompletionhelper. It repeatedly calls the sameapi.hasTaskApiConversationHistorySequenceread used by the original assertions until the sequence becomes visible or the existing 30-secondwaitFordeadline expires. There are no fixed sleeps and no production persistence behavior changes.Test Procedure
pnpm --filter @roo-code/vscode-e2e lint pnpm --filter @roo-code/vscode-e2e check-types pnpm lint pnpm check-types pnpm exec prettier --check apps/vscode-e2e/src/suite/restart-persistence.test.ts git diff --checkAll commands pass. The commit and push hooks also completed the full workspace lint/typecheck gates (11/11 packages).
The targeted command below built the extension and webview and downloaded the pinned VS Code binary, but could not launch native VS Code in the local orb because no X display or
xvfb-runis available. CI's mocked E2E runner should execute this exact scenario:Pre-Submission Checklist
Visual Snapshots
N/A — no UI changes.
Videos (interaction / animation only)
N/A.
Documentation Updates
Additional Notes
The production write ordering and history loader are intentionally unchanged. This only makes the E2E tolerate the already-documented atomic publication window.
Get in Touch
GitHub: @PierrunoYT