Drain the version probe's pipes while waiting so a chatty --version cannot deadlock startup

[realtonyyoung]

Refs #859 — AI-2684

What & why

The daemon's startup vendor --version probe deadlocks on any CLI that prints more than the OS pipe buffer. ProbeCliVersionOnce redirected both stdout and stderr but called WaitForExit(timeout) before reading either, so a child that writes ~16-64 KB+ (some vendor CLIs emit a banner or an "update available" notice) blocks on the full pipe while the parent blocks in the wait — neither side moves until the per-attempt timeout kills the child, then it retries three times, across several probe sites. That is the multi-minute silent daemon startup: observed live as agy --version alive 75+ seconds as a daemon child though it returns in ~1 s standalone. The probe now drains both streams while the child runs, so it exits promptly and the version is read.

This is the pipe-deadlock cause only. The other AI-2684 items — moving the probes off the startup critical path so a genuinely hung CLI cannot delay the socket bind, a startup phase trace for the pre-host window, and kcap daemon status distinguishing pid-alive from serving — stay open on the issue.

Where to look

DrainToEndAsync swallows read faults to "", so the abandoned timeout path (kill then return) leaves no unobserved task exception behind the two in-flight reads.

Verification

• New DaemonRunnerVersionProbeTests.A_version_whose_output_exceeds_the_pipe_buffer_is_drained_not_deadlocked: a POSIX stub floods 500 KB to stderr before its version line. Before the fix the probe hit its 3 s timeout and returned null; after, it returns 9.9.9. Red-then-green confirmed.
• Full Capacitor.Cli.Daemon.Tests.Unit suite green; daemon AOT publish clean.

[qodo-code-review]

PR Summary by Qodo

Drain version probe output to prevent startup deadlocks

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Drain version-probe stdout and stderr concurrently to prevent full-pipe startup deadlocks.
• Preserve timeout handling while safely suppressing abandoned drain read failures.
• Add a POSIX regression test using 500 KB of stderr output.

Diagram

sequenceDiagram
    participant D as Daemon Runner
    participant P as Vendor CLI
    participant O as Stdout Drain
    participant E as Stderr Drain
    participant V as Version Parser
    D->>P: Start --version
    par Drain stdout
        P-->>O: stdout bytes
    and Drain stderr
        P-->>E: stderr bytes
    end
    D->>P: Wait with timeout
    P-->>D: Exit
    O-->>D: stdout text
    E-->>D: stderr text
    D->>V: Parse output
    V-->>D: Version

Loading

High-Level Assessment

Concurrent ReadToEndAsync drains are the appropriate minimal fix because they preserve the synchronous probe API, existing timeout behavior, and output parsing while removing pipe backpressure. Fully asynchronous process waiting or temporary-file redirection would add complexity without improving this focused correction.

Files changed (2) +40 / -4

Bug fix (1) +21 / -4

DaemonRunner.csDrain version probe pipes before waiting for exit +21/-4

Drain version probe pipes before waiting for exit

• Starts asynchronous reads for stdout and stderr before waiting for the vendor process, preventing verbose output from filling redirected pipes. Read failures resolve to empty output so timeout and process-kill paths cannot leave faulted tasks unobserved.

src/Capacitor.Cli.Daemon/DaemonRunner.cs

Tests (1) +19 / -0

DaemonRunnerVersionProbeTests.csCover version probes that exceed pipe capacity +19/-0

Cover version probes that exceed pipe capacity

• Adds a POSIX regression test whose stub writes approximately 500 KB to stderr before returning a version on stdout. The assertion verifies that the probe drains both streams and returns the version instead of timing out.

test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (2) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Chatty probes can exhaust daemon memory ✓ Resolved 🐞 Bug ☼ Reliability

Description

DrainToEndAsync materializes both redirected streams with unbounded ReadToEndAsync calls,
retaining even discarded stderr until the probe completes. A continuously writing CLI can therefore
consume substantial daemon memory during each 3-second or 10-second attempt, and registration
retries can repeat that allocation three times during startup.

Code

src/Capacitor.Cli.Daemon/DaemonRunner.cs[R1571-1572]

+            var stdout = DrainToEndAsync(process.StandardOutput);
+            var stderr = DrainToEndAsync(process.StandardError);

Relevance

●●● Strong

PR #402 accepted the same full-stream buffering risk and recommended capped streaming drains.

PR-#402

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new helper captures each complete stream in a string, while the probe budgets permit up to 10
seconds of output and registration performs as many as three attempts. The added regression test
deliberately demonstrates that outputs are already expected to exceed ordinary pipe sizes, and past
PR #402 identified the same unbounded ReadToEndAsync pattern as a memory risk requiring capped
streaming drains.

src/Capacitor.Cli.Daemon/DaemonRunner.cs[1526-1549]
src/Capacitor.Cli.Daemon/DaemonRunner.cs[1571-1587]
test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs[61-70]
PR-#402

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The version probe drains stdout and stderr into unbounded strings, allowing a high-volume CLI to exhaust daemon memory before the process timeout expires. Both pipes must continue being drained to prevent deadlock, but only a bounded prefix needed for version parsing should be retained.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/DaemonRunner.cs[1571-1587]

## Recommended Fix
Replace `ReadToEndAsync` with concurrent streaming drains that retain only a fixed, reasonable number of characters from each stream while discarding all additional data. Continue reading through EOF so the child cannot block, await both drains on successful exit, and add coverage showing that output beyond the capture limit is drained without being retained while a version within the retained prefix is still parsed.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Probe tests bypass subprocess limits 📘 Rule violation ▣ Testability

Description

DaemonRunnerVersionProbeTests adds a test that launches a real POSIX shell process through
ProbeCliVersionForLaunch without placing the class under ParallelLimiter<SubprocessLimit>. Under
the default concurrent test schedule, this probe competes outside the suite-wide subprocess budget
with every other process-heavy test class.

Code

test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs[70]

+        await Assert.That(DaemonRunner.ProbeCliVersionForLaunch(cli)).IsEqualTo("9.9.9");

Relevance

●●● Strong

Process-heavy tests receive explicit concurrency controls; this real subprocess probe should follow
that repository pattern.

PR-#627
PR-#584

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance rule 2808212 requires test classes that invoke real child processes to use
ParallelLimiter<SubprocessLimit>. The class has no limiter at its declaration, while the added
test creates an executable shell script and passes it to the production version probe that starts
the process.

Rule 2808212: Use ParallelLimiter<SubprocessLimit> for test classes running real subprocesses
test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs[13-13]
test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs[61-70]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`DaemonRunnerVersionProbeTests` now runs a real shell subprocess but does not participate in the suite-wide subprocess concurrency limit.

## Fix Focus Areas
- test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs[13-13]
- test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs[61-70]

## Recommended Fix
Annotate `DaemonRunnerVersionProbeTests` with `[ParallelLimiter<SubprocessLimit>]` so this real-process test draws from the shared subprocess budget.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Test temp storage bypasses injection 📘 Rule violation ▣ Testability

Description

A_version_whose_output_exceeds_the_pipe_buffer_is_drained_not_deadlocked constructs and disposes a
local TempDir instead of using a framework-injected [TempDir] property. Whenever this test runs,
its temporary state follows a manually managed lifecycle that differs from the required lifecycle
used by other TUnit fixtures.

Code

test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs[63]

+        using var tmp = new TempDir();

Relevance

●●● Strong

Recent test precedents enforce framework-managed temporary resources instead of manually
constructing TempDir.

PR-#844
PR-#590

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance rule 2808173 prohibits new TempDir() in test classes and requires a public required
[TempDir] property. The added test directly constructs TempDir, and the containing class
declares no injected property.

Rule 2808173: Use injected [TempDir] public required property in test classes instead of manual fields
test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs[13-13]
test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs[61-66]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The added test manually creates `TempDir`, while test classes must receive temporary storage through one public required property annotated with `[TempDir]`.

## Fix Focus Areas
- test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs[13-13]
- test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs[61-66]

## Recommended Fix
Add `[TempDir] public required TempDir Tmp { get; init; }` to the test class, remove the local `new TempDir()`, and create the stub through `Tmp.CreateFile(...)`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Compliance rules (platform): 64 rules

✅ Cross-repo context — repo relationships

Review mode: ⚖️ Balanced: This is a localized runtime process/I/O concurrency fix affecting daemon startup, with timeout, stream-draining, and process-lifecycle edge cases that warrant a complete review but not redundant extended passes.

Tip of the day

💡 Did you know, you can turn these tips off under Display preferences

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-code-review]

2. Probe tests bypass subprocess limits 📘 Rule violation ▣ Testability

DaemonRunnerVersionProbeTests adds a test that launches a real POSIX shell process through
ProbeCliVersionForLaunch without placing the class under ParallelLimiter<SubprocessLimit>. Under
the default concurrent test schedule, this probe competes outside the suite-wide subprocess budget
with every other process-heavy test class.

Agent Prompt

## Issue description
`DaemonRunnerVersionProbeTests` now runs a real shell subprocess but does not participate in the suite-wide subprocess concurrency limit.

## Fix Focus Areas
- test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs[13-13]
- test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs[61-70]

## Recommended Fix
Annotate `DaemonRunnerVersionProbeTests` with `[ParallelLimiter<SubprocessLimit>]` so this real-process test draws from the shared subprocess budget.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-code-review]

3. Test temp storage bypasses injection 📘 Rule violation ▣ Testability

A_version_whose_output_exceeds_the_pipe_buffer_is_drained_not_deadlocked constructs and disposes a
local TempDir instead of using a framework-injected [TempDir] property. Whenever this test runs,
its temporary state follows a manually managed lifecycle that differs from the required lifecycle
used by other TUnit fixtures.

Agent Prompt

## Issue description
The added test manually creates `TempDir`, while test classes must receive temporary storage through one public required property annotated with `[TempDir]`.

## Fix Focus Areas
- test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs[13-13]
- test/Capacitor.Cli.Daemon.Tests.Unit/DaemonRunnerVersionProbeTests.cs[61-66]

## Recommended Fix
Add `[TempDir] public required TempDir Tmp { get; init; }` to the test class, remove the local `new TempDir()`, and create the stub through `Tmp.CreateFile(...)`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[realtonyyoung]

Ran a Codex review of this branch (codex exec review --base main). One finding, now addressed:

• Unbounded probe buffers (P2). The concurrent drain kept every byte in a string, removing pipe backpressure, so a CLI that floods --version could allocate without limit for the whole probe budget. Fixed by bounding retention to an 8 KB prefix (the version is at the front) while still reading each stream to EOF unretained, so the child never blocks. New test A_version_at_the_front_survives_a_trailing_flood_on_the_same_stream guards both halves — it fails if the drain stops at the cap.

No other findings.