feat(relay): schedule deferred binary release with grace-period reclaim (#20)

[ilmoniemi]

What

Adds Registry.ScheduleReleaseServer(serverID, d) — a deferred-release primitive that, on timer expiry, removes the binary entry and closes phones outside the registry lock (snapshot-and-iterate, same shape as PhonesFor).

ClaimServer gains a grace-aware fast path: when a timer is pending for serverID, the claim cancels the timer and atomically replaces the binary Conn, returning nil instead of ErrServerIDConflict. The grace window IS the reclaim path — phones registered during the window inherit the new binary on reclaim, or are Close()'d on expiry.

ReleaseServer (synchronous) is unchanged. No callers migrate in this slice; the /v1/server handler swap lives in #21.

Issue

Closes #20. Split from #8.

Testing

Seven new subtests in internal/relay/registry_test.go, mapped 1:1 to AC bullets:

• TestScheduleReleaseServer_ReclaimWithinGrace_NoReleaseFires
• TestScheduleReleaseServer_ExpiryRemovesBinaryAndClosesPhones
• TestScheduleReleaseServer_PhoneRegisteredDuringGrace_SurvivesReclaim
• TestScheduleReleaseServer_PhoneRegisteredDuringGrace_ClosedOnExpiry
• TestScheduleReleaseServer_ReplacesPendingTimer
• TestScheduleReleaseServer_OnUnheldID_NoOp
• TestScheduleReleaseServer_RaceFreedomUnderRapidCycles — 16 goroutines × 200 ops hammering claim/reclaim/schedule/cancel; clean under -race -count=20.

fakeConn.closed is now mutex-guarded with an optional closeCh for deterministic synchronisation in expiry tests.

go test -race ./..., go vet ./..., go build ./... all clean.

Architecture compliance

• Single sync.RWMutex continues to guard all maps (now three) per ADR-0003.
• Stale-fire race closed by pointer-identity check on a wrapper struct (graceEntry); using *time.Timer directly tripped the race detector because the closure captures the local t variable assigned after time.AfterFunc returns. Wrapping in graceEntry lets the closure capture a stable pointer set before the timer is created.
• Phones closed outside the lock; matches PhonesFor snapshot pattern.
• No new sentinels; degenerate durations (d<=0, very large) are safe per security review.

🤖 Generated with Claude Code

[ilmoniemi]

Code Review: #20

Decision: PASS

Findings

• [SHOULD FIX] internal/relay/registry.go:81-89 — ClaimServer's doc comment is now stale. It still asserts "First-claim-wins: a second concurrent caller for the same serverID receives ErrServerIDConflict and the conflicting caller's conn is left untouched (the registry does not Close it)." That's no longer strictly true: during a pending grace window, a second caller succeeds (the reclaim path) and returns nil. A reader who only sees ClaimServer's doc has no way to know that its behavior is conditional on r.timers[serverID]. Add a sentence noting the grace-window reclaim semantic and pointing at ScheduleReleaseServer for details, e.g.: "If a grace-period release is pending for serverID (see ScheduleReleaseServer), ClaimServer cancels the pending timer, atomically replaces the binary Conn, inherits the existing phones, and returns nil." Consider rewording the "First-claim-wins" sentence to scope it to the no-timer case.

• [NIT] internal/relay/registry.go:63-70 — The graceEntry wrapper works, but the doc comment justifies it as avoiding "the race-detector flag on a self-referential local var." The standard var t *time.Timer; t = time.AfterFunc(d, func() { ... captures t ... }) pattern is idiomatic Go (closures capture by reference) and isn't a race-detector violation under any version I'm aware of. The actual benefit of the wrapper is that the closure no longer needs to read the timer pointer at all (the expiry handler compares entry pointers, not timer pointers), so entry.timer only matters on the cancellation paths. If you'd like to keep the wrapper, consider rewording the comment to that effect; otherwise the simpler *time.Timer map works fine.

• [NIT] internal/relay/registry_test.go:378-399 — TestScheduleReleaseServer_ReplacesPendingTimer proves the 20ms timer fires (closing p1) but doesn't directly prove the 1-hour timer was stopped — if the second Stop() were a no-op, the test would still pass since p.Close() is idempotent and delete on an absent key is a no-op. The spec acknowledged this trade-off; flagging only so future readers don't over-rely on the test as a cancellation guarantee.

Summary

Faithful implementation of the spec. The pointer-identity stale-fire defense is correct, the lock discipline matches ADR-0003 (single mutex over all three maps), Close() is invoked outside the lock per the established pattern, and goroutine lifetime is bounded by d. Tests are comprehensive and all seven spec'd subtests are present. make test (with -race) and CI's test and security checks are green.

Security goggles applied (label: security-sensitive): the architect's spec contains a Security review section with verdict PASS; the diff matches its findings (no new untrusted-input boundaries, no logging of sensitive data, time.AfterFunc lifetime bounded, timers map cleanup symmetric in ClaimServer grace path and handleGraceExpiry). Phone-side close code 1011 is correctly deferred per the ticket's "Out of scope" section.

Doc-comment polish before merging would be ideal but is not blocking.