fix(desktop): serialize shared state writes across processes - #718
fix(desktop): serialize shared state writes across processes#718tonyfettes wants to merge 11 commits into
Conversation
![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: de080ae365
ℹ️ 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".
| @fs.rename(temp.to_string(), target) catch { | ||
| error if @async.is_being_cancelled() => raise error | ||
| _ => { | ||
| remove_if_exists(path) |
There was a problem hiding this comment.
When the target path exists as a directory—for example, a workspace already contains .openseek/worktrees.json/—the initial file-to-directory rename fails and this fallback calls remove_if_exists, which recursively deletes directories (desktop/internal/fsx/fsx.mbt lines 103–110). Since workspace and worktree registry writes now use this helper, an attach or worktree mutation can silently erase everything under that directory instead of reporting that the registry path has the wrong type; restrict the fallback to regular files or only the specific Windows replacement error.
Useful? React with 👍 / 👎.
dda4151 to
bd70ba7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd70ba73ea
ℹ️ 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".
| let temp = path.dirname().join(path.basename().to_owned() + ".tmp") | ||
| @fs.write_file( | ||
| temp.to_string(), | ||
| content, | ||
| create_mode=CreateOrTruncate, |
There was a problem hiding this comment.
When a workspace already contains a symlink such as .openseek/worktrees.json.tmp pointing to another writable file, creating or updating a worktree reaches this predictable path with CreateOrTruncate, follows the symlink, and truncates the linked file before the rename merely moves the symlink into place. A repository can contain this path before attachment despite the later Git exclusion, so use an exclusively created, unpredictable temporary regular file (and reject links) rather than truncating a fixed name.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de43e82511
ℹ️ 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".
| @fs.remove(target) | ||
| @fs.rename(temp.to_string(), target) |
There was a problem hiding this comment.
On Windows, when replacing an existing file reaches this fallback, cancellation after remove completes but before rename completes leaves the target absent; the outer handler then deletes the only completed temp file. This path is reachable from the unshielded legacy archive migration, so cancelling session_list_archived while it rewrites a nonempty remaining list can permanently delete archived_sessions.txt and prevent those sessions from being retried. Protect the remove-and-rename sequence from cancellation, or use a platform replacement operation that cannot expose this state.
Useful? React with 👍 / 👎.
| @fs.write_file(temp.to_string(), content, create_mode=CreateNew, permission~) | ||
| commit_temp_file(temp, target) catch { |
There was a problem hiding this comment.
If write_file creates the random CreateNew file and then raises—for example because the task is cancelled during the write or the filesystem returns ENOSPC—the cleanup handler is never entered because it only wraps commit_temp_file. Each retry therefore leaves another unguessable partial file behind, and failed settings writes can leave credential material in these abandoned files. Wrap the write itself in the same shielded cleanup path.
Useful? React with 👍 / 👎.
ac30712 to
39cdb1f
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39cdb1f5cf
ℹ️ 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".
| // renames, so without this the loser of a race reads its own "already | ||
| // gone" as a failed move and writes the id back for the next launch to | ||
| // retry — forever. | ||
| let shared = @fsx.FileLock::acquire(list, Exclusive) |
There was a problem hiding this comment.
When archived_sessions.txt is readable but its sidecar cannot be created or locked—for example, the runtime directory became read-only or the filesystem does not support this lock—FileLock::acquire raises here and aborts the entire archived_sessions request. Previously this migration treated filesystem failures as best-effort and still proceeded to list the current archive stores; catch lock-acquisition failures and skip migration so users can still view their archives.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d656de21d6
ℹ️ 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".
| let file = @fs.open( | ||
| lock_file(path).to_string(), | ||
| mode=ReadWrite, | ||
| create_mode=OpenOrCreate, | ||
| ) |
There was a problem hiding this comment.
When an attached repository already contains .openseek/worktrees.json.lock as a symlink, OpenOrCreate follows it, so a repository can make the desktop create an empty file outside the workspace or lock an attacker-chosen inode. A link to a per-process or concurrently replaced target also lets separate instances hold different locks, defeating the cross-process serialization and permitting the lost registry updates this change is intended to prevent; open the sidecar without following links and verify that it is a regular file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9068c1912
ℹ️ 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".
| } | ||
| worktree_registry_lock.acquire() | ||
| defer worktree_registry_lock.release() | ||
| let shared = @fsx.FileLock::acquire(worktrees_file(dir).to_path(), Exclusive) |
There was a problem hiding this comment.
When the sidecar cannot be opened or locked—for example, the workspace contains the symlink or non-regular .openseek/worktrees.json.lock that open_lock_file deliberately refuses—this acquisition raises only after git worktree remove has already deleted the checkout. The request therefore reports failure while leaving the registry entry behind (and a forced removal may already have discarded changes); acquire the cross-process lock before the initial registry read and destructive Git operation so lock failures leave the worktree untouched.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f51726dfb3
ℹ️ 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".
| (Some(ownership), Some(dir)) if ownership.hands_off() => | ||
| Some(@instance.identity(dir)) |
There was a problem hiding this comment.
When two launches overlap during startup, the first process can acquire the heartbeat claim and then be preempted before registering the Proton identity; the second sees Healthy, also enables single_instance, and can register/run first. The claimant then hands off and exits, leaving the surviving primary without a Claim::run heartbeat; if that primary later wedges, subsequent launches see an unlocked heartbeat, classify themselves as Claimed, and hand off to the wedged Proton owner instead of taking the intended fallback path. Ensure only the heartbeat claimant can become the Proton primary, or transfer the claim to whichever process wins the single-instance election.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0964af771
ℹ️ 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".
| let now = @env.now() | ||
| // A stamp from the future is a clock that moved backwards, not a wedge. | ||
| guard now > stamped else { return false } | ||
| now - stamped > HeartbeatStaleMs |
There was a problem hiding this comment.
When a machine resumes after being suspended for more than 60 seconds, this wall-clock comparison can run before the existing host's sleeping heartbeat task is scheduled to restamp. A launch in that window is classified as seeing a wedged owner, so main.mbt omits single_instance and starts a competing host even though the original event loop is healthy; confirm that the stamp remains unchanged for another heartbeat interval before bypassing the election.
Useful? React with 👍 / 👎.
| /// must not exclude each other. It also keeps a development host from | ||
| /// silently swallowing the launch of the installed app. | ||
| pub fn identity(runtime_dir : @pathx.Path) -> String { | ||
| "community.moonbit.proton.openseek-desktop:" + runtime_dir.to_string() |
There was a problem hiding this comment.
When the same existing runtime directory is reached through lexical aliases—for example, a development checkout invoked once through a symlinked parent and once through its physical path—this produces different Proton identities even though both heartbeat paths resolve to the same inode. The processes therefore win separate elections; the second fails claim because the first holds the physical lock, but its on_start handler simply continues without a heartbeat and both hosts run. This is a distinct remaining non-claimant case exposed by the raw path-based identity, so derive the key from the directory's canonical filesystem path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4d8ceda56
ℹ️ 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".
| on_shutdown=fn(_) { }, | ||
| ) | ||
| let app = match instance_identity { | ||
| Some(identity) => app.single_instance(identity) |
There was a problem hiding this comment.
When a second launch reaches single_instance after the primary has written the shared relaunch marker but before its run loop exits, the second process hands off and returns from app.run(), then continues into take_relaunch_target at main.mbt:199-200. That consumes the primary's marker and opens the new bundle while the primary still owns the instance identity, so the reopened process is handed off and exits; when the primary finally closes, its marker is gone and the updated app is not relaunched. Track whether this process actually became the primary, and only let that process consume the marker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 131a1e3211
ℹ️ 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".
| @async.protect_from_cancel(() => { | ||
| @fs.remove(target) | ||
| @fs.rename(temp.to_string(), target) |
There was a problem hiding this comment.
On Windows, when the first rename cannot overwrite an existing registry or settings file, this fallback removes the target before the second rename. Readers in another process intentionally take no sidecar lock, and registered_workspaces, read_worktrees_unlocked, and load_engine_settings interpret a missing file as an empty registry or default settings, so any read during this window observes false state despite the new atomic-read contract. Cancellation shielding does not hide this gap from other processes; use a platform atomic-replace operation or synchronize those readers.
Useful? React with 👍 / 👎.
| is_primary.val = true | ||
| match runtime_dir { | ||
| Some(dir) => | ||
| if @instance.claim(dir) is Some(held) { |
There was a problem hiding this comment.
When a launch previously took the stopped-owner path, it omits single_instance; if the old owner exits before on_start, that bypass host can successfully claim and stamp the heartbeat without owning the Proton identity. A later launch then sees the fresh heartbeat, enables single_instance, wins the otherwise-free Proton election, gets None here because the bypass host holds the claim, and nevertheless continues as a second primary with no heartbeat. The fresh evidence after the earlier startup-race fix is this new identity-bypass path; abort or retry/transfer the election when the elected primary cannot claim.
Useful? React with 👍 / 👎.
131a1e3 to
7a3ae6e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a3ae6e722
ℹ️ 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".
| // when the primary finally closed, its marker would be gone and the | ||
| // update would never be entered. The nesting is deliberate; a `&&` chain | ||
| // would take the marker while deciding whether it was allowed to. | ||
| if is_primary.val { |
There was a problem hiding this comment.
When another launch is forwarded while the primary is finishing an update, this tail guard does not fully protect the relaunch marker: before app.run() determines that the process is secondary, spawn_host_pumps at lines 171–173 starts SelfUpdate::run, whose startup cleanup_leftovers unconditionally removes the shared relaunch marker (desktop/internal/update/apply.mbt:133-142). The fresh evidence after the earlier marker-consumption fix is this independent startup-cleanup path, which can delete the marker before the primary reaches take_relaunch_target, leaving the updated app swapped on disk but not relaunched; defer that cleanup until on_start confirms this process is primary.
Useful? React with 👍 / 👎.
The desktop has never been the only writer of its durable state. A TUI launched in a workspace shares that workspace's store by design, a dev build shares the session root with the installed app, and a leftover process from a previous run outlives the launch that spawned it. Every registry mutation nevertheless serialized on an `@async.Mutex`, which orders this process's own tasks and nothing else: two writers could each read the same list and let the later write erase the earlier one. Add the two primitives that were missing and apply them: - `@fsx.FileLock` — an advisory flock on a sidecar `<file>.lock`, held across processes until `release`. A sidecar rather than the data file because an atomic save replaces the target by rename, so a lock taken on the data file would be attached to the inode rename orphans. - `@fsx.write_text_atomic` — temp file plus rename, so a reader that never takes the lock (another tool, an older build) still sees one complete version or the other. Mutations take the exclusive lock and re-read inside it; reads take no lock at all and rely on the write being atomic. That keeps the tolerant "unreadable registry means nothing is attached" behaviour on the read paths, which are everywhere, instead of turning them into error paths. Applied to workspaces.json, worktrees.json, engine-settings.json, and the legacy archived-sessions list. `write_settings_file` now delegates its rename dance to the shared helper rather than keeping a second copy of the Windows fallback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The retry after a failed rename exists for Windows, which may refuse to replace an existing file. It removed the target unconditionally through `remove_if_exists`, which deletes a directory recursively — so a path occupied by a directory (`.openseek/worktrees.json/`, say) was erased along with everything under it, and the write then reported success. This was inherited from `write_settings_file`, but promoting it into a shared helper spread it to the workspace and worktree registries. Restrict the retry to a regular file at the target and re-raise the original rename failure otherwise, so a wrong path is reported rather than cleared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The temp file's name was the target's plus `.tmp`, opened with `CreateOrTruncate`. That is safe in the runtime directory but not in a workspace, which is whatever repository the user attached: a repository can carry a symlink at `.openseek/worktrees.json.tmp`, committed long before the attach that adds the git exclusion. Creating a worktree then followed it, truncating whatever it pointed at and writing the registry there, and the rename afterwards moved the link itself into place — so the registry became a symlink aimed at the clobbered file. Name the temp file with random hex and create it with `CreateNew` (O_CREAT|O_EXCL), which refuses any pre-existing name including a symlink. The random suffix is what keeps a fixed name plus O_EXCL from turning one leftover into a permanent write failure; in exchange the file is no longer reused, so every path out of the commit that is not the rename now removes it, under a cancellation shield. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two gaps left by the private-temp-file change: The cleanup handler only wrapped the commit, so a `write_file` that raised after creating the file — a cancelled task, a full disk — left the temp behind. With a fixed name that self-corrected on the next save; with a random one every attempt abandons another file, and for engine-settings.json those files hold a credential. Wrap the write in the same shielded cleanup. The Windows replacement retry removed the target and then renamed. A cancellation between the two left the target absent, and the caller's cleanup then took the temp file as well, destroying the file the call was meant to update. Commit the pair under a cancellation shield. Reachable from `session_list_archived`, whose legacy-archive migration rewrites its remaining list without a shield of its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`archived_sessions` migrates the zip-era list before listing anything, and every filesystem failure in that migration is deliberately swallowed — a one-time cleanup must never stand between the user and their archived conversations. The new registry lock was the one path out of it that raised, so a runtime directory that turned read-only, or one on a filesystem without advisory locks, failed the whole `session.list_archived` request instead of skipping the migration. Treat a lock that cannot be acquired like every other failure there and return, leaving cancellation as the only reason to propagate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The temp file was hardened against a link planted at its name; the lock sidecar beside it was not, and it sits in the same directory. An attached repository can ship `.openseek/worktrees.json.lock`, and `OpenOrCreate` follows it twice over: it creates a file wherever the link points, and it locks that inode, so two instances following a link that moved would each hold a lock on a different file and both walk into the registry write the lock exists to serialize. Create the sidecar with `CreateNew`, which never follows a link, and reject anything that is not a regular file by its own lstat on the already-exists path. A link swapped between that check and the open still wins; closing that needs O_NOFOLLOW, which the filesystem API does not expose. The case this does close is a link committed to a repository, which is the one that arrives without an attacker at the keyboard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closing the window quits the app, and a second launch starts a rival host against the same session root, settings, and worktree registries. `App::single_instance` hands that launch to the process already there instead — keyed by the runtime directory, since that is what a host owns: two builds sharing it must not both run, and two checkouts with separate `dev-state` directories share nothing and must not exclude each other. Enforcing it unconditionally would be unsafe. A host wedged in CEF's shutdown keeps its lock, and its instance listener runs on its own thread, so the launch is accepted, enqueued, acknowledged — and never acted on, because the thread that would act on it is the wedged one. The app would stop opening at all, with no error anywhere to react to, until the user found the process by hand. Today that same wedge merely costs a second instance. So the rule is conditional on liveness the lock cannot express. The owning host stamps a heartbeat from its event loop; a launch that finds a live owner whose stamp has stopped advancing declines to hand over and starts alongside it. A stamp left behind by a host that exited is never read, because reading it requires someone to be holding the lock. `@fsx.FileLock::try_acquire` is the probe that asks who holds a lock without waiting on the process being asked about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The claim was taken before `App::run`, so it and the single-instance primary could end up in different processes. Two overlapping launches both find the directory free or freshly stamped and both enable `single_instance`; Proton then elects one of them, and if that is not the process holding the claim, the claimant forwards and exits — taking the claim with it. The surviving primary runs unstamped, so every later launch reads an unlocked heartbeat, believes nothing is wrong, and hands over. Should that primary wedge, the fallback this was built for never fires. Split the two questions. Before the election, `owner_is_wedged` probes — it takes the claim only to learn whether it can, and lets go. The claim itself is taken in the `app_lifecycle` start hook, which only the election winner reaches, so the heartbeat always describes the host a later launch would actually be handed to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`owner_is_wedged` named a symptom from the CEF shutdown investigation and left the caller reading a negation to reach the decision it was actually making. `can_hand_off_to` states the question `single_instance` asks, so the call site reads forwards. The positive form also has to carry the case the old name hid: no owner at all answers true, because the rule still applies — this process becomes the owner. That is now in the documentation rather than in the reader's head. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Canonicalize the identity. It was the runtime directory's lexical path, so one directory reached through two spellings — `/tmp` and `/private/tmp`, a checkout opened once through a symlinked parent — won two separate elections and both hosts ran. The second then failed to claim the heartbeat, whose lock is on the physical inode, and carried on unstamped and invisible. The workspace registry collapses aliases for the same reason. Confirm a stopped stamp before acting on it. A machine resuming from suspend produces the same evidence as a host that died: its heartbeat task has not been scheduled since the wake, so the stamp is as old as the sleep. One extra interval of watching separates the two, and costs nothing on any path that is not already suspicious. Let only the primary consume the relaunch marker. A forwarded launch reaches the tail of `main` too, having done nothing; taking the marker there reopened the bundle, which handed itself straight back to the still-running primary and exited, and when that primary finally closed its marker was gone and the update was never entered. The check is nested rather than a `&&` chain because `&&` evaluates its async operand eagerly — it would take the marker while deciding whether it was allowed to. Also drops "wedged" throughout for plain wording: it was jargon from the CEF shutdown investigation and meant nothing on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tory Two things a launch did before it could know whether it was the host or merely on its way to one. The update pump swept leftovers as its first act, and the sweep discards the relaunch marker. That marker is how a host that just applied an update tells its own exit to reopen the new bundle, and it exists only across the gap between the apply and the run loop returning. A launch arriving in that gap starts its pumps before `app.run()` decides it is secondary, so it swept the marker away and the update landed on disk with nothing left to reopen it. The sweep moves out of the pump and onto the owning host, which runs it once the election has named it. The pump keeps the downloads, which only ever come from a page a forwarded launch does not have. The heartbeat claim gave up when it was already taken, which the elected primary can genuinely find: a host started on the stopped-owner path holds a claim without ever entering an election, so its claim outlives it. The primary then ran with no heartbeat at all, and a later launch, reading the other host's fresh stamp, would hand itself to this one however stuck it had become — the exact failure the stamp exists to catch. `hold` waits the other host out instead of giving up, and takes the claim the moment it exits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7a3ae6e to
5c2c08b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c2c08be7c
ℹ️ 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".
| tasks.spawn_bg(allow_failure=true, () => { | ||
| state.host_state().sweep_update_leftovers() |
There was a problem hiding this comment.
When a host started through the stopped-owner bypass has claimed the heartbeat but has no Proton single-instance identity, a later launch can win the identity election and reach this hook while hold(dir) is still waiting for that bypass host. Because the cleanup task is spawned independently, it can delete the active host's staging directory or relaunch marker, interrupting an update or preventing the updated bundle from reopening. The fresh evidence after moving cleanup out of the pump is that this final code still runs it before the process has actually acquired the runtime claim; start owner-only cleanup only after that acquisition succeeds.
Useful? React with 👍 / 👎.
1 participant
The desktop has never been the only writer of its durable state:
workspaces.mbtsays so in its header — the two are meant to resume each other's conversations),~/.openseekwith the installed app,Every registry mutation nevertheless serialized only on an
@async.Mutex, which orders this process's own tasks and nothing else. Two writers could each read the same list and let the later write silently erase the earlier one. The non-atomicwrite_textalso let a concurrent reader observe a truncated file and read it as "nothing is attached".What this adds
Two primitives in
internal/fsx/lock.mbt:FileLock::acquire(path, kind)/release()— an advisoryflockon a sidecar<file>.lock, honored across processes. A sidecar rather than the data file itself: an atomic save replaces the target by rename, so a lock taken on the data file would be attached to the inode the rename orphans, and two writers would each hold "the lock" on a different file.write_text_atomic(path, content, permission?)— temp file + rename.Shaped as an acquire/release pair rather than a scoped callback because every call site already sits next to
mutex.acquire(); defer mutex.release()— this inserts two lines instead of reindenting the surrounding block into a closure.The convention
Leaving reads unlocked is deliberate. They are everywhere (
registered_workspaces()alone is called from a dozen places) and they are tolerant by contract: an unreadable registry means "nothing attached", not an error. Locking them would convert that into a raising path. It also would not help the readers that matter most — an external tool or an older build never takes our lock, and what actually protects those is the atomic write.Applied to
workspaces.jsonworktrees.json(create and remove; the create holds the lock across thegit worktree addtoo, so another instance cannot pick the samewt-Nafter this one probed it as free)engine-settings.jsonarchived_sessions.txtmigration (probe before locking, so the common "already migrated" launch leaves no sidecar behind)write_settings_filenow delegates to the shared helper instead of keeping a second copy of the Windows rename fallback.Testing
moon check --target native: cleanmoon test --target native: 3343/3343internal/fsx: atomic save leaves no.tmp, an exclusive lock makes the next holder wait, shared locks admit each other, a lock can be retaken after release@fs.File::lockisflock, which binds the open file description rather than the process, so two acquires of one path contend inside a single process through exactly the mechanism a second instance would hit — the tests cover the cross-process path without spawning a helper.Note on a pre-existing flake
The first full-suite run failed one test,
package/internal/packaging→ "Proton CEF root follows the active runtime manifest", withmkdir target: File exists. A second full run passed 3343/3343. That package's ownensure_diris check-then-mkdir, and two packaging tests race to createdesktop/targetwhen it does not yet exist. Pre-existing, untouched by this change, and left alone as out of scope — but it is the same bug class, if someone wants it.🤖 Generated with Claude Code