fix(ui): serialize ConPTY env launch and close open-path dedupe races
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:
@@ -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()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user