Skip to content

Give each vendor probe a dedicated thread - #912

Merged
realtonyyoung merged 1 commit into
mainfrom
claude-tyoung/ai-2729-7e1147
Sep 11, 2026
Merged

realtonyyoung merged 1 commit into
mainfrom
claude-tyoung/ai-2729-7e1147

Conversation

@realtonyyoung

Copy link
Copy Markdown
Collaborator

Closes #911 — AI-2729

What & why

ProbeVendorsConcurrently ran each vendor probe on Task.Run. A probe blocks its thread in WaitForExit for up to its whole budget, and the pool is not told about that kind of block, so once the pool is saturated the queued probes start about one thread a second and the pass serializes — the flaky assertion was observing exactly that under full-suite load. Each probe now runs on its own long-running thread, which holds the overlap regardless of pool state. The two timing tests become a rendezvous that cannot pass sequentially and a gate that cannot finish before the ceiling.

Where to look

The saturation test blocks pool threads with ManualResetEventSlim.Wait, not Task.Wait: a Task.Wait on a pool thread is compensated with a new thread, and with it the test passed against the unfixed code. The assertion quoted in the issue belongs to the sibling …OverlapsProbes… test; both tests share this cause.

Verification

Repro with a saturated pool (12 cores, 16 blocked threads): Task.Run gave maxConcurrent=1 over 4.3s and fast2=TIMEOUT; LongRunning gave maxConcurrent=5 in 303ms and fast2=fast2-v.

ProbeVendorsConcurrently_OverlapsProbes_EvenWhenThreadPoolIsSaturated fails before the fix (every probe times out at the 2s ceiling) and passes after; the class passes 5/5 repeated runs. Full Capacitor.Cli.Daemon.Tests.Unit: 3233 total, 3193 passed, 2 failed, both pre-existing (AcpMetricsTests shared-meter race, passes in isolation; codex schema pin vs locally installed codex 0.151.0). dotnet publish -c Release of the daemon: 0 IL warnings.

The pool is not told about a WaitForExit or event wait, so a saturated pool grows by about one thread a second and queued probes serialize. The test saturates it with the same silent kind of wait: a Task.Wait on a pool thread is compensated with a new thread and would prove nothing.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Sep 11, 2026

Copy link
Copy Markdown

AI-2729

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Run vendor probes on dedicated long-running threads

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

Grey Divider

AI Description

• Run each blocking vendor probe on a dedicated long-running thread.
• Preserve concurrent startup probing when the shared thread pool is saturated.
• Add deterministic saturation and timeout regression coverage.
Diagram

sequenceDiagram
    participant S as Startup
    participant C as Probe Coordinator
    participant T as Dedicated Thread
    participant V as Vendor CLI
    S->>C: Probe vendors
    loop Each vendor
        C->>T: Start long-running task
        T->>V: Run version probe
    end
    C->>C: Wait to ceiling
    alt Probe completed
        V-->>T: Version
        T-->>C: Probe result
    else Probe unfinished
        C-->>C: Map timeout
    end
    C-->>S: Vendor results
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Make the probe pipeline asynchronous
  • ➕ Avoids dedicating an operating-system thread to each blocked process.
  • ➕ Scales better if the number of vendor probes grows substantially.
  • ➖ Requires changing the generic synchronous probe seam and its callers.
  • ➖ Adds complexity around process cancellation, output draining, retries, and cleanup.
  • ➖ Creates a larger regression surface for a small, bounded startup workload.
2. Increase thread-pool minimum workers
  • ➕ Could reduce probe startup delays without changing the synchronous probe API.
  • ➖ Mutates process-wide scheduling behavior rather than isolating this workload.
  • ➖ Cannot reliably guarantee immediate overlap under arbitrary pool saturation.
  • ➖ May create unnecessary worker threads for unrelated daemon operations.

Recommendation: Keep the PR's dedicated long-running tasks. They provide a focused guarantee for a small, bounded set of inherently blocking probes without changing global thread-pool settings or rewriting process management. An asynchronous pipeline is preferable only if probe volume grows enough to make one thread per vendor material.

Files changed (3) +58 / -35

Bug fix (1) +5 / -1
DaemonRunner.csSchedule each vendor probe as a long-running task +5/-1

Schedule each vendor probe as a long-running task

• Replaces thread-pool-backed 'Task.Run' probes with 'Task.Factory.StartNew' using 'TaskCreationOptions.LongRunning' and the default scheduler. Each blocking probe therefore receives a dedicated thread while preserving existing ceiling, fault, and result behavior.

src/Capacitor.Cli.Daemon/DaemonRunner.cs

Tests (1) +43 / -34
DaemonRunnerConcurrentProbeTests.csMake probe concurrency regressions deterministic under saturation +43/-34

Make probe concurrency regressions deterministic under saturation

• Reworks the overlap test to saturate the process-wide thread pool and require every probe to rendezvous before returning. Replaces timing-based sleeping in the timeout test with an explicit gate and ensures blockers and wedged probes are released during cleanup.

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

Documentation (1) +10 / -0
CHANGES.mdDocument dedicated vendor probe threads +10/-0

Document dedicated vendor probe threads

• Explains why blocking version probes serialize when queued on a saturated thread pool and why long-running tasks preserve overlap. It also records why the regression test uses an uncompensated event wait.

docs/CHANGES.md

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

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

@realtonyyoung
realtonyyoung merged commit 49cdeaf into main Sep 11, 2026
8 checks passed
@realtonyyoung
realtonyyoung deleted the claude-tyoung/ai-2729-7e1147 branch September 11, 2026 21:51
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.

Flaky: ProbeVendorsConcurrently_SlowVendorMapsToTimeout_FastVendorsStillResolve under full-suite load

1 participant