feat: §16 retry, §17 memo, §18 Close(), §19 telemetry (D5) - #31
Merged
Conversation
Implements the four contract-1.8 quality-of-life sections. Unlike the other
SDKs so far, §16 here REPLACES an existing policy rather than adding one.
Re-vendors CONTRACT.md at 1.8.2.
§16. authz.go's retryReadOnly used a 100 ms base and `backoff *= 2` with no
cap, no jitter and no Retry-After handling. Both omissions matter:
- Uncapped, the wait is bounded by nothing but the attempt count. At three
attempts that is survivable; the constant was one edit away from not being.
- Unjittered, every client that saw the same outage retries at the same
instant. That is the thundering herd a backoff exists to prevent, and an
unjittered backoff schedules it rather than avoiding it.
Go was one of five SDKs that had each invented a policy, and all five
disagreed — the divergence contract 1.8 exists to end. retry.go is now the
shared table: 3 attempts, 200 ms base, 5 s cap, full jitter over [0, backoff],
Retry-After as a floor. A cancelled context still wins over a pending backoff,
and returns ctx.Err() rather than the transport error, because a cancelled
context is the caller's decision and not the server's failure.
NetworkError gains a RetryAfter field so the policy can honor the header. It
stores the parsed DURATION, never the raw header text, so the D-04/CR-04
redaction invariant is untouched — a duration cannot carry a token. Both RFC
7231 forms parse: delta-seconds and HTTP-date, the latter being what CDNs and
proxies commonly send on 429/503, and dropping it would silently discard the
server's own statement about when it will be ready. A negative value collapses
to zero rather than becoming a floor.
§17. Opt-in decision memo, off by default, clamped to 5 s, safe for concurrent
use — a Go client is routinely shared across goroutines. Allows and denies
memoized identically, because asymmetric caching leaks which outcome occurred
through latency. Failures never memoized: set() is only reachable after a
successful decode, so §17.1 rule 7 is structural rather than a check that could
be forgotten. Cleared on Login/VerifyMfa/Refresh/Logout, since entries are
keyed by subject rather than session.
§18. Close() sets an atomic flag, clears the memo and closes idle connections.
CloseIdleConnections rather than anything more forceful: an in-flight request on
another goroutine is the caller's to finish, and tearing its connection out
would turn a lifecycle bug into a truncated response. It does not log out and
never reaches the network — the server-side session outlives the Client value,
and a Close that logged out would end every user's session on each deploy. The
test asserts that against the wire, because a Logout wired into Close succeeds
silently.
§19. TelemetryEvent is a closed interface — the marker method is unexported, so
no outside package can add a variant carrying a secret. emit() recovers from a
panicking hook: telemetry may not fail an authorization check, and in Go an
unrecovered panic in a hook would take the process down rather than just the
request. One request pair per attempt, with the attempt threaded through the
retry helper.
d5_conformance_test.go (31 cases) asserts through the public CheckAccess
surface and counts requests reaching the test server, as contract 1.8.1
requires. Jitter is pinned to 0 in those tests so they do not really sleep; the
delay arithmetic is asserted directly instead.
Gates: go build, go vet, go test ./... all green; examples build; gofmt clean.
The coverage job enforces a 94% library floor and my D5 code took it to 93.7%.
The conformance suite asserts the contract's behaviour, which left the
defensive and bookkeeping paths around it unexercised:
- memo eviction at the entry cap, and re-insert refreshing rather than
duplicating a key
- a negative TTL disabling the memo rather than wrapping
- the nil-receiver memo methods a Client built without one would hit
- delayFor clamping a jitter fraction outside [0, 1] — a caller-supplied
source is not trusted to stay in range, and a fraction above 1 would
exceed the §16.1 cap while a negative one would produce a negative sleep
- a cancelled context beating a pending backoff, asserting it returns
ctx.Err() rather than the transport error AND that the loop stops
- the unexported isTelemetryEvent markers, so the "closed set" guarantee is
an executed line rather than an untested claim
These are real assertions rather than coverage filler: the eviction test is
the difference between a bounded cache and a memory leak in any service that
checks many resources, and the clamp test is the difference between a bounded
backoff and a negative sleep.
Library statement coverage: 93.7% -> 94.5%.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the four contract-1.8 quality-of-life sections. Unlike the SDKs so far, §16 here replaces an existing policy rather than adding one. Re-vendors
CONTRACT.mdat 1.8.2.Fourth of eleven, after rust#45, typescript#47 and python#34.
§16 — what
retryReadOnlywas doingNo cap. No jitter. No
Retry-After. Both omissions matter, and neither is theoretical:Go was one of five SDKs that had each invented a policy, and all five disagreed. That divergence is what contract 1.8.2 documents and §16 exists to end.
[0, backoff]Retry-AfterA cancelled context still wins over a pending backoff — and returns
ctx.Err()rather than the transport error, because a cancelled context is the caller's decision, not the server's failure.authzRetryMaxAttemptsbecomes the exportedMaxAttempts, alongsideBaseDelayandMaxDelay.NetworkError.RetryAfter, without breaking redactionThe policy needs the header, and this SDK has a hard invariant (D-04/CR-04) that
NetworkErrornever carries unredacted response data.The field stores the parsed
time.Duration, never the raw header text — a duration cannot carry a token, a URL, or anything else a header might. Both RFC 7231 forms parse: delta-seconds and HTTP-date. The date form is not hypothetical; CDNs and proxies commonly send it on429/503, and treating it as unparseable would silently discard the server's own statement about when it will be ready. A negative value collapses to zero rather than becoming a floor.§17 — opt-in, concurrent-safe
Off by default, clamped to
MaxMemoTTL(5 s), mutex-guarded — a Go client is routinely shared across goroutines, and a cache that corrupted under concurrency would be a worse bug than the one it is optimising away.Allows and denies memoized identically (asymmetric caching leaks the outcome through latency). Failures never memoized — structurally, since
setis only reachable after a successful decode. Cleared on any credential change, since entries are keyed by subject rather than session.§18 — Close() does not log out
Idempotent, satisfies
io.Closer, clears the memo, and callsCloseIdleConnections— not anything more forceful. An in-flight request on another goroutine is the caller's to finish; tearing its connection out would turn a lifecycle bug into a truncated response.It never reaches the network. The server-side session deliberately outlives the
Clientvalue, so aClosethat logged out would silently end every user's session on each deploy. Asserted against the wire, because aLogoutwired intoClosesucceeds silently.§19 — a closed interface, and panic recovery that matters more here
TelemetryEvent's marker method is unexported, so no package outside this one can add a variant carrying a secret. That makes "no field can hold a token" checkable rather than aspirational.emitrecovers from a panicking hook. §19.2 rule 2 says telemetry may not fail an authorization check — and in Go the stakes are higher than in the other SDKs: an unrecovered panic in a hook would take the process down, not just the request.One request pair per attempt, with the attempt threaded through the retry helper.
Verification
go build ./...go vet ./...go test ./...go build ./examples/...gofmt -l .d5_conformance_test.goasserts through the publicCheckAccesssurface and counts requests reaching anhttptestserver, as contract 1.8.1 requires. Jitter is pinned to0there so the tests do not really sleep — "a test that really waits 200 ms is a test nobody runs" — and the delay arithmetic is asserted directly instead.The pre-existing
TestRetryReadOnly_*tests pass unchanged against the new policy; only the renamed constant was updated.Notes
withJitterSourceis deliberately unexported — it is a test seam, not API surface.Generated by Claude Code