ADFA-4128: qb 07 review fixes — parseDiagnostics no-throw contract + request bound

Important 1: unguarded asString on diagnostic severity/message threw out of compile() on object/array values -> primitive-guarded, degrading to ERROR / "unknown error"; covered by `a non-primitive severity or message degrades instead of throwing out of compile`.
Important 2: line/column asInt threw NumberFormatException on non-numeric string primitives -> runCatching like the protocol-version read, degrading to absent; covered by `a non-numeric line or column string reads as absent instead of throwing`.
Important 3: the request write had no bound, so a wedged child holding a full stdin pipe parked the mutex forever and shutdown() deadlocked on the writer monitor -> write runs on the client scope under requestTimeoutMillis with destroyForcibly on expiry, and shutdown()'s EOF close moved off the teardown path; covered by `a request the daemon never reads times out instead of wedging the client` and `shutdown is not deadlocked by a write the daemon never reads`.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kj9YeCDHGp9DU8LPtfWJ7W

Author: fryanpan

quickbuild/core/src/main/java/org/appdevforall/cotg/quickbuild/data/DaemonProcessClient.kt

@@ -7,6 +7,8 @@ import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.CompletableDeferred
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.async
+import kotlinx.coroutines.isActive
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock
@@ -26,6 +28,7 @@ import java.io.IOException
 import java.util.concurrent.ConcurrentHashMap
 import java.util.concurrent.atomic.AtomicBoolean
 import java.util.concurrent.atomic.AtomicLong
+import kotlin.coroutines.coroutineContext
 
 /**
  * Runs the quick-build daemon as a child JVM and speaks its line-delimited JSON protocol.
@@ -301,8 +304,13 @@ class DaemonProcessClient(
 		configured = false
 		// Best effort polite stop; the protocol also treats stdin EOF as shutdown.
 		withTimeoutOrNull(SHUTDOWN_TIMEOUT_MILLIS) { request(DaemonOps.SHUTDOWN) {} }
+		val out = writer
 		withContext(Dispatchers.IO) {
-			runCatching { writer?.close() }
+			// EOF is the second polite signal, but close() blocks on the BufferedWriter
+			// monitor while a wedged write holds it - so it runs on [scope] rather than
+			// inline, and the kill path below (which closes the pipe and thereby frees any
+			// such writer) is always reached instead of deadlocking teardown.
+			scope.launch(Dispatchers.IO) { runCatching { out?.close() } }
 			if (proc.isAlive && !proc.waitFor(2, java.util.concurrent.TimeUnit.SECONDS)) {
 				proc.destroyForcibly()
 			}
@@ -313,8 +321,9 @@ class DaemonProcessClient(
 
 	/**
 	 * Sends one request and awaits the matching-id response. Failure of the transport
-	 * (dead process, EOF, timeout) is a [DaemonReply.Failed]; a well-formed
-	 * `ok=false` response is a [DaemonReply.BuildFailed] with parsed diagnostics.
+	 * (dead process, EOF, timeout - on the response, or on a write the child never drains)
+	 * is a [DaemonReply.Failed]; a well-formed `ok=false` response is a
+	 * [DaemonReply.BuildFailed] with parsed diagnostics.
 	 *
 	 * @param op protocol op name, sent as `op` and echoed in timeout messages.
 	 * @param fill adds the op's own keys to the request object; `id` and `op` are already set
@@ -339,15 +348,44 @@ class DaemonProcessClient(
 					fill()
 				}
 
-			try {
-				withContext(Dispatchers.IO) {
-					out.write(requestJson.toString())
-					out.newLine()
-					out.flush()
+			// The write runs on [scope], not the caller's context: a blocking pipe write to a
+			// child that stopped reading stdin cannot be cancelled, only abandoned, and it must
+			// not park the caller (and [requestMutex]) forever while it blocks.
+			val writeJob =
+				scope.async(Dispatchers.IO) {
+					runCatching {
+						out.write(requestJson.toString())
+						out.newLine()
+						out.flush()
+					}
 				}
-			} catch (e: IOException) {
+			val writeOutcome =
+				try {
+					withTimeoutOrNull(requestTimeoutMillis) { writeJob.await() }
+				} catch (e: CancellationException) {
+					pending.remove(id)
+					// The caller's own cancellation propagates; a dead [scope] (client torn
+					// down under the caller) degrades to a reply instead.
+					if (coroutineContext.isActive) {
+						return DaemonReply.Failed("Daemon is not running", daemonDied = true)
+					}
+					throw e
+				}
+			if (writeOutcome == null) {
+				// The child wedged with a full stdin pipe. Only closing the pipe frees the
+				// blocked thread, so the daemon is killed; its death watcher then fails any
+				// pending requests and fires the respawn flow.
 				pending.remove(id)
-				return DaemonReply.Failed("Daemon write failed: ${e.message}", daemonDied = true)
+				process?.destroyForcibly()
+				return DaemonReply.Failed(
+					"Daemon stopped reading requests ('$op' write timed out)",
+					daemonDied = true,
+				)
+			}
+			val writeError = writeOutcome.exceptionOrNull()
+			if (writeError != null) {
+				pending.remove(id)
+				return DaemonReply.Failed("Daemon write failed: ${writeError.message}", daemonDied = true)
 			}
 
 			val response =
@@ -374,9 +412,9 @@ class DaemonProcessClient(
 				// which reports no compile counts - carries none rather than a measured zero.
 				DaemonReply.BuildFailed(
 					parseDiagnostics(response),
-					CompileStats.fromValues { key ->
-						response.get(key)?.takeIf { it.isJsonPrimitive }?.asLong
-					},
+					// longOrNull, not a bare asLong: a malformed stats value degrades to
+					// absent instead of throwing out of the facade's no-throw contract.
+					CompileStats.fromValues { key -> response.longOrNull(key) },
 				)
 			}
 		}
@@ -450,24 +488,48 @@ class DaemonProcessClient(
 	 *
 	 * @param response the `ok=false` response object.
 	 * @return one [BuildDiagnostic] per well-formed entry, empty when the key is absent or not an
-	 *   array; anything but an explicit `WARNING` reads as an error and a missing message becomes
-	 *   "unknown error", so a diagnostic is never dropped for being thin.
+	 *   array; anything but an explicit `WARNING` reads as an error, a missing or non-primitive
+	 *   message becomes "unknown error", and a non-numeric line or column reads as absent, so a
+	 *   diagnostic is never dropped - and never thrown on - for being thin or oddly shaped.
 	 */
 	private fun parseDiagnostics(response: JsonObject): List<BuildDiagnostic> {
 		val array = response.get(ResponseKeys.DIAGNOSTICS) as? JsonArray ?: return emptyList()
 		return array.mapNotNull { element ->
 			val obj = element as? JsonObject ?: return@mapNotNull null
 			BuildDiagnostic(
+				// Primitive-guarded like every other read in this file: asString on an object
+				// or array throws, and this facade promises never to throw for a build problem.
 				severity =
-					if (obj.get(ResponseKeys.Diagnostics.SEVERITY)?.asString.equals("WARNING", ignoreCase = true)) {
+					if (obj
+							.get(ResponseKeys.Diagnostics.SEVERITY)
+							?.takeIf { it.isJsonPrimitive }
+							?.asString
+							.equals("WARNING", ignoreCase = true)
+					) {
 						BuildDiagnostic.Severity.WARNING
 					} else {
 						BuildDiagnostic.Severity.ERROR
 					},
-				message = obj.get(ResponseKeys.Diagnostics.MESSAGE)?.asString ?: "unknown error",
+				message =
+					obj
+						.get(ResponseKeys.Diagnostics.MESSAGE)
+						?.takeIf { it.isJsonPrimitive }
+						?.asString ?: "unknown error",
 				file = obj.get(ResponseKeys.Diagnostics.FILE)?.takeIf { it.isJsonPrimitive }?.asString,
-				line = obj.get(ResponseKeys.Diagnostics.LINE)?.takeIf { it.isJsonPrimitive }?.asInt,
-				column = obj.get(ResponseKeys.Diagnostics.COLUMN)?.takeIf { it.isJsonPrimitive }?.asInt,
+				// The primitive guard alone does not stop asInt throwing NumberFormatException
+				// on a non-numeric string primitive ("line":"abc"); runCatching does.
+				line =
+					obj
+						.get(ResponseKeys.Diagnostics.LINE)
+						?.takeIf { it.isJsonPrimitive }
+						?.runCatching { asInt }
+						?.getOrNull(),
+				column =
+					obj
+						.get(ResponseKeys.Diagnostics.COLUMN)
+						?.takeIf { it.isJsonPrimitive }
+						?.runCatching { asInt }
+						?.getOrNull(),
 			)
 		}
 	}

quickbuild/core/src/test/java/org/appdevforall/cotg/quickbuild/data/DaemonProcessClientEdgeTest.kt

@@ -4,9 +4,12 @@ import com.google.common.truth.Truth.assertThat
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.SupervisorJob
+import kotlinx.coroutines.async
 import kotlinx.coroutines.cancel
+import kotlinx.coroutines.delay
 import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.withTimeout
+import kotlinx.coroutines.withTimeoutOrNull
 import org.appdevforall.cotg.quickbuild.domain.reload.BuildDiagnostic
 import org.appdevforall.cotg.quickbuild.protocol.ConfigureRequest
 import org.junit.jupiter.api.Test
@@ -439,6 +442,207 @@ class DaemonProcessClientEdgeTest {
 		assertThat(oddShapes.column).isNull()
 	}
 
+	@Test
+	fun `a non-primitive severity or message degrades instead of throwing out of compile`() {
+		// asString on an object or array throws UnsupportedOperationException, which unguarded
+		// escaped parseDiagnostics straight out of compile() - the facade's no-throw contract
+		// says a malformed diagnostic must degrade (severity -> ERROR, message -> the default).
+		val diagnostics =
+			"""[
+				{"severity":{"level":"WARNING"},"message":"kept"},
+				{"severity":"ERROR","message":["broken","in","parts"]}
+			]""".replace(Regex("\\s+"), "")
+		val paths =
+			scriptedPaths(
+				"""
+				read line
+				printf '%s\n' '${okConfigure()}'
+				read line
+				printf '%s\n' '{"id":2,"ok":false,"diagnostics":$diagnostics}'
+				read line
+				printf '%s\n' '{"id":3,"ok":true}'
+				""".trimIndent(),
+			)
+
+		val reply =
+			withClient(paths) { client ->
+				check(client.start(config()) is DaemonReply.Ok)
+				client.compile(emptyList(), emptyList())
+			}
+
+		val failure = reply as DaemonReply.BuildFailed
+		assertThat(failure.diagnostics).hasSize(2)
+		val (objectSeverity, arrayMessage) = failure.diagnostics
+		assertThat(objectSeverity.severity).isEqualTo(BuildDiagnostic.Severity.ERROR)
+		assertThat(objectSeverity.message).isEqualTo("kept")
+		assertThat(arrayMessage.severity).isEqualTo(BuildDiagnostic.Severity.ERROR)
+		assertThat(arrayMessage.message).isEqualTo("unknown error")
+	}
+
+	@Test
+	fun `a non-numeric line or column string reads as absent instead of throwing`() {
+		// "abc" IS a JSON primitive, so the isJsonPrimitive guard passes and asInt throws
+		// NumberFormatException - the crash path the "odd shapes" test stopped short of
+		// (its "3" coerces cleanly). The message must still come through untouched.
+		val diagnostics =
+			"""[{"severity":"ERROR","message":"bad positions","file":"A.kt","line":"abc","column":"1.5"}]"""
+		val paths =
+			scriptedPaths(
+				"""
+				read line
+				printf '%s\n' '${okConfigure()}'
+				read line
+				printf '%s\n' '{"id":2,"ok":false,"diagnostics":$diagnostics}'
+				read line
+				printf '%s\n' '{"id":3,"ok":true}'
+				""".trimIndent(),
+			)
+
+		val reply =
+			withClient(paths) { client ->
+				check(client.start(config()) is DaemonReply.Ok)
+				client.compile(emptyList(), emptyList())
+			}
+
+		val failure = reply as DaemonReply.BuildFailed
+		assertThat(failure.diagnostics).hasSize(1)
+		val diagnostic = failure.diagnostics.single()
+		assertThat(diagnostic.message).isEqualTo("bad positions")
+		assertThat(diagnostic.file).isEqualTo("A.kt")
+		assertThat(diagnostic.line).isNull()
+		assertThat(diagnostic.column).isNull()
+	}
+
+	@Test
+	fun `a malformed stats value on a build failure degrades instead of throwing`() {
+		// Same crash class as line/column: "slow" IS a JSON primitive, so a primitive guard
+		// alone lets asLong throw NumberFormatException out of the BuildFailed arm; a
+		// non-primitive value must degrade too. The readable key still comes through.
+		val paths =
+			scriptedPaths(
+				"""
+				read line
+				printf '%s\n' '${okConfigure()}'
+				read line
+				printf '%s\n' '{"id":2,"ok":false,"diagnostics":[],"preSnapMillis":"slow","nKotlinToCompile":[3],"compileOrdinal":2}'
+				read line
+				printf '%s\n' '{"id":3,"ok":true}'
+				""".trimIndent(),
+			)
+
+		val reply =
+			withClient(paths) { client ->
+				check(client.start(config()) is DaemonReply.Ok)
+				client.compile(emptyList(), emptyList())
+			}
+
+		val failure = reply as DaemonReply.BuildFailed
+		val stats = failure.stats
+		assertThat(stats).isNotNull()
+		assertThat(stats!!.compileOrdinal).isEqualTo(2)
+		assertThat(stats.preSnapMillis).isEqualTo(0)
+		assertThat(stats.kotlinToCompile).isEqualTo(0)
+	}
+
+	@Test
+	fun `a request the daemon never reads times out instead of wedging the client`() {
+		// After configure the script stops reading stdin, so a request larger than the pipe
+		// buffer blocks the write forever while it holds the request mutex - unfixed, every
+		// later request parks on the mutex and shutdown() deadlocks on the writer's monitor.
+		// The client must bound the write, kill the wedged child, and report a Failed reply.
+		val paths =
+			scriptedPaths(
+				"""
+				printf '%s' "${'$'}${'$'}" > '$tmp/daemon.pid'
+				read line
+				printf '%s\n' '${okConfigure()}'
+				exec sleep 120
+				""".trimIndent(),
+			)
+		// ~4MB of source paths: far past any pipe buffer, so the write reliably blocks.
+		val bigSources = (1..40_000).map { File(tmp, "src/deeply/nested/pkg/SourceFile$it.kt") }
+		val scope = CoroutineScope(SupervisorJob() + Dispatchers.IO)
+		val client = DaemonProcessClient(paths, scope, requestTimeoutMillis = 500)
+		var reply: DaemonReply<CompileOutput>? = null
+
+		try {
+			runBlocking {
+				check(client.start(config()) is DaemonReply.Ok)
+				// On [scope], not runBlocking's: unfixed, the call never returns, and a
+				// structured child would deadlock runBlocking itself on the way out.
+				val call = scope.async { client.compile(bigSources, emptyList()) }
+				reply = withTimeoutOrNull(30_000) { call.await() }
+			}
+			// null means the client sat on the wedged write - the hang this test pins.
+			assertThat(reply).isNotNull()
+			assertThat(reply).isInstanceOf(DaemonReply.Failed::class.java)
+			val failed = reply as DaemonReply.Failed
+			assertThat(failed.message).contains("compile")
+			assertThat(failed.message).contains("write timed out")
+			assertThat(failed.daemonDied).isTrue()
+		} finally {
+			// Unwedge a stuck writer before shutdown: killing the child closes the pipe, so
+			// a still-blocked write (the unfixed case) throws instead of deadlocking
+			// writer.close() and hanging the test run in teardown.
+			runCatching {
+				val pid = File(tmp, "daemon.pid").readText().trim()
+				ProcessBuilder("/bin/sh", "-c", "kill -9 $pid 2>/dev/null").start().waitFor()
+			}
+			runBlocking { client.shutdown() }
+			scope.cancel()
+		}
+	}
+
+	@Test
+	fun `shutdown is not deadlocked by a write the daemon never reads`() {
+		// The wedged-write scenario again, but teardown-first: with the write still blocked
+		// (its own timeout deliberately far off), shutdown() used to park on the
+		// BufferedWriter monitor in writer.close() and never reach destroyForcibly.
+		val paths =
+			scriptedPaths(
+				"""
+				printf '%s' "${'$'}${'$'}" > '$tmp/daemon.pid'
+				read line
+				printf '%s\n' '${okConfigure()}'
+				exec sleep 120
+				""".trimIndent(),
+			)
+		val bigSources = (1..40_000).map { File(tmp, "src/deeply/nested/pkg/SourceFile$it.kt") }
+		val scope = CoroutineScope(SupervisorJob() + Dispatchers.IO)
+		val client = DaemonProcessClient(paths, scope, requestTimeoutMillis = 120_000)
+
+		try {
+			val completed =
+				runBlocking {
+					check(client.start(config()) is DaemonReply.Ok)
+					scope.async { client.compile(bigSources, emptyList()) }
+					// Long enough for the compile write to fill the pipe and block; a
+					// shutdown that won the race to the writer would close it cleanly
+					// and pass even unfixed.
+					delay(2_000)
+					// On [scope]: unfixed, shutdown never returns, and neither a structured
+					// child nor withTimeoutOrNull could pull the test out of it.
+					val shutdownJob = scope.async { client.shutdown() }
+					withTimeoutOrNull(30_000) {
+						shutdownJob.await()
+						true
+					}
+				}
+			// null means shutdown deadlocked behind the wedged writer.
+			assertThat(completed).isNotNull()
+			assertThat(client.isRunning).isFalse()
+		} finally {
+			// Frees the blocked write in the unfixed case so teardown can finish - see the
+			// wedged-request test above.
+			runCatching {
+				val pid = File(tmp, "daemon.pid").readText().trim()
+				ProcessBuilder("/bin/sh", "-c", "kill -9 $pid 2>/dev/null").start().waitFor()
+			}
+			runBlocking { client.shutdown() }
+			scope.cancel()
+		}
+	}
+
 	@Test
 	fun `a build failure without a diagnostics array reports none`() {
 		val paths =