Uh oh!
There was an error while loading. Please reload this page.
perf: reduce /api/tools server time on the read path - #9
Conversation
`tools.list` ran the stale-connection-tools sync on every call: an unconditional read of revised integrations plus a connection scan, even when nothing was stale (the steady state, since config_revised_at stays set after the first revision). Cache, per binding, the highest revision watermark at which everything was confirmed synced, keyed by the DB handle so the entry survives per-request executor rebuilds in a prod isolate and stays isolated across tests. The watermark is read fresh each call, so any new revision busts the cache and lazy convergence stays correct; a failed rebuild is left uncached so it retries. The openapi operation store filtered listOperations in memory after reading the whole collection. Push the integration filter to the storage layer via key-prefix `starts with` clauses (covering both the v2 hashed prefix and the legacy plaintext prefix), keeping the data.integration guard as the source of truth.
amondnet
commented
Jun 25, 2026
/gemini review |
amondnet
commented
Jun 25, 2026
@cubic-dev-ai review |
@amondnet I have started the AI code review. It will take a few minutes to complete. |
amondnet
commented
Jun 25, 2026
@greptileai review |
There was a problem hiding this comment.
Code Review
This pull request optimizes the performance of the tools.list read path and the OpenAPI operation store. It introduces a keyPrefixes option to the plugin storage layer, allowing listOperations to filter by integration at the storage layer (using both v2 hashed and legacy plaintext prefixes) rather than filtering in memory. Additionally, the stale-connection-tools sync now skips scanning connections once they are synced at the current revision watermark. Comprehensive tests have been added to verify these behaviors. There are no active review comments remaining, and we have no further feedback to provide.
amondnet
commented
Jun 25, 2026
@coderabbitai review |
✅ Action performedReview finished.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughPluginStorage 리스트 입력이 여러 접두사를 지원하도록 바뀌고, OpenAPI operation 조회는 integration별 prefix로 스토리지 범위를 좁히며, executor는 revision watermark 기반 stale-sync 캐시와 재시도 동작을 추가했다. ChangesAPI 도구 읽기 경로 변경
Sequence Diagram(s)sequenceDiagram
participant toolsList
participant syncStaleConnectionTools
participant pluginStorage
participant resolveTools
toolsList->>syncStaleConnectionTools: 현재 revision watermark 확인
syncStaleConnectionTools->>pluginStorage: keyPrefixes로 stale 연결 스캔
alt watermark unchanged
syncStaleConnectionTools-->>toolsList: 스캔 건너뜀
else watermark changed
syncStaleConnectionTools->>resolveTools: stale 연결 재빌드
alt 재빌드 실패
resolveTools-->>syncStaleConnectionTools: 실패
syncStaleConnectionTools-->>toolsList: 다음 읽기에서 재시도
else 재빌드 성공
resolveTools-->>syncStaleConnectionTools: 성공
syncStaleConnectionTools-->>toolsList: watermark 캐시 저장
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR reduces
Confidence Score: 5/5Safe to merge: both optimizations are additive read-path changes with comprehensive tests and correct fallback behavior on failure. The watermark cache uses a NUL-byte key separator (confirmed in code at line 2501), a WeakMap for natural GC scoping, and leaves failures uncached so the next read always retries. The LIKE prefix scan retains a JS post-filter as the authoritative guard, and the base-36 hash output contains no LIKE wildcard characters. Test coverage spans cache skip, cache bust on revision, retry-on-failure, and cross-subject isolation. No existing behavior is removed; the only behavioral change is skipping a redundant connection-table scan in the steady state. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[tools.list called] --> B[syncStaleConnectionTools]
B --> C[Read revised integrations\nconfig_revised_at IS NOT NULL]
C --> D{revised.length == 0?}
D -- yes --> E[Return early: nothing to sync]
D -- no --> F[Compute watermark = max config_revised_at]
F --> G{staleSyncCache.get\ncacheKey == watermark?}
G -- yes: cache hit --> H[Skip connection scan]
G -- no: cache miss --> I[Read connections for revised integrations]
I --> J[For each connection: syncedAt < revisedTime?]
J -- no: already synced --> K[continue]
J -- yes: stale --> L[produceConnectionTools]
L -- success --> M[allClean stays true]
L -- failure --> N[allClean = false]
K --> O{all done?}
M --> O
N --> O
O -- allClean=true --> P[staleSyncCache.set\ncacheKey, watermark]
O -- allClean=false --> Q[Leave uncached: retry next read]
H --> R[Read tool rows and return]
P --> R
Q --> R
subgraph openapi["OpenAPI listOperations"]
S[listOperations integration] --> T[pluginStorage.list with keyPrefixes:\nop.hash. + integration.]
T --> U[DB: LIKE 'op.hash.%' OR LIKE 'integration.%']
U --> V[JS post-filter: data.integration === integration]
V --> W[Return filtered operations]
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[tools.list called] --> B[syncStaleConnectionTools]
B --> C[Read revised integrations\nconfig_revised_at IS NOT NULL]
C --> D{revised.length == 0?}
D -- yes --> E[Return early: nothing to sync]
D -- no --> F[Compute watermark = max config_revised_at]
F --> G{staleSyncCache.get\ncacheKey == watermark?}
G -- yes: cache hit --> H[Skip connection scan]
G -- no: cache miss --> I[Read connections for revised integrations]
I --> J[For each connection: syncedAt < revisedTime?]
J -- no: already synced --> K[continue]
J -- yes: stale --> L[produceConnectionTools]
L -- success --> M[allClean stays true]
L -- failure --> N[allClean = false]
K --> O{all done?}
M --> O
N --> O
O -- allClean=true --> P[staleSyncCache.set\ncacheKey, watermark]
O -- allClean=false --> Q[Leave uncached: retry next read]
H --> R[Read tool rows and return]
P --> R
Q --> R
subgraph openapi["OpenAPI listOperations"]
S[listOperations integration] --> T[pluginStorage.list with keyPrefixes:\nop.hash. + integration.]
T --> U[DB: LIKE 'op.hash.%' OR LIKE 'integration.%']
U --> V[JS post-filter: data.integration === integration]
V --> W[Return filtered operations]
end
Reviews (4): Last reviewed commit: "test(sdk): scope the per-subject cache t..." | Re-trigger Greptile |
Uh oh!
There was an error while loading. Please reload this page.
Greptile SummaryThis PR optimizes the
Confidence Score: 4/5The change is a focused read-path optimization with no mutations to the write path; the watermark cache logic is correct and well-tested, and the storage-layer prefix filtering preserves a JS-level correctness guard. Both findings are theoretical: the cache-key space-separator collision requires tenant or subject IDs to contain spaces (unlikely given slug validation elsewhere), and the inner-Map growth only matters in long-lived processes handling very large numbers of distinct bindings. The new test suite covers cache-hit, cache-bust, and failure-retry paths against real SQLite. No correctness regressions were found. packages/core/sdk/src/executor.ts — the watermark cache construction (key format and Map lifetime) is the only area that warrants a second look before merging to a high-tenant production environment. Important Files Changed
|
Uh oh!
There was an error while loading. Please reload this page.
From code review of #4: - Add a regression test proving the stale-sync watermark cache is keyed per subject: two subjects sharing one DB handle must each scan independently, so one binding converging cannot make another skip and serve a stale catalog. - Write the cache-key separator as an explicit unicode escape instead of an invisible literal NUL byte (which read as a space and misled the reviewer), and note why. No behavior change; the separator was and stays NUL.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/plugins/openapi/src/sdk/store.test.ts (1)
81-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win인메모리 facade가
LIKE와일드카드 과매칭을 재현하지 않아data.integration가드가 테스트되지 않습니다.이 facade는 JS
startsWith로만 필터링하므로 실제 스토리지 레이어의LIKE 'prefix%'(_/%를 와일드카드로 취급)에서 발생하는 과매칭을 흉내내지 못합니다.store.ts가 "source of truth"라고 명시한rowToOperation(row)?.integration === integration가드를 실제로 검증하는 경로가 없어, 향후 그 가드가 제거되어도 테스트가 통과합니다. 통합 슬러그에_가 포함된 두 통합(예:a_b,axb)을 시드해 가드가 교차 통합 행을 걸러내는지 확인하는 케이스를 추가하는 것을 고려해 보세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/plugins/openapi/src/sdk/store.test.ts` around lines 81 - 90, The in-memory storage facade in store.test.ts is only using startsWith, so it does not reproduce the real LIKE prefix overmatching behavior and leaves the rowToOperation(row)?.integration === integration guard untested. Update the test setup around list to include a case where integration slugs contain wildcard-relevant characters such as an underscore, seed two integrations like a_b and axb with overlapping prefixes, and verify that the store.ts filtering path still excludes cross-integration rows. Use the existing list/input/prefixesOf and rowToOperation symbols to add an assertion that the integration guard is what prevents the incorrect match.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/plugins/openapi/src/sdk/store.test.ts`:
- Around line 81-90: The in-memory storage facade in store.test.ts is only using
startsWith, so it does not reproduce the real LIKE prefix overmatching behavior
and leaves the rowToOperation(row)?.integration === integration guard untested.
Update the test setup around list to include a case where integration slugs
contain wildcard-relevant characters such as an underscore, seed two
integrations like a_b and axb with overlapping prefixes, and verify that the
store.ts filtering path still excludes cross-integration rows. Use the existing
list/input/prefixesOf and rowToOperation symbols to add an assertion that the
integration guard is what prevents the incorrect match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b1efcba1-b7c7-43fa-800a-45d3fd84286e
📒 Files selected for processing (7)
.changeset/reduce-api-tools-server-time.mdpackages/core/sdk/src/executor.test.tspackages/core/sdk/src/executor.tspackages/core/sdk/src/plugin-storage.test.tspackages/core/sdk/src/plugin-storage.tspackages/plugins/openapi/src/sdk/store.test.tspackages/plugins/openapi/src/sdk/store.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
chatbot-pf/reference-please(manual)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/sdk/src/executor.ts (1)
2507-2512: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift워터마크를 벽시계 밀리초 값으로만 비교하면 같은 ms의 새 revision을 놓칠 수 있습니다.
Line 2512에서 캐시된
watermark와 숫자 동등성만 비교하므로,config_revised_at = now.getTime()으로 찍힌 두 config 변경이 같은 millisecond에 발생하면 새 변경이 있어도 connection scan을 건너뛰고 stale tool catalog를 계속 반환할 수 있습니다. revision은 벽시계가 아니라 단조 증가하는 DB revision/counter로 만들거나, 최소한 기존 row 값보다 반드시 크게 bump되도록 보장해 주세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/sdk/src/executor.ts` around lines 2507 - 2512, The staleSyncCache watermark check in executor.ts is using wall-clock milliseconds from config_revised_at, so equal-ms updates can be skipped and leave stale tool catalog data. Update the watermark logic around the revised.reduce and staleSyncCache.get comparison to use a monotonically increasing DB revision/counter, or otherwise guarantee each new config revision is strictly greater than the previous one before comparing and returning early.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/sdk/src/executor.test.ts`:
- Around line 688-698: The cleanup in executor.test is not guaranteed if an
assertion or earlier effect fails, so the executor and SQLite handle may remain
open. Wrap the block around a.tools.list(), b.tools.list(), and the
counts.connection assertion in a try/finally or use Effect.ensuring so that
a.close(), b.close(), and Effect.promise(() => real.close()) always run. Use the
existing a, b, and real handles in executor.test to place the cleanup in the
guaranteed finalization path.
---
Outside diff comments:
In `@packages/core/sdk/src/executor.ts`:
- Around line 2507-2512: The staleSyncCache watermark check in executor.ts is
using wall-clock milliseconds from config_revised_at, so equal-ms updates can be
skipped and leave stale tool catalog data. Update the watermark logic around the
revised.reduce and staleSyncCache.get comparison to use a monotonically
increasing DB revision/counter, or otherwise guarantee each new config revision
is strictly greater than the previous one before comparing and returning early.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5178cf35-ee69-441d-a4f8-b6e091285802
📒 Files selected for processing (3)
.please/docs/review/rejected-findings.jsonlpackages/core/sdk/src/executor.test.tspackages/core/sdk/src/executor.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
chatbot-pf/reference-please(manual)
✅ Files skipped from review due to trivial changes (1)
- .please/docs/review/rejected-findings.jsonl
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Auto-approved: Performance optimizations on the read path: caching the stale-connection sync watermark and narrowing storage queries by key prefix. Both are backward-compatible, well-tested, and carry low risk.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Performance optimization adding caching and storage-layer filtering across core SDK and openapi plugin; requires human review to verify correctness and edge cases.
Re-trigger cubic
Uh oh!
There was an error while loading. Please reload this page.
Summary
Sub-task of #3 (Fix A): cut
/api/toolsserver time, the measured long pole on the web UI.Both issues called out in #4 were verified against the code first, then fixed TDD-style.
1. Skip the stale-sync scan on reads (
packages/core/sdk)tools.listransyncStaleConnectionToolsunconditionally: a read of revised integrations plus a connection scan, on every call. Becauseconfig_revised_atstays set after the first config change, the steady state was two full reads that rebuilt nothing.Now the read of revised integrations doubles as a revision watermark. Once every connection is confirmed synced at the current watermark, that is cached per binding and the connection scan is skipped until a new revision moves the watermark:
WeakMapkeyed by the underlying DB handle, then by tenant+subject. In production every per-request scoped executor in an isolate shares one long-lived handle, so the cache survives executor rebuilds and the saving lands; each test builds a fresh DB object, so entries are naturally isolated (no cross-test contamination) and GC'd with the handle.Net: steady-state
tools.listdrops from two table reads to one.2. Filter
listOperationsat the storage layer (packages/plugins/openapi)listOperationsread the wholeoperationcollection and filtered by integration in memory. Even passing a key prefix did not reach the DB (the facade applied it in JS after a full scan).keyPrefixesto the plugin-storage list input; the facade now pusheskey starts withclauses to the storage layer (LIKE 'prefix%').op.<hash>.) and the legacy plaintext prefix (<slug>.), keeping thedata.integrationJS guard as the source of truth, so un-migrated legacy rows are never dropped andLIKEwildcard over-match is harmless.Tests
listwithkeyPrefixesover real SQLite; openapi storelistOperations(per-integration, legacy-safe, prefix-narrowing);syncStaleConnectionToolsskip / cache-bust / retry, driven by a per-table query counter.updateSpec propagates to OTHER subjects' personal connections) exercises the new cache +starts withpath on real SQLite and stays green.packages/core/sdk352/352,packages/plugins/openapi151/151.format:check,lint, and typecheck clean.Closes#4
Summary by CodeRabbit
keyPrefix외에keyPrefixes를 지원해 여러 접두사로 더 정확히 조회합니다.