fix(firmware): cancel hard-timeout token before throwing TimeoutException (fixes flaky test + ordering race)

[tylerkron]

Summary

WifiBridgeActivator.RunWithHardTimeoutAsync had a subtle ordering gap that was intermittently failing CI (the RunWithHardTimeoutAsync_HardTimeoutElapses_StopsWorkerBeforeLateStep test) and, in production, could let an abandoned worker run one extra state-changing step after a timeout.

Root cause

Two independent timers of the same duration:

1. Task.Delay(hardTimeoutMs) in the WhenAny race — decides when to throw TimeoutException.
2. hardTimeoutCts's own timer — decides when the worker's linked token is cancelled.

Because they're separate timers, the Task.Delay can win a hair before hardTimeoutCts fires. In that window TimeoutException is thrown while the worker's linkedCts.Token is still un-cancelled — so a worker whose blocking Open() returns late sees an un-cancelled token and can perform a further step, defeating the guarantee #326 finding #2 added ("stop before performing a further state-changing step"). Under CI load the ordering varies, so the test flaked (observed failing on unrelated PRs #356 and #357).

Fix

The abandon path now calls hardTimeoutCts.Cancel() up front (idempotent if the timer already fired), so the worker's linked token is deterministically cancelled before TimeoutException propagates. No reliance on the two timers racing in a lucky order. Placement is before the existing abandon/observe-fault/dispose logic; disposal ownership and the caller-cancellation preference are unchanged.

Testing

• dotnet test — 1635 passed / 0 failed / 2 skipped (net9.0 + net10.0); WifiBridgeActivator suite 17/17.
• The previously-flaky RunWithHardTimeoutAsync_HardTimeoutElapses_StopsWorkerBeforeLateStep — which is the correct regression guard for this race — passed 30/30 consecutive runs after the fix.
• No bench validation: this is firmware-activation timeout concurrency ordering, not device-facing behavior.

Not merging — for review.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Cancel hard-timeout CTS before throwing TimeoutException in WifiBridgeActivator

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Cancel the hard-timeout CancellationTokenSource before propagating TimeoutException.
• Remove a race where the worker could observe an uncancelled token after timeout.
• Stabilize a flaky CI regression test by making timeout ordering deterministic.

Diagram

graph TD
  A["ActivateAsync / DeactivateAsync"] --> B["RunWithHardTimeoutAsync"] --> D["WhenAny race"] --> H["Cancel hard-timeout CTS + throw TimeoutException"]
  D --> E["Task.Delay(hardTimeoutMs)"]
  B --> C["workerTask (LongRunning)"] --> G["linkedCts.Token"]
  B --> F["hardTimeoutCts (timer)"] --> G

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Unify to a single timeout mechanism (CTS-driven)

• ➕ Eliminates duplicated timers entirely (no Delay-vs-CTS ordering concerns).
• ➕ Timeout detection and worker cancellation come from the same source by design.
• ➖ More refactor risk in subtle abandonment/cleanup logic.
• ➖ Harder to keep the existing ‘prefer caller cancellation’ semantics perfectly unchanged.

2. Use the CTS as the WhenAny signal (wait on token)

• ➕ Keep hardTimeoutCts as the source of truth; WhenAny waits for its cancellation rather than a separate Delay(hardTimeoutMs).
• ➕ Preserves existing token wiring while making ordering deterministic.
• ➖ Slightly more complex to express cleanly (e.g., token-based task completion patterns).
• ➖ Still requires careful handling of caller cancellation vs timeout precedence.

Recommendation: The current approach (explicitly calling hardTimeoutCts.Cancel() on the abandon/timeout path) is the lowest-risk fix: it preserves the existing control flow, disposal ownership, and the caller-cancellation precedence while deterministically closing the ordering gap. Consider a follow-up refactor to unify timeout signaling (CTS-driven) if this pattern appears elsewhere or grows more complex.

Files changed (1) +10 / -0

Bug fix (1) +10 / -0

WifiBridgeActivator.csCancel hard-timeout CTS before abandoning worker and throwing TimeoutException +10/-0

Cancel hard-timeout CTS before abandoning worker and throwing TimeoutException

• In RunWithHardTimeoutAsync, the timeout/abandonment path now explicitly cancels hardTimeoutCts before continuing with observe-fault/dispose logic and throwing TimeoutException. This removes a race where Task.Delay could win slightly before the CTS timer fired, allowing the worker to briefly observe an uncancelled linked token after the caller already timed out.

src/Daqifi.Core/Firmware/WifiBridgeActivator.cs

[qodo-code-review]

Code Review by Qodo

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

Great, no issues found!

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

[tylerkron]

Qodo review clean (Bugs 0), CI green, full suite 1635 pass, flaky test 30/30 after the fix. Ready for review.

[qodo-code-review]

review clean (Bugs 0), CI green, full suite 1635 pass, flaky test 30/30 after the fix. Ready for review.

The PR looks ready for review. The fix explicitly cancels hardTimeoutCts before throwing, closing the race where a late worker could perform another state-changing step. The clean bug scan, green CI, 1,635 passing tests, and 30/30 flaky-test runs provide strong validation.