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..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.42.0" + version: "2.44.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..6d17283d2 100644 --- a/feeds/skills/.system/files/netclaw-operations/references/scheduling.md +++ b/feeds/skills/.system/files/netclaw-operations/references/scheduling.md @@ -66,10 +66,11 @@ 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. +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 @@ -254,6 +255,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 +285,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 9a209c2d0..8f52ef3b2 100644 --- a/src/Netclaw.Actors.Tests/Jobs/BackgroundJobDefinitionStoreTests.cs +++ b/src/Netclaw.Actors.Tests/Jobs/BackgroundJobDefinitionStoreTests.cs @@ -97,6 +97,191 @@ 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); + } + + [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 + /// 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")); + } + + [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 05e3fe114..eab2ab54d 100644 --- a/src/Netclaw.Actors.Tests/Jobs/BackgroundJobManagerActorTests.cs +++ b/src/Netclaw.Actors.Tests/Jobs/BackgroundJobManagerActorTests.cs @@ -49,6 +49,31 @@ 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), + 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, @@ -364,4 +389,148 @@ 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 RunTerminalSweepAsync(manager); + + 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)); + + await RunTerminalSweepAsync(manager); + + 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)); + + await RunTerminalSweepAsync(manager); + + 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)); + + await RunTerminalSweepAsync(manager); + + Assert.Null(_store.Get(jobId)); + Assert.False(File.Exists(outputLogPath)); + Assert.False(Directory.Exists(Path.GetDirectoryName(outputLogPath))); + } + + [Fact] + public async Task TerminalSweep_CleanupFailureDoesNotRestartManagerAndLaterSweepRetries() + { + 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); + + try + { + var health = await RunTerminalSweepAsync(manager); + + Assert.Equal(1, health.ActiveJobCount); + Assert.NotNull(_store.Get(blocked.Id)); + Assert.Null(_store.Get(removable.Id)); + + 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_KeepsTerminalJobWithMissingCompletionTime() + { + 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); + var outputLogPath = _store.GetOutputLogPath(def.Id); + File.WriteAllText(outputLogPath, "diagnostic output"); + + await RunTerminalSweepAsync(manager); + + Assert.NotNull(_store.Get(def.Id)); + Assert.True(File.Exists(outputLogPath)); + } } diff --git a/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs b/src/Netclaw.Actors.Tests/Reminders/ReminderManagerActorTests.cs index 5a1772a13..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] @@ -1238,7 +1259,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 +1292,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/Jobs/BackgroundJobDefinitionStore.cs b/src/Netclaw.Actors/Jobs/BackgroundJobDefinitionStore.cs index 06fa6026e..b775849c2 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 // @@ -54,7 +54,55 @@ 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; + } + + var expectedFileName = $"{Uri.EscapeDataString(definition.Id.Value)}.json"; + var actualFileName = Path.GetFileName(path); + if (!string.Equals(actualFileName, expectedFileName, StringComparison.Ordinal)) + { + _logger.LogError( + "Background job document {Path} has id '{JobId}', which does not match its canonical file name {ExpectedFileName}. " + + "The job will not be loaded.", + path, definition.Id.Value, expectedFileName); + 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; } /// @@ -145,6 +193,60 @@ 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; + + // 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) + { + // 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)) + return false; + + if (File.Exists(fullDir)) + throw new IOException($"Background job artifact path '{fullDir}' is not a directory."); + + if (Directory.Exists(fullDir)) + { + Directory.Delete(fullDir, recursive: true); + removed = true; + } + } + + // Keep the definition until every output artifact is gone. A later + // sweep can retry if the directory delete fails because of a lock, + // permissions, or another transient filesystem error. + var path = GetPath(id); + if (File.Exists(path)) + { + File.Delete(path); + 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..6a199fd9b 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 // @@ -21,11 +21,27 @@ 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; + /// + /// 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 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 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 // its full output in memory and OOM the daemon. The log holds a head+tail view @@ -46,6 +62,17 @@ public sealed class BackgroundJobManagerActor : ReceiveActor private readonly Queue _deferredQueue = new(); private readonly Dictionary _definitions = []; + /// + /// 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, TimeProvider timeProvider, @@ -62,6 +89,7 @@ public BackgroundJobManagerActor( Receive(HandleQuery); Receive(HandleKillJobsForSession); Receive(_ => HandleGetHealth()); + Receive(_ => HandleSweepTerminalJobs()); } protected override void PreStart() @@ -72,6 +100,12 @@ 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. + // IWithTimers cancels the timer automatically when the actor stops. + Timers.StartPeriodicTimer(TerminalSweepTimerKey, SweepTerminalJobs.Instance, TerminalSweepInterval); } private async Task HandleStartAsync(StartBackgroundJob cmd) @@ -322,8 +356,88 @@ 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; + + // Every manager-owned terminal transition writes CompletedAtMs. A + // missing value is corrupt state, so do not infer a completion time + // and risk deletion before the full retention window has elapsed. + if (def.CompletedAtMs is not { } completedAtMs) + { + _log.Warning( + "Cannot sweep terminal background job {JobId} with status {Status}: completion timestamp is missing", + def.Id, def.Status); + continue; + } + + if (completedAtMs > cutoffMs) + continue; + + 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) + { + // 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); + } + } + + 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 +611,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(); + } } diff --git a/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs b/src/Netclaw.Actors/Reminders/ReminderManagerActor.cs index 3af97195b..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) @@ -872,22 +875,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); } @@ -1058,6 +1046,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) { @@ -1114,6 +1113,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, @@ -1135,13 +1141,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