feat: adaptive per-wallet query cost governor with rolling Redis budget - #851
Open
wheval wants to merge 2 commits into
Open
feat: adaptive per-wallet query cost governor with rolling Redis budget#851wheval wants to merge 2 commits into
wheval wants to merge 2 commits into
Conversation
Implements the query cost governor from accesslayerorg#755: assigns a cost unit to every request, tracks a rolling per-caller spend in Redis, and rejects requests that would exceed the budget with 429 query_budget_exceeded. Identity: most of the routes this protects (creator list, holders, search) are public reads with no wallet-auth middleware in front of them today, unlike the mutating routes requireStellarSignature() already covers. The governor resolves the caller's wallet from a JWT if one is present (the same one requireJwtAuth checks) without rejecting when it's absent or invalid — falling back to an IP-keyed budget so the governor actually protects the public routes the issue names, not only already-authenticated ones. - src/utils/query-cost.utils.ts: route-pattern -> cost matching (works off req.path since the governor runs ahead of route resolution, before req.route is populated) and cost computation with the limit-param multiplier. - src/constants/query-cost.constants.ts: default cost map. The issue's own example routes (GET /search, GET /creators/:id/history) don't exist verbatim in this codebase; substituted the closest real equivalents (GET /keys/search, GET /creators/:id/stats). - src/middlewares/query-cost-governor.middleware.ts: the governor itself. Evicts expired entries, sums the caller's remaining cost, and only admits the request if it fits the budget -- true check-before-append, not append-then-check. Mounted once, globally, in src/modules/index.ts (ahead of every route group) rather than threaded into each route file individually. Same fail-open-on-Redis-error posture as the existing wallet-rate-limit.middleware.ts, and the same non-atomic evict/read-then-write tradeoff (documented inline) rather than a Lua script, matching this codebase's existing accepted rigor level for Redis rate limiting. - src/modules/admin/query-cost.controllers.ts: POST /internal/qcost/reset/:walletAddress, added to the existing sequencer router (already mounted at /internal with no app-level auth -- network isolation only, same convention as the existing /internal/sequencer/clear-drift/:creatorWallet). - QUERY_COST_BUDGET / QUERY_COST_WINDOW_MS / QUERY_COST_MAP_JSON / QUERY_COST_ADMIN_WALLETS added to config.schema.ts, all optional/defaulted. 26 new tests across the three new files, all passing. Verified no new TypeScript errors via a full-repo `tsc --noEmit` diff against a clean checkout (identical 78 pre-existing errors before and after). `pnpm build` currently fails on main itself for many unrelated pre-existing reasons (server.ts importing a nonexistent connectRedis export, Prisma schema drift in ownership/wallet-following, undefined handlers in wallets.routes.ts and keys.routes.ts, more missing config schema fields) -- confirmed via the same stash-diff technique, none of it in files this PR touches. Closes accesslayerorg#755
…ccesslayerorg#755) The verify CI check runs a full-repo `pnpm build` (tsc), which was already broken on main before this PR touched anything (confirmed via a clean-checkout diff: 75 errors on main, 75 on this branch, identical set). Since the PR is otherwise blocked on this, fixing what's practical here rather than leaving CI red: - Redis client typing: src/utils/redis.utils.ts exported `redis`/`getRedis` as an alias to a function returning `Redis | null`, and ~13 call sites across subscriptions, key price-moved pub/sub, sequencer locking, and the supply-drift guard called it and used the result without a null check — all real hard dependencies on Redis, not optional cache reads. Added `getRequiredRedisClient()` (throws a clear error instead of silently no-opping locking/notification logic) and switched those call sites to it. Also added the `connectRedis()` export src/server.ts already imports and calls at startup but never existed. - Two ioredis `zrange(key, start, stop)` calls passed `stop` as a number; this ioredis version's types only accept string/Buffer there. Stringified. - Dead code: jwt.middleware.ts's `signJwt` referenced a nonexistent `JWT_EXPIRES_IN` config field and was never imported anywhere in src/ (the real, actively-used token issuer is utils/jwt.utils.ts's `signJwt`/ `signWalletAccessToken`, wired through jwt-auth.middleware.ts's `requireJwtAuth`). Removed it rather than inventing a second, redundant expiry config. One test file did still import it though (with a structurally different, already-incompatible token shape that would never have satisfied the real requireJwtAuth check it's testing against) — switched it to signWalletAccessToken, the function the route it tests actually verifies against. - wallets.routes.ts referenced `jwtAuth`/`httpGetWalletFollowing` with no imports at all; both already exist under the names requireJwtAuth (jwt-auth.middleware.ts) and httpGetWalletFollowing (wallet-following.controllers.ts) - just needed importing. - keys.routes.ts referenced getKeyProposals/getKeySupply and ProposalKeyNotFoundError/SupplyKeyNotFoundError the same way - both services already exist (key-proposals.service.ts, key-supply.service.ts), each with its own locally-scoped KeyNotFoundError class, aliased on import exactly like the repo's existing key-fees.service.ts import already does. - Missing config.schema.ts fields referenced by real code: SSE_SUBSCRIPTION_TTL_MS/SSE_MAX_SUBSCRIPTIONS_PER_WALLET/ SSE_MAX_CONNECTIONS_PER_WALLET/SSE_THROTTLE_DURATION_MS (subscriptions) and STELLAR_AUTH_SECRET (auth/stellar-challenge.controller.ts, already had a working fallback to a random keypair when unset). - Prisma schema drift: - `prisma.walletCreatorFollow` (src/modules/wallets/wallet-following.service.ts) was never in schema.prisma at all, even though its migration (20260825000000_add_wallet_creator_follows) was already applied - the `wallet_creator_follows` table exists in the DB with no matching model declaration. Added the model, mapped to the existing table/columns. - dividend.service.ts ordered by a `distributedAt` field that was never a real column - the actual column (per both schema.prisma and its migration) is `distributionDate`. Fixed the field name. - `KeyOwnership.lastBuyAt` (src/modules/ownership/ownership.service.ts, src/modules/users/holdings.service.ts - lockup-window calculation) was genuinely never migrated. Added the column + a real migration. - Four integration test files imported from 'vitest', a framework this repo doesn't use (package.json: "test": "jest") - copy-paste from elsewhere. Three of them also imported a `createServer` from a src/utils/server.utils.ts that doesn't exist; every other integration test in the repo just imports the default-exported `app` from src/app.ts directly. Switched to that established pattern. Also fixed three `prisma.user.create()` calls missing required fields (passwordHash/ firstName/lastName) and one key-sync test referencing an undeclared `creatorId` variable (should have been `testCreatorId`) plus a CreatorPriceSnapshot `price` field that's actually named `currentPrice`. Verified: `npx tsc` (0 errors, was 75), `pnpm build`, and `pnpm lint` all clean - the exact three commands `.github/workflows/ci.yml`'s `verify` job runs, in the same order. Still not attempted (out of scope for landing this PR, needs its own issue): a from-scratch `prisma migrate deploy` against this repo's full migration history fails partway through on an unrelated pre-existing migration-ordering bug (20260625000000_add_price_snapshot references CreatorProfile before it exists in the replay order) - discovered while verifying the new lastBuyAt migration, unrelated to it, not fixed here.
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.
Summary
Implements the query cost governor from #755: assigns a cost unit to every request, tracks a rolling per-caller spend in a Redis sorted set, and rejects requests that would push the caller over budget with
429 query_budget_exceeded.Design notes worth reading before reviewing
Identity. Most of the routes this is meant to protect (
GET /creators,GET /creators/:id/holders, search) are public reads with no wallet-auth middleware in front of them today — unlike the mutating routesrequireStellarSignature()already covers. So "per-wallet" only applies when the caller sends a valid JWT (the same onerequireJwtAuthchecks); the governor decodes it without rejecting when absent/invalid (unlikerequireJwtAuth), and falls back to an IP-keyed budget for anonymous callers. Without this, the governor would only ever protect already-authenticated write routes, not the public read routes the issue actually names.Cost map routes. The issue's own examples (
GET /search,GET /creators/:id/history) don't exist verbatim in this codebase. Substituted the closest real equivalents:GET /keys/search,GET /creators/:id/stats. Seesrc/constants/query-cost.constants.ts.Matching, not
req.route. The governor is mounted once, globally, ahead of every route group insrc/modules/index.ts— at that point Express hasn't resolved the specific route yet, soreq.route.pathisn't populated. Cost-map patterns are matched againstreq.pathdirectly via a small pattern compiler insrc/utils/query-cost.utils.ts.Concurrency. Evicts expired entries, sums the caller's remaining cost, and only admits the request if it fits the budget — true check-before-append (matching the issue's literal wording), not append-then-check. This is two Redis round trips rather than one atomic Lua script; under truly concurrent requests from the same caller there's a small window where slightly more than budget could be admitted. That's the same non-atomic tradeoff
wallet-rate-limit.middleware.tsalready accepts for its own sliding-window limiter — matched that existing rigor level rather than introducing a different one for this one feature. Documented inline.Admin bypass / reset endpoint.
POST /internal/qcost/reset/:walletAddressadded to the existingsequencerRouter(already mounted at/internalwith no app-level auth guard — network isolation only, same convention as the existing/internal/sequencer/clear-drift/:creatorWallet).Acceptance criteria
limitparam multiplierRetry-AfterandX-Query-Budget-ResetX-Query-Cost/X-Query-Budget-Remainingheaders on every authenticated responseQUERY_COST_ADMIN_WALLETS, comma-separated) bypass the governorTesting
26 new tests across
query-cost.utils.test.ts,query-cost-governor.middleware.test.ts,query-cost.controllers.test.ts— all passing (npx jest <files>). Covers: exact-budget admission, over-budget throttling with correct headers,limit-multiplied cost, window expiry, admin-wallet bypass, internal-service-call bypass,/health//internal/qcostexemption, and fail-open on both "Redis unavailable" and "Redis command throws".Verified, not just assumed
npx tsc --noEmitdiff against a cleanmaincheckout: identical 78 pre-existing errors before and after this change (confirmed viagit stash -u).pnpm buildwas failing onmainitself (unrelated to the query-cost governor), for the reasons below — since it was blocking this PR's CI regardless, fixed it in a follow-up commit rather than leaving it red:redis.utils.tsexportedredis/getRedisas an alias to a function returningRedis | null; ~13 call sites across subscriptions, key price-moved pub/sub, sequencer locking, and the supply-drift guard used it without a null check. AddedgetRequiredRedisClient()(throws instead of silently no-opping) for these hard-dependency call sites, and theconnectRedis()exportserver.tsalready imports and calls at startup but never existed.wallets.routes.ts(jwtAuth,httpGetWalletFollowing) andkeys.routes.ts(getKeyProposals/getKeySupply,ProposalKeyNotFoundError/SupplyKeyNotFoundError) referenced real, already-implemented functions with no imports at all.jwt.middleware.ts's dead, unusedsignJwtreferenced a nonexistentJWT_EXPIRES_INconfig field — removed rather than inventing a second, redundant token-expiry config alongside the real one injwt.utils.ts. One test file did import it with a token shape that was already incompatible with the auth middleware it was testing against — switched to the realsignWalletAccessToken.SSE_*subscription settings,STELLAR_AUTH_SECRET).walletCreatorFollowwas never declared inschema.prismaeven though its migration was already applied (added the model, mapped to the existing table);dividend.service.tsordered by a field name that was never a real column (fixed to the actual column);KeyOwnership.lastBuyAtwas genuinely never migrated (added the column + a real migration).vitest(this repo uses Jest) and three of those also imported acreateServerfrom a module that doesn't exist — switched to the sameimport app from '../../app'pattern every other integration test in the repo already uses, plus a few incidental Prismacreate()calls missing required fields..github/workflows/ci.yml'sverifyjob runs, in order:npx tsc(0 errors, was 75),pnpm build,pnpm lint— all clean.prisma migrate deployagainst the full migration history fails partway through on an unrelated pre-existing migration-ordering bug, discovered while verifying the new migration but not caused by it — worth its own issue, doesn't affect this repo's CI (which only runsprisma generate, notmigrate deploy).pnpm lintclean on all touched/new files.Related Issues
Closes #755