Skip to content

Commit debf099

Browse files
fryanpanclaude
andcommitted
ADFA-4128: qb-07 review fixes - per-spawn daemon state, IO-confined installer, re-prompt guard, scratch and counter edge fixes
Akash's 08-31 review of #1719, all 11 items (incl. the ProxyAppInstaller:241 CodeRabbit thread, adopted per Bryan's 08-31 decision): - DaemonProcessClient: process, writer, pending map and deliberate-stop marker become one per-spawn object; the death watcher fails its own spawn's requests FIRST, so an in-flight request no longer orphans across a replacement (it held requestMutex for its full timeout and blocked the next configure). shutdown() also clears scratchFsType. - ProxyAppInstaller: APK hashing and every PackageManager read run under an injectable ioDispatcher (session thread never blocks); the broadcast collection degrades a completed flow to Failed instead of throwing; both launchInstall guards rethrow CancellationException; a seen PENDING_USER_ACTION suppresses the 45 s re-prompt - the OS confirmed a dialog exists, so re-committing would stack a second dialog over it - and the prompt-timeout KDoc now describes that behavior. - QuickBuildScratch: an uncreatable root reports ScratchDirUnavailable instead of a false NotEnoughStorage(100, 0). - FileGenerationStore: when both renames fail, fall back to a direct write (non-atomic beats a lost counter) and delete the stale tmp. - QuickBuildDaemonController: the low-memory teardown flag is consumed only past the isRunning guard; daemonEpoch's KDoc states the restart-bumps-twice obligation. - Fakes: unresolvable "Bug-12" planning code reworded to name the behavior. Seven new tests, each verified red against the pre-fix code; quickbuild:core green both flavors. Also: plain-language pass over the comments added by these fixes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STCsdMzx9daNBcqMN424Ci
1 parent 43e601e commit debf099

12 files changed

Lines changed: 329 additions & 90 deletions

File tree

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

Lines changed: 62 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -53,19 +53,29 @@ class DaemonProcessClient(
5353
) : QuickBuildDaemon {
5454
private val requestMutex = Mutex()
5555
private val nextId = AtomicLong(1)
56-
private val pending = ConcurrentHashMap<Long, CompletableDeferred<JsonObject>>()
57-
58-
@Volatile private var process: Process? = null
59-
60-
@Volatile private var writer: BufferedWriter? = null
6156

6257
/**
63-
* Deliberate-stop marker of the child [process] currently holds, replaced on every spawn
64-
* rather than shared between them: a replaced child's watcher passes its identity guard and
65-
* only then reads this, so a shared flag the next [start] had already cleared would report a
66-
* death for a daemon that was deliberately replaced.
58+
* Everything owned by one spawned child JVM, installed whole by [start] and never shared
59+
* between spawns. The response pump and death watcher close over the instance they were
60+
* started for, so a watcher waking up late - after the NEXT child is already spawned -
61+
* can fail only its own child's requests and can never touch a newer session's state.
6762
*/
68-
@Volatile private var deliberateStop = AtomicBoolean(false)
63+
private class Spawn(
64+
val process: Process,
65+
val writer: BufferedWriter,
66+
) {
67+
/** Requests awaiting responses on THIS child's stdout, keyed by request id. */
68+
val pending = ConcurrentHashMap<Long, CompletableDeferred<JsonObject>>()
69+
70+
/**
71+
* Deliberate-stop marker for this child alone: its watcher passes its identity guard
72+
* and only then reads this, so a shared flag the next [start] had already cleared
73+
* would report a death for a daemon that was deliberately replaced.
74+
*/
75+
val deliberateStop = AtomicBoolean(false)
76+
}
77+
78+
@Volatile private var spawn: Spawn? = null
6979

7080
@Volatile private var deathListener: ((Int) -> Unit)? = null
7181

@@ -76,7 +86,7 @@ class DaemonProcessClient(
7686
private set
7787

7888
override val isRunning: Boolean
79-
get() = configured && process?.isAlive == true
89+
get() = configured && spawn?.process?.isAlive == true
8090

8191
/**
8292
* Installs the unexpected-exit callback, replacing any previous one.
@@ -98,14 +108,9 @@ class DaemonProcessClient(
98108
* the child shut down first, so a failed start never leaves a daemon behind.
99109
*/
100110
override suspend fun start(config: DaemonConfig): DaemonReply<Unit> {
111+
// Also clears scratchFsType and marks the old child's stop on its own Spawn - the
112+
// fresh Spawn below starts with a clean marker of its own.
101113
shutdown()
102-
// A fresh marker instead of clearing the old one: the child shutdown() just stopped
103-
// keeps - and its watcher still reads - the instance it was marked on.
104-
val stopFlag = AtomicBoolean(false)
105-
this.deliberateStop = stopFlag
106-
// Belongs to the session being replaced; a failed configure must not leave the
107-
// previous daemon's filesystem stamped on the next session's timings.
108-
this.scratchFsType = null
109114

110115
val proc =
111116
try {
@@ -133,9 +138,9 @@ class DaemonProcessClient(
133138
return DaemonReply.Failed("Failed to spawn daemon: ${e.message}", daemonDied = true)
134139
}
135140

136-
process = proc
137-
writer = proc.outputStream.bufferedWriter()
138-
startReaders(proc, stopFlag)
141+
val spawn = Spawn(proc, proc.outputStream.bufferedWriter())
142+
this.spawn = spawn
143+
startReaders(spawn)
139144

140145
val configureReply =
141146
request(DaemonOps.CONFIGURE) {
@@ -297,26 +302,29 @@ class DaemonProcessClient(
297302
* nothing is running; the exit it causes is marked deliberate so no death listener fires.
298303
*/
299304
override suspend fun shutdown() {
300-
val proc = process ?: return
305+
val spawn = this.spawn ?: return
301306
// Marked before anything can kill it, so every exit from here on is deliberate to the
302307
// watcher no matter how late it observes it.
303-
deliberateStop.set(true)
308+
spawn.deliberateStop.set(true)
304309
configured = false
310+
// Belongs to the child being stopped: left in place it would stamp the previous
311+
// daemon's filesystem on the next session's timings.
312+
scratchFsType = null
305313
// Best effort polite stop; the protocol also treats stdin EOF as shutdown.
306314
withTimeoutOrNull(SHUTDOWN_TIMEOUT_MILLIS) { request(DaemonOps.SHUTDOWN) {} }
307-
val out = writer
315+
val proc = spawn.process
316+
val out = spawn.writer
308317
withContext(Dispatchers.IO) {
309318
// EOF is the second polite signal, but close() blocks on the BufferedWriter
310319
// monitor while a wedged write holds it - so it runs on [scope] rather than
311320
// inline, and the kill path below (which closes the pipe and thereby frees any
312321
// such writer) is always reached instead of deadlocking teardown.
313-
scope.launch(Dispatchers.IO) { runCatching { out?.close() } }
322+
scope.launch(Dispatchers.IO) { runCatching { out.close() } }
314323
if (proc.isAlive && !proc.waitFor(2, java.util.concurrent.TimeUnit.SECONDS)) {
315324
proc.destroyForcibly()
316325
}
317326
}
318-
process = null
319-
writer = null
327+
this.spawn = null
320328
}
321329

322330
/**
@@ -336,10 +344,11 @@ class DaemonProcessClient(
336344
fill: JsonObject.() -> Unit,
337345
): DaemonReply<JsonObject> =
338346
requestMutex.withLock {
339-
val out = writer ?: return DaemonReply.Failed("Daemon is not running", daemonDied = true)
347+
val spawn = this.spawn ?: return DaemonReply.Failed("Daemon is not running", daemonDied = true)
348+
val out = spawn.writer
340349
val id = nextId.getAndIncrement()
341350
val deferred = CompletableDeferred<JsonObject>()
342-
pending[id] = deferred
351+
spawn.pending[id] = deferred
343352

344353
val requestJson =
345354
JsonObject().apply {
@@ -363,7 +372,7 @@ class DaemonProcessClient(
363372
try {
364373
withTimeoutOrNull(requestTimeoutMillis) { writeJob.await() }
365374
} catch (e: CancellationException) {
366-
pending.remove(id)
375+
spawn.pending.remove(id)
367376
// The caller's own cancellation propagates; a dead [scope] (client torn
368377
// down under the caller) degrades to a reply instead.
369378
if (coroutineContext.isActive) {
@@ -375,16 +384,16 @@ class DaemonProcessClient(
375384
// The child wedged with a full stdin pipe. Only closing the pipe frees the
376385
// blocked thread, so the daemon is killed; its death watcher then fails any
377386
// pending requests and fires the respawn flow.
378-
pending.remove(id)
379-
process?.destroyForcibly()
387+
spawn.pending.remove(id)
388+
spawn.process.destroyForcibly()
380389
return DaemonReply.Failed(
381390
"Daemon stopped reading requests ('$op' write timed out)",
382391
daemonDied = true,
383392
)
384393
}
385394
val writeError = writeOutcome.exceptionOrNull()
386395
if (writeError != null) {
387-
pending.remove(id)
396+
spawn.pending.remove(id)
388397
return DaemonReply.Failed("Daemon write failed: ${writeError.message}", daemonDied = true)
389398
}
390399

@@ -396,11 +405,11 @@ class DaemonProcessClient(
396405
} catch (e: Exception) {
397406
null
398407
} finally {
399-
pending.remove(id)
408+
spawn.pending.remove(id)
400409
}
401410
?: return DaemonReply.Failed(
402411
"Daemon did not answer '$op' (dead or timed out)",
403-
daemonDied = process?.isAlive != true,
412+
daemonDied = !spawn.process.isAlive,
404413
)
405414

406415
// Primitive-guarded like every other read: asBoolean on an object or array throws,
@@ -422,15 +431,12 @@ class DaemonProcessClient(
422431
/**
423432
* Launches the stdout response pump, the stderr log drain, and the process-death watcher.
424433
*
425-
* @param proc the freshly spawned child; all three coroutines live on [scope] and end when its
426-
* streams close, so they need no separate cancellation.
427-
* @param stopFlag [proc]'s own [deliberateStop] marker, closed over by the watcher so a later
428-
* spawn's marker can never answer "was this exit deliberate?" for this child.
434+
* @param spawn the freshly spawned child's whole per-spawn state; all three coroutines
435+
* live on [scope], end when the child's streams close, and close over [spawn] itself -
436+
* so however late any of them wakes, it reads and fails only its own child's state.
429437
*/
430-
private fun startReaders(
431-
proc: Process,
432-
stopFlag: AtomicBoolean,
433-
) {
438+
private fun startReaders(spawn: Spawn) {
439+
val proc = spawn.process
434440
scope.launch(Dispatchers.IO) {
435441
try {
436442
proc.inputStream.bufferedReader().forEachLine { line ->
@@ -445,7 +451,7 @@ class DaemonProcessClient(
445451
log.debug("daemon: {}", line)
446452
return@forEachLine
447453
}
448-
pending.remove(id)?.complete(json)
454+
spawn.pending.remove(id)?.complete(json)
449455
?: log.warn("Daemon response for unknown request id {}", id)
450456
}
451457
} catch (e: IOException) {
@@ -463,20 +469,22 @@ class DaemonProcessClient(
463469
}
464470
scope.launch(Dispatchers.IO) {
465471
val exitCode = runCatching { proc.waitFor() }.getOrDefault(-1)
466-
// A child the respawn replaced dies asynchronously - destroyForcibly returns before
467-
// the exit - so this can wake up after the NEXT child is already spawned. pending and
468-
// configured below are shared across spawns, so touching them then would fail the new
469-
// session's configure ("Daemon did not answer 'configure'").
470-
if (process !== proc) {
472+
// This child's own requests are failed FIRST, replaced or not: a request holds
473+
// [requestMutex] for its whole round trip, so an orphan left pending would burn
474+
// its full timeout and hold the replacement's configure behind the mutex. The
475+
// per-spawn map is what makes this safe however late the watcher wakes - a
476+
// child the respawn replaced dies asynchronously (destroyForcibly returns
477+
// before the exit), and this can run after the NEXT child is already spawned.
478+
val abandoned = IOException("Daemon process exited (code $exitCode)")
479+
spawn.pending.values.forEach { it.completeExceptionally(abandoned) }
480+
spawn.pending.clear()
481+
if (this@DaemonProcessClient.spawn !== spawn) {
471482
log.debug("Replaced quick-build daemon exited with code {}", exitCode)
472483
return@launch
473484
}
474-
val abandoned = IOException("Daemon process exited (code $exitCode)")
475-
pending.values.forEach { it.completeExceptionally(abandoned) }
476-
pending.clear()
477485
configured = false
478-
// This child's own marker, not a shared flag - see [deliberateStop].
479-
if (!stopFlag.get()) {
486+
// This child's own marker, not a shared flag - see [Spawn.deliberateStop].
487+
if (!spawn.deliberateStop.get()) {
480488
log.error("Quick-build daemon died with exit code {}", exitCode)
481489
deathListener?.invoke(exitCode)
482490
}

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

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,8 @@ class FileGenerationStore(
3939
*
4040
* @param generation the value to store; the caller guarantees it is strictly greater than
4141
* any previously saved one, since the installed proxy app keys its payloads by it.
42-
* @throws IOException when the value could not be persisted, including the second rename
43-
* attempt after clearing the destination; unlike [load] this is never swallowed, since
42+
* @throws IOException when the value could not be persisted by any means - both renames
43+
* AND the direct-write fallback failed; unlike [load] this is never swallowed, since
4444
* losing it would let a later session reuse a generation.
4545
*/
4646
override fun save(generation: Long) {
@@ -52,7 +52,17 @@ class FileGenerationStore(
5252
// keeps the store correct wherever the JVM tests run.
5353
file.delete()
5454
if (!tmp.renameTo(file)) {
55-
throw IOException("Unable to persist generation $generation to $file")
55+
// The old value is already deleted, so a bare throw here would leave NO
56+
// counter at all - the next load() would restart the sequence, the exact
57+
// reuse the class exists to rule out. Non-atomic beats lost.
58+
try {
59+
file.writeText(generation.toString())
60+
} catch (e: IOException) {
61+
throw IOException("Unable to persist generation $generation to $file", e)
62+
} finally {
63+
tmp.delete()
64+
}
65+
return
5666
}
5767
}
5868
}

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,12 @@ class QuickBuildScratch(
104104
* as a side effect, since usable space cannot be read through a directory that is not there.
105105
*/
106106
fun freeSpaceShortfall(): QuickBuildMessage? {
107-
root.mkdirs()
107+
// An uncreatable root must not fall through to the space read: usableSpace on a
108+
// nonexistent path is 0, which would report "not enough storage" - the wrong
109+
// remedy on screen - for what is a permissions or path problem.
110+
if (!root.isDirectory && !root.mkdirs()) {
111+
return QuickBuildMessage.ScratchDirUnavailable(root.absolutePath)
112+
}
108113
val usable = root.usableSpace
109114
if (usable >= minFreeBytes) return null
110115
return QuickBuildMessage.NotEnoughStorage(

0 commit comments

Comments
 (0)