Skip to content

Fix XTaskQueue delayed-callback timing and wait-timer teardown races#975

Merged
jasonsandlin merged 7 commits into
mainfrom
fix-xtaskqueue-termination-early-wake
May 13, 2026
Merged

Fix XTaskQueue delayed-callback timing and wait-timer teardown races#975
jasonsandlin merged 7 commits into
mainfrom
fix-xtaskqueue-termination-early-wake

Conversation

@jhugard

@jhugard jhugard commented May 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR fixes several correctness bugs in the XTaskQueueSubmitDelayedCallback
pipeline and the Linux STL wait-timer backend so delayed callbacks are neither
dispatched before their requested delay has elapsed nor stranded waiting for an
unrelated wake, and STL timer teardown can no longer race an in-flight callback
dispatch. See issues #971, #973, and #974 for additional detail.

The public XTaskQueue API surface is unchanged. The fixes are internal to the
delayed-callback scheduling path and timer backends, but they matter to any
consumer relying on timed waits, deferred continuations, retry back-off,
timeout enforcement, or queue teardown.

This branch also includes a small iOS backend cleanup identified during the
same audit: fractional-millisecond delays now round up instead of down, and the
iOS wait timer now routes destruction through WaitTimer::Terminate() for
consistency with the shared backend lifecycle. Unlike the STL change, however,
this iOS work does not add new teardown-quiescing logic.

Bugs addressed

This PR addresses five distinct correctness issues in delayed-callback
scheduling and the STL backend, and also includes one small iOS cleanup:

  1. Termination early-promotion. XTaskQueueTerminate on a composite queue
    could disturb shared delayed-port state and make a sibling queue's not-yet-
    due callback appear ready immediately.
  2. Stale timer notification. An OS timer callback queued for an earlier
    deadline could arrive after the timer had already been re-armed for a later
    entry. The old code treated any timer fire as proof that the current armed
    deadline had elapsed.
  3. Equal-deadline aliasing. The old logic matched pending entries by exact
    uint64_t timestamp instead of by whether the deadline had actually passed.
    Two entries with near-identical due times could see one promoted and the
    other stranded until the next unrelated timer fire.
  4. Same-port empty-sweep lost wake. If a future delayed callback was queued
    on the same port while the dispatcher was clearing m_timerDue after an
    empty sweep, the new work could strand itself with no timer armed.
  5. STL timer teardown race. The old Linux STL backend could pop a raw timer
    pointer, drop the queue lock, and then invoke the callback after
    WaitTimer::Terminate() had already destroyed the owning timer object.
  6. iOS timer rounding and lifecycle cleanup. The iOS backend truncated
    fractional-millisecond delays and now routes destruction through
    WaitTimer::Terminate() for consistency with the shared WaitTimer lifecycle.

Across the five core correctness fixes, the external symptom is the same: a
delayed callback becomes runnable at the wrong time, either too early, only
after unrelated later work happens to wake the queue, or after teardown has
already started. The iOS cleanup is narrower and is limited to due-time
conversion, rounding, and lifecycle consistency.

What changed

Delayed callback scheduling (TaskQueue.cpp)

  • PromoteReadyPendingCallbacks sweeps all pending entries whose
    enqueueTime <= now, promotes every ready callback, and then re-arms the
    timer for the next future deadline. This replaces the old single-entry,
    exact-timestamp flow.
  • SubmitPendingCallback now treats an early timer fire as a stale or early
    notification and retries against the currently armed due time instead of
    silently returning.
  • The same-port path performs a rescue pass after clearing m_timerDue to
    UINT64_MAX, using a loop instead of recursive re-entry, which prevents the
    empty-sweep lost-wake race from leaving a newly queued delayed callback
    without an armed timer.
  • CancelPendingEntries no longer mutates shared timer state during
    termination. It gathers canceled entries first and only publishes them after
    the pending-list traversal completes.

Timer backends (WaitTimer_win32.cpp, WaitTimer_stl.cpp, ios_WaitTimer.mm)

  • The backends use monotonic steady_clock-based due times instead of mixing
    absolute wall-clock and relative timers.
  • Win32 and STL retain the existing one-shot timer model, but now convert the
    stored due time back to a relative wait only when arming the platform timer.
  • The STL backend now keeps callback context in shared per-timer state,
    generation-tags heap entries instead of null-scanning the queue on every
    re-arm, and uses an explicit quiescing Terminate() path so teardown can
    wait for in-flight dispatch safely without leaking the timer queue for process
    lifetime.
  • iOS now uses std::chrono::ceil<std::chrono::milliseconds> instead of
    truncating with duration_cast<std::chrono::milliseconds>.
  • The iOS backend now implements WaitTimer::Terminate(), and its destructor
    routes through that entry point so timer cleanup follows the shared
    WaitTimer lifecycle shape. This is a narrow consistency cleanup, not the
    same quiescing teardown hardening used by STL.

Thread pool (ThreadPool_stl.cpp)

  • SubmitWork now uses notify_one instead of notify_all when waking a
    worker thread. Each queued call issues its own wake, so thundering-herd wakes
    on every submit are unnecessary.

Test hooks and regression coverage

  • Unit-test-only hooks are compiled only for unit-test builds (HC_UNITTEST_API).
  • Commits are layered red-then-green so a reviewer can walk from failing repro
    to passing fix for each bug.

Regression coverage

Deterministic coverage now includes:

  • VerifyDelayedCallbackTimerRaceOnManualQueue
  • VerifyFutureDelayedCallbackQueuedDuringEmptySweepDoesNotStall
  • VerifyTerminationDoesNotEarlyPromoteSiblingDelayedCallback
  • VerifyStaleDelayedCallbackDoesNotEarlyPromoteNextPendingEntry

Together these cover the original stale callback repro, the shared-port
termination bug, the stale-timer retargeting bug, and the same-port lost-wake
race. Correctness and hardening are further exercised by a downstream Win32
and Linux coroutines test suite and the benchmark validation called out below.

Public API surface

  • No public XTaskQueue signatures change.
  • XTaskQueueSubmitDelayedCallback now correctly preserves its not-before-
    deadline behavior under shared delayed ports, concurrent termination, timer
    retargeting, and same-port requeue races.
  • WaitTimer::Terminate() remains an internal backend concern. The STL fix only
    changes the internal lifetime model so delayed callbacks cannot outlive their
    timer object during teardown.
  • XTaskQueueTerminate is unchanged from the caller's perspective: callbacks on
    the terminated queue still complete as canceled. The fix only stops that
    termination from affecting sibling queues.

Validation

libHttpClient Windows unit test suite:

  • Added regression tests to expose the bugs fixed in this PR
  • Full TAEF suite passed (87/87)

Additional downstream integration validation:

  • Exercised through an internal coroutines library that uses XTaskQueue
    delegate queues and delayed callbacks to implement sleep, timeout, and
    cancellation behavior.
  • Also exercised through an internal WebSocket scale/throughput benchmark built
    on that coroutine layer.
  • Repeated Windows and Linux soak and end-to-end runs remained stable after
    the fix, including workloads that had previously exposed the original
    shutdown hang.

Not performed:

  • iOS build or runtime validation. The iOS cleanup was code-audited only; it
    was not exercised on-device or in an iOS CI environment.
  • Performance regression testing. The unfixed code path is unstable under
    sustained load — the timer bugs cause crashes during connection scaling and
    throttle delayed-callback throughput to polling frequency — so no meaningful
    pre-fix performance baseline exists for comparison.

Scope

  • No public API additions or removals.
  • Win32 and STL carry the substantive timer correctness fixes; iOS receives a
    narrow due-time and lifecycle cleanup.
  • The change is scoped to delayed-callback scheduling and teardown correctness.
    Immediate dispatch and unrelated task-queue behavior are unchanged.

Review focus

  • The promote-all-ready sweep and its enqueueTime <= now criterion.
  • The stale-timer retry path in SubmitPendingCallback.
  • The same-port rescue path after m_timerDue is cleared.
  • The decision to keep shared timer state intact during termination and to let
    STL teardown quiesce in-flight dispatch instead of relying on raw timer
    pointers.
  • The iOS due-time conversion, ceil rounding, and narrow lifecycle plumbing
    through WaitTimer::Terminate().

Potential follow-up: iOS hardening

The current PR deliberately stops short of a full iOS timer hardening pass.
The iOS changes here are limited to monotonic due-time conversion, ceiling
rounding, and routing destruction through WaitTimer::Terminate(); they do not
attempt to add the shared-state teardown quiescing used by the STL backend.

If we decide to pursue that Apple-platform hardening in a follow-up, the likely
shape is:

  • Replace the current NSTimer plus raw WaitTimerImpl* userInfo handoff
    with a one-shot dispatch_source_t timer or another non-run-loop primitive.
  • Keep callback payload and teardown bookkeeping in shared state so teardown can
    block new dispatch and wait for any in-flight callback to quiesce before the
    timer object is destroyed.
  • Preserve one-shot semantics while ensuring canceled or superseded arms cannot
    dispatch later, for example by per-arm source replacement or generation
    tagging.
  • Validate callback-thread behavior and performance on macOS/iOS before landing
    it, since moving away from NSTimer would change both the timer primitive and
    the execution context.

That would be a reasonable direction if we later decide to fix all timer
backends to the same lifetime standard, but it is intentionally out of scope
for this PR because we do not currently have Apple-platform correctness or
performance validation in hand.

jhugard added 6 commits May 11, 2026 21:43
…races

Retarget delayed-callback wake handling so stale timer notifications cannot promote future work too early, keep the same-port empty-sweep rescue path iterative, and harden the STL wait timer so teardown cannot race an in-flight callback after a due heap entry is popped. The regression and hook changes remain grouped here because they cover the same delayed-callback scheduling line and the Linux timer backend that services it.
Keep the iOS backend on monotonic due times without truncating sub-millisecond delays, and implement the missing WaitTimer termination path so the Apple backend matches the shared TaskQueue lifecycle contract.
@jhugard jhugard requested a review from brianpepin May 12, 2026 14:58

@brianpepin brianpepin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change looks good and I appreciate the addl testing.

@jasonsandlin jasonsandlin merged commit 3239b2a into main May 13, 2026
15 checks passed
@jasonsandlin jasonsandlin deleted the fix-xtaskqueue-termination-early-wake branch May 13, 2026 00:08
jhugard added a commit that referenced this pull request May 21, 2026
Follow-up to #975 and #980: the CAS-then-Start pattern for retargeting
m_timerDue had a TOCTOU window where thread A could win the CAS but
before calling Start(), thread B could CAS+Start an earlier deadline,
which thread A's Start() would then overwrite — stranding the earlier
callback until independent traffic arrived.

The fix in #980 added post-Start verification in SubmitPendingCallbacks,
but the same unguarded pattern existed in QueueItem and
PromoteReadyPendingCallbacks.

This change extracts the CAS+Start+verify logic into three helpers
(ArmTimerIfEarlier, ArmNextPendingCallback, RearmObservedDueTime) so
the post-Start verification is applied uniformly at every call site.
~67 lines of duplicated inline CAS logic removed.
jasonsandlin added a commit that referenced this pull request May 27, 2026
Follow-up to #975 and #980: the CAS-then-Start pattern for retargeting
m_timerDue had a TOCTOU window where thread A could win the CAS but
before calling Start(), thread B could CAS+Start an earlier deadline,
which thread A's Start() would then overwrite — stranding the earlier
callback until independent traffic arrived.

The fix in #980 added post-Start verification in SubmitPendingCallbacks,
but the same unguarded pattern existed in QueueItem and
PromoteReadyPendingCallbacks.

This change extracts the CAS+Start+verify logic into three helpers
(ArmTimerIfEarlier, ArmNextPendingCallback, RearmObservedDueTime) so
the post-Start verification is applied uniformly at every call site.
~67 lines of duplicated inline CAS logic removed.

Co-authored-by: Jason Sandlin <jasonsa@microsoft.com>
rgomez391 pushed a commit that referenced this pull request Jun 23, 2026
#975 rewrote the shared STL wait-timer backend (WaitTimer_stl.cpp) to replace
the O(N) pointer-keyed cancellation scan with an O(log N) per-timer generation
scheme: every Start() bumps a generation counter and pushes a fresh heap entry,
and the worker discards any popped entry whose generation no longer matches the
timer's current generation.

That scheme can drop a due callback. A Start() that races the worker between
its Peek() of the earliest entry and its dispatch of that entry bumps the
generation, so when the worker pops the entry it now reads a stale generation
and discards it instead of firing it. The re-pushed entry carries a new
deadline, so the original due callback is never delivered. With the entry gone,
the worker sleeps on the next (later or absent) deadline and goes idle.

On a single-port manual XTaskQueue this is unrecoverable: the platform HTTP
provider re-arms its poll via XTaskQueueSubmitDelayedCallback, and once that
re-poll is dropped there is no independent traffic to wake the queue and sweep
the pending list again. The symptom is an infinite sign-in / inventory hang
(the delayed re-poll sits in the pending list while the timer thread waits in
_Cnd_timedwait, never re-armed).

Why the arming-layer fixes did not catch or fix it:
- #975, #980 and #983 (#983 = "Centralize timer-arming to close TOCTOU race")
  all hardened the CAS->Start bookkeeping in TaskQueuePortImpl (m_timerDue) in
  TaskQueue.cpp. Those fixes are correct and are retained here. But they assume
  Start() reliably arms the backend for the published deadline. The generation
  backend violates that assumption by dropping the entry after Start(), so no
  amount of arming-layer verification can recover it.
- The defect only affects WaitTimer_stl.cpp. Win32/UWP use WaitTimer_win32.cpp
  (OS CreateThreadpoolTimer, no generation scheme) and are unaffected. The
  libHttpClient unit-test project compiles the Win32 backend, so the Windows
  suite stayed green and none of #975/#980/#983 exercised the STL backend.

Fix: restore the proven pre-#975 STL backend algorithm (libHttpClient 2.3.1,
commit 0fa5f24) - pointer-keyed cancellation and an unconditional notify on
every Set, which cannot discard a due entry - bridged to the post-#975 public
WaitTimer API (GetCurrentTime / GetDueTime / Start(dueTime)). The clock is
pinned to steady_clock (the pre-#975 code used high_resolution_clock, which is
steady_clock on libc++ but wall-clock system_clock on libstdc++); pinning keeps
the delayed-callback ordering monotonic on every platform that compiles this
backend, preserving #975's monotonic-time intent.

The arming-layer improvements in TaskQueue.cpp from #975/#980/#983 are kept
unchanged; only the STL timer backend is reverted.

Validated on-device in a shipping title: the reproducing infinite sign-in /
inventory hang no longer occurs with this backend and the current TaskQueue
arming logic.

Follow-up: the STL backend has no unit-test coverage (the test project compiles
only the Win32 backend). A dedicated STL-backend test lane is needed so a future
re-land of the generation optimization cannot reintroduce this class of strand.
rgomez391 pushed a commit that referenced this pull request Jun 25, 2026
Addresses PR review: restore the unnamed-cond-var type (matches the rest of
the codebase / the platforms that require unnamed sync objects) instead of the
plain std::condition_variable the pre-#975 file used. DefaultUnnamedConditionVariable
is a drop-in for std::condition_variable and pairs with the existing
std::unique_lock<std::mutex> usage unchanged; no behavior change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants