Share the shared-config-dir cleanup instead of hardening one class at a time

[realtonyyoung]

Fixes Linear AI-1743 (GitHub #466). Recurrence of AI-1620's exception, one class over.

Symptom

Build and test (windows-latest) goes red with a single failure, in a hook rather than an assertion:

failed SaveAsync_leaves_no_temp_residue (0ms)
  BeforeTestException: BeforeTest hook failed: The process cannot access the file
  'C:\...\kcap-repopathstore-tests-952701da\config.json' because it is being used by another process.

Windows only; ubuntu green on the identical commit.

The premise correction that shapes the fix

The issue proposed either per-class config directories or a shared NotInParallel key. Neither can work, and it's worth stating before the fix: .github/workflows/ci.yml:66 runs this suite with --maximum-parallel-tests 1 across the whole matrix, Windows included, and line 56 says so outright.

So there is no concurrency to serialise and no race to isolate. Which means a lock at [Before(Test)] time proves a handle outlived its owning test — an undisposed stream awaiting finalization, or a child process (watcher/daemon) not yet reaped. Both observed properties follow from that and only from it:

• Windows-only — Windows refuses to delete a file with an open handle; Unix unlinks regardless, so the identical leak is invisible there.
• Intermittent — whether the holder has released by the time the next hook runs is timing, not ordering. Hence passing on rerun.

A shared key would have been worse than useless: it would look like a fix, the flake would recur at a lower rate, and the leak would still be there.

The fix: own the resource, not the class

A retrying cleanup for exactly this already existed — privately, inside CrossProcessRefreshTests, from the previous round. That round hardened the one class failing at the time, and the identical exception then appeared in the next class touching the same files. There are 12+ candidates.

So the retry now lives in SharedConfigDirCleanup, and both classes use it. TokenStoreProfileTests' hook went from bare deletes to the shared helper; CrossProcessRefreshTests delegates and drops its private copy.

Nothing about the mechanism is new — it is the already-reviewed logic from #418, moved to where it covers every user of the shared files.

Retry, then throw — never swallow

Kept from AI-1620 deliberately. These tests assert on token and profile state, and a stale tokens directory already satisfies "a peer already refreshed it". Swallowing a persistent lock would convert a rerun-costing flake into a false pass that hides a real regression.

One addition: a GC pass that doubles as a discriminator

After the first failed attempt only:

GC.Collect();
GC.WaitForPendingFinalizers();

This can only release an in-process undisposed stream, so its effect distinguishes the two candidate causes. If the retry budget stops being exhausted, the holder was an unreferenced stream awaiting finalization; if failures continue, it is a live holder and the owner is still running. The thrown message records that the pass was attempted, so the next occurrence carries that fact rather than needing another investigation.

Verification — and what it cannot show

run	result
TokenStoreProfileTests	20/20
CrossProcessRefreshTests	4/4

Neither establishes the fix. The failure is Windows-only and I am on macOS, where the delete never fails; and it is intermittent, so even a green Windows CI run is weak evidence. What is verified is that behaviour is unchanged where it can be observed, and that the retry-and-throw contract is the one the earlier round concluded was correct.

🤖 Generated with Claude Code

[linear-code]

AI-1743

[qodo-code-review]

PR Summary by Qodo

Share KCAP shared-config-dir cleanup retry across unit tests

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Centralize Windows-safe shared KCAP_CONFIG_DIR cleanup with bounded retry and clear errors.
• Update token/profile tests to use shared cleanup, removing duplicated per-class retry logic.
• Add one-time GC/finalizer pass after first failure to distinguish leaked streams vs live holders.

Diagram

graph TD
  A["Before(Test) hooks"] --> B["SharedConfigDirCleanup"] --> C[("KCAP_CONFIG_DIR")]
  B --> D["AppConfig"]
  B --> E["GC + Finalizers"]
  A --> F["Token/profile tests"]

  subgraph Legend
    direction LR
    _hook["Hook/Tests"] ~~~ _helper["Helper"] ~~~ _fs[("Filesystem/Dir")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Fix the underlying handle leak (streams/processes) instead of retrying deletes

• ➕ Eliminates the root cause rather than mitigating symptoms
• ➕ Reduces flakiness and test runtime spent in retries
• ➕ Improves confidence that tests are not leaving background processes running
• ➖ May require deeper investigation across multiple tests/components
• ➖ Could be time-consuming to reproduce locally and attribute ownership
• ➖ Might involve production code or external-process lifecycle changes

2. Make config dir per-test (remove static capture of PathHelpers.ConfigDir)

• ➕ Avoids cross-test interference entirely by construction
• ➕ Simplifies cleanup logic (no shared directory to contend over)
• ➖ Potentially invasive refactor (static readonly capture, global setup assumptions)
• ➖ Higher risk of breaking existing tests relying on assembly-level config
• ➖ More moving parts than a targeted mitigation

3. Use OS-assisted file handle diagnostics (e.g., log open handles / owner PID on failure)

• ➕ Improves debuggability when retries exhaust
• ➕ Can help pinpoint whether a child process is the holder
• ➖ Platform-specific implementation complexity (especially in CI)
• ➖ May require elevated permissions or additional tooling not available on runners

Recommendation: The PR’s approach is the right near-term fix: it hardens the shared resource (KCAP_CONFIG_DIR artifacts) once, prevents class-by-class whack-a-mole, and preserves the critical behavior of retry-then-throw (avoiding false passes). The added one-time GC/finalizer pass is a reasonable, low-cost discriminator for leaked in-process streams. Longer-term, consider pursuing root-cause handle ownership if flakes persist after this change, but that work is larger and orthogonal to stabilizing CI now.

Files changed (3) +103 / -53

Bug fix (2) +100 / -12

SharedConfigDirCleanup.csAdd shared bounded-retry cleanup for token/profile test artifacts +93/-0

Add shared bounded-retry cleanup for token/profile test artifacts

• Introduces a centralized helper to delete shared KCAP_CONFIG_DIR artifacts with bounded retry for transient Windows sharing violations, and throws if cleanup cannot be proven. Adds a one-time GC + finalizer pass after the first failure to help distinguish leaked streams from live holders, and provides a convenience method for clearing token/profile state plus resetting AppConfig resolved profile state.

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

TokenStoreProfileTests.csUse SharedConfigDirCleanup in [Before(Test)] hook instead of bare deletes +7/-12

Use SharedConfigDirCleanup in [Before(Test)] hook instead of bare deletes

• Replaces File.Delete/Directory.Delete cleanup with SharedConfigDirCleanup.ClearTokenAndProfileState to avoid Windows-only flake failures caused by transient open handles. Preserves semantics by still failing loudly if cleanup cannot complete rather than running against leftover state.

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

Refactor (1) +3 / -41

CrossProcessRefreshTests.csDelegate shared KCAP_CONFIG_DIR cleanup to SharedConfigDirCleanup +3/-41

Delegate shared KCAP_CONFIG_DIR cleanup to SharedConfigDirCleanup

• Removes the class-private bounded-retry delete helper and replaces direct cleanup calls with SharedConfigDirCleanup.ClearTokenAndProfileState. Keeps the same test intent (clean shared token/profile artifacts) while ensuring the logic is shared across all users of the resource.

test/Capacitor.Cli.Tests.Unit/CrossProcessRefreshTests.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. Unbounded finalizer wait ✓ Resolved 🐞 Bug ☼ Reliability

Description

SharedConfigDirCleanup.ClearWithRetry calls GC.WaitForPendingFinalizers() from a [Before(Test)]
cleanup path when the first delete attempt fails, which can stall the entire test run if any
finalizer blocks or runs long. This turns an intermittent cleanup failure into a potential hang
rather than a bounded retry + clear failure.

Code

test/Capacitor.Cli.Tests.Unit/SharedConfigDirCleanup.cs[R60-62]

+                GC.Collect();
+                GC.WaitForPendingFinalizers();
+            }

Evidence

The helper unconditionally performs a full GC and waits for all pending finalizers (no timeout)
after the first caught delete exception, and this code is reached from [Before(Test)] cleanup
hooks in multiple test classes, so a stall here blocks the whole test run.

test/Capacitor.Cli.Tests.Unit/SharedConfigDirCleanup.cs[54-65]
test/Capacitor.Cli.Tests.Unit/TokenStoreProfileTests.cs[23-32]
test/Capacitor.Cli.Tests.Unit/CrossProcessRefreshTests.cs[33-41]

Agent prompt

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

### Issue description
`SharedConfigDirCleanup.ClearWithRetry` performs `GC.Collect()` + `GC.WaitForPendingFinalizers()` after the first failed delete attempt. `GC.WaitForPendingFinalizers()` is unbounded and can hang the test process if any finalizer blocks.

### Issue Context
This helper is invoked from `[Before(Test)]` hooks (e.g., `TokenStoreProfileTests`, `CrossProcessRefreshTests`). If the finalizer wait stalls, tests can stop progressing before even reaching the bounded retry/throw behavior.

### Fix Focus Areas
- test/Capacitor.Cli.Tests.Unit/SharedConfigDirCleanup.cs[54-65]

### Suggested change
Replace the unbounded `GC.WaitForPendingFinalizers()` with a bounded wait (e.g., run it on a worker task and `Wait(timeout)`), and if it times out, continue retrying/degrading gracefully (still throw after the normal retry budget if the target remains).

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

---

2. Verbose Cleanup() comment block ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

TokenStoreProfileTests.Cleanup() and SharedConfigDirCleanup include long, narrative
comment/documentation blocks that restate historical rationale and platform behavior rather than
concisely documenting intent or contract. This adds noise to the test code, reduces readability, and
increases ongoing maintenance cost.

Code

test/Capacitor.Cli.Tests.Unit/TokenStoreProfileTests.cs[R25-28]

+        // Bare File.Delete/Directory.Delete here is what made this hook fail on Windows: the shared
+        // KCAP_CONFIG_DIR artefacts can still be held when this runs, and Windows treats that as a
+        // hard sharing violation, which from a Before hook fails the test before it starts. The
+        // shared helper retries the transient window and then throws with a named cause rather than

Evidence

PR Compliance ID 9 requires comments to be concise and not overly verbose. The cited sections in
TokenStoreProfileTests.Cleanup() and the XML doc block in SharedConfigDirCleanup span multiple
lines of explanatory, rationale-heavy context (historical failures/platform behavior) instead of a
brief statement of purpose/contract, demonstrating the kind of verbosity the guideline aims to
avoid.

CLAUDE.md: Prefer self-explanatory code over verbose comments
test/Capacitor.Cli.Tests.Unit/TokenStoreProfileTests.cs[25-31]
test/Capacitor.Cli.Tests.Unit/SharedConfigDirCleanup.cs[3-23]

Agent prompt

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

## Issue description
`TokenStoreProfileTests.Cleanup()` and `SharedConfigDirCleanup` contain overly verbose, narrative-style comments/XML documentation that should be reduced to short intent/contract notes, leaving historical rationale and platform-behavior exposition to PR or ticket context.

## Issue Context
Compliance prefers self-explanatory code with concise comments; long explanatory blocks are harder to scan in tests, distract from the logic, and are costly to maintain because rationale/history can become stale.

## Fix Focus Areas
- test/Capacitor.Cli.Tests.Unit/TokenStoreProfileTests.cs[25-31]
- test/Capacitor.Cli.Tests.Unit/SharedConfigDirCleanup.cs[3-24]

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

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}