From d7919774dea17918066eaf5daa3f5a760c8d0a89 Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Sat, 13 Jun 2026 17:00:22 +0200 Subject: [PATCH 1/5] Fix Claude watcher parent-PID watchdog never starting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `kcap watch` sidecar's parent-exit watchdog self-terminates and POSTs session-end when the coding agent dies, so an abruptly-killed agent (crash, SIGKILL, daemon death) doesn't leave the watcher orphaned and the session stuck "active". For Claude it silently never started: GetCodingAgentPid() used getpgrp(), which resolves the durable agent for Codex but a transient hook process group for Claude — already dead by the time the watcher's IsProcessAlive guard ran, so monitoring was skipped with no log line. Resolve the agent PID by walking the ppid ancestry and matching the agent by name (claude/codex), robust to the differing process-group/launcher topologies of each vendor. Add per-PID introspection (macOS proc_pidinfo, Linux /proc//stat). Harden the watcher startup guard (DecideParentWatchdog) so a dead-at-startup parent PID is logged loudly instead of silently skipped. Co-Authored-By: Claude Opus 4.8 --- src/Capacitor.Cli/Commands/WatchCommand.cs | 81 +++++++-- src/Capacitor.Cli/ProcessHelpers.cs | 166 +++++++++++++++++- src/Capacitor.Cli/WatcherManager.cs | 15 +- .../CodingAgentPidResolverTests.cs | 131 ++++++++++++++ .../ParentWatchdogDecisionTests.cs | 35 ++++ .../ProcessHelpersTests.cs | 43 +++++ 6 files changed, 446 insertions(+), 25 deletions(-) create mode 100644 test/Capacitor.Cli.Tests.Unit/CodingAgentPidResolverTests.cs create mode 100644 test/Capacitor.Cli.Tests.Unit/ParentWatchdogDecisionTests.cs diff --git a/src/Capacitor.Cli/Commands/WatchCommand.cs b/src/Capacitor.Cli/Commands/WatchCommand.cs index 47afa1bd04..ce223c19fd 100644 --- a/src/Capacitor.Cli/Commands/WatchCommand.cs +++ b/src/Capacitor.Cli/Commands/WatchCommand.cs @@ -11,6 +11,30 @@ namespace Capacitor.Cli.Commands; static partial class WatchCommand { + /// Outcome of deciding whether the parent-exit watchdog can run. + internal enum ParentWatchdog { + /// Parent PID is alive — start the 5s liveness poll. + Monitor, + + /// No parent PID was supplied — nothing to monitor. + NoParentPid, + + /// A parent PID was supplied but it's already dead at startup + /// (typically a transient process from bad PID resolution). Must be surfaced, + /// never silently skipped. + ParentAlreadyDead + } + + /// + /// Decides whether the parent-exit watchdog should run. Pure so the three + /// outcomes — including the dead-at-startup case that caused stuck sessions — + /// are unit-testable without spawning processes. + /// + internal static ParentWatchdog DecideParentWatchdog(int? parentPid, Func isAlive) => + parentPid is not { } ppid ? ParentWatchdog.NoParentPid + : !isAlive(ppid) ? ParentWatchdog.ParentAlreadyDead + : ParentWatchdog.Monitor; + public static async Task RunWatch( string baseUrl, string sessionId, @@ -85,26 +109,47 @@ public static async Task RunWatch( ctx.Cancel = true; }); - // Watch the spawning claude process. If it dies without firing session-end - // (crash, force-kill, IDE-detach), self-terminate within ~5s instead of orphaning. - if (parentPid is { } ppid && ProcessHelpers.IsProcessAlive(ppid)) { - Log($"Monitoring parent pid {ppid}"); - _ = Task.Run(async () => { - while (!cts.Token.IsCancellationRequested) { - try { - await Task.Delay(TimeSpan.FromSeconds(5), cts.Token); - } catch (OperationCanceledException) { - return; - } + // Watch the spawning coding-agent process. If it dies without firing + // session-end (crash, force-kill, IDE-detach), self-terminate within ~5s and + // POST session-end instead of orphaning. Crucially, the cases where we DON'T + // monitor are now logged: a silently-skipped watchdog is exactly the failure + // that left sessions stuck "active" with the watcher still connected — the + // resolved parent PID was already dead at startup and nothing recorded it. + switch (DecideParentWatchdog(parentPid, ProcessHelpers.IsProcessAlive)) { + case ParentWatchdog.NoParentPid: + Log("No parent pid supplied; parent-exit watchdog disabled (session-end relies on the agent's own hook)"); - if (!ProcessHelpers.IsProcessAlive(ppid)) { - Log($"Parent pid {ppid} exited; shutting down watcher"); - Interlocked.Exchange(ref parentExited, 1); - cts.Cancel(); - return; + break; + + case ParentWatchdog.ParentAlreadyDead: + Log($"Parent pid {parentPid} already dead at watcher startup; parent-exit watchdog NOT started — " + + "session-end will not be POSTed if the agent ends abruptly. This usually means parent-PID " + + "resolution returned a transient process; see ProcessHelpers.GetCodingAgentPid."); + + break; + + case ParentWatchdog.Monitor: + var ppid = parentPid!.Value; + Log($"Monitoring parent pid {ppid}"); + _ = Task.Run(async () => { + while (!cts.Token.IsCancellationRequested) { + try { + await Task.Delay(TimeSpan.FromSeconds(5), cts.Token); + } catch (OperationCanceledException) { + return; + } + + if (!ProcessHelpers.IsProcessAlive(ppid)) { + Log($"Parent pid {ppid} exited; shutting down watcher"); + Interlocked.Exchange(ref parentExited, 1); + cts.Cancel(); + + return; + } } - } - }, cts.Token); + }, cts.Token); + + break; } var state = new WatchState(); diff --git a/src/Capacitor.Cli/ProcessHelpers.cs b/src/Capacitor.Cli/ProcessHelpers.cs index e5fbe66a26..29ff0f0f4d 100644 --- a/src/Capacitor.Cli/ProcessHelpers.cs +++ b/src/Capacitor.Cli/ProcessHelpers.cs @@ -1,5 +1,6 @@ using System.Runtime.CompilerServices; using System.Runtime.InteropServices; +using System.Text; namespace Capacitor.Cli; @@ -30,6 +31,18 @@ static partial class ProcessHelpers { [UnmanagedCallConv(CallConvs = [typeof(CallConvCdecl)])] private static partial int setsid_native(); + // macOS: proc_pidinfo(pid, PROC_PIDTBSDINFO, 0, &info, sizeof(info)) fills a + // proc_bsdinfo struct. We read only pbi_ppid (offset 16) and pbi_name (offset + // 64, 32 bytes) out of the 136-byte struct, so no struct marshalling is needed. + [LibraryImport("libproc", EntryPoint = "proc_pidinfo")] + [UnmanagedCallConv(CallConvs = [typeof(CallConvCdecl)])] + private static unsafe partial int proc_pidinfo(int pid, int flavor, ulong arg, byte* buffer, int buffersize); + + const int ProcPidTBsdInfo = 3; + const int ProcBsdInfoSize = 136; + const int ProcBsdInfoPpid = 16; + const int ProcBsdInfoName = 64; // pbi_name[2*MAXCOMLEN] = 32 bytes + [LibraryImport("kernel32.dll", SetLastError = true)] private static partial nint OpenProcess(uint dwDesiredAccess, [MarshalAs(UnmanagedType.Bool)] bool bInheritHandle, uint dwProcessId); @@ -209,11 +222,30 @@ internal static bool TryClearInheritFlag(nint handle) { /// parent PID. Codex on Windows is uncommon and the Claude path is already /// covered by Claude's own SessionEnd hook. /// - public static int? GetCodingAgentPid() { + public static int? GetCodingAgentPid() => GetCodingAgentPid(vendor: null); + + /// + /// Resolves the PID of the long-lived coding-agent process for + /// ("claude"/"codex"), suitable for the watcher's + /// parent-PID watchdog. On Unix it first walks the ppid ancestry looking for the + /// agent by name ( + ), + /// which is robust to whatever process-group/launcher topology the agent uses to + /// spawn its hook — notably Claude, whose hook runs in a separate transient + /// process group that makes the bare getpgrp() heuristic resolve an + /// already-dead PID. It falls back to that legacy heuristic when no agent is found + /// on the chain (or is unknown), preserving prior + /// behaviour for Codex. + /// + public static int? GetCodingAgentPid(string? vendor) { if (OperatingSystem.IsWindows()) { return GetParentPidWindows(); } + if (vendor is "claude" or "codex" + && ResolveCodingAgentPid(getppid_native(), vendor, GetProcessInfo) is { } agentPid) { + return agentPid; + } + // Fall back to getppid for the degenerate cases: pgid <= 1 means no // group / init, and pgid == our own pid means we're our own group // leader — monitoring ourselves would let the watcher self-terminate. @@ -222,6 +254,138 @@ internal static bool TryClearInheritFlag(nint handle) { return pgid > 1 && pgid != getpid_native() ? pgid : getppid_native(); } + /// + /// Walks the parent-process chain from and returns + /// the PID of the nearest ancestor whose executable name identifies the coding + /// agent for ("claude"/"codex"), or null if none is + /// found within . Pure: the process table is supplied + /// via (pid → (ppid, comm)) so it is unit-testable + /// with synthetic ancestries and shared across platforms. + /// + /// + /// The durable coding-agent process always sits on the ppid chain above the + /// short-lived hook/launcher process, whatever the process-group topology — + /// which is why the chain walk is reliable where getpgrp()/getppid() + /// alone are not. Claude in particular spawns its hook in a separate, transient + /// process group, so getpgrp() resolved a PID that was already dead by the + /// time the watcher checked it, and the parent-PID watchdog silently never started. + /// + internal static int? ResolveCodingAgentPid( + int startPid, + string vendor, + Func lookup, + int maxHops = 16 + ) { + var pid = startPid; + + for (var hop = 0; hop < maxHops && pid > 1; hop++) { + if (lookup(pid) is not { } info) { + return null; + } + + if (MatchesAgentName(info.comm, vendor)) { + return pid; + } + + pid = info.ppid; + } + + return null; + } + + /// + /// Returns (ppid, comm) for an arbitrary live PID, or null if the process + /// can't be inspected (gone, or unsupported platform). Feeds the ancestry walk in + /// . Unix-only — Windows keeps the legacy + /// parent-PID path (codex/Windows is uncommon and claude has its own SessionEnd + /// hook there), so this returns null on Windows. + /// + public static (int ppid, string comm)? GetProcessInfo(int pid) { + if (pid <= 0 || OperatingSystem.IsWindows()) { + return null; + } + + return OperatingSystem.IsMacOS() ? GetProcessInfoMac(pid) : GetProcessInfoLinux(pid); + } + + static unsafe (int ppid, string comm)? GetProcessInfoMac(int pid) { + Span buf = stackalloc byte[ProcBsdInfoSize]; + buf.Clear(); + + int written; + + fixed (byte* p = buf) { + written = proc_pidinfo(pid, ProcPidTBsdInfo, 0, p, ProcBsdInfoSize); + } + + // proc_pidinfo returns the number of bytes written; anything short of the + // full struct means the call failed (e.g. no such process). + if (written < ProcBsdInfoSize) { + return null; + } + + var ppid = BitConverter.ToInt32(buf.Slice(ProcBsdInfoPpid, sizeof(int))); + + var nameSpan = buf.Slice(ProcBsdInfoName, 32); + var nul = nameSpan.IndexOf((byte)0); + var comm = Encoding.UTF8.GetString(nul >= 0 ? nameSpan[..nul] : nameSpan); + + return (ppid, comm); + } + + static (int ppid, string comm)? GetProcessInfoLinux(int pid) { + string stat; + + try { + stat = File.ReadAllText($"/proc/{pid}/stat"); + } catch { + return null; + } + + // Format: "pid (comm) state ppid ...". comm can contain spaces and parens, + // so anchor on the LAST ')': name is between the first '(' and last ')', + // and ppid is the 2nd whitespace field after it (state is the 1st). + var open = stat.IndexOf('('); + var close = stat.LastIndexOf(')'); + + if (open < 0 || close <= open) { + return null; + } + + var comm = stat.Substring(open + 1, close - open - 1); + var rest = stat[(close + 1)..].Split(' ', StringSplitOptions.RemoveEmptyEntries); + + return rest.Length >= 2 && int.TryParse(rest[1], out var ppid) ? (ppid, comm) : null; + } + + /// + /// True when names the coding-agent executable for + /// . Matches the basename exactly (optionally plus a + /// single file extension like .exe), case-insensitively — never a loose + /// substring, so descriptive names such as the Electron desktop "Claude Helper + /// (Renderer)" don't masquerade as the claude CLI. + /// + static bool MatchesAgentName(string comm, string vendor) { + if (string.IsNullOrEmpty(comm)) { + return false; + } + + var slash = comm.LastIndexOfAny(['/', '\\']); + var basename = slash >= 0 ? comm[(slash + 1)..] : comm; + + if (basename.Equals(vendor, StringComparison.OrdinalIgnoreCase)) { + return true; + } + + // "claude.exe" style: vendor token followed by a single extension and nothing else. + var dot = basename.IndexOf('.'); + + return dot > 0 + && basename[..dot].Equals(vendor, StringComparison.OrdinalIgnoreCase) + && !basename.AsSpan(dot + 1).Contains('.') + && !basename.AsSpan(dot + 1).Contains(' '); + } + /// /// Detaches the current process from its controlling terminal on Unix so that /// closing the parent terminal does not deliver SIGHUP to this process. diff --git a/src/Capacitor.Cli/WatcherManager.cs b/src/Capacitor.Cli/WatcherManager.cs index f7adcf882b..bd3e9ada8f 100644 --- a/src/Capacitor.Cli/WatcherManager.cs +++ b/src/Capacitor.Cli/WatcherManager.cs @@ -65,12 +65,15 @@ public static async Task SpawnWatcher( Directory.CreateDirectory(watcherDir); var kcapPath = Environment.ProcessPath ?? "kcap"; - // Use the coding-agent's PID (process group leader on Unix) rather than - // getppid(): coding agents invoke hooks through a transient executor that - // dies the moment the hook returns, so by the time the watcher checks - // IsProcessAlive it sees a dead PID and never starts the monitor task — - // leaving Codex sessions stuck "active" because session-end is never POSTed. - var parentPid = ProcessHelpers.GetCodingAgentPid(); + // Resolve the long-lived coding-agent PID rather than getppid(): coding + // agents invoke hooks through a transient executor that dies the moment the + // hook returns, so by the time the watcher checks IsProcessAlive it sees a + // dead PID and never starts the monitor task — leaving sessions stuck + // "active" because session-end is never POSTed. The vendor-aware resolver + // walks the ppid ancestry to find the agent by name, which is robust to the + // differing process-group topologies of Claude (transient hook group → bare + // getpgrp() resolves a dead PID) and Codex (inherits the agent's group). + var parentPid = ProcessHelpers.GetCodingAgentPid(vendor); var arguments = BuildSpawnArgs(key, transcriptPath, agentId, sessionIdOverride, cwd, skipTitle, parentPid, vendor); var psi = new ProcessStartInfo(kcapPath, arguments) { diff --git a/test/Capacitor.Cli.Tests.Unit/CodingAgentPidResolverTests.cs b/test/Capacitor.Cli.Tests.Unit/CodingAgentPidResolverTests.cs new file mode 100644 index 0000000000..e5422180a2 --- /dev/null +++ b/test/Capacitor.Cli.Tests.Unit/CodingAgentPidResolverTests.cs @@ -0,0 +1,131 @@ +namespace Capacitor.Cli.Tests.Unit; + +/// +/// A synthetic process table (pid -> (ppid, comm)) for resolver tests. Kept out of +/// the test class so the TUnit source generator only sees [Test] methods there. +/// +static class ProcTable { + public static Func Of(params (int pid, int ppid, string comm)[] rows) => + pid => { + foreach (var (p, ppid, comm) in rows) { + if (p == pid) return (ppid, comm); + } + + return null; + }; +} + +/// +/// Tests for the pure ancestry-walk resolver behind GetCodingAgentPid. +/// The watcher's parent-PID watchdog only works if this resolves the durable +/// coding-agent process (claude/codex) rather than a transient hook/launcher +/// process that dies the moment the hook returns. Claude spawns its hook in a +/// separate, short-lived process group, so the old getpgrp() heuristic +/// resolved a dead PID and the watchdog silently never started — orphaning the +/// watcher and leaving the session stuck "active". The walk follows the ppid +/// chain (where the durable agent always appears) and matches by process name. +/// +public class CodingAgentPidResolverTests { + [Test] + public async Task Resolves_claude_when_it_is_the_immediate_parent() { + // hook(100) -> claude(50) -> zsh(20) -> init(1) + var lookup = ProcTable.Of((50, 20, "claude"), (20, 1, "-zsh")); + + var pid = ProcessHelpers.ResolveCodingAgentPid(startPid: 50, vendor: "claude", lookup); + + await Assert.That(pid).IsEqualTo(50); + } + + [Test] + public async Task Skips_transient_launcher_and_resolves_claude_higher_up() { + // The real failure mode: a transient launcher/shell sits between the hook + // and claude, so getppid() points at the launcher (which dies immediately). + // hook(100) -> sh(90) -> claude(50) -> zsh(20) + var lookup = ProcTable.Of((90, 50, "sh"), (50, 20, "claude"), (20, 1, "-zsh")); + + var pid = ProcessHelpers.ResolveCodingAgentPid(startPid: 90, vendor: "claude", lookup); + + await Assert.That(pid).IsEqualTo(50); + } + + [Test] + public async Task Resolves_codex_by_name() { + // codex(50) -> zsh(20). The ancestry walk must work uniformly for both vendors. + var lookup = ProcTable.Of((50, 20, "codex"), (20, 1, "-zsh")); + + var pid = ProcessHelpers.ResolveCodingAgentPid(startPid: 50, vendor: "codex", lookup); + + await Assert.That(pid).IsEqualTo(50); + } + + [Test] + public async Task Returns_nearest_agent_ancestor_when_several_match() { + // Nested agents (a hosted agent's claude under an outer claude). The nearest + // ancestor is the one whose death should end THIS watcher's session. + // start(50) -> claude(50) -> sh(40) -> claude(30) + var lookup = ProcTable.Of((50, 40, "claude"), (40, 30, "sh"), (30, 1, "claude")); + + var pid = ProcessHelpers.ResolveCodingAgentPid(startPid: 50, vendor: "claude", lookup); + + await Assert.That(pid).IsEqualTo(50); + } + + [Test] + public async Task Matches_agent_name_in_a_full_path_case_insensitively() { + // comm may be a full executable path or differently cased. + var lookup = ProcTable.Of((50, 20, "/Users/alexey/.local/bin/Claude"), (20, 1, "-zsh")); + + var pid = ProcessHelpers.ResolveCodingAgentPid(startPid: 50, vendor: "claude", lookup); + + await Assert.That(pid).IsEqualTo(50); + } + + [Test] + public async Task Returns_null_when_no_agent_in_chain() { + // No claude/codex ancestor — caller falls back to the legacy heuristic. + var lookup = ProcTable.Of((90, 20, "sh"), (20, 1, "-zsh")); + + var pid = ProcessHelpers.ResolveCodingAgentPid(startPid: 90, vendor: "claude", lookup); + + await Assert.That(pid).IsNull(); + } + + [Test] + public async Task Stops_at_init_without_matching() { + var lookup = ProcTable.Of((20, 1, "-zsh")); + + var pid = ProcessHelpers.ResolveCodingAgentPid(startPid: 20, vendor: "claude", lookup); + + await Assert.That(pid).IsNull(); + } + + [Test] + public async Task Terminates_on_a_cyclic_chain_within_maxHops() { + // Defensive: a malformed/cyclic table must not loop forever. + var lookup = ProcTable.Of((50, 60, "sh"), (60, 50, "sh")); + + var pid = ProcessHelpers.ResolveCodingAgentPid(startPid: 50, vendor: "claude", lookup, maxHops: 8); + + await Assert.That(pid).IsNull(); + } + + [Test] + public async Task Returns_null_when_start_pid_is_not_inspectable() { + var lookup = ProcTable.Of(); // every lookup returns null + + var pid = ProcessHelpers.ResolveCodingAgentPid(startPid: 50, vendor: "claude", lookup); + + await Assert.That(pid).IsNull(); + } + + [Test] + public async Task Does_not_match_unrelated_substring_processes() { + // "Claude Helper (Renderer)" is the Electron desktop app, not the CLI: a + // basename match (not a loose substring) must reject it. + var lookup = ProcTable.Of((50, 20, "Claude Helper (Renderer)"), (20, 1, "-zsh")); + + var pid = ProcessHelpers.ResolveCodingAgentPid(startPid: 50, vendor: "claude", lookup); + + await Assert.That(pid).IsNull(); + } +} diff --git a/test/Capacitor.Cli.Tests.Unit/ParentWatchdogDecisionTests.cs b/test/Capacitor.Cli.Tests.Unit/ParentWatchdogDecisionTests.cs new file mode 100644 index 0000000000..63780c86b3 --- /dev/null +++ b/test/Capacitor.Cli.Tests.Unit/ParentWatchdogDecisionTests.cs @@ -0,0 +1,35 @@ +using Capacitor.Cli.Commands; + +namespace Capacitor.Cli.Tests.Unit; + +/// +/// The watcher's parent-exit watchdog must NEVER be skipped silently. The original +/// bug was invisible precisely because, when the resolved parent PID was already +/// dead at watcher startup, the guard did nothing and logged nothing — the watcher +/// ran forever, orphaned, holding the session "active". These tests pin the explicit +/// three-way decision so the skip cases are observable. +/// +public class ParentWatchdogDecisionTests { + [Test] + public async Task Monitors_when_parent_pid_is_alive() { + var decision = WatchCommand.DecideParentWatchdog(parentPid: 4242, isAlive: _ => true); + + await Assert.That(decision).IsEqualTo(WatchCommand.ParentWatchdog.Monitor); + } + + [Test] + public async Task Reports_no_parent_pid_when_null() { + var decision = WatchCommand.DecideParentWatchdog(parentPid: null, isAlive: _ => true); + + await Assert.That(decision).IsEqualTo(WatchCommand.ParentWatchdog.NoParentPid); + } + + [Test] + public async Task Reports_parent_already_dead_at_startup() { + // The actual stuck-session failure mode: a dead PID at startup must be a + // distinct, surfaced outcome — not silently treated as "monitoring". + var decision = WatchCommand.DecideParentWatchdog(parentPid: 4242, isAlive: _ => false); + + await Assert.That(decision).IsEqualTo(WatchCommand.ParentWatchdog.ParentAlreadyDead); + } +} diff --git a/test/Capacitor.Cli.Tests.Unit/ProcessHelpersTests.cs b/test/Capacitor.Cli.Tests.Unit/ProcessHelpersTests.cs index 0bcca4a4d9..abbc1e30a3 100644 --- a/test/Capacitor.Cli.Tests.Unit/ProcessHelpersTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/ProcessHelpersTests.cs @@ -88,4 +88,47 @@ public async Task GetCodingAgentPid_does_not_return_own_pid() { await Assert.That(pid).IsNotEqualTo(Environment.ProcessId); } + + [Test] + public async Task GetCodingAgentPid_with_vendor_returns_a_live_non_self_process() { + // With no claude/codex ancestor in the test host, the vendor-aware overload + // must fall back to the legacy heuristic and still yield a live, non-self PID. + var pid = ProcessHelpers.GetCodingAgentPid("claude"); + + await Assert.That(pid).IsNotNull(); + await Assert.That(ProcessHelpers.IsProcessAlive(pid!.Value)).IsTrue(); + await Assert.That(pid).IsNotEqualTo(Environment.ProcessId); + } + + [Test] + public async Task GetProcessInfo_returns_ppid_and_name_for_current_process() { + // Backs the ancestry walk: it must report a process's real parent PID and a + // non-empty executable name so the walk can match the coding agent by name. + if (OperatingSystem.IsWindows()) { + return; // Unix-only; Windows keeps the legacy parent-PID path. + } + + var info = ProcessHelpers.GetProcessInfo(Environment.ProcessId); + + await Assert.That(info).IsNotNull(); + await Assert.That(info!.Value.ppid).IsEqualTo(ProcessHelpers.GetParentPid()!.Value); + await Assert.That(info.Value.comm).IsNotNull(); + await Assert.That(info.Value.comm.Length).IsGreaterThan(0); + } + + [Test] + public async Task GetProcessInfo_reports_parent_chain_reaching_a_live_ancestor() { + // Walking ppid from this process must reach our real parent and report it alive. + if (OperatingSystem.IsWindows()) { + return; + } + + var self = ProcessHelpers.GetProcessInfo(Environment.ProcessId); + await Assert.That(self).IsNotNull(); + + var parent = ProcessHelpers.GetProcessInfo(self!.Value.ppid); + + await Assert.That(parent).IsNotNull(); + await Assert.That(ProcessHelpers.IsProcessAlive(self.Value.ppid)).IsTrue(); + } } From 875f14d88e71b1e31b71befe13b65e4b8cda7b35 Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Sat, 13 Jun 2026 17:00:22 +0200 Subject: [PATCH 2/5] Make daemon EndAgentSession resilient to SignalR reconnects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A hosted agent's AgentSession could stay "active" if the daemon's EndAgentSession invoke hit a transient WebSocket drop — it failed fast and was only logged. Route EndAgentSessionAsync through the same ConnectionRetry helper RequestPermission uses, so it rides through a reconnect. Because the retry can block while the connection recovers, a user-initiated stop must not wait on it: HandleStopAgent now terminates the process and lets the post-exit FinalizeAgentRunAsync backstop end the session (with retry, using the "agent_stopped" reason), so teardown isn't delayed. Server-side end is idempotent, so it's a no-op if claude already fired its own session-end. Co-Authored-By: Claude Opus 4.8 --- .../Services/AgentOrchestrator.cs | 26 ++--- .../Services/ServerConnection.cs | 12 ++- .../AgentOrchestratorVendorTests.cs | 96 ++++++++++++++++++- 3 files changed, 115 insertions(+), 19 deletions(-) diff --git a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs index 74e438586a..9912590280 100644 --- a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs +++ b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs @@ -605,22 +605,16 @@ async Task HandleStopAgent(string agentId) { LogGracefulExitTimedOut(agentId, GracefulExitWait.TotalSeconds); } - // Tell the server to end the session. Idempotent server-side: if claude - // did fire session-end during the graceful window, this is a no-op - // (returns GenerateWhatsDone=false and the CLI's session-end handler - // already spawned the what's-done generator on its end). - try { - var result = await _server.EndAgentSessionAsync(agentId, agent.PendingEndReason); - - if (result is { GenerateWhatsDone: true, SessionId: not null }) { - SpawnWhatsDoneGenerator(result.SessionId); - } - } catch (Exception ex) { - LogEndSessionFailed(ex, agentId); - } - - // Cancel the read loop, then terminate the process if it didn't exit gracefully. - // The read loop's finally block will handle CleanupAgentAsync. + // Cancel the read loop and terminate the process. We deliberately do NOT end + // the AgentSession here: EndAgentSessionAsync now retries across SignalR + // reconnects (so it can block while a dropped connection recovers), and a + // user-initiated stop must not wait on that. Cancelling ReadCts unblocks the + // read loop, whose finally block runs FinalizeAgentRunAsync once the process + // exits — that ends the session (with retry) using agent.PendingEndReason + // ("agent_stopped") and spawns the what's-done generator if the server asks. + // So session-end is reliable as the post-exit backstop without delaying + // teardown, and is idempotent: if claude already fired its own session-end + // during the graceful window above, the backstop call is a server-side no-op. await agent.ReadCts.CancelAsync(); await agent.Process.TerminateAsync(TimeSpan.FromSeconds(10)); } catch (Exception ex) { diff --git a/src/Capacitor.Cli.Daemon/Services/ServerConnection.cs b/src/Capacitor.Cli.Daemon/Services/ServerConnection.cs index b7275ecc5b..7553c70e89 100644 --- a/src/Capacitor.Cli.Daemon/Services/ServerConnection.cs +++ b/src/Capacitor.Cli.Daemon/Services/ServerConnection.cs @@ -20,6 +20,7 @@ internal partial class ServerConnection : IAsyncDisposable, IDaemonHeartbeatPort readonly RegistrationGate _gate = new(); static readonly TimeSpan PermissionRetryPollInterval = TimeSpan.FromMilliseconds(500); + static readonly TimeSpan EndSessionRetryPollInterval = TimeSpan.FromMilliseconds(500); // Events for incoming commands from server public event Func? OnLaunchAgent; @@ -439,7 +440,13 @@ public virtual Task LaunchFailedAsync(string agentId, string reason) /// behaviour of the CLI session-end handler for the local-claude case. /// public virtual Task EndAgentSessionAsync(string agentId, string reason) - => _hub.InvokeAsync("EndAgentSession", agentId, reason, cancellationToken: _ct); + => ConnectionRetry.InvokeWithConnectionRetryAsync( + () => _hub.InvokeAsync("EndAgentSession", agentId, reason, cancellationToken: _ct), + () => IsReady, + EndSessionRetryPollInterval, + attempt => LogEndSessionRetry(agentId, attempt), + _ct + ); /// /// Forwards a hosted-agent permission request to the server's RequestPermission @@ -646,6 +653,9 @@ public async ValueTask DisposeAsync() { [LoggerMessage(Level = LogLevel.Information, Message = "RequestPermission for session {SessionId} interrupted by a connection drop (retry {Attempt}); waiting for the daemon connection to recover before retrying")] partial void LogPermissionRetry(string sessionId, int attempt); + [LoggerMessage(Level = LogLevel.Information, Message = "EndAgentSession for agent {AgentId} interrupted by a connection drop (retry {Attempt}); waiting for the daemon connection to recover before retrying")] + partial void LogEndSessionRetry(string agentId, int attempt); + [LoggerMessage(Level = LogLevel.Warning, Message = "Failed to post agent run event, retrying in {Delay}s")] partial void LogEventPostFailed(Exception ex, double delay); diff --git a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorVendorTests.cs b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorVendorTests.cs index 6d6d656c4e..60a23e376b 100644 --- a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorVendorTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorVendorTests.cs @@ -329,8 +329,84 @@ await orch.HandleLaunchAgentForTest(new LaunchAgentCommand( } } + [Test] + public async Task Stopping_an_agent_terminates_promptly_even_when_end_session_is_blocked() { + // Option B: EndAgentSession is the post-exit backstop and retries across SignalR + // reconnects, so it can block while the connection recovers. A user-initiated stop + // must NOT wait on it — HandleStopAgent terminates the process, and the read-loop's + // finalize backstop ends the session afterwards. With EndAgentSession blocked, + // termination must still happen promptly (before the fix HandleStopAgent awaited + // its own EndAgentSession call and never reached TerminateAsync). + var (repoPath, cleanup) = CreateGitRepo(); + using var endSessionBlock = new CancellationTokenSource(); + + try { + var terminated = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var server = new CaptureServerConnection { EndSessionBlockUntil = endSessionBlock }; + var ptyFactory = new FixedPtyProcessFactory(new TerminateSignalingPtyProcess(terminated)); + var launchers = new Dictionary { ["claude"] = new SpyHostedAgentLauncher("claude", cliPath: "spy-claude") }; + + await using var orch = BuildOrchestrator(server, ptyFactory, launchers, allowedRepoPath: repoPath); + + await orch.HandleLaunchAgentForTest(new LaunchAgentCommand( + AgentId: "agent-stop", + Prompt: "go", + Model: "opus", + Effort: null, + RepoPath: repoPath, + Tools: null, + AttachmentIds: null, + Vendor: "claude" + )); + + // Fire-and-forget: before the fix, HandleStopAgent awaits the blocked + // EndAgentSession and never reaches termination, so we must not await it here. + _ = orch.HandleStopAgentForTest("agent-stop"); + + // The process must be terminated even though EndAgentSession is still blocked. + await terminated.Task.WaitAsync(TimeSpan.FromSeconds(5)); + } finally { + endSessionBlock.Cancel(); // release the finalize backstop's blocked end-session + cleanup(); + } + } + // ── Test doubles ───────────────────────────────────────────────────── + /// + /// PTY that reports already-exited (so HandleStopAgent's graceful window is instant) + /// and signals when TerminateAsync runs. ReadOutputAsync blocks until ReadCts cancels, + /// keeping the read loop alive until the stop. + /// + sealed class TerminateSignalingPtyProcess(TaskCompletionSource terminated) : IPtyProcess { + public int Pid => 4243; + public bool HasExited => true; + public int? ExitCode => 0; + + public ValueTask DisposeAsync() => default; + public Task WaitForExitAsync(TimeSpan? _) => Task.CompletedTask; + + public Task TerminateAsync(TimeSpan? _) { + terminated.TrySetResult(); + + return Task.CompletedTask; + } + + public async IAsyncEnumerable ReadOutputAsync([EnumeratorCancellation] CancellationToken ct = default) { + try { await Task.Delay(Timeout.InfiniteTimeSpan, ct); } catch (OperationCanceledException) { + /* released on stop */ + } + + yield break; + } + + public Task WriteAsync(string _) => Task.CompletedTask; + public Task WriteAsync(byte[] _) => Task.CompletedTask; + public void Resize(ushort _, ushort __) { } + public void SendInterrupt() { } + } + sealed class SpyHostedAgentLauncher(string vendor, string cliPath) : IHostedAgentLauncher { public string Vendor { get; } = vendor; public string CliPath { get; } = cliPath; @@ -455,6 +531,13 @@ sealed class CaptureServerConnection() : ServerConnection( ) { public List<(string AgentId, string Reason)> LaunchFailedCalls { get; } = []; + /// When set, EndAgentSessionAsync blocks until this token is cancelled, + /// simulating a session-end call stuck waiting for a SignalR reconnect. + public CancellationTokenSource? EndSessionBlockUntil { get; init; } + + /// Reasons passed to EndAgentSessionAsync, in call order. + public List EndSessionReasons { get; } = []; + public override Task LaunchFailedAsync(string agentId, string reason) { LaunchFailedCalls.Add((agentId, reason)); @@ -496,8 +579,17 @@ public override async Task SendTerminalOutputAsync(string agentId, string base64 public override Task AppendAgentRunEventAsync(string agentId, object evt) => Task.CompletedTask; - public override Task EndAgentSessionAsync(string agentId, string reason) - => Task.FromResult(new EndAgentSessionResult()); + public override async Task EndAgentSessionAsync(string agentId, string reason) { + EndSessionReasons.Add(reason); + + if (EndSessionBlockUntil is { } cts) { + try { await Task.Delay(Timeout.InfiniteTimeSpan, cts.Token); } catch (OperationCanceledException) { + /* released by the test */ + } + } + + return new EndAgentSessionResult(); + } public override Task RequestPermissionAsync( string sessionId, From 47a5a6145f173561c51ddbf93d990178342bd456 Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Sat, 13 Jun 2026 17:00:22 +0200 Subject: [PATCH 3/5] Add spec for server-side reconciliation of stuck hosted sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The client-side fixes (watcher watchdog, daemon EndAgentSession) can't cover a daemon/host that dies hard — by definition the cleanup code is dead. Document the server-side backstop (daemon-disconnect and watcher-disconnect reconcile with a grace window) for the kcap-server repo, plus the open question of why a replacement daemon masked the old one's disconnect in the observed incident. Co-Authored-By: Claude Opus 4.8 --- ...13-hosted-session-server-reconcile-spec.md | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 docs/superpowers/specs/2026-06-13-hosted-session-server-reconcile-spec.md diff --git a/docs/superpowers/specs/2026-06-13-hosted-session-server-reconcile-spec.md b/docs/superpowers/specs/2026-06-13-hosted-session-server-reconcile-spec.md new file mode 100644 index 0000000000..3f3fc75ae0 --- /dev/null +++ b/docs/superpowers/specs/2026-06-13-hosted-session-server-reconcile-spec.md @@ -0,0 +1,99 @@ +# Server-side reconciliation of stuck hosted/recorded sessions + +**Date:** 2026-06-13 +**Status:** Spec (for the kcap-**server** repo — cannot be implemented from kcap-cli) +**Related:** client-side fixes in this repo (watcher watchdog, daemon `EndAgentSession`); design `2026-06-13-permission-bridge-reconnect-retry-design.md` + +## Problem (observed incident) + +Session `9687912eed69420d9a21ba1c66b7698d` (a daemon-hosted agent) showed as **active +forever** on the server after the hosted `claude` process had already died. Forensics: + +- The hosted `claude` PID was dead. +- Its two `kcap watch` sidecar processes were **orphaned but still alive**, holding live + SignalR connections to the server (`…:443 ESTABLISHED`), 1h+ after the agent died. +- The hosting daemon had been replaced by a new daemon instance that had no knowledge of + the session, so it never sent `EndAgentSession`. + +So "active" was being held open by (a) orphaned watcher connections that never sent +session-end, and (b) a missing `EndAgentSession` for the agent-run. + +## Why the client side alone can't guarantee this + +There are three independent producers of "this session is active" and three matching +failure modes. The client-side fixes (this repo) close two of them, but a daemon/agent +that dies **hard** (SIGKILL, crash, host reboot, daemon replacement) can leave residue +the client can never clean up — by definition the cleanup code is dead. The server is +the only component guaranteed to be alive to reconcile. Defense in depth requires a +server-side backstop. + +Client-side fixes already in flight (for reference, not server work): + +1. **Watcher watchdog** — `kcap watch` now reliably resolves the durable coding-agent PID + and self-terminates + POSTs `session-end` when the agent dies (previously the watchdog + silently never started for Claude, orphaning the watcher). +2. **Daemon `EndAgentSession`** — daemon attempts to end the hosted AgentSession on stop / + agent-exit / shutdown. + +Neither survives a hard daemon/host death. Hence this spec. + +## Goal + +The server must converge a session/agent-run to a terminal state when its producers are +demonstrably gone, without waiting on a client message that may never arrive. + +## Proposed reconciliation (server) + +Two triggers, each with a short grace period to tolerate transient reconnects (the +cloudflared-instability pattern — connections drop and re-establish within seconds): + +### A. Daemon disconnect → end that daemon's in-flight agent-runs + +When a daemon's hub connection drops and is **not** re-established (by the same daemon +identity / `(owner, name)` slot) within a grace window (suggest **60–90s**, comfortably +above the daemon's auto-reconnect cadence which retries at ≤30s): + +- Mark every `AgentRun`/`AgentSession` that daemon owned and that is still `active` as + ended, reason `daemon_disconnected` (distinct from `agent_stopped`/`agent_exited` so it's + attributable in the read model). +- This is the backstop the daemon code already *assumes* exists — see the comments in + `AgentOrchestrator.cs` (`ReadAgentOutputAsync` finally, `CleanupAgentAsync`, + `DisposeAsync`) which skip `EndAgentSession` on shutdown "because the server detects the + daemon disconnection and ends its sessions on its own." **This assumption must be + verified; the incident suggests it is not happening (or not for replaced daemons).** + +### B. Watcher disconnect → end sessions with no remaining live producer + +When the last `kcap watch` connection for a session drops and is not re-established within +a grace window (suggest **60–90s**), and there is no other live producer (no connected +daemon hosting it, no active local-CLI watcher): + +- Mark the session ended, reason `watcher_disconnected`. +- Optionally trigger the same post-session work the normal `session-end` path does + (what's-done generation) if not already done — idempotently. + +### Idempotency + +Both triggers must be **idempotent** with the existing `SessionEnded` / `EndAgentSession` +paths: if the client later reconnects and sends session-end (or the daemon's +`EndAgentSession` lands), it must be a no-op, not a duplicate end or a resurrection. + +## Open questions for the server team + +1. **Does the server already react to daemon hub disconnect at all today?** The daemon code + asserts it does. If yes: why did the replaced-daemon case (incident) leave the session + active — is reconciliation keyed on connection-id rather than daemon identity, so a + *replacement* daemon connecting masks the old one's disconnect without ending its runs? +2. **Is "active session" derived from a live watcher connection, from `SessionEnded` + absence, or both?** Determines whether trigger B is needed or whether watcher-connection + teardown already clears it. +3. **Grace-period source of truth** — align with the daemon heartbeat (7s tick / 5s ping + deadline) and auto-reconnect (≤30s) so the window can't fire during a normal blip. + +## Acceptance + +- Kill a hosted agent's `claude` and its watchers without any client-side session-end → + session converges to ended within the grace window, attributed `daemon_disconnected` / + `watcher_disconnected`. +- A transient reconnect within the grace window does **not** end the session. +- Re-delivery of a client session-end afterwards is a no-op. From 21d4890fd5b4019bc0c8349e2bc56a3281a48f49 Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Sat, 13 Jun 2026 17:08:14 +0200 Subject: [PATCH 4/5] Cover Copilot (and any vendor) in watcher parent-PID resolution The ancestry-walk resolver was gated to claude/codex, but CopilotHookCommand also spawns `kcap watch --vendor copilot`, leaving Copilot on the legacy getpgrp() heuristic. Broaden the gate to any non-empty vendor: the walk matches by the vendor's process name and falls back to the legacy heuristic when no match is found, so it can't regress a vendor whose process isn't named after it. Co-Authored-By: Claude Opus 4.8 --- src/Capacitor.Cli/ProcessHelpers.cs | 24 +++++++++++-------- .../CodingAgentPidResolverTests.cs | 13 ++++++++++ 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/Capacitor.Cli/ProcessHelpers.cs b/src/Capacitor.Cli/ProcessHelpers.cs index 29ff0f0f4d..b26f60f4e4 100644 --- a/src/Capacitor.Cli/ProcessHelpers.cs +++ b/src/Capacitor.Cli/ProcessHelpers.cs @@ -226,22 +226,26 @@ internal static bool TryClearInheritFlag(nint handle) { /// /// Resolves the PID of the long-lived coding-agent process for - /// ("claude"/"codex"), suitable for the watcher's - /// parent-PID watchdog. On Unix it first walks the ppid ancestry looking for the - /// agent by name ( + ), - /// which is robust to whatever process-group/launcher topology the agent uses to - /// spawn its hook — notably Claude, whose hook runs in a separate transient - /// process group that makes the bare getpgrp() heuristic resolve an - /// already-dead PID. It falls back to that legacy heuristic when no agent is found - /// on the chain (or is unknown), preserving prior - /// behaviour for Codex. + /// (claude/codex/copilot, or a future one), suitable for + /// the watcher's parent-PID watchdog. On Unix it first walks the ppid ancestry + /// looking for the agent by name ( + + /// ), which is robust to whatever process-group/launcher + /// topology the agent uses to spawn its hook — notably Claude, whose hook runs in a + /// separate transient process group that makes the bare getpgrp() heuristic + /// resolve an already-dead PID. It falls back to that legacy heuristic when no agent + /// is found on the chain (or is null/empty), preserving + /// prior behaviour for Codex and any vendor whose process isn't named after it. /// public static int? GetCodingAgentPid(string? vendor) { if (OperatingSystem.IsWindows()) { return GetParentPidWindows(); } - if (vendor is "claude" or "codex" + // Walk the ancestry for any named vendor. The match is by the vendor's process + // name and falls back to the legacy heuristic below when no agent is found, so + // this can't regress a vendor whose process isn't named after it (e.g. an npm + // shim) — it only adds coverage where the name matches. + if (!string.IsNullOrEmpty(vendor) && ResolveCodingAgentPid(getppid_native(), vendor, GetProcessInfo) is { } agentPid) { return agentPid; } diff --git a/test/Capacitor.Cli.Tests.Unit/CodingAgentPidResolverTests.cs b/test/Capacitor.Cli.Tests.Unit/CodingAgentPidResolverTests.cs index e5422180a2..577d81ae90 100644 --- a/test/Capacitor.Cli.Tests.Unit/CodingAgentPidResolverTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/CodingAgentPidResolverTests.cs @@ -58,6 +58,19 @@ public async Task Resolves_codex_by_name() { await Assert.That(pid).IsEqualTo(50); } + [Test] + public async Task Resolves_copilot_skipping_a_transient_launcher() { + // Copilot is the third watcher vendor (CopilotHookCommand spawns kcap watch + // --vendor copilot). GetCodingAgentPid now walks the ancestry for it too, so the + // resolver must match "copilot" by name like the others. + // hook(100) -> sh(90) -> copilot(50) -> zsh(20) + var lookup = ProcTable.Of((90, 50, "sh"), (50, 20, "copilot"), (20, 1, "-zsh")); + + var pid = ProcessHelpers.ResolveCodingAgentPid(startPid: 90, vendor: "copilot", lookup); + + await Assert.That(pid).IsEqualTo(50); + } + [Test] public async Task Returns_nearest_agent_ancestor_when_several_match() { // Nested agents (a hosted agent's claude under an outer claude). The nearest From 843ec316b8f83a81d0e8b20ba7e6b1900f77970a Mon Sep 17 00:00:00 2001 From: Alexey Zimarev Date: Sat, 13 Jun 2026 17:55:34 +0200 Subject: [PATCH 5/5] Don't block agent cleanup on the reconnect-retrying EndAgentSession Qodo review: FinalizeAgentRunAsync awaited EndAgentSessionAsync (now unbounded across SignalR reconnects) before CleanupAgentAsync, so a prolonged outage could stall worktree/process cleanup and pin the agent in _agents until daemon shutdown. Bound the wait to EndAgentSessionBudget (30s); on timeout, proceed to cleanup while the retry continues in the background (observed so it isn't an unobserved task exception). Cleanup now runs unconditionally. A genuinely long outage falls back to server-side daemon-disconnect reconcile (see the reconcile spec). Co-Authored-By: Claude Opus 4.8 --- .../Services/AgentOrchestrator.cs | 52 +++++++++++-- .../AgentOrchestratorVendorTests.cs | 77 ++++++++++++++++++- 2 files changed, 120 insertions(+), 9 deletions(-) diff --git a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs index 9912590280..769170a8d6 100644 --- a/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs +++ b/src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs @@ -91,6 +91,14 @@ internal partial class AgentOrchestrator : IAsyncDisposable { static readonly TimeSpan PingDeadline = TimeSpan.FromSeconds(5); + /// + /// How long waits on the (reconnect-retrying) + /// EndAgentSession call before proceeding with local cleanup regardless. Covers a + /// typical transient SignalR blip with margin; a longer outage proceeds to cleanup + /// while the retry continues in the background. Settable so tests don't wait 30s. + /// + internal TimeSpan EndAgentSessionBudget { get; set; } = TimeSpan.FromSeconds(30); + // Linked to IHostApplicationLifetime.ApplicationStopping so the shutdown gate // trips as soon as the host begins stopping — the same instant ServerConnection's // _ct (also ApplicationStopping) cancels SignalR calls. Otherwise there's a @@ -526,12 +534,19 @@ async Task FinalizeAgentRunAsync(AgentInstance agent) { // its own session-end hook on SIGTERM/exit, so without this call the // session would stay "active" forever in the read model. Server-side is // idempotent — if claude did fire session-end first, this is a no-op. - // Reason is read from agent.PendingEndReason so that if HandleStopAgent's - // own call failed (transient SignalR error) and this finally-block call - // is the one that lands, a user-initiated stop is still recorded as - // "agent_stopped" rather than "agent_exited". + // Reason is read from agent.PendingEndReason so a user-initiated stop is + // recorded as "agent_stopped" rather than "agent_exited". + // + // EndAgentSessionAsync retries across SignalR reconnects, so it can block + // for the length of an outage. We must NOT let that stall local cleanup + // (worktree/process disposal, removing the agent from _agents), so bound how + // long we WAIT on it to EndAgentSessionBudget. The retry keeps running in the + // background — a reconnect shortly after still lands the session-end — and a + // genuinely long outage falls back to server-side daemon-disconnect reconcile. + var endTask = _server.EndAgentSessionAsync(agent.Id, agent.PendingEndReason); + try { - var result = await _server.EndAgentSessionAsync(agent.Id, agent.PendingEndReason); + var result = await endTask.WaitAsync(EndAgentSessionBudget, _shutdownCts.Token); // The daemon doesn't track sessionId on its own (only agentId), so // the server returns it in the result. Spawn what's-done locally @@ -539,14 +554,21 @@ async Task FinalizeAgentRunAsync(AgentInstance agent) { if (result is { GenerateWhatsDone: true, SessionId: not null }) { SpawnWhatsDoneGenerator(result.SessionId); } + } catch (TimeoutException) { + // Outage outlasted the budget. Don't block cleanup; the retry continues + // in the background (observed below so a later fault isn't unobserved). + LogEndSessionTimedOut(agent.Id, EndAgentSessionBudget.TotalSeconds); + ObserveEndSessionInBackground(endTask, agent.Id); } catch (OperationCanceledException) when (_shutdownCts.IsCancellationRequested) { - // Shutdown fired mid-call — connection is being torn down. Server + // Shutdown fired mid-wait — connection is being torn down. Server // detects daemon disconnection independently. No warning needed. } catch (Exception ex) { LogEndSessionFailed(ex, agent.Id); } - // Clean up worktree and unregister from server + // Clean up worktree and unregister from server. Runs unconditionally — even + // when end-session timed out and is still retrying in the background — so a + // prolonged outage can never pin the agent in _agents or leak its worktree. await CleanupAgentAsync(agent.Id); } catch (Exception ex) { LogCleanupError(ex, agent.Id); @@ -622,6 +644,19 @@ async Task HandleStopAgent(string agentId) { } } + /// + /// Observes a background EndAgentSession retry that outlived the finalize budget so a + /// later fault isn't an unobserved task exception. Success and shutdown-cancellation + /// are intentionally ignored; only a genuine fault is logged. + /// + void ObserveEndSessionInBackground(Task endTask, string agentId) => + _ = endTask.ContinueWith( + t => LogEndSessionFailed(t.Exception!.GetBaseException(), agentId), + CancellationToken.None, + TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default + ); + /// /// Spawns kcap generate-whats-done {sessionId} as a detached process. /// Used when the daemon-driven session-end path supplants claude's own session-end @@ -1012,6 +1047,9 @@ static string ExtractTerminalText(TerminalOutputBuffer buffer) { [LoggerMessage(Level = LogLevel.Warning, Message = "Failed to end session for agent {AgentId} (server may not record SessionEnded)")] partial void LogEndSessionFailed(Exception ex, string agentId); + [LoggerMessage(Level = LogLevel.Warning, Message = "EndAgentSession for agent {AgentId} did not complete within {Seconds}s; proceeding with cleanup while the retry continues in the background (server reconciles on daemon disconnect)")] + partial void LogEndSessionTimedOut(string agentId, double seconds); + [LoggerMessage(Level = LogLevel.Information, Message = "Spawned what's-done generator for session {SessionId} (PID {Pid})")] partial void LogWhatsDoneSpawned(string sessionId, int pid); diff --git a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorVendorTests.cs b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorVendorTests.cs index 60a23e376b..a2c9d19364 100644 --- a/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorVendorTests.cs +++ b/test/Capacitor.Cli.Tests.Unit/AgentOrchestratorVendorTests.cs @@ -372,8 +372,74 @@ await orch.HandleLaunchAgentForTest(new LaunchAgentCommand( } } + [Test] + public async Task Cleanup_runs_even_when_end_session_never_recovers() { + // Qodo: EndAgentSession now retries across reconnects, so it can block for a whole + // outage. FinalizeAgentRunAsync must not stall local cleanup on it — it waits only + // up to EndAgentSessionBudget, then proceeds to CleanupAgentAsync (which unregisters + // the agent) while the retry continues in the background. Here end-session never + // recovers, yet cleanup must still run. + var (repoPath, cleanup) = CreateGitRepo(); + using var neverRecovers = new CancellationTokenSource(); + + try { + var unregistered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var server = new CaptureServerConnection { + EndSessionBlockUntil = neverRecovers, // end-session blocks for the whole test + OnAgentUnregistered = () => unregistered.TrySetResult() // fires when cleanup completes + }; + var ptyFactory = new FixedPtyProcessFactory(new ImmediateExitPtyProcess()); + var launchers = new Dictionary { ["claude"] = new SpyHostedAgentLauncher("claude", cliPath: "spy-claude") }; + + await using var orch = BuildOrchestrator(server, ptyFactory, launchers, allowedRepoPath: repoPath); + orch.EndAgentSessionBudget = TimeSpan.FromMilliseconds(250); // don't wait the real 30s in a test + + await orch.HandleLaunchAgentForTest(new LaunchAgentCommand( + AgentId: "agent-x", + Prompt: "go", + Model: "opus", + Effort: null, + RepoPath: repoPath, + Tools: null, + AttachmentIds: null, + Vendor: "claude" + )); + + // The PTY exits immediately → the read loop ends → FinalizeAgentRunAsync runs. + // End-session blocks (never recovers), but cleanup must still run after the budget. + await unregistered.Task.WaitAsync(TimeSpan.FromSeconds(5)); + } finally { + neverRecovers.Cancel(); // release the background end-session task + cleanup(); + } + } + // ── Test doubles ───────────────────────────────────────────────────── + /// PTY that has already exited and produces no output, so the read loop ends + /// immediately and FinalizeAgentRunAsync runs right after launch. + sealed class ImmediateExitPtyProcess : IPtyProcess { + public int Pid => 4244; + public bool HasExited => true; + public int? ExitCode => 0; + + public ValueTask DisposeAsync() => default; + public Task WaitForExitAsync(TimeSpan? _) => Task.CompletedTask; + public Task TerminateAsync(TimeSpan? _) => Task.CompletedTask; + +#pragma warning disable CS1998 + public async IAsyncEnumerable ReadOutputAsync([EnumeratorCancellation] CancellationToken _ = default) { + yield break; + } +#pragma warning restore CS1998 + + public Task WriteAsync(string _) => Task.CompletedTask; + public Task WriteAsync(byte[] _) => Task.CompletedTask; + public void Resize(ushort _, ushort __) { } + public void SendInterrupt() { } + } + /// /// PTY that reports already-exited (so HandleStopAgent's graceful window is instant) /// and signals when TerminateAsync runs. ReadOutputAsync blocks until ReadCts cancels, @@ -550,8 +616,15 @@ public override Task AgentRegisteredAsync(string agentId, string? prompt, string public override Task AgentStatusChangedAsync(string agentId, string status, string? sessionId) => Task.CompletedTask; - public override Task AgentUnregisteredAsync(string agentId) - => Task.CompletedTask; + /// Invoked when AgentUnregisteredAsync runs — the last step of + /// CleanupAgentAsync, so a useful signal that local cleanup completed. + public Action? OnAgentUnregistered { get; init; } + + public override Task AgentUnregisteredAsync(string agentId) { + OnAgentUnregistered?.Invoke(); + + return Task.CompletedTask; + } public override Task UpdateRepoPathsAsync() => Task.CompletedTask;