From 133ef49de26ae251c2c86417f9c673ebc7166f76 Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Thu, 11 Jun 2026 08:40:10 +0300 Subject: Fix unsafe time.Timer.Reset on active timers in runlock and filelock Both filelock.AcquireExclusive and askcli.waitOrAcquireAskLockFD created a single time.Timer and called Reset() on it each retry iteration before it had necessarily fired. Per the Go timer API, Reset()-ing a timer that may still be pending is unsafe: a stale value can already be queued on the channel, causing a spurious early wake-up. Replace the reused-timer + Reset() pattern with a fresh time.After(...) per loop iteration, which guarantees a clean full retry interval (or context cancellation) every time and removes the reuse-while-active hazard. Dropped the now-unused *time.Timer parameter from waitOrAcquireAskLockFD and introduced named retry-interval constants. Add a context-cancel-while-blocked test for the runlock retry loop. Co-Authored-By: Claude Opus 4.8 --- internal/askcli/runlock.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) (limited to 'internal/askcli/runlock.go') diff --git a/internal/askcli/runlock.go b/internal/askcli/runlock.go index 9fbe0cd..f6a3cc6 100644 --- a/internal/askcli/runlock.go +++ b/internal/askcli/runlock.go @@ -66,13 +66,15 @@ func writeLockMetadata(f *os.File, pid int, comm string) error { return f.Sync() } +// askLockRetryInterval is the backoff between successive non-blocking lock attempts. +const askLockRetryInterval = 5 * time.Millisecond + // waitOrAcquireAskLockFD tries to take an exclusive lock on f, or blocks until ctx ends. // On success it writes lock metadata and returns an unlock function (which closes f). func waitOrAcquireAskLockFD( ctx context.Context, f *os.File, comm string, - retryTimer *time.Timer, ) (func() error, error) { for { err := filelock.TryExclusive(f) @@ -100,12 +102,16 @@ func waitOrAcquireAskLockFD( // Intentional no-op: contention is resolved only by waiting for flock release. } - retryTimer.Reset(5 * time.Millisecond) + // Use a fresh timer per iteration via time.After instead of reusing and + // Reset()-ing a single timer. Reset() on a timer that may still be pending is + // the documented Go timer hazard: a stale value can already be queued on the + // channel and trigger a spurious early wake-up. A new timer each loop guarantees + // a clean, full askLockRetryInterval delay (or ctx cancellation). select { case <-ctx.Done(): _ = f.Close() return nil, ctx.Err() - case <-retryTimer.C: + case <-time.After(askLockRetryInterval): } } } @@ -119,12 +125,10 @@ func acquireAskRepoLock(ctx context.Context, gitRoot string) (func() error, erro } comm := lockProcessLabel() - retryTimer := time.NewTimer(5 * time.Millisecond) - defer retryTimer.Stop() f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600) if err != nil { return nil, fmt.Errorf("ask lock: open %s: %w", lockPath, err) } - return waitOrAcquireAskLockFD(ctx, f, comm, retryTimer) + return waitOrAcquireAskLockFD(ctx, f, comm) } -- cgit v1.2.3