Skip to content

test(e2e): poll restart conversation history - #1663

Merged
edelauna merged 1 commit into
Zoo-Code-Org:mainfrom
PierrunoYT:fix/1641-restart-persistence-poll
Sep 20, 2026
Merged

edelauna merged 1 commit into
Zoo-Code-Org:mainfrom
PierrunoYT:fix/1641-restart-persistence-poll

Conversation

@PierrunoYT

Copy link
Copy Markdown
Contributor

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 waitForMarkedCompletion helper. It repeatedly calls the same api.hasTaskApiConversationHistorySequence read used by the original assertions until the sequence becomes visible or the existing 30-second waitFor deadline 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 --check

All 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-run is available. CI's mocked E2E runner should execute this exact scenario:

TEST_FILE=restart-persistence.test pnpm --filter @roo-code/vscode-e2e test:ci:mock

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue.
  • Scope: The change is limited to the restart-persistence E2E read race.
  • Self-Review: I have performed a self-review.
  • Testing: The affected package and workspace lint/typecheck gates pass; CI will run the graphical E2E.
  • Visual Snapshot: N/A — test synchronization only; no UI changes.
  • Documentation Impact: No documentation updates are required.
  • Contribution Guidelines: I have read and agree to the contributor guidelines.

Visual Snapshots

N/A — no UI changes.

Videos (interaction / animation only)

N/A.

Documentation Updates

  • No documentation updates are required.

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

@coderabbitai

coderabbitai Bot commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: c67781b8-e285-4a4c-9f2f-32a766463f6a

📥 Commits

Reviewing files that changed from the base of the PR and between 500152b and 1312150.

📒 Files selected for processing (1)
  • apps/vscode-e2e/src/suite/restart-persistence.test.ts

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:

  • apps/vscode-e2e/src/suite/restart-persistence.test.ts
Check strict typing and exhaustive behavior across normal, boundary, error, cancellation, retry, and compatibility paths.

⚙️ CodeRabbit configuration file

Files:

  • apps/vscode-e2e/src/suite/restart-persistence.test.ts
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:

  • apps/vscode-e2e/src/suite/restart-persistence.test.ts
Act as an adversarial second-opinion reviewer.

⚙️ CodeRabbit configuration file

Files:

  • apps/vscode-e2e/src/suite/restart-persistence.test.ts
Use short, stable, unique text in the task prompt.

📄 CodeRabbit inference engine (apps/vscode-e2e/AGENTS.md)

Files:

  • apps/vscode-e2e/src/suite/restart-persistence.test.ts
🔇 Additional comments (1)
apps/vscode-e2e/src/suite/restart-persistence.test.ts (1)

28-37: LGTM!

Also applies to: 99-99, 110-110


📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Improved verification of restored and reopened tasks by waiting for completion sequences to appear before checking results.
    • Increased reliability of persistence checks during application restart scenarios.

Walkthrough

The 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.

Changes

Restart persistence validation

Layer / File(s) Summary
Poll completion history before assertions
apps/vscode-e2e/src/suite/restart-persistence.test.ts
Adds waitForMarkedCompletion and uses it for restored and reopened task completion checks. The helper waits for the expected smoke-test user turn, attempt_completion tool call, and marker input.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 13121

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)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #1641 requires bounded polling without fixed sleeps and requires the polling read to match the assertion read. The changed restart-persistence.test.ts adds waitForMarkedCompletion, which cal…
Out of Scope Changes check ✅ Passed The diff changes only apps/vscode-e2e/src/suite/restart-persistence.test.ts. It adds a shared polling helper and replaces the two restart-persistence history checks. These changes directly support i…
Regression Evidence ✅ Passed The PR changes only restart-persistence.test.ts. The changed polling behavior has focused coverage in the same restart-persistence E2E: waitForMarkedCompletion calls `api.hasTaskApiConversationHis…
Security Boundaries ✅ Passed PASS. The pull request changes only apps/vscode-e2e/src/suite/restart-persistence.test.ts. The new helper repeatedly performs the existing read-only api.hasTaskApiConversationHistorySequence call …
Persistence Integrity ✅ Passed PASS. The pull request changes only apps/vscode-e2e/src/suite/restart-persistence.test.ts. It replaces two immediate reads with await waitForMarkedCompletion(...); that helper awaits waitFor, an…
Lifecycle Resource Cleanup ✅ Passed No changed lifecycle path can be shown to leak or duplicate work. The PR only adds waitForMarkedCompletion, which performs sequential api.hasTaskApiConversationHistorySequence reads through the ex…
Title check ✅ Passed The title clearly and concisely describes the main change: polling restart conversation history in the E2E test.
Description check ✅ Passed The description includes the linked issue, implementation details, test procedure, test limitations, checklist, snapshot and documentation decisions, and reviewer context. It is complete and aligned w…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Review status

Thanks 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

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions github-actions Bot added coderabbit-review-active Required CI passed; CodeRabbit review is active awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit labels Sep 16, 2026
@github-actions github-actions Bot added awaiting-maintainer CodeRabbit approved; waiting for a human maintainer and removed coderabbit-review-active Required CI passed; CodeRabbit review is active awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit labels Sep 16, 2026

@edelauna edelauna left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing this.

@edelauna
edelauna added this pull request to the merge queue Sep 20, 2026
Merged via the queue into Zoo-Code-Org:main with commit 914f0c4 Sep 20, 2026
18 checks passed
DaubnerF added a commit to DaubnerF/Zoo-Code that referenced this pull request Sep 20, 2026
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.
myk1yt added a commit to myk1yt/Zoo-Code that referenced this pull request Sep 27, 2026
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.
edelauna pushed a commit to myk1yt/Zoo-Code that referenced this pull request Oct 28, 2026
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.
rrewll pushed a commit to rrewll/Zoo-Code that referenced this pull request Nov 1, 2026
…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]>
edelauna added a commit that referenced this pull request Nov 2, 2026
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-maintainer CodeRabbit approved; waiting for a human maintainer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Make the restart-persistence e2e tolerate the atomic-rename swap window when reading API conversation history

3 participants