Skip to content

fix(windows_event_log): fix lost-wakeup freeze with pre-drain reset and speculative timeout pull - #8

Closed
tot19 wants to merge 8 commits into
windowsfrom
tot19/windows-event-log-ci-test
Closed

fix(windows_event_log): fix lost-wakeup freeze with pre-drain reset and speculative timeout pull#8
tot19 wants to merge 8 commits into
windowsfrom
tot19/windows-event-log-ci-test

Conversation

@tot19

@tot19 tot19 commented Apr 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Summary

Fixes a freeze in the windows_event_log source that manifested after ~3 minutes of idle time, typically coinciding with h2 GoAway/backpressure events.

Two complementary fixes:

1. Pre-drain signal reset (subscription.rs)

The subscription's manual-reset wait handle was previously reset after draining events via EvtNext. Any SetEvent call from the Windows Event Log service that arrived between the last EvtNext and the ResetEvent was silently lost — the source then hung until the next OS event arrived.

Fix: reset the handle before entering the drain loop. Notifications raised mid-drain are preserved because SetEvent on an already-signaled handle is a no-op. Re-arming (SetEvent) is done on early exits (budget exhausted, bookmark failure, or transient EvtNext error) so those paths still revisit channels on the next call without waiting for a fresh OS notification.

2. Speculative pull on timeout (mod.rs)

Even with the pre-drain fix, if the signal is lost through any other mechanism (e.g. a GoAway/backpressure interaction that interrupts the drain loop in a way that doesn't hit the SetEvent re-arm paths), the source would still freeze until the next event arrived.

Fix: on each WaitResult::Timeout, call pull_events speculatively. EvtNext returns ERROR_NO_MORE_ITEMS immediately on an empty channel (near-zero cost), so this is safe every cycle. If events are recovered, a warn! is emitted. This guarantees self-healing within one timeout period regardless of root cause — belt and suspenders.

Regression test

test_pull_events_preserves_setevent_during_drain pins the pre-drain reset invariant by driving the real EvtSubscribe handle with a DRAIN_STEP_HOOK that fires SetEvent mid-drain (simulating the race window). After pull_events returns, wait_for_events_blocking must return EventsAvailable, not Timeout. Under the old post-drain reset order, it would time out — which is exactly the freeze reported.

Vector configuration

N/A — internal fix, no configuration changes.

How did you test this PR?

  • Regression test test_pull_events_preserves_setevent_during_drain (Windows CI)
  • Manual inspection of the drain loop pre/post-reset ordering
  • Speculative pull path verified by code review; near-zero cost confirmed by EvtNext returning ERROR_NO_MORE_ITEMS immediately on an empty channel

Change Type

  • Bug fix
  • New feature
  • Dependencies
  • Non-functional (chore, refactoring, docs)
  • Performance

Is this a breaking change?

  • Yes
  • No

Does this PR include user facing changes?

  • Yes. Please add a changelog fragment based on our guidelines.
  • No. A maintainer will apply the no-changelog label to this PR.

(Changelog fragment is included at changelog.d/25194_windows_event_log_lost_wakeup.fix.md.)

References

Notes

tot19 added 3 commits April 15, 2026 16:35
@tot19
fix(windows_event_log source): prevent permanent freeze from signal-e…
2c8b89a 
…vent lost wakeup

The Windows Event Log service signals pull-mode subscriptions via SetEvent
on a manual-reset event each time a new matching event is recorded. Because
SetEvent on an already-signaled handle is a no-op, resetting the handle
AFTER draining EvtNext creates a race: a signal arriving between the last
EvtNext and ResetEvent is silently lost and the source freezes until the
next event arrives. Reset before the drain so signals raised mid-drain are
preserved; re-SetEvent on early break (budget exhausted / bookmark failure)
so channels are not starved.

Fixes vectordotdev#25194
@tot19
fix(sources): re-arm windows_event_log signal on EvtNext error and ha…
ab329bc 
…rden regression test

Address review feedback on vectordotdev#25195:

- Re-SetEvent on the EvtNext error-return path. Without this, a transient
  EvtNext failure leaves the pre-drain-reset signal cleared with events
  possibly still un-drained — the same class of lost-wakeup the PR fixes
  for the success path.

- Replace the primitive-level Barrier test with an integration-style test
  that drives real pull_events against a real EvtSubscribe handle. A new
  cfg(test) DRAIN_STEP_HOOK fires SetEvent on the subscription's signal
  handle inside the drain loop, simulating the OS signaling a fresh event
  mid-drain. After pull_events returns, wait_for_events_blocking must
  return EventsAvailable; under the old post-drain ResetEvent order it
  would time out. This test would now actually catch a reversion.
@tot19
chore(sources): harden windows_event_log lost-wakeup regression test
8fe9083 
Two small hardenings to make the test deterministically catch a
reversion of the pull_events signal-reset ordering:

- Use pull_events(usize::MAX) instead of pull_events(100) so the drain
  exits via ERROR_NO_MORE_ITEMS (channel_drained = true). On hosts with
  >100 Application events the old budget would exit via budget
  exhaustion, skipping the post-drain ResetEvent in the pre-fix code
  and letting the test false-pass against the buggy ordering.

- Wrap hook cleanup in a drop-guard so a panic mid-test doesn't leak
  the global DRAIN_STEP_HOOK into other tests sharing the process.
@tot19
fix(sources): key windows_event_log drain-loop test hook to its subsc…
ae716ca 
…ription

Addresses Codex review feedback on vectordotdev#25195. DRAIN_STEP_HOOK is a
process-global and cargo runs tests in parallel, so a concurrent test's
pull_events could trigger this test's one-shot hook first, flip `fired`,
and SetEvent on the wrong signal handle — intermittently timing out
test_pull_events_preserves_setevent_during_drain and perturbing the
sibling pull_events tests.

Capture the subscription's own signal handle up front and make the hook
no-op when invoked for any other handle. Add a cfg(test)-only
first_channel_signal_raw accessor for the test to read it.
@tot19
tot19 force-pushed the tot19/windows-event-log-ci-test branch from c1f8c06 to c00bf90 Compare April 15, 2026 08:04
tot19 added 3 commits April 15, 2026 17:07
@tot19
style(sources): cargo fmt windows_event_log subscription.rs
0f017a5 
Fixes check-fmt CI failure on vectordotdev#25195.
@tot19
ci: use windows-latest runner for Windows CI workflows
b32552b 
Swap windows-2025-8core (vectordotdev private runner, unavailable on
forks) for windows-latest so the Windows integration workflow can run
against this PR's branch on the tot19/vector fork. Matches the runner
choice used successfully on the windows-latest-runner branch while
bringing up this source originally.
@tot19
ci: serialize windows_event_log integration tests with --test-threads=1
e27da1a 
Matches the serialization that was applied on the original
windows-latest-runner branch. Even with per-test provider names
isolating eventcreate + XPath filters, the Windows Event Log service
has shared per-channel signal handles and bookmarks that can cause
cross-test interference when nextest parallelizes across cores.
Running with --test-threads=1 is what actually went green in the
prior runner bring-up, so mirror it here to keep the integration
workflow deterministic on windows-latest.
@tot19
tot19 force-pushed the tot19/windows-event-log-ci-test branch from c00bf90 to e27da1a Compare April 15, 2026 08:09
@tot19
ci: add pull_request + workflow_dispatch triggers to Unit - Windows
b93f333 
So Windows unit tests (including the windows_event_log source's
#[cfg(windows)] regression test for vectordotdev#25194) actually execute on PRs
and can be manually triggered on the fork. Scoped via paths filter
to files that could change Windows-only code.
@tot19 tot19 changed the title Tot19/windows event log ci test fix(windows_event_log): fix lost-wakeup freeze with pre-drain reset and speculative timeout pull Apr 15, 2026
@tot19 tot19 closed this Apr 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

domain: ci domain: sources

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@tot19