Skip to content

Commit 5c2c08b

Browse files
tonyfettesclaude
andcommitted
fix(desktop): keep owner-only work in the process that owns the directory
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>
1 parent 2d7ea04 commit 5c2c08b

6 files changed

Lines changed: 134 additions & 36 deletions

File tree

desktop/internal/host/host.mbt

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,11 +86,17 @@ pub fn new_host_state(
8686
///|
8787
/// The background tasks behind the host ops, spawned into the caller's
8888
/// app-lifetime task group: the moon-check diagnostics forwarder and the
89-
/// self-update pump with its startup leftover sweep. These are shared by
89+
/// self-update download pump. These are shared by
9090
/// every connection;
9191
/// connection-owned filesystem watcher tasks start in `HostConnection::run`.
9292
/// Every background task is best-effort: a language-service hiccup degrades
9393
/// that feature, never the connection.
94+
///
95+
/// The update pump is safe to start before this process knows whether it owns
96+
/// the runtime directory: it only ever acts on requests from a page, and a
97+
/// launch that gets handed to another host never has one. The one piece of
98+
/// update work that touches shared state unprompted is
99+
/// `sweep_update_leftovers`, which is separate for exactly that reason.
94100
pub fn[G] spawn_host_pumps(
95101
group : @async.TaskGroup[G],
96102
state : HostState,
@@ -104,6 +110,20 @@ pub fn[G] spawn_host_pumps(
104110
group.spawn_bg(allow_failure=true, () => state.updates.run(emit))
105111
}
106112

113+
///|
114+
/// Clear what a previous update left in the runtime directory. Call this only
115+
/// from the host that owns that directory, and only once it does: the sweep
116+
/// discards a relaunch marker, which belongs to the owner and may be the one
117+
/// it is about to act on.
118+
pub async fn HostState::sweep_update_leftovers(self : HostState) -> Unit {
119+
self.updates.cleanup() catch {
120+
error if @async.is_being_cancelled() => raise error
121+
error =>
122+
@xlog.warn(category="update") <?
123+
{ "event": "cleanup_failed", "error": "\{error}" }
124+
}
125+
}
126+
107127
///|
108128
/// Run the filesystem watcher and PTY pump owned by this connection. Their
109129
/// notifications return only to the same transport instead of entering the

desktop/internal/host/pkg.generated.mbti

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ pub fn HostConnection::new() -> Self
4444
pub async fn HostConnection::run(Self, async (@protocol.Notification) -> Unit) -> Unit
4545

4646
type HostState
47+
pub async fn HostState::sweep_update_leftovers(Self) -> Unit
4748

4849
// Type aliases
4950

desktop/internal/host/update_check.mbt

Lines changed: 44 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,17 @@ fn SelfUpdate::new(
5353

5454
///|
5555
/// Sweep debris a previous update may have left behind (a staging dir, the
56-
/// old bundle parked aside) — the host's only unprompted update work.
56+
/// old bundle parked aside, a relaunch marker nobody acted on) — the host's
57+
/// only unprompted update work.
58+
///
59+
/// The relaunch marker is why this is not pump work. The marker is shared
60+
/// state belonging to whichever host owns the runtime directory, and it lives
61+
/// exactly across the gap between that host applying an update and its run
62+
/// loop returning. A launch that is about to be handed to that host runs its
63+
/// own pumps first and would sweep the marker out from under it — the bundle
64+
/// swapped on disk, and nothing left to say it should reopen. So the sweep
65+
/// belongs to the host that owns the directory, and runs only once that is
66+
/// settled.
5767
async fn SelfUpdate::cleanup(self : SelfUpdate) -> Unit {
5868
if self.work_dir is Some(dir) && self.bundle_path is Some(bundle) {
5969
@update.cleanup_leftovers(work_dir=dir, bundle_path=bundle)
@@ -211,23 +221,18 @@ async fn SelfUpdate::download_package(
211221
}
212222

213223
///|
214-
/// The app-lifetime pump: sweep leftovers once, then perform accepted
215-
/// downloads one at a time and report each outcome as a notification. Only
216-
/// this task downloads and installs staged state, so `busy` stays true from
217-
/// acceptance until the matching completion notification.
224+
/// The app-lifetime pump: perform accepted downloads one at a time and report
225+
/// each outcome as a notification. Only this task downloads and installs
226+
/// staged state, so `busy` stays true from acceptance until the matching
227+
/// completion notification. The leftover sweep is `SelfUpdate::cleanup`,
228+
/// which the owning host runs separately.
218229
async fn SelfUpdate::run(
219230
self : SelfUpdate,
220231
emit : async (@protocol.Notification) -> Unit,
221232
) -> Unit {
222233
// A closed queue turns later `update.download` requests into rejections
223234
// instead of accepted work nobody will perform.
224235
defer self.downloads.close()
225-
self.cleanup() catch {
226-
error if @async.is_being_cancelled() => raise error
227-
error =>
228-
@xlog.warn(category="update") <?
229-
{ "event": "cleanup_failed", "error": "\{error}" }
230-
}
231236
for ;; {
232237
let payload = self.downloads.get()
233238
try self.download_package(payload, emit) catch {
@@ -339,6 +344,34 @@ async test "self-update pump reports download failures" {
339344
})
340345
}
341346

347+
///|
348+
/// The relaunch marker belongs to whichever host owns the runtime directory,
349+
/// and it exists exactly across the gap between that host applying an update
350+
/// and its run loop returning. Every launch starts this pump, including one
351+
/// that is about to be handed straight to that host — so the pump must leave
352+
/// the marker alone, and only the owner's sweep may clear it.
353+
async test "the download pump leaves the relaunch marker to the owning host" {
354+
let root : @pathx.Path = @fs.tmpdir(prefix="openseek-update-marker-")
355+
let work_dir = root.to_string()
356+
let bundle_path = "\{work_dir}/SeekMoon.app"
357+
let updates = SelfUpdate::new(
358+
Some(root),
359+
Some("\{bundle_path}/Contents/MacOS/seekmoon"),
360+
)
361+
@update.write_relaunch_marker(work_dir~, bundle_path~)
362+
@async.with_task_group(group => {
363+
group.spawn_bg(allow_failure=true, () => updates.run(fn(_) { }))
364+
// Well past the point where a sweep at the pump's start would have run.
365+
@async.sleep(50)
366+
group.return_immediately(())
367+
})
368+
assert_true(@update.take_relaunch_target(work_dir~) is Some(_))
369+
@update.write_relaunch_marker(work_dir~, bundle_path~)
370+
updates.cleanup()
371+
assert_true(@update.take_relaunch_target(work_dir~) is None)
372+
@fs.rmdir(work_dir, recursive=true)
373+
}
374+
342375
///|
343376
async test "self-update apply refuses while a download is active" {
344377
let updates : SelfUpdate = {

desktop/internal/instance/instance.mbt

Lines changed: 54 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ pub async fn identity(runtime_dir : @pathx.Path) -> String {
7272
/// Dropping it is the kernel's job: a host that dies — cleanly, by signal, or
7373
/// stuck and then killed — releases it without leaving anything to clean up,
7474
/// which is why the claim is a lock and not a recorded pid.
75-
pub struct Claim {
75+
priv struct Claim {
7676
lock : @fsx.FileLock
7777
runtime_dir : @pathx.Path
7878
}
@@ -118,14 +118,37 @@ pub async fn can_hand_off_to(runtime_dir : @pathx.Path) -> Bool {
118118
}
119119

120120
///|
121-
/// Take ownership of `runtime_dir` for this host, or `None` if another
122-
/// process holds it.
121+
/// Own `runtime_dir` and stamp for it for as long as this host runs, taking
122+
/// the claim as soon as it is free.
123123
///
124124
/// Call this from the process that won the single-instance election and
125125
/// nowhere else. That is what keeps the claim and the primary the same
126126
/// process, so the heartbeat always describes the host a later launch would
127127
/// actually be handed to.
128-
pub async fn claim(runtime_dir : @pathx.Path) -> Claim? {
128+
///
129+
/// It waits rather than gives up, because the primary can legitimately find
130+
/// the claim taken: a host that started on the stopped-owner path holds no
131+
/// single-instance identity, so its claim outlives the election it never
132+
/// entered. Giving up there would leave this — the process every later launch
133+
/// is handed to — with no heartbeat at all, and a launch reading the other
134+
/// host's fresh stamp would go on handing itself to this one however stuck it
135+
/// became. Waiting costs nothing and ends the moment that host exits.
136+
pub async fn hold(
137+
runtime_dir : @pathx.Path,
138+
retry_interval_ms? : Int = HeartbeatIntervalMs,
139+
) -> Unit {
140+
for ;; {
141+
if claim(runtime_dir) is Some(held) {
142+
return held.run()
143+
}
144+
@async.sleep(retry_interval_ms)
145+
}
146+
}
147+
148+
///|
149+
/// Take ownership of `runtime_dir` for this host, or `None` if another
150+
/// process holds it.
151+
async fn claim(runtime_dir : @pathx.Path) -> Claim? {
129152
guard @fsx.FileLock::try_acquire(heartbeat_file(runtime_dir), Exclusive)
130153
is Some(lock) else {
131154
return None
@@ -140,7 +163,7 @@ pub async fn claim(runtime_dir : @pathx.Path) -> Claim? {
140163
/// Restamp for as long as the host runs. Spawned on the app's task group, so
141164
/// the stamp advances exactly while the loop that would act on a hand-off is
142165
/// running — which is the whole signal.
143-
pub async fn Claim::run(self : Claim) -> Unit {
166+
async fn Claim::run(self : Claim) -> Unit {
144167
for ;; {
145168
@async.sleep(HeartbeatIntervalMs)
146169
stamp(self.runtime_dir)
@@ -247,6 +270,32 @@ async test "a live owner with a stopped stamp cannot be handed to" {
247270
@fs.rmdir(root.to_string(), recursive=true)
248271
}
249272

273+
///|
274+
/// The primary can find the claim already taken — a host that started on the
275+
/// stopped-owner path holds one without ever entering an election. Giving up
276+
/// there would leave the process every later launch is handed to with no
277+
/// heartbeat at all, so it waits the other host out instead.
278+
async test "a primary that loses the claim takes it when the holder leaves" {
279+
let root : @pathx.Path = @fs.tmpdir(prefix="openseek-instance-wait-")
280+
guard claim(root) is Some(other) else { fail("expected the first claim") }
281+
@async.with_task_group(group => {
282+
group.spawn_bg(allow_failure=true, () => hold(root, retry_interval_ms=10))
283+
// Many retries' worth of waiting changes nothing while the other host is
284+
// there — the claim stays that host's.
285+
@async.sleep(50)
286+
assert_true(claim(root) is None)
287+
other.lock.release()
288+
// Now nobody else holds it, so a claim that still cannot be taken can
289+
// only be the waiting host's — and it stamps, which is what makes the
290+
// heartbeat describe the process a launch would be handed to.
291+
@async.sleep(50)
292+
assert_true(claim(root) is None)
293+
assert_true(can_hand_off_to(root))
294+
group.return_immediately(())
295+
})
296+
@fs.rmdir(root.to_string(), recursive=true)
297+
}
298+
250299
///|
251300
/// The probe must not keep what it takes. Two launches racing here both find
252301
/// the directory free and both go on to the single-instance election; if the

desktop/internal/instance/pkg.generated.mbti

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,25 +2,19 @@
22
package "openseek_desktop/internal/instance"
33

44
import {
5-
"openseek_desktop/internal/fsx",
65
"openseek_desktop/internal/pathx",
76
}
87

98
// Values
109
pub async fn can_hand_off_to(@pathx.Path) -> Bool
1110

12-
pub async fn claim(@pathx.Path) -> Claim?
11+
pub async fn hold(@pathx.Path, retry_interval_ms? : Int) -> Unit
1312

1413
pub async fn identity(@pathx.Path) -> String
1514

1615
// Errors
1716

1817
// Types and methods
19-
pub struct Claim {
20-
lock : @fsx.FileLock
21-
runtime_dir : @pathx.Path
22-
}
23-
pub async fn Claim::run(Self) -> Unit
2418

2519
// Type aliases
2620

desktop/main.mbt

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -134,25 +134,26 @@ fn main {
134134
None => ()
135135
}
136136
})
137-
// The heartbeat is claimed here and nowhere earlier: this hook runs
138-
// only in the process that won the single-instance election, so the
139-
// claim and the primary can never end up being different processes —
140-
// which would leave the primary unstamped and every later launch
141-
// mistaking a stopped host for a healthy one. It is spawned on the
142-
// application task group so it stops exactly when the loop it is
143-
// evidence of does.
137+
// Everything that acts on state belonging to whoever owns the runtime
138+
// directory starts here and nowhere earlier: this hook runs only in the
139+
// process that won the single-instance election. A launch that is
140+
// handed to another host returns from `app.run()` without ever reaching
141+
// it, so it can neither stamp a heartbeat the other host's would be
142+
// mistaken for, nor sweep away the relaunch marker that host is about
143+
// to act on. Both are spawned on the application task group, so they
144+
// stop exactly when the loop they belong to does.
144145
.app_lifecycle(
145146
on_start=async fn(context) {
146147
is_primary.val = true
148+
let tasks = context.task_group()
147149
match runtime_dir {
148150
Some(dir) =>
149-
if @instance.claim(dir) is Some(held) {
150-
context
151-
.task_group()
152-
.spawn_bg(allow_failure=true, () => held.run())
153-
}
151+
tasks.spawn_bg(allow_failure=true, () => @instance.hold(dir))
154152
None => ()
155153
}
154+
tasks.spawn_bg(allow_failure=true, () => {
155+
state.host_state().sweep_update_leftovers()
156+
})
156157
},
157158
// The claim needs no shutdown of its own: the kernel releases it when
158159
// the process dies, however it dies.

0 commit comments

Comments
 (0)