[AI-1439] Await import background tasks; de-flake retry timing tests

[realtonyyoung]

Fixes two independent sources of kcap-cli unit-test flakiness surfaced while working #335. Linear: AI-1439.

Part 1 — production bug: kcap import never awaited background title/summary work

ImportCommand.HandleImport computed the background-phase guard inverted:

var ranBackground = backgroundTasks.IsEmpty;   // missing a '!'

The one boolean gates (a) the if (…) block that await Task.WhenAll(backgroundTasks), and (b) the two Done-summary guards that print the Titles/Summaries rows. Because it was inverted, an import that has background work (title generation, or summaries under --generate-summaries):

• skipped the await block → the method returned while a title task could still be running → the shared using var concurrencyLimit = new SemaphoreSlim(3) was disposed → the straggler's finally { concurrencyLimit.Release(); } threw ObjectDisposedException (the intermittent unit failure — rare only because tasks usually finish before the method returns);
• dropped the Titles/Summaries rows from the Done summary.

Introduced in fa2f013 (2026-04-26); #335's new tests merely exercised HandleImport enough to surface it.

Fix: !backgroundTasks.IsEmpty, renamed to hasBackgroundWork and extracted into a named HadBackgroundWork seam so the decision is unit-testable and can't silently re-invert — mirroring the existing ComputePerSourceFinalCounts "extract-and-test-the-helper-directly" convention in this file. The two RanBackground: false early-exit constructions stay correct.

This changes kcap import's observable Done-summary output (the Titles/Summaries rows now appear when there was background work) — intended.

Part 2 — parallel-load timing flakes

• SendWithRetry timing tests (the genuine remaining flaky-failure vector): widen the wall-clock margins while preserving each invariant.
  • …_after_budget_exhausted: totalTimeout 1200 ms → 5000 ms, so a scheduler stall during attempt 1 can't consume the whole budget before attempt 2 begins (attempts > 1 is itself condition-based).
  • …_enforces_total_timeout_…: elapsed upper bound 2000 ms → 5000 ms (still ≪ the 30 s per-attempt cap it guards against).
  • These verify timeout enforcement, so they're inherently wall-clock; generous bounds is the correct hardening.
• Port contention (LocalPermissionBridgeTests, AgentOrchestratorReviewerTokenTests, AgentOrchestratorVendorTests): already fixed at the production level — LocalPermissionBridge binds an OS-assigned ephemeral loopback port behind a process-global claim set + 15-attempt transient-bind retry (Fix flaky LocalPermissionBridge bind under parallel tests (jittered backoff) #340), and the bridge tests carry [NotInParallel] grouping. No test change needed.
• AcpTranscriptAggregationTests / AgentOrchestratorVendorTests Task.Delay(200/100): these are "settle, then assert a negative" waits — a short wait risks a false pass (missed regression), not a flaky fail — so they're not the parallel-failure vector and are left as-is.

Tests

• New ImportBackgroundWorkTests: HadBackgroundWork empty → false, non-empty → true.
• ImportDisplayGridTests: WriteDoneGrid renders the Titles/Summaries rows iff RanBackground.
• Widened HttpClientExtensionsRetryTests margins.

Verified locally: full Import namespace (234), SetupCommandTests/ImportVisibilityTests (26, drive HandleImport), and HttpClientExtensionsRetryTests (5) all pass; build clean; check-linear-ids.sh passes.

Out of scope / follow-ups

• Deeper process-global refactor of import background-task lifetime → AI-741.
• AI-1438 is a textual duplicate of AI-1439 — close as dup.

🤖 Generated with Claude Code

[linear-code]

AI-1439

[qodo-code-review]

PR Summary by Qodo

Await import background tasks and harden SendWithRetry timing tests

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Fix inverted background-work guard so kcap import awaits title/summary tasks.
• Restore Done-summary Titles/Summaries rows when background generation ran.
• Widen wall-clock bounds in retry timeout tests to reduce CI scheduler flakiness.

Diagram

graph TD
  A["ImportCommand.HandleImport"] --> B["backgroundTasks (ConcurrentBag<Task>)"] --> C["HadBackgroundWork()"] --> D{"Has background work?"}
  D -->|"yes"| E["await Task.WhenAll()"] --> F["FinalCounts.RanBackground"] --> G["ImportDisplay.WriteDoneGrid"]
  H["ImportBackgroundWorkTests"] --> C
  I["ImportDisplayGridTests"] --> G
  J["HttpClientExtensionsRetryTests"] --> K["HttpClientExtensions.SendWithRetryAsync"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Minimal fix (flip boolean only)

• ➕ Smallest code change
• ➕ No additional public/internal surface area
• ➖ Easy to accidentally re-introduce the inversion later
• ➖ Harder to unit-test the decision directly without exercising full import flow

2. Track background work via explicit counters/flags

• ➕ Avoids relying on concurrent collection state for semantics
• ➕ Can distinguish titles vs summaries more explicitly
• ➖ More moving parts and state to maintain
• ➖ Still requires tests to ensure the await/report guards remain correct

3. Always await Task.WhenAll(backgroundTasks) (even if empty)

• ➕ Eliminates guard logic entirely
• ➕ Makes awaiting unconditional and simpler to reason about
• ➖ Still needs separate logic for whether to render Titles/Summaries rows
• ➖ May obscure intent that most imports do not schedule background work

Recommendation: The chosen approach (named helper seam + unit tests) is a good balance: it fixes the production bug, makes the guard decision explicit, and directly pins the previously fragile negation with focused tests. The timing-test relaxations are appropriate for wall-clock assertions in CI and keep the key invariants intact.

Files changed (4) +80 / -12

Bug fix (1) +12 / -3

ImportCommand.csFix background-work guard and await/report titles & summaries correctly +12/-3

Fix background-work guard and await/report titles & summaries correctly

• Introduces 'HadBackgroundWork()' to correctly detect scheduled background tasks ('!backgroundTasks.IsEmpty'). Uses the fixed guard to ensure 'Task.WhenAll(backgroundTasks)' is awaited and to propagate the correct 'RanBackground' flag into the Done summary, preventing premature 'SemaphoreSlim' disposal and restoring Titles/Summaries rows.

src/Capacitor.Cli/Commands/ImportCommand.cs

Tests (3) +68 / -9

HttpClientExtensionsRetryTests.csDe-flake SendWithRetry timeout tests by widening time budgets +9/-4

De-flake SendWithRetry timeout tests by widening time budgets

• Increases the total timeout budget and elapsed-time upper bound in two timing-based retry tests to tolerate CI scheduler stalls. Comments clarify that the assertions remain about timeout-governed behavior, not tight wall-clock precision.

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

ImportBackgroundWorkTests.csAdd regression tests for background-work decision seam +25/-0

Add regression tests for background-work decision seam

• Adds unit tests asserting 'HadBackgroundWork()' returns false for an empty bag and true when tasks were scheduled. This directly guards against reintroducing the missing-negation bug.

test/Capacitor.Cli.Tests.Unit/Import/ImportBackgroundWorkTests.cs

ImportDisplayGridTests.csTest Done grid Titles/Summaries rows are conditional on RanBackground +34/-5

Test Done grid Titles/Summaries rows are conditional on RanBackground

• Adds Done-grid tests ensuring Titles/Summaries rows render when 'RanBackground' is true and are omitted otherwise. Refactors the 'MakeFinal' helper to parameterize 'RanBackground', 'RequestedSummaries', and generated counts.

test/Capacitor.Cli.Tests.Unit/Import/ImportDisplayGridTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials

1. Slow retry timeout test ✓ Resolved 🐞 Bug ➹ Performance

Description

SendWithRetry_converts_per_attempt_timeout_to_HttpRequestException_after_budget_exhausted now sets
totalTimeout to 5s while the send delegate never completes, so SendWithRetryAsync will keep
retrying until the full budget is exhausted, making the test take several seconds per run.

Code

test/Capacitor.Cli.Tests.Unit/HttpClientExtensionsRetryTests.cs[R22-31]

+        // Generous total budget relative to the per-attempt cap: even if a loaded CI runner
+        // stalls the scheduler during the first attempt, plenty of budget remains for a second
+        // one, so the `attempts > 1` assertion stays robust (it is itself condition-based —
+        // it counts real attempts, not elapsed time).
        var ex = await Assert.That(async () => await HttpClientExtensions.SendWithRetryAsync(
                    Send,
-                    totalTimeout: TimeSpan.FromMilliseconds(1_200),
+                    totalTimeout: TimeSpan.FromMilliseconds(5_000),
                    perAttemptTimeout: TimeSpan.FromMilliseconds(150),
                    ct: CancellationToken.None
                )

Evidence

The test now passes totalTimeout: 5000ms to a retry loop where the request always times out;
SendWithRetryAsync only throws once the overall budget is exhausted, so the wall-clock duration
scales with totalTimeout.

test/Capacitor.Cli.Tests.Unit/HttpClientExtensionsRetryTests.cs[16-37]
src/Capacitor.Cli.Core/HttpClientExtensions.cs[278-345]

Agent prompt

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

### Issue description
A unit test increased `totalTimeout` to 5000ms while using a `Send` delegate that blocks indefinitely until cancelled. Because `SendWithRetryAsync` retries until the total budget is exhausted, this makes the test take close to the full budget, slowing the unit test suite/CI.

### Issue Context
The retry implementation loops until `Stopwatch.Elapsed` reaches `totalTimeout`, so tests that intentionally never succeed will run for ~`totalTimeout`.

### Fix Focus Areas
- test/Capacitor.Cli.Tests.Unit/HttpClientExtensionsRetryTests.cs[22-37]
- src/Capacitor.Cli.Core/HttpClientExtensions.cs[278-345]

### Suggested fix
- Prefer a smaller-but-still-robust `totalTimeout` (e.g., 2000–2500ms) that comfortably allows at least 2 attempts given the 150ms per-attempt timeout plus backoff.
- Alternatively, redesign the test to avoid waiting for real-time budget exhaustion (e.g., add a seam to inject backoff/clock or use a send delegate that deterministically fails after N attempts while still exercising the timeout→HttpRequestException conversion).

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

---

2. HadBackgroundWork comment too verbose ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The new/modified comments—both the XML doc on HadBackgroundWork and several test comments—are
overly long and include historical bug narratives/CI scheduling rationale rather than concise intent
or contract. This adds noise, reduces maintainability, and violates the guideline to keep comments
concise and prefer self-explanatory code.

Code

src/Capacitor.Cli/Commands/ImportCommand.cs[R469-476]

+    /// <summary>
+    /// True when the import scheduled background title/summary work that the Done phase must
+    /// await (and whose completion the Titles/Summaries rows report). Extracted as a named seam
+    /// so the await-guard decision is unit-testable and can't silently re-invert: the whole class
+    /// of "background tasks never awaited → SemaphoreSlim disposed mid-flight → ObjectDisposedException"
+    /// bug was a single missing negation here.
+    /// </summary>
+    internal static bool HadBackgroundWork(ConcurrentBag<Task> backgroundTasks) => !backgroundTasks.IsEmpty;

Evidence

PR Compliance ID 5 requires that new or modified comments remain concise; however, the cited
HadBackgroundWork XML documentation spans multiple lines detailing a historical failure chain and
root-cause story instead of a minimal description of intent, and the referenced test files similarly
add multi-line narrative comments about bug history and CI timing/scheduler behavior rather than
relying on test naming and structure to convey intent.

CLAUDE.md: Keep Comments Concise; Prefer Self-Explanatory Code Over Verbose Commentary
src/Capacitor.Cli/Commands/ImportCommand.cs[469-476]
test/Capacitor.Cli.Tests.Unit/Import/ImportBackgroundWorkTests.cs[6-12]
test/Capacitor.Cli.Tests.Unit/HttpClientExtensionsRetryTests.cs[22-25]
test/Capacitor.Cli.Tests.Unit/HttpClientExtensionsRetryTests.cs[62-65]
test/Capacitor.Cli.Tests.Unit/Import/ImportDisplayGridTests.cs[74-76]

Agent prompt

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

## Issue description
New/modified comments are too verbose: the `HadBackgroundWork` XML doc and several test comments contain long historical narratives (bug stories, CI scheduling/timing rationale, failure chains) rather than concise intent/contract, which violates the repo guidance to keep comments concise.

## Issue Context
Per PR Compliance ID 5 and repo compliance guidance, comments should be short and focused on non-obvious intent; extended historical explanations are better captured in PR/issue context and tests should generally communicate intent through names and structure.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/ImportCommand.cs[469-476]
- test/Capacitor.Cli.Tests.Unit/Import/ImportBackgroundWorkTests.cs[6-12]
- test/Capacitor.Cli.Tests.Unit/HttpClientExtensionsRetryTests.cs[22-25]
- test/Capacitor.Cli.Tests.Unit/HttpClientExtensionsRetryTests.cs[62-65]
- test/Capacitor.Cli.Tests.Unit/Import/ImportDisplayGridTests.cs[74-76]

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

---

[realtonyyoung]

Addressed both Qodo findings in fd4d109:

1. Verbose comments — trimmed the HadBackgroundWork XML doc and the ImportBackgroundWork/retry/display-grid test comments to concise intent (the bug narrative lives in the PR/Linear, not the code).
2. Slow retry test — the budget-exhausted test no longer uses a 5s totalTimeout (which the never-completing send ran to the full budget). It now uses a small per-attempt cap (50 ms) under a modest 1500 ms total, so retries accrue fast — attempts > 1 stays robust and the test finishes in ~1.5 s (test 2's elapsed-bound widening, which costs no runtime, is kept).

Also for the record: the first ubuntu run failed on HandleStopAgent_unknown_id_reaps_by_pid_record_where_env_is_readable — an unrelated native UnixSpawnerThread PID-reaping test that FailFasts the process (windows passed the identical code). Not touched by this PR; a separate parallel-load flake candidate.