SDK-6983: upload SDK logs and emit kill telemetry on signal termination

[kamal-kaur04]

What is this about?

Signal-terminated wdio runs (SIGTERM/SIGINT — Ctrl-C, CI cancellation) lose their SDK-log tarball and, on the v9 line, emit no SDKTestSuccessful at all — leaving killed builds undebuggable (no log object in S3) and invisible to query-side kill detection. Fleet impact quantified under SDK-6983/SDK-6969 (Bucket E; WDIO-mocha is the largest terminated-run cohort, and killed wdio builds dominate the "logs not found" residue).

Kill-matrix validation on production packages showed: event drain and (post-test) stopBuild survive a kill, but the logs.tar.gz upload only happens in launcher.onComplete, which a kill either never reaches or cuts short — 3/3 killed cucumber builds and 2/3 killed mocha builds had no S3 log object.

Changes:

• Capture interrupt signals on BrowserStackConfig (killSignal) — the v9 line previously had no signal awareness at all.
• Kill telemetry: SDKTestSuccessful now carries finishedMetadata: { reason: 'user_killed', signal } when the run was signal-terminated (rides the same field the session-linking dashboard already reads for session_reloaded).
• Log-upload rescue: the exit-time detached cleanup.js now 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.
• funnelDataSent regression fix: the flag was set after ANY funnel event (including SDKTestAttempted at startup), permanently disarming the exit-time SDKTestSuccessful resend — killed v9 runs sent no finish event at all. Now only the finish event sets it.
• Credential-redaction hazard: fireFunnelRequest redacts funnelData credentials in place; cleanup now snapshots them before the funnel send (the log-upload rescue otherwise authenticates as [REDACTED]).

Validation (wdio 9, mocha + cucumber, mid-run SIGTERM): killed builds now land their full log tar in S3 (verified via the SDK-logs bucket; e.g. 202KB mocha tar, and a very-early cucumber kill still rescued), funnel carries user_killed/SIGTERM, exit code 143; baselines unaffected (single upload, no behavior change). Evidence: workspace work/tra-session-linking-instability-split/E/FIX-flush-on-signal-validation.md.

Companion PR (node SDK direct flow, same lane): browserstack/browserstack-node-agent#2344.
Note for the v8 line: the same cleanup rescue should be cherry-picked onto the v8 branch when it lands (v8 has signal capture + telemetry already, but the same missing log upload).

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 correctly reported with their termination reason.

Release notes (internal):

• Signal-terminated runs: capture killSignal, emit SDKTestSuccessful with finishedMetadata {reason: user_killed, signal}, and rescue the logs.tar.gz upload from exit-time cleanup.js (--uploadLogs, gated by logsUploaded). Fixed funnelDataSent being set by SDKTestAttempted (killed runs emitted no finish event) and snapshot funnelData credentials before fireFunnelRequest's in-place redaction.

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.

[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) — 4 inline finding(s). Full report in the PR comment below. Verdict: Failed - see PR comment.

[07souravkunda]

HIGH — signal handlers never re-exit. These process.on(sig,…) handlers only set the kill flag; attaching a listener suppresses Node's default termination. SIGTERM (the primary CI-cancellation path) and SIGHUP/SIGQUIT/SIGABRT no longer exit the process — it hangs until CI sends SIGKILL, which fires no 'exit' event, so the cleanup child never spawns and logs are not uploaded. After stamping kill metadata, drive a deterministic exit (re-raise via process.kill(process.pid, sig) or process.exit(128+n)) without preempting WDIO's own graceful SIGINT/SIGTERM shutdown.

[kamal-kaur04]

Fixed in 9f48766 — valid catch. The listener now arms an unref()'d 5s grace timer after stamping the signal and forces process.exit(128 + n) (SIGTERM→143) if nothing else has terminated the process. WDIO's own graceful shutdown gets first shot (a naturally exiting process is never held open — timer is unref'd); process.exit still fires the 'exit' listener, so the cleanup child spawns and the log/funnel/build-stop rescue runs. 5s stays inside typical CI kill grace (e.g. GitHub Actions ~7.5s before SIGKILL). Unit-tested with fake timers (no exit pre-grace, exit 143 post-grace).

[07souravkunda]

MEDIUM — finished_metadata shape. Sent here as an array [{reason,signal}], but the funnel path sends an object {reason,signal}. No existing precedent for finished_metadata on the HTTP builds/stop payload in main — confirm the server schema and align the shape, else the kill reason is silently dropped.

[kamal-kaur04]

Checked against the server schema — the array is correct for this payload, no change made. The HTTP builds/stop endpoint takes finished_metadata as an array: the node-agent direct flow sends finished_metadata: [{reason, signal, failure_data}] (browserstack-node-agent testhubHandler.js), and the binary's own stopBuild builds finished_metadata: [...] with [] default (browserstack-binary packages/@browserstack/testhub/index.js). The object shape you saw is the EDS funnel event (finishedMetadata inside SDKTestSuccessful event props) — a different destination with its own schema. Both destinations verified end-to-end in the kill-matrix validation (BQ web_events shows the object; build record gets the array via the stop).

[07souravkunda]

LOW — duplicate import. Second import … from './constants.js' (line 6 is the first). Merge into one (no-duplicate-imports). The v8 port #87 already has this merged.

[kamal-kaur04]

Fixed in 9f48766 — merged into a single ./constants.js import.

[07souravkunda]

LOW — logsUploaded set only on a truthy response. If the upload succeeds but returns an empty body, the flag stays false and the detached cleanup child re-uploads. Wasteful, not harmful.

[kamal-kaur04]

Fixed in 9f48766 — and your comment surfaced a worse inverse case: a truthy server rejection (response.status !== 'success', which uploadLogs records as a failure) also set logsUploaded = true, disabling the rescue for a build whose upload never landed. Now delivered = !!response && !(response.status && response.status !== 'success') gates the flag. The empty-body-success case can't be distinguished from a transport failure (nodeRequest returns undefined for both on this path), so it stays rescue-eligible — wasteful-but-safe as you noted. Unit-tested (rejection → flag false; success/no-status → true).

[07souravkunda]

Claude Code Review — PR #86

SDK-6983: upload SDK logs and emit kill telemetry on signal termination
Head a781b67 → base main

Rescues the SDK-log upload and stamps kill telemetry when a WDIO run is interrupted by a signal. Registers signal handlers that record the kill signal on config + BROWSERSTACK_SDK_KILL_SIGNAL; shouldCallCleanup pushes --uploadLogs <uuid> so the detached cleanup child uploads logs; and finished_metadata/finishedMetadata = {reason:'user_killed', signal} is stamped on the build-stop API, the funnel finish event, and the gRPC stop request. Well-scoped, each new unit has a test.

Findings

1 · HIGH · Correctness — signal handlers never re-exit — src/exitHandler.ts:70-74
The new process.on(sig, …) handlers only set the kill flag; they never process.exit() or re-raise. In Node, attaching a listener suppresses the default termination action, so nothing else drives the process to exit — notably SIGTERM (the primary CI-cancellation path, which WDIO's CLI does not handle) and SIGHUP/SIGQUIT/SIGABRT. The run keeps going; if any handle stays alive (gRPC socket, BrowserStackLocal child, Percy, timers) it hangs until CI escalates to SIGKILL — uncatchable, fires no 'exit' event, so the detached cleanup child is never spawned and logs are still not uploaded. Net: in the exact scenario this PR targets, the fix may not fire, and cancellation can get worse (a grace-period hang first).
Fix: after stamping kill metadata, drive a deterministic exit — remove the listener and re-raise (process.kill(process.pid, sig)) or process.exit(128+signum) — so 'exit' runs and cleanup spawns, without preempting WDIO's own graceful SIGINT/SIGTERM shutdown. Verify against a real interrupted wdio run.

2 · MEDIUM · Contract — finished_metadata array vs object shape — src/util.ts:743 vs src/instrumentation/funnelInstrumentation.ts:174
The build-stop HTTP path sends an array [{reason,signal}], while the funnel path sends an object {reason,signal}. There is no existing precedent for finished_metadata on the HTTP builds/stop payload in main, so the array shape is net-new and unverifiable from the repo. If the endpoint expects an object, the kill reason is silently dropped server-side.
Fix: confirm the builds/stop schema against the server/binary contract and align the shape.

3 · LOW · Quality — duplicate import from ./constants.js — src/exitHandler.ts:6 & :11
Two separate import { … } from './constants.js' statements. Merge into one (no-duplicate-imports). (The v8 port #87 already has this merged.)

4 · LOW · Efficiency — logsUploaded set only on a truthy response — src/launcher.ts:817
If the upload succeeds server-side but returns an empty body, the flag stays false and the detached cleanup child re-uploads. Wasteful, not harmful.

Verified clean

• gRPC exitSignal/exitReason — proto StopBinSessionRequest defines exit_signal (field 3) / exit_reason (field 4); the camelCase ts-proto binding is correct. Not a defect.
• Normal/graceful path unchanged (onComplete awaited before exit → no double upload, no cleanup child for passing runs).
• Credential snapshot taken before in-place redaction, with env fallback; child inherits env.
• Kill-metadata provenance consistent across funnel / HTTP / gRPC; log files persist in the kill path.
• New units are test-covered (gap: the signal-handler exit behavior itself — finding Browserstack wdio service code #1).

---

Verdict: FAIL — one High-severity correctness finding (#1) that also undermines the PR's own goal.

_{Automated review. Findings verified against the branch head and the proto/API contracts reachable from the repo; finding #2 needs a server-side schema confirmation.}

[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 #86 (re-review)

Head 9f48766 → base main. Re-review of the fix commit 9f48766 addressing the prior review.

Prior findings — all resolved

#1 (HIGH) signal handlers never re-exit → ✅ resolved. exitHandler.ts now arms setTimeout(() => process.exit(128 + SIGNAL_EXIT_CODES[sig]), 5000) with timer.unref(). This is the right shape: the 5 s grace window lets WDIO's own graceful SIGINT/SIGTERM shutdown run first, the forced conventional exit fires before CI escalates to SIGKILL (so the 'exit' handler runs and the detached cleanup child spawns), and unref() never holds a naturally-exiting process open. New unit tests cover the forced-exit path.

#2 (MEDIUM) finished_metadata array vs object → ✅ resolved (not a defect). Verified against the consumer: the O11Y collector serving PUT /api/v1/builds/{uuid}/stop uses finished_metadata: [] in its ingest template (observability/api sdkIngestEvents), the SDK team's own contract test PUTs an array to that exact path with expectStatus(200), and the pipeline DTO types the field as a generic JsonNode. The SDK's [{reason, signal}] is the correct wire shape for this endpoint; the funnel object shape is a separate analytics sink.

#3 (LOW) duplicate ./constants.js import → ✅ resolved. Merged into one import.

#4 (LOW) logsUploaded set on any truthy response → ✅ resolved. Now gated on delivered = !!response && !(response.status && response.status !== 'success'), so a non-success response stays eligible for the cleanup rescue.

Non-blocking note

• Forced-exit grace = 5000 ms. Sensible default (under the common ~10 s docker-stop / CI grace). If a legitimate teardown ever needs longer than 5 s it would be cut short — worth keeping in mind, not worth changing.

---

Verdict: PASS — the High-severity issue is fixed correctly and every prior finding is resolved.

[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 9f48766.

The prior High-severity signal-handler finding is fixed correctly (setTimeout(() => process.exit(128+n), 5000).unref()), and every other prior finding is resolved. 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.