From 248bfa37ef73b7ae000e2492ce26cd796a31cdbf Mon Sep 17 00:00:00 2001 From: Raman Gupta Date: Wed, 4 Nov 2026 09:56:16 -0500 Subject: [PATCH 1/2] fix(git): make spawn timeouts actually kill a hung git The timeout was a waitFor() after reading stdout to EOF, which a hung git never reaches, so no timeout ever fired. A field report had a fetch running for 2h 11m against its 90s limit. A shared reaper thread now kills git and its descendants when time runs out. The descendants matter: a fetch's ssh, or the shell behind an alias, holds the same stdout, so killing git alone would leave the read waiting. Signed-off-by: Raman Gupta --- src/main/kotlin/com/gitvantage/git/Git.kt | 40 +++++++++++++++++-- .../kotlin/com/gitvantage/git/GitPerfTest.kt | 27 +++++++++++++ 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/src/main/kotlin/com/gitvantage/git/Git.kt b/src/main/kotlin/com/gitvantage/git/Git.kt index 602e730..2e28055 100644 --- a/src/main/kotlin/com/gitvantage/git/Git.kt +++ b/src/main/kotlin/com/gitvantage/git/Git.kt @@ -6,7 +6,10 @@ package com.gitvantage.git import com.gitvantage.model.Activity import com.gitvantage.model.Perf import java.io.File +import java.util.concurrent.Executors +import java.util.concurrent.ScheduledThreadPoolExecutor import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicBoolean /** * The one place the app spawns `git`. @@ -189,6 +192,12 @@ object Git { * malloc and Objective-C locks the UI thread needs, so the app grew laggier the longer it ran * (and the bigger the process got). Mutations through [run] keep the default; a commit or a * switch is expected to write the index, and the app is the one asking. + * + * [timeoutSeconds] is enforced by [reaper], which kills git *and everything it started* when + * time runs out. It used to be a `waitFor(timeout)` after reading stdout to EOF — which a hung + * git never reaches, so no timeout ever fired: a field report had a `fetch` running for 2h 11m + * against its 90s. The children matter as much as git: a fetch's `ssh`, or the shell behind an + * alias, holds the same stdout, and the read only ends when the last of them lets go. */ private fun spawnNow(dir: File, args: List, timeoutSeconds: Long, captureErr: Boolean, readOnly: Boolean): Result = runCatching { val pb = ProcessBuilder(listOf("git") + args).directory(dir) @@ -196,20 +205,43 @@ object Git { if (readOnly) pb.environment()["GIT_OPTIONAL_LOCKS"] = "0" val proc = pb.start() + val timedOut = AtomicBoolean(false) + val deadline = reaper.schedule({ timedOut.set(true); killTree(proc) }, timeoutSeconds, TimeUnit.SECONDS) + var err = "" val pump = if (captureErr) { Thread { err = runCatching { proc.errorStream.bufferedReader().readText() }.getOrDefault("") } .apply { isDaemon = true; start() } } else null - val out = proc.inputStream.bufferedReader().readText() - val finished = proc.waitFor(timeoutSeconds, TimeUnit.SECONDS) - if (!finished) proc.destroyForcibly() + val out = try { + proc.inputStream.bufferedReader().readText().also { proc.waitFor() } + } finally { + deadline.cancel(false) + } // join() also publishes the pump thread's write to `err` to this thread. pump?.join(TimeUnit.SECONDS.toMillis(2)) - if (finished) Result(proc.exitValue(), out, err) else Result(-1, out, "timed out") + if (timedOut.get()) Result(-1, out, "timed out") else Result(proc.exitValue(), out, err) }.getOrElse { e -> Result(-1, "", e.message ?: "failed to run git") } + + /** + * Enforces every spawn's timeout: one daemon thread for the whole app rather than a thread per + * spawn, since reads run many times per repo per scan and almost all of them finish in + * milliseconds. Cancelled deadlines are removed at once, so they don't pile up in its queue. + */ + private val reaper = (Executors.newScheduledThreadPool(1) { r -> Thread(r, "git-timeout").apply { isDaemon = true } } + as ScheduledThreadPoolExecutor).apply { removeOnCancelPolicy = true } + + /** + * Kill [proc] and its descendants. Children first, listed while git is still alive: once git + * dies they are reparented, stop being its descendants, and keep the pipe open with nobody + * left to find them. + */ + private fun killTree(proc: Process) { + runCatching { proc.descendants().forEach { it.destroyForcibly() } } + proc.destroyForcibly() + } } diff --git a/src/test/kotlin/com/gitvantage/git/GitPerfTest.kt b/src/test/kotlin/com/gitvantage/git/GitPerfTest.kt index d11d2c6..a470c93 100644 --- a/src/test/kotlin/com/gitvantage/git/GitPerfTest.kt +++ b/src/test/kotlin/com/gitvantage/git/GitPerfTest.kt @@ -32,3 +32,30 @@ val GitPerfCounters by testSuite { } } } + +/** + * A git that never finishes is killed when its time is up, with whatever it started. + * + * The timeout used to be checked only after stdout reached EOF, which a hung git never gets to, so + * a stalled `fetch` ran for hours. The alias runs `sleep` under a shell: a grandchild holding git's + * stdout, the way a fetch's `ssh` does, so killing git alone would still leave the read waiting. + */ +val GitTimeout by testSuite { + testFixture { Sandbox() } asContextForEach { + + test("a hung git and its children are killed at the timeout") { + val work = repo() + val started = System.nanoTime() + val out = Git.readOrNull(work, "-c", "alias.hang=!sleep 60", "hang", timeoutSeconds = 1) + val tookMs = (System.nanoTime() - started) / 1_000_000 + + assert(out == null) { "a timed-out git read as success: '$out'" } + assert(tookMs < 10_000) { "the 1s timeout took ${tookMs}ms to fire" } + } + + test("a git that finishes in time is unaffected") { + val work = repo() + assert(Git.readOrNull(work, "rev-parse", "--is-inside-work-tree", timeoutSeconds = 5)?.trim() == "true") + } + } +} From 15a3a01174129d008f3d475c4f2c10b160ce10d7 Mon Sep 17 00:00:00 2001 From: Raman Gupta Date: Wed, 4 Nov 2026 10:24:30 -0500 Subject: [PATCH 2/2] refactor(git): run the spawn timeout as a coroutine beside the read A timeout coroutine started next to the read, under one runBlocking parent, and cancelled when the read finishes, says what the shared reaper executor did in less code. Signed-off-by: Raman Gupta --- src/main/kotlin/com/gitvantage/git/Git.kt | 45 ++++++++++++----------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/src/main/kotlin/com/gitvantage/git/Git.kt b/src/main/kotlin/com/gitvantage/git/Git.kt index 2e28055..e75572f 100644 --- a/src/main/kotlin/com/gitvantage/git/Git.kt +++ b/src/main/kotlin/com/gitvantage/git/Git.kt @@ -5,11 +5,13 @@ package com.gitvantage.git import com.gitvantage.model.Activity import com.gitvantage.model.Perf +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.delay +import kotlinx.coroutines.launch +import kotlinx.coroutines.runBlocking import java.io.File -import java.util.concurrent.Executors -import java.util.concurrent.ScheduledThreadPoolExecutor import java.util.concurrent.TimeUnit -import java.util.concurrent.atomic.AtomicBoolean +import kotlin.time.Duration.Companion.seconds /** * The one place the app spawns `git`. @@ -193,8 +195,8 @@ object Git { * (and the bigger the process got). Mutations through [run] keep the default; a commit or a * switch is expected to write the index, and the app is the one asking. * - * [timeoutSeconds] is enforced by [reaper], which kills git *and everything it started* when - * time runs out. It used to be a `waitFor(timeout)` after reading stdout to EOF — which a hung + * [timeoutSeconds] is a second coroutine beside the read, which kills git *and everything it + * started* when time runs out and is cancelled when the read finishes first. It used to be a `waitFor(timeout)` after reading stdout to EOF — which a hung * git never reaches, so no timeout ever fired: a field report had a `fetch` running for 2h 11m * against its 90s. The children matter as much as git: a fetch's `ssh`, or the shell behind an * alias, holds the same stdout, and the read only ends when the last of them lets go. @@ -205,36 +207,37 @@ object Git { if (readOnly) pb.environment()["GIT_OPTIONAL_LOCKS"] = "0" val proc = pb.start() - val timedOut = AtomicBoolean(false) - val deadline = reaper.schedule({ timedOut.set(true); killTree(proc) }, timeoutSeconds, TimeUnit.SECONDS) - var err = "" val pump = if (captureErr) { Thread { err = runCatching { proc.errorStream.bufferedReader().readText() }.getOrDefault("") } .apply { isDaemon = true; start() } } else null - val out = try { - proc.inputStream.bufferedReader().readText().also { proc.waitFor() } - } finally { - deadline.cancel(false) + var timedOut = false + // runBlocking returns only once the timeout coroutine has finished, cancelled or not, which + // also makes its write to `timedOut` visible here. + val out = runBlocking { + // Not on runBlocking's own thread: that thread is about to block in the read, and a + // timeout waiting for it to be free would never fire. + val timeout = launch(Dispatchers.Default) { + delay(timeoutSeconds.seconds) + timedOut = true + killTree(proc) + } + try { + proc.inputStream.bufferedReader().readText().also { proc.waitFor() } + } finally { + timeout.cancel() + } } // join() also publishes the pump thread's write to `err` to this thread. pump?.join(TimeUnit.SECONDS.toMillis(2)) - if (timedOut.get()) Result(-1, out, "timed out") else Result(proc.exitValue(), out, err) + if (timedOut) Result(-1, out, "timed out") else Result(proc.exitValue(), out, err) }.getOrElse { e -> Result(-1, "", e.message ?: "failed to run git") } - /** - * Enforces every spawn's timeout: one daemon thread for the whole app rather than a thread per - * spawn, since reads run many times per repo per scan and almost all of them finish in - * milliseconds. Cancelled deadlines are removed at once, so they don't pile up in its queue. - */ - private val reaper = (Executors.newScheduledThreadPool(1) { r -> Thread(r, "git-timeout").apply { isDaemon = true } } - as ScheduledThreadPoolExecutor).apply { removeOnCancelPolicy = true } - /** * Kill [proc] and its descendants. Children first, listed while git is still alive: once git * dies they are reparented, stop being its descendants, and keep the pipe open with nobody