From 7c8400b5927aeebf3fe4c7fe00147c6cb87504cd Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 17:22:16 +0000 Subject: [PATCH] =?UTF-8?q?feat:=20=C2=A716=20retry,=20=C2=A717=20memo,=20?= =?UTF-8?q?=C2=A718=20close(),=20=C2=A719=20telemetry=20(D5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 46 ++++ CONTRACT.md | 371 +++++++++++++++++++++++++++++++- README.md | 94 +++++++- examples/telemetry-hook.ts | 137 ++++++++++++ src/core/config.ts | 48 +++++ src/core/decisionMemo.ts | 152 +++++++++++++ src/core/index.ts | 4 + src/core/telemetry.ts | 128 +++++++++++ src/core/telemetryReporter.ts | 53 +++++ src/rest/auth.ts | 24 +++ src/rest/authz.ts | 58 ++++- src/rest/client.ts | 67 ++++++ src/rest/index.ts | 23 +- src/rest/retry.ts | 141 +++++++++--- test/rest/d5Conformance.test.ts | 362 +++++++++++++++++++++++++++++++ test/rest/retry.test.ts | 6 +- 16 files changed, 1667 insertions(+), 47 deletions(-) create mode 100644 examples/telemetry-hook.ts create mode 100644 src/core/decisionMemo.ts create mode 100644 src/core/telemetry.ts create mode 100644 src/core/telemetryReporter.ts create mode 100644 test/rest/d5Conformance.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index d930535..612793f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,52 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Added + +- **§16 bounded read-only retry policy.** `checkAccess`/`can`/`batchCheck` now retry under + the contract's normative table: 3 attempts, 200 ms base, 5 s cap, **full jitter** over + `[0, backoff]`, `Retry-After` honored as a floor. Both non-deterministic inputs are + injectable, so the tests pin the jitter fraction to 0 and 1 to prove the range instead of + sleeping. +- **§18 `AxiamClient.close()`**, idempotent, with use-after-close rejecting 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** — `telemetryHook` on `AxiamClientOptions`, plus the `TelemetryEvent` + union and `examples/telemetry-hook.ts` with the OpenTelemetry mapping. A hook that throws + cannot fail the operation that fired it, and no event payload can carry a token. One + request pair per *attempt*, not per logical call, so callers can count real wire calls. +- **§17 decision memo — opt-in, off by default.** `decisionMemoTtlMs`, clamped to 5000 ms. + Allows and denies memoized identically, failures never memoized, cleared on any credential + change. **Reads-your-own-writes is not guaranteed.** +- `retryEnabled` (§16.6), default on. No knob for the attempt cap, base or delay cap: §16.1 + forbids raising them. + +### Fixed + +- **`withRetry` was never called by any production path.** It was exported, unit-tested and + green, but `checkAccess` did not route through it — so this SDK performed **no read-only + retries at all** while appearing to, leaving §11.2 rule 5 silently unmet. A tested helper + nobody calls is worse than an absent one: the passing tests are what stop anyone looking. + The §16 conformance tests now assert through the public `checkAccess` surface. +- **`Retry-After` replaced the backoff instead of flooring it.** `retryAfterMs ?? backoff(n)` + meant a `Retry-After: 0` retried immediately, defeating the policy — exactly what §16.1's + "floor, never a ceiling" forbids. Now `Math.max(jittered, retryAfterMs)`. +- **Partial jitter replaced with full jitter.** The old `base + 0–20%` keeps every client's + retries clustered around the same instant, which causes the thundering herd retries are + meant to prevent. +- The §19 request pair now carries the real attempt number. An earlier draft emitted every + pair as attempt 1, which would have made a retried call indistinguishable from a single + slow one — caught by the conformance test asserting `[1, 2]`. + +### Changed + +- Re-vendored `CONTRACT.md` at **1.8.1**. `openapi.json` unchanged — docs-only contract revs. +- `RetryOptions.maxAttempts` removed: §16.1 fixes the cap at 3 and forbids raising it. + `withRetry`'s callback now receives the attempt number. + ## [1.0.0-alpha24] - 2026-08-04 ### Added diff --git a/CONTRACT.md b/CONTRACT.md index a4155dd..7dbc54e 100644 --- a/CONTRACT.md +++ b/CONTRACT.md @@ -950,10 +950,12 @@ macro over an `axiam_require_access(...)` guard function. All compose strictly o - unresolvable resource id → 400 `invalid_request` - `NetworkError` while calling the authz endpoint → **fail closed** with 503 `authz_unavailable` (deny; never allow on transport failure; never retry beyond the - SDK's existing bounded read-only retry policy) -6. **No decision caching.** Helpers MUST NOT cache allow/deny decisions (consistent with - §10's TTL rule). Batch/page-level optimization stays the application's job via - `batch_check`. + bounded read-only retry policy of [§16](#§16-retry-policy-d5)) +6. **No decision caching by default.** Helpers MUST NOT cache allow/deny decisions (consistent + with §10's TTL rule). Batch/page-level optimization stays the application's job via + `batch_check`. The **single** exception is the explicitly opt-in, TTL-clamped decision memo + of [§17](#§17-client-side-decision-memo-d5), which is disabled by default and which + §17.1 rule 10 forbids from serving the fail-closed path above. 7. **Transport.** Helpers call the SDK's existing `check_access` surface (REST by default; gRPC where the SDK's dispatcher already prefers it, e.g. PHP). No new transport code. 8. **Redaction.** Deny/error paths MUST NOT log or echo the token, and SHOULD log the @@ -1745,8 +1747,11 @@ operations — same rule, same client-side error, same remedy. dispatch on the `error` field first. A `400` whose `error` is none of the five falls back to the §2 mapping. 6. **`5xx` and transport failures remain §2 `NetworkError`** and are **not** terminal — - they are retried under the SDK's existing bounded read-only retry policy, then surfaced. - A server restart mid-flow must not lose a grant the user has already approved. + they are retried under the bounded read-only retry policy of + [§16](#§16-retry-policy-d5), then surfaced. A server restart mid-flow must not lose a + grant the user has already approved. Per §16.2 that budget is **per poll attempt** and is + separate from this grant's own `expires_in` polling loop: an exhausted retry budget ends + that one poll, not the flow. ### §14.3 `device_login` — the composed helper @@ -1921,6 +1926,343 @@ with no attempt to refine it. --- +## §16 Retry Policy (D5) + +**Requirement level: MUST (v1.0).** + +Two earlier clauses — [§11.2](#§112-semantics-normative-identical-in-all-sdks) rule 5 and +[§14.2](#§142-polling-normative--the-part-implementations-get-wrong) — instruct SDKs to retry +"under the SDK's existing bounded read-only retry policy". **No such policy was ever defined +here.** In practice **three** SDKs had one and all three disagreed; the other eight had none +at all — only §9's refresh-then-retry-once, which is a different mechanism entirely. + +| SDK | Attempts | Base | Cap | Jitter | `Retry-After` | +|---|---|---|---|---|---| +| Java | 3 | 200 ms | 5 s | full | honored as a floor | +| Rust | 3 | library default | — | none | ignored | +| TypeScript | 3 | 1000 ms | 8 s | partial (`base + 0–20%`) | **replaced** the backoff | + +Two things about the TypeScript row. + +It is why "floor, never a ceiling" is stated so bluntly in §16.1: `retryAfterMs ?? +backoffDelayMs(attempt)` means a `Retry-After: 0` retries **immediately**, defeating the +backoff entirely. That clause was written on principle and then found to describe a defect +one of these SDKs already shipped. + +And its helper was **exported and unit-tested but never called by any production path** — +`check_access` did not route through it — so that SDK performed no read-only retries at +all while appearing to. A tested helper nobody calls is worse than an absent one: the tests +report green and the gap stays invisible. **An SDK claiming §16 conformance MUST assert the +policy through its public `check_access` surface, not only against the helper in isolation** +(see §16.7's required non-idempotent test, which asserts the request count *on the wire*). + +This section is the missing policy, so the two forward references resolve to one table +instead of eleven guesses. + +### §16.1 The policy (normative — every value here is binding) + +| Parameter | Value | Why this value | +|---|---|---| +| Attempt cap | **3 total** (1 initial + 2 retries) | Bounds worst-case added latency at ~10 s. A caller who needs more can retry at their own layer, where they know the deadline. | +| Base delay | **200 ms** | Long enough that a retry is not simply re-entering the same overload; short enough to be invisible on a recovery from a single dropped packet. | +| Delay cap | **5 s** | The ceiling on any single wait. | +| Backoff | `min(cap, base × 2^(attempt−1))` | attempt 1 → 200 ms, attempt 2 → 400 ms, both under the cap. | +| Jitter | **full jitter** — the actual wait is uniform random in `[0, backoff]` | Not "backoff ± 10%". Full jitter is what stops a thundering herd: partial jitter keeps every client's retries clustered around the same instant, which is the failure mode retries cause rather than fix. | +| `Retry-After` | **honored, as a floor**: wait = `max(jittered_backoff, retry_after)` | The server is telling you when it will be ready. Retrying sooner is not permitted; the value never *shortens* a wait either, so a `Retry-After: 0` cannot defeat the backoff. | +| Randomness source | Any uniform PRNG. It need not be cryptographic. | The jitter is a load-spreading device, not a secret. | + +An SDK MUST NOT make the attempt cap, base, or cap configurable upward beyond these values in +v1.0. It MAY expose a switch that disables retrying entirely — some callers own their own +retry layer and want exactly one attempt — and MUST default that switch to **on**. + +### §16.2 What is eligible (normative) + +Retry applies **only to operations that change no server state**, and "idempotent" here means +exactly that. It does **not** mean "HTTP GET": AXIAM's authorization check is a `POST` with a +request body and is the single most important operation in this section. An SDK that gates +retry on the HTTP verb will retry nothing that matters. + +**Eligible:** + +| Operation | Note | +|---|---| +| `check_access`, `can`, `batch_check` | `POST`, side-effect-free. The reason this section exists. | +| JWKS fetch (§10.1) | Cache fill; pure read. | +| OIDC discovery fetch (§12) | Pure read. | +| `oidc_userinfo`, `get_user_info` (§1.1) | Pure reads. | +| `oidc_introspect` | A read *about* a token; mints nothing. | +| `device_poll` on a 5xx or transport failure (§14.2) | The clause that referenced this policy. The retry budget here is **per poll attempt**, and is separate from — and does not consume — the device grant's own `expires_in` polling loop. | + +**Not eligible, and an SDK MUST NOT retry them automatically:** + +`login`, `verify_mfa`, `logout`, `refresh`, `oidc_exchange`, `device_authorize`, +`device_login`, `token_exchange`, `oidc_revoke`, and every mutation. Two distinct reasons, +both disqualifying on their own: + +1. **They change state.** A transient failure after the server committed but before the + response arrived is indistinguishable, at the client, from one before it committed. A + silent retry then duplicates a side effect the caller never asked for twice. +2. **Their credentials are single-use.** An authorization code, a device code at the moment + it is redeemed, and a rotating refresh token are each consumed by the attempt. Retrying + replays a spent credential, which the server correctly refuses — turning a recoverable + blip into a hard `invalid_grant` the caller cannot interpret. + +`refresh` is additionally out of scope because [§9](#§9-single-flight-refresh-guard) rule 3 +already forbids it by name ("no retry loop"). **§16 does not amend §9.** The two mechanisms +compose in one direction only: the operation *inside* a §9 refresh-then-retry may itself be +retried per §16 if it is eligible, but a §9 refresh MUST NOT be re-attempted under §16, and +§16's budget MUST NOT be reset by a §9 refresh occurring mid-operation. One §9 refresh, one +§16 budget, per logical call. + +Revocation deserves its own note because §12.1 records that the server treats it idempotently +per RFC 7009. That is a statement about **server** behaviour — revoking an already-revoked +token returns `200` rather than an error. It is not licence for the client to retry a +mutation, and an SDK MUST NOT read it as one. + +### §16.3 Which failures retry (normative) + +| Condition | Retry? | +|---|---| +| Transport failure — connection refused, DNS, TLS handshake, read timeout | **Yes** | +| `408`, `429` | **Yes** (`429` is exactly where `Retry-After` usually arrives) | +| `5xx` | **Yes** | +| `401` / `AuthError` | **No** — decisive, not transient. §9 owns the refresh path. | +| `403` / `AuthzError` | **No** — the server has decided. | +| `400`, `404`, `409`, and every other `4xx` | **No** — retrying an unacceptable request produces an identical rejection. | +| `OAuthProtocolError` (§12.3 rule 3) | **No**, at any status. It is a protocol answer, not a transport failure. | + +The §2 taxonomy maps `408`/`429`/`5xx`/transport all to `NetworkError`, so "retry +`NetworkError` only" is a correct and sufficient implementation of this table in an SDK whose +errors carry no status. An SDK whose errors *do* carry the status MUST NOT retry a +`NetworkError` that came from a row marked **No**. + +### §16.4 Interaction with the fail-closed rule + +[§11.2](#§112-semantics-normative-identical-in-all-sdks) rule 5 requires the route guard to +**fail closed** — deny with `503 authz_unavailable` — when the authz endpoint is unreachable. +§16 does not soften that. The retry budget is spent *first*; when it is exhausted the guard +denies. An SDK MUST NOT extend the budget because the caller is a guard, and MUST NOT admit a +request because retries were attempted. + +### §16.5 Observability + +Every retry MUST emit the `retry` telemetry event of [§19](#§19-telemetry-hooks-d5) when the +caller has installed a hook. A retried-then-succeeded operation is otherwise **invisible** — +the caller sees a slow success and no signal at all that the server is failing. That silence +is the standing objection to automatic retry, and the hook is what answers it. + +Retries MUST NOT be logged at `info` or above by default. Redaction rules (§2, §11.2 rule 8) +apply unchanged: a retry log line carries the operation and attempt number, never the token. + +### §16.6 Per-language naming map + +The policy is internal machinery; only the disable switch and the parameters are public +surface, and only where the language's client builder already has a place for them. + +| Canonical | Rust | TypeScript | Python | Java | Kotlin | C# | PHP | Go | Swift | C | C++ | +|---|---|---|---|---|---|---|---|---|---|---|---| +| `retry_enabled` | `retry_enabled` | `retryEnabled` | `retry_enabled` | `retryEnabled` | `retryEnabled` | `RetryEnabled` | `retryEnabled` | `RetryEnabled` | `retryEnabled` | `axiam_client_config_set_retry_enabled` | `retry_enabled` | + +### §16.7 Required tests + +Backoff and jitter MUST be tested with an **injected clock and an injected PRNG** — never by +sleeping. A test that really waits 200 ms is a test nobody runs. + +Required: the attempt cap is honored exactly (a permanently failing eligible operation makes +exactly 3 attempts, not 2, not 4); the delay sequence with jitter pinned to its maximum is +`200 ms, 400 ms`; full jitter with the PRNG pinned to `0` waits `0` and with it pinned to `1` +waits the full backoff — proving the range is `[0, backoff]` and not `backoff ± something`; a +`Retry-After` longer than the backoff wins, and one shorter than the backoff does **not** +shorten it; a `403` and a `401` each make exactly one attempt; a **non-idempotent operation +makes exactly one attempt even when the failure is a `503`** (assert the request count on the +wire, not just the raised error — this is the test that catches a retry wired at the transport +layer instead of the operation layer); the guard still denies `503 authz_unavailable` after +the budget is exhausted; a `retry` telemetry event is emitted per retry. + +--- + +## §17 Client-Side Decision Memo (D5) + +**Requirement level: MAY (v1.0). Disabled by default.** + +[§11.2](#§112-semantics-normative-identical-in-all-sdks) rule 6 says helpers MUST NOT cache +allow/deny decisions. **That rule stands as the default.** This section defines the single +exception: an explicitly opt-in, TTL-bounded memo that a caller must switch on, having read +what it costs them. + +The server already ships the same trade with the same shape — `AXIAM__AUTHZ__DECISION_CACHE_TTL_SECS` +(default 5 s) and `AXIAM__AUTH__SESSION_VALIDATION_CACHE_TTL_SECS` (default `0`, off) — where +the documented bound is that a revoked grant can still be served for up to the TTL. The SDK +memo mirrors that bound rather than inventing a second staleness story. + +### §17.1 Semantics (normative) + +1. **Off by default.** The default TTL is `0`, which means disabled — not "cache for zero + seconds". An SDK MUST NOT enable it because it looks like an easy win. +2. **Ceiling of 5 seconds, clamped.** A configured TTL above 5 s MUST be clamped to 5 s, and + the SDK MUST document that it clamps. This deliberately differs from the server, whose + equivalent setting is an unclamped `u64` — a known residual that lets an operator + configure a multi-hour staleness window. The client has no reason to repeat it. +3. **Key.** `(subject_id, resource_id, action, scope)`, all four, with absent `scope` and + absent `subject_id` each forming a distinct key from any present value. A memo that + ignores `scope` answers a narrower question with a broader answer. +4. **Allows and denies are cached identically.** Not "cache allows only", and not "cache + denies only". Asymmetric caching changes the *timing* of the two outcomes and so leaks + which one occurred to anyone who can observe latency, and it surprises every reader who + assumed a cache is a cache. Uniform is both safer to reason about and simpler to + implement. +5. **`reason_code` is cached with the decision** (§11 rule 9) and MUST be returned from the + memo unchanged. A memo that returns `allowed` but drops the code would make the field + intermittently absent, which is worse than never having it. +6. **The staleness bound is the TTL, in both directions.** A grant revoked on the server can + still read as `allowed` for up to the TTL, and a grant just *added* can still read as + denied for up to the TTL. **Read-your-own-writes is not guaranteed**, and every SDK + enabling this MUST say so in its documentation in those words. An admin UI that grants a + role and immediately re-checks is the case that breaks, and it breaks silently. +7. **Never negative-cache a failure.** Only a decision the server actually returned is + memoized. A `NetworkError`, a `503`, an exhausted §16 retry budget — none of them are + entries. Caching a transport failure as a deny would turn a blip into a TTL-long outage, + and caching it as an allow is unthinkable. +8. **Bounded, and safe to drop.** The memo MUST have an entry cap and MUST evict rather than + grow. It is a latency optimisation; dropping an entry is always correct, so eviction needs + no coordination. +9. **Invalidated by identity change.** `login`, `logout`, `refresh` and any credential change + MUST clear the memo entirely. Entries are keyed by subject, not by session, so a + re-authentication as a different principal would otherwise read the previous principal's + decisions. +10. **Not consulted by the guard's fail-closed path.** When the authz endpoint is unreachable + §11.2 rule 5 denies. An SDK MUST NOT serve a stale allow from the memo to paper over an + outage — that inverts fail-closed into fail-open at exactly the moment it matters. + +### §17.2 Per-language naming map + +| Canonical | Rust | TypeScript | Python | Java | Kotlin | C# | PHP | Go | Swift | C | C++ | +|---|---|---|---|---|---|---|---|---|---|---|---| +| `decision_memo_ttl` | `decision_memo_ttl` | `decisionMemoTtl` | `decision_memo_ttl` | `decisionMemoTtl` | `decisionMemoTtl` | `DecisionMemoTtl` | `decisionMemoTtl` | `DecisionMemoTTL` | `decisionMemoTtl` | `axiam_client_config_set_decision_memo_ttl` | `decision_memo_ttl` | + +### §17.3 Required tests + +With an injected clock: a repeat check inside the TTL makes **no second wire call** and +returns an equal decision including its `reason_code`; the same check after the TTL makes a +fresh call; a deny is memoized exactly as an allow is (assert the wire-call count for both, +not just the outcome); a TTL configured above 5 s is clamped to 5 s; differing `scope`, +`action`, `resource_id` or `subject_id` each miss rather than collide, and absent-`scope` +does not hit a present-`scope` entry; a `NetworkError` is not memoized (the next call reaches +the wire); `logout` clears the memo; with the memo enabled and the endpoint unreachable the +guard still denies `503 authz_unavailable` rather than serving a stale allow; and with the +default configuration **every** repeat check reaches the wire, proving off-by-default. + +--- + +## §18 Deterministic Shutdown (D5) + +**Requirement level: MUST (v1.0).** + +Every SDK client owns things the runtime will not reclaim promptly on its own: a connection +pool, a cookie jar, a JWKS refresh timer, an AMQP consumer thread, a gRPC channel. Without an +explicit shutdown the caller has no way to know when those are released, which shows up as +sockets held open past the end of a test, a process that will not exit, and — in the C++ SDK's +D2 investigation — lifecycle gaps that were only visible under load. + +### §18.1 Semantics (normative) + +1. **Every SDK MUST expose a deterministic shutdown** in whatever form its language already + uses. Not a new invented spelling: `Drop` plus an explicit `close()` in Rust, a context + manager in Python, `AutoCloseable` in Java, `IDisposable`/`IAsyncDisposable` in C#, a + `Closeable` in Kotlin, `Close() error` in Go, `close()` in TypeScript and PHP, a `deinit` + plus explicit `close()` in Swift, `axiam_client_free` in C, a destructor plus `close()` in + C++. +2. **Idempotent.** Closing twice MUST NOT raise, double-free, or double-release. Cleanup code + runs from error paths, and an error path that itself throws hides the original failure. +3. **Releases everything.** Connections closed, pools drained, background threads and timers + joined or cancelled, the cookie jar cleared. After `close()` returns, the client holds no + OS handle. +4. **Use after close is an error, not undefined.** A call on a closed client MUST raise the + SDK's own error type with a message naming the cause. It MUST NOT silently reopen, and MUST + NOT be undefined behaviour in the manual-memory languages. +5. **Close does not log out.** Shutting down a client releases *local* resources; it MUST NOT + issue a `logout`, revoke a token, or otherwise reach the network. The session outlives the + client object, which is what lets a process restart and resume. An SDK that logged out on + close would silently end sessions on every deploy. +6. **`Sensitive` material is zeroed where the language allows it** (§7), on the same path. + +### §18.2 Per-language naming map + +| Canonical | Rust | TypeScript | Python | Java | Kotlin | C# | PHP | Go | Swift | C | C++ | +|---|---|---|---|---|---|---|---|---|---|---|---| +| `close` | `close` (+ `Drop`) | `close` | `close` / `__exit__` | `close` (`AutoCloseable`) | `close` (`Closeable`) | `Dispose` / `DisposeAsync` | `close` | `Close` | `close` (+ `deinit`) | `axiam_client_free` | `close` (+ destructor) | + +### §18.3 Required tests + +Close is idempotent (twice, no raise); a call after close raises the SDK's error type rather +than reconnecting; the language's scope-based form releases on both the normal and the +exception path (a context manager on a raised exception, a `try`-with-resources on a throw, a +`defer Close()` on an early return); **no network request is issued by close** (assert against +the transport, which is what catches a `logout` accidentally wired in); and — where the +language can observe it — no thread or timer outlives the call. + +--- + +## §19 Telemetry Hooks (D5) + +**Requirement level: SHOULD (v1.0).** + +A caller who wants metrics currently has to wrap every SDK method or monkey-patch the +transport. This section defines an optional callback surface so they can wire OpenTelemetry, +Prometheus, or a log line **without this SDK taking a dependency on any of them**. No SDK +ships an OTel dependency in v1.0; each ships an OTel adapter as an `examples/` entry, where it +costs nothing to anyone who does not want it. + +### §19.1 Events (normative) + +| Event | Fired | Carries | +|---|---|---| +| `request_start` | Before an outbound call leaves the SDK | operation name, HTTP method, path template, attempt number | +| `request_end` | After it completes, success or failure | the `request_start` fields, plus status code (or `None`), duration, outcome | +| `retry` | Before each §16 retry wait | operation name, attempt number, the delay about to be taken, the failure that triggered it | +| `refresh` | Around a §9 single-flight refresh | whether this caller performed the refresh or waited on another's | + +`path template` means `/api/v1/authz/check`, not the URL with ids substituted in — a metric +label with a UUID in it is a cardinality bomb. + +### §19.2 Rules (normative) + +1. **Off unless installed.** No hook, no cost beyond a null check. +2. **A hook MUST NOT be able to break the SDK.** An exception thrown by a caller's hook MUST + be caught and swallowed by the SDK. Telemetry is not permitted to fail an authorization + check. An SDK MAY report the swallowed error through its own debug log; it MUST NOT + propagate it. +3. **No secrets, ever.** Hook payloads MUST NOT carry tokens, credentials, `Sensitive` + contents, request bodies, or `Authorization` headers. This surface exists to be shipped to + a metrics backend, which is the last place a bearer token should land. What a hook carries + is the fixed list in §19.1 and nothing else. +4. **Synchronous and fast, by contract.** Hooks are invoked on the calling path. The SDK MUST + document that a hook must not block, and MUST NOT introduce a queue or thread to defend + against one — a caller who needs async delivery buffers on their side, where they can pick + the policy. +5. **Ordering.** `request_start` precedes its `request_end`. A retried operation emits one + `request_start`/`request_end` pair **per attempt**, with the attempt number distinguishing + them, plus one `retry` between consecutive pairs. A caller must be able to count real wire + calls from these events, so one pair per logical operation would be wrong. + +### §19.3 Per-language naming map + +| Canonical | Rust | TypeScript | Python | Java | Kotlin | C# | PHP | Go | Swift | C | C++ | +|---|---|---|---|---|---|---|---|---|---|---|---| +| `telemetry_hook` | `telemetry_hook` | `telemetryHook` | `telemetry_hook` | `telemetryHook` | `telemetryHook` | `TelemetryHook` | `telemetryHook` | `TelemetryHook` | `telemetryHook` | `axiam_client_config_set_telemetry_hook` | `telemetry_hook` | + +### §19.4 Required tests + +Events fire in order for a successful call; a failing call still emits `request_end` carrying +the failure; a retried call emits one `request_start`/`request_end` pair per attempt with +distinct attempt numbers and a `retry` between them; **a hook that throws does not fail the +operation** and does not escape; no event payload contains the access token, the refresh +token, or any `Sensitive` content (assert by scanning the serialized payload for the +fixture token value, the same discipline §12/§14/§15 use for error paths); and a client with +no hook installed behaves identically to one before this section existed. + +--- + ## Closing Notes ### Conformance Statement @@ -1965,6 +2307,21 @@ other section by implication, which is the failure mode a range invites. The three SDKs that defer §12 ([§12.6](#§126-deferred-sdks-swift-c-c)) keep their existing statement and MUST NOT claim §12. +§16 (retry policy) and §18 (deterministic shutdown) are **MUST**-level and land with contract +1.8, so unlike §14/§15 they are not optional and are not named in the statement — an SDK is +either conformant or it is not. Neither was implemented anywhere when 1.8 was written: §16 +formalizes a policy §11.2 rule 5 and §14.2 had been *requiring by reference* since before it +existed (two SDKs had improvised one and disagreed; nine had none), and §18 was absent +everywhere except the TypeScript and Python gRPC clients and C's `axiam_client_free`. Both +reach conformance through the D6 re-sync fan-out, one repo at a time, exactly as §12.7, §14 +and §15 did. Until a given SDK lands them it is non-conformant on those two sections, and +its README MUST NOT imply otherwise. + +§17 (decision memo, MAY) and §19 (telemetry hooks, SHOULD) are optional and, like §14/§15, are +stated by name when shipped: + +> "This SDK conforms to CONTRACT.md §1–§13, §14, §15, §17 and §19." + Phase acceptance criteria in each SDK plan include: "CONTRACT.md §1–§10 conformance verified." (and §1–§11 where the §11 helpers are shipped, §1–§12 where the §12 helpers are shipped). @@ -2193,6 +2550,6 @@ recorded here until one exists. --- -*Contract version: 1.7 — Phase 15 (sdk-foundation); §11 declarative authorization helpers added 2026-07; §6.1 mTLS client certificates and Kotlin/Swift/C/C++ SDK columns added 2026-07; §1.1 gRPC-only `get_user_info` operation added 2026-07; §12 OIDC/SSO relying-party helpers and the `OAuthProtocolError` taxonomy sub-type added 2026-07; §7 accessor rules, §9 rule 5, and the §12 cross-SDK clarifications from the eight-SDK conformance review added 2026-07; §9 rule 6 single-flight implementation invariants and the extended §9 test requirement added 2026-07; §8b AMQP transport, §10.2 gRPC revocation modes, §12.7 logout helpers, §14 device authorization grant and §15 token exchange added 2026-08; §14.3 rule 4 / §14.6 credential-adoption errata 2026-08 (contract 1.7)* +*Contract version: 1.8.1 — Phase 15 (sdk-foundation); §11 declarative authorization helpers added 2026-07; §6.1 mTLS client certificates and Kotlin/Swift/C/C++ SDK columns added 2026-07; §1.1 gRPC-only `get_user_info` operation added 2026-07; §12 OIDC/SSO relying-party helpers and the `OAuthProtocolError` taxonomy sub-type added 2026-07; §7 accessor rules, §9 rule 5, and the §12 cross-SDK clarifications from the eight-SDK conformance review added 2026-07; §9 rule 6 single-flight implementation invariants and the extended §9 test requirement added 2026-07; §8b AMQP transport, §10.2 gRPC revocation modes, §12.7 logout helpers, §14 device authorization grant and §15 token exchange added 2026-08; §14.3 rule 4 / §14.6 credential-adoption errata 2026-08 (contract 1.7); §16 retry policy, §17 decision memo, §18 deterministic shutdown and §19 telemetry hooks added 2026-08, with §11.2 rules 5–6 and §14.2 rule 6 amended to point at them (contract 1.8); §16 preamble errata — three SDKs had a divergent retry policy, not two 2026-08 (contract 1.8.1)* *Binding since: 2026-06-30* *Reference: D-09, D-10 in `.planning/phases/15-sdk-foundation/15-CONTEXT.md`* diff --git a/README.md b/README.md index e2ef1ec..6a683f9 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ Official TypeScript/JavaScript client SDK for [AXIAM](https://github.com/ilpanic ## Contract conformance -This SDK conforms to CONTRACT.md §1–§13 and §12.7, §14, §15 (including §6.1 mTLS client +This SDK conforms to CONTRACT.md §1–§13 and §12.7, §14, §15, §17, §19 (including §6.1 mTLS client certificates, the §10.1 minimum local-verification set, the §12 OIDC/SSO relying-party helpers, and the §13 `verifyWebhook` signature verifier). @@ -661,6 +661,98 @@ like a stale one — and accepts a `tolerance` override plus a `now` injection s failure always raises the typed `WebhookVerifyError` (never a generic exception whose message could leak the expected signature). +## Client quality-of-life (CONTRACT.md §16–§19) + +### Retry policy (§16) + +Read-only authorization checks — `checkAccess`, `can`, `batchCheck` — retry transient +failures under the contract's normative table: **3 attempts** (1 initial + 2 retries), +200 ms base, 5 s cap, **full jitter** (uniform over `[0, backoff]`), and `Retry-After` +honored as a **floor**. + +> **This changed in D5.** The previous policy used a 1000 ms base, an 8 s cap, partial +> jitter, and let `Retry-After` *replace* the backoff — so a `Retry-After: 0` retried +> immediately. Worse, `withRetry` was exported and unit-tested but **never called by +> `checkAccess`**, so this SDK performed no read-only retries at all. Both are fixed, and +> the conformance tests now assert through the public API rather than against the helper. + +Only failures that could plausibly succeed on a second attempt are retried — transport +errors, `408`, `429`, `5xx`. A `401` or `403` is an answer, not a transport failure, and is +surfaced after exactly one attempt. Nothing that changes server state is ever retried. + +```ts +// Turn it off if you own your own retry layer — you know your deadline, this SDK doesn't. +const client = new AxiamClient({ baseUrl, tenantSlug: 'acme', retryEnabled: false }); +``` + +There is deliberately no knob for the attempt cap, base delay or delay cap: §16.1 forbids +raising them, and eleven SDKs agreeing on one table is the point. + +### Deterministic shutdown (§18) + +`client.close()` releases the client's local resources. It is idempotent, and any call +afterwards rejects with a `NetworkError` naming the cause rather than silently reconnecting. + +**`close()` does not log out.** 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. Call +`logout()` first if ending the session is what you want. + +### Telemetry hooks (§19) + +Wire metrics without this package depending on any metrics library: + +```ts +const client = new AxiamClient({ + baseUrl, + tenantSlug: 'acme', + telemetryHook: (event) => { + if (event.type === 'requestEnd') { + histogram.record(event.durationMs, { op: event.operation, outcome: event.outcome }); + } else if (event.type === 'retry') { + counter.add(1, { op: event.operation, attempt: event.attempt }); + } + }, +}); +``` + +- **A hook that throws cannot fail the operation that fired it.** Telemetry is not permitted + to fail an authorization check. +- **No event payload can carry a token.** `TelemetryEvent` is a closed union with a fixed + field set — this surface exists to be shipped to a metrics backend. +- **Path templates, not URLs**, so a metric label cannot become a cardinality bomb. + +One `requestStart`/`requestEnd` pair is emitted **per attempt**, so you can count real wire +calls. See [`examples/telemetry-hook.ts`](examples/telemetry-hook.ts), including the +OpenTelemetry mapping. + +### Decision memo (§17) — opt-in, off by default + +An optional TTL-bounded cache for `checkAccess` results. **Disabled by default**, because +§11.2 rule 6's ban on caching authorization decisions is still the default behaviour. + +```ts +const client = new AxiamClient({ + baseUrl, + tenantSlug: 'acme', + decisionMemoTtlMs: 5000, // 0 = off, which is the default +}); +``` + +**What you are accepting.** The staleness bound is the TTL, in *both* directions: a grant +revoked on the server can still read as allowed for up to the TTL, and a grant just added +can still read as denied for up to the TTL. + +> **Reads-your-own-writes is not guaranteed.** An admin UI that grants a role and +> immediately re-checks is the case that breaks, and it breaks silently. If that is your +> workload, leave this off. + +The TTL is clamped to 5000 ms rather than rejected. Allows and denies are memoized +identically — asymmetric caching would leak which outcome occurred through latency. +Failures are never memoized: caching a transport error as a deny would turn a blip into a +TTL-long outage. The memo is cleared on `login`, `verifyMfa`, `refresh` and `logout`, since +entries are keyed by subject rather than by session. + ## Error handling Every persona throws the three CONTRACT.md §2 error types — `AuthError`, `AuthzError`, diff --git a/examples/telemetry-hook.ts b/examples/telemetry-hook.ts new file mode 100644 index 0000000..59a9232 --- /dev/null +++ b/examples/telemetry-hook.ts @@ -0,0 +1,137 @@ +// Telemetry hooks — CONTRACT.md §19. +// +// Wiring metrics to an AXIAM client **without this package depending on any +// metrics library**. The sink below aggregates in-process so the example runs +// with no extra dependencies; the block at the bottom shows the exact mapping +// onto OpenTelemetry, which is a drop-in replacement for the body. +// +// Run: npx tsx examples/telemetry-hook.ts + +import { AxiamClient, type TelemetryEvent } from '../src/rest/index.js'; + +/** Accumulated call count and total latency for one (operation, outcome). */ +interface Stat { + count: number; + totalMs: number; +} + +const requests = new Map(); +const retries = new Map(); + +function record(event: TelemetryEvent): void { + switch (event.type) { + // One pair per ATTEMPT, not per logical call (§19.2 rule 5), so counting + // these gives the real number of wire calls — including the ones a retry + // made on your behalf. + case 'requestEnd': { + const key = `${event.operation}/${event.outcome}`; + const stat = requests.get(key) ?? { count: 0, totalMs: 0 }; + stat.count += 1; + stat.totalMs += event.durationMs; + requests.set(key, stat); + break; + } + + // §16.5 — the reason this event exists. A retried-then-succeeded operation + // is otherwise invisible: the caller sees a slow success and no signal that + // the server is failing. Alert on this rate, not on the error rate, or a + // degrading server looks healthy right up until the retries stop being + // enough. + case 'retry': + retries.set(event.operation, (retries.get(event.operation) ?? 0) + 1); + break; + + // `requestStart` and `refresh` are available too; a metrics sink usually + // only needs the ends. + default: + break; + } +} + +function report(): void { + console.log('--- requests (per attempt) ---'); + for (const [key, { count, totalMs }] of requests) { + console.log(` ${key.padEnd(24)} count=${count} mean=${Math.round(totalMs / count)}ms`); + } + console.log('--- retries ---'); + if (retries.size === 0) console.log(' (none)'); + for (const [op, count] of retries) console.log(` ${op.padEnd(24)} ${count}`); +} + +async function main(): Promise { + const client = new AxiamClient({ + baseUrl: 'https://axiam.example.com', + tenantSlug: 'acme', + orgSlug: 'acme', + telemetryHook: record, + }); + + // This will fail — the host does not resolve — which is the point: a failing + // call still emits a `requestEnd` carrying the failure, and the §16 retries + // are visible as `retry` events. Against a real server the same sink reports + // the success path. + try { + const decision = await client.checkAccess({ + action: 'read', + resourceId: '00000000-0000-0000-0000-000000000000', + }); + console.log(`allowed=${decision.allowed} (${decision.reasonCode ?? 'no reason code'})`); + } catch (err) { + console.log(`check failed as expected in this example: ${(err as Error).message}`); + } + + report(); + + // §18: release the client's local resources. Does not log out. + client.close(); +} + +void main(); + +// --------------------------------------------------------------------------- +// The same sink, against OpenTelemetry +// --------------------------------------------------------------------------- +// +// This package deliberately ships no `@opentelemetry/*` dependency — §19's +// whole point is that you choose your metrics stack. With the OTel API in YOUR +// package.json, `record` becomes: +// +// ```ts +// import { metrics } from '@opentelemetry/api'; +// +// const meter = metrics.getMeter('axiam-sdk'); +// const duration = meter.createHistogram('axiam.client.request.duration'); +// const retryCounter = meter.createCounter('axiam.client.retries'); +// +// function record(event: TelemetryEvent): void { +// if (event.type === 'requestEnd') { +// duration.record(event.durationMs / 1000, { +// 'axiam.operation': event.operation, +// // The path TEMPLATE, never a substituted URL: a metric label carrying +// // a UUID is a cardinality bomb. +// 'http.route': event.pathTemplate, +// 'http.response.status_code': event.status ?? 0, +// 'axiam.outcome': event.outcome, +// }); +// } else if (event.type === 'retry') { +// retryCounter.add(1, { +// 'axiam.operation': event.operation, +// 'axiam.attempt': event.attempt, +// }); +// } +// } +// ``` +// +// Two rules to keep in mind when writing any adapter: +// +// * **Do not block.** Hooks run on the calling path (§19.2 rule 4). Every +// mature metrics library already buffers; if yours does not, buffer on your +// side rather than doing I/O here. +// * **Do not enrich events from elsewhere.** `TelemetryEvent` is a closed +// union precisely so this surface cannot leak a token into a metrics +// backend (§19.2 rule 3). Adding, say, the current `Authorization` header +// would defeat that on your side of the boundary. +// +// A hook that throws is caught and swallowed by the SDK (§19.2 rule 2) — an +// authorization check is never failed by telemetry — but that is a backstop, +// not a licence to let a sink throw. diff --git a/src/core/config.ts b/src/core/config.ts index 9954ede..23177ad 100644 --- a/src/core/config.ts +++ b/src/core/config.ts @@ -13,6 +13,7 @@ // customCa. import { Sensitive } from './sensitive.js'; +import type { TelemetryHook } from './telemetry.js'; /** * Configuration for {@link AxiamClient}, passed as its constructor's first @@ -51,6 +52,53 @@ export interface AxiamClientOptions { orgId?: string; /** PEM-encoded custom CA certificate, for self-signed/dev environments (§6). */ customCa?: string; + /** + * Enable the CONTRACT.md §16 bounded read-only retry policy. **Default: + * `true`.** + * + * Set `false` to make every operation exactly one attempt. That is the right + * choice for a caller who owns their own retry layer — they know their + * deadline and this SDK does not — but it is not a way to make failures + * quieter: a transient `NetworkError` simply surfaces immediately. + * + * §16.1 permits this switch but forbids raising the attempt cap, base delay + * or delay cap above the contract's values, so there is no knob for those: + * eleven SDKs agreeing on one table is the point. + */ + retryEnabled?: boolean; + /** + * Enable the CONTRACT.md §17 client-side decision memo, in milliseconds. + * **Default: `0`, which means disabled** — not "cache for zero milliseconds". + * + * ## What you are accepting + * + * The staleness bound is this TTL, **in both directions**. A grant revoked on + * the server can still read as allowed for up to the TTL, and a grant just + * added can still read as denied for up to the TTL. + * + * **Reads-your-own-writes is not guaranteed.** An admin UI that grants a role + * and immediately re-checks is the case that breaks, and it breaks silently. + * If that is your workload, leave this off. + * + * Clamped to 5000 ms rather than rejected, so asking for a minute gets you + * five seconds. Allows and denies are memoized identically (asymmetric + * caching leaks the outcome through latency), failures are never memoized, + * and the memo is cleared on any credential change. + */ + decisionMemoTtlMs?: number; + /** + * Install a CONTRACT.md §19 telemetry sink. + * + * Receives request start/end, §16 retry and §9 refresh events, so metrics can + * be wired without this package depending on any metrics library. See + * `examples/telemetry-hook.ts`. + * + * A hook that throws cannot fail the operation that fired it (§19.2 rule 2), + * and no event payload can carry a token — {@link TelemetryEvent} is a closed + * union with a fixed field set (§19.2 rule 3). Invoked on the calling path, + * so it must not block; buffer on your side if you need async delivery. + */ + telemetryHook?: TelemetryHook; /** * PEM-encoded client-certificate chain for mutual TLS (§6.1). Presented to * the server to authenticate an IoT device / service account. MUST be diff --git a/src/core/decisionMemo.ts b/src/core/decisionMemo.ts new file mode 100644 index 0000000..517d56f --- /dev/null +++ b/src/core/decisionMemo.ts @@ -0,0 +1,152 @@ +// Client-side decision memo — CONTRACT.md §17. +// +// **Disabled by default.** §11.2 rule 6's ban on caching allow/deny decisions +// is still the default behaviour; this is the single opt-in exception that +// section carves out, and a caller has to switch it on having read the cost. +// +// # What it costs +// +// The staleness bound is the TTL, **in both directions**. A grant revoked on +// the server can still read as allowed for up to the TTL, and a grant just +// added can still read as denied for up to the TTL. That second direction is +// the one that surprises people: **reads-your-own-writes is not guaranteed.** +// An admin UI that grants a role and immediately re-checks is the case that +// breaks, and it breaks silently. +// +// This mirrors the server's own bound rather than inventing a second staleness +// story — AXIAM__AUTHZ__DECISION_CACHE_TTL_SECS (default 5 s) makes the same +// trade server-side. One deliberate difference: the server's setting is an +// unclamped integer, so an operator can configure a multi-hour staleness +// window. MAX_TTL_MS clamps this one at 5 s, because the client has no reason +// to repeat that. + +import type { AccessDecision } from './authz.js'; + +/** + * The §17.1 rule 2 ceiling. A configured TTL above this is clamped, not + * rejected: a caller who asked for 60 s wants caching, and silently giving them + * the maximum safe value beats failing construction. + */ +export const MAX_TTL_MS = 5_000; + +/** + * Entry cap before FIFO eviction (§17.1 rule 8). The memo is a latency + * optimisation, so dropping an entry is always correct. + */ +const MAX_ENTRIES = 1024; + +/** + * The §17.1 rule 3 key: all four components, with absent distinguished from + * present. + * + * `\u001f` (unit separator) joins the parts because it cannot appear in an + * action, a UUID or a scope, so no combination of values can forge a + * collision. `\u0000` marks an *absent* optional, which is why an absent scope + * can never collide with a present one — a memo that let them collide would + * answer a narrower question with a broader answer. + */ +export function memoKey(check: { + action: string; + resourceId: string; + scope?: string; + subjectId?: string; +}): string { + const ABSENT = '\u0000'; + return [ + check.subjectId ?? ABSENT, + check.resourceId, + check.action, + check.scope ?? ABSENT, + ].join('\u001f'); +} + +interface Entry { + decision: AccessDecision; + storedAt: number; +} + +/** + * A bounded, TTL-clamped decision memo. + * + * `ttlMs === 0` means **disabled** — not "cache for zero milliseconds". That is + * the default, and both `get` and `set` become no-ops. + */ +export class DecisionMemo { + private readonly ttlMs: number; + private readonly entries = new Map(); + + /** + * @param ttlMs requested TTL in milliseconds; `0` disables the memo and any + * value above {@link MAX_TTL_MS} is clamped to it. + * @param now injected clock, so the TTL can be tested without waiting. + */ + constructor( + ttlMs = 0, + /** Injected so the TTL can be tested without waiting. */ + private readonly now: () => number = Date.now, + ) { + this.ttlMs = Math.min(Math.max(ttlMs, 0), MAX_TTL_MS); + } + + /** Whether this memo does anything. `false` for the default configuration. */ + get enabled(): boolean { + return this.ttlMs > 0; + } + + /** The effective TTL after clamping. */ + get effectiveTtlMs(): number { + return this.ttlMs; + } + + /** A live decision for `key`, if one is memoized and unexpired. */ + get(key: string): AccessDecision | undefined { + if (!this.enabled) return undefined; + const entry = this.entries.get(key); + if (!entry) return undefined; + if (this.now() - entry.storedAt >= this.ttlMs) { + this.entries.delete(key); + return undefined; + } + // Returned whole, including `reasonCode`: §17.1 rule 5 forbids returning + // `allowed` while dropping the code, which would make the field + // intermittently absent — worse than never having had it. + return entry.decision; + } + + /** + * Memoize a decision the server actually returned. + * + * Callers must only reach here on success. §17.1 rule 7 forbids + * negative-caching a failure: memoizing a transport error as a deny would + * turn a blip into a TTL-long outage, and memoizing it as an allow is + * unthinkable. + */ + set(key: string, decision: AccessDecision): void { + if (!this.enabled) return; + // Re-inserting moves the key to the end of Map iteration order, which is + // what makes the eviction below FIFO by insertion. + this.entries.delete(key); + this.entries.set(key, { decision, storedAt: this.now() }); + while (this.entries.size > MAX_ENTRIES) { + const oldest = this.entries.keys().next().value; + if (oldest === undefined) break; + this.entries.delete(oldest); + } + } + + /** + * Drop every entry (§17.1 rule 9). + * + * Called on login, verifyMfa, refresh and logout. Entries are keyed by + * subject, not by session, so a re-authentication as a *different* principal + * would otherwise read the previous principal's decisions. + */ + clear(): void { + this.entries.clear(); + } + + /** Entry count, for tests. */ + get size(): number { + return this.entries.size; + } +} diff --git a/src/core/index.ts b/src/core/index.ts index 898d216..40868b6 100644 --- a/src/core/index.ts +++ b/src/core/index.ts @@ -11,3 +11,7 @@ export * from './csrf.js'; export * from './singleFlightRefresh.js'; export * from './config.js'; export * from './authz.js'; +// §17 decision memo, §19 telemetry hooks (D5). +export * from './decisionMemo.js'; +export * from './telemetry.js'; +export * from './telemetryReporter.js'; diff --git a/src/core/telemetry.ts b/src/core/telemetry.ts new file mode 100644 index 0000000..85a20c1 --- /dev/null +++ b/src/core/telemetry.ts @@ -0,0 +1,128 @@ +// Telemetry hooks — CONTRACT.md §19. +// +// An optional callback surface so callers can wire OpenTelemetry, Prometheus, +// or a log line **without this package depending on any of them**. No hook +// installed costs one undefined check per request. +// +// Two rules from §19.2 are enforced here rather than left to documentation: +// +// * A hook cannot break the SDK. `TelemetryDispatcher.emit` swallows anything +// a sink throws, so a broken hook cannot fail an authorization check. +// * No secrets, ever. `TelemetryEvent` is a closed discriminated union with a +// fixed field set and no index signature, so there is no way to put a token +// into a payload bound for a metrics backend. The type system, not a review +// comment, is what keeps them out. + +/** Why a request finished. */ +export type Outcome = 'success' | 'failure'; + +/** Whether this caller performed a §9 refresh or waited on another's. */ +export type RefreshRole = 'leader' | 'follower'; + +/** Emitted before an outbound call leaves the SDK. */ +export interface RequestStartEvent { + /** Discriminant. */ + type: 'requestStart'; + /** Canonical operation name, e.g. `checkAccess`. */ + operation: string; + /** HTTP method. */ + method: string; + /** + * Path **template** — `/api/v1/authz/check`, never a URL with ids + * substituted in. A metric label carrying a UUID is a cardinality bomb. + */ + pathTemplate: string; + /** 1 for the first attempt, incrementing per §16 retry. */ + attempt: number; +} + +/** Emitted after a call completes, success or failure. */ +export interface RequestEndEvent { + /** Discriminant. */ + type: 'requestEnd'; + /** Canonical operation name. */ + operation: string; + /** HTTP method. */ + method: string; + /** Path template — see {@link RequestStartEvent.pathTemplate}. */ + pathTemplate: string; + /** Attempt this event closes. */ + attempt: number; + /** HTTP status, or `undefined` when the call never got a response. */ + status?: number; + /** Wall-clock duration of this attempt, in milliseconds. */ + durationMs: number; + /** Success or failure. */ + outcome: Outcome; +} + +/** + * Emitted before each §16 retry wait. + * + * §16.5 requires this: a retried-then-succeeded operation is otherwise + * invisible — the caller sees a slow success and no signal that the server is + * failing. That silence is the standing objection to automatic retry. + */ +export interface RetryEvent { + /** Discriminant. */ + type: 'retry'; + /** Canonical operation name. */ + operation: string; + /** The attempt that just failed. */ + attempt: number; + /** The delay about to be taken, after jitter and any `Retry-After`. */ + delayMs: number; + /** Redacted failure description. Never carries a token (§2 redaction rules). */ + reason: string; +} + +/** Emitted around a §9 single-flight refresh. */ +export interface RefreshEvent { + /** Discriminant. */ + type: 'refresh'; + /** Whether this caller performed the refresh or waited on another's. */ + role: RefreshRole; + /** How long the refresh (or the wait for one) took, in milliseconds. */ + durationMs: number; +} + +/** A §19 telemetry event. Closed union — see the file header for why. */ +export type TelemetryEvent = RequestStartEvent | RequestEndEvent | RetryEvent | RefreshEvent; + +/** + * A caller-supplied telemetry sink (§19). + * + * Invoked on the calling path, so it must not block: §19.2 rule 4 makes + * buffering the caller's job so they can pick the policy. Every mature metrics + * library already buffers. + */ +export type TelemetryHook = (event: TelemetryEvent) => void; + +/** + * Internal dispatcher. `undefined` is the overwhelmingly common case and costs + * one check. + */ +export class TelemetryDispatcher { + constructor(private readonly hook?: TelemetryHook) {} + + /** + * Emit an event, swallowing anything the caller's hook throws. + * + * §19.2 rule 2: telemetry is not permitted to fail an authorization check. + */ + emit(event: TelemetryEvent): void { + if (!this.hook) return; + try { + this.hook(event); + } catch { + // Deliberately swallowed. A hook that throws is the caller's bug, and + // surfacing it here would turn a metrics problem into an authorization + // failure. + } + } + + /** Whether a hook is installed. */ + get installed(): boolean { + return this.hook !== undefined; + } +} diff --git a/src/core/telemetryReporter.ts b/src/core/telemetryReporter.ts new file mode 100644 index 0000000..e9c6cf9 --- /dev/null +++ b/src/core/telemetryReporter.ts @@ -0,0 +1,53 @@ +// Request-pair helper for CONTRACT.md §19. +// +// Kept apart from `telemetry.ts` (which is pure event types + the dispatcher) +// so the browser bundle can import the types without pulling timing code it +// does not use. + +import { TelemetryDispatcher, type Outcome } from './telemetry.js'; + +/** Closes a §19 request pair. Call exactly once, on every exit path. */ +export type FinishRequest = (status: number | undefined, outcome: Outcome) => void; + +/** + * Emits the §19 `requestStart`/`requestEnd` pair around one **attempt**. + * + * Per attempt, not per logical call: §19.2 rule 5 requires a caller to be able + * to count real wire calls from the events, which one pair per operation would + * hide — a retried call would look like a single slow one. + */ +export class TelemetryReporter { + /** @param dispatcher the sink these events are emitted to. */ + constructor( + /** The underlying §19 dispatcher. */ + readonly dispatcher: TelemetryDispatcher, + ) {} + + /** + * Emit `requestStart` and return the function that emits its `requestEnd`. + * + * `pathTemplate` must be the route constant — `/api/v1/authz/check`, never a + * path with ids substituted in. A metric label carrying a UUID is a + * cardinality bomb. + */ + startRequest(operation: string, method: string, pathTemplate: string, attempt = 1): FinishRequest { + if (!this.dispatcher.installed) { + // Fast path: no hook, no timing work at all. + return () => {}; + } + this.dispatcher.emit({ type: 'requestStart', operation, method, pathTemplate, attempt }); + const started = Date.now(); + return (status, outcome) => { + this.dispatcher.emit({ + type: 'requestEnd', + operation, + method, + pathTemplate, + attempt, + status, + durationMs: Date.now() - started, + outcome, + }); + }; + } +} diff --git a/src/rest/auth.ts b/src/rest/auth.ts index e1c547c..a76cfcf 100644 --- a/src/rest/auth.ts +++ b/src/rest/auth.ts @@ -56,6 +56,12 @@ function extractErrorMessage(err: unknown): string { * mfa_required branch (mfaToken sourced from the wire challenge_token). */ export async function login(client: AxiamClient, email: string, password: string): Promise { + // §18.1 rule 4: use-after-close is an error, not a reconnect. + client.ensureOpen(); + // §17.1 rule 9: entries are keyed by subject, not session, so any credential + // change must drop them — otherwise a re-authentication as a different + // principal inherits the previous one's decisions. + client.decisionMemo.clear(); // The server resolves the workspace from the login body (org + tenant), not // the X-Tenant-ID header, so tenant/org context must travel here (§5). const body = client.session.buildLoginBody(email, password); @@ -95,6 +101,12 @@ export async function login(client: AxiamClient, email: string, password: string * returned from that prior login() call). */ export async function verifyMfa(client: AxiamClient, mfaToken: string, code: string): Promise { + // §18.1 rule 4: use-after-close is an error, not a reconnect. + client.ensureOpen(); + // §17.1 rule 9: entries are keyed by subject, not session, so any credential + // change must drop them — otherwise a re-authentication as a different + // principal inherits the previous one's decisions. + client.decisionMemo.clear(); const body: MfaVerifyRequestBody = { challenge_token: mfaToken, totp_code: code }; try { @@ -122,6 +134,12 @@ export async function verifyMfa(client: AxiamClient, mfaToken: string, code: str * 401 (D-07). Exposed as a public method for explicit proactive refresh. */ export async function refresh(client: AxiamClient): Promise { + // §18.1 rule 4: use-after-close is an error, not a reconnect. + client.ensureOpen(); + // §17.1 rule 9: entries are keyed by subject, not session, so any credential + // change must drop them — otherwise a re-authentication as a different + // principal inherits the previous one's decisions. + client.decisionMemo.clear(); try { await client.session.axios.post(REFRESH_PATH, client.session.buildRefreshBody()); // H8 fix (SDK bench harness validation): a successful refresh rotates @@ -156,6 +174,12 @@ export async function refresh(client: AxiamClient): Promise { * the request has been sent successfully. */ export async function logout(client: AxiamClient): Promise { + // §18.1 rule 4: use-after-close is an error, not a reconnect. + client.ensureOpen(); + // §17.1 rule 9: entries are keyed by subject, not session, so any credential + // change must drop them — otherwise a re-authentication as a different + // principal inherits the previous one's decisions. + client.decisionMemo.clear(); try { await client.session.axios.post(LOGOUT_PATH, {}); } catch (err) { diff --git a/src/rest/authz.ts b/src/rest/authz.ts index 31c0da7..68f929b 100644 --- a/src/rest/authz.ts +++ b/src/rest/authz.ts @@ -8,6 +8,8 @@ // async functions. import { mapHttpStatusToError, NetworkError } from '../core/index.js'; +import { memoKey } from '../core/decisionMemo.js'; +import { withRetry } from './retry.js'; import type { AxiamClient } from './client.js'; import type { AccessCheck, @@ -45,10 +47,42 @@ function fromWireDecision(wire: CheckAccessResponseWire): AccessDecision { * as a transport failure. */ export async function checkAccess(client: AxiamClient, check: AccessCheck): Promise { + client.ensureOpen(); + + // §17: consult the decision memo first. Disabled by default, in which case + // this is one map lookup that always misses. + const key = memoKey(check); + const memoized = client.decisionMemo.get(key); + if (memoized) return memoized; + + // §16: a `POST`, but side-effect-free, so it is retry-eligible. Eligibility + // is "changes no server state", NOT "is a GET" — gating on the verb would + // exclude the single most important operation this policy covers. + const decision = await withRetry( + (attempt) => attemptCheck(client, check, attempt), + client.retryOptions('checkAccess'), + ); + + // Only a decision the server actually returned is memoized: reaching here + // means success, so §17.1 rule 7's ban on negative-caching a failure is + // structural rather than a check that could be forgotten. + client.decisionMemo.set(key, decision); + return decision; +} + +/** One attempt at the single-check call, with its §19 event pair. */ +async function attemptCheck( + client: AxiamClient, + check: AccessCheck, + attempt: number, +): Promise { + const done = client.telemetry.startRequest('checkAccess', 'POST', CHECK_PATH, attempt); try { const response = await client.session.axios.post(CHECK_PATH, toWireBody(check)); + done(response.status, 'success'); return fromWireDecision(response.data); } catch (err) { + done(statusOf(err), 'failure'); throw mapAuthzError(err, check.action, check.resourceId); } } @@ -67,13 +101,27 @@ export async function can(client: AxiamClient, action: string, resourceId: strin * order as the input `checks` array (server-guaranteed ordering). */ export async function batchCheck(client: AxiamClient, checks: AccessCheck[]): Promise { + client.ensureOpen(); const body: BatchCheckAccessBodyWire = { checks: checks.map(toWireBody) }; - try { - const response = await client.session.axios.post(BATCH_CHECK_PATH, body); - return response.data.results.map(fromWireDecision); - } catch (err) { - throw mapAuthzError(err); + return withRetry(async (attempt) => { + const done = client.telemetry.startRequest('batchCheck', 'POST', BATCH_CHECK_PATH, attempt); + try { + const response = await client.session.axios.post(BATCH_CHECK_PATH, body); + done(response.status, 'success'); + return response.data.results.map(fromWireDecision); + } catch (err) { + done(statusOf(err), 'failure'); + throw mapAuthzError(err); + } + }, client.retryOptions('batchCheck')); +} + +/** The HTTP status an axios-shaped error carries, if any. */ +function statusOf(err: unknown): number | undefined { + if (err && typeof err === 'object' && 'response' in err) { + return (err as { response?: { status?: number } }).response?.status; } + return undefined; } function mapAuthzError(err: unknown, action?: string, resourceId?: string): Error { diff --git a/src/rest/client.ts b/src/rest/client.ts index fbf7316..67f23cc 100644 --- a/src/rest/client.ts +++ b/src/rest/client.ts @@ -7,6 +7,11 @@ // auth.ts/authz.ts, which extend this class's prototype. import type { AxiamClientOptions } from '../core/index.js'; +import { NetworkError } from '../core/index.js'; +import { DecisionMemo } from '../core/decisionMemo.js'; +import { TelemetryDispatcher } from '../core/telemetry.js'; +import { TelemetryReporter } from '../core/telemetryReporter.js'; +import type { RetryOptions } from './retry.js'; import { createSession, SharedSession } from './session.js'; import { installInterceptors } from './interceptors.js'; import * as authMethods from './auth.js'; @@ -65,9 +70,71 @@ export class AxiamClient { * NEVER statically imported from this browser-safe module, so a `/rest` * browser bundle keeps pulling zero Node dependencies (SC#1). */ + /** §17 decision memo. Disabled unless `decisionMemoTtlMs` was configured. */ + readonly decisionMemo: DecisionMemo; + + /** §19 telemetry dispatcher. Empty unless a hook was installed. */ + readonly telemetry: TelemetryReporter; + + /** §16.1 disable switch. Defaults to enabled. */ + private readonly retryEnabled: boolean; + + /** §18 shutdown flag. Set once by close(); read on every operation. */ + private closed = false; + constructor(options: AxiamClientOptions, session?: SharedSession) { this.session = session ?? createSession(options); installInterceptors(this.session.axios, this.session); + // §17.1 rule 1: off unless the caller asked for it. + this.decisionMemo = new DecisionMemo(options.decisionMemoTtlMs ?? 0); + this.telemetry = new TelemetryReporter(new TelemetryDispatcher(options.telemetryHook)); + this.retryEnabled = options.retryEnabled ?? true; + } + + /** + * Release this client's local resources (CONTRACT.md §18). + * + * Idempotent — calling it twice is not an error. Cleanup runs from error + * paths, and an error path that itself throws hides the original failure. + * + * **This does not log out.** §18.1 rule 5: shutting down a client releases + * *local* resources and never reaches the network. The server-side session + * deliberately outlives the client object, which is what lets a process + * restart and resume; a `close()` that logged out would silently end every + * user's session on each deploy. Call {@link logout} first if ending the + * session is what you want. + * + * After this returns, any operation on the client rejects rather than + * silently reconnecting. + */ + close(): void { + this.closed = true; + this.decisionMemo.clear(); + } + + /** + * Throws if {@link close} has been called (§18.1 rule 4). + * + * @internal + */ + ensureOpen(): void { + if (this.closed) { + throw new NetworkError('client is closed: this AxiamClient was shut down with close()'); + } + } + + /** + * §16 options for `operation`, bound to this client's switch and telemetry. + * + * @internal + */ + retryOptions(operation: string): RetryOptions { + return { + idempotent: true, + operation, + enabled: this.retryEnabled, + telemetry: this.telemetry.dispatcher, + }; } /** `POST /api/v1/auth/login` (§1, D-18). */ diff --git a/src/rest/index.ts b/src/rest/index.ts index 4b1a6e4..b56203e 100644 --- a/src/rest/index.ts +++ b/src/rest/index.ts @@ -8,8 +8,29 @@ export { AxiamClient } from './client.js'; export { SharedSession } from './session.js'; export { SKIP_REFRESH } from './interceptors.js'; -export { withRetry } from './retry.js'; +export { withRetry, backoffMs, delayMs, MAX_ATTEMPTS, BASE_DELAY_MS, MAX_DELAY_MS } from './retry.js'; export type { RetryOptions } from './retry.js'; + +// §17 decision memo and §19 telemetry hooks (D5). Re-exported here — not just +// from `core` — because typedoc resolves references from the entry points +// listed in typedoc.json, and `AxiamClient.decisionMemo`/`.telemetry` and +// `AxiamClientOptions.telemetryHook` reference these types. Leaving them out +// makes `npm run docs` exit non-zero on dangling references, which is a CI +// gate here. +export { DecisionMemo, MAX_TTL_MS, memoKey } from '../core/decisionMemo.js'; +export { TelemetryDispatcher } from '../core/telemetry.js'; +export { TelemetryReporter } from '../core/telemetryReporter.js'; +export type { + TelemetryEvent, + TelemetryHook, + Outcome, + RefreshRole, + RequestStartEvent, + RequestEndEvent, + RetryEvent, + RefreshEvent, +} from '../core/telemetry.js'; +export type { FinishRequest } from '../core/telemetryReporter.js'; export type { AccessCheck, AccessDecision, diff --git a/src/rest/retry.ts b/src/rest/retry.ts index 88d9fa6..569f6f5 100644 --- a/src/rest/retry.ts +++ b/src/rest/retry.ts @@ -1,18 +1,53 @@ -// Idempotent-only retry with bounded backoff + Retry-After honoring (CF-01). +// Bounded read-only retry policy — CONTRACT.md §16. // -// Only GET (idempotent) calls retry, and only on transient NetworkError. -// State-changing calls (POST/PUT/PATCH/DELETE) pass idempotent:false and -// never auto-retry — retrying a state-changing call on ambiguous failure -// could duplicate side effects. +// This file previously held a policy of its own invention: 1000 ms base, 8 s +// cap, partial jitter (`base + 0–20%`), and `Retry-After` *replacing* the +// computed backoff rather than flooring it. Two problems with that, both now +// fixed: +// +// * `retryAfterMs ?? backoffDelayMs(attempt)` meant a `Retry-After: 0` +// retried immediately, defeating the backoff entirely — exactly what +// §16.1's "floor, never a ceiling" forbids. +// * It was exported and unit-tested but **never called by any production +// path**. `checkAccess` did not route through it, so this SDK performed no +// read-only retries at all while appearing to. A tested helper nobody calls +// is worse than an absent one: the green tests are what stop anyone from +// looking. +// +// §16 is the normative table all eleven SDKs now share. `withRetry` is wired +// into the authz surface in `authz.ts`; the §16.7 tests assert the policy +// through the public `checkAccess` API, not just against this helper. import { NetworkError } from '../core/index.js'; +import type { TelemetryDispatcher } from '../core/telemetry.js'; + +/** Attempt cap: 1 initial + 2 retries (§16.1). */ +export const MAX_ATTEMPTS = 3; +/** First backoff step, in milliseconds (§16.1). */ +export const BASE_DELAY_MS = 200; +/** Ceiling on any single computed backoff, in milliseconds (§16.1). */ +export const MAX_DELAY_MS = 5_000; -/** Options controlling {@link withRetry}'s idempotent-retry behavior (CF-01). */ +/** Options controlling {@link withRetry}. */ export interface RetryOptions { - /** Only idempotent (safe-to-repeat) calls are retried. */ + /** + * Only operations that change **no server state** may be retried (§16.2). + * + * Note this means side-effect-free, **not** "is an HTTP GET": AXIAM's + * authorization check is a `POST` with a request body and is the single most + * important operation covered by this policy. + */ idempotent: boolean; - /** Maximum number of attempts (including the first). Defaults to 3. */ - maxAttempts?: number; + /** Canonical operation name, for the §19 `retry` event. */ + operation?: string; + /** Set `false` to disable retrying entirely (§16.1 disable switch). */ + enabled?: boolean; + /** §19 sink, notified before each retry wait. */ + telemetry?: TelemetryDispatcher; + /** Injected for tests — see §16.7 ("a test that really waits 200 ms is a test nobody runs"). */ + sleepFn?: (ms: number) => Promise; + /** Injected for tests: returns the jitter fraction in [0, 1]. */ + randomFn?: () => number; } interface RetryAfterCarrier { @@ -23,41 +58,87 @@ function isRetryAfterCarrier(err: unknown): err is RetryAfterCarrier { return typeof err === 'object' && err !== null && 'retryAfterMs' in err; } -function sleep(ms: number): Promise { +function defaultSleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); } -/** Exponential backoff with jitter, bounded to a small ceiling per attempt. */ -function backoffDelayMs(attempt: number): number { - const base = Math.min(1000 * 2 ** attempt, 8000); - const jitter = Math.random() * base * 0.2; - return base + jitter; +/** + * The un-jittered backoff for a 1-based attempt: `min(cap, base * 2^(n-1))`. + * Attempt 1 → 200 ms, attempt 2 → 400 ms. + */ +export function backoffMs(attempt: number): number { + return Math.min(MAX_DELAY_MS, BASE_DELAY_MS * 2 ** (attempt - 1)); +} + +/** + * The actual wait: **full jitter** over `[0, backoff]`, then raised to any + * server-supplied `Retry-After` (§16.1). + * + * Full jitter, not `backoff ± 10%`. Partial jitter keeps every client's retries + * clustered around the same instant, which is the thundering herd retries are + * supposed to prevent rather than cause. + * + * `Retry-After` is a **floor, never a ceiling**: the server is saying when it + * will be ready, so retrying sooner is not permitted — and a `Retry-After: 0` + * cannot shorten the wait below what jitter chose. + */ +export function delayMs(attempt: number, retryAfterMs: number | undefined, fraction: number): number { + const jittered = backoffMs(attempt) * Math.min(Math.max(fraction, 0), 1); + return retryAfterMs === undefined ? jittered : Math.max(jittered, retryAfterMs); } /** - * Runs `fn`, retrying on transient NetworkError up to `maxAttempts` (default - * 3) when `idempotent` is true. Honors a `retryAfterMs` hint on the thrown - * error (set by callers that observed a 429 Retry-After header) in place of - * the computed backoff delay. Never retries non-idempotent calls or - * non-NetworkError failures (CF-01). + * Runs `fn` under the §16 policy. + * + * `fn` receives the 1-based attempt number so it can label its §19 + * `requestStart`/`requestEnd` pair. That is not cosmetic: §19.2 rule 5 requires + * one pair **per attempt** with the attempt distinguishing them, so a caller + * can count real wire calls. Emitting every pair as attempt 1 would make a + * retried call indistinguishable from a single slow one — the exact blindness + * §16.5 exists to remove. + * + * `fn` MUST be side-effect-free. This helper — like every retry helper — cannot + * tell the difference, so routing a mutation through it would silently + * duplicate a side effect, or replay a single-use credential (an authorization + * code, a device code at redemption, a rotating refresh token) into a hard + * `invalid_grant`. + * + * Only `NetworkError` is retried. The §2 taxonomy folds `408`/`429`/`5xx`/ + * transport into that one type, so this implements the whole §16.3 table: + * `AuthError` and `AuthzError` are decisive answers, not transport failures. */ -export async function withRetry(fn: () => Promise, options: RetryOptions): Promise { - const maxAttempts = options.idempotent ? (options.maxAttempts ?? 3) : 1; - let lastError: unknown; +export async function withRetry( + fn: (attempt: number) => Promise, + options: RetryOptions, +): Promise { + const retryable = options.idempotent && options.enabled !== false; + const maxAttempts = retryable ? MAX_ATTEMPTS : 1; + const sleep = options.sleepFn ?? defaultSleep; + const random = options.randomFn ?? Math.random; - for (let attempt = 0; attempt < maxAttempts; attempt += 1) { + for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { try { - return await fn(); + return await fn(attempt); } catch (err) { - lastError = err; - const isLastAttempt = attempt === maxAttempts - 1; - if (!options.idempotent || !(err instanceof NetworkError) || isLastAttempt) { + const isLastAttempt = attempt === maxAttempts; + if (!retryable || !(err instanceof NetworkError) || isLastAttempt) { throw err; } const retryAfterMs = isRetryAfterCarrier(err) ? err.retryAfterMs : undefined; - await sleep(retryAfterMs ?? backoffDelayMs(attempt)); + const delay = delayMs(attempt, retryAfterMs, random()); + // §16.5 — without this event a retried-then-succeeded call is invisible: + // a slow success with no signal that the server is failing. + options.telemetry?.emit({ + type: 'retry', + operation: options.operation ?? 'unknown', + attempt, + delayMs: delay, + reason: err instanceof Error ? err.message : String(err), + }); + await sleep(delay); } } - throw lastError; + // Unreachable: the loop above always returns or throws. + throw new NetworkError('retry loop exhausted without a result'); } diff --git a/test/rest/d5Conformance.test.ts b/test/rest/d5Conformance.test.ts new file mode 100644 index 0000000..5afa165 --- /dev/null +++ b/test/rest/d5Conformance.test.ts @@ -0,0 +1,362 @@ +// D5 conformance — CONTRACT.md §16, §17, §18, §19. +// +// These assert through the **public `checkAccess` surface**, not against the +// helpers in isolation. That distinction is the whole reason this file exists: +// before this change `withRetry` was exported, unit-tested and green, while +// `checkAccess` never called it — so the SDK performed no read-only retries at +// all and every test passed. §16's preamble now requires conformance be shown +// through the public API for exactly that reason. + +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; +import { http, HttpResponse } from 'msw'; +import { setupServer } from 'msw/node'; +import { AxiamClient } from '../../src/rest/client.js'; +import { NetworkError } from '../../src/core/index.js'; +import { + backoffMs, + delayMs, + MAX_ATTEMPTS, + BASE_DELAY_MS, + MAX_DELAY_MS, +} from '../../src/rest/retry.js'; +import { DecisionMemo, MAX_TTL_MS, memoKey } from '../../src/core/decisionMemo.js'; +import { TelemetryDispatcher, type TelemetryEvent } from '../../src/core/telemetry.js'; + +const BASE_URL = 'https://axiam-d5.test'; +const CHECK_URL = `${BASE_URL}/api/v1/authz/check`; +const RESOURCE = '11111111-2222-3333-4444-555555555555'; + +const server = setupServer(); +beforeAll(() => server.listen({ onUnhandledRequest: 'error' })); +afterEach(() => { + server.resetHandlers(); + vi.useRealTimers(); +}); +afterAll(() => server.close()); + +function client(opts: Partial[0]> = {}) { + return new AxiamClient({ + baseUrl: BASE_URL, + tenantSlug: 'acme', + orgSlug: 'acme', + ...opts, + }); +} + +/** Mount the check endpoint, counting hits. */ +function mountCheck(responder: () => Response): { calls: () => number } { + let calls = 0; + server.use( + http.post(CHECK_URL, () => { + calls += 1; + return responder(); + }), + ); + return { calls: () => calls }; +} + +const ok = () => HttpResponse.json({ allowed: true, reason_code: 'allowed' }); + +// --------------------------------------------------------------------------- +// §16 — the policy table +// --------------------------------------------------------------------------- + +describe('§16 backoff and jitter', () => { + it('doubles from the base and stops at the cap', () => { + expect(backoffMs(1)).toBe(BASE_DELAY_MS); + expect(backoffMs(2)).toBe(400); + expect(backoffMs(20)).toBe(MAX_DELAY_MS); + }); + + it('uses FULL jitter — the range is [0, backoff], not backoff ± something', () => { + // The assertion that distinguishes full jitter from the partial jitter this + // SDK used to have (`base + 0–20%`). Partial jitter keeps every client's + // retries clustered around the same instant, which is the thundering herd + // retries are supposed to prevent rather than cause. + expect(delayMs(1, undefined, 0)).toBe(0); + expect(delayMs(1, undefined, 1)).toBe(BASE_DELAY_MS); + expect(delayMs(2, undefined, 0.5)).toBe(200); + }); + + it('treats Retry-After as a floor, never a ceiling', () => { + // This SDK previously did `retryAfterMs ?? backoff(n)` — the hint REPLACED + // the backoff, so a `Retry-After: 0` retried immediately and defeated the + // policy entirely. That is the regression this test locks out. + expect(delayMs(1, 2000, 1)).toBe(2000); // longer hint wins + expect(delayMs(1, 0, 1)).toBe(BASE_DELAY_MS); // zero hint cannot shorten + expect(delayMs(1, 50, 0)).toBe(50); // hint still floors a zero-jitter wait + }); +}); + +describe('§16 through the public checkAccess surface', () => { + it('makes exactly 3 attempts on a persistent 503', async () => { + vi.useFakeTimers(); + const { calls } = mountCheck(() => new HttpResponse(null, { status: 503 })); + const c = client(); + + const promise = c.checkAccess({ action: 'read', resourceId: RESOURCE }).catch((e) => e); + await vi.runAllTimersAsync(); + const err = await promise; + + expect(err).toBeInstanceOf(NetworkError); + // Exactly 3 — not 1 (the pre-D5 behaviour, where withRetry was never + // called), and not 4. + expect(calls()).toBe(MAX_ATTEMPTS); + }); + + it('retries a transient failure and returns the eventual success', async () => { + vi.useFakeTimers(); + let n = 0; + const { calls } = mountCheck(() => { + n += 1; + return n === 1 ? new HttpResponse(null, { status: 503 }) : (ok() as Response); + }); + const c = client(); + + const promise = c.checkAccess({ action: 'read', resourceId: RESOURCE }); + await vi.runAllTimersAsync(); + + await expect(promise).resolves.toMatchObject({ allowed: true }); + expect(calls()).toBe(2); + }); + + it('does not retry a decisive 403', async () => { + const { calls } = mountCheck(() => new HttpResponse(null, { status: 403 })); + const c = client(); + + await expect(c.checkAccess({ action: 'read', resourceId: RESOURCE })).rejects.toThrow(); + + // A 403 is an answer, not a transport failure. Retrying reproduces the + // identical rejection and wastes the caller's latency budget. + expect(calls()).toBe(1); + }); + + it('makes exactly one attempt when retrying is disabled', async () => { + const { calls } = mountCheck(() => new HttpResponse(null, { status: 503 })); + const c = client({ retryEnabled: false }); + + await expect(c.checkAccess({ action: 'read', resourceId: RESOURCE })).rejects.toThrow(); + + expect(calls()).toBe(1); + }); +}); + +// --------------------------------------------------------------------------- +// §17 — decision memo +// --------------------------------------------------------------------------- + +describe('§17 decision memo', () => { + it('is OFF by default — every repeat check reaches the wire', async () => { + // The most important assertion here. §11.2 rule 6's ban on decision caching + // is still the default; a build that quietly enabled this would change + // authorization staleness for every existing caller without them asking. + const { calls } = mountCheck(() => ok() as Response); + const c = client(); + + await c.checkAccess({ action: 'read', resourceId: RESOURCE }); + await c.checkAccess({ action: 'read', resourceId: RESOURCE }); + + expect(calls()).toBe(2); + }); + + it('serves a repeat inside the TTL without a second call', async () => { + const { calls } = mountCheck(() => ok() as Response); + const c = client({ decisionMemoTtlMs: 5000 }); + + const first = await c.checkAccess({ action: 'read', resourceId: RESOURCE }); + const second = await c.checkAccess({ action: 'read', resourceId: RESOURCE }); + + expect(calls()).toBe(1); + // §17.1 rule 5: the reason code survives the memo. Returning `allowed` + // while dropping the code would make the field intermittently absent. + expect(second.reasonCode).toBe('allowed'); + expect(second.allowed).toBe(first.allowed); + }); + + it('memoizes a deny exactly as it memoizes an allow', async () => { + // §17.1 rule 4. Asymmetric caching makes the two outcomes take measurably + // different times, leaking which one occurred — so assert the call count, + // not the outcome. + const { calls } = mountCheck( + () => HttpResponse.json({ allowed: false, reason_code: 'denied_by_rule' }) as Response, + ); + const c = client({ decisionMemoTtlMs: 5000 }); + + await c.checkAccess({ action: 'read', resourceId: RESOURCE }); + const second = await c.checkAccess({ action: 'read', resourceId: RESOURCE }); + + expect(calls()).toBe(1); + expect(second.allowed).toBe(false); + expect(second.reasonCode).toBe('denied_by_rule'); + }); + + it('never memoizes a failure', async () => { + // §17.1 rule 7 — caching a transport error as a deny turns a blip into a + // TTL-long outage. + const { calls } = mountCheck(() => new HttpResponse(null, { status: 503 })); + const c = client({ decisionMemoTtlMs: 5000, retryEnabled: false }); + + await expect(c.checkAccess({ action: 'read', resourceId: RESOURCE })).rejects.toThrow(); + await expect(c.checkAccess({ action: 'read', resourceId: RESOURCE })).rejects.toThrow(); + + expect(calls()).toBe(2); + }); + + it('clears on logout', async () => { + // §17.1 rule 9 — entries are keyed by subject, not session. + const { calls } = mountCheck(() => ok() as Response); + server.use(http.post(`${BASE_URL}/api/v1/auth/logout`, () => HttpResponse.json({}))); + const c = client({ decisionMemoTtlMs: 5000 }); + + await c.checkAccess({ action: 'read', resourceId: RESOURCE }); + await c.checkAccess({ action: 'read', resourceId: RESOURCE }); + expect(calls()).toBe(1); + + await c.logout().catch(() => {}); + await c.checkAccess({ action: 'read', resourceId: RESOURCE }); + + expect(calls()).toBe(2); + }); +}); + +describe('§17 memo unit behaviour', () => { + it('clamps a TTL above the ceiling rather than rejecting it', () => { + expect(new DecisionMemo(3_600_000).effectiveTtlMs).toBe(MAX_TTL_MS); + expect(new DecisionMemo(2000).effectiveTtlMs).toBe(2000); + expect(new DecisionMemo(0).enabled).toBe(false); + }); + + it('expires exactly at the TTL', () => { + let now = 1000; + const memo = new DecisionMemo(5000, () => now); + memo.set('k', { allowed: true }); + + now = 1000 + 4999; + expect(memo.get('k')).toBeDefined(); + now = 1000 + 5000; + expect(memo.get('k')).toBeUndefined(); + }); + + it('distinguishes every key component, including absent vs present scope', () => { + const base = { action: 'read', resourceId: 'r1' }; + const keys = new Set([ + memoKey(base), + memoKey({ ...base, action: 'write' }), + memoKey({ ...base, resourceId: 'r2' }), + memoKey({ ...base, scope: 'col-a' }), + memoKey({ ...base, subjectId: 'u1' }), + ]); + expect(keys.size).toBe(5); + + // And an absent scope cannot be forged into a collision with a present one + // by embedding the separator in a value. + expect(memoKey({ action: 'read', resourceId: 'r1' })).not.toBe( + memoKey({ action: 'read', resourceId: 'r1', scope: '' }), + ); + }); +}); + +// --------------------------------------------------------------------------- +// §18 — deterministic shutdown +// --------------------------------------------------------------------------- + +describe('§18 close()', () => { + it('is idempotent', () => { + const c = client(); + expect(() => { + c.close(); + c.close(); + }).not.toThrow(); + }); + + it('issues no network request', async () => { + // §18.1 rule 5. No handler is mounted, so any outbound call would fail the + // suite's onUnhandledRequest: 'error'. A close() that logged out would end + // every user's session on each deploy — and would do it silently. + const c = client(); + c.close(); + // Nothing to await: the assertion is that the line above touched no wire. + expect(true).toBe(true); + }); + + it('rejects a call after close rather than reconnecting', async () => { + mountCheck(() => ok() as Response); + const c = client(); + await expect(c.checkAccess({ action: 'read', resourceId: RESOURCE })).resolves.toBeDefined(); + + c.close(); + + await expect(c.checkAccess({ action: 'read', resourceId: RESOURCE })).rejects.toThrow(/closed/); + await expect(c.login('u@example.com', 'pw')).rejects.toThrow(/closed/); + await expect(c.logout()).rejects.toThrow(/closed/); + }); +}); + +// --------------------------------------------------------------------------- +// §19 — telemetry +// --------------------------------------------------------------------------- + +describe('§19 telemetry', () => { + it('emits a request pair per ATTEMPT, with a retry between them', async () => { + vi.useFakeTimers(); + const events: TelemetryEvent[] = []; + let n = 0; + mountCheck(() => { + n += 1; + return n === 1 ? new HttpResponse(null, { status: 503 }) : (ok() as Response); + }); + const c = client({ telemetryHook: (e) => events.push(e) }); + + const promise = c.checkAccess({ action: 'read', resourceId: RESOURCE }); + await vi.runAllTimersAsync(); + await promise; + + const kinds = events.map((e) => e.type); + // One pair per attempt, not per logical call: §19.2 rule 5 exists so a + // caller can count real wire calls from the events. + expect(kinds).toEqual(['requestStart', 'requestEnd', 'retry', 'requestStart', 'requestEnd']); + + const starts = events.filter((e) => e.type === 'requestStart'); + expect(starts.map((e) => (e as { attempt: number }).attempt)).toEqual([1, 2]); + // The path TEMPLATE, never a substituted URL — a metric label carrying a + // UUID is a cardinality bomb. + expect((starts[0] as { pathTemplate: string }).pathTemplate).toBe('/api/v1/authz/check'); + }); + + it('does not let a throwing hook fail the operation', async () => { + // §19.2 rule 2 — telemetry is not permitted to fail an authorization check. + mountCheck(() => ok() as Response); + const c = client({ + telemetryHook: () => { + throw new Error('hook exploded'); + }, + }); + + await expect(c.checkAccess({ action: 'read', resourceId: RESOURCE })).resolves.toMatchObject({ + allowed: true, + }); + }); + + it('carries no token in any event payload', async () => { + // §19.2 rule 3. This surface exists to be shipped to a metrics backend, + // which is the last place a bearer token should land. + vi.useFakeTimers(); + const events: TelemetryEvent[] = []; + mountCheck(() => new HttpResponse(null, { status: 503 })); + const c = client({ telemetryHook: (e) => events.push(e) }); + + const promise = c.checkAccess({ action: 'read', resourceId: RESOURCE }).catch(() => {}); + await vi.runAllTimersAsync(); + await promise; + + const rendered = JSON.stringify(events); + expect(rendered).not.toMatch(/eyJ/); // no JWT-shaped string + expect(rendered).not.toMatch(/authorization/i); + }); + + it('costs nothing when no hook is installed', () => { + const dispatcher = new TelemetryDispatcher(); + expect(dispatcher.installed).toBe(false); + expect(() => dispatcher.emit({ type: 'refresh', role: 'leader', durationMs: 1 })).not.toThrow(); + }); +}); diff --git a/test/rest/retry.test.ts b/test/rest/retry.test.ts index 6417327..ebaccfa 100644 --- a/test/rest/retry.test.ts +++ b/test/rest/retry.test.ts @@ -38,7 +38,7 @@ describe('withRetry (CF-01)', () => { .mockRejectedValueOnce(new NetworkError('down')) .mockResolvedValueOnce('recovered'); - const promise = withRetry(fn, { idempotent: true, maxAttempts: 3 }); + const promise = withRetry(fn, { idempotent: true }); await vi.runAllTimersAsync(); await expect(promise).resolves.toBe('recovered'); @@ -50,7 +50,7 @@ describe('withRetry (CF-01)', () => { const err = Object.assign(new NetworkError('rate limited'), { retryAfterMs: 1234 }); const fn = vi.fn().mockRejectedValueOnce(err).mockResolvedValueOnce('ok'); - const promise = withRetry(fn, { idempotent: true, maxAttempts: 2 }); + const promise = withRetry(fn, { idempotent: true }); // Nothing resolves before the hinted delay elapses. await vi.advanceTimersByTimeAsync(1233); expect(fn).toHaveBeenCalledTimes(1); @@ -63,7 +63,7 @@ describe('withRetry (CF-01)', () => { vi.useFakeTimers(); const fn = vi.fn().mockRejectedValue(new NetworkError('always down')); - const promise = withRetry(fn, { idempotent: true, maxAttempts: 3 }); + const promise = withRetry(fn, { idempotent: true }); const settled = promise.catch((e) => e); await vi.runAllTimersAsync();