releases/ClaudeDo
2
0
Fork 0
Code Issues 1 Pull Requests 2 Actions Packages Projects Releases 24 Wiki Activity

fix(ui): serialize ConPTY env launch and close open-path dedupe races

Browse Source 
Two sessions starting back-to-back could interleave SetEnvironmentVariable
calls before either LaunchProcess() forks, leaking one task's env (e.g.
CLAUDEDO_PLANNING_TOKEN) into another's claude process. Serialize the
set-env + LaunchProcess critical section behind a static SemaphoreSlim in
PtyTerminalSession.

OpenConPtySessionAsync/OpenPlanningConPtySessionAsync ran their TaskId
dedupe check before an awaited DB title lookup, and OpenMergeHelperConPtySessionAsync
awaited task creation before any dedupe was possible - rapid double-invocation
could open two panes or mint two host tasks. Claim the key synchronously at
method entry, before any await, and release it in a finally.
This commit is contained in:
mika kuns
2026-08-06 13:43:00 +02:00
parent 0d1e3b9a6f
commit 176ba78e11
3 changed files with 187 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ClaudeDo.Ui/Services/PtyTerminalSession.cs
+20 -4
View File
@@ -19,6 +19,14 @@ namespace ClaudeDo.Ui.Services;
/// </summary>
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<string>(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
src/ClaudeDo.Ui/ViewModels/MissionControlViewModel.cs
+78 -44
View File
@@ -26,6 +26,13 @@ public sealed partial class MissionControlViewModel : ViewModelBase, IDisposable
// review/merge/status machinery.
public ObservableCollection<ConPtyPaneViewModel> 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<string> _pendingTaskOpens = new();
private readonly HashSet<string> _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<IMissionControlPane> Panes { get; } = new();
@@ -151,18 +158,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
@@ -176,20 +191,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
@@ -213,38 +236,49 @@ public sealed partial class MissionControlViewModel : ViewModelBase, IDisposable
public async System.Threading.Tasks.Task OpenMergeHelperConPtySessionAsync(string listId, IReadOnlyList<string> 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.
tests/ClaudeDo.Ui.Tests/ViewModels/MissionControlViewModelTests.cs
+89
View File
@@ -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<ClaudeDoDbContext>
{
private readonly Func<ClaudeDoDbContext> _create;
private readonly TaskCompletionSource _gate = new();
public GatedDbFactory(Func<ClaudeDoDbContext> create) => _create = create;
public ClaudeDoDbContext CreateDbContext() => _create();
public async Task<ClaudeDoDbContext> 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]
@@ -246,6 +273,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()
{
@@ -430,6 +500,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()
{