diff --git a/docs/ISSUES.md b/docs/ISSUES.md index f39bc608..881ed834 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -24,6 +24,57 @@ What does NOT go here: - Every session: scan OPEN issues at start; promote/close anything we touched during the session before ending. - Promoting to a Phase: mark as `DONE (promoted to Phase X)` + commit SHA where the Phase entry landed. +## #397 — Windows: LauncherProcessSupervisor.Stop has no reliable graceful-stop signal for a no-window console host + +**Status:** OPEN +**Severity:** MODERATE (a hard-killed `AcDream.Headless` leaves the ACE +account session stuck for several minutes — a documented project landmine; +see CLAUDE.md "Logout-before-reconnect") +**Filed:** 2026-08-14 (Campaign LA plan §LA3 review-fix round, finding F3) +**Component:** Launcher.Core / process supervision + +**Description.** `LauncherProcessSupervisor.Stop` now attempts a graceful +stop signal (`ILauncherChildProcess.TryRequestGracefulStop`) BEFORE +`CloseMainWindow`. On Linux this sends `SIGINT` via a `libc` P/Invoke +(`kill(pid, 2)`), which the K4-proven headless host already turns into an +ACE-confirmed graceful logout. On Windows there is no equivalent today for a +console process with no message-pump window: `CloseMainWindow` is a no-op +for a console host (there is no `HWND` to target), and +`GenerateConsoleCtrlEvent` cannot usefully target an arbitrary child process +today — Windows delivers console control events to every process attached +to the SAME console as the calling process, so an unscoped call would also +signal the launcher itself (and anything else sharing that console), not +just the intended child. `TryRequestGracefulStop` therefore returns `false` +on Windows unconditionally, and `Stop` degrades straight to `CloseMainWindow` +(still a no-op for a console child) and then the timeout-driven `Kill()` — +exactly the hard-kill behavior this finding was written to describe, just +with a documented (rather than silent) gap. + +**Known fix direction (not yet implemented).** Spawn the Windows child with +the `CREATE_NEW_PROCESS_GROUP` creation flag (available via a native +`CreateProcess` call or by setting it on the `ProcessStartInfo`/`Process` +plumbing in `SystemChildProcess`) so the child gets its own console process +group, detached from the launcher's own group. Then +`TryRequestGracefulStop` on Windows calls +`GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, childProcessGroupId)` — +`CTRL_BREAK` (unlike `CTRL_C`) can target a specific process group ID and, +unlike `CTRL_CLOSE`/`CTRL_LOGOFF`/`CTRL_SHUTDOWN`, is deliverable to a +process that has installed no console-control handler at all (the default +CRT handler treats it as a terminating signal, so `AcDream.Headless` doesn't +strictly need new code to receive SOME form of shutdown from it) — though +wiring a real `SetConsoleCtrlHandler` handler that routes `CTRL_BREAK` into +the same graceful-shutdown path K4 already built for Linux SIGINT is the +better long-term target, so a Windows headless launch gets the identical +ACE-confirmed graceful logout instead of just "exits somehow." + +**Acceptance for closing this issue:** `SystemChildProcess` spawns Windows +children with `CREATE_NEW_PROCESS_GROUP`; `TryRequestGracefulStop` sends +`CTRL_BREAK_EVENT` to that child's process group on Windows; a live +connected gate proves `AcDream.Headless` exits gracefully (ACE clears the +session immediately, not after the ~3-minute stale-session window) when +stopped via `LauncherProcessSupervisor.Stop` on Windows, matching the +Linux SIGINT behavior. + ## #396 — Configure Keyboard: no capture-instruction dialog on a mapping-button click **Status:** ROOT-CAUSED + FIXED — pending the user's visual re-gate of the diff --git a/src/AcDream.Launcher.Core/AcDream.Launcher.Core.csproj b/src/AcDream.Launcher.Core/AcDream.Launcher.Core.csproj index 89be4968..f31af6e8 100644 --- a/src/AcDream.Launcher.Core/AcDream.Launcher.Core.csproj +++ b/src/AcDream.Launcher.Core/AcDream.Launcher.Core.csproj @@ -5,6 +5,12 @@ enable latest true + + true diff --git a/src/AcDream.Launcher.Core/Launching/ILauncherChildProcess.cs b/src/AcDream.Launcher.Core/Launching/ILauncherChildProcess.cs index 7ee3ed95..872c0130 100644 --- a/src/AcDream.Launcher.Core/Launching/ILauncherChildProcess.cs +++ b/src/AcDream.Launcher.Core/Launching/ILauncherChildProcess.cs @@ -1,4 +1,5 @@ using System.Diagnostics; +using System.Runtime.InteropServices; namespace AcDream.Launcher.Core.Launching; @@ -28,6 +29,26 @@ public interface ILauncherChildProcess : IDisposable void Start(); + /// + /// Attempts a graceful stop signal appropriate to the platform, + /// tried BEFORE (Campaign LA plan §LA3 + /// review finding F3): a no-window console host (e.g. + /// AcDream.Headless) never has a main window for + /// to close, so without this step + /// always degraded + /// straight to a timeout + hard — and a hard kill + /// leaves the ACE account session stuck for several minutes (a + /// documented project landmine; see CLAUDE.md + /// "Logout-before-reconnect"). On Linux this sends SIGINT (K4 proved + /// the headless host's SIGINT handler produces an ACE-confirmed + /// graceful logout). On Windows there is no reliable cross-console + /// mechanism for an arbitrary no-window child process today — see + /// docs/ISSUES.md for the tracked gap and fix direction; this + /// returns false there. Returns true only when the signal was + /// actually delivered; never throws. + /// + bool TryRequestGracefulStop(); + /// Mirrors — requests /// a graceful close via WM_CLOSE. Returns false for a console/no- /// window process (never throws), matching the real API. @@ -54,8 +75,16 @@ public sealed class SystemChildProcessFactory : ILauncherChildProcessFactory new SystemChildProcess(spec); } -internal sealed class SystemChildProcess : ILauncherChildProcess +internal sealed partial class SystemChildProcess : ILauncherChildProcess { + // SIGINT's numeric value (POSIX-stable across Linux distributions). + // K4/Slice K already proved the headless host's SIGINT handler + // produces an ACE-confirmed graceful logout. + private const int Sigint = 2; + + [LibraryImport("libc", SetLastError = true)] + private static partial int kill(int pid, int sig); + private readonly Process _process; private bool _raisingEnabled; @@ -99,6 +128,31 @@ internal sealed class SystemChildProcess : ILauncherChildProcess _process.Start(); } + public bool TryRequestGracefulStop() + { + if (!OperatingSystem.IsLinux()) + { + // No reliable cross-console mechanism exists for an + // arbitrary no-window Windows child process — tracked gap, + // see docs/ISSUES.md. + return false; + } + + try + { + return kill(_process.Id, Sigint) == 0; + } + catch + { + // Matches CloseMainWindow's "never throws" contract — the + // process may not have started yet, may have already exited + // (ESRCH), or the platform may lack libc under an unusual + // Linux runtime; any of these degrade to "signal not sent" + // rather than an exception out of Stop(). + return false; + } + } + public bool CloseMainWindow() => _process.CloseMainWindow(); public void Kill() => _process.Kill(entireProcessTree: true); diff --git a/src/AcDream.Launcher.Core/Launching/LauncherProcessSupervisor.cs b/src/AcDream.Launcher.Core/Launching/LauncherProcessSupervisor.cs index c6749fdf..f9b10694 100644 --- a/src/AcDream.Launcher.Core/Launching/LauncherProcessSupervisor.cs +++ b/src/AcDream.Launcher.Core/Launching/LauncherProcessSupervisor.cs @@ -56,9 +56,11 @@ public sealed class LauncherProcessSupervisor : IDisposable SetState(LauncherSessionState.Starting); + bool started = false; try { process.Start(); + started = true; if (password is not null) { @@ -77,6 +79,28 @@ public sealed class LauncherProcessSupervisor : IDisposable _process = null; } + // A failure after the child actually started (e.g. the stdin + // pipe breaks while feeding the password) must not leave a + // live, unsupervised, undisposable child running (Campaign LA + // plan §LA3 review finding F8) — kill the whole process tree + // and release the handle before propagating the original + // failure. + if (started) + { + try + { + process.Kill(); + } + catch + { + // Best-effort — the ORIGINAL failure, rethrown below, + // is what the caller needs to see; a failed cleanup + // kill must not replace it. + } + } + + process.Dispose(); + throw; } @@ -84,10 +108,22 @@ public sealed class LauncherProcessSupervisor : IDisposable } /// - /// Requests a graceful stop (CloseMainWindow), falling back to Kill - /// if the process has not exited within . - /// A no-op if was never called or the process has - /// already exited. + /// Requests a graceful stop — first + /// (SIGINT + /// on Linux; a no-op on Windows today, see + /// 's docs), + /// then — falling + /// back to if the process has + /// not exited within . A no-op if + /// was never called or the process has already + /// exited. + /// + /// BLOCKS THE CALLING THREAD for up to + /// (via the real child's WaitForExit) — callers on a UI thread + /// must dispatch this off-thread rather than calling it directly (a + /// binding requirement for the LA4 Avalonia UI, which will call this + /// method from a "stop session" action). + /// /// public void Stop(TimeSpan timeout) { @@ -102,6 +138,7 @@ public sealed class LauncherProcessSupervisor : IDisposable return; } + process.TryRequestGracefulStop(); process.CloseMainWindow(); if (!process.WaitForExit(timeout) && !process.HasExited) { @@ -121,10 +158,28 @@ public sealed class LauncherProcessSupervisor : IDisposable SetState(LauncherSessionState.Exited); } + /// + /// Applies a state transition, or silently ignores it (Campaign LA + /// plan §LA3 review finding F9): once reaches the + /// terminal , no later call + /// may move it anywhere else, and only + /// fires for a transition that was actually applied. This matters + /// because 's trailing + /// SetState(LauncherSessionState.Running) can race a + /// synchronous callback fired from + /// inside itself (a child that dies immediately) + /// — without this guard, "Running" would silently resurrect a + /// process that has already reported its exit. + /// private void SetState(LauncherSessionState state) { lock (_gate) { + if (State == LauncherSessionState.Exited) + { + return; + } + State = state; } diff --git a/src/AcDream.Launcher.Core/Launching/SessionConfigComposer.cs b/src/AcDream.Launcher.Core/Launching/SessionConfigComposer.cs index 9bba0226..1b4349d2 100644 --- a/src/AcDream.Launcher.Core/Launching/SessionConfigComposer.cs +++ b/src/AcDream.Launcher.Core/Launching/SessionConfigComposer.cs @@ -52,13 +52,7 @@ public static class SessionConfigComposer ArgumentNullException.ThrowIfNull(paths); ArgumentException.ThrowIfNullOrWhiteSpace(sessionId); - string sessionDirectory = Path.Combine( - paths.CacheDirectory, - "launcher", - "sessions", - sessionId); - string configFilePath = Path.Combine(sessionDirectory, "session.json"); - string statusFilePath = Path.Combine(sessionDirectory, "status.jsonl"); + (string configFilePath, string statusFilePath) = BuildSessionPaths(paths, sessionId); SessionCharacterSelector? selector = character.LaunchMode == LaunchMode.GuiSelect ? null @@ -92,7 +86,69 @@ public static class SessionConfigComposer { Process = new SessionProcessSettings { - Paths = new SessionPathOverrides(), + Content = new SessionContentDescriptor + { + DatDirectory = install.DatDirectory, + PreparedAssetPath = install.PreparedAssetPath, + }, + }, + Sessions = [descriptor], + }; + + return new ComposedSessionConfig( + sessionId, + configFilePath, + statusFilePath, + document); + } + + /// + /// Builds a probe session-config document (Campaign LA plan §LA2/ + /// §LA3 review finding F2): the session carries mode: "probe", + /// no character selector, and no policy — the host + /// reports the account's character roster over the status stream and + /// exits without entering the world. plugins/loginCommands + /// don't apply to a probe and are always omitted, exactly like an + /// empty configured set on a normal session. + /// + public static ComposedSessionConfig ComposeProbe( + ServerProfile server, + AccountProfile account, + LauncherInstallRecord install, + ApplicationPathSet paths, + string sessionId) + { + ArgumentNullException.ThrowIfNull(server); + ArgumentNullException.ThrowIfNull(account); + ArgumentNullException.ThrowIfNull(install); + ArgumentNullException.ThrowIfNull(paths); + ArgumentException.ThrowIfNullOrWhiteSpace(sessionId); + + (string configFilePath, string statusFilePath) = BuildSessionPaths(paths, sessionId); + + var descriptor = new SessionDescriptor + { + Id = sessionId, + Mode = "probe", + Endpoint = new SessionEndpointDescriptor + { + Host = server.Host, + Port = server.Port, + }, + Account = account.Account, + Character = null, + Policy = null, + Credential = new SessionCredentialDescriptor(), + Plugins = null, + LoginCommands = null, + LoginCommandDelayMs = null, + StatusFile = statusFilePath, + }; + + var document = new SessionConfigDocument + { + Process = new SessionProcessSettings + { Content = new SessionContentDescriptor { DatDirectory = install.DatDirectory, @@ -149,9 +205,28 @@ 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) + { + string sessionDirectory = Path.Combine( + paths.CacheDirectory, + "launcher", + "sessions", + sessionId); + + return ( + Path.Combine(sessionDirectory, "session.json"), + Path.Combine(sessionDirectory, "status.jsonl")); + } + private static SessionCharacterSelector BuildSelector(CharacterProfile character) { - if (CharacterIdFormat.TryParse(character.Id, out uint id)) + // A parsed id of 0 is not a usable selector — both host loaders + // (App/Headless) reject `id: 0` outright, so falling through to + // the name selector here is the only shape that reaches a real + // character (Campaign LA plan §LA3 review finding F10). + if (CharacterIdFormat.TryParse(character.Id, out uint id) && id != 0) { return new SessionCharacterSelector { Id = id }; } diff --git a/src/AcDream.Launcher.Core/Launching/SessionConfigDocument.cs b/src/AcDream.Launcher.Core/Launching/SessionConfigDocument.cs index fca7c970..0f013f53 100644 --- a/src/AcDream.Launcher.Core/Launching/SessionConfigDocument.cs +++ b/src/AcDream.Launcher.Core/Launching/SessionConfigDocument.cs @@ -29,7 +29,18 @@ public sealed class SessionConfigDocument public sealed class SessionProcessSettings { - public SessionPathOverrides Paths { get; init; } = new(); + /// + /// PINNED CONTRACT (Campaign LA plan §LA3 review, finding F1): the + /// paths KEY is entirely OMITTED from the written JSON unless + /// a caller explicitly supplies overrides — never an empty object. + /// The App-side loader parses with strict + /// UnmappedMemberHandling.Disallow and has no paths + /// member of its own, so an emitted "paths":{} is a null- + /// omission artifact (the object's own members are all optional and + /// omit cleanly, but the containing property was never null itself) + /// that would fail every gui/guiSelect launch at config load. + /// + public SessionPathOverrides? Paths { get; init; } public SessionContentDescriptor Content { get; init; } = new(); } @@ -65,6 +76,13 @@ public sealed class SessionDescriptor { public string Id { get; init; } = string.Empty; + /// Present only for a probe session ("probe", + /// Campaign LA plan §LA2/§LA3) — the host reports the account's + /// character roster and exits without entering the world. OMITTED + /// entirely for a normal gui/guiSelect/headless play session. + /// + public string? Mode { get; init; } + public SessionEndpointDescriptor Endpoint { get; init; } = new(); public string Account { get; init; } = string.Empty; diff --git a/src/AcDream.Launcher.Core/Profiles/CharacterIdFormat.cs b/src/AcDream.Launcher.Core/Profiles/CharacterIdFormat.cs index 55570cd5..573ceef0 100644 --- a/src/AcDream.Launcher.Core/Profiles/CharacterIdFormat.cs +++ b/src/AcDream.Launcher.Core/Profiles/CharacterIdFormat.cs @@ -13,6 +13,15 @@ public static class CharacterIdFormat public static string ToHexString(uint id) => "0x" + id.ToString("X8", CultureInfo.InvariantCulture); + /// + /// Parses as a hex character id — the + /// 0x prefix (case-insensitive) is REQUIRED (Campaign LA plan + /// §LA3 review finding F10). Every all-digit id is ALSO a valid hex + /// number (e.g. "12345678"), so accepting a bare unprefixed + /// string as hex silently reinterprets a hand-typed decimal id and + /// selects the wrong character; requiring the prefix makes "this is + /// hex" an explicit, unambiguous signal instead of a guess. + /// public static bool TryParse(string? text, out uint id) { id = 0; @@ -20,8 +29,10 @@ public static class CharacterIdFormat return false; ReadOnlySpan span = text.AsSpan().Trim(); - if (span.StartsWith("0x", StringComparison.OrdinalIgnoreCase)) - span = span[2..]; + if (!span.StartsWith("0x", StringComparison.OrdinalIgnoreCase)) + return false; + + span = span[2..]; return uint.TryParse( span, diff --git a/src/AcDream.Launcher.Core/Profiles/LauncherProfileStore.cs b/src/AcDream.Launcher.Core/Profiles/LauncherProfileStore.cs index 2fac9f0b..26f3957e 100644 --- a/src/AcDream.Launcher.Core/Profiles/LauncherProfileStore.cs +++ b/src/AcDream.Launcher.Core/Profiles/LauncherProfileStore.cs @@ -66,6 +66,13 @@ public sealed class LauncherProfileStore /// public bool Load() { + // Opportunistic cleanup of a stale ".tmp" left behind by a Save() + // that crashed between creating the temp file and the atomic + // rename (Campaign LA plan §LA3 review finding F4) — a stray + // temp file carries the same plaintext credentials as the real + // store and should not linger. + DeleteStaleTempFile(FilePath + ".tmp"); + if (!File.Exists(FilePath)) { Document = new LauncherProfileDocument(); @@ -108,9 +115,16 @@ public sealed class LauncherProfileStore /// /// Persists to via a /// write-then-atomic-rename so a crash mid-write never leaves a - /// truncated credentials file. On Linux, restricts the final file to - /// owner read/write (0600) per Campaign LA's plaintext-credential - /// decision (spec §5, decisions log). + /// truncated credentials file. On Linux, the temp file is chmod'd to + /// owner read/write (0600) immediately after creation — BEFORE any + /// plaintext credential is serialized into it — so there is no window + /// where the temp file carries the process umask's (potentially + /// world/group-readable) default permissions while holding a + /// password; the final path gets the same restriction after the + /// rename (Campaign LA's plaintext-credential decision, spec §5, + /// decisions log; the temp-file window itself is review finding F4). + /// A failure between temp-file creation and the rename deletes the + /// stale temp file rather than leaving it behind. /// public void Save() { @@ -121,12 +135,27 @@ public sealed class LauncherProfileStore } string tempPath = FilePath + ".tmp"; - using (FileStream stream = File.Create(tempPath)) + try { - JsonSerializer.Serialize(stream, Document, SerializerOptions); - } + using (FileStream stream = File.Create(tempPath)) + { + if (OperatingSystem.IsLinux()) + { + File.SetUnixFileMode( + tempPath, + UnixFileMode.UserRead | UnixFileMode.UserWrite); + } - File.Move(tempPath, FilePath, overwrite: true); + JsonSerializer.Serialize(stream, Document, SerializerOptions); + } + + File.Move(tempPath, FilePath, overwrite: true); + } + catch + { + DeleteStaleTempFile(tempPath); + throw; + } if (OperatingSystem.IsLinux()) { @@ -136,6 +165,23 @@ public sealed class LauncherProfileStore } } + private static void DeleteStaleTempFile(string tempPath) + { + try + { + if (File.Exists(tempPath)) + { + File.Delete(tempPath); + } + } + catch + { + // Best-effort cleanup only — the caller's own exception (a + // failed Save()) or the fresh Load() already in progress is + // what matters; a cleanup failure must not mask either. + } + } + // --- Server CRUD ----------------------------------------------- public ServerProfile AddServer(string name, string host, int port) @@ -312,16 +358,26 @@ public sealed class LauncherProfileStore foreach (CharacterRosterEntry entry in roster) { string idText = CharacterIdFormat.ToHexString(entry.Id); - CharacterProfile? existing = profile.Characters.Find( - character => string.Equals( - character.Id, - idText, - StringComparison.OrdinalIgnoreCase)); - // Defensive fallback for a hand-edited file where a character - // row was added with a name but no id yet. + // Normalize BOTH sides through TryParse/ToHexString rather + // than a raw string compare (Campaign LA plan §LA3 review + // finding F10): a stored id that round-trips to the same + // uint (different case, or — before this fix — no "0x" + // prefix) must match even though its text isn't byte- + // identical to the canonical form this method itself always + // writes. + CharacterProfile? existing = profile.Characters.Find( + character => CharacterIdFormat.TryParse(character.Id, out uint existingId) + && existingId == entry.Id); + + // Defensive fallback for a row whose id is missing OR + // unparseable (e.g. a hand-edited id with no "0x" prefix, + // which TryParse now rejects outright) — match by name + // instead so a later merge self-heals the id into the + // canonical form rather than creating a permanent duplicate + // row. existing ??= profile.Characters.Find( - character => character.Id is null + character => !CharacterIdFormat.TryParse(character.Id, out _) && string.Equals( character.Name, entry.Name, diff --git a/src/AcDream.Launcher.Core/Status/StatusEvent.cs b/src/AcDream.Launcher.Core/Status/StatusEvent.cs index b216392b..b4471e36 100644 --- a/src/AcDream.Launcher.Core/Status/StatusEvent.cs +++ b/src/AcDream.Launcher.Core/Status/StatusEvent.cs @@ -26,7 +26,7 @@ public sealed record ConnectedStatusEvent : StatusEvent; public readonly record struct StatusCharacterEntry( uint Id, string Name, - int SecondsGreyedOut); + uint SecondsGreyedOut); public sealed record CharacterListStatusEvent : StatusEvent { @@ -79,3 +79,18 @@ public sealed record UnknownStatusEvent : StatusEvent { public required string RawJson { get; init; } } + +/// +/// A status line whose e value IS one of the recognized event +/// names, but whose payload does not match that event's expected shape +/// (a missing required field, or a field present with the wrong JSON +/// kind). Distinguished from (Campaign +/// LA plan §LA3 review finding F12) so a launcher can tell "a newer/older +/// host sent an event I've never heard of" apart from "a host I recognize +/// sent me garbage for an event I do know" — the two cases call for +/// different diagnostics. The tailer never throws for either case. +/// +public sealed record MalformedStatusEvent : StatusEvent +{ + public required string Error { get; init; } +} diff --git a/src/AcDream.Launcher.Core/Status/StatusEventParser.cs b/src/AcDream.Launcher.Core/Status/StatusEventParser.cs index 17495b7c..112ddfe5 100644 --- a/src/AcDream.Launcher.Core/Status/StatusEventParser.cs +++ b/src/AcDream.Launcher.Core/Status/StatusEventParser.cs @@ -11,23 +11,40 @@ namespace AcDream.Launcher.Core.Status; /// "accountName":"...","slotCount":6,"characters":[...]}. /// /// -/// Never throws: a line whose e is not one of the eight known -/// values, or whose payload doesn't match that event's expected shape, -/// or that isn't valid JSON at all, degrades to a typed -/// rather than an exception — a -/// launcher must keep tailing a session's status stream even against a -/// host running a newer/older wire version. +/// Never throws: a null/blank/malformed-JSON line, an unrecognized +/// e value, or a recognized e whose payload doesn't match +/// that event's expected shape, all degrade to a typed event +/// ( or +/// — see each type's docs) rather than an exception — a launcher must +/// keep tailing a session's status stream even against a host running a +/// newer/older wire version, or a host that briefly writes a torn line. /// /// public static class StatusEventParser { public static StatusEvent Parse(string line) { - ArgumentException.ThrowIfNullOrWhiteSpace(line); + if (string.IsNullOrWhiteSpace(line)) + { + // Campaign LA plan §LA3 review finding F7: a blank/whitespace + // line is a normal "nothing complete here yet" degrade, not a + // caller error — the old ArgumentException.ThrowIfNullOrWhiteSpace + // guard ran BEFORE the try/catch below and escaped uncaught. + return UnknownEvent(line ?? string.Empty); + } + JsonDocument document; try { - using JsonDocument document = JsonDocument.Parse(line); + document = JsonDocument.Parse(line); + } + catch (JsonException) + { + return UnknownEvent(line); + } + + using (document) + { JsonElement root = document.RootElement; int v = GetInt32OrDefault(root, "v"); @@ -35,51 +52,70 @@ public static class StatusEventParser DateTimeOffset t = GetDateTimeOffsetOrDefault(root, "t"); string sessionId = GetStringOrDefault(root, "sessionId"); - return e switch + try { - "started" => - new StartedStatusEvent { V = v, E = e, T = t, SessionId = sessionId }, - "connected" => - new ConnectedStatusEvent { V = v, E = e, T = t, SessionId = sessionId }, - "characterList" => - ParseCharacterList(root, v, e, t, sessionId), - "enteredWorld" => - ParseEnteredWorld(root, v, e, t, sessionId), - "pluginLoaded" => - ParsePluginLoaded(root, v, e, t, sessionId), - "pluginFailed" => - ParsePluginFailed(root, v, e, t, sessionId), - "disconnected" => - ParseDisconnected(root, v, e, t, sessionId), - "exited" => - ParseExited(root, v, e, t, sessionId), - _ => - new UnknownStatusEvent - { - V = v, - E = e, - T = t, - SessionId = sessionId, - RawJson = line, - }, - }; - } - catch (Exception) - { - // JsonException (malformed JSON), FormatException (a - // required-field miss inside a Parse* helper) — all degrade - // the same way: never throw out of the tailer. - return new UnknownStatusEvent + return e switch + { + "started" => + new StartedStatusEvent { V = v, E = e, T = t, SessionId = sessionId }, + "connected" => + new ConnectedStatusEvent { V = v, E = e, T = t, SessionId = sessionId }, + "characterList" => + ParseCharacterList(root, v, e, t, sessionId), + "enteredWorld" => + ParseEnteredWorld(root, v, e, t, sessionId), + "pluginLoaded" => + ParsePluginLoaded(root, v, e, t, sessionId), + "pluginFailed" => + ParsePluginFailed(root, v, e, t, sessionId), + "disconnected" => + ParseDisconnected(root, v, e, t, sessionId), + "exited" => + ParseExited(root, v, e, t, sessionId), + _ => + new UnknownStatusEvent + { + V = v, + E = e, + T = t, + SessionId = sessionId, + RawJson = line, + }, + }; + } + catch (Exception ex) when (ex is FormatException or InvalidOperationException) { - V = 0, - E = string.Empty, - T = default, - SessionId = string.Empty, - RawJson = line, - }; + // FormatException: a Require* helper found a missing + // field or a field of the wrong JSON kind (e.g. + // "secondsGreyedOut": true"). InvalidOperationException: + // a JsonElement API call (EnumerateArray, TryGetProperty) + // against an element of the wrong ValueKind (e.g. + // "characters" present but not an array). Both mean `e` + // WAS recognized but its payload wasn't — distinguished + // from UnknownStatusEvent (Campaign LA plan §LA3 review + // finding F12). + return new MalformedStatusEvent + { + V = v, + E = e, + T = t, + SessionId = sessionId, + Error = ex.Message, + }; + } } } + private static UnknownStatusEvent UnknownEvent(string rawLine) => + new() + { + V = 0, + E = string.Empty, + T = default, + SessionId = string.Empty, + RawJson = rawLine, + }; + private static StatusEvent ParseCharacterList( JsonElement root, int v, @@ -96,7 +132,7 @@ public static class StatusEventParser { uint id = RequireUInt32(item, "id"); string name = RequireString(item, "name"); - int secondsGreyedOut = RequireInt32(item, "secondsGreyedOut"); + uint secondsGreyedOut = RequireUInt32(item, "secondsGreyedOut"); characters.Add(new StatusCharacterEntry(id, name, secondsGreyedOut)); } diff --git a/src/AcDream.Launcher.Core/Status/StatusFileTailer.cs b/src/AcDream.Launcher.Core/Status/StatusFileTailer.cs index 69b81852..a37b4075 100644 --- a/src/AcDream.Launcher.Core/Status/StatusFileTailer.cs +++ b/src/AcDream.Launcher.Core/Status/StatusFileTailer.cs @@ -33,10 +33,31 @@ public sealed class StatusFileTailer /// /// Reads and parses every complete line appended to the file since /// the last call. Returns an empty list (never null, never throws) - /// when the file doesn't exist yet or nothing new/complete has - /// arrived since the last poll. + /// when the file doesn't exist yet, has been deleted/rotated between + /// the existence check and the open (a TOCTOU window — Campaign LA + /// plan §LA3 review finding F7), or nothing new/complete has arrived + /// since the last poll. /// public IReadOnlyList ReadNewEvents() + { + try + { + return ReadNewEventsCore(); + } + catch (Exception ex) when ( + ex is FileNotFoundException or DirectoryNotFoundException or IOException) + { + // The host process deleted/rotated the file (or its + // directory) between File.Exists and the open below, or + // another transient I/O condition hit mid-read — degrade to + // "nothing new this poll" rather than throwing out of a + // method documented never to throw; the next poll picks up + // wherever the file (or its replacement) actually is. + return []; + } + } + + private IReadOnlyList ReadNewEventsCore() { if (!File.Exists(_path)) { diff --git a/tests/AcDream.Launcher.Core.Tests/LauncherCoreDependencyBoundaryTests.cs b/tests/AcDream.Launcher.Core.Tests/LauncherCoreDependencyBoundaryTests.cs new file mode 100644 index 00000000..8009b02c --- /dev/null +++ b/tests/AcDream.Launcher.Core.Tests/LauncherCoreDependencyBoundaryTests.cs @@ -0,0 +1,79 @@ +using System.Runtime.CompilerServices; +using System.Xml.Linq; + +namespace AcDream.Launcher.Core.Tests; + +// Campaign LA plan §LA3 review finding F5: AcDream.Launcher.Core's entire +// premise (spec §LA3, SessionConfigDocument.cs's "PINNED CONTRACT" remarks) +// is being the BCL-plus-Platform-only assembly the external Avalonia +// launcher (LA4) can reference without pulling in any game-solution +// dependency. That contract is what this guard enforces — the csproj must +// declare exactly one ProjectReference (AcDream.Platform) and zero +// PackageReference entries, forever, in the same spirit as Platform's, +// Runtime's, and Headless's dependency-boundary guards. +public sealed class LauncherCoreDependencyBoundaryTests +{ + [Fact] + public void LauncherCoreProjectReferencesOnlyPlatformAndDeclaresNoPackages() + { + string repositoryRoot = FindRepositoryRoot(); + string projectPath = Path.Combine( + repositoryRoot, + "src", + "AcDream.Launcher.Core", + "AcDream.Launcher.Core.csproj"); + var project = XDocument.Load(projectPath); + + var projectReferences = project.Descendants("ProjectReference") + .Select(element => element.Attribute("Include")?.Value) + // The csproj is authored with Windows-style "..\Foo\Foo.csproj" + // separators; Path.GetFileName only recognizes the platform's + // own separator, so on Linux it would return the whole + // relative path unchanged instead of just the filename. + // Normalizing to '/' first keeps this assertion + // platform-agnostic (this project's tests run under both + // native Windows and WSL — see Campaign LA plan §LA3 review + // finding F5's acceptance). + .Select(include => include is null + ? null + : Path.GetFileName(include.Replace('\\', '/'))) + .ToList(); + + Assert.Equal(["AcDream.Platform.csproj"], projectReferences); + Assert.Empty(project.Descendants("PackageReference")); + } + + private static string FindRepositoryRoot( + [CallerFilePath] string sourcePath = "") + { + string[] starts = + { + Path.GetDirectoryName(sourcePath) ?? string.Empty, + Directory.GetCurrentDirectory(), + AppContext.BaseDirectory, + }; + foreach (string start in starts) + { + if (string.IsNullOrEmpty(start)) + { + continue; + } + + var directory = new DirectoryInfo(start); + 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 above the source, working, or output directory."); + } +} diff --git a/tests/AcDream.Launcher.Core.Tests/Launching/LauncherProcessSupervisorTests.cs b/tests/AcDream.Launcher.Core.Tests/Launching/LauncherProcessSupervisorTests.cs index df12ea29..bab8b6c2 100644 --- a/tests/AcDream.Launcher.Core.Tests/Launching/LauncherProcessSupervisorTests.cs +++ b/tests/AcDream.Launcher.Core.Tests/Launching/LauncherProcessSupervisorTests.cs @@ -109,6 +109,132 @@ public sealed class LauncherProcessSupervisorTests Assert.Equal(0, fake.KillCallCount); } + [Fact] + public void StopAttemptsTheGracefulStopSignalBeforeCloseMainWindow() + { + var factory = new FakeChildProcessFactory(exitsWithinStopTimeout: true); + using var supervisor = new LauncherProcessSupervisor(factory); + supervisor.Start(Spec(), "pw"); + + supervisor.Stop(TimeSpan.FromMilliseconds(50)); + + FakeChildProcess fake = factory.LastCreated!; + Assert.Equal(1, fake.TryRequestGracefulStopCallCount); + Assert.Equal(["gracefulStop", "closeMainWindow"], fake.CallOrder); + } + + [Fact] + public void GracefulStopSignalSendsSigintToARealChildOnLinux() + { + // Review finding F3, proven end to end against the real + // SystemChildProcess: Stop() sends SIGINT before falling back to + // CloseMainWindow/Kill, and the trapped child exits gracefully + // with code 0 well within the timeout. A hard SIGKILL fallback + // (or a signal arriving before the shell's trap is even armed, + // which falls back to the shell's default SIGINT disposition — + // terminate with exit 128+2=130) would not produce this clean + // exit code, so ExitCode == 0 is a hermetic proof the graceful + // path is what actually stopped the child. + if (!OperatingSystem.IsLinux()) + return; + + string readyMarker = Path.Combine( + Path.GetTempPath(), "acdream-la3-sigint-" + Guid.NewGuid().ToString("N")); + try + { + using var supervisor = new LauncherProcessSupervisor(); + var exited = new ManualResetEventSlim(false); + supervisor.StateChanged += (_, s) => + { + if (s == LauncherSessionState.Exited) + exited.Set(); + }; + + supervisor.Start( + new LauncherProcessSpec( + "/bin/bash", + [ + "-c", + "trap 'kill $child 2>/dev/null; exit 0' INT; " + + "sleep 30 & child=$!; " + + $"touch '{readyMarker}'; " + + "wait $child", + ]), + password: null); + + // Wait for the child to prove its SIGINT trap is armed AND + // its background `sleep` is tracked (touch runs after both, + // in program order) before sending the signal — otherwise + // this test would race the shell's own startup and + // intermittently observe the shell's default SIGINT + // disposition instead of the trap, or leave an untracked + // orphaned `sleep`. + DateTime readyDeadline = DateTime.UtcNow + TimeSpan.FromSeconds(5); + while (!File.Exists(readyMarker) && DateTime.UtcNow < readyDeadline) + { + Thread.Sleep(10); + } + + Assert.True( + File.Exists(readyMarker), + "child did not signal trap-armed readiness in time"); + + supervisor.Stop(TimeSpan.FromSeconds(10)); + + Assert.True(exited.Wait(TimeSpan.FromSeconds(5))); + Assert.Equal(0, supervisor.ExitCode); + } + finally + { + try + { + File.Delete(readyMarker); + } + catch (IOException) + { + } + } + } + + [Fact] + public void StartKillsAndDisposesTheChildWhenFeedingStdinThrowsAfterTheProcessHasStarted() + { + var factory = new FakeChildProcessFactory( + exitsWithinStopTimeout: true, + throwOnStandardInputWrite: true); + using var supervisor = new LauncherProcessSupervisor(factory); + + Assert.ThrowsAny(() => supervisor.Start(Spec(), "pw")); + + FakeChildProcess fake = factory.LastCreated!; + Assert.True(fake.Started); + Assert.Equal(1, fake.KillCallCount); + Assert.True(fake.Disposed); + } + + [Fact] + public void SetStateIsMonotonicAndIgnoresATransitionAfterExited() + { + // Simulates the child exiting synchronously from inside + // process.Start() itself (a child that dies immediately) — the + // trailing SetState(Running) at the end of Start() must not + // resurrect State from the terminal Exited it already reached, + // nor fire a spurious StateChanged(Running). + var factory = new FakeChildProcessFactory( + exitsWithinStopTimeout: true, + exitDuringStart: true); + using var supervisor = new LauncherProcessSupervisor(factory); + var states = new List(); + supervisor.StateChanged += (_, s) => states.Add(s); + + supervisor.Start(Spec(), "pw"); + + Assert.Equal(LauncherSessionState.Exited, supervisor.State); + Assert.Equal( + [LauncherSessionState.Starting, LauncherSessionState.Exited], + states); + } + [Fact] public void LauncherProcessSpecCarriesNoCredentialLikeMember() { @@ -162,22 +288,34 @@ public sealed class LauncherProcessSupervisorTests // because this test is itself running under `dotnet test`. OperatingSystem.IsWindows() ? "dotnet.exe" : "dotnet"; - private sealed class FakeChildProcessFactory(bool exitsWithinStopTimeout) + private sealed class FakeChildProcessFactory( + bool exitsWithinStopTimeout, + bool exitDuringStart = false, + bool throwOnStandardInputWrite = false) : ILauncherChildProcessFactory { public FakeChildProcess? LastCreated { get; private set; } public ILauncherChildProcess Create(LauncherProcessSpec spec) { - LastCreated = new FakeChildProcess(spec, exitsWithinStopTimeout); + LastCreated = new FakeChildProcess( + spec, + exitsWithinStopTimeout, + exitDuringStart, + throwOnStandardInputWrite); return LastCreated; } } - private sealed class FakeChildProcess(LauncherProcessSpec spec, bool exitsWithinStopTimeout) + private sealed class FakeChildProcess( + LauncherProcessSpec spec, + bool exitsWithinStopTimeout, + bool exitDuringStart = false, + bool throwOnStandardInputWrite = false) : ILauncherChildProcess { private readonly RecordingTextWriter _standardInput = new(); + private readonly ThrowingTextWriter _throwingStandardInput = new(); public LauncherProcessSpec Spec { get; } = spec; @@ -191,27 +329,59 @@ public sealed class LauncherProcessSupervisorTests public int CloseMainWindowCallCount { get; private set; } + public int TryRequestGracefulStopCallCount { get; private set; } + public int KillCallCount { get; private set; } + public bool Disposed { get; private set; } + + /// Records the order , + /// , and were + /// actually invoked in — review finding F3's ordering guarantee. + /// + public List CallOrder { get; } = []; + public bool HasExited { get; private set; } public int ExitCode { get; private set; } - public TextWriter StandardInput => _standardInput; + public TextWriter StandardInput => + throwOnStandardInputWrite ? _throwingStandardInput : _standardInput; public event EventHandler? Exited; - public void Start() => Started = true; + public void Start() + { + Started = true; + + if (exitDuringStart) + { + // Simulates a child that dies synchronously from inside + // Process.Start() itself (review finding F9's race). + HasExited = true; + ExitCode = 0; + Exited?.Invoke(this, EventArgs.Empty); + } + } + + public bool TryRequestGracefulStop() + { + TryRequestGracefulStopCallCount++; + CallOrder.Add("gracefulStop"); + return false; + } public bool CloseMainWindow() { CloseMainWindowCallCount++; + CallOrder.Add("closeMainWindow"); return true; } public void Kill() { KillCallCount++; + CallOrder.Add("kill"); HasExited = true; ExitCode = -1; Exited?.Invoke(this, EventArgs.Empty); @@ -230,6 +400,7 @@ public sealed class LauncherProcessSupervisorTests public void Dispose() { + Disposed = true; } } @@ -243,4 +414,16 @@ public sealed class LauncherProcessSupervisorTests base.Dispose(disposing); } } + + /// Simulates a broken stdin pipe (review finding F8): the + /// child process started successfully, but feeding it the password + /// fails. + private sealed class ThrowingTextWriter : StringWriter + { + public override void Write(string? value) => + throw new IOException("simulated broken stdin pipe"); + + public override void Write(char value) => + throw new IOException("simulated broken stdin pipe"); + } } diff --git a/tests/AcDream.Launcher.Core.Tests/Launching/SessionConfigComposerTests.cs b/tests/AcDream.Launcher.Core.Tests/Launching/SessionConfigComposerTests.cs index 49d8615b..26bd3c0a 100644 --- a/tests/AcDream.Launcher.Core.Tests/Launching/SessionConfigComposerTests.cs +++ b/tests/AcDream.Launcher.Core.Tests/Launching/SessionConfigComposerTests.cs @@ -135,6 +135,45 @@ public sealed class SessionConfigComposerTests Assert.Equal("+Acdream", (string?)session["character"]!["name"]); } + [Fact] + public void GuiModeFallsBackToNameSelectorWhenIdIsAHandTypedDecimalWithoutThe0xPrefix() + { + // Review finding F10: an 8-digit all-decimal-digit string is ALSO + // a syntactically valid hex number. Without requiring the "0x" + // prefix, this used to silently reinterpret a hand-typed decimal + // id as hex and select the wrong character; it must now fall + // through to the name selector instead of guessing. + ComposedSessionConfig composed = SessionConfigComposer.Compose( + Server(), + Account(), + Character(LaunchMode.Gui, id: "12345678"), + Install, + Paths, + sessionId: "session-gui-decimal-id"); + + JsonObject session = SingleSession(composed); + Assert.Null(session["character"]!["id"]); + Assert.Equal("+Acdream", (string?)session["character"]!["name"]); + } + + [Fact] + public void GuiModeFallsBackToNameSelectorWhenTheParsedIdIsZero() + { + // Review finding F10: both host loaders reject `id: 0` outright, + // so a parsed-but-zero id is not a usable selector either. + ComposedSessionConfig composed = SessionConfigComposer.Compose( + Server(), + Account(), + Character(LaunchMode.Gui, id: "0x00000000"), + Install, + Paths, + sessionId: "session-gui-zero-id"); + + JsonObject session = SingleSession(composed); + Assert.Null(session["character"]!["id"]); + Assert.Equal("+Acdream", (string?)session["character"]!["name"]); + } + [Fact] public void PluginsAndLoginCommandsAreOmittedWhenEmptyRatherThanEmptyArrays() { @@ -156,7 +195,7 @@ public sealed class SessionConfigComposerTests } [Fact] - public void ProcessContentCarriesInstallRecordAndPathsIsAlwaysPresent() + public void ProcessContentCarriesInstallRecordAndPathsIsOmittedByDefault() { ComposedSessionConfig composed = SessionConfigComposer.Compose( Server(), @@ -169,11 +208,15 @@ public sealed class SessionConfigComposerTests JsonObject root = ParseRoot(composed); Assert.Equal(1, (int?)root["version"]); JsonObject process = root["process"]!.AsObject(); - AssertKeys(process, "paths", "content"); - // Paths is always present as an object; every member is omitted - // when unset (hosts resolve their own default ApplicationPathSet). - Assert.Empty(process["paths"]!.AsObject()); + // PINNED CONTRACT (review finding F1): process.paths is OMITTED + // entirely — not an empty object — unless a caller explicitly + // supplies overrides. The App-side loader parses with strict + // UnmappedMemberHandling.Disallow and has no `paths` member of + // its own, so an emitted "paths":{} would reject the whole + // document at config load for every gui/guiSelect launch. + AssertKeys(process, "content"); + Assert.False(process.ContainsKey("paths")); JsonObject content = process["content"]!.AsObject(); AssertKeys(content, "datDirectory", "preparedAssetPath"); @@ -181,6 +224,92 @@ public sealed class SessionConfigComposerTests Assert.Equal(Install.PreparedAssetPath, (string?)content["preparedAssetPath"]); } + [Fact] + public void NormalPlaySessionsOmitTheModeFieldEntirely() + { + foreach (LaunchMode mode in new[] { LaunchMode.Gui, LaunchMode.GuiSelect, LaunchMode.Headless }) + { + ComposedSessionConfig composed = SessionConfigComposer.Compose( + Server(), + Account(), + Character(mode), + Install, + Paths, + sessionId: $"session-mode-omit-{mode}"); + + JsonObject session = SingleSession(composed); + Assert.False(session.ContainsKey("mode")); + } + } + + [Fact] + public void ProbeModeSetsModeAndOmitsCharacterPolicyPluginsAndLoginCommands() + { + ComposedSessionConfig composed = SessionConfigComposer.ComposeProbe( + Server(), + Account(), + Install, + Paths, + sessionId: "session-probe"); + + JsonObject session = SingleSession(composed); + + AssertKeys( + session, + "id", "mode", "endpoint", "account", "credential", "statusFile"); + + Assert.Equal("session-probe", (string?)session["id"]); + Assert.Equal("probe", (string?)session["mode"]); + Assert.Equal("127.0.0.1", (string?)session["endpoint"]!["host"]); + Assert.Equal(9000, (int?)session["endpoint"]!["port"]); + Assert.Equal("testaccount", (string?)session["account"]); + Assert.Equal("standardInput", (string?)session["credential"]!["provider"]); + Assert.False(session.ContainsKey("character")); + Assert.False(session.ContainsKey("policy")); + Assert.False(session.ContainsKey("plugins")); + Assert.False(session.ContainsKey("loginCommands")); + Assert.False(session.ContainsKey("loginCommandDelayMs")); + Assert.Equal( + Path.Combine( + Paths.CacheDirectory, "launcher", "sessions", "session-probe", "status.jsonl"), + (string?)session["statusFile"]); + } + + [Fact] + public void ProbeModeDocumentNeverContainsThePassword() + { + AccountProfile account = Account(); + + ComposedSessionConfig composed = SessionConfigComposer.ComposeProbe( + Server(), + account, + Install, + Paths, + sessionId: "session-probe-pw"); + + string json = SessionConfigComposer.Serialize(composed.Document); + Assert.DoesNotContain(account.Password, json, StringComparison.Ordinal); + } + + [Fact] + public void ProbeModeProcessSettingsMatchNormalComposition() + { + ComposedSessionConfig composed = SessionConfigComposer.ComposeProbe( + Server(), + Account(), + Install, + Paths, + sessionId: "session-probe-content"); + + JsonObject root = ParseRoot(composed); + JsonObject process = root["process"]!.AsObject(); + AssertKeys(process, "content"); + + JsonObject content = process["content"]!.AsObject(); + Assert.Equal(Install.DatDirectory, (string?)content["datDirectory"]); + Assert.Equal(Install.PreparedAssetPath, (string?)content["preparedAssetPath"]); + } + [Fact] public void ComposedDocumentNeverContainsThePassword() { diff --git a/tests/AcDream.Launcher.Core.Tests/Profiles/CharacterIdFormatTests.cs b/tests/AcDream.Launcher.Core.Tests/Profiles/CharacterIdFormatTests.cs index fdade48b..089e0ea7 100644 --- a/tests/AcDream.Launcher.Core.Tests/Profiles/CharacterIdFormatTests.cs +++ b/tests/AcDream.Launcher.Core.Tests/Profiles/CharacterIdFormatTests.cs @@ -14,8 +14,8 @@ public sealed class CharacterIdFormatTests [Theory] [InlineData("0x5000000A", 0x5000000Au)] [InlineData("0x5000000a", 0x5000000Au)] - [InlineData("5000000A", 0x5000000Au)] - public void TryParseAcceptsWithAndWithoutPrefixAndCase(string text, uint expected) + [InlineData("0X5000000A", 0x5000000Au)] + public void TryParseAcceptsThe0xPrefixCaseInsensitively(string text, uint expected) { Assert.True(CharacterIdFormat.TryParse(text, out uint id)); Assert.Equal(expected, id); @@ -26,8 +26,15 @@ public sealed class CharacterIdFormatTests [InlineData("")] [InlineData(" ")] [InlineData("not-hex")] - public void TryParseRejectsNullEmptyOrNonHex(string? text) + [InlineData("5000000A")] + [InlineData("12345678")] + public void TryParseRejectsNullEmptyNonHexOrAnUnprefixedString(string? text) { + // "5000000A"/"12345678" are all-hex-digit strings that would + // parse fine as hex WITHOUT the "0x" prefix — review finding F10 + // requires the prefix precisely so a hand-typed decimal id (which + // is ALSO syntactically valid hex) is never silently + // misinterpreted as one. Assert.False(CharacterIdFormat.TryParse(text, out uint id)); Assert.Equal(0u, id); } diff --git a/tests/AcDream.Launcher.Core.Tests/Profiles/LauncherProfileStoreTests.cs b/tests/AcDream.Launcher.Core.Tests/Profiles/LauncherProfileStoreTests.cs index db6a672a..957fa2cb 100644 --- a/tests/AcDream.Launcher.Core.Tests/Profiles/LauncherProfileStoreTests.cs +++ b/tests/AcDream.Launcher.Core.Tests/Profiles/LauncherProfileStoreTests.cs @@ -1,3 +1,4 @@ +using System.Threading; using AcDream.Launcher.Core.Profiles; namespace AcDream.Launcher.Core.Tests.Profiles; @@ -284,4 +285,105 @@ public sealed class LauncherProfileStoreTests : IDisposable UnixFileMode.UserRead | UnixFileMode.UserWrite, mode); } + + [Fact] + public void SaveNeverLeavesTheTempFileWorldOrGroupReadableDuringTheWrite() + { + // Review finding F4: the temp file used to be created with the + // process's default umask and only chmod'd AFTER the atomic + // rename, leaving a window where the plaintext-credential temp + // file could be world/group-readable. The fix chmods the temp + // file immediately after creation, BEFORE any content (including + // the password) is serialized into it. A large document makes + // the write take long enough for a concurrent poller to have a + // real chance at observing a regression. + if (!OperatingSystem.IsLinux()) + return; + + var store = new LauncherProfileStore(_filePath); + store.Load(); + store.AddServer("Local ACE", "127.0.0.1", 9000); + for (int i = 0; i < 300; i++) + { + store.AddAccount("Local ACE", $"account{i}", new string('x', 4096)); + } + + string tempPath = _filePath + ".tmp"; + bool observedLooseMode = false; + bool stop = false; + var poller = new Thread(() => + { + while (!Volatile.Read(ref stop)) + { + if (File.Exists(tempPath)) + { + try + { + // The platform-compat analyzer can't see the + // enclosing test method's `OperatingSystem.IsLinux()` + // guard across this lambda boundary; suppressed + // rather than restructured, since the guard is + // real and this whole method is a no-op off Linux. +#pragma warning disable CA1416 + UnixFileMode mode = File.GetUnixFileMode(tempPath); +#pragma warning restore CA1416 + if ((mode & ~(UnixFileMode.UserRead | UnixFileMode.UserWrite)) != 0) + { + observedLooseMode = true; + } + } + catch (IOException) + { + // Renamed/deleted between the Exists check and + // GetUnixFileMode — not a finding, just keep + // polling. + } + } + } + }); + poller.Start(); + + store.Save(); + + Volatile.Write(ref stop, true); + poller.Join(); + + Assert.False(observedLooseMode); + } + + [Fact] + public void SaveDeletesTheStaleTempFileWhenTheFinalRenameFails() + { + // Review finding F4: force the rename step to fail (the + // destination path names an existing DIRECTORY, which + // File.Move(..., overwrite: true) refuses to replace — Windows + // reports this as UnauthorizedAccessException, Linux as + // IOException, so the assertion below accepts either) and assert + // the temp file — which still carries the just-serialized + // plaintext credentials — doesn't linger on disk afterward. + Directory.CreateDirectory(_filePath); + var store = new LauncherProfileStore(_filePath); + store.Load(); + store.AddServer("Local ACE", "127.0.0.1", 9000); + store.AddAccount("Local ACE", "testaccount", "testpassword"); + + Assert.ThrowsAny(() => store.Save()); + + Assert.False(File.Exists(_filePath + ".tmp")); + } + + [Fact] + public void LoadDeletesAStaleTempFileLeftBehindByACrashedSave() + { + // Review finding F4: a Save() that crashed between creating the + // temp file and the atomic rename leaves a ".tmp" carrying the + // same plaintext credentials as the real store. Load() cleans it + // up opportunistically the next time the store is opened. + File.WriteAllText(_filePath + ".tmp", """{"version":1,"servers":[]}"""); + + var store = new LauncherProfileStore(_filePath); + store.Load(); + + Assert.False(File.Exists(_filePath + ".tmp")); + } } diff --git a/tests/AcDream.Launcher.Core.Tests/Profiles/RosterMergeTests.cs b/tests/AcDream.Launcher.Core.Tests/Profiles/RosterMergeTests.cs index 9b967ea9..c151be08 100644 --- a/tests/AcDream.Launcher.Core.Tests/Profiles/RosterMergeTests.cs +++ b/tests/AcDream.Launcher.Core.Tests/Profiles/RosterMergeTests.cs @@ -126,6 +126,32 @@ public sealed class RosterMergeTests Assert.Contains(characters, c => c.Name == "+PendingDelete"); } + [Fact] + public void MergeNormalizesAnUnprefixedHexIdInsteadOfCreatingADuplicateRow() + { + // Review finding F10: a hand-edited row can carry an id without + // the "0x" prefix (e.g. copy-pasted from somewhere that dropped + // it). CharacterIdFormat.TryParse now REJECTS that string + // outright (it no longer guesses hex-without-a-prefix), so the + // old raw string-equality comparison against the roster's + // canonical "0x..." form would never match and would add a + // second row forever. The name-fallback match must still + // recognize this as the SAME character and self-heal its id. + LauncherProfileStore store = NewStoreWithServerAndAccount(); + store.Document.Servers.Single().Accounts.Single().Characters.Add( + new CharacterProfile { Id = "5000000A", Name = "+Acdream" }); + + store.MergeRoster( + "Local ACE", + "testaccount", + [new CharacterRosterEntry(0x5000000A, "+Acdream", 0)]); + + CharacterProfile character = Assert.Single( + store.Document.Servers.Single().Accounts.Single().Characters); + Assert.Equal("0x5000000A", character.Id); + Assert.Equal("+Acdream", character.Name); + } + [Fact] public void MergeThrowsForUnknownServerOrAccount() { diff --git a/tests/AcDream.Launcher.Core.Tests/Status/StatusEventParserTests.cs b/tests/AcDream.Launcher.Core.Tests/Status/StatusEventParserTests.cs index ef98becd..d28b642b 100644 --- a/tests/AcDream.Launcher.Core.Tests/Status/StatusEventParserTests.cs +++ b/tests/AcDream.Launcher.Core.Tests/Status/StatusEventParserTests.cs @@ -46,9 +46,9 @@ public sealed class StatusEventParserTests Assert.Equal(2, list.Characters.Count); Assert.Equal(1342177290u, list.Characters[0].Id); Assert.Equal("+Acdream", list.Characters[0].Name); - Assert.Equal(0, list.Characters[0].SecondsGreyedOut); + Assert.Equal(0u, list.Characters[0].SecondsGreyedOut); Assert.Equal(1342177291u, list.Characters[1].Id); - Assert.Equal(1, list.Characters[1].SecondsGreyedOut); + Assert.Equal(1u, list.Characters[1].SecondsGreyedOut); } [Fact] @@ -112,14 +112,59 @@ public sealed class StatusEventParserTests Assert.IsType(e); } - [Fact] - public void KnownEValueWithMissingRequiredFieldSurfacesAsUnknownEventRatherThanThrowing() + [Theory] + [InlineData("")] + [InlineData(" ")] + [InlineData("\t")] + public void WhitespaceOrEmptyLineSurfacesAsUnknownEventRatherThanThrowing(string line) { - // characterList without "characters" — a shape mismatch, not - // just an unrecognized e value. - var e = StatusEventParser.Parse( - """{"v":1,"e":"characterList","t":"2026-08-14T12:00:09Z","sessionId":"s1","accountName":"a","slotCount":6}"""); + // Review finding F7: ArgumentException.ThrowIfNullOrWhiteSpace + // used to guard this method BEFORE the try/catch, so a + // whitespace-only line (e.g. a stray blank line the tailer + // happens to hand over) escaped as an uncaught exception instead + // of degrading like every other malformed-input case. + var e = StatusEventParser.Parse(line); Assert.IsType(e); } + + [Fact] + public void NullLineSurfacesAsUnknownEventRatherThanThrowing() + { + var e = StatusEventParser.Parse(null!); + + Assert.IsType(e); + } + + [Fact] + public void KnownEValueWithMissingRequiredFieldSurfacesAsMalformedEventRatherThanThrowing() + { + // characterList without "characters" — a shape mismatch on a + // KNOWN event name. Review finding F12: this must be + // distinguishable from an unrecognized e value, so it now + // surfaces as MalformedStatusEvent rather than UnknownStatusEvent. + var e = StatusEventParser.Parse( + """{"v":1,"e":"characterList","t":"2026-08-14T12:00:09Z","sessionId":"s1","accountName":"a","slotCount":6}"""); + + var malformed = Assert.IsType(e); + Assert.Equal("characterList", malformed.E); + Assert.Equal("s1", malformed.SessionId); + Assert.False(string.IsNullOrWhiteSpace(malformed.Error)); + } + + [Fact] + public void KnownEValueWithAFieldOfTheWrongJsonKindSurfacesAsMalformedEvent() + { + // "characters" present but not an array — this throws + // InvalidOperationException out of JsonElement.EnumerateArray() + // rather than the FormatException a missing/wrong-kind scalar + // field throws, so it exercises the parser's other malformed- + // payload catch path. + var e = StatusEventParser.Parse( + """{"v":1,"e":"characterList","t":"2026-08-14T12:00:10Z","sessionId":"s1","accountName":"a","slotCount":6,"characters":"not-an-array"}"""); + + var malformed = Assert.IsType(e); + Assert.Equal("characterList", malformed.E); + Assert.False(string.IsNullOrWhiteSpace(malformed.Error)); + } } diff --git a/tests/AcDream.Launcher.Core.Tests/Status/StatusFileTailerTests.cs b/tests/AcDream.Launcher.Core.Tests/Status/StatusFileTailerTests.cs index f908ec06..b619fe25 100644 --- a/tests/AcDream.Launcher.Core.Tests/Status/StatusFileTailerTests.cs +++ b/tests/AcDream.Launcher.Core.Tests/Status/StatusFileTailerTests.cs @@ -141,6 +141,33 @@ public sealed class StatusFileTailerTests : IDisposable Assert.IsType(secondPoll[0]); } + [Fact] + public void ReadNewEventsReturnsEmptyRatherThanThrowingOnASharingViolation() + { + // A deterministic proxy for the File.Exists -> new FileStream + // TOCTOU window (review finding F7): Windows enforces FileShare + // at the OS level, so holding an exclusive (FileShare.None) + // handle open while the tailer tries to open the same path + // reliably reproduces the IOException the tailer must now + // swallow instead of throwing out of a method documented never + // to throw. (.NET's FileStream doesn't apply mandatory locking + // on Linux by default, so this specific scenario isn't + // reproducible there — the fix itself is platform-agnostic, only + // this particular deterministic trigger is Windows-only.) + if (!OperatingSystem.IsWindows()) + return; + + AppendShared(Line("started", "s1")); + using var exclusiveHandle = new FileStream( + _path, FileMode.Open, FileAccess.ReadWrite, FileShare.None); + + var tailer = new StatusFileTailer(_path); + + IReadOnlyList events = tailer.ReadNewEvents(); + + Assert.Empty(events); + } + [Fact] public void RestartsFromTheTopWhenTheFileIsTruncatedOrReplaced() {