test(logservice): avoid HAKeeper double-driver race in task scheduler test - #28668
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed exact head 6e9203d against main cd8e8d9. No concrete merge-blocking issue found. COMMENT only for this self-authored PR.
The fixture flag actually prevents startHAKeeperReplica from starting the production HAKeeper ticker, whose startup would also create a second task-scheduling ticker. The manual bootstrap remains the sole bootstrap/ID-allocation driver, while the explicitly launched production tickerForTaskSchedule remains active with the real leader-state getter and taskSchedule implementation. The assignment assertion is unchanged: the newly created task must become assigned to the newly heartbeating CN. Thus disabling the duplicate fixture owner does not bypass the behavior this test is intended to observe.
Cleanup now cancels and joins the explicit ticker before the callback returns, so the outer fixture cannot close the store or task service underneath it. The done channel is closed by the goroutine's defer, with one sender/one waiter and no detached cleanup task. Normal return, FailNow on the test goroutine and panic unwinding all execute the cancellation/join defer. There are no production synchronization changes, skipped assertions, new ports/data directories or shared cross-test state.
The join intentionally waits for an in-progress checker/scheduler callback to return; cancellation does not interrupt that callback by itself. This is an honest lifecycle wait, not a new claim of an independent short shutdown deadline. Existing helper/query timing limits and package timeout still apply. The original test's one-second SQL context versus ten-second polling timer is unchanged and could be cleaned up separately if it proves flaky.
Scope is correctly narrow: this fixes the manually bootstrapped ticker test, not the separate expiration/rescheduling test that needs HAKeeper ticks. It is not a claim that all #28550 occurrences or real production leader transitions are solved.
Evidence: complete one-file diff, current PR discussion/review state, exact-head fixture flag/startup, manual bootstrap, production tickers, scheduler dispatch and outer teardown ordering inspected; head/base rechecked before submission. Author reports normal execution, 14 race repetitions and a related scheduling control on macOS arm64. I did not independently rerun native tests or Linux CI and did not wait for CI.
f60d882 to
d6558e7
Compare
## What type of PR is this? - [ ] API-change - [x] BUG - [ ] Improvement - [ ] Documentation - [ ] Feature - [x] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: Fixes #28550. Completes the expired-task test follow-up documented in #28668. ## What this PR does / why we need it: HAKeeper state transitions need one driver per store. Two paths violated that ownership: 1. `TestTaskSchedulerCanReScheduleExpiredTasks` manually bootstrapped and scheduled tasks while its fixture also ran the production HAKeeper checker and task ticker. This caused the allocator race reported by [the #28646 UT job](https://github.com/matrixorigin/matrixone/actions/runs/34554939075/job/103145324437). 2. In production, stopping a local HAKeeper replica leaves the store-owned ticker alive, but starting that replica again admitted another ticker. A real NodeHost stop/resume probe against unchanged main production code observed **two worker entries instead of one**, through both voting and non-voting startup paths. Extra checkers share the allocator/bootstrap state and also start extra task schedulers. Both replica startup paths now share one store-lifetime startup guard. Concurrent callers observe the same admission result; shutdown rejection is terminal for that store. Replica stop/resume reuses the original ticker, while a new store gets a fresh guard. This adds synchronization only at worker startup, with no new locks, scans, or allocations in tick, task scheduling, ID allocation, or query execution. The manual task-service fixture now always disables background workers. Its shared bootstrap helper enforces that precondition, eliminating duplicated bootstrap code. The expired-task test advances the **real replicated HAKeeper clock** explicitly instead of sleeping for expiration. It retains the existing complete-and-truncate behavior for expired tasks and adds live-runner and exact-boundary controls. The explicit task-ticker test remains asynchronous and joins its worker before teardown. FD checks now inspect the initialized fixture filesystem. ### Test plan / QA reference | Contract | Verification | |---|---| | Original flaky case | `TestTaskSchedulerCanReScheduleExpiredTasks`: no eligible CN retains a Created task; exact expiry boundary retains the running task; the next tick removes only the expired runner's task; repeated scheduling preserves the live task; a returning CN accepts new work without resurrecting the old task. | | Other manual-bootstrap consumers | `TestTaskSchedulerCanScheduleTasksToCNs`, `TestTickerForTaskSchedule`, `TestAllocateIDByKeyWithRequestID`. All use the checked shared helper; the ticker case still proves asynchronous assignment. | | Production worker lifetime | `TestHAKeeperTickerSurvivesReplicaRestart`: concurrent startup admission; real voting and non-voting stop/resume; replicated tick progress before and after voting-replica restart; exactly one worker entry after joining all workers. | | Shutdown / disabled-worker admission | `TestHAKeeperTickerRejectsStartupAfterShutdown`: concurrent callers receive `stopper.ErrUnavailable`; disabled-worker fixtures create no worker. | | Counterexample strength | Temporary Go overlays changing CN expiry from `<` to `<=`, disabling expiry entirely, or re-enabling the manual fixture's background workers each fail the corresponding assertion. These deliberately incorrect variants are not committed. | Validation on Linux/amd64, Go 1.26.4, using source-matched native artifacts and `.agents/skills/mo-dev/scripts/mo-cgo-test`: - The four manual-bootstrap consumers above each passed `-race -count=100` in a single process. Their worker-disabled execution paths are unchanged by the subsequent production startup guard. - The restart test passed `-race -count=68`; shutdown admission passed `-race -count=100`. Counts follow a 30-second per-test stress budget (restart measured 0.44 seconds). - Final `pkg/logservice` normal suite passed (110.622 seconds); the full race suite passed (112.295 seconds), both with `-count=1 -timeout=600s`. - Scoped `golangci-lint --new-from-rev=<base> ./pkg/logservice`, package `molint`, and `git diff --check` passed. - Expiration-test body time under race decreased from roughly 1.3 seconds to 0.2 seconds locally. The new two-mode restart test takes roughly 0.44 seconds. No production throughput gain is claimed. The SQL surface and wire/disk formats do not change. The affected restart boundary is exercised with real in-memory NodeHosts; no full SQL cluster, retained-data upgrade, or 55-machine test is claimed. Rebased onto main `4a7bfac4f9` before delivery. The intervening commit changes only `pkg/embed`, outside the verified `pkg/logservice` test dependency graph; native inputs and all validated code/test paths are unchanged, so the completed evidence is reused. ### Ownership / unhappy-path review | Boundary | Closure | |---|---| | Startup | One `sync.Once` admission per store; both replica roles share it; no RPC is held under the guard. | | Replica stop/resume | The original ticker remains the owner and observes the current replica identity; no additional checker/task ticker accumulates. | | Store shutdown | Existing stopper cancellation joins its workers before NodeHost closure; manual ticker cleanup also joins before fixture teardown. | | Admission failure / fresh store | Shutdown rejection is published to every caller; a fresh store has independent startup state. | | Test failure | Defers release contexts, stores and task services; leak/FD checks run after the resources close. |
2 participants
What type of PR is this?
Which issue(s) this PR fixes:
Related to #28550
What this PR does / why we need it:
Root cause
TestTickerForTaskSchedulemanually drives HAKeeper bootstrap throughproceedHAKeeperToRunning, while its fixture also starts the production HAKeeper worker. The two drivers can concurrently advance the same bootstrap state andidAllocator, which caused the race reported by the race-enabled Ubuntu UT job.The task-scheduling ticker under test is a separate owner and must remain enabled.
Changes
TestTickerForTaskSchedulewith the HAKeeper worker disabled.This keeps one owner for manual bootstrap and one owner for the task-scheduling behavior being tested. It does not add synchronization or overhead to the production allocator, and it does not mask concurrent HAKeeper state-machine ownership with a lock.
Verification
.agents/skills/mo-dev/scripts/mo-cgo-test -count=1 -timeout=240s -run '^TestTickerForTaskSchedule$' ./pkg/logservice— PASS on macOS arm64..agents/skills/mo-dev/scripts/mo-cgo-test -race -count=14 -timeout=300s -run '^TestTickerForTaskSchedule$' ./pkg/logservice— PASS, 14 repetitions in one process..agents/skills/mo-dev/scripts/mo-cgo-test -race -count=1 -timeout=240s -run '^TestTaskSchedulerCanScheduleTasksToCNs$' ./pkg/logservice— PASS.GOWORK=off go vet -mod=readonly ./pkg/logservicewith the repository CGo include/library paths — PASS.git diff --check— PASS.Scope and known follow-up
This is a focused follow-up for the
TestTickerForTaskScheduleoccurrence in #28550.TestTaskSchedulerCanReScheduleExpiredTasksis intentionally not changed here because it relies on the background HAKeeper tick to advance expiration; disabling that worker without redesigning the test makes the test fail. The broader issue remains open for that separate single-driver redesign.The CodeQL failure on PR #28459 is unrelated: it is the existing SHA-256/password alert at
pkg/sql/compile/scheduler.go:553, outside this PR and this change.