From e41bafdb7cd7fe3e3b0aaf09ec3127b65d9fa6b0 Mon Sep 17 00:00:00 2001 From: Nucleic Date: Wed, 29 Jul 2026 17:09:35 -0700 Subject: [PATCH] Merge nucleic/lucid-river-toad-6efj into dev --- NucleicBroker.Tests/BrokerServiceTests.cs | 61 ++++++++++++ NucleicBroker.Tests/FakeWslc.cs | 21 ++++- NucleicBroker/BrokerService.cs | 39 +++++++- NucleicBroker/IWslc.cs | 33 ++++++- NucleicBroker/Wslc/WslcFacade.cs | 110 +++++++++++++++++++--- spikes/WslcSpike/BrokerClient.cs | 8 ++ spikes/WslcSpike/Program.cs | 77 ++++++++++----- 7 files changed, 305 insertions(+), 44 deletions(-) diff --git a/NucleicBroker.Tests/BrokerServiceTests.cs b/NucleicBroker.Tests/BrokerServiceTests.cs index 3c08033..abbd86d 100644 --- a/NucleicBroker.Tests/BrokerServiceTests.cs +++ b/NucleicBroker.Tests/BrokerServiceTests.cs @@ -129,6 +129,67 @@ public sealed class BrokerServiceTests Assert.Equal("session_exists", error.GetProperty("data").GetProperty("kind").GetString()); } + /// + /// Regression for the bug the first live run hit (docs/WINDOWS_PORT.md §13.3): a command that + /// finishes during start — `echo` — used to put `proc.stdout`/`proc.exit` on the wire BEFORE + /// the response carrying the `procId` that names them. A client that registers interest when + /// it learns the procId then never sees the exit and waits forever, which is exactly how the + /// spike hung. + /// + /// The fix is that BrokerService responds before calling StartAsync, so this asserts the + /// ORDER and not merely that the messages all arrived. + /// + [Fact] + public async Task ProcExec_RespondsBeforeAnyOutputFromAnInstantProcess() + { + wslc.ContainerStates["box"] = "running"; + // The fake emits during StartAsync, standing in for a process that has already exited by + // the time Start() returns. + wslc.NextProcessOnStart = () => + { + wslc.Events!.ProcOutput(wslc.LastProcId, stderr: false, "done\n"u8); + wslc.Events!.ProcExited(wslc.LastProcId, 0); + }; + + var lines = await RoundTrip( + """{"jsonrpc":"2.0","id":1,"method":"proc.exec","params":{"container":"box","argv":["echo","done"],"tty":false}}"""); + + Assert.True(wslc.LastProcess!.Started, "BrokerService must start the process"); + // Line 0 must be the response: everything else is meaningless to a client that cannot yet + // attribute the notifications. + var procId = Result(lines[0]).GetProperty("procId").GetInt64(); + Assert.Equal(wslc.LastProcId, procId); + var methods = lines.Skip(1) + .Select(l => l.TryGetProperty("method", out var m) ? m.GetString() : null).ToList(); + Assert.Equal(["proc.stdout", "proc.exit"], methods); + } + + /// + /// The response is enqueued before the process is started, so a start failure can no longer be + /// a JSON-RPC error — that would put TWO responses under one id. Observed on hardware + /// (docs/WINDOWS_PORT.md §13.3), where the client then waited for an exit that never came. + /// It must arrive as the process's own stderr plus an exit instead. + /// + [Fact] + public async Task ProcExec_StartFailure_ReportsAsExitNotSecondResponse() + { + wslc.ContainerStates["box"] = "running"; + wslc.NextProcessOnStart = () => throw new WslcError(WslcError.StartFailed, "container is not running"); + + var lines = await RoundTrip( + """{"jsonrpc":"2.0","id":1,"method":"proc.exec","params":{"container":"box","argv":["sh"],"tty":false}}"""); + + // Exactly one message carries id 1 — anything else is a protocol violation. + var responses = lines.Where(l => l.TryGetProperty("id", out var i) && i.GetInt32() == 1).ToList(); + Assert.Single(responses); + Assert.False(responses[0].TryGetProperty("error", out _), "must be the success response"); + + var methods = lines.Skip(1) + .Select(l => l.TryGetProperty("method", out var m) ? m.GetString() : null).ToList(); + Assert.Equal(["proc.stderr", "proc.exit"], methods); + Assert.Equal(126, lines.Last().GetProperty("params").GetProperty("code").GetInt32()); + } + [Fact] public async Task ImagePull_EmitsProgressNotificationsBeforeResult() { diff --git a/NucleicBroker.Tests/FakeWslc.cs b/NucleicBroker.Tests/FakeWslc.cs index da88094..7aacfa9 100644 --- a/NucleicBroker.Tests/FakeWslc.cs +++ b/NucleicBroker.Tests/FakeWslc.cs @@ -22,6 +22,10 @@ public sealed class FakeWslc : IWslc public long LastProcId; public Exception? NextError; + /// Set before a proc.exec to make that process emit during StartAsync — i.e. behave + /// like a command that finishes instantly. + public Action? NextProcessOnStart; + public string? WslcVersion => "9.9-test"; /// The fake is deliberately fully-capable: it stands in for a facade with BOTH wslc surfaces @@ -157,7 +161,10 @@ public sealed class FakeWslc : IWslc if (ContainerStates.GetValueOrDefault(spec.Container) != "running") throw new WslcError(WslcError.NotRunning, $"container {spec.Container} isn't running"); LastProcId = procId; - LastProcess = new FakeProcess(); + // Mirror the real facade: create and attach, but do NOT run — BrokerService runs it after + // the procId response is on the wire. + LastProcess = new FakeProcess { OnStart = NextProcessOnStart }; + NextProcessOnStart = null; return Task.FromResult(LastProcess); } @@ -167,6 +174,18 @@ public sealed class FakeWslc : IWslc public bool StdinClosed; public List Signals = []; public (int Cols, int Rows)? LastResize; + public bool Started; + + /// Lets a test stand in for a process that finishes DURING start — an `echo`, + /// which is what exposed the response-ordering bug on hardware. + public Action? OnStart; + + public Task StartAsync(CancellationToken ct) + { + Started = true; + OnStart?.Invoke(); + return Task.CompletedTask; + } public Task WriteStdinAsync(ReadOnlyMemory data, CancellationToken ct) { diff --git a/NucleicBroker/BrokerService.cs b/NucleicBroker/BrokerService.cs index cd97724..7810bcd 100644 --- a/NucleicBroker/BrokerService.cs +++ b/NucleicBroker/BrokerService.cs @@ -58,8 +58,11 @@ public sealed class BrokerService : IBrokerEvents try { - var result = await DispatchAsync(method, @params, ct).ConfigureAwait(false); - if (id is { } requestId) + var result = await DispatchAsync(method, @params, id, ct).ConfigureAwait(false); + // A null result means the handler already enqueued its own response because it had to + // send it before doing something else (proc.exec — see below). Everything else + // returns a value and is responded to here. + if (result is not null && id is { } requestId) outbound.EnqueueJson(Rpc.Response(requestId, result)); } catch (JsonException e) @@ -80,7 +83,10 @@ public sealed class BrokerService : IBrokerEvents } } - private async Task DispatchAsync(string method, JsonElement? p, CancellationToken ct) + /// Returns the result to respond with, or null when the handler has already + /// responded (it needed the response on the wire before continuing). + private async Task DispatchAsync( + string method, JsonElement? p, JsonElement? id, CancellationToken ct) { switch (method) { @@ -176,7 +182,32 @@ public sealed class BrokerService : IBrokerEvents var procId = Interlocked.Increment(ref nextProcId); var proc = await wslc.ExecAsync(procId, spec, ct).ConfigureAwait(false); lock (procsLock) procs[procId] = proc; - return new { procId }; + // Respond BEFORE running it. Output and exit are enqueued on the same ordered + // outbound queue as this response, so a command that finishes fast (`echo`) would + // otherwise put `proc.exit` on the wire ahead of the `procId` naming it — and a + // client that registers interest when it learns the procId then waits forever. + // Hit on the first live run, hardware-confirmed (docs/WINDOWS_PORT.md §13.3). + if (id is { } requestId) + outbound.EnqueueJson(Rpc.Response(requestId, new { procId })); + + try + { + await proc.StartAsync(ct).ConfigureAwait(false); + } + catch (Exception e) + { + // The response is already on the wire, so this failure CANNOT be reported as a + // JSON-RPC error — that would put two responses under one id, which is a + // protocol violation and left the client waiting for an exit that never came. + // Report it the way the process itself would have: the reason on stderr, then + // an exit. 126 is the shell's "command found but not executable", which is + // what "could not start" means to every caller above. + lock (procsLock) procs.Remove(procId); + var reason = $"nucleic-brokerd: could not start process: {e.Message}\n"; + ProcOutput(procId, stderr: true, System.Text.Encoding.UTF8.GetBytes(reason)); + ProcExited(procId, 126); + } + return null; } case "proc.stdin": { diff --git a/NucleicBroker/IWslc.cs b/NucleicBroker/IWslc.cs index 4e60873..0ec29e4 100644 --- a/NucleicBroker/IWslc.cs +++ b/NucleicBroker/IWslc.cs @@ -64,14 +64,32 @@ public interface IWslc /// cgroup counters for the resource monitor; null when not running. Task ContainerStatsAsync(string name, CancellationToken ct); - /// Start a process in a running container. `procId` is minted by the broker and - /// keys every event this process emits through . + /// + /// Create a process in a running container and attach its event handlers, but **do not run + /// it** — the caller runs it with once it has sent the + /// `procId` downstream. `procId` is minted by the broker and keys every event this process + /// emits through . + /// + /// The two-step split is not ceremony. See . + /// Task ExecAsync(long procId, ProcSpec spec, CancellationToken ct); } /// Control half of a running in-container process (output arrives via events). public interface IWslcProcess { + /// + /// Actually run the process. Separate from because a short + /// command can finish before the `proc.exec` RESPONSE has been written: output and exit ride + /// the same ordered outbound queue, so starting first puts `proc.exit` on the wire ahead of + /// the `procId` that identifies it, and a client that registers interest on receiving that + /// procId waits forever. Observed on hardware with `echo` (docs/WINDOWS_PORT.md §13.3). + /// + /// This is the same reasoning that makes wslc itself split `CreateProcess` from `Start` — so + /// handlers can attach before output flows — applied one level up, to the RPC boundary. + /// + Task StartAsync(CancellationToken ct); + Task WriteStdinAsync(ReadOnlyMemory data, CancellationToken ct); Task CloseStdinAsync(CancellationToken ct); Task SignalAsync(int signal, CancellationToken ct); @@ -107,6 +125,17 @@ public sealed class WslcError(string kind, string message) : Exception(message) /// permanent capability gap, not a transient failure — hostd must not retry. public const string Unsupported = "unsupported"; + /// + /// A CONTAINER of that name already exists in the session. Distinct from + /// because wslc answers `ERROR_ALREADY_EXISTS` for both and the + /// remedies differ completely — remove one container, versus restart the whole WSL stack. + /// + /// Reachable today because this broker's container roster is process-local (there is no + /// enumeration on the compat surface), so a container left behind by a crashed broker holds + /// its name against every later one. See docs/WINDOWS_PORT.md §13.3. + /// + public const string AlreadyExists = "already_exists"; + /// A session of that name is already running and this facade cannot re-adopt it /// (the compat SDK's `Start()` answers ERROR_ALREADY_EXISTS, and its constructor is lazy, so /// a second handle is not a second session). Distinct from because diff --git a/NucleicBroker/Wslc/WslcFacade.cs b/NucleicBroker/Wslc/WslcFacade.cs index 73001a6..e5d9eb7 100644 --- a/NucleicBroker/Wslc/WslcFacade.cs +++ b/NucleicBroker/Wslc/WslcFacade.cs @@ -72,7 +72,17 @@ public sealed class WslcFacade : IWslc private readonly Dictionary containers = []; private readonly Lock containersLock = new(); - private sealed record Entry(Sdk.Container Container, string Image); + /// + /// A container handle plus what we have learned about it. SetprivWorks is resolved + /// lazily on the first uid-dropping exec and then cached — see + /// . + /// + private sealed class Entry(Sdk.Container container, string image) + { + internal Sdk.Container Container { get; } = container; + internal string Image { get; } = image; + internal bool? SetprivWorks { get; set; } + } public string? WslcVersion { @@ -541,7 +551,7 @@ public sealed class WslcFacade : IWslc // MARK: - Processes - public Task ExecAsync(long procId, ProcSpec spec, CancellationToken ct) + public async Task ExecAsync(long procId, ProcSpec spec, CancellationToken ct) { if (spec.Tty) // Not a failure to retry: ProcessSettings has no Terminal and Process has no resize. @@ -553,7 +563,7 @@ public sealed class WslcFacade : IWslc var container = RequireContainer(spec.Container); var settings = new Sdk.ProcessSettings { - CommandLine = WithPrivilegeDrop(spec).ToList(), // CommandLine, not CmdLine + CommandLine = (await WithPrivilegeDropAsync(spec, ct).ConfigureAwait(false)).ToList(), OutputMode = Sdk.ProcessOutputMode.Event, }; if (spec.Cwd is { } cwd) settings.WorkingDirectory = cwd; @@ -578,27 +588,35 @@ public sealed class WslcFacade : IWslc process.ErrorReceived += data => events?.ProcOutput(procId, stderr: true, data); process.Exited += code => events?.ProcExited(procId, code); - try - { - process.Start(); - } - catch (Exception e) when (e is not WslcError) - { - process.Dispose(); - throw Translate(e, WslcError.StartFailed); - } - return Task.FromResult(new WslcProcess(process)); + // NOT started here — BrokerService starts it after the `procId` response is on the wire. + // See IWslcProcess.StartAsync for why that ordering is load-bearing. + return new WslcProcess(process); } /// /// ProcessSettings has no UserId/GroupId, so dropping to the agent uid is /// done in-guest by wrapping argv — the fallback §3.2 always named, which costs nothing /// because the interceptors and nash never look at the numeric uid. + /// + /// argv is passed to `setpriv` directly rather than through a shell, so nothing here can be + /// quoted wrong or injected into. /// - private static IReadOnlyList WithPrivilegeDrop(ProcSpec spec) + private async Task> WithPrivilegeDropAsync( + ProcSpec spec, CancellationToken ct) { if (spec.Uid is not { } uid || uid == 0) return spec.Argv; var gid = spec.Gid ?? uid; + + if (!await ResolvePrivilegeDropAsync(spec.Container, ct).ConfigureAwait(false)) + // Refuse rather than run the agent as root. This path exists because the alternative + // is a silent privilege escalation: the caller asked for uid 501 and got 0, in the + // one place the sandbox's user separation is enforced. + throw new WslcError( + WslcError.Unsupported, + $"cannot drop to uid {uid} in this image: it has no util-linux `setpriv` " + + "(BusyBox ships a `setpriv` that does not support --reuid). Use an image with " + + "util-linux, as the narOS agent image does — refusing to run as root instead."); + return [ "setpriv", $"--reuid={uid}", $"--regid={gid}", "--init-groups", "--", @@ -606,6 +624,48 @@ public sealed class WslcFacade : IWslc ]; } + /// + /// Does this image have a `setpriv` that can actually change uid? Probed once per container + /// and cached, because the answer is a property of the image and an extra exec per agent + /// command would not be free. + /// + /// The probe is `setpriv --reuid=0 --regid=0 --init-groups -- true`: a no-op on util-linux, + /// and an "unrecognized option" failure on BusyBox's namesake, which accepts only capability + /// flags. Testing for the *binary* is not enough — BusyBox has one, it just cannot do this + /// (observed on hardware, docs/WINDOWS_PORT.md §13.3). + /// + private async Task ResolvePrivilegeDropAsync(string name, CancellationToken ct) + { + Entry entry; + lock (containersLock) + { + if (!containers.TryGetValue(name, out entry!)) + throw new WslcError(WslcError.NotFound, $"no container named {name}"); + if (entry.SetprivWorks is { } cached) return cached; + } + + bool works; + try + { + var (exitCode, _) = await RunCapturingAsync( + entry.Container, + ["setpriv", "--reuid=0", "--regid=0", "--init-groups", "--", "true"], + TimeSpan.FromSeconds(15), ct).ConfigureAwait(false); + works = exitCode == 0; + } + catch (Exception) + { + works = false; + } + + lock (containersLock) entry.SetprivWorks = works; + if (!works) + Console.Error.WriteLine( + $"wslc: container '{name}' has no usable setpriv — uid-dropping execs will be " + + "refused rather than run as root"); + return works; + } + /// Run to completion and capture stdout+stderr. The in-guest half of what the macOS /// engine's `runCapturing` does, and the only way stats and shim re-seeding work without a /// second mechanism. @@ -654,6 +714,20 @@ public sealed class WslcFacade : IWslc private sealed class WslcProcess(Sdk.Process process) : IWslcProcess { + public Task StartAsync(CancellationToken ct) + { + try + { + process.Start(); + } + catch (Exception e) when (e is not WslcError) + { + process.Dispose(); + throw Translate(e, WslcError.StartFailed); + } + return Task.CompletedTask; + } + // stdin is a WinRT stream, not a WriteStdin call, and DataWriter is how you put bytes // into an IOutputStream. Held for the process lifetime and guarded, because hostd may // pipeline proc.stdin writes and StoreAsync is not reentrant. @@ -752,7 +826,13 @@ public sealed class WslcFacade : IWslc 0x80040605 => WslcError.NotRunning, // WSLC_E_CONTAINER_NOT_RUNNING 0x8004060F => WslcError.NotFound, // WSLC_E_SESSION_NOT_FOUND 0x80040607 => WslcError.SessionExists, // WSLC_E_SESSION_RESERVED - 0x800700B7 => WslcError.SessionExists, // ERROR_ALREADY_EXISTS + // ERROR_ALREADY_EXISTS is CONTEXT-FREE: wslc returns it for a session name conflict + // AND a container name conflict. It used to map to session_exists here, which made a + // stale container report itself as a stuck session — a wrong diagnosis with a wrong + // remedy (`wsl --shutdown` instead of removing one container). The session paths catch + // this code by number before reaching Translate, so anything arriving here is the + // other kind. + 0x800700B7 => WslcError.AlreadyExists, // ERROR_ALREADY_EXISTS (container name in use) // Nothing installed vs. installed-but-too-old: opposite diagnoses, and both mean the // sandbox is unusable rather than this call being wrong. 0x80040154 => WslcError.Unavailable, // REGDB_E_CLASSNOTREG diff --git a/spikes/WslcSpike/BrokerClient.cs b/spikes/WslcSpike/BrokerClient.cs index 98eaea6..066b694 100644 --- a/spikes/WslcSpike/BrokerClient.cs +++ b/spikes/WslcSpike/BrokerClient.cs @@ -24,6 +24,11 @@ internal sealed class BrokerClient : IAsyncDisposable /// reader thread. Handlers must not block. internal event Action? Notification; + /// Dump every inbound line. When a step hangs, the wire trace says whether the + /// broker answered at all, answered and then went quiet, or never saw the request — three + /// very different bugs that look identical from a timeout. + internal bool Verbose; + private BrokerClient(Process process) => this.process = process; internal static BrokerClient Spawn(string brokerPath) @@ -59,6 +64,8 @@ internal sealed class BrokerClient : IAsyncDisposable for (string? line; (line = await process.StandardOutput.ReadLineAsync()) is not null;) { if (line.Length == 0) continue; + if (Verbose) + Console.WriteLine($" [<-] {(line.Length > 300 ? line[..300] + "…" : line)}"); JsonElement root; try { root = JsonDocument.Parse(line).RootElement.Clone(); } catch (JsonException) { Console.WriteLine($" [unparseable] {line}"); continue; } @@ -101,6 +108,7 @@ internal sealed class BrokerClient : IAsyncDisposable var request = args is null ? $$"""{"jsonrpc":"2.0","id":{{id}},"method":"{{method}}"}""" : $$"""{"jsonrpc":"2.0","id":{{id}},"method":"{{method}}","params":{{JsonSerializer.Serialize(args)}}}"""; + if (Verbose) Console.WriteLine($" [->] {request}"); await process.StandardInput.WriteAsync(request + "\n"); await process.StandardInput.FlushAsync(); diff --git a/spikes/WslcSpike/Program.cs b/spikes/WslcSpike/Program.cs index 0277fe8..2b27477 100644 --- a/spikes/WslcSpike/Program.cs +++ b/spikes/WslcSpike/Program.cs @@ -40,6 +40,7 @@ internal static class Program // does not: alpine's PID 1 is /bin/sh, which exits immediately with no tty, so the // container is `Exited` before the first exec and every later step fails `not_running`. var sleepInit = args.Contains("--sleep-init"); + var verbose = args.Contains("--verbose"); if (broker is null || !File.Exists(broker)) { @@ -57,7 +58,7 @@ internal static class Program try { - await RunScenarioAsync(broker, sessionName, image, container, repo, iterations, sleepInit); + await RunScenarioAsync(broker, sessionName, image, container, repo, iterations, sleepInit, verbose); if (!skipRecovery) await RunRecoveryAsync(broker, sessionName); return 0; } @@ -73,9 +74,10 @@ internal static class Program private static async Task RunScenarioAsync( string brokerPath, string sessionName, string image, string containerName, - string repo, int iterations, bool sleepInit) + string repo, int iterations, bool sleepInit, bool verbose) { await using var broker = BrokerClient.Spawn(brokerPath); + broker.Verbose = verbose; // Pull progress is high-rate; collapse it to one line per phase change so the transcript // stays readable but a stalled pull is still visible. @@ -164,13 +166,23 @@ internal static class Program Console.WriteLine($" exit {code}: {output.Trim()}"); Step("exec: uid drop (setpriv wrapper, §3.2)"); - // ProcessSettings has no uid/gid, so the facade wraps argv in setpriv. If the image lacks - // util-linux this is where that shows, and the fallback is `su agent -c`. - var (idCode, idOut) = await ExecAsync( - broker, containerName, ["/bin/sh", "-c", "id -u; id -g"], uid: 501, gid: 501); - Console.WriteLine(idCode == 0 - ? $" uid/gid inside container: {idOut.Replace("\n", "/").Trim('/')}" - : $" setpriv wrapper FAILED (exit {idCode}): {idOut.Trim()} — try `su agent -c`"); + // ProcessSettings has no uid/gid, so the facade wraps argv in setpriv. A BusyBox image has + // a setpriv that cannot change uid, and the facade now REFUSES rather than silently + // running the agent as root — so `unsupported` here is correct behaviour on such an image, + // not a failure of the run. + try + { + var (idCode, idOut) = await ExecAsync( + broker, containerName, ["/bin/sh", "-c", "id -u; id -g"], uid: 501, gid: 501); + Console.WriteLine(idCode == 0 + ? $" uid/gid inside container: {idOut.Replace("\n", "/").Trim('/')}" + : $" ran but reported failure (exit {idCode}): {idOut.Trim()}"); + } + catch (BrokerError e) when (e.Kind == "unsupported") + { + Console.WriteLine($" REFUSED (correctly): {e.Message}"); + Console.WriteLine(" → expected on a BusyBox image; narOS ships util-linux."); + } Step("the mounted worktree"); var (lsCode, lsOut) = await ExecAsync(broker, containerName, @@ -200,10 +212,26 @@ internal static class Program /// private static async Task MeasureNinePAsync(BrokerClient broker, string container, int iterations) { - Step($"9P vs ext4 — `git status` x{iterations} (§15 risk, D8)"); + // `find -type f` rather than `git status`, because it needs only busybox and measures the + // same thing that makes git slow over a mount: a full lstat() traversal of the tree. Using + // git would make the number depend on the image having git, which is what derailed the + // first attempt at this measurement. + var probe = "find . -type f | wc -l"; + var (gitCode, _) = await ExecAsync(broker, container, ["/bin/sh", "-c", "command -v git"]); + if (gitCode == 0) + { + probe = "git status --porcelain >/dev/null"; + Console.WriteLine(" (git present — measuring `git status`)"); + } + else + { + Console.WriteLine(" (no git in this image — measuring `find -type f`, which is the " + + "lstat traversal that dominates `git status`)"); + } - var mounted = await TimeCommandAsync( - broker, container, "cd /work && git status --porcelain >/dev/null", iterations); + Step($"9P vs ext4 — `{probe}` x{iterations} (§15 risk, D8)"); + + var mounted = await TimeCommandAsync(broker, container, $"cd /work && {probe}", iterations); Report("/work (NTFS via 9P)", mounted); Console.WriteLine(" copying the worktree to container-local ext4…"); @@ -217,8 +245,7 @@ internal static class Program } Console.WriteLine($" copied in {copyWatch.Elapsed.TotalSeconds:F1}s"); - var local = await TimeCommandAsync( - broker, container, "cd /tmp/ext4 && git status --porcelain >/dev/null", iterations); + var local = await TimeCommandAsync(broker, container, $"cd /tmp/ext4 && {probe}", iterations); Report("/tmp/ext4 (container-local)", local); if (mounted.Count > 0 && local.Count > 0) @@ -330,12 +357,22 @@ internal static class Program { var output = new MemoryStream(); var exited = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - long procId = -1; + // Deliberately NOT filtered by procId. + // + // The broker now guarantees the `proc.exec` response precedes any notification, but that + // is not sufficient for a client: completing the response's TaskCompletionSource only + // SCHEDULES the awaiting continuation, so the reader thread can dispatch the very next + // line — proc.stdout, or proc.exit — before the continuation has recorded the procId. + // Filtering on a not-yet-assigned procId silently drops the exit and hangs forever, which + // is exactly how this spike hung twice (docs/WINDOWS_PORT.md §13.3). + // + // Safe here because the spike runs one process at a time and awaits each to completion. + // A real client cannot take this shortcut: it must buffer notifications for procIds it + // has not yet learned. Worth checking `WslcBrokerClient.swift` for the same race. void OnNotification(string method, JsonElement args) { - if (!args.TryGetProperty("procId", out var idElement)) return; - if (idElement.GetInt64() != Volatile.Read(ref procId)) return; + if (!args.TryGetProperty("procId", out _)) return; switch (method) { case "proc.stdout": @@ -355,11 +392,7 @@ internal static class Program object spec = uid is null ? new { container, argv, tty = false } : new { container, argv, uid, gid = gid ?? uid, tty = false }; - var result = await broker.CallAsync("proc.exec", spec); - // Set procId only after exec returns — but the broker may already have emitted output - // by then. That race is why WslcProcessHandle exists on the Swift side; here it costs - // at most a few dropped bytes of a diagnostic, so it is accepted rather than solved. - Volatile.Write(ref procId, result.GetProperty("procId").GetInt64()); + await broker.CallAsync("proc.exec", spec); var code = await exited.Task.WaitAsync(TimeSpan.FromSeconds(timeoutSeconds)); lock (output) return (code, System.Text.Encoding.UTF8.GetString(output.ToArray()));