Skip to content

Stabilize three flaky cross-platform CI tests - #924

Merged
realtonyyoung merged 3 commits into
mainfrom
tonyyoung/flaky-ci-test-fixes
Sep 14, 2026
Merged

realtonyyoung merged 3 commits into
mainfrom
tonyyoung/flaky-ci-test-fixes

Conversation

@realtonyyoung

Copy link
Copy Markdown
Collaborator

Closes #916 — AI-2732
Closes #917 — AI-2733
Closes #918 — AI-2734

What & why

Three cross-platform CI flakes, each a test-only timing race, fixed together:

  • PostWithRetry (macOS): the retry test stopped a WireMock server to free a port then restarted one on it. A neighbour could grab the freed port, or a request could land after the listener opened but before its mapping registered (a final 404). The retry path only reacts to a transport fault, so the fault is now injected through a scripted HttpMessageHandler — no port, no server, no race.
  • WorktreeManager snapshot (Windows): the test killed a cmd.exe whose cwd was the snapshot and deleted the snapshot immediately. Process.Kill returns before the child releases its directory handle, so the delete raced the exit. It now awaits WaitForExitAsync first.
  • Avalonia headless session (macOS): Dispatcher.UIThread binds to whichever thread first touches it; a parallel test binding it off the session thread made the headless app build fail VerifyAccess. A [Before(Assembly)] hook now builds the app on the session thread first, claiming the binding.

Verification

  • PostWithRetryTests — 2/2 passed
  • WorktreeManagerTests.BorrowedSnapshot_RefreshPreservesRunningExecutionDirectory — 1/1 passed
  • full Capacitor.App.Tests.Unit suite — 1921/1921 passed

🤖 Generated with Claude Code

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Stabilize three flaky cross-platform CI tests

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Replace port-based retry timing with deterministic transport-fault injection.
• Await process termination before deleting Windows worktree snapshots.
• Pre-bind Avalonia's dispatcher to the headless session thread before tests.
Diagram

graph TD
  CI["Cross-platform CI"] --> Retry["Retry test"] --> Handler["Scripted handler"]
  CI --> Snapshot["Snapshot test"] --> Exit["Process exit"]
  CI --> Avalonia["Avalonia tests"] --> Dispatcher["Session dispatcher"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Synchronize the real HTTP server
  • ➕ Preserves an end-to-end network path
  • ➕ Can validate actual socket behavior
  • ➖ Still depends on port allocation and listener readiness
  • ➖ Adds latency and more failure modes than the behavior requires
2. Serialize all Avalonia tests
  • ➕ Prevents competing tests from claiming the dispatcher first
  • ➕ Requires little initialization logic
  • ➖ Reduces test-suite parallelism
  • ➖ Treats the symptom rather than establishing deterministic process-global ownership

Recommendation: Keep the PR's targeted approach. The retry test only needs to verify handling of a transport exception, so a scripted handler is more precise than socket orchestration; awaiting process exit is the required Windows cleanup boundary; and assembly-level dispatcher initialization preserves parallelism while deterministically assigning UI-thread ownership.

Files changed (3) +36 / -23

Tests (3) +36 / -23
AvaloniaSession.csClaim Avalonia UI-thread ownership before assembly tests +8/-0

Claim Avalonia UI-thread ownership before assembly tests

• Adds an assembly-level setup hook that dispatches through the shared headless session before any test runs. This binds Dispatcher.UIThread to the session worker and prevents parallel tests from claiming it on a thread-pool thread.

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

WorktreeManagerTests.csWait for snapshot holder termination before cleanup +6/-1

Wait for snapshot holder termination before cleanup

• After killing the child process that holds the snapshot as its working directory, the test now awaits process exit. This ensures Windows releases the directory handle before snapshot deletion begins.

test/Capacitor.Cli.Daemon.Tests.Unit/Services/WorktreeManagerTests.cs

HookForwardingTests.csReplace retry server race with scripted HTTP responses +22/-22

Replace retry server race with scripted HTTP responses

• Reworks the connection-retry test to use a custom HttpMessageHandler that throws once and then returns success. This removes port reuse, server startup, and route-registration races while directly exercising transport-failure retry behavior.

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

@qodo-code-review

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

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Remediation recommended

1. Three commits omit issue links 📘 Rule violation ⚙ Maintainability
Description
The subjects Inject the transport fault in the retry test instead of a real server, `Wait for the
killed holder to exit before deleting its snapshot, and Claim the Avalonia UI thread before any
test in the assembly runs omit the required trailing (#<digits>)` reference. All three commits
reach the branch history without an explicit GitHub issue association, despite the PR description
identifying corresponding issues #916, #917, and #918.
Code

test/Capacitor.Cli.Tests.Unit/HookForwardingTests.cs[R31-34]

+        // The retry path fires on a transport fault, so inject one: a first attempt that faults like
+        // a refused connection, then a success. Driving this through a real server on a restarted
+        // port is racy — a neighbour can grab the freed port, or a request can land after the
+        // listener opens but before its mapping is registered, which returns a final 404.
Relevance

●●● Strong

Explicit commit-subject rule; accepted authoring guidance requires trailing GitHub issue references.

PR-#659
PR-#205

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 2897961 requires each commit subject to end with exactly one GitHub issue
reference, while all three supplied commit subjects end without one. The pointer identifies a
representative change belonging to one of the affected commits because commit-message metadata has
no line location of its own.

Rule 2897961: Enforce single-clause imperative commit subject with GitHub issue reference and 80-char limit


Grey Divider

Context sources
✅ Compliance rules (platform): 64 rules
✅ Cross-repo context — repo relationships
Review mode: ⚖️ Balanced: This is a focused test-only change, but it alters async process cleanup, UI-thread initialization, and HTTP retry simulation across three independent paths, warranting a careful single-pass review.

Grey Divider

Tip of the day
💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment on lines +31 to +34
// The retry path fires on a transport fault, so inject one: a first attempt that faults like
// a refused connection, then a success. Driving this through a real server on a restarted
// port is racy — a neighbour can grab the freed port, or a request can land after the
// listener opens but before its mapping is registered, which returns a final 404.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Three commits omit issue links 📘 Rule violation ⚙ Maintainability

The subjects Inject the transport fault in the retry test instead of a real server, `Wait for the
killed holder to exit before deleting its snapshot, and Claim the Avalonia UI thread before any
test in the assembly runs omit the required trailing (#<digits>)` reference. All three commits
reach the branch history without an explicit GitHub issue association, despite the PR description
identifying corresponding issues #916, #917, and #918.

@realtonyyoung
realtonyyoung force-pushed the tonyyoung/flaky-ci-test-fixes branch from 0812031 to e989f30 Compare September 13, 2026 14:23
realtonyyoung and others added 3 commits September 13, 2026 10:38
…#916)

The retry path fires on a transport fault. A real server on a restarted port
raced two ways: a neighbour grabbing the freed port, or a request landing after
the listener opened but before its mapping registered (a final 404).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Process.Kill returns before a process releases its working-directory handle, so
the test launches the long-lived cwd owner directly (no wrapper child that could
outlive the wait) and awaits its exit before deleting the snapshot on Windows.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Dispatcher.UIThread binds to whichever thread first touches it. A parallel test
on a thread-pool thread binding it elsewhere makes the headless app build fail
VerifyAccess in the compositor ctor, so the session must claim it first.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@realtonyyoung
realtonyyoung force-pushed the tonyyoung/flaky-ci-test-fixes branch from e989f30 to 3039d5e Compare September 13, 2026 14:38
@realtonyyoung
realtonyyoung merged commit a3cb8e1 into main Sep 14, 2026
8 checks passed
@realtonyyoung
realtonyyoung deleted the tonyyoung/flaky-ci-test-fixes branch September 14, 2026 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant