Repository navigation
[1433] Prisma $transaction isolation level defaults to ReadCommitted allowing phantom reads in allocation rebalance - #1517
Merged
Conversation
added 7 commits
September 29, 2026 23:10
main is currently unbuildable and its test suite is red, so every back-end CI job fails before it reaches the code under review. The breakage all traces back to two bad merges (9a15975, 7300a48) that truncated a Prisma model block, dropped a closing brace, renamed an imported limiter and replaced the idempotency replay cache. Prisma schema - close `WalletTenantAssociation` and separate it from `IdempotencyKey` (a missing `}` made `prisma generate` fail, so `@prisma/client` stayed an uninitialised stub and every suite crashed on import) - add `SessionAuditLog`, `Transaction.deletedAt` and `WebhookEndpoint.tenantId`, all of which sessionAudit.ts and the tenant boundary guard already query - add the matching migration and refresh the committed SQLite dev.db Type errors - import `readsLimiter` in vaultEndpoints.ts (used by GET /receipts) - restore the idempotency replay cache as idempotencyStore.ts and re-export it from idempotency.ts, so transferOrchestrator.ts, vaultEndpoints.ts, index.ts and idempotencyRetention.ts resolve - fix the unterminated `if` in diffSchemaShapes and a possibly-undefined `baseline.properties` read in apiContractSnapshots.ts - build the OTel resource through whichever factory the installed @opentelemetry/resources major exposes (v1 `new Resource()`, v2 `resourceFromAttributes()`) - `ZodObject._shape` → `.shape` for Zod 4 - return the strategy-switch 200 response, order receipts by `timestamp`, serialise session metadata, and type the request mocks in src/tests Error contract - restore `summary`, `errors` and 404 `path` on the error envelope Tests - issues711 "newly added required fields" mutated the live schema instead of the baseline snapshot, so it asserted against a scenario that can never produce the expected diff
An unauthenticated caller could pass `?limit=100000` and have it forwarded straight into Prisma's `take`, forcing the API to materialise 100k vault rows and OOM-kill the 512MB container. Chosen behaviour (documented in the route, the spec and the tests): reject rather than silently clamp. A caller asking for 100000 rows has a bug or is probing, and quietly returning 50 hides that behind a response that looks like a complete page. So `limit > 50` fails fast with `400` and `code: 'LIMIT_EXCEEDED'`, and the body repeats the ceiling. `page` is the opposite case — a benign mistake — so it is clamped into 1..1000 and the effective value is echoed in the pagination envelope. - new `middleware/paginationGuard.ts`: `DEFAULT_PAGE_SIZE` (20), `MAX_PAGE_SIZE` (50), `MAX_PAGE` (1000), `resolvePagination()` and the `enforcePaginationLimits()` middleware - new `routes/vaults.ts`: `GET /api/v1/vaults`, rate limited, reads `limit + 1` rows for the lookahead, and never exposes `tenantId` - `parsePaginationQuery` gains `maxPage` and clamps out-of-range pages; `PaginationQuerySchema.page` accepts a signed integer so those requests reach the clamp instead of being rejected - OpenAPI: reusable `pageSize`/`pageNumber` parameters carrying the ceilings, plus a `GET /api/v1/vaults` path documenting the LIMIT_EXCEEDED response; `openapi.json` regenerated - `GET /api/v1/vaults` joins the committed contract snapshots, so the response shape is now covered by the backward-compatibility check - tests spy on the Prisma client to prove no read is ever issued with a `take` above the ceiling, that an oversized limit is rejected before any query runs, and that the committed `openapi.json` still matches
…request The verifier applied one shared tolerance to every time claim, so a token stayed acceptable for 60s past `exp`. A stolen bearer token therefore kept authorising `POST /vault/:id/withdraw` for a full minute after the user logged out, and nothing consulted a revocation list at all on the request path — `/auth/logout` only returned 200 without revoking anything. Time claims (backend/src/auth.ts) - add `assertTimeClaims()`, `TokenExpiredError` and `TokenNotYetValidError`, and an optional `nbf` claim on JwtPayload - `exp` is checked with **zero** tolerance and is inclusive of the expiry second (RFC 7519: a token is invalid at and after `exp`) - only `nbf`/`iat` get `CLOCK_SKEW_TOLERANCE_SECONDS` (5s) of slack, for clock skew between the pod and whatever minted the token Revocation (backend/src/tokenRevocation.ts) - `RevocationStore` gains `revokeWalletBefore()` and `isWalletRevokedBefore()`: a per-wallet high-water mark rather than an id list, so `logout-all` stays O(1) per wallet. The marker never moves backwards, so a later, broader revocation always wins - `revokeAllForWallet()` no longer just deletes records — it recorded nothing, which silently un-revoked every token it touched - Redis store implements the new methods and falls back to its in-process store on error, including for `revokeAllForWallet` (was `return 0`) - the store is now actually wired to Redis when `REDIS_URL` is set, so a logout handled by one pod is visible to the others Request path - `requireAuth` checks the revocation list on every authenticated request and answers 401 `TOKEN_REVOKED`; it fails **closed** with 503 if the store itself is unreachable rather than downgrading to "token is fine" - `/auth/logout` revokes the presented `jti` and, when a refreshToken is supplied, the whole refresh family - `/auth/logout-all` writes the wallet marker and revokes every refresh family for the wallet - OpenAPI: the `bearerAuth` scheme documents the zero-tolerance `exp`, the 5s `nbf` skew and the per-request revocation check Tests: 29 new cases in `jwtExpiryAndRevocation.test.ts`, including a token signed to expire *now* and read 10s later with a mocked `Date.now` (401, not 200), the 59s case the old 60s tolerance allowed, a 5s `nbf` skew that must still pass, logout/logout-all over HTTP, and the Redis-error fallback path.
…comment Two review-level corrections to the revocation path added in the previous commit: - `isAccessTokenRevoked` looked the wallet marker up under the raw `sub` claim. Revocations are written under the canonical (upper-case) address via `normalizeWalletAddress`, so a token whose `sub` happened to be lower-case would not have matched a wallet-wide revocation. Normalise the lookup key the same way the write path does. - requireAuth's comment claimed a `res.headersSent` guard that was never implemented. Replace it with the reasoning that actually matters: Express ignores middleware return values, so returning before `next()` is safe for route usage, and the single direct caller passes a synchronous next().
…s path `transferOrchestrator.test.ts` exercises the happy path end to end, but with no `REDIS_URL` the `redisClientManager` never reports ready, so `IdempotencyStore.redis` returns null and every Redis helper is skipped — the store's Redis branch, and more importantly its error fallback, had no coverage at all. These tests drive the store through a stub client so they reach: the Redis get/set/del round trip, conflict detection across instances, the read/write/delete failure fallbacks, and `pruneStaleKeys` across expired TTL, unparsable payloads and dryRun. Plus the in-process semantics that money-moving code relies on: single execution, replay, fingerprint conflict, in-flight coalescing, and that a rejected operation frees the pending slot for a retry. idempotencyStore.ts: 70.9% -> 94.5% statements, 50.9% -> 81.8% branches.
…d-allows-limit-100000-to-oom-the-api' into fix/1431-auth-middleware-accepts-expired-jwt-for-60s-due-to-clocktolerance-misconfiguration
…034 retry
The rebalance is a read-compute-write over one vault's `Allocation`
rows: read the current allocations, compute target weights, upsert them.
Prisma defaults `$transaction` to ReadCommitted, so a concurrent
rebalance over the same vault could change a row between our read and our
write. The loser then overwrote the winner's weights and
`sum(weight) = 100` was violated — silently, because nothing downstream
checks it.
The invariant was not even representable: `Allocation` had no `weight`
column, and nothing stopped two rows existing for the same
(vaultId, strategyId).
- new `services/allocation.ts`
- the whole read-compute-write runs inside one
`prisma.$transaction(..., { isolationLevel: 'Serializable' })`; every
input to the write is read inside that transaction, so a concurrent
commit cannot land mid-rebalance
- a lost race is a "someone else went first", not a bug, so it is
retried once with exponential backoff and counted in
`rebalance_serialization_retry_total`. Any other error is surfaced
untouched; exhausting the attempts raises a typed
`RebalanceConflictError` carrying the underlying error
- targets are validated against the invariant *before* the transaction
opens, so a malformed request never contends for a write lock
- amounts are derived with Decimal and the rounding residual is given to
the largest-weight target, so the amounts sum to exactly the AUM
- allocations for strategies dropped from the target list are retired,
otherwise their weight would persist and break the invariant
- `getAllocationSummary()` is a read and is deliberately left outside
the transaction, as are the existing `exposureGuardrails.ts` queries
- metrics: `rebalance_serialization_retry_total` (partitioned by attempt)
and `rebalance_total` (by outcome)
- schema: `Allocation.weight`, `@@unique([vaultId, strategyId])` and a
`(vaultId, weight)` index. The migration is additive
(add column with default -> fold duplicate amounts -> remove duplicates ->
backfill weight from current exposure -> add constraints), so no row is
dropped and no existing ADD COLUMN lacks a DEFAULT, keeping it inside
the repo's migration-safety policy
Tests: 28 unit cases (isolation level, retry/metric/typed conflict,
invariant, vault state, amount derivation, read path) plus 7 integration
cases against the real database covering two concurrent rebalances on the
same vault, repeated collisions, cross-vault isolation, and that amounts
sum to AUM. The retry assertion fault-injects P2034 on the first attempt
because SQLite waits on SQLITE_BUSY rather than aborting, so a real
P2034 is not reproducible in CI — that limitation is documented in both
test files.
Contributor
Author
🧭 CI triage — same 12 pre-existing failures as #1515 / #1516, none from this diff
New in this PR, and green
✅ Final local verificationBranch status: |
…defaults-to-readcommitted-allowing-phantom-reads-in-allocation-rebalance
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
2 participants
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.
📋 Description
Goal
The allocation rebalance is a read-compute-write over one vault's
Allocationrows: read the current allocations, compute the target weights, upsert them. Prisma defaults$transactiontoReadCommitted, so a concurrent rebalance over the same vault can change or insert a row between our read and our write. The loser then overwrites the winner's weights, and the per-vault invariantsum(weight) = 100is violated — silently, because nothing downstream checks it.Closes #1433
Worth flagging up front: the invariant was not even representable.
Allocationhad noweightcolumn and no uniqueness constraint on(vaultId, strategyId), so two rows for the same strategy pair could coexist. Both are fixed here.Changes
backend/src/services/allocation.ts(new)prisma.$transaction(..., { isolationLevel: 'Serializable' }). Every input to the write is read inside that transaction — vault AUM,deletedAt, the current allocation set — so a concurrent commit cannot land mid-rebalance.Serializableis also the strongest level Postgres accepts, and it is the only level Prisma exposes for this repo's SQLite datasource, so one setting is correct on both.rebalance_serialization_retry_total. Any other error is surfaced untouched. Exhausting the attempts raises a typedRebalanceConflictErrorcarrying the underlying error.Decimal; the rounding residual goes to the largest-weight target so amounts sum to exactly the AUM.getAllocationSummary()is a read and is deliberately left outside the transaction, as are the existingexposureGuardrails.tsquery paths.backend/src/metrics.tsrebalance_serialization_retry_total(partitioned byattempt) — a sustained rise means rebalances collide often enough to need jitter or coarser locking; a value that never moves means the isolation level is not doing its job.rebalance_total(byoutcome:ok,ok_after_retry,error,conflict).backend/prisma/schema.prisma+ migrationAllocation.weight,@@unique([vaultId, strategyId]),@@index([vaultId, weight]).weightfrom current exposure → add constraints. Prisma's generated version rebuilds the table (DROP TABLE), which the repo's migration-safety linter rejects. This version drops nothing, renames nothing, and everyADD COLUMNcarries aDEFAULT.weightfrom current exposure means the invariant is true for every pre-existing row and the first rebalance after the migration is a no-op relative to reality. A vault whose allocations are all zero getsweight0 and must be rebalanced before it means anything.Deliberately not in this PR
POST /api/v1/vault/strategyis the natural caller, but it is a live money-adjacent endpoint and wiring a new write path into it (with its own authorisation, idempotency and rollback story) is a separate decision from fixing the transaction. The acceptance criteria scope this to the transaction, so this PR is the correctness fix only.🔗 Type of Change
🛡️ Risk Assessment
Risk Level
Blast Radius & Impact Analysis
Detailed Risk & Blast Radius Notes:
🔄 Rollback Plan
Rollback Strategy & Feasibility
Rollback Trigger Criteria
Step-by-Step Rollback Procedure
git revertthe single commit. Nothing calls the service, so reverting removes the behaviour entirely.Allocation.weightin place is the safer half-revert: it is nullable-with-default and harmless, and dropping it would be the one irreversible step.npx prisma migrate deploy && npm run prisma:generateto resync the client.⚡ Performance Impact
Performance & Resource Assessment
Performance & Gas Profiling Summary:
🔒 SECURITY REVIEW
Smart-contract sections do not apply — no Solidity touched.
strategyId, finite, non-negative, sum within tolerance of 100).NaNandInfinityweights are rejected explicitly, andNumber.isFiniteguards the coercion.sum(weight) = 100check is a typed error surfaced to the caller, not a log line. A caller that wants a different invariant must change this code deliberately.Option A: Fixed in This PR ✅
📝 Testing
Functional Testing
Security Testing
weightsum 99.99 (rejected), 100 ± tolerance/2 (accepted), duplicatestrategyId, empty list,-10,NaN,+Infinity, zero-AUM vault, unknown vault, soft-deleted vault,REBALANCE_MAX_ATTEMPTSoverrideTest Coverage
allocationRebalance.test.ts— 28 unit cases (Prisma double)Against the issue's acceptance criteria:
$transactionreceives a second argument equal to{ isolationLevel: 'Serializable' }, that the argument exists at all (so it cannot silently default to ReadCommitted), and that every attempt re-opens at Serializable.P2034, second succeeds; assertsattempts === 2,retried === true, andrebalance_serialization_retry_total === 1. Also: exhausting attempts throwsRebalanceConflictErrorafter exactly 2 attempts,REBALANCE_MAX_ATTEMPTS=3gives 3 attempts and 2 retries, the underlying error is carried, and a non-serialization error (P2002) is not retried and not counted.isSerializationFailurerecognisesP2034,P2028,SQLITE_BUSY/database is locked, and rejectsP2002, connection resets,nulland non-objects.sum(weight) = 100reported and persisted; stale allocations retired; nothing deleted when nothing is stale; upsert targets the unique key; over/under-allocation rejected without opening a transaction; negative/NaN/Infinity rejected; missingstrategyIdrejected.getAllocationSummary()runs no transaction and no extrafindMany.allocationRebalanceConcurrency.test.ts— 7 integration cases (real SQLite database)rows === distinct(no phantom row), whichever caller won.P2034fault-injected into the first attempt only → exactly one returnsretried: true,$transactionwas called 3 times for 2 operations,rebalance_serialization_retry_total === 1, and the invariant still holds.Documented limitation: SQLite takes a database-wide write lock and Prisma waits on
SQLITE_BUSYrather than aborting, so a naturally occurringP2034is not reproducible in CI (verified empirically). The retry assertion therefore injects the failure at the transaction boundary while keeping the reads and writes real. This is stated in the docblock of both test files so nobody later mistakes it for a real race.🚀 Deployment Notes
Mainnet Readiness
Environment variables added (all optional, with defaults)
REBALANCE_MAX_ATTEMPTS2REBALANCE_BASE_BACKOFF_MS25base * 2^(N-1), capped atREBALANCE_MAX_BACKOFF_MS(500)Known issues inherited from
main(out of scope, flagged for maintainers)These fail identically on
mainand are unrelated to this issue — full triage table is on PR #1515:npm testcoverage gate —jest.config.jsrequires 80% global coverage;mainsits at 37.11%, this branch at ~68%. Closing that gap is a repo-wide programme.npm run prisma:schema-check—ScopedAdminTokenindex mismatch; the zero-diff fix needs aDROP INDEX, whichcheck-migrations.jsclassifies as an error, so the two gates contradict each other. (My migration is clean here:prisma migrate diffreports no Allocation difference.)npm audit/ missingpnpm-lock.yaml/ CodeQL (Rust, TypeScript) — dependency and infrastructure issues on untouched paths.✅ Reviewer Checklist
POST /api/v1/vault/strategyis the next step, deliberately left out of this PR📞 Questions or Issues?
docs/SECURITY_CHECKLIST.mddocs/FALSE_POSITIVE_HANDLING.md@security-teamin comments