Repository navigation
♻️ feat: Share Pull Request Lookups Across Replicas Through Redis - #16988
Conversation
27ab9bf to
bc806b4
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc806b4846
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const lifetime = lifetimeOf(result); | ||
| const entry: Entry = { result, expiresAt: now() + lifetime }; | ||
| remember(scope, key, entry, capacity, credentials); | ||
| if (shared != null) share(shared, storeKey, entry, lifetime, sharedCacheTimeoutMs); |
There was a problem hiding this comment.
When sharedCache is enabled and a credential looks up more than cacheMaxEntries distinct repository/branch combinations within the TTL, every result is written to a separate Redis key even though cacheMaxEntries and cacheMaxCredentials only constrain the process-local remember map. Thus the documented/default cache bounds no longer bound the actual cache: a busy or adversarial deployment can accumulate an arbitrary number of Redis entries until expiration. Apply equivalent eviction/accounting to the shared store, or introduce an explicit bounded shared-cache policy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4f6d8d3: the shared store is now a fixed set of slots (cacheMaxEntries x cacheMaxCredentials results, cacheMaxCredentials cooldowns), each value carries its key digest so another key in the slot is a miss; covered by "holds no more results than every partition together" and "holds one rate limit cooldown per credential slot".
| timer.unref?.(); | ||
| }); | ||
| try { | ||
| return await Promise.race([call(), timeout]); |
There was a problem hiding this comment.
When Redis is disconnected or stalls and the shared cache is enabled, this race only stops awaiting the operation; it does not cancel the underlying get or set. Because the Redis client enables its offline queue by default, each local cache miss can therefore leave multiple commands and their promises retained after sharedCacheTimeoutMs, eventually consuming memory and producing a large command burst when Redis recovers. Make the store calls abortable or configure an actual command deadline so timed-out operations are removed rather than merely ignored.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4f6d8d3: each GET/SET carries sharedCacheTimeoutMs as the node-redis command timeout, which removes an unsent command from the offline queue and rejects it; "drops a command queued while Redis is unreachable at its deadline" drives a real client at a closed port.
2a5fba5 to
f39c98c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f39c98c5d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function createPullRequestLookupStore(): PullRequestLookupStore | undefined { | ||
| if ( | ||
| !cacheConfig.USE_REDIS || | ||
| cacheConfig.FORCED_IN_MEMORY_CACHE_NAMESPACES?.includes(CacheKeys.PULL_REQUESTS) |
There was a problem hiding this comment.
This new packages/api factory reads the module-global cacheConfig and constructs standardCache directly, so tests and alternate shared-store implementations cannot supply the deployment configuration or backend without editing this module. Accept the cache configuration and factory/client as caller-supplied dependencies, while continuing to pass the resulting PullRequestLookupStore interface into the lookup.
AGENTS.md reference: AGENTS.md:L94-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4039ff4: createPullRequestLookupStore takes the Redis client and cache config from the caller, and convos.js passes keyvRedisClient and cacheConfig; the spec injects a fake client and config.
| sharedCache, | ||
| sharedCacheTimeoutMs = DEFAULT_SHARED_TIMEOUT_MS, |
There was a problem hiding this comment.
When two principals use the same token and lookup tuple but resolve different sharedCache or sharedCacheTimeoutMs settings, the second call joins the first call's inflight promise because neither new field participates in its key. The first caller's captured settings then govern both: a caller with sharing disabled can still trigger Redis access, while a caller configured with a 10 ms store deadline can wait for another caller's 5-second deadline on the header route. Include these operational settings in in-flight coalescing or enforce each caller's toggle and deadline independently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4039ff4: the in-flight key now includes the sharing toggle and deadline; covered by "does not join a lookup running under another sharing policy".
| if (isEntry(stored) && stored.expiresAt > now()) { | ||
| /** Never kept longer than this replica would have kept its own answer. */ | ||
| const expiresAt = Math.min(stored.expiresAt, now() + lifetimeOf(stored.result)); | ||
| remember(scope, key, { result: stored.result, expiresAt }, capacity, credentials); | ||
| return stored.result; |
There was a problem hiding this comment.
If one replica enters a local credential cooldown while another concurrent replica successfully writes a result for a different branch, requests to the cooled-down replica return RATE_LIMITED before reaching this shared-result read. This contradicts the existing cached-result behavior and can hide a valid Redis-cached pull request for the entire cooldown, even though this block already deliberately prefers a stored result over the shared cooldown. Consult the shared result before the earlier local-cooldown return when sharing is enabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4039ff4: with sharing on, the store is read before a local cooldown answers; covered by "serves a shared answer while this replica is cooling down".
f39c98c to
4039ff4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4039ff4107
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sharedCache: settings.sharedCache === true, | ||
| sharedCacheTimeoutMs: settings.sharedCacheTimeoutMs ?? 250, |
There was a problem hiding this comment.
When concurrent batch requests use the same token and lane but resolve different sharedCache or sharedCacheTimeoutMs settings, createConversationPullRequestsHandler gives them the same shareKey because that key omits both newly added fields. The second request therefore joins the first request's promise before createPullRequestLookup can apply its policy-aware flight key, so the first principal's toggle and timeout control both requests—for example, an enabled caller can skip Redis by joining a disabled caller. Fresh evidence after the inner lookup fix is the earlier coalescing layer in handler.ts, whose shareKey includes the answer policy but not these two fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in be5b06e: the batch share key now includes cacheMaxEntries, cacheMaxCredentials, sharedCache and sharedCacheTimeoutMs; covered by handler.spec 'does not join a lookup started with another %s' (sharing setting, shared cache deadline, cache capacity). The commits cited above for this PR are now 3dcfc32 and 1905b1c after a restack.
4039ff4 to
be5b06e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be5b06e5d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| share( | ||
| shared, | ||
| cooldownKey, | ||
| { key: cooldownDigest, until: end }, | ||
| wait, |
There was a problem hiding this comment.
When two replicas concurrently receive rate limits for the same credential and host, both writes target this cooldownKey, but the later Redis SET wins even if it carries a shorter retryAfterMs. For example, a 10-second fallback written after a one-hour reset hint erases the longer deadline and expires the shared key after 10 seconds, allowing other replicas to resume provider requests far too early. Update the cooldown atomically only when its until value extends the stored deadline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0edf402: the store writes through one Lua script that keeps the stored value when it belongs to the same credential and runs out no earlier (order = cooldown end), so a shorter later limit never ends a longer one; the local cooldown also only extends. Covered by the integration spec 'keeps the later value for one owner whatever order the writes arrive in' against Redis and lookup.spec 'writes a rate limit with its end, so a shorter one written later cannot end it'.
| const storeKey = slotOf('result', resultDigest, capacity * credentials); | ||
| const cooldownKey = slotOf('cooldown', cooldownDigest, credentials); |
There was a problem hiding this comment.
When principals sharing a token resolve different cacheMaxEntries or cacheMaxCredentials values, the same result or cooldown digest is reduced modulo a different slot count. The replicas therefore read and write different Redis keys for identical lookups, so shared answers and rate-limit cooldowns silently stop propagating between those callers. Derive slot addresses from a stable deployment-wide slot count rather than the current request's capacity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0edf402: slot addresses now come from sharedCacheSlots, a deployment-wide setting read from the YAML only (BASE_ONLY_CONFIG_PATHS), not from a caller's cache capacity; covered by 'finds a shared answer whatever cache capacity each caller has' and the resolution spec.
| if (shared != null) { | ||
| share(shared, storeKey, { ...entry, key: resultDigest }, lifetime, sharedCacheTimeoutMs); |
There was a problem hiding this comment.
When source.find fails because of a replica-local condition such as its network, DNS, proxy, or request timeout, the resulting UPSTREAM_ERROR is written to Redis here just like a successful answer. Healthy replicas then accept that cached failure and return 503 without trying their own working upstream path for up to the failure TTL; a faulty replica receiving continued traffic can repeatedly poison lookups for the rest of the deployment. Share successful results and provider-wide cooldowns, but keep generic operational failures local.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0edf402: only found answers (a pull request or none) and provider rate limits are shared; any failed lookup stays with the replica that saw it, and a stored failure from an older replica is ignored. Covered by 'never shares %s, so another replica tries the provider itself' (upstream error, source failure, unconfigured source).
…ore a Local Cooldown
0edf402 to
9e5951b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e5951b176
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /** Only when this caller's settings ask for it; the store is there whenever Redis is. */ | ||
| const shared = sharedCache === true ? store : undefined; |
There was a problem hiding this comment.
When this replica has cached an UPSTREAM_ERROR and another replica subsequently succeeds for the same lookup and writes that answer to Redis, the unconditional local-cache return above this shared-store selection keeps returning the local 503 until FAILURE_TTL_MS expires. This defeats the reason operational failures are kept replica-local: a healthy replica's shared answer cannot repair the failing replica during that window. Return local successes immediately, but consult the shared store before returning a cached failure when sharing is enabled.
Useful? React with 👍 / 👎.
Pull Request
Summary
Each API replica caches pull request lookups and rate-limit cooldowns in its own memory, so with several replicas every one asks the provider separately and a 429 seen by one does not stop the others, which burns the token's hourly quota much faster than one replica would. With
pullRequests.sharedCache: trueon a deployment that runs Redis, results and cooldowns are also written to and read from Redis, behind the existing in-memory cache. It is off by default, so nothing changes until it is turned on.Store keys hold a digest of the credential, never the token, and the result key is a fixed-length hash. Every store call is bounded (
sharedCacheTimeoutMs, 250 ms by default), and a slow, failing or malformed store reads as a miss rather than failing the lookup. A stored entry is validated before use, including an https page URL and a known provider, and can never outlive what the reading replica would have kept itself.Part of a stack on #16982, merge from the bottom up: host (1), GitLab (2), Bitbucket (3), shared cache (4), user OAuth backend (5), connected account setting (6). Each PR is based on the one below it, so its diff is only its own link.
Type of change
Testing
Tested environments/configuration:
Automated tests:
packages/api:npx jest src/pulls src/cache/pullRequests.spec.tspasses: two replicas share an answer and a cooldown, no token in keys or values, rejecting and hanging stores, malformed entries ignored, and the setting off never touching the store.packages/apibuild and the convos route specs pass.Screenshots / recordings
No user-facing change
Risk / compatibility
Off by default. Only takes effect with
USE_REDIS; thepullRequestscache namespace can be forced in memory like any other.Checklist
pullRequestsfields are documented inlibrechat.example.yaml