perf: optimize html/pdf rendering with persistent caches, pooled workers and faster multi-pass PDFs#60
perf: optimize html/pdf rendering with persistent caches, pooled workers and faster multi-pass PDFs#60yashmehrotra wants to merge 8 commits into
Conversation
WalkthroughAdds benchmark tooling and documentation, content-addressed build and Tailwind caches, persistent SSR loaders, configurable worker lifecycle controls, reusable template workspaces, streamed render-cache responses, and a shared PDF rendering pipeline with readiness synchronization. ChangesRendering platform and benchmarks
Sequence Diagram(s)sequenceDiagram
participant RenderRoute
participant BuildTemplate
participant SsrLoaderPool
participant SsrDaemon
RenderRoute->>BuildTemplate: request persistent render
BuildTemplate->>SsrLoaderPool: runPersistentSsrLoader
SsrLoaderPool->>SsrDaemon: send cache-keyed NDJSON request
SsrDaemon-->>SsrLoaderPool: return HTML and CSS
SsrLoaderPool-->>BuildTemplate: return render result
BuildTemplate-->>RenderRoute: return cached build output
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify 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. Comment |
Add content-addressed Vite SSR and Tailwind caches so data-only renders avoid rebuilding unchanged templates and styles. Keep bounded persistent template workspaces and SSR loader processes for warm server requests, with idle/LRU cleanup and a one-shot fallback. Serialize workspace builds and content-address generated header/footer fragments to make concurrent requests safe. Stream cached responses from disk and harden the browser worker pool with queue limits, acquisition timeouts, age/render/RSS recycling, health metrics, and graceful shutdown. Expose the new server controls through CLI flags and environment-backed configuration, and cover build keys, workspace reuse, worker queue policy, and concurrent fragments with regression tests.
Consolidate the duplicated PDF entry points behind one browser-backed pipeline and isolate each render in its own browser context. Reduce multi-pass work by extracting minimal group and overlay HTML, rendering only page type/size combinations that actually occur, and releasing intermediate group and overlay buffers after embedding. Avoid the metadata-only pdf-lib rewrite for direct Chromium PDFs while preserving Facet metadata for composited documents. Preserve pre-existing HTML output when PDF generation uses the same output name. Re-enable the visual PDF suite, fix its large-page footer fixture and rasterization timeout, and add regression coverage for overlay type/size selection.
Add cold/warm CLI and persistent-server benchmark harnesses with stage timings, per-request process-tree, Chromium and Node memory metrics, output sizes, and configurable iteration/document sizes. Add a deterministic mixed-page fixture that exercises typed headers and footers, three page sizes, content grouping, and pdf-lib compositing. Document benchmark methodology and measured checkpoints, including retained improvements, memory tradeoffs, rejected overlay concurrency, and all new server tuning controls.
Allow zero-browser worker pools for health-only server tests while continuing to reject negative and non-integer worker counts. Increase the integration render timeout and large mixed-page visual hook timeout so concurrent full-suite execution has enough time for dependency builds and ImageMagick rasterization. Verified with task test:unit: 310 passed, 1 network-dependent test skipped.
Invoke the TypeScript CLI entry through Node and tsx when visual tests generate HTML, removing the unit suite's dependency on dist/facet being built first. Clean up the temporary kitchen-sink node_modules symlink produced by source CLI execution without touching consumer-owned dependencies. Verified task test:unit with dist/facet temporarily removed: 310 passed, 1 network-dependent test skipped.
Guard FACET_PACKAGE_PATH directory rebuilds with an atomic filesystem lock so concurrent render workers cannot resolve @flanksource/facet while another process empties and rebuilds dist/. Recheck component and CSS freshness after acquiring the lock, recover stale locks, bound lock waits by the existing local build timeout, and always clean up the lock file. Verified by touching component sources to force a stale local package and running task test:unit: 310 passed, 1 network-dependent test skipped.
Make test:unit depend on the project build because the visual PDF tests intentionally exercise dist/facet and the built local @flanksource/facet package. Restore visual HTML generation through the release binary instead of the Node/tsx source entry, avoiding Node 22 and Vite directory-link resolution failures for the kitchen-sink file dependency. Clean up only the test-generated kitchen-sink node_modules symlink after the suite. Verified from a missing dist/facet state: task test:unit builds first, then passes 310 tests with 1 network-dependent skip.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/src/server/worker-pool.ts (1)
145-178: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
recyclingcounter leaks when recycling during shutdown.The
this.shuttingDownearly return (this.total--; return;) is outside thetry/finallythat decrementsthis.recycling, so any recycle triggered while shutting down leavesthis.recyclingpermanently incremented (never reset inshutdown()either).🔧 Proposed fix
if (this.shuttingDown) { this.total--; + this.recycling--; return; }🤖 Prompt for 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. In `@cli/src/server/worker-pool.ts` around lines 145 - 178, Ensure recycle always decrements this.recycling, including when this.shuttingDown is true. Move the shutdown early return inside the existing try/finally structure in recycle, preserving the total decrement and avoiding browser replacement during shutdown.cli/src/bundler/vite-builder.ts (1)
187-274: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftBuild lock scope includes the SSR render step, potentially serializing concurrent renders of the same template.
withBuildLockwraps all ofbuildTemplateUnlocked, including the actualrunPersistentSsrLoader/runLoaderWithRetryrender call. That's necessary for the filesystem setup (symlinking, config/package.json generation, pnpm install) which can't safely run concurrently for the same.facet/dir, but once the workspace is current, concurrent render requests for the same template (differentdata) are still forced to execute one-at-a-time by this lock — including inside the persistent-SSR path this PR adds to speed things up.Worth considering whether the lock can be narrowed to just the "ensure workspace is built" phase, letting persistent-loader renders for the same template run concurrently once the build is current.
🤖 Prompt for 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. In `@cli/src/bundler/vite-builder.ts` around lines 187 - 274, Narrow withBuildLock in buildTemplate so it protects only the shared workspace setup and dependency-installation phase, not the SSR rendering performed by runPersistentSsrLoader or runLoaderWithRetry. Ensure concurrent requests can render different data for an already-prepared facet workspace while preserving serialized filesystem/configuration setup for the same lock key.
🧹 Nitpick comments (4)
cli/scripts/benchmark-render.mjs (2)
8-11: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSame cwd-relative path resolution fragility in both scripts. Both scripts resolve the CLI binary and default template/input paths via
resolve()againstprocess.cwd()instead of the script's own location, so they only work when invoked with cwd pinned tocli/(e.g. via the newpnpm run bench:*scripts).
cli/scripts/benchmark-render.mjs#L8-L11: resolvecli,template, anddatarelative toimport.meta.urlinstead ofprocess.cwd().cli/scripts/benchmark-server.mjs#L9-L18: resolvecliandtemplatesDirthe same way for consistency.🤖 Prompt for 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. In `@cli/scripts/benchmark-render.mjs` around lines 8 - 11, Update path resolution in cli/scripts/benchmark-render.mjs lines 8-11 and cli/scripts/benchmark-server.mjs lines 9-18 to resolve the CLI binary and default template/input directories relative to each script’s import.meta.url rather than process.cwd(); preserve command-line overrides while making both benchmark scripts runnable from any working directory.
14-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
processTreeRssimplementation across both benchmark scripts. Both files define an identical/proc-based process-tree RSS sampler; extracting it to a shared module avoids drift between the two.
cli/scripts/benchmark-render.mjs#L14-L26: move this implementation into a small shared helper module (e.g.cli/scripts/lib/proc-rss.mjs) and import it here.cli/scripts/benchmark-server.mjs#L20-L32: import the same shared helper instead of keeping a second copy.🤖 Prompt for 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. In `@cli/scripts/benchmark-render.mjs` around lines 14 - 26, Extract the duplicate processTreeRss implementation from cli/scripts/benchmark-render.mjs lines 14-26 and cli/scripts/benchmark-server.mjs lines 20-32 into a shared helper module, such as cli/scripts/lib/proc-rss.mjs. Export and import the helper in both scripts, removing their local copies while preserving the existing Linux, recursion, and error-handling behavior.cli/src/server/worker-pool.ts (1)
180-194: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
stats()recomputes Chromium RSS synchronously for every worker on every call.
stats()is called from the/healthzendpoint (per graph evidence), so every health probe walks each worker's full Chromium process tree via synchronous/procreads (already done once per worker inrelease()). Under frequent probing with multiple long-lived workers/children this adds blocking I/O on a hot, frequently-polled path. Consider caching/throttling this (e.g. only refresh on a TTL, or reuseworker.lastRssMbfrom the lastrelease()instead of recomputing on everystats()call).🤖 Prompt for 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. In `@cli/src/server/worker-pool.ts` around lines 180 - 194, Update WorkerPool.stats() to avoid synchronously recomputing Chromium RSS for every worker on each health check. Reuse the per-worker lastRssMb values populated by release(), or refresh them only under a suitable TTL, while preserving the chromiumRssMb aggregate and existing stats fields.cli/src/bundler/build-cache.ts (1)
22-64: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFull source-tree content hash runs on every render.
computeTemplateBuildKeyreads and hashes the full content of every matching file (including.json— potentially large data fixtures) underconsumerRoot, and this runs on everybuildTemplateUnlockedcall, not just when something actually needs rebuilding. For larger consumer projects this read+hash cost is paid on every request, partially offsetting the caching benefits this PR is trying to deliver.Consider a cheaper first-pass check (e.g. mtime+size digest) with the full content hash only as a fallback/verification step, to keep the common "nothing changed" case cheap.
🤖 Prompt for 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. In `@cli/src/bundler/build-cache.ts` around lines 22 - 64, Optimize computeTemplateBuildKey so the routine does not read and hash every matching file on each render. Add a cheap first-pass fingerprint based on stable metadata such as each file’s path, size, and modification time, and reuse it for the unchanged-tree case; retain full content hashing only as a fallback or verification step when metadata indicates a possible change. Preserve the existing exclusions, file selection, sorting, and selected template-entry handling.
🤖 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 `@cli/src/builders/facet-directory.ts`:
- Around line 996-1004: Update the lock acquisition loop around openSync and
writeFileSync so any write failure after successfully opening the lock closes fd
and removes lockPath before rethrowing. Preserve the existing EEXIST retry
behavior, and ensure cleanup applies only to the newly acquired descriptor and
lock file.
- Around line 993-1022: Update withLocalFacetBuildLock and its callers
buildTemplateUnlocked/startViteServer so lock acquisition no longer uses
blocking Atomics.wait on the Node event loop. Make the lock-wait path
asynchronous, preserving timeout, stale-lock cleanup, action execution, and
finally-based descriptor/file cleanup while awaiting the lock.
In `@cli/src/bundler/ssr-pool.ts`:
- Around line 112-125: Update runPersistentSsrLoader and SsrLoaderProcess so
eviction never closes a loader with outstanding requests: expose and use a
pending-request status such as hasPending() or pendingCount, select the oldest
idle loader for eviction, and if none is idle reject the new request with a
clear pool-exhausted error instead of terminating active work.
- Around line 70-75: Validate the parsed environment values in
scheduleIdleShutdown and the loader-cap logic before applying Math.max or
comparing against the limit. If FACET_SSR_LOADER_IDLE_MS or
FACET_MAX_SSR_LOADERS is missing, non-numeric, or otherwise invalid, fall back
to the existing defaults (300000 and 4 respectively), while preserving the
existing minimum bounds.
- Around line 46-56: Update the reply handler in the SSR pool’s createInterface
callback so scheduleIdleShutdown() is invoked only after all pending requests
have completed. Check this.pending after removing the replied request,
preserving the existing resolve/reject behavior while preventing close() from
terminating an in-flight request.
In `@cli/src/cli.ts`:
- Around line 230-235: Update the Commander option declarations for
max-renders-per-worker, max-queue-depth, max-worker-age, max-worker-rss, and
worker-acquire-timeout to coerce and validate their values as numbers before
storing them in options. Reject invalid or non-numeric input at CLI parsing time
so WorkerPool receives valid numeric limits.
In `@cli/src/loaders/ssr.ts`:
- Around line 111-128: Ensure the temporary staging directory created in the
!cachedBundle build flow is removed when await build(...) throws. Wrap the build
operation and subsequent cache promotion in cleanup handling, removing buildDir
recursively with force enabled on failure while preserving the existing rename
race handling and rethrowing the original build error.
In `@cli/src/server/preview.ts`:
- Line 9: Update createServer’s stop() cleanup to avoid calling the process-wide
shutdownPersistentSsrLoaders for all loaders; track the facetRoot values used by
that server instance and shut down only those loaders, preserving loaders owned
by other live server instances.
In `@cli/src/server/routes.ts`:
- Around line 457-469: Add per-workspace pruning for `.facet-fragments` within
acquireTemplateWorkspace after writing the content-hashed fragment, using the
existing pruneWorkspaces or pruneTailwindCache pattern and an entry-count or age
limit. Keep current fragment creation and buildTemplate behavior unchanged while
ensuring stale or excess fragment files are removed.
In `@cli/src/utils/browser-readiness.ts`:
- Around line 36-53: Update the readiness flow containing document.fonts.ready,
image loading/decoding, and __FACET_READY__ so timeoutMs applies to the entire
phase through one shared deadline or remaining-time budget rather than creating
a fresh timeout for each await. Preserve the existing readiness tasks and
timeout behavior, and ensure the budget is consumed across them collectively.
In `@cli/src/utils/pdf-generator.ts`:
- Around line 485-506: Update the zero-margin fast path inside renderSinglePass
to apply the same creator/producer PDF metadata stamping performed by
compositeHeaderFooter before returning page.pdf() bytes. Preserve the existing
behavior for non-zero margins and ensure the metadata update occurs before any
subsequent debug typography processing in the caller.
In `@cli/src/utils/tailwind.ts`:
- Around line 78-88: Guard the cache-directory listing and file metadata
collection in pruneTailwindCache with a try/catch, returning without pruning
when readdir or stat encounters a concurrent deletion or other filesystem error.
Keep the existing sorting and removal behavior for successful reads, and ensure
pruning failures do not propagate through runTailwindCached.
- Around line 107-153: Update the cache-hit path in runTailwindCached to refresh
cachePath’s access and modification timestamps after successfully reading the
cached CSS, using the node:fs/promises utimes API; preserve the existing
read-and-return behavior and allow timestamp refresh errors to propagate
consistently.
---
Outside diff comments:
In `@cli/src/bundler/vite-builder.ts`:
- Around line 187-274: Narrow withBuildLock in buildTemplate so it protects only
the shared workspace setup and dependency-installation phase, not the SSR
rendering performed by runPersistentSsrLoader or runLoaderWithRetry. Ensure
concurrent requests can render different data for an already-prepared facet
workspace while preserving serialized filesystem/configuration setup for the
same lock key.
In `@cli/src/server/worker-pool.ts`:
- Around line 145-178: Ensure recycle always decrements this.recycling,
including when this.shuttingDown is true. Move the shutdown early return inside
the existing try/finally structure in recycle, preserving the total decrement
and avoiding browser replacement during shutdown.
---
Nitpick comments:
In `@cli/scripts/benchmark-render.mjs`:
- Around line 8-11: Update path resolution in cli/scripts/benchmark-render.mjs
lines 8-11 and cli/scripts/benchmark-server.mjs lines 9-18 to resolve the CLI
binary and default template/input directories relative to each script’s
import.meta.url rather than process.cwd(); preserve command-line overrides while
making both benchmark scripts runnable from any working directory.
- Around line 14-26: Extract the duplicate processTreeRss implementation from
cli/scripts/benchmark-render.mjs lines 14-26 and
cli/scripts/benchmark-server.mjs lines 20-32 into a shared helper module, such
as cli/scripts/lib/proc-rss.mjs. Export and import the helper in both scripts,
removing their local copies while preserving the existing Linux, recursion, and
error-handling behavior.
In `@cli/src/bundler/build-cache.ts`:
- Around line 22-64: Optimize computeTemplateBuildKey so the routine does not
read and hash every matching file on each render. Add a cheap first-pass
fingerprint based on stable metadata such as each file’s path, size, and
modification time, and reuse it for the unchanged-tree case; retain full content
hashing only as a fallback or verification step when metadata indicates a
possible change. Preserve the existing exclusions, file selection, sorting, and
selected template-entry handling.
In `@cli/src/server/worker-pool.ts`:
- Around line 180-194: Update WorkerPool.stats() to avoid synchronously
recomputing Chromium RSS for every worker on each health check. Reuse the
per-worker lastRssMb values populated by release(), or refresh them only under a
suitable TTL, while preserving the chromiumRssMb aggregate and existing stats
fields.
🪄 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
Run ID: 22814b86-71bf-43db-ac75-1ddb3d1d96aa
📒 Files selected for processing (33)
README.mdTaskfile.ymlcli/examples/BenchmarkMixedReport.tsxcli/package.jsoncli/scripts/benchmark-render.mjscli/scripts/benchmark-server.mjscli/src/builders/facet-directory.tscli/src/bundler/build-cache.test.tscli/src/bundler/build-cache.tscli/src/bundler/ssr-pool.tscli/src/bundler/vite-builder.tscli/src/cli.tscli/src/generators/html.tscli/src/generators/pdf.tscli/src/loaders/ssr.tscli/src/server/config.tscli/src/server/preview.tscli/src/server/render-cache.tscli/src/server/routes.tscli/src/server/template-workspaces.test.tscli/src/server/template-workspaces.tscli/src/server/worker-pool.test.tscli/src/server/worker-pool.tscli/src/utils/browser-readiness.tscli/src/utils/pdf-generator.tscli/src/utils/pdf-multipass.test.tscli/src/utils/pdf-multipass.tscli/src/utils/performance.tscli/src/utils/tailwind.test.tscli/src/utils/tailwind.tscli/test/pdf-bleed.test.tscli/test/render-api.test.tsexamples/kitchen-sink/PageSizeTest.tsx
| private withLocalFacetBuildLock(packageRoot: string, action: () => void): void { | ||
| const lockPath = join(packageRoot, '.facet-local-build.lock'); | ||
| const deadline = Date.now() + LOCAL_BUILD_TIMEOUT_MS; | ||
| let fd: number | undefined; | ||
| while (fd === undefined) { | ||
| try { | ||
| fd = openSync(lockPath, 'wx'); | ||
| writeFileSync(fd, `${process.pid}\n`); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== 'EEXIST') throw error; | ||
| try { | ||
| if (Date.now() - statSync(lockPath).mtimeMs > LOCAL_BUILD_TIMEOUT_MS * 2) { | ||
| unlinkSync(lockPath); | ||
| continue; | ||
| } | ||
| } catch { continue; } | ||
| if (Date.now() >= deadline) { | ||
| throw new Error(`Timed out waiting for local Facet build lock: ${lockPath}`); | ||
| } | ||
| Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 100); | ||
| } | ||
| } | ||
| try { | ||
| action(); | ||
| } finally { | ||
| closeSync(fd); | ||
| try { unlinkSync(lockPath); } catch { /* already cleaned up */ } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether ensureLocalFacetPackageBuilt / generatePackageJson can run inside the persistent server process (not just one-shot CLI renders).
rg -n "generatePackageJson|ensureLocalFacetPackageBuilt" cli/src -A2 -B5Repository: flanksource/facet
Length of output: 6905
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- facet-directory lock implementation ---'
sed -n '970,1028p' cli/src/builders/facet-directory.ts
echo
echo '--- render/build call sites ---'
rg -n "buildTemplateUnlocked|buildTemplate\(|renderHTML(Streamed)?\(|startViteServer|generatePackageJson\(" cli/src -A4 -B4
echo
echo '--- routes around renderHTML/renderHTMLStreamed ---'
fd -a 'routes.ts' cli/srcRepository: flanksource/facet
Length of output: 17491
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- buildTemplate / render call graph ---'
rg -n "buildTemplateUnlocked|buildTemplate\(|renderHTML(Streamed)?\(" cli/src -A6 -B6
printf '\n%s\n' '--- routes file presence ---'
fd -a 'routes.ts' cli/src
printf '\n%s\n' '--- vite-server path ---'
sed -n '1,120p' cli/src/bundler/vite-server.ts
printf '\n%s\n' '--- vite-builder path ---'
sed -n '200,260p' cli/src/bundler/vite-builder.tsRepository: flanksource/facet
Length of output: 15556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "renderHTMLStreamed|renderHTML|buildTemplateUnlocked|buildTemplate" . -A5 -B5Repository: flanksource/facet
Length of output: 12103
Synchronous lock wait blocks the Node process
Atomics.wait blocks the main thread while waiting on .facet-local-build.lock. This runs from buildTemplateUnlocked() (used by the server routes) and startViteServer(), so a concurrent local-facet rebuild can stall the entire process for up to 5 minutes. Make the wait async or move the lock handling off the event loop.
🤖 Prompt for 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.
In `@cli/src/builders/facet-directory.ts` around lines 993 - 1022, Update
withLocalFacetBuildLock and its callers buildTemplateUnlocked/startViteServer so
lock acquisition no longer uses blocking Atomics.wait on the Node event loop.
Make the lock-wait path asynchronous, preserving timeout, stale-lock cleanup,
action execution, and finally-based descriptor/file cleanup while awaiting the
lock.
| let fd: number | undefined; | ||
| while (fd === undefined) { | ||
| try { | ||
| fd = openSync(lockPath, 'wx'); | ||
| writeFileSync(fd, `${process.pid}\n`); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== 'EEXIST') throw error; | ||
| try { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Lock fd/file leak if writeFileSync fails after openSync succeeds.
If writeFileSync(fd, ...) throws (non-EEXIST path), the function rethrows without closing fd or unlinking lockPath — the fd stays open for the process lifetime and the lock file lingers until another process's staleness check (10 minutes) removes it.
🔧 Proposed fix
try {
fd = openSync(lockPath, 'wx');
- writeFileSync(fd, `${process.pid}\n`);
} catch (error) {
+ writeFileSync(fd, `${process.pid}\n`);
+ } catch (error) {
+ if (fd !== undefined) {
+ try { closeSync(fd); } catch {}
+ try { unlinkSync(lockPath); } catch {}
+ fd = undefined;
+ }
const code = (error as NodeJS.ErrnoException).code;🤖 Prompt for 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.
In `@cli/src/builders/facet-directory.ts` around lines 996 - 1004, Update the lock
acquisition loop around openSync and writeFileSync so any write failure after
successfully opening the lock closes fd and removes lockPath before rethrowing.
Preserve the existing EEXIST retry behavior, and ensure cleanup applies only to
the newly acquired descriptor and lock file.
| createInterface({ input: this.child.stdout }).on('line', (line) => { | ||
| let reply: Reply; | ||
| try { reply = JSON.parse(line) as Reply; } | ||
| catch { return; } | ||
| const pending = this.pending.get(reply.id); | ||
| if (!pending) return; | ||
| this.pending.delete(reply.id); | ||
| if (reply.result) pending.resolve(reply.result); | ||
| else pending.reject(new Error(reply.error ?? 'Persistent SSR loader failed')); | ||
| this.scheduleIdleShutdown(); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Idle shutdown can fire while other requests are still in flight.
scheduleIdleShutdown() runs after every reply regardless of whether this.pending still has outstanding entries. If two requests are sent back-to-back to the same loader, the first reply arms an idle timer; if it elapses before the remaining reply arrives (e.g. under a shortened FACET_SSR_LOADER_IDLE_MS), close() tears down the child and rejects the still-pending request via fail().
🔧 Proposed fix
if (reply.result) pending.resolve(reply.result);
else pending.reject(new Error(reply.error ?? 'Persistent SSR loader failed'));
- this.scheduleIdleShutdown();
+ if (this.pending.size === 0) this.scheduleIdleShutdown();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| createInterface({ input: this.child.stdout }).on('line', (line) => { | |
| let reply: Reply; | |
| try { reply = JSON.parse(line) as Reply; } | |
| catch { return; } | |
| const pending = this.pending.get(reply.id); | |
| if (!pending) return; | |
| this.pending.delete(reply.id); | |
| if (reply.result) pending.resolve(reply.result); | |
| else pending.reject(new Error(reply.error ?? 'Persistent SSR loader failed')); | |
| this.scheduleIdleShutdown(); | |
| }); | |
| createInterface({ input: this.child.stdout }).on('line', (line) => { | |
| let reply: Reply; | |
| try { reply = JSON.parse(line) as Reply; } | |
| catch { return; } | |
| const pending = this.pending.get(reply.id); | |
| if (!pending) return; | |
| this.pending.delete(reply.id); | |
| if (reply.result) pending.resolve(reply.result); | |
| else pending.reject(new Error(reply.error ?? 'Persistent SSR loader failed')); | |
| if (this.pending.size === 0) this.scheduleIdleShutdown(); | |
| }); |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for 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.
In `@cli/src/bundler/ssr-pool.ts` around lines 46 - 56, Update the reply handler
in the SSR pool’s createInterface callback so scheduleIdleShutdown() is invoked
only after all pending requests have completed. Check this.pending after
removing the replied request, preserving the existing resolve/reject behavior
while preventing close() from terminating an in-flight request.
| private scheduleIdleShutdown(): void { | ||
| if (this.idleTimer) clearTimeout(this.idleTimer); | ||
| const idleMs = Math.max(1_000, parseInt(process.env['FACET_SSR_LOADER_IDLE_MS'] ?? '300000', 10)); | ||
| this.idleTimer = setTimeout(() => void this.close(), idleMs); | ||
| this.idleTimer.unref(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unvalidated parseInt on env vars can silently disable limits.
parseInt(process.env['FACET_SSR_LOADER_IDLE_MS'] ?? '300000', 10) (line 72) and parseInt(process.env['FACET_MAX_SSR_LOADERS'] ?? '4', 10) (line 118) return NaN for a malformed value, and Math.max(1, NaN)/Math.max(1_000, NaN) both evaluate to NaN. That disables the loader cap entirely (unbounded growth, since loaders.size >= NaN is always false) or makes idle shutdown fire almost immediately (NaN delay).
🔧 Proposed fix
- const idleMs = Math.max(1_000, parseInt(process.env['FACET_SSR_LOADER_IDLE_MS'] ?? '300000', 10));
+ const parsedIdleMs = parseInt(process.env['FACET_SSR_LOADER_IDLE_MS'] ?? '300000', 10);
+ const idleMs = Math.max(1_000, Number.isNaN(parsedIdleMs) ? 300_000 : parsedIdleMs);- const maxLoaders = Math.max(1, parseInt(process.env['FACET_MAX_SSR_LOADERS'] ?? '4', 10));
+ const parsedMaxLoaders = parseInt(process.env['FACET_MAX_SSR_LOADERS'] ?? '4', 10);
+ const maxLoaders = Math.max(1, Number.isNaN(parsedMaxLoaders) ? 4 : parsedMaxLoaders);Also applies to: 112-119
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for 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.
In `@cli/src/bundler/ssr-pool.ts` around lines 70 - 75, Validate the parsed
environment values in scheduleIdleShutdown and the loader-cap logic before
applying Math.max or comparing against the limit. If FACET_SSR_LOADER_IDLE_MS or
FACET_MAX_SSR_LOADERS is missing, non-numeric, or otherwise invalid, fall back
to the existing defaults (300000 and 4 respectively), while preserving the
existing minimum bounds.
| export function runPersistentSsrLoader( | ||
| request: PersistentLoaderRequest, | ||
| logger: Logger, | ||
| ): Promise<PersistentLoaderResult> { | ||
| let loader = loaders.get(request.facetRoot); | ||
| if (!loader) { | ||
| const maxLoaders = Math.max(1, parseInt(process.env['FACET_MAX_SSR_LOADERS'] ?? '4', 10)); | ||
| if (loaders.size >= maxLoaders) { | ||
| const oldest = loaders.entries().next().value as [string, SsrLoaderProcess] | undefined; | ||
| if (oldest) { | ||
| loaders.delete(oldest[0]); | ||
| void oldest[1].close(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
LRU eviction can kill a loader with in-flight requests.
request() picks the oldest map entry once FACET_MAX_SSR_LOADERS is reached and closes it unconditionally. If that loader is currently servicing a request, close() ends stdin/kills the process and fail() rejects the still-pending request — a legitimate in-flight render fails just because a new, unrelated facetRoot needed a slot.
Consider skipping eviction of loaders with outstanding pending requests (expose a hasPending()/pendingCount accessor on SsrLoaderProcess and prefer an idle candidate, or reject the new request with a clear "pool exhausted" error instead of killing active work).
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for 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.
In `@cli/src/bundler/ssr-pool.ts` around lines 112 - 125, Update
runPersistentSsrLoader and SsrLoaderProcess so eviction never closes a loader
with outstanding requests: expose and use a pending-request status such as
hasPending() or pendingCount, select the oldest idle loader for eviction, and if
none is idle reject the new request with a clear pool-exhausted error instead of
terminating active work.
| persistentLoader = true, | ||
| ): Promise<string> { | ||
| await writeFile(join(consumerRoot, filename), code, 'utf-8'); | ||
| const fragmentDir = join(consumerRoot, '.facet-fragments'); | ||
| const contentKey = createHash('sha256').update(code).digest('hex').slice(0, 20); | ||
| const fragmentPath = join('.facet-fragments', `${filename.replace(/\.tsx$/, '')}-${contentKey}.tsx`); | ||
| await mkdir(fragmentDir, { recursive: true }); | ||
| await writeFile(join(consumerRoot, fragmentPath), code, 'utf-8'); | ||
| const result = await buildTemplate({ | ||
| templatePath: filename, | ||
| templatePath: fragmentPath, | ||
| data, | ||
| consumerRoot, | ||
| logger, | ||
| persistentLoader, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the workspace/cache flow.
git ls-files | rg '(^|/)(cli/src/server/routes\.ts|.*workspace.*|.*cache.*|.*template.*)$' || true
printf '\n--- routes.ts outline ---\n'
ast-grep outline cli/src/server/routes.ts --view expanded || true
printf '\n--- search for fragment/cache helpers ---\n'
rg -n "facet-fragments|acquireTemplateWorkspace|cleanupTempDir|pruneTailwindCache|pruneWorkspaces|FACET_WORKSPACE_CACHE_ENTRIES|FACET_TAILWIND_CACHE_ENTRIES|buildFragment|resolveTemplateSource" cli/src/server/routes.ts cli/src || trueRepository: flanksource/facet
Length of output: 6891
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- template-workspaces.ts ---\n'
cat -n cli/src/server/template-workspaces.ts | sed -n '1,220p'
printf '\n--- relevant routes.ts sections ---\n'
sed -n '322,530p' cli/src/server/routes.ts | cat -n
printf '\n--- build-cache.ts ---\n'
cat -n cli/src/bundler/build-cache.ts | sed -n '1,220p'Repository: flanksource/facet
Length of output: 15660
Add a prune step for .facet-fragments
acquireTemplateWorkspace() only bounds the number of workspace roots; a hot workspace can keep accumulating content-hashed header/footer files under .facet-fragments with no per-workspace cap. Add entry-count or age-based pruning here, similar to pruneWorkspaces()/pruneTailwindCache().
🤖 Prompt for 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.
In `@cli/src/server/routes.ts` around lines 457 - 469, Add per-workspace pruning
for `.facet-fragments` within acquireTemplateWorkspace after writing the
content-hashed fragment, using the existing pruneWorkspaces or
pruneTailwindCache pattern and an entry-count or age limit. Keep current
fragment creation and buildTemplate behavior unchanged while ensuring stale or
excess fragment files are removed.
| await withTimeout(document.fonts.ready); | ||
| await withTimeout(Promise.all( | ||
| Array.from(document.images, async (image) => { | ||
| if (!image.complete) { | ||
| await new Promise<void>((resolve) => { | ||
| image.addEventListener('load', () => resolve(), { once: true }); | ||
| image.addEventListener('error', () => resolve(), { once: true }); | ||
| }); | ||
| } | ||
| if (typeof image.decode === 'function') { | ||
| await image.decode().catch(() => undefined); | ||
| } | ||
| }), | ||
| )); | ||
|
|
||
| if (facet) { | ||
| const ready = (window as typeof window & { __FACET_READY__?: Promise<unknown> }).__FACET_READY__; | ||
| if (ready && typeof ready.then === 'function') await withTimeout(ready); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Apply timeoutMs to the entire readiness phase.
Fonts, images, and __FACET_READY__ each receive a fresh timeout, so the default 30-second setting can take 90 seconds here—or 120 seconds including setContent. Run all readiness tasks under one deadline/shared remaining budget.
🤖 Prompt for 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.
In `@cli/src/utils/browser-readiness.ts` around lines 36 - 53, Update the
readiness flow containing document.fonts.ready, image loading/decoding, and
__FACET_READY__ so timeoutMs applies to the entire phase through one shared
deadline or remaining-time budget rather than creating a fresh timeout for each
await. Preserve the existing readiness tasks and timeout behavior, and ensure
the budget is consumed across them collectively.
| const typeInfo = await detectPageTypes(page, options.defaultPageSize) ?? await detectMixedSizes(page); | ||
| let result: Buffer; | ||
| if (typeInfo != null) { | ||
| log.info(`Multi-pass mode: ${typeInfo.definitions.size} type(s), ${typeInfo.pageSizes.length} pages`); | ||
| await page.close(); | ||
| result = await renderMultiPass(browser, html, typeInfo, log, debug, outputPath, debugTypography); | ||
| result = await renderMultiPass( | ||
| context, html, typeInfo, log, options.debug, | ||
| options.debugOutputPath, options.debugTypography, | ||
| ); | ||
| } else { | ||
| log.info('Single-pass mode (no typed headers/footers)'); | ||
| result = await renderSinglePass(browser, html, page, log, debug, landscape, defaultPageSize, outputPath, margins, debugTypography); | ||
| await page.close(); | ||
| result = await renderSinglePass( | ||
| context, html, page, log, options.debug, options.landscape, | ||
| options.defaultPageSize, options.debugOutputPath, options.margins, | ||
| options.debugTypography, | ||
| ); | ||
| } | ||
|
|
||
| if (debugTypography) { | ||
| result = Buffer.from(await appendDebugFontPage(result, fontSize, fontCombos)); | ||
| if (options.debugTypography) { | ||
| result = Buffer.from(await appendDebugFontPage(result, options.fontSize, fontCombos)); | ||
| } | ||
| result = await stampPDFMetadata(result); | ||
| writeFileSync(outputPath, result); | ||
| log.debug(`PDF saved to: ${outputPath}`); | ||
| return result; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
git ls-files 'cli/src/utils/pdf-generator.ts' 'cli/src/utils/*' | sed -n '1,20p'
echo '--- outline ---'
ast-grep outline cli/src/utils/pdf-generator.ts --view expanded
echo '--- metadata refs ---'
rg -n "creator|producer|metadata|compositeHeaderFooter|renderSinglePass|page\.pdf|appendDebugFontPage" cli/src/utils/pdf-generator.tsRepository: flanksource/facet
Length of output: 3310
🏁 Script executed:
set -euo pipefail
sed -n '1,260p' cli/src/utils/pdf-generator.ts | cat -nRepository: flanksource/facet
Length of output: 11935
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('cli/src/utils/pdf-generator.ts')
text = p.read_text()
for needle in ['async function renderSinglePass', 'async function renderMultiPass', 'async function compositeHeaderFooter']:
idx = text.find(needle)
print(f'=== {needle} @ {idx} ===')
if idx != -1:
start = max(0, idx - 800)
end = min(len(text), idx + 4000)
snippet = text[start:end]
for i, line in enumerate(snippet.splitlines(), 1):
print(f'{i:4}: {line}')
print()
PYRepository: flanksource/facet
Length of output: 11247
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('cli/src/utils/pdf-generator.ts')
lines = p.read_text().splitlines()
targets = ['async function renderSinglePass', 'async function renderMultiPass', 'async function compositeHeaderFooter']
for t in targets:
for i,l in enumerate(lines):
if t in l:
start = max(0, i-30)
end = min(len(lines), i+140)
print(f'\n### {t} at line {i+1}\n')
for j in range(start, end):
print(f'{j+1:4}: {lines[j]}')
break
PYRepository: flanksource/facet
Length of output: 16275
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('cli/src/utils/pdf-generator.ts')
lines = p.read_text().splitlines()
for start in [416, 460, 520]:
end = min(len(lines), start+140)
print(f'\n### lines {start}-{end}\n')
for j in range(start-1, end):
print(f'{j+1:4}: {lines[j]}')
PYRepository: flanksource/facet
Length of output: 11888
🏁 Script executed:
set -euo pipefail
echo '--- outline pdf-multipass.ts ---'
ast-grep outline cli/src/utils/pdf-multipass.ts --view expanded
echo '--- metadata refs ---'
rg -n "creator|producer|setCreator|setProducer|PDFDocument.load|save\\(|compositeHeaderFooter" cli/src/utils/pdf-multipass.tsRepository: flanksource/facet
Length of output: 4677
Keep PDF metadata on the zero-margin fast path.
renderSinglePass returns raw page.pdf() bytes when all margins are zero, so it skips the creator/producer stamping applied in compositeHeaderFooter. Apply the same metadata update before returning there. cli/src/utils/pdf-generator.ts:336-356
🤖 Prompt for 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.
In `@cli/src/utils/pdf-generator.ts` around lines 485 - 506, Update the
zero-margin fast path inside renderSinglePass to apply the same creator/producer
PDF metadata stamping performed by compositeHeaderFooter before returning
page.pdf() bytes. Preserve the existing behavior for non-zero margins and ensure
the metadata update occurs before any subsequent debug typography processing in
the caller.
| async function pruneTailwindCache(cacheDir: string): Promise<void> { | ||
| const maxEntries = Math.max(1, parseInt(process.env['FACET_TAILWIND_CACHE_ENTRIES'] ?? '50', 10)); | ||
| const names = (await readdir(cacheDir)).filter((name) => name.endsWith('.css')); | ||
| if (names.length <= maxEntries) return; | ||
| const entries = await Promise.all(names.map(async (name) => { | ||
| const path = join(cacheDir, name); | ||
| return { path, mtimeMs: (await stat(path)).mtimeMs }; | ||
| })); | ||
| entries.sort((a, b) => b.mtimeMs - a.mtimeMs); | ||
| await Promise.all(entries.slice(maxEntries).map((entry) => rm(entry.path, { force: true }))); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unguarded stat failure in pruneTailwindCache can discard freshly-generated CSS.
readdir/stat aren't wrapped in a try/catch, so a concurrent delete of a cache file (e.g. from a parallel prune by another process/request) throws here. Since pruneTailwindCache() is awaited before return css; in runTailwindCached (line 139), that exception propagates out and causes the just-generated CSS to be discarded — applyTailwind then silently falls back to non-cached CSS, wasting the Tailwind run. The sibling pruneWorkspaces() in cli/src/server/template-workspaces.ts guards the equivalent readdir+stat block with try { ... } catch { return; } for exactly this race.
🛠️ Proposed fix
async function pruneTailwindCache(cacheDir: string): Promise<void> {
const maxEntries = Math.max(1, parseInt(process.env['FACET_TAILWIND_CACHE_ENTRIES'] ?? '50', 10));
- const names = (await readdir(cacheDir)).filter((name) => name.endsWith('.css'));
- if (names.length <= maxEntries) return;
- const entries = await Promise.all(names.map(async (name) => {
- const path = join(cacheDir, name);
- return { path, mtimeMs: (await stat(path)).mtimeMs };
- }));
+ let entries;
+ try {
+ const names = (await readdir(cacheDir)).filter((name) => name.endsWith('.css'));
+ if (names.length <= maxEntries) return;
+ entries = await Promise.all(names.map(async (name) => {
+ const path = join(cacheDir, name);
+ return { path, mtimeMs: (await stat(path)).mtimeMs };
+ }));
+ } catch {
+ return;
+ }
entries.sort((a, b) => b.mtimeMs - a.mtimeMs);
await Promise.all(entries.slice(maxEntries).map((entry) => rm(entry.path, { force: true })));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function pruneTailwindCache(cacheDir: string): Promise<void> { | |
| const maxEntries = Math.max(1, parseInt(process.env['FACET_TAILWIND_CACHE_ENTRIES'] ?? '50', 10)); | |
| const names = (await readdir(cacheDir)).filter((name) => name.endsWith('.css')); | |
| if (names.length <= maxEntries) return; | |
| const entries = await Promise.all(names.map(async (name) => { | |
| const path = join(cacheDir, name); | |
| return { path, mtimeMs: (await stat(path)).mtimeMs }; | |
| })); | |
| entries.sort((a, b) => b.mtimeMs - a.mtimeMs); | |
| await Promise.all(entries.slice(maxEntries).map((entry) => rm(entry.path, { force: true }))); | |
| } | |
| async function pruneTailwindCache(cacheDir: string): Promise<void> { | |
| const maxEntries = Math.max(1, parseInt(process.env['FACET_TAILWIND_CACHE_ENTRIES'] ?? '50', 10)); | |
| let entries; | |
| try { | |
| const names = (await readdir(cacheDir)).filter((name) => name.endsWith('.css')); | |
| if (names.length <= maxEntries) return; | |
| entries = await Promise.all(names.map(async (name) => { | |
| const path = join(cacheDir, name); | |
| return { path, mtimeMs: (await stat(path)).mtimeMs }; | |
| })); | |
| } catch { | |
| return; | |
| } | |
| entries.sort((a, b) => b.mtimeMs - a.mtimeMs); | |
| await Promise.all(entries.slice(maxEntries).map((entry) => rm(entry.path, { force: true }))); | |
| } |
🤖 Prompt for 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.
In `@cli/src/utils/tailwind.ts` around lines 78 - 88, Guard the cache-directory
listing and file metadata collection in pruneTailwindCache with a try/catch,
returning without pruning when readdir or stat encounters a concurrent deletion
or other filesystem error. Keep the existing sorting and removal behavior for
successful reads, and ensure pruning failures do not propagate through
runTailwindCached.
| export async function runTailwindCached(opts: CachedTailwindOptions): Promise<string> { | ||
| const classSet = renderedClassKey(opts.html); | ||
| const cacheDir = join(opts.facetRoot, 'tailwind-cache'); | ||
| const cachePath = join(cacheDir, `${opts.buildCacheKey}-${classSet.key}.css`); | ||
| const existing = tailwindLocks.get(cachePath); | ||
| if (existing) return existing; | ||
|
|
||
| const operation = (async () => { | ||
| try { | ||
| return await readFile(cachePath, 'utf-8'); | ||
| } catch { /* cache miss */ } | ||
|
|
||
| await mkdir(cacheDir, { recursive: true }); | ||
| const nonce = `${process.pid}-${randomUUID()}`; | ||
| const contentPath = join(cacheDir, `${nonce}.html`); | ||
| const outputPath = join(cacheDir, `${nonce}.css`); | ||
| try { | ||
| await writeFile(contentPath, classSet.content, 'utf-8'); | ||
| await runTailwind({ | ||
| facetRoot: opts.facetRoot, | ||
| stylesInput: opts.stylesInput, | ||
| contentPath, | ||
| outputCssPath: outputPath, | ||
| verbose: opts.verbose, | ||
| }); | ||
| const css = await readFile(outputPath, 'utf-8'); | ||
| try { | ||
| await rename(outputPath, cachePath); | ||
| } catch { | ||
| // A separate process may have populated the same content-addressed key. | ||
| await rm(outputPath, { force: true }); | ||
| } | ||
| await pruneTailwindCache(cacheDir); | ||
| return css; | ||
| } finally { | ||
| await rm(contentPath, { force: true }); | ||
| await rm(outputPath, { force: true }); | ||
| } | ||
| })(); | ||
|
|
||
| tailwindLocks.set(cachePath, operation); | ||
| try { | ||
| return await operation; | ||
| } finally { | ||
| if (tailwindLocks.get(cachePath) === operation) tailwindLocks.delete(cachePath); | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Cache hits don't bump mtime, so pruneTailwindCache's LRU sort is based on creation time, not last-use.
Line 116's readFile(cachePath, ...) on a cache hit never touches the file's mtime, so a frequently-reused entry can still look "old" relative to entries that were merely regenerated recently, and get evicted ahead of truly cold entries. RenderCache.lookup() (cli/src/server/render-cache.ts) and ensureWorkspace() (cli/src/server/template-workspaces.ts) both explicitly utimes/utimesSync the file on hit for this reason.
🛠️ Proposed fix
const operation = (async () => {
try {
- return await readFile(cachePath, 'utf-8');
+ const css = await readFile(cachePath, 'utf-8');
+ const now = new Date();
+ await utimes(cachePath, now, now).catch(() => undefined);
+ return css;
} catch { /* cache miss */ }(Requires importing utimes from node:fs/promises.)
🤖 Prompt for 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.
In `@cli/src/utils/tailwind.ts` around lines 107 - 153, Update the cache-hit path
in runTailwindCached to refresh cachePath’s access and modification timestamps
after successfully reading the cached CSS, using the node:fs/promises utimes
API; preserve the existing read-and-return behavior and allow timestamp refresh
errors to propagate consistently.
1 participant
Summary by CodeRabbit
New Features
Documentation
Bug Fixes