diff --git a/src/Netclaw.Cli.Tests/Daemon/DaemonPathEnvironmentFileTests.cs b/src/Netclaw.Cli.Tests/Daemon/DaemonPathEnvironmentFileTests.cs index db130cdc5..78ff21425 100644 --- a/src/Netclaw.Cli.Tests/Daemon/DaemonPathEnvironmentFileTests.cs +++ b/src/Netclaw.Cli.Tests/Daemon/DaemonPathEnvironmentFileTests.cs @@ -108,6 +108,15 @@ public void BuildDaemonUnitContent_WiresEnvironmentFile_AndOmitsInlinePath() Assert.DoesNotContain("Environment=PATH=", unit, StringComparison.Ordinal); Assert.Contains("ExecStart=/opt/netclaw/netclawd", unit, StringComparison.Ordinal); Assert.Contains("ExecStop=/opt/netclaw/netclaw daemon stop", unit, StringComparison.Ordinal); + + // Producer/consumer contract (netclaw-dev/netclaw#1665): the generated unit's + // TimeoutStopSec= must track DaemonConfig.SystemdTimeoutStopSec, not a stale literal, + // so systemd never SIGKILLs the cgroup out from under a still-legitimately-waiting + // `netclaw daemon stop` (ExecStop=). + Assert.Contains( + $"TimeoutStopSec={(int)DaemonConfig.SystemdTimeoutStopSec.TotalSeconds}", + unit, + StringComparison.Ordinal); } [Fact] diff --git a/src/Netclaw.Cli/Daemon/DaemonManager.cs b/src/Netclaw.Cli/Daemon/DaemonManager.cs index 57ddaabba..5af8f115d 100644 --- a/src/Netclaw.Cli/Daemon/DaemonManager.cs +++ b/src/Netclaw.Cli/Daemon/DaemonManager.cs @@ -177,10 +177,29 @@ await http.PostAsync( process.Kill(); } - // Wait up to 10 seconds for graceful exit. - if (!await WaitForExitAsync(process, TimeSpan.FromSeconds(10), cancellationToken)) - { - // Timed out — hard cutoff. + // Wait for graceful exit. DaemonConfig.GracefulShutdownBudget matches the daemon's own + // Akka CoordinatedShutdown "before-service-unbind" phase timeout — where session + // draining (SessionDrainHelper.DrainAsync) actually happens — so the CLI does not give + // up on a daemon that is still legitimately draining in-flight sessions (TurnLlmTimeout + // defaults to 3 minutes). See DaemonConfig.GracefulShutdownBudget remarks for the full + // layering this must respect: bounded drain < Akka phase timeout < this budget + the + // grace window below < systemd's TimeoutStopSec= (netclaw-dev/netclaw#1664, #1665). + var exitedWithinBudget = await WaitForExitAsync(process, DaemonConfig.GracefulShutdownBudget, cancellationToken); + var exitedDuringGraceWindow = false; + if (!exitedWithinBudget) + { + // The daemon's own bounded drain (netclaw-dev/netclaw#1664) can time out at almost + // exactly this same budget boundary, and still needs to finish tearing down (actor + // system termination, PID file cleanup) afterward. Poll a short additional grace + // window before escalating to SIGKILL — production evidence (#1665) showed a + // daemon force-killed ~100ms from a clean exit because the CLI escalated the + // instant its budget elapsed, with no headroom at all. + exitedDuringGraceWindow = await WaitForExitAsync(process, DaemonConfig.CliForceKillGraceWindow, cancellationToken); + } + + if (!exitedWithinBudget && !exitedDuringGraceWindow) + { + // Both the budget and the grace window elapsed — hard cutoff. string? killError = null; if (OperatingSystem.IsLinux() || OperatingSystem.IsMacOS()) { @@ -203,7 +222,27 @@ await http.PostAsync( } CleanupPidFile(); - return new DaemonResult(true, $"Daemon stopped (was PID {pid})."); + + if (exitedWithinBudget) + return new DaemonResult(true, $"Daemon stopped (was PID {pid})."); + + if (exitedDuringGraceWindow) + { + // Clean exit — no kill needed — but worth surfacing: the daemon used its full + // graceful-shutdown budget, which usually means a session was still mid-LLM-call + // at shutdown. + return new DaemonResult(true, + $"Daemon stopped (was PID {pid}); exited during the " + + $"{DaemonConfig.CliForceKillGraceWindow.TotalSeconds:F0}s grace window after the " + + $"{DaemonConfig.GracefulShutdownBudget.TotalSeconds:F0}s graceful-wait budget elapsed " + + "(no kill needed)."); + } + + return new DaemonResult(true, + $"Daemon stopped (was PID {pid}), but did not exit gracefully within " + + $"{DaemonConfig.CliForceKillBudget.TotalSeconds:F0}s (budget + grace window) and had to be " + + "force-killed. This usually means a session was still mid-LLM-call at shutdown; if it " + + "recurs, check for stuck sessions before stopping the daemon."); } /// @@ -358,6 +397,13 @@ internal void RemoveDaemonEnvironmentFile() /// The - prefix makes systemd tolerant of a missing env file: a deleted PATH /// file degrades tool resolution (which SystemdUnitPathDoctorCheck flags) /// rather than preventing the entire daemon from starting. + /// + /// TimeoutStopSec= is — comfortably + /// longer than this 's own graceful-wait-plus-grace budget + /// () so systemd never SIGKILLs the whole + /// cgroup out from under ExecStop= (netclaw daemon stop) while that command is + /// still legitimately waiting on the daemon's own shutdown (netclaw-dev/netclaw#1665). + /// Existing installs only pick this up after re-running netclaw daemon install. /// internal static string BuildDaemonUnitContent(string binaryPath, string cliBinaryPath, string environmentFilePath) => $""" [Unit] @@ -368,6 +414,7 @@ internal static string BuildDaemonUnitContent(string binaryPath, string cliBinar Type=simple ExecStart={binaryPath} ExecStop={cliBinaryPath} daemon stop + TimeoutStopSec={(int)DaemonConfig.SystemdTimeoutStopSec.TotalSeconds} Restart=always RestartSec=5 Environment=DOTNET_ENVIRONMENT=Production diff --git a/src/Netclaw.Configuration.Tests/DaemonConfigTests.cs b/src/Netclaw.Configuration.Tests/DaemonConfigTests.cs index 7adb3a7d2..abc2f063f 100644 --- a/src/Netclaw.Configuration.Tests/DaemonConfigTests.cs +++ b/src/Netclaw.Configuration.Tests/DaemonConfigTests.cs @@ -287,4 +287,50 @@ public void Validator_rejects_invalid_trusted_proxy_entry() Assert.Contains(issues, issue => issue.Message.Contains("not-an-ip", StringComparison.OrdinalIgnoreCase)); } + + // ── Shutdown-budget layering (netclaw-dev/netclaw#1664, #1665) ─────────── + // + // Four surfaces derive from DaemonConfig.GracefulShutdownBudget and must stay strictly + // ordered: the daemon-stop drain bound, the Akka before-service-unbind phase timeout, the + // CLI's force-kill budget (graceful wait + grace window), and the generated systemd unit's + // TimeoutStopSec=. If any margin is changed inconsistently in the future — e.g. the CLI's + // grace window grows past the systemd teardown margin — this test fails instead of silently + // reintroducing the guaranteed-SIGKILL race these issues were filed against. + + [Fact] + public void ShutdownBudgetLayering_bounded_drain_finishes_before_the_akka_phase_timeout() + { + Assert.True( + DaemonConfig.BoundedDrainTimeout < DaemonConfig.GracefulShutdownBudget, + "the daemon-stop drain's own deadline must fire before the Akka phase timeout abandons the task outright"); + } + + [Fact] + public void ShutdownBudgetLayering_cli_waits_at_least_as_long_as_the_daemon_phase_timeout() + { + Assert.True( + DaemonConfig.GracefulShutdownBudget <= DaemonConfig.CliForceKillBudget, + "the CLI's graceful-wait budget must be at least the daemon's own Akka phase timeout"); + } + + [Fact] + public void ShutdownBudgetLayering_cli_force_kill_budget_stays_under_systemd_timeout_stop_sec() + { + Assert.True( + DaemonConfig.CliForceKillBudget < DaemonConfig.SystemdTimeoutStopSec, + "systemd's TimeoutStopSec= must exceed the CLI's own budget+grace, or systemd SIGKILLs " + + "the cgroup out from under a `netclaw daemon stop` (ExecStop=) that is still legitimately waiting"); + } + + [Fact] + public void ShutdownBudgetLayering_matches_the_documented_second_values() + { + // Pins the concrete values referenced in netclaw-dev/netclaw#1665's evidence trail + // (200s phase timeout, 230s TimeoutStopSec) so a change to any constant is a visible, + // deliberate diff rather than a silent drift. + Assert.Equal(TimeSpan.FromSeconds(190), DaemonConfig.BoundedDrainTimeout); + Assert.Equal(TimeSpan.FromSeconds(200), DaemonConfig.GracefulShutdownBudget); + Assert.Equal(TimeSpan.FromSeconds(215), DaemonConfig.CliForceKillBudget); + Assert.Equal(TimeSpan.FromSeconds(230), DaemonConfig.SystemdTimeoutStopSec); + } } diff --git a/src/Netclaw.Configuration/DaemonConfig.cs b/src/Netclaw.Configuration/DaemonConfig.cs index 6b881865d..d2d87182a 100644 --- a/src/Netclaw.Configuration/DaemonConfig.cs +++ b/src/Netclaw.Configuration/DaemonConfig.cs @@ -20,6 +20,84 @@ public sealed record DaemonConfig /// public const int DefaultPort = 5199; + /// + /// Worst-case time the daemon's graceful shutdown drain is allotted before something gives + /// up and forces termination. Sized to comfortably exceed SessionConfig.TurnLlmTimeout's + /// default (3 minutes) so a session mid-LLM-call during shutdown can finish draining instead + /// of being interrupted. + /// + /// Single source of truth shared by every shutdown-timing surface that must stay in + /// lockstep (netclaw-dev/netclaw#1664, #1665): + /// - Netclaw.Daemon's Akka coordinated-shutdown.phases.before-service-unbind.timeout + /// HOCON override (see DaemonShutdownConfiguration.BuildCoordinatedShutdownHocon), + /// where SessionDrainHelper.DrainAsync actually drains sessions + /// - , the deadline the daemon-stop CoordinatedShutdown + /// task gives that same drain call — always strictly under this budget, so the drain + /// finishes (timed out or not) before the phase timeout above abandons it outright + /// - Netclaw.Daemon's generic-host HostOptions.ShutdownTimeout + /// - Netclaw.Cli.Daemon.DaemonManager's graceful-wait before force-kill in + /// netclaw daemon stop, followed by + /// - the generated systemd unit's TimeoutStopSec= + /// (; see DaemonManager.BuildDaemonUnitContent) + /// + /// #1664: the SIGTERM/daemon-stop drain previously waited unbounded + /// (CancellationToken.None), so a session parked on interactive tool approval — which + /// cannot ack within any budget — hung the call for the full phase timeout, leaking the + /// abandoned drain task. #1665: the CLI's force-kill wait equaled this exact budget with no + /// headroom, so whenever the drain legitimately used the full budget, the CLI guaranteed a + /// SIGKILL race the daemon could not win (evidence: a production daemon force-killed ~100ms + /// from a clean exit). + /// + public static readonly TimeSpan GracefulShutdownBudget = TimeSpan.FromSeconds(200); + + /// + /// Safety margin subtracted from to produce + /// , so the daemon-stop session drain always finishes — + /// timed out or not — strictly before the Akka before-service-unbind phase timeout + /// itself fires and abandons the drain task. + /// + public static readonly TimeSpan DrainSafetyMargin = TimeSpan.FromSeconds(10); + + /// + /// Additional time netclaw daemon stop polls for process exit after + /// elapses, before escalating to SIGKILL. Covers the + /// daemon's own post-drain teardown (actor system termination, PID file cleanup) so a + /// daemon that finishes right at the budget boundary is not killed out from under itself + /// (netclaw-dev/netclaw#1665). + /// + public static readonly TimeSpan CliForceKillGraceWindow = TimeSpan.FromSeconds(15); + + /// + /// Headroom added on top of for the systemd unit's + /// TimeoutStopSec= (), so systemd never SIGKILLs + /// the whole cgroup out from under ExecStop= (netclaw daemon stop) while that + /// command is still legitimately waiting on the daemon's own bounded shutdown. + /// + public static readonly TimeSpan SystemdTeardownMargin = TimeSpan.FromSeconds(30); + + /// + /// The deadline the daemon-stop CoordinatedShutdown task gives + /// SessionDrainHelper.DrainAsync. Always strictly less than + /// so the drain completes — as timed out if + /// necessary — before the Akka phase timeout fires. + /// + public static TimeSpan BoundedDrainTimeout => GracefulShutdownBudget - DrainSafetyMargin; + + /// + /// Total time netclaw daemon stop allows before escalating to SIGKILL: the + /// graceful wait (matching the daemon's own Akka phase timeout) plus + /// . + /// + public static TimeSpan CliForceKillBudget => GracefulShutdownBudget + CliForceKillGraceWindow; + + /// + /// The systemd unit's TimeoutStopSec= value: comfortably longer than + /// so systemd never SIGKILLs the whole cgroup out from + /// under ExecStop= (netclaw daemon stop) while that command is still + /// legitimately waiting on the daemon's own shutdown. + /// + public static TimeSpan SystemdTimeoutStopSec => GracefulShutdownBudget + SystemdTeardownMargin; + /// /// IP address the daemon binds to. Defaults to loopback (127.0.0.1). /// diff --git a/src/Netclaw.Daemon.Tests/DaemonShutdownConfigurationTests.cs b/src/Netclaw.Daemon.Tests/DaemonShutdownConfigurationTests.cs new file mode 100644 index 000000000..67a9e20b4 --- /dev/null +++ b/src/Netclaw.Daemon.Tests/DaemonShutdownConfigurationTests.cs @@ -0,0 +1,47 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using Akka.Configuration; +using Netclaw.Configuration; +using Xunit; + +namespace Netclaw.Daemon.Tests; + +/// +/// Tests for , which +/// replaced the inline HOCON literal Program.cs used to prepend a hardcoded 200s +/// before-service-unbind phase timeout (netclaw-dev/netclaw#1664, #1665). Proves the emitted +/// HOCON tracks whatever resolves to instead +/// of drifting back to a literal that could disagree with the daemon-stop drain's own bound. +/// +public sealed class DaemonShutdownConfigurationTests +{ + [Theory] + [InlineData(200, 200)] + [InlineData(90, 90)] + public void BuildCoordinatedShutdownHocon_interpolates_the_given_budget_in_seconds( + int budgetSeconds, int expectedPhaseTimeoutSeconds) + { + var hocon = DaemonShutdownConfiguration.BuildCoordinatedShutdownHocon(TimeSpan.FromSeconds(budgetSeconds)); + + var config = ConfigurationFactory.ParseString(hocon); + + Assert.Equal(TimeSpan.FromSeconds(expectedPhaseTimeoutSeconds), + config.GetTimeSpan("akka.coordinated-shutdown.phases.before-service-unbind.timeout")); + Assert.False(config.GetBoolean("akka.coordinated-shutdown.exit-clr")); + } + + [Fact] + public void BuildCoordinatedShutdownHocon_tracks_DaemonConfig_GracefulShutdownBudget() + { + var hocon = DaemonShutdownConfiguration.BuildCoordinatedShutdownHocon(DaemonConfig.GracefulShutdownBudget); + + var config = ConfigurationFactory.ParseString(hocon); + + Assert.Equal( + DaemonConfig.GracefulShutdownBudget, + config.GetTimeSpan("akka.coordinated-shutdown.phases.before-service-unbind.timeout")); + } +} diff --git a/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs b/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs index e2171c0b0..0e548af4f 100644 --- a/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs +++ b/src/Netclaw.Daemon.Tests/Services/DaemonRestartCoordinatorTests.cs @@ -194,6 +194,42 @@ public async Task SessionDrainHelper_propagates_caller_cancellation() await Assert.ThrowsAnyAsync(() => operation); } + [Fact] + public async Task SessionDrainHelper_daemon_stop_bound_times_out_instead_of_hanging_when_a_session_never_acks() + { + // Mirrors the daemon-stop CoordinatedShutdown drain task wired in Program.cs + // (netclaw-dev/netclaw#1664): a session whose in-flight turn is parked on interactive + // tool approval never acks PrepareForDaemonRestart. Previously this call passed + // CancellationToken.None for the operation token and hung until Akka's own 200s + // before-service-unbind phase timeout abandoned the task. The bounded CTS below — + // sized from DaemonConfig.BoundedDrainTimeout (GracefulShutdownBudget minus + // DrainSafetyMargin) and driven by TimeProvider exactly as Program.cs constructs it — + // must make the drain complete with a timed-out result well before that. + var time = new FakeTimeProvider(); + var activeIds = new[] { "slack/approval-parked" }; + var drain = new DrainControl(activeIds, activeIds); // never acknowledged + var sessionManager = _system.ActorOf(Props.Create(() => new StubSessionManagerActor( + activeIds, + drain, + throwOnEnumeration: false))); + using var deadlineCts = new CancellationTokenSource(DaemonConfig.BoundedDrainTimeout, time); + + var operation = SessionDrainHelper.DrainAsync( + sessionManager, + "daemon-stop", + NullLogger.Instance, + deadlineCts.Token, + CancellationToken.None); + await drain.AllRequestsObserved; + time.Advance(DaemonConfig.BoundedDrainTimeout); + + var result = await operation; + + Assert.Single(result.AllSessionIds); + Assert.Empty(result.DrainedSessionIds); + Assert.Equal("slack/approval-parked", Assert.Single(result.TimedOutSessionIds).Value); + } + public async ValueTask DisposeAsync() { await _system.Terminate(); diff --git a/src/Netclaw.Daemon/DaemonShutdownConfiguration.cs b/src/Netclaw.Daemon/DaemonShutdownConfiguration.cs new file mode 100644 index 000000000..55d897e02 --- /dev/null +++ b/src/Netclaw.Daemon/DaemonShutdownConfiguration.cs @@ -0,0 +1,32 @@ +// ----------------------------------------------------------------------- +// +// Copyright (C) 2026 - 2026 Petabridge, LLC +// +// ----------------------------------------------------------------------- +using Netclaw.Configuration; + +namespace Netclaw.Daemon; + +/// +/// Builds the Akka CoordinatedShutdown HOCON override that bounds the daemon's session-drain +/// phase. Extracted into its own testable method (rather than an inline string literal in +/// Program.cs's top-level statements) so a unit test can assert the interpolated timeout +/// tracks instead of drifting back to a +/// hardcoded literal (see remarks for why +/// that drift is the exact class of bug behind netclaw-dev/netclaw#1664 and #1665). +/// +internal static class DaemonShutdownConfiguration +{ + /// + /// Coordinated-shutdown HOCON: disables the CLR-exit side effect (the daemon's own + /// restart loop owns process lifetime, not CoordinatedShutdown) and sizes the + /// before-service-unbind phase — where session draining + /// (SessionDrainHelper.DrainAsync) actually runs — to . + /// + public static string BuildCoordinatedShutdownHocon(TimeSpan gracefulShutdownBudget) => $$""" + akka.coordinated-shutdown { + exit-clr = off + phases.before-service-unbind.timeout = {{(int)gracefulShutdownBudget.TotalSeconds}}s + } + """; +} diff --git a/src/Netclaw.Daemon/Program.cs b/src/Netclaw.Daemon/Program.cs index a81e6eb59..35ccbf7bf 100644 --- a/src/Netclaw.Daemon/Program.cs +++ b/src/Netclaw.Daemon/Program.cs @@ -419,7 +419,15 @@ static void ConfigureDaemonServices( services.Configure(options => { - options.ShutdownTimeout = TimeSpan.FromSeconds(30); + // Generic-host shutdown ceiling for all hosted services combined. Kept in lockstep + // with DaemonConfig.SystemdTimeoutStopSec (= GracefulShutdownBudget + teardown margin) + // rather than a bare, unrelated literal: a value shorter than the Akka + // before-service-unbind phase timeout below would silently reintroduce the same class + // of mismatch behind netclaw-dev/netclaw#1665 (budgets that look independent but must + // stay ordered). AkkaHostedService.StopAsync does not observe this cancellation token + // and awaits CoordinatedShutdown.Run to its own natural completion, so this value does + // not itself truncate the drain — it only keeps the documented layering consistent. + options.ShutdownTimeout = DaemonConfig.SystemdTimeoutStopSec; }); // Resolve models for session config @@ -990,16 +998,13 @@ static void ConfigureDaemonServices( { // Prevent coordinated shutdown from calling Environment.Exit(), // which would kill the process before the restart loop can iterate. - // The before-service-unbind phase needs a generous timeout because sessions - // mid-LLM-call (TurnLlmTimeout defaults to 3 minutes) must finish before - // passivation can begin. + // The before-service-unbind phase needs a generous timeout (DaemonConfig. + // GracefulShutdownBudget) because sessions mid-LLM-call (TurnLlmTimeout defaults to + // 3 minutes) must finish before passivation can begin. See DaemonConfig. + // GracefulShutdownBudget remarks for the full set of surfaces this must stay in + // lockstep with. akkaBuilder.AddHocon( - """ - akka.coordinated-shutdown { - exit-clr = off - phases.before-service-unbind.timeout = 200s - } - """, + DaemonShutdownConfiguration.BuildCoordinatedShutdownHocon(DaemonConfig.GracefulShutdownBudget), HoconAddMode.Prepend); akkaBuilder = akkaBuilder.ConfigureLoggers(setup => @@ -1055,8 +1060,8 @@ static void ConfigureDaemonServices( // Runs in an early CoordinatedShutdown phase while actors are still alive. // If DaemonRestartCoordinator already drained sessions (config reload), the ingress // gate will be closed and this task skips its drain to avoid double-draining. - // The phase timeout (200s) is generous because sessions mid-LLM-call must finish - // before passivation can begin. + // The phase timeout (DaemonConfig.GracefulShutdownBudget) is generous because + // sessions mid-LLM-call must finish before passivation can begin. var cs = CoordinatedShutdown.Get(system); var sessionManager = registry.Get(); var ingressGate = sp.GetRequiredService(); @@ -1073,11 +1078,20 @@ static void ConfigureDaemonServices( try { + // Bounded strictly under DaemonConfig.GracefulShutdownBudget (the Akka phase + // timeout above) so the drain always completes -- timed out or not -- before + // the phase timeout itself fires and abandons this task outright. + // netclaw-dev/netclaw#1664: a session parked on interactive tool approval + // never acks PrepareForDaemonRestart, so an unbounded wait here (previously + // CancellationToken.None, CancellationToken.None) hung for the full 200s + // phase timeout with no timeout of its own, leaking the abandoned drain task. + using var drainDeadlineCts = new CancellationTokenSource(DaemonConfig.BoundedDrainTimeout, tp); + var drainResult = await SessionDrainHelper.DrainAsync( sessionManager, "daemon-stop", drainLogger, - CancellationToken.None, + drainDeadlineCts.Token, CancellationToken.None); lifecycleNotifier.NotifyShutdown("daemon-stop", drainResult.ToNotificationContext()); diff --git a/src/Netclaw.Daemon/Services/SessionDrainHelper.cs b/src/Netclaw.Daemon/Services/SessionDrainHelper.cs index 14481c893..b9dbdc6b0 100644 --- a/src/Netclaw.Daemon/Services/SessionDrainHelper.cs +++ b/src/Netclaw.Daemon/Services/SessionDrainHelper.cs @@ -100,9 +100,10 @@ public static async Task DrainAsync( else { logger.LogWarning( - "Drain completed with {DrainedCount} session(s) drained and {TimedOutCount} timed out; timed-out sessions will recover from the last durable checkpoint.", + "Drain completed with {DrainedCount} session(s) drained and {TimedOutCount} timed out ({TimedOutSessionIds}); timed-out sessions will recover from the last durable checkpoint.", drained.Length, - timedOut.Length); + timedOut.Length, + string.Join(", ", timedOut.Select(static id => id.Value))); } return new DrainResult(sessionIds, drained, timedOut);