From e01c91dfbf5c7acb79332250b73202ae76c94dae Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 29 Oct 2026 20:47:56 +0000 Subject: [PATCH 1/3] test(state/redis): cover ExtendLock with a sub-millisecond TTL ExtendLock with a positive TTL under 1 ms deletes the lock and still returns true. This test fails until the fix lands. Refs #125. --- state/redis/redis_test.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/state/redis/redis_test.go b/state/redis/redis_test.go index 5a5cef3..7fd09c5 100644 --- a/state/redis/redis_test.go +++ b/state/redis/redis_test.go @@ -38,6 +38,34 @@ func TestStateConformance(t *testing.T) { }) } +func TestExtendLockSubMillisecondTTLKeepsLock(t *testing.T) { + server := miniredis.RunT(t) + client := goredis.NewClient(&goredis.Options{Addr: server.Addr()}) + t.Cleanup(func() { + _ = client.Close() + }) + ctx := context.Background() + state, err := chatredis.New(ctx, chatredis.Options{Client: client, Prefix: "test"}) + if err != nil { + t.Fatalf("new redis state: %v", err) + } + + lease, acquired, err := state.AcquireLock(ctx, "thread", time.Minute) + if err != nil || !acquired { + t.Fatalf("AcquireLock = %v, %v; want true, nil", acquired, err) + } + extended, err := state.ExtendLock(ctx, lease, 500*time.Microsecond) + if err != nil || !extended { + t.Fatalf("ExtendLock = %v, %v; want true, nil", extended, err) + } + if !server.Exists("test:lock:thread") { + t.Fatal("lock key was deleted by ExtendLock") + } + if _, acquired, err := state.AcquireLock(ctx, "thread", time.Minute); err != nil || acquired { + t.Fatalf("second AcquireLock = %v, %v; want false, nil", acquired, err) + } +} + func TestStateConformanceWithRedisContainer(t *testing.T) { testcontainers.SkipIfProviderIsNotHealthy(t) From e6f8c6829c2f3f40e075f30f0b6281d5a29c45a4 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 29 Oct 2026 20:48:30 +0000 Subject: [PATCH 2/3] fix(state/redis): round sub-millisecond ExtendLock TTL up to 1 ms ExtendLock sent ttl.Milliseconds() to PEXPIRE. A positive TTL under 1 ms became 0, Redis deleted the lock, and ExtendLock still returned true. Round it up to 1 ms, as go-redis does on its SetNX path, and update the README contract. MarkEvent and AcquireLock already get this rounding from go-redis SetNX. --- state/redis/README.md | 14 +++++--------- state/redis/redis.go | 2 +- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/state/redis/README.md b/state/redis/README.md index 6729c2b..04e3a90 100644 --- a/state/redis/README.md +++ b/state/redis/README.md @@ -69,14 +69,10 @@ with its own `go.mod` [go.mod:module]. `redis.go` imports the Go methods return `false` [redis.go:extendScript] [redis.go:releaseScript] [redis.go:State.ExtendLock] [redis.go:State.ReleaseLock]. -- `ExtendLock` passes `ttl.Milliseconds()` to the script, so it truncates - sub-millisecond precision [redis.go:State.ExtendLock]. -- A positive `ttl` under 1 ms therefore reaches the script as `0`. Redis - deletes a key given a non-positive `PEXPIRE` and returns `1`, so - `ExtendLock` deletes the lock and still returns `true` - [redis.go:State.ExtendLock] [redis.go:extendScript]. The runtime renews - with `ThreadLockTTL` (default 2 minutes), so only a sub-millisecond - configuration reaches this path. +- `ExtendLock` passes `ttl` to the script in whole milliseconds, so it + truncates sub-millisecond precision. A positive `ttl` under 1 ms becomes + 1 ms, as go-redis does for `SetNX`, so `PEXPIRE` never gets `0` and never + deletes the lock [redis.go:State.ExtendLock] [redis.go:extendScript]. - Redis errors from the state methods are returned unwrapped. `New` wraps a ping error as `"redis state: ping: %w"` [redis.go:New]. - `Shutdown` closes the client at most once. It returns the `Close` error from @@ -97,7 +93,7 @@ with its own `go.mod` [go.mod:module]. `redis.go` imports with the token and the caller's `ttl` [redis.go:State.AcquireLock] [redis.go:newToken]. - `ExtendLock` sends `EVAL extendScript` on `:lock:` with - `ARGV = [token, ttl in ms]` [redis.go:State.ExtendLock]. + `ARGV = [token, ttl in ms, at least 1]` [redis.go:State.ExtendLock]. - `ReleaseLock` sends `EVAL releaseScript` on `:lock:` with `ARGV = [token]` [redis.go:State.ReleaseLock]. - `Shutdown` calls `Close` on the caller-supplied `redis.UniversalClient` diff --git a/state/redis/redis.go b/state/redis/redis.go index 9067b98..b776f51 100644 --- a/state/redis/redis.go +++ b/state/redis/redis.go @@ -109,7 +109,7 @@ func (s *State) ExtendLock(ctx context.Context, lease chat.LockLease, ttl time.D if ttl <= 0 { return false, errors.New("redis state: lock ttl must be positive") } - result, err := s.client.Eval(ctx, extendScript, []string{s.key("lock", lease.Key)}, lease.Token, ttl.Milliseconds()).Int() + result, err := s.client.Eval(ctx, extendScript, []string{s.key("lock", lease.Key)}, lease.Token, max(ttl.Milliseconds(), 1)).Int() if err != nil { return false, err } From 2874b46cec05c7e5796dfb6c99697cfc0aa46eda Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 29 Oct 2026 20:56:17 +0000 Subject: [PATCH 3/3] test(state/redis): cover a 1 ns ExtendLock TTL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A round-to-nearest fix passes the 500 µs case but still sends 0 for 1 ns. Write the extend ARGV in the README as two values. --- state/redis/README.md | 2 +- state/redis/redis_test.go | 31 ++++++++++++++++++------------- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/state/redis/README.md b/state/redis/README.md index 04e3a90..a62a308 100644 --- a/state/redis/README.md +++ b/state/redis/README.md @@ -93,7 +93,7 @@ with its own `go.mod` [go.mod:module]. `redis.go` imports with the token and the caller's `ttl` [redis.go:State.AcquireLock] [redis.go:newToken]. - `ExtendLock` sends `EVAL extendScript` on `:lock:` with - `ARGV = [token, ttl in ms, at least 1]` [redis.go:State.ExtendLock]. + `ARGV = [token, max(ttl in ms, 1)]` [redis.go:State.ExtendLock]. - `ReleaseLock` sends `EVAL releaseScript` on `:lock:` with `ARGV = [token]` [redis.go:State.ReleaseLock]. - `Shutdown` calls `Close` on the caller-supplied `redis.UniversalClient` diff --git a/state/redis/redis_test.go b/state/redis/redis_test.go index 7fd09c5..5c141d2 100644 --- a/state/redis/redis_test.go +++ b/state/redis/redis_test.go @@ -50,19 +50,24 @@ func TestExtendLockSubMillisecondTTLKeepsLock(t *testing.T) { t.Fatalf("new redis state: %v", err) } - lease, acquired, err := state.AcquireLock(ctx, "thread", time.Minute) - if err != nil || !acquired { - t.Fatalf("AcquireLock = %v, %v; want true, nil", acquired, err) - } - extended, err := state.ExtendLock(ctx, lease, 500*time.Microsecond) - if err != nil || !extended { - t.Fatalf("ExtendLock = %v, %v; want true, nil", extended, err) - } - if !server.Exists("test:lock:thread") { - t.Fatal("lock key was deleted by ExtendLock") - } - if _, acquired, err := state.AcquireLock(ctx, "thread", time.Minute); err != nil || acquired { - t.Fatalf("second AcquireLock = %v, %v; want false, nil", acquired, err) + for _, ttl := range []time.Duration{time.Nanosecond, 500 * time.Microsecond} { + t.Run(ttl.String(), func(t *testing.T) { + key := "thread-" + ttl.String() + lease, acquired, err := state.AcquireLock(ctx, key, time.Minute) + if err != nil || !acquired { + t.Fatalf("AcquireLock = %v, %v; want true, nil", acquired, err) + } + extended, err := state.ExtendLock(ctx, lease, ttl) + if err != nil || !extended { + t.Fatalf("ExtendLock = %v, %v; want true, nil", extended, err) + } + if !server.Exists("test:lock:" + key) { + t.Fatal("lock key was deleted by ExtendLock") + } + if _, acquired, err := state.AcquireLock(ctx, key, time.Minute); err != nil || acquired { + t.Fatalf("second AcquireLock = %v, %v; want false, nil", acquired, err) + } + }) } }