fix(browserstack-service): rebind per-test session after reloadSession (SDK-6767)

[Bhargavi-BS]

Problem

WDIO + Mocha on App Automate: within one spec, tests after browser.reloadSession() (called in a beforeTest hook or in-body) all report under the FIRST device session — one video shows for all of them. Customer (Circle, SDK-6767): 7 tests of posts-actions.spec.ts all carry session_id 79b8e560… across 6 udids; build-wide 56 udids collapse onto 28 hashed_ids.

Root cause

The CLI/binary flow captures the device session id once per worker into KEY_FRAMEWORK_SESSION_ID (webdriverIOModule.ts onDriverCreated, fired once from service.ts before()) and each test's TestHub event reads that cached value (testHubModule.ts sendTestSessionEvent). onReload never refreshes it (grep -rn reload cli/ = 0). Present in both 9.23.1 and 9.29.1 — upgrading does not fix it.

Fix

• testHubModule.sendTestSessionEvent: read the live driver.sessionId (via the already-present getDriver()) instead of the cached id; fall back to the cached state for LTS local-Selenium instances.
• Register a TEST/POST binding so the per-test session event is (re)sent after the reload finalizes the session; the binary does last-write-wins per test_run.uuid.
• Retain the onReload CREATE re-fire.

Verification

Tier-1 App Automate repro (WikipediaSample, pipe device-pool, reloadSession() between tests): per-test frameworkSessionId distinct 1 → 3, each test bound to its own session. Confirmed for both in-body and beforeTest-hook reload patterns (fixed builds c51d4754, 50c5cd10) vs broken baseline (988f1517, distinct=1).

Follow-on (not in this PR)

A separate App Automate session-NAMING path (setSessionName / _update(oldSessionId)) shows a similar off-by-one; distinct from the Observability session-binding fixed here.

🤖 Generated with Claude Code

[harshit-browserstack]

Automated SDK PR Review

Verdict: 🟡 Needs changes before merge

Summary: 1 critical · 2 warnings · 2 suggestions across 4 files reviewed.

Root-cause analysis and repro evidence are solid, and the live-session-id read fix is the right shape. The one blocking concern is reusing the CREATE/POST lifecycle event mid-session to force a refresh — that event may carry side effects beyond session-id capture that shouldn't fire twice.

See inline comments below for full Problem and Suggested Fix detail on each finding.

Generated by Automated SDK PR review.

[harshit-browserstack]

🔴 Critical — [CORRECTNESS] Re-firing CREATE/POST mid-session may trigger side effects beyond session-id capture

Problem

onReload now re-invokes the full CREATE/POST lifecycle event (automationFramework.trackEvent(AutomationFrameworkState.CREATE, HookState.POST, ...)) purely to force WebdriverIOModule.onDriverCreated to re-run and refresh KEY_FRAMEWORK_SESSION_ID. That event was designed to fire exactly once, at real session creation.

If onDriverCreated (or anything else observing CREATE/POST) does more than write session state — e.g. emits a "session created" log/telemetry event, registers listeners, increments a session counter, or notifies the dashboard — all of that now fires a second time on every reloadSession() call within the same worker. The diff and tests only assert that trackEvent was called with the right arguments; nothing verifies that re-invoking it is side-effect-safe for a session that's already running.

Suggested Fix

Verify WebdriverIOModule.onDriverCreated's full body is idempotent for repeated calls within one worker lifetime (i.e. it only ever touches state that's safe to overwrite, with no "fire once" side effects). If it isn't, extract a narrower "refresh session id/capabilities" path instead of reusing the full CREATE/POST lifecycle event for this refresh.

Confidence: 🟡 Subjective/contextual — depends on onDriverCreated's full implementation, which isn't visible in this diff; needs verification against the actual method body.

[harshit-browserstack]

⚠️ Warning — [ERROR-HANDLING] Unguarded await on trackEvent inside onReload

Problem

await automationFramework.trackEvent(AutomationFrameworkState.CREATE, HookState.POST, { browser: this._browser, hubUrl: this._config.hostname })

This call has no try/catch. If it throws (CLI hiccup, binary temporarily unavailable, network issue), the exception propagates out of onReload and the rest of the method — including session name/status handling below — never runs. That fails the reload/test outright due to an instrumentation error, which conflicts with the general BrowserStack SDK principle that instrumentation failures must not break the customer's test run.

Suggested Fix

Wrap the call in a try/catch, log at debug/warn on failure, and let onReload continue regardless:

if (BrowserstackCLI.getInstance().isRunning()) {
    const automationFramework = BrowserstackCLI.getInstance().getAutomationFramework()
    if (automationFramework) {
        try {
            await automationFramework.trackEvent(AutomationFrameworkState.CREATE, HookState.POST, { browser: this._browser, hubUrl: this._config.hostname })
        } catch (error) {
            this._bsLogger?.debug?.(`onReload: failed to re-anchor session identity: ${error}`)
        }
    }
}

Confidence: 🟡 Subjective/contextual — depends on the team's policy for how strictly instrumentation calls must be isolated from test-affecting control flow, but matches the SDK's stated graceful-degradation principle.

[harshit-browserstack]

⚠️ Warning — [CORRECTNESS] _lastReportedSessionId is a single scalar, not keyed per instance

Problem

private _lastReportedSessionId?: string

TestHubModule appears to be a single shared module instance per worker. _lastReportedSessionId holds only the last session bound across whichever autoInstance most recently called sendTestSessionEvent. In a multiremote setup (multiple concurrent browser instances within one worker), one instance's session id can overwrite this field, causing onAfterTestSession's "did the session change" comparison to be evaluated against the wrong instance's last-known session — leading to either a spurious re-bind or, worse, a skipped re-bind for a genuinely reloaded session.

Suggested Fix

If multiremote is a supported scenario for this module, key _lastReportedSessionId by the instance/session ref (e.g. Map<string, string> keyed by autoInstance.getRef()) instead of a single scalar.

Confidence: 🟡 Subjective/contextual — depends on whether this module is ever used with multiremote/multiple concurrent instances per worker; not verifiable from this diff alone.

[harshit-browserstack]

⚠️ Warning — [CORRECTNESS] Silent skip when live session read fails, no fallback to cached state

Problem

if (!liveSessionId || liveSessionId === this._lastReportedSessionId) {
    return
}

If the live driver.sessionId read fails or is unavailable, liveSessionId is '' (caught above) and this unconditionally returns — there's no fallback to the cached KEY_FRAMEWORK_SESSION_ID state the way sendTestSessionEvent does. If a reload genuinely happened but the live read transiently fails for this particular test, the stale PRE binding for that test is never corrected, silently reproducing part of the original bug for that one test.

Suggested Fix

On a failed/empty live read, fall back to the cached state (mirroring the fallback already used in sendTestSessionEvent) and still compare against _lastReportedSessionId before deciding whether to skip:

if (!liveSessionId) {
    liveSessionId = (AutomationFramework.getState(autoInstance, AutomationFrameworkConstants.KEY_FRAMEWORK_SESSION_ID)?.toString() ?? '')
}
if (!liveSessionId || liveSessionId === this._lastReportedSessionId) {
    return
}

Confidence: 🟡 Subjective/contextual — the failure window this covers is narrow (live read failing specifically on a test where a reload also happened), but the current code has no fallback at all for it.

[harshit-browserstack]

💡 Suggestion — [MAINTAINABILITY] Duplicated “live driver session, fallback to cached state” logic

Problem

The pattern of trying the live driver.sessionId via getDriver() and falling back to the cached KEY_FRAMEWORK_SESSION_ID state on failure/empty appears near-identically in both onAfterTestSession (here) and sendTestSessionEvent further down this file. As the fix evolves (e.g. addressing the fallback gap noted in the other comment), the two copies are likely to drift out of sync.

Suggested Fix

Extract a small shared helper, e.g. getLiveOrCachedSessionId(autoInstance): string, and call it from both sites.