Repository navigation
Conversation
Pin runtime cache writes outside the replaceable component tree and retain read-only build seed fallback. Capture each app/build binding before production serving, preserve Next cache semantics, and document migration and memory-cache boundaries. Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
Persist key ownership before disk writes so invalidation cannot revive a build seed. Read artifact hashes asynchronously, validate prepared configuration, wire cacheDirectory, and retain namespaces for safe stopped-worker cleanup. Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
Keep public declarations tied to the installed Next peer, guard startup compatibility, and require an explicit cache root outside the standard component layout. Verify persistence through both native and default Harper loaders and wait for strictly later native tag expiration in tests. Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
Skip app-local module comparison when the plugin is absent, leaving valid same-named custom handlers unchanged. Verify the guard against real files and clarify native worker-restart persistence, power-loss limits, and regular fetch-seed requirements. Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a versioned filesystem cache for Next.js to isolate incremental render and data caches on disk across deployments. It includes the core cache handler implementation, build identity verification, configuration validation, and comprehensive integration and unit tests. The feedback focuses on optimizing disk I/O in VersionedCacheHandler by using an in-memory Set to track owned keys, and ensuring complete delegation of filesystem methods in seedFileSystem by spreading the original fs object.
kriszyp
marked this pull request as ready for review
October 9, 2026 03:35
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
⊙ Problem
An outgoing Next.js worker can finish an ISR render after Harper replaces
components/<app>, writing old HTML into the incoming build's disk cache. A restart then serves that old HTML with stale chunk references. Closes HarperFast/harper#3031.💡 Solution
Add
versionedCacheHandlerPath()insrc/withHarper.ctsfor an incremental cache backed by each app's installed Next filesystem handler. Production workers capture an app/build-specific directory outside the replaceable component before serving, so an old worker's writes remain in its own cache. Component paths and file-editing APIs stay unchanged.⚖️ Alternatives
Independent planning review:
Framing-Verdict: chosen-approach-sound(d721d889ce0d).Core versioned component directories would also protect arbitrary app reads but complicate direct editing and component-file APIs. Stopping workers before activation changes rolling availability.
isrFlushToDisk: falseremoves durability; the existing Harper-backed handler changes the storage backend and cluster behavior. The selected folder approach retains Next's cache formats and local disk semantics.🔧 Changes
Startup order
plugin.ts:407
Product and architecture tour
Ownership before serving
How does a worker keep its write destination through a swap?
Production startup binds before preparing Next in src/plugin.ts, then checks the loaded runtime configuration and artifact digest before HTTP attaches at the startup validation. The plugin also types, validates and returns
cacheDirectoryat option resolution.src/versionedCache.cts binds the logical server directory to a fixed runtime directory, its installed Next cache constructor and read-only seed filesystem before
app.prepare(). A native CommonJS registry bridges the plugin's VM realm and Next's native handler. Rebinding a different artifact in one worker fails; the build is checked again after preparation. Startup checks the prepared renderer's actual configuration against the binding; mismatches fail before HTTP attaches. Missing renderer/config properties are tolerated for unbound apps; bound apps fail closed, while initialization errors still fail startup. Cleanup preserves the original failure. A missing binding in Harper production throws instead of falling back to writes in the app.Old and new builds keep separate write destinations
The captured directory remains unchanged when the live component path is replaced.
flowchart LR A[Old worker] --> B[App hash / old artifact hash] C[New worker] --> D[App hash / new artifact hash] C -. Cold whole-entry miss .-> E[Read-only build seeds]Durable caches and build seeds
What survives restarts without reviving invalidated build output?
The directory is
<cacheDirectory>/<app-path-hash>/<build-artifact-hash>, defaulting to~/harper/.nextjs-cachefor the standard component layout. Other layouts must setcacheDirectory; startup gives a clear error. Startup resolves existing cache-root ancestors, rejects configured or physical roots inside the component, and captures the resolved external path so retargeting an alias cannot redirect an existing worker. Missing cache-root descendants are created normally. Debug logs identify each bound directory for stopped-worker cleanup. Hashing BUILD_ID, manifests, server artifacts, initial fetch seeds and Next package metadata also isolates builds that reuse BUILD_ID. Hashing awaits bounded 256 KiB file reads and sorted traversal twice per worker at startup; its I/O cost grows with build size. Requests do not re-read the identity or reload the constructor from the replaced app. A hoisted Next installation is resolved through Node for both metadata and the captured constructor. Handler detection recognizes the plugin’s own canonical path when it is linked from a parent workspace, while an unrelated same-basename custom handler remains unbound.src/VersionedCacheHandler.cts reads the runtime cache first. Before a disk-backed runtime write, it persists ownership of that key in the captured directory; later native nulls remain misses instead of reviving older seeds with different tags. Ownership storage errors propagate rather than falling back to a seed; marker failures prevent writes. Only unowned whole-entry misses fall back to a separate read-only seed delegate. This prevents combining partial runtime HTML with seed RSC. Legacy
server/route-cacheentries are excluded; the read-only seed filesystem blocks seed promotion and fetch-tag backfill writes into the app. Writes, tag revalidation and request-cache reset use the installed Next implementation.Compatibility and retention
What does rollout require, and who owns cleanup?
The handler is opt-in; default/custom handlers, development and external builds keep their existing paths. The primary example keeps Next's memory default; shared installations must explicitly disable it. It supplies local disk persistence; it does not replicate entries across nodes or broadcast Next's worker-local tag invalidation. Set zero memory in every app sharing a physical Next installation and restart workers, because Next's inherited module-level LRU has unqualified keys. Stop outgoing stock-cache workers before the first migration: render seeds share their paths on 14/15/16.2, and fetch seeds share paths on every supported SDK. Subsequent outgoing workers must already use this handler.
README.md documents activation, artifact/startup constraints, migration, memory requirements and retained-cache cleanup; its option reference explains the default and relative/absolute roots. src/DESIGN.md records constructor binding, ownership, whole-entry seeds, asynchronous identity and retention invariants, indexed from DESIGN.md. No documentation-repository companion is needed: these are plugin-specific APIs documented in this repository.
✅ Verification
Local cache benchmarks use installed Next 16.2, warm filesystem, 4 KiB HTML plus RSC, 1,000 reads and 500 writes, with additional 10,000-hit memory checks. Native/versioned times in milliseconds per operation were 0.266/0.331 for seed disk hits, 0.284/0.241 for runtime disk hits, and 0.266/0.251 for disk writes. Including a constructor per seed request gave 0.260/0.378 ms on disk and 0.0003/0.0044 ms with memory; reused-handler memory hits were 0.0003–0.0005 ms. These local results show extra cold-seed and per-request construction cost; small differences vary with filesystem timing. They isolate cache cost and do not estimate HTTP throughput.
A synthetic startup benchmark uses Next 16.3.8, 1,000 one-KiB files, optionally an additional 256-MiB file, warm filesystem, and both full identity passes in fresh worker threads. With roughly 1 MiB, wall times for 1/4/16 threads were 0.338/0.358/0.677 seconds; with roughly 257 MiB, 1.158/1.082/1.544 seconds. This excludes Next preparation and is a local scaling comparison, not a cold-disk deployment estimate. The full passes detect changed artifacts with reused IDs; a deployment-supplied trusted identity could remove this cost later.
npm test: 193 passed (TypeScript build included), Node 26.2.0. src/VersionedCacheHandler.test.ts uses real installed Next 14.2.35, 15.5.15, 16.2.4 and 16.3.8 caches cover late writes, reused build IDs, restarts, same-module app isolation, read-only seeds, partial files, pages/route/fetch data, runtime-only tag expiration with seeds, ownership across fresh workers, storage failures, startup configuration mismatches, hoisted Next resolution, failures and retained namespaces. src/withHarper.test.ts proves the new incremental handler is separate from the existing handlers. The final compatibility guard prevents an unrelated same-basename custom handler from requiring an absent app-local plugin; a real-file regression covers binding and prepared-config checks. Additional real-file tests cover parent-workspace links, physical-root rejection and safe writes after an external alias is retargeted. A final regression proves that an unrelated handler with a vanished build-machine path remains unbound while its valid runtime path passes configuration checks; it failed with MODULE_NOT_FOUND before the guard.HARPER_CLUSTER_REQUIRED=1 npm run test:integration -- --workers=1: 44 passed with freshly installed fixtures, including existing cluster and browser tests.npm run test:integration -- integrationTests/next-16-versioned-cache.pw.ts --workers=1: both loader regressions passed. Bound Next14/15 and Turbopack versioned-cache startup are not exercised end-to-end; the actual-cache unit matrix and ordinary version startup tests provide the other coverage.npm pack --dry-run --jsonconfirms the new CJS modules and declarations ship; type declarations use the installednextpeer and dev aliases are absent from runtime imports. An isolated declaration consumer passes on all four SDKs and rejects an invalid constructor context; the old test-alias declarations failed that type-safety check. The corrected type imports emit identical runtime JavaScript. No lint/format script exists in this repository; TypeScript build andgit diff --checkare the available mechanical checks. No production dependency is added; package.json adds the exact 16.3.8 dev alias for the regression matrix and package-lock.json locks it and its platform packages. npm 11.13.0 regenerated the lockfile, including optional-platform metadata and semver 7.7.4→7.8.5, nanoid 3.3.12→3.3.20 and emnapi 1.10.0→1.11.3 transitive resolutions; the lockfile is not published.🤖 Generated by OpenAI Codex; posted via @kriszyp.
Related PRs: none found
Complexity: complicated
Review-Coverage: authored=codex; ran=cursor-composer,gemini,claude,cursor-muse; adjudicated=domain; declined=cursor-grok,cursor-kimi; rounds=6; full=5 @ 43f0210
Review-Attention: study ~9m (sensitive: package-lock.json; decisions: do-less-alternative, native-cache-semantics, ownership-error-policy, isolation-boundary, namespace-retention) @ 43f0210