fix: prevent stale extension bundle cache when source has bare specifiers#876
fix: prevent stale extension bundle cache when source has bare specifiers#876
Conversation
…iers ## Summary - Extension loaders (models, vaults, drivers, datastores) could serve stale cached bundles even when source files had changed, if the source used bare specifiers like from "zod" - The mtime check correctly detected staleness, but the bare specifier fallback unconditionally returned the cached bundle, bypassing the freshness result - Changed all four loaders to attempt rebundling first, falling back to cached bundle only if rebundling fails (e.g. pulled extensions without a deno.json import map) ## Test plan - 3562 unit tests pass - Verified stale bundle is replaced when source changes (backdated bundle mtime, confirmed rebundle on next run) - Verified pulled extensions with bare specifiers and pre-built bundles still load (@swamp/hetzner-cloud with 11 models in a repo without deno.json) - Verified rebundle failure logs a warning and falls back to cached bundle safely
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Blocking Issues
None.
Suggestions
-
The docstring on
sourceHasBareSpecifiersinsrc/domain/models/bundle.ts:176now says "Used by runtime loaders to determine whether a cached bundle should be preferred over re-bundling" — after this PR, it's only used byextension_push.ts. Consider updating the docstring to reflect the narrowed usage. -
All four loaders (
user_model_loader.ts,user_datastore_loader.ts,user_driver_loader.ts,user_vault_loader.ts) share an identicalbundleWithCachepattern. This is pre-existing duplication and not in scope for this fix, but worth noting for a future refactor into a shared utility.
Overall: Clean, well-reasoned fix. The "try rebundle first, fall back to cache on failure" approach correctly addresses the staleness bug while preserving the safety net for pulled extensions without deno.json. The in-memory bundling before write prevents cache corruption, assertSafePath is preserved, and the log level upgrade to warn on fallback is appropriate.
There was a problem hiding this comment.
Adversarial Review
Critical / High
None.
Medium
-
Performance regression for pulled extensions with bare specifiers (
user_model_loader.ts:467, and equivalents in the other 3 loaders): The old code detected bare specifiers upfront and short-circuited to the cached bundle. The new code always attemptsbundleExtension()first, which will invoke the Deno bundler, fail (potentially after seconds of work), and then fall back to cache. For repos with many pulled extensions (the PR mentions@swamp/hetzner-cloudwith 11 models), this adds non-trivial latency on every load when the bundle is stale or freshness check fails. This is a deliberate correctness-over-speed trade-off and is documented in the PR, but worth noting that users with many pulled bare-specifier extensions may see slower cold starts. -
TOCTOU on cache fallback read (
user_model_loader.ts:475-478, same in all 4 files):bundleExistsis set viaDeno.stat()earlier, but the fallbackDeno.readTextFile(bundlePath)in thecatch (bundleError)block runs later. If the bundle file is deleted between the stat and the read (e.g., concurrentgcor manual cleanup),readTextFilethrows, and that exception escapes unhandled — it won't be caught by the outer catch since we're already inside it. The same TOCTOU existed in the old code, and the window is tiny on local filesystems, so this is not a regression — just a pre-existing theoretical concern.
Low
- Unnecessary rebundle when mtime is unavailable: If
bundleStat.mtimeisnull(some filesystems don't support mtime), the code falls through to rebundle even though the bundle exists and may be perfectly fine. The old code had the same behavior here, so not a regression.
Verdict
PASS — The change is logically correct and consistent across all four loaders. The core fix (don't let bare-specifier detection bypass staleness) is sound. The rebundle-then-fallback pattern safely preserves the old bundle on failure since bundleExtension returns a string in memory before any write occurs. The sourceHasBareSpecifiers import is correctly removed from all four files and remains used in extension_push.ts. No security, data integrity, or correctness issues found.
…llback (#878) ## Summary - When a rebundle fails for pulled extensions with bare specifiers, touch the cached bundle's mtime so subsequent loads short-circuit at the freshness check instead of re-invoking the Deno bundler on every cold start - Fix a pre-existing TOCTOU race where the cache file could be deleted between the initial `Deno.stat()` and the fallback `Deno.readTextFile()` - Applied consistently to all 4 extension loaders: models, vaults, drivers, and datastores ## Problem PR #876 changed extension loaders to attempt rebundling before falling back to cached bundles, fixing a bug where stale caches were served even when source files had changed. However, CI review identified a performance regression: For **pulled extensions with bare specifiers** (e.g. `from "zod"` resolved via a project's `deno.json`), repos without the import map can't rebundle. The old code detected bare specifiers upfront and returned the cache immediately. The new code attempts `bundleExtension()`, which invokes the Deno bundler, waits for it to fail, then falls back — adding seconds of latency **per extension on every load**. This is because the cache file's mtime was never updated after a fallback, so the next mtime check would see the cache as stale again and trigger the same failed rebundle cycle. ## User Impact Users with many pulled extensions using bare specifiers (e.g. `@swamp/hetzner-cloud` with 11 models) would see significantly slower cold starts — the Deno bundler would be invoked and fail 11 times on every `swamp` command, adding potentially 10+ seconds of startup latency. ## Fix **Mtime touch**: After falling back to the cached bundle on rebundle failure, `Deno.utime()` touches the cache file's mtime. The next load sees the cache as "fresh" and returns it immediately at the mtime check — no bundler invocation. - First load with stale bare-specifier bundle: tries rebundle → fails → falls back → touches cache (one-time cost) - Subsequent loads: mtime check sees cache as fresh → returns immediately (no cost) - If source changes again later: mtime check detects newer source → tries rebundle again **TOCTOU fix**: The `bundleExists` flag was set via `Deno.stat()` early in the method, but the fallback `Deno.readTextFile()` runs in a later catch block. If the bundle is deleted between the stat and the read (e.g. concurrent garbage collection), `readTextFile` would throw unhandled. The fallback read is now wrapped in a try/catch — if the file is gone, it falls through to throw the original `bundleError`. ## Test plan - [x] `deno check` passes on all 4 loaders - [x] `deno lint` passes on all 4 loaders - [x] `deno fmt` passes - [x] 3562 unit tests pass - [x] Verified pulled extensions with bare specifiers still load (`@swamp/hetzner-cloud` — 11 models in a repo without `deno.json`) - [x] Verified stale bundles are replaced when source changes and `deno.json` is present (local dev scenario) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
Test plan