From 51957eb0ac25a6fa11b8e1aad6ec2664e395c8dc Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Fri, 12 Jun 2026 01:08:33 +0200 Subject: [PATCH] coalesce concurrent session spawns onto one future (T-374) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Orchestrator.spawn() check-then-acts on the session registry across two awaits (transcript-tail read, process start) — two racing callers for the same id both passed the check and the loser's live claude process was orphaned, never killed, never observed. The first caller now installs the spawn future synchronously; later callers await the same future, and a failed spawn clears the entry so a retry proceeds. Co-Authored-By: Claude Fable 5 --- .pql/changelog/ticket_history/2026-06.sql | 2 ++ .pql/changelog/tickets/2026-06.sql | 10 +++++++ CHANGELOG.md | 5 ++++ .../claude/src/session_orchestrator.dart | 23 +++++++++++++++- .../claude/session_orchestrator_test.dart | 26 +++++++++++++++++++ 5 files changed, 65 insertions(+), 1 deletion(-) diff --git a/.pql/changelog/ticket_history/2026-06.sql b/.pql/changelog/ticket_history/2026-06.sql index ac411854..66d7c54e 100644 --- a/.pql/changelog/ticket_history/2026-06.sql +++ b/.pql/changelog/ticket_history/2026-06.sql @@ -3999,3 +3999,5 @@ INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, chang INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHD098CV2N73823KX4Z99P4', 'status', 'in_progress', 'done', NULL, '2026-06-11 23:04:46', '2026-06-11 23:04:46', '2026-06-11 23:04:46', NULL, '5d8d8c7a17da4894510db957e7af3c3f', 2) ON CONFLICT(hash) DO NOTHING; INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHCP03EJ9CDBGZGRPD19N8W', 'status', 'backlog', 'in_progress', NULL, '2026-06-11 23:05:16', '2026-06-11 23:05:16', '2026-06-11 23:05:16', NULL, '252a8c28446f3c86876ec826fce03987', 2) ON CONFLICT(hash) DO NOTHING; INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHCP03EJ9CDBGZGRPD19N8W', 'status', 'in_progress', 'done', NULL, '2026-06-11 23:06:26', '2026-06-11 23:06:26', '2026-06-11 23:06:26', NULL, '4e1cc82f703d717f7592890e230caf00', 2) ON CONFLICT(hash) DO NOTHING; +INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHC90B72A270CAKA7AP1ZX8', 'status', 'backlog', 'in_progress', NULL, '2026-06-11 23:07:01', '2026-06-11 23:07:01', '2026-06-11 23:07:01', NULL, '795c139edb5acd0d2a187dac6a3066c7', 2) ON CONFLICT(hash) DO NOTHING; +INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHC90B72A270CAKA7AP1ZX8', 'status', 'in_progress', 'done', NULL, '2026-06-11 23:08:13', '2026-06-11 23:08:13', '2026-06-11 23:08:13', NULL, 'f83a2fc13f3839070957bea6ac7fdc5f', 2) ON CONFLICT(hash) DO NOTHING; diff --git a/.pql/changelog/tickets/2026-06.sql b/.pql/changelog/tickets/2026-06.sql index f905f89b..d3c8f52f 100644 --- a/.pql/changelog/tickets/2026-06.sql +++ b/.pql/changelog/tickets/2026-06.sql @@ -4099,3 +4099,13 @@ INSERT INTO tickets (record_id, type, parent_record_id, title, description, stat Fix: pass the active workspace root as the spawn cwd (and re-derive it on project switch for new panes). Acceptance: test that a terminal pane''s SpawnSpec.cwd equals the workspace root, not Directory.current.', 'done', 'medium', NULL, NULL, NULL, '2026-06-11 21:59:42', '2026-06-11 23:06:26', NULL, '8cad068c28a7a0cb6772a5fbdcbc34c3', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash); +INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHC90B72A270CAKA7AP1ZX8', 'bug', '06FBHBGHNEQTAEPGNJKN42C1E8', 'Orchestrator.spawn() races itself — concurrent spawns for one id leak a live claude process', 'lib/builtin/claude/src/session_orchestrator.dart:191-249 — spawn() does check-then-act on the sessions map across two awaits (transcript-tail read, process start). Two concurrent spawns for the same session id both pass the check; the loser''s live claude process is orphaned, never killed, never observed. + +Fix: hold a Map> — first caller installs the future synchronously, later callers await the same future; remove the entry on failure. + +Acceptance: test issuing two concurrent spawn() calls for one id yields the same ManagedSession instance and exactly one process spawn (count via injected spawner); failure path clears the in-flight entry so a retry can proceed.', 'in_progress', 'medium', NULL, NULL, NULL, '2026-06-11 21:57:56', '2026-06-11 23:07:01', NULL, 'f2df2148315332d48e2bf66cc2942bd3', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash); +INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBHC90B72A270CAKA7AP1ZX8', 'bug', '06FBHBGHNEQTAEPGNJKN42C1E8', 'Orchestrator.spawn() races itself — concurrent spawns for one id leak a live claude process', 'lib/builtin/claude/src/session_orchestrator.dart:191-249 — spawn() does check-then-act on the sessions map across two awaits (transcript-tail read, process start). Two concurrent spawns for the same session id both pass the check; the loser''s live claude process is orphaned, never killed, never observed. + +Fix: hold a Map> — first caller installs the future synchronously, later callers await the same future; remove the entry on failure. + +Acceptance: test issuing two concurrent spawn() calls for one id yields the same ManagedSession instance and exactly one process spawn (count via injected spawner); failure path clears the in-flight entry so a retry can proceed.', 'done', 'medium', NULL, NULL, NULL, '2026-06-11 21:57:56', '2026-06-11 23:08:13', NULL, 'fde3f9e912ea1d25256003ef56dcf491', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash); diff --git a/CHANGELOG.md b/CHANGELOG.md index ca0cf08d..42833ffb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,6 +94,11 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit. app process's working directory — `$HOME` for desktop launches, and the wrong repo after a project switch. (T-381) +- **Two simultaneous spawns of the same Claude session no longer leak a + process.** Concurrent spawn calls for one pane id both passed the registry + check and the loser's live process was orphaned; spawns for an id are now + coalesced onto one in-flight future. (T-374) + - **Accepting ExitPlanMode now leaves plan mode in the conversation panel.** Approving Claude's plan (the ExitPlanMode tool) transitioned the underlying session out of plan mode, but clide's tracked permission mode didn't follow, diff --git a/lib/builtin/claude/src/session_orchestrator.dart b/lib/builtin/claude/src/session_orchestrator.dart index 98125c08..5674f698 100644 --- a/lib/builtin/claude/src/session_orchestrator.dart +++ b/lib/builtin/claude/src/session_orchestrator.dart @@ -188,7 +188,28 @@ class ClaudeSessionOrchestrator extends ChangeNotifier { /// (T-269): the existing session belongs to the old repo, so it is torn down /// and a fresh one spawned for the new repo — a pane must never inherit /// another workspace's conversation. - Future spawn(SpawnSpec spec) async { + Future spawn(SpawnSpec spec) { + // Serialize concurrent spawns per id (T-374): the body check-then-acts + // on _sessions across two awaits, so two racing callers would both + // pass the check and the loser's live claude process would be orphaned. + // The first caller installs the future synchronously; the rest await + // it. (A racing different-cwd spawn for the same id also coalesces — + // the workspace-switch flow is sequential, so that pair never races.) + final inFlight = _spawning[spec.id]; + if (inFlight != null) return inFlight; + final f = _spawn(spec); + _spawning[spec.id] = f; + unawaited( + f.then((_) {}, onError: (Object _) {}).whenComplete(() { + if (identical(_spawning[spec.id], f)) _spawning.remove(spec.id); + }), + ); + return f; + } + + final Map> _spawning = {}; + + Future _spawn(SpawnSpec spec) async { final existing = _sessions[spec.id]; if (existing != null) { if (existing.cwd == spec.cwd) return existing; diff --git a/test/builtin/claude/session_orchestrator_test.dart b/test/builtin/claude/session_orchestrator_test.dart index a7a14bb5..50b68ac6 100644 --- a/test/builtin/claude/session_orchestrator_test.dart +++ b/test/builtin/claude/session_orchestrator_test.dart @@ -60,6 +60,32 @@ void main() { expect(created, hasLength(1)); }); + // T-374: spawn() check-then-acts across awaits; without the in-flight + // map, two CONCURRENT spawns both passed the registry check and the + // loser's live claude process was orphaned. + test('two concurrent spawns for one id share one session and one process (T-374)', () async { + final (a, b) = await (orch.spawn(spec('primary')), orch.spawn(spec('primary'))).wait; + expect(identical(a, b), isTrue); + expect(created, hasLength(1)); + }); + + test('a failed spawn clears the in-flight entry so a retry can proceed (T-374)', () async { + var calls = 0; + final flaky = ClaudeSessionOrchestrator( + processFactory: ({required sessionArgs, required cwd, env}) async { + calls++; + if (calls == 1) throw StateError('spawn blew up'); + final p = _FakeProc(); + created.add(p); + return p; + }, + ); + await expectLater(flaky.spawn(spec('primary')), throwsStateError); + final m = await flaky.spawn(spec('primary')); + expect(m.id, 'primary'); + expect(calls, 2); + }); + test('hide keeps the process alive and in the registry; show restores it', () async { await orch.spawn(spec('primary')); orch.hide('primary');