test: deterministic fix for flaky StartupReconciliation test - #1731
Conversation
…iation test StartupReconciliation_DeliversLostNotificationToOwningSession was flaky on Windows CI: the manager created in ConfigureAkka runs its PreStart reconciliation asynchronously on the dispatcher, and if it landed after the orphan was persisted but before its output.log was written, it delivered a 'lost' notification with no output path (NotifyLostJob swallows FileNotFound and nulls OutputFilePath) — so Assert.Contains (logPath, delivery.Content) failed. PreStart completes before any user message dispatches, so a health barrier against the setup manager before seeding the orphan proves it reconciled an empty store. Only the freshly created manager (after the log write) reconciles the orphan, so the path is always present. Fixes the race deterministically (no timeout changes); matches the readiness-barrier idiom from #1410/#1417.
There was a problem hiding this comment.
🟢 Ready to approve
The change is test-only and it uses an existing deterministic health barrier pattern to remove flakiness.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR makes StartupReconciliation_DeliversLostNotificationToOwningSession deterministic by adding a startup readiness barrier for the manager that ConfigureAkka creates. This prevents the setup manager from reconciling after the test seeds an orphan job but before the test writes output.log.
Changes:
- Add a
GetBackgroundJobManagerHealthAsk barrier for theConfigureAkkamanager before the test persists the orphan. - Keep the existing barrier for the fresh manager that must reconcile and emit the lost-job notification.
File summaries
| File | Description |
|---|---|
| src/Netclaw.Actors.Tests/Jobs/BackgroundJobManagerActorTests.cs | Add a deterministic health barrier to remove a startup reconciliation race in one test. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Aaronontheweb
added
tests
There was a problem hiding this comment.
🟢 Ready to approve
The change is small and focused, and it adds a deterministic readiness barrier without altering production behavior.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
src/Netclaw.Actors.Tests/Jobs/BackgroundJobManagerActorTests.cs:226
- Rewrite this new comment block in Simplified Technical English. Use short sentences and avoid long, multi-clause explanations.
// The manager created in ConfigureAkka runs its PreStart reconciliation
// on the dispatcher, not necessarily before this test body. If it
// reconciles after the orphan is persisted but before its output.log
// exists, it delivers a "lost" notification with no output path
// (NotifyLostJob swallows the missing-file error and nulls
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
What
Barriers the setup-time
BackgroundJobManagerActor(created inConfigureAkka) before seeding the orphan inStartupReconciliation_DeliversLostNotificationToOwningSession, so its PreStart reconciliation provably finishes on an empty store.Why
The test was flaky on Windows CI (surfaced on PR #1726): the first manager's
PreStartreconciliation runs asynchronously on the dispatcher. If it landed after the orphan was persisted but before itsoutput.logwas written, it delivered a "lost" notification with no output path (NotifyLostJobswallowsFileNotFoundExceptionand nullsOutputFilePath), soAssert.Contains(logPath, delivery.Content)failed — the path was absent, not divergent.PreStartcompletes before any user message dispatches (Akka.NET guarantee, per #1410/#1417), so a health barrier against the setup manager before seeding the orphan proves it reconciled an empty store. Only the freshly created manager (created after the log write) reconciles the orphan — the notification always carries the full path.Deterministic fix; no timeout changes. Matches the existing readiness-barrier idiom in this suite (#1410, #1417).
Verification
StartupReconciliation_*tests: 3/3 passBackgroundJobManagerActorTests: 8/8 pass