fix(SDK-7061): resilient TRA build-stop — retry + success-gated flag + synchronous shutdown stop

[pri-gadhiya]

What is this about?

Fixes SDK-7061: WebdriverIO runs where the Test Observability / TRA build appears "stuck" and only closes via the ~60-min server-side inactivity timeout, even though the run finished.

Root cause: the build-stop (PUT /api/v1/builds/<uuid>/stop) was sent once, best-effort — no retry, and it did not check the HTTP status. If that request failed/timed out, or the WDIO launcher was interrupted (CI cancel/timeout) before it was sent, the build was never marked complete.

This PR makes the build-stop resilient (verified on WebdriverIO-cucumber, before/after):

1. util.ts stopBuildUpstream — retries the stop 3× with backoff and treats a non-2xx response as a failure (previously a non-2xx was logged as success).
2. launcher.ts onComplete — buildStopped is set only when the stop actually succeeds, so a failed stop stays recoverable.
3. exitHandler.ts — synchronous/blocking shutdown stop. On SIGTERM/SIGINT/SIGHUP/uncaughtException/exit, a blocking spawnSync child issues the stop PUT and the process waits for it before exiting — the Node analog of Java's blocking Runtime.getRuntime().addShutdownHook. This closes the build on a graceful CI cancel/timeout. (An earlier async handler was verified not to work here: WDIO's launcher calls process.exit before an async PUT can land; the synchronous approach fixes that. The JWT is read from the inherited environment, never written to disk.)

Consistency: this mirrors how browserstack-javaagent (JVM shutdown hook) and browserstack-node-agent (process.on signal suite) handle build-stop on termination — an in-process shutdown hook, no separate watchdog process.

Known boundary (out of scope here, same as java/node): a true uncatchable kill -9/OOM of the launcher runs no handler, so that build still relies on a server-side inactivity/heartbeat auto-close — tracked separately (the shared browserstack-binary stopBuild path has the same retry gap and is the highest-leverage place to harden across all binary-flow SDKs).

Related Jira task/s

• https://browserstack.atlassian.net/browse/SDK-7061
• Related (server-side appearance analysis): https://browserstack.atlassian.net/browse/TRAP-4046

Release (mandatory for every PR — required for the ready-for-review label)

Version bump: (required — tick exactly one)

• minor (backwards-compatible feature)
• patch (bug fix or other small change)

Release notes type: (optional)

• New Feature
• Bug Fix
• Other Improvement

Release notes (customer-facing): (optional but encouraged)

• Fixed WebdriverIO test results sometimes not appearing (builds staying "in progress") when the build-completion signal failed or the test runner was interrupted.

Release notes (internal): (required — engineer-facing; what actually changed / why)

• stopBuildUpstream hardened: 3× retry + backoff + response.ok check (was a single best-effort PUT; non-2xx was logged as success).
• buildStopped gated on stop success (was set unconditionally), so the shutdown path can re-attempt a failed stop.
• Added a synchronous/blocking shutdown stop in exitHandler.ts (SIGTERM/SIGINT/SIGHUP/uncaughtException/exit via spawnSync) — Java addShutdownHook analog; closes the build on graceful interruption. JWT read from env, not persisted. Bounded: blocks exit ≤15s if the collector is unreachable (teardown-only).

Checklist

• Ready to review
• Has it been tested locally?

PR Validations

Run Tests: Comment RUN_TESTS to trigger sanity tests.

[pri-gadhiya]

RUN_TESTS

[pri-gadhiya]

🔴 SDK PR Review — SDK-7061 (automated stack:sdk-pr-review)

Verdict: 🔴 Blocking — the util.ts retry/response.ok hardening and the launcher.ts buildStopped gating are correct and match sibling-SDK precedent (javaagent addShutdownHook, node-agent process.on suite); the new synchronous shutdown-stop in exitHandler.ts has one grounded correctness bug in its guard clauses, plus no test coverage for the three changed behaviors.

🔴 Blocking (must-fix before merge)

packages/browserstack-service/src/exitHandler.ts:81-90 — stopBuildSyncBlocking's early-return guards run outside its own try, and can throw before BrowserStackConfig is initialized, defeating the "never throws" contract.

• BrowserStackConfig.getInstance() called bare returns the singleton, which is undefined until the launcher constructor reaches getInstance(_options, _config, capabilities) at launcher.ts:122.
• But setupExitHandlers() arms the uncaughtException listener at launcher.ts:91 — 31 lines earlier. Anything that throws synchronously in that window fires the new uncaughtException handler → stopBuildSyncBlocking('uncaughtException') → config is undefined → config.testObservability throws a second, unguarded TypeError.
• A throw from inside an uncaughtException listener is fatal in Node — it crashes outside the intended process.exit(1) path and masks the original error.
• Suggested resolution: add if (!config) { return } right after getInstance() (or move the three early-return checks inside the existing try) so the function is unconditionally throw-safe regardless of constructor init order.

🟠 Non-blocking (strongly recommended)

• No tests for any of the three changed behaviors (gh pr diff touches only the 3 source files + the changeset). util.test.ts doesn't assert the non-2xx→retry behavior (and the existing failure test now sleeps ~1.5s of real backoff with no new assertion); launcher.test.ts doesn't assert the buildStopped gating; exitHandler.ts has no test file. Given the PR's value is these behaviors, worth closing before merge.
• Narrow double-send window: if SIGTERM/SIGINT arrives while onComplete's await stopBuildUpstream() is still in flight (before buildStopped is set), the sync path fires a second concurrent stop. Likely harmless (idempotent, bounded, best-effort) but a real race this PR introduces — worth an acknowledging comment or an extra "stop in flight" guard.
• CLI-path scoping: stopBuildSyncBlocking early-returns when the CLI is running (intentional), and handleCLICleanup() only kill()s the child without awaiting its stop. Scope the release note to the direct-HTTP path so it doesn't imply CLI-mode users get the same guarantee.

💡 Nits

• launcher.ts — unchecked cast on the stopResult shape (both stop functions pass through wrappers that erase the return type; correct today, no compiler safety net).
• Inline sync-stop script exits 0 on missing UUID/JWT/endpoint (reads as success). Unreachable today (parent pre-checks) but consider exiting non-zero for defense-in-depth.
• SIGHUP registered on win32 where it has limited semantics — harmless, worth a comment since the platform branch below shows platform-awareness.

✅ Confirmed clean (traced, not just diff-read)

Interaction with merged #77 (observability-disable-on-blocked-buildstart) — no conflict (stop path gates on UUID/JWT env, not .enabled); single handler registration (launcher only, no per-worker duplication); no watchdog remnants; JWT never in argv (env-read inside the child); spawnSync retry budget (13.5s) fits the 15s timeout; framework-agnostic (cucumber/mocha/jasmine); not the isRunning() bootstrap anti-pattern; tsc --noEmit + eslint pass.

Bottom line: one real must-fix (the unguarded throw), then this is good to go; the test coverage is a strong recommendation.

[pri-gadhiya]

✅ SDK PR Review (re-review) — SDK-7061 · cec4d73

Verdict: ✅ Green — the prior 🔴 blocking finding is resolved; good to merge from a correctness standpoint.

The earlier 🔴 — stopBuildSyncBlocking dereferencing BrowserStackConfig.getInstance() while it's still undefined during the early uncaughtException window (handler armed at launcher.ts:91, config initialized at launcher.ts:122) — is fixed in cec4d73: the function now guards with if (!config) { return } as its first statement, before any other access. Traced all three entry paths (signal handler, uncaughtException handler, exit handler) — all route through the guarded function and are safe. Optional chaining on config.testObservability?.buildStopped closes the related soft spot; the one remaining write (config.testObservability.buildStopped = true) is reached only after the guard and is inside a try/catch, so it can't escape as an unhandled throw either.

tsc --noEmit, npm run build, and eslint on exitHandler.ts are all clean on Node 20. No new issues introduced by cec4d73 (it's an isolated 4-line guard).

Non-blocking, still open (deferred — follow-up hardening, not blockers):

• No test coverage yet for the three shutdown behaviors (signal / exit-hook / uncaughtException sync stop).
• Narrow signal-vs-onComplete double-send window — guarded by buildStopSentOnSignal / buildStopped, not structurally eliminated.
• Release note doesn't explicitly scope out the CLI-managed build-stop path (stopBuildSyncBlocking early-returns when the CLI is running).
• Minor naming / log-level nits.

[anish353]

🔎 SDK PR Review — follow-up pass on 6b39a69 (stack:sdk-pr-review)

Building on the earlier self-review + the cec4d73 re-review — the original 🔴 (unguarded config deref in the early uncaughtException window) is confirmed fixed, and the shutdown-behavior tests added since are non-vacuous. ✅ on all of that.

This pass traced the new exitHandler.ts shutdown path against the actual @wdio/cli launcher source and the async-exit-hook package (not just this repo's diff), and surfaced two blocking issues the earlier passes didn't cover. Both sit in the exact interruption scenarios SDK-7061 targets.

🔴 Critical 1 — the raw signal handlers race @wdio/cli's own graceful shutdown

@wdio/cli's Launcher constructor registers async-exit-hook first (wdio-cli/src/launcher.ts → exitHook(this._exitHandler.bind(this))), and its handler starts this.runner.shutdown() — the comment there: "prevent having dead driver processes." BrowserstackLauncherService is instantiated later, so this PR's registerSignalHandlers() listener fires second on SIGINT/SIGTERM, and:

• stopBuildSyncBlocking() → spawnSync(...) fully blocks the event loop (up to the ~13.5s child-retry budget / 15s cap), during which runner.shutdown()'s promise chain cannot advance;
• the moment spawnSync returns, the handler calls process.exit(0) unconditionally, winning the race — before WDIO's session cleanup finishes and before async-exit-hook emits the conventional 130/143.

Net, on every Ctrl+C/SIGTERM mid-run: (a) the conventional signal exit code is masked with a hardcoded 0 (CI that gates on "was this cancelled?" sees a false-clean 0), and (b) WDIO's own WebDriver-session cleanup is aborted mid-flight — the dead-driver-process regression the upstream hook exists to prevent. Fires regardless of CLI vs direct-HTTP mode (the CLI/buildStopped guards only gate the stop body; registerSignalHandlers() and the process.exit(0) are unconditional). Separately, intercepting SIGABRT/SIGQUIT → exit(0) silently swallows genuine crash signals.

Suggested direction: register the build-stop through async-exit-hook's own exitHook() API so it participates in the same ordered, code-preserving exit (it supports sync + async hooks) — or, if an independent listener is kept, never call process.exit() from it; only fire the stop and let WDIO/async-exit-hook own the exit.

🔴 Critical 2 — the synchronous shutdown-stop bypasses this repo's own proxy support

The SYNC_STOP_SCRIPT child issues the PUT via raw require('http'/'https') — no proxy awareness. The async path it mirrors (util.ts::stopBuildUpstream) goes through fetchWrapper.ts, which honors HTTP_PROXY/HTTPS_PROXY/NO_PROXY via undici's ProxyAgent — the reason that wrapper exists. In proxy-gated enterprise CI (the same environments that emit job-timeout/cancel SIGTERMs this PR is meant to handle), the blocking stop attempts a direct connection, exhausts all 3 retries (~13.5s), and never closes the build — defeating the fix precisely where it's needed most.

Suggested fix: have the child honor the proxy env the same way fetchWrapper.ts does (undici is already a dependency and require-able from CJS), or pass the resolved proxy target down via env like STOP_ENDPOINT_ENV.

Already tracked (no action asked here)

• The signal-vs-onComplete double-send window is the same one already noted as a deferred non-blocker above — unchanged.
• Minor residual: the isRunning() / env-lookup guard clauses still sit outside stopBuildSyncBlocking's try (the config guard — the original 🔴 — is correctly fixed); neither throws today, but moving the whole chain inside the try would fully honor the "never throws" contract the comment promises.

Net

The util.ts retry/response.ok + launcher.ts buildStopped-gating half remains clean and well-tested. The two Criticals above are both in exitHandler.ts and both objectively grounded (traced to @wdio/cli/async-exit-hook and to this repo's own fetchWrapper.ts) — worth resolving before merge so the shutdown-stop actually lands in the CI-cancel / proxied-network cases it targets, without regressing WDIO's own shutdown.

---

_{🤖 Generated with Claude Code · follow-up delta on top of the earlier automated passes · comment-only, no gate/status posted}

[github-actions]

⚠️ The ready-for-review label was removed because the PR body does not pass the release gate:

• Tick - [x] Ready to review once the PR body is complete.

How to get (and keep) the ready-for-review label:

1. Make sure the PR body follows the repo PR template (all sections present).
2. In the ## Release section, tick exactly one version bump (minor or patch).
3. Fill in at least one bullet under **Release notes (internal):** (engineer-facing; feeds the internal CHANGELOG.md).
4. Tick - [x] Ready to review in the checklist.
5. Apply the ready-for-review label yourself — this workflow does NOT add it for you; it only removes it if the body stops passing, and the check stays red until the label is present.

[pri-gadhiya]

RUN_TESTS

[osho-20]

Claude Code PR Review

PR: #99 • Head: e345fba • Reviewers: stack:sdk-pr-review, stack:sdk-code-review

Summary

Makes the TRA/TestHub build-stop resilient (SDK-7061): stopBuildUpstream now retries the stop PUT up to 3× with backoff and treats non-2xx as failure; launcher.onComplete only sets buildStopped=true when the stop actually succeeded; and a new async-exit-hook-registered shutdown hook best-effort stops the build on SIGTERM/SIGINT/SIGHUP when onComplete never ran.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	JWT/UUID read from env vars only
High	Security	Authentication/authorization checks present	Pass	Bearer JWT sent on the stop PUT; guarded absent-JWT no-op
High	Security	Input validation and sanitization	N/A	No new user input surfaces
High	Security	No IDOR — resource ownership validated	N/A	Client-side SDK; server enforces ownership
High	Security	No SQL injection (parameterized queries)	N/A	No database access
High	Correctness	Logic is correct, handles edge cases	Fail	Retry-loop fetch has no timeout — a stalled socket hangs onComplete() (Finding 1)
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Failures caught, logged, surfaced via {status:'error'}; shutdown hook never throws
High	Correctness	No race conditions or concurrency issues	Fail	Exit-hook stop and onComplete() stop can fire concurrently; failure latch never resets (Finding 2)
Medium	Testing	New code has corresponding tests	Pass	New exitHandler suite + launcher gating tests + util retry tests, deterministic fake timers
Medium	Testing	Error paths and edge cases tested	Pass	Success/failure/latch/guard paths covered; the cross-module concurrent race of Finding 2 is untested
Medium	Testing	Existing tests still pass (no regressions)	N/A	Suite not executed in this review; pre-existing util tests were updated to match the new retry contract
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Bounded 3 attempts, ~1.5s max backoff — but 4xx is retried pointlessly (Finding 3)
Medium	Performance	Long-running tasks use background jobs	N/A	Shutdown work bounded by async-exit-hook's ~10s force-exit budget
Medium	Quality	Follows existing codebase patterns	Pass	One divergence: nodeRequest() in the same file uses AbortController+timeout, the new retry loop doesn't (root cause of Finding 1)
Medium	Quality	Changes are focused (single concern)	Pass	All changes serve SDK-7061; changeset patch bump appropriate
Low	Quality	Meaningful names, no dead code	Pass	Clear naming; latch semantics slightly misleading (see Finding 2b)
Low	Quality	Comments explain why, not what	Pass	Comments carry design rationale; hoisting claim independently verified as true
Low	Quality	No unnecessary dependencies added	Pass	async-exit-hook@^2.0.1 is already a @wdio/cli dependency (verified v5→v9), so it hoists to the existing copy

Findings

• File: packages/browserstack-service/src/util.ts:748
• Severity: High
• Reviewer: stack:sdk-pr-review
• Issue: The new retry loop calls fetch with no AbortController/timeout. On the signal path async-exit-hook's ~10s force-exit budget bounds it, but on the normal onComplete() path (launcher.ts:637) nothing does — a half-open connection or hung proxy stalls attempt 1 forever, hanging the whole WDIO process and recreating the "build stays running" failure class this PR fixes. nodeRequest() in the same file (util.ts:207-220) already shows the correct pattern.
• Suggestion: Wrap each attempt in the same AbortController + setTimeout abort pattern nodeRequest() uses (or route the stop through nodeRequest()).

---

• File: packages/browserstack-service/src/exitHandler.ts:75-97 (with launcher.ts:637)
• Severity: Medium
• Reviewer: stack:sdk-pr-review + stack:sdk-code-review (consolidated)
• Issue: (a) The buildStopInFlight latch is module-local to exitHandler.ts; onComplete()'s own stopBuildUpstream() call doesn't consult or set it. A signal arriving while onComplete() is mid-stop (a real window — up to ~1.5s with retry backoff) fires a second concurrent stop PUT; idempotency is delegated to the server unverified, and the guard is only once-per-path, not once-across-paths. (b) The latch is set before the await and never reset, so after a failed shutdown stop, a second signal's re-stop is a permanent no-op while buildStopped is still false. Neither behavior is covered by a test.
• Suggestion: Promote the "stop dispatched" latch to shared state both call sites check-and-set (e.g. alongside buildStopped on testObservability), and reset buildStopInFlight in a finally on failure so only success latches.

---

• File: packages/browserstack-service/src/util.ts:755-771
• Severity: Low
• Reviewer: stack:sdk-code-review + stack:sdk-pr-review (consolidated)
• Issue: Every non-2xx is treated as retryable, so terminal 4xx failures (401 expired JWT, 404 unknown build) burn all 3 attempts + ~1.5s of shutdown-critical backoff, and since buildStopped stays false, the detached cleanup.js re-stop repeats the same doomed 3 attempts in a second process (≈6 failed PUTs where there used to be 1).
• Suggestion: Retry only network errors and 5xx; treat 4xx as terminal.

---

• File: packages/browserstack-service/src/exitHandler.ts:94 (scope questions)
• Severity: Low
• Reviewer: stack:sdk-code-review
• Issue: Three scope confirmations for the author: (1) the CLI path is explicitly skipped in stopBuildOnShutdown — if the CLI child doesn't guarantee its own build-stop on a mid-run kill, the SDK-7061 symptom persists for CLI users; (2) peerDependencies still allow @wdio/cli v5–7, where async-exit-hook usage was only verified on v8/v9 — on older majors this package's own copy would install signal handlers that drive process.exit; (3) only signals are hooked — a crash via uncaughtException won't trigger the shutdown stop, though the changeset wording ("test runner was interrupted") could be read to include it.
• Suggestion: Confirm CLI-path coverage and v5–7 intent in the PR description; state the uncaughtException exclusion explicitly.

---

• File: packages/browserstack-service/src/exitHandler.ts:94
• Severity: Low
• Reviewer: stack:sdk-pr-review
• Issue: The BrowserstackCLI.getInstance().isRunning() skip inherits a known ambiguity (can report true before the CLI child has actually connected, under BROWSERSTACK_CLI_ENV=development). Pre-existing pattern, not introduced by this PR — noted for awareness only.
• Suggestion: None required for this PR.

Independently verified as sound (not findings): the async-exit-hook hoisting/ordered-exit design claim (checked against the published async-exit-hook@2.0.1 source and @wdio/cli v5.18.7→v9.30.0, all pinning ^2.0.1); the hand-rolled .d.ts matches the real package API; TESTOPS_BUILD_COMPLETED_ENV is set at build creation so it doesn't block the shutdown stop; return-value passthrough survives both instrumentation wrappers; the success-gated buildStopped flag correctly re-arms the cleanup.js re-stop path.

---

Verdict: FAIL — one High-severity correctness issue (unbounded fetch in the retry loop can hang the normal exit path); the fix is small and the rest of the PR is well-built and well-tested.