Skip to content

Commit b4e87be

Browse files
matt2eclaude
andcommitted
fix(desktop): retain in-flight detached agent starts across community switches
PR #7154 review point 4 (P2): resetCommunityState cleared the in-flight detached-start map on every community switch, and the justification — "those starts belong to the community being left and are about to fail closed at the backend's scope assertion anyway" — is wrong for a round trip. The scope assertion is a current-state check, so a deploy still held from community A becomes valid again the moment A is re-applied. Nothing settles the held start across the switch (the mutation lives on the QueryClient above the remount boundary; apply_workspace aborts no in-flight deploys), the record still reads not_deployed for the whole window, and the map entry was the only duplicate guard on this path — so an A→B→A round trip plus a second mention deployed a provider agent twice, handing the provider the second message's replay floor (past the first message: the exact failure class this branch exists to prevent). Local spawns were never exposed; the backend coalesces those under the process lock. Implement the reviewer's fix (option (a) of the analysis): retain the entries across switches. - resetCommunityState no longer clears the map; an explicit exemption comment at the call site (the canonical singleton-reset inventory) records why, so a future contributor does not "fix" the omission. Retention is safe by construction: the key IS the tenant scope (relay URL + pubkey), so a retained A entry can never suppress or affect a wake in B, and the identity-guarded finally delete self-cleans at settlement, which the deploy op's timeout bounds. The backend's own provider_deploy_locks map is likewise never cleared on workspace apply. - resetDetachedAgentStarts stays exported as a test-only isolation seam and its doc now says so; the map header and finally-guard comments that encoded the refuted "fail closed anyway" rationale are rewritten to record the retention contract instead. - Deliberately NOT scope-filtered clearing ("only entries for the community being left"): the A-scoped entry is exactly the one that must survive the A→B switch, so that variant recreates the bug. - The detachedToastScope mirror reset stays: on-screen warning delivery is genuinely per-community state. Out of scope, unchanged: backend deploy-epoch coalescing (option (b)) remains the deferred durable fix for the wake paths that do not funnel through useDetachedAgentStart (Agents-panel Start, restore, inbound-persona deploys, reconcile_on_workspace_apply). Residual accepted: on a same-relay identity switch, a retained old-identity entry suppresses the new identity's wake until the old start fails closed and settles — the same accepted within-community residual, self-resolving on the user's next send. Tests: - New E2E regression, the load-bearing one — it drives the real rail-switch → remount → resetCommunityState path that did the clearing: a not_deployed provider agent's deploy is held open via startManagedAgentDelayMs; the mention publishes and deploys once, scoped to A; a real A→B→A rail round-trip runs under the hold; a second mention back in A publishes while start_managed_agent stays at exactly 1; the held deploy is then settled on demand and rejects, its failure warning delivers back in A, and a third mention re-fires the wake (2 calls) — retention ends at settlement rather than latching the agent. - New mock-bridge seam for that spec: __BUZZ_E2E_RELEASE_MANAGED_AGENT_STARTS__() settles starts held behind startManagedAgentDelayMs on demand, following the bridge's defer/release idiom — the hold must deterministically outlast the round-trip AND settle before the retry leg. The first settlement is armed to reject via startManagedAgentErrors because a successful mock settle writes `deployed` into the record, which would hide the retry leg behind the status check rather than the map — the seam under test. - Two unit tests pin the complementary hook contracts in the existing real-CommunitiesProvider harness: an A→B→A switch hands back a callback whose key matches the retained entry, so the wake stays suppressed; and a rejection across the round-trip re-permits. The existing cross-community test keeps pinning that a B wake during the interlude is unaffected. Verified: desktop tsc --noEmit, biome check over the touched files, desktop unit tests (5856 passed; the 5 failures are the pre-existing inboxReopenNavigation/useRetainedProjectGitViews baseline, present on origin/main), the full mentions (87) and community-rail (25) Playwright smoke suites against a fresh pnpm build:e2e bundle, a 3x stress rerun of the new spec, and the repository file-size gate. The new E2E spec was confirmed red on the pre-fix build — 2 deploys vs 1 at the second-send snapshot — with the map clearing temporarily restored. Co-Authored-By: Claude Fable 5 <[email protected]> Signed-off-by: Matt Toohey <[email protected]>
1 parent 7c372c4 commit b4e87be

5 files changed

Lines changed: 292 additions & 12 deletions

File tree

‎desktop/src/features/communities/useCommunityInit.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ import {
2828
resetDetachedToastScope,
2929
setDetachedToastScope,
3030
} from "@/features/messages/lib/detachedToastScope";
31-
import { resetDetachedAgentStarts } from "@/features/messages/ui/useDetachedAgentStart";
3231
import {
3332
resetActiveAgentTurnsStore,
3433
saveActiveAgentTurnsForCommunity,
@@ -83,7 +82,13 @@ async function resetCommunityState({
8382
resetBackgroundMediaUploads();
8483
resetLinkPreviewPreparations();
8584
resetPersistentAgentAudienceStore();
86-
resetDetachedAgentStarts();
85+
// Intentionally NOT reset: the in-flight detached agent-start map
86+
// (`useDetachedAgentStart`). Its entries are keyed by tenant scope (relay
87+
// URL + pubkey), so they cannot leak into the new community, and they
88+
// self-clean when the start settles. Clearing them here is what permitted
89+
// the A→B→A duplicate provider deploy: the backend's scope assertion is a
90+
// current-state check, so a start held across a round-trip is valid again
91+
// once A is re-applied — the map entry is its only duplicate guard.
8792
resetDetachedToastScope();
8893
clearSearchHitEventCache();
8994
clearMarkdownNodeCache();

‎desktop/src/features/messages/ui/useDetachedAgentStart.test.mjs‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,12 @@
2323
* agent over community B's UI. Delivery compares the captured scope against
2424
* the on-screen mirror (`detachedToastScope`) at toast time — not a reset
2525
* generation, so an A→B→A round-trip warns again once A is back on screen.
26+
* 5. The in-flight map outlives a community switch on purpose: the backend's
27+
* scope assertion is a current-state check, so an A→B→A round-trip
28+
* re-validates a still-held start — the map entry is its only duplicate
29+
* guard, and it is tenant-keyed, so retention cannot affect the community
30+
* being entered. (`resetCommunityState` no longer clears it; that seam is
31+
* pinned E2E.)
2632
*
2733
* These tests drive the real hook against the real CommunitiesProvider; the
2834
* scope mirror is driven directly through its module seam, standing in for
@@ -370,6 +376,76 @@ test("a wake fires again once the previous one has settled", async () => {
370376
rendered.unmount();
371377
});
372378

379+
test("an A→B→A community round-trip does not reopen the in-flight window", async () => {
380+
// The production seam — `resetCommunityState` deliberately not clearing the
381+
// map on switch — is pinned E2E, where the real switch path runs. This pins
382+
// the hook contract that makes retention sufficient: the round-trip hands
383+
// back a callback carrying A's scope again, its key matches the retained
384+
// entry, and the wake stays suppressed while the first start (whose scope
385+
// is valid again now that A is re-applied) is still deploying.
386+
holdStarts = true;
387+
const { act, rendered } = await renderDetachedStart();
388+
389+
await act(async () => {
390+
rendered.result.current.startDetached(AGENT_RECORD);
391+
await settle();
392+
});
393+
await act(async () => {
394+
rendered.result.current.switchCommunity("community-b");
395+
});
396+
await act(async () => {
397+
rendered.result.current.switchCommunity("community-a");
398+
});
399+
400+
let refire;
401+
await act(async () => {
402+
refire = rendered.result.current.startDetached(AGENT_RECORD);
403+
await settle();
404+
});
405+
406+
assert.equal(refire, false, "the retained entry must still suppress");
407+
assert.equal(
408+
startCalls.length,
409+
1,
410+
"a round-trip must not duplicate a deploy the first start is still performing",
411+
);
412+
rendered.unmount();
413+
});
414+
415+
test("a wake re-fires once the start held across a round-trip has rejected", async () => {
416+
// Retention ends at settlement, not at some later reset: the `finally`
417+
// self-cleans, so a failed start never latches the agent — the user's next
418+
// send after the failure toast gets a real wake.
419+
holdStarts = true;
420+
const { act, rendered } = await renderDetachedStart();
421+
422+
await act(async () => {
423+
rendered.result.current.startDetached(AGENT_RECORD);
424+
await settle();
425+
});
426+
await act(async () => {
427+
rendered.result.current.switchCommunity("community-b");
428+
});
429+
await act(async () => {
430+
rendered.result.current.switchCommunity("community-a");
431+
});
432+
await act(async () => {
433+
heldStarts[0].reject();
434+
await settle();
435+
});
436+
await act(async () => {
437+
assert.equal(rendered.result.current.startDetached(AGENT_RECORD), true);
438+
await settle();
439+
});
440+
441+
assert.equal(
442+
startCalls.length,
443+
2,
444+
"settlement must end the suppression even across a round-trip",
445+
);
446+
rendered.unmount();
447+
});
448+
373449
test("a failed wake clears the key instead of latching the agent", async () => {
374450
holdStarts = true;
375451
const { act, rendered } = await renderDetachedStart();

‎desktop/src/features/messages/ui/useDetachedAgentStart.ts‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,23 @@ import { getErrorMessage } from "./useMentionSendFlow.helpers";
2323
* Keyed by relay as well as pubkey to mirror the backend's own runtime pair
2424
* key, so a start in one community can never suppress one in another — the
2525
* same tenant boundary `expectedRelayUrl` enforces below.
26+
*
27+
* Entries deliberately survive community switches (`resetCommunityState` does
28+
* NOT clear this map). A held start is not invalidated by leaving its
29+
* community: the backend's scope assertion is a current-state check, so a
30+
* start fired in A is valid again the moment A is re-applied — an A→B→A
31+
* round-trip that cleared the map would let a second send duplicate a
32+
* provider deploy the first start is still performing (and hand the harness
33+
* the second message's replay floor, past the first message). The key is the
34+
* tenant scope, so a retained entry cannot affect another community, and the
35+
* `finally` below self-cleans on settlement, which the deploy op bounds.
2636
*/
2737
const inFlightDetachedStarts = new Map<string, Promise<unknown>>();
2838

2939
/**
30-
* Drops every tracked in-flight start. Registered in `resetCommunityState`:
31-
* these starts belong to the community being left and are about to fail closed
32-
* at the backend's scope assertion anyway.
40+
* Drops every tracked in-flight start. Test-only isolation seam — one test's
41+
* held start must not suppress the next test's. Production deliberately never
42+
* calls this: see the map's doc for why entries survive community switches.
3343
*/
3444
export function resetDetachedAgentStarts(): void {
3545
inFlightDetachedStarts.clear();
@@ -181,11 +191,12 @@ export function useDetachedAgentStart(): (
181191
warnAgentMayNotRespond(agent.name, detachedStartFailureDetail(error));
182192
})
183193
.finally(() => {
184-
// Identity-guarded: `resetDetachedAgentStarts` may have cleared the
185-
// map and a later start re-registered this key while this one was in
186-
// flight (an A→B→A community switch), and an unguarded delete would
187-
// drop that newer entry. Clearing in `finally` rather than on success
188-
// is what keeps a failed start from latching the agent permanently.
194+
// Identity-guarded: only test isolation clears this map now
195+
// (production retains entries across community switches), but if a
196+
// reset-and-re-register did interleave while this start was in
197+
// flight, an unguarded delete would drop the newer entry. Clearing
198+
// in `finally` rather than on success is what keeps a failed start
199+
// from latching the agent permanently.
189200
if (inFlightDetachedStarts.get(key) === started) {
190201
inFlightDetachedStarts.delete(key);
191202
}

‎desktop/src/testing/e2eBridge.ts‎

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,8 @@ type E2eConfig = {
360360
openDmDelayMs?: number;
361361
sendMessageDelayMs?: number;
362362
/** Delay (ms) for `start_managed_agent` so e2e tests can switch the
363-
* community mid-startup and observe the fail-closed scope check. */
363+
* community mid-startup and observe the fail-closed scope check.
364+
* Releasable early via `__BUZZ_E2E_RELEASE_MANAGED_AGENT_STARTS__()`. */
364365
startManagedAgentDelayMs?: number;
365366
/** Hold the media proxy at port 0 until the E2E release seam is invoked. */
366367
mediaProxyInitiallyUnavailable?: boolean;
@@ -1467,6 +1468,11 @@ declare global {
14671468
/** Count of `get_event` invocations for the current defer-target ID since
14681469
* the last time `__BUZZ_E2E_DEFER_GET_EVENT__` was set. */
14691470
__BUZZ_E2E_GET_EVENT_CALL_COUNT__?: number;
1471+
/** Release every `start_managed_agent` currently held behind
1472+
* `startManagedAgentDelayMs`, letting it settle (scope checks, then any
1473+
* armed `startManagedAgentErrors` rejection) without waiting out the
1474+
* delay. Returns the number of holds flushed. */
1475+
__BUZZ_E2E_RELEASE_MANAGED_AGENT_STARTS__?: () => number;
14701476
/** Hold the next channel read until released. */
14711477
__BUZZ_E2E_DEFER_NEXT_CHANNELS_READ__?: () => void;
14721478
/** Disarm the latch and release the held channel read, if any. */
@@ -1593,6 +1599,11 @@ let deferredGetEventQueue: DeferredGetEvent[] = [];
15931599
let deferredLinkPreviewMetadataQueue: Array<() => void> = [];
15941600
let deferredLinkPreviewUploadQueue: Array<() => void> = [];
15951601
let deferredThreadRepliesQueue: Array<() => void> = [];
1602+
// Starts currently held behind `startManagedAgentDelayMs`, releasable early
1603+
// via `__BUZZ_E2E_RELEASE_MANAGED_AGENT_STARTS__()`: a spec that holds a
1604+
// start across a community round-trip needs the hold long enough to be
1605+
// deterministic AND a way to settle it on demand afterwards.
1606+
let heldManagedAgentStartReleases: Array<() => void> = [];
15961607
let cancelledMediaUploadIds = new Set<string>();
15971608
let deferNextChannelsRead = false;
15981609
let deferredChannelsReadResolve: (() => void) | null = null;
@@ -9486,7 +9497,20 @@ async function handleStartManagedAgent(
94869497
): Promise<RawManagedAgent> {
94879498
const delayMs = config?.mock?.startManagedAgentDelayMs ?? 0;
94889499
if (delayMs > 0) {
9489-
await new Promise((resolve) => window.setTimeout(resolve, delayMs));
9500+
await new Promise<void>((resolve) => {
9501+
let settled = false;
9502+
const release = () => {
9503+
if (settled) return;
9504+
settled = true;
9505+
window.clearTimeout(timer);
9506+
heldManagedAgentStartReleases = heldManagedAgentStartReleases.filter(
9507+
(held) => held !== release,
9508+
);
9509+
resolve();
9510+
};
9511+
const timer = window.setTimeout(release, delayMs);
9512+
heldManagedAgentStartReleases.push(release);
9513+
});
94909514
}
94919515
// After the injected delay, like the real command's checks before any
94929516
// spawn/deploy side effect: a caller-captured tenant scope and signer
@@ -11311,6 +11335,12 @@ export function maybeInstallE2eTauriMocks() {
1131111335
window.__BUZZ_E2E_GET_EVENT_CALL_COUNT__ = 0;
1131211336
window.__BUZZ_E2E_DEFER_GET_EVENT__ = null;
1131311337
deferredGetEventQueue = [];
11338+
heldManagedAgentStartReleases = [];
11339+
window.__BUZZ_E2E_RELEASE_MANAGED_AGENT_STARTS__ = () => {
11340+
const held = heldManagedAgentStartReleases.splice(0);
11341+
for (const release of held) release();
11342+
return held.length;
11343+
};
1131411344
deferNextChannelsRead = false;
1131511345
deferredChannelsReadResolve = null;
1131611346
window.__BUZZ_E2E_CHANNELS_READ_PENDING__ = 0;

‎desktop/tests/e2e/mentions.spec.ts‎

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2981,6 +2981,164 @@ test("a detached start fired before a real community switch fails closed and kee
29812981
).toBe(0);
29822982
});
29832983

2984+
test("a deploy held across an A→B→A community round-trip is not fired twice", async ({
2985+
page,
2986+
}) => {
2987+
// The in-flight detached-start map used to be cleared by every community
2988+
// switch, and the backend's scope assertion is a current-state check — so a
2989+
// deploy still held from community A became valid again the moment A was
2990+
// re-applied, and a second mention back in A deployed the agent a second
2991+
// time (carrying the second message's replay floor, past the first
2992+
// message). The entries are tenant-keyed and self-cleaning, so they now
2993+
// survive the switch. This drives the real rail-switch path (provider →
2994+
// remount → resetCommunityState) that did the clearing; the map contract
2995+
// itself is pinned at the unit level.
2996+
const COMMUNITY_A = {
2997+
id: "ws-a",
2998+
name: "Alpha",
2999+
relayUrl: "ws://localhost:3000",
3000+
addedAt: "2026-01-01T00:00:00.000Z",
3001+
};
3002+
const COMMUNITY_B = {
3003+
id: "ws-b",
3004+
name: "Bravo",
3005+
relayUrl: "ws://localhost:3001",
3006+
addedAt: "2026-01-02T00:00:00.000Z",
3007+
};
3008+
await installMockBridge(
3009+
page,
3010+
{
3011+
managedAgents: [
3012+
{
3013+
pubkey: OUT_OF_CHANNEL_PROVIDER_AGENT_PUBKEY,
3014+
name: "portal",
3015+
status: "not_deployed",
3016+
channelNames: ["general"],
3017+
backend: {
3018+
type: "provider",
3019+
id: "portal",
3020+
config: { region: "test" },
3021+
},
3022+
},
3023+
],
3024+
// Far longer than the round-trip below ever takes, so the first deploy
3025+
// is deterministically still in flight when the second send fires;
3026+
// settled on demand via the release seam for the retry leg.
3027+
startManagedAgentDelayMs: 45_000,
3028+
// Arms the first settlement to reject. A successful mock settle writes
3029+
// `deployed` into the record, and the third send below would then skip
3030+
// the wake on status alone — the retry leg has to prove the *map entry*
3031+
// self-cleaned, so the record must still read `not_deployed`.
3032+
startManagedAgentErrors: ["Mock provider deploy failed."],
3033+
},
3034+
{ skipCommunitySeed: true },
3035+
);
3036+
await page.addInitScript(
3037+
({ list, active }) => {
3038+
window.localStorage.setItem("buzz-communities", JSON.stringify(list));
3039+
window.localStorage.setItem("buzz-active-community-id", active);
3040+
},
3041+
{ list: [COMMUNITY_A, COMMUNITY_B], active: COMMUNITY_A.id },
3042+
);
3043+
await page.goto("/");
3044+
await page.getByTestId("channel-general").click();
3045+
await expect(page.getByTestId("chat-title")).toHaveText("general");
3046+
3047+
const sendMention = async (text: string) => {
3048+
const input = page.getByTestId("message-input");
3049+
await input.fill("Hey @portal");
3050+
await expect(autocomplete(page).getByText("portal")).toBeVisible();
3051+
await input.press("Enter");
3052+
await page.keyboard.type(` ${text}`);
3053+
// Enter rather than the send button: the third send happens while the
3054+
// first deploy's failure toast is on screen, and the toast overlay
3055+
// intercepts pointer events aimed at the composer's corner.
3056+
await input.press("Enter");
3057+
await expect(
3058+
page.getByTestId("message-row").filter({ hasText: text }),
3059+
).toBeVisible();
3060+
};
3061+
3062+
const baselineCommands = await readCommandLog(page);
3063+
const baselineStartCount = commandCount(
3064+
baselineCommands,
3065+
"start_managed_agent",
3066+
);
3067+
const baselineSettledCount = commandCount(
3068+
baselineCommands,
3069+
"start_managed_agent:settled",
3070+
);
3071+
const baselinePayloadCount = (await readCommandPayloadLog(page)).length;
3072+
3073+
await sendMention("do X");
3074+
await expect
3075+
.poll(async () =>
3076+
commandCount(await readCommandLog(page), "start_managed_agent"),
3077+
)
3078+
.toBe(baselineStartCount + 1);
3079+
const startCall = (await readCommandPayloadLog(page))
3080+
.slice(baselinePayloadCount)
3081+
.find((entry) => entry.command === "start_managed_agent");
3082+
expect(startCall?.payload).toMatchObject({
3083+
expectedRelayUrl: COMMUNITY_A.relayUrl,
3084+
expectedSignerPubkey: MOCK_VIEWER_PUBKEY,
3085+
});
3086+
3087+
// The round trip, while the deploy is still held.
3088+
await page.getByTestId(`community-rail-button-${COMMUNITY_B.id}`).click();
3089+
await expect(
3090+
page.getByTestId(`community-rail-button-${COMMUNITY_B.id}`),
3091+
).toHaveAttribute("aria-current", "true");
3092+
await page.getByTestId(`community-rail-button-${COMMUNITY_A.id}`).click();
3093+
await expect(
3094+
page.getByTestId(`community-rail-button-${COMMUNITY_A.id}`),
3095+
).toHaveAttribute("aria-current", "true");
3096+
await page.getByTestId("channel-general").click();
3097+
await expect(page.getByTestId("chat-title")).toHaveText("general");
3098+
3099+
// Back in A, the held deploy's scope is valid again and the record still
3100+
// reads `not_deployed`, so this send queues a wake — the retained map entry
3101+
// is the only thing standing between it and a duplicate deploy. A
3102+
// suppressed wake makes no call, so give the post-publish flush a moment
3103+
// before snapshotting: pre-fix the duplicate invoke landed well inside it.
3104+
await sendMention("also Y");
3105+
await page.waitForTimeout(500);
3106+
expect(commandCount(await readCommandLog(page), "start_managed_agent")).toBe(
3107+
baselineStartCount + 1,
3108+
);
3109+
3110+
// Settle the held deploy on demand (the armed rejection). Retention must
3111+
// end at settlement rather than latching the agent for the session.
3112+
const released = await page.evaluate(
3113+
() =>
3114+
(
3115+
window as {
3116+
__BUZZ_E2E_RELEASE_MANAGED_AGENT_STARTS__?: () => number;
3117+
}
3118+
).__BUZZ_E2E_RELEASE_MANAGED_AGENT_STARTS__?.() ?? 0,
3119+
);
3120+
expect(released).toBe(1);
3121+
await expect
3122+
.poll(async () =>
3123+
commandCount(await readCommandLog(page), "start_managed_agent:settled"),
3124+
)
3125+
.toBe(baselineSettledCount + 1);
3126+
// The failure settled with A on screen, so its warning delivers here — the
3127+
// round-trip kept it out of B without dropping it.
3128+
await expect(
3129+
page.getByText("Could not start portal", { exact: false }),
3130+
).toBeVisible();
3131+
3132+
// The map entry self-cleaned at settlement, so a third mention of the
3133+
// still-undeployed agent re-fires the wake.
3134+
await sendMention("try again");
3135+
await expect
3136+
.poll(async () =>
3137+
commandCount(await readCommandLog(page), "start_managed_agent"),
3138+
)
3139+
.toBe(baselineStartCount + 2);
3140+
});
3141+
29843142
test("mentioning an in-channel provider managed agent publishes first and deploys it detached", async ({
29853143
page,
29863144
}) => {

0 commit comments

Comments
 (0)