Skip to content

fix(cloudflare): retry catalog load after failure - #255

Merged
BlackHole1 merged 2 commits into
oomol-lab:mainfrom
cyphercodes:fix/cloudflare-catalog-retry
Aug 2, 2026
Merged

fix(cloudflare): retry catalog load after failure#255
BlackHole1 merged 2 commits into
oomol-lab:mainfrom
cyphercodes:fix/cloudflare-catalog-retry

Conversation

@cyphercodes

Copy link
Copy Markdown
Contributor

Summary

  • clear rejected Cloudflare app and catalog promises so a recovered asset binding is retried
  • evict only the promise that is still cached, preserving concurrent request deduplication
  • cover concurrent failure sharing and a successful retry through worker.fetch

Fixes #246

Validation

  • npx vitest run src/server/cloudflare.test.ts (5 tests)
  • npm run fix-check
  • npm test (66 files, 704 tests)
  • npm run lint
  • npm run format
  • git diff --check

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a6160a28-3c2f-4407-ad8b-43793754a5ce

📥 Commits

Reviewing files that changed from the base of the PR and between a4b5c30 and ba3b600.

📒 Files selected for processing (4)
  • src/server/cloudflare.test.ts
  • src/server/cloudflare.ts
  • src/server/cloudflare/isolate-promise-cache.test.ts
  • src/server/cloudflare/isolate-promise-cache.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/server/cloudflare.test.ts

Summary by CodeRabbit

  • Bug Fixes
    • Improved app initialization reliability during concurrent requests.
    • Prevented duplicate catalog requests while initialization is in progress.
    • Failed catalog loads can now be retried successfully instead of leaving the app in a failed state.
    • Improved reliability when loading application configuration and security-related settings concurrently.
    • Ensured newer initialization attempts are not affected by failures from earlier attempts.

Walkthrough

Cloudflare initialization now uses IsolatePromiseCache for app, catalog, and secret-codec promises. Same-key concurrent requests share initialization work. Rejected promises are evicted only when they still own the cache entry. A later request can retry after a transient catalog failure. Tests cover cache reuse, key replacement, rejection handling, and concurrent Cloudflare requests.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title follows the required format and clearly describes the Cloudflare catalog retry fix.
Description check ✅ Passed The description explains the rejection handling, concurrency behavior, regression coverage, and validation for the Cloudflare changes.
Linked Issues check ✅ Passed The changes clear rejected catalog and app promises, preserve concurrent deduplication, and add regression coverage for failure followed by retry [#246].
Out of Scope Changes check ✅ Passed The cache abstraction and related secret-codec handling support the same rejection-retry behavior and remain consistent with the pull request objectives.
✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 `@src/server/cloudflare.test.ts`:
- Around line 28-63: Reset the module-scoped catalogPromise before each test so
the concurrent app-creation test always initializes its catalog from the
supplied env.ASSETS binding. Add a beforeEach reset in the test setup or use an
existing catalog-cache reset hook, preserving the retry assertions and avoiding
reuse of a resolved cache from earlier tests.
🪄 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: c159e20a-1c57-4a7e-8c64-ea042e4c6902

📥 Commits

Reviewing files that changed from the base of the PR and between deadc1c and 6751cda.

📒 Files selected for processing (2)
  • src/server/cloudflare.test.ts
  • src/server/cloudflare.ts

Comment thread src/server/cloudflare.test.ts
@cyphercodes
cyphercodes force-pushed the fix/cloudflare-catalog-retry branch from 6751cda to a4b5c30 Compare August 2, 2026 05:56
@BlackHole1 BlackHole1 self-assigned this Aug 2, 2026
The app and catalog fixes each open-coded the same slot: memoize a
promise, then drop it if it rejects so the isolate self-heals. A third
slot in the same file, `cachedSecretCodec`, kept the original
memoize-the-rejection shape, so a codec failure still poisons the
isolate for its lifetime even though the app around it now retries.

Route all three through `IsolatePromiseCache`, which memoizes one
promise per key, hands the in-flight promise to concurrent callers, and
clears the slot on rejection. Net effect: the third slot is fixed and
the invariant lives in one place instead of being restated per caller.

Unit-test the cache directly. The worker-level regression test cannot
reach the guard that lets only the owning entry clear the slot, since
that path needs an older promise to reject after a newer key replaced
it; dropping the guard leaves `cloudflare.test.ts` green.
@BlackHole1
BlackHole1 merged commit 56c325a into oomol-lab:main Aug 2, 2026
4 checks passed
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.

fix(cloudflare): a failed catalog load is cached for the isolate's lifetime

2 participants