Skip to content

Make daemon startup fast, observable, and honestly reported - #882

Merged
realtonyyoung merged 2 commits into
mainfrom
claude-tyoung/ai2684-startup-residuals
Sep 11, 2026
Merged

realtonyyoung merged 2 commits into
mainfrom
claude-tyoung/ai2684-startup-residuals

Conversation

@realtonyyoung

Copy link
Copy Markdown
Collaborator

Closes #859 — AI-2684

What & why

The version-probe deadlock that caused the multi-minute silent startup was fixed earlier (#869). This closes the three residuals that issue left open.

The vendor --version probes ran sequentially before the host existed, so N bounded probes serialized ahead of the socket bind and the server connect. They now run concurrently under one wall-clock ceiling — strictly above a single vendor's own budget, so overlapping only removes the N-way serialization and never cuts a probe short; a vendor still unfinished at the ceiling is a miss the capability-refresh path re-probes. Startup now emits a phase trace ("lock acquired", "vendors probed", "host built", …) so a future stall shows the last phase reached, where before the pre-host window logged nothing. And kcap daemon status now distinguishes a daemon that is serving from one whose PID is live but whose control socket does not yet answer, reporting "running (PID, starting — not yet serving)" instead of a flat "running".

Where to look

The concurrent probe keeps each vendor's per-vendor budget; the ceiling is only a backstop above it. status dials the local control socket with a 2 s Hello — a bound daemon answers immediately, an unbound one refuses at once — so it does not hang.

Deferred (a minor follow-up, not a core residual): moving the full vendor-list advertisement after connect. The list is produced by the gated-reviewer spawns that the connect path reads synchronously, so deferring it risks a brief no-reviewers window; the concurrency here already delivers the fast-startup goal.

Verification

  • New tests: a concurrent probe pass finishes in roughly one probe's budget, not the sum; the phase trace records each phase; status reports serving vs still-starting from the Hello outcome. Regression DaemonRunnerCursorAvailabilityTests 37/37.
  • Daemon and CLI suites green apart from known env flakes; both AOT publishes clean; README updated for the new status wording.

Startup vendor --version probes now run concurrently under one ceiling set
above a single vendor's own budget: N vendors cost ~one probe and no outcome
changes, so the transient-miss-never-disables invariant holds. A pre-host
stderr phase trace records how far a boot got before the logger exists, and
`daemon status` calls it starting, not serving, until its socket answers.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Make daemon startup fast, observable, and accurately reported

🐞 Bug fix ✨ Enhancement 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

Grey Divider

AI Description

• Run vendor startup probes concurrently under a shared ceiling to prevent serialized delays.
• Trace startup phases before and after host construction for stall diagnosis.
• Report live daemons as starting until their control socket answers Hello.
Diagram

sequenceDiagram
    actor Operator
    participant CLI as Status CLI
    participant PID as PID Store
    participant Daemon
    participant Vendors as Vendor CLIs
    participant Socket as Control Socket
    participant Server

    Daemon->>PID: Acquire lock
    Daemon->>Daemon: Trace boot phases
    Daemon->>Daemon: Build host
    par Vendor A
        Daemon->>Vendors: Probe version
    and Vendor N
        Daemon->>Vendors: Probe version
    end
    Daemon->>Socket: Bind listener
    Daemon->>Server: Connect

    Operator->>CLI: Request status
    CLI->>PID: Validate PID
    CLI->>Socket: Hello within 2s
    alt Socket answers
        Socket-->>CLI: Well-formed Hello
        CLI-->>Operator: Running
    else Socket unavailable
        CLI-->>Operator: Starting, not serving
    end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Bind before vendor advertisement
  • ➕ Makes the local control socket available before any vendor probing completes.
  • ➕ Removes vendor probe latency entirely from socket readiness.
  • ➖ Creates a temporary no-reviewers advertisement window.
  • ➖ The connection path currently consumes the gated reviewer list synchronously.
  • ➖ Requires broader lifecycle and capability-refresh coordination.
2. Use cancellable asynchronous probes
  • ➕ Could cancel unfinished subprocess work when the shared ceiling expires.
  • ➕ Avoids blocking the startup thread with Task.WaitAll.
  • ➖ Requires converting synchronous factory and version-probe APIs to asynchronous contracts.
  • ➖ Broadens the change across runtime factories and subprocess handling.
  • ➖ Adds complexity without reducing the normal one-probe startup budget.

Recommendation: Keep the PR's concurrent bounded-probe approach. It removes N-way serialization while preserving each vendor's existing probe budget and startup advertisement semantics. Binding first is potentially faster but risks a transient no-reviewers state, while a fully asynchronous probe contract would require a disproportionate cross-cutting refactor.

Files changed (6) +249 / -17

Enhancement (1) +24 / -1
DaemonCommands.csProbe serving readiness in daemon status +24/-1

Probe serving readiness in daemon status

• Adds a two-second control-socket Hello probe after PID validation. Status now distinguishes a serving daemon from a live process that is still starting.

src/Capacitor.Cli/Commands/DaemonCommands.cs

Bug fix (1) +89 / -16
DaemonRunner.csParallelize startup probes and trace boot phases +89/-16

Parallelize startup probes and trace boot phases

• Runs unattended-support and CLI-version probes concurrently under a shared ceiling while preserving per-vendor outcomes. Adds stderr breadcrumbs before host construction and structured phase logging through socket binding and server connection.

src/Capacitor.Cli.Daemon/DaemonRunner.cs

Tests (3) +130 / -0
DaemonRunnerConcurrentProbeTests.csVerify concurrent vendor probe behavior +73/-0

Verify concurrent vendor probe behavior

• Tests that vendor probes overlap, slow probes map to the timeout value without blocking completed results, and empty probe sets return an empty result.

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

DaemonRunnerStartupPhaseTests.csVerify pre-host startup breadcrumbs +32/-0

Verify pre-host startup breadcrumbs

• Tests that startup phases are written to stderr with their names and remain ordered by execution.

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

DaemonStatusServingTests.csVerify serving and starting status messages +25/-0

Verify serving and starting status messages

• Tests the pure status classifier for both a responsive control socket and a live daemon that is not yet serving.

test/Capacitor.Cli.Tests.Unit/Commands/DaemonStatusServingTests.cs

Documentation (1) +6 / -0
README.mdDocument the still-starting daemon status +6/-0

Document the still-starting daemon status

• Documents the status shown when a daemon process is alive but its local control socket is not yet serving requests.

README.md

@qodo-code-review

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

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Action required

1. Older daemons appear stuck starting ✓ Resolved 🐞 Bug ≡ Correctness
Description
Status treats HelloProbe.RunAsync(...).WellFormed == false as proof that a validated daemon PID
has not yet started serving. A daemon from before the Hello frame support accepts the control-socket
connection but drops the unknown frame without replying, so updating only the CLI makes an
already-serving daemon report as still starting.
Code

src/Capacitor.Cli/Commands/DaemonCommands.cs[R638-639]

+                var hello = await HelloProbe.RunAsync(store, name, ServingProbeTimeout);
+                await Console.Out.WriteLineAsync($"Daemon '{name}': {DescribeRunningDaemon(entry.Pid, hello.WellFormed)}");
Relevance

●●● Strong

Breaks backward compatibility: older daemons can be alive while rejecting the newer Hello frame.

PR-#147

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The added status path uses only WellFormed to select the new starting wording. HelloProbe
returns false for all failures after connection as well as connection failures, and the repository's
IPC design explicitly documents that pre-Hello daemons drop an unknown Hello frame and return EOF
rather than a reply.

src/Capacitor.Cli/Commands/DaemonCommands.cs[634-639]
src/Capacitor.Cli/Services/HelloProbe.cs[20-35]
docs/superpowers/specs/2026-08-01-slice2-prework-control-ipc-design.md[144-148]

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

## Issue description
`daemon status` currently maps every unsuccessful Hello exchange to `starting — not yet serving`. Preserve whether the local control socket was successfully connected, and classify a connected daemon that rejects or closes on an unsupported Hello as serving; only connection refusal, timeout, or inability to connect should produce the starting state.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/DaemonCommands.cs[634-639]
- src/Capacitor.Cli/Services/HelloProbe.cs[20-43]

## Recommended Fix
Extend the probe result with a socket-reachability signal set after `ConnectAsync` succeeds. Have `Status` use that signal for the serving/not-serving wording while retaining the existing well-formed Hello data for callers that require protocol validation, and add an integration test covering a reachable daemon that closes after an unsupported Hello frame.

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



Remediation recommended

2. Stalled vendor probes survive startup ✓ Resolved 🐞 Bug ☼ Reliability
Description
ProbeVendorsConcurrently returns fallback values when Task.WaitAll reaches its ceiling but
neither cancels nor joins unfinished Task.Run probes, and ProbeCliVersionOnce can then remain
blocked waiting for DrainToEndAsync to observe EOF. When a version process exits while a
descendant retains an inherited redirected pipe, the probe and its resources survive daemon startup
and can overlap with additional probes launched by later capability refreshes.
Code

src/Capacitor.Cli.Daemon/DaemonRunner.cs[R1575-1587]

+        var tasks = new Dictionary<string, Task<T>>(StringComparer.Ordinal);
+        foreach (var vendor in vendors)
+            tasks.TryAdd(vendor, Task.Run(() => probe(vendor)));
+
+        if (tasks.Count == 0) return FrozenDictionary<string, T>.Empty;
+
+        try { Task.WaitAll([.. tasks.Values], ceilingMs); } catch { /* per-task faults handled below */ }
+
+        var result = new Dictionary<string, T>(tasks.Count, StringComparer.Ordinal);
+        foreach (var (vendor, task) in tasks) {
+            if (task.IsFaulted) _ = task.Exception;   // observe so a fault is never an unobserved-task exception
+            result[vendor] = task.Status == TaskStatus.RanToCompletion ? task.Result : timedOut;
+        }
Relevance

●●● Strong

Matches accepted precedent requiring bounded subprocess drains and cleanup to prevent
redirected-pipe deadlocks.

PR-#869
PR-#448

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The helper starts blocking probe delegates on background tasks, maps tasks that remain incomplete at
the shared ceiling to fallback values, and returns without cancellation or joining them. The version
probe synchronously waits for asynchronous output drains whose helper reads until EOF without a
deadline, so a descendant retaining an inherited pipe can prevent completion indefinitely; because
capability refresh invokes the same probing path again, these unfinished operations can overlap
future passes, consistent with the known redirected-output deadlock failure mode in this area.

src/Capacitor.Cli.Daemon/DaemonRunner.cs[1572-1589]
src/Capacitor.Cli.Daemon/DaemonRunner.cs[1619-1636]
src/Capacitor.Cli.Daemon/DaemonRunner.cs[1643-1655]}]}}]}## END OF ORIGINAL RESPONSE >>>## START OF CRITIQUE >>>1. Summary and outline of the approach taken</br></br>The response took a focused, evidence-driven review approach rather than trying to comment on every changed line superficially.</br></br>### Analysis/reasoning approach</br>- It first identified the main risk areas in the diff:</br> - concurrent vendor probing in DaemonRunner</br> - status behavior via HelloProbe</br> - startup phase logging</br>- It then used parallel repository exploration effectively:</br> - one exploration for concurrent probe behavior and factory call sites</br> - one for daemon status and HelloProbe protocol semantics</br> - one for startup ordering and lingering probe behavior</br>- It manually read the most relevant code regions after the subagents returned, including:</br> - ProbeVendorsConcurrently</br> - ProbeCliVersionOnce / DrainToEndAsync</br> - HelloProbe.RunAsync</br> - the control IPC design spec for down-level Hello behavior</br> - Status formatting logic</br>- It considered additional possibilities, such as duplicate vendor dictionary keys, thread-pool starvation, and status latency, but did not report them without sufficient evidence.</br></br>### Final-answer approach</br>The final answer reported exactly two findings:</br>1. A correctness issue: `: TBD
src/Capacitor.Cli.Daemon/DaemonRunner.cs[1572-1590]
src/Capacitor.Cli.Daemon/DaemonRunner.cs[1625-1655]
src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[1267-1285]
PR-#869

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 concurrent probe ceiling limits only how long startup waits for results, not the lifetime of the tasks it starts. A version command whose descendant retains an inherited redirected pipe can leave a probe blocked indefinitely after the child exits, allowing later capability refreshes to accumulate additional tasks and process resources.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/DaemonRunner.cs[1572-1590]
- src/Capacitor.Cli.Daemon/DaemonRunner.cs[1608-1655]

## Recommended Fix
Make version probing cancellation-aware and give post-exit pipe draining an explicit bounded deadline. At the shared ceiling, cancel unfinished probes, close or cancel output drains, kill and reap reachable process trees, and retain and observe all task handles until bounded cleanup completes before returning a miss. Add a regression test using a child or descendant that keeps a redirected output pipe open, asserting that the probe task finishes, startup does not retain background work, and later refreshes cannot accumulate stale probes.

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


3. Startup breadcrumbs evade daemon logs ✓ Resolved 🐞 Bug ◔ Observability
Description
StartupPhase writes the new pre-host breadcrumbs to standard error, which detached startup
redirects to the sibling .out.log rather than the primary daemon log. The supported `kcap daemon
logs command and detached-start output expose only daemon.log`, so operators following the
documented path cannot see the phases intended to diagnose an early startup stall.
Code

src/Capacitor.Cli.Daemon/DaemonRunner.cs[R1886-1889]

+    internal static void StartupPhase(string phase) {
+        try {
+            Console.Error.WriteLine($"{DateTime.Now:yyyy-MM-dd HH:mm:ss.fff} [startup] {phase}");
+            Console.Error.Flush();
Relevance

●●● Strong

Observability breadcrumbs should follow the documented daemon-log path; prior stderr-capture
diagnostics were accepted.

PR-#243

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Detached launch passes a .out.log path for standard-error capture, and StdErrCapture redirects
file descriptors 1 and 2 there before the host exists. The new pre-host method writes to that
descriptor, but the logs command reads only the primary LogPath.

src/Capacitor.Cli.Daemon/DaemonRunner.cs[1880-1892]
src/Capacitor.Cli/Commands/DaemonCommands.cs[214-220]
src/Capacitor.Cli.Daemon/StdErrCapture.cs[45-65]
src/Capacitor.Cli/Commands/DaemonCommands.cs[879-893]

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

## Issue description
Detached pre-host phase messages are captured in an undisclosed sibling output file, while the daemon log command reads only the primary logger file. This hides the exact startup diagnostics introduced for failures before host construction.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/DaemonRunner.cs[1886-1892]
- src/Capacitor.Cli/Commands/DaemonCommands.cs[214-220]
- src/Capacitor.Cli/Commands/DaemonCommands.cs[879-893]

## Recommended Fix
Make the supported log path surface the pre-host capture, either by including `.out.log` in `kcap daemon logs` and advertising it at detached startup or by writing the breadcrumbs into a file that the command already displays. Preserve the separate standard-error capture needed for native and pre-host failures.

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


4. Status delays grow per silent daemon ✓ Resolved 🐞 Bug ➹ Performance
Description
Status awaits each two-second HelloProbe inside the ordered daemon-name loop instead of probing
the live daemons concurrently. When several valid daemon processes have sockets that accept but do
not answer, an unscoped status request takes up to two seconds per daemon rather than one bounded
probe interval.
Code

src/Capacitor.Cli/Commands/DaemonCommands.cs[R638-639]

+                var hello = await HelloProbe.RunAsync(store, name, ServingProbeTimeout);
+                await Console.Out.WriteLineAsync($"Daemon '{name}': {DescribeRunningDaemon(entry.Pid, hello.WellFormed)}");
Relevance

●● Moderate

Sequential bounded probes create linear latency, but no close team precedent establishes concurrent
status probing.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The status command enumerates all running and installed names and awaits each probe directly inside
its sequential loop. Each probe owns a two-second cancellation timeout covering connection, request,
and response, so silent sockets incur that budget serially.

src/Capacitor.Cli/Commands/DaemonCommands.cs[617-639]
src/Capacitor.Cli/Services/HelloProbe.cs[20-35]

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 all-daemons status path serializes a separate two-second Hello timeout for every validated live process. Multiple silent control sockets therefore multiply command latency by the number of daemon names.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/DaemonCommands.cs[617-639]
- src/Capacitor.Cli/Services/HelloProbe.cs[20-35]

## Recommended Fix
Start Hello probes concurrently for all validated live daemons, await them under one aggregate interval, and then render results in the existing deterministic name order. Keep the explicit single-name behavior and per-daemon failure classification unchanged.

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


Grey Divider

Context sources
✅ Compliance rules (platform): 64 rules
✅ Cross-repo context — repo relationships
  Explored: repo: kurrent-io/cloud-meta (sha: 15782900)
Review mode: 🧠 Deep: This changes daemon startup concurrency, timeout/error handling, observability, and CLI serving-state reporting across multiple independent runtime paths, creating a dense set of subtle lifecycle and regression risks.

Grey Divider

Tip of the day
💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/Capacitor.Cli.Daemon/DaemonRunner.cs
Comment thread src/Capacitor.Cli/Commands/DaemonCommands.cs Outdated
Comment thread src/Capacitor.Cli/Commands/DaemonCommands.cs Outdated
Comment thread src/Capacitor.Cli.Daemon/DaemonRunner.cs
Four startup-path fixes: status keyed serving on a well-formed Hello, so
a daemon built before the Hello frame — it accepts the connection and
drops the unknown frame — read as still starting; socket reachability is
the serving signal instead. Status now probes every live daemon at once,
not one two-second timeout each. A version probe whose child exits while
a descendant holds the redirected pipe no longer blocks forever draining
to an EOF that never comes: the drains are bounded, and closing our read
ends hands back the version buffered before the block rather than losing
it. And daemon logs surfaces the stderr capture sibling, where the
pre-host breadcrumbs land, rather than only the primary log.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@realtonyyoung

Copy link
Copy Markdown
Collaborator Author

Codex exec review (on the review-round commit)

Ran a local codex exec review --base main. It confirmed the concurrent probe no longer hangs, and raised two P2s on the version-probe path, both addressed in 944abef:

  • [P2] Preserve buffered version after cancellation — the first revision discarded the version when a descendant held the pipe (Task.WaitAll raced the cancelled drain's catch, so ResultOrEmpty read ""), which would advertise an unknown version and reject a launch. Reworked so recovery does not depend on read-cancellation at all: after the child is dealt with, closing our own pipe read-ends deterministically unblocks the wedged drain, which returns the bytes it had already buffered. The probe test now asserts both a bounded return and the recovered version (9.9.9) for the descendant-holds-pipe shape.

  • [P2] Reap descendants after the parent exits — ProcessTree.Kill no-ops once the parent has exited. The timeout path (child still alive) is reaped as before. A descendant that outlives a cleanly-exited child has reparented to init and is no longer reachable from the parent's pid; reaping it portably would require process-group isolation at spawn — a disproportionate change for a --version that double-forks a long-lived pipe-holder. It is detached instead (our read end closed, so its next write fails), which prevents it from wedging the probe or accumulating a live resource against us. This is a documented, deliberate limitation.

Local verification: DaemonRunnerVersionProbeTests (incl. the bounded+recovered descendant case), DaemonRunnerConcurrentProbeTests, DaemonRunnerStartupPhaseTests, HelloProbeTests, DaemonStatusServingTests, DaemonLogsTests, and the ServiceVerify suites all green; both AOT publishes clean.

@realtonyyoung
realtonyyoung merged commit d5e4968 into main Sep 11, 2026
7 checks passed
@realtonyyoung
realtonyyoung deleted the claude-tyoung/ai2684-startup-residuals branch September 11, 2026 12:38
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.

Service-installed daemon takes minutes to start, then hangs before binding its socket while status reports running

1 participant