feat: §16 retry, §17 memo, §18 close(), §19 telemetry (D5) - #47
Merged
Conversation
Implements the four contract-1.8 quality-of-life sections, and fixes two defects found while doing it. Re-vendors CONTRACT.md at 1.8.1. THE BUG THAT MATTERED: withRetry was never called. `src/rest/retry.ts` was exported, unit-tested and green — and no production path invoked it. `checkAccess` went straight to axios. So this SDK performed NO read-only retries at all, while `test/rest/retry.test.ts` passed and the exported symbol made it look wired. §11.2 rule 5 has required retries here since it was written, and the requirement was silently unmet. A tested helper nobody calls is worse than an absent one: the passing tests are exactly what stop anyone from looking. That is why the new conformance tests assert through the public `checkAccess` surface — counting requests on the wire — rather than against the helper in isolation, and why contract 1.8.1 now requires that of every SDK claiming §16. The second defect, in the same file: `retryAfterMs ?? backoffDelayMs(attempt)` made the server's hint REPLACE the computed backoff, so a `Retry-After: 0` retried immediately and defeated the policy entirely. That is precisely what §16.1's "floor, never a ceiling" forbids. I wrote that clause on principle for contract 1.8; it turned out to describe a defect we already shipped. §16. The policy now matches the normative table: 3 attempts, 200 ms base, 5 s cap, full jitter over [0, backoff], Retry-After as a floor. The old parameters (1000 ms / 8 s / partial jitter) were a third divergent invention — Java, Rust and this SDK each had different ones, which is what contract 1.8 exists to end. Full jitter is the substantive change: partial jitter keeps every client's retries clustered around the same instant, causing the thundering herd retries are meant to prevent. `maxAttempts` is gone from RetryOptions — §16.1 fixes the cap and forbids raising it. §17. Opt-in decision memo, off by default (`decisionMemoTtlMs`, clamped to 5 s). Allows and denies memoized identically, because asymmetric caching leaks which outcome occurred through latency. Failures never memoized — structurally, since `set` is only reachable after a successful response. Cleared on any credential change, since entries are keyed by subject rather than session. The key joins its four components with U+001F and marks absent optionals with U+0000, so no combination of values can forge a collision between an absent and a present scope. §18. `close()`, idempotent, with `ensureOpen()` on every entry point so use-after-close rejects rather than silently reconnecting. It does not log out and never reaches the network — the server-side session outlives the client object, and a close() that logged out would end every user's session on each deploy. §19. Telemetry hooks with a closed event union: a hook that throws is swallowed (telemetry may not fail an authorization check) and there is no field a token could ride in. One requestStart/requestEnd pair PER ATTEMPT — an earlier draft emitted every pair as attempt 1, which would have made a retried call indistinguishable from a single slow one. The conformance test asserting [1, 2] caught it, and withRetry now passes the attempt into its callback. typedoc caught the same class of failure as in D6: four new public types were referenced from AxiamClient/AxiamClientOptions but not exported from an entry point, so `npm run docs` exited 4. Re-exported from `src/rest/index.ts` with a comment saying why, then documented every member typedoc demanded. All gates green locally: 606 tests (58 files), tsc, typedoc exit 0, bundle-grep (browser bundle still free of grpc/amqplib), middleware module smoke test, 96.7% line coverage.
This was referenced Aug 9, 2026
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 quality-of-life sections from contract 1.8, and fixes two defects found while doing it. Re-vendors
CONTRACT.mdat 1.8.1.Second of eleven SDKs; rust#45 landed the same four sections first.
The bug that mattered:
withRetrywas never calledsrc/rest/retry.tswas exported, unit-tested and green — and no production path invoked it.checkAccesswent straight to axios.So this SDK performed no read-only retries at all, while
test/rest/retry.test.tspassed and the exported symbol made it look wired. §11.2 rule 5 has required retries on the authz path since it was written, and the requirement was silently unmet.A tested helper nobody calls is worse than an absent one — the passing tests are exactly what stop anyone from looking. That is why the new conformance tests assert through the public
checkAccesssurface, counting requests on the wire, rather than against the helper in isolation. Contract 1.8.1 (axiam#284) now requires that of every SDK claiming §16, because of this.The second defect:
Retry-Afterdefeated the backoff??means the server's hint replaced the computed backoff rather than flooring it — so aRetry-After: 0retried immediately. That is exactly what §16.1's "floor, never a ceiling" forbids. I wrote that clause on principle when drafting contract 1.8; it turned out to describe a defect we had already shipped.Now
Math.max(jittered, retryAfterMs), with a test asserting a zero hint cannot shorten the wait.§16 — the policy
The old parameters were a third divergent invention: Java, Rust and this SDK each had different ones, which is the whole reason §16 exists.
base + 0–20%)[0, backoff]Retry-AftercheckAccessFull jitter is the substantive change. Partial jitter keeps every client's retries clustered around the same instant, which causes the thundering herd retries are meant to prevent. The test pins the fraction to
0and1to prove the range is[0, backoff]and notbackoff ± something.RetryOptions.maxAttemptsis removed — §16.1 fixes the cap at 3 and forbids raising it. There is no knob for the base or cap either: eleven SDKs agreeing on one table is the point.§17 — decision memo, opt-in and off by default
decisionMemoTtlMs, clamped to 5000 ms rather than rejected. Allows and denies memoized identically (asymmetric caching leaks which outcome occurred through latency). Failures never memoized — structurally, sincesetis only reachable after a successful response, so rule 7 can't be forgotten rather than merely checked. Cleared on any credential change, since entries are keyed by subject rather than session.The key joins its four components with
U+001Fand marks absent optionals withU+0000, so no combination of caller-supplied values can forge a collision between an absent and a present scope — a memo that let those collide would answer a narrower question with a broader answer.§18 —
close()does not log outIdempotent;
ensureOpen()on every entry point so use-after-close rejects rather than silently reconnecting.It never reaches the network. The server-side session deliberately outlives the client object — that is what lets a process restart and resume — so a
close()that logged out would silently end every user's session on each deploy.§19 — and a bug my own test caught
A hook that throws is swallowed (telemetry may not fail an authorization check), and
TelemetryEventis a closed union with no field a token could ride in.One
requestStart/requestEndpair per attempt. An earlier draft emitted every pair as attempt 1, which would have made a retried call indistinguishable from a single slow one — the exact blindness §16.5 exists to remove. The conformance test asserting[1, 2]caught it;withRetrynow passes the attempt number into its callback.typedoc, again
Same class of failure as D6: four new public types were referenced from
AxiamClient/AxiamClientOptionsbut not exported from an entry point, sonpm run docsexited 4. Found by running the gate locally, not by CI. Re-exported fromsrc/rest/index.tswith a comment explaining why they must live there, then documented every member typedoc demanded.Verification
npm test -- --runnpx tsc --noEmitnpm run docs(typedoc)npm run bundle-grep@grpc/grpc-js/amqplib(SC#1)node -e "require('./dist/middleware/index.js')"npm run coverageNew:
test/rest/d5Conformance.test.ts(22 cases). The pre-existingwithRetrysuite passes unchanged against the new policy, minus themaxAttemptsargument §16.1 removed.Notes
Generated by Claude Code