fix(lockservice): defer active txn retries to next sweep

[aptend]

Summary

• remove the effectively unbounded same-service retry loop from cleanCommitState
• probe each service at most once per timer sweep so one retryable failure cannot starve later services
• derive the GetActiveTxn RPC timeout from the cleaner task context so allocator shutdown cancels an in-flight probe
• retain unexpired commit-control state on retryable errors and retry on the next sweep
• honor inactive-service retention expiry even when the current probe is retryable
• preserve generation-watermark protection and disconnected-service tombstone semantics

Tests

• retryable and healthy services both make progress in one sweep
• retryable state is retained and no tight retry loop occurs
• cancellation releases an in-flight probe
• expired retryable service state is cleaned with watermark protection for newer state
• existing stale-snapshot and disconnected-service behavior remains covered
• full pkg/lockservice
• targeted repeated and race tests

Fixes #25206

[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]

Blocking finding

[P1] Do not refresh the inactive-service retention window on every persistent disconnect error.

In cleanCommitState, the non-retryable branch still executes l.inactiveService.Store(sid, time.Now()) on every sweep. ErrBackendClosed and ErrBackendCannotConnect enter this branch. Because the services commitCtl is now intentionally retained, the service remains in services and is probed again every 2 * keepBindTimeout; each failure overwrites the timestamp before it can reach removeDisconnectDuration.

Therefore the common permanent-disconnect case never enters expiredUnknownServices: both the cannot-commit tombstones and inactive-service marker remain indefinitely. The new expiry test only starts with an already-old timestamp and then returns a retryable error, so it does not cover this path.

I reproduced this deterministically with keepBindTimeout=1ms, removeDisconnectDuration=20ms, one cannot-commit state, and a probe that always returns moerr.NewBackendClosedNoCtx(): after 250ms the control state was still present, while every sweep refreshed the timestamp.

Please preserve the first-disconnect time/generation instead of overwriting it (for example, LoadOrStore or an explicit inactive epoch), and conditionally expire only the epoch that was observed so a concurrent re-disconnect is not deleted. Keep the existing commit-state generation watermark when removing tombstones. Add regression coverage for a persistent non-retryable error and for a fresh disconnect racing an expiry sweep.

Non-blocking scaling note: probes remain serial, so a sweep can still take N * defaultRPCTimeout when many services time out. The PR fixes the single-service infinite retry, but bounded concurrency or a total sweep budget would provide stronger fleet-scale fairness.

Validation performed: full pkg/lockservice, targeted race tests repeated 5 times, go vet ./pkg/lockservice, and git diff --check; all existing tests pass.

[aptend]

Addressed the blocking retention finding in commit 0976657:

• preserve the first failure time of a disconnected epoch with LoadOrStore, so persistent ErrBackendClosed / ErrBackendCannotConnect probes cannot refresh retention forever;
• expire only the exact marker observed by the sweep with CompareAndDelete, so a resume + fresh disconnect racing the sweep is not deleted;
• keep the existing commit-state generation watermark when removing expired tombstones;
• add deterministic coverage for a persistent non-retryable disconnect and for a fresh disconnect racing expiry.

Validation:

• go test ./pkg/lockservice
• targeted tests, -count=10
• targeted race tests, -count=5
• go vet ./pkg/lockservice
• git diff --check

@XuPeng-SH please re-review when convenient.

[XuPeng-SH]

Blocking finding

[P1] Bind the tombstone cleanup decision to the same inactive-service epoch, not only the marker deletion.

CompareAndDelete correctly prevents expireInactiveService from deleting a fresh marker, but after it successfully deletes the old marker, expiredUnknownServices[sid] remains a service-level boolean for the rest of the sweep (lines 675-687). If that service resumes and disconnects again before/during the probe, a fresh marker is installed. A retryable probe then reaches lines 727-732, and lines 747-752 still call cleanCtlLocked(..., preserveCannotCommit=false, ...) solely because the old epoch expired. The commit-state generation watermark does not protect an existing cannot-commit state that was not refreshed after the watermark.

I reproduced this deterministically by starting with an expired marker and a cannotCommit state, then having the probe callback simulate resume + fresh disconnect (Delete followed by markInactiveService) and return TxnNeedRetry. After the sweep, the fresh inactive marker remains but the cannot-commit tombstone is deleted. The reproduction failed 10/10 runs. A later ResumeInvalidCN can remove that fresh marker while CheckOrphan has already lost the tombstone.

The new TestExpireInactiveServicePreservesFreshDisconnectEpoch only stops after proving the marker survived; it does not run the remainder of cleanCommitState, so it misses this second-stage race.

Please make expiry/cleanup epoch-aware as one atomic lifecycle decision. For example, represent inactive epochs with an identity/generation and synchronize markInactiveService, ResumeInvalidCN, and the final cleanup decision so cleanup is allowed only if the exact expired epoch is still the terminal epoch. Do not rely on a final unsynchronized Load, since a fresh disconnect can race immediately after it. Add an integrated cleaner regression covering: old epoch expires -> resume -> fresh disconnect -> retryable/non-retryable probe -> fresh marker and pre-existing tombstone both remain. Preserve the existing commit-state generation watermark for states created/refreshed while the probe is in flight.

The original unbounded retry/fairness/cancellation fix otherwise looks correct, and the previous persistent-disconnect timestamp-refresh finding is fixed.

[aptend]

Closing this PR because #25651 has already covered the main correctness and recovery issues addressed here:

• bounded active-txn probing instead of an unbounded retry loop
• context-aware cancellation
• preservation of commit-control/tombstone state on unknown service state
• inactive-service expiry, cleanup watermarks, and recovery-epoch protection
• MORPC backend reset and CN hot-recreation recovery

The remaining behavioral difference is retry fairness within one cleaner sweep. Current main retries connection failures up to 3 times with 100ms backoff before moving to the next service, while this PR probes each service only once per sweep. The fixed backoff is about 200ms, but total delay can also include RPC latency (each probe is bounded by the 10s RPC timeout).

That remaining tradeoff is substantially smaller than the original issue and does not justify merging this now-conflicting implementation. If strict one-probe-per-service fairness becomes necessary, it can be proposed as a small follow-up based on the current cleanCommitStateOnce implementation.