[AI-2644] Let a stopped Pi agent exit on closed stdin before killing it

[realtonyyoung]

Closes #845 — AI-2644

What & why

Stopping a hosted Pi agent sent only the abort RPC, which ends the current turn but not the process, so every stop waited out the grace and tree-killed Pi (exit 137). The stop now closes Pi's stdin after the abort; Pi ends its session and exits 0 on EOF, and the kill remains only for a child that outlives the grace. The status-report and SIGHUP halves of the issue were already fixed on main by #846 and #841.

Where to look

PiRpcProcess.CloseInputAsync takes the stdin write gate with a timeout, so a write blocked on a child that stopped reading cannot hang the stop — it falls through to the kill.

Verification

• pi --mode rpc </dev/null with Pi 0.85.1: exits 0.
• Removing the stdin close fails both RequestGracefulStopAsync_lets_a_child_that_exits_on_closed_stdin_exit_cleanly and An_owner_stop_lets_pi_exit_cleanly_and_reports_completed.
• Tree filters AgentOrchestrator* 249/249, Pi* 206 passed + 17 skipped, *Status* 65/65, *Reap* 41/41.

🤖 Generated with Claude Code

[linear-code]

AI-2644

[qodo-code-review]

PR Summary by Qodo

Let hosted Pi exit cleanly when stopped

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Close Pi’s stdin after abort so it can end its session and exit normally.
• Keep stdin closure bounded and kill children that remain alive after the grace period.
• Verify clean exits and Completed status from process, runtime, and orchestrator tests.

Diagram

sequenceDiagram
    actor Owner
    participant Orchestrator
    participant Runtime
    participant Process as PiRpcProcess
    participant Pi
    participant Server
    Owner->>Orchestrator: Stop agent
    Orchestrator->>Server: Report Completed
    Orchestrator->>Runtime: Request graceful stop
    Runtime->>Process: Write abort RPC
    Process->>Pi: Abort current turn
    Runtime->>Process: Close stdin (bounded)
    Process->>Pi: Stdin EOF
    alt Pi exits during grace
        Pi-->>Process: Exit 0
        Process-->>Runtime: Exit confirmed
    else Pi remains alive
        Runtime->>Process: Terminate process tree
        Process->>Pi: Kill
    end

Loading

High-Level Assessment

Closing stdin matches Pi’s session-end behavior and preserves the existing kill fallback. Sending abort alone cannot end the process, while killing immediately would prevent a clean exit.

Files changed (7) +123 / -0

Bug fix (2) +28 / -0

IPiRpcProcess.csAdd bounded stdin closure to the Pi process abstraction +26/-0

Add bounded stdin closure to the Pi process abstraction

• Adds CloseInputAsync to the process contract and implements it under the existing stdin write gate. The gate wait is bounded so a blocked writer cannot indefinitely delay stopping the child.

src/Capacitor.Cli.Daemon/Harness/Pi/IPiRpcProcess.cs

PiRpcHostedAgentRuntime.csClose Pi stdin during graceful stop +2/-0

Close Pi stdin during graceful stop

• Closes stdin after sending abort, then waits for Pi to exit. Process-tree termination remains the fallback when the child does not exit during the grace period.

src/Capacitor.Cli.Daemon/Harness/Pi/PiRpcHostedAgentRuntime.cs

Tests (5) +95 / -0

PiHostedLaunchTests.csAdapt launch-test process fakes to stdin closure +2/-0

Adapt launch-test process fakes to stdin closure

• Adds no-op CloseInputAsync implementations to both launch-test process fakes.

test/Capacitor.Cli.Daemon.Tests.Unit/Harness/Pi/PiHostedLaunchTests.cs

PiRpcHostedAgentRuntimeTests.csVerify graceful stop avoids termination after stdin EOF +14/-0

Verify graceful stop avoids termination after stdin EOF

• Adds a test confirming that a child exiting when stdin closes receives no terminate call and retains exit code 0.

test/Capacitor.Cli.Daemon.Tests.Unit/Harness/Pi/PiRpcHostedAgentRuntimeTests.cs

PiRpcProcessTests.csExercise stdin closure against real child processes +34/-0

Exercise stdin closure against real child processes

• Tests that closing stdin lets an EOF-reading child exit normally and tolerates a second close. A non-reading child verifies that a blocked write cannot keep CloseInputAsync waiting past its gate timeout.

test/Capacitor.Cli.Daemon.Tests.Unit/Harness/Pi/PiRpcProcessTests.cs

PiRpcRuntimeFakes.csModel child exit on stdin closure in the Pi fake +10/-0

Model child exit on stdin closure in the Pi fake

• Adds a configurable exit-on-close behavior and a close-call counter for runtime and orchestrator tests.

test/Capacitor.Cli.Daemon.Tests.Unit/Harness/Pi/PiRpcRuntimeFakes.cs

AgentOrchestratorPiStopTests.csVerify owner stop reports Pi as Completed +35/-0

Verify owner stop reports Pi as Completed

• Adds an orchestrator test using the Pi runtime and an EOF-exiting process fake. It checks for Completed status with the session ID, no Failed status, and exit code 0.

test/Capacitor.Cli.Daemon.Tests.Unit/Services/AgentOrchestratorPiStopTests.cs

[qodo-code-review]

Code Review by Qodo

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

1. Stopped agents can lose final messages ✓ Resolved 🐞 Bug ≡ Correctness

Description

RequestGracefulStopAsync waits for process exit after closing stdin but does not wait for
RunPumpAsync to finish reading Pi's stdout. If Pi emits final event frames as it exits, the
orchestrator's subsequent TerminateAsync closes the transcript and cancels the pump before those
frames are processed.

Code

src/Capacitor.Cli.Daemon/Harness/Pi/PiRpcHostedAgentRuntime.cs[863]

+        await _process.CloseInputAsync(_stopGrace).ConfigureAwait(false);

Relevance

●●● Strong

Final stdout frames can race termination; preserving transcript output is a clear correctness fix.

PR-#846
PR-#710

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new EOF path makes a natural exit possible, but process exit and stdout-pump completion are
separate. The orchestrator calls runtime termination immediately after its exit waits; termination
completes the transcript and cancels the pump, while the finalizer's transcript drain occurs only
afterward.

src/Capacitor.Cli.Daemon/Harness/Pi/PiRpcHostedAgentRuntime.cs[306-342]
src/Capacitor.Cli.Daemon/Harness/Pi/PiRpcHostedAgentRuntime.cs[809-851]
src/Capacitor.Cli.Daemon/Harness/Pi/PiRpcHostedAgentRuntime.cs[855-883]
src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[3920-3948]
src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[3337-3344]

Agent prompt

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

## Issue description
A clean exit on stdin EOF can leave final stdout frames unread when the stop path immediately enters terminal teardown.
## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Harness/Pi/PiRpcHostedAgentRuntime.cs[855-875]
- src/Capacitor.Cli.Daemon/Harness/Pi/PiRpcHostedAgentRuntime.cs[306-342]
## Recommended Fix
After a clean process exit, give the stdout pump a bounded opportunity to reach EOF and process remaining frames before entering terminal teardown. Preserve the existing bounded kill fallback for a child or pump that does not finish.

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

---

2. Child-process tests run without a limit ✓ Resolved 📘 Rule violation ▣ Testability

Description

PiRpcProcessTests adds two tests that start real child processes, but the class has no
[ParallelLimiter<SubprocessLimit>] attribute. Under parallel test execution, these tests run
outside the shared subprocess limit alongside the class’s existing process tests.

Code

test/Capacitor.Cli.Daemon.Tests.Unit/Harness/Pi/PiRpcProcessTests.cs[120]

+        await using var proc = new PiRpcProcess(StartNonReadingChild(), NullLogger<PiRpcProcess>.Instance, TimeProvider.System);

Relevance

●●● Strong

Recent precedent accepts adding subprocess limiters to real-child test classes.

PR-#1102

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new test starts a real /bin/sh child through StartNonReadingChild(), while the test class
has no subprocess-limiting attribute.

Rule 2808212: Use ParallelLimiter<SubprocessLimit> for test classes that spawn real subprocesses
test/Capacitor.Cli.Daemon.Tests.Unit/Harness/Pi/PiRpcProcessTests.cs[14-14]
test/Capacitor.Cli.Daemon.Tests.Unit/Harness/Pi/PiRpcProcessTests.cs[117-120]

Agent prompt

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

## Issue description
New tests start real child processes in a class that does not participate in the shared subprocess limit.

## Fix Focus Areas
- test/Capacitor.Cli.Daemon.Tests.Unit/Harness/Pi/PiRpcProcessTests.cs[14-14]
- test/Capacitor.Cli.Daemon.Tests.Unit/Harness/Pi/PiRpcProcessTests.cs[104-120]

## Recommended Fix
Add `[ParallelLimiter<SubprocessLimit>]` to `PiRpcProcessTests` so its real-child tests use the shared limit.

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

---

3. A stopped agent can outlast its grace ✓ Resolved 🐞 Bug ☼ Reliability

Description

RequestGracefulStopAsync gives CloseInputAsync the full _stopGrace to acquire the stdin gate,
then starts a separate _stopGrace wait for exit before attempting termination. If another write
acquires the gate after the abort and blocks because the child stops reading, the close times out
and the stop spends a second grace period waiting for a child whose stdin was never closed.

Code

src/Capacitor.Cli.Daemon/Harness/Pi/PiRpcHostedAgentRuntime.cs[R863-864]

+        await _process.CloseInputAsync(_stopGrace).ConfigureAwait(false);
        await _process.WaitForExitAsync(_stopGrace).ConfigureAwait(false);

Relevance

●●● Strong

Independent grace periods can exceed the promised stop bound when stdin remains blocked.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Writes hold the stdin gate through flush, and CloseInputAsync returns without closing stdin when
its gate wait times out. The added call and the following exit wait each receive an independent full
grace period; the real-process test demonstrates a blocked writer holding the gate past the close
timeout.

src/Capacitor.Cli.Daemon/Harness/Pi/IPiRpcProcess.cs[190-210]
src/Capacitor.Cli.Daemon/Harness/Pi/IPiRpcProcess.cs[262-281]
src/Capacitor.Cli.Daemon/Harness/Pi/PiRpcHostedAgentRuntime.cs[855-869]
test/Capacitor.Cli.Daemon.Tests.Unit/Harness/Pi/PiRpcProcessTests.cs[114-129]

Agent prompt

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

## Issue description
A timed-out stdin close is followed by a fresh full exit wait, delaying the kill despite the configured stop grace.
## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Harness/Pi/PiRpcHostedAgentRuntime.cs[855-869]
- src/Capacitor.Cli.Daemon/Harness/Pi/IPiRpcProcess.cs[262-282]
## Recommended Fix
Use one deadline for gate acquisition and the subsequent exit wait. When closing stdin consumes the grace without succeeding, proceed directly to the termination fallback rather than starting another full grace period.

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

---

View medium (1)

4. A fake-only test reserves a process slot ✗ Dismissed 📘 Rule violation ▣ Testability

Description

AgentOrchestratorPiStopTests has [ParallelLimiter<SubprocessLimit>], although its test uses a
fake Pi process and a spied process factory rather than starting a child. Each run therefore
occupies a slot intended for subprocess-heavy tests.

Code

test/Capacitor.Cli.Daemon.Tests.Unit/Services/AgentOrchestratorPiStopTests.cs[9]

+[ParallelLimiter<SubprocessLimit>]

Relevance

●● Moderate

The rule supports limiting real subprocesses, but evidence is insufficient for removing it from
fake-only coverage.

PR-#1102

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The sole test obtains its runtime from PiRpcRuntimeFakes, supplies SpyPtyProcessFactory, and
seeds an agent rather than launching a real child; the class nevertheless carries the subprocess
limiter.

Rule 2808212: Use ParallelLimiter<SubprocessLimit> for test classes that spawn real subprocesses
test/Capacitor.Cli.Daemon.Tests.Unit/Services/AgentOrchestratorPiStopTests.cs[9-9]
test/Capacitor.Cli.Daemon.Tests.Unit/Services/AgentOrchestratorPiStopTests.cs[16-23]

Agent prompt

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

## Issue description
A test that uses only fakes is assigned a slot in the shared subprocess limit.

## Fix Focus Areas
- test/Capacitor.Cli.Daemon.Tests.Unit/Services/AgentOrchestratorPiStopTests.cs[9-9]

## Recommended Fix
Remove `[ParallelLimiter<SubprocessLimit>]`. Add a test-specific isolation attribute only if the test has a demonstrated shared-state conflict.

ⓘ 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

  Explored: repo: kurrent-io/kcap-server (sha: 0c67cc60) — View relationship

Review mode: ⚖️ Balanced: This changes process shutdown behavior across production runtime and orchestration paths, including timeout/concurrency handling and exit-status reporting, so it carries meaningful localized risk but not enough independent logic for extended review.

Tip of the day

💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗