think
16px
820px
← all files

Task 10 Report: RedisPool — cluster-wide slots for the HA pair

Status: DONE

Resolved and committed. See "Resolution (applied)" at the top for what
changed after the initial NEEDS_CONTEXT block below; the rest of this
document is preserved as the original diagnosis for the record.

Resolution (applied)

Team lead confirmed this was a plan defect and picked Option A: seed an
already-expired lease directly instead of trying to time-travel miniredis.
Rationale: this exercises the actual production reclaim path (the Lua
ZREMRANGEBYSCORE prune in acquireScript) deterministically, without
depending on FastForward semantics it was never meant to have.

Changes made:

  1. internal/pool/redispool_test.go — replaced
    TestLeaseExpiryFreesSlotAfterCrash with the team lead's version: seeds
    gwslots:u1 with a lease (crashed-holder-lease) whose score is
    time.Now().Add(-1*time.Second).UnixMilli() (already expired), then
    asserts a fresh NewRedisPool instance's Enqueue gets admitted. Added
    "context" to imports for the context.Background() call.
  2. internal/pool/redispool.go — removed the now-unused stopRefresh()
    test hook (YAGNI, per instruction). Nothing else in production code
    changed; Release's per-lease refresh cancel-map is untouched.

Proof the test pins the prune path (per instruction, before declaring
GREEN): temporarily replaced the script body's first line
(redis.call('ZREMRANGEBYSCORE', KEYS[1], '-inf', ARGV[1])) with a comment,
leaving only the batch-ZSET prune:

=== RUN   TestLeaseExpiryFreesSlotAfterCrash
    redispool_test.go:57: expired lease never reclaimed
--- FAIL: TestLeaseExpiryFreesSlotAfterCrash (2.01s)

Confirmed FAIL as expected — the test genuinely exercises that prune call,
not something else (e.g. the batch ZSET, or fail-open). Restored the line;
re-ran:

=== RUN   TestLeaseExpiryFreesSlotAfterCrash
--- PASS: TestLeaseExpiryFreesSlotAfterCrash (0.01s)

GREEN, gofmt -l clean on both files.

Full verification:

$ go test ./internal/pool/ -v -race -count=2
=== RUN   TestCapacityAndFIFO
--- PASS: TestCapacityAndFIFO (0.05s)
=== RUN   TestInteractiveJumpsBatch
--- PASS: TestInteractiveJumpsBatch (0.05s)
=== RUN   TestBatchMaxLeavesHeadroom
--- PASS: TestBatchMaxLeavesHeadroom (0.05s)
=== RUN   TestTwoInstancesShareCap
--- PASS: TestTwoInstancesShareCap (0.33s)
=== RUN   TestLeaseExpiryFreesSlotAfterCrash
--- PASS: TestLeaseExpiryFreesSlotAfterCrash (0.01s)
=== RUN   TestFailOpenWhenRedisDown
--- PASS: TestFailOpenWhenRedisDown (0.01s)
... (repeats identically on the count=2 pass)
PASS
ok      datahive.id/ahu-gpu-manager/internal/pool   2.014s

$ go build ./cmd/gateway
BUILD OK
$ go test ./... -count=1
ok      datahive.id/ahu-gpu-manager/internal/audit
ok      datahive.id/ahu-gpu-manager/internal/config
ok      datahive.id/ahu-gpu-manager/internal/idem
ok      datahive.id/ahu-gpu-manager/internal/pool
ok      datahive.id/ahu-gpu-manager/internal/proxy
ok      datahive.id/ahu-gpu-manager/internal/registry
ok      datahive.id/ahu-gpu-manager/internal/server

Commit (main.go guard removal included in the same commit, per
instruction):

dbc8d3a feat(pool): redis-coordinated cluster-wide slots with lease expiry and fail-open
 3 files changed, 238 insertions(+), 1 deletion(-)
 create mode 100644 internal/pool/redispool.go
 create mode 100644 internal/pool/redispool_test.go

(cmd/gateway/main.go modified in the same commit — guard removed, log
import confirmed still used elsewhere.)

Note: a stray gateway binary was left in the repo root by
go build ./cmd/gateway (no .gitignore entry for it) — removed before
staging so it wasn't committed. Not otherwise relevant to this task, flagging
in case it recurs for later tasks that build the binary in-place.

Original diagnosis (preserved for the record)

Original status: NEEDS_CONTEXT (blocked on one test)

Everything in the brief was implemented verbatim. Two of the three new tests
pass; one (TestLeaseExpiryFreesSlotAfterCrash) fails deterministically
because the brief's exact test code and exact implementation code are
mutually incompatible given miniredis v2.38.0's real semantics for
FastForward. Details, evidence, and options below.

Files changed

  • internal/pool/redispool_test.go — created, byte-for-byte identical to the
    brief's Step 1 code (verified by diff).
  • internal/pool/redispool.go — created, identical to the brief's Step 3
    code except for gofmt struct-field alignment (cosmetic only; verified by
    diff).
  • cmd/gateway/main.go — removed the Task-9 guard:
    go if cfg.Coordination == "redis" { pools[u.ID] = pool.NewRedisPool(rdb, u.ID, u.Slots) } else { pools[u.ID] = pool.NewLocal(u.Slots) }
    Confirmed log is still used elsewhere in main.go (config/redis-url/listen
    fatals, drain log) — import kept, go build ./cmd/gateway succeeds.

(At the time this section was written, no commit had been made — see "Why I
stopped" below. It has since been resolved and committed; see "Resolution
(applied)" at the top.)

TDD evidence

RED (Step 2 of the brief):

$ go test ./internal/pool/ -run TestTwoInstances -v
internal/pool/redispool_test.go:26:8: undefined: NewRedisPool
...
FAIL    datahive.id/ahu-gpu-manager/internal/pool [build failed]

GREEN after implementing redispool.go, target test:

$ go test ./internal/pool/ -run TestTwoInstances -v
=== RUN   TestTwoInstancesShareCap
--- PASS: TestTwoInstancesShareCap (0.40s)
PASS

Full pool suite, -race -count=2 (Task 3 regressions + new tests):

=== RUN   TestCapacityAndFIFO
--- PASS: TestCapacityAndFIFO (0.05s)
=== RUN   TestInteractiveJumpsBatch
--- PASS: TestInteractiveJumpsBatch (0.05s)
=== RUN   TestBatchMaxLeavesHeadroom
--- PASS: TestBatchMaxLeavesHeadroom (0.05s)
=== RUN   TestTwoInstancesShareCap
--- PASS: TestTwoInstancesShareCap (0.42s)
=== RUN   TestLeaseExpiryFreesSlotAfterCrash
    redispool_test.go:57: expired lease never reclaimed
--- FAIL: TestLeaseExpiryFreesSlotAfterCrash (2.01s)
=== RUN   TestFailOpenWhenRedisDown
--- PASS: TestFailOpenWhenRedisDown (0.01s)
... (repeats identically on the count=2 pass — deterministic, not a race)
FAIL    datahive.id/ahu-gpu-manager/internal/pool   5.193s

go build ./cmd/gateway: succeeds, no output.

go test ./... -count=1: everything passes except the single test above:

ok      datahive.id/ahu-gpu-manager/internal/audit
ok      datahive.id/ahu-gpu-manager/internal/config
ok      datahive.id/ahu-gpu-manager/internal/idem
--- FAIL: TestLeaseExpiryFreesSlotAfterCrash (2.00s)
FAIL    datahive.id/ahu-gpu-manager/internal/pool
ok      datahive.id/ahu-gpu-manager/internal/proxy
ok      datahive.id/ahu-gpu-manager/internal/registry
ok      datahive.id/ahu-gpu-manager/internal/server

Root cause (verified, not guessed)

The implementation's lease expiry is score-based: tryAcquire computes
now := time.Now().UnixMilli() in the Go client, and the Lua script
prunes ZSET members with ZREMRANGEBYSCORE KEYS[1] '-inf' ARGV[1] (ARGV[1]
= that Go-computed now). Leases are ZADDed with score = now + leaseTTL. This is a pure application-level, real-wall-clock comparison —
Redis/miniredis never sees or computes it; it's just a number sent as a
script argument.

TestLeaseExpiryFreesSlotAfterCrash tries to simulate "30s passed with no
refresh" via mr.FastForward(31 * time.Second). I verified against the
miniredis/v2@v2.38.0 source that FastForward:

// miniredis.go
func (m *Miniredis) FastForward(duration time.Duration) {
    m.Lock()
    defer m.Unlock()
    for _, db := range m.dbs {
        db.fastForward(duration)
    }
}

// db.go
func (db *RedisDB) fastForward(duration time.Duration) {
    for _, key := range db.allKeys() {
        if value, ok := db.ttl[key]; ok {
            db.ttl[key] = value - duration
            db.checkTTL(key)
        }
        ...
    }
}

only decrements the internal countdown for keys that carry a native Redis
TTL (EXPIRE/PEXPIRE/etc., stored in db.ttl), deleting the whole key
once its TTL goes negative. It does not touch m.now (the field read by
effectiveNow(), which backs the TIME command) — only SetTime() does
that. Our ZSET members carry no native TTL at all (they're plain sorted-set
entries), so FastForward has zero effect on them, and it has zero effect on
time.Now() in the Go process either (that's real wall-clock time, entirely
outside miniredis's control).

I confirmed this empirically with a throwaway instrumented test (not
committed) that dumped the ZSET member/score and time.Now().UnixMilli()
immediately before and after stopRefresh() + FastForward(31s):

before stopRefresh+FF: members=[{1.783153363802e+12 u1-...}] now_ms=1783153333806
after  FF:              members=[{1.783153363802e+12 u1-...}] now_ms=1783153333806

The score (now_at_acquire + 30000) and the wall clock are byte-identical
before and after FastForward — nothing moved. So when p2 polls
tryAcquire for up to the test's real 2-second budget, now in the pruning
call is only ~0–2000ms past the original acquire, nowhere near the lease's
30000ms score, so ZREMRANGEBYSCORE never removes it, ZCARD stays at cap,
and the test times out on rpAdmitted. This reproduces on every run
(confirmed via -count=2), so it is a deterministic logic mismatch, not a
flaky timing race.

I also verified I did not introduce this myself: both redispool_test.go and
redispool.go diff byte-identical (test) / gofmt-whitespace-only (impl)
against the brief's Step 1 and Step 3 code blocks.

Why this is a genuine brief contradiction, not an implementation bug

There is no way to satisfy both "exact test code" and "exact implementation
code" as given, because:
- FastForward only affects keys with a real Redis-native TTL; it never
advances any client-observable clock (not time.Now(), not even the Lua
TIME command, since that also reads effectiveNow()/m.now, which
FastForward doesn't touch).
- The given implementation's lease-expiry design is deliberately
score-in-a-shared-ZSET (gwslots:{upstream}, member=leaseID, score=expiry
epoch-ms), which is the correct choice for O(log n) atomic prune+admit via
one Lua call across many concurrent leases — but it is fundamentally
incompatible with FastForward as a time-travel mechanism, regardless of
whether the "now" comparator is computed client-side or via
redis.call('TIME') server-side.
- Making the test pass as-is would require either:
1. changing the test to advance time some other way (e.g. mr.SetTime(...)
— which would work if the script used redis.call('TIME'), since
SetTime does set m.now — but that still means editing the "verbatim"
test), or
2. redesigning lease storage to use one native-TTL key per lease (so
FastForward can delete it), which changes the gwslots:{upstream}
ZSET contract named in the brief's Interfaces section — a schema/design
decision I'm not authorized to make unilaterally, especially since other
tasks (packaging/observatory) may assume this ZSET shape for
introspection (e.g. ZCARD = active-lease count).

Either fix is a legitimate design call, not a "guess the typo" fix, so per
your instructions I stopped rather than silently picking one.

Options for resolution (not decided by me)

  • A. Keep the ZSET/score design; change the test to seed an
    already-expired lease directly (e.g. rdb.ZAdd with a past score, or
    mr.SetTime + Lua redis.call('TIME')) instead of FastForward.
  • B. Keep the test's use of FastForward; redesign lease storage to use
    one native-TTL Redis key per lease (PEXPIRE 30s) with SCAN/keyspace
    notifications for counting, dropping the shared-ZSET pruning approach.
  • C. Keep both as-is and accept this test only exercises real Redis in
    integration (skip/adjust it for the miniredis unit-test tier).

(At the time, nothing had been committed. Team lead picked Option A —
see "Resolution (applied)" at the top for the final, committed outcome.)

Self-review of what's otherwise in place

  • NewRedisPool(rdb redis.UniversalClient, upstreamID string, s config.Slots) Pool —
    signature matches exactly what main.go now calls.
  • Lua script prunes both ZSETs (ZREMRANGEBYSCORE) before counting via
    ZCARD, per KEYS[1]/KEYS[2] — matches brief.
  • Batch cap enforced via the second ZSET only when ARGV[6] == '1'
    (class == config.ClassBatch) — matches brief.
  • rticket.once guards double-close of cancel in Cancel(). Release()
    ZRems both all and batch keys when class == ClassBatch. Stage-2
    goroutine in Enqueue selects on tick.C/t.cancel and returns (no leak)
    on cancel; startRefresh's goroutine selects on ctx.Done()/tick.C and
    is cancelled from Release. (stopRefresh() was originally added as an
    unexported test hook for the FastForward-based test; it's unused after
    the Option-A rewrite and was deleted per the team lead's YAGNI note.)
  • Fail-open: tryAcquire returns (false, err) on Redis error; Enqueue's
    goroutine closes t.ready on any such error (local admission already
    happened in Stage 1), which is exactly what
    TestFailOpenWhenRedisDown exercises, and it passes.

Post-review fix pass (code review findings, commit 4329116)

Three findings from the code review of this task's diff were fixed in
internal/pool/redispool.go (+ redispool_test.go), TDD'd RED→GREEN, then
committed as 4329116 fix(pool): release local slot on stage-2 cancel; conditional lease refresh; ULID lease ids.

Defect 1 (CRITICAL): local slot leak on Cancel during stage-2 wait

Problem: rticket.Cancel() closes t.cancel and calls t.inner.Cancel()
— a no-op once the inner local ticket is already admitted (see
pool.go's ticket.Cancel(): if t.admitted { return }). The stage-2
goroutine's <-t.cancel exits then returned without releasing the
already-admitted inner ticket, so the local slot (localPool.active) was
never decremented. It leaked permanently on every cancel that landed after
stage-1 admission but before/during stage-2's cluster wait.

RED (TestCancelDuringStage2ReleasesLocalSlot, new test): admit a holder
via p1 to fill the single cluster-wide slot, then Enqueue a waiter on a
second instance p2 (stage-1 admits it locally immediately since p2's
local pool is independent/empty; stage-2 then polls forever because the
cluster is full), let it settle in the poll loop, Cancel() it, and assert
p2.Active() returns to 0 within 2s:

=== RUN   TestCancelDuringStage2ReleasesLocalSlot
    redispool_test.go:83: local slot leaked after stage-2 cancel: Active()=1
--- FAIL: TestCancelDuringStage2ReleasesLocalSlot (2.32s)

Fix: added a releaseInner() closure in the Enqueue goroutine that
drains-releases the inner ticket only if it was actually admitted (select
on t.inner.Ready() with a default, to correctly handle the race where
stage-1's own select picks the <-t.cancel branch at the same instant the
inner ticket becomes ready). Called before return in both cancel-exit
paths: stage-1's initial select and the stage-2 poll loop's select. The
acquired-lease exit path (ok == true) is untouched, as instructed — that
path already releases correctly via the ticket holder's own Release().

GREEN:

=== RUN   TestCancelDuringStage2ReleasesLocalSlot
--- PASS: TestCancelDuringStage2ReleasesLocalSlot (0.33s)

Defect 2 (IMPORTANT): refresh resurrects reclaimed leases

Problem: the refresh goroutine ran two unconditional ZAdds on every
tick. If this instance's refresh was delayed past the lease's 30s TTL (e.g.
a Redis partition), another instance could prune the member and admit a
replacement over that slot; when the partition healed, the zombie
refresher's next tick would blindly re-add the pruned member, resurrecting a
lease for a slot that had already been reassigned — pushing the cluster over
its configured capacity with no bound on how long the over-commit persists.

Converted leaseTTL, refreshTick, pollTick from const to package
var (needed so the test can shrink refreshTick to 20ms; production
startup never mutates them).

RED (TestRefreshDoesNotResurrectReclaimedLease, new test, run against
the old unconditional-ZAdd goroutine): shrink refreshTick to 20ms
(save/restore via t.Cleanup), acquire a lease, read its member back via
ZRange and ZRem it directly (simulating another instance's prune), sleep
150ms (several refresh ticks), then assert the member was not resurrected:

=== RUN   TestRefreshDoesNotResurrectReclaimedLease
    redispool_test.go:115: reclaimed lease was resurrected by refresh goroutine: score=1.78315425892e+12
--- FAIL: TestRefreshDoesNotResurrectReclaimedLease (0.19s)

Fix: added refreshScript, a conditional-extend Lua script that only
ZAdds (both the all and, when applicable, batch ZSETs) if ZSCORE
still finds the member in the all ZSET; otherwise returns 0 and does
nothing. The refresh goroutine now runs this script each tick instead of the
two bare ZAdds, and treats err != nil || n == 0 as "stop refreshing" —
covering both "lease was legitimately reclaimed" and "Redis is unreachable,
don't keep hammering it hoping to resurrect a lease that may already be
gone." (The ticket's own Release() still cleans up the p.refresh map
entry and does its own ZRems regardless, so no leak of the cancel-func map
entry.)

GREEN:

=== RUN   TestRefreshDoesNotResurrectReclaimedLease
--- PASS: TestRefreshDoesNotResurrectReclaimedLease (0.16s)

Defect 3 (IMPORTANT): leaseID entropy

Problem: leaseID was fmt.Sprintf("%s-%d", p.id, time.Now().UnixNano()).
Two Enqueue calls landing in the same nanosecond tick (same process, or two
HA instances sharing the same p.id for the same upstream) produce identical
lease IDs; since the acquire/refresh paths ZAdd (upsert) by member name, a
collision silently merges two leases into one ZSET entry, shrinking the
effective cluster-wide count below the real number of active holders — an
undercount that could let the cluster over-admit.

Fix: replaced the leaseID with p.id + "-" + ulid.Make().String()
(github.com/oklog/ulid/v2, already a module dependency — also already used
in internal/audit/event.go). ulid.Make() uses the package's default
monotonic entropy source, which is safe for concurrent use, and its 80 bits
of randomness make same-millisecond collisions practically impossible
regardless of clock resolution.

Test (TestConcurrentLeaseIDsDoNotCollide, new test): pool with
Total=64/BatchMax=64, Enqueue 64 tickets concurrently from 64 goroutines,
wait for all to admit, assert ZCard("gwslots:u1") == 64 (any collision
would upsert and shrink the count).

RED evidence — with a caveat. Run 8+ times (-count=3 each) against the
actual unmodified old code (time.Now().UnixNano()), the test never
failed on this host:

$ for i in 1..8; do go test ./internal/pool/ -run TestConcurrentLeaseIDsDoNotCollide -count=3; done
ok  datahive.id/ahu-gpu-manager/internal/pool ... (x8, all PASS)

I verified why with a standalone probe (64 goroutines barrier-released,
racing to read time.Now().UnixNano()): 0 collisions in 5 trials of 64. This
host's time.Now() has genuine sub-nanosecond-effective resolution (fine-
grained clock_gettime), so back-to-back calls from concurrent goroutines
essentially never land on the identical integer nanosecond — meaning the
literal RED reproduction described in the brief doesn't occur on this
machine, even though the entropy defect is real (platforms/containers with
coarser clocks, or any future refactor that batches/caches time.Now(),
would collide immediately).

To get honest RED evidence that this test is a real regression guard (not
one that merely can't fail), I temporarily degraded the leaseID formula to
millisecond resolution (time.Now().UnixMilli() — a stand-in for a coarser
clock source), re-ran, confirmed collisions and a hard FAIL, then reverted
back to the ulid.Make() fix (diffed byte-identical against the pre-
experiment file to confirm nothing else changed):

=== RUN   TestConcurrentLeaseIDsDoNotCollide
    redispool_test.go:153: lease id collision: expected ZCard=64, got 9
--- FAIL: TestConcurrentLeaseIDsDoNotCollide (0.06s)
=== RUN   TestConcurrentLeaseIDsDoNotCollide
    redispool_test.go:153: lease id collision: expected ZCard=64, got 7
--- FAIL: TestConcurrentLeaseIDsDoNotCollide (0.03s)
=== RUN   TestConcurrentLeaseIDsDoNotCollide
    redispool_test.go:153: lease id collision: expected ZCard=64, got 5
--- FAIL: TestConcurrentLeaseIDsDoNotCollide (0.03s)

GREEN (with the real ulid.Make() fix restored):

=== RUN   TestConcurrentLeaseIDsDoNotCollide
--- PASS: TestConcurrentLeaseIDsDoNotCollide (0.28s)

Full verification

$ gofmt -l internal/pool/          # clean, no output
$ go build ./cmd/gateway           # ok
$ go vet ./...                     # ok
$ go test ./internal/pool/ -v -race -count=2
--- PASS x2 for all 9 tests (TestCapacityAndFIFO, TestInteractiveJumpsBatch,
    TestBatchMaxLeavesHeadroom, TestTwoInstancesShareCap,
    TestLeaseExpiryFreesSlotAfterCrash, TestCancelDuringStage2ReleasesLocalSlot,
    TestRefreshDoesNotResurrectReclaimedLease, TestConcurrentLeaseIDsDoNotCollide,
    TestFailOpenWhenRedisDown)
ok  datahive.id/ahu-gpu-manager/internal/pool  3.386s / 3.704s

$ go test ./... -count=1
ok  datahive.id/ahu-gpu-manager/internal/audit    2.047s
ok  datahive.id/ahu-gpu-manager/internal/config   0.011s
ok  datahive.id/ahu-gpu-manager/internal/idem     0.017s
ok  datahive.id/ahu-gpu-manager/internal/pool     1.118s
ok  datahive.id/ahu-gpu-manager/internal/proxy    1.794s
ok  datahive.id/ahu-gpu-manager/internal/registry 0.052s
ok  datahive.id/ahu-gpu-manager/internal/server   0.309s

Commit: 4329116 fix(pool): release local slot on stage-2 cancel; conditional lease refresh; ULID lease ids.