Walkthrough

Switches upload handling from a single global lock to per-bundle locks (derived from getRequestBundleFilePath), implements per-bundle copy flows with aggregate settling and standardized lock errors, updates render-path aggregation to await all bundle operations, and expands tests to validate cross-endpoint coordination.

Changes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant UploadHandler as Upload (/worker.ts)
  participant RenderHandler as Render (/handleRenderRequest.ts)
  participant LockMgr as LockManager (locks.ts)
  participant FS as FileSystem

  Note over Client,UploadHandler: Upload flow
  Client->>UploadHandler: POST /upload-assets (bundleTimestamp)
  UploadHandler->>LockMgr: getRequestBundleFilePath(ts) -> acquire lock(bundlePath)
  alt lock acquired
    UploadHandler->>FS: copyUploadedAssets -> write files to bundlePath
    FS-->>UploadHandler: copy complete
    UploadHandler->>LockMgr: release lock(bundlePath)
    UploadHandler-->>Client: 200 OK
  else lock failed
    LockMgr-->>UploadHandler: lock error
    UploadHandler-->>Client: error
  end

  Note over Client,RenderHandler: Render-request flow (may target same bundle)
  Client->>RenderHandler: POST /bundles/... (bundleTimestamp)
  RenderHandler->>LockMgr: getRequestBundleFilePath(ts) -> acquire lock(bundlePath)
  alt lock acquired
    RenderHandler->>FS: copyUploadedAssets / write render-request files
    FS-->>RenderHandler: copy complete
    RenderHandler->>LockMgr: release lock(bundlePath)
    RenderHandler-->>Client: 200 OK
  else lock failed
    LockMgr-->>RenderHandler: lock error
    RenderHandler-->>Client: error
  end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I hopped from lock to lock and found,
Each bundle fenced within its ground,
Copies wait their proper turn,
No scrambled files, no midnight burn —
Hooray, the directories are sound! 🥕✨

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 @coderabbitai help to get the list of available commands and usage tips.}

Greptile Summary

This PR replaces the global transferring-assets lock with per-bundle locks using getRequestBundleFilePath as the lock key, matching the locking strategy in handleRenderRequest. This prevents race conditions when /upload-assets and render requests write to the same bundle directory concurrently, while allowing different bundles to be processed in parallel.

Key changes:

• Removed global transferring-assets lock that serialized all upload requests
• Implemented per-bundle locking using getRequestBundleFilePath(bundleTimestamp) as lock key
• Each bundle in targetBundles now acquires its own lock independently via Promise.all
• Lock acquisition and release wrapped in try-finally for proper cleanup
• Moved log.info(taskDescription) outside the lock acquisition to avoid unnecessary logging before validation

The implementation correctly handles lock failures by throwing and stopping execution, while allowing successful locks to be released in their respective finally blocks.

Confidence Score: 4/5

• This PR is safe to merge with low risk, pending test verification
• The implementation correctly replaces global locking with per-bundle locking to prevent cross-endpoint race conditions. Lock acquisition/release logic is sound with proper try-finally cleanup. Score is 4/5 (not 5) because: (1) the PR description mentions adding a cross-endpoint race condition test but no test changes are included in this commit, and (2) existing test cleanup code references the old lock file name (though this won't cause test failures, just ineffective cleanup)
• No files require special attention - the implementation is straightforward and correct

Important Files Changed

Sequence Diagram

sequenceDiagram
    participant Upload as /upload-assets
    participant Render as /render
    participant Lock as Lock System
    participant FS as File System

    Note over Upload,Render: Concurrent requests to same bundle

    Upload->>Lock: lock(bundle-123.js)
    activate Lock
    Lock-->>Upload: acquired
    
    Render->>Lock: lock(bundle-123.js)
    Note right of Lock: Waits - same bundle
    
    Upload->>FS: copy assets to bundle-123/
    Upload->>Lock: unlock(bundle-123.js)
    deactivate Lock
    
    activate Lock
    Lock-->>Render: acquired
    Render->>FS: write bundle-123/bundle-123.js
    Render->>Lock: unlock(bundle-123.js)
    deactivate Lock

    Note over Upload,Render: Different bundles run in parallel

    Upload->>Lock: lock(bundle-456.js)
    activate Lock
    Lock-->>Upload: acquired
    
    Render->>Lock: lock(bundle-789.js)
    activate Lock
    Lock-->>Render: acquired (different bundle)
    
    par Parallel execution
        Upload->>FS: copy assets to bundle-456/
        Upload->>Lock: unlock(bundle-456.js)
        deactivate Lock
    and
        Render->>FS: write bundle-789/bundle-789.js
        Render->>Lock: unlock(bundle-789.js)
        deactivate Lock
    end

_{Last reviewed commit: 53f6ce9}

Review: Per-bundle lock for /upload-assets

The core idea is sound: using getRequestBundleFilePath as the shared lock key for both /upload-assets and handleRenderRequest ensures that asset copies and bundle writes to the same directory are mutually exclusive. The change also improves throughput for independent bundles, since different bundle directories are no longer serialized behind a single global lock.

That said, there are a few issues to address before merging.

1. Partial-failure semantics with Promise.all (medium)

With the new code, when targetBundles contains multiple entries and one bundle fails to acquire its lock, Promise.all(copyPromises) rejects and the endpoint returns an error. But bundles that acquired their locks earlier will already have had their assets copied and their locks released before the rejection propagates. The caller receives a 400 and likely retries the entire upload, not knowing which bundles already succeeded.

The old global-lock approach didn't have this problem — if the single lock wasn't acquired, nothing was done; if it was, everything ran together. The new code is better for the common (non-contention) case, but the partial-failure path is harder to reason about for callers. At a minimum, the error response should identify which bundles succeeded and which failed so callers can act appropriately, or the implementation should accumulate errors and still attempt all bundles before responding.

2. Cross-endpoint race condition test is missing (medium)

The PR's own test plan calls for:

Add cross-endpoint race condition test verifying /upload-assets and render requests coordinate via the shared lock

This is the central correctness property being introduced and it isn't tested. The existing tests in uploadRaceCondition.test.ts only verify isolation between concurrent requests to the same endpoint with different bundles — which was already working before this PR. A test that races a concurrent /upload-assets call and a /bundles/:ts/render/... call to the same bundle timestamp is needed to actually validate the fix.

3. Stale cleanup code in test teardown (minor)

afterEach still deletes transferring-assets.lock from the working directory. That global lock file no longer exists in the new implementation, so this line is dead code.

4. Outdated comment in test file (minor)

The comment block above the /upload-assets test suite still describes the old 'transferring-assets' global lock and its failure mode. This should be updated to reflect the per-bundle lock approach introduced by this PR.

Review: Use shared per-bundle locks for asset transfer and render

The core idea is sound and a clear improvement: replacing the global transferring-assets lock with per-bundle locks using the same key (getRequestBundleFilePath) as handleRenderRequest is the right fix for #2463. Different bundles can now proceed concurrently while same-bundle operations across endpoints are correctly serialized. The new cross-endpoint test is a good addition.

Issues found

1. Partial-success inconsistency with multi-bundle requests (moderate)

When targetBundles has N entries, all N per-bundle operations run concurrently via Promise.all. If bundle K fails to acquire its lock (after all retries), Promise.all rejects and the outer catch sends a 400 response — but bundles 1..K-1 that already hold their locks continue copying assets in the background. The client receives an error even though some bundles are being (or have been) written.

In practice this is rare (lock failures require ~13.5 s of contention), and a retry with overwrite: true semantics would recover, but the server state is inconsistent with the error response. Consider Promise.allSettled plus per-bundle error reporting, or sequential processing, and at minimum add a comment documenting the trade-off. The same pattern exists in handleNewBundlesProvided in handleRenderRequest.ts.

2. Double-embedded taskDescription in error logs (minor)

When a lock fails, formatExceptionMessage(taskDescription, ...) already includes taskDescription in its output. That error is thrown, caught, and re-wrapped in the outer catch, causing taskDescription (the bundle list) to appear twice in the logged message.

Additionally, formatExceptionMessage's first parameter is semantically "JS rendering request code"; passing a human-readable task string produces the misleading label "JS code for rendering request was: Uploading files ..." in logs. (Pre-existing in the old code, but worth addressing.)

3. Inconsistent lock-failure pattern vs handleRenderRequest.ts (minor)

handleNewBundleProvided returns errorResponseResult(msg) on lock failure; this PR throws new Error(msg) and lets the outer catch convert it. Both produce a 400, but the two patterns create an asymmetry that makes the codebase harder to reason about. Aligning on one approach would help.

Positive notes

• Removing the lockAcquired flag in favour of a direct try/finally around the acquired-lock path is cleaner.
• Removing the fileExistsAsync TOCTOU check before mkdir and relying on { recursive: true } idempotency is the right call.
• The addBarrier upgrade to accept string | string[] is a clean, non-breaking generalisation.
• The new cross-endpoint test correctly exercises the shared-lock coordination path.

Responding to both Claude reviews:

First review (on commit before test update):

• "Cross-endpoint test is missing" → Added in the second commit (see uploadRaceCondition.test.ts)
• "afterEach still deletes transferring-assets.lock" → Removed in the second commit
• "Outdated comment in test file" → Updated in the second commit

These were all addressed before the review was posted (the first review was based on the first commit only).

Both reviews — Promise.all partial-success:
This is an acceptable trade-off, not a regression. The old code also had partial copy risk via Promise.all, and handleNewBundlesProvided in handleRenderRequest.ts uses the exact same pattern. Copies are idempotent, retries are safe, and lock-contention failures are extremely rare (~13.5s of sustained contention required).

formatExceptionMessage misuse + double taskDescription:
The formatExceptionMessage usage is pre-existing (identical to old code). The double-embedding is a minor side effect of the structural change but only affects the extremely rare lock-failure log path.

Inconsistency with handleRenderRequest.ts:
Different control flows — handleNewBundleProvided returns values for its caller to inspect; our code uses throw+catch because per-bundle lambdas don't return ResponseResult. Both produce identical 400 responses.

Review: Per-bundle lock sharing between /upload-assets and render requests

Overall Assessment

The approach is sound and addresses a real race condition (issue #2463). Using the same getRequestBundleFilePath key across both endpoints is the right fix, and the new cross-endpoint test meaningfully validates it. A few areas worth attention:

Issues

1. Double-wrapping of lock-failure error messages

When !wasLockAcquired, the code builds a formatted message via formatExceptionMessage and wraps it in new Error(msg). That error then gets caught by the outer catch and formatted again:

ERROR when trying to copy assets. Error: <already-formatted-msg>. Task: <taskDescription>

This produces a redundant, harder-to-read log entry. The original code just returned the formatExceptionMessage result directly.

2. Silent swallowing of non-first failures

Promise.allSettled collects all rejections, but only the first one is re-thrown:

const firstFailure = results.find((r): r is PromiseRejectedResult => r.status === 'rejected');
if (firstFailure) {
  throw firstFailure.reason;
}

If targetBundles has multiple entries and several fail, the subsequent failures are never logged. Consider logging all failures before re-throwing the first, or collecting a multi-error.

3. Inconsistency with handleNewBundlesProvided

The render path (handleRenderRequest.ts) uses Promise.all in handleNewBundlesProvided, while this PR uses Promise.allSettled for the upload path. The render path is safe because handleNewBundleProvided never rejects (returns error-response objects instead), but the asymmetry could confuse future maintainers — especially since the allSettled rationale (preventing uploadDir cleanup racing against in-flight copies) also conceptually applies to the render path's assetsToCopy. Worth a comment explaining the difference.

4. Test: cross-endpoint test doesn't assert lock serialization behaviour on timeout

The new test only covers the happy path (both requests succeed). A test for the scenario where one endpoint holds the lock long enough that the other's retry window expires would verify the error-response code path and confirm partial state (bundles already copied before the failure) is left in a consistent state.

Minor observations

• mkdir before lock — removing the fileExistsAsync check in favour of unconditional mkdir({ recursive: true }) is correct and simpler.
• allSettled rationale comment — the comment explaining why allSettled is used instead of Promise.all (to prevent premature uploadDir cleanup) is clear and useful; keep it.
• Barrier generalisation — the addBarrier change to accept string | string[] is a clean, non-breaking extension.
• Lock file lives inside the bundle directory — lock(bundleFilePath) creates bundleFilePath.lock inside the bundle directory, so mkdir must run first. The PR correctly orders these; just confirming the implicit requirement is met.

Review Summary

The core idea — replacing the global transferring-assets lock with per-bundle locks keyed on getRequestBundleFilePath — is correct and well-motivated. It fixes the race described in #2463 and improves throughput by allowing different bundles to be handled concurrently. The Promise.allSettled fix in worker.ts is the right call there (since lock() can throw on failure, Promise.all would short-circuit and leave in-flight copies reading from a directory that the onResponse hook might concurrently delete).

A few issues worth addressing before merge:

Functional issues

1. Silent error loss (both files) — when Promise.allSettled collects multiple rejections, the code re-throws only firstFailure.reason and discards the rest. If several bundles fail concurrently, only one error surface to logs/callers. Aggregating failures would make production debugging much easier.

2. Misleading comment in handleRenderRequest.ts — the comment says Promise.allSettled is needed because Promise.all would "short-circuit" and leave copies running. However, handleNewBundleProvided never throws (all errors are caught internally and returned as ResponseResult), so Promise.all would already wait for every promise. The allSettled change is a reasonable defensive move but the comment misstates why.

3. Double-wrapped error messages in worker.ts — lock-failure errors are already formatted by formatExceptionMessage, then wrapped in new Error(msg), and then wrapped again by the outer catch block template literal which also repeats taskDescription. The resulting log messages are verbose and redundant.

Test coverage gap

4. Cross-endpoint lock test doesn't exercise the actual race — the new test writes different filenames from each endpoint (render-asset.json vs upload-asset.json), so they can't corrupt each other even without the lock. The test verifies that both operations complete without error, but it doesn't validate that the lock actually prevents torn writes to a shared file. A stronger test would have both operations upload an asset with the same filename to the same bundle directory and assert the final file is coherent.

Minor / style

• The mkdir(bundleDirectory) call happens before the lock is acquired in worker.ts. This is safe (idempotent), but documenting the intentional ordering would prevent future readers from moving it inside the lock inadvertently.

Review: Per-bundle locking for asset transfer and render

The core design is correct and well-motivated. Replacing the global transferring-assets lock with per-bundle locks keyed by getRequestBundleFilePath ensures mutual exclusion at the right granularity—between operations on the same bundle directory—while allowing unrelated bundles to run concurrently. The Promise.allSettled switch is the right tool here: it guarantees every in-flight copy finishes before the handler returns, preventing the cleanup race where onResponse deletes req.uploadDir while background copies are still reading from it.

A few issues worth addressing:

Partial bundle failures silently dropped

In the /upload-assets handler, when Promise.allSettled finds failures, only firstFailure.reason is thrown. If 3 out of 5 bundles fail to acquire their locks (or fail during copy), the response will report only the first error—the other failures are silently swallowed. The caller has no way to know which bundles actually failed or retry just those.

Aggregating all failure reasons (e.g. collecting all rejected reasons into one error message before throwing) would give operators enough information to diagnose multi-bundle upload failures without guessing.

handleNewBundlesProvided now throws—behavior change worth documenting

Previously the function always resolved (errors were returned as ResponseResult values, never thrown). With the allSettled + re-throw pattern the function can now throw, which changes its contract. The callers (handleRenderRequest's outer try/catch) handle it correctly today, but this is a non-obvious change for future contributors. A brief doc comment noting that the function may throw on unexpected per-bundle errors would help.

Error message has misleading framing in upload-assets

formatExceptionMessage(taskDescription, errorMessage, ...) uses taskDescription as the first positional argument, which the helper labels as "JS code for rendering request was". The output will read JS code for rendering request was: Uploading files [...] to bundle directories: ... which is misleading in an upload-assets context. This predates the PR but is propagated into the new lock-failure error path. A separate error formatting path (or an overloaded first-arg label) for upload operations would avoid confusion in logs.

Test coverage gap: concurrent /upload-assets to the same bundle

The existing tests cover concurrent uploads to different bundles and (new) cross-endpoint to the same bundle. There is no test for two concurrent /upload-assets requests both targeting the same bundle timestamp. With per-bundle locking this should serialize correctly, but verifying it would close the coverage gap and confirm that the second request's assets aren't corrupted by a race on the bundle directory.

The lock coordination logic, barrier refactoring to accept arrays of prefixes, and new cross-endpoint test are all well-done. The issues above are mostly about observability and test completeness rather than correctness of the race-condition fix itself.