Repository navigation
feat(storage): 30-day partner-file retention job + deletion audit - #3733
PetrefiedThunder wants to merge 8 commits into
Conversation
Scheduler IntervalTrigger job selects partner_files past delete_after, deletes the partner-uploads object, appends a hash-chained partner_file_deletion_log row (actor=retention_job, reason=retention_30d), marks the metadata row deleted, and emits partner_file.retention_delete audit. Synthetic unit tests only; no migration apply. Signed-off-by: Christopher <131207452+PetrefiedThunder@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review: 1
Please check the settings in the CodeRabbit UI or the
 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 635a7f9339
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| def run_retention_pass(db, *, limit: int = BATCH_LIMIT) -> Dict[str, int]: | ||
| """Process up to ``limit`` due partner files. Returns counts.""" | ||
| due = _due_rows(db, limit=limit) |
There was a problem hiding this comment.
In the production scheduler, SessionLocal() uses the ordinary RLS-enforced role, and the transaction begin hook clears app.tenant_id. This query runs before any tenant context is set, but tenant_isolation_partner_files in supabase/migrations/20261028100000_partner_files_storage_retention.sql requires get_tenant_context(). Consequently, the initial selection cannot discover due files across tenants, and retention never reaches delete_one_due_file. Discover tenants with the existing maintenance helper and select due rows under each tenant's context, as drain_all_tenants does.
AGENTS.md reference: AGENTS.md:L54-L56
Useful? React with 👍 / 👎.
| if is_object_storage_configured(): | ||
| delete_object(row["storage_bucket"] or PARTNER_UPLOADS_BUCKET, row["storage_path"]) |
There was a problem hiding this comment.
If storage credentials are absent while the database contains previously uploaded objects, this condition skips the actual deletion but still appends a successful deletion log and marks the file deleted. Subsequent passes exclude that metadata row, so restoring credentials will not retry the deletion: the partner payload remains in storage indefinitely while the audit evidence claims otherwise. Treat unconfigured storage as a retryable failure and leave the file eligible for retention.
AGENTS.md reference: AGENTS.md:L56-L56
Useful? React with 👍 / 👎.
| try: | ||
| delete_one_due_file(db, row) | ||
| deleted += 1 | ||
| except Exception as exc: # pragma: no cover — per-row isolation | ||
| errors += 1 |
There was a problem hiding this comment.
If a deletion-log INSERT or metadata UPDATE raises a PostgreSQL error after the storage object has been deleted, catching the exception does not recover the transaction. Every subsequent row then fails in the aborted transaction, and the scheduler's single final commit cannot persist earlier successful rows either. This can leave multiple permanently deleted objects without their deletion evidence. Use per-file transaction boundaries or savepoints with rollback on failure so one database error cannot discard the rest of the batch's logs and metadata.
AGENTS.md reference: AGENTS.md:L56-L56
Useful? React with 👍 / 👎.
|
REVIEW: FAIL claude 635a7f9 Unit tests at this SHA: Timing (no early delete) — OK
Blocking
Nonblocking
Re-review after RLS-aware due-row selection, fail-closed when storage is required, per-row savepoints (or claim-before-delete), and chain/tip locking. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Claude FAIL #3733: select due rows per tenant after set_tenant_guc; no-op (no deletes/audits) when object storage is unconfigured; per-row SAVEPOINT; advisory xact lock + FOR UPDATE on chain tip and file row; S3 delete then durable DB update. Synthetic tests only. Signed-off-by: Christopher <131207452+PetrefiedThunder@users.noreply.github.com>
|
REVIEW: PASS claude 7842e70 Delta from FAIL at 635a7f9. Heads unchanged at post time. Unit tests: cutover + Prior blockers — resolved
S3-before-DB retry (checked)Order: claim file → S3
Nonblocking
No merge from me. |
PR Blocker ChecklistHead SHA: Status: Blocked (7 open blockers)Review Threads (6 unresolved)
Changes Requested (0)
Blocked Engines (1)
|
Bring #3746 (PR Gate Frontend rerun) onto the stacked retention branch. No rebase.
regengine has SELECT+INSERT only on partner_file_deletion_log (UPDATE revoked). Tip serialization already comes from pg_advisory_xact_lock. Also discover tenants via list_maintenance_tenant_ids (sysadmin) instead of a FORCE-RLS public.tenants SELECT that returned empty under the runtime role. Signed-off-by: VS Code Hermes <hermes@regengine.local> Signed-off-by: Christopher Sellers <1@christophersellers.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a8cc6d1. Configure here.
| } | ||
| if (!runCompleted) { | ||
| core.setFailed(`PR Gate run ${latest.id} did not complete within 90 minutes; not re-running.`); | ||
| return; |
There was a problem hiding this comment.
Rerun wait shorter than backend gate
High Severity
The rerun job now waits for the whole PR Gate run to finish, but the wait deadline is 90 minutes while PR Gate: Backend can run for 150 minutes. When Frontend CI finishes first, this workflow gives up and never calls reRunWorkflowFailedJobs, so the timed-out Frontend gate stays red.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a8cc6d1. Configure here.
| row.get("tenant_id"), | ||
| exc, | ||
| ) | ||
| remaining -= 1 |
There was a problem hiding this comment.
Tenant error rolls back deletions
Medium Severity
Only per-row work is savepoint-isolated. A failure in set_tenant_guc or _due_rows_for_tenant for a later tenant aborts run_retention_pass before db.commit(), rolling back every earlier tenant's log, metadata, and audit while the S3 objects are already gone.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a8cc6d1. Configure here.
| "actor": ACTOR, | ||
| "reason": REASON, | ||
| } | ||
| entry_hash = compute_entry_hash(prev_entry_hash=prev, fields=fields) |
There was a problem hiding this comment.
Deletion hash datetime serialization mismatch
Medium Severity
compute_entry_hash serializes report_delivered_at via json.dumps default=str (space separator) but hashes deleted_at as isoformat() (T separator). A verifier reading both timestamps from partner_file_deletion_log cannot apply one datetime encoding and recompute entry_hash.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a8cc6d1. Configure here.
|
REVIEW: PASS claude a8cc6d1 Full review of hermes/partner-file-retention-job (30-day partner-file retention), stacked on #3732; tip ahead of main by 13, behind 0. (1) FOR UPDATE removed on append-only partner_file_deletion_log tip. (2) Tenant discovery uses (3) Due predicate: (4) Box tests: Soft (non-blocking): |
|
This PR is stacked on #3732. I merged main and the updated Local checks (Python 3.11): The earlier Engine Review Pass failure was fixed on main in #3756. New head SHA: |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: d50dbd7e-6b1b-450d-a61d-aff5f2323444) |
Summary
Stacked on #3732 (cutover). Adds the 30-day deletion job driven by
partner_files.delete_after: deletes thepartner-uploadsobject, appends hash-chainedpartner_file_deletion_log(actor=retention_job,reason=retention_30d), marks metadata deleted, and auditspartner_file.retention_delete. Scheduled hourly viaIntervalTrigger(same pattern as remediation drain).Touched
services/ingestion/app/partner_file_retention.py(new)services/ingestion/app/s3_utils.py(delete_object)services/scheduler/app/config.py+services/scheduler/service.pyOut of scope / rules
Test plan
pytest services/ingestion/tests/test_partner_file_retention_job.py(3 passed)Note
Medium Risk
Permanent S3 deletion and metadata mutation are irreversible, but the job gates on configured storage, uses row locking and per-row savepoints, and writes an auditable hash chain before marking files deleted.
Overview
Adds a scheduled retention pass that enforces the 30-day clock on
partner_files.delete_after: remove the object from partner uploads storage, append a hash-chained row topartner_file_deletion_log(actor=retention_job,reason=retention_30d), set the file metadata to deleted, and emitpartner_file.retention_deleteaudit events.The ingestion module scans tenants via maintenance tenant discovery, sets the tenant GUC for RLS, claims due rows with
FOR UPDATE SKIP LOCKED, and runs each delete inside a savepoint so one failure does not abort the batch. Deletion-log tips are serialized per tenant withpg_advisory_xact_lock(noFOR UPDATEon the append-only log).delete_objectis added tos3_utilsand treats missing keys as success for idempotent reruns; if object storage is not configured, the job no-ops (batch) or refuses to mark rows deleted (single-file).The scheduler registers
partner_file_retentionon an hourly interval (configurable viapartner_file_retention_interval_seconds), matching the remediation outbox drain pattern. Synthetic unit tests cover hashing, audit/log ordering, unconfigured storage, and hash chaining.Reviewed by Cursor Bugbot for commit 4211255. Bugbot is set up for automated code reviews on this repo. Configure here.