Skip to content

[AI-647] Close orphaned session when watcher's parent coding agent exits - #75

Merged
alexeyzimarev merged 2 commits into
mainfrom
alexeyzimarev/ai-647-close-orphaned-sessions-when-watchers-parent-coding-agent
May 16, 2026
Merged

alexeyzimarev merged 2 commits into
mainfrom
alexeyzimarev/ai-647-close-orphaned-sessions-when-watchers-parent-coding-agent

Conversation

@alexeyzimarev

Copy link
Copy Markdown
Member

Summary

  • When the watcher's parent-PID monitor detects the parent coding agent died without firing SessionEnd, it now POSTs /hooks/session-end/{vendor} with reason: "parent_exited", reads back generate_whats_done, and spawns the what's-done generator when needed.
  • Only fires for session watchers (agentId is null) that crossed the transcript threshold; subagent watchers and trivial sessions are skipped.
  • Disposes the SignalR connection before posting so the server's StopAndDrainAsync skips its 10s drain wait.

Context

Closes the local-watcher equivalent of the gap the daemon already handles for hosted agents via AgentOrchestrator.EndAgentSessionAsync. Without this, users who close their terminal mid-session see the session stuck "Active" in the UI even though the agent is clearly gone.

Paired with kurrent-io/Kurrent.Capacitor#628 — the server change adds SessionEndReason.ParentExited plus idempotency guards on HandleSessionEnd that protect against the duplicate-POST race when this fallback fires alongside a successful claude session-end. Merge server PR first.

Linear: AI-647

Test plan

  • WatcherParentExitPostTests (WireMock) — 3 new tests: payload shape, optional repository, vendor-specific route (claude vs codex).
  • Full CLI integration suite — 12/12 pass.
  • Full CLI unit suite — 750/750 pass.
  • dotnet publish -c Release — clean, no IL3050/IL2026 AOT warnings.

When a user closes the terminal hosting their coding agent (or the agent
crashes / is force-killed) without firing the SessionEnd hook, the
session stays "Active" forever in the UI. The watcher already detects
this via its parent-PID monitor and self-terminates cleanly, but until
now it never told the server "the session is over", so no SessionEnded
event was written.

This change has the watcher take over the role of the missing
session-end hook: after the existing drain + WatcherDrainComplete +
SignalR dispose, POST /hooks/session-end/{vendor} with
reason="parent_exited", read back the generate_whats_done flag, and
spawn the what's-done generator when the server says so. SignalR is
disposed first so the server's StopAndDrainAsync skips its 10s drain
wait (no live watcher connection to signal).

Only fires for session watchers (agentId is null) that have crossed the
transcript threshold — short-lived sessions are left to the server's
existing trivial-session cleanup, and subagent watchers don't own a
session.

Mirrors the daemon's existing EndSessionForAgentAsync fallback for
hosted agents. Server-side idempotency (added in the paired
kapacitor-server PR) protects against the duplicate-POST race where
this fallback fires alongside a successful claude session-end hook.
@linear-code

linear-code Bot commented May 16, 2026

Copy link
Copy Markdown

AI-647

@qodo-code-review

Copy link
Copy Markdown

Review Summary by Qodo

Close orphaned sessions when parent coding agent exits

✨ Enhancement 🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Watcher now POSTs /hooks/session-end/{vendor} when parent coding agent exits unexpectedly
• Prevents orphaned sessions from staying "Active" in UI indefinitely
• Reads generate_whats_done flag and spawns generator when needed
• Only fires for session watchers above transcript threshold; skips subagents
Diagram
flowchart LR
  A["Parent Process Dies"] -->|Detected by PID Monitor| B["Set parentExited Flag"]
  B -->|After SignalR Dispose| C["POST /hooks/session-end/{vendor}"]
  C -->|reason: parent_exited| D["Server Writes SessionEnded"]
  D -->|Reads generate_whats_done| E["Spawn Generator if Needed"]
  E -->|Session Closed| F["UI Updates Session Status"]
Loading

Grey Divider

File Changes

1. src/kapacitor/Commands/WatchCommand.cs ✨ Enhancement +75/-0

Implement parent-exit session-end fallback mechanism

• Added parentExited flag to track when parent process dies without firing session-end
• Set flag to true when parent PID monitor detects process exit
• Implemented PostSessionEndOnParentExitAsync method to POST session-end hook with `reason:
 "parent_exited"`
• Reads generate_whats_done from response and spawns what's-done generator when needed
• Only executes for session watchers (agentId is null) that crossed transcript threshold

src/kapacitor/Commands/WatchCommand.cs


2. test/kapacitor.Tests.Integration/WatcherParentExitPostTests.cs 🧪 Tests +112/-0

Add parent-exit session-end POST integration tests

• Added three new WireMock-based integration tests for parent-exit session-end POST
• Tests validate payload shape with session_id, transcript_path, cwd, hook_event_name, and reason
 fields
• Tests verify optional repository field inclusion when provided
• Tests confirm vendor-specific routing (claude vs codex endpoints)

test/kapacitor.Tests.Integration/WatcherParentExitPostTests.cs


Grey Divider

Qodo Logo

@qodo-code-review

qodo-code-review Bot commented May 16, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Shutdown POST can hang ✓ Resolved 🐞 Bug ☼ Reliability
Description
PostSessionEndOnParentExitAsync performs auth discovery and a retrying POST without any bounded
CancellationToken/timeout, so watcher shutdown can stall for the HttpClient default on /auth/config
(notably ~100s) plus retry time when the server is unreachable or stalls. This can keep the watcher
process alive long after the parent has exited, and still fail to end the session.
Code

src/kapacitor/Commands/WatchCommand.cs[R287-292]

+            using var httpClient = await HttpClientExtensions.CreateAuthenticatedClientAsync(baseUrl);
+            using var content    = new StringContent(endHook.ToJsonString(), Encoding.UTF8, "application/json");
+
+            var url = $"{baseUrl}/hooks/session-end/{vendor}";
+            using var response = await httpClient.PostWithRetryAsync(url, content);
+
Evidence
The new shutdown hook creates an authenticated client and posts with retry but never supplies a
cancellation token, while the codebase elsewhere explicitly warns that unbounded /auth/config
discovery can consume the full default HttpClient timeout (~100s).

src/kapacitor/Commands/WatchCommand.cs[271-292]
src/Kapacitor.Core/HttpClientExtensions.cs[17-22]
src/Kapacitor.Core/HttpClientExtensions.cs[46-60]
src/kapacitor/Commands/CodexHookCommand.cs[175-183]

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

### Issue description
`PostSessionEndOnParentExitAsync` calls `CreateAuthenticatedClientAsync(baseUrl)` and then `PostWithRetryAsync(...)` without any explicit deadline/cancellation. If the server accepts TCP but stalls on `/auth/config`, `DiscoverProviderAsync` can take up to the default HttpClient timeout (called out elsewhere as ~100s), delaying shutdown and preventing timely session end.

### Issue Context
This code runs on the shutdown path after the watcher has decided to exit. The intent is best-effort cleanup; it should *not* block process termination for long periods.

### Fix Focus Areas
- src/kapacitor/Commands/WatchCommand.cs[264-312]
- src/Kapacitor.Core/HttpClientExtensions.cs[17-77]
- src/kapacitor/Commands/CodexHookCommand.cs[175-187]

### Suggested change
- Create a short `CancellationTokenSource` (e.g., 5–10s) inside `PostSessionEndOnParentExitAsync`.
- Pass that token into `CreateAuthenticatedClientAsync(baseUrl, ct)` so `/auth/config` discovery is bounded.
- Pass both `ct` and an explicit `timeout:` into `PostWithRetryAsync` so retries also respect the deadline.
- Treat `OperationCanceledException` as best-effort failure (log + return).

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



Remediation recommended

2. Unvalidated vendor in URL ✓ Resolved 🐞 Bug ⛨ Security
Description
The watcher interpolates vendor directly into the request path, and --vendor is accepted without
validation, so unexpected values containing path separators (e.g., /, ..) can change the request
target and hit unintended endpoints. This can lead to misrouting at best and server-side side
effects at worst.
Code

src/kapacitor/Commands/WatchCommand.cs[R290-291]

+            var url = $"{baseUrl}/hooks/session-end/{vendor}";
+            using var response = await httpClient.PostWithRetryAsync(url, content);
Evidence
The CLI accepts --vendor without validation and the new code builds a URL by directly appending
that string as a path segment.

src/kapacitor/Program.cs[455-490]
src/kapacitor/Commands/WatchCommand.cs[287-292]

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

### Issue description
`vendor` is concatenated into the URL path without validation/escaping. Since `vendor` comes from a CLI argument, malformed values can alter the request path.

### Issue Context
`kapacitor watch ... --vendor <value>` is parsed verbatim and passed into `RunWatch`, and the new parent-exit POST uses it in `/hooks/session-end/{vendor}`.

### Fix Focus Areas
- src/kapacitor/Commands/WatchCommand.cs[264-292]
- src/kapacitor/Program.cs[455-490]

### Suggested change
- Restrict `vendor` to known values (e.g., `"claude"` and `"codex"`) and/or reject values containing `/`, `\\`, or `..`.
- When building the URL, use `Uri.EscapeDataString(vendor)` for the path segment (after validation).
- Consider normalizing to lowercase (`ToLowerInvariant()`) before validation.

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


3. Claude route compatibility risk 🐞 Bug ≡ Correctness
Description
The new fallback always posts to /hooks/session-end/{vendor}, but other CLI paths still use
/hooks/session-end for Claude, so this can fail (404) against servers that only implement the
legacy Claude route and leave sessions stuck active. This is especially risky because this code runs
specifically when the normal session-end path did not occur.
Code

src/kapacitor/Commands/WatchCommand.cs[R290-292]

+            var url = $"{baseUrl}/hooks/session-end/{vendor}";
+            using var response = await httpClient.PostWithRetryAsync(url, content);
+
Evidence
Within this repo, the existing session-end POST for Claude uses the non-vendor route, and only Codex
is explicitly vendor-routed, while the new watcher fallback introduces a Claude vendor route.

src/kapacitor/Commands/HistoryCommand.cs[1364-1378]
src/kapacitor/Commands/CodexHookCommand.cs[13-16]
src/kapacitor/Commands/WatchCommand.cs[287-292]

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 watcher’s parent-exit cleanup uses `/hooks/session-end/{vendor}` even for `vendor == "claude"`, while other code posts Claude session-end to `/hooks/session-end` (no vendor suffix). If the server doesn’t support `/hooks/session-end/claude` (or during rollout mismatch), the fallback will fail and the session remains active.

### Issue Context
Codex is already vendor-routed (`/hooks/session-end/codex`), but Claude historically uses the generic route.

### Fix Focus Areas
- src/kapacitor/Commands/WatchCommand.cs[287-296]
- src/kapacitor/Commands/HistoryCommand.cs[1364-1378]
- src/kapacitor/Commands/CodexHookCommand.cs[13-16]

### Suggested change
- If `vendor == "claude"`, POST to `${baseUrl}/hooks/session-end`.
 - OR: try the vendor route first and if the response is 404, retry once against `/hooks/session-end` (best-effort).
- Keep Codex posting to `/hooks/session-end/codex` as-is.

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



Advisory comments

4. parentExited visibility race ✓ Resolved 🐞 Bug ☼ Reliability
Description
parentExited is written by a background Task and later read on the main thread without
synchronization, so the write is not formally guaranteed to be observed and the fallback POST may be
skipped. This makes the orphan-session cleanup less reliable in edge timing cases.
Code

src/kapacitor/Commands/WatchCommand.cs[R41-49]

+        // Tracks whether shutdown was triggered by the parent coding-agent process
+        // dying without firing session-end. When true, the watcher takes over the
+        // server's session-end POST (which the parent normally fires) so the read
+        // model doesn't keep the session "active" forever.
+        var parentExited = false;
+
        // 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)) {
Evidence
The variable is mutated from the background parent PID polling task and later used to guard the
session-end POST, but it is a plain local without any interlocked/volatile semantics.

src/kapacitor/Commands/WatchCommand.cs[41-66]
src/kapacitor/Commands/WatchCommand.cs[245-257]

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

### Issue description
`parentExited` is shared across threads (monitor task writes, main thread reads) without a memory barrier. While it will usually work, it’s not guaranteed by the memory model.

### Issue Context
The parent-exit monitor is `Task.Run` fire-and-forget, and the boolean is later checked to decide whether to send the session-end POST.

### Fix Focus Areas
- src/kapacitor/Commands/WatchCommand.cs[41-67]
- src/kapacitor/Commands/WatchCommand.cs[245-257]

### Suggested change
- Replace `bool parentExited` with `int parentExited = 0;`.
- Write with `Interlocked.Exchange(ref parentExited, 1);`.
- Read with `Volatile.Read(ref parentExited) == 1` (or `Interlocked.CompareExchange(ref parentExited, 0, 0) == 1`).

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


Grey Divider

Qodo Logo

Comment thread src/kapacitor/Commands/WatchCommand.cs Outdated
Three Qodo findings on #75:

- Bound the entire parent-exit POST in a 10s CancellationTokenSource so
  /auth/config discovery and PostWithRetryAsync can't stall watcher
  shutdown for ~130s when the server accepts TCP but never responds.
  Token is threaded into CreateAuthenticatedClientAsync, into
  PostWithRetryAsync (as both ct and timeout), and into the response
  body read. OperationCanceledException is treated as best-effort
  failure with a clear log line.

- Validate vendor against a known whitelist (claude, codex) before
  interpolating into the URL path. Defence-in-depth against malformed
  --vendor values (e.g. "../admin") producing path traversal — though
  the CLI runs locally and the user would already have code execution,
  a verbatim path-segment concat is still a smell.

- Promote parentExited from a bool local to an int + Interlocked /
  Volatile so the C# memory model formally guarantees the
  background-task write is observed on the main thread. Awaits already
  act as barriers in practice, but the explicit synchronization is
  cheap and removes the ambiguity.

Adds a WireMock test for the unknown-vendor skip path. Did NOT change
the URL form (vendor-routed even for claude) — server's route is
/hooks/session-end/{vendor=claude} which matches both /session-end and
/session-end/claude, and the paired server PR is what makes
parent_exited semantically meaningful in the first place, so older
servers aren't a concern here.
@alexeyzimarev

Copy link
Copy Markdown
Member Author

Thanks Qodo. Reviewed all four findings:

#1 Shutdown POST can hang — accepted, fixed in e85ddbb.

Real reliability concern. Wrapped the entire helper in a CancellationTokenSource(TimeSpan.FromSeconds(10)) budget, threaded into CreateAuthenticatedClientAsync, PostWithRetryAsync (as both ct and timeout), and the response body read. OperationCanceledException becomes a best-effort log + return. Watcher self-terminates in ≤10s on a stalled server, preserving the "self-terminate within ~5s" promise of the parent-PID watchdog.

#2 Unvalidated vendor in URL — accepted, fixed in e85ddbb.

Added a KnownVendors whitelist (claude, codex) and a skip + log if vendor is anything else. CLI runs locally so the user would already have code execution to exploit this, but a verbatim path-segment concat is still worth guarding. New WireMock test covers the skip path with a path-traversal vendor.

#3 Claude route compatibility — declined.

The server route is MapPost("/session-end/{vendor=claude}", …) — {vendor=claude} is an optional parameter with default "claude", so /hooks/session-end and /hooks/session-end/claude both match the same handler. There's no 404 risk against the current server. And since parent_exited is a new wire value (introduced by the paired server PR #628), older servers without that PR wouldn't recognize the reason anyway — version skew of this exact form isn't a real scenario.

#4 parentExited visibility race — accepted, fixed in e85ddbb.

Awaits between write and read already act as memory barriers in practice, but Qodo's right that the C# memory model doesn't formally guarantee it. Promoted to int + Interlocked.Exchange on write, Volatile.Read on read. Cheap and removes the ambiguity.

@alexeyzimarev
alexeyzimarev merged commit d69fbf6 into main May 16, 2026
4 checks passed
@alexeyzimarev
alexeyzimarev deleted the alexeyzimarev/ai-647-close-orphaned-sessions-when-watchers-parent-coding-agent branch May 16, 2026 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant