Conversation
…oof framework, tech debt - Complete audit of every MongoDB call in cloud server (11 collections, 262 call sites) - 18 hot-path calls identified (session connect, message handling, location updates) - Only 18% of reads use .lean() — 82% instantiate full Mongoose Documents unnecessarily - France event loop blocked 162s/1800s (9%) by MongoDB RTT alone - GC confirmed NOT primary cause (54ms probes, 0MB freed) - Proof framework: 4 steps to definitively confirm/deny MongoDB as crash cause - Tech debt doc: 8 items found (app cache, atomic updates, lean, N+1, JWT centralization) - Key finding: latency is pure network RTT, not query execution (indexes work, 0ms on server)
…mory app cache
Three changes:
B1. Event loop gap detector — 1s setInterval that detects >2s gaps, logs the
blocking duration. The missing link between 'query was slow' and 'health
check failed because the event loop was blocked at that exact moment.'
B2. Cumulative MongoDB blocking metric — mongoQueryCount, mongoTotalBlockingMs,
mongoMaxQueryMs in 30s system vitals. Extends existing slow-query plugin.
B3. In-memory app cache — loads all 1,314 apps (~2MB) at boot, refreshes
every 5 min with write-through invalidation. Eliminates 18 hot-path DB
round-trips (80-370ms each) per session. Largest single-change impact
on crash frequency.
Migration: 9 hot-path call sites switch from App.findOne() to appCache.
Cold paths left as-is.
…plica guide, B4 operation timing - Detailed every write path (16 across 5 files) that must call invalidate() - Documented every staleness edge case: publicUrl, hashedApiKey, permissions are critical — bounded to 30s max staleness (down from 5 min) - Added refresh failure detection (warn if 3 missed cycles) - Multi-pod staleness: write-through only works on local pod, other regions wait for 30s timer — this is the tradeoff vs 9% event loop blocking - Atlas read replica step-by-step: UI steps, readPreference=nearest, cost (~$228/mo for 4 replicas). Future work, not in scope for this spec. - B4: wire operationTimers to audio processing, glasses/app message handlers, display rendering. Measures actual synchronous CPU budget consumption.
…tion timing
B1. Event Loop Gap Detector (SystemVitalsLogger)
1s setInterval detects >2s gaps — catches ALL blocking regardless of cause.
Logs 'event-loop-gap' with gap duration, RSS, session count.
The missing link between 'query was slow' and 'health check failed.'
B2. Cumulative MongoDB Blocking Metric (mongodb.connection + SystemVitalsLogger)
MongoQueryStats accumulator records all slow queries (>MONGOOSE_SLOW_QUERY_MS).
SystemVitalsLogger reads/resets every 30s: mongoQueryCount, mongoTotalBlockingMs,
mongoMaxQueryMs. Shows exact % of event loop time consumed by MongoDB.
B3. In-Memory App Cache (app-cache.service.ts + 9 hot-path migrations)
Loads all 1,314 apps (~2MB) at boot. Refreshes every 30s.
9 hot-path call sites migrated from App.findOne() to appCache.getByPackageName()
with DB fallback. 16 write paths call appCache.invalidate() fire-and-forget.
Eliminates 80-370ms RTT per hot-path app lookup.
Files: AppManager, SubscriptionManager, app-message-handler, sdk.auth.service,
system-app.api (5 call sites). Invalidation in: console.apps.service, app.service,
developer.service, developer.routes, permissions.routes, admin.routes.
B4. Hot Path Operation Timing (UdpAudioServer, bun-websocket, DisplayManager)
Wires existing operationTimers framework to: audioProcessing (3,250 ops/sec),
glassesMessage, appMessage, displayRendering. Vitals now show op_*_ms and
opBudgetUsedPct — exact % of event loop consumed by application code.
Also: .gitignore for .heap/
📋 PR Review Helper📱 Mobile App Build⏳ Waiting for build... 🕶️ ASG Client Build⏳ Waiting for build... 🔀 Test Locallygh pr checkout 2333 |
📝 WalkthroughWalkthroughAdds an in-memory AppCache with cache-first reads and DB fallback, widespread fire-and-forget cache invalidation after app mutations, cumulative MongoDB slow-query stats, a 1s event-loop gap detector, and operation-timing instrumentation across several hot-path handlers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Cache as AppCache
participant DB as MongoDB
participant Vitals as SystemVitals
rect rgba(100, 200, 100, 0.5)
Note over Client,Cache: Cache-first read flow
Client->>Cache: getByPackageName(pkg)
alt cache hit
Cache-->>Client: cached app (lean)
else cache miss
Cache->>DB: find(...).lean()
DB-->>Cache: lean app(s)
Cache-->>Client: lean app(s)
end
end
rect rgba(100, 150, 200, 0.5)
Note over Client,Cache: Write + invalidation
Client->>DB: findOneAndUpdate(...) / save()
DB-->>Client: success
Client->>Cache: invalidate() (fire-and-forget)
Cache->>DB: find().lean() (refresh)
DB-->>Cache: all apps
Cache->>Cache: atomic swap Map/array
end
rect rgba(200, 150, 100, 0.5)
Note over Vitals,DB: Telemetry collection & gap detection
DB->>Vitals: slow-query post-hook records duration -> mongoQueryStats.record()
Vitals->>Vitals: periodic gap detector tick (1s) detects event-loop gaps
Vitals->>Vitals: mongoQueryStats.getAndReset() -> emit mongoQuery* fields
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 62d5353b5a
ℹ️ 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".
| // Fetch apps from cache with DB fallback | ||
| const cachedApps = appCache.getByPackageNames(installedPackageNames); | ||
| const installedApps = ( | ||
| cachedApps.length > 0 ? cachedApps : await App.find({ packageName: { $in: installedPackageNames } }).lean() |
There was a problem hiding this comment.
This branch treats any non-empty cache hit as complete, so a partial cache miss (for example, when one installed app is new/stale in cache but another is present) skips the DB query and returns an incomplete app set. In that case users can temporarily lose installed apps/tools from responses even though the DB has them. The fallback should trigger when cached results do not cover all requested package names, not only when the cache is empty.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Valid — fixed in 73be58b. Changed cachedApps.length > 0 to cachedApps.length === names.length so partial hits fall back to DB.
Comment on lines
668
to
669
| } catch (error) { | ||
| logger.error({ error, userId, packageName }, "Error processing app message"); |
There was a problem hiding this comment.
The error path now only logs and keeps the app WebSocket open; previously it closed with code 1011. If userSession.handleAppMessage throws, the connection stays alive in a bad state and can keep sending messages that repeatedly fail, producing noisy retries/logs instead of forcing a clean reconnect. Restoring socket close in this catch path preserves the existing recovery behavior.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Valid — regression. Fixed in 73be58b. Restored ws.close(1011, "Internal server error") in the catch block.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
cloud/.gitignore (1)
1-22: Optional cleanup: Remove duplicate entries.The file contains several duplicate patterns that could be cleaned up for maintainability:
- Lines 15-17 duplicate lines 10-12 (
.env.development,.env.production,.env.staging)- Line 19 (
.logs) duplicates line 5 (.logs/)- Lines 13-14 use an unusual
./prefix pattern (./node_modules,./env) that is atypical for.gitignorefilesThese duplicates don't break functionality but removing them would improve clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cloud/.gitignore` around lines 1 - 22, Remove duplicate ignore patterns and normalize prefixes: delete the repeated .env.development, .env.production, .env.staging entries and the duplicated .logs entry, and replace atypical entries like ./node_modules and ./env with the conventional node_modules/ and env/ (or node_modules and env) patterns; keep one canonical listing for each directory/file (e.g., .env, .env.local, .env.* entries once, node_modules/, env/, .logs/).cloud/packages/cloud/src/services/sdk/sdk.auth.service.ts (1)
86-97: TheinvalidateCachefunction exists but callers need to use it after API key regeneration.This service exports
invalidateCache(packageName?)for clearing stale entries, but as noted in theconsole.apps.service.tsandapp.service.tsreviews, theregenerateApiKeyimplementations don't call this function, leaving stale hashes in the SDK auth cache.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cloud/packages/cloud/src/services/sdk/sdk.auth.service.ts` around lines 86 - 97, The SDK auth cache invalidation function invalidateCache(packageName?) is exported but not invoked after API key rotation; update the regenerateApiKey implementations in console.apps.service (regenerateApiKey) and app.service (regenerateApiKey) to call invalidateCache with the affected package name (or call invalidateCache() to clear all entries if package-scoped name is unavailable) immediately after successfully updating the API key so the stale hash is removed from the cache and subsequent auth uses the new key.cloud/packages/cloud/src/services/core/app-cache.service.ts (1)
21-24: Use the shared logger in this new cache service.Please verify the shared logger exposes the same
.child()API and switch this file to it so logging configuration stays centralized as more hot paths depend on the cache.As per coding guidelines,
cloud/**/*.{ts,tsx}: Use logger from@mentra/utilspackage for logging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cloud/packages/cloud/src/services/core/app-cache.service.ts` around lines 21 - 24, Replace the local rootLogger import with the shared logger from `@mentra/utils` and use its .child({ service: "AppCache" }) to create the AppCache logger; specifically, change the import that currently brings in rootLogger from "../logging/pino-logger" to import the shared logger (verify it exposes a .child() method) and instantiate const logger = sharedLogger.child({ service: "AppCache" }) so the AppCache service (logger variable used in this file) uses the centralized logging configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cloud/issues/062-mongodb-latency/spec.md`:
- Around line 164-177: The document currently contradicts itself by defining
REFRESH_INTERVAL_MS as 5 * 60 * 1000 (5 minutes) while another part of the same
section (B3 / the rollout/testing guidance) says 30 seconds; pick one staleness
interval and make both places consistent: update the REFRESH_INTERVAL_MS
constant and any textual references in the B3 section (and the other occurrence
that mentions "30 seconds") to the chosen value so the code constant and all
descriptive text match.
In `@cloud/packages/cloud/src/services/console/console.apps.service.ts`:
- Around line 397-400: After saving the new hashed key on appDoc and calling
appCache.invalidate(), also invalidate the SDK auth cache for that app by
calling sdk.auth.service.invalidateCache(packageName) (or the equivalent method
on the SDK auth instance) so the internal cache (const cache = new Map<string,
CacheEntry>() inside sdk.auth.service) is updated; locate the regeneration flow
around appDoc.hashedApiKey = hashed / await appDoc.save() and add a
fire-and-forget call to invalidateCache(packageName) (ensuring you reference the
correct sdk.auth.service instance) immediately after appCache.invalidate().
In `@cloud/packages/cloud/src/services/core/app-cache.service.ts`:
- Around line 44-50: AppCacheService can suffer from race conditions where an
older scheduled refresh overwrites a newer post-write invalidate; fix by
serializing or versioning refreshes: increment a monotonic token (e.g.,
reuse/rename refreshCount) at the start of every refresh/invalidate operation,
capture that token in the async refresh routine (the method that loads and swaps
cached data, e.g., refreshAll/refresh), and only perform the final swap of
byPackageName/allApps/lastRefresh if the captured token still equals the current
token; alternatively wrap refresh/invalidate logic with a simple mutex so only
one swap can succeed—update methods invalidate() and the scheduled refresh
handler to follow this token/lock pattern so older snapshots cannot win.
- Around line 56-84: initialize() currently awaits refresh() and will reject on
a transient first-load error, preventing the setInterval from being registered;
instead, register refreshInterval (using setInterval with REFRESH_INTERVAL_MS)
immediately and invoke this.refresh() without awaiting (or await it but catch
errors) so initial failures are logged and do not stop the interval; ensure the
same error handling used inside the interval (logger.error with the error, do
not flip loaded/lastRefresh as if successful) and keep references to refresh(),
refreshInterval, loaded, lastRefresh, STALE_WARNING_MS, REFRESH_INTERVAL_MS, and
byPackageName so the cache will recover on subsequent refresh attempts.
In `@cloud/packages/cloud/src/services/core/app.service.ts`:
- Around line 811-816: regenerateApiKey updates the DB and calls
appCache.invalidate() but fails to clear the SDK auth service's hashed-key
cache, leaving a stale entry in sdk.auth; after the App.findOneAndUpdate and
alongside the existing appCache.invalidate() call, invoke the SDK auth cache
invalidation function (e.g., sdk.auth.service.invalidateCache(packageName) or
sdk.auth.invalidateCache(packageName) depending on the module export) to remove
the old hashed API key for this package before returning apiKey.
In `@cloud/packages/cloud/src/services/core/developer.service.ts`:
- Line 162: The appCache.invalidate() call is currently invoked fire-and-forget
causing a stale-cache window after writes; change the two call sites in the
developer service (in the createApp and regenerateApiKey flows) to await
appCache.invalidate() so the method finishes before returning —
appCache.invalidate() already logs errors, so awaiting it preserves current
error behavior while preventing a same-pod stale read window.
- Line 98: Remove secret material from the debug log: the call to _logger.debug
that currently includes hashedKey and apiKey should be changed to log only
non-sensitive context (e.g., packageName and/or a non-reversible identifier), or
redact/truncate the hash (do not log the raw apiKey at all). Locate the
_logger.debug usage referencing hashedKey, apiKey and packageName in
developer.service.ts (the debug line) and update it to omit apiKey and hashedKey
(or replace hashedKey with a fixed label like "<redacted>" or a short,
non-reversible prefix), ensuring no secret-valued variables are emitted to logs.
In `@cloud/packages/cloud/src/services/websocket/bun-websocket.ts`:
- Around line 668-672: The catch block in handleAppMessage currently logs errors
but does not close the WebSocket; add a ws.close(1011, "Internal server error")
call inside the catch that handles unexpected exceptions from
userSession.handleAppMessage() so the connection is terminated on server-side
failures; ensure you still call logger.error({ error, userId, packageName },
"Error processing app message") before closing and preserve the finally behavior
(operationTimers.addTiming("appMessage", ...)) so timing is recorded.
---
Nitpick comments:
In `@cloud/.gitignore`:
- Around line 1-22: Remove duplicate ignore patterns and normalize prefixes:
delete the repeated .env.development, .env.production, .env.staging entries and
the duplicated .logs entry, and replace atypical entries like ./node_modules and
./env with the conventional node_modules/ and env/ (or node_modules and env)
patterns; keep one canonical listing for each directory/file (e.g., .env,
.env.local, .env.* entries once, node_modules/, env/, .logs/).
In `@cloud/packages/cloud/src/services/core/app-cache.service.ts`:
- Around line 21-24: Replace the local rootLogger import with the shared logger
from `@mentra/utils` and use its .child({ service: "AppCache" }) to create the
AppCache logger; specifically, change the import that currently brings in
rootLogger from "../logging/pino-logger" to import the shared logger (verify it
exposes a .child() method) and instantiate const logger = sharedLogger.child({
service: "AppCache" }) so the AppCache service (logger variable used in this
file) uses the centralized logging configuration.
In `@cloud/packages/cloud/src/services/sdk/sdk.auth.service.ts`:
- Around line 86-97: The SDK auth cache invalidation function
invalidateCache(packageName?) is exported but not invoked after API key
rotation; update the regenerateApiKey implementations in console.apps.service
(regenerateApiKey) and app.service (regenerateApiKey) to call invalidateCache
with the affected package name (or call invalidateCache() to clear all entries
if package-scoped name is unavailable) immediately after successfully updating
the API key so the stale hash is removed from the cache and subsequent auth uses
the new key.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d7afeb2-bfa4-4c4d-876d-f48f06ec0970
📒 Files selected for processing (22)
cloud/.gitignorecloud/issues/062-mongodb-latency/spec.mdcloud/issues/062-mongodb-latency/spike.mdcloud/issues/062-mongodb-latency/tech-debt.mdcloud/packages/cloud/src/api/hono/routes/admin.routes.tscloud/packages/cloud/src/api/hono/routes/developer.routes.tscloud/packages/cloud/src/api/hono/routes/permissions.routes.tscloud/packages/cloud/src/api/hono/sdk/system-app/system-app.api.tscloud/packages/cloud/src/connections/mongodb.connection.tscloud/packages/cloud/src/index.tscloud/packages/cloud/src/services/console/console.apps.service.tscloud/packages/cloud/src/services/core/app-cache.service.tscloud/packages/cloud/src/services/core/app.service.tscloud/packages/cloud/src/services/core/developer.service.tscloud/packages/cloud/src/services/layout/DisplayManager6.1.tscloud/packages/cloud/src/services/metrics/SystemVitalsLogger.tscloud/packages/cloud/src/services/sdk/sdk.auth.service.tscloud/packages/cloud/src/services/session/AppManager.tscloud/packages/cloud/src/services/session/SubscriptionManager.tscloud/packages/cloud/src/services/session/handlers/app-message-handler.tscloud/packages/cloud/src/services/udp/UdpAudioServer.tscloud/packages/cloud/src/services/websocket/bun-websocket.ts
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
cloud/packages/cloud/src/services/metrics/SystemVitalsLogger.ts (1)
248-251: Name these as slow-query counters.These values are populated only from the slow-query hook, so
mongoQueryCount/mongoTotalBlockingMs/mongoMaxQueryMsread broader than the underlying data. Renaming them tomongoSlowQuery*will keep dashboards and alerts honest.Suggested change
- mongoQueryCount: mongoStats.count, - mongoTotalBlockingMs: Math.round(mongoStats.totalMs), - mongoMaxQueryMs: Math.round(mongoStats.maxMs * 10) / 10, + mongoSlowQueryCount: mongoStats.count, + mongoSlowQueryTotalMs: Math.round(mongoStats.totalMs), + mongoSlowQueryMaxMs: Math.round(mongoStats.maxMs * 10) / 10,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cloud/packages/cloud/src/services/metrics/SystemVitalsLogger.ts` around lines 248 - 251, The metrics are misnamed — mongoQueryCount, mongoTotalBlockingMs, and mongoMaxQueryMs are only populated from the slow-query hook; rename them to mongoSlowQueryCount, mongoSlowQueryTotalMs, and mongoSlowQueryMaxMs (and update any references that read or write these fields, e.g., where mongoStats is used and where the metrics object is constructed in SystemVitalsLogger) so dashboards/alerts reflect that these are slow-query counters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cloud/issues/062-mongodb-latency/spec.md`:
- Line 177: The spec has a contradiction between the REFRESH_INTERVAL_MS
constant and the prose that says "30 seconds" — choose the intended refresh
interval and make both the constant and any textual references consistent;
update the REFRESH_INTERVAL_MS value (and any occurrences of that symbol) to the
chosen duration in milliseconds or change the prose that mentions "30 seconds"
to match the 5-minute setting so implementation and rollout checks reference the
same interval.
In `@cloud/packages/cloud/src/api/hono/sdk/system-app/system-app.api.ts`:
- Around line 143-147: appCache.getByPackageNames drops misses so the current
check "cachedApps.length > 0 ? cachedApps : await App.find(...)" can return an
incomplete set when some package names are missing; change the logic in the
block using appCache.getByPackageNames, installedPackageNames, and installedApps
to compute which package names were not returned by the cache, fetch only those
from the DB via App.find({ packageName: { $in: missingNames } }).lean(), then
merge the DB results with the cached results into a single installedApps array
(ensuring you include all requested package names and avoid duplicates); apply
the same missing-names + DB-fallback pattern to getUserTools as well.
In `@cloud/packages/cloud/src/services/core/app-cache.service.ts`:
- Around line 56-74: The refreshes can run concurrently and an older/slow
refresh may overwrite a newer cache update; make refresh application monotonic
by introducing a serial token/counter (e.g., this.refreshSerial) that you
increment before starting any refresh (whether from initialize interval,
invalidate(), or manual calls in initialize()), capture the token in the
refresh() closure, and only apply the fetched results and update
this.lastRefresh/this.loaded when the captured token equals the latest token (or
is >= lastAppliedToken) so stale completions are ignored; update references in
initialize(), invalidate(), and any other callers of refresh() to use this
token-based guard around applying cache state.
In `@cloud/packages/cloud/src/services/core/app.service.ts`:
- Around line 772-774: The delete/rotate flow currently calls
App.findOneAndDelete and only appCache.invalidate, which leaves entries in the
SDK in-memory auth cache active; import the invalidateCache function from
sdk.auth.service (e.g., invalidateCache or exported alias) and call it whenever
you invalidate appCache (both in the App.findOneAndDelete path and the
rotate-credentials path where appCache.invalidate is called) so the SDK auth
in-memory cache is cleared in addition to appCache.invalidate.
In `@cloud/packages/cloud/src/services/core/developer.service.ts`:
- Line 98: The debug call is leaking secret material by logging apiKey and
hashedKey; update the _logger.debug invocation in the API key validation logic
(the call that currently passes hashedKey, apiKey and `Validating API key for
${packageName}`) to stop sending apiKey and hashedKey to logs—only log
non-sensitive context such as packageName and/or a boolean like "hasApiKey:
true" or a masked indicator (e.g., first 4 chars followed by ****) if you must
show a token fragment; remove hashedKey and apiKey from the structured payload
and keep the message concise (e.g., _logger.debug({ packageName, hasApiKey: true
}, `Validating API key for ${packageName}`)).
- Around line 221-223: The update currently writes the new hashed key with
App.findOneAndUpdate but fires appCache.invalidate() without awaiting it, which
can leave stale cache entries and cause immediate key/JWT rejection; modify the
code that calls App.findOneAndUpdate and appCache.invalidate to await the cache
invalidation (e.g., await appCache.invalidate(...)) so the cache is refreshed
before returning the rotated key, ensuring the cache invalidation is awaited and
that appCache.invalidate returns a Promise (or wrap it in a Promise) so callers
like the SDK auth check see the new hashedApiKey.
In `@cloud/packages/cloud/src/services/sdk/sdk.auth.service.ts`:
- Around line 102-107: The fetchHashedKeyFromDb function must not read hashed
credentials from the general appCache; remove the fallback that uses
appCache.getByPackageName and instead always load the app record directly from
MongoDB (via App.findOne(...).select("packageName hashedApiKey").lean()) or a
dedicated auth cache with synchronous invalidation; update fetchHashedKeyFromDb
to only rely on the DB query result for the hashedApiKey and eliminate any use
of appCache.getByPackageName to avoid validating against stale key material.
In `@cloud/packages/cloud/src/services/session/AppManager.ts`:
- Around line 613-620: The current logic in AppManager.ts uses
appCache.getByPackageNames(runningAppsPackageNames) and treats any non-empty
cachedApps as a full hit, which can miss package names and allow duplicate
foreground apps; update the logic so you only skip Mongo when the cache returned
a result for every requested package (i.e., all runningAppsPackageNames are
present), otherwise query the DB for the missing packages or call App.find(...)
for the full set; ensure runningForegroundApps still filters by a.appType ===
AppType.STANDARD and reference appCache.getByPackageNames,
runningAppsPackageNames, App.find, runningForegroundApps when making the change.
In `@cloud/packages/cloud/src/services/udp/UdpAudioServer.ts`:
- Around line 260-265: The audioProcessing timing is only recorded on the happy
path and is skipped if an exception occurs; declare a start timestamp (t0)
before iterating packetsToProcess, move the
operationTimers.addTiming("audioProcessing", ...) into a finally block so it
always runs, and ensure the finally uses the same t0 to compute duration; update
the try/catch around session.audioManager.processAudioData (and reference
packetsToProcess, session.audioManager.processAudioData,
operationTimers.addTiming) so exceptions are still rethrown or handled but
timing is recorded regardless.
---
Nitpick comments:
In `@cloud/packages/cloud/src/services/metrics/SystemVitalsLogger.ts`:
- Around line 248-251: The metrics are misnamed — mongoQueryCount,
mongoTotalBlockingMs, and mongoMaxQueryMs are only populated from the slow-query
hook; rename them to mongoSlowQueryCount, mongoSlowQueryTotalMs, and
mongoSlowQueryMaxMs (and update any references that read or write these fields,
e.g., where mongoStats is used and where the metrics object is constructed in
SystemVitalsLogger) so dashboards/alerts reflect that these are slow-query
counters.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 934d7b8e-538b-4193-afc5-b7d2bf92a709
📒 Files selected for processing (23)
.github/workflows/porter-debug.ymlcloud/.gitignorecloud/issues/062-mongodb-latency/spec.mdcloud/issues/062-mongodb-latency/spike.mdcloud/issues/062-mongodb-latency/tech-debt.mdcloud/packages/cloud/src/api/hono/routes/admin.routes.tscloud/packages/cloud/src/api/hono/routes/developer.routes.tscloud/packages/cloud/src/api/hono/routes/permissions.routes.tscloud/packages/cloud/src/api/hono/sdk/system-app/system-app.api.tscloud/packages/cloud/src/connections/mongodb.connection.tscloud/packages/cloud/src/index.tscloud/packages/cloud/src/services/console/console.apps.service.tscloud/packages/cloud/src/services/core/app-cache.service.tscloud/packages/cloud/src/services/core/app.service.tscloud/packages/cloud/src/services/core/developer.service.tscloud/packages/cloud/src/services/layout/DisplayManager6.1.tscloud/packages/cloud/src/services/metrics/SystemVitalsLogger.tscloud/packages/cloud/src/services/sdk/sdk.auth.service.tscloud/packages/cloud/src/services/session/AppManager.tscloud/packages/cloud/src/services/session/SubscriptionManager.tscloud/packages/cloud/src/services/session/handlers/app-message-handler.tscloud/packages/cloud/src/services/udp/UdpAudioServer.tscloud/packages/cloud/src/services/websocket/bun-websocket.ts
Comment on lines
772
to
+774
| await App.findOneAndDelete({ packageName }); | ||
|
|
||
| appCache.invalidate(); // fire-and-forget — no await |
There was a problem hiding this comment.
Invalidate the SDK auth cache when deleting or rotating credentials.
cloud/packages/cloud/src/services/sdk/sdk.auth.service.ts short-circuits out of its own in-memory cache before consulting MongoDB or appCache. Only invalidating appCache here means a deleted app or rotated key can keep authenticating until that auth-cache entry expires.
Suggested change
import { invalidateCache as invalidateSdkAuthCache } from '../sdk/sdk.auth.service'; await App.findOneAndDelete({ packageName });
+invalidateSdkAuthCache(packageName);
appCache.invalidate();
@@
await App.findOneAndUpdate({ packageName }, { $set: { hashedApiKey } });
+invalidateSdkAuthCache(packageName);
appCache.invalidate();Also applies to: 812-814
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cloud/packages/cloud/src/services/core/app.service.ts` around lines 772 -
774, The delete/rotate flow currently calls App.findOneAndDelete and only
appCache.invalidate, which leaves entries in the SDK in-memory auth cache
active; import the invalidateCache function from sdk.auth.service (e.g.,
invalidateCache or exported alias) and call it whenever you invalidate appCache
(both in the App.findOneAndDelete path and the rotate-credentials path where
appCache.invalidate is called) so the SDK auth in-memory cache is cleared in
addition to appCache.invalidate.
Contributor
Author
There was a problem hiding this comment.
Valid and critical. Fixed in 73be58b. SDK auth now always validates credentials against DB, not the app cache. clearSdkAuthCache() exported and called from invalidate().
There was a problem hiding this comment.
Rate Limit Exceeded
@isaiahb have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 11 seconds before sending another message.
Comment on lines
221
to
+223
| // Update app with new hashed API key | ||
| await App.findOneAndUpdate({ packageName }, { $set: { hashedApiKey } }); | ||
| appCache.invalidate(); // fire-and-forget — no await |
There was a problem hiding this comment.
Await the cache refresh before returning a rotated key.
cloud/packages/cloud/src/services/sdk/sdk.auth.service.ts:101-109 checks appCache before MongoDB. Returning here before invalidate() finishes leaves the old hashedApiKey in cache, so the brand-new key/JWT can be rejected immediately on the same pod.
🛠️ Suggested fix
// Update app with new hashed API key
await App.findOneAndUpdate({ packageName }, { $set: { hashedApiKey } });
- appCache.invalidate(); // fire-and-forget — no await
+ await appCache.invalidate();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Update app with new hashed API key | |
| await App.findOneAndUpdate({ packageName }, { $set: { hashedApiKey } }); | |
| appCache.invalidate(); // fire-and-forget — no await | |
| // Update app with new hashed API key | |
| await App.findOneAndUpdate({ packageName }, { $set: { hashedApiKey } }); | |
| await appCache.invalidate(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cloud/packages/cloud/src/services/core/developer.service.ts` around lines 221
- 223, The update currently writes the new hashed key with App.findOneAndUpdate
but fires appCache.invalidate() without awaiting it, which can leave stale cache
entries and cause immediate key/JWT rejection; modify the code that calls
App.findOneAndUpdate and appCache.invalidate to await the cache invalidation
(e.g., await appCache.invalidate(...)) so the cache is refreshed before
returning the rotated key, ensuring the cache invalidation is awaited and that
appCache.invalidate returns a Promise (or wrap it in a Promise) so callers like
the SDK auth check see the new hashedApiKey.
Contributor
Author
There was a problem hiding this comment.
SDK auth now goes directly to DB for credential validation — cache staleness no longer affects key rotation. See fix in 73be58b.
There was a problem hiding this comment.
Rate Limit Exceeded
@isaiahb have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 10 seconds before sending another message.
…robustness CRITICAL: - SDK auth: revert to DB-only credential validation — never use stale cache for hashedApiKey. Security: stale cache = revoked keys still validate. - SDK auth: export clearSdkAuthCache(), called from appCache.invalidate() so key rotation clears both caches. - developer.service: stop logging API key material (raw key + hash were in debug logs — credential leak to all log sinks). MAJOR: - bun-websocket: restore ws.close(1011) in app message error handler (accidentally removed when adding finally block — regression). - system-app.api + AppManager: partial cache hits now fall back to DB. Changed cachedApps.length > 0 to cachedApps.length === names.length. - app-cache: concurrent refresh protection (refreshing flag prevents overlapping refresh() calls from racing). - app-cache: start interval before initial refresh() so transient first-load failures don't permanently disable the cache. - UdpAudioServer: move audioProcessing timing to finally block. MINOR: - spec.md: fix 5-min vs 30-sec inconsistency in B3 description and code.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cloud/packages/cloud/src/services/core/app-cache.service.ts (2)
97-101: Therefreshingmutex prevents race conditions but silently skips refreshes.When
invalidate()is called during an in-progress refresh, the call returns immediately without waiting. This means a write-through invalidation can be "lost" if it arrives during a scheduled refresh. The post-write state might not be visible until the next scheduled refresh (30s later).For most use cases this is acceptable given the documented 30s staleness window. However, if precise write-through semantics are needed on the local pod, consider returning a promise that resolves when the in-flight refresh completes instead of skipping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cloud/packages/cloud/src/services/core/app-cache.service.ts` around lines 97 - 101, The refresh() method currently uses the boolean mutex this.refreshing to skip concurrent refreshes, causing invalidate() calls during an in-flight refresh to return immediately and lose write-through semantics; change refresh() to create and store a promise (e.g., this.refreshPromise) when a refresh starts, set this.refreshing = true, and clear both this.refreshPromise and this.refreshing in the finally block so callers can await completion; update invalidate() to, when this.refreshing is true, return await this.refreshPromise (or return the promise) instead of returning immediately so invalidations wait for the in-flight refresh to finish; reference the existing methods and properties refresh(), invalidate(), this.refreshing (and add this.refreshPromise) in app-cache.service.ts and ensure proper typing/cleanup.
155-158: Edge case:getByPackageNames([])returns[]even when cache is not loaded.When
packageNamesis empty and!this.loaded, the method returns[]. Callers usingcachedApps.length === packageNames.lengthwill see0 === 0astrueand skip the DB fallback, which is correct. However, for non-empty inputs when the cache isn't loaded, callers will get a partial result (empty array) and the length check will correctly trigger the fallback.The current behavior is acceptable given the fallback pattern at call sites, but consider logging a warning when
!this.loadedandpackageNames.length > 0to help debug cache initialization race conditions.Optional: Add warning for non-empty requests when cache not loaded
getByPackageNames(packageNames: string[]): any[] { - if (!this.loaded) return []; + if (!this.loaded) { + if (packageNames.length > 0) { + logger.warn({ feature: "app-cache", count: packageNames.length }, "App cache not loaded yet — caller should fall back to DB"); + } + return []; + } return packageNames.map((name) => this.byPackageName.get(name)).filter((app): app is any => app != null); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cloud/packages/cloud/src/services/core/app-cache.service.ts` around lines 155 - 158, The method getByPackageNames currently returns [] immediately when this.loaded is false; add a conditional that, when !this.loaded and packageNames.length > 0, emits a warning (via whatever logger the service uses) indicating a cache-not-loaded race for the requested packageNames, then continue to return []; update getByPackageNames to reference this.loaded and packageNames.length so the warning only fires for non-empty requests and include identifying context (e.g., packageNames or length) in the log to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cloud/packages/cloud/src/services/core/app-cache.service.ts`:
- Around line 97-101: The refresh() method currently uses the boolean mutex
this.refreshing to skip concurrent refreshes, causing invalidate() calls during
an in-flight refresh to return immediately and lose write-through semantics;
change refresh() to create and store a promise (e.g., this.refreshPromise) when
a refresh starts, set this.refreshing = true, and clear both this.refreshPromise
and this.refreshing in the finally block so callers can await completion; update
invalidate() to, when this.refreshing is true, return await this.refreshPromise
(or return the promise) instead of returning immediately so invalidations wait
for the in-flight refresh to finish; reference the existing methods and
properties refresh(), invalidate(), this.refreshing (and add
this.refreshPromise) in app-cache.service.ts and ensure proper typing/cleanup.
- Around line 155-158: The method getByPackageNames currently returns []
immediately when this.loaded is false; add a conditional that, when !this.loaded
and packageNames.length > 0, emits a warning (via whatever logger the service
uses) indicating a cache-not-loaded race for the requested packageNames, then
continue to return []; update getByPackageNames to reference this.loaded and
packageNames.length so the warning only fires for non-empty requests and include
identifying context (e.g., packageNames or length) in the log to aid debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 130fc8ab-46ac-416e-9c92-fa45a64b253d
📒 Files selected for processing (8)
cloud/issues/062-mongodb-latency/spec.mdcloud/packages/cloud/src/api/hono/sdk/system-app/system-app.api.tscloud/packages/cloud/src/services/core/app-cache.service.tscloud/packages/cloud/src/services/core/developer.service.tscloud/packages/cloud/src/services/sdk/sdk.auth.service.tscloud/packages/cloud/src/services/session/AppManager.tscloud/packages/cloud/src/services/udp/UdpAudioServer.tscloud/packages/cloud/src/services/websocket/bun-websocket.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- cloud/packages/cloud/src/services/websocket/bun-websocket.ts
- cloud/packages/cloud/src/services/sdk/sdk.auth.service.ts
Two App.updateOne() calls for onboardingStatus were missing cache invalidation. Found during manual review — bots missed these.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cloud/packages/cloud/src/api/hono/routes/onboarding.routes.ts (1)
72-72: Consider package-scoped or coalesced invalidation for onboarding writes.
appCache.invalidate()triggers a full app-cache refresh; calling it on onboarding-status mutations can create avoidable refresh fan-out under higher onboarding traffic. Consider a package-scoped invalidation API (or short debounce/coalescing) to keep consistency with lower DB pressure.Based on learnings:
appCache.invalidate()is intentionally called fire-and-forget at write sites, and a worst-case ~30s stale window is accepted to avoid blocking HTTP responses.Also applies to: 104-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cloud/packages/cloud/src/api/hono/routes/onboarding.routes.ts` at line 72, The full-app invalidation call appCache.invalidate() in the onboarding write handler causes unnecessary refresh fan-out; update the onboarding mutation handlers (where appCache.invalidate() is called) to use a package-scoped or coalesced invalidation API instead — e.g., replace appCache.invalidate() with a scoped call like appCache.invalidatePackage('onboarding') or appCache.invalidate({scope: 'onboarding'}), and if that API is not present add a lightweight coalescing wrapper (debounce/short TTL) around invalidations so multiple onboarding writes within ~30s collapse into a single refresh; ensure the change is applied to the onboarding route handlers referenced by onboarding.routes.ts (both occurrences) and preserve the fire-and-forget behavior for response latency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cloud/packages/cloud/src/api/hono/routes/onboarding.routes.ts`:
- Line 72: The full-app invalidation call appCache.invalidate() in the
onboarding write handler causes unnecessary refresh fan-out; update the
onboarding mutation handlers (where appCache.invalidate() is called) to use a
package-scoped or coalesced invalidation API instead — e.g., replace
appCache.invalidate() with a scoped call like
appCache.invalidatePackage('onboarding') or appCache.invalidate({scope:
'onboarding'}), and if that API is not present add a lightweight coalescing
wrapper (debounce/short TTL) around invalidations so multiple onboarding writes
within ~30s collapse into a single refresh; ensure the change is applied to the
onboarding route handlers referenced by onboarding.routes.ts (both occurrences)
and preserve the fire-and-forget behavior for response latency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20b227ba-2e8a-4702-83f1-a795f85ec182
📒 Files selected for processing (1)
cloud/packages/cloud/src/api/hono/routes/onboarding.routes.ts
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
1 participant
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.
Summary by CodeRabbit
New Features
Performance Improvements
Documentation