Agent Host: stop the reconnecting banner nagging on a flapping transport - #334348
Conversation
A tunnel relay that drops and restores the transport every few seconds made the banner appear on a session that worked fine throughout. Two causes. The outage start time lived in a cached derived. While the host is connected nothing reads the reconnecting state, so that derived lost its observers while its cache survived them; the next outage on the same session inherited the previous outage's start time and showed the banner immediately. It is now an explicit value maintained by an autorun, which runs whether or not anything is rendering the reconnecting state. The delay before the banner appears was also shorter than a routine self-healed reconnect. Such a reconnect preserves session state and the user would not otherwise notice it, so the threshold now outlasts one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A directly replaced session can inherit the previous session’s elapsed reconnect delay.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
src/vs/sessions/browser/parts/sessionRemoteConnection.ts — setSession can replace one session directly with another, but this value is no longer keyed by… |
|
src/vs/sessions/browser/parts/sessionRemoteConnection.ts — This source comment preserves the reported tunnel incident and UI chronology rather than the… |
|
src/vs/sessions/test/browser/chatGroupsView.test.ts — The test name and loop already establish the scenario, while this multi-line comment narrates it… |
What changed in this PR
Prevents reconnecting-banner flicker during short, recurring Agent Host transport outages.
Changes:
- Raises the banner delay from one to five seconds.
- Tracks outage start time with explicit observable state.
- Adds regression coverage for flapping transports.
| File | Description |
|---|---|
src/vs/sessions/test/browser/chatGroupsView.test.ts |
Updates timing tests and adds flapping-transport coverage; test comments should be condensed. |
src/vs/sessions/browser/parts/sessionRemoteConnection.ts |
Revises outage timing and debounce logic; outage state must reset when replacing sessions. Incident-specific comments should also be simplified. |
Suppressed comments (4)
src/vs/sessions/browser/parts/sessionRemoteConnection.ts:149
_reconnectingSinceis both tracked as an autorun dependency and written below. Each write therefore synchronously re-enters this autorun; when that nested run finishes, the outer run's reader is no longer marked running, so its nextread(reader)reports aBugIndicatingError. Read this self-owned latch untracked instead.
const since = this._reconnectingSince.read(reader);
src/vs/sessions/browser/parts/sessionRemoteConnection.ts:143
- This repeats the field-level explanation and spans multiple lines to narrate the next block. Remove the duplicate so the invariant has one concise source of truth.
// Tracked here rather than in a derived so it is maintained whether or
// not anything is currently rendering the reconnecting state.
src/vs/sessions/browser/parts/sessionRemoteConnection.ts:92
- This field documentation embeds the investigation history and duplicates the PR description. Retain only the non-obvious ownership invariant so the comment remains useful if the implementation changes.
/**
* When the current outage began, or `undefined` while the host is reachable.
*
* Held in an explicit value rather than a cached derived: while the host is
* connected nothing reads the reconnecting state, so the derived loses its
src/vs/sessions/test/browser/chatGroupsView.test.ts:1091
- Condense this test comment to the single behavioral reason for waiting; the current two-line explanation restates the assertion mechanics.
// Past the delay, so this proves the settled connection suppresses the
// banner rather than the threshold simply not having elapsed.
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Base:
|
recomputeInitiallyAndOnChange keeps the cached derived observed, so it recomputes when the host reconnects rather than retaining a start time nothing was left to invalidate. Restores the session keying and drops the autorun that replaced it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: Benjamin Christopher Simmonds (@benibenj)Matched files:
|


A coworker reported a reconnecting banner appearing constantly on a session that otherwise worked fine. Their tunnel relay was dropping and restoring the transport on a ~6s cycle — each outage healed in ~2s with session state preserved, so the chat kept working the whole time.
Two separate causes, both from #334111.
Stale outage start time
The outage start lives in a
derivedObservableWithCache. While the host is connected nothing reads the reconnecting state, so that derived lost its observers — and a derived that is not observed keeps its last value without ever recomputing it. The next outage readlast?.session === session, matched, and reused the previous outage's timestamp. The delay had therefore already elapsed, so the banner appeared immediately.That is why it looked constant rather than merely frequent: only the first outage was ever debounced.
Fixed with
recomputeInitiallyAndOnChange, which keeps the derived observed for the lifetime of the view so it recomputes on reconnect. The derived and its session keying are otherwise unchanged.Threshold shorter than a routine reconnect
RECONNECTING_BANNER_DELAYwas 1s, but a self-healed reconnect here took ~2.05s. The delay exists to suppress blips the user would not otherwise notice — a reconnect that preserves session state is exactly that — so 1s was mistuned. Raised to 5s, comfortably past an observed reconnect.Both changes are needed: with only the longer delay, a 2s outage still shows the banner on every cycle after the first, because the stale timestamp makes the delay a no-op.
Test
stays quiet while a flapping transport keeps healing itselfreplays the reported cadence (2.1s outage, 3.7s healthy, five cycles) and asserts the banner never appears. It fails with either fix reverted — the stale-cache bug reproduces as the banner appearing from the second cycle onward.The underlying relay instability (
[TunnelAgentHost] onDidRelayClose) is pre-existing and out of scope here; this only stops the UI nagging about a connection that is working.