Robustness: bounded ASIO close, native service self-update, rollback pinned (review Phase 3)
- AsioApartment gains a bounded Invoke; the close path uses it (8s cap - a healthy close is ~5ms). A driver that wedges inside Stop/Dispose can no longer hang a live driver-switch or the resume path: the caller logs the timeout and abandons the driver (old park semantics, reclaimed at process exit). Apartment test extended to pin the timeout path. - Service self-update is now NATIVE code: a detached copy of the NEW build runs --service-selfupdate (stop -> CopyProgramTo -> start, every step logged to the update log). The old PowerShell restart script silently died wherever Group Policy enforces execution policy (Bypass is ignored there), stranding the service on the old build. Verb wired through IsServiceInvocation + ServiceEntry; verb-gate test now pins all six verbs. - UpdateApplier SwapInNewFiles/RollBack made internal + pinned by a real-folder test: swap lands exactly the release's files (user files untouched), backup holds the originals, rollback restores BYTE-EXACT including deleting newly-created files - the contract that stops a failed update bricking an install. - RouterPortMapper: discovery callback now checks disposed under the gate, so an in-flight callback can't re-open the port map Dispose just removed. Gate 59/59. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -23,7 +23,8 @@ internal static class Program
|
||||
internal static bool IsServiceInvocation(string[] args) =>
|
||||
HasArg(args, ServiceControl.RunVerb) || HasArg(args, ServiceControl.InstallVerb)
|
||||
|| HasArg(args, ServiceControl.UninstallVerb)
|
||||
|| HasArg(args, ServiceControl.StartVerb) || HasArg(args, ServiceControl.StopVerb);
|
||||
|| HasArg(args, ServiceControl.StartVerb) || HasArg(args, ServiceControl.StopVerb)
|
||||
|| HasArg(args, ServiceControl.SelfUpdateVerb);
|
||||
|
||||
// Writes an otherwise-fatal exception to a timestamped crash file in the logs folder, so a
|
||||
// "RemSound just disappeared, no dialog" report (#16) leaves a stack behind to diagnose instead
|
||||
|
||||
@@ -237,6 +237,10 @@ internal sealed class RouterPortMapper : IDisposable
|
||||
|
||||
private void OnDeviceFound(object? sender, DeviceEventArgs args)
|
||||
{
|
||||
// A discovery callback can already be in flight when Stop()/Dispose() unsubscribes. Without this
|
||||
// guard it would re-open the port map that Dispose just removed, leaving the router forwarding to
|
||||
// us until the lease expires. Narrow race, cheap check (review sweep).
|
||||
lock (gate) { if (disposed) return; }
|
||||
try
|
||||
{
|
||||
var found = args.Device;
|
||||
|
||||
@@ -67,6 +67,7 @@ internal static class SelfTest
|
||||
RunStep(results, "Heartbeat wire round-trip + ping→pong echo", HeartbeatEcho);
|
||||
RunStep(results, "Profile persistence tripwire (every field wired or declared)", ProfileDriftTripwire);
|
||||
RunStep(results, "Cue variant resolution (dedupe, order, chosen-default fallback)", CueVariantResolution);
|
||||
RunStep(results, "Updater swap + rollback (a failed update restores exactly)", UpdaterSwapRollback);
|
||||
RunStep(results, "App settings save and reload", SettingsRoundTrip);
|
||||
RunStep(results, "Per-peer shaping DSP", PeerShapingDsp);
|
||||
RunStep(results, "Multi-output fan-out (both lanes)", FanOutToBothOutputs);
|
||||
@@ -962,6 +963,51 @@ internal static class SelfTest
|
||||
return "follower flagged + sentinel shared with the app; service resolves it to the live default render endpoint";
|
||||
}
|
||||
|
||||
/// <summary>The updater's back-up-and-swap is the one piece of code that can BRICK an install: a bad
|
||||
/// rollback leaves a half-swapped folder that won't start. Pins, on real temp folders: the swap
|
||||
/// replaces + adds exactly the release's files (user files untouched), the backup holds the originals,
|
||||
/// and RollBack restores the target to its byte-exact pre-swap state including deleting new files.</summary>
|
||||
private static string? UpdaterSwapRollback()
|
||||
{
|
||||
var root = Path.Combine(Path.GetTempPath(), "remsound-selftest-swap-" + Guid.NewGuid().ToString("N"));
|
||||
var source = Path.Combine(root, "source");
|
||||
var target = Path.Combine(root, "target");
|
||||
var backup = Path.Combine(target, "_update-backup");
|
||||
try
|
||||
{
|
||||
Directory.CreateDirectory(Path.Combine(source, "sub"));
|
||||
Directory.CreateDirectory(target);
|
||||
File.WriteAllText(Path.Combine(source, "a.dll"), "A-NEW");
|
||||
File.WriteAllText(Path.Combine(source, "b.dll"), "B-NEW");
|
||||
File.WriteAllText(Path.Combine(source, "sub", "c.dll"), "C-NEW");
|
||||
File.WriteAllText(Path.Combine(target, "a.dll"), "A-OLD");
|
||||
File.WriteAllText(Path.Combine(target, "user.json"), "USER"); // not in the release — must survive everything
|
||||
|
||||
var moved = new List<(string backup, string dest)>();
|
||||
var created = new List<string>();
|
||||
UpdateApplier.SwapInNewFiles(source, target, backup, moved, created, _ => { });
|
||||
|
||||
Check(File.ReadAllText(Path.Combine(target, "a.dll")) == "A-NEW"
|
||||
&& File.ReadAllText(Path.Combine(target, "b.dll")) == "B-NEW"
|
||||
&& File.ReadAllText(Path.Combine(target, "sub", "c.dll")) == "C-NEW",
|
||||
"the swap must land every release file, including subfolders");
|
||||
Check(File.ReadAllText(Path.Combine(target, "user.json")) == "USER", "files not in the release must be untouched");
|
||||
Check(moved.Count == 1 && created.Count == 2, $"bookkeeping must be exact (moved {moved.Count}, created {created.Count})");
|
||||
Check(File.ReadAllText(Path.Combine(backup, "a.dll")) == "A-OLD", "the backup must hold the original bytes");
|
||||
|
||||
UpdateApplier.RollBack(moved, created, _ => { });
|
||||
Check(File.ReadAllText(Path.Combine(target, "a.dll")) == "A-OLD", "rollback must restore the original file bytes");
|
||||
Check(!File.Exists(Path.Combine(target, "b.dll")) && !File.Exists(Path.Combine(target, "sub", "c.dll")),
|
||||
"rollback must DELETE files the failed update newly created");
|
||||
Check(File.ReadAllText(Path.Combine(target, "user.json")) == "USER", "user files must survive the rollback too");
|
||||
return "swap exact + user files untouched; rollback restores byte-exact incl. deleting new files";
|
||||
}
|
||||
finally
|
||||
{
|
||||
try { Directory.Delete(root, recursive: true); } catch { /* temp cleanup is best-effort */ }
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Cue variant resolution feeds the Preferences sound picker AND which WAV actually plays.
|
||||
/// Wrong resolution = the wrong sound (or a duplicate 'Sound 1' row NVDA can't disambiguate). Pins:
|
||||
/// numbered ordering, the drop-bare-when-numbered-exists dedupe, the 'Sound N' labels, and the
|
||||
@@ -1958,7 +2004,7 @@ internal static class SelfTest
|
||||
foreach (var verb in new[]
|
||||
{
|
||||
ServiceControl.RunVerb, ServiceControl.InstallVerb, ServiceControl.UninstallVerb,
|
||||
ServiceControl.StartVerb, ServiceControl.StopVerb,
|
||||
ServiceControl.StartVerb, ServiceControl.StopVerb, ServiceControl.SelfUpdateVerb,
|
||||
})
|
||||
{
|
||||
Check(Program.IsServiceInvocation(new[] { verb }), $"'{verb}' must be recognised as a service invocation");
|
||||
@@ -1976,7 +2022,7 @@ internal static class SelfTest
|
||||
if (!loadedBefore)
|
||||
Check(!IsAssemblyLoaded(svcAsm), "deciding a normal launch must not load the Windows-service assembly");
|
||||
|
||||
return "normal launches stay load-safe; all five service verbs recognised (case-insensitive)";
|
||||
return "normal launches stay load-safe; all six service verbs recognised (case-insensitive)";
|
||||
}
|
||||
|
||||
/// <summary>The Service menu's "View service log" opens the newest diagnostic log — the log that says
|
||||
@@ -2323,7 +2369,17 @@ internal static class SelfTest
|
||||
var ranAfter = false;
|
||||
apt.Invoke(() => ranAfter = true);
|
||||
Check(ranAfter, "the apartment must keep working after a work item threw");
|
||||
return "runs work on one dedicated STA thread; exceptions propagate; survives a throw";
|
||||
|
||||
// Bounded invoke: a wedged work item must return false promptly instead of hanging the caller
|
||||
// (this is what stops a wedged ASIO driver freezing a live driver-switch or the resume path).
|
||||
using var release = new ManualResetEventSlim(false);
|
||||
var sw = Stopwatch.StartNew();
|
||||
var completed = apt.Invoke(() => release.Wait(3000), timeoutMs: 150);
|
||||
sw.Stop();
|
||||
Check(!completed, "a work item that overruns the timeout must report false, not block");
|
||||
Check(sw.ElapsedMilliseconds < 1500, $"the bounded wait must return promptly (took {sw.ElapsedMilliseconds} ms)");
|
||||
release.Set(); // free the apartment thread so Dispose joins cleanly
|
||||
return "one dedicated STA thread; exceptions propagate; survives a throw; bounded invoke times out clean";
|
||||
}
|
||||
|
||||
/// <summary>The instant capture-on-app-open watcher (AudioSessionStartWatcher) must construct, re-hook
|
||||
|
||||
@@ -251,6 +251,66 @@ public static class ServiceControl
|
||||
return rc;
|
||||
}
|
||||
|
||||
/// <summary>Verb for the native self-update helper (see <see cref="DoSelfUpdate"/>). Spawned BY the
|
||||
/// running service as SYSTEM, so no elevation is involved.</summary>
|
||||
public const string SelfUpdateVerb = "--service-selfupdate";
|
||||
|
||||
/// <summary>The self-update worker: stop the service, copy the recorded app-source build into the
|
||||
/// service's own bin, start the service again — all in managed code. Replaces the old PowerShell
|
||||
/// restart script: where execution policy is enforced by Group Policy, the script's -ExecutionPolicy
|
||||
/// Bypass is IGNORED and the self-update silently died, stranding the service on the old build.
|
||||
/// Native code has no policy to fall foul of. Runs detached (spawned by the service just before the
|
||||
/// stop kills it); logs every step to the update log so the after-death part is still recorded.</summary>
|
||||
public static int DoSelfUpdate()
|
||||
{
|
||||
static void Log(string m)
|
||||
{
|
||||
try
|
||||
{
|
||||
Directory.CreateDirectory(ServiceStore.Directory);
|
||||
File.AppendAllText(ServiceStore.UpdateLogPath, $"{DateTime.Now:yyyy-MM-dd HH:mm:ss} selfupdate: {m}\r\n");
|
||||
}
|
||||
catch { /* logging is best-effort */ }
|
||||
}
|
||||
try
|
||||
{
|
||||
Log($"stopping {ServiceName}");
|
||||
try
|
||||
{
|
||||
using var sc = new ServiceController(ServiceName);
|
||||
if (sc.Status != ServiceControllerStatus.Stopped)
|
||||
{
|
||||
sc.Stop();
|
||||
sc.WaitForStatus(ServiceControllerStatus.Stopped, TimeSpan.FromSeconds(30));
|
||||
}
|
||||
}
|
||||
catch (Exception ex) { Log($"stop failed ({ex.GetType().Name}: {ex.Message}) — continuing"); }
|
||||
|
||||
var appDir = ServiceStore.LoadAppSourcePath();
|
||||
if (!string.IsNullOrEmpty(appDir) && Directory.Exists(appDir))
|
||||
{
|
||||
// CopyProgramTo already excludes every user-state folder — same routine the installer uses.
|
||||
try { CopyProgramTo(appDir, ServiceStore.BinDirectory); Log("copied the new build into bin"); }
|
||||
catch (Exception ex) { Log($"COPY FAILED ({ex.GetType().Name}: {ex.Message}) — starting the existing build"); }
|
||||
}
|
||||
else
|
||||
{
|
||||
Log("no app-source recorded; restarting onto the existing bin");
|
||||
}
|
||||
|
||||
try
|
||||
{
|
||||
using var sc = new ServiceController(ServiceName);
|
||||
sc.Start();
|
||||
sc.WaitForStatus(ServiceControllerStatus.Running, TimeSpan.FromSeconds(30));
|
||||
Log("service started");
|
||||
return 0;
|
||||
}
|
||||
catch (Exception ex) { Log($"START FAILED — {ex.GetType().Name}: {ex.Message}"); return 1; }
|
||||
}
|
||||
catch (Exception ex) { Log($"FATAL {ex.GetType().Name}: {ex.Message}"); return 2; }
|
||||
}
|
||||
|
||||
/// <summary>Restart the service WITHOUT elevation, using the start/stop rights the installer granted
|
||||
/// the installing user (the SDDL ACE from <see cref="GrantUserStartStop"/>). Returns true when the
|
||||
/// service ends up Running. No UAC prompt, no elevated helper — so it's safe to run from a background
|
||||
|
||||
@@ -38,6 +38,7 @@ internal static class ServiceEntry
|
||||
: Has(args, ServiceControl.UninstallVerb) ? "uninstall"
|
||||
: Has(args, ServiceControl.StartVerb) ? "start"
|
||||
: Has(args, ServiceControl.StopVerb) ? "stop"
|
||||
: Has(args, ServiceControl.SelfUpdateVerb) ? "selfupdate"
|
||||
: null;
|
||||
if (verb is null) return 0;
|
||||
|
||||
@@ -54,6 +55,7 @@ internal static class ServiceEntry
|
||||
"uninstall" => ServiceControl.DoUninstall(),
|
||||
"start" => ServiceControl.DoStart(),
|
||||
"stop" => ServiceControl.DoStop(),
|
||||
"selfupdate" => ServiceControl.DoSelfUpdate(),
|
||||
_ => 0,
|
||||
};
|
||||
ServiceStore.AppendServiceEvent($"elevated {verb}: finished with code {rc}");
|
||||
|
||||
@@ -51,42 +51,31 @@ internal static class ServiceUpdate
|
||||
|
||||
public static Version? RunningVersion() => Assembly.GetExecutingAssembly().GetName().Version;
|
||||
|
||||
/// <summary>Restart the service onto the new binary. Spawns a DETACHED PowerShell (as SYSTEM, inherited
|
||||
/// from the service) that stops this service — which exits this process — then starts it again, so the
|
||||
/// SCM launches the freshly-installed exe. The script LOGS its own stop/start outcome to the update log,
|
||||
/// so even the part that runs after this process is gone (and any failed start) is recorded. Never
|
||||
/// throws; worst case the service picks up the update on the next reboot.</summary>
|
||||
/// <summary>Restart the service onto the new binary. Spawns a DETACHED helper — a copy of the NEW
|
||||
/// RemSound.exe running the <see cref="ServiceControl.SelfUpdateVerb"/> verb, as SYSTEM (inherited
|
||||
/// from the service) — which stops this service (exiting this process), copies the new build into the
|
||||
/// service bin, and starts the service again. All managed code: the previous PowerShell restart
|
||||
/// script silently died on machines where Group Policy enforces execution policy (Bypass is ignored
|
||||
/// there), stranding the service on the old build. The helper logs every step to the update log, so
|
||||
/// the part that runs after this process is gone is still recorded. Never throws; worst case the
|
||||
/// service picks up the update on the next reboot.</summary>
|
||||
public static void RestartSelf()
|
||||
{
|
||||
try
|
||||
{
|
||||
// Run the helper from the NEW build in the app-source folder — it must not execute from the
|
||||
// bin it's about to overwrite. Falls back to the bin exe when no app source is recorded (the
|
||||
// helper then just restarts the service without copying, so nothing conflicts).
|
||||
var appDir = ServiceStore.LoadAppSourcePath();
|
||||
var binDir = ServiceStore.BinDirectory;
|
||||
var dir = ServiceStore.Directory;
|
||||
System.IO.Directory.CreateDirectory(dir);
|
||||
var script = Path.Combine(dir, "restart.ps1");
|
||||
var log = ServiceStore.UpdateLogPath;
|
||||
var svc = ServiceControl.ServiceName;
|
||||
// robocopy the new build into bin, minus user-state; exit codes 0-7 are success (8+ = failure).
|
||||
var copyLine = string.IsNullOrEmpty(appDir)
|
||||
? "$rc = 0 # no app-source recorded; restart onto whatever is already in bin"
|
||||
: $"robocopy \"{appDir}\" \"{binDir}\" /E /XD \"user settings and logs\" logs recordings profiles config /XF \"global config.json\" remsound.config.json /R:2 /W:1 | Out-Null; $rc = $LASTEXITCODE";
|
||||
var content =
|
||||
"$ts = { (Get-Date).ToString('yyyy-MM-dd HH:mm:ss') }\r\n" +
|
||||
$"Add-Content -LiteralPath '{log}' -Value \"$(& $ts) restarter: stopping {svc}\"\r\n" +
|
||||
$"Stop-Service -Name {svc} -Force -ErrorAction SilentlyContinue\r\n" +
|
||||
copyLine + "\r\n" +
|
||||
$"Add-Content -LiteralPath '{log}' -Value \"$(& $ts) restarter: copied new build (robocopy code $rc)\"\r\n" +
|
||||
$"if ($rc -ge 8) {{ Add-Content -LiteralPath '{log}' -Value \"$(& $ts) restart: COPY FAILED (code $rc) - starting existing build\" }}\r\n" +
|
||||
$"try {{ Start-Service -Name {svc} -ErrorAction Stop; $r = 'restart: service started' }} catch {{ $r = 'restart: START FAILED - ' + $_.Exception.Message }}\r\n" +
|
||||
$"Add-Content -LiteralPath '{log}' -Value \"$(& $ts) $r\"\r\n";
|
||||
File.WriteAllText(script, content);
|
||||
var appExe = string.IsNullOrEmpty(appDir) ? null : Path.Combine(appDir, "RemSound.exe");
|
||||
var helperExe = appExe is not null && File.Exists(appExe) ? appExe : ServiceStore.BinExePath;
|
||||
var psi = new ProcessStartInfo
|
||||
{
|
||||
FileName = "powershell.exe",
|
||||
Arguments = $"-NonInteractive -WindowStyle Hidden -ExecutionPolicy Bypass -File \"{script}\"",
|
||||
FileName = helperExe,
|
||||
Arguments = ServiceControl.SelfUpdateVerb,
|
||||
UseShellExecute = false,
|
||||
CreateNoWindow = true,
|
||||
WorkingDirectory = Path.GetDirectoryName(helperExe)!,
|
||||
};
|
||||
Process.Start(psi);
|
||||
}
|
||||
|
||||
@@ -124,8 +124,10 @@ internal static class UpdateApplier
|
||||
|
||||
/// <summary>Rename each existing target file aside into the backup folder, then copy the new
|
||||
/// one in. Throws if a file genuinely can't be replaced after the retry window — the caller
|
||||
/// rolls back. Only writes files that exist in the new release, so user data is left untouched.</summary>
|
||||
private static void SwapInNewFiles(string source, string target, string backupDir,
|
||||
/// rolls back. Only writes files that exist in the new release, so user data is left untouched.
|
||||
/// Internal (was private) so the self-test can pin the swap/rollback contract — a broken rollback
|
||||
/// is the one failure mode of the updater that bricks an install.</summary>
|
||||
internal static void SwapInNewFiles(string source, string target, string backupDir,
|
||||
List<(string backup, string dest)> moved, List<string> created, Action<string> log)
|
||||
{
|
||||
var srcFull = Path.GetFullPath(source);
|
||||
@@ -154,8 +156,9 @@ internal static class UpdateApplier
|
||||
log($"swap complete: {copiedCount} files in, {moved.Count} replaced, {created.Count} new");
|
||||
}
|
||||
|
||||
/// <summary>Restore the old files (and remove the partial new ones) after a failed swap.</summary>
|
||||
private static void RollBack(List<(string backup, string dest)> moved, List<string> created, Action<string> log)
|
||||
/// <summary>Restore the old files (and remove the partial new ones) after a failed swap.
|
||||
/// Internal for the self-test — see <see cref="SwapInNewFiles"/>.</summary>
|
||||
internal static void RollBack(List<(string backup, string dest)> moved, List<string> created, Action<string> log)
|
||||
{
|
||||
foreach (var dest in created)
|
||||
{
|
||||
|
||||
@@ -46,15 +46,23 @@ internal sealed class AsioApartment : IDisposable
|
||||
/// <summary>Run <paramref name="action"/> on the apartment thread and block until it finishes,
|
||||
/// rethrowing anything it raised. If called from the apartment thread itself, or after Dispose, runs
|
||||
/// inline so a teardown path can never deadlock on itself.</summary>
|
||||
public void Invoke(Action action)
|
||||
public void Invoke(Action action) => Invoke(action, Timeout.Infinite);
|
||||
|
||||
/// <summary>Bounded variant: wait at most <paramref name="timeoutMs"/> for the work to finish.
|
||||
/// Returns false on timeout — the work is NOT cancelled (it may still complete later on the
|
||||
/// apartment thread); the caller just stops waiting. Used by the ASIO close path so a driver that
|
||||
/// wedges on Stop/Dispose can no longer hang a live driver-switch or the resume path — the caller
|
||||
/// abandons the driver (the old park semantics) and the OS reclaims it at process exit.</summary>
|
||||
public bool Invoke(Action action, int timeoutMs)
|
||||
{
|
||||
if (shutdown || Thread.CurrentThread == thread) { action(); return; }
|
||||
if (shutdown || Thread.CurrentThread == thread) { action(); return true; }
|
||||
var item = new WorkItem { Action = action, Done = new ManualResetEventSlim(false) };
|
||||
lock (queueGate) queue.Enqueue(item);
|
||||
workAvailable.Set();
|
||||
item.Done.Wait();
|
||||
if (!item.Done.Wait(timeoutMs)) return false; // deliberately NOT disposing Done — the worker's finally will still Set it
|
||||
item.Done.Dispose();
|
||||
if (item.Error is not null) throw item.Error;
|
||||
return true;
|
||||
}
|
||||
|
||||
private void Run()
|
||||
|
||||
@@ -229,7 +229,12 @@ internal sealed class AsioCaptureBackend : ICaptureBackend
|
||||
// blew past these try/catch blocks with no managed stack). Step-by-step logging still pinpoints
|
||||
// any native call that dies, and the callback is unhooked + drained before stop/dispose so the
|
||||
// close isn't racing a live buffer callback (a common trigger for the crash).
|
||||
apartment.Invoke(() =>
|
||||
//
|
||||
// BOUNDED (review sweep): a driver that wedges inside Stop/Dispose used to hang the CALLER
|
||||
// forever — and live driver-switches and the resume path close on the UI thread. 8 s is
|
||||
// generous for a healthy close (Ed's Audient releases in ~5 ms); past that we abandon the
|
||||
// driver (the old park semantics — the OS reclaims it at process exit) and move on.
|
||||
var closed = apartment.Invoke(() =>
|
||||
{
|
||||
onDiagnostic?.Invoke("asio close: unhooking callback");
|
||||
try { toClose.AudioAvailable -= OnAudioAvailable; } catch { /* ignore */ }
|
||||
@@ -239,7 +244,9 @@ internal sealed class AsioCaptureBackend : ICaptureBackend
|
||||
onDiagnostic?.Invoke("asio close: releasing driver (dispose)");
|
||||
try { toClose.Dispose(); } catch (Exception ex) { onDiagnostic?.Invoke($"asio close: dispose threw {ex.GetType().Name}: {ex.Message}"); }
|
||||
onDiagnostic?.Invoke("asio close: driver released cleanly");
|
||||
});
|
||||
}, timeoutMs: 8000);
|
||||
if (!closed)
|
||||
onDiagnostic?.Invoke("asio close: TIMED OUT after 8s — abandoning the driver (parked; reclaimed at process exit)");
|
||||
asio = null;
|
||||
}
|
||||
uptime.Stop();
|
||||
|
||||
Reference in New Issue
Block a user