Skip to content

perf: cache allowed origins set instead of rebuilding per request#486

Open
hobostay wants to merge 1 commit intonexu-io:mainfrom
hobostay:fix/cache-allowed-origins
Open

perf: cache allowed origins set instead of rebuilding per request#486
hobostay wants to merge 1 commit intonexu-io:mainfrom
hobostay:fix/cache-allowed-origins

Conversation

@hobostay
Copy link
Copy Markdown
Contributor

@hobostay hobostay commented May 5, 2026

Summary

  • Caches the Set of allowed origins after first build instead of creating a new Set on every API request

Problem

buildAllowedOrigins() was called inside the /api middleware on every single request, creating a new Set with ~12-18 entries each time. The allowed origins depend only on resolvedPort and OD_WEB_PORT, which don't change at runtime.

Test plan

  • API requests still enforce origin checks correctly
  • No behavioral change — the cached set contains the same entries

🤖 Generated with Claude Code

buildAllowedOrigins() was called on every API request, creating a new
Set each time. Since the allowed origins only depend on resolvedPort
and OD_WEB_PORT (which don't change at runtime), cache the result
after the first build.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@lefarcen lefarcen self-requested a review May 5, 2026 00:42
@lefarcen lefarcen added the enhancement New feature or request label May 5, 2026
@lefarcen
Copy link
Copy Markdown
Contributor

lefarcen commented May 5, 2026

Hi @hobostay! 🎉

Thanks for the contribution — this is a smart optimization that eliminates repeated Set construction on every request.

I will run a deep review and get back to you within 24h.

Thanks for making open-design better!
— open-design team

Copy link
Copy Markdown
Contributor

@lefarcen lefarcen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @hobostay, thanks for the optimization! This eliminates ~1200-1800 Set constructions per second on a busy daemon (12-18 origins × 100 req/s). I verified the caching is safe — resolvedPort, host, and OD_WEB_PORT are all immutable after startServer() completes.

Two minor suggestions below (non-blocking):

Comment thread apps/daemon/src/server.ts
);
return _cachedAllowedOrigins;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Stale comment reference — this line still mentions buildAllowedOrigins after the rename to getAllowedOrigins. Not a functional issue, but in security-sensitive origin-checking code it's worth keeping comments accurate to avoid future confusion.

Suggested fix: update the comment to reference getAllowedOrigins().

@lefarcen
Copy link
Copy Markdown
Contributor

lefarcen commented May 5, 2026

Additional suggestion — test coverage gap:

The existing origin tests in apps/daemon/tests/origin-validation.test.ts copy the middleware logic instead of exercising the real startServer() and the new cached getAllowedOrigins() closure. That means they would still pass if production caching captured a stale or missing OD_WEB_PORT.

Suggestion: Add a focused integration test against real startServer({ port: 0, returnServer: true }) with process.env.OD_WEB_PORT set before the first browser-origin request, asserting:

  • The web port origin is allowed
  • An unrelated port is rejected

This would catch any future cache lifetime issues. Not blocking, but worth considering for future robustness.

Copy link
Copy Markdown
Contributor

@mrcfps mrcfps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hobostay I reviewed the daemon origin-cache change and found one merge-blocking TypeScript issue in the new cache variable. Once the cache is explicitly typed, the optimization should preserve the intended origin policy while avoiding the repeated Set allocation. Thanks for the focused performance cleanup 🙂

Generated by Looper 0.5.4 · runner=reviewer · agent=opencode

Comment thread apps/daemon/src/server.ts
// both use the same policy (loopback + explicit bind host, HTTP + HTTPS,
// OD_WEB_PORT support).
function buildAllowedOrigins() {
let _cachedAllowedOrigins = null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cache needs an explicit Set<string> | null type before it can merge. With apps/daemon/tsconfig.json enabling strict, a let initialized only with null is not safely typed as the later Set, but the changed code assigns new Set(...) and then returns it for .has(...) checks in the middleware. That makes the daemon typecheck fail (or leaves the cache without the intended Set<string> contract), blocking this otherwise small optimization.

Please type the cache and return value explicitly, for example:

let _cachedAllowedOrigins: Set<string> | null = null;
function getAllowedOrigins(): Set<string> {
  if (_cachedAllowedOrigins) return _cachedAllowedOrigins;
  // ...
}

That keeps the cached origin set type aligned with the middleware usage and avoids a strict-mode compile break. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants