[Putluru Sandeep (sunny-kavixo)]

Summary

This is a test-focused contribution to #235, targeting its fix/logmonitor-lint branch rather than competing with its production fix.

The existing TestShutdownDuringReplay sleeps for 100 ms, so it can pass without proving that shutdown interrupts a replay batch. This change adds test-only synchronization that:

1. pauses the worker immediately after EvtNext returns a real replay batch;
2. observes the destructor requesting shutdown;
3. releases the worker and verifies that no event is processed after the request; and
4. verifies that every unprocessed event handle in the batch is closed.

The hooks are guarded by LOGMONITOR_TESTS and compile out of production builds.

Evidence

• Positive control on Windows Server 2022: the focused test executed and passed exactly once in each of three runs, with TRX counters validated by CI.
  • https://github.com/sunny-kavixo/windows-container-tools/actions/runs/40195757540
• Negative control: removing only the per-event CheckForStop() block caused the test to fail on its first run with Expected:<0> Actual:<1> - Replay processed an event after shutdown was requested.
  • https://github.com/sunny-kavixo/windows-container-tools/actions/runs/40195752008
• cpplint==2.0.2 passed on all changed C++ files in both runs.

This does not claim to reproduce the original large-backlog crash in #238. It closes the deterministic regression-coverage gap identified in the review of #235.

Related: #238

[Copilot]

🟡 Changes recommended

The replay barrier can time out and still satisfy every assertion, allowing the regression test to pass without exercising shutdown cancellation.

1 open issue

• Assert replay thread started before validating shutdown cancellation · New

What changed in this PR

This test-focused PR strengthens replay-shutdown coverage for #235’s EventMonitor fix without changing production behavior or claiming to reproduce #238’s crash.

Changes:

• Replaces sleep-based replay testing with batch and shutdown synchronization.
• Adds test-only hooks to track event processing and handle closure.
• Enables the hooks and thread support in the test build.

File	Description
LogMonitor/​src/​LogMonitor/​EventMonitor.h	Declares test-only replay hooks and constructor parameter.
LogMonitor/​src/​LogMonitor/​EventMonitor.cpp	Adds guarded synchronization and event counters.
LogMonitor/​LogMonitorTests/​pch.h	Enables test hooks and includes thread support.
LogMonitor/​LogMonitorTests/​EventMonitorTests.cpp	Replaces timing-based checks with synchronized shutdown assertions.

_{Review effort: Balanced}

[bob sira (bobsira)]

Thanks Putluru Sandeep (@sunny-kavixo) -- this is a useful, well-scoped addition to #235. I independently checked commit ae5a80b9b23bca55f7bc16075903a6354ae36292 and confirmed the barrier-timeout false positive raised in the existing inline review.

Local evidence

Using an isolated checkout, an x64 Release CMake build, and VSTest filtered to TestShutdownDuringReplay, I observed:

Scenario	Executed	Passed	Failed
Unmodified PR	1	1	0
Remove only the per-event CheckForStop() block inside the replay loop	1	0	1
Same removal, plus Sleep(6000) immediately before ULONGLONG start = GetTickCount64() in the replay test	1	1	0

The no-delay negative control failed with:

Expected:<0> Actual:<10> - Replay processed an event after shutdown was requested.

The delayed negative control incorrectly passed (TRX duration approximately 6.32 seconds). The delay models the test thread being descheduled after receiving batchReady but before starting the shutdown timer. The worker's five-second releaseBatch wait expires, and cleanup closes the batch without processing events. Destruction then completes quickly, leaving every current assertion satisfied even though the per-event cancellation check is missing.

Suggested follow-up

1. Add a DWORD barrier-wait result to EventMonitorReplayTestHooks, initialized to a non-success sentinel such as WAIT_FAILED.
2. Store the return value from WaitForSingleObject(releaseBatch, ...) in that field, and use it for the worker's existing timeout/error branch.
3. After shutdownThread.join(), assert that the stored result is WAIT_OBJECT_0. This ensures only an explicitly released barrier can satisfy the cancellation test; timeout or wait failure must fail it.
4. Repeat the controls above: the normal PR should pass, removal of the per-event cancellation check should fail, and removal plus the six-second delay should also fail, this time because the barrier timed out.

Please keep the artificial delay and cancellation removal confined to the negative-control experiment, not the final patch. Increasing the timeout alone would only make the false positive less likely, rather than close it.

This supports the existing review finding rather than identifying a separate issue. These experiments validate the test's cancellation coverage; they do not establish the faulting instruction behind #238's original crash. No changes were pushed to your branch.

[Putluru Sandeep (sunny-kavixo)]

Thanks for identifying the barrier-timeout false positive.

I've addressed this in commit 130bc46.

The replay worker now records the releaseBatch wait result, initialized to WAIT_FAILED. TestShutdownDuringReplay asserts WAIT_OBJECT_0 after the shutdown thread joins.

Windows validation completed successfully:

• Corrected test: 3/3 passes, with TRX counters confirming one executed test per run.
• Cancellation negative control: failed as expected.
• Six-second barrier-timeout negative control: failed as expected with WAIT_TIMEOUT.
• cpplint==2.0.2: passed.
• Production library build: passed.

Verification evidence:
https://github.com/sunny-kavixo/windows-container-tools/actions/runs/40201959639

The correction remains test-scoped, with no intended production behavior changes.

I'd appreciate a re-review of the updated commit.

[bob sira (bobsira)]

Thanks for contributions!