Fix deleted files persisting in published realm on republish#4221
Conversation
Clear the published realm directory before copying from source so that files deleted from the source realm don't linger in the published realm. The existing fullIndex() call already handles DB-level cleanup by tombstoning index entries for files no longer on disk. Closes CS-10189 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jurgenwerk
force-pushed
the
cs-10189-deleted-files-should-be-removed-from-published-realm-upon
branch
from
63a6b51 to
e3ccf23
Compare
Host Test Results 1 files ± 0 1 suites ±0 3h 47m 46s ⏱️ + 1h 27m 14s For more details on these errors, see this check. Results for commit af7990c. ± Comparison against base commit c459617. ♻️ This comment has been updated with latest results. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3ccf23075
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| removeSync(publishedRealmPath); | ||
| copySync(sourceRealmPath, publishedRealmPath); |
There was a problem hiding this comment.
Deleting publishedRealmPath before copying makes republish non-atomic for existing published realms: if copySync (or any subsequent read/write in this block) fails due to I/O conditions like disk-full or permission errors, the request returns 500 after the old on-disk realm has already been removed, and there is no rollback path to restore it. In that failure mode, the currently mounted published realm is left serving from a deleted/partially rebuilt directory, causing user-visible outages until another successful publish.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This does seem worth thinking through, we don’t have much in the way of error-handling here
There was a problem hiding this comment.
Pull request overview
This PR fixes republish behavior for published realms by ensuring files deleted from a source realm do not persist in the published realm after republishing (CS-10189).
Changes:
- Clear the published realm directory before copying files from the source realm during publish/republish.
- Add a regression test verifying deleted source files are removed from the published realm on republish.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/realm-server/handlers/handle-publish-realm.ts | Removes existing published realm directory prior to copying from the source realm to prevent stale files on republish. |
| packages/realm-server/tests/publish-unpublish-realm-test.ts | Adds a test that publishes, deletes a source file, republishes, and asserts the file is removed from the published realm on disk. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let sourceRealmPath = sourceRealm.dir; | ||
| let publishedDir = join(realmsRootPath, PUBLISHED_DIRECTORY_NAME); | ||
| let publishedRealmPath = join(publishedDir, publishedRealmId); | ||
| // Remove existing published files so that files deleted from the source | ||
| // realm don't persist in the published realm on republish | ||
| removeSync(publishedRealmPath); | ||
| copySync(sourceRealmPath, publishedRealmPath); |
There was a problem hiding this comment.
removeSync(publishedRealmPath) runs before unmounting existingPublishedRealm (the unmount happens later), which means a republish can delete/partially overwrite the on-disk directory while the realm is still mounted and potentially serving requests. This can cause transient 5xxs or inconsistent reads during republish and leaves the published realm in a broken state if copySync or subsequent steps fail. Consider unmounting the existing published realm before touching the filesystem, or publishing into a fresh temp directory and then swapping/renaming it into place atomically once the copy and config rewrite succeed.
There was a problem hiding this comment.
I agree with this, I don’t know that we’ve defined behaviour around requests for published realms when (re)publishing is in progress
backspace
left a comment
backspace
left a comment
There was a problem hiding this comment.
The test looks good to me, the bot comments do seem relevant but could be addressed in followup issues
Copy source realm to a temp directory first, then swap it into place, so that a failed copy (e.g. disk-full, permission error) doesn't destroy the existing published realm with no rollback path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move the unmount of the existing published realm to happen before the directory swap, so it can't serve requests from a partially replaced filesystem during republish. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
copySyncwould only add/overwrite files but never remove files that existed in the destination but not in the sourcefullIndex()call already handles DB-level cleanup by tombstoning index entries for files no longer on diskCloses CS-10189
Test plan
🤖 Generated with Claude Code