mvcc: avoid a non-positive interval when resetting the watch resync ticker

[martin-k-m]

Fixes #22296

What

Extract the delay computation from syncWatchersLoop into a small helper and make it fall back
to watchResyncPeriod when the measured duration is not positive.

// resyncDelay returns how long syncWatchersLoop waits before calling
// syncWatchers again. When the last pass made progress but watchers are still
// unsynced, it yields the time that pass took so that store operations are not
// starved. Otherwise it falls back to watchResyncPeriod.
//
// The returned duration is always positive. time.Ticker.Reset panics on a
// non-positive interval, and syncDuration can be zero on platforms whose
// monotonic clock resolution is coarser than a sync pass takes.
func resyncDelay(syncDuration time.Duration, lastUnsyncedWatchers, unsyncedWatchers int) time.Duration {
	// more work pending?
	if unsyncedWatchers != 0 && lastUnsyncedWatchers > unsyncedWatchers && syncDuration > 0 {
		return syncDuration
	}
	return watchResyncPeriod
}

The loop body becomes a single delayTicker.Reset(resyncDelay(...)), replacing the previous
reset-then-maybe-reset-again pair.

Behaviour change

Only for the case that currently panics. Whenever syncDuration > 0, the returned delay is
identical to today. When it measures as zero, the loop waits watchResyncPeriod instead of
crashing.

I considered clamping to a small non-zero minimum instead, so that a very fast pass still loops
promptly rather than waiting the full 100ms. I did not do that because it introduces a new tuning
constant, and a pass that completes in under the clock resolution had almost nothing to do. Happy
to switch if reviewers prefer it.

Test

TestResyncDelay is a table test over the four combinations that matter, including the
zero-duration case. Each case asserts the expected delay, asserts it is positive, and calls
time.Ticker.Reset with the value so that the actual panic condition is exercised rather than
only approximated.

Test evidence

Run on Linux, golang:1.26 container. The test is platform independent, so it reproduces the
bug on Linux too even though the panic itself only fires on Windows.

Without the fix (guard removed, helper and test present):

=== RUN   TestResyncDelay
=== RUN   TestResyncDelay/progress_with_unmeasurable_sync_duration_falls_back_to_resync_period
    watchable_store_test.go:1036:
        	Error:      	Not equal:
        	            	expected: 100ms
        	            	actual  : 0s
        	Test:       	TestResyncDelay/progress_with_unmeasurable_sync_duration_falls_back_to_resync_period
--- FAIL: TestResyncDelay (0.00s)
    --- PASS: TestResyncDelay/no_progress_falls_back_to_resync_period (0.00s)
    --- PASS: TestResyncDelay/all_watchers_synced_falls_back_to_resync_period (0.00s)
    --- PASS: TestResyncDelay/progress_with_work_pending_yields_time_taken (0.00s)
    --- FAIL: TestResyncDelay/progress_with_unmeasurable_sync_duration_falls_back_to_resync_period (0.00s)
FAIL
FAIL	go.etcd.io/etcd/server/v3/storage/mvcc	0.014s

With the fix:

=== RUN   TestResyncDelay
--- PASS: TestResyncDelay (0.00s)
    --- PASS: TestResyncDelay/no_progress_falls_back_to_resync_period (0.00s)
    --- PASS: TestResyncDelay/all_watchers_synced_falls_back_to_resync_period (0.00s)
    --- PASS: TestResyncDelay/progress_with_work_pending_yields_time_taken (0.00s)
    --- PASS: TestResyncDelay/progress_with_unmeasurable_sync_duration_falls_back_to_resync_period (0.00s)
PASS
ok  	go.etcd.io/etcd/server/v3/storage/mvcc	0.012s

Original panic, on Windows 11 native, before and after the fix, 5 runs each of
go test -count=1 -run "TestWatch" ./storage/mvcc/:

before:  run 1: PANIC   run 2: PANIC   run 3: PANIC   run 4: PANIC   run 5: PANIC
after:   run 1: no panic  run 2: no panic  run 3: no panic  run 4: no panic  run 5: no panic

Full server module unit suite on Linux: no failures.

Lint:

### LINT: fix-watch-resync-ticker-panic ###
0 issues.
'lint' PASSED and completed at Sat Aug 15 14:31:31 UTC 2026
SUCCESS

Not fixed here

While reproducing this I hit two other Windows-only failures in the same package. Both look
unrelated and are left alone:

• TestStoreRev fails in cleanup with TempDir RemoveAll cleanup: unlinkat ...: The process cannot access the file because it is being used by another process, which is bbolt's mmap
  holding a handle open.
• TestWatchVictims times out. There is already an open PR, mvcc: Deflake TestWatchVictims with synctime #22269, "mvcc: deflake
  TestWatchVictims with synctime".

Checklist

• Test fails without the fix and passes with it, both states shown above
• make verify-lint clean
• Full server module unit tests pass on Linux
• Original panic no longer reproduces on Windows
• DCO sign-off on the commit
• Issue filed and referenced (mvcc: syncWatchersLoop can panic with "non-positive interval for Ticker.Reset" where the monotonic clock is coarse (windows/amd64) #22296)
• Rebased on current main

A note on the test, so a reviewer is not misled

TestResyncDelay covers resyncDelay directly. It cannot be run against
unpatched main to show it failing, because the function it tests is the one
this change introduces: reverting watchable_store.go alone leaves the test
unable to compile. What is demonstrable without the patch is the mechanism, and
both halves of it were measured on this machine (Windows 11, go1.26.6):

time.Since(time.Now()) == 0 in 199999 of 200000 samples
Ticker.Reset(0) panics: non-positive interval for Ticker.Reset

So syncDuration is zero on this platform essentially whenever a sync pass is
shorter than the clock's resolution, and a zero interval is exactly what
Ticker.Reset refuses. The guard is syncDuration > 0.

---

This PR was written in part with the assistance of generative AI. Every change was reviewed, built and tested before submitting, and no AI co-author or assisted-by trailers are used, per https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#ai-guidance

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: martin-k-m
Once this PR has been reviewed and has the lgtm label, please assign ahrtr for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kubernetes-prow]

Hi @martin-k-m. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[Deln0r]

Verified locally on darwin/arm64, go1.26.6. The mvcc suite is green on this branch with -count=1 and with -race. The premise checks out mechanically: time.Ticker.Reset(0) and Reset(-1ns) both panic with non-positive interval for Ticker.Reset, while Reset(1ns) is accepted.

Two things that strengthen the case for fixing it. etcd ships a windows/amd64 artifact, etcd-v3.7.1-windows-amd64.zip, so this is a crash in a released binary rather than only a developer-machine annoyance. And line 246 is the only Ticker.Reset in the repository fed a measured duration; the one other Ticker.Reset takes a constant, so there is no sibling site needing the same treatment.

On the open question about clamping, I would prefer the clamp, for three reasons.

Precedent for exactly this shape exists twice in the repo. backend.go computes warningTimeout from the database size and raises it to minSnapshotWarningTimeout before time.NewTicker, and v3rpc/watch.go raises a configured interval to minWatchProgressInterval. Clamping a computed duration before handing it to the timer package is the established idiom here.

The fallback changes the loop's cadence on the platform being fixed, by a large factor. When a pass measures zero, the loop now waits 100ms rather than roughly the time the pass took. On this machine a full 512-watcher batch runs 0.5ms to 1.1ms and a partial pass runs in tens of microseconds, so on a platform whose tick is coarser than the pass, a yield that was microseconds becomes 100ms. That is a three-orders-of-magnitude change in resync cadence, on exactly the platform that motivated the patch.

The cost you weighed against a clamp does not quite apply. max(syncDuration, time.Microsecond) introduces no tuning knob, it only says the yield is at least a microsecond, and it leaves behaviour identical everywhere the clock can measure the pass at all.

That said, the fallback is the more conservative of the two and it does fix the crash, so treat this as a preference rather than a blocker.

Three smaller points.

1. resyncDelay collides with a local variable of the same name in TestWatchRestore, watchable_store_test.go:585, in the very file this PR edits. It compiles and the linters are quiet, but renaming one of the two would spare the next reader.

2. The doc comment says the returned duration is always positive. That is not structurally guaranteed, since watchResyncPeriod is a package var carrying the comment "non-const so modifiable by tests". A future test that sets it to zero reintroduces the panic while TestResyncDelay keeps passing. Taking the period as a parameter would close that and let the test vary it.

3. You are right that TestResyncDelay cannot be a regression test, and saying so plainly in the PR is the right call. As written, require.Positivef and require.NotPanics cannot fail once the preceding require.Equal passes, because every want is a positive constant. A case asserting that Reset(0) does panic would at least show the guard has a live target.

[serathius]

Never seen the issue in production, doesn't seem like a real issue.

[Deln0r]

Correcting my own comment above. I offered the windows/amd64 release artifact as evidence that this is more than a developer-machine annoyance, and that argument is weaker than I made it sound. The supported-platforms policy puts AMD64 Windows in Tier 3, where the guarantee is that etcd builds, testing may be light or absent, and the platform is to be considered unstable; Tier 3 CI requires a successful build and nothing more. A shipped binary is not the same as a supported runtime, and I should have checked the tier before leaning on the artifact.

What holds is narrower. The mechanism is real, and it is not reachable on Tier 1: the branch runs only after a pass that made measurable progress, and at nanosecond granularity such a pass does not measure zero. That is consistent with @serathius not having seen it in production.

Whether a Tier 3 runtime bug earns a patch is a maintainer call and I am not pressing one. Putting the tier on the record here mainly for whoever decides, and for #22296.

[serathius]

What? So what's the impact of this change? Respond in 1 sentence or using prose.

[Deln0r]

Nothing on Linux, where the duration is never zero. On Windows a panic that kills the process becomes a 100ms wait