Skip to content

Commit 9b30cc1

Browse files
AbanoubGhadbanclaude
AbanoubGhadban
and
claude
authored
Fix upload-assets endpoint duplicating bundles across directories (#2768)
Fix upload-assets endpoint duplicating bundles across directories The /upload-assets endpoint treated all uploaded files as a flat list and copied every file into every target bundle directory. When RSC support sends both server and RSC bundles, each bundle JS was duplicated in the other's directory. - Extract shared `extractBundlesAndAssets` helper that parses form body into bundles (bundle_<hash> keys) and shared assets, used by both the render and /upload-assets endpoints - Rewrite /upload-assets to reuse `handleNewBundlesProvided` from the render path, ensuring each bundle goes only to its own directory while shared assets are distributed to all directories - Always copy assets even when the bundle already exists (EEXIST), fixing the case where re-running the rake task skipped asset writes - Keep sending targetBundles from Ruby for backward compatibility with older node renderers; added TODO to remove at next breaking version - Rename renderingRequest to requestContext in bundle handler functions - Warn on empty bundle_ hash suffix instead of silently skipping Closes #2766 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent aa9a002 commit 9b30cc1

6 files changed

Lines changed: 129 additions & 108 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@ After a release, run `/update-changelog` in Claude Code to analyze commits, writ
2424

2525
### [Unreleased]
2626

27+
#### Fixed
28+
29+
- **[Pro] Fixed bundle duplication in remote node renderer asset uploads**: When RSC support is enabled, running `rake react_on_rails_pro:copy_assets_to_remote_vm_renderer` no longer duplicates bundle JS files across bundle directories. Previously, both the server bundle and RSC bundle were copied into every target directory; now each bundle is placed only in its own directory while shared assets (manifests, stats) are correctly distributed to all. [PR 2768](https://github.com/shakacode/react_on_rails/pull/2768) by [AbanoubGhadban](https://github.com/AbanoubGhadban). Fixes [Issue 2766](https://github.com/shakacode/react_on_rails/issues/2766).
30+
2731
### [16.5.0] - 2026-03-25
2832

2933
Stable release — no changes from 16.5.0.rc.0.

‎packages/react-on-rails-pro-node-renderer/src/worker.ts‎

Lines changed: 67 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
import path from 'path';
77
import cluster from 'cluster';
88
import { randomUUID } from 'crypto';
9-
import { mkdir, rm } from 'fs/promises';
9+
import { rm } from 'fs/promises';
1010
import fastify from 'fastify';
1111
import fastifyFormbody from '@fastify/formbody';
1212
import fastifyMultipart from '@fastify/multipart';
@@ -17,21 +17,20 @@ import fileExistsAsync from './shared/fileExistsAsync.js';
1717
import type { FastifyInstance, FastifyReply, FastifyRequest } from './worker/types.js';
1818
import checkProtocolVersion from './worker/checkProtocolVersionHandler.js';
1919
import authenticate from './worker/authHandler.js';
20-
import { handleRenderRequest, type ProvidedNewBundle } from './worker/handleRenderRequest.js';
20+
import {
21+
handleRenderRequest,
22+
handleNewBundlesProvided,
23+
type ProvidedNewBundle,
24+
} from './worker/handleRenderRequest.js';
2125
import handleGracefulShutdown from './worker/handleGracefulShutdown.js';
2226
import {
2327
errorResponseResult,
2428
formatExceptionMessage,
25-
copyUploadedAssets,
2629
ResponseResult,
27-
workerIdLabel,
2830
saveMultipartFile,
2931
Asset,
3032
getAssetPath,
31-
getBundleDirectory,
32-
getRequestBundleFilePath,
3333
} from './shared/utils.js';
34-
import { lock, unlock } from './shared/locks.js';
3534
import { startSsrRequestOptions, trace } from './shared/tracing.js';
3635

3736
// Uncomment the below for testing timeouts:
@@ -96,6 +95,42 @@ function assertAsset(value: unknown, key: string): asserts value is Asset {
9695
}
9796
}
9897

98+
/**
99+
* Parses the multipart form body to separate bundle files from shared assets.
100+
* Used by both the render and /upload-assets endpoints to avoid duplicating
101+
* bundle-vs-asset classification logic.
102+
*
103+
* @param body The parsed multipart request body.
104+
* @param primaryBundleTimestamp If provided, a field with key `"bundle"` is
105+
* treated as a bundle for this timestamp (render endpoint convention).
106+
*/
107+
function extractBundlesAndAssets(
108+
body: Record<string, unknown>,
109+
primaryBundleTimestamp?: string | number,
110+
): { providedNewBundles: ProvidedNewBundle[]; assetsToCopy: Asset[] } {
111+
const providedNewBundles: ProvidedNewBundle[] = [];
112+
const assetsToCopy: Asset[] = [];
113+
Object.entries(body).forEach(([key, value]) => {
114+
if (key === 'bundle' && primaryBundleTimestamp != null) {
115+
assertAsset(value, key);
116+
providedNewBundles.push({ timestamp: primaryBundleTimestamp, bundle: value });
117+
} else if (key.startsWith('bundle_')) {
118+
const timestamp = key.slice('bundle_'.length);
119+
if (!timestamp) {
120+
log.warn(
121+
'Received form field with key "bundle_" but no hash suffix — possible bug in the Ruby client',
122+
);
123+
} else {
124+
assertAsset(value, key);
125+
providedNewBundles.push({ timestamp, bundle: value });
126+
}
127+
} else if (isAsset(value)) {
128+
assetsToCopy.push(value);
129+
}
130+
});
131+
return { providedNewBundles, assetsToCopy };
132+
}
133+
99134
// Remove after this issue is resolved: https://github.com/fastify/light-my-request/issues/315
100135
let useHttp2 = true;
101136

@@ -271,19 +306,7 @@ export default function run(config: Partial<Config>) {
271306

272307
const { renderingRequest } = req.body;
273308
const { bundleTimestamp } = req.params;
274-
const providedNewBundles: ProvidedNewBundle[] = [];
275-
const assetsToCopy: Asset[] = [];
276-
Object.entries(req.body).forEach(([key, value]) => {
277-
if (key === 'bundle') {
278-
assertAsset(value, key);
279-
providedNewBundles.push({ timestamp: bundleTimestamp, bundle: value });
280-
} else if (key.startsWith('bundle_')) {
281-
assertAsset(value, key);
282-
providedNewBundles.push({ timestamp: key.replace('bundle_', ''), bundle: value });
283-
} else if (isAsset(value)) {
284-
assetsToCopy.push(value);
285-
}
286-
});
309+
const { providedNewBundles, assetsToCopy } = extractBundlesAndAssets(req.body, bundleTimestamp);
287310

288311
try {
289312
const dependencyBundleTimestamps = extractBodyArrayField(req.body, 'dependencyBundleTimestamps');
@@ -315,88 +338,47 @@ export default function run(config: Partial<Config>) {
315338

316339
// There can be additional files that might be required at the runtime.
317340
// Since the remote renderer doesn't contain any assets, they must be uploaded manually.
341+
// Bundle files use the form key convention "bundle_<hash>" and are placed in
342+
// their own directory; remaining assets are copied to every bundle directory.
318343
app.post<{
319-
Body: WithBodyArrayField<Record<string, Asset>, 'targetBundles'>;
344+
Body: Record<string, unknown>;
320345
}>('/upload-assets', async (req, res) => {
321346
if (!(await requestPrechecks(req, res))) {
322347
return;
323348
}
324-
const assets: Asset[] = Object.values(req.body).filter(isAsset);
325349

326-
// Handle targetBundles as either a string or an array
327-
const targetBundles = extractBodyArrayField(req.body, 'targetBundles');
328-
if (!targetBundles || targetBundles.length === 0) {
329-
const errorMsg = 'No targetBundles provided. As of protocol version 2.0.0, targetBundles is required.';
350+
const { providedNewBundles, assetsToCopy } = extractBundlesAndAssets(req.body);
351+
352+
if (providedNewBundles.length === 0) {
353+
const errorMsg =
354+
'No bundle_<hash> fields provided. ' +
355+
'The /upload-assets endpoint requires at least one bundle file with a "bundle_<hash>" form key.';
330356
log.error(errorMsg);
331357
await setResponse(errorResponseResult(errorMsg), res);
332358
return;
333359
}
334360

335-
const assetsDescription = JSON.stringify(assets.map((asset) => asset.filename));
336-
const taskDescription = `Uploading files ${assetsDescription} to bundle directories: ${targetBundles.join(', ')}`;
337-
361+
const bundleNames = providedNewBundles.map((b) => b.bundle.filename);
362+
const assetNames = assetsToCopy.map((a) => a.filename);
363+
const taskDescription = `Uploading bundles [${bundleNames.join(', ')}] with assets [${assetNames.join(', ')}]`;
338364
log.info(taskDescription);
339-
try {
340-
// Use per-bundle locks (same lock key as handleRenderRequest) so that
341-
// asset copies and render-request bundle writes to the same directory
342-
// are mutually exclusive. See https://github.com/shakacode/react_on_rails/issues/2463
343-
//
344-
// Use allSettled (not Promise.all) to ensure every in-flight copy
345-
// finishes before the handler returns. Otherwise the onResponse hook
346-
// can delete req.uploadDir while background copies still read from it.
347-
const copyPromises = targetBundles.map(async (bundleTimestamp) => {
348-
const bundleDirectory = getBundleDirectory(bundleTimestamp);
349-
await mkdir(bundleDirectory, { recursive: true });
350-
351-
const bundleFilePath = getRequestBundleFilePath(bundleTimestamp);
352-
const { lockfileName, wasLockAcquired, errorMessage } = await lock(bundleFilePath);
353-
354-
if (!wasLockAcquired) {
355-
const msg = formatExceptionMessage(
356-
taskDescription,
357-
errorMessage,
358-
`Failed to acquire lock ${lockfileName}. Worker: ${workerIdLabel()}.`,
359-
);
360-
throw new Error(msg);
361-
}
362365

363-
try {
364-
await copyUploadedAssets(assets, bundleDirectory);
365-
log.info(`Copied assets to bundle directory: ${bundleDirectory}`);
366-
} finally {
367-
try {
368-
await unlock(lockfileName);
369-
} catch (error) {
370-
log.warn({
371-
msg: `Error unlocking ${lockfileName} from worker ${workerIdLabel()}`,
372-
err: error,
373-
task: taskDescription,
374-
});
375-
}
376-
}
377-
});
378-
379-
const results = await Promise.allSettled(copyPromises);
380-
const firstFailure = results.find((r): r is PromiseRejectedResult => r.status === 'rejected');
381-
if (firstFailure) {
382-
throw firstFailure.reason;
366+
try {
367+
// Reuses the same per-bundle lock + move/copy logic as the render
368+
// endpoint so that concurrent /upload-assets and render requests
369+
// targeting the same bundle directory are mutually exclusive.
370+
// See https://github.com/shakacode/react_on_rails/issues/2463
371+
const result = await handleNewBundlesProvided(taskDescription, providedNewBundles, assetsToCopy);
372+
if (result) {
373+
await setResponse(result, res);
374+
return;
383375
}
384376

385-
await setResponse(
386-
{
387-
status: 200,
388-
headers: {},
389-
},
390-
res,
391-
);
377+
await setResponse({ status: 200, headers: {} }, res);
392378
} catch (err) {
393-
const msg = 'ERROR when trying to copy assets';
379+
const msg = 'ERROR when trying to upload bundles and assets';
394380
const message = `${msg}. ${err}. Task: ${taskDescription}`;
395-
log.error({
396-
msg,
397-
err,
398-
task: taskDescription,
399-
});
381+
log.error({ msg, err, task: taskDescription });
400382
await setResponse(errorResponseResult(message), res);
401383
}
402384
});

‎packages/react-on-rails-pro-node-renderer/src/worker/handleRenderRequest.ts‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ async function prepareResult(
7777
* @param assetsToCopy might be null
7878
*/
7979
async function handleNewBundleProvided(
80-
renderingRequest: string,
80+
requestContext: string,
8181
providedNewBundle: ProvidedNewBundle,
8282
assetsToCopy: Asset[] | null | undefined,
8383
): Promise<ResponseResult | undefined> {
@@ -95,7 +95,7 @@ async function handleNewBundleProvided(
9595

9696
if (!wasLockAcquired) {
9797
const msg = formatExceptionMessage(
98-
renderingRequest,
98+
requestContext,
9999
errorMessage,
100100
`Failed to acquire lock ${lockfileName}. Worker: ${workerIdLabel()}.`,
101101
);
@@ -107,18 +107,14 @@ async function handleNewBundleProvided(
107107
`Moving uploaded file ${providedNewBundle.bundle.savedFilePath} to ${bundleFilePathPerTimestamp}`,
108108
);
109109
await moveUploadedAsset(providedNewBundle.bundle, bundleFilePathPerTimestamp);
110-
if (assetsToCopy) {
111-
await copyUploadedAssets(assetsToCopy, bundleDirectory);
112-
}
113-
114110
log.info(
115111
`Completed moving uploaded file ${providedNewBundle.bundle.savedFilePath} to ${bundleFilePathPerTimestamp}`,
116112
);
117113
} catch (error) {
118114
const fileExists = await fileExistsAsync(bundleFilePathPerTimestamp);
119115
if (!fileExists) {
120116
const msg = formatExceptionMessage(
121-
renderingRequest,
117+
requestContext,
122118
error,
123119
`Unexpected error when moving the bundle from ${providedNewBundle.bundle.savedFilePath} \
124120
to ${bundleFilePathPerTimestamp})`,
@@ -132,6 +128,13 @@ to ${bundleFilePathPerTimestamp})`,
132128
);
133129
}
134130

131+
// Always copy assets to the bundle directory — even if the bundle was
132+
// already present (e.g., from a prior upload or another worker).
133+
// copyUploadedAssets uses overwrite:true, so this is idempotent.
134+
if (assetsToCopy) {
135+
await copyUploadedAssets(assetsToCopy, bundleDirectory);
136+
}
137+
135138
return undefined;
136139
} finally {
137140
if (lockAcquired && lockfileName) {
@@ -140,7 +143,7 @@ to ${bundleFilePathPerTimestamp})`,
140143
await unlock(lockfileName);
141144
} catch (error) {
142145
const msg = formatExceptionMessage(
143-
renderingRequest,
146+
requestContext,
144147
error,
145148
`Error unlocking ${lockfileName} from worker ${workerIdLabel()}.`,
146149
);
@@ -150,15 +153,15 @@ to ${bundleFilePathPerTimestamp})`,
150153
}
151154
}
152155

153-
async function handleNewBundlesProvided(
154-
renderingRequest: string,
156+
export async function handleNewBundlesProvided(
157+
requestContext: string,
155158
providedNewBundles: ProvidedNewBundle[],
156159
assetsToCopy: Asset[] | null | undefined,
157160
): Promise<ResponseResult | undefined> {
158161
log.info('Worker received new bundles: %s', providedNewBundles);
159162

160163
const handlingPromises = providedNewBundles.map((providedNewBundle) =>
161-
handleNewBundleProvided(renderingRequest, providedNewBundle, assetsToCopy),
164+
handleNewBundleProvided(requestContext, providedNewBundle, assetsToCopy),
162165
);
163166
// Defensive: use allSettled so that if handleNewBundleProvided ever throws
164167
// unexpectedly, all in-flight operations still complete before the handler

‎packages/react-on-rails-pro-node-renderer/tests/uploadRaceCondition.test.ts‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import formAutoContent from 'form-auto-content';
2323
// eslint-disable-next-line import/no-relative-packages
2424
import packageJson from '../package.json';
2525
import worker, { disableHttp2 } from '../src/worker';
26-
import { resetForTest, serverBundleCachePath, getFixtureBundle } from './helper';
26+
import { resetForTest, serverBundleCachePath, getFixtureBundle, getFixtureSecondaryBundle } from './helper';
2727

2828
const testName = 'uploadRaceCondition';
2929
const serverBundleCachePathForTest = () => serverBundleCachePath(testName);
@@ -176,14 +176,14 @@ describe('concurrent upload isolation (issue #2449)', () => {
176176
gemVersion,
177177
protocolVersion,
178178
railsEnv,
179-
targetBundles: [bundleHashA],
179+
[`bundle_${bundleHashA}`]: fs.createReadStream(getFixtureBundle()),
180180
asset1: fs.createReadStream(path.join(tmpDirA, 'loadable-stats.json')),
181181
});
182182
const formB = formAutoContent({
183183
gemVersion,
184184
protocolVersion,
185185
railsEnv,
186-
targetBundles: [bundleHashB],
186+
[`bundle_${bundleHashB}`]: fs.createReadStream(getFixtureBundle()),
187187
asset1: fs.createReadStream(path.join(tmpDirB, 'loadable-stats.json')),
188188
});
189189

@@ -282,15 +282,15 @@ describe('concurrent upload isolation (issue #2449)', () => {
282282
gemVersion,
283283
protocolVersion,
284284
railsEnv,
285-
targetBundles: [bundleHashA],
285+
[`bundle_${bundleHashA}`]: fs.createReadStream(getFixtureBundle()),
286286
asset1: fs.createReadStream(path.join(tmpDirA, 'loadable-stats.json')),
287287
asset2: fs.createReadStream(path.join(tmpDirA, 'manifest.json')),
288288
});
289289
const formB = formAutoContent({
290290
gemVersion,
291291
protocolVersion,
292292
railsEnv,
293-
targetBundles: [bundleHashB],
293+
[`bundle_${bundleHashB}`]: fs.createReadStream(getFixtureBundle()),
294294
asset1: fs.createReadStream(path.join(tmpDirB, 'loadable-stats.json')),
295295
asset2: fs.createReadStream(path.join(tmpDirB, 'manifest.json')),
296296
});
@@ -349,14 +349,14 @@ describe('concurrent upload isolation (issue #2449)', () => {
349349
gemVersion,
350350
protocolVersion,
351351
railsEnv,
352-
targetBundles: [sharedBundleHash],
352+
[`bundle_${sharedBundleHash}`]: fs.createReadStream(getFixtureBundle()),
353353
asset1: fs.createReadStream(path.join(tmpDirA, 'loadable-stats.json')),
354354
});
355355
const formB = formAutoContent({
356356
gemVersion,
357357
protocolVersion,
358358
railsEnv,
359-
targetBundles: [sharedBundleHash],
359+
[`bundle_${sharedBundleHash}`]: fs.createReadStream(getFixtureBundle()),
360360
asset1: fs.createReadStream(path.join(tmpDirB, 'loadable-stats.json')),
361361
});
362362

@@ -528,12 +528,12 @@ describe('concurrent upload isolation (issue #2449)', () => {
528528
asset1: fs.createReadStream(path.join(tmpDirA, 'loadable-stats.json')),
529529
});
530530

531-
// Upload-assets request: sends the same-named asset to the same bundle
531+
// Upload-assets request: sends bundle + the same-named asset to the same bundle
532532
const uploadForm = formAutoContent({
533533
gemVersion,
534534
protocolVersion,
535535
railsEnv,
536-
targetBundles: [bundleTimestamp],
536+
[`bundle_${bundleTimestamp}`]: fs.createReadStream(getFixtureBundle()),
537537
asset1: fs.createReadStream(path.join(tmpDirB, 'loadable-stats.json')),
538538
});
539539

0 commit comments

Comments
 (0)