daemon: input delivery is activity — round-dispatch grace hardening by alexeyzimarev · Pull Request #537 · kurrent-io/kcap-cli

alexeyzimarev · 2026-08-11T19:08:09Z

Summary

This is the daemon (kcap-cli) half of kurrent-io/kcap-server#1403 — round-dispatch grace for the participant-inactivity rule. Design: docs/superpowers/specs/2026-08-10-ai1842-round-dispatch-grace-design.md (kcap-server, PR #1403).

The server side treats a stopped participant as reachable again once it can prove an in-flight round; this PR gives the daemon the two things that proof needs: input delivery is now activity (so a round that's actually being worked doesn't look idle), and a delivery-triggered status report so the server learns that fact promptly instead of waiting out the 60s cycle. It also closes the gap that made "input delivery is activity" unsafe on its own — a reviewer being idle-reaped while a round is mid-delivery.

What changed

1. Delivered input advances the per-agent activity clock — the sixth source. HandleSendInput now calls AgentActivityClock.Advance() on the delivery success path, joining PTY output, ACP envelope emission and turn start/end, Antigravity's and Pi's own agentActivity events, and LocalPermissionBridge's two reviewer tool-call-hit sites as clock sources. "Successfully handled" is decided per vendor mechanism: PTY and RPC throw on a genuine delivery failure, and Antigravity's exec-per-turn queue explicitly faults even its fire-and-forget branch — all three are clean. ACP's default (non-borrowed) path is not: EnqueueTurn(acknowledgeWrite: false) returns success on enqueue, not on the agent actually reading it, so a full _pendingTurns queue silently drops the input while still looking like a successful delivery. That residual is accepted and documented as kill-delaying-only — it can make a genuinely stuck reviewer look briefly alive to the clock, but it can never manufacture activity that isn't there for a healthy one, and it never suppresses a real timeout, only delays it by however long the delivery pipeline was already broken.

2. A delivery-triggered, uncorrelated status report — and the ordering gate it needs. Every delivered input now fires an out-of-cycle DaemonStatusReport (fire-and-forget, off the SignalR receive loop via Task.Run, matching the existing launch-stage-triggered report), so the server sees the advanced ActivitySeq/reset IdleForMs without waiting on the 60s periodic loop. This is only safe because of a new per-daemon _statusReportOrderingGate (SemaphoreSlim(1,1)), held from snapshot construction (BuildStatusReport(), evaluated inside the section) through completion of the hub send — never just through task creation. Without it, two concurrent emissions can enqueue on the semaphore in the opposite order from their content's causal order (SemaphoreSlim is only approximately FIFO), putting older content on the wire after newer content and permanently tripping the server's Regressed latch on wire-order content regression. All existing emission sites (periodic loop, legacy on-request nudge, launch-stage transition) already funnel through one method (SendDaemonStatusReportOnceAsync), so the gate covers them for free; the new delivery-triggered report is the second site added by this PR.

3. The atomic reap claim — fencing the reviewer-idle-reap race against the delivery window. Before this change, the heartbeat's FindReviewersToReap selection and the actual stop were separated only by an "does the agent still exist" check — no revalidation that the agent was still idle by the time the stop executed. Now:

The reap claim runs inside the same per-agent gate HandleSendInput already holds across its whole delivery body (BorrowedSnapshotGate, extended rather than renamed — its doc now names both purposes) — so a delivery and a reap claim are mutually exclusive.
The gate wait is bounded (ReapClaimGateWait, 20s, deliberately under the 30s heartbeat period) and falls back to a lock-free CAS claim for the unfenced rule (the absolute ReviewerMaxLifetime cap) on timeout — this closes a real circular-wait hazard: an unbounded wait would let a parked, non-draining PTY write(2) (no timeout, no cancellation) defer the reap that's supposed to end it, forever.
A permanently parked delivery defers idle/wedge reaps on every tick, by design — an in-progress delivery IS activity, so FindReviewersToReap's idle/wedge rules never select an agent whose delivery is still parked in the section, and reclaim shifts entirely to the unfenced absolute-lifetime cap (ReviewerMaxLifetime, ≤6h by default). With ReviewerMaxLifetime=0 (that rule disabled) a permanently parked delivery defers reaping indefinitely — accepted, not fixed, by this PR.
A late in-section refusal re-read (if (agent.IsReapClaimed) return;) sits immediately before the actual write, because the unfenced TTL claim's wait is deliberately shorter than a delivery's worst-case in-section time (borrowed-snapshot refresh can take up to 30s) — without the re-read, a perfectly healthy but slow delivery could complete into an agent already condemned by a claim that landed mid-flight, writing input and emitting an activity report for a stop that's already happening.
The graceful stop is now bounded (GracefulExitWait, 15s) around the send half, not just the exit-wait half — this applies to every stop, reap or otherwise, not just this PR's new path. It closes a pre-existing (not new) hazard: a non-draining child parks the graceful /exit send on the same uncancellable write(2) a parked delivery would, making TerminateAsync — the step that actually ends the wedge — unreachable exactly when it's needed most. Per-runtime check before widening the bound: ACP sends one session/cancel notify, Antigravity's graceful stop returns Task.CompletedTask immediately, and Pi's own stop grace is 3s — 15s truncates no legitimate graceful path on any of the four vendors; only a wedged PTY's raw write(2) was ever at risk.

Selection now captures a ReapCandidate (agent instance, reason, activity generation, FencedOnActivity) with the generation read before the age/idle/turn reads — the ordering is load-bearing (reasoned and documented in code; not independently testable without a clock-internal seam). FencedOnActivity distinguishes the two rules explicitly rather than by string-matching the reason later: idle/wedge reaps abort on an activity advance, the 6h absolute-lifetime cap never does (by design — "absolute" means absolute) but still takes the section so incarnation/membership are revalidated.

Scope: idle/wedge reaping only. User- and server-commanded stops are untouched — StopAgentCoreAsync/HandleStopAgent are the same shared executor for every caller, unmodified except for the graceful-stop bound, which is deliberately global (see above).

Test evidence

Full daemon unit-test suite (test/Capacitor.Cli.Tests.Unit, 7066 tests): 6981 passed, 50 failed, 35 skipped (gated live-vendor certification tests). Every one of the 50 failures was individually investigated and traced to one of two causes, neither caused by this branch:
- 44 failures across 6 test classes (CodexConfigTomlTests, CodexConfigWriterTests, CodexLauncherTests, CodexProjectKeyTests, PluginCommandCodexInstallIntegrationTests, UninstallCommandTests) reproduce identically (same names, same counts) on a byte-for-byte pristine origin/main checkout in an isolated worktree — a pre-existing, machine-local environment issue (Codex config.toml temp-path handling on this host), wholly unrelated to this diff, which touches no Codex/uninstall code.
- 6 failures (BorrowedReviewAuthBrokerTests ×3, GitProviderRouterTests ×1, and 2 in the touched AgentOrchestratorVendorTests partial class — PTY_output_chunk_advances_the_agents_activity_clock, part of the documented closed set of ~5 PTY/consent-dialog tests, and Shutdown_with_live_children_and_queued_stops_discards_the_queue_and_kills_every_child, a real-child-process dispose test whose own file this branch never touches) are machine-load timing flakes: all pass cleanly and quickly (sub-second to ~1s) when re-run individually, including the shutdown test, which missed its 30s bound by ~1s under load (31.1s) and completed in 1.17s alone.
The reap-claim suite (ReviewerReapingTests.cs): six claim tests plus a bound-relation pin, written across three TDD rounds — the initial three (stale-selection-aborts-after-delivery, contention-has-exactly-one-winner, max-lifetime-reaps-regardless-of-delivery), the bounded-wait round's parked-delivery-does-not-defer-the-absolute-lifetime-reap, and the graceful-stop round's parked-exit-write-does-not-stall-the-reap's-terminate and healthy-in-section-delivery-refuses-after-a-claim-lands-mid-flight — plus the reap-claim-gate-wait-stays-under-the-heartbeat-interval relation pin (not itself a claim test).
Mutation probes, all run by hand (mutate → red → restore → green): the ordering-gate removal and release-before-send mutations on the status-report test; the capture-before-acquire mutation (content built outside the section) on the same test; the unbounded-wait reversion reproducing the circular-wait deadlock; the unbounded-graceful-send reversion reproducing the unreachable-terminate hang; and the reap-claimed-refusal removal on the delivery side of the contention test.
scripts/check-linear-ids.sh — clean (exit 0) against the full branch diff.

Known residuals

Claim-during-write is still a live window. If a reap claim lands during an in-flight write (rather than before or after it), the delivery still advances the clock and emits a report once the write itself completes — the window is bounded by the write's own duration, not by the claim's timing. This is a smaller, inherent version of the race the late re-read (item 3 above) closes for the common case; closing it fully would need a second IsReapClaimed read positioned after the write but before Advance(), which was not built.
The correlated status-report handler (RequestStatusReport2/EchoNonce, AI-1787) remains unbuilt. It did not exist in the daemon at this branch's base commit. The_report_shape_carries_no_correlation_nonce is a deliberate tripwire test — reflection over DaemonStatusReport/LiveAgentInfo asserting no *nonce*/*echo* member exists — so that whoever lands the correlated handler inherits a red test by design, forcing them to route it through the same ordering section rather than silently bypassing it.
The must-route-through-the-gate rule is documented, not enforced. SendDaemonStatusReportOnceAsync is the one production call site of ServerConnection.DaemonStatusReportAsync, and the gate's field doc states that every future emission site must go through it — but nothing stops a future call site from invoking DaemonStatusReportAsync directly, compiling cleanly and silently reintroducing the server's Regressed latch. No architectural enforcement (visibility narrowing, a call-site-count test) was added; flagged as a candidate follow-up.

🤖 Generated with Claude Code

Do NOT merge.

HandleSendInput never touched AgentActivityClock, so an agent that only ever received input (never producing its own output before the next reap sweep) looked idle to the reviewer reaper even while actively in use. Advance the same clock PTY output/ACP envelopes/turn transitions already use, gated on the delivery await returning without throwing so a failed/cancelled write advances nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Review follow-up on the previous commit: add an ACP-mechanism test (SeedAcpAgent + the existing FakeAcpRuntime double) pinning that a non-throwing SendUserInputAsync (enqueue-accepted, not "the agent read it") also advances the clock — the deliberate, documented residual for that mechanism. SeedAcpAgent gains an optional activityClock override, mirroring SeedAgentForTest's existing param, so the test can control idle timing precisely. Also update AgentActivityClock's class/Advance doc comments to list delivered SendInput as a fifth activity source. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

A delivered SendInput now advances the activity clock (previous commit), but the server only learns about it on the next 60s report tick — long after the dispatch-relative grace it feeds has elapsed. Emit one DaemonStatusReport per successfully handled input invocation, right after the advance and gated on the same delivery success, fire-and-forget so a send failure can never fail the delivery. SendInputCommand carries no round/dispatch identity, so "one per round" is not implementable and duplicates are tolerated by design (a duplicate is content-honest). That second emitter makes report ordering load-bearing rather than hygiene: the server folds every flow-participant report and permanently latches "regressed" on ANY activity-seq regression, disabling that agent's liveness supervision for good. A report whose content was captured before the delivery but sent after the delivery-triggered one would present seq N after seq N+1 and trip exactly that latch — silently disabling supervision, the opposite failure direction. So every emission now captures its snapshot AND completes its hub send under one per-daemon SemaphoreSlim section, held through DaemonStatusReportAsync returning rather than merely through task creation. All existing emitters (periodic loop, on-request nudge, launch-stage transition) already funnel through SendDaemonStatusReportOnceAsync, so the gate lives there and covers them; the field doc states that every future emitter must come through it too. The settlement ack redelivery stays outside the section — it re-sends acks, not the report. Tests pin: the delivery emits exactly one report carrying the advanced seq and reset idle; a failed delivery emits none; content is monotone in send-COMPLETION order, driven by a probe connection that parks its first send (removing the gate makes the second emission overtake the parked one, which the test catches); and the report shape carries no correlation nonce, so an unsolicited report cannot masquerade as a correlated reply. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… the receive loop Review follow-up on the previous commit, three findings. The ordering test killed gate-removal and release-before-send but SURVIVED hoisting BuildStatusReport() above the acquire: both emissions build the same content either way, so entry count and wire order stayed as asserted. That mutation is unsound, not untidy — two emissions building outside the section can enqueue on the semaphore in the opposite order to their capture order (SemaphoreSlim is only approximately FIFO), putting older content on the wire second and permanently latching the server's fold into regressed. Pin it deterministically: once the EnteredCount == 1 assertion has proven the second emission is still parked on the gate, advance the clock again; a snapshot captured inside the section reads the new seq, a hoisted one is frozen at the delivery-time seq. Verified by hand-running that mutation — it now fails on the added assertion. The delivery-site emission was a bare discard, which is only asynchronous when it blocks: on an uncontended gate WaitAsync completes synchronously, so BuildStatusReport() ran inline on the SignalR receive-loop thread while this agent's BorrowedSnapshotGate was still held — and that build reads the PID record store off disk twice. Offload it with Task.Run; ordering is unaffected, since content is made monotone by the gate rather than by the order emissions are started in. Also name the delivered-input report as the fourth trigger of the settlement ack redelivery, which it silently moves from the 60s cadence to input rate (benign: normally-empty set, and duplicate acks are tolerated by design). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

FindReviewersToReap selects from a snapshot and the teardown happens later, so a reviewer handed its next round in between was still torn down under it — a recheck immediately before the stop only narrows that window, it does not close it. The claim now runs inside the per-agent section that already wraps every vendor's whole delivery body, so a delivery's clock advance and the reaper's claim are mutually exclusive and exactly one side wins: the claim re-validates incarnation and the activity generation captured at selection and aborts if anything advanced; a claim that lands first marks the agent, and the next delivery refuses rather than writing into a runtime about to be terminated. Selection captures the generation BEFORE its threshold reads, which is what makes every interleaving of the clock's independent lock acquisitions safe. Scoped to the idle and wedge rules — both are 'nothing has happened' claims a delivery falsifies. The absolute lifetime cap reaps regardless of activity, unchanged. The end-reason stamp moved into the won claim, so an aborted reap no longer leaves a reap reason on a live agent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…e lifetime cap The fence introduced a circular wait: a delivery holds the per-agent section across UnixPtyProcess.WriteAsync — a raw write(2) on the pty master, no timeout, not cancellable — which parks indefinitely if the child stops draining stdin, and the reap that would terminate that child (the only thing that releases the write) queued behind it. The unconditional reap used to break that cycle by construction. The claim now waits a bounded 20s (under the 30s heartbeat, so at most one waiter per agent is alive) and the two rules answer a timeout oppositely: a fenced idle/wedge claim gives up, since an in-progress delivery is activity anyway, while the absolute lifetime cap claims WITHOUT the section and proceeds — a rule a possibly-never- completing write can defer is not absolute. The claim latch is therefore an interlocked 0/1 CAS shared by both paths rather than a gate-guarded bool. Also: AgentActivityClock.Snapshot() gives selection all four observables under one lock acquisition, replacing the four separate reads and the ordering argument they needed; the reap sweep excludes IsPrivate explicitly, matching the stuck-Starting sweep beside it; both gates document the lock ordering they rely on (per-agent -> clock and ordering -> clock, never per-agent -> ordering, which is also why the delivery-triggered report is offloaded); and the single-flight claim collision logs instead of returning silently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… write Two gaps the bounded claim left open. The circular wait was broken at the claim but not at the stop: StopAgentCoreAsync awaited RequestGracefulStopAsync() unbounded, and for a PTY that writes /exit to the same uncancellable master fd the delivery was parked on — so against a child that has stopped draining stdin the graceful send parked too and TerminateAsync, the one step that ends the wedge, was never reached. The hang is pre-existing (every stop, incl. a user's, could hit it) but the reaper now depends on reaching terminate, so the send is bounded by the existing GracefulExitWait and the abandoned task observed. No legitimate graceful path is truncated: ACP sends one notify, Antigravity returns immediately, Pi's own grace is 3s. And the delivery-side refusal was checked only on entry to the section, while the steps after it are slower than the claim's gate wait (the borrowed-snapshot refresh alone is budgeted 30s) — so the unfenced claim fires against healthy in-flight deliveries, which then completed into a condemned agent: wrote the round, advanced the clock, emitted a report. The latch is monotonic, so re-reading it immediately before the write is free and sound. Tests: the parked-delivery double gains a park-every-write mode (a non-draining child parks /exit too, which the earlier arms let through); a new arm proves the reap reaches terminate through it; another proves a healthy mid-flight delivery refuses at the pre-write re-read. That one needed a window seam — parking a borrowed refresh instead was vacuous, since the refresh's own auth failure returns before the write. Also pins ReapClaimGateWait < HeartbeatInterval, now a named constant rather than a comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

qodo-code-review · 2026-08-11T19:09:49Z

PR Summary by Qodo

Daemon: treat delivered input as activity and harden reap/report ordering

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Advance per-agent activity clock on successful SendInput delivery to prevent false idleness.
• Emit an immediate, ordered DaemonStatusReport after delivery to satisfy round-dispatch grace.
• Add bounded, fenced reviewer-reap claiming to avoid mid-delivery teardown and deadlocks.

Diagram

graph TD
A["SendInput command"] --> B["HandleSendInput (per-agent gate)"] --> C{"Reap claimed?"}
C -->|"no"| D["Deliver input + Advance clock"] --> E["Offloaded status report (ordering gate)"] --> F["Server folds report"]
G["Heartbeat reaper"] --> B

High-Level Assessment

The following are alternative approaches to this PR:

1. Single-threaded report emitter (Channel worker)

➕ Naturally serializes snapshot+send without relying on SemaphoreSlim fairness
➕ Avoids Task.Run fan-out; one background loop can coalesce bursts
➖ More moving parts (lifetime, shutdown, backpressure) than a simple semaphore
➖ Still must ensure snapshot is captured in-worker to preserve monotonicity

2. Correlate SendInput/dispatch with a dedicated “activity ack” message

➕ Avoids sending full status reports at input rate
➕ Makes the grace proof explicit and per-round (if IDs exist)
➖ Requires protocol/DTO additions across daemon/server; larger surface area
➖ PR notes SendInput currently lacks round identity, so correlation would require broader redesign

3. Make ACP delivery acknowledgement real (don’t advance clock on enqueue-only)

➕ Eliminates the documented “kill-delaying-only” residual for ACP
➕ Stronger semantic guarantee: clock advances only when agent actually reads input
➖ Potentially invasive changes to ACP runtime queueing and flow control
➖ May add latency/complexity to ACP path; likely not necessary for the grace design goal

Recommendation: Current approach is sound for the stated design constraints: it adds the missing liveness signal (delivered input), ensures the server learns it promptly (delivery-triggered report), and hardens correctness with ordering + a reap/delivery fence. If status-report frequency becomes a concern later, consider a Channel-based emitter with coalescing, but the semaphore-gated snapshot+send is the lowest-risk change for now.

Files changed (8) +1325 / -43

Enhancement (1) +31 / -2

AgentActivityClock.csAdd atomic clock snapshot + document SendInput as activity source +31/-2

Add atomic clock snapshot + document SendInput as activity source
• Updates documentation to include successfully delivered SendInput as a fifth activity source. Introduces Snapshot() returning an ActivitySnapshot record so idle/age/turn/seq can be read atomically under one lock, avoiding mixed-instant reads.
src/Capacitor.Cli.Daemon/Services/AgentActivityClock.cs

Bug fix (1) +497 / -28

AgentOrchestrator.csFence reaping vs delivery, advance activity on delivery, order status reports +497/-28

Fence reaping vs delivery, advance activity on delivery, order status reports

• HandleSendInput now refuses delivery if the agent was already claimed for reap, advances AgentActivityClock on successful delivery, and triggers an out-of-cycle status report offloaded via Task.Run. Status reporting is serialized with a per-daemon ordering gate held across snapshot construction and hub send to prevent ActivitySeq regressions on the wire. Reviewer reaping is reworked into selection plus a bounded, per-agent claim step (with an unfenced TTL fallback) to prevent mid-delivery teardown and avoid deadlocks with uncancellable PTY writes; StopAgentCoreAsync also bounds the graceful-stop send to ensure terminate remains reachable.

src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs

Tests (6) +797 / -13

AgentOrchestratorFinalizerVerdictTests.csAllow ACP agent seeding with a controllable activity clock +8/-3

Allow ACP agent seeding with a controllable activity clock
• Extends SeedAcpAgent to accept an optional AgentActivityClock override, enabling tests to precisely control idle/age timing via FakeTimeProvider while preserving existing defaults.
test/Capacitor.Cli.Tests.Unit/AgentOrchestratorFinalizerVerdictTests.cs

AntigravityReviewerReapingTests.csUpdate reaping assertions for new ReapCandidate shape +2/-2

Update reaping assertions for new ReapCandidate shape
• Adjusts tests to assert on (id, reason) projections rather than the previous tuple list, matching FindReviewersToReap now returning structured ReapCandidate objects.
test/Capacitor.Cli.Tests.Unit/AntigravityReviewerReapingTests.cs

ReviewerTtlTests.csUpdate TTL/idle selection assertions for ReapCandidate +2/-2

Update TTL/idle selection assertions for ReapCandidate
• Updates TTL and idle reap selection assertions to compare verdict projections instead of raw tuples, aligning with the new ReapCandidate return type.
test/Capacitor.Cli.Tests.Unit/Daemon/ReviewerTtlTests.cs

DeliveryTriggeredStatusReportTests.csAdd tests for delivery-triggered report and monotone snapshot/send ordering +200/-0

Add tests for delivery-triggered report and monotone snapshot/send ordering

• Adds a new test suite validating that successful input delivery emits exactly one immediate DaemonStatusReport carrying the advanced ActivitySeq and reset idle, and that failed deliveries emit none. Includes an ordering probe server double to prove the ordering gate is held through the send and that report content is captured inside the gate, preventing ActivitySeq regression in completion order.

test/Capacitor.Cli.Tests.Unit/DeliveryTriggeredStatusReportTests.cs

ReviewerReapingTests.csAdd claim-fence tests for reap vs delivery races and deadlock bounds +468/-6

Add claim-fence tests for reap vs delivery races and deadlock bounds

• Introduces extensive tests for the new reap claim behavior: stale selection abort after delivery, mutual exclusion with exactly one winner, TTL rule reaping regardless of activity, bounded wait behavior under parked PTY writes, bounding graceful-stop send to reach terminate, and a mid-flight unfenced claim causing pre-write delivery refusal. Adds helper Verdicts() projection and a ParkingPtyProcess test double.

test/Capacitor.Cli.Tests.Unit/ReviewerReapingTests.cs

SendInputActivityClockTests.csAdd tests proving SendInput delivery advances activity clock (and ACP residual) +117/-0

Add tests proving SendInput delivery advances activity clock (and ACP residual)
• Adds tests verifying that successful SendInput advances ActivitySeq and resets idle time, while failed deliveries do not. Includes an ACP-specific contract test showing enqueue-accepted SendUserInputAsync also advances the clock as a documented, deliberate residual.
test/Capacitor.Cli.Tests.Unit/SendInputActivityClockTests.cs

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3f049f0efe

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-11T19:11:41Z

+        try {
+            if (!await TryClaimReapAsync(candidate)) return;
+
+            await HandleStopAgent(candidate.Id);


Stop the claimed agent instance, not a reused ID

If the selected reviewer exits and cleanup/relaunch replaces its _agents entry after TryClaimReapAsync validates the captured instance but before this call runs, HandleStopAgent(candidate.Id) resolves the reused ID again and can terminate the fresh incarnation. This defeats the incarnation check carried by ReapCandidate; pass the claimed candidate.Agent into the stop executor or revalidate ReferenceEquals at the stop boundary.

Useful? React with 👍 / 👎.

Fixed in 3b364e1: the post-claim stop is split into StopClaimedReapAsync, which re-validates ReferenceEquals(_agents[candidate.Id], candidate.Agent) immediately before HandleStopAgent and aborts with the existing agent_gone log on mismatch — the claimed instance, not the reused id, is now what authorizes the stop. Agent ids are unique per launch today, so this makes the property structural rather than reachable; pinned by Relaunch_between_claim_and_stop_aborts_the_reap (map entry swapped between claim and stop, reap aborts).

qodo-code-review · 2026-08-11T19:14:09Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. ~~Verbose HandleSendInput comments~~ ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

New and updated comments in HandleSendInput and the _statusReportOrderingGate XML documentation
are overly verbose and embed design-doc-level rationale directly in code, which hurts readability
and increases maintenance cost. This violates the guideline (PR Compliance ID 3) to keep comments
minimal and rely on self-explanatory structure/naming with concise intent comments instead.

Code

src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[R3120-3123]

+            // Re-checked HERE, immediately before the write, and NOT only at the top of the section.
+            // The steps between the two checks are slow by design — the borrowed-snapshot refresh alone
+            // is budgeted 30s (BorrowedSnapshotRefreshTimeout), plus any attachment downloads — which is
+            // LONGER than the reap claim's own gate wait, so a perfectly healthy borrowed delivery

Evidence
PR Compliance ID 3 flags overly verbose comments as a failure mode, and the cited areas show this
pattern: HandleSendInput contains extended narrative comment blocks spanning many lines (including
the pre-write re-check and delivery-triggered report sections), and _statusReportOrderingGate has
a multi-paragraph XML doc comment that goes beyond concise intent/constraints by including extensive
rationale better suited for a design/spec document.
CLAUDE.md: Avoid verbose comments; prefer self-explanatory code
src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[3120-3187]
src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[1167-1198]

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 code includes long, multi-paragraph comment blocks (both inline and XML docs) that restate design rationale at length—specifically in `HandleSendInput` and the `_statusReportOrderingGate` field documentation. Per PR Compliance ID 3, refactor these comments to be concise and intent-focused, relying on clearer code structure/naming where possible.

## Issue Context
Deeper design rationale should live in design/spec documentation rather than in-source comments; in code, prefer a short statement of purpose/constraints (e.g., monotone status report ordering / lock-ordering invariant) and, if needed, a brief pointer/link to the relevant design doc.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[3097-3187]
- src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[1167-1198]

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

2. ~~Unbounded report task backlog~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

HandleSendInput now starts a new fire-and-forget Task for every successful input delivery, but
SendDaemonStatusReportOnceAsync serializes on _statusReportOrderingGate with an unbounded WaitAsync
and can be held indefinitely by a stalled SignalR send. Under a prolonged transport stall, this can
accumulate an arbitrary number of blocked Tasks (memory/CPU pressure) and also delay the server’s
RequestStatusReport handler behind the backlog.

Code

src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[R3187-3188]
+            _ = Task.Run(() => SendDaemonStatusReportOnceAsync());
+

Evidence
The PR adds a per-delivery Task.Run trigger for status reporting, while report sending is strictly
single-filed by a semaphore acquired with an unbounded wait; the underlying SignalR send has no
explicit timeout beyond the daemon-wide cancellation token, so a stalled send can hold the semaphore
and cause arbitrary accumulation of blocked tasks.
src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[1205-1232]
src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[3080-3192]
src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[155-163]

Agent prompt

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

## Issue description
`HandleSendInput` spawns a `Task.Run(() => SendDaemonStatusReportOnceAsync())` per delivered input. `SendDaemonStatusReportOnceAsync` serializes sends with `_statusReportOrderingGate.WaitAsync()` but the wait has no cancellation/timeout. If one report send stalls (SignalR send has no explicit timeout besides the daemon-wide token), the semaphore can stay held and subsequent per-input tasks will pile up waiting forever.

## Issue Context
This PR intentionally added ordering (`_statusReportOrderingGate`) and delivery-triggered reporting. The ordering guarantee can be preserved without creating a new Task per input and without indefinite semaphore waits.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[1205-1232]
- src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[3120-3188]
- src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[155-163]

## Suggested remediation (choose one)
1) **Cancellation-aware ordering wait**: acquire `_statusReportOrderingGate` with `_shutdownCts.Token` (and handle `OperationCanceledException`) so shutdown and stalled sends don’t leave waiters stuck indefinitely.
2) **Single background worker + queue**: replace `Task.Run` per delivery with a dedicated background loop (e.g., `Channel<int>` or `Channel<bool>`) that serially drains “report requested” signals and calls `SendDaemonStatusReportOnceAsync`. This preserves ordering while avoiding unbounded Task creation.
3) **Explicit timeout on report send**: wrap `_server.DaemonStatusReportAsync(...)` in `WaitAsync(timeout, _shutdownCts.Token)` and on timeout release the gate and drop/log the report (prevents permanent semaphore hold).

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

Tip of the day

💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

- Move the graceful-stop request inside the inner try so a synchronous throw falls through to terminate instead of skipping it via the outer catch (a reap-path skip would leave a permanent zombie). - Drop a stale duplicate GracefulExitWait <summary> block. - Reword the delivery/reap gate doc: the true rule is that a waiter whose own action unblocks the holder must bound its wait; the delivery's entry wait is exempt (holder class, not bounded class). - Give the late in-section reap-claim refusal its own log message, distinguishing it from the entry-check refusal. - Offload the on-request status-report nudge via Task.Run, matching the delivery site, so it doesn't park the SignalR receive loop behind a gated hub send. - Correct AgentActivityClock's source-list doc: six families, not five (was missing Antigravity/Pi agentActivity and LocalPermission- Bridge's second site). - Update PR #537's body to match: sixth clock source (not fifth), parked-delivery deferral semantics, and six claim tests + a relation pin (not seven claim tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…, tighten comments Three follow-up review findings on round-dispatch grace: - ReapReviewerAsync's stop now re-validates the claimed AgentInstance is still the live _agents entry (StopClaimedReapAsync) immediately before handing off to HandleStopAgent, which re-resolves by id. Unreachable today (agent ids are unique per launch) but closes the TOCTOU structurally instead of relying on that invariant holding forever. - SendDaemonStatusReportOnceAsync no longer holds _statusReportOrderingGate for an unbounded hub send: the gate acquisition and the send invocation are each bounded by _shutdownCts, and the WAIT for send completion is capped by the new StatusReportSendTimeout, releasing the gate on timeout without waiting further (invocation order, not completion order, is what the gate's wire-FIFO invariant needs). The abandoned send is observed so a later fault isn't an unobserved task exception. - Compressed the essay-length comment blocks around the ordering gate and HandleSendInput's reap-claim checks into concise constraint statements, pointing at the spec for rationale instead of re-deriving it inline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

alexeyzimarev and others added 7 commits August 11, 2026 18:03

chatgpt-codex-connector Bot reviewed Aug 11, 2026

View reviewed changes

qodo-code-review Bot reviewed Aug 11, 2026

View reviewed changes

Comment thread src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs Outdated

Comment thread src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs

alexeyzimarev and others added 3 commits August 11, 2026 21:33

Merge branch 'main' into round-dispatch-grace

eff004f

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

alexeyzimarev merged commit 05d9bb9 into main Aug 12, 2026
6 checks passed

alexeyzimarev deleted the round-dispatch-grace branch August 12, 2026 09:50

Navigation Menu

Uh oh!

daemon: input delivery is activity — round-dispatch grace hardening - #537

daemon: input delivery is activity — round-dispatch grace hardening#537
alexeyzimarev merged 10 commits into
mainfrom
round-dispatch-grace

alexeyzimarev commented Aug 11, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Aug 11, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Aug 11, 2026

Uh oh!

alexeyzimarev Aug 12, 2026

Uh oh!

qodo-code-review Bot commented Aug 11, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

alexeyzimarev commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What changed

Test evidence

Known residuals

Uh oh!

qodo-code-review Bot commented Aug 11, 2026

PR Summary by Qodo

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 11, 2026

Choose a reason for hiding this comment

Uh oh!

alexeyzimarev Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

qodo-code-review Bot commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

alexeyzimarev commented Aug 11, 2026 •

edited

Loading

qodo-code-review Bot commented Aug 11, 2026 •

edited

Loading