Fix node renderer upload race condition with per-request directories#2456
Conversation
…2449) Concurrent requests uploading same-named files (e.g. loadable-stats.json) all wrote to a single shared path (uploads/<filename>), causing overwrites, ENOENT errors, and cross-contamination between requests. Each request now gets its own upload directory (uploads/<uuid>/), and the directory is cleaned up in an onResponse hook after the response is sent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Greptile SummaryFixes a critical race condition where concurrent multipart uploads overwrote each other's files in a shared Core changes:
Test coverage:
The fix is clean, well-tested, and directly addresses the root cause identified in the production postmortem. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client1 as Request A
participant Client2 as Request B
participant Fastify as Fastify Server
participant UploadA as uploads/UUID-A/
participant UploadB as uploads/UUID-B/
participant BundleA as bundle-A/
participant BundleB as bundle-B/
Note over Client1,BundleB: Before: Shared Upload Path (Race Condition)
Note over Client1,Client2: Both write to uploads/file.json → overwrite!
Note over Client1,BundleB: After: Per-Request Upload Directories
Client1->>Fastify: POST /upload-assets
activate Fastify
Fastify->>Fastify: onRequest: req.uploadDir = uploads/UUID-A/
Client2->>Fastify: POST /upload-assets
Fastify->>Fastify: onRequest: req.uploadDir = uploads/UUID-B/
Fastify->>UploadA: onFile: save to uploadDir/file.json
Fastify->>UploadB: onFile: save to uploadDir/file.json
Note over UploadA,UploadB: Files isolated - no collision!
Fastify->>BundleA: Handler: copy from UUID-A/ to bundle-A/
Fastify->>BundleB: Handler: copy from UUID-B/ to bundle-B/
Fastify-->>Client1: 200 OK
deactivate Fastify
Fastify->>UploadA: onResponse: rm uploads/UUID-A/
Fastify-->>Client2: 200 OK
Fastify->>UploadB: onResponse: rm uploads/UUID-B/
Last reviewed commit: 5e120cd |
Review: Fix node renderer upload race condition with per-request directoriesThe fix correctly identifies and addresses the root cause of the race condition. Assigning a UUID-based per-request upload directory via A few issues worth addressing before merging: Functional concern:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
size-limit report 📦
|
…e comment - Log warning instead of silently swallowing onResponse cleanup errors - Add runtime assertion in onFile to catch if @fastify/multipart changes this binding behavior - Rewrite test file header to describe post-fix invariant, not pre-fix state Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds per-request upload directories (UUID-named) set on Fastify requests; multipart onFile now writes to the request-specific directory via Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Fastify as Fastify
participant FS as Filesystem
participant Master as Master
rect rgba(135,206,235,0.5)
Client->>Fastify: POST /upload-assets (multipart)
note right of Fastify: onRequest sets this.uploadDir = <cache>/uploads/<uuid>/
Fastify->>FS: mkdir(this.uploadDir)
Fastify->>Fastify: multipart parsing (onFile uses this.uploadDir)
Fastify->>FS: write file -> this.uploadDir/filename
Fastify->>Fastify: acquire lock and process upload
Fastify->>FS: move/copy assets -> bundle directory
Fastify->>FS: rm -r this.uploadDir (onResponse)
Fastify-->>Client: 200 OK
end
rect rgba(240,230,140,0.5)
Master->>FS: periodic scan of <cache>/uploads/*
Master->>FS: rm -r old upload dirs
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
ReviewThe core fix is correct and well-motivated: per-request UUID upload directories eliminate the shared-path race condition cleanly, and the barrier-based test strategy makes the formerly non-deterministic race reproducible. A few items to address: Barrier function can deadlock (tests)If one of the two concurrent requests fails before reaching Previous review's "silent cleanup failure" note is incorrectThe await rm(req.uploadDir, { recursive: true, force: true }).catch((err: unknown) => {
log.warn({ msg: 'Failed to clean up per-request upload directory', uploadDir: req.uploadDir, err });
});This is correctly implemented.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-on-rails-pro-node-renderer/tests/uploadRaceCondition.test.ts (1)
62-76: Consider closing the Fastify app after each test.Each test creates a new Fastify instance via
worker()but never callsapp.close(). Whileinject()doesn't open sockets, Fastify's internal lifecycle (timers, plugin state) isn't torn down. This can cause "open handles" warnings from the test runner.One approach: hoist
appto aletatdescribescope and close it inafterEach:Suggested pattern
describe('concurrent upload isolation (issue `#2449`)', () => { let tmpDirA: string; let tmpDirB: string; + let app: ReturnType<typeof worker>; // ... beforeEach ... afterEach(async () => { + await app?.close(); await resetForTest(testName); // ... });Then assign
appin each test instead of declaring it withconst.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails-pro-node-renderer/tests/uploadRaceCondition.test.ts` around lines 62 - 76, Tests create Fastify instances via worker() but never close them, leaving timers/plugins open; hoist a let app to describe scope, assign the Fastify instance returned by worker() into that app in each test (replace local const app declarations), and call await app.close() in afterEach to properly tear down Fastify lifecycle; reference worker(), app.inject() and app.close() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-on-rails-pro-node-renderer/tests/uploadRaceCondition.test.ts`:
- Around line 62-76: Tests create Fastify instances via worker() but never close
them, leaving timers/plugins open; hoist a let app to describe scope, assign the
Fastify instance returned by worker() into that app in each test (replace local
const app declarations), and call await app.close() in afterEach to properly
tear down Fastify lifecycle; reference worker(), app.inject() and app.close()
when making the change.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review: Fix node renderer upload race condition with per-request directoriesThe core fix is correct and well-motivated. Assigning each request an isolated UUID upload directory eliminates the shared-path collision at the root of the ENOENT/corruption bug, and the barrier technique in the tests makes the race deterministic rather than relying on timing. Good work overall. A few items to address: Security: path traversal in uploaded filenames
const destinationPath = path.join(this.uploadDir, part.filename);If a client (or compromised Rails gem) sends a filename like Moderate:
|
…eteUploadedAssets The onResponse hook was redundant since deleteUploadedAssets already deletes individual files. Now the parent UUID directory is cleaned up in the same function, keeping the cleanup logic co-located with the file deletion. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review: Fix node renderer upload race condition with per-request directoriesThe root cause analysis is correct and the fix is conceptually sound — per-request UUID directories are the right approach for eliminating shared-path collisions. The test barrier technique is particularly clever. A few issues to address: Bug: UUID directory leaks when a bundle is uploaded without assetsIn the render endpoint, when a new bundle is provided but The simplest fix is to clean up Security: unsanitized
|
AbanoubGhadban
deleted the
2449-fix-node-renderer-upload-race-condition
branch
…2456) - **Root cause**: All concurrent multipart uploads wrote to a single shared path (`<serverBundleCachePath>/uploads/<filename>`), causing overwrites, ENOENT errors, and cross-contamination between requests. This was confirmed by a production postmortem showing `SyntaxError: Bad control character in string literal in JSON` during pod rollovers. - **Fix**: Each request now gets its own upload directory (`uploads/<randomUUID>/`) assigned in an `onRequest` hook. The `onFile` handler writes to the per-request directory. An `onResponse` hook cleans up the directory after the response is sent. - **Tests**: 3 new deterministic race condition tests using a `preHandler` barrier technique that forces concurrent requests' `onFile` phases to complete before any route handler runs. Tests cover `/upload-assets` (single asset, multiple assets) and `/bundles/:ts/render/:digest` endpoints. Closes #2449 - Added `declare module 'fastify'` augmentation for `uploadDir` on `FastifyRequest` - Added `decorateRequest('uploadDir', '')` + `onRequest` hook assigning `uploads/<randomUUID>/` - Added `onResponse` hook to clean up per-request upload directory - Changed `onFile` from arrow function to method function so `this` (the Fastify request) provides the per-request `uploadDir` `packages/react-on-rails-pro-node-renderer/tests/uploadRaceCondition.test.ts` (new) - Concurrent `/upload-assets` — single asset, different content, different target bundles - Concurrent `/upload-assets` — multiple assets, different content, different target bundles - Concurrent render requests — different bundle timestamps, same-named assets with different content - [x] All 3 new race condition tests pass (previously failed deterministically) - [x] All 17 existing `worker.test.ts` tests pass (no regressions) - [x] ESLint passes 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **Bug Fixes** * Per-request isolated upload directories to prevent cross-request asset contamination; automatic periodic cleanup of stale temporary upload folders. * Uploaded assets are no longer removed immediately after processing, avoiding accidental loss. * **Tests** * Added comprehensive tests validating concurrent upload and render request isolation across endpoints and bundle directories. * **Documentation** * Recorded the fix in the changelog under Unreleased. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Justin Gordon <justin808@users.noreply.github.com>
3 participants
Summary
<serverBundleCachePath>/uploads/<filename>), causing overwrites, ENOENT errors, and cross-contamination between requests. This was confirmed by a production postmortem showingSyntaxError: Bad control character in string literal in JSONduring pod rollovers.uploads/<randomUUID>/) assigned in anonRequesthook. TheonFilehandler writes to the per-request directory. AnonResponsehook cleans up the directory after the response is sent.preHandlerbarrier technique that forces concurrent requests'onFilephases to complete before any route handler runs. Tests cover/upload-assets(single asset, multiple assets) and/bundles/:ts/render/:digestendpoints.Closes #2449
Changes
packages/react-on-rails-pro-node-renderer/src/worker.tsdeclare module 'fastify'augmentation foruploadDironFastifyRequestdecorateRequest('uploadDir', '')+onRequesthook assigninguploads/<randomUUID>/onResponsehook to clean up per-request upload directoryonFilefrom arrow function to method function sothis(the Fastify request) provides the per-requestuploadDirpackages/react-on-rails-pro-node-renderer/tests/uploadRaceCondition.test.ts(new)/upload-assets— single asset, different content, different target bundles/upload-assets— multiple assets, different content, different target bundlesTest plan
worker.test.tstests pass (no regressions)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation