test(taskservice): prove stale/failed refresh keeps the active task store (issue #24144 regression coverage)

[gouhongshen]

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

Related to #24144

What this PR does / why we need it:

Adds the regression coverage that issue #24144 asked for, proving the task-storage
refresh lifecycle contract that the issue's CI failure violated.

Root cause (already fixed on main; this PR adds the missing proof)

refreshableTaskStorage.refresh() used to execute _ = s.mu.store.Close() at
function entry — before rejecting a stale lastAddress request and before
storeFactory.Create() confirmed a replacement existed — and advanced
s.mu.lastAddress to the new address before Create succeeded. A stale refresh
or a failed creation therefore left the holder serving a closed store (or the old
store paired with the replacement address), and the ping that precedes every
forwarded SQL-task operation surfaced sql: database is closed: in compose
pessimistic CI, execute task sql_task_overlap failed, overlap_sink returned 1
instead of 2, and only one SUCCESS run was recorded.

The production ordering fix is already on main (commit 26226d2a57, PR #27135,
which referenced issue #27120 and never linked #24144): refresh now rejects
closed/stale requests before any factory work, preserves both mu.store and
mu.lastAddress when address resolution or Create fails, publishes the
replacement store/address together under the lock after a generation re-check,
and retires only a distinct previous store. Issue #24144 lacked its named
regression coverage: the two requested tests did not exist, and existing
coverage could not detect premature closure because memTaskStorage.Close and
PingContext are no-op successes, so pointer assertions passed even when the
installed store had been closed.

This PR is intentionally test-only (pkg/taskservice/task_service_holder_test.go,
+602 lines) so #24144 can be closed with concrete regression evidence. No
production behavior changes.

Exact changes

• TestRefreshTaskStorageSkipStaleRefreshKeepsCurrentStore (issue-named):
  a usable store at s1 plus a stale request carrying s2 → zero closes, zero
  address/Create calls, unchanged store/address, and successful subsequent
  public AddSQLTask/QuerySQLTask through a closure-sensitive store whose
  PingContext fails with sql: database is closed once closed.
• TestRefreshTaskStorageCreateFailureKeepsCurrentStore (issue-named): table
  over Create error, nil store, and address-factory error at s2 while serving
  s1 → current store preserved and usable, lastAddress stays s1.
• TestRefreshTaskStorageStaleRequestAfterReplacementKeepsNewStore: a later
  serialized request still carrying the old address must not close the new
  store; the distinct previous store is retired exactly once.
• TestRefreshTaskStorageFixedFactoryReuseKeepsStoreOpen: a fixed factory
  returning the active instance must not retire it.
• TestRefreshTaskStorageSameAddressReplacementPublishesNewInstance: a fresh
  instance at the same address is a distinct replacement (retire once, publish).
• TestRefreshTaskStorageRecoversAfterInitialFailure: failed initial and queued
  not-ready refreshes keep the holder not-ready (ErrNotReady); an observed
  recovery refresh publishes a working store (atomic failure flag, factory
  attempts announced on a channel so the async worker's read and the test's
  write are happens-before ordered).
• TestRefreshTaskStorageClosedStateIsTerminal: after Close,
  maybeRefresh returns false, refresh does no factory work and no second
  close, and Close is idempotent.
• TestRefreshTaskStorageStoreErrorReachesCaller: a genuine store ping error
  reaches the caller; preserving ownership must not fabricate success.
• TestRefreshTaskStorageOperationCompletesBeforeRetirement: deterministic
  ordering proof — refresh parks inside the factory (entry pre-check already
  passed), a public operation then parks inside the gated store holding
  mu.RLock, the resumed refresh is positively observed (goroutine-stack
  inspection) blocked on the publication write lock, any retirement attempt or
  refresh completion while the reader is parked fails immediately, and only then
  is the operation released; bounded waits assert exactly the two recorded
  events in the order add-completed, retired (detecting missing and
  duplicated retirement), with nonblocking event recording, per-launch worker
  registration, and cleanup that releases barriers, joins workers, then closes
  the holder.

Issue-to-test proof

• Differential: with only the verbatim pre-fix: make task and embedded lifecycles generation-safe #27135 refresh()
  (git show 26226d2a57^) swapped in, 7 of the 9 new tests fail promptly with
  the issue's exact defects — premature close on stale request, on all three
  failed-refresh injections, on stale-after-replacement, and on fixed-factory
  reuse; lastAddress advanced on failed creation; double close after terminal
  Close; and the ordering test fails because the pre-fix refresh holds the
  write lock across factory work. With the fixed code all pass.
• Mutation validation of the ordering oracle: early-return refresh (fails
  5/5 in 0.00 s), single early retirement before the publication lock
  (10/10 in ≤0.08 s), duplicate early retirement (5/5 in 0.00 s, workers
  joined), and missing retirement (3/3 at the 15 s bound) all fail
  deterministically with completed cleanup; the healthy implementation passes
  20/20 plain and 20/20 under -race.
• Supplementary (not part of this diff): a local single-CN cluster replay of the
  sql_task.sql overlap section (two manual executions with sleep(1)
  between, 10 s retry-polls) returned overlap_sink count = 2 and two
  SUCCESS runs with zero sql: database is closed occurrences, matching
  test/distributed/cases/task/sql_task.result.

Test commands

go vet ./pkg/taskservice/
gofmt -l pkg/taskservice/
go test ./pkg/taskservice/ -run 'TestRefreshTaskStorage|TestTaskHolder' -count=1 -timeout 600s
go test ./pkg/taskservice/ -race -count=3 -timeout 1800s -run 'TestRefreshTaskStorage|TestTaskHolder'
go test ./pkg/taskservice/ -race -count=20 -timeout 1800s -run 'TestRefreshTaskStorageRecoversAfterInitialFailure|TestRefreshTaskStorageOperationCompletesBeforeRetirement'
go test ./pkg/taskservice/ -count=1 -timeout 1800s

All green (focused 27 tests including the 9 new; full package includes the
sql-mock trigger/acquire-complete and SQL-task executor tests).

Residual risks

• Test-only change; the production fix shipped earlier in fix: make task and embedded lifecycles generation-safe #27135, which never
  referenced taskservice: stale refresh can close active task store and surface as "sql: database is closed" #24144 — hence the non-closing reference here.
• The ordering test's oracle uses goroutine-stack inspection matched against the
  refresh frame and sync.(*RWMutex).Lock/runtime_SemacquireRWMutex; a future
  refresh redesign fails the test at its 15 s bound with an accurate message
  (intended regression behavior, verified by the mutant runs).
• The new tests are white-box (package taskservice), consistent with the
  existing holder tests; require assertions stay outside mu.RLock
  (holderState() helper) and barrier releases precede worker join and holder
  close in defer order.

[qodo-code-review]

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]

mo-bot review · mo-bot-local · done

Commit: 34f751cf801e0d8015dbdcc38e97dea23614f454

Review completed. Read the assessment.

[XuPeng-SH]

mo-bot review · mo-bot-local · review

Commit: 34f751cf801e0d8015dbdcc38e97dea23614f454

Assessment: approve

Reviewed the complete test-only delta and storage lifecycle owners. The closure-sensitive fixture strengthens existing coverage with observable operation results, retirement counts and ordering assertions. Barrier release, worker joining and holder cleanup are correctly ordered. No actionable defects found; production behavior and SQL performance are unchanged.

Model: gpt-6-astra / xhigh

Session: 01a12d8f-8e48-7622-ad17-b605f08450ce · slot 1 · generation 0

Validation:

• All nine new tests passed in normal mode: focused-normal.log, receipt 1791998835797597715-2.json; package execution took 0.088s.

• Recovery and retirement-ordering tests each passed 100 repetitions under -race, with nonempty selections and exit 0: receipts 1791998803618704105-2.json and 1791998803618247719-2.json.

• A diagnostic Go overlay introducing early retirement triggered the intended ordering assertion and exited without hanging: mutation-early-retirement.log, receipt 1791998831341298114-2.json. This expected negative-control failure is not a candidate failure.

• Isolation preflight passed. Reconciled 192 native inputs and 46 retained artifacts against the current sources and repository build receipt before testing.

• Reused successful frozen-head Ubuntu UT and arm64 static-analysis CI. Source integrity, unchanged checkout and diff checks passed; validation-audit.json maps commands, logs, source fingerprints and exits under the assigned diagnostic directory.

Limitations:

• The ordering oracle depends on runtime stack-frame names; local validation covered the current Linux/amd64 toolchain.

• Author-reported historical mutation counts and live SQL replay lacked retained receipts and were not independently credited. No BVT rerun was required for this test-only change.

• The linked issue's full text was absent; its contract was assessed from the supplied PR description and existing SQL overlap case.

[mergify]

Merge Queue Status

• ✅ Entered queue — 2026-10-14 17:30 UTC · Rule: main · triggered by rule Automatic queue on approval for main
• 🚫 Left the queue — 2026-10-14 17:38 UTC · at 34f751cf801e0d8015dbdcc38e97dea23614f454

This pull request spent 8 minutes 43 seconds in the queue, with no time running CI.

Reason

Pull request #30147 has been dequeued

Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.

The author needs to enable "Allow edits from maintainers" on this pull request.

Hint

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Tick the box to put this pull request back in the merge queue (same as @mergifyio queue).

• Requeue this pull request