Skip to content

fix(opencode): memoize /provider payload + response compression; optional slow-request log - #44132

Closed
camalolo wants to merge 5 commits into
anomalyco:devfrom
camalolo:pr/http-caching
Closed

camalolo wants to merge 5 commits into
anomalyco:devfrom
camalolo:pr/http-caching

Conversation

@camalolo

@camalolo camalolo commented Aug 22, 2026 •

Copy link
Copy Markdown

Issue for this PR

Closes #44180
Related: #35897 (provider payload size — orthogonal cap/pagination request, but same pain)

Type of change

  • Bug fix

What does this PR do?

Three commits, all targeting the cost measured in #44129:

  1. Memoize the /provider payload. The four inputs (config, models.dev catalog, connected providers, credentials) are reference-stable between reloads, so the encoded bytes are cached keyed on input identity (=== on all four) and served via handleRaw + HttpServerResponse.uint8Array. A config/auth/catalog reload swaps an input reference, which invalidates instantly. Responses are byte-identical (verified: 5,509,310 bytes, 193 providers, same shape before/after).
  2. Memoize response compression. A WeakMap keyed on the body object caches gzip/deflate results; the cache lives exactly as long as the body does. Streams and no-transform paths unchanged.
  3. Optional slow-request log: appends method/path/status/ms as JSONL to <data>/log/slow-requests.jsonl above OPENCODE_SLOW_REQUEST_MS (default 250, 0 disables). Zero cost on the fast path. Useful for catching regressions like this one without a collector attached.

How did you verify your code works?

  • Byte-for-byte comparison of /provider responses before/after (identical payload, headers apart from timing).
  • Benchmark on a production server: /provider p50 726ms → 22-29ms; p95 under 6x concurrency 2054-2356ms → 48ms; mixed-endpoint throughput ~5 req/s → ~205 req/s.
  • Under-load stall check: while /provider churns, other endpoints' p95 went from up to 11.7s to milliseconds.
  • Cache invalidation: config reload and auth changes produce a fresh payload (input identity changes).

Screenshots / recordings

N/A (server-side only).

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

Identical bodies (memoized provider list, doc pages) were re-gzipped
per request. WeakMap-keyed on the body identity so cache lives exactly
as long as the body object; streams and no-transform paths unchanged.
Appends method/path/status/duration as JSONL to
<data>/log/slow-requests.jsonl for requests slower than
OPENCODE_SLOW_REQUEST_MS (default 250, 0 disables). Survives collector
downtime; zero cost on the fast path.
Building this response walks the whole models.dev catalog through
deep clones and per-model schema validation, then serializes ~5MB of
JSON - ~700ms p50 on every web-UI page load. The four inputs (config,
catalog, connected providers, credentials) are reference-stable between
reloads, so memoize the encoded payload keyed on input identity and
serve the cached bytes via handleRaw; any reload changes an input
reference and invalidates instantly.
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

The /provider memoization design is good — single-slot cache keyed on four input references means no unbounded growth, and serving pre-serialized bytes skips both re-transform and re-stringify. Points to address:

1. Crash risk: WeakMap.set throws on primitive keys (packages/opencode/src/server/routes/instance/httpapi/middleware/compression.ts:16, :68-70). The old code passed body.body straight to zlib where both Uint8Array and string bodies work; the new code calls compressedCache.get(body.body) (safe, returns undefined for primitives) and then compressedCache.set(body.body, e) — which throws TypeError: Invalid value used as weak map key if any compressible-text route produces a string body. Unless this middleware provably only ever sees HttpBody.uint8Array, add a body.body instanceof Uint8Array guard around the whole memoization block, falling back to compress-without-cache.

2. The memoization silently depends on an unstated invariant (handlers/provider.ts:75-81). If cfg.get(), ModelsDev.Service.use(...), provider.list(), or authStore.all() happen to allocate fresh objects/references per call, the four-way === check never hits and this is a pure overhead addition — no error, no signal. Please add a test that calls list twice with unchanged state and asserts the returned byte array is the same reference (proving a hit), plus one mutation case proving invalidation. That pins the invariant against future refactors of those services.

3. slow-request-log details (slow-request-log.ts):

  • fs.appendFileSync (:29) runs exactly when the system is already degraded — sync I/O on the hot path amplifies the thing being measured. A fire-and-forget write or tiny buffer would be kinder.
  • Nothing caps slow-requests.jsonl growth; a long-lived server accumulates forever. Consider a simple size-based rotate/truncate.
  • Placement in server.ts:289 (inside errorLayer + compressionLayer) means the timer excludes compression time — for the very endpoints that motivated this PR, compressing 5MB is likely the dominant cost and will be invisible in the log. If that's intentional ("handler time"), document it at :7-9; if not, move the layer outward.

Minor: const path = request.url.split("?")[0] (:20) shadows the imported path module within the handler — works because OUT is resolved at module load, but it's a trap for future edits.

Also flagging scope: input-memoization fix, compression memoization, and a brand-new middleware are three separable changes; splitting would make bisecting a perf regression much easier.

@camalolo

Copy link
Copy Markdown
Author

Thanks — working through these found a real bug. Responses point by point:

1. WeakMap crash — false positive, but worth stating why. The middleware bails first with if (body._tag !== "Uint8Array") return response, and in effect/unstable/http the Uint8Array variant's body field is typed globalThis.Uint8Array (HttpBody.d.ts line 165-170). String bodies never reach the WeakMap: HttpBody.text/json return the Uint8Array variant (encoded), so by construction the key is always a Uint8Array instance. No guard added — the _tag check is the guard.

2. Your instability concern was correct and had already happened — fixed in 4de8e3a. I audited all four inputs: Config.get() and Provider.list() go through InstanceState (reference-stable), ModelsDev.get() is Effect.cachedInvalidateWithTTL(populate, infinity) (stable). But Auth.all() allocates a fresh object on every call (JSON.parse of env content or Record.filterMap of the auth file), so listCache.credentials === credentials was always false and the memo never hit — dead code. Fix: credentials are now compared by serialized value (JSON.stringify of a tiny auth record, microseconds vs. the ~700ms compute it guards); the three stable inputs still compare by reference. Added a behavioral test (same file): identical bytes on repeated requests, payload refreshes when OPENCODE_AUTH_CONTENT changes, byte-identical again when restored. Reference identity of the cached Uint8Array isn't observable through the HTTP boundary, so the test pins the observable contract (stability + invalidation) rather than the hit itself.

3. slow-request-log (a316dcd): append is now fire-and-forget (fs.promises chain, errors swallowed), the file rotates to slow-requests.jsonl.1 past 10 MB, the header documents that the timer intentionally measures handler time (layer sits inside compression — slow handlers are the signal), and the shadowed path local is gone (inlined request.url.split("?")[0].slice(0, 200)). Also hardened OPENCODE_SLOW_REQUEST_MS parsing the same way the otel PR now handles its ratio env (empty/garbage → 250 default instead of Number("") === 0 → log everything).

On scope: kept together deliberately — the slow-request log was added to measure exactly these two memoizations (it's how the 482ms list.compute average was found), and the compression memo gets its hits precisely because the /provider memo serves a stable byte instance. Happy to split the log into its own PR if maintainers prefer.

@camalolo

Copy link
Copy Markdown
Author

Closing — these changes continue to be maintained in our fork. Thanks for the automated review feedback; feel free to reach out if upstream interest revives.

@camalolo camalolo closed this Sep 18, 2026
@camalolo
camalolo deleted the pr/http-caching branch September 21, 2026 12:38
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.

/provider rebuilds and re-compresses a ~5.5MB payload on every web UI page load

2 participants