Skip to content

perf: thread pre-fetched token to checkRateLimit avoiding re-query - #6930

Merged
diegosouzapw merged 2 commits into
diegosouzapw:release/v3.8.47from
oyi77:fix/relay-thread-token
Jul 12, 2026
Merged

diegosouzapw merged 2 commits into
diegosouzapw:release/v3.8.47from
oyi77:fix/relay-thread-token

Conversation

@oyi77

@oyi77 oyi77 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Problem: getRelayTokenByHash fetches full relay token row. A few lines later checkRateLimit(token.id) does a second SELECT * FROM relay_tokens on a different predicate.

Change:

  • checkRateLimit accepts optional existingToken param; skips re-query when provided
  • Both relay routes pass the already-fetched token
  • Uses RelayToken (camelCase) instead of RelayTokenRow (snake_case) uniformly

Verification: all relay unit tests pass (only pre-existing Cloudflare deploy failures).

@oyi77
oyi77 requested a review from diegosouzapw as a code owner July 11, 2026 23:24
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for the perf fix — threading the already-fetched RelayToken into checkRateLimit to skip the redundant SELECT * FROM relay_tokens WHERE id = ? is a real, correct improvement. I verified it behaviorally: the existingToken fast-path and the legacy re-query path return identical allowed/remaining, and the per-minute cap is still enforced correctly (ran a probe unit test against real SQLite, both cases green).

A few things need to happen before this can merge as-is:

  1. Scope: this branch also carries an unrelated commit adding 46 provider SVG icons (b6ae243) plus 3 more unrelated "drift" commits (file-size-baseline reformat, a stryker.conf.json test registration, and a codex client-version test bump). None of these are part of the "perf: thread pre-fetched token" change. Could you split them into their own PR(s)? It'll make review and the git history much cleaner, and it'll also unblock this fix faster.

  2. Possible unintended regression in the icon commit: b6ae243 removes cohere: "Cohere" from LOBE_PROVIDER_ALIASES in src/shared/components/lobeProviderIcons.ts — that's not mentioned in the commit message (which only lists additions) and looks accidental. Could you confirm whether that removal was intentional?

  3. Merge conflicts: config/quality/file-size-baseline.json and tests/unit/provider-models-route-codex.test.ts currently conflict against release/v3.8.47 (that branch has moved since this PR was opened) — a rebase will be needed regardless of the split above.

  4. Tests: checkRateLimit in src/lib/db/relayProxies.ts currently has zero test coverage in the repo. Since this PR changes its signature and internal field-mapping (snake_case → camelCase when a token is passed), could you add a unit test asserting the existing-token fast-path agrees with the legacy re-query path and still enforces the rate-limit windows? Happy to share the quick probe test I used for verification if useful.

  5. A changelog.d fragment for this fix would also be appreciated per repo convention.

Once split down to just the relay fix + a regression test + changelog fragment, this looks mergeable.

oyi77 and others added 2 commits July 12, 2026 10:25
getRelayTokenByHash already fetches the full RelayToken row. A few
lines later checkRateLimit(token.id) does a second SELECT * FROM
relay_tokens on a different predicate (id instead of token_hash).

Change:
- checkRateLimit accepts an optional existingToken parameter; when
  provided, skips the re-query entirely.
- Both relay routes (chat completions + bifrost) pass the already-
  fetched token.
- The function now uses RelayToken (camelCase) instead of RelayTokenRow
  (snake_case) when the token is passed in.

PR-URL: fix-relay-thread-token
…st-path

Adds node:test coverage for src/lib/db/relayProxies.ts::checkRateLimit
proving the existingToken fast-path (pre-fetched RelayToken threaded in,
no re-query) agrees with the legacy re-query path (no token passed),
and that the per-minute cap is still enforced through the fast-path.
Also adds a changelog.d fragment for the perf fix in 9d4cd90.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
@diegosouzapw
diegosouzapw force-pushed the fix/relay-thread-token branch from 55449c3 to aed01e9 Compare July 12, 2026 13:29
@diegosouzapw
diegosouzapw merged commit 66cb93f into diegosouzapw:release/v3.8.47 Jul 12, 2026
3 checks passed
@diegosouzapw

Copy link
Copy Markdown
Owner

Merged — reconstructed to just the checkRateLimit pre-fetched-token perf fix (dropped the duplicate 46-icon commit + drift), with a new regression test (3/3, +28/28 related green) + a changelog fragment. Thanks @oyi77! (Remaining red checks are pre-existing release-tip base-reds tracked in #6967.)

@oyi77
oyi77 deleted the fix/relay-thread-token branch July 20, 2026 15:24
HouMinXi pushed a commit to HouMinXi/OmniRoute that referenced this pull request Aug 2, 2026
…iegosouzapw#6930)

* perf: thread pre-fetched token to checkRateLimit avoiding re-query

getRelayTokenByHash already fetches the full RelayToken row. A few
lines later checkRateLimit(token.id) does a second SELECT * FROM
relay_tokens on a different predicate (id instead of token_hash).

Change:
- checkRateLimit accepts an optional existingToken parameter; when
  provided, skips the re-query entirely.
- Both relay routes (chat completions + bifrost) pass the already-
  fetched token.
- The function now uses RelayToken (camelCase) instead of RelayTokenRow
  (snake_case) when the token is passed in.

PR-URL: fix-relay-thread-token

* test(db): add regression coverage for checkRateLimit existingToken fast-path

Adds node:test coverage for src/lib/db/relayProxies.ts::checkRateLimit
proving the existingToken fast-path (pre-fetched RelayToken threaded in,
no re-query) agrees with the legacy re-query path (no token passed),
and that the per-minute cap is still enforced through the fast-path.
Also adds a changelog.d fragment for the perf fix in 9d4cd90.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>

---------

Co-authored-by: oyi77 <oyi77@users.noreply.github.com>
Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…iegosouzapw#6930)

* perf: thread pre-fetched token to checkRateLimit avoiding re-query

getRelayTokenByHash already fetches the full RelayToken row. A few
lines later checkRateLimit(token.id) does a second SELECT * FROM
relay_tokens on a different predicate (id instead of token_hash).

Change:
- checkRateLimit accepts an optional existingToken parameter; when
  provided, skips the re-query entirely.
- Both relay routes (chat completions + bifrost) pass the already-
  fetched token.
- The function now uses RelayToken (camelCase) instead of RelayTokenRow
  (snake_case) when the token is passed in.

PR-URL: fix-relay-thread-token

* test(db): add regression coverage for checkRateLimit existingToken fast-path

Adds node:test coverage for src/lib/db/relayProxies.ts::checkRateLimit
proving the existingToken fast-path (pre-fetched RelayToken threaded in,
no re-query) agrees with the legacy re-query path (no token passed),
and that the per-minute cap is still enforced through the fast-path.
Also adds a changelog.d fragment for the perf fix in 9d4cd90.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>

---------

Co-authored-by: oyi77 <oyi77@users.noreply.github.com>
Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants