[WRONG BRANCH] fix(server): bound remote catalog serialization - #383
[WRONG BRANCH] fix(server): bound remote catalog serialization#383luvs01 wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 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: 12fc5cf0d4
ℹ️ 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".
| const after = catalogFileIdentity(path); | ||
| if (after?.key !== identity.key) return { body: null }; |
There was a problem hiding this comment.
When a catalog sync atomically replaces this file after the initial identity check but before this post-serialization check, the new file is valid and present, yet this branch returns { body: null }, which /v1/catalog maps to 404 catalog_not_found. This race can occur during normal startup because src/cli/index.ts starts the listener at line 268 before running the startup catalog sync at lines 393–399, and it can also occur during a manual sync; retry serialization against the new identity instead of reporting that no catalog exists.
Useful? React with 👍 / 👎.
| const identity = catalogFileIdentity(path); | ||
| if (!identity) return { body: null }; | ||
| if (identity.size > MAX_REMOTE_CATALOG_BYTES) return { body: null, error: "too_large" }; |
There was a problem hiding this comment.
When a previously cached catalog is later deleted, becomes unreadable, or is replaced by an oversized file, these early returns never clear remoteCache, so the old serialized body remains strongly referenced for the rest of the process unless another valid catalog is successfully serialized. Because one cached entry may be nearly MAX_REMOTE_CATALOG_BYTES (256 MiB), this can retain substantial heap even after the operator removes the problematic catalog; clear the stale cache whenever the current path or identity is missing or rejected.
Useful? React with 👍 / 👎.
1 participant
Motivation
GET/HEAD /v1/catalogrequest and exhausting CPU/memory or blocking the Bun event loop.Description
serializeRemotePersistedCatalog()insrc/server/catalog-download.tsthat preflights the on-disk file size viastatSync, returns an immediate over-limit sentinel, and otherwise calls the existing shared serializer; the function caches the serialized body/ETag by a high-resolution file identity and coalesces concurrent cache misses behind a single promise.serializePersistedCatalog()unchanged so the management route still materializes and emits the same bytes; the remote path enforces the 256 MiB limit before materialization and rechecks after serialization as defense-in-depth.src/server/index.tsto useserializeRemotePersistedCatalog(), to return507 catalog_too_largefor preflighted oversize files, and to preserve existing conditional-GET (If-None-Match) andHEADsemantics.tests/api-catalog-route.test.tsthat verifies the remote serialization is cached and that an oversized sparse/truncated file is rejected from metadata before JSON parsing.Testing
bun x tsc --noEmitwhich completed successfully.bun test tests/api-catalog-route.test.ts --test-name-pattern 'caches remote serialization|serves a supported large catalog'and both targeted tests passed.node_modules/bun/bin/bun.exe test tests/api-catalog-route.test.ts(bundled Bun 1.4.0) and observed the serializer-focused tests passed; system Bun 1.2.14 lackednode:zlibsupport referenced by some tests, so the bundled Bun was used for focused verification.Codex Task