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

packages/browserstack-service/src/cleanup.ts

@@ -1,4 +1,4 @@
-import { getErrorString, stopBuildUpstream } from './util.js'
+import { getErrorString, stopBuildUpstream, uploadLogs } from './util.js'
 import { BStackLogger } from './bstackLogger.js'
 import fs from 'node:fs'
 import util from 'node:util'
@@ -17,6 +17,10 @@ export default class BStackCleanup {
                 const filePath = process.argv[index + 1]
                 funnelData = this.getFunnelDataFromFile(filePath)
             }
+            // Snapshot before sendFunnelData — fireFunnelRequest redacts the
+            // credentials in place.
+            const funnelUser = (funnelData as { userName?: string } | null)?.userName
+            const funnelKey = (funnelData as { accessKey?: string } | null)?.accessKey
 
             if (process.argv.includes('--observability')) {
                 await this.executeObservabilityCleanup(funnelData)
@@ -25,6 +29,12 @@ export default class BStackCleanup {
             if (funnelDataCleanup && funnelData) {
                 await this.sendFunnelData(funnelData)
             }
+
+            // Rescue the SDK-log upload for runs whose launcher never reached
+            // onComplete's upload (signal termination) — after events.
+            if (process.argv.includes('--uploadLogs')) {
+                await this.executeLogsUpload(funnelUser, funnelKey)
+            }
         } catch (err) {
             const error = err as string
             BStackLogger.error(error)
@@ -38,6 +48,23 @@ export default class BStackCleanup {
             BStackLogger.debug(`Error in sending events data ${util.format(er)}`)
         }
     }
+    static async executeLogsUpload(funnelUser?: string, funnelKey?: string) {
+        try {
+            const index = process.argv.indexOf('--uploadLogs')
+            const clientBuildUuid = process.argv[index + 1]
+            const user = funnelUser || process.env.BROWSERSTACK_USERNAME
+            const key = funnelKey || process.env.BROWSERSTACK_ACCESS_KEY
+            if (!clientBuildUuid || !user || !key) {
+                BStackLogger.debug('Skipping logs upload in cleanup: missing uuid or credentials')
+                return
+            }
+            BStackLogger.debug(`Uploading SDK logs from cleanup for ${clientBuildUuid}`)
+            await uploadLogs(user, key, clientBuildUuid)
+        } catch (e: unknown) {
+            BStackLogger.error('Error uploading SDK logs in cleanup: ' + getErrorString(e))
+        }
+    }
+
     static async executeObservabilityCleanup(funnelData: any) {
         if (!process.env[BROWSERSTACK_TESTHUB_JWT]) {
             return

packages/browserstack-service/src/cli/grpcClient.ts

@@ -262,8 +262,12 @@ export class GrpcClient {
             }
 
             const clientWorkerId = CLIUtils.getClientWorkerId()
+            // Forward the interrupt signal so the binary stamps
+            // finished_metadata on the build stop.
+            const killSignal = process.env.BROWSERSTACK_SDK_KILL_SIGNAL
             const request = StopBinSessionRequestConstructor.create({
-                binSessionId: this.binSessionId
+                binSessionId: this.binSessionId,
+                ...(killSignal ? { exitSignal: killSignal, exitReason: 'user_killed' } : {})
             })
             // Add clientWorkerId to request (proto field 500)
             ;(request as any).clientWorkerId = clientWorkerId

packages/browserstack-service/src/config.ts

@@ -29,6 +29,7 @@ class BrowserStackConfig {
     public funnelDataSent: boolean = false
     public sdkRunID: string
     public killSignal?: string
+    public logsUploaded: boolean = false
     public percyBuildId?: number | null
     public isPercyAutoEnabled = false
 

packages/browserstack-service/src/constants.ts

@@ -102,6 +102,9 @@ export const TESTOPS_SCREENSHOT_ENV = 'BS_TESTOPS_ALLOW_SCREENSHOTS'
 
 // To store build hashed id
 export const BROWSERSTACK_TESTHUB_UUID = 'BROWSERSTACK_TESTHUB_UUID'
+// Interrupt signal, propagated via env so the detached cleanup child and gRPC
+// stop path can stamp the kill reason onto the build stop.
+export const BROWSERSTACK_KILL_SIGNAL = 'BROWSERSTACK_SDK_KILL_SIGNAL'
 
 // To store test run uuid
 export const TEST_ANALYTICS_ID = 'TEST_ANALYTICS_ID'

packages/browserstack-service/src/exitHandler.ts

@@ -3,7 +3,7 @@ import path from 'node:path'
 import BrowserStackConfig from './config.js'
 import { saveFunnelData } from './instrumentation/funnelInstrumentation.js'
 import { fileURLToPath } from 'node:url'
-import { BROWSERSTACK_TESTHUB_JWT } from './constants.js'
+import { BROWSERSTACK_TESTHUB_JWT, BROWSERSTACK_TESTHUB_UUID, BROWSERSTACK_KILL_SIGNAL } from './constants.js'
 import { BStackLogger } from './bstackLogger.js'
 import PerformanceTester from './instrumentation/performance/performance-tester.js'
 import TestOpsConfig from './testOps/testOpsConfig.js'
@@ -12,6 +12,9 @@ import { BrowserstackCLI } from './cli/index.js'
 const __filename = fileURLToPath(import.meta.url)
 const __dirname = path.dirname(__filename)
 
+const SIGNAL_EXIT_CODES: Record<string, number> = { SIGHUP: 1, SIGINT: 2, SIGQUIT: 3, SIGABRT: 6, SIGTERM: 15, SIGBREAK: 21 }
+const FORCED_EXIT_GRACE_MS = 5000
+
 function getInterruptSignals(): string[] {
     const allSignals: string[] = [
         'SIGTERM',
@@ -83,6 +86,17 @@ export function setupExitHandlers() {
         process.on(sig, () => {
             BStackLogger.debug(`${sig} received, setting kill signal`)
             BrowserStackConfig.getInstance().setKillSignal(sig)
+            process.env[BROWSERSTACK_KILL_SIGNAL] = sig
+
+            // Listening on a signal suppresses Node's default termination. Give the
+            // runner's own shutdown a grace window, then force the conventional 128+n
+            // exit — a hung shutdown would otherwise live until CI's SIGKILL, which
+            // fires no 'exit' event and skips the cleanup rescue. unref() so a
+            // naturally exiting process is never held open.
+            const timer = setTimeout(() => {
+                process.exit(128 + (SIGNAL_EXIT_CODES[sig] || 0))
+            }, FORCED_EXIT_GRACE_MS)
+            timer.unref()
         })
     })
 
@@ -107,5 +121,13 @@ export function shouldCallCleanup(config: BrowserStackConfig, isCLIEnabled = fal
         args.push('--performanceData')
     }
 
+    // A signal-terminated run never reaches onComplete's log upload, leaving
+    // the build with no SDK-log object — rescue it from the detached cleanup
+    // process.
+    const clientBuildUuid = process.env[BROWSERSTACK_TESTHUB_UUID] || config.sdkRunID
+    if (!config.logsUploaded && config.userName && config.accessKey && clientBuildUuid) {
+        args.push('--uploadLogs', clientBuildUuid)
+    }
+
     return args
 }

packages/browserstack-service/src/launcher.ts

@@ -750,6 +750,12 @@ export default class BrowserstackLauncherService implements Services.ServiceInst
 
         await PerformanceTester.measureWrapper(PERFORMANCE_SDK_EVENTS.EVENTS.SDK_UPLOAD_LOGS, async () => {
             const response = await uploadLogs(getBrowserStackUser(this._config), getBrowserStackKey(this._config), clientBuildUuid)
+            const delivered = !!response && !(response.status && response.status !== 'success')
+            if (delivered) {
+                // A delivered upload must not be repeated by the exit-time
+                // cleanup rescue.
+                this.browserStackConfig.logsUploaded = true
+            }
             BStackLogger.logToFile(`Response - ${format(response)}`, 'debug')
         })()
     }

packages/browserstack-service/tests/cleanup.test.ts

@@ -50,6 +50,68 @@ describe('BStackCleanup', () => {
             expect(fs.rmSync).toHaveBeenNthCalledWith(1, filePath, expect.any(Object))
             expect(FunnelTestEvent.fireFunnelRequest).toHaveBeenCalled()
         })
+
+        it('uploads logs with the credentials snapshotted before funnel redaction', async () => {
+            const filePath = 'some_file.json'
+            process.argv.push('--funnelData', filePath, '--uploadLogs', 'build-uuid-1')
+            vi.spyOn(fs, 'readFileSync').mockReturnValue('{"userName": "snapUser", "accessKey": "snapKey"}')
+            vi.spyOn(fs, 'rmSync')
+            const uploadLogsSpy = vi.spyOn(utils, 'uploadLogs').mockResolvedValueOnce('ok')
+            // fireFunnelRequest redacts the credentials on the funnel object in place.
+            vi.spyOn(FunnelTestEvent, 'fireFunnelRequest').mockImplementationOnce(async (data: any) => {
+                data.userName = 'REDACTED'
+                data.accessKey = 'REDACTED'
+            })
+
+            await BStackCleanup.startCleanup()
+
+            expect(uploadLogsSpy).toHaveBeenCalledWith('snapUser', 'snapKey', 'build-uuid-1')
+        })
+
+        it('does not upload logs when --uploadLogs is absent', async () => {
+            const uploadLogsSpy = vi.spyOn(utils, 'uploadLogs')
+            await BStackCleanup.startCleanup()
+            expect(uploadLogsSpy).not.toHaveBeenCalled()
+        })
+    })
+
+    describe('executeLogsUpload', () => {
+        it('uploads logs with the funnel credentials and the --uploadLogs uuid', async () => {
+            process.argv.push('--uploadLogs', 'build-uuid-2')
+            const uploadLogsSpy = vi.spyOn(utils, 'uploadLogs').mockResolvedValueOnce('ok')
+
+            await BStackCleanup.executeLogsUpload('funnelUser', 'funnelKey')
+
+            expect(uploadLogsSpy).toHaveBeenCalledWith('funnelUser', 'funnelKey', 'build-uuid-2')
+        })
+
+        it('falls back to env credentials when funnel credentials are absent', async () => {
+            process.argv.push('--uploadLogs', 'build-uuid-3')
+            process.env.BROWSERSTACK_USERNAME = 'envUser'
+            process.env.BROWSERSTACK_ACCESS_KEY = 'envKey'
+            const uploadLogsSpy = vi.spyOn(utils, 'uploadLogs').mockResolvedValueOnce('ok')
+
+            await BStackCleanup.executeLogsUpload()
+
+            expect(uploadLogsSpy).toHaveBeenCalledWith('envUser', 'envKey', 'build-uuid-3')
+        })
+
+        it('skips upload when credentials are missing', async () => {
+            process.argv.push('--uploadLogs', 'build-uuid-4')
+            const uploadLogsSpy = vi.spyOn(utils, 'uploadLogs')
+
+            await BStackCleanup.executeLogsUpload()
+
+            expect(uploadLogsSpy).not.toHaveBeenCalled()
+        })
+
+        it('skips upload when the uuid is missing', async () => {
+            const uploadLogsSpy = vi.spyOn(utils, 'uploadLogs')
+
+            await BStackCleanup.executeLogsUpload('funnelUser', 'funnelKey')
+
+            expect(uploadLogsSpy).not.toHaveBeenCalled()
+        })
     })
 
     describe('executeObservabilityCleanup', () => {

packages/browserstack-service/tests/cli/grpcClient.test.ts

@@ -129,6 +129,31 @@ describe('GrpcClient', () => {
 
             await expect(grpcClient.stopBinSession()).resolves.toBeUndefined()
         })
+
+        it('stamps exitSignal and user_killed reason when the kill-signal env is set', async () => {
+            process.env.BROWSERSTACK_SDK_KILL_SIGNAL = 'SIGINT'
+            const mockStopBinSession = vi.fn().mockImplementation((req, cb) => cb(null, {}))
+            grpcClient.client = { stopBinSession: mockStopBinSession } as any
+
+            await grpcClient.stopBinSession()
+
+            const request = mockStopBinSession.mock.calls[0][0]
+            expect(request.exitSignal).toBe('SIGINT')
+            expect(request.exitReason).toBe('user_killed')
+            delete process.env.BROWSERSTACK_SDK_KILL_SIGNAL
+        })
+
+        it('omits exit metadata when the kill-signal env is unset', async () => {
+            delete process.env.BROWSERSTACK_SDK_KILL_SIGNAL
+            const mockStopBinSession = vi.fn().mockImplementation((req, cb) => cb(null, {}))
+            grpcClient.client = { stopBinSession: mockStopBinSession } as any
+
+            await grpcClient.stopBinSession()
+
+            const request = mockStopBinSession.mock.calls[0][0]
+            expect(request.exitSignal).toBe('')
+            expect(request.exitReason).toBe('')
+        })
     })
 
     describe('connectBinSession', () => {

packages/browserstack-service/tests/exitHandler.test.ts

@@ -0,0 +1,91 @@
+import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'
+import { shouldCallCleanup, setupExitHandlers } from '../src/exitHandler.js'
+import BrowserStackConfig from '../src/config.js'
+import * as bstackLogger from '../src/bstackLogger.js'
+import * as FunnelInstrumentation from '../src/instrumentation/funnelInstrumentation.js'
+import PerformanceTester from '../src/instrumentation/performance/performance-tester.js'
+import { BROWSERSTACK_TESTHUB_UUID } from '../src/constants.js'
+
+vi.mock('node:child_process', () => ({ spawn: vi.fn(() => ({ unref: vi.fn() })) }))
+vi.mock('../src/cli/index.js', () => ({
+    BrowserstackCLI: { getInstance: () => ({ isRunning: () => false, process: null }) }
+}))
+
+vi.spyOn(bstackLogger.BStackLogger, 'logToFile').mockImplementation(() => {})
+vi.spyOn(FunnelInstrumentation, 'saveFunnelData').mockReturnValue('funnel.json')
+vi.spyOn(PerformanceTester, 'isEnabled').mockReturnValue(false)
+
+function makeConfig(overrides: Record<string, unknown> = {}) {
+    return {
+        userName: 'user',
+        accessKey: 'key',
+        funnelDataSent: true,
+        logsUploaded: false,
+        sdkRunID: 'run-123',
+        testObservability: { buildStopped: false },
+        ...overrides
+    } as any
+}
+
+describe('shouldCallCleanup', () => {
+    let originalEnv: NodeJS.ProcessEnv
+
+    beforeEach(() => {
+        originalEnv = process.env
+        process.env = {}
+    })
+
+    afterEach(() => {
+        process.env = originalEnv
+    })
+
+    it('pushes --uploadLogs with the testhub uuid when logs are not yet uploaded', () => {
+        process.env[BROWSERSTACK_TESTHUB_UUID] = 'testhub-uuid'
+        const args = shouldCallCleanup(makeConfig())
+        expect(args).toContain('--uploadLogs')
+        expect(args[args.indexOf('--uploadLogs') + 1]).toBe('testhub-uuid')
+    })
+
+    it('falls back to sdkRunID when the testhub uuid is absent', () => {
+        const args = shouldCallCleanup(makeConfig())
+        expect(args[args.indexOf('--uploadLogs') + 1]).toBe('run-123')
+    })
+
+    it('omits --uploadLogs when logs were already uploaded', () => {
+        const args = shouldCallCleanup(makeConfig({ logsUploaded: true }))
+        expect(args).not.toContain('--uploadLogs')
+    })
+
+    it('omits --uploadLogs when credentials are missing', () => {
+        const args = shouldCallCleanup(makeConfig({ userName: undefined, accessKey: undefined }))
+        expect(args).not.toContain('--uploadLogs')
+    })
+})
+
+describe('setupExitHandlers forced exit', () => {
+    let exitSpy: ReturnType<typeof vi.spyOn>
+
+    beforeEach(() => {
+        vi.useFakeTimers()
+        exitSpy = vi.spyOn(process, 'exit').mockImplementation((() => undefined) as never)
+        vi.spyOn(BrowserStackConfig, 'getInstance').mockReturnValue({ setKillSignal: vi.fn() } as any)
+    })
+
+    afterEach(() => {
+        process.removeAllListeners('SIGTERM')
+        exitSpy.mockRestore()
+        vi.useRealTimers()
+        vi.restoreAllMocks()
+    })
+
+    it('forces the conventional 128+n exit once the grace window elapses', () => {
+        setupExitHandlers()
+        process.emit('SIGTERM' as NodeJS.Signals)
+
+        expect(exitSpy).not.toHaveBeenCalled()
+
+        vi.advanceTimersByTime(5000)
+
+        expect(exitSpy).toHaveBeenCalledWith(143)
+    })
+})

packages/browserstack-service/tests/launcher.test.ts

@@ -1388,6 +1388,33 @@ describe('_uploadServiceLogs', () => {
     })
 
     const service = new BrowserstackLauncher(options as any, caps, config)
+
+    it('marks logsUploaded when the upload resolves truthy', async () => {
+        vi.spyOn(utils, 'uploadLogs').mockResolvedValueOnce('success')
+        ;(service as any).browserStackConfig.logsUploaded = false
+
+        await service._uploadServiceLogs()
+
+        expect((service as any).browserStackConfig.logsUploaded).toBe(true)
+    })
+
+    it('leaves logsUploaded false when the upload resolves falsy', async () => {
+        vi.spyOn(utils, 'uploadLogs').mockResolvedValueOnce(undefined as any)
+        ;(service as any).browserStackConfig.logsUploaded = false
+
+        await service._uploadServiceLogs()
+
+        expect((service as any).browserStackConfig.logsUploaded).toBe(false)
+    })
+
+    it('leaves logsUploaded false when the server rejects the upload', async () => {
+        vi.spyOn(utils, 'uploadLogs').mockResolvedValueOnce({ status: 'error' } as any)
+        ;(service as any).browserStackConfig.logsUploaded = false
+
+        await service._uploadServiceLogs()
+
+        expect((service as any).browserStackConfig.logsUploaded).toBe(false)
+    })
 })
 
 describe('_getClientBuildUuid', () => {