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');