From 691b925952660edd5018647ff0a826d8987920a1 Mon Sep 17 00:00:00 2001 From: Erik Date: Sun, 16 Aug 2026 11:44:46 +0200 Subject: [PATCH] fix #406: launcher session exit observation carries the real code + captures client stderr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GameWindow.Dispose() (via Program.cs's `using var window = ...`) runs unconditionally even when invoked mid-unwind of an exception that escaped Run()'s Silk.NET frame loop. Resource teardown itself can converge cleanly regardless, so CompleteShutdown had no way to tell "normal Run() return" from "a crash is propagating through me right now" and always wrote the hardcoded exited{code:0,reason:"graceful"} — exactly the symptom #406 observed against a real 0xE0434352 crash. Fixed by latching _runFailure in Run()'s existing catch block (before the pre-existing throw) and consulting it from a new ReportExited method, the one call site for the terminal status write: crashed(1)/graceful(0)/ shutdown-incomplete(1) as appropriate. No wire-contract amendment needed — §LA1 pins the exited event NAME, and reason is already free text that StatusEventParser round-trips unchanged. Sibling gap fixed in the same commit: the launcher discarded the child's stdout/stderr entirely, which is why diagnosing this exact crash required a manual console re-run. Added BoundedProcessOutputCapture, a 2 MiB-capped sink mirroring SessionStatusWriter's open-append-flush-close-per-write posture (a long-lived write handle is not actually concurrently readable on Windows even with FileShare.Read — confirmed by isolated repro), wired into both SystemChildProcess (ProcessStartInfo.RedirectStandardError; Linux + Windows graphical children, i.e. this bug's own scenario) and WindowsSystemChildProcess (a real native pipe via CreateChildOutputPipe, mirroring the existing stdin pipe; Windows console-capable/Headless children). Opt-in via LauncherProcessSpec.StderrLogPath (null = unchanged behavior), threaded through SessionConfigComposer -> client.err.log beside status.jsonl -> LauncherExecutableSet -> LauncherOrchestrator. Tests: GameWindowCrashStatusTests (source-shape, matching the existing GameWindow test pattern — the class cannot be constructed without a live GPU/window), BoundedProcessOutputCaptureTests (10 unit tests), and three new LauncherProcessSupervisorTests spawning real child processes through both capture code paths. Launcher.Core.Tests: 337/0 (was 324/0). Launcher.Tests: 67/0 (unchanged). Full solution build green. Co-Authored-By: Claude Fable 5 --- docs/ISSUES.md | 64 ++++- src/AcDream.App/Rendering/GameWindow.cs | 51 +++- .../Launching/BoundedProcessOutputCapture.cs | 238 +++++++++++++++++ .../Launching/ILauncherChildProcess.cs | 32 +++ .../Launching/LauncherProcessSpec.cs | 7 +- .../Launching/SessionConfigComposer.cs | 29 ++- .../Launching/WindowsSystemChildProcess.cs | 160 +++++++++++- .../Orchestration/LauncherExecutableSet.cs | 16 +- .../Orchestration/LauncherOrchestrator.cs | 7 +- .../Rendering/GameWindowCrashStatusTests.cs | 139 ++++++++++ .../Program.cs | 29 +++ .../BoundedProcessOutputCaptureTests.cs | 240 ++++++++++++++++++ .../LauncherProcessSupervisorTests.cs | 176 +++++++++++++ 13 files changed, 1150 insertions(+), 38 deletions(-) create mode 100644 src/AcDream.Launcher.Core/Launching/BoundedProcessOutputCapture.cs create mode 100644 tests/AcDream.App.Tests/Rendering/GameWindowCrashStatusTests.cs create mode 100644 tests/AcDream.Launcher.Core.Tests/Launching/BoundedProcessOutputCaptureTests.cs diff --git a/docs/ISSUES.md b/docs/ISSUES.md index 307ebde0..e189a20a 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -103,26 +103,62 @@ amendment. Immediate workaround (confirmed live): drag-resize the windowed client — resize events rebuild the swapchain (#387) and the retail UI rescales from its 800x600 authored canvas. -## #406 — Launcher records a crashed client as `exited{code:0,reason:"graceful"}` +## #406 — CLOSED: Launcher records a crashed client as `exited{code:0,reason:"graceful"}` -**Status:** OPEN (Campaign CC gate round 1, 2026-08-16) +**Status:** DONE (this commit, 2026-08-16) **Severity:** MEDIUM (diagnosis-misleading, not data-loss) Found while diagnosing #405: the client process died with exit code `0xE0434352` (.NET unhandled exception, stack on stderr), but the launcher's session status stream recorded `{"e":"exited","code":0, -"reason":"graceful"}` — the exact opposite of what happened. Running the -identical binary + session config from a console shows the true nonzero -exit code, so the corruption is in the launcher's session-orchestrator -exit observation (wrong process handle/exit-code read, or a default that -masks the real code), not in the client. §LA1 explicitly promises -`exited{code,reason}` carries the real termination; a launcher that -reports "graceful" for a crash sends any future gate/automation -diagnosis in the wrong direction (it did exactly that this round until -the console repro). Investigate the launcher-side session orchestrator's -exit capture; a test should pin a nonzero-exit child producing -`exited{code:,reason:"crashed"|"failed"}` per the LA contract's -vocabulary. +"reason":"graceful"}` — the exact opposite of what happened. + +Root cause was NOT in the launcher's process supervision (its own +OS-level exit-code read was always correct) — it was in the CLIENT's own +self-report. `GameWindow.Dispose()` (`src/AcDream.App/Rendering/GameWindow.cs`) +runs unconditionally via `Program.cs`'s `using var window = new +GameWindow(...)` even when invoked mid-unwind of an exception that +escaped `Run()`'s Silk.NET frame loop — the resource-shutdown transaction +itself can converge cleanly (nothing it tears down touches the crash), +so `CompleteShutdown` had no way to tell "normal `Run()` return" from "an +exception is propagating through me right now" and always wrote the +hardcoded `exited{code:0,reason:"graceful"}`. Fixed by latching +`_runFailure` in `Run()`'s existing `catch (Exception failure)` block +(right before the `throw;` that already existed for the +`_constructionCleanup.RetainFrom(failure)` ledger) and consulting it from +a new `ReportExited` method that is now the ONE call site for the +terminal status write: `exited{code:1,reason:"crashed"}` when a crash was +observed, `exited{code:0,reason:"graceful"}` on a real graceful +Dispose(), `exited{code:1,reason:"shutdown-incomplete"}` unchanged for a +non-crash teardown failure. `"crashed"` is a new value for the already- +free-text `reason` field (§LA1's `exited{code,reason}` vocabulary pins +the EVENT name, not an enum of `reason` strings — `StatusEventParser` +already round-trips any string there) so no wire-contract amendment was +needed. Pinned as a source-shape test (`GameWindowCrashStatusTests`) since +`GameWindow` cannot be constructed without a live GPU/window. + +Sibling gap fixed in the same commit: the launcher previously discarded +the child's stdout/stderr entirely, which is why diagnosing this exact +crash required a manual console re-run. Added +`BoundedProcessOutputCapture` (`src/AcDream.Launcher.Core/Launching/`) — +a 2 MiB-capped, additive-only sink mirroring `SessionStatusWriter`'s +open-append-flush-close-per-write posture (a long-lived write handle is +NOT actually concurrently readable on Windows even with +`FileShare.Read` — confirmed by isolated repro) — wired into BOTH +`SystemChildProcess` (`ProcessStartInfo.RedirectStandardError` + +`ErrorDataReceived`; used on Linux for every child and on Windows for +graphical/non-console children, i.e. exactly this bug's own App/GUI +scenario) and `WindowsSystemChildProcess` (a real native pipe via a new +`CreateChildOutputPipe`, mirroring the existing stdin pipe in the +opposite direction, drained on a background pump thread; used on Windows +for console-capable children, i.e. Headless). The capture path is opt-in +via a new `LauncherProcessSpec.StderrLogPath` (null = behave exactly as +before) threaded through `SessionConfigComposer` → `client.err.log` +beside `status.jsonl` in the per-session directory → +`LauncherExecutableSet.CreatePlaySpec`/`CreateProbeSpec` → +`LauncherOrchestrator`. Real end-to-end tests +(`LauncherProcessSupervisorTests`) spawn an actual child via both code +paths and assert the captured file. ## #405 — CLOSED: chargen/summary preview leases missing Transfer killed every retail-UI window load diff --git a/src/AcDream.App/Rendering/GameWindow.cs b/src/AcDream.App/Rendering/GameWindow.cs index 4a0bc6aa..50cc1d79 100644 --- a/src/AcDream.App/Rendering/GameWindow.cs +++ b/src/AcDream.App/Rendering/GameWindow.cs @@ -137,6 +137,14 @@ public sealed class GameWindow : _constructionCleanup = new(); private readonly AcDream.App.World.WorldEnvironmentController _worldEnvironment; private readonly GameWindowLifetime _lifetime = new(); + // fix #406: set by Run()'s own catch the instant an exception escapes + // the Silk.NET frame loop, BEFORE it is rethrown and unwinds through + // Program.cs's `using var window = ...` (which calls Dispose() — + // therefore CompleteShutdown() — while that exception is still in + // flight). CompleteShutdown consults this so a crash is never reported + // as the hardcoded "exited{code:0,reason:graceful}" the resource + // teardown transaction's own convergence would otherwise imply. + private Exception? _runFailure; private readonly DisplayFramePacingController _displayFramePacing; private readonly RuntimeSettingsController _runtimeSettings; @@ -820,6 +828,10 @@ public sealed class GameWindow : catch (Exception failure) { _constructionCleanup.RetainFrom(failure); + // fix #406: latch BEFORE rethrowing — Dispose() (and therefore + // CompleteShutdown) can run mid-unwind of this exact exception, + // via Program.cs's `using var window = ...`. + _runFailure = failure; throw; } } @@ -1698,7 +1710,7 @@ public sealed class GameWindow : // OnClosing() native-window-close-request pass) represents the // process actually being done. if (releaseNativeWindow) - _statusWriter.Exited(_options.SessionId ?? "app", 0, "graceful"); + ReportExited(report); return; } @@ -1715,12 +1727,41 @@ public sealed class GameWindow : Console.Error.WriteLine($"[shutdown] {report.Error}"); if (releaseNativeWindow) + ReportExited(report); + } + + /// + /// Writes the ONE terminal "exited" status event for this session + /// (fix #406). A resource-shutdown transaction can converge cleanly + /// ('s own + /// says nothing about this) even though this call + /// is running mid-unwind of an exception that escaped + /// 's frame loop and is about to terminate the process + /// via the CLR's unhandled-exception path — + /// is the one signal that actually distinguishes those two cases. + /// Before this fix every such crash wrote the exact same + /// "exited{code:0,reason:graceful}" as a real graceful shutdown, + /// sending any launcher-side diagnosis in the wrong direction (#406). + /// + private void ReportExited(GameWindowLifetimeReport report) + { + string sessionId = _options.SessionId ?? "app"; + if (_runFailure is not null) { - _statusWriter.Exited( - _options.SessionId ?? "app", - 1, - "shutdown-incomplete"); + // The real OS-level exit code (e.g. 0xE0434352 on Windows for + // an unhandled .NET exception) is produced by the runtime AFTER + // this method returns and the exception keeps propagating — it + // cannot be predicted from here. "crashed" is the truthful, + // platform-independent classification; the launcher's own + // process supervisor observes the real OS exit code separately. + _statusWriter.Exited(sessionId, 1, "crashed"); + return; } + + if (report.Status == GameWindowLifetimeStatus.Complete) + _statusWriter.Exited(sessionId, 0, "graceful"); + else + _statusWriter.Exited(sessionId, 1, "shutdown-incomplete"); } private GameWindowShutdownRoots CaptureShutdownRoots() => new( diff --git a/src/AcDream.Launcher.Core/Launching/BoundedProcessOutputCapture.cs b/src/AcDream.Launcher.Core/Launching/BoundedProcessOutputCapture.cs new file mode 100644 index 00000000..0803a711 --- /dev/null +++ b/src/AcDream.Launcher.Core/Launching/BoundedProcessOutputCapture.cs @@ -0,0 +1,238 @@ +using System.Text; + +namespace AcDream.Launcher.Core.Launching; + +/// +/// Captures a supervised child's stderr into a per-session file, bounded +/// so a log-spamming (or endlessly crash-looping) child can never fill the +/// disk (fix #406 sibling gap). Before this class existed the launcher +/// discarded a child's stdout/stderr entirely — including the unhandled- +/// exception stack trace a crash writes there — so diagnosing exactly the +/// #406 crash required re-running the identical binary + session config +/// from a console by hand. This is purely additive diagnostics: it does +/// not touch the pinned status.jsonl event vocabulary (Campaign LA +/// plan §LA1) at all. +/// +/// +/// Every write opens the file fresh (), +/// writes its chunk, flushes, and closes — mirroring +/// 's "no long- +/// lived file handle" posture exactly, and for the SAME reason: a +/// long-lived write handle only opened with +/// is NOT actually concurrently readable in practice — Windows' sharing +/// check is bidirectional, and a plain File.ReadAllText-style +/// reader (which itself only requests , not +/// ) fails with a sharing violation +/// against ANY still-open handle that holds write access, regardless of +/// what share flags that writer declared. Opening fresh per write avoids +/// the problem entirely: there is never a handle open except for the +/// duration of one small, synchronous write. +/// +/// +/// +/// Every write is defensively guarded the same way +/// guards its own +/// I/O: a recoverable failure latches this sink into a permanent no-op +/// rather than throwing back into the caller's read-and-forward loop. The +/// launcher's job is to supervise the child, not to go down because a +/// local diagnostics file could not be written. +/// +/// +/// +/// Callers are responsible for continuing to drain the child's stderr +/// stream/pipe even after this sink stops accepting bytes (cap reached or +/// latched off) — this class only bounds what lands on disk, never how +/// much the caller may read. A caller that stopped draining on a full +/// sink could leave the child blocked writing to a full OS pipe buffer. +/// +/// +public sealed class BoundedProcessOutputCapture : IDisposable +{ + /// 2 MiB is generous for the lifecycle/shutdown diagnostics + /// and a crash stack trace this exists to capture, while still being a + /// firm, small bound against a pathological child that spams stderr + /// for an entire long-running headless-bot session. + public const long DefaultMaxBytes = 2 * 1024 * 1024; + + private static readonly byte[] Newline = "\n"u8.ToArray(); + + private readonly string _path; + private readonly long _maxBytes; + private readonly object _gate = new(); + private bool _directoryEnsured; + private long _written; + private bool _capped; + private bool _latchedOff; + private bool _disposed; + + public BoundedProcessOutputCapture(string path, long maxBytes = DefaultMaxBytes) + { + ArgumentException.ThrowIfNullOrWhiteSpace(path); + if (maxBytes <= 0) + { + throw new ArgumentOutOfRangeException( + nameof(maxBytes), + "The bounded capture size must be positive."); + } + + _path = Path.GetFullPath(path); + _maxBytes = maxBytes; + } + + /// True once no further byte will ever be written — either + /// the size cap was reached (a truncation marker was appended) or a + /// local I/O failure latched this sink off. Exposed for tests; a + /// caller never needs to check this before calling + /// — it is always safe to call. + public bool IsDone + { + get + { + lock (_gate) + { + return _capped || _latchedOff || _disposed; + } + } + } + + /// Appends one line of already-decoded text (e.g. one + /// Process.ErrorDataReceived line) followed by a newline. Never + /// throws. A line (the sentinel .NET's + /// ErrorDataReceived raises once when the stream closes) is a + /// silent no-op. + public void AppendLine(string? line) + { + if (line is null) + { + return; + } + + lock (_gate) + { + AppendLocked(Encoding.UTF8.GetBytes(line)); + AppendLocked(Newline); + } + } + + /// Appends a raw decoded chunk (no implied line boundary). + /// Never throws. + public void Append(ReadOnlySpan data) + { + if (data.IsEmpty) + { + return; + } + + lock (_gate) + { + AppendLocked(data); + } + } + + private void AppendLocked(ReadOnlySpan data) + { + if (data.IsEmpty || _disposed || _latchedOff || _capped) + { + return; + } + + try + { + long remaining = _maxBytes - _written; + if (remaining <= 0) + { + CapLocked(); + return; + } + + int toWrite = data.Length > remaining + ? checked((int)remaining) + : data.Length; + WriteChunkLocked(data[..toWrite]); + _written += toWrite; + + if (toWrite < data.Length) + { + CapLocked(); + } + } + catch (Exception error) when (IsRecoverableIoFailure(error)) + { + _latchedOff = true; + } + } + + /// Opens the file fresh, writes one chunk, flushes, and + /// closes — see the class doc for why this never keeps a long-lived + /// handle. Exceptions propagate to the caller's own guard. + private void WriteChunkLocked(ReadOnlySpan chunk) + { + EnsureDirectoryLocked(); + using FileStream stream = new( + _path, + FileMode.Append, + FileAccess.Write, + FileShare.Read); + stream.Write(chunk); + stream.Flush(); + } + + private void EnsureDirectoryLocked() + { + if (_directoryEnsured) + { + return; + } + + string? directory = Path.GetDirectoryName(_path); + if (!string.IsNullOrEmpty(directory)) + { + Directory.CreateDirectory(directory); + } + + _directoryEnsured = true; + } + + /// Writes the one-time truncation marker — every later + /// / becomes a cheap + /// no-op via . + private void CapLocked() + { + if (_capped) + { + return; + } + + _capped = true; + try + { + byte[] marker = Encoding.UTF8.GetBytes( + $"\n[acdream-launcher] client.err.log truncated at {_maxBytes} bytes\n"); + WriteChunkLocked(marker); + } + catch (Exception error) when (IsRecoverableIoFailure(error)) + { + // The marker itself is best-effort — the cap already took + // effect via _capped regardless of whether it could be written. + } + } + + private static bool IsRecoverableIoFailure(Exception error) => + error is IOException + or UnauthorizedAccessException + or NotSupportedException + or System.Security.SecurityException + or DirectoryNotFoundException; + + /// No open handle to release — see the class doc. Marks this + /// sink permanently done so any late-arriving chunk from a caller's + /// still-draining pump is a silent no-op instead of reopening the + /// file after the caller considers capture finished. + public void Dispose() + { + lock (_gate) + { + _disposed = true; + } + } +} diff --git a/src/AcDream.Launcher.Core/Launching/ILauncherChildProcess.cs b/src/AcDream.Launcher.Core/Launching/ILauncherChildProcess.cs index b8816ab7..b3f9a3ce 100644 --- a/src/AcDream.Launcher.Core/Launching/ILauncherChildProcess.cs +++ b/src/AcDream.Launcher.Core/Launching/ILauncherChildProcess.cs @@ -88,7 +88,9 @@ internal sealed partial class SystemChildProcess : ILauncherChildProcess private readonly Process _process; private readonly bool _supportsConsoleGracefulStop; + private readonly BoundedProcessOutputCapture? _stderrCapture; private bool _raisingEnabled; + private bool _errorReadingEnabled; internal SystemChildProcess(LauncherProcessSpec spec) { @@ -102,6 +104,17 @@ internal sealed partial class SystemChildProcess : ILauncherChildProcess UseShellExecute = false, }; + // fix #406 sibling gap: capture stderr (crash stack traces land + // there) into a bounded per-session file instead of discarding it. + // Purely additive — RedirectStandardOutput/CreateNoWindow are left + // untouched, and a spec with no StderrLogPath behaves exactly as + // before. + if (!string.IsNullOrWhiteSpace(spec.StderrLogPath)) + { + startInfo.RedirectStandardError = true; + _stderrCapture = new BoundedProcessOutputCapture(spec.StderrLogPath); + } + foreach (string argument in spec.Arguments) { startInfo.ArgumentList.Add(argument); @@ -128,7 +141,17 @@ internal sealed partial class SystemChildProcess : ILauncherChildProcess _process.EnableRaisingEvents = true; _process.Exited += OnExited; _raisingEnabled = true; + if (_stderrCapture is not null) + { + _process.ErrorDataReceived += OnErrorDataReceived; + _errorReadingEnabled = true; + } + _process.Start(); + if (_errorReadingEnabled) + { + _process.BeginErrorReadLine(); + } } public bool TryRequestGracefulStop() @@ -169,9 +192,18 @@ internal sealed partial class SystemChildProcess : ILauncherChildProcess _process.Exited -= OnExited; } + if (_errorReadingEnabled) + { + _process.ErrorDataReceived -= OnErrorDataReceived; + } + _process.Dispose(); + _stderrCapture?.Dispose(); } private void OnExited(object? sender, EventArgs e) => Exited?.Invoke(this, EventArgs.Empty); + + private void OnErrorDataReceived(object? sender, DataReceivedEventArgs e) => + _stderrCapture?.AppendLine(e.Data); } diff --git a/src/AcDream.Launcher.Core/Launching/LauncherProcessSpec.cs b/src/AcDream.Launcher.Core/Launching/LauncherProcessSpec.cs index 599a932d..9cdac21b 100644 --- a/src/AcDream.Launcher.Core/Launching/LauncherProcessSpec.cs +++ b/src/AcDream.Launcher.Core/Launching/LauncherProcessSpec.cs @@ -11,9 +11,14 @@ namespace AcDream.Launcher.Core.Launching; /// so Windows starts them /// as isolated process-group leaders for targeted CTRL_BREAK_EVENT and /// Linux sends SIGINT; graphical specs leave it false and use WM_CLOSE. +/// is an optional, purely-additive +/// diagnostics sink (fix #406 sibling gap): when set, the launcher +/// captures the child's stderr into a bounded file at that path instead +/// of discarding it; when null, behavior is exactly as before. /// public sealed record LauncherProcessSpec( string ExecutablePath, IReadOnlyList Arguments, string? WorkingDirectory = null, - bool SupportsConsoleGracefulStop = true); + bool SupportsConsoleGracefulStop = true, + string? StderrLogPath = null); diff --git a/src/AcDream.Launcher.Core/Launching/SessionConfigComposer.cs b/src/AcDream.Launcher.Core/Launching/SessionConfigComposer.cs index 1fceee5c..7d9eb594 100644 --- a/src/AcDream.Launcher.Core/Launching/SessionConfigComposer.cs +++ b/src/AcDream.Launcher.Core/Launching/SessionConfigComposer.cs @@ -5,12 +5,17 @@ using AcDream.Platform; namespace AcDream.Launcher.Core.Launching; -/// The composed session-config document plus the two per-launch -/// paths derived from the session id, per Campaign LA spec §6. +/// The composed session-config document plus the per-launch +/// paths derived from the session id, per Campaign LA spec §6. +/// is launcher-internal (fix #406 sibling +/// gap) — it never appears in the written session.json, only in +/// the the launcher spawns the +/// child with. public sealed record ComposedSessionConfig( string SessionId, string ConfigFilePath, string StatusFilePath, + string StderrLogPath, SessionConfigDocument Document); /// @@ -110,7 +115,8 @@ public static class SessionConfigComposer ArgumentNullException.ThrowIfNull(paths); ArgumentException.ThrowIfNullOrWhiteSpace(sessionId); - (string configFilePath, string statusFilePath) = BuildSessionPaths(paths, sessionId); + (string configFilePath, string statusFilePath, string stderrLogPath) = + BuildSessionPaths(paths, sessionId); SessionCharacterSelector? selector = character.LaunchMode == LaunchMode.GuiSelect ? null @@ -159,6 +165,7 @@ public static class SessionConfigComposer sessionId, configFilePath, statusFilePath, + stderrLogPath, document); } @@ -184,7 +191,8 @@ public static class SessionConfigComposer ArgumentNullException.ThrowIfNull(paths); ArgumentException.ThrowIfNullOrWhiteSpace(sessionId); - (string configFilePath, string statusFilePath) = BuildSessionPaths(paths, sessionId); + (string configFilePath, string statusFilePath, string stderrLogPath) = + BuildSessionPaths(paths, sessionId); var descriptor = new SessionDescriptor { @@ -222,6 +230,7 @@ public static class SessionConfigComposer sessionId, configFilePath, statusFilePath, + stderrLogPath, document); } @@ -282,9 +291,10 @@ public static class SessionConfigComposer public static string Serialize(SessionConfigDocument document) => JsonSerializer.Serialize(document, SerializerOptions); - private static (string ConfigFilePath, string StatusFilePath) BuildSessionPaths( - ApplicationPathSet paths, - string sessionId) + private static (string ConfigFilePath, string StatusFilePath, string StderrLogPath) + BuildSessionPaths( + ApplicationPathSet paths, + string sessionId) { string sessionDirectory = Path.Combine( paths.CacheDirectory, @@ -294,7 +304,10 @@ public static class SessionConfigComposer return ( Path.Combine(sessionDirectory, "session.json"), - Path.Combine(sessionDirectory, "status.jsonl")); + Path.Combine(sessionDirectory, "status.jsonl"), + // fix #406 sibling gap: lives beside status.jsonl in the same + // per-session directory. + Path.Combine(sessionDirectory, "client.err.log")); } private static SessionCharacterSelector BuildSelector(CharacterProfile character) diff --git a/src/AcDream.Launcher.Core/Launching/WindowsSystemChildProcess.cs b/src/AcDream.Launcher.Core/Launching/WindowsSystemChildProcess.cs index 00138c30..335f58cb 100644 --- a/src/AcDream.Launcher.Core/Launching/WindowsSystemChildProcess.cs +++ b/src/AcDream.Launcher.Core/Launching/WindowsSystemChildProcess.cs @@ -20,6 +20,10 @@ internal sealed class WindowsSystemChildProcess : ILauncherChildProcess private TextWriter? _standardInput; private int _processGroupId; private bool _raisingEnabled; + // fix #406 sibling gap: null unless _spec.StderrLogPath was set. + private BoundedProcessOutputCapture? _stderrCapture; + private FileStream? _stderrReadStream; + private Thread? _stderrPumpThread; internal WindowsSystemChildProcess( LauncherProcessSpec spec, @@ -54,6 +58,30 @@ internal sealed class WindowsSystemChildProcess : ILauncherChildProcess _raisingEnabled = true; _standardInput = started.TakeStandardInput(); _processGroupId = started.ProcessId; + if (!string.IsNullOrWhiteSpace(_spec.StderrLogPath)) + { + // fix #406 sibling gap: drain the real stderr pipe + // WindowsProcessNative.StartCore created for this spec into + // a bounded file, BEFORE resuming the suspended child below + // — the pump is already running by the time the child can + // write anything. + SafeFileHandle stderrRead = started.TakeStandardErrorRead() + ?? throw new InvalidOperationException( + "The launcher child stderr pipe was not created."); + _stderrCapture = new BoundedProcessOutputCapture(_spec.StderrLogPath); + _stderrReadStream = new FileStream( + stderrRead, + FileAccess.Read, + 4096, + isAsync: false); + _stderrPumpThread = new Thread(PumpStderr) + { + IsBackground = true, + Name = "acdream-launcher-stderr-pump", + }; + _stderrPumpThread.Start(); + } + started.Resume(); } catch @@ -61,6 +89,10 @@ internal sealed class WindowsSystemChildProcess : ILauncherChildProcess started.Terminate(); _standardInput?.Dispose(); _standardInput = null; + _stderrReadStream?.Dispose(); + _stderrReadStream = null; + _stderrCapture?.Dispose(); + _stderrCapture = null; if (_process is not null) { if (_raisingEnabled) @@ -80,6 +112,43 @@ internal sealed class WindowsSystemChildProcess : ILauncherChildProcess } } + /// + /// Runs on a dedicated background thread for the lifetime of the + /// capture (fix #406 sibling gap): continuously drains the child's + /// stderr pipe into so the child's writes + /// never block on a full OS pipe buffer, even after the capture sink + /// itself has stopped accepting bytes (size cap reached, or a local + /// I/O failure latched it off — + /// never throws). Returns cleanly once the pipe's write end closes + /// (the child exited) or closes the read end. + /// + private void PumpStderr() + { + FileStream? stream = _stderrReadStream; + BoundedProcessOutputCapture? capture = _stderrCapture; + if (stream is null || capture is null) + { + return; + } + + byte[] buffer = new byte[4096]; + try + { + int read; + while ((read = stream.Read(buffer, 0, buffer.Length)) > 0) + { + capture.Append(buffer.AsSpan(0, read)); + } + } + catch (Exception error) + when (error is IOException or ObjectDisposedException) + { + // The pipe's write end closed (the child exited) or Dispose() + // released the read end concurrently — either way, this pump + // is simply done; never propagate onto this background thread. + } + } + public bool TryRequestGracefulStop() { try @@ -111,6 +180,21 @@ internal sealed class WindowsSystemChildProcess : ILauncherChildProcess { _standardInput?.Dispose(); _standardInput = null; + if (_stderrReadStream is not null) + { + // Closing the read end unblocks PumpStderr's pending Read() + // (ObjectDisposedException, caught there). The bounded Join + // lets that last in-flight chunk land in the capture file + // before it is disposed below, without letting a wedged pump + // thread ever hang this Dispose() call. + _stderrReadStream.Dispose(); + _stderrReadStream = null; + _stderrPumpThread?.Join(TimeSpan.FromSeconds(2)); + _stderrPumpThread = null; + } + + _stderrCapture?.Dispose(); + _stderrCapture = null; if (_process is not null) { if (_raisingEnabled) @@ -225,18 +309,21 @@ internal sealed class WindowsProcessStartResult : IDisposable private readonly SafeKernelHandle _processHandle; private readonly SafeKernelHandle _threadHandle; private SafeFileHandle? _standardInput; + private SafeFileHandle? _stderrRead; private bool _resumed; internal WindowsProcessStartResult( int processId, SafeKernelHandle processHandle, SafeKernelHandle threadHandle, - SafeFileHandle standardInput) + SafeFileHandle standardInput, + SafeFileHandle? stderrRead = null) { ProcessId = processId; _processHandle = processHandle; _threadHandle = threadHandle; _standardInput = standardInput; + _stderrRead = stderrRead; } internal int ProcessId { get; } @@ -263,6 +350,22 @@ internal sealed class WindowsProcessStartResult : IDisposable } } + /// + /// Transfers ownership of the parent-side stderr pipe read handle (fix + /// #406 sibling gap) — non-null only when + /// was set, in which case + /// created a real pipe for the child's stderr instead of the usual + /// duplicate-or-NUL handle. Returns null if capture was not requested, + /// or if this handle was already claimed. The caller owns disposal + /// after this call. + /// + internal SafeFileHandle? TakeStandardErrorRead() + { + SafeFileHandle? handle = _stderrRead; + _stderrRead = null; + return handle; + } + internal void Resume() { if (WindowsProcessNative.ResumeThread(_threadHandle) == uint.MaxValue) @@ -290,6 +393,7 @@ internal sealed class WindowsProcessStartResult : IDisposable } _standardInput?.Dispose(); + _stderrRead?.Dispose(); _threadHandle.Dispose(); _processHandle.Dispose(); } @@ -361,13 +465,20 @@ internal static class WindowsProcessNative private static WindowsProcessStartResult StartCore(LauncherProcessSpec spec) { SafeFileHandle? parentInput = null; + // fix #406 sibling gap: when the spec requests stderr capture, the + // child's stderr handle is a real pipe (this parent-side read end) + // instead of the usual duplicate-or-NUL handle below. + SafeFileHandle? parentStderrRead = null; + SafeHandle? childError = null; try { using SafeFileHandle childInput = CreateChildInputPipe( out SafeFileHandle createdParentInput); parentInput = createdParentInput; using SafeKernelHandle childOutput = DuplicateOrOpenNull(StdOutputHandle); - using SafeKernelHandle childError = DuplicateOrOpenNull(StdErrorHandle); + childError = string.IsNullOrWhiteSpace(spec.StderrLogPath) + ? DuplicateOrOpenNull(StdErrorHandle) + : CreateChildOutputPipe(out parentStderrRead); using var attributes = new ProcessThreadAttributeList( childInput.DangerousGetHandle(), childOutput.DangerousGetHandle(), @@ -420,8 +531,10 @@ internal static class WindowsProcessNative checked((int)information.ProcessId), processHandle, threadHandle, - parentInput); + parentInput, + parentStderrRead); parentInput = null; + parentStderrRead = null; return result; } catch @@ -435,6 +548,8 @@ internal static class WindowsProcessNative finally { parentInput?.Dispose(); + parentStderrRead?.Dispose(); + childError?.Dispose(); } } @@ -525,6 +640,45 @@ internal static class WindowsProcessNative return child; } + /// + /// Mirror of with the roles + /// reversed (fix #406 sibling gap): the CHILD gets the pipe's WRITE + /// end (its stderr handle, inheritable across + /// ), the PARENT keeps the READ end + /// (inherit flag cleared, exactly like 's + /// counterpart on the stdin pipe) so the launcher can drain the + /// child's stderr into a bounded file. + /// + private static SafeFileHandle CreateChildOutputPipe(out SafeFileHandle parentRead) + { + var security = new SecurityAttributes + { + Length = Marshal.SizeOf(), + InheritHandle = true, + }; + if (!CreatePipe(out IntPtr read, out IntPtr write, ref security, 0)) + { + throw new Win32Exception(Marshal.GetLastWin32Error(), + "The launcher child stderr pipe could not be created."); + } + + var child = new SafeFileHandle(write, ownsHandle: true); + parentRead = new SafeFileHandle(read, ownsHandle: true); + if (!SetHandleInformation( + parentRead, + HandleFlagInherit, + 0)) + { + int error = Marshal.GetLastWin32Error(); + child.Dispose(); + parentRead.Dispose(); + throw new Win32Exception(error, + "The launcher child stderr pipe could not be isolated."); + } + + return child; + } + private static SafeKernelHandle DuplicateOrOpenNull(int standardHandle) { IntPtr source = GetStdHandle(standardHandle); diff --git a/src/AcDream.Launcher.Core/Orchestration/LauncherExecutableSet.cs b/src/AcDream.Launcher.Core/Orchestration/LauncherExecutableSet.cs index 5a98b795..4add2816 100644 --- a/src/AcDream.Launcher.Core/Orchestration/LauncherExecutableSet.cs +++ b/src/AcDream.Launcher.Core/Orchestration/LauncherExecutableSet.cs @@ -91,7 +91,8 @@ public sealed class LauncherExecutableSet public LauncherProcessSpec CreatePlaySpec( LaunchMode mode, - string configFilePath) + string configFilePath, + string? stderrLogPath = null) { ArgumentException.ThrowIfNullOrWhiteSpace(configFilePath); ExecutablePaths paths = RequireAvailable(mode); @@ -100,22 +101,27 @@ public sealed class LauncherExecutableSet ? new LauncherProcessSpec( paths.HeadlessHostPath, ["--config", configFilePath], - paths.WorkingDirectory) + paths.WorkingDirectory, + StderrLogPath: stderrLogPath) : new LauncherProcessSpec( paths.GraphicalHostPath, ["--session-config", configFilePath], paths.WorkingDirectory, - SupportsConsoleGracefulStop: false); + SupportsConsoleGracefulStop: false, + StderrLogPath: stderrLogPath); } - public LauncherProcessSpec CreateProbeSpec(string configFilePath) + public LauncherProcessSpec CreateProbeSpec( + string configFilePath, + string? stderrLogPath = null) { ArgumentException.ThrowIfNullOrWhiteSpace(configFilePath); ExecutablePaths paths = RequireAvailable(LaunchMode.Headless); return new LauncherProcessSpec( paths.HeadlessHostPath, ["--config", configFilePath], - paths.WorkingDirectory); + paths.WorkingDirectory, + StderrLogPath: stderrLogPath); } public static LauncherExecutableSet FromDirectory(string directory) diff --git a/src/AcDream.Launcher.Core/Orchestration/LauncherOrchestrator.cs b/src/AcDream.Launcher.Core/Orchestration/LauncherOrchestrator.cs index a734fcaf..9bb48560 100644 --- a/src/AcDream.Launcher.Core/Orchestration/LauncherOrchestrator.cs +++ b/src/AcDream.Launcher.Core/Orchestration/LauncherOrchestrator.cs @@ -683,10 +683,13 @@ public sealed class LauncherOrchestrator : ILauncherOrchestrator request.Cancellation.Token.ThrowIfCancellationRequested(); LauncherProcessSpec processSpec = request.IsProbe - ? _executables.CreateProbeSpec(composed.ConfigFilePath) + ? _executables.CreateProbeSpec( + composed.ConfigFilePath, + composed.StderrLogPath) : _executables.CreatePlaySpec( request.Activity.LaunchMode!.Value, - composed.ConfigFilePath); + composed.ConfigFilePath, + composed.StderrLogPath); supervisor.Start(processSpec, password); hostStarted = true; diff --git a/tests/AcDream.App.Tests/Rendering/GameWindowCrashStatusTests.cs b/tests/AcDream.App.Tests/Rendering/GameWindowCrashStatusTests.cs new file mode 100644 index 00000000..779aa6d8 --- /dev/null +++ b/tests/AcDream.App.Tests/Rendering/GameWindowCrashStatusTests.cs @@ -0,0 +1,139 @@ +namespace AcDream.App.Tests.Rendering; + +/// +/// Fix #406: before this change, GameWindow.CompleteShutdown wrote +/// a hardcoded exited{code:0,reason:"graceful"} status event +/// whenever the resource-shutdown transaction converged — even when +/// Dispose() (and therefore CompleteShutdown) ran mid-unwind +/// of an exception that escaped Run()'s Silk.NET frame loop and was +/// about to crash the process via the CLR's unhandled-exception path. +/// GameWindow cannot be constructed without a live GPU/window (see +/// the established pattern in GameWindowSlice8BoundaryTests), so +/// this pins the fix as a source-shape test exactly like that file does +/// for the surrounding shutdown machinery. +/// +public sealed class GameWindowCrashStatusTests +{ + [Fact] + public void Run_LatchesRunFailureBeforeRethrowingFromTheFrameLoopCatch() + { + string body = MethodBody( + "public void Run()", + "void IGameWindowPlatformPublication.PublishGraphics("); + string tryBlock = Slice(body, "try\n {\n _window.Run();", "}\n }"); + + AssertAppearsInOrder( + tryBlock, + "_window.Run();", + "catch (Exception failure)", + "_constructionCleanup.RetainFrom(failure);", + // The latch MUST happen before the rethrow: Dispose() (and + // therefore CompleteShutdown/ReportExited) can run mid-unwind + // of this exact exception, via Program.cs's + // `using var window = ...`. + "_runFailure = failure;", + "throw;"); + } + + [Fact] + public void ReportExited_ChecksRunFailureBeforeEitherGracefulOrShutdownIncompletePaths() + { + string source = GameWindowSource(); + string reportExited = Slice( + source, + "private void ReportExited(GameWindowLifetimeReport report)", + "\n }\n"); + + Assert.Contains( + "string sessionId = _options.SessionId ?? \"app\";", + reportExited, + StringComparison.Ordinal); + AssertAppearsInOrder( + reportExited, + "if (_runFailure is not null)", + "_statusWriter.Exited(sessionId, 1, \"crashed\");", + "return;", + "if (report.Status == GameWindowLifetimeStatus.Complete)", + "_statusWriter.Exited(sessionId, 0, \"graceful\");", + "_statusWriter.Exited(sessionId, 1, \"shutdown-incomplete\");"); + + // Every terminal-status write in CompleteShutdown funnels through + // this ONE method — a second, uncoordinated call site would be + // exactly how the pre-fix bug reappears. + Assert.Equal( + 2, + CountOccurrences(source, "ReportExited(report)")); + Assert.DoesNotContain( + "_statusWriter.Exited(_options.SessionId ?? \"app\", 0, \"graceful\")", + source, + StringComparison.Ordinal); + } + + [Fact] + public void RunFailureFieldExistsAndDefaultsToNull() + { + string source = GameWindowSource(); + + Assert.Contains( + "private Exception? _runFailure;", + source, + StringComparison.Ordinal); + } + + private static string MethodBody(string start, string end) => + Slice(GameWindowSource(), start, end); + + private static string Slice(string source, string start, string end) + { + int first = source.IndexOf(start, StringComparison.Ordinal); + int last = source.IndexOf(end, first + 1, StringComparison.Ordinal); + Assert.True(first >= 0, $"Missing source boundary: {start}"); + Assert.True(last > first, $"Missing source boundary: {end}"); + return source[first..last]; + } + + private static int CountOccurrences(string source, string value) + { + int count = 0; + int cursor = 0; + while ((cursor = source.IndexOf(value, cursor, StringComparison.Ordinal)) >= 0) + { + count++; + cursor += value.Length; + } + + return count; + } + + private static void AssertAppearsInOrder(string source, params string[] fragments) + { + int cursor = -1; + foreach (string fragment in fragments) + { + int next = source.IndexOf(fragment, cursor + 1, StringComparison.Ordinal); + Assert.True(next >= 0, $"Missing expected source fragment: {fragment}"); + Assert.True(next > cursor, $"Out-of-order source fragment: {fragment}"); + cursor = next; + } + } + + private static string GameWindowSource() => File.ReadAllText(Path.Combine( + FindRepoRoot(), + "src", + "AcDream.App", + "Rendering", + "GameWindow.cs")).Replace("\r\n", "\n", StringComparison.Ordinal); + + private static string FindRepoRoot() + { + DirectoryInfo? directory = new(AppContext.BaseDirectory); + while (directory is not null) + { + if (File.Exists(Path.Combine(directory.FullName, "AcDream.slnx"))) + return directory.FullName; + directory = directory.Parent; + } + + throw new DirectoryNotFoundException("Could not find AcDream.slnx."); + } +} diff --git a/tests/AcDream.Launcher.Core.Tests.Fixtures.ConsoleSignalChild/Program.cs b/tests/AcDream.Launcher.Core.Tests.Fixtures.ConsoleSignalChild/Program.cs index dc047cdf..092616a0 100644 --- a/tests/AcDream.Launcher.Core.Tests.Fixtures.ConsoleSignalChild/Program.cs +++ b/tests/AcDream.Launcher.Core.Tests.Fixtures.ConsoleSignalChild/Program.cs @@ -1,5 +1,34 @@ using System.Text.Json; +// fix #406 sibling gap: a "write-stderr" mode alongside the existing +// "wait-for-break" mode so the launcher's stderr-capture tests can drive a +// real child process (both WindowsSystemChildProcess's native pipe path +// and SystemChildProcess's ProcessStartInfo.RedirectStandardError path) +// without a second fixture project. Usage: +// write-stderr +// Writes followed by its 0-based index, one per line, to +// stderr times (flushing every line so a launcher-side pump +// observes them incrementally rather than all at once on process exit), +// then returns . +if (args.Length >= 1 && args[0] == "write-stderr") +{ + if (args.Length < 4 + || !int.TryParse(args[1], out int exitCode) + || !int.TryParse(args[2], out int lineCount)) + { + return 64; + } + + string lineText = args[3]; + for (int index = 0; index < lineCount; index++) + { + Console.Error.WriteLine($"{lineText}{index}"); + Console.Error.Flush(); + } + + return exitCode; +} + if (args.Length < 4 || args[0] != "wait-for-break" || string.IsNullOrWhiteSpace(args[1]) diff --git a/tests/AcDream.Launcher.Core.Tests/Launching/BoundedProcessOutputCaptureTests.cs b/tests/AcDream.Launcher.Core.Tests/Launching/BoundedProcessOutputCaptureTests.cs new file mode 100644 index 00000000..5ad94363 --- /dev/null +++ b/tests/AcDream.Launcher.Core.Tests/Launching/BoundedProcessOutputCaptureTests.cs @@ -0,0 +1,240 @@ +using System.Text; +using AcDream.Launcher.Core.Launching; + +namespace AcDream.Launcher.Core.Tests.Launching; + +/// Fix #406 sibling gap: the launcher previously discarded a +/// supervised child's stderr entirely, so diagnosing a crash (including +/// exactly the #406 crash) required re-running the identical binary by +/// hand. These tests cover in +/// isolation — the real-child-process end-to-end capture tests live in +/// LauncherProcessSupervisorTests alongside the existing real-process +/// coverage. +public sealed class BoundedProcessOutputCaptureTests +{ + [Fact] + public void AppendLineWritesEachLineWithATrailingNewline() + { + string path = TempPath(); + try + { + using var capture = new BoundedProcessOutputCapture(path); + + capture.AppendLine("first"); + capture.AppendLine("second"); + capture.Dispose(); + + Assert.Equal("first\nsecond\n", File.ReadAllText(path)); + } + finally + { + TryDelete(path); + } + } + + [Fact] + public void ANullLineFromTheEndOfStreamSentinelIsANoOp() + { + string path = TempPath(); + try + { + using var capture = new BoundedProcessOutputCapture(path); + + capture.AppendLine("kept"); + capture.AppendLine(null); + capture.Dispose(); + + Assert.Equal("kept\n", File.ReadAllText(path)); + } + finally + { + TryDelete(path); + } + } + + [Fact] + public void WritesBeyondTheCapAreDroppedAndAOneTimeTruncationMarkerIsAppended() + { + string path = TempPath(); + try + { + using var capture = new BoundedProcessOutputCapture(path, maxBytes: 16); + + capture.AppendLine("0123456789"); // 11 bytes incl. newline + capture.AppendLine("this line is dropped entirely"); + capture.AppendLine("so is this one"); + + Assert.True(capture.IsDone); + string written = File.ReadAllText(path); + Assert.StartsWith("0123456789\n", written, StringComparison.Ordinal); + Assert.Contains("truncated at 16 bytes", written, StringComparison.Ordinal); + // The cap is a hard ceiling: nothing past it EVER lands on disk, + // even the marker's own text does not push the file arbitrarily + // far past the configured bound. + Assert.True( + written.Length < 200, + $"expected a small bounded file, got {written.Length} bytes"); + } + finally + { + TryDelete(path); + } + } + + [Fact] + public void ALogSpammingChildCannotGrowTheFileUnboundedly() + { + string path = TempPath(); + try + { + using var capture = new BoundedProcessOutputCapture( + path, + maxBytes: BoundedProcessOutputCapture.DefaultMaxBytes); + + // Far more than the 2 MiB default cap. + string spamLine = new('x', 4096); + for (int i = 0; i < 4096; i++) + { + capture.AppendLine(spamLine); + if (capture.IsDone) + { + break; + } + } + + Assert.True(capture.IsDone); + long fileLength = new FileInfo(path).Length; + Assert.True( + fileLength < BoundedProcessOutputCapture.DefaultMaxBytes + 256, + $"expected the file to stay near the {BoundedProcessOutputCapture.DefaultMaxBytes}-byte " + + $"cap, got {fileLength} bytes"); + } + finally + { + TryDelete(path); + } + } + + [Fact] + public void AppendCreatesTheSessionDirectoryOnFirstWrite() + { + string directory = Path.Combine( + Path.GetTempPath(), + "acdream-406-capture-" + Guid.NewGuid().ToString("N")); + string path = Path.Combine(directory, "client.err.log"); + Assert.False(Directory.Exists(directory)); + + try + { + using var capture = new BoundedProcessOutputCapture(path); + capture.AppendLine("hello"); + capture.Dispose(); + + Assert.True(File.Exists(path)); + } + finally + { + try + { + Directory.Delete(directory, recursive: true); + } + catch (IOException) + { + } + } + } + + [Fact] + public void RawByteAppendsAreConcatenatedWithoutAnImpliedLineBoundary() + { + string path = TempPath(); + try + { + using var capture = new BoundedProcessOutputCapture(path); + + capture.Append(Encoding.UTF8.GetBytes("abc")); + capture.Append(Encoding.UTF8.GetBytes("def")); + capture.Dispose(); + + Assert.Equal("abcdef", File.ReadAllText(path)); + } + finally + { + TryDelete(path); + } + } + + [Fact] + public void EmptyAppendsAreNoOps() + { + string path = TempPath(); + try + { + using var capture = new BoundedProcessOutputCapture(path); + + capture.Append(ReadOnlySpan.Empty); + capture.AppendLine(string.Empty); + capture.Dispose(); + + // An empty string line still gets its trailing newline — + // only a genuinely zero-length byte span (or a null line) is + // a true no-op. + Assert.Equal("\n", File.ReadAllText(path)); + } + finally + { + TryDelete(path); + } + } + + [Fact] + public void AppendAfterDisposeIsASilentNoOp() + { + string path = TempPath(); + try + { + var capture = new BoundedProcessOutputCapture(path); + capture.AppendLine("before"); + capture.Dispose(); + + capture.AppendLine("after — must not throw or reopen the file"); + + Assert.Equal("before\n", File.ReadAllText(path)); + } + finally + { + TryDelete(path); + } + } + + [Fact] + public void ConstructorRejectsANonPositiveMaxBytes() + { + Assert.Throws( + () => new BoundedProcessOutputCapture(TempPath(), maxBytes: 0)); + Assert.Throws( + () => new BoundedProcessOutputCapture(TempPath(), maxBytes: -1)); + } + + [Fact] + public void ConstructorRejectsANullOrBlankPath() + { + Assert.Throws(() => new BoundedProcessOutputCapture("")); + Assert.Throws(() => new BoundedProcessOutputCapture(" ")); + } + + private static string TempPath() => Path.Combine( + Path.GetTempPath(), + "acdream-406-capture-" + Guid.NewGuid().ToString("N") + ".log"); + + private static void TryDelete(string path) + { + try + { + File.Delete(path); + } + catch (IOException) + { + } + } +} diff --git a/tests/AcDream.Launcher.Core.Tests/Launching/LauncherProcessSupervisorTests.cs b/tests/AcDream.Launcher.Core.Tests/Launching/LauncherProcessSupervisorTests.cs index ca55ff60..837ed01b 100644 --- a/tests/AcDream.Launcher.Core.Tests/Launching/LauncherProcessSupervisorTests.cs +++ b/tests/AcDream.Launcher.Core.Tests/Launching/LauncherProcessSupervisorTests.cs @@ -543,6 +543,182 @@ public sealed class LauncherProcessSupervisorTests Assert.Equal(0, supervisor.ExitCode); } + [Fact] + public async Task RealChildStderrIsCapturedForTheProcessStartInfoPath() + { + // Fix #406 sibling gap: SystemChildProcess is used on Linux for + // EVERY child, and on Windows for graphical/non-console children — + // exactly #406's own App/GUI crash scenario + // (SupportsConsoleGracefulStop: false is the graphical shape; + // see LauncherExecutableSet.CreatePlaySpec). This proves the real + // child's stderr actually lands in the configured bounded file + // instead of being discarded, and that the real nonzero exit code + // is still observed independently of the capture. + string root = Path.Combine( + Path.GetTempPath(), + "acdream-406-stderr-psi", + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(root); + string stderrPath = Path.Combine(root, "client.err.log"); + + try + { + using var supervisor = new LauncherProcessSupervisor(); + var exited = new ManualResetEventSlim(false); + supervisor.StateChanged += (_, s) => + { + if (s == LauncherSessionState.Exited) + exited.Set(); + }; + + supervisor.Start( + new LauncherProcessSpec( + FindDotnetExecutable(), + [GetConsoleFixturePath(), "write-stderr", "7", "3", "line-"], + SupportsConsoleGracefulStop: false, + StderrLogPath: stderrPath), + password: null); + + Assert.True( + exited.Wait(TimeSpan.FromSeconds(30)), + "the write-stderr fixture did not exit within 30s"); + Assert.Equal(7, supervisor.ExitCode); + + string captured = await ReadFileEventuallyContainingAsync( + stderrPath, "line-2", TimeSpan.FromSeconds(5)); + Assert.Contains("line-0", captured, StringComparison.Ordinal); + Assert.Contains("line-1", captured, StringComparison.Ordinal); + Assert.Contains("line-2", captured, StringComparison.Ordinal); + } + finally + { + try + { + Directory.Delete(root, recursive: true); + } + catch (IOException) + { + } + } + } + + [Fact] + public async Task RealChildStderrIsCapturedForTheWindowsNativeConsolePath() + { + // Fix #406 sibling gap: on Windows, console-capable children + // (Headless) spawn through WindowsSystemChildProcess's native + // CreateProcessW path, a completely separate code path from + // SystemChildProcess above — WindowsProcessNative.StartCore + // creates a real pipe for stderr instead of the usual + // duplicate-or-NUL handle, and WindowsSystemChildProcess pumps it + // on a background thread. This proves that path end to end too. + if (!OperatingSystem.IsWindows()) + { + return; + } + + string root = Path.Combine( + Path.GetTempPath(), + "acdream-406-stderr-native", + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(root); + string stderrPath = Path.Combine(root, "client.err.log"); + + try + { + using var supervisor = new LauncherProcessSupervisor(); + var exited = new ManualResetEventSlim(false); + supervisor.StateChanged += (_, s) => + { + if (s == LauncherSessionState.Exited) + exited.Set(); + }; + + supervisor.Start( + new LauncherProcessSpec( + FindDotnetExecutable(), + [GetConsoleFixturePath(), "write-stderr", "9", "3", "native-line-"], + StderrLogPath: stderrPath), + password: null); + + Assert.True( + exited.Wait(TimeSpan.FromSeconds(30)), + "the write-stderr fixture did not exit within 30s"); + Assert.Equal(9, supervisor.ExitCode); + + string captured = await ReadFileEventuallyContainingAsync( + stderrPath, "native-line-2", TimeSpan.FromSeconds(5)); + Assert.Contains("native-line-0", captured, StringComparison.Ordinal); + Assert.Contains("native-line-1", captured, StringComparison.Ordinal); + Assert.Contains("native-line-2", captured, StringComparison.Ordinal); + } + finally + { + try + { + Directory.Delete(root, recursive: true); + } + catch (IOException) + { + } + } + } + + [Fact] + public void ANullStderrLogPathBehavesExactlyAsBeforeForBothChildProcessKinds() + { + // The additive-diagnostics contract: a spec with no StderrLogPath + // must not change behavior at all (fix #406 sibling gap review + // guard against a regression that always redirects stderr). + string dotnet = FindDotnetExecutable(); + using var supervisor = new LauncherProcessSupervisor(); + var exited = new ManualResetEventSlim(false); + supervisor.StateChanged += (_, s) => + { + if (s == LauncherSessionState.Exited) + exited.Set(); + }; + + supervisor.Start(new LauncherProcessSpec(dotnet, ["--version"]), null); + + Assert.True(exited.Wait(TimeSpan.FromSeconds(30))); + Assert.Equal(0, supervisor.ExitCode); + } + + private static async Task ReadFileEventuallyContainingAsync( + string path, + string expectedFragment, + TimeSpan timeout) + { + DateTime deadline = DateTime.UtcNow + timeout; + string last = string.Empty; + while (DateTime.UtcNow < deadline) + { + if (File.Exists(path)) + { + try + { + last = await File.ReadAllTextAsync(path); + if (last.Contains(expectedFragment, StringComparison.Ordinal)) + { + return last; + } + } + catch (IOException) + { + // The pump/writer may hold the file open for a + // moment — retry within the deadline. + } + } + + await Task.Delay(20); + } + + throw new TimeoutException( + $"'{path}' never contained '{expectedFragment}' within {timeout}. " + + $"Last observed content: {last}"); + } + private static LauncherProcessSpec Spec() => new("fake-host", ["--session-config", "session.json"]);