feat(local-cache): added optional local filesystem caching - #6
Conversation
IKatsuba
left a comment
There was a problem hiding this comment.
Nice abstraction — the route handlers got a lot cleaner. Two things to address before merging:
- Path traversal in
FilesystemBackend— thehashURL param is passed straight intopath.joinwith no validation. A request likePUT /v1/cache/..%2F..%2Fetc%2Fpasswdwill write outsidecacheDir. Auth helps, but this is still a sandbox escape, especially relevant when the cache dir is a mounted volume. - Unrelated
deno.lockchanges —npm:deno@^2.7.9and the newpackageJsonblock look like accidental local contamination. Please revert those — they're unrelated to this PR.
Also heads-up: the branch currently has merge conflicts with main (likely from #7 and #8 which landed recently). Please rebase.
Smaller things worth considering (non-blocking): Deno.writeFile isn't atomic — a crashed/concurrent write leaves a partial file that subsequent GETs will serve as a valid cache hit; FilesystemBackend.get reads the whole artifact into memory instead of streaming; an invalid NX_CACHE_BACKEND value silently falls back to S3 instead of erroring.
| constructor(private readonly cacheDir: string) {} | ||
|
|
||
| private filePath(key: string): string { | ||
| return join(this.cacheDir, key); |
There was a problem hiding this comment.
Path traversal. key comes from the URL param with no validation, and path.join normalizes .. but doesn't prevent escaping cacheDir. join('/tmp/nx-cache', '../../etc/passwd') resolves to /etc/passwd.
Suggested fix: validate the hash at the route level (/^[a-f0-9]+$/ or whatever shape nx hashes have), or guard here with path.resolve(filePath).startsWith(path.resolve(this.cacheDir) + path.SEPARATOR).
| "npm:@aws-sdk/client-s3@*": "3.779.0", | ||
| "npm:@aws-sdk/client-s3@^3.779.0": "3.779.0", | ||
| "npm:@aws-sdk/s3-request-presigner@^3.779.0": "3.779.0", | ||
| "npm:deno@^2.7.9": "2.7.9", |
There was a problem hiding this comment.
This (and the @deno/* platform binaries + packageJson block further down) looks like accidental contamination — deno shouldn't be an npm dependency of this project. Please revert the lockfile to only what this PR actually needs.
IKatsuba
left a comment
There was a problem hiding this comment.
Follow-up: inline threads for the nice-to-haves mentioned above.
|
|
||
| async put(key: string, data: Uint8Array): Promise<void> { | ||
| await Deno.mkdir(this.cacheDir, { recursive: true }); | ||
| await Deno.writeFile(this.filePath(key), data); |
There was a problem hiding this comment.
Non-atomic write. If two PUTs race on the same hash, or a write is interrupted (kill, OOM, disk full), you get a partial file. Subsequent GETs will serve it as a valid cache hit — silently corrupting nx builds, which is worse than a cache miss.
Suggestion: write to <hash>.tmp.<uuid> then Deno.rename into place. Rename on the same filesystem is atomic and also resolves the concurrent-PUT race.
|
|
||
| async get(key: string): Promise<Response> { | ||
| try { | ||
| const data = await Deno.readFile(this.filePath(key)); |
There was a problem hiding this comment.
Loads entire artifact into memory. The S3 backend streams via fetch(presigned); this one buffers the whole file. Nx cache entries can be hundreds of MB, so concurrent GETs will grow RSS linearly.
Suggestion: const file = await Deno.open(this.filePath(key)); return new Response(file.readable, { ... });
| c.set( | ||
| 's3', | ||
| new S3Client({ | ||
| const backend = c.env.NX_CACHE_BACKEND || 's3'; |
There was a problem hiding this comment.
Invalid values silently fall back to S3. A typo like NX_CACHE_BACKEND=fs or Filesystem won't error — it'll quietly run S3 mode, and the user will be confused why AWS_* vars are still required. Worth validating explicitly: accept only s3 | filesystem, throw otherwise.
Add filesystem storage backend
S3 is great for production, but it's overkill when you're running the cache server locally or in a CI environment where you just want artifacts written to disk. This PR adds a local filesystem backend as an alternative to S3.
What changed
A
StorageBackendinterface now sits between the route handlers and the underlying storage, with two implementations:S3Backend(the existing behaviour) andFilesystemBackend. The route handlers themselves got noticeably simpler as a result — they no longer care about S3-specific details.Two new environment variables control the filesystem backend:
NX_CACHE_BACKEND— set tofilesystemto opt in; defaults tos3so nothing changes for existing deploymentsNX_LOCAL_CACHE_DIR— where files are written; defaults to/tmp/nx-cacheTesting
Added four unit tests for the filesystem backend (PUT success, PUT conflict/idempotency, GET success, GET not found). Each test spins up an isolated temp directory and cleans up after itself, so no external dependencies are needed to run them.
The
--allow-writeDeno permission flag was also missing from thestart,dev, andtesttasks — added that too, since the filesystem backend obviously needs it.Notes for reviewers
S3Backendis just the same logic wrapped in the interface