diff --git a/core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt b/core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt index d5f744dc2..978f8b7b3 100644 --- a/core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt +++ b/core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt @@ -275,11 +275,13 @@ open class DefaultBrowser( val waitedMs = config.browserConnectionTimeout * (config.browserConnectionMaxTries + 1) val stderr = process?.readStderrSnapshot() logger.error( - "Browser never opened its debug port on ${config.host}:${config.port} after ${waitedMs}ms " + - "(pid=${process?.pid()}, alive=${process?.isAlive()}). " + - "Last connection error: " + - (lastConnectionError?.let { "${it::class.simpleName}: ${it.message}" } ?: "none") + - ". Browser stderr: " + (stderr?.trim()?.takeIf { it.isNotEmpty() } ?: "") + browserStartFailureMessage( + endpoint = "${config.host}:${config.port}", + waitedMs = waitedMs, + fate = processFate(process?.pid(), process?.isAlive() ?: false, process?.exitCodeOrNull()), + lastConnectionError = lastConnectionError, + stderr = stderr, + ) ) stop() throw FailedToConnectToBrowserException() diff --git a/core/src/commonMain/kotlin/dev/kdriver/core/browser/StartFailure.kt b/core/src/commonMain/kotlin/dev/kdriver/core/browser/StartFailure.kt new file mode 100644 index 000000000..19c05331a --- /dev/null +++ b/core/src/commonMain/kotlin/dev/kdriver/core/browser/StartFailure.kt @@ -0,0 +1,56 @@ +package dev.kdriver.core.browser + +/** + * Reports the exit status of a process that has already terminated, or null if it is still running + * (or the platform cannot tell). + * + * Paired with [Process.isAlive]: `isAlive() == false` says the browser is gone, this says *how* it + * went. The distinction is the whole point — on Windows a Chrome that finds a live instance on the + * same `--user-data-dir` hands its command line over and exits **0**, silently, without ever opening + * a debug port; a Chrome that crashed exits non-zero. Both look identical without this. + */ +expect fun Process.exitCodeOrNull(): Int? + +/** + * Describes what became of the browser process. + * + * Reports the exit status as-is rather than interpreting it: the whole reason for reading it is that + * we do not yet know which statuses a browser that never opened its debug port exits with. Guessing + * here would put an unverified claim into every log line. + * + * @param pid the browser's process id, if we had one. + * @param alive whether the process was still running when we gave up. + * @param exitCode its exit status, or null if it is alive or the platform cannot tell. + * + * @return a fragment such as `pid=1916, exited with 21`, meant to be embedded in a larger message. + */ +internal fun processFate(pid: Long?, alive: Boolean, exitCode: Int?): String = "pid=$pid, " + when { + alive -> "still running" + exitCode == null -> "already gone (exit status unavailable)" + else -> "exited with $exitCode" +} + +/** + * Builds the message logged when a browser never opened its debug port. + * + * Pure and separated from [DefaultBrowser.start] so the wording is covered by tests on any OS — the + * facts it reports come from platform calls that cannot be exercised in CI. + * + * @param endpoint the address the debug port was expected on. + * @param waitedMs how long we waited for it. + * @param fate the process's id and what became of it — see [processFate]. + * @param lastConnectionError the last failure seen while polling, if any. + * @param stderr whatever the browser wrote to stderr, if anything. + * + * @return a single line naming the endpoint, the process's fate, and the last connection error. + */ +internal fun browserStartFailureMessage( + endpoint: String, + waitedMs: Long, + fate: String, + lastConnectionError: Throwable?, + stderr: String?, +): String = "Browser never opened its debug port on $endpoint after ${waitedMs}ms ($fate). " + + "Last connection error: " + + (lastConnectionError?.let { "${it::class.simpleName}: ${it.message}" } ?: "none") + + ". Browser stderr: " + (stderr?.trim()?.takeIf { it.isNotEmpty() } ?: "") diff --git a/core/src/jsMain/kotlin/dev/kdriver/core/browser/Process.js.kt b/core/src/jsMain/kotlin/dev/kdriver/core/browser/Process.js.kt index af1e2b93c..3c9c224d2 100644 --- a/core/src/jsMain/kotlin/dev/kdriver/core/browser/Process.js.kt +++ b/core/src/jsMain/kotlin/dev/kdriver/core/browser/Process.js.kt @@ -18,6 +18,10 @@ actual suspend fun Process.readStderrSnapshot(maxBytes: Int, timeoutMillis: Long throw UnsupportedOperationException() } +actual fun Process.exitCodeOrNull(): Int? { + throw UnsupportedOperationException() +} + actual fun Process.killTree() { throw UnsupportedOperationException() } diff --git a/core/src/jvmMain/kotlin/dev/kdriver/core/browser/Process.jvm.kt b/core/src/jvmMain/kotlin/dev/kdriver/core/browser/Process.jvm.kt index ff6dda8c7..4dadd020a 100644 --- a/core/src/jvmMain/kotlin/dev/kdriver/core/browser/Process.jvm.kt +++ b/core/src/jvmMain/kotlin/dev/kdriver/core/browser/Process.jvm.kt @@ -120,3 +120,11 @@ actual fun defaultBrowserSearchConfig(): BrowserSearchConfig { else -> BrowserSearchConfig(File.pathSeparator, searchWindowsProgramFiles = true) } } + +/** + * Reads the exit status, or null while the process is still running. + * + * `exitValue()` throws `IllegalThreadStateException` rather than returning a sentinel when the + * process is alive, which is exactly the "still running" case we report as null. + */ +actual fun Process.exitCodeOrNull(): Int? = runCatching { exitValue() }.getOrNull() diff --git a/core/src/jvmTest/kotlin/dev/kdriver/core/browser/StartFailureMessageTest.kt b/core/src/jvmTest/kotlin/dev/kdriver/core/browser/StartFailureMessageTest.kt new file mode 100644 index 000000000..12e92b9d3 --- /dev/null +++ b/core/src/jvmTest/kotlin/dev/kdriver/core/browser/StartFailureMessageTest.kt @@ -0,0 +1,61 @@ +package dev.kdriver.core.browser + +import kotlin.test.Test +import kotlin.test.assertTrue + +class StartFailureMessageTest { + + private fun message( + alive: Boolean = false, + exitCode: Int? = null, + stderr: String? = null, + error: Throwable? = null, + ) = browserStartFailureMessage( + endpoint = "127.0.0.1:53997", waitedMs = 30500, + fate = processFate(1916, alive, exitCode), lastConnectionError = error, stderr = stderr, + ) + + @Test + fun message_alwaysNamesTheEndpointTheWaitAndThePid() { + val m = message() + assertTrue("127.0.0.1:53997" in m, m) + assertTrue("30500ms" in m, m) + assertTrue("pid=1916" in m, m) + } + + /** + * Reported as-is, with no interpretation: we are reading the exit status precisely because we do + * not know yet which statuses this failure produces. A zero must not be dressed up as anything. + */ + @Test + fun message_reportsTheExitStatusVerbatim() { + assertTrue("exited with 0" in message(exitCode = 0), message(exitCode = 0)) + assertTrue("exited with 21" in message(exitCode = 21), message(exitCode = 21)) + } + + @Test + fun message_whenTheBrowserIsStillRunning_saysSoRatherThanGuessing() { + val m = message(alive = true, exitCode = null) + assertTrue("still running" in m, m) + } + + @Test + fun message_whenTheExitStatusIsUnavailable_doesNotClaimACleanExit() { + val m = message(alive = false, exitCode = null) + assertTrue("exit status unavailable" in m, m) + assertTrue("exited with" !in m, "an unknown status must not be reported as an exit code: $m") + } + + @Test + fun message_reportsStderrWhenThereIsSomeAndSaysSoWhenThereIsNot() { + assertTrue("" in message(stderr = " "), "blank stderr reads as none") + assertTrue("Fontconfig error" in message(stderr = "Fontconfig error\n"), "stderr is quoted") + } + + @Test + fun message_reportsTheLastConnectionError() { + val m = message(error = IllegalStateException("Connection refused")) + assertTrue("IllegalStateException: Connection refused" in m, m) + assertTrue("none" in message(error = null), "says none when there was no error") + } +} diff --git a/core/src/mingwMain/kotlin/dev/kdriver/core/browser/Process.mingw.kt b/core/src/mingwMain/kotlin/dev/kdriver/core/browser/Process.mingw.kt index 3c86da365..c62e3b8f8 100644 --- a/core/src/mingwMain/kotlin/dev/kdriver/core/browser/Process.mingw.kt +++ b/core/src/mingwMain/kotlin/dev/kdriver/core/browser/Process.mingw.kt @@ -61,6 +61,21 @@ actual suspend fun Process.readStderrSnapshot(maxBytes: Int, timeoutMillis: Long * a `CreateToolhelp32Snapshot` by parent id, which this target does not do — the JVM target, which is * the one running browsers in production, uses `ProcessHandle.descendants()` for that. */ +/** + * Reads the exit status via `GetExitCodeProcess`, the same call [Process.isAlive] uses — null while + * it still reports `STILL_ACTIVE`. + */ +@OptIn(ExperimentalForeignApi::class) +actual fun Process.exitCodeOrNull(): Int? { + val handle = processHandle ?: return null + val code = memScoped { + val c = alloc() + GetExitCodeProcess(handle, c.ptr) + c.value + } + return if (code == STILL_ACTIVE) null else code.toInt() +} + @OptIn(ExperimentalForeignApi::class) actual fun Process.killTree() { processHandle?.let { if (isAlive()) TerminateProcess(it, 1u) } diff --git a/core/src/posixMain/kotlin/dev/kdriver/core/browser/Process.posix.kt b/core/src/posixMain/kotlin/dev/kdriver/core/browser/Process.posix.kt index 9834ec3c0..61b2f1e21 100644 --- a/core/src/posixMain/kotlin/dev/kdriver/core/browser/Process.posix.kt +++ b/core/src/posixMain/kotlin/dev/kdriver/core/browser/Process.posix.kt @@ -101,3 +101,9 @@ actual fun tempProfileDir(): Path { return Path(profilePath) } + +/** + * Not available on this target: liveness here is `kill(pid, 0)`, which says whether the process + * exists but never carries its exit status, and nothing reaps the child to collect one. + */ +actual fun Process.exitCodeOrNull(): Int? = null