Repository navigation
feat(retention): add workflow execution and log cleanup maintenance script (#6942) - #6959
granbellllc-png wants to merge 3 commits into
Conversation
… validate_scopes (keephq#6957)
…cript (keephq#6942) Addresses keephq#6942 by providing a supported database maintenance script to purge historical workflow executions and logs: - Bounded batch deletion chunks to prevent lock contention and WAL growth - Configurable retention window (--retention-days) and batch size (--batch-size) - Safe dry-run mode (--dry-run) to inspect purge counts without committing - Tenant isolation (--tenant-id) and error preservation (--keep-failed) - Fully covered by unit tests verifying isolation of active/fresh runs
|
Alex Granbell seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
528d9f4 to
8ab1c6c
Compare
Danny5487401
left a comment
There was a problem hiding this comment.
Thanks for implementing a concrete solution for #6942. A standalone maintenance script is a useful starting point. I reviewed commit 8ab1c6c and found four issues to address before using it against an existing database.
Validation scope: these reproductions used the upstream models and get_session implementation in an isolated harness with SQLModel 0.0.22 and SQLite, with foreign keys enabled for the deletion case. These are not PostgreSQL integration or performance test results.
-
The CLI entry point fails before cleanup, including with
--dry-run(L120).get_session()is a generator dependency, not a context manager. Callingmain()raises:TypeError: 'generator' object does not support the context manager protocolPlease use a properly managed synchronous session, such as
with get_session_sync() as session:, and add a CLI entry-point test. -
Execution associations are not handled before deleting the parent (L90–L99).
WorkflowToAlertExecutionandWorkflowToIncidentExecutionalso referenceWorkflowExecution. Deleting an associated execution raised:IntegrityError: NOT NULL constraint failed: workflowtoalertexecution.workflow_execution_idPlease handle both association tables before deleting the execution, preserving the alert/incident records themselves. Verify the full dependency chain in PostgreSQL with actual constraints enabled.
-
Dry-run reports only the first batch (L72–L79).
With five eligible executions and
batch_size=2, it reports two and exits. This understates the proposed deletion scope.Please report complete eligible execution/dependency counts, or explicitly label a bounded preview as partial. Prefer aggregate counts over loading every log ID, and test a dataset larger than one batch.
-
Invalid retention values can delete fresh history (L113–L114).
retention_days=-1moves the cutoff into the future; the reproduction deleted a newly created successful execution.Please validate retention days and batch size before database access. Consider requiring an explicit apply flag and an explicit tenant/all-tenants choice.
Additional suggestions:
- Bound memory and transaction work. The code loads all logs for the selected executions, followed by complete execution objects containing potentially large
resultsJSON. Limiting parent executions does not bound log volume. Prefer ID-based operations, a separate log-row budget, and a runtime limit. - Add PostgreSQL tests. Cover associations, tenant isolation, multiple batches, invalid arguments, rollback/retry, and terminal statuses. Check
providers_not_configured, which is terminal in Keep 0.55.0 but absent from the current lists. - Document checkpoint behavior accurately. Default 30-day retention does not remove recent successful results used by normally running incremental workflows. Preserving the latest non-test success and scheduler-relevant completed record would still retain recovery evidence. The existing one-day history lookup limit remains a separate limitation.
- Document disk-space expectations. DELETE plus ordinary VACUUM generally makes space reusable inside PostgreSQL rather than immediately reducing filesystem usage. VACUUM FULL requires separate space and downtime planning.
I suggest keeping the first iteration focused on correctness fixes, bounded deletion, and PostgreSQL tests. Separate retention windows and scheduling can follow.
|
Following up on the review, here is the concrete implementation plan I will work on for #6942, referencing this PR. Read-only inspection of our self-hosted PostgreSQL 16 deployment found that execution history accounts for approximately 97.6% of database storage: Proposed first iteration:
For our deployment, success history of 14 days and failure history of 30 days, with protected executions excluded, currently covers about 532k executions and 13.55 GiB of stored results. This is not a promise of immediately reclaimed filesystem space and excludes log volume; retention remains configurable. I will keep reducing newly persisted intermediate step results as a separate follow-up: Keep 0.55.0 currently saves both action and step outputs, while incremental workflows still need their successful checkpoint action results. That optimization should not be mixed into the first cleanup change or truncate checkpoints. I will submit the implementation as a linked follow-up PR. No live database cleanup or index creation has been performed. |
…, full dry-run counts, and input validation (keephq#6942)
|
Thank you @Danny5487401 for the thorough review and for sharing the production PostgreSQL 16 storage telemetry ( I have pushed commit
|
8ab1c6c to
db31adc
Compare
Fixes #6942
Description
This PR addresses #6942 by providing a dedicated maintenance script to purge historical completed workflow executions and execution logs, preventing PVC disk exhaustion on self-hosted PostgreSQL deployments.
Problem
As reported in #6942, workflow execution history and step output TOAST data can exhaust the database storage PVC over time.
Solution
scripts/cleanup_workflow_executions.py:--retention-days: purge terminal workflow runs older than N days (default: 30)--batch-size: chunked deletes (default: 500) to avoid table lock contention and WAL surges--dry-run: non-destructive inspection mode--tenant-id: per-tenant scope filter--keep-failed: retain failure/timeout executions for auditWorkflowExecutionLogentries before deletingWorkflowExecutiontests/test_cleanup_workflow_executions.pycovering dry-run, actual purge, error preservation, and active execution isolation (3/3 passed).