refactor(auth): give coalesced API-key usage stats their own service - #1026
Merged
Conversation
pendingUsage was the one piece of state shared between three concerns that change for entirely different reasons: the per-request authentication hot path that incremented it, admin key lifecycle that dropped a deleted or revoked key's counters, and shutdown that flushed whatever was left. That is what made AuthService a god object rather than a large one. ApiKeyUsageTracker owns the map, the two window constants, the windowed save and the bounded shutdown flush. record() takes the ENTITY rather than an id, because the hot path mutates lastUsedAt/usageCount on the instance validateApiKey is about to return so the response reflects the true count including pending increments. forget(keyId) replaces the two lifecycle deletes. AuthService keeps a one-line onModuleDestroy that delegates. Five spec callsites drive shutdown through it, and it is the natural owner of the Nest hook — the tracker exposes flushOnShutdown() rather than implementing the hook itself, so the flush has exactly one trigger. 572 to 505 lines, and grep for pendingUsage in auth.service.ts returns nothing. Behaviour identical: the coalescing window, the lost-update merge on a failed windowed write, and the 5s shutdown bound all moved verbatim. Spec change is two lines — the provider and its import. No assertion, harness or fixture was touched; 141 auth tests pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
pendingUsagewas the one piece of state shared between three concerns that change for entirelydifferent reasons:
auth.service.ts:522-546),:367,:394),:141-172).That shared map is what made
AuthServicea god object rather than merely a large one — the concernsotherwise touch none of the same state.
What moves
ApiKeyUsageTrackerowns the map, both window constants, the windowed save and the bounded shutdownflush.
record()takes the entity, not an id: the hot path mutateslastUsedAt/usageCounton theinstance
validateApiKeyis about to return, so the response reflects the true count includingincrements still pending.
forget(keyId)replaces the two lifecycle deletes.AuthServicekeeps a one-lineonModuleDestroythat delegates. Five spec callsites drive shutdownthrough it (
spec:713, :729, :732, :737, :749), and it is the natural owner of the Nest hook — thetracker exposes
flushOnShutdown()rather than implementing the hook itself, so the flush has exactlyone trigger rather than two.
Scope
572 → 505 lines;
grep -c pendingUsage auth.service.tsreturns 0.Behaviour is identical — the coalescing window, the lost-update merge that re-accumulates on a failed
windowed write, and the 5s bounded shutdown flush all moved verbatim.
Spec change is two lines: the provider and its import. No assertion, harness or fixture was touched.
141 auth tests pass.
Remaining
auth.service.tsstill has one spanning item — theAPI_KEY_FILEmodule const derived fromprocess.cwd(), shared between operator bootstrap and admin lifecycle. That is a separate change andcarries a real e2e defect with it:
test/setup-e2e.tsredirects both SQLite databases but has no leverfor this path, so an e2e boot rewrites the developer's repo-root
data/.api-key. Fixing it means makingthe path injected rather than a const, which is why it is not bundled here.
Verification
Backend lint,
prettier --check,tsc --noEmitover the full project including specs, build, 4012 unittests, 137 e2e tests, and the dashboard build all pass on Node 22.