Skip to content

fix(logservice): start one HAKeeper ticker per store - #28696

Merged
XuPeng-SH merged 2 commits into
matrixorigin:mainfrom
XuPeng-SH:codex/fix-hakeeper-test-ownership
Sep 11, 2026
Merged

fix(logservice): start one HAKeeper ticker per store#28696
XuPeng-SH merged 2 commits into
matrixorigin:mainfrom
XuPeng-SH:codex/fix-hakeeper-test-ownership

Conversation

@XuPeng-SH

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • 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.
  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.

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Deep review completed at exact head 20fda8e against base 4a7bfac. No concrete blocker found. COMMENT only for the self-authored PR.

Both voting and non-voting startup paths share one store-lifetime admission guard, and sync.Once publishes the same admission result to concurrent callers. The guarded operation is only Stopper admission, not Raft/RPC work. RunNamedTask's returned failure is terminal shutdown rejection, so retaining it does not suppress a transient retry. Normal ticker execution survives missing/local-restarting replicas and reads the current atomic replica identity; its only normal early exit besides cancellation is task-worker admission rejection during shutdown. Store Close still cancels/joins both ticker workers before closing NodeHost. A fresh store owns fresh Once/error state, and disabled-worker fixtures bypass admission. This is an admission guarantee, not a new readiness, panic-recovery or bounded-callback guarantee; existing worker panics remain unrecovered and existing in-flight callback waits are unchanged.

The fixture changes remove competing manual/background drivers without replacing replicated clock semantics with a fake counter. The expiry test now checks no-CN retention, exact boundary, expired-versus-live task separation, repeated scheduling and returning-CN behavior; the asynchronous task-ticker case remains. Restart tests use real NodeHosts, prove voting-replica clock progress, join workers before counting entries and cover both replica roles. Cleanup order and FD inspection now use the actual initialized filesystem. No new hot-loop lock/allocation or accumulating worker generation found.

Evidence: full diff, startup/Stopper/ticker/task-worker/replica-stop/Close closures and test oracles inspected. The documented Linux race stress, owning-package normal/race runs and mutation counterexamples are reused as author-reported evidence; no new native test, retained-data upgrade or live SQL cluster run is claimed. CI was not awaited. Exact head/base rechecked before submission.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working kind/test-ci size/L Denotes a PR that changes [500,999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: logservice HAKeeper bootstrap tests race on ID allocation and initial cluster state

2 participants