diff --git a/docs/explore-notes/conpty-sessions.md b/docs/explore-notes/conpty-sessions.md index c04c88a2..28c73c74 100644 --- a/docs/explore-notes/conpty-sessions.md +++ b/docs/explore-notes/conpty-sessions.md @@ -1,7 +1,7 @@ # ConPTY interactive sessions & launch specs > **Explore-note — verify before trusting.** Distilled map of a subsystem, not authoritative. -> Last verified against commit `ddf68d6` (2026-08-06). +> Last verified against commit `8dbdfb3` (2026-08-06). > Drift check: `git log --oneline bdee731..HEAD -- src/ClaudeDo.Worker/Planning src/ClaudeDo.Worker/Hub src/ClaudeDo.Worker/Runner/ClaudeArgsBuilder.cs src/ClaudeDo.Ui/ViewModels/MissionControlViewModel.cs src/ClaudeDo.Ui/Views/InteractiveTerminalView.axaml` > Stable structure only (no line numbers). See docs/explore-notes/README.md. @@ -144,6 +144,35 @@ add/remove/column change; focus-mode tab switches re-present content). Two-part (`InteractiveTerminalViewModel.IsStarting`) and in place of the refine button while `TaskRowViewModel.IsRefining`. +### ⚠️ Gotcha: env-var launch race across sessions + +`PtyTerminalSession.StartAsync` applies `TerminalLaunchDescriptor.Env` via +`Environment.SetEnvironmentVariable` onto the **whole UI process** (Porta.Pty has no per-launch +env seam — it always inherits the calling process's environment), then calls +`TerminalControl.LaunchProcess()`. Two sessions starting back-to-back (e.g. planning sessions for +two different tasks) could interleave: task B's `SetEnvironmentVariable` calls could land between +task A's env-set and its `LaunchProcess()` fork, so task A's `claude` process inherits B's env +(e.g. `CLAUDEDO_PLANNING_TOKEN`) and fails its own MCP auth. Fixed by serializing the +set-env-then-launch critical section behind a process-wide `static SemaphoreSlim(1,1)` in +`PtyTerminalSession`. Env leakage onto the whole process *after* a launch has forked remains a +documented limitation — only the fork-time race is closed. + +### ⚠️ Gotcha: open-path dedupe races + +`MissionControlViewModel.OpenConPtySessionAsync` / `OpenPlanningConPtySessionAsync` dedupe by +`TaskId` against `ConPtySessions`, but the check ran before an **awaited** DB title lookup and +only `AddConPtyPane` registers the pane — two rapid invocations for the same task (e.g. a +double-click) could both pass the dedupe check before either pane existed, opening two panes. +`OpenMergeHelperConPtySessionAsync` was worse: it awaits `CreateMergeHelperTaskAsync` (which mints +a brand-new task id every call) *before* any `TaskId` dedupe is even possible, so a double-trigger +always minted two host tasks in the DB. + +Fixed with synchronous, pre-await claims: `_pendingTaskOpens` (shared by the two `TaskId`-keyed +open paths) and `_pendingMergeHelperLists` (keyed by `listId`, guarding the whole method since +there's no `TaskId` yet to dedupe on) are `HashSet` fields checked-and-added at method +entry, before any `await`, and released in a `finally`. A second overlapping call for the same key +bails out immediately instead of racing past the collection-based dedupe. + ## Focus / key handling `InteractiveTerminalView` lives in `MissionControlWindow`, so the `FocusClearing` Escape handler diff --git a/src/ClaudeDo.Ui/Services/PtyTerminalSession.cs b/src/ClaudeDo.Ui/Services/PtyTerminalSession.cs index 1be2a2cd..aa1f5750 100644 --- a/src/ClaudeDo.Ui/Services/PtyTerminalSession.cs +++ b/src/ClaudeDo.Ui/Services/PtyTerminalSession.cs @@ -19,6 +19,14 @@ namespace ClaudeDo.Ui.Services; /// public sealed class PtyTerminalSession : IDisposable { + // Guards the set-env + LaunchProcess critical section below: two sessions starting + // back-to-back (e.g. planning sessions for two different tasks) could otherwise interleave + // their SetEnvironmentVariable calls before either LaunchProcess() forks, so one process + // inherits the other's env (e.g. CLAUDEDO_PLANNING_TOKEN, breaking that session's own MCP + // auth). Process-wide env leakage AFTER a launch has forked remains a documented limitation + // — Porta.Pty has no per-launch env seam, so the vars stay set on the whole UI process. + private static readonly SemaphoreSlim s_launchGate = new(1, 1); + private TerminalControl? _control; private bool _disposed; @@ -38,14 +46,22 @@ public sealed class PtyTerminalSession : IDisposable _control = control; control.ProcessExited += OnControlProcessExited; - foreach (var (key, value) in descriptor.Env) - Environment.SetEnvironmentVariable(key, value); - control.Process = descriptor.Exe; control.Args = new List(descriptor.Args); control.StartingDirectory = descriptor.Cwd; - await control.LaunchProcess(); + await s_launchGate.WaitAsync(ct); + try + { + foreach (var (key, value) in descriptor.Env) + Environment.SetEnvironmentVariable(key, value); + + await control.LaunchProcess(); + } + finally + { + s_launchGate.Release(); + } // Permanent reparent mode: TerminalView.OnDetachedFromLogicalTree kills the child // process unless BeginReparent() suppressed it, and Mission Control detaches pane diff --git a/src/ClaudeDo.Ui/ViewModels/MissionControlViewModel.cs b/src/ClaudeDo.Ui/ViewModels/MissionControlViewModel.cs index 187b0d7b..c4e739ab 100644 --- a/src/ClaudeDo.Ui/ViewModels/MissionControlViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/MissionControlViewModel.cs @@ -26,6 +26,13 @@ public sealed partial class MissionControlViewModel : ViewModelBase, IDisposable // review/merge/status machinery. public ObservableCollection ConPtySessions { get; } = new(); + // Claims a TaskId/listId for the duration of an Open*ConPtySessionAsync call so a second, + // overlapping invocation for the same key (e.g. a double-click) bails out instead of racing + // past the ConPtySessions dedupe check before the first call has added its pane. Checked and + // claimed synchronously at method entry — before any await — and released in a finally. + private readonly HashSet _pendingTaskOpens = new(); + private readonly HashSet _pendingMergeHelperLists = new(); + // Mirror of ConPtySessions typed as the pane abstraction so the layout toggle (grid/tabs) // binds one contract rather than a ConPTY-specific type. public ObservableCollection Panes { get; } = new(); @@ -145,18 +152,26 @@ public sealed partial class MissionControlViewModel : ViewModelBase, IDisposable FocusedPane = existing; return; } + if (!_pendingTaskOpens.Add(taskId)) return; - var title = taskId; try { - await using var ctx = await _dbFactory.CreateDbContextAsync(); - var entity = await ctx.Tasks.AsNoTracking().FirstOrDefaultAsync(t => t.Id == taskId); - if (entity?.Title is { Length: > 0 } t) title = t; - } - catch { /* best-effort title lookup */ } + var title = taskId; + try + { + await using var ctx = await _dbFactory.CreateDbContextAsync(); + var entity = await ctx.Tasks.AsNoTracking().FirstOrDefaultAsync(t => t.Id == taskId); + if (entity?.Title is { Length: > 0 } t) title = t; + } + catch { /* best-effort title lookup */ } - AddConPtyPane(new ConPtyPaneViewModel(taskId, title, - () => DescribeAsync(() => _worker.GetInteractiveLaunchSpecAsync(taskId)))); + AddConPtyPane(new ConPtyPaneViewModel(taskId, title, + () => DescribeAsync(() => _worker.GetInteractiveLaunchSpecAsync(taskId)))); + } + finally + { + _pendingTaskOpens.Remove(taskId); + } } // Starts (or resumes) a planning session and hosts it as an embedded ConPTY Command Center @@ -170,20 +185,28 @@ public sealed partial class MissionControlViewModel : ViewModelBase, IDisposable FocusedPane = existing; return; } + if (!_pendingTaskOpens.Add(taskId)) return; - var title = taskId; try { - await using var ctx = await _dbFactory.CreateDbContextAsync(); - var entity = await ctx.Tasks.AsNoTracking().FirstOrDefaultAsync(t => t.Id == taskId); - if (entity?.Title is { Length: > 0 } t) title = t; - } - catch { /* best-effort title lookup */ } - title += Loc.T("missionControl.planningTitleSuffix"); + var title = taskId; + try + { + await using var ctx = await _dbFactory.CreateDbContextAsync(); + var entity = await ctx.Tasks.AsNoTracking().FirstOrDefaultAsync(t => t.Id == taskId); + if (entity?.Title is { Length: > 0 } t) title = t; + } + catch { /* best-effort title lookup */ } + title += Loc.T("missionControl.planningTitleSuffix"); - AddConPtyPane(new ConPtyPaneViewModel(taskId, title, () => DescribeAsync(() => resume - ? _worker.GetPlanningResumeLaunchSpecAsync(taskId) - : _worker.GetPlanningStartLaunchSpecAsync(taskId)))); + AddConPtyPane(new ConPtyPaneViewModel(taskId, title, () => DescribeAsync(() => resume + ? _worker.GetPlanningResumeLaunchSpecAsync(taskId) + : _worker.GetPlanningStartLaunchSpecAsync(taskId)))); + } + finally + { + _pendingTaskOpens.Remove(taskId); + } } // Ad-hoc (task-less) ConPTY session in a user-chosen directory. Never deduped — every call @@ -207,38 +230,49 @@ public sealed partial class MissionControlViewModel : ViewModelBase, IDisposable public async System.Threading.Tasks.Task OpenMergeHelperConPtySessionAsync(string listId, IReadOnlyList taskIds) { if (taskIds is not { Count: > 0 }) return; + // The TaskId dedupe below can't help here — CreateMergeHelperTaskAsync mints a brand-new + // task id every call, so a double-trigger for the same list would always mint two host + // tasks before either pane exists to dedupe against. Guard the whole method per listId. + if (!_pendingMergeHelperLists.Add(listId)) return; - var title = Loc.T("missionControl.mergeHelperTitle"); - var listName = listId; try { - await using var ctx = await _dbFactory.CreateDbContextAsync(); - var list = await ctx.Lists.AsNoTracking().FirstOrDefaultAsync(l => l.Id == listId); - if (list?.Name is { Length: > 0 } name) { listName = name; title = $"{title} — {name}"; } - } - catch { /* best-effort title lookup */ } + var title = Loc.T("missionControl.mergeHelperTitle"); + var listName = listId; + try + { + await using var ctx = await _dbFactory.CreateDbContextAsync(); + var list = await ctx.Lists.AsNoTracking().FirstOrDefaultAsync(l => l.Id == listId); + if (list?.Name is { Length: > 0 } name) { listName = name; title = $"{title} — {name}"; } + } + catch { /* best-effort title lookup */ } - string taskId; - try - { - taskId = await _worker.CreateMergeHelperTaskAsync(taskIds, listId, - Loc.T("missionControl.mergeHelperTaskTitle", listName), - Loc.T("missionControl.mergeHelperTaskDescriptionHeader")); - } - catch (Exception ex) - { - ErrorReported?.Invoke(Loc.T("missionControl.conptyLaunchFailed", ex.Message)); - return; - } + string taskId; + try + { + taskId = await _worker.CreateMergeHelperTaskAsync(taskIds, listId, + Loc.T("missionControl.mergeHelperTaskTitle", listName), + Loc.T("missionControl.mergeHelperTaskDescriptionHeader")); + } + catch (Exception ex) + { + ErrorReported?.Invoke(Loc.T("missionControl.conptyLaunchFailed", ex.Message)); + return; + } - if (ConPtySessions.FirstOrDefault(s => s.TaskId == taskId) is { } existing) - { - FocusedPane = existing; - return; - } + if (ConPtySessions.FirstOrDefault(s => s.TaskId == taskId) is { } existing) + { + FocusedPane = existing; + return; + } - AddConPtyPane(new ConPtyPaneViewModel(taskId, title, - () => DescribeAsync(() => _worker.GetMergeHelperLaunchSpecAsync(taskIds, listId)))); + AddConPtyPane(new ConPtyPaneViewModel(taskId, title, + () => DescribeAsync(() => _worker.GetMergeHelperLaunchSpecAsync(taskIds, listId)))); + } + finally + { + _pendingMergeHelperLists.Remove(listId); + } } // List-handler handoff: the running session called handoff_list_handler at the end of Phase 2. diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/MissionControlViewModelTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/MissionControlViewModelTests.cs index c38442f2..97171709 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/MissionControlViewModelTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/MissionControlViewModelTests.cs @@ -47,6 +47,33 @@ public class MissionControlViewModelTests : IDisposable private MissionControlViewModel BuildVm(StubWorkerClient worker) => new MissionControlViewModel(new TestDbFactory(NewContext), worker, new UsagePillViewModel(worker)); + // Gates the title/list-name DB lookup behind a manually-released TaskCompletionSource so two + // overlapping Open*ConPtySessionAsync calls both run their synchronous dedupe-check prefix + // to completion before either resumes past the DB await. This reproduces the open-path races + // deterministically: a real async gap (e.g. Task.Yield) resumes its continuation on a + // thread-pool thread with no synchronization context to serialize it back onto the caller, + // which turns the repro into a genuine (flaky) data race instead of the intended + // same-thread double-invocation the bug describes. + private sealed class GatedDbFactory : IDbContextFactory + { + private readonly Func _create; + private readonly TaskCompletionSource _gate = new(); + public GatedDbFactory(Func create) => _create = create; + public ClaudeDoDbContext CreateDbContext() => _create(); + public async Task CreateDbContextAsync(CancellationToken ct = default) + { + await _gate.Task; + return _create(); + } + public void Release() => _gate.TrySetResult(); + } + + private MissionControlViewModel BuildGatedVm(StubWorkerClient worker, out GatedDbFactory factory) + { + factory = new GatedDbFactory(NewContext); + return new MissionControlViewModel(factory, worker, new UsagePillViewModel(worker)); + } + // ── acceptance criterion (a): TaskStarted must NOT add a pane ────────────── [Fact] @@ -290,6 +317,49 @@ public class MissionControlViewModelTests : IDisposable Assert.Single(vm.ConPtySessions); } + [Fact] + public async Task OpenConPtySessionAsync_OverlappingCallsSameTask_ProducesSinglePane() + { + var worker = new FakeWorker(); + using var vm = BuildGatedVm(worker, out var factory); + + var t1 = vm.OpenConPtySessionAsync("t1"); + var t2 = vm.OpenConPtySessionAsync("t1"); + factory.Release(); + await System.Threading.Tasks.Task.WhenAll(t1, t2); + + Assert.Single(vm.ConPtySessions); + Assert.Single(vm.Panes); + } + + [Fact] + public async Task OpenPlanningConPtySessionAsync_AddsPane_ToConPtySessionsAndPanes() + { + var worker = new FakeWorker(); + using var vm = BuildVm(worker); + + await vm.OpenPlanningConPtySessionAsync("t1", resume: false); + + Assert.Single(vm.ConPtySessions); + Assert.Equal("t1", vm.ConPtySessions[0].TaskId); + Assert.Single(vm.Panes); + } + + [Fact] + public async Task OpenPlanningConPtySessionAsync_OverlappingCallsSameTask_ProducesSinglePane() + { + var worker = new FakeWorker(); + using var vm = BuildGatedVm(worker, out var factory); + + var t1 = vm.OpenPlanningConPtySessionAsync("t1", resume: false); + var t2 = vm.OpenPlanningConPtySessionAsync("t1", resume: false); + factory.Release(); + await System.Threading.Tasks.Task.WhenAll(t1, t2); + + Assert.Single(vm.ConPtySessions); + Assert.Single(vm.Panes); + } + [Fact] public async Task OpenConPtySessionAsync_WorkerThrows_RaisesErrorReported_PaneShowsFailure() { @@ -474,6 +544,25 @@ public class MissionControlViewModelTests : IDisposable Assert.Same(firstPane, vm.FocusedPane); } + [Fact] + public async Task OpenMergeHelperConPtySessionAsync_OverlappingCallsSameList_CreatesOnlyOneHostTask() + { + var worker = new FixedTaskIdMergeHelperWorker(); + var factory = new GatedDbFactory(NewContext); + using var vm = new MissionControlViewModel(factory, worker, new UsagePillViewModel(worker)); + + // Both calls run their synchronous guard-check prefix before either resumes past the + // gated DB lookup — call 2 must lose the race and bail out immediately. + var t1 = vm.OpenMergeHelperConPtySessionAsync("L1", new[] { "t1" }); + var t2 = vm.OpenMergeHelperConPtySessionAsync("L1", new[] { "t1" }); + factory.Release(); + await System.Threading.Tasks.Task.WhenAll(t1, t2); + + Assert.Equal(1, worker.CreateCallCount); + Assert.Single(vm.ConPtySessions); + Assert.Single(vm.Panes); + } + [Fact] public async Task OpenMergeHelperConPtySessionAsync_EmptySelection_NoPane() {