Fix two flaky ubuntu CI tests: revoked-token keep-alive artifact + StopsInFlight collector race

[realtonyyoung]

Fixes two independent, pre-existing intermittent failures on the ubuntu CI leg. Tracked in Linear as AI-1868.

Flake 2 (production defect) — LocalPermissionBridgeTests.Revoked_reviewer_token_returns_404

RevokeReviewerToken removed the HttpListener prefix in addition to the dictionary grant. On the managed (Linux/macOS) HttpListener, a request on a reused keep-alive connection to a just-removed prefix no longer routes to HandleAsync and yields a transport-level artifact — a spurious empty-body 200 or a connection reset — instead of the intended 404.

The issue's original hypothesis (a dict revocation race between two lookup sites) does not hold: both requests hit the same general handler, and a live codex token would auto-approve both tools (200/200), a revoked one → 404/404 — so the observed r1=404 / r2=200 split can't come from the dict. Instrumentation confirmed every anomalous r2=200 had an empty body and ReviewerTokenCountForTest == 0, i.e. the request was never admitted by our code.

Fix: remove the grant from _reviewerTokens only; do not remove the listener prefix. HandleAsync already re-validates every request against the dictionary, so a dict miss is a deterministic 404, and keeping the prefix ensures the revoked-token request still routes to our handler. Prefixes are freed when the listener closes; their count is bounded by the daemon's reviewer-launch count.

• Reproduced at ~4% per request over an 8000-iteration stress loop; 0 anomalies after the fix.
• Added regression test Revoked_token_requests_404_on_reused_keepalive_connection (200 register→revoke→request cycles over one reused keep-alive client).

Flake 1 (test defect) — AgentActionServiceTests.Second_request_same_id_while_inflight_is_noop (and siblings)

The tests collected StopsInFlight pushes into a plain List<T>. AgentActionService pushes synchronously on the test thread (RequestStop) and on a threadpool thread (RunStopAsync completion) — BehaviorSubject invokes subscribers on whichever thread calls OnNext — while the test thread polls Count/[^1]. A concurrent Add during an internal array resize leaves a torn backing store → NullReferenceException on read (load-dependent, CI-only).

Fix: replaced the shared List<T> with a lock-guarded StopStateRecorder across all eight sibling tests in the file.

Verification

• LocalPermissionBridgeTests: 61/61 green.
• AgentActionServiceTests: 19/19 green across 5 consecutive runs.

[linear-code]

AI-1868

[qodo-code-review]

PR Summary by Qodo

Fix Ubuntu CI flakes: keep HttpListener prefixes on revoke + thread-safe StopsInFlight capture

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Keep revoked reviewer-token requests routed through HttpListener to return deterministic 404s.
• Add keep-alive regression coverage for revoked-token behavior on Linux/macOS HttpListener.
• Make StopsInFlight test observers thread-safe to eliminate concurrent List resize/read races.

Diagram

graph TD
  CI["Ubuntu CI"] --> LPT["LocalPermissionBridgeTests"] --> LPB["LocalPermissionBridge"]
  LPB --> HL["HttpListener prefixes"]
  LPB --> RT["Reviewer token map"]
  CI --> AAT["AgentActionServiceTests"] --> AAS["AgentActionService"] --> SSR["StopStateRecorder"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Remove prefix but drain/close connections on revoke

• ➕ Preserves original intent of freeing prefixes early
• ➕ Potentially reduces long-lived prefix set in very long daemon sessions
• ➖ Hard to do safely/portably; keep-alive reuse makes correctness subtle
• ➖ Higher risk of introducing listener downtime or new race conditions

2. Disable keep-alive in tests (new HttpClient per request)

• ➕ May avoid the specific managed-HttpListener artifact in CI
• ➖ Masks a production behavior defect rather than fixing it
• ➖ Does not guarantee determinism across platforms/runtimes

3. Use a concurrent collection instead of lock in StopStateRecorder

• ➕ Avoids explicit locks; simple append semantics
• ➖ Tests also need indexed access (Count/[^1]); most concurrent collections don’t provide stable indexing
• ➖ Would still require additional synchronization for consistent snapshot reads

Recommendation: Keep the HttpListener prefix registered and treat the token dictionary as the single source of truth for revocation, as implemented here; it fixes the production symptom (transport-level artifacts on keep-alive) while preserving deterministic 404 behavior. For the test flake, the lock-guarded StopStateRecorder is the most direct fix because the tests require consistent Count and index-based reads, not just append-only recording.

Files changed (3) +57 / -12

Bug fix (1) +10 / -4

LocalPermissionBridge.csMake reviewer token revocation dictionary-only to avoid keep-alive routing artifacts +10/-4

Make reviewer token revocation dictionary-only to avoid keep-alive routing artifacts

• Changes RevokeReviewerToken to remove only the token grant from _reviewerTokens and to stop removing the HttpListener prefix. This ensures revoked-token requests still reach HandleAsync and deterministically return 404 instead of sometimes producing a transport-level empty-body 200/connection reset on managed (Linux/macOS) HttpListener keep-alive connections.

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

Tests (2) +47 / -8

AgentActionServiceTests.csEliminate StopsInFlight test race with a thread-safe recorder +22/-8

Eliminate StopsInFlight test race with a thread-safe recorder

• Replaces a shared List<IReadOnlySet<string>> used by multiple tests with a lock-protected StopStateRecorder. This prevents intermittent NullReferenceExceptions caused by concurrent OnNext/Add operations (from different threads) while tests read Count/[^1].

test/Capacitor.App.Tests.Unit/AgentActionServiceTests.cs

LocalPermissionBridgeTests.csAdd keep-alive regression test for revoked reviewer token returning 404 +25/-0

Add keep-alive regression test for revoked reviewer token returning 404

• Adds Revoked_token_requests_404_on_reused_keepalive_connection, which reuses a single HttpClient across many register→revoke→request cycles. This pins the Linux/macOS managed HttpListener behavior so revoked tokens reliably produce 404s even on reused keep-alive connections.

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

[qodo-code-review]

Code Review by Qodo

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

1. Prefixes retained after revoke ✓ Resolved 🐞 Bug ➹ Performance

Description

RevokeReviewerToken no longer removes the per-token HttpListener prefix, so every
RegisterReviewerToken permanently grows HttpListener.Prefixes until the daemon shuts down. In a
long-lived daemon that launches many review-flow reviewers, this can steadily increase
memory/resource usage and potentially degrade request routing/registration overhead over time.

Code

src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[R255-257]

+        // reaches HandleAsync, where the dict miss produces the intended 404. The prefixes are freed
+        // when the listener closes; their count is bounded by the daemon's reviewer-launch count.
+        _reviewerTokens.TryRemove(token, out _);

Evidence

The codebase now always adds a unique prefix per reviewer token, but revocation no longer removes
it; the service is designed to remain running while the daemon spawns many agents/reviewers, so
prefixes can accumulate over the daemon lifetime.

src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[14-25]
src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[209-240]
src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[242-258]
src/Capacitor.Cli.Daemon/Services/AgentOrchestrator.cs[1824-1863]

Agent prompt

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

## Issue description
`RevokeReviewerToken` now removes only `_reviewerTokens` entries and intentionally keeps the `HttpListener` prefix. This prevents the keep-alive artifact but also means prefixes accumulate for the entire daemon lifetime.

## Issue Context
- `RegisterReviewerToken` adds a new listener prefix per reviewer token.
- `RevokeReviewerToken` no longer removes that prefix.
- The bridge is a hosted service used across many spawned agents/reviewers in a daemon lifetime.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[14-25]
- src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[216-240]
- src/Capacitor.Cli.Daemon/Services/LocalPermissionBridge.cs[242-258]

### What to implement
Choose one bounded strategy that preserves the keep-alive correctness fix:
1) **Prefix count guardrails + observability**: track current `Prefixes.Count` (or a separate counter of minted reviewer prefixes) and log a warning / emit a metric once it crosses a threshold so growth is diagnosable.
2) **Prefix reuse design**: if feasible, reduce per-token prefixes by using a smaller set of stable prefixes and dispatching by token inside `HandleAsync` (while keeping the security invariants you need).
3) **Safe prefix cleanup**: implement deferred cleanup (e.g., keep revoked prefixes for a grace period, and/or force `Connection: close` on revoked-token responses) and remove prefixes only when safe.

Include a small regression/behavior test if you implement anything beyond logging/metrics.

ⓘ 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 ↗