PR #68669: fix(agents): dedupe subagent browser session cleanup wrapper with dispatch flag

• Repository: openclaw/openclaw
• Author: Feelw00
• State: open | merged: False
• Link: https://github.com/openclaw/openclaw/pull/68669

Description (problem / solution / changelog)

Summary

• Problem: embedded subagent completion invokes cleanupBrowserSessionsForLifecycleEnd wrapper twice for the same runId. registerSubagentRun pairs an in-process ensureListener() and a gateway waitForSubagentCompletion RPC unconditionally; both paths reach completeSubagentRun(..., triggerCleanup: true) and the wrapper sits outside any dedup guard.
• Why it matters: actual CDP browserCloseTab IPC is already idempotent via extensions/browser/src/browser/session-tab-registry.ts take-and-drain. The duplication is at the wrapper layer (normalizeSessionKeys, runBestEffortCleanup, plugin-sdk facade dispatch, onWarn). Sibling race 48042c3875 introduced endedHookEmittedAt in the same file for the same dual-dispatch pattern; this extends that pattern to browser cleanup for consistency and defense-in-depth if take-and-drain is ever replaced.
• What changed: added SubagentRunRecord.browserCleanupDispatchedAt?: number, sync check-then-set guard in completeSubagentRun before the cleanup wrapper, and rearm reset in subagent-registry-run-manager.ts alongside endedHookEmittedAt. New regression test in subagent-registry-lifecycle.test.ts.
• What did NOT change (scope boundary): no change to startSubagentAnnounceCleanupFlow, no change to the bundled browser extension, no change to completeSubagentRun behavior for single-caller paths. CDP IPC semantics unchanged.

Change Type (select all)

• Bug fix

Scope (select all touched areas)

• Gateway / orchestration

Linked Issue/PR

• Closes #68668
• Related 48042c3875
• This PR fixes a bug or regression

Root Cause (if applicable)

• Root cause: registerSubagentRun fires both ensureListener() and void waitForSubagentCompletion(runId, ...) unconditionally for every subagent. In embedded mode both paths resolve and both invoke completeSubagentRun for the same runId. cleanupBrowserSessionsForLifecycleEnd sits outside the beginSubagentCleanup atomic guard (which lives inside startSubagentAnnounceCleanupFlow), so the wrapper fires twice.
• Missing detection / guardrail: no per-entry dispatch flag for the browser cleanup side-effect, so double-dispatch was silent at runtime and not covered by any existing test.
• Contributing context: 48042c3875 added endedHookEmittedAt for the sibling hook-emit race, but only covered the hook dispatch — the browser cleanup wrapper in the same function body remained unguarded.

Regression Test Plan (if applicable)

• Coverage level that should have caught this:
  • Unit test
• Target test or file: src/agents/subagent-registry-lifecycle.test.ts — new case "dedupes browser cleanup when two callers complete the same run in parallel".
• Scenario the test should lock in: Promise.all([completeSubagentRun(p), completeSubagentRun(p)]) with the same runId + triggerCleanup: true must result in cleanupBrowserSessionsForLifecycleEnd being called exactly once, with entry.browserCleanupDispatchedAt set to a number.
• Why this is the smallest reliable guardrail: both production completion paths (ensureListener callback in subagent-registry.ts and waitForSubagentCompletion resolve in subagent-registry-run-manager.ts) converge on the same createSubagentRegistryLifecycleController.completeSubagentRun helper, so a controller-level parallel invocation exercises the same branch without requiring a full gateway harness.
• Existing test that already covers this (if any): none.

User-visible / Behavior Changes

None. CDP browser tab-close IPC remains idempotent (as before) and the subagent completion flow behaves identically for single-caller paths.

Security Impact (required)

• New permissions/capabilities? No
• Secrets/tokens handling changed? No
• New/changed network calls? No
• Command/tool execution surface changed? No
• Data access scope changed? No

Repro + Verification

Environment

• OS: macOS 15.4
• Runtime/container: Node 22 (pnpm 10.33.0)
• Model/provider: N/A (unit test only)
• Integration/channel: N/A
• Relevant config: N/A

Steps

1. pnpm install in a fresh checkout.
2. Add the new regression case to src/agents/subagent-registry-lifecycle.test.ts (or run the existing branch).
3. Without the fix: pnpm test src/agents/subagent-registry-lifecycle.test.ts -t "dedupes browser cleanup" → expected "vi.fn()" to be called 1 times, but got 2 times.
4. With the fix: same command → passes.

Expected

cleanupBrowserSessionsForLifecycleEnd fires exactly once; entry.browserCleanupDispatchedAt is a number.

Actual

Matches expected after this PR.

Evidence

• Failing test/log before + passing after

Before (on clean upstream/main):

FAIL  |agents| src/agents/subagent-registry-lifecycle.test.ts
  × dedupes browser cleanup when two callers complete the same run in parallel
  AssertionError: expected "vi.fn()" to be called 1 times, but got 2 times

After (this PR):

Test Files  1 passed (1)
Tests  10 passed (10)

Full scoped suite (subagent-registry-lifecycle, subagent-registry-completion, subagent-registry.steer-restart, subagent-registry): 41/41 passing. pnpm tsgo:all, pnpm check, pnpm build all green.

Human Verification (required)

• Verified scenarios: controller-level Promise.all parallel invocation with same runId + triggerCleanup: true; pnpm tsgo:all, pnpm check, pnpm build all clean.
• Edge cases checked:
  • Rearm path (subagent-registry-run-manager.ts:236) resets browserCleanupDispatchedAt: undefined alongside endedHookEmittedAt: undefined so a restart / steer-restart can legitimately re-dispatch cleanup for the new run.
  • Verified session-tab-registry.ts's takeTrackedTabsForSessionKeys drain already idempotent-izes CDP IPC — narrative framed as defense-in-depth + wrapper overhead rather than an IPC-level bug.
  • Synchronous check-then-set (before any await) — no micro-race between check and set.
• What I did not verify: real embedded subagent run against a live browser driver — unit test covers the controller branch both production paths converge on.

Review Conversations

• I replied to or resolved every bot review conversation I addressed in this PR.
• I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

• Backward compatible? Yes
• Config/env changes? No
• Migration needed? No

Risks and Mitigations

• Risk: browserCleanupDispatchedAt persisting across disk persistence means a crashed process that already dispatched cleanup will skip the wrapper on restore.
  • Mitigation: wrapper-level skip is benign because the underlying session-tab-registry tabs are drained on first call; restart-side orphan handling (subagent-registry.ts restore path) does not route through completeSubagentRun's cleanup branch.

[AI-assisted]

Changed files

• src/agents/subagent-registry-lifecycle.test.ts (modified, +35/-0)
• src/agents/subagent-registry-lifecycle.ts (modified, +9/-0)
• src/agents/subagent-registry-run-manager.ts (modified, +1/-0)
• src/agents/subagent-registry.types.ts (modified, +1/-0)