feat: key cache - #1212
feat: key cache#1212
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: cf4f698 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
🟡 Changes recommended
The cache lacks resolved-crypter caching, invalidation, and metrics required by issue #1100.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces bounded in-memory project-key caches to reduce database reads during token encryption, decryption, and signing.
Changes:
- Adds configurable LRU caches for encryption and signing keys.
- Routes project key persistence through
KeyService. - Updates test wiring, mocks, and release notes.
File summaries
| File | Description |
|---|---|
internal/service/project.go |
Persists project keys through KeyService. |
internal/service/project_test.go |
Mocks key persistence behavior. |
internal/service/mocks/service.mock.go |
Adds generated key-save mocks. |
internal/service/keys.go |
Implements key caches and save methods. |
internal/service/keys_test.go |
Supplies caches to service tests. |
internal/service/event_payload_emit_test.go |
Updates project-event test mocks. |
internal/bootstrap/platform/ensure_sqlite_test.go |
Adds caches to bootstrap tests. |
internal/api/integration_test/helpers/key.go |
Configures integration-test caches. |
cmd/server/server.go |
Constructs caches and sets defaults. |
cmd/server/config.go |
Defines cache configuration fields. |
.changeset/project-key-cache.md |
Documents the shipped behavior. |
Review details
Files not reviewed (1)
- internal/service/mocks/service.mock.go: Generated file
Suppressed comments (1)
internal/service/keys.go:38
- This description names the wrong key type and contains duplicated prepositions.
// SigningKeyCache caches encryption keys to reduce the load of on the database.
- Files reviewed: 10/11 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
Cache misses currently recurse until stack exhaustion, and required observability and measurement criteria remain incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- internal/service/mocks/service.mock.go: Generated file
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
internal/service/keys.go:210
- The new signing-key cache-hit path has no regression test: the added tests cover only crypters. Add a test that calls
GetProjectSigningKeytwice, asserts one storage read, and verifies project ID and purpose cannot cross-hit so this separate cache contract is protected.
internal/service/keys.go:163
- On every cache miss this calls
getCrypterOfKeyagain with the same key before any entry is added, so the first token/secret decryption recurses until the process exhausts its stack. Resolve the wrapping key instead (recursing only for a project KEK), then cache the resulting crypter; the direct-master-key branch must also flow through the cache insertion.
crypter, err := s.getCrypterOfKey(ctx, key)
if err != nil {
return nil, err
}
- Files reviewed: 10/11 changed files
- Comments generated: 3
- Review effort level: Balanced
mridang
left a comment
mridang
left a comment
There was a problem hiding this comment.
I had a deep look and I really couldn't spot anything out of the orginary. LGTM!
|
A side note:
That's fine at two, but it grows with every cache we add. Each new one means another interface, wrapper, constructor and mock repeating the same instrumentation. We can already see the drift: the schema cache in server.go uses lru.New2Q directly and isn't instrumented at all. Could we write this once as a generic type instead? Something like:
|
|
Good point, initially there were more differences between them but during development they grew more and more towards each other. Maybe I should consolidate them into one. Will have a look. |
|
@mridang I made the |
…#1278) ## Summary - The claim page could spend its single-use challenge twice — once in the document hosting the sign-in widget and once in the document the widget navigated to — so the second `claim/complete` answered 409 and the page told the developer their project was "Already claimed". This is what failed `cli-journey-e2e` `claim.spec.ts` on #1247 (run 35725532364), and it is the same symptom #1212 saw; #1212's `globalThis` gate covers remounts within one document, and this one is two documents. - Cause, from the Playwright trace: on the terminal step `<zitadel-login>` retires its back-gesture sentinel (ADR 022) with `history.back()`. That `popstate` is a navigation to the console router, which reloads the claim route's loader; by then `sessions/exchange` has set the cookie, so the loader finds a session, the page swaps the widget for `CompleteClaim` and POSTs `claim/complete` — 2 ms after `location.assign` had already requested the next document. The browser aborts the request on unload, the server has already completed the claim, and the next document spends again. It's a race against the next document's HTML response, which is why it shows up on a loaded CI runner (`--concurrency 5`) and not locally. - Two independent guards, each with a unit test that fails without it: - `packages/components` — when the terminal step is about to navigate to `post-sign-in-url` (`this.completing`), the sentinel is retired in place with `history.replaceState({ ..., zl: false })` instead of a traversal, so the host never sees a `popstate`. Steps that stay on screen traverse exactly as before. The retired entry stays under the destination (a same-URL destination like the claim page replaces it), which is the same one-hop stale-sentinel tolerance the code already accepts. - `apps/console` — the `/claim/` route sets `shouldReload: false`: the session is read once per document, and no navigation the router observes while the widget is up can hand the page to `CompleteClaim`. The intended way back is a full-document navigation, so the initial read is the only one that matters; `router.invalidate()` still reloads. ## Validation - `moon run components:test` — `zitadel-login.spec.ts` 48/48; the new "retires the sentinel in place when the terminal step navigates away" case fails on the unpatched orchestrator (at the `history.back` call count) and passes with the fix. - `moon run console:test` — `claim.spec.tsx` 18/18; the new "does not spend the challenge when the widget's history sentinel moves the router" case renders on a real `createBrowserHistory`, replays the widget's `pushState({ zl: true })` → `history.back()` with a session now available, and asserts the widget stays up, the loader ran once and `claim/complete` was never called. Without `shouldReload: false` it fails with the page on "Project claimed". - `tsc --noEmit -p apps/console/tsconfig.spec.json` clean; `oxlint` clean on the four touched files. - Not run: the `cli-journey-e2e` claim journey itself (needs the release snapshot); the `full-pr` job on this PR covers it. ## Release notes / changeset - Changeset: `.changeset/login-terminal-sentinel.md` — `@zitadel/components` patch: `<zitadel-login>` no longer fires a host `popstate` when retiring its sentinel on a navigating terminal step. The console change ships with the server binary and needs no separate entry. ## Notes - Either guard alone prevents this failure; both are here so the claim page doesn't depend on widget behaviour and the widget doesn't leak navigations into any host router, not just ours. - #1247 (and any other PR that reaches the fresh-app journey) can be re-run once this merges — the journey builds the release snapshot from the merge commit. - Follow-up worth a small PR: `skipPasskeyUpsellIfVisible` in `claim.spec.ts` still races on a "Skip for now" button that #1169 removed from the default flow, so a claim failure surfaces as a bare 30 s timeout; asserting on the outcome heading and failing fast on "Already claimed" would have named this bug directly. - Behavioural nit for reviewers: for a different-URL `post-sign-in-url` (e.g. `/login` → `/`), the in-place retirement leaves one extra history entry for the login URL under the destination; a back press lands on the login page signed in, which redirects. The previous `history.back()` popped it first. Judged acceptable given the alternative; happy to discuss. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Summary
Project keys are now resolved once and kept in memory, so an authenticated request no longer re-reads its encryption key from the database and re-unwraps it with the master key on every call. Two new settings size the caches:
keys.crypter_lru_cache_sizeandkeys.signing_key_lru_cache_size, both defaulting to 1000 entries. Cached entries are dropped when the cache is full and when the server restarts; which key is currently active is never cached, so a future key rotation still takes effect immediately.uncached 854 µs/op, cached 30 ns/op, from BenchmarkGetCrypter with the database read mocked in both arms so the difference is the unwrap.
Validation
go test ./...go test -tags postgres_integration ./...go test -tags sqlite_integration ./...go test -tags spanner_integration ./...Release notes / changeset
Notes
Also fixes the claim page spending a claim link twice across a remount. Not caused by the cache, but the cache is what made it deterministic: the first completion now commits before the second is sent, so the second always gets the contractual 409 and always lands last, replacing "Project claimed" with "Already claimed". That is what turned cli-journey-e2e red.