fix(launcher): Campaign LA LA3 review fixes — contract paths omission, probe composition, graceful stop, hygiene
Opus review of LA3 returned FIX FIRST; this addresses every finding in
scope (F1-F5, F7-F12; F6 CI-lane addition excluded per instructions):
- F1 (CRITICAL): SessionProcessSettings.Paths is now nullable and left
null by SessionConfigComposer unless a caller supplies overrides, so
the JSON key is entirely absent instead of "paths":{} — the App-side
loader's strict UnmappedMemberHandling.Disallow would otherwise reject
every gui/guiSelect session-config document at load.
- F2: added SessionConfigComposer.ComposeProbe and a nullable
SessionDescriptor.Mode field ("probe", omitted for normal play) per
the pinned contract — no character/policy/plugins/loginCommands.
- F3: LauncherProcessSupervisor.Stop now tries
ILauncherChildProcess.TryRequestGracefulStop (Linux: libc SIGINT via
LibraryImport, K4-proven graceful headless logout) before
CloseMainWindow. Windows has no reliable no-window-console equivalent
today; filed docs/ISSUES.md #397 with the CREATE_NEW_PROCESS_GROUP +
CTRL_BREAK fix direction. Stop()'s blocking-timeout contract is now
documented for LA4.
- F4: LauncherProfileStore.Save chmods the Linux temp file to 0600
immediately after creation, before any credential is serialized;
failure paths and Load() clean up a stale .tmp.
- F5: added LauncherCoreDependencyBoundaryTests asserting Launcher.Core
references exactly AcDream.Platform and no packages.
- F7: StatusEventParser.Parse no longer throws on a whitespace/null
line; StatusFileTailer.ReadNewEvents swallows the File.Exists/open
TOCTOU window (FileNotFoundException/DirectoryNotFoundException/
IOException) instead of throwing.
- F8: Start() now kills (entire process tree) and disposes a child that
started successfully but failed while being fed its stdin password,
instead of orphaning it.
- F9: SetState is monotonic — once Exited, no later transition applies
or fires StateChanged, closing a Start()-path race where a
synchronously-exiting child could be "resurrected" to Running.
- F10: CharacterIdFormat.TryParse now requires the "0x" prefix (an
unprefixed hand-typed decimal id is also valid hex and was silently
misread); a parsed id of 0 is treated as unusable and falls back to
the name selector; LauncherProfileStore.MergeRoster normalizes both
sides through TryParse/ToHexString instead of raw string equality, so
a legacy unprefixed-hex row self-heals via name match instead of
duplicating.
- F11: StatusCharacterEntry.SecondsGreyedOut is now uint, matching
CharacterRosterEntry and the host writer.
- F12: added MalformedStatusEvent, returned for a recognized `e` whose
payload doesn't match its shape, distinguished from UnknownStatusEvent
(an unrecognized `e`).
AllowUnsafeBlocks was added to AcDream.Launcher.Core.csproj — required
by the LibraryImport source generator's function-pointer marshalling
stub for F3's Linux SIGINT P/Invoke.
Verification: dotnet build AcDream.slnx -c Release green (0 errors);
dotnet test tests/AcDream.Launcher.Core.Tests -c Release green at 94/94
on native Windows and under WSL (Ubuntu, verified across multiple runs
for the timing-sensitive SIGINT/sharing-violation tests, no flakes
observed).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
37d74e4402
commit
26feba8186
19 changed files with 1101 additions and 105 deletions
|
|
@ -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();
|
||||
|
||||
/// <summary>
|
||||
/// Attempts a graceful stop signal appropriate to the platform,
|
||||
/// tried BEFORE <see cref="CloseMainWindow"/> (Campaign LA plan §LA3
|
||||
/// review finding F3): a no-window console host (e.g.
|
||||
/// <c>AcDream.Headless</c>) never has a main window for
|
||||
/// <see cref="CloseMainWindow"/> to close, so without this step
|
||||
/// <see cref="LauncherProcessSupervisor.Stop"/> always degraded
|
||||
/// straight to a timeout + hard <see cref="Kill"/> — 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
|
||||
/// <c>docs/ISSUES.md</c> for the tracked gap and fix direction; this
|
||||
/// returns false there. Returns true only when the signal was
|
||||
/// actually delivered; never throws.
|
||||
/// </summary>
|
||||
bool TryRequestGracefulStop();
|
||||
|
||||
/// <summary>Mirrors <see cref="Process.CloseMainWindow"/> — requests
|
||||
/// a graceful close via WM_CLOSE. Returns false for a console/no-
|
||||
/// window process (never throws), matching the real API.</summary>
|
||||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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
|
|||
}
|
||||
|
||||
/// <summary>
|
||||
/// Requests a graceful stop (CloseMainWindow), falling back to Kill
|
||||
/// if the process has not exited within <paramref name="timeout"/>.
|
||||
/// A no-op if <see cref="Start"/> was never called or the process has
|
||||
/// already exited.
|
||||
/// Requests a graceful stop — first
|
||||
/// <see cref="ILauncherChildProcess.TryRequestGracefulStop"/> (SIGINT
|
||||
/// on Linux; a no-op on Windows today, see
|
||||
/// <see cref="ILauncherChildProcess.TryRequestGracefulStop"/>'s docs),
|
||||
/// then <see cref="ILauncherChildProcess.CloseMainWindow"/> — falling
|
||||
/// back to <see cref="ILauncherChildProcess.Kill"/> if the process has
|
||||
/// not exited within <paramref name="timeout"/>. A no-op if
|
||||
/// <see cref="Start"/> was never called or the process has already
|
||||
/// exited.
|
||||
/// <para>
|
||||
/// BLOCKS THE CALLING THREAD for up to <paramref name="timeout"/>
|
||||
/// (via the real child's <c>WaitForExit</c>) — 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).
|
||||
/// </para>
|
||||
/// </summary>
|
||||
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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Applies a state transition, or silently ignores it (Campaign LA
|
||||
/// plan §LA3 review finding F9): once <see cref="State"/> reaches the
|
||||
/// terminal <see cref="LauncherSessionState.Exited"/>, no later call
|
||||
/// may move it anywhere else, and <see cref="StateChanged"/> only
|
||||
/// fires for a transition that was actually applied. This matters
|
||||
/// because <see cref="Start"/>'s trailing
|
||||
/// <c>SetState(LauncherSessionState.Running)</c> can race a
|
||||
/// synchronous <see cref="OnProcessExited"/> callback fired from
|
||||
/// inside <see cref="Start"/> itself (a child that dies immediately)
|
||||
/// — without this guard, "Running" would silently resurrect a
|
||||
/// process that has already reported its exit.
|
||||
/// </summary>
|
||||
private void SetState(LauncherSessionState state)
|
||||
{
|
||||
lock (_gate)
|
||||
{
|
||||
if (State == LauncherSessionState.Exited)
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
State = state;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Builds a probe session-config document (Campaign LA plan §LA2/
|
||||
/// §LA3 review finding F2): the session carries <c>mode: "probe"</c>,
|
||||
/// no <c>character</c> selector, and no <c>policy</c> — the host
|
||||
/// reports the account's character roster over the status stream and
|
||||
/// exits without entering the world. <c>plugins</c>/<c>loginCommands</c>
|
||||
/// don't apply to a probe and are always omitted, exactly like an
|
||||
/// empty configured set on a normal session.
|
||||
/// </summary>
|
||||
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 };
|
||||
}
|
||||
|
|
|
|||
|
|
@ -29,7 +29,18 @@ public sealed class SessionConfigDocument
|
|||
|
||||
public sealed class SessionProcessSettings
|
||||
{
|
||||
public SessionPathOverrides Paths { get; init; } = new();
|
||||
/// <summary>
|
||||
/// PINNED CONTRACT (Campaign LA plan §LA3 review, finding F1): the
|
||||
/// <c>paths</c> 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
|
||||
/// <c>UnmappedMemberHandling.Disallow</c> and has no <c>paths</c>
|
||||
/// member of its own, so an emitted <c>"paths":{}</c> 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.
|
||||
/// </summary>
|
||||
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;
|
||||
|
||||
/// <summary>Present only for a probe session (<c>"probe"</c>,
|
||||
/// 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.
|
||||
/// </summary>
|
||||
public string? Mode { get; init; }
|
||||
|
||||
public SessionEndpointDescriptor Endpoint { get; init; } = new();
|
||||
|
||||
public string Account { get; init; } = string.Empty;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue