Skip to content

Commit c624e63

Browse files
committed
fix(desktop): close the two ways a command still made a same-id twin
The population guard promised the app never adds to the duplicate id population, and two paths could still walk past it. `ensure_new_record_is_unique` probed only each other store's live `sessions/<id>`. A start naming a store whose neighbour holds that id *archived* was therefore treated as new and opened a record — and the duplicate appeared later, when the user restored what they had put away. Another store's archived record is one unarchive away from standing beside this one, so it counts, and its refusal says which state it found. `agent.goal` and `agent.compact` never asked at all. Both spawn an engine when the conversation has no live one, and a spawn writes the record, so either could file the second twin that `agent.start` refuses to. A goal is reachable before the first prompt — the composer offers one on a conversation that has never run — which is exactly when the id is new to its store and old to another. Reported by Codex on #886.
1 parent 2e81c2b commit c624e63

3 files changed

Lines changed: 105 additions & 0 deletions

File tree

desktop/internal/engine/archive.mbt

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,11 @@ async fn EngineManager::sweep_archived_deletions(self : EngineManager) -> Unit {
7373
/// not a creation, and passes untouched — including for an id that is
7474
/// duplicated on disk, because refusing to continue an existing record would
7575
/// strand the user with a conversation they can open but never answer.
76+
///
77+
/// Another store's *archived* record counts too. It is one unarchive away from
78+
/// being live beside this one, so treating the id as free here would create
79+
/// the very duplicate this guard promises the app never makes — and the user
80+
/// would meet it only later, when restoring the record they had put away.
7681
async fn ensure_new_record_is_unique(
7782
session : String,
7883
selected? : @pathx.Absolute,
@@ -88,6 +93,16 @@ async fn ensure_new_record_is_unique(
8893
"this conversation already has a record in \{root}; start a new one here",
8994
)
9095
}
96+
if @fsx.is_dir(
97+
@session_store.archived_root(root)
98+
.join("sessions")
99+
.join(session)
100+
.to_path(),
101+
) {
102+
raise EngineError(
103+
"this conversation has an archived record in \{root}; start a new one here",
104+
)
105+
}
91106
}
92107
}
93108

desktop/internal/engine/ops.mbt

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,10 @@ pub async fn compact_run(
250250
// reused engine never builds one, and the store must be known either way.
251251
let store = requested_store(payload.workspace)
252252
@session_store.ensure_not_archived(session, Some(store))
253+
// A compaction with no live engine spawns one, and a spawn writes the
254+
// record: without this the command would open a second record for an id
255+
// another store already owns, which `start_run` refuses.
256+
ensure_new_record_is_unique(session, selected=store)
253257
let sessions = manager.serving()
254258
let slot = slot_for(sessions, session)
255259
// Claim before anything else — even with a live engine. A start between
@@ -352,6 +356,9 @@ pub async fn goal_run(
352356
// the slot is read — see `compact_run`.
353357
let store = requested_store(payload.workspace)
354358
@session_store.ensure_not_archived(session, Some(store))
359+
// A goal is reachable before the first prompt, and its spawn writes the
360+
// record just as a start's does — see `compact_run`.
361+
ensure_new_record_is_unique(session, selected=store)
355362
let sessions = manager.serving()
356363
let slot = slot_for(sessions, session)
357364
// Claim before looking at the engine, exactly as compact_run does: this op

desktop/internal/engine/store_identity_wbtest.mbt

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,89 @@ async test "a start cannot open a second record for an id another store owns" {
139139
@fs.rmdir(root.to_string(), recursive=true)
140140
}
141141

142+
///|
143+
#cfg(not(platform="windows"))
144+
async test "no command opens a second record for an id another store owns" {
145+
ambient_env_test_lock.acquire()
146+
defer ambient_env_test_lock.release()
147+
let previous = @sys.get_env_var("OPENSEEK_SESSION_ROOT")
148+
let (root, actor) = store_identity_host("openseek-unique-commands-")
149+
defer restore_session_root_env(previous)
150+
let manager = actor.manager()
151+
let owner = root.join("owner")
152+
let ws = root.join("ws")
153+
@fsx.ensure_dir(ws.to_path())
154+
@fsx.ensure_dir(owner.to_path())
155+
let sink = EventSink(fn(_) { })
156+
@async.with_task_group(group => {
157+
group.spawn_bg(no_wait=true, allow_failure=true, () => {
158+
actor.run(sink, fn(_) { })
159+
})
160+
while manager.pump is Stopped {
161+
@async.sleep(1)
162+
}
163+
ignore(@workspaces.add(owner.to_string(), fn(_) { }))
164+
ignore(@workspaces.add(ws.to_string(), fn(_) { }))
165+
// The first project owns `s-live`. A goal is reachable before the first
166+
// prompt, and its spawn writes a record, so it must refuse for the same
167+
// reason a start does rather than filing the second twin itself.
168+
@fsx.ensure_dir(
169+
@workspaces.store_root(owner).join("sessions/s-live").to_path(),
170+
)
171+
let goal_refusal = try {
172+
ignore(
173+
goal_run(manager, {
174+
session: "s-live",
175+
workspace: ws.to_string(),
176+
text: Some("ship it"),
177+
auto: None,
178+
model: None,
179+
max_steps: None,
180+
}),
181+
)
182+
"no error"
183+
} catch {
184+
EngineError(detail) => detail
185+
error if @async.is_being_cancelled() => raise error
186+
error => "\{error}"
187+
}
188+
assert_true(goal_refusal.has_suffix("; start a new one here"))
189+
guard manager.pump is Serving(sessions~) else { fail("pump stopped") }
190+
assert_true(sessions.get("s-live") is None)
191+
// An *archived* record in the other store counts as well: it is one
192+
// unarchive away from standing beside whatever this start would create.
193+
@fsx.ensure_dir(
194+
@workspaces.store_root(owner)
195+
.join("archived/sessions/s-put-away")
196+
.to_path(),
197+
)
198+
let start_refusal = try {
199+
ignore(
200+
start_run(sink, manager, {
201+
task: "fork me",
202+
submission_id: None,
203+
model: None,
204+
max_steps: None,
205+
session: "s-put-away",
206+
workspace: ws.to_string(),
207+
}),
208+
)
209+
"no error"
210+
} catch {
211+
EngineError(detail) => detail
212+
error if @async.is_being_cancelled() => raise error
213+
error => "\{error}"
214+
}
215+
assert_true(start_refusal.contains("archived record"))
216+
assert_true(
217+
start_refusal.contains(@workspaces.store_root(owner).to_string()),
218+
)
219+
assert_true(sessions.get("s-put-away") is None)
220+
group.return_immediately(())
221+
})
222+
@fs.rmdir(root.to_string(), recursive=true)
223+
}
224+
142225
///|
143226
#cfg(not(platform="windows"))
144227
async test "an archived record in another store does not block a live one" {

0 commit comments

Comments
 (0)