feat(admin): configurable retention policy for retained files-import archives - #1001
feat(admin): configurable retention policy for retained files-import archives#1001devin-ai-integration[bot] wants to merge 2 commits into
Conversation
…archives Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| try: | ||
| await delete_files_import_archive(session, archive_task_id) | ||
| except (LookupError, FileNotFoundError, ValueError, RuntimeError, OSError): | ||
| # In use by an active import, already gone, or not deletable — | ||
| # keep it and move on. | ||
| kept += 1 | ||
| continue |
There was a problem hiding this comment.
📝 Info: Undeletable (active/in-use) over-count archives still consume a keep slot
When delete_files_import_archive raises (e.g. RuntimeError because the archive is referenced by an active import), the loop does kept += 1 (backend/app/admin_ops.py:700-704). This means an archive that could not be deleted counts toward kept, which is a conservative choice: it avoids deleting an extra deletable archive to compensate, so the effective retained count can temporarily exceed retention_count when some archives are locked. This is reasonable behavior and matches test_enforce_retention_keeps_archives_in_active_use, but reviewers should be aware that the count limit is a soft bound under active-use contention.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # Apply the retained files-import archive retention policy so age-based | ||
| # limits take effect even when no new import runs (no-op unless the | ||
| # FILES_IMPORT_ARCHIVE_RETENTION_* settings opt in). | ||
| try: | ||
| async with get_async_session()() as session: | ||
| await enforce_files_import_archive_retention(session) | ||
| except Exception as exc: # pragma: no cover - best effort on startup | ||
| logger.warning( | ||
| "Files-import archive retention enforcement failed: %s", | ||
| exc, | ||
| extra={ | ||
| "event": "admin_task.archive_retention_failed", | ||
| "error": str(exc), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
📝 Info: Startup retention enforcement is ordered after stale-task reconciliation, preserving active-import protection
In backend/app/main.py:184-198 the startup retention pass runs after reconcile_stale_tasks (line 172-176). This ordering matters: delete_files_import_archive refuses to delete archives referenced by tasks in ACTIVE_TASK_STATUSES, and a genuinely-active sibling-pod import keeps its task active (freshness on updated_at), so startup pruning will skip it via the RuntimeError→keep path. Multi-replica races where two pods delete the same file are absorbed by the FileNotFoundError/OSError branch. This all holds up; noting it because the safety of startup pruning depends on these interactions rather than being locally obvious.
Was this helpful? React with 👍 or 👎 to provide feedback.
…e semantics Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| for archive in await list_files_import_archives(session): | ||
| archive_task_id = archive["archive_task_id"] | ||
| assert isinstance(archive_task_id, int) | ||
| task = await session.get(AdminTask, archive_task_id) | ||
| if task is None or not task.input_path or task.input_path in seen_paths: | ||
| continue | ||
| seen_paths.add(task.input_path) | ||
|
|
||
| over_count = count_limit > 0 and kept >= count_limit | ||
| created_at = archive["created_at"] | ||
| assert isinstance(created_at, datetime) | ||
| if created_at.tzinfo is None: | ||
| created_at = created_at.replace(tzinfo=timezone.utc) | ||
| too_old = cutoff is not None and created_at < cutoff | ||
| if not (over_count or too_old): | ||
| kept += 1 | ||
| continue |
There was a problem hiding this comment.
📝 Info: Count-based retention can evict a genuinely newer archive when an older one is rerun
enforce_files_import_archive_retention measures recency by AdminTask.id (list_files_import_archives orders by id.desc()), and dedupes reruns to the highest-id task per input_path (backend/app/admin_ops.py:680-686). This means re-running an old retained archive creates a new higher-id task that bumps that archive to the 'newest' slot, so under a count-based policy a genuinely newer distinct archive can be deleted in favor of the reused-but-older file. This matches the documented intent ('an archive that is actively being reused is treated as fresh', docs/admin-import-export.md:205-210), so it is not a bug, but operators should be aware that reruns influence count-based eviction ordering, not just age.
Was this helpful? React with 👍 or 👎 to provide feedback.
| try: | ||
| await delete_files_import_archive(session, archive_task_id) | ||
| except (LookupError, FileNotFoundError, ValueError, RuntimeError, OSError): | ||
| # In use by an active import, already gone, or not deletable — | ||
| # keep it and move on. | ||
| kept += 1 | ||
| continue | ||
| deleted.append(archive_task_id) |
There was a problem hiding this comment.
📝 Info: Retention deletion leaves stale input_path on the task row
delete_files_import_archive only os.unlinks the file and never clears AdminTask.input_path (backend/app/admin_ops.py:665-670). After retention prunes an archive, the task row still points at the now-deleted file. This is safe because list_files_import_archives re-validates each path via _validate_retained_files_import_archive_path and skips missing files, so pruned archives will not reappear or be re-deleted on subsequent runs. Noted only because it is pre-existing behavior now exercised automatically rather than solely on manual delete.
Was this helpful? React with 👍 or 👎 to provide feedback.
| try: | ||
| pruned = await enforce_files_import_archive_retention(session) | ||
| if pruned: | ||
| await _update_task( | ||
| session, task, | ||
| log_line=( | ||
| "Retention policy removed " | ||
| f"{len(pruned)} older retained archive(s)." | ||
| ), | ||
| ) | ||
| except Exception: | ||
| logger.exception( | ||
| "Retention enforcement after files import failed", | ||
| ) |
There was a problem hiding this comment.
📝 Info: Retention runs on the import task's own session and is fully guarded
In run_files_import, retention enforcement is invoked with the same session used to mark the task completed, wrapped in its own try/except that logs and swallows any exception (backend/app/admin_ops.py:2568-2581). This correctly prevents a retention failure from flipping a successful import to failed. delete_files_import_archive performs only reads plus os.unlink (no uncommitted DB writes), and the only commit in the retention path is the optional log-line _update_task, so a no-op or failed enforcement leaves the session in a clean/read-only state before the context manager closes.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Closes #839 (follow-up to PR #838). Retained filesystem-import archives could grow unbounded on the
/datavolume; this adds an opt-in, env-driven retention policy.0= retain indefinitely, preserving current behavior):FILES_IMPORT_ARCHIVE_RETENTION_COUNT— keep only the newest N distinct archivesFILES_IMPORT_ARCHIVE_RETENTION_DAYS— delete archives older than N daysenforce_files_import_archive_retention(session)inadmin_ops.py:list_files_import_archives(newest first), dedupes rerun tasks sharing oneinput_path, deletes anything over-count or over-agedelete_files_import_archive, so archives referenced by an active files import are never removed (RuntimeError → keep and continue)run_files_import(appends a "Retention policy removed N older retained archive(s)." log line) and once at backend startup (best-effort, so age-based limits apply without a new import).GET /api/admin/tasks/files-import/archive-retentionreturns{retention_count, retention_days}; the admin UI shows an info alert next to the cumulative-usage summary whenever a non-zero policy is active.docs/admin-import-export.md(new "Automatic retention policy" section) and.agents/skills/hriv-admin-operations/SKILL.mdupdated additively.Testing
poetry run pytest— 934 passed (new unit tests for policy defaults, count/age enforcement, rerun dedupe, active-archive skip, and the new endpoint).npm test— 1323 passed (AdminPage tests for the retention alert shown/hidden).npm run lintreports only the pre-existingCanvasOverlay.tsxreact-refresh error;npm run format:checkandnpm run buildpass.Part of backlog Tier 1 (knowledge-note prioritized order).
Link to Devin session: https://app.devin.ai/sessions/414754bd88374dc693f47100ac269e78