From 1b2ebbd9531ba96573f38ab9c84103f75a5ffa04 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 8 Aug 2026 21:34:53 +0000 Subject: [PATCH 1/8] fix(jobs): sweep terminal background jobs past retention window (#1820) Background job definitions and output logs were never deleted: the BackgroundJobDefinitionStore.Delete method was dead code, so every submitted job (shell_execute + _background:true) left a {jobId}.json and output.log behind forever. This is the background-job half of #1820. Add a terminal-job cleanup sweep: - Terminal states (Completed/Failed/Cancelled/TimedOut/Lost/Reaped) whose CompletedAtMs is older than TerminalJobRetentionWindow (24h) have their definition AND output-log directory deleted. - The window doubles as the delivery grace period: the completed-result message references the output-log path, and a slow polling session can still check_background_job within the window. - Sweep runs once at startup reconciliation and on a self-scheduled hourly timer (canceled on PostStop), so the store stops growing during long-lived daemon runs. - Active jobs are never touched (status guard + in-memory active set). Store: add BackgroundJobDefinitionStore.DeleteJobArtifacts (definition json + output directory, idempotent). Tests: DeleteJobArtifacts removes both artifacts and is idempotent; manager sweep deletes past-window jobs (definition + log), keeps within-window jobs, and never touches Running/Pending jobs. --- .../Jobs/BackgroundJobDefinitionStoreTests.cs | 53 +++++++++ .../Jobs/BackgroundJobManagerActorTests.cs | 104 +++++++++++++++++- .../Jobs/BackgroundJobDefinitionStore.cs | 31 +++++- .../Jobs/BackgroundJobManagerActor.cs | 100 ++++++++++++++++- 4 files changed, 284 insertions(+), 4 deletions(-) diff --git a/src/Netclaw.Actors.Tests/Jobs/BackgroundJobDefinitionStoreTests.cs b/src/Netclaw.Actors.Tests/Jobs/BackgroundJobDefinitionStoreTests.cs index 9a209c2d0..5fb00160b 100644 --- a/src/Netclaw.Actors.Tests/Jobs/BackgroundJobDefinitionStoreTests.cs +++ b/src/Netclaw.Actors.Tests/Jobs/BackgroundJobDefinitionStoreTests.cs @@ -97,6 +97,59 @@ public void Current_job_with_trust_fields_roundtrips_exact_values() Assert.Equal("C0ABC/1712000000.000001", loaded.SessionId.Value); } + /// + /// Terminal-job cleanup: + /// removes BOTH the definition file and the job's output-log directory, so the + /// store cannot grow without bound after a job's retention window elapses. + /// + [Fact] + public void DeleteJobArtifacts_removes_definition_and_output_directory() + { + var store = new BackgroundJobDefinitionStore(_paths); + var jobId = new BackgroundJobId("cleanup-job-001"); + + store.Save(new BackgroundJobDefinition + { + Id = jobId, + Command = "dotnet test", + SessionId = new Netclaw.Actors.Protocol.SessionId("C0ABC/1712000000.000001"), + Rationale = "Run the test suite.", + Status = BackgroundJobStatus.Completed, + TimeoutSeconds = 300, + Audience = TrustAudience.Team, + Boundary = TrustBoundary.Team, + OriginChannelType = Netclaw.Actors.Channels.ChannelType.Slack + }); + + // Simulate a real job's output log on disk. + var outputLogPath = store.GetOutputLogPath(jobId); + File.WriteAllText(outputLogPath, "build output"); + + Assert.NotNull(store.Get(jobId)); + Assert.True(File.Exists(outputLogPath)); + + var removed = store.DeleteJobArtifacts(jobId); + + Assert.True(removed); + Assert.Null(store.Get(jobId)); + Assert.False(File.Exists(outputLogPath)); + Assert.False(Directory.Exists(Path.GetDirectoryName(outputLogPath))); + } + + /// + /// Idempotent cleanup: deleting artifacts for an already-removed job reports + /// false and does not throw. + /// + [Fact] + public void DeleteJobArtifacts_missing_job_returns_false_without_throwing() + { + var store = new BackgroundJobDefinitionStore(_paths); + + var removed = store.DeleteJobArtifacts(new BackgroundJobId("never-existed")); + + Assert.False(removed); + } + /// /// Byte-equality gate for issue #994 Pass 7b. Wrapping BackgroundJobDefinition.Id /// in and SessionId in diff --git a/src/Netclaw.Actors.Tests/Jobs/BackgroundJobManagerActorTests.cs b/src/Netclaw.Actors.Tests/Jobs/BackgroundJobManagerActorTests.cs index 05e3fe114..cf37c1b96 100644 --- a/src/Netclaw.Actors.Tests/Jobs/BackgroundJobManagerActorTests.cs +++ b/src/Netclaw.Actors.Tests/Jobs/BackgroundJobManagerActorTests.cs @@ -49,6 +49,21 @@ protected override async Task AfterAllAsync() private IActorRef GetManager() => ActorRegistry.For(Sys).Get(); + private BackgroundJobDefinition MakeTerminalDefinition(string jobId, BackgroundJobStatus status, long completedAtMs) => new() + { + Id = new BackgroundJobId(jobId), + Command = "echo hello", + SessionId = new SessionId("test/thread"), + Rationale = "test run", + Status = status, + StartedAtMs = completedAtMs - 60_000, + CompletedAtMs = completedAtMs, + TimeoutSeconds = 60, + Audience = TrustAudience.Personal, + Boundary = TrustBoundary.Personal, + OriginChannelType = ChannelType.Tui + }; + private StartBackgroundJob MakeStartCommand(string command = "echo hello") => new() { Command = command, @@ -309,8 +324,7 @@ await manager.Ask( [Fact] public async Task StartupReconciliation_EmitsAlert_ForLegacyJobMissingTrustFields() - { - var paths = new NetclawPaths(_dir.Path); + { var paths = new NetclawPaths(_dir.Path); paths.EnsureDirectoriesExist(); const string jobId = "legacy-job-alert"; @@ -364,4 +378,90 @@ public void Emit(OperationalAlert alert) _alerts.Add(alert); } } + + [Fact] + public async Task TerminalSweep_DeletesJobPastRetentionWindow() + { + var manager = GetManager(); + var pastWindowMs = TimeProvider.System.GetUtcNow() + .Subtract(BackgroundJobManagerActor.TerminalJobRetentionWindow) + .Subtract(TimeSpan.FromMinutes(1)) + .ToUnixTimeMilliseconds(); + + _store.Save(MakeTerminalDefinition("sweep-past", BackgroundJobStatus.Completed, pastWindowMs)); + + await manager.Ask( + GetBackgroundJobManagerHealth.Instance, + TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken); + manager.Tell(BackgroundJobManagerActor.SweepTerminalJobs.Instance); + await manager.Ask( + GetBackgroundJobManagerHealth.Instance, + TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken); + + Assert.Null(_store.Get(new BackgroundJobId("sweep-past"))); + } + + [Fact] + public async Task TerminalSweep_KeepsJobWithinRetentionWindow() + { + var manager = GetManager(); + var recentMs = TimeProvider.System.GetUtcNow().ToUnixTimeMilliseconds(); + + _store.Save(MakeTerminalDefinition("sweep-recent", BackgroundJobStatus.Completed, recentMs)); + + manager.Tell(BackgroundJobManagerActor.SweepTerminalJobs.Instance); + await manager.Ask( + GetBackgroundJobManagerHealth.Instance, + TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken); + + Assert.NotNull(_store.Get(new BackgroundJobId("sweep-recent"))); + } + + [Fact] + public async Task TerminalSweep_DoesNotTouchNonTerminalJobs() + { + var manager = GetManager(); + var pastWindowMs = TimeProvider.System.GetUtcNow() + .Subtract(BackgroundJobManagerActor.TerminalJobRetentionWindow) + .Subtract(TimeSpan.FromMinutes(1)) + .ToUnixTimeMilliseconds(); + + // A Running job with a stale CompletedAtMs must never be swept — the + // status guard is independent of the timestamp. + _store.Save(MakeTerminalDefinition("sweep-running", BackgroundJobStatus.Running, pastWindowMs)); + _store.Save(MakeTerminalDefinition("sweep-pending", BackgroundJobStatus.Pending, pastWindowMs)); + + manager.Tell(BackgroundJobManagerActor.SweepTerminalJobs.Instance); + await manager.Ask( + GetBackgroundJobManagerHealth.Instance, + TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken); + + Assert.NotNull(_store.Get(new BackgroundJobId("sweep-running"))); + Assert.NotNull(_store.Get(new BackgroundJobId("sweep-pending"))); + } + + [Fact] + public async Task TerminalSweep_DeletesOutputLogWithDefinition() + { + var manager = GetManager(); + var pastWindowMs = TimeProvider.System.GetUtcNow() + .Subtract(BackgroundJobManagerActor.TerminalJobRetentionWindow) + .Subtract(TimeSpan.FromMinutes(1)) + .ToUnixTimeMilliseconds(); + var jobId = new BackgroundJobId("sweep-log"); + + var outputLogPath = _store.GetOutputLogPath(jobId); + File.WriteAllText(outputLogPath, "some output"); + + _store.Save(MakeTerminalDefinition("sweep-log", BackgroundJobStatus.Failed, pastWindowMs)); + + manager.Tell(BackgroundJobManagerActor.SweepTerminalJobs.Instance); + await manager.Ask( + GetBackgroundJobManagerHealth.Instance, + TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken); + + Assert.Null(_store.Get(jobId)); + Assert.False(File.Exists(outputLogPath)); + Assert.False(Directory.Exists(Path.GetDirectoryName(outputLogPath))); + } } diff --git a/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs b/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs index 06fa6026e..a20346f86 100644 --- a/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs +++ b/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs @@ -1,4 +1,4 @@ -// ----------------------------------------------------------------------- +// ----------------------------------------------------------------------- // // Copyright (C) 2026 - 2026 Petabridge, LLC // @@ -145,6 +145,35 @@ public bool Delete(BackgroundJobId id) } } + /// + /// Deletes the job definition file AND its output-log directory. Used by + /// terminal-job cleanup once a job's retention window has elapsed. Returns + /// true when anything was removed. + /// + public bool DeleteJobArtifacts(BackgroundJobId id) + { + lock (_sync) + { + var removed = false; + + var path = GetPath(id); + if (File.Exists(path)) + { + File.Delete(path); + removed = true; + } + + var outputDir = Path.Combine(_directory, Uri.EscapeDataString(id.Value)); + if (Directory.Exists(outputDir)) + { + Directory.Delete(outputDir, recursive: true); + removed = true; + } + + return removed; + } + } + /// /// Returns the output log directory for a job, creating it if needed. /// diff --git a/src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs b/src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs index 29e4a8018..e7d172a14 100644 --- a/src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs +++ b/src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs @@ -1,4 +1,4 @@ -// ----------------------------------------------------------------------- +// ----------------------------------------------------------------------- // // Copyright (C) 2026 - 2026 Petabridge, LLC // @@ -26,6 +26,22 @@ public sealed class BackgroundJobManagerActor : ReceiveActor internal const int MaxConcurrentJobs = 5; internal const int MaxOutputTailChars = 2000; + /// + /// How long a terminal job's definition and output log are retained after + /// completion before the cleanup sweep deletes them. The delivered result + /// message references the full output-log path, so the grace period must be + /// long enough for the owning session to poll check_background_job or + /// read the log after delivery. See . + /// + internal static TimeSpan TerminalJobRetentionWindow = TimeSpan.FromHours(24); + + /// + /// Cadence of the periodic terminal-job cleanup sweep. The sweep also runs + /// once during startup reconciliation, so a daemon restart immediately + /// purges anything past the retention window. + /// + internal static TimeSpan TerminalSweepInterval = TimeSpan.FromHours(1); + // Capture ceiling for a job's output log: the execution actor drains each // stream to this bound (head+tail) so a chatty long-running job can't buffer // its full output in memory and OOM the daemon. The log holds a head+tail view @@ -45,6 +61,7 @@ public sealed class BackgroundJobManagerActor : ReceiveActor private readonly HashSet _activeJobIds = []; private readonly Queue _deferredQueue = new(); private readonly Dictionary _definitions = []; + private ICancelable? _sweepCancelable; public BackgroundJobManagerActor( BackgroundJobDefinitionStore store, @@ -62,6 +79,7 @@ public BackgroundJobManagerActor( Receive(HandleQuery); Receive(HandleKillJobsForSession); Receive(_ => HandleGetHealth()); + Receive(_ => HandleSweepTerminalJobs()); } protected override void PreStart() @@ -72,6 +90,22 @@ protected override void PreStart() // (macOS CI): ActorOf returns before PreStart executes, allowing external // messages to queue ahead of the Reconcile message. HandleReconcile(); + + // Periodic cleanup for terminal jobs past their retention window. The + // startup reconcile also sweeps, so a restart purges immediately; this + // timer keeps the store from growing during a long-lived daemon run. + _sweepCancelable = Context.System.Scheduler.ScheduleTellRepeatedlyCancelable( + TerminalSweepInterval, + TerminalSweepInterval, + Self, + SweepTerminalJobs.Instance, + Self); + } + + protected override void PostStop() + { + _sweepCancelable?.Cancel(); + _sweepCancelable = null; } private async Task HandleStartAsync(StartBackgroundJob cmd) @@ -322,8 +356,65 @@ private void HandleReconcile() if (reconciled > 0) _log.Info("Background job startup reconciliation: marked {0} orphaned job(s) as lost", reconciled); + + // Startup sweep: purge terminal jobs whose retention window has elapsed, + // in the same pass as the orphan reconciliation. + HandleSweepTerminalJobs(); + } + + /// + /// Deletes definitions (and output logs) for terminal jobs whose + /// CompletedAtMs is older than . + /// Active jobs are never touched. Terminal jobs stay queryable via + /// check_background_job during the window — the delivered result + /// message references the output-log path, and a slow polling session may + /// still read it — then get swept so the store cannot grow without bound. + /// + private void HandleSweepTerminalJobs() + { + var swept = 0; + var cutoffMs = _timeProvider.GetUtcNow() + .Subtract(TerminalJobRetentionWindow) + .ToUnixTimeMilliseconds(); + + foreach (var def in _store.List()) + { + if (!IsTerminalStatus(def.Status)) + continue; + + // Belt-and-braces: never sweep a job this actor still considers + // active, even if the on-disk snapshot is stale. + if (_activeJobIds.Contains(def.Id.Value)) + continue; + + if (def.CompletedAtMs is null || def.CompletedAtMs.Value > cutoffMs) + continue; + + if (_store.DeleteJobArtifacts(def.Id)) + { + _definitions.Remove(def.Id.Value); + swept++; + _log.Info( + "Swept terminal background job {JobId} (status={Status}, completed_at={CompletedAtMs}) past retention window", + def.Id, def.Status, def.CompletedAtMs); + } + } + + if (swept > 0) + _log.Info("Background job terminal sweep: deleted {0} job(s) past the retention window", swept); } + private static bool IsTerminalStatus(BackgroundJobStatus status) => status switch + { + BackgroundJobStatus.Completed or + BackgroundJobStatus.Failed or + BackgroundJobStatus.Cancelled or + BackgroundJobStatus.TimedOut or + BackgroundJobStatus.Lost or + BackgroundJobStatus.Reaped => true, + _ => false + }; + private void NotifyLostJob(BackgroundJobDefinition lost, long nowMs) { var outputFilePath = _store.GetOutputLogPathOnly(lost.Id); @@ -497,4 +588,11 @@ private static string BuildResultContent(BackgroundJobCompleted completed, Backg output + filePath; } + /// + /// Internal self-message: periodic terminal-job cleanup sweep. + /// + internal sealed record SweepTerminalJobs : INoSerializationVerificationNeeded + { + public static readonly SweepTerminalJobs Instance = new(); + } } From d81c55b4019c7eea7eb46b3d4970020e66eee51a Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 8 Aug 2026 21:38:05 +0000 Subject: [PATCH 2/8] refactor(jobs): use IWithTimers for terminal sweep timer Replace the manual Context.System.Scheduler.ScheduleTellRepeatedlyCancelable + ICancelable bookkeeping with the actor's injected ITimerScheduler. Timers cancel automatically on stop, so the PostStop override goes away entirely. --- .../Jobs/BackgroundJobManagerActor.cs | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs b/src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs index e7d172a14..2939a087b 100644 --- a/src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs +++ b/src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs @@ -21,7 +21,7 @@ namespace Netclaw.Actors.Jobs; /// Infrastructure singleton that manages background job lifecycle independently /// of any session. Follows the same pattern as ReminderManagerActor. /// -public sealed class BackgroundJobManagerActor : ReceiveActor +public sealed class BackgroundJobManagerActor : ReceiveActor, IWithTimers { internal const int MaxConcurrentJobs = 5; internal const int MaxOutputTailChars = 2000; @@ -61,7 +61,17 @@ public sealed class BackgroundJobManagerActor : ReceiveActor private readonly HashSet _activeJobIds = []; private readonly Queue _deferredQueue = new(); private readonly Dictionary _definitions = []; - private ICancelable? _sweepCancelable; + + /// + /// Timer identity for the periodic terminal-job sweep. + /// + private static readonly object TerminalSweepTimerKey = new(); + + /// + /// Timer scheduler injected by Akka for . Timers + /// are canceled automatically when the actor stops. + /// + public ITimerScheduler Timers { get; set; } = null!; public BackgroundJobManagerActor( BackgroundJobDefinitionStore store, @@ -94,18 +104,8 @@ protected override void PreStart() // Periodic cleanup for terminal jobs past their retention window. The // startup reconcile also sweeps, so a restart purges immediately; this // timer keeps the store from growing during a long-lived daemon run. - _sweepCancelable = Context.System.Scheduler.ScheduleTellRepeatedlyCancelable( - TerminalSweepInterval, - TerminalSweepInterval, - Self, - SweepTerminalJobs.Instance, - Self); - } - - protected override void PostStop() - { - _sweepCancelable?.Cancel(); - _sweepCancelable = null; + // IWithTimers cancels the timer automatically when the actor stops. + Timers.StartPeriodicTimer(TerminalSweepTimerKey, SweepTerminalJobs.Instance, TerminalSweepInterval); } private async Task HandleStartAsync(StartBackgroundJob cmd) From 39881c2dd6bc59035b039fe82cc80ae3acf52008 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 8 Aug 2026 21:51:52 +0000 Subject: [PATCH 3/8] fix(jobs): address code-quality review on PR #1821 - Mark TerminalJobRetentionWindow / TerminalSweepInterval static readonly (review: missed readonly opportunity). - DeleteJobArtifacts: derive the output directory from the canonical GetOutputLogPathOnly path instead of a second Path.Combine, so the artifact directory always matches what the execution actor wrote (review: Path.Combine may drop earlier arguments). --- src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs | 8 ++++++-- src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs | 4 ++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs b/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs index a20346f86..1944c287e 100644 --- a/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs +++ b/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs @@ -163,8 +163,12 @@ public bool DeleteJobArtifacts(BackgroundJobId id) removed = true; } - var outputDir = Path.Combine(_directory, Uri.EscapeDataString(id.Value)); - if (Directory.Exists(outputDir)) + // Reuse the canonical output-log path (same encoding as + // GetOutputLogPathOnly) so the artifact directory matches exactly + // what the execution actor wrote. + var outputLogPath = GetOutputLogPathOnly(id); + var outputDir = Path.GetDirectoryName(outputLogPath); + if (outputDir is not null && Directory.Exists(outputDir)) { Directory.Delete(outputDir, recursive: true); removed = true; diff --git a/src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs b/src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs index 2939a087b..180ed3ae2 100644 --- a/src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs +++ b/src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs @@ -33,14 +33,14 @@ public sealed class BackgroundJobManagerActor : ReceiveActor, IWithTimers /// long enough for the owning session to poll check_background_job or /// read the log after delivery. See . /// - internal static TimeSpan TerminalJobRetentionWindow = TimeSpan.FromHours(24); + internal static readonly TimeSpan TerminalJobRetentionWindow = TimeSpan.FromHours(24); /// /// Cadence of the periodic terminal-job cleanup sweep. The sweep also runs /// once during startup reconciliation, so a daemon restart immediately /// purges anything past the retention window. /// - internal static TimeSpan TerminalSweepInterval = TimeSpan.FromHours(1); + internal static readonly TimeSpan TerminalSweepInterval = TimeSpan.FromHours(1); // Capture ceiling for a job's output log: the execution actor drains each // stream to this bound (head+tail) so a chatty long-running job can't buffer From 69f1b28495067f30924ffdee606e506a60cea67c Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 8 Aug 2026 22:06:30 +0000 Subject: [PATCH 4/8] fix(jobs): harden sweep against traversal ids and null completion times Adversarial review findings on PR #1821: HIGH-1 (directory traversal): Uri.EscapeDataString does not escape dots, so a persisted definition with id "." or ".." resolved DeleteJobArtifacts to the jobs directory or its parent and Directory.Delete(recursive) would wipe it. Two layers of defense: - Deserialize rejects unsafe ids (dot-only, path separators) so they never load into List()/Get(). - DeleteJobArtifacts canonicalizes the output dir and refuses to touch anything outside GetFullPath(jobs directory). MED-1 (permanent leak): a terminal-status definition with null CompletedAtMs was never swept. Fall back to StartedAtMs (always set at submission) so that class still gets cleaned up; a recent StartedAtMs keeps it in the retention window. Tests: dot-only ids rejected at load (Theory . / ..); DeleteJobArtifacts with id ".." leaves the parent untouched; null-CompletedAtMs terminal swept via StartedAt fallback; null-CompletedAtMs with recent StartedAt kept. 44 background-job tests green; slopwatch 0; headers clean. --- .../Jobs/BackgroundJobDefinitionStoreTests.cs | 63 +++++++++++++++++++ .../Jobs/BackgroundJobManagerActorTests.cs | 44 +++++++++++++ .../Jobs/BackgroundJobDefinitionStore.cs | 57 +++++++++++++++-- .../Jobs/BackgroundJobManagerActor.cs | 8 ++- 4 files changed, 167 insertions(+), 5 deletions(-) diff --git a/src/Netclaw.Actors.Tests/Jobs/BackgroundJobDefinitionStoreTests.cs b/src/Netclaw.Actors.Tests/Jobs/BackgroundJobDefinitionStoreTests.cs index 5fb00160b..3c6cfd4ce 100644 --- a/src/Netclaw.Actors.Tests/Jobs/BackgroundJobDefinitionStoreTests.cs +++ b/src/Netclaw.Actors.Tests/Jobs/BackgroundJobDefinitionStoreTests.cs @@ -150,6 +150,69 @@ public void DeleteJobArtifacts_missing_job_returns_false_without_throwing() Assert.False(removed); } + /// + /// Traversal guard (adversarial review, HIGH): Uri.EscapeDataString does not + /// escape dots, so a dot-only id must NOT resolve to the jobs directory's + /// parent and delete it recursively. The store rejects such ids at + /// deserialization, and DeleteJobArtifacts contains the delete to the jobs + /// directory as belt-and-braces. + /// + [Fact] + public void DeleteJobArtifacts_dot_only_id_cannot_escape_jobs_directory() + { + var store = new BackgroundJobDefinitionStore(_paths); + var jobsDir = _paths.JobsDirectory; + var parentDir = Path.GetDirectoryName(jobsDir.TrimEnd(Path.DirectorySeparatorChar))!; + var sentinel = Path.Combine(parentDir, "sentinel-file.txt"); + File.WriteAllText(sentinel, "do not delete"); + + // ".." would resolve to the parent of the jobs directory. + var removed = store.DeleteJobArtifacts(new BackgroundJobId("..")); + + // The delete must be refused (nothing removed) — the parent survives. + Assert.False(removed); + Assert.True(File.Exists(sentinel)); + Assert.True(Directory.Exists(jobsDir)); + + File.Delete(sentinel); + } + + /// + /// Traversal guard (adversarial review, HIGH): a persisted definition whose + /// id is "." or ".." must be rejected at load — it never appears in + /// List(), so the sweep can never act on it. + /// + [Theory] + [InlineData(".")] + [InlineData("..")] + public void Definition_with_dot_only_id_is_rejected_at_load(string unsafeId) + { + var logger = new CapturingJobLogger(); + var store = new BackgroundJobDefinitionStore(_paths, logger); + + // File name mirrors what GetPath would produce for this id: + // Uri.EscapeDataString leaves dots unescaped, so the file is "..json". + var filePath = Path.Combine(_paths.JobsDirectory, $"{Uri.EscapeDataString(unsafeId)}.json"); + File.WriteAllText(filePath, $$""" + { + "id": "{{unsafeId}}", + "command": "echo pwn", + "sessionId": "C0TEST/1712000000.000001", + "rationale": "test", + "status": "Completed", + "timeoutSeconds": 600, + "startedAtMs": 0, + "completedAtMs": 0, + "audience": "Personal", + "boundary": "Personal" + } + """); + + Assert.Empty(store.List()); + Assert.Null(store.Get(new BackgroundJobId(unsafeId))); + Assert.Contains(logger.Errors, e => e.Contains("unsafe id")); + } + /// /// Byte-equality gate for issue #994 Pass 7b. Wrapping BackgroundJobDefinition.Id /// in and SessionId in diff --git a/src/Netclaw.Actors.Tests/Jobs/BackgroundJobManagerActorTests.cs b/src/Netclaw.Actors.Tests/Jobs/BackgroundJobManagerActorTests.cs index cf37c1b96..a48781277 100644 --- a/src/Netclaw.Actors.Tests/Jobs/BackgroundJobManagerActorTests.cs +++ b/src/Netclaw.Actors.Tests/Jobs/BackgroundJobManagerActorTests.cs @@ -464,4 +464,48 @@ await manager.Ask( Assert.False(File.Exists(outputLogPath)); Assert.False(Directory.Exists(Path.GetDirectoryName(outputLogPath))); } + + [Fact] + public async Task TerminalSweep_SweepsTerminalJobWithNullCompletedAtMs_UsingStartedAtFallback() + { + // Adversarial review, MED: a terminal-status definition with null + // CompletedAtMs must still be swept once its StartedAtMs is past the + // window — otherwise it leaks forever (the exact bug this PR fixes). + var manager = GetManager(); + var pastWindowMs = TimeProvider.System.GetUtcNow() + .Subtract(BackgroundJobManagerActor.TerminalJobRetentionWindow) + .Subtract(TimeSpan.FromMinutes(1)) + .ToUnixTimeMilliseconds(); + + var def = MakeTerminalDefinition("sweep-null-completed", BackgroundJobStatus.Failed, pastWindowMs); + def = def with { CompletedAtMs = null }; + _store.Save(def); + + manager.Tell(BackgroundJobManagerActor.SweepTerminalJobs.Instance); + await manager.Ask( + GetBackgroundJobManagerHealth.Instance, + TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken); + + Assert.Null(_store.Get(new BackgroundJobId("sweep-null-completed"))); + } + + [Fact] + public async Task TerminalSweep_KeepsTerminalJobWithNullCompletedAtMs_WhenStartedAtRecent() + { + // A terminal job with null CompletedAtMs but a RECENT StartedAtMs must + // survive the sweep — the StartedAt fallback must not over-delete. + var manager = GetManager(); + var recentMs = TimeProvider.System.GetUtcNow().ToUnixTimeMilliseconds(); + + var def = MakeTerminalDefinition("sweep-null-recent", BackgroundJobStatus.Failed, recentMs); + def = def with { CompletedAtMs = null }; + _store.Save(def); + + manager.Tell(BackgroundJobManagerActor.SweepTerminalJobs.Instance); + await manager.Ask( + GetBackgroundJobManagerHealth.Instance, + TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken); + + Assert.NotNull(_store.Get(new BackgroundJobId("sweep-null-recent"))); + } } diff --git a/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs b/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs index 1944c287e..a543dd8f6 100644 --- a/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs +++ b/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs @@ -54,7 +54,44 @@ public BackgroundJobDefinitionStore(NetclawPaths paths, ILogger(text, JsonOptions); + var definition = JsonSerializer.Deserialize(text, JsonOptions); + if (definition is null) + return null; + + // Reject ids that resolve to special directory entries ("." / "..") — + // Uri.EscapeDataString does NOT escape dots, so such an id would make + // DeleteJobArtifacts target the jobs directory itself or its parent + // (see DeleteJobArtifacts containment check). The manager only ever + // generates 12-hex ids; anything else on disk is corrupt or hostile. + if (!IsSafeJobId(definition.Id.Value)) + { + _logger.LogError( + "Background job document {Path} has an unsafe id '{JobId}'. The job will not be loaded.", + path, definition.Id.Value); + return null; + } + + return definition; + } + + /// + /// True when the id is a plain file-name token that cannot traverse out of + /// the jobs directory. Dots pass through Uri.EscapeDataString + /// unescaped, so "." / ".." (and any id containing a path separator) are + /// rejected outright. + /// + private static bool IsSafeJobId(string id) + { + if (string.IsNullOrWhiteSpace(id)) + return false; + + if (id is "." or "..") + return false; + + if (id.IndexOfAny(['/', '\\']) >= 0) + return false; + + return true; } /// @@ -168,10 +205,22 @@ public bool DeleteJobArtifacts(BackgroundJobId id) // what the execution actor wrote. var outputLogPath = GetOutputLogPathOnly(id); var outputDir = Path.GetDirectoryName(outputLogPath); - if (outputDir is not null && Directory.Exists(outputDir)) + if (outputDir is not null) { - Directory.Delete(outputDir, recursive: true); - removed = true; + // Containment guard: Uri.EscapeDataString does not escape dots, + // so an id like ".." would otherwise resolve to the jobs + // directory's parent and Directory.Delete(recursive) would wipe + // it. Never touch anything outside the jobs directory. + var root = Path.GetFullPath(_directory); + var fullDir = Path.GetFullPath(outputDir); + var prefix = root.EndsWith(Path.DirectorySeparatorChar) + ? root + : root + Path.DirectorySeparatorChar; + if (fullDir.StartsWith(prefix, StringComparison.Ordinal) && Directory.Exists(fullDir)) + { + Directory.Delete(fullDir, recursive: true); + removed = true; + } } return removed; diff --git a/src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs b/src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs index 180ed3ae2..1393ddfb0 100644 --- a/src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs +++ b/src/Netclaw.Actors/Jobs/BackgroundJobManagerActor.cs @@ -387,7 +387,13 @@ private void HandleSweepTerminalJobs() if (_activeJobIds.Contains(def.Id.Value)) continue; - if (def.CompletedAtMs is null || def.CompletedAtMs.Value > cutoffMs) + // Terminal jobs always carry CompletedAtMs from the manager's own + // transitions, but a corrupt/legacy file can have terminal status + // with a null completion time — fall back to StartedAtMs so that + // class still gets swept instead of leaking forever. StartedAtMs is + // always present (set at submission), so the fallback is total. + var completedAtMs = def.CompletedAtMs ?? def.StartedAtMs; + if (completedAtMs > cutoffMs) continue; if (_store.DeleteJobArtifacts(def.Id)) From 6580d03e36e3b068d62ed07f6555d54635201ca1 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sat, 8 Aug 2026 22:53:25 +0000 Subject: [PATCH 5/8] fix(jobs): harden terminal artifact cleanup --- docs/runbooks/background-jobs.md | 16 +++ .../.system/files/netclaw-operations/SKILL.md | 3 +- .../references/scheduling.md | 5 +- .../Jobs/BackgroundJobDefinitionStoreTests.cs | 69 +++++++++++ .../Jobs/BackgroundJobManagerActorTests.cs | 109 +++++++++++------- .../Jobs/BackgroundJobDefinitionStore.cs | 36 ++++-- .../Jobs/BackgroundJobManagerActor.cs | 41 +++++-- 7 files changed, 215 insertions(+), 64 deletions(-) diff --git a/docs/runbooks/background-jobs.md b/docs/runbooks/background-jobs.md index 623a76ac9..14c8ac3f5 100644 --- a/docs/runbooks/background-jobs.md +++ b/docs/runbooks/background-jobs.md @@ -82,6 +82,19 @@ is notified** with the log path so the agent can relaunch. Notification volume is bounded by design: passivated sessions have no live jobs, so only sessions that were warm at crash time appear here. +### Terminal artifact retention + +The manager retains each terminal definition and its output directory for 24 +hours after completion. The policy applies to `Completed`, `Failed`, +`Cancelled`, `TimedOut`, `Lost`, and `Reaped` jobs. + +The manager starts an hourly cleanup sweep after startup reconciliation. The +sweep deletes the output directory before it deletes the definition. A file +error keeps the definition and lets the next sweep retry the cleanup. + +A terminal definition without `CompletedAtMs` is corrupt. The sweep reports +the problem and retains its artifacts instead of inferring a deletion time. + ## Monitoring ### Active jobs in context @@ -111,6 +124,9 @@ Returns: status, elapsed time, rationale, exit code (if finished), and the live output tail (last 2000 chars, read from the streaming log). Only accessible from the same session/audience/boundary that submitted the job. +The tool can query a terminal job during its 24-hour retention window. After +that window, the tool reports that it cannot find the job. + This tool is only available when shell execution is granted (same `shell` grant category as `shell_execute`). diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index c14e99532..fb92fb002 100644 --- a/feeds/skills/.system/files/netclaw-operations/SKILL.md +++ b/feeds/skills/.system/files/netclaw-operations/SKILL.md @@ -3,7 +3,7 @@ name: netclaw-operations description: "REQUIRED when the user asks about scheduling, reminders, cron jobs, timers, background jobs, diagnostics, troubleshooting, MCP tools, daemon health, identity updates, or Netclaw capabilities and self-maintenance." metadata: author: netclaw - version: "2.42.0" + version: "2.43.0" --- # Netclaw Operations @@ -104,6 +104,7 @@ view inline plus a pointer to the full output — not the whole thing: `shell_execute` to get around it — that just spills again. - **`background_job`** output goes to `~/.netclaw/jobs/{id}/output.log` (bounded); `check_background_job` returns a tail, and you can `file_read`/`grep` the log for the rest. + Netclaw deletes a terminal job's definition and logs 24 hours after completion. Reading a targeted range or grepping is always cheaper than re-running a command or re-reading a whole file. Secret-bearing values are redacted from all tool output. diff --git a/feeds/skills/.system/files/netclaw-operations/references/scheduling.md b/feeds/skills/.system/files/netclaw-operations/references/scheduling.md index b68fe9266..becaa1621 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/scheduling.md +++ b/feeds/skills/.system/files/netclaw-operations/references/scheduling.md @@ -254,6 +254,8 @@ Lifecycle: notification with the log path — relaunch if still needed. - Process exit (success or failure) delivers a result turn with exit code, output tail, and log path — even if the session was passivated mid-flight. +- Netclaw retains every terminal job definition and its logs for 24 hours after + completion. The hourly cleanup sweep then deletes both artifact types. Monitoring a running job (e.g. waiting for a dev server to come up): @@ -282,7 +284,8 @@ Rules: model server that takes minutes to respond) should run as background jobs. - The user must approve the command before it starts running in the background. - Maximum 5 concurrent background jobs; overflow queues FIFO. -- Job definitions persist to `~/.netclaw/jobs/{id}.json`. +- Job definitions persist to `~/.netclaw/jobs/{id}.json` until 24 hours after + the job reaches a terminal state. `check_background_job` is only available when shell execution is granted (same `shell` grant category). It validates that the requesting session matches the diff --git a/src/Netclaw.Actors.Tests/Jobs/BackgroundJobDefinitionStoreTests.cs b/src/Netclaw.Actors.Tests/Jobs/BackgroundJobDefinitionStoreTests.cs index 3c6cfd4ce..8f52ef3b2 100644 --- a/src/Netclaw.Actors.Tests/Jobs/BackgroundJobDefinitionStoreTests.cs +++ b/src/Netclaw.Actors.Tests/Jobs/BackgroundJobDefinitionStoreTests.cs @@ -150,6 +150,42 @@ public void DeleteJobArtifacts_missing_job_returns_false_without_throwing() Assert.False(removed); } + [Fact] + public void DeleteJobArtifacts_keeps_definition_when_output_cleanup_fails_then_retries() + { + var store = new BackgroundJobDefinitionStore(_paths); + var jobId = new BackgroundJobId("cleanup-retry-001"); + store.Save(new BackgroundJobDefinition + { + Id = jobId, + Command = "dotnet test", + SessionId = new Netclaw.Actors.Protocol.SessionId("C0ABC/1712000000.000001"), + Rationale = "Run the test suite.", + Status = BackgroundJobStatus.Completed, + TimeoutSeconds = 300, + Audience = TrustAudience.Team, + Boundary = TrustBoundary.Team, + OriginChannelType = Netclaw.Actors.Channels.ChannelType.Slack + }); + + var outputLogPath = store.GetOutputLogPathOnly(jobId); + var outputDirectory = Path.GetDirectoryName(outputLogPath)!; + File.WriteAllText(outputDirectory, "path collision"); + + var error = Assert.Throws(() => store.DeleteJobArtifacts(jobId)); + + Assert.Contains("is not a directory", error.Message); + Assert.NotNull(store.Get(jobId)); + Assert.True(File.Exists(outputDirectory)); + + File.Delete(outputDirectory); + File.WriteAllText(store.GetOutputLogPath(jobId), "build output"); + + Assert.True(store.DeleteJobArtifacts(jobId)); + Assert.Null(store.Get(jobId)); + Assert.False(Directory.Exists(outputDirectory)); + } + /// /// Traversal guard (adversarial review, HIGH): Uri.EscapeDataString does not /// escape dots, so a dot-only id must NOT resolve to the jobs directory's @@ -213,6 +249,39 @@ public void Definition_with_dot_only_id_is_rejected_at_load(string unsafeId) Assert.Contains(logger.Errors, e => e.Contains("unsafe id")); } + [Fact] + public void Definition_with_id_that_does_not_match_file_name_is_rejected_at_load() + { + var logger = new CapturingJobLogger(); + var store = new BackgroundJobDefinitionStore(_paths, logger); + var victimId = new BackgroundJobId("victim-job"); + store.Save(new BackgroundJobDefinition + { + Id = victimId, + Command = "dotnet test", + SessionId = new Netclaw.Actors.Protocol.SessionId("C0ABC/1712000000.000001"), + Rationale = "Run the test suite.", + Status = BackgroundJobStatus.Completed, + StartedAtMs = 1, + CompletedAtMs = 2, + Audience = TrustAudience.Team, + Boundary = TrustBoundary.Team, + OriginChannelType = Netclaw.Actors.Channels.ChannelType.Slack + }); + + var victimPath = Path.Combine(_paths.JobsDirectory, $"{Uri.EscapeDataString(victimId.Value)}.json"); + var aliasPath = Path.Combine(_paths.JobsDirectory, "stale-alias.json"); + File.Copy(victimPath, aliasPath); + + var loaded = store.List(); + + var definition = Assert.Single(loaded); + Assert.Equal(victimId, definition.Id); + Assert.Null(store.Get(new BackgroundJobId("stale-alias"))); + Assert.Contains(logger.Errors, error => + error.Contains("does not match its canonical file name", StringComparison.Ordinal)); + } + /// /// Byte-equality gate for issue #994 Pass 7b. Wrapping BackgroundJobDefinition.Id /// in and SessionId in diff --git a/src/Netclaw.Actors.Tests/Jobs/BackgroundJobManagerActorTests.cs b/src/Netclaw.Actors.Tests/Jobs/BackgroundJobManagerActorTests.cs index a48781277..eab2ab54d 100644 --- a/src/Netclaw.Actors.Tests/Jobs/BackgroundJobManagerActorTests.cs +++ b/src/Netclaw.Actors.Tests/Jobs/BackgroundJobManagerActorTests.cs @@ -49,6 +49,16 @@ protected override async Task AfterAllAsync() private IActorRef GetManager() => ActorRegistry.For(Sys).Get(); + private async Task RunTerminalSweepAsync(IActorRef manager) + { + // Both messages use the same sender, so the health response is a strict + // mailbox barrier after the sweep. + manager.Tell(BackgroundJobManagerActor.SweepTerminalJobs.Instance, TestActor); + manager.Tell(GetBackgroundJobManagerHealth.Instance, TestActor); + return await ExpectMsgAsync( + TimeSpan.FromSeconds(30), cancellationToken: TestContext.Current.CancellationToken); + } + private BackgroundJobDefinition MakeTerminalDefinition(string jobId, BackgroundJobStatus status, long completedAtMs) => new() { Id = new BackgroundJobId(jobId), @@ -324,7 +334,8 @@ await manager.Ask( [Fact] public async Task StartupReconciliation_EmitsAlert_ForLegacyJobMissingTrustFields() - { var paths = new NetclawPaths(_dir.Path); + { + var paths = new NetclawPaths(_dir.Path); paths.EnsureDirectoriesExist(); const string jobId = "legacy-job-alert"; @@ -390,13 +401,7 @@ public async Task TerminalSweep_DeletesJobPastRetentionWindow() _store.Save(MakeTerminalDefinition("sweep-past", BackgroundJobStatus.Completed, pastWindowMs)); - await manager.Ask( - GetBackgroundJobManagerHealth.Instance, - TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken); - manager.Tell(BackgroundJobManagerActor.SweepTerminalJobs.Instance); - await manager.Ask( - GetBackgroundJobManagerHealth.Instance, - TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken); + await RunTerminalSweepAsync(manager); Assert.Null(_store.Get(new BackgroundJobId("sweep-past"))); } @@ -409,10 +414,7 @@ public async Task TerminalSweep_KeepsJobWithinRetentionWindow() _store.Save(MakeTerminalDefinition("sweep-recent", BackgroundJobStatus.Completed, recentMs)); - manager.Tell(BackgroundJobManagerActor.SweepTerminalJobs.Instance); - await manager.Ask( - GetBackgroundJobManagerHealth.Instance, - TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken); + await RunTerminalSweepAsync(manager); Assert.NotNull(_store.Get(new BackgroundJobId("sweep-recent"))); } @@ -431,10 +433,7 @@ public async Task TerminalSweep_DoesNotTouchNonTerminalJobs() _store.Save(MakeTerminalDefinition("sweep-running", BackgroundJobStatus.Running, pastWindowMs)); _store.Save(MakeTerminalDefinition("sweep-pending", BackgroundJobStatus.Pending, pastWindowMs)); - manager.Tell(BackgroundJobManagerActor.SweepTerminalJobs.Instance); - await manager.Ask( - GetBackgroundJobManagerHealth.Instance, - TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken); + await RunTerminalSweepAsync(manager); Assert.NotNull(_store.Get(new BackgroundJobId("sweep-running"))); Assert.NotNull(_store.Get(new BackgroundJobId("sweep-pending"))); @@ -455,10 +454,7 @@ public async Task TerminalSweep_DeletesOutputLogWithDefinition() _store.Save(MakeTerminalDefinition("sweep-log", BackgroundJobStatus.Failed, pastWindowMs)); - manager.Tell(BackgroundJobManagerActor.SweepTerminalJobs.Instance); - await manager.Ask( - GetBackgroundJobManagerHealth.Instance, - TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken); + await RunTerminalSweepAsync(manager); Assert.Null(_store.Get(jobId)); Assert.False(File.Exists(outputLogPath)); @@ -466,46 +462,75 @@ await manager.Ask( } [Fact] - public async Task TerminalSweep_SweepsTerminalJobWithNullCompletedAtMs_UsingStartedAtFallback() + public async Task TerminalSweep_CleanupFailureDoesNotRestartManagerAndLaterSweepRetries() { - // Adversarial review, MED: a terminal-status definition with null - // CompletedAtMs must still be swept once its StartedAtMs is past the - // window — otherwise it leaks forever (the exact bug this PR fixes). var manager = GetManager(); var pastWindowMs = TimeProvider.System.GetUtcNow() .Subtract(BackgroundJobManagerActor.TerminalJobRetentionWindow) .Subtract(TimeSpan.FromMinutes(1)) .ToUnixTimeMilliseconds(); + var blocked = MakeTerminalDefinition("sweep-blocked", BackgroundJobStatus.Failed, pastWindowMs); + var removable = MakeTerminalDefinition("sweep-removable", BackgroundJobStatus.Completed, pastWindowMs); + _store.Save(blocked); + _store.Save(removable); + + var blockedOutputPath = _store.GetOutputLogPathOnly(blocked.Id); + var blockedOutputDirectory = Path.GetDirectoryName(blockedOutputPath)!; + File.WriteAllText(blockedOutputDirectory, "path collision"); + File.WriteAllText(_store.GetOutputLogPath(removable.Id), "completed output"); + + var active = await manager.Ask( + MakeStartCommand("sleep 60"), + TimeSpan.FromSeconds(5), + TestContext.Current.CancellationToken); - var def = MakeTerminalDefinition("sweep-null-completed", BackgroundJobStatus.Failed, pastWindowMs); - def = def with { CompletedAtMs = null }; - _store.Save(def); + try + { + var health = await RunTerminalSweepAsync(manager); - manager.Tell(BackgroundJobManagerActor.SweepTerminalJobs.Instance); - await manager.Ask( - GetBackgroundJobManagerHealth.Instance, - TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken); + Assert.Equal(1, health.ActiveJobCount); + Assert.NotNull(_store.Get(blocked.Id)); + Assert.Null(_store.Get(removable.Id)); - Assert.Null(_store.Get(new BackgroundJobId("sweep-null-completed"))); + File.Delete(blockedOutputDirectory); + await RunTerminalSweepAsync(manager); + + Assert.Null(_store.Get(blocked.Id)); + } + finally + { + if (File.Exists(blockedOutputDirectory)) + File.Delete(blockedOutputDirectory); + + await manager.Ask( + new CancelBackgroundJob( + active.JobId, + new SessionId("test/thread"), + TrustAudience.Personal, + TrustBoundary.Personal), + TimeSpan.FromSeconds(5), + TestContext.Current.CancellationToken); + } } [Fact] - public async Task TerminalSweep_KeepsTerminalJobWithNullCompletedAtMs_WhenStartedAtRecent() + public async Task TerminalSweep_KeepsTerminalJobWithMissingCompletionTime() { - // A terminal job with null CompletedAtMs but a RECENT StartedAtMs must - // survive the sweep — the StartedAt fallback must not over-delete. var manager = GetManager(); - var recentMs = TimeProvider.System.GetUtcNow().ToUnixTimeMilliseconds(); + var pastWindowMs = TimeProvider.System.GetUtcNow() + .Subtract(BackgroundJobManagerActor.TerminalJobRetentionWindow) + .Subtract(TimeSpan.FromMinutes(1)) + .ToUnixTimeMilliseconds(); - var def = MakeTerminalDefinition("sweep-null-recent", BackgroundJobStatus.Failed, recentMs); + var def = MakeTerminalDefinition("sweep-null-completed", BackgroundJobStatus.Failed, pastWindowMs); def = def with { CompletedAtMs = null }; _store.Save(def); + var outputLogPath = _store.GetOutputLogPath(def.Id); + File.WriteAllText(outputLogPath, "diagnostic output"); - manager.Tell(BackgroundJobManagerActor.SweepTerminalJobs.Instance); - await manager.Ask( - GetBackgroundJobManagerHealth.Instance, - TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken); + await RunTerminalSweepAsync(manager); - Assert.NotNull(_store.Get(new BackgroundJobId("sweep-null-recent"))); + Assert.NotNull(_store.Get(def.Id)); + Assert.True(File.Exists(outputLogPath)); } } diff --git a/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs b/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs index a543dd8f6..b775849c2 100644 --- a/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs +++ b/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs @@ -71,6 +71,17 @@ public BackgroundJobDefinitionStore(NetclawPaths paths, ILogger cutoffMs) continue; - if (_store.DeleteJobArtifacts(def.Id)) + try + { + if (_store.DeleteJobArtifacts(def.Id)) + { + _definitions.Remove(def.Id.Value); + swept++; + _log.Info( + "Swept terminal background job {JobId} (status={Status}, completed_at={CompletedAtMs}) past retention window", + def.Id, def.Status, def.CompletedAtMs); + } + } + catch (Exception ex) { - _definitions.Remove(def.Id.Value); - swept++; - _log.Info( - "Swept terminal background job {JobId} (status={Status}, completed_at={CompletedAtMs}) past retention window", - def.Id, def.Status, def.CompletedAtMs); + // Keep the definition so the next periodic sweep can retry the + // cleanup. One filesystem failure must not stop the manager or + // prevent cleanup of other terminal jobs. + _log.Warning( + "Failed to sweep terminal background job {JobId}; cleanup will retry: {Error}", + def.Id, ex.Message); } } From c2bb2aa9c427008e9a46eeccafb81d79002404e0 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sun, 9 Aug 2026 02:03:55 +0000 Subject: [PATCH 6/8] Delete successful one-shot reminders --- .../.system/files/netclaw-operations/SKILL.md | 2 +- .../netclaw-operations/references/scheduling.md | 8 ++++---- .../Reminders/ReminderManagerActorTests.cs | 16 ++++------------ .../Reminders/ReminderManagerActor.cs | 17 +---------------- 4 files changed, 10 insertions(+), 33 deletions(-) diff --git a/feeds/skills/.system/files/netclaw-operations/SKILL.md b/feeds/skills/.system/files/netclaw-operations/SKILL.md index fb92fb002..a0cff7f80 100644 --- a/feeds/skills/.system/files/netclaw-operations/SKILL.md +++ b/feeds/skills/.system/files/netclaw-operations/SKILL.md @@ -3,7 +3,7 @@ name: netclaw-operations description: "REQUIRED when the user asks about scheduling, reminders, cron jobs, timers, background jobs, diagnostics, troubleshooting, MCP tools, daemon health, identity updates, or Netclaw capabilities and self-maintenance." metadata: author: netclaw - version: "2.43.0" + version: "2.44.0" --- # Netclaw Operations diff --git a/feeds/skills/.system/files/netclaw-operations/references/scheduling.md b/feeds/skills/.system/files/netclaw-operations/references/scheduling.md index becaa1621..a4b063a16 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/scheduling.md +++ b/feeds/skills/.system/files/netclaw-operations/references/scheduling.md @@ -66,10 +66,10 @@ A known execution or delivery failure starts the Akka.Reminders retry policy. The retry uses bounded backoff and the same durable occurrence identity. A successful attempt resets the consecutive failure count. -A one-shot reminder stays enabled while an occurrence can retry. A successful -one-shot becomes disabled with a `Completed` outcome. A poison one-shot becomes -disabled with a `Failed` outcome. Both definitions and their history remain -available until an operator uses the permanent delete command. +A one-shot reminder stays enabled while an occurrence can retry. After a +successful acknowledgement, Netclaw deletes its definition and history. A poison +one-shot becomes disabled with a `Failed` outcome. Its definition and history +remain available until an operator uses the permanent delete command. Each attempt has a 20-minute inactivity limit and a one-hour absolute limit. The durable acknowledgement lease is 70 minutes. A daemon crash therefore lets diff --git a/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs b/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs index 5a1772a13..7a75a2be6 100644 --- a/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs +++ b/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs @@ -1238,7 +1238,7 @@ await AwaitAssertAsync(() => } [Fact] - public async Task Successful_retry_resets_failure_count_and_soft_deletes_oneshot() + public async Task Successful_retry_deletes_oneshot_definition_and_history() { var manager = await GetManagerAsync(); var gatewayProbe = CreateTestProbe("retry-success-gateway"); @@ -1271,17 +1271,9 @@ await gatewayProbe.ExpectMsgAsync( await AwaitAssertAsync(async () => { - var stored = _definitionStore.Get(definition.Id); - Assert.NotNull(stored); - Assert.False(stored!.Enabled); - Assert.Equal(0, stored.ConsecutiveFailures); - Assert.Equal(ReminderTerminalOutcome.Completed, stored.TerminalOutcome); - - var status = await manager.Ask( - new GetReminderStatusQuery(definition.Id), - TimeSpan.FromSeconds(3), - TestContext.Current.CancellationToken); - Assert.Equal("Delivered", status.Occurrence?.CompletionStatus); + Assert.Null(_definitionStore.Get(definition.Id)); + var historyStore = new ReminderHistoryStore(new NetclawPaths(_basePath)); + Assert.Empty(await historyStore.ReadAsync(definition.Id, 10)); }, duration: TimeSpan.FromSeconds(5), cancellationToken: TestContext.Current.CancellationToken); } diff --git a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs index 3af97195b..c1f1aeb67 100644 --- a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs +++ b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs @@ -872,22 +872,7 @@ private async Task SettleSuccessfulExecutionAsync( } if (definition is { Schedule.Type: ReminderScheduleType.OneShot }) - { - try - { - _definitionStore.Save(definition with - { - Enabled = false, - ConsecutiveFailures = 0, - TerminalOutcome = ReminderTerminalOutcome.Completed, - UpdatedAtMs = _timeProvider.GetUtcNow().ToUnixTimeMilliseconds() - }); - } - catch (Exception ex) - { - _log.Error(ex, "Failed to save completed state for one-shot reminder '{0}'", outcome.Id.Value); - } - } + await DeleteReminderInternalAsync(outcome.Id); _log.Info("Reminder '{0}' execution completed successfully", outcome.Id.Value); } From 4c6debbda9c1f7a275025a323798a830918348be Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sun, 9 Aug 2026 02:10:33 +0000 Subject: [PATCH 7/8] Sweep completed one-shot reminders --- .../references/scheduling.md | 1 + .../Reminders/ReminderManagerActorTests.cs | 23 +++++++++++++++- .../Reminders/ReminderManagerActor.cs | 26 ++++++++++++++++--- 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/feeds/skills/.system/files/netclaw-operations/references/scheduling.md b/feeds/skills/.system/files/netclaw-operations/references/scheduling.md index a4b063a16..6d17283d2 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/scheduling.md +++ b/feeds/skills/.system/files/netclaw-operations/references/scheduling.md @@ -70,6 +70,7 @@ A one-shot reminder stays enabled while an occurrence can retry. After a successful acknowledgement, Netclaw deletes its definition and history. A poison one-shot becomes disabled with a `Failed` outcome. Its definition and history remain available until an operator uses the permanent delete command. +Startup reconciliation also removes completed one-shots from prior versions. Each attempt has a 20-minute inactivity limit and a one-hour absolute limit. The durable acknowledgement lease is 70 minutes. A daemon crash therefore lets diff --git a/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs b/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs index 7a75a2be6..be3eeeff1 100644 --- a/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs +++ b/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs @@ -321,7 +321,7 @@ await ExpectTerminatedAsync( } [Fact] - public async Task Reconcile_retains_past_oneshot_without_durable_outcome() + public async Task Reconcile_deletes_completed_oneshot_and_retains_ambiguous_or_failed_oneshots() { var manager = await GetManagerAsync(); var now = TimeProvider.System.GetUtcNow(); @@ -361,6 +361,23 @@ public async Task Reconcile_retains_past_oneshot_without_durable_outcome() await historyStore.AppendAsync( zombie.Id, new HistoryRecord(now.AddMinutes(-30), false, 100, "session-1", "recovery failed")); + var completed = zombie with + { + Id = new ReminderId("completed-old"), + Enabled = false, + TerminalOutcome = ReminderTerminalOutcome.Completed + }; + var failed = zombie with + { + Id = new ReminderId("failed-old"), + Enabled = false, + ConsecutiveFailures = ReminderManagerActor.FailurePauseThreshold, + TerminalOutcome = ReminderTerminalOutcome.Failed + }; + _definitionStore.Save(completed); + _definitionStore.Save(failed); + await historyStore.AppendAsync(completed.Id, new HistoryRecord(now, true, 100, "session-1", null)); + await historyStore.AppendAsync(failed.Id, new HistoryRecord(now, false, 100, "session-1", "failed")); // Confirm it shows up as scheduled var healthBefore = await manager.Ask( @@ -379,6 +396,10 @@ await historyStore.AppendAsync( Assert.True(afterReconcile!.Enabled); Assert.Null(afterReconcile.TerminalOutcome); Assert.Single(await historyStore.ReadAsync(zombie.Id, 10)); + Assert.Null(_definitionStore.Get(completed.Id)); + Assert.Empty(await historyStore.ReadAsync(completed.Id, 10)); + Assert.NotNull(_definitionStore.Get(failed.Id)); + Assert.Single(await historyStore.ReadAsync(failed.Id, 10)); } [Theory] diff --git a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs index c1f1aeb67..c80ffe873 100644 --- a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs +++ b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs @@ -1043,6 +1043,17 @@ private async Task HandleReconcileAsync() var definitions = _definitionStore.List(); var definitionsById = definitions.ToDictionary(d => d.Id.Value, StringComparer.Ordinal); + var deletedCompletedOneShots = 0; + foreach (var definition in definitions.Where(d => + !d.Enabled && + d.Schedule.Type == ReminderScheduleType.OneShot && + d.TerminalOutcome == ReminderTerminalOutcome.Completed)) + { + await DeleteReminderInternalAsync(definition.Id); + definitionsById.Remove(definition.Id.Value); + deletedCompletedOneShots++; + } + var cancelledOrphans = 0; foreach (var (id, _) in scheduled) { @@ -1099,6 +1110,13 @@ private async Task HandleReconcileAsync() if (outcome is null) continue; + if (outcome == ReminderTerminalOutcome.Completed) + { + await DeleteReminderInternalAsync(definition.Id); + deletedCompletedOneShots++; + continue; + } + var terminalDefinition = definition with { Enabled = false, @@ -1120,13 +1138,15 @@ d.Schedule.Type is not ReminderScheduleType.OneShot && disabledExpired++; } - if (cancelledOrphans > 0 || restoredSchedules > 0 || softDeletedOneShots > 0 || disabledExpired > 0) + if (cancelledOrphans > 0 || restoredSchedules > 0 || softDeletedOneShots > 0 + || disabledExpired > 0 || deletedCompletedOneShots > 0) { - _log.Info("Reminder reconcile complete: cancelled_orphans={0}, restored={1}, soft_deleted_oneshots={2}, disabled_expired={3}", + _log.Info("Reminder reconcile complete: cancelled_orphans={0}, restored={1}, soft_deleted_oneshots={2}, disabled_expired={3}, deleted_completed_oneshots={4}", cancelledOrphans, restoredSchedules, softDeletedOneShots, - disabledExpired); + disabledExpired, + deletedCompletedOneShots); } // Only ack external callers — skip Self.Tell from PreStart From 14b8ab2357a8a208eccffcb26846fcf716e18d71 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Sun, 9 Aug 2026 02:20:26 +0000 Subject: [PATCH 8/8] Keep reminder cleanup retriable --- src/Netclaw.Actors/Reminders/ReminderManagerActor.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs index c80ffe873..1faba717b 100644 --- a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs +++ b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs @@ -443,7 +443,6 @@ private async Task DisableReminderInternalAsync(ReminderI /// private async Task DeleteReminderInternalAsync(ReminderId id) { - _definitionStore.Delete(id); await CancelScheduleOnlyAsync(id); _skipCounts.Remove(id); @@ -454,7 +453,11 @@ private async Task DeleteReminderInternalAsync(ReminderId id) catch (Exception ex) { _log.Warning(ex, "Failed to delete history for reminder '{0}'", id.Value); + throw; } + + // Delete the definition last so reconciliation can retry a partial cleanup. + _definitionStore.Delete(id); } private async Task EnableReminderInternalAsync(ReminderId id)