From f137da058b9589980dad004628683d6395a4a030 Mon Sep 17 00:00:00 2001 From: Sri Roopa Ramesh Babu Date: Thu, 13 Aug 2026 12:29:40 -0400 Subject: [PATCH] OLS-3566 Add TTL lifecycle CRD fields, reconciler enforcement, and regression tests Co-Authored-By: Claude Opus 4.6 (1M context) --- .ai/spec/what/crd-api.md | 13 +- .ai/spec/what/run-lifecycle.md | 15 +- api/v1alpha1/agenticolsconfig_types.go | 25 + api/v1alpha1/agenticrun_types.go | 39 +- ...gentic.openshift.io_agenticolsconfigs.yaml | 23 + .../agentic.openshift.io_agenticruns.yaml | 41 +- controller/agenticrun/handlers.go | 5 + controller/agenticrun/handlers_test.go | 65 ++ controller/agenticrun/helpers.go | 13 + controller/agenticrun/reconciler.go | 160 ++++- controller/agenticrun/ttl_test.go | 677 ++++++++++++++++++ 11 files changed, 1034 insertions(+), 42 deletions(-) create mode 100644 controller/agenticrun/ttl_test.go diff --git a/.ai/spec/what/crd-api.md b/.ai/spec/what/crd-api.md index ebf690d5..b890d077 100644 --- a/.ai/spec/what/crd-api.md +++ b/.ai/spec/what/crd-api.md @@ -9,7 +9,9 @@ Kubernetes API surface for the agentic operator. **Lifecycle and gates** are in 3. **Scope — cluster**: `Agent`, `LLMProvider`, `ApprovalPolicy`, and `AgenticOLSConfig` MUST be cluster-scoped; `metadata.name` is the global identifier. 4. **AgenticRun identity**: A `AgenticRun` MUST include required immutable fields per CEL: at minimum `spec.request` and `spec.analysis`. Omitting `spec.execution` or `spec.verification` means those steps do not exist for that run (see `run-lifecycle.md`). 5. **AgenticRun — `spec.request`**: Human/agent input text; immutable after creation; max length enforced by validation. -6. **AgenticRun — `spec.revisionFeedback`**: Only mutable spec field; when set/non-empty and `metadata.generation` advances beyond the analyzed condition’s `observedGeneration`, operators MUST trigger re-analysis per `run-lifecycle.md`. +6. **AgenticRun — `spec.revisionFeedback`**: Mutable spec field; when set/non-empty and `metadata.generation` advances beyond the analyzed condition’s `observedGeneration`, operators MUST trigger re-analysis per `run-lifecycle.md`. `spec.ttlAfterTerminal` (rule 6a) is also mutable and also advances `metadata.generation` when written; the operator MUST advance `Analyzed.observedGeneration` to match whenever it stamps `ttlAfterTerminal` itself, so that its own internal spec write is never mistaken for a user-initiated revision request. +6a. **AgenticRun — `spec.ttlAfterTerminal`**: Optional `*int32` (seconds), mutable, minimum `0`. Per-run override of the cluster-wide default (`AgenticOLSConfig.spec.lifecycle.terminalTTL`, rule 48). `0` explicitly disables auto-deletion for that run. Adapters/admins MAY pre-set it before the run reaches a terminal state; the operator MUST NOT overwrite a pre-set value with the cluster default. See `run-lifecycle.md` for the stamping and deletion behavior. +6b. **AgenticRun — `status.terminalTime`**: Optional `*metav1.Time`. Set once by the operator on the first reconcile after the run reaches a terminal phase (`Completed`, `Failed`, `Denied`, `Escalated`, `EmergencyStopped`, `NoActionRequired`); not updated again while the run remains terminal. Cleared by the revision handler when a terminal run re-enters analysis (rule 6/23), so a subsequent terminal phase gets a fresh timestamp instead of reusing the prior terminal event's. Used with `spec.ttlAfterTerminal` to compute the deletion deadline per `run-lifecycle.md`. 7. **AgenticRun — `spec.targetNamespaces`**: Optional list of namespaces for context and RBAC targeting; immutable once set; when empty, RBAC targeting MAY fall back to namespaces declared in analysis RBAC output at execution time (see `sandbox-execution.md`). 8. **AgenticRun — `spec.analysisOutput`**: Immutable after set. `mode` defaults to full analysis schema when empty/default. `mode=Minimal` REQUIRES `schema` to be set, forbids `spec.execution` and `spec.verification`, and restricts option shape accordingly. 9. **AgenticRun — `spec.tools`**: Default `ToolsSpec` for all steps; immutable once set. Per-step `tools` on `spec.analysis` / `spec.execution` / `spec.verification` replaces the default for that step only when non-zero. @@ -59,14 +61,15 @@ Kubernetes API surface for the agentic operator. **Lifecycle and gates** are in 45. **AgenticOLSConfig — absence**: When no `AgenticOLSConfig` CR exists, the system MUST behave as if `spec.suspended` is `false`. 46. **AgenticOLSConfig — status subresource**: `AgenticOLSConfig` MUST have a `/status` subresource with `conditions` array (`metav1.Condition`). Condition type `Suspended` tracks whether the operator has acknowledged and acted on `spec.suspended`. See `system-config.md` rules 5a–5e for full semantics. 47. **AgenticOLSConfig — status RBAC**: The operator’s service account MUST have `get`, `update`, `patch` on `agenticolsconfigs/status` in addition to existing permissions on the main resource. +48. **AgenticOLSConfig — `spec.lifecycle.terminalTTL`**: Optional `*int32` (seconds), minimum `0`, nested under `spec.lifecycle` (`MinProperties=1`). Cluster-wide default TTL applied to terminal `AgenticRun` resources that don't already carry a pre-set `spec.ttlAfterTerminal` (rule 6a). A pre-set `spec.ttlAfterTerminal: 0` explicitly disables auto-deletion for that run; only non-zero pre-set values remain eligible for expiry. When `spec.lifecycle` or the field is omitted, or no `AgenticOLSConfig` CR exists, no *default* gets stamped at that time — but this is not permanent: if an effective `terminalTTL` becomes available later (config created or updated), it is applied retroactively on the next reconcile to any already-terminal `AgenticRun` still lacking `spec.ttlAfterTerminal`. This does NOT affect runs that already carry a pre-set `spec.ttlAfterTerminal` independently of the cluster config — those are still deleted (or exempted, if `0`) on schedule regardless of whether `AgenticOLSConfig` exists. See `run-lifecycle.md` rule 23 for reconciliation behavior. ## Configuration Surface (by path) ### AgenticRun - `metadata.*` -- `spec.request`, `spec.targetNamespaces`, `spec.revisionFeedback`, `spec.analysisOutput`, `spec.tools`, `spec.analysis`, `spec.execution`, `spec.verification` +- `spec.request`, `spec.targetNamespaces`, `spec.revisionFeedback`, `spec.ttlAfterTerminal`, `spec.analysisOutput`, `spec.tools`, `spec.analysis`, `spec.execution`, `spec.verification` - `spec.analysis.instructions`, `spec.execution.instructions`, `spec.verification.instructions` [PLANNED: OLS-3491] -- `status.conditions`, `status.steps.analysis|execution|verification|escalation.*` +- `status.conditions`, `status.steps.analysis|execution|verification|escalation.*`, `status.terminalTime` ### Agent - `metadata.name`, `spec.llmProvider.name`, `spec.model`, `spec.reasoningConfig`, `spec.timeouts.*`, `spec.maxTurns`, `status.conditions` @@ -78,8 +81,9 @@ Kubernetes API surface for the agentic operator. **Lifecycle and gates** are in - `metadata.name` (must be `cluster`), `spec.stages[]`, `spec.maxConcurrentRuns` ### AgenticOLSConfig -- `metadata.name` (must be `cluster`), `spec.suspended`, `spec.templog` +- `metadata.name` (must be `cluster`), `spec.suspended`, `spec.templog`, `spec.lifecycle.terminalTTL` - `spec.templog` (bool, default `true`): When `true` or absent, the lightspeed-operator deploys a custom OTel Collector for temporary audit log storage in PostgreSQL. See `templog.md`. +- `spec.lifecycle.terminalTTL` (`*int32`, seconds): Cluster-wide default TTL for terminal `AgenticRun` garbage collection. See rule 48 and `run-lifecycle.md`. - `status.conditions` — condition types: `Suspended` - See `system-config.md` for full behavioral rules @@ -110,3 +114,4 @@ Kubernetes API surface for the agentic operator. **Lifecycle and gates** are in - [OLS-3328] Add `spec.templog` to `AgenticOLSConfig` CRD for temporary audit log storage. - [DONE: OLS-3295] Renamed `Proposal` → `AgenticRun`, `ProposalApproval` → `AgenticRunApproval` CRD kinds and all associated field names, RBAC resources, and label keys. - [PLANNED: OLS-3594] Optional `disableDefaultMCP` (and related auto-injection) — deferred; blocked by OLS-3526 and OLS-3572. Not near-term. +- [DONE: OLS-3566] Added `AgenticOLSConfig.spec.lifecycle.terminalTTL`, `AgenticRun.spec.ttlAfterTerminal`, and `AgenticRun.status.terminalTime` for terminal-run garbage collection. See rules 6a, 6b, 48 and `run-lifecycle.md`. diff --git a/.ai/spec/what/run-lifecycle.md b/.ai/spec/what/run-lifecycle.md index 44dd8b4e..0f794817 100644 --- a/.ai/spec/what/run-lifecycle.md +++ b/.ai/spec/what/run-lifecycle.md @@ -9,7 +9,7 @@ Behavioral specification for the `AgenticRun` resource lifecycle. **Approval gat 3. **Condition types (run-level)**: The workflow uses `Analyzed`, `Executed`, `Verified`, `Denied`, `Escalated`, `EmergencyStopped` (string values as defined on the API). Status values are `True`, `False`, or `Unknown`. 4. **Terminal phases**: `Completed`, `Denied`, `Escalated`, `Failed`, `EmergencyStopped`, and `NoActionRequired` are terminal for reconciliation progression. After `Completed`, `Denied`, `Escalated`, `EmergencyStopped`, or `NoActionRequired`, the controller MUST stop active work and MAY release sandbox claims when present. `Failed` triggers failure cleanup behaviors (see `sandbox-execution.md` for RBAC cleanup interactions). `EmergencyStopped` indicates the run was terminated by the system kill switch (see `system-config.md`). `NoActionRequired` indicates the analysis agent determined no remediation is needed (see rule 9). 5. **Workflow shape**: `spec.analysis` is always required. `spec.execution` and `spec.verification` MAY be omitted; omission skips those steps subject to rules 20–22. -6. **Revision loop**: If `spec.revisionFeedback` is non-empty AND `metadata.generation` is greater than `Analyzed.observedGeneration`, the system MUST treat the run as needing **re-analysis** before continuing downstream steps. Re-analysis MUST append revision context to the user-visible request text (after `spec.request`), then reset execution/verification/escalation progress as implemented for revision handling, and MUST NOT advance execution until the new analysis completes. Revision feedback is supported from the `NoActionRequired` terminal phase — patching `spec.revisionFeedback` resets conditions and re-runs analysis. +6. **Revision loop**: If `spec.revisionFeedback` is non-empty AND `metadata.generation` is greater than `Analyzed.observedGeneration`, the system MUST treat the run as needing **re-analysis** before continuing downstream steps. Re-analysis MUST append revision context to the user-visible request text (after `spec.request`), then reset execution/verification/escalation progress as implemented for revision handling, and MUST NOT advance execution until the new analysis completes. Revision feedback is supported from the `NoActionRequired` terminal phase — patching `spec.revisionFeedback` resets conditions and re-runs analysis. **Exception**: When the controller internally writes `spec.ttlAfterTerminal` (rule 23) on a terminal run, it bumps `metadata.generation`; the controller MUST simultaneously advance `Analyzed.observedGeneration` to match so this operator-internal mutation is never misread by the revision detector as a new revision request. This ensures terminal runs (advisory-only `Completed`, execution-less `Failed`, and `NoActionRequired`) do not re-enter analysis when only TTL stamping causes a generation bump. 7. **Verification failure → escalation**: When `spec.verification` is present, after a successful execution the verification step MAY fail **objectively** if the agent reports failure **or** any verification check records a non-pass outcome (even when a coarse success flag might otherwise read true). On verification failure, the system MUST NOT retry execution. The system MUST set `Verified` to `False` with reason `VerificationFailed` and MUST set `Escalated` to `Unknown`, entering the escalating phase. The escalation summary includes the execution result and failed verification result so a human operator can assess what happened. 8. **No execution retries**: The operator does not re-execute remediation after verification failure. Convergence-dependent checks (alerts, metrics, pod readiness) are handled within the verification agent's single sandbox call via prompt-guided wait-and-retry. This avoids the risk of re-executing non-idempotent remediations against a cluster in an unknown intermediate state. 9. **DerivePhase — precedence (first match in order)**: @@ -47,20 +47,29 @@ Behavioral specification for the `AgenticRun` resource lifecycle. **Approval gat | `False` | `SandboxTimeout` | Per-step timeout fired, pod killed (see `sandbox-execution.md` rule 40) | | `False` | `SandboxFailed` | Pod exited without creating Result CR | | `False` | `ImagePullFailed` | Pod stuck in ImagePullBackOff | + 15. **Success**: `Verified=True` MUST yield `Completed` once rule 9 reaches the `Verified` branch, unless an earlier branch already returned `Escalated` or `Denied` per rules 9–10. 16. **Step failure**: Any of `Analyzed`, `Executed`, or `Verified` with status `False` MUST yield `Failed` when reached by the derivation order in rule 9 (unless superseded by `Escalated` / `Denied` per rules 9–10). 16a. **[OLS-3666] Failure condition message**: When the controller sets a step condition to `False` (reason `Failed`) because the agent returned `success: false`, the condition `message` MUST include context from the agent response rather than a generic string. The controller MUST use the first available source from this fallback chain: (1) the sandbox response `summary` field (which contains the error message for sandbox-level failures or the raw agent output when the output schema has no top-level `summary` property); (2) for analysis: the top-level or per-option `diagnosis.summary`; for execution: the first failed action's `description` and `error`; (3) a properly-cased generic fallback (`"Analysis agent reported failure"` / `"Execution agent reported failure"`). The message MUST use sentence casing. 17. **Escalation failure**: `Escalated` with status `False` MUST yield `Failed` once rule 9 evaluates the `Escalated` presence branch (non-`True`, non-`Unknown`). 18. **Result CR linkage**: Each analysis/execution/verification/escalation attempt SHOULD append a `status.steps.*.results[]` entry naming the corresponding result resource with an outcome matching agent success/failure for that attempt. **Exception:** when the execution agent reports `success=false` but all mutating actions succeeded (only observation actions failed), the controller MUST override the outcome to `Succeeded` and proceed to the verification step. Observation action types (`pre-check`, `post-check`, `verification`, `check`, `wait`) are not considered when determining mutation success. 19. **Observed generation**: Conditions SHOULD carry `observedGeneration` aligned with `metadata.generation` when the controller updates them for the current spec generation, except revision completion MAY pin the analyzed condition to the generation that triggered the revision, per existing reconciliation behavior. -20. **Immutable spec (excluding revision)**: Once set, `spec.request`, `spec.targetNamespaces`, `spec.analysisOutput`, `spec.tools`, `spec.analysis`, `spec.execution`, and `spec.verification` MUST NOT change; CEL on the CRD enforces this. Only `spec.revisionFeedback` is mutable for iterative feedback. +20. **Immutable spec (excluding revision and TTL)**: Once set, `spec.request`, `spec.targetNamespaces`, `spec.analysisOutput`, `spec.tools`, `spec.analysis`, `spec.execution`, and `spec.verification` MUST NOT change; CEL on the CRD enforces this. `spec.revisionFeedback` (iterative feedback) and `spec.ttlAfterTerminal` (terminal-run TTL override, rule 23) are the two mutable spec fields; see rule 24 for how the controller keeps its own `ttlAfterTerminal` writes from being misread as a revision request. 21. **Option trim after analysis**: When multiple remediation options exist, execution MUST use the option selected through the approval resource; non-selected options MAY be removed from the stored analysis result before execution (see `approval.md`). 22. **Selected option for verification**: Verification MUST use the same selected remediation option as execution (latest trimmed analysis result). +23. **[OLS-3566] Terminal-run TTL / auto-deletion**: On every reconcile of a run in a terminal phase (rule 4), the controller MUST call a `handleTerminalTTL` step, integrated into all terminal branches (`Completed`, `Failed`, `Denied`, `Escalated`, `EmergencyStopped`, `NoActionRequired`): + - If `status.terminalTime` is unset, stamp it to the current time (set once per terminal episode, not updated again while still terminal — see `crd-api.md` rule 6b). When a terminal run re-enters revision (rule 6), `handleRevision` MUST clear `status.terminalTime` so a later terminal phase gets a fresh timestamp rather than computing TTL expiry off the earlier terminal event. + - If `spec.ttlAfterTerminal` is unset, stamp it from `AgenticOLSConfig.spec.lifecycle.terminalTTL` (rule 48 in `crd-api.md`) when that cluster-wide default exists; a pre-set `spec.ttlAfterTerminal` (by adapter or admin) MUST NOT be overwritten. When no cluster config exists, or the config has no `lifecycle.terminalTTL`, no default is stamped, and the run's `spec.ttlAfterTerminal` stays unset unless it was already pre-set. This absence of a cluster default only controls whether a *default* gets stamped — it does NOT suppress deletion of a run whose `spec.ttlAfterTerminal` was already pre-set independently of the cluster config; any non-zero pre-set value is still honored per the next two bullets regardless of `AgenticOLSConfig`'s presence. + - If `spec.ttlAfterTerminal` is unset, or explicitly `0`, the controller MUST NOT delete the run. + - Otherwise, once `status.terminalTime + spec.ttlAfterTerminal` has elapsed, the controller MUST delete the `AgenticRun` CR (Kubernetes garbage collection cascades to owned result CRs and sandbox resources via owner references per `crd-api.md` rule 40); before elapsing, the controller MUST requeue with `RequeueAfter` set to the remaining duration. + - The `AgenticOLSConfig`/`ApprovalPolicy`/watched-`ConfigMap` fan-out (see `system-config.md`) MUST re-enqueue terminal runs still missing `status.terminalTime` (this stamping is unconditional). It MUST re-enqueue terminal runs missing `spec.ttlAfterTerminal` only when an effective cluster-wide `terminalTTL` is currently configured, so a cluster TTL added after runs have already gone terminal is retroactively applied — and MUST NOT do so when no cluster default exists, since there would be nothing to stamp and re-enqueuing every such run on every config-adjacent change would be pure churn. +24. **[OLS-3566] Generation sync on internal TTL stamp**: Stamping `spec.ttlAfterTerminal` (rule 23) is a spec write and therefore advances `metadata.generation`, the same signal the revision loop (rule 6) uses to detect user-initiated feedback. Because `spec.revisionFeedback` is never cleared once processed, the controller MUST advance `Analyzed.observedGeneration` to match the post-stamp `metadata.generation` in the same operation that stamps `ttlAfterTerminal`, so this internal, operator-driven mutation is never misread by the revision loop as a new revision request. This generation sync runs unconditionally (outside the `TTLAfterTerminal == nil` conditional) so that a crash between the spec patch and the status patch is self-healing on the next reconcile. External `spec.ttlAfterTerminal` patches (by adapters or admins) also advance `metadata.generation`; the same idempotent sync handles these without triggering spurious revision re-entry. ## Configuration Surface - `spec.request` - `spec.revisionFeedback` +- `spec.ttlAfterTerminal` - `spec.targetNamespaces` - `spec.analysisOutput` / `spec.analysisOutput.mode` / `spec.analysisOutput.schema` - `spec.tools` and per-step `spec.analysis.tools`, `spec.execution.tools`, `spec.verification.tools` @@ -68,6 +77,7 @@ Behavioral specification for the `AgenticRun` resource lifecycle. **Approval gat - `metadata.generation` (revision detection vs `status.conditions`) - `status.conditions[*].type`, `status.conditions[*].status`, `status.conditions[*].reason`, `status.conditions[*].observedGeneration` - `status.steps.*.results`, `status.steps.*.sandbox` +- `status.terminalTime` ## Constraints @@ -83,3 +93,4 @@ Behavioral specification for the `AgenticRun` resource lifecycle. **Approval gat - [DONE: OLS-3268] `NoActionRequired` terminal phase: when analysis returns `actionRequired=false`, the operator sets `Analyzed=True` with reason `NoActionRequired` and the run auto-completes, bypassing approval/execution/verification. - [DONE: OLS-3295] Renamed `Proposal` CRD kind to `AgenticRun`, `ProposalApproval` to `AgenticRunApproval`, and updated all associated API surface (labels, RBAC resources, CLI commands, audit events, OTEL spans). - [DONE: OLS-3558] Execution outcome override — controller no longer hard-fails when `success=false` but all mutating actions succeeded; defers outcome to the verification step. See `sandbox-execution.md` rule 21b. +- [DONE: OLS-3566] Terminal-run TTL / auto-deletion (Kubernetes Jobs `ttlSecondsAfterFinished` pattern): `AgenticOLSConfig.spec.lifecycle.terminalTTL` cluster-wide default plus `AgenticRun.spec.ttlAfterTerminal` per-run override, stamped by `handleTerminalTTL` on terminal runs. See rules 23–24 and `crd-api.md` rules 6a, 6b, 48. diff --git a/api/v1alpha1/agenticolsconfig_types.go b/api/v1alpha1/agenticolsconfig_types.go index 869de448..671643e5 100644 --- a/api/v1alpha1/agenticolsconfig_types.go +++ b/api/v1alpha1/agenticolsconfig_types.go @@ -29,6 +29,26 @@ const ( AgenticOLSConfigConditionSuspended = "Suspended" ) +// LifecycleConfig controls automatic cleanup of terminal AgenticRun resources. +// +// +kubebuilder:validation:MinProperties=1 +type LifecycleConfig struct { + // terminalTTL is the default time-to-live in seconds for terminal + // AgenticRun resources (Completed, Failed, Denied, Escalated, + // EmergencyStopped, NoActionRequired). After a run reaches a terminal + // state and this many seconds elapse, the operator deletes the + // AgenticRun CR. Kubernetes garbage collection cascades deletion to + // owned resources via owner references. + // + // Per-run overrides via AgenticRun.spec.ttlAfterTerminal take + // precedence over this cluster-wide default. + // + // When omitted (nil), no automatic deletion occurs. + // +optional + // +kubebuilder:validation:Minimum=0 + TerminalTTL *int32 `json:"terminalTTL,omitempty"` +} + // AgenticOLSConfigSpec defines the desired state of AgenticOLSConfig. // // +kubebuilder:validation:MinProperties=1 @@ -41,6 +61,11 @@ type AgenticOLSConfigSpec struct { // +optional // +default=false Suspended bool `json:"suspended,omitempty"` //nolint:kubeapilinter // kill switch is genuinely binary; bool is the right type + + // lifecycle controls automatic cleanup of terminal AgenticRun resources. + // When omitted, no automatic deletion occurs (backwards-compatible). + // +optional + Lifecycle LifecycleConfig `json:"lifecycle,omitzero"` } // AgenticOLSConfigStatus defines the observed state of AgenticOLSConfig. diff --git a/api/v1alpha1/agenticrun_types.go b/api/v1alpha1/agenticrun_types.go index 311c4d83..021aa502 100644 --- a/api/v1alpha1/agenticrun_types.go +++ b/api/v1alpha1/agenticrun_types.go @@ -372,12 +372,36 @@ type AgenticRunSpec struct { // the operator detects (generation > observedGeneration) and triggers // re-analysis with the feedback appended to the original request. // - // Mutable: this is the only mutable spec field. All other spec fields - // are immutable via CEL rules, so generation changes signal revision. + // Mutable: this and ttlAfterTerminal below are the only mutable spec + // fields. All other spec fields are immutable via CEL rules, so a + // generation change coming from one of those would signal revision; + // see ttlAfterTerminal for how the controller avoids that trap for its + // own writes. // +optional // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=32768 RevisionFeedback string `json:"revisionFeedback,omitempty"` + + // ttlAfterTerminal is the time-to-live in seconds for this AgenticRun + // after it reaches a terminal state (Completed, Failed, Denied, + // Escalated, EmergencyStopped, NoActionRequired). When the TTL expires, + // the operator deletes the AgenticRun CR and Kubernetes garbage + // collection cascades deletion to owned resources. + // + // Overrides the cluster-wide default from + // AgenticOLSConfig.spec.lifecycle.terminalTTL for this run. + // + // Set to 0 to disable automatic deletion for this run. + // + // Mutable: adapters or admins may pre-set this before the run reaches + // terminal state. The operator will not overwrite a pre-set value. + // When the operator stamps this itself, it also advances + // Analyzed.observedGeneration to match in the same operation, so its + // own write is never mistaken by needsRevision() for a pending + // revisionFeedback request. + // +optional + // +kubebuilder:validation:Minimum=0 + TTLAfterTerminal *int32 `json:"ttlAfterTerminal,omitempty"` } // AgenticRunStatus defines the observed state of AgenticRun. All fields are @@ -406,6 +430,17 @@ type AgenticRunStatus struct { // info, and references to result CRs. // +optional Steps StepsStatus `json:"steps,omitzero"` + + // terminalTime is the timestamp when the run reached its current + // terminal state (Completed, Failed, Denied, Escalated, + // EmergencyStopped, NoActionRequired). Set once by the operator and not + // updated again while the run remains terminal; cleared when a + // revision request moves the run out of a terminal phase back into + // analysis, so a later terminal phase gets a fresh timestamp. + // Used together with spec.ttlAfterTerminal to compute when the run + // should be garbage-collected. + // +optional + TerminalTime *metav1.Time `json:"terminalTime,omitempty"` } // +kubebuilder:object:root=true diff --git a/config/crd/bases/agentic.openshift.io_agenticolsconfigs.yaml b/config/crd/bases/agentic.openshift.io_agenticolsconfigs.yaml index 3c3a2189..0e3ad021 100644 --- a/config/crd/bases/agentic.openshift.io_agenticolsconfigs.yaml +++ b/config/crd/bases/agentic.openshift.io_agenticolsconfigs.yaml @@ -54,6 +54,29 @@ spec: description: spec defines the desired system configuration. minProperties: 1 properties: + lifecycle: + description: |- + lifecycle controls automatic cleanup of terminal AgenticRun resources. + When omitted, no automatic deletion occurs (backwards-compatible). + minProperties: 1 + properties: + terminalTTL: + description: |- + terminalTTL is the default time-to-live in seconds for terminal + AgenticRun resources (Completed, Failed, Denied, Escalated, + EmergencyStopped, NoActionRequired). After a run reaches a terminal + state and this many seconds elapse, the operator deletes the + AgenticRun CR. Kubernetes garbage collection cascades deletion to + owned resources via owner references. + + Per-run overrides via AgenticRun.spec.ttlAfterTerminal take + precedence over this cluster-wide default. + + When omitted (nil), no automatic deletion occurs. + format: int32 + minimum: 0 + type: integer + type: object suspended: default: false description: |- diff --git a/config/crd/bases/agentic.openshift.io_agenticruns.yaml b/config/crd/bases/agentic.openshift.io_agenticruns.yaml index 17e1fd83..fa819f46 100644 --- a/config/crd/bases/agentic.openshift.io_agenticruns.yaml +++ b/config/crd/bases/agentic.openshift.io_agenticruns.yaml @@ -884,8 +884,11 @@ spec: the operator detects (generation > observedGeneration) and triggers re-analysis with the feedback appended to the original request. - Mutable: this is the only mutable spec field. All other spec fields - are immutable via CEL rules, so generation changes signal revision. + Mutable: this and ttlAfterTerminal below are the only mutable spec + fields. All other spec fields are immutable via CEL rules, so a + generation change coming from one of those would signal revision; + see ttlAfterTerminal for how the controller avoids that trap for its + own writes. maxLength: 32768 minLength: 1 type: string @@ -1275,6 +1278,28 @@ spec: - image x-kubernetes-list-type: map type: object + ttlAfterTerminal: + description: |- + ttlAfterTerminal is the time-to-live in seconds for this AgenticRun + after it reaches a terminal state (Completed, Failed, Denied, + Escalated, EmergencyStopped, NoActionRequired). When the TTL expires, + the operator deletes the AgenticRun CR and Kubernetes garbage + collection cascades deletion to owned resources. + + Overrides the cluster-wide default from + AgenticOLSConfig.spec.lifecycle.terminalTTL for this run. + + Set to 0 to disable automatic deletion for this run. + + Mutable: adapters or admins may pre-set this before the run reaches + terminal state. The operator will not overwrite a pre-set value. + When the operator stamps this itself, it also advances + Analyzed.observedGeneration to match in the same operation, so its + own write is never mistaken by needsRevision() for a pending + revisionFeedback request. + format: int32 + minimum: 0 + type: integer verification: description: |- verification defines per-step configuration for the verification step. @@ -2268,6 +2293,18 @@ spec: type: object type: object type: object + terminalTime: + description: |- + terminalTime is the timestamp when the run reached its current + terminal state (Completed, Failed, Denied, Escalated, + EmergencyStopped, NoActionRequired). Set once by the operator and not + updated again while the run remains terminal; cleared when a + revision request moves the run out of a terminal phase back into + analysis, so a later terminal phase gets a fresh timestamp. + Used together with spec.ttlAfterTerminal to compute when the run + should be garbage-collected. + format: date-time + type: string type: object required: - spec diff --git a/controller/agenticrun/handlers.go b/controller/agenticrun/handlers.go index 5b4f501e..2e7e0f9e 100644 --- a/controller/agenticrun/handlers.go +++ b/controller/agenticrun/handlers.go @@ -159,6 +159,11 @@ func (r *AgenticRunReconciler) handleRevision( meta.RemoveStatusCondition(&run.Status.Conditions, agenticv1alpha1.AgenticRunConditionVerified) meta.RemoveStatusCondition(&run.Status.Conditions, agenticv1alpha1.AgenticRunConditionEscalated) resetExecutionAndVerification(&run.Status.Steps) + // The run is leaving its terminal phase to re-analyze; clear terminalTime + // so that if it reaches a terminal phase again, handleTerminalTTL stamps + // a fresh timestamp instead of computing expiry off the prior terminal + // event (see run-lifecycle.md rule 23/24). + run.Status.TerminalTime = nil meta.SetStatusCondition(&run.Status.Conditions, metav1.Condition{ Type: agenticv1alpha1.AgenticRunConditionAnalyzed, Status: metav1.ConditionUnknown, diff --git a/controller/agenticrun/handlers_test.go b/controller/agenticrun/handlers_test.go index 023b67f6..0c339b66 100644 --- a/controller/agenticrun/handlers_test.go +++ b/controller/agenticrun/handlers_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "testing" + "time" rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/meta" @@ -786,6 +787,70 @@ func TestReconcile_RevisionFromCompleted(t *testing.T) { } } +// TestReconcile_RevisionClearsTerminalTime verifies that a run which already +// carries a terminalTime (stamped by handleTerminalTTL, OLS-3566) has it +// cleared once a revision moves it back out of the terminal phase -- +// otherwise a later terminal phase would compute TTL expiry off the stale, +// earlier terminal event instead of a fresh one (run-lifecycle.md rule 23/24). +func TestReconcile_RevisionClearsTerminalTime(t *testing.T) { + scheme := testScheme() + run := &agenticv1alpha1.AgenticRun{ + ObjectMeta: metav1.ObjectMeta{Name: "fix-crash", Namespace: "default"}, + Spec: agenticv1alpha1.AgenticRunSpec{ + Request: "Investigate issue", + Tools: testTools(), + TargetNamespaces: []string{"production"}, + Analysis: agenticv1alpha1.AgenticRunStep{Agent: "default"}, + }, + } + + objs := append([]client.Object{run}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...). + WithStatusSubresource(run, &agenticv1alpha1.AnalysisResult{}, &agenticv1alpha1.ExecutionResult{}, &agenticv1alpha1.VerificationResult{}, &agenticv1alpha1.EscalationResult{}).Build() + + r := &AgenticRunReconciler{Client: fc, Agent: newTestAgentCaller(), Namespace: "default"} + + if _, err := reconcileOnce(r, "fix-crash"); err != nil { + t.Fatalf("reconcile 1: %v", err) + } + approveAgenticRun(t, fc, "fix-crash") + if _, err := reconcileOnce(r, "fix-crash"); err != nil { + t.Fatalf("reconcile 2: %v", err) + } + p, err := getAgenticRun(r, "fix-crash") + if err != nil { + t.Fatalf("get completed run: %v", err) + } + if agenticv1alpha1.DerivePhase(p.Status.Conditions) != agenticv1alpha1.AgenticRunPhaseCompleted { + t.Fatalf("expected Completed, got %s", agenticv1alpha1.DerivePhase(p.Status.Conditions)) + } + + // Simulate handleTerminalTTL having already stamped terminalTime on an + // earlier reconcile of this terminal run. + staleTerminalTime := metav1.NewTime(time.Now().Add(-1 * time.Hour)) + base := p.DeepCopy() + p.Status.TerminalTime = &staleTerminalTime + if err := fc.Status().Patch(context.Background(), p, client.MergeFrom(base)); err != nil { + t.Fatalf("stamp stale terminalTime: %v", err) + } + + reviseAgenticRun(t, fc, "fix-crash", "re-analyse with different focus") + if _, err := reconcileOnce(r, "fix-crash"); err != nil { + t.Fatalf("reconcile 3 (revision from Completed): %v", err) + } + + p, err = getAgenticRun(r, "fix-crash") + if err != nil { + t.Fatalf("get revised run: %v", err) + } + if agenticv1alpha1.DerivePhase(p.Status.Conditions) != agenticv1alpha1.AgenticRunPhaseProposed { + t.Fatalf("expected Proposed after revision from Completed, got %s", agenticv1alpha1.DerivePhase(p.Status.Conditions)) + } + if p.Status.TerminalTime != nil { + t.Errorf("expected terminalTime to be cleared once revision moves run out of terminal phase, got %v", p.Status.TerminalTime) + } +} + func TestReconcile_RevisionFromFailed(t *testing.T) { agent := newTestAgentCaller() scheme := testScheme() diff --git a/controller/agenticrun/helpers.go b/controller/agenticrun/helpers.go index 104477c6..feabf3b0 100644 --- a/controller/agenticrun/helpers.go +++ b/controller/agenticrun/helpers.go @@ -83,6 +83,19 @@ func isSuspended(ctx context.Context, c client.Client) (bool, error) { return config.Spec.Suspended, nil } +// getTerminalTTL returns the cluster-wide default TTL from AgenticOLSConfig, +// or nil if no config exists or no TTL is configured. +func getTerminalTTL(ctx context.Context, c client.Client) (*int32, error) { + var config agenticv1alpha1.AgenticOLSConfig + if err := c.Get(ctx, client.ObjectKey{Name: "cluster"}, &config); err != nil { + if client.IgnoreNotFound(err) == nil { + return nil, nil + } + return nil, err + } + return config.Spec.Lifecycle.TerminalTTL, nil +} + // failStep marks a step as failed and creates a failure result CR. // The caller must have set the step condition to ConditionUnknown before // calling failStep so that conditionTime can extract the start time. diff --git a/controller/agenticrun/reconciler.go b/controller/agenticrun/reconciler.go index 3828d933..218d9135 100644 --- a/controller/agenticrun/reconciler.go +++ b/controller/agenticrun/reconciler.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "strconv" + "time" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" @@ -24,6 +25,8 @@ const ( ErrAddFinalizer = "add finalizer" ErrPatchTemplogCleanupAttempts = "patch templog cleanup attempts" ErrPatchRBACCleanupAttempts = "patch rbac cleanup attempts" + ErrStampTerminalTTL = "stamp terminal TTL" + ErrDeleteExpiredRun = "delete expired run" ) // TempLogCleaner is the interface for deleting templog records on CR deletion. @@ -106,49 +109,25 @@ func (r *AgenticRunReconciler) Reconcile(ctx context.Context, req ctrl.Request) switch phase { case agenticv1alpha1.AgenticRunPhaseNoActionRequired: if !needsRevision(&run) { - if hasSandboxClaims(&run) { - if err := r.Agent.ReleaseSandboxes(ctx, &run); err != nil { - log.Error(err, "sandbox cleanup failed at terminal phase") - } - } - if r.Audit != nil { - r.Audit.EmitTerminalSpan(ctx, &run, string(phase), terminalReason(&run)) - r.Audit.Cleanup(&run) - } - return ctrl.Result{}, nil + return r.handleTerminalCleanup(ctx, &run, phase) } case agenticv1alpha1.AgenticRunPhaseCompleted: if !(run.Spec.Execution.IsZero() && needsRevision(&run)) { - if hasSandboxClaims(&run) { - if err := r.Agent.ReleaseSandboxes(ctx, &run); err != nil { - log.Error(err, "sandbox cleanup failed at terminal phase") - } - } - if r.Audit != nil { - r.Audit.EmitTerminalSpan(ctx, &run, string(phase), terminalReason(&run)) - r.Audit.Cleanup(&run) - } - return ctrl.Result{}, nil + return r.handleTerminalCleanup(ctx, &run, phase) } case agenticv1alpha1.AgenticRunPhaseDenied, agenticv1alpha1.AgenticRunPhaseEscalated, agenticv1alpha1.AgenticRunPhaseEmergencyStopped: - if hasSandboxClaims(&run) { - if err := r.Agent.ReleaseSandboxes(ctx, &run); err != nil { - log.Error(err, "sandbox cleanup failed at terminal phase") - } - } - if r.Audit != nil { - r.Audit.EmitTerminalSpan(ctx, &run, string(phase), terminalReason(&run)) - r.Audit.Cleanup(&run) - } - return ctrl.Result{}, nil + return r.handleTerminalCleanup(ctx, &run, phase) case agenticv1alpha1.AgenticRunPhaseFailed: if !(run.Spec.Execution.IsZero() && needsRevision(&run)) { - return r.handleFailed(ctx, &run) + if result, err := r.handleFailed(ctx, &run); err != nil { + return result, err + } + return r.handleTerminalCleanup(ctx, &run, phase) } } @@ -255,9 +234,24 @@ func (r *AgenticRunReconciler) SetupWithManager(mgr ctrl.Manager) error { if err := r.List(ctx, &runs); err != nil { return nil } + // Only re-enqueue terminal runs for a missing ttlAfterTerminal stamp + // when there's actually a cluster default to stamp -- otherwise + // every terminal run with no cluster TTL configured would be + // re-enqueued on every ApprovalPolicy/AgenticOLSConfig/ConfigMap + // change forever, for no effect. + clusterTTL, err := getTerminalTTL(ctx, r.Client) + if err != nil { + clusterTTL = nil + } var reqs []ctrl.Request for _, p := range runs.Items { - if !isTerminal(agenticv1alpha1.DerivePhase(p.Status.Conditions)) { + phase := agenticv1alpha1.DerivePhase(p.Status.Conditions) + // Enqueue non-terminal runs (normal workflow), terminal runs + // still missing terminalTime (stamped unconditionally), and + // terminal runs missing ttlAfterTerminal only when a cluster + // default currently exists to stamp. + needsTTLStamp := clusterTTL != nil && p.Spec.TTLAfterTerminal == nil + if !isTerminal(phase) || p.Status.TerminalTime == nil || needsTTLStamp { reqs = append(reqs, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(&p)}) } } @@ -286,6 +280,108 @@ func (r *AgenticRunReconciler) SetupWithManager(mgr ctrl.Manager) error { Complete(r) } +// handleTerminalCleanup performs common cleanup for all terminal phases: releases +// sandbox claims, emits audit spans, and delegates to TTL handling. +func (r *AgenticRunReconciler) handleTerminalCleanup(ctx context.Context, run *agenticv1alpha1.AgenticRun, phase agenticv1alpha1.AgenticRunPhase) (ctrl.Result, error) { + log := logf.FromContext(ctx) + if hasSandboxClaims(run) { + if err := r.Agent.ReleaseSandboxes(ctx, run); err != nil { + log.Error(err, "sandbox cleanup failed at terminal phase") + } + } + if r.Audit != nil { + r.Audit.EmitTerminalSpan(ctx, run, string(phase), terminalReason(run)) + r.Audit.Cleanup(run) + } + if result, requeue, err := r.handleTerminalTTL(ctx, run); requeue || err != nil { + return result, err + } + return ctrl.Result{}, nil +} + +// handleTerminalTTL stamps terminalTime and ttlAfterTerminal on a terminal run, +// then checks whether the TTL has expired. If expired, it deletes the AgenticRun +// CR (Kubernetes GC cascades to owned resources). If not expired, it returns a +// RequeueAfter for the remaining TTL. Returns (result, requeue, error) where +// requeue=true means the caller should return the result instead of continuing. +func (r *AgenticRunReconciler) handleTerminalTTL(ctx context.Context, run *agenticv1alpha1.AgenticRun) (ctrl.Result, bool, error) { + log := logf.FromContext(ctx) + now := metav1.Now() + + // --- Stamp terminalTime if not yet set --- + if run.Status.TerminalTime == nil { + base := run.DeepCopy() + run.Status.TerminalTime = &now + if err := r.statusPatch(ctx, run, base); err != nil { + log.Error(err, "failed to stamp terminalTime") + return ctrl.Result{}, false, fmt.Errorf("%s: %w", ErrStampTerminalTTL, err) + } + } + + // --- Stamp ttlAfterTerminal from cluster config if not already set --- + if run.Spec.TTLAfterTerminal == nil { + clusterTTL, err := getTerminalTTL(ctx, r.Client) + if err != nil { + return ctrl.Result{}, false, fmt.Errorf("%s: %w", ErrStampTerminalTTL, err) + } + if clusterTTL != nil { + original := run.DeepCopy() + run.Spec.TTLAfterTerminal = clusterTTL + if err := r.Patch(ctx, run, client.MergeFrom(original)); err != nil { + log.Error(err, "failed to stamp ttlAfterTerminal") + return ctrl.Result{}, false, fmt.Errorf("%s: %w", ErrStampTerminalTTL, err) + } + } + } + + // --- Idempotent observedGeneration repair --- + // Patching spec.ttlAfterTerminal bumps metadata.generation. Advance + // the Analyzed condition's ObservedGeneration in lockstep so this + // operator-driven mutation isn't mistaken for a user-initiated + // revision request by needsRevision(). This runs unconditionally + // (outside the TTLAfterTerminal == nil block) so that a crash between + // the spec patch and this status patch is self-healing on the next + // reconcile. + if analyzed := meta.FindStatusCondition(run.Status.Conditions, agenticv1alpha1.AgenticRunConditionAnalyzed); analyzed != nil && analyzed.ObservedGeneration != run.Generation { + base := run.DeepCopy() + analyzed.ObservedGeneration = run.Generation + if err := r.statusPatch(ctx, run, base); err != nil { + log.Error(err, "failed to advance observedGeneration after ttlAfterTerminal stamp") + return ctrl.Result{}, false, fmt.Errorf("%s: %w", ErrStampTerminalTTL, err) + } + } + + // --- Evaluate TTL --- + if run.Spec.TTLAfterTerminal == nil { + // No TTL configured — no auto-deletion. + return ctrl.Result{}, false, nil + } + + ttlSeconds := *run.Spec.TTLAfterTerminal + if ttlSeconds == 0 { + // TTL=0 explicitly disables auto-deletion for this run. + return ctrl.Result{}, false, nil + } + + terminalTime := run.Status.TerminalTime.Time + expiry := terminalTime.Add(time.Duration(ttlSeconds) * time.Second) + remaining := time.Until(expiry) + + if remaining <= 0 { + log.Info("TTL expired, deleting AgenticRun", LogKeyName, run.Name) + if err := r.Delete(ctx, run); err != nil { + if client.IgnoreNotFound(err) == nil { + return ctrl.Result{}, true, nil + } + return ctrl.Result{}, false, fmt.Errorf("%s: %w", ErrDeleteExpiredRun, err) + } + return ctrl.Result{}, true, nil + } + + log.V(1).Info("TTL not yet expired, requeueing", LogKeyName, run.Name, "remaining", remaining) + return ctrl.Result{RequeueAfter: remaining}, true, nil +} + // handleTemplogCleanup deletes audit logs from the Collector's Postgres store // for this AgenticRun. Retries up to templogMaxCleanupAttempts, then removes // the finalizer regardless to unblock CR deletion. diff --git a/controller/agenticrun/ttl_test.go b/controller/agenticrun/ttl_test.go new file mode 100644 index 00000000..57e4ae9a --- /dev/null +++ b/controller/agenticrun/ttl_test.go @@ -0,0 +1,677 @@ +package agenticrun + +import ( + "context" + "testing" + "time" + + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + agenticv1alpha1 "github.com/openshift/lightspeed-agentic-operator/api/v1alpha1" +) + +// ptr32 is defined in helpers_test.go + +func TestGetTerminalTTL(t *testing.T) { + tests := []struct { + name string + objects []client.Object + want *int32 + }{ + { + name: "no config CR returns nil", + objects: nil, + want: nil, + }, + { + name: "config without lifecycle returns nil", + objects: []client.Object{&agenticv1alpha1.AgenticOLSConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: agenticv1alpha1.AgenticOLSConfigSpec{}, + }}, + want: nil, + }, + { + name: "config with terminalTTL returns value", + objects: []client.Object{&agenticv1alpha1.AgenticOLSConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: agenticv1alpha1.AgenticOLSConfigSpec{ + Lifecycle: agenticv1alpha1.LifecycleConfig{TerminalTTL: ptr32(3600)}, + }, + }}, + want: ptr32(3600), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + objects := tt.objects + if objects == nil { + objects = []client.Object{} + } + fc := fake.NewClientBuilder(). + WithScheme(testScheme()). + WithObjects(objects...). + Build() + got, err := getTerminalTTL(context.Background(), fc) + if err != nil { + t.Fatalf("getTerminalTTL() error = %v", err) + } + if (got == nil) != (tt.want == nil) { + t.Fatalf("getTerminalTTL() = %v, want %v", got, tt.want) + } + if got != nil && *got != *tt.want { + t.Fatalf("getTerminalTTL() = %d, want %d", *got, *tt.want) + } + }) + } +} + +func TestHandleTerminalTTL_StampsTerminalTimeAndTTL(t *testing.T) { + config := &agenticv1alpha1.AgenticOLSConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: agenticv1alpha1.AgenticOLSConfigSpec{ + Lifecycle: agenticv1alpha1.LifecycleConfig{TerminalTTL: ptr32(3600)}, + }, + } + + run := testAgenticRun() + run.Status.Conditions = []metav1.Condition{{ + Type: agenticv1alpha1.AgenticRunConditionVerified, + Status: metav1.ConditionTrue, + Reason: "Complete", + }} + + objs := append([]client.Object{run, config}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(objs...). + WithStatusSubresource(run).Build() + + r := &AgenticRunReconciler{Client: fc, Agent: newTestAgentCaller(), Namespace: "default"} + + result, err := reconcileOnce(r, "fix-crash") + if err != nil { + t.Fatalf("reconcile: %v", err) + } + + got, getErr := getAgenticRun(r, "fix-crash") + if getErr != nil { + t.Fatalf("getAgenticRun: %v", getErr) + } + + if got.Status.TerminalTime == nil { + t.Fatal("terminalTime should be stamped") + } + if got.Spec.TTLAfterTerminal == nil { + t.Fatal("ttlAfterTerminal should be stamped from config") + } + if *got.Spec.TTLAfterTerminal != 3600 { + t.Errorf("ttlAfterTerminal = %d, want 3600", *got.Spec.TTLAfterTerminal) + } + if result.RequeueAfter <= 0 { + t.Error("expected RequeueAfter > 0 for non-expired TTL") + } + + stampedTerminalTime := got.Status.TerminalTime.DeepCopy() + + // A second reconcile of the same still-terminal run must not refresh + // terminalTime -- otherwise expiry would perpetually postpone itself. + if _, err := reconcileOnce(r, "fix-crash"); err != nil { + t.Fatalf("second reconcile: %v", err) + } + after, getErr := getAgenticRun(r, "fix-crash") + if getErr != nil { + t.Fatalf("getAgenticRun after second reconcile: %v", getErr) + } + if after.Status.TerminalTime == nil { + t.Fatal("terminalTime should still be set after second reconcile") + } + if !after.Status.TerminalTime.Equal(stampedTerminalTime) { + t.Errorf("terminalTime changed on second reconcile: got %v, want unchanged %v", after.Status.TerminalTime, stampedTerminalTime) + } +} + +func TestHandleTerminalTTL_PresetTTLNotOverwritten(t *testing.T) { + config := &agenticv1alpha1.AgenticOLSConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: agenticv1alpha1.AgenticOLSConfigSpec{ + Lifecycle: agenticv1alpha1.LifecycleConfig{TerminalTTL: ptr32(3600)}, + }, + } + + run := testAgenticRun() + run.Spec.TTLAfterTerminal = ptr32(7200) // pre-set by adapter + run.Status.Conditions = []metav1.Condition{{ + Type: agenticv1alpha1.AgenticRunConditionVerified, + Status: metav1.ConditionTrue, + Reason: "Complete", + }} + + objs := append([]client.Object{run, config}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(objs...). + WithStatusSubresource(run).Build() + + r := &AgenticRunReconciler{Client: fc, Agent: newTestAgentCaller(), Namespace: "default"} + + _, err := reconcileOnce(r, "fix-crash") + if err != nil { + t.Fatalf("reconcile: %v", err) + } + + got, getErr := getAgenticRun(r, "fix-crash") + if getErr != nil { + t.Fatalf("getAgenticRun: %v", getErr) + } + if got.Spec.TTLAfterTerminal == nil || *got.Spec.TTLAfterTerminal != 7200 { + t.Errorf("ttlAfterTerminal = %v, want 7200 (pre-set should not be overwritten)", got.Spec.TTLAfterTerminal) + } +} + +func TestHandleTerminalTTL_ZeroDisablesAutoDeletion(t *testing.T) { + run := testAgenticRun() + run.Spec.TTLAfterTerminal = ptr32(0) // explicitly disable + now := metav1.NewTime(time.Now().Add(-1 * time.Hour)) + run.Status.TerminalTime = &now + run.Status.Conditions = []metav1.Condition{{ + Type: agenticv1alpha1.AgenticRunConditionVerified, + Status: metav1.ConditionTrue, + Reason: "Complete", + }} + + objs := append([]client.Object{run}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(objs...). + WithStatusSubresource(run).Build() + + r := &AgenticRunReconciler{Client: fc, Agent: newTestAgentCaller(), Namespace: "default"} + + result, err := reconcileOnce(r, "fix-crash") + if err != nil { + t.Fatalf("reconcile: %v", err) + } + if result.RequeueAfter != 0 { + t.Error("ttl=0 should not requeue") + } + + // Run should still exist. + got, getErr := getAgenticRun(r, "fix-crash") + if getErr != nil { + t.Fatalf("run should not be deleted when ttl=0: %v", getErr) + } + if got == nil { + t.Fatal("run should still exist when ttl=0") + } +} + +func TestHandleTerminalTTL_ExpiredRunDeleted(t *testing.T) { + run := testAgenticRun() + run.Spec.TTLAfterTerminal = ptr32(60) // 60 seconds TTL + pastTime := metav1.NewTime(time.Now().Add(-2 * time.Minute)) + run.Status.TerminalTime = &pastTime + run.Status.Conditions = []metav1.Condition{{ + Type: agenticv1alpha1.AgenticRunConditionVerified, + Status: metav1.ConditionTrue, + Reason: "Complete", + }} + + objs := append([]client.Object{run}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(objs...). + WithStatusSubresource(run).Build() + + r := &AgenticRunReconciler{Client: fc, Agent: newTestAgentCaller(), Namespace: "default"} + + _, err := reconcileOnce(r, "fix-crash") + if err != nil { + t.Fatalf("reconcile: %v", err) + } + + // Every non-deleting reconcile adds rbacCleanupFinalizer and + // templogCleanupFinalizer before reaching the terminal-phase handling + // (see the "Finalizers" block near the top of Reconcile), so by the + // time handleTerminalTTL calls Delete, the run always has finalizers. + // The fake client mirrors real Kubernetes semantics here: it keeps the + // object with DeletionTimestamp set rather than removing it outright. + // Assert that concrete outcome rather than accepting either, so a real + // regression (e.g. Delete not called at all) can't slip through as + // "acceptable." + var updated agenticv1alpha1.AgenticRun + getErr := fc.Get(context.Background(), types.NamespacedName{Name: "fix-crash", Namespace: "default"}, &updated) + if getErr != nil { + t.Fatalf("expired run should still exist with DeletionTimestamp set (finalizers present): %v", getErr) + } + if updated.DeletionTimestamp.IsZero() { + t.Fatal("expired run should have DeletionTimestamp set") + } +} + +func TestHandleTerminalTTL_NotExpiredRequeues(t *testing.T) { + run := testAgenticRun() + run.Spec.TTLAfterTerminal = ptr32(3600) // 1 hour TTL + now := metav1.NewTime(time.Now()) + run.Status.TerminalTime = &now + run.Status.Conditions = []metav1.Condition{{ + Type: agenticv1alpha1.AgenticRunConditionVerified, + Status: metav1.ConditionTrue, + Reason: "Complete", + }} + + objs := append([]client.Object{run}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(objs...). + WithStatusSubresource(run).Build() + + r := &AgenticRunReconciler{Client: fc, Agent: newTestAgentCaller(), Namespace: "default"} + + result, err := reconcileOnce(r, "fix-crash") + if err != nil { + t.Fatalf("reconcile: %v", err) + } + if result.RequeueAfter <= 0 { + t.Error("non-expired TTL should requeue with remaining time") + } + if result.RequeueAfter > 1*time.Hour { + t.Errorf("RequeueAfter = %v, should be <= 1h", result.RequeueAfter) + } + + // Run should still exist. + _, getErr := getAgenticRun(r, "fix-crash") + if getErr != nil { + t.Fatalf("non-expired run should still exist: %v", getErr) + } +} + +func TestHandleTerminalTTL_NoConfigNoAutoDeletion(t *testing.T) { + // No AgenticOLSConfig CR exists — backwards-compatible, no auto-deletion. + run := testAgenticRun() + run.Status.Conditions = []metav1.Condition{{ + Type: agenticv1alpha1.AgenticRunConditionVerified, + Status: metav1.ConditionTrue, + Reason: "Complete", + }} + + objs := append([]client.Object{run}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(objs...). + WithStatusSubresource(run).Build() + + r := &AgenticRunReconciler{Client: fc, Agent: newTestAgentCaller(), Namespace: "default"} + + result, err := reconcileOnce(r, "fix-crash") + if err != nil { + t.Fatalf("reconcile: %v", err) + } + if result.RequeueAfter != 0 || result.Requeue { + t.Error("no config should not cause requeue for TTL") + } + + // Run should still exist and have terminalTime stamped but no ttlAfterTerminal. + got, getErr := getAgenticRun(r, "fix-crash") + if getErr != nil { + t.Fatalf("getAgenticRun: %v", getErr) + } + if got.Status.TerminalTime == nil { + t.Error("terminalTime should still be stamped") + } + if got.Spec.TTLAfterTerminal != nil { + t.Errorf("ttlAfterTerminal should be nil when no config, got %d", *got.Spec.TTLAfterTerminal) + } +} + +func TestHandleTerminalTTL_DeniedPhase(t *testing.T) { + config := &agenticv1alpha1.AgenticOLSConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: agenticv1alpha1.AgenticOLSConfigSpec{ + Lifecycle: agenticv1alpha1.LifecycleConfig{TerminalTTL: ptr32(60)}, + }, + } + + run := testAgenticRun() + run.Status.Conditions = []metav1.Condition{ + {Type: agenticv1alpha1.AgenticRunConditionAnalyzed, Status: metav1.ConditionTrue, Reason: "AnalysisComplete"}, + {Type: agenticv1alpha1.AgenticRunConditionDenied, Status: metav1.ConditionTrue, Reason: "UserDenied"}, + } + + objs := append([]client.Object{run, config}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(objs...). + WithStatusSubresource(run).Build() + + r := &AgenticRunReconciler{Client: fc, Agent: newTestAgentCaller(), Namespace: "default"} + + result, err := reconcileOnce(r, "fix-crash") + if err != nil { + t.Fatalf("reconcile: %v", err) + } + + got, getErr := getAgenticRun(r, "fix-crash") + if getErr != nil { + t.Fatalf("getAgenticRun: %v", getErr) + } + if got.Status.TerminalTime == nil { + t.Fatal("Denied run should have terminalTime stamped") + } + if got.Spec.TTLAfterTerminal == nil || *got.Spec.TTLAfterTerminal != 60 { + t.Errorf("ttlAfterTerminal should be 60 for Denied run, got %v", got.Spec.TTLAfterTerminal) + } + if result.RequeueAfter <= 0 { + t.Error("expected RequeueAfter > 0 for non-expired Denied run") + } +} + +func TestHandleTerminalTTL_FailedPhase(t *testing.T) { + config := &agenticv1alpha1.AgenticOLSConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: agenticv1alpha1.AgenticOLSConfigSpec{ + Lifecycle: agenticv1alpha1.LifecycleConfig{TerminalTTL: ptr32(120)}, + }, + } + + run := testAgenticRun() + run.Status.Conditions = []metav1.Condition{ + {Type: agenticv1alpha1.AgenticRunConditionAnalyzed, Status: metav1.ConditionFalse, Reason: "Failed", Message: "analysis error"}, + } + + objs := append([]client.Object{run, config}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(objs...). + WithStatusSubresource(run).Build() + + r := &AgenticRunReconciler{Client: fc, Agent: newTestAgentCaller(), Namespace: "default"} + + result, err := reconcileOnce(r, "fix-crash") + if err != nil { + t.Fatalf("reconcile: %v", err) + } + + got, getErr := getAgenticRun(r, "fix-crash") + if getErr != nil { + t.Fatalf("getAgenticRun: %v", getErr) + } + if got.Status.TerminalTime == nil { + t.Fatal("Failed run should have terminalTime stamped") + } + if result.RequeueAfter <= 0 { + t.Error("expected RequeueAfter > 0 for non-expired Failed run") + } +} + +// TestHandleTerminalTTL_StampSyncsObservedGeneration guards against a +// regression where stamping ttlAfterTerminal (a spec write) bumps +// metadata.generation — on a real cluster, every accepted spec Patch does +// this — while needsRevision() treats any generation > Analyzed. +// observedGeneration as "revision requested." Since spec.revisionFeedback is +// never cleared once processed, an internal TTL-stamping generation bump +// left unsynced would spuriously re-arm the revision workflow later. +// +// The fake client used here (controller-runtime v0.23) does not simulate +// apiserver-side generation incrementing on Patch, so this test seeds an +// already-elevated Generation directly (as if a prior spec write, including +// a pre-fix ttlAfterTerminal stamp, had already bumped it) to exercise the +// sync logic in handleTerminalTTL deterministically. +func TestHandleTerminalTTL_StampSyncsObservedGeneration(t *testing.T) { + config := &agenticv1alpha1.AgenticOLSConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: agenticv1alpha1.AgenticOLSConfigSpec{ + Lifecycle: agenticv1alpha1.LifecycleConfig{TerminalTTL: ptr32(3600)}, + }, + } + + run := testAgenticRun() + run.Generation = 5 + // Stale feedback from a revision that was already fully processed; + // RevisionFeedback is never cleared by the controller. + run.Spec.RevisionFeedback = "please double check the fix" + run.Status.Conditions = []metav1.Condition{ + { + Type: agenticv1alpha1.AgenticRunConditionAnalyzed, + Status: metav1.ConditionTrue, + Reason: "Complete", + ObservedGeneration: 3, // stale relative to Generation=5 + }, + {Type: agenticv1alpha1.AgenticRunConditionVerified, Status: metav1.ConditionTrue, Reason: "Complete"}, + } + + objs := append([]client.Object{run, config}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(objs...). + WithStatusSubresource(run).Build() + + r := &AgenticRunReconciler{Client: fc, Agent: newTestAgentCaller(), Namespace: "default"} + + before, err := getAgenticRun(r, "fix-crash") + if err != nil { + t.Fatalf("get: %v", err) + } + if !needsRevision(before) { + t.Fatal("test setup invalid: expected needsRevision() true before the TTL stamp corrects observedGeneration") + } + + if _, _, err := r.handleTerminalTTL(context.Background(), before); err != nil { + t.Fatalf("handleTerminalTTL: %v", err) + } + + after, err := getAgenticRun(r, "fix-crash") + if err != nil { + t.Fatalf("get: %v", err) + } + if after.Spec.TTLAfterTerminal == nil || *after.Spec.TTLAfterTerminal != 3600 { + t.Fatalf("ttlAfterTerminal not stamped: %v", after.Spec.TTLAfterTerminal) + } + analyzed := meta.FindStatusCondition(after.Status.Conditions, agenticv1alpha1.AgenticRunConditionAnalyzed) + if analyzed == nil || analyzed.ObservedGeneration != after.Generation { + t.Fatalf("Analyzed.observedGeneration = %v, want %d (current generation)", analyzed, after.Generation) + } + if needsRevision(after) { + t.Error("stamping ttlAfterTerminal must not leave stale revisionFeedback able to spuriously re-trigger needsRevision()") + } +} + +// TestNoActionRequired_TTLStampDoesNotReTriggerRevision is a regression test +// ensuring that stamping ttlAfterTerminal on a NoActionRequired terminal run +// (advisory-only, analysis determined no action needed) does not cause it to +// re-enter analysis via needsRevision, even when the run carries stale +// revisionFeedback from a prior revision. +func TestNoActionRequired_TTLStampDoesNotReTriggerRevision(t *testing.T) { + config := &agenticv1alpha1.AgenticOLSConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: agenticv1alpha1.AgenticOLSConfigSpec{ + Lifecycle: agenticv1alpha1.LifecycleConfig{TerminalTTL: ptr32(7200)}, + }, + } + + run := testAgenticRun() + run.Generation = 4 + run.Spec.RevisionFeedback = "previous feedback from revision attempt" + run.Status.Conditions = []metav1.Condition{ + { + Type: agenticv1alpha1.AgenticRunConditionAnalyzed, + Status: metav1.ConditionTrue, + Reason: agenticv1alpha1.ReasonNoActionRequired, + Message: "Analysis determined no remediation action is required", + ObservedGeneration: 2, // stale relative to current Generation=4 + }, + } + + objs := append([]client.Object{run, config}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(objs...). + WithStatusSubresource(run).Build() + + r := &AgenticRunReconciler{Client: fc, Agent: newTestAgentCaller(), Namespace: "default"} + + before, err := getAgenticRun(r, "fix-crash") + if err != nil { + t.Fatalf("get before: %v", err) + } + if !needsRevision(before) { + t.Fatal("test setup invalid: NoActionRequired run with stale revisionFeedback should report needsRevision=true before TTL sync") + } + + if _, _, err := r.handleTerminalTTL(context.Background(), before); err != nil { + t.Fatalf("handleTerminalTTL: %v", err) + } + + after, err := getAgenticRun(r, "fix-crash") + if err != nil { + t.Fatalf("get after: %v", err) + } + + // Verify TTL was stamped + if after.Spec.TTLAfterTerminal == nil || *after.Spec.TTLAfterTerminal != 7200 { + t.Fatalf("ttlAfterTerminal not stamped: %v", after.Spec.TTLAfterTerminal) + } + + // Verify Analyzed condition's ObservedGeneration was synced + analyzed := meta.FindStatusCondition(after.Status.Conditions, agenticv1alpha1.AgenticRunConditionAnalyzed) + if analyzed == nil || analyzed.ObservedGeneration != after.Generation { + t.Fatalf("Analyzed.observedGeneration = %v, want %d (post-stamp generation)", analyzed, after.Generation) + } + + // Critical assertion: needsRevision must now return false because the TTL stamp + // synced the generation mismatch. NoActionRequired terminal runs must not re-enter + // analysis due to internal TTL stamping, even with stale revisionFeedback present. + if needsRevision(after) { + t.Error("NoActionRequired terminal run must not re-enter analysis due to TTL stamping; observedGeneration sync failed") + } +} + +// TestAdvisoryCompleted_TTLStampDoesNotReTriggerRevision is a regression test +// for advisory-only Completed runs (execution omitted, no verification, analysis +// success). Stamping ttlAfterTerminal must not cause re-entry into analysis. +func TestAdvisoryCompleted_TTLStampDoesNotReTriggerRevision(t *testing.T) { + config := &agenticv1alpha1.AgenticOLSConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: agenticv1alpha1.AgenticOLSConfigSpec{ + Lifecycle: agenticv1alpha1.LifecycleConfig{TerminalTTL: ptr32(3600)}, + }, + } + + run := testAgenticRun() + // Advisory-only: no execution or verification steps configured. + run.Spec.Execution = agenticv1alpha1.AgenticRunStep{} + run.Spec.Verification = agenticv1alpha1.AgenticRunStep{} + run.Generation = 3 + run.Spec.RevisionFeedback = "stale feedback" + run.Status.Conditions = []metav1.Condition{ + { + Type: agenticv1alpha1.AgenticRunConditionAnalyzed, + Status: metav1.ConditionTrue, + Reason: "Complete", + Message: "Analysis succeeded", + ObservedGeneration: 1, // stale relative to Generation=3 + }, + { + Type: agenticv1alpha1.AgenticRunConditionExecuted, + Status: metav1.ConditionTrue, + Reason: "Skipped", + }, + { + Type: agenticv1alpha1.AgenticRunConditionVerified, + Status: metav1.ConditionTrue, + Reason: "Skipped", + }, + } + + objs := append([]client.Object{run, config}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(objs...). + WithStatusSubresource(run).Build() + + r := &AgenticRunReconciler{Client: fc, Agent: newTestAgentCaller(), Namespace: "default"} + + before, err := getAgenticRun(r, "fix-crash") + if err != nil { + t.Fatalf("get before: %v", err) + } + if !needsRevision(before) { + t.Fatal("test setup invalid: advisory Completed run with stale revisionFeedback should report needsRevision=true before TTL sync") + } + + if _, _, err := r.handleTerminalTTL(context.Background(), before); err != nil { + t.Fatalf("handleTerminalTTL: %v", err) + } + + after, err := getAgenticRun(r, "fix-crash") + if err != nil { + t.Fatalf("get after: %v", err) + } + + // Verify TTL was stamped + if after.Spec.TTLAfterTerminal == nil || *after.Spec.TTLAfterTerminal != 3600 { + t.Fatalf("ttlAfterTerminal not stamped: %v", after.Spec.TTLAfterTerminal) + } + + // Verify Analyzed condition's ObservedGeneration was synced + analyzed := meta.FindStatusCondition(after.Status.Conditions, agenticv1alpha1.AgenticRunConditionAnalyzed) + if analyzed == nil || analyzed.ObservedGeneration != after.Generation { + t.Fatalf("Analyzed.observedGeneration = %v, want %d (post-stamp generation)", analyzed, after.Generation) + } + + // Critical assertion: advisory-only Completed runs must not re-enter analysis + // due to internal TTL stamping, even with stale revisionFeedback. + if needsRevision(after) { + t.Error("advisory-only Completed run must not re-enter analysis due to TTL stamping; observedGeneration sync failed") + } +} + +// TestExecutionlessFailed_TTLStampDoesNotReTriggerRevision is a regression test +// for execution-less Failed runs (analysis failed, no execution/verification). +// Stamping ttlAfterTerminal must not cause re-entry into analysis. +func TestExecutionlessFailed_TTLStampDoesNotReTriggerRevision(t *testing.T) { + config := &agenticv1alpha1.AgenticOLSConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Spec: agenticv1alpha1.AgenticOLSConfigSpec{ + Lifecycle: agenticv1alpha1.LifecycleConfig{TerminalTTL: ptr32(1800)}, + }, + } + + run := testAgenticRun() + // Execution-less: no execution or verification steps configured. + run.Spec.Execution = agenticv1alpha1.AgenticRunStep{} + run.Spec.Verification = agenticv1alpha1.AgenticRunStep{} + run.Generation = 2 + run.Spec.RevisionFeedback = "stale feedback from prior revision" + run.Status.Conditions = []metav1.Condition{ + { + Type: agenticv1alpha1.AgenticRunConditionAnalyzed, + Status: metav1.ConditionFalse, + Reason: "Failed", + Message: "Analysis agent reported failure", + ObservedGeneration: 1, // stale relative to Generation=2 + }, + } + + objs := append([]client.Object{run, config}, defaultObjects()...) + fc := fake.NewClientBuilder().WithScheme(testScheme()).WithObjects(objs...). + WithStatusSubresource(run).Build() + + r := &AgenticRunReconciler{Client: fc, Agent: newTestAgentCaller(), Namespace: "default"} + + before, err := getAgenticRun(r, "fix-crash") + if err != nil { + t.Fatalf("get before: %v", err) + } + if !needsRevision(before) { + t.Fatal("test setup invalid: execution-less Failed run with stale revisionFeedback should report needsRevision=true before TTL sync") + } + + if _, _, err := r.handleTerminalTTL(context.Background(), before); err != nil { + t.Fatalf("handleTerminalTTL: %v", err) + } + + after, err := getAgenticRun(r, "fix-crash") + if err != nil { + t.Fatalf("get after: %v", err) + } + + // Verify TTL was stamped + if after.Spec.TTLAfterTerminal == nil || *after.Spec.TTLAfterTerminal != 1800 { + t.Fatalf("ttlAfterTerminal not stamped: %v", after.Spec.TTLAfterTerminal) + } + + // Verify Analyzed condition's ObservedGeneration was synced + analyzed := meta.FindStatusCondition(after.Status.Conditions, agenticv1alpha1.AgenticRunConditionAnalyzed) + if analyzed == nil || analyzed.ObservedGeneration != after.Generation { + t.Fatalf("Analyzed.observedGeneration = %v, want %d (post-stamp generation)", analyzed, after.Generation) + } + + // Critical assertion: execution-less Failed runs must not re-enter analysis + // due to internal TTL stamping, even with stale revisionFeedback. + if needsRevision(after) { + t.Error("execution-less Failed run must not re-enter analysis due to TTL stamping; observedGeneration sync failed") + } +}