fix(publish): Windows-safe slot symlink swap#187
Open
RemiAsselin42 wants to merge 2 commits into
Open
Conversation
POSIX `rename(2)` atomically replaces a directory symlink, but Win32 `MoveFile` refuses to overwrite an existing target (`EEXIST`/`EPERM`/`EISDIR`/`EACCES`), leaving `current` stuck on the old slot and `current.tmp` orphaned — so a publish never takes effect on a Windows dev machine. Add an unlink-then-rename fallback in `swapSlot`, plus a `removeSymlinkEntry` helper that removes a Windows directory-symlink link via `rmdir` (never recursing into the slot it points at). The fallback opens a sub-millisecond window where `current` is absent, which `readArtefact` already treats as a Layer A miss and serves via the Layer B live-render path, so a visitor never sees a 404. Production (Linux/Docker) always takes the atomic branch; this is a dev-on-Windows affordance only. The strict never-null assertion in `publishAtomicityRace` depends on atomic symlink rename, so it is now `it.skipIf(win32)`; the coherence test that tolerates the transient miss still runs on every platform.
There was a problem hiding this comment.
Pull request overview
This PR makes the publisher’s two-slot “static artefact” symlink swap work on Windows development machines, where Win32 rename semantics can’t atomically replace an existing directory symlink, causing publishes to silently fail to take effect.
Changes:
- Add a Win32 fallback path in
swapSlotthat removes the existingcurrentlink before renaming the newcurrent.tmplink into place. - Introduce
removeSymlinkEntryto remove a symlink entry cross-platform (without deleting its target). - Update the publish atomicity race test to skip the strict “never null” invariant on Windows, while keeping the coherence test enabled everywhere.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
server/publish/staticArtefact.ts |
Adds Windows-safe symlink swap fallback and a helper for removing symlink entries without touching targets. |
src/__tests__/server/publishAtomicityRace.test.ts |
Documents platform differences and skips the POSIX-only atomicity assertion on Windows while retaining the coherence test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+292
to
+301
| } catch (err) { | ||
| // Win32 `MoveFile` won't replace an existing target (`EEXIST`/`EPERM`/ | ||
| // `EACCES`). Drop the old `current` link first, then rename into the now- | ||
| // free path. Sub-millisecond gap where `current` is absent → `readArtefact` | ||
| // treats it as a miss and serves via Layer B. POSIX never reaches here. | ||
| const code = (err as NodeJS.ErrnoException).code | ||
| if (code !== 'EEXIST' && code !== 'EPERM' && code !== 'EISDIR' && code !== 'EACCES') throw err | ||
| await removeSymlinkEntry(currentPath) | ||
| await rename(tmpPath, currentPath) | ||
| } |
Comment on lines
+311
to
322
| async function removeSymlinkEntry(path: string): Promise<void> { | ||
| try { | ||
| await unlink(path) | ||
| } catch (err) { | ||
| if ((err as NodeJS.ErrnoException).code === 'ENOENT') return | ||
| try { | ||
| await rmdir(path) | ||
| } catch (err2) { | ||
| if ((err2 as NodeJS.ErrnoException).code !== 'ENOENT') throw err2 | ||
| } | ||
| } | ||
| } |
Address review feedback: the remove-then-rename fallback is a Windows-only affordance, but the catch matched errno codes alone. A POSIX `rename(2)` failure carrying one of those codes (unexpected state, permissions) would also trigger the fallback and let `removeSymlinkEntry` delete a real `current` directory, masking the underlying error. Gate both the `swapSlot` fallback and `removeSymlinkEntry`'s `rmdir` branch behind `process.platform === 'win32'` and rethrow on every other platform.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes the publisher's two-slot static-artefact symlink swap work on Windows dev machines.
swapSlotrelies onrename(2)atomically replacing thecurrentdirectory symlink. That holds on POSIX, but Win32MoveFilerefuses to overwrite an existing target (EEXIST,EPERM,EISDIR,EACCES), so on Windowscurrentstays stuck on the old slot andcurrent.tmpis orphaned, and a publish silently never takes effect.Changes:
swapSlotcatches the Win32 rename failure and falls back to unlink-then-rename.removeSymlinkEntryhelper removes a symlink entry cross-platform:unlinkfirst, thenrmdirfor Windows directory-symlinks (which removes the link without recursing into the slot it points at).currentis absent.readArtefactalready treats that as a Layer A miss and serves the route via the Layer B live-render path, so a visitor never sees a 404.Production runs Linux/Docker and always takes the atomic branch, so this is strictly a dev-on-Windows affordance.
Review follow-up (dda0252): gated both the fallback and the
rmdirbranch behindprocess.platform === 'win32'and rethrow elsewhere, so a POSIX rename failure can never trigger a destructive remove-then-rename.Tests: the strict "never null" atomicity assertion depends on atomic symlink rename, so it is now
it.skipIf(process.platform === 'win32'). The coherence test (which tolerates the transient miss) still runs on every platform.Verification
Run on Windows 11, Bun 1.3.14:
bun run buildbun test(covering suitebun test publishAtomicityRace: 1 pass, 1 skip on win32, 0 fail; full suite left to CI)bun run lintChecklist
swapSlot.)