Clean up terminal jobs and successful one-shot reminders - #1821
Merged
Conversation
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.
Aaronontheweb
added
cleanup
Collaborator
Author
|
Fixes #1820 (background-job half). |
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.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Comment on lines
+380
to
+401
| 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); | ||
| } | ||
| } |
- 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).
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.
| 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 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"); |
Comment on lines
+415
to
+423
| 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); | ||
| } |
| OriginChannelType = Netclaw.Actors.Channels.ChannelType.Slack | ||
| }); | ||
|
|
||
| var victimPath = Path.Combine(_paths.JobsDirectory, $"{Uri.EscapeDataString(victimId.Value)}.json"); |
| }); | ||
|
|
||
| var victimPath = Path.Combine(_paths.JobsDirectory, $"{Uri.EscapeDataString(victimId.Value)}.json"); | ||
| var aliasPath = Path.Combine(_paths.JobsDirectory, "stale-alias.json"); |
Collaborator
Author
|
Going to go with a different approach and have session-scoped reminders and background jobs store their state in the session directory and get cleaned up alongside sessions. |
Aaronontheweb
changed the title
Aaronontheweb
enabled auto-merge (squash)
August 9, 2026 02:21
Aaronontheweb
disabled auto-merge
August 9, 2026 02:48
Aaronontheweb
added a commit
that referenced
this pull request
Aug 20, 2026
* Sync delta specs for completed OpenSpec changes Apply the delta specs of 16 completed changes to the main specs. Create four new capability specs: daemon-shell-path, shell-policy-evaluator-architecture, skillserver-native-sidecar-sync, and named-model-definitions. Correct three reminder requirements against the merged code: - One-shot success removes the definition and its history. Only a poisoned one-shot is soft-deleted (PR #1821). - No execution capacity cap exists (PR #1839). The nack and ack-skip policy now covers a duplicate active occurrence and a short acknowledgement lease. - Every delivery kind now holds its envelope. ReminderDeliveryResult replaces ReminderDeliveryObserved. * Archive 16 completed OpenSpec changes Move each completed change to openspec/changes/archive/2026-08-19-<name>/. Their code is merged on dev. Also tick task 5.3 of consolidate-binding-actor-engines. PR #2005 merged that work. Leave eval-run checkboxes unticked. An evals-only gap does not block the archive.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Scope
This change does not alter reminder storage, scheduler payloads, session ownership, delivery routes, or retry settlement.
Existing active reminders and background jobs keep their current paths.
The failed one-shot retention window remains separate work in #1820.
Validation
Netclaw.Actors.Tests: 2,921 passed.git diff --checkpassed.The eval suite did not run.
Addresses #1820.