Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions desktop/internal/engine/archive.mbt
Original file line number Diff line number Diff line change
Expand Up @@ -336,12 +336,35 @@ async fn migrate_legacy_archive(
) -> Unit {
guard manager.runtime_dir is Some(dir) else { return }
let list = dir.join(LegacyArchivedListFile)
// Probe before locking: this migration has already run for almost every
// launch, and a lock sidecar should not outlive the file it guards.
guard (@fsx.try_read_text(list) catch {
error if @async.is_being_cancelled() => raise error
_ => None
})
is Some(_) else {
return
}
guard resolved_session_root() is Some(root) else { return }
// Another instance may be migrating the same list. The record moves are
// 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) catch {
error if @async.is_being_cancelled() => raise error
// Every other filesystem failure here is best-effort, and the lock is no
// different: a runtime directory that turned read-only, or one on a
// filesystem without advisory locks, must leave the caller listing the
// archives it already has rather than failing the whole request over a
// one-time migration.
_ => return
}
defer shared.release()
let text = (@fsx.try_read_text(list) catch {
error if @async.is_being_cancelled() => raise error
_ => None
}).unwrap_or("")
guard !text.is_empty() else { return }
guard resolved_session_root() is Some(root) else { return }
let remaining : Array[String] = []
for line in text.split("\n") {
let session = line.trim().to_owned()
Expand All @@ -358,7 +381,7 @@ async fn migrate_legacy_archive(
_ => ()
}
} else {
@fsx.write_text(list, "\{remaining.join("\n")}\n") catch {
@fsx.write_text_atomic(list, "\{remaining.join("\n")}\n") catch {
error if @async.is_being_cancelled() => raise error
_ => ()
}
Expand Down
41 changes: 13 additions & 28 deletions desktop/internal/engine/settings.mbt
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,12 @@ pub async fn update_settings(
guard manager.runtime_dir is Some(dir) else {
raise EngineError("engine settings cannot persist: no runtime directory")
}
// `settings_lock` orders this process's clients; the runtime directory is
// shared with every other instance running against it. The load below has
// to follow this lock — settings read before it would carry back the very
// fields another instance just changed.
let shared = @fsx.FileLock::acquire(settings_file(dir), Exclusive)
defer shared.release()
let current = load_engine_settings(manager.runtime_dir)
guard current.version <= EngineSettingsVersion else {
raise EngineError(
Expand Down Expand Up @@ -274,36 +280,15 @@ fn apply_text_patch(
}

///|
/// Atomic save: write a 0600 temp file, then rename over the target. The
/// file holds a credential, so a crash mid-write must never leave a
/// truncated store — the temp file keeps the full content until the rename
/// lands. Windows may refuse to rename over an existing file; removing the
/// target first narrows atomicity to that one platform's small window.
/// Atomic save at 0600. The file holds a credential, so a crash mid-write
/// must never leave a truncated store, and every reader of it — this process,
/// another instance, the next launch — sees one complete version or the
/// other. Reads therefore take no lock; only the read-modify-write in
/// `update_settings` does.
async fn write_settings_file(runtime_dir : @pathx.Path, text : String) -> Unit {
let target = settings_file(runtime_dir)
let temp = runtime_dir.join(EngineSettingsFileName + ".tmp")
@fs.write_file(
temp.to_string(),
text,
create_mode=CreateOrTruncate,
permission=0o600,
) catch {
@fsx.write_text_atomic(settings_file(runtime_dir), text, permission=0o600) catch {
error if @async.is_being_cancelled() => raise error
error => raise EngineError("could not write engine settings: \{error}")
}
@fs.rename(temp.to_string(), target.to_string()) catch {
error if @async.is_being_cancelled() => raise error
_ => {
@fsx.remove_if_exists(target) catch {
error if @async.is_being_cancelled() => raise error
error =>
raise EngineError("could not replace engine settings: \{error}")
}
@fs.rename(temp.to_string(), target.to_string()) catch {
error if @async.is_being_cancelled() => raise error
error => raise EngineError("could not save engine settings: \{error}")
}
}
error => raise EngineError("could not save engine settings: \{error}")
}
}

Expand Down
53 changes: 42 additions & 11 deletions desktop/internal/engine/workspaces.mbt
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ const WorkspacesFile = "workspaces.json"
/// lock. Remote requests run in independent tasks; without this serialization
/// two concurrent add/remove operations can both read the same old list and
/// let the later write silently erase the earlier one.
///
/// It orders this process only. The registry is shared with every other
/// writer of the same session root — a second desktop instance, a TUI in the
/// same workspace — so each mutation also takes the cross-process
/// `@fsx.FileLock` on the registry file. Reads need neither lock: the write
/// is atomic, so a reader sees one complete version or the other.
let workspace_registry_lock : @async.Mutex = Mutex()

///|
Expand All @@ -23,6 +29,19 @@ fn workspaces_file() -> @pathx.Absolute? {
resolved_session_root().map(root => root.join(WorkspacesFile))
}

///|
/// The registry file a mutation must lock and write. Unlike reads, which
/// degrade to "no workspaces", a mutation with nowhere to persist has to say
/// so rather than silently succeed against nothing.
fn required_workspaces_file() -> @pathx.Absolute raise EngineError {
guard workspaces_file() is Some(file) else {
raise EngineError(
"cannot determine a session root; set OPENSEEK_SESSION_ROOT",
)
}
file
}

///|
/// The canonical (symlink-resolved) spelling of a workspace directory — the
/// registry's one identity per project, so `/tmp/x` and `/private/tmp/x`
Expand Down Expand Up @@ -77,21 +96,20 @@ pub async fn registered_workspaces() -> Array[@pathx.Absolute] {
}

///|
/// Save the registry. Atomic, so the concurrent readers this file has — other
/// desktop instances, a TUI, the next launch after a crash — never observe a
/// truncated list and read it as "no workspaces attached".
async fn write_registry(paths : Array[@pathx.Absolute]) -> Unit {
guard resolved_session_root() is Some(root) else {
raise EngineError(
"cannot determine a session root; set OPENSEEK_SESSION_ROOT",
)
}
@fsx.ensure_dir(root.to_path()) catch {
let file = required_workspaces_file()
@fsx.ensure_dir(file.to_path().dirname()) catch {
error if @async.is_being_cancelled() => raise error
error => raise EngineError("could not create \{root}: \{error}")
error =>
raise EngineError(
"could not create \{file.to_path().dirname()}: \{error}",
)
}
let registry : Json = { "workspaces": paths.map(path => path.0).to_json() }
@fsx.write_text(
root.join(WorkspacesFile).to_path(),
registry.stringify(indent=2),
) catch {
@fsx.write_text_atomic(file.to_path(), registry.stringify(indent=2)) catch {
error if @async.is_being_cancelled() => raise error
error => raise EngineError("could not save the workspace list: \{error}")
}
Expand Down Expand Up @@ -133,6 +151,14 @@ pub async fn add_workspace(
exclude_from_git(dir, ".openseek")
workspace_registry_lock.acquire()
defer workspace_registry_lock.release()
// The registry read has to follow both locks: a list read before another
// process released this one is exactly the stale snapshot whose write-back
// would erase that process's entry.
let shared = @fsx.FileLock::acquire(
required_workspaces_file().to_path(),
Exclusive,
)
defer shared.release()
let paths = read_registry_unlocked()
// The write and broadcast callback are one finite, cancellation-shielded
// linearization point while the lock is held. Once disk changes, every
Expand All @@ -159,6 +185,11 @@ pub async fn remove_workspace(
let target = canonical_workspace_path(expanded_workspace_path(path))
workspace_registry_lock.acquire()
defer workspace_registry_lock.release()
let shared = @fsx.FileLock::acquire(
required_workspaces_file().to_path(),
Exclusive,
)
defer shared.release()
let paths = read_registry_unlocked().filter(p => p != target)
@async.protect_from_cancel(() => {
write_registry(paths)
Expand Down
29 changes: 21 additions & 8 deletions desktop/internal/engine/worktrees.mbt
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ const WorktreeBranchPrefix = "openseek/"
/// Registry reads and read-modify-write mutations share one process-wide
/// lock, exactly like `workspaces.json` — without it two concurrent
/// create/remove operations could both read the same old list and let the
/// later write silently erase the earlier one.
/// later write silently erase the earlier one. And exactly like
/// `workspaces.json`, it orders this process only: each mutation also takes
/// the cross-process `@fsx.FileLock` on that workspace's registry file, while
/// reads rely on the write being atomic.
let worktree_registry_lock : @async.Mutex = Mutex()

///|
Expand Down Expand Up @@ -130,7 +133,9 @@ async fn write_worktrees(
error => raise EngineError("could not create \{store}: \{error}")
}
let registry : Json = { "worktrees": entries.to_json() }
@fsx.write_text(
// Atomic, so a concurrent reader never sees a truncated list and mistakes
// a bound conversation for one that runs in the workspace root.
@fsx.write_text_atomic(
worktrees_file(workspace).to_path(),
registry.stringify(indent=2),
) catch {
Expand Down Expand Up @@ -612,6 +617,11 @@ pub async fn create_worktree(
ensure_session_not_archived(session)
worktree_registry_lock.acquire()
defer worktree_registry_lock.release()
// Held across the git work below as well as the registry write: a create
// that released between them would let another instance's create pick the
// same `wt-N` after this one probed it as free.
let shared = @fsx.FileLock::acquire(worktrees_file(dir).to_path(), Exclusive)
defer shared.release()
let entries = read_worktrees_unlocked(dir)
// This conversation already holds its environment. Two shapes land here:
// the idempotent replay of a create whose reply was lost, and the repair
Expand Down Expand Up @@ -707,11 +717,11 @@ pub async fn create_worktree(
}
exclude_from_git(dir, WorktreesDirName)
let base = git_in(dir, ["rev-parse", "HEAD"])
// The branch comes first, on its own: our locks are in-process only, so
// another process can race the probes above — `git branch` then fails
// atomically having created nothing, and from here on the branch is
// provably THIS invocation's, which is what makes the rollback's
// `branch -D` safe.
// The branch comes first, on its own: our locks bind desktop instances,
// not the user's own git, so anything outside the app can still race the
// probes above — `git branch` then fails atomically having created
// nothing, and from here on the branch is provably THIS invocation's,
// which is what makes the rollback's `branch -D` safe.
ignore(git_in(dir, ["branch", branch, base]))
ignore(
git_in(dir, ["worktree", "add", "\{WorktreesDirName}/\{name}", branch]) catch {
Expand Down Expand Up @@ -894,8 +904,11 @@ pub async fn remove_worktree(
}
worktree_registry_lock.acquire()
defer worktree_registry_lock.release()
let shared = @fsx.FileLock::acquire(worktrees_file(dir).to_path(), Exclusive)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Acquire the file lock before deleting the worktree

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 👍 / 👎.

defer shared.release()
// Re-read: another worktree may have been created while the checkout was
// being removed (creation takes only the registry lock).
// being removed — by this process, which takes only the registry lock for
// that, or by another one, which this file lock has just been waiting on.
let remaining = read_worktrees_unlocked(dir).filter(entry => {
entry.name != name
})
Expand Down
Loading
Loading