perf: tenant cache, single-flight cache reads, and pool env config - #1198
Open
realcodesiman wants to merge 5 commits into
Open
realcodesiman wants to merge 5 commits into
realcodesiman wants to merge 5 commits into
Conversation
| try { | ||
| return await sourcePromise | ||
| } finally { | ||
| if (inFlightCacheMisses.get(cacheKey) === sourcePromise) { |
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.
Implements P0/P1 items A3, A4, A5, B11 (env-toggle) from the system-design-review audit.
A3 — tenant status cache (refined scope)
The audit assumed both the user→tenantId lookup and the tenant-status lookup were uncached. In reality
QuotaEnforcementService.resolveContextalready cached the tenantId lookup (60s TTL). The real uncached extra round-trip wastenantService.findById(tenantId)(fetchesstatus/ownerIdon every pooled-quota check) — wrapped it withwithCache(30s TTL,tenants:<id>tag), invalidated on every tenant mutation (upsertByOwner,upsertById,setStatusByOwner).Deliberately NOT implemented: caching
WorkspaceMemberService.findMembership/isMember. That code carries an explicit comment that membership revocation must take effect immediately — caching it, even for 5-10s, is a real security regression. Investigated whetherresolveWorkspaceAccessduplicates the membership lookup already done by the caller — it doesn't (it only re-fetches the workspace whenrealMember.workspaceisn't already attached, which it always is on the real member path), so there was nothing to memoize there either.A4 — single-flight for
withCachepackages/redis/src/cache-utils.ts'swithCachehad no stampede protection — N concurrent callers on a cold key all missed and all hit the DB. Added in-process promise de-duplication (aMap<string, Promise<T>>, cleaned up via try/finally). Deliberately in-process only, not a cross-process Redis lock (that needs its own failure semantics for comparatively little extra benefit here).A5 — connection pool config
packages/database/src/client.tshardcodedpgPooloptions. Added env-configurableDATABASE_POOL_MAX/_MIN,DATABASE_STATEMENT_TIMEOUT_MS,DATABASE_IDLE_IN_TRANSACTION_TIMEOUT_MSwith defaults matching today's behavior (max: 10, no timeouts). Added a short PgBouncer recommendation note referencing the existingSET LOCALcompatibility comment.B11 — read-replica env toggle
packages/database/src/sharding/message/connection-manager.tshardcodedREAD_REPLICAS_ENABLED = false. Replaced with an env-driven default (SHARD_READ_REPLICAS_ENABLED), still defaulting tofalse— no behavior change today, just no longer a code change to opt in. No replica infrastructure exists in this environment to test failover against; that remains a staging-only follow-up per the audit's own recommendation.Verification
pnpm --filter @chatbotx.io/database check-types && test,pnpm --filter @chatbotx.io/redis check-types && test,pnpm --filter @chatbotx.io/business check-types && test,pnpm --filter builder check-types— all green