SDK-6983: rescue SDK-log upload on signal termination (v8 port of #86)

[kamal-kaur04]

What is this about?

Port of the v9 fix (#86) to the v8 line, for SDK-6983 (Bucket E — terminated runs): a signal-terminated run's logs.tar.gz upload only happens in launcher.onComplete, which a kill never reaches or cuts short — killed wdio-8 builds land with no SDK-log object in S3 (kill-matrix validation on production 8.48.0: 3/3 killed cucumber and 2/3 killed mocha builds had no log object).

The v8 line already has the signal machinery the v9 port had to add (signal capture, user_killed funnel telemetry, killSignal-aware stopBuildUpstream, SIGINT to the CLI child) — this PR adds only what it lacks:

• Log-upload rescue: exit-time detached cleanup.js takes --uploadLogs <clientBuildUuid> and uploads logs.tar.gz when the launcher's own upload never ran — gated by a logsUploaded flag so delivered uploads are never repeated.
• Credential snapshot: fireFunnelRequest redacts funnelData credentials in place; cleanup snapshots them before the funnel send so the rescue doesn't authenticate as [REDACTED].
• CLI-mode stop stamping: forward exitSignal/exitReason (user_killed) on StopBinSessionRequest so the binary stamps finished_metadata on the build stop (the direct-flow stop already stamps it on v8).

Validation (wdio 8 mocha sample, mid-spec SIGTERM, this branch): exit 143, cleanup rescue fired, killed build's full log tar (1.9MB: service + CLI debug logs) present in the SDK-logs S3 bucket; baseline behavior unchanged. Evidence: workspace work/tra-session-linking-instability-split/E/FIX-flush-on-signal-validation.md.

Companion PRs (same lane): #86 (v9), browserstack/browserstack-node-agent#2344, browserstack/browserstack-javaagent#2102.

Related Jira task/s

• https://browserstack.atlassian.net/browse/SDK-6983

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 SDK logs not being uploaded when a test run is interrupted (Ctrl-C or CI job cancellation); interrupted runs are now reported with their termination reason on the build record.

Release notes (internal):

• Exit-time cleanup.js rescues the logs.tar.gz upload for signal-terminated runs (--uploadLogs, gated by logsUploaded); funnelData credentials snapshotted before fireFunnelRequest's in-place redaction; StopBinSessionRequest carries exitSignal/exitReason so the binary stamps finished_metadata (SDK-6983, port of SDK-6983: upload SDK logs and emit kill telemetry on signal termination #86).

Checklist

• Ready to review
• Has it been tested locally?

PR Validations

Run Tests: Comment RUN_TESTS to trigger sanity tests.

🤖 Generated with Claude Code

[github-actions]

🔴 SDK PR Review gate is red. Pending:

• The SDK PR Review Agent has not reviewed the current head commit yet — run the SDK PR Review Agent.

It turns green once the latest SDK PR Review Agent run reports GTG on the current head commit. A native reviewer approval is separately required by branch protection before merge.

[github-actions]

🔴 SDK PR Review gate is red. Pending:

• The SDK PR Review Agent has not reviewed the current head commit yet — run the SDK PR Review Agent.

It turns green once the latest SDK PR Review Agent run reports GTG on the current head commit. A native reviewer approval is separately required by branch protection before merge.

[07souravkunda]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Failed - see PR comment.

[07souravkunda]

HIGH — signal handlers never re-exit. Same as #86: the handler only sets the kill flag + env, never process.exit()/re-raises. SIGTERM/SIGHUP/SIGQUIT/SIGABRT no longer terminate the process — it hangs until CI sends SIGKILL, which fires no 'exit' event, so the cleanup child never spawns and the log-upload rescue this PR adds may not fire in its own target scenario. Drive a deterministic exit after stamping the signal, without preempting WDIO's graceful shutdown.

[kamal-kaur04]

Fixed in aec8d08 — same treatment as #86: after stamping the kill flag/env, the listener arms an unref()'d 5s grace timer that forces process.exit(128 + n) (SIGTERM→143) if nothing else has terminated the process. process.exit fires the 'exit' listener, so the detached cleanup child (and this PR's --uploadLogs rescue) runs even on the hung-shutdown path; a naturally exiting run is unaffected. Unit-tested with fake timers.

[07souravkunda]

LOW — logsUploaded set only on a truthy response. An upload that succeeds with an empty body leaves the flag false, so the detached cleanup child re-uploads. Wasteful, not harmful.

[kamal-kaur04]

Fixed in aec8d08 — also covered the inverse case: a truthy server rejection (response.status !== 'success') previously marked logsUploaded = true, disabling the rescue for an upload that never landed. Now gated on delivered = !!response && !(response.status && response.status !== 'success'). Empty-body success remains indistinguishable from transport failure on this path, so it stays rescue-eligible (wasteful-but-safe). Unit-tested.

[07souravkunda]

Claude Code Review — PR #87

SDK-6983: rescue SDK-log upload on signal termination (v8 port of #86)
Head 50a06fe → base v8

Narrower port of #86: the SDK-log-upload rescue (detached cleanup child re-uploads via --uploadLogs), the logsUploaded guard, the kill-signal env propagation, and the gRPC exitSignal/exitReason forwarding. Does not stamp finished_metadata on the HTTP builds/stop or funnel paths (no util.ts / funnelInstrumentation.ts changes) — so on v8 the kill reason reaches the binary only via the gRPC stop request.

Findings

1 · HIGH · Correctness — signal handlers never re-exit — src/exitHandler.ts:86
Identical to #86: the process.on(sig, …) handlers only set the kill flag + env, never process.exit() or re-raise. Attaching the listener suppresses Node's default termination, so SIGTERM (the primary CI-cancellation path) and SIGHUP/SIGQUIT/SIGABRT no longer terminate the process — it hangs until CI escalates to SIGKILL, which fires no 'exit' event, so the detached cleanup child never spawns and logs are still not uploaded. The rescue this PR adds may not fire in its own target scenario.
Fix: after stamping the kill signal, drive a deterministic exit (remove the listener and re-raise, or process.exit(128+signum)) without preempting WDIO's graceful SIGINT/SIGTERM shutdown. Verify on a real interrupted wdio run.

2 · LOW · Efficiency — logsUploaded set only on a truthy response — src/launcher.ts:756
Same as #86: an upload that succeeds with an empty body leaves the flag false, so the detached cleanup child re-uploads. Wasteful, not harmful.

3 · LOW · Consistency — detached child stdio diverges from #86 — src/exitHandler.ts
#87 spawns the cleanup child with stdio: 'ignore'; #86 uses stdio: 'inherit'. 'ignore' is the safer choice for a detached unref'd child, but the two ports should agree — pick one and apply it to both PRs.

Scope note (not a defect)

On v8 the kill reason is forwarded to the binary only through the gRPC stop request (exitSignal/exitReason) — the HTTP builds/stop and funnel finished_metadata stamping present in #86 is absent here. Confirm this is intentional for v8 (i.e. v8 always uses the gRPC stop path and never the direct-HTTP build stop); otherwise kill telemetry is lost for any v8 run that stops over HTTP.

Verified clean

• gRPC exitSignal/exitReason — matches proto StopBinSessionRequest fields 3/4 (correct binding).
• Single merged ./constants.js import (no duplicate-import issue — cleaner than SDK-6983: upload SDK logs and emit kill telemetry on signal termination #86).
• Credential snapshot before in-place redaction, with env fallback.
• New units test-covered (gap: signal-handler exit behavior — finding Browserstack wdio service code #1).

---

Verdict: FAIL — one High-severity correctness finding (#1), identical to #86.

_{Automated review. Findings verified against the branch head and the proto contract reachable from the repo.}

[github-actions]

🔴 SDK PR Review gate is red. Pending:

• The SDK PR Review Agent has not reviewed the current head commit yet — run the SDK PR Review Agent.

It turns green once the latest SDK PR Review Agent run reports GTG on the current head commit. A native reviewer approval is separately required by branch protection before merge.

[07souravkunda]

Claude Code Review — PR #87 (re-review)

Head aec8d08 → base v8. Re-review of the fix commit aec8d08 addressing the prior review.

Prior findings — all resolved

#1 (HIGH) signal handlers never re-exit → ✅ resolved. Same fix as #86: setTimeout(() => process.exit(128 + SIGNAL_EXIT_CODES[sig]), 5000) with timer.unref() — the grace window preserves WDIO's own graceful shutdown, the forced conventional exit beats CI's SIGKILL so the 'exit' handler runs and the cleanup rescue spawns, and unref() keeps a naturally-exiting process free to exit. New unit tests cover it.

#2 (LOW) logsUploaded set on any truthy response → ✅ resolved. Now gated on delivered = !!response && !(response.status && response.status !== 'success').

Non-blocking notes

• stdio divergence from SDK-6983: upload SDK logs and emit kill telemetry on signal termination #86. SDK-6983: rescue SDK-log upload on signal termination (v8 port of #86) #87 spawns the detached cleanup child with stdio: 'ignore'; SDK-6983: upload SDK logs and emit kill telemetry on signal termination #86 uses stdio: 'inherit'. 'ignore' is the safer choice for a detached unref'd child — consider aligning SDK-6983: upload SDK logs and emit kill telemetry on signal termination #86 to it rather than the reverse. Cosmetic; not a blocker.
• Kill telemetry is gRPC-only on v8. SDK-6983: rescue SDK-log upload on signal termination (v8 port of #86) #87 forwards the kill reason to the binary via the gRPC StopBinSessionRequest (exitSignal/exitReason) but does not stamp finished_metadata on the HTTP builds/stop path (unlike SDK-6983: upload SDK logs and emit kill telemetry on signal termination #86). Fine if v8 always stops via the gRPC path; if any v8 run stops over direct HTTP, the kill reason won't be recorded there. Confirm the intent — not a blocker.

---

Verdict: PASS — the High-severity issue is fixed correctly; only non-blocking consistency/scope notes remain.

[github-actions]

🔴 SDK PR Review gate is red. Pending:

• The SDK PR Review Agent has not reviewed the current head commit yet — run the SDK PR Review Agent.

It turns green once the latest SDK PR Review Agent run reports GTG on the current head commit. A native reviewer approval is separately required by branch protection before merge.

[07souravkunda]

🟢 SDK PR Review Agent — GTG on head aec8d08.

The prior High-severity signal-handler finding is fixed correctly (setTimeout(() => process.exit(128+n), 5000).unref()), and the logsUploaded finding is resolved. Only non-blocking notes remain (stdio consistency with #86; gRPC-only kill telemetry on v8). Full re-review: comment above.

Verdict: PASS. (A native GitHub reviewer approval is still separately required by branch protection before merge.)

[github-actions]

🟢 SDK PR Review gate is green — the SDK PR Review Agent has given a GTG for this PR (the ready-for-review label is present and the latest SDK PR Review Agent run reports success on the current head commit).

A native GitHub reviewer approval is still separately required by branch protection before this PR can merge — this check does not substitute for that.

[github-actions]

🟢 SDK PR Review gate is green — the SDK PR Review Agent has given a GTG for this PR (the ready-for-review label is present and the latest SDK PR Review Agent run reports success on the current head commit).

A native GitHub reviewer approval is still separately required by branch protection before this PR can merge — this check does not substitute for that.