diff --git a/docs/architecture/rebac-authorization.md b/docs/architecture/rebac-authorization.md index ee0c0738..b031960e 100644 --- a/docs/architecture/rebac-authorization.md +++ b/docs/architecture/rebac-authorization.md @@ -360,11 +360,11 @@ grace window has zero effect on live access decisions — an expired grant is invisible to `check(...)` even during the grace period. Cleanup only affects storage bloat and the `list_grants_*` history surface. -The daemon runs inside the same process (`tokio::spawn` at startup, same -lifecycle as trash-cleanup / storage-usage sweep), so no external scheduler -is needed. An admin-triggered `POST /api/admin/internal/trigger-grant-cleanup` -lets operators force a purge in test or incident scenarios; the internal- -endpoints gate (`OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS`) applies. +The daemon runs inside the same process, registered with the periodic-job +scheduler (`docs/plan/job-registry.md`) on a 24-hour tick. An admin- +triggered `POST /api/admin/jobs/grant_cleanup/trigger?force=true` lets +operators force a purge in test or incident scenarios — `force=true` +collapses the grace window to zero for that call only. The [Share Integration](/architecture/share-integration) doc's reverse trigger takes it from there: when the daemon deletes the last `role_grants` diff --git a/docs/config/env.md b/docs/config/env.md index 20815600..0b5d4ea1 100644 --- a/docs/config/env.md +++ b/docs/config/env.md @@ -73,7 +73,6 @@ Most runtime variables use the `OXICLOUD_` prefix. A few build-time or allocator | `OXICLOUD_ENABLE_SEARCH` | `true` | Full-text and metadata search | | `OXICLOUD_ENABLE_MUSIC` | `true` | Music playlists and audio metadata | | `OXICLOUD_EXPOSE_SYSTEM_USERS` | `true` | Expose other OxiCloud users as a read-only address book at `GET /api/address-books` | -| `OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS` | `false` | Expose `POST /api/admin/internal/trigger-sweep`, `POST /api/admin/internal/trigger-gc`, and `POST /api/admin/internal/trigger-grant-cleanup` — test-only synchronous triggers for the storage-usage reconciliation sweep, blob garbage collector, and expired-grant purge respectively. Used by the API test suite to assert convergence deterministically without waiting out the periodic tickers. Leave **off** in production: the routes return 404 even to an admin token when disabled. | | `OXICLOUD_GRANT_CLEANUP_ENABLED` | `true` | Background daemon that deletes expired rows from `storage.role_grants`. The authorization engine already filters expired grants out of every permission check at read time (`expires_at IS NULL OR expires_at > NOW()`), so leaving expired rows in place is a hygiene issue — not a security one. This daemon garbage-collects them daily. Set to `false` to keep every expired grant row forever (uncommon; a fresh install rarely wants this). | | `OXICLOUD_GRANT_CLEANUP_GRACE_DAYS` | `15` | Days past a grant's `expires_at` before the row is eligible for deletion. The grace window preserves the audit / support answer to "what happened to my access?" for a couple of weeks past expiration. Values below 1 are legal but discouraged — the recommendation is **≥ 15 days**. Values above the actual grant TTL used by clients waste index space; a few weeks is the sweet spot. | | `OXICLOUD_GRANT_CLEANUP_INTERVAL_HOURS` | `24` | How often the grant-cleanup daemon fires. Clamped to a minimum of 1 hour. Adjusting this doesn't change what gets deleted — only how promptly. Daily is fine for any realistic grant volume. | diff --git a/docs/plan/consistency-check.md b/docs/plan/consistency-check.md new file mode 100644 index 00000000..b2b17fdc --- /dev/null +++ b/docs/plan/consistency-check.md @@ -0,0 +1,822 @@ +# Plan — Resumable consistency checks + `StatefulAdapter` contract + +## Context + +OxiCloud persists state in several independent subsystems: content-addressable +blobs on disk / S3, thumbnails (server-generated per blob + user-uploaded per +file), text-extraction cache, audio metadata cache, `storage.folders` + `file_metadata`, +`storage.trash`, `storage.drives.used_bytes`, WebDAV dead properties, and more. +Each has invariants that can silently drift: + +- A blob on disk with no `file_blobs` reference (leak — wasted disk). +- A `file_blobs` row whose bytes are gone from storage (**data loss** — GET + returns 500). +- A `drives.used_bytes` counter that no longer matches `SUM(size)`. +- A `folders.parent_id` pointing at a deleted row (historical raw-SQL fix). +- A thumbnail cache entry with no live file id (leak) or a missing entry + the user actually uploaded (data loss). + +Today the only "consistency" primitives are targeted point solutions — +`dedup_service` GC (orphan blob reap with 1 h grace), `storage_usage_service` +reconciliation (rebuild `used_bytes` from `SUM(size)`), and the trash cleaner. +None of them SURFACE inconsistencies for operators; they act blindly and +best-effort. `tests/api/storage_cleanup_check.sh` polls with a 5 s window +and races the 1 h GC grace (memory note `project_dedup_gc_test_trigger`). + +At scale — Ed's example: 1000 users × ~1000 files each = 1M files — an ad-hoc +"is my disk usage accurate?" check must be **resumable** across restarts, +cooperative on cancellation, and non-blocking to live traffic. A batch job +that starts over from scratch after a container restart or SIGTERM never +completes. + +This plan lands: + +1. Two **traits** — `ConsistencyCheck` (one check) and `StatefulAdapter` + (marker + registration on every state-owning port). +2. A **contract** every new adapter must satisfy at compile time — via a + supertrait bound on existing state-owning ports, no new adapter + compiles without declaring its consistency contract. +3. An **educational surface** in trait doc-comments — decision axes + (severity, direction, grace, cursor) and canonical-example pointers. +4. **Consistency-specific persistence** — `admin.consistency_findings`, + idempotent-on-`(run_id, kind, resource_id)`. +5. A **first check** — `BlobConsistencyCheck` (both directions, + blob-keyed cursor, severity split). + +**Layer boundary — the runtime is not in this plan.** The resumable +execution engine (cursor persistence, exclusivity, cancel protocol, +crash recovery, `admin.background_runs` schema, `JobStore`, +`RunOutcome`, `run_or_resume`) lives in `docs/plan/job-registry.md` +Part 2. This plan describes what `ConsistencyCheck` implementors +write and how the check-specific bits (findings, severity, +`StatefulAdapter` supertrait wiring) compose on top of that engine. + +**Order:** ships **after** the job-registry Part 2 engine lands. +Consistency closes an operator-visibility gap today, but it depends +on Part 2's `RecoverableJob` + `JobStore` + `admin.background_runs` +primitives — those come first. Once both are in, consistency runs +are admin-triggered v1, becoming periodic-triggered when a +`JobRegistry` (Part 1) tenant wraps `run_or_resume` for each +`ConsistencyCheck`. + +## Design decisions + +### Two-trait split + +`ConsistencyCheck` = one check (implements `run_resumable`). +`StatefulAdapter` = a subsystem that CONTRIBUTES checks (one or more). + +This split is load-bearing: +- Some subsystems emit **multiple** checks (`ThumbnailStore` emits four — + server-generated × 2 directions, user-uploaded × 2 directions). +- Some checks are **composed across** adapters (`UsedBytesConsistencyCheck` + reads from both `FileMetadataRepository` and `DriveRepository`). + +Bundling them into one trait would over-constrain the shape. + +### Compile-time enforcement via supertrait bound + +`StatefulAdapter` is added as a **supertrait** on every port that persists +state: + +```rust +pub trait BlobStorage: StatefulAdapter { … } +pub trait ThumbnailStore: StatefulAdapter { … } +pub trait FileBlobReadRepository: StatefulAdapter { … } +pub trait FolderRepository: StatefulAdapter { … } +``` + +Any new impl of these ports — a new S3-alike backend, a new mock in tests, +a plugin-provided storage backend — will not compile without providing +`subsystem()` and `consistency_checks()`. The compiler is the enforcement; +reviewers cannot merge a stateful adapter without an answer to "what can +go wrong with this state, and how do you check it?" + +### The severity axis + +Every finding carries a `Severity` so the admin UI can order results and +operators can dismiss the low-impact ones without hiding real risk. + +| Severity | Meaning | Examples | +|---|---|---| +| `DataLoss` | User-visible impact (500 on GET, missing user bytes) | Missing blob for a live `file_blobs` row; missing user-uploaded thumbnail | +| `Reclaimable` | Disk waste, no user impact | Orphan blob on storage, orphan thumbnail file | +| `Regenerable` | Auto-heals on next request | Missing server-generated thumbnail (server rebuilds), missing text-index row | +| `Drift` | Accounting mismatch, no user impact | `drives.used_bytes` vs `SUM(size)` | + +Rule of thumb: if a human user notices, it's `DataLoss`. If only the disk +accountant notices, it's `Reclaimable` or `Drift`. If the next automatic +regeneration will fix it, it's `Regenerable`. + +### Bidirectional in every check + +Every check emits BOTH directions where they exist: + +- **Backward (storage → DB) — orphan detection.** Wasted disk. `Reclaimable`. +- **Forward (DB → storage) — missing detection.** User-visible data loss. + `DataLoss`. Higher severity — a single missing content-addressable blob + silently breaks every file that referenced it. + +Skipping the forward direction is the single most common consistency-check +mistake. It's easy because Pass 1 (list storage, cross-check DB) LOOKS +complete. Pass 2 (list DB, cross-check storage) is where data-loss surfaces. + +### Report shape for missing findings — blob-level, not file-level + +`MissingInStorage { blob_hash, ref_count, affected_file_ids: Vec }`. +One row per missing blob with the fan-out of broken files. Operator gets +a triage-ordered "biggest impact first" list. File-per-line reports lose +that ordering. + +### Two-pass discipline eliminates the need for maintenance mode + +Pass 1 — build candidate list from a snapshot read (storage listing for +orphan direction, DB SELECT for missing direction). Exclude anything younger +than `grace_window`. + +Pass 2 — per candidate, re-read the OTHER side's state right before +flagging. If it transitioned (ref went up, blob just landed, row was +deleted, etc.), silently drop. + +Race matrix — orphan direction: +- **Upload lands mid-scan** (dedup hit → ref_count ↑ after we sampled) — + grace window skips young objects. +- **Last ref deleted mid-scan** (ref_count → 0, GC not yet) — cross-reference + `blobs.orphaned_at`; expected transient state, not flagged. +- **Deep hash on partial upload** — deep mode runs only on rows older + than a LONGER grace (24 h). + +Race matrix — missing direction: +- **Blob written but DB row not yet inserted** (young file looks missing + at flag time) — grace window skips DB rows younger than 1 h. +- **File deleted mid-scan** — Pass 2 re-reads `file_metadata` by id; + if gone, drop the finding. +- **Blob just now landed** — Pass 2 re-verifies storage `HEAD`; if now + present, drop. + +Nothing before "byte-exact whole-table snapshot verification" needs a +quiescent server. Reserve `concurrent_safe() = false` for that one. + +### Resumability — runs live in Part 2's `background_runs` + +Consistency runs are ordinary `RecoverableJob`s. The runtime plumbing +— cursor persistence, exclusivity, cancel protocol, crash recovery, +`admin.background_runs` schema, `JobStore` trait, `RunOutcome`, +`run_or_resume` helper — lives in `docs/plan/job-registry.md` Part 2. +This plan does not redefine any of it. + +**How consistency slots into the shared table:** +- Each consistency check registers under `job_name = + 'consistency_'` (e.g. `'consistency_blobs'`, + `'consistency_thumbnails'`). Naming convention lets a single + `SELECT DISTINCT ON (job_name)` return the last run of every check + alongside every other background job. +- Consistency's per-check knobs — `grace_window_secs`, `batch_size`, + `concurrent_safe` — live inside `background_runs.params` JSONB at + run-start time. The check reads them back via + `serde_json::from_value(store.params()?)`. +- `background_runs.stats` accumulates `{"scanned_count": …, + "findings_this_run": …}`; readers call + `(stats->>'scanned_count')::bigint`. + +The findings themselves are Layer C (this plan) — they don't +generalise to storage-migration or reextract: + +```sql +CREATE TABLE admin.consistency_findings ( + id UUID PRIMARY KEY, + run_id UUID NOT NULL REFERENCES admin.background_runs(id) ON DELETE CASCADE, + kind TEXT NOT NULL, -- OrphanBlob / MissingBlob / ... + severity TEXT NOT NULL, -- DataLoss / Reclaimable / ... + resource_id TEXT NOT NULL, + detail JSONB, + found_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + UNIQUE (run_id, kind, resource_id) -- idempotent re-scan on resume +); +CREATE INDEX ON admin.consistency_findings (run_id, severity); +``` + +FK on `background_runs.id` links a finding back to the run that +produced it; `ON DELETE CASCADE` clears findings when their run row +is pruned by a future retention job. + +`admin.*` is a NEW schema, created by Part 2's migration — keep it +distinct from `auth.*` / `storage.*` so operational tables don't +pollute domain schemas. + +### Non-obvious traps + +Recorded here (and in the trait doc-comments) because every one has been +learned the hard way in similar systems: + +1. **Grace window uses `scan_started_at`, NOT `NOW()`.** A resumable scan + spanning 6 h must snapshot its grace boundary at start. Otherwise items + uploaded 30 min in flip from "young, skip" (Pass 1's view) to "old, flag" + (Pass 2's view) mid-flight — the scan produces false positives against + itself. +2. **Cursor is per-check, opaque bytes.** Blob check cursors on BLAKE3 + hash (fixed 64 hex chars — natural lex order). Thumbnail cursors on + file_id UUID. Folder-tree cursor on ltree path. The trait treats it + as `Vec`; each impl serializes what it needs. +3. **Findings are idempotent on `(run_id, kind, resource_id)`.** Resume + revisit must not double-count. Pass 2 can also DELETE findings that + transitioned (was `MissingBlob`, blob has since landed → drop the + finding, not the whole run). +4. **Cooperative cancellation ONLY.** Between batches, poll + `background_runs.status`. A `tokio::spawn` abort mid-batch leaks — + cursor unpersisted, findings half-written. Cancel path writes + `status='Paused'` + current cursor before returning. +5. **Crash recovery on boot.** Any `status='Running'` at server start = + server died mid-scan. Auto-transition to `Paused`; DON'T auto-resume + (the bug that killed the last run may still be present). Admin decides. +6. **Batch size 1000 items or 30 s, whichever comes first.** Cursor commit + per-row makes DB write cost dominate at 1M items; longer batches leak + more progress on crash. +7. **Two directions don't share a cursor.** `BlobConsistencyCheck` orphan + side walks storage listing (S3 continuation token / readdir); missing + side walks `file_blobs` by hash. Sequence them (orphan phase → missing + phase); cursor encodes current phase. + `ThumbnailConsistencyCheck` is worse — four phases (2 subspaces × 2 + directions), each with its own natural cursor. Cursor encodes + `(subspace, direction, key)`. + +## Trait shapes + +### `ConsistencyCheck` + +```rust +/// A single consistency check with a resumable, cursor-based scan. +/// +/// # For implementors +/// +/// Every implementation must decide five things before the first line of +/// code. Answer them in comments at the top of the impl: +/// +/// 1. **Direction.** Backward (storage → DB) surfaces orphans; forward +/// (DB → storage) surfaces missing. Most checks do BOTH — sequence +/// them and encode the current phase in the cursor. +/// +/// 2. **Severity per finding kind.** `DataLoss` (user impact) / +/// `Reclaimable` (disk waste) / `Regenerable` (auto-heals) / +/// `Drift` (accounting). The single most common mistake is treating +/// a missing user-uploaded thumbnail as `Regenerable` — it's not, +/// the server can't recreate what the user provided. It's `DataLoss`. +/// +/// 3. **Cursor format.** Opaque `Vec` to the framework. Yours to +/// serialize. Content-addressable blobs → 32-byte BLAKE3. UUID rows → +/// 16-byte UUID. Path rows → the path bytes. Multi-phase check → +/// prepend a phase byte. +/// +/// 4. **Grace window.** Default 1 h (matches dedup GC). Deep checks +/// (hash verification) use 24 h. Grace ALWAYS refers to +/// `scan_started_at`, never `NOW()` — see trap #1 below. +/// +/// 5. **Batch boundary.** 1000 items or 30 s. Call `store.checkpoint` +/// and `store.should_cancel` between batches — cancellation is +/// cooperative, never task-abort. +/// +/// # Two-pass discipline +/// +/// Pass 1 — build candidate list from a snapshot read, excluding items +/// younger than `grace_window`. +/// +/// Pass 2 — per candidate, re-read the OTHER side's state right before +/// flagging. If it transitioned (ref went up, blob just landed, row was +/// deleted), silently drop. +/// +/// Pass 1 alone LOOKS complete but produces false positives on every +/// race. Never skip Pass 2. +/// +/// # Canonical example +/// +/// See `BlobConsistencyCheck` in +/// `src/infrastructure/services/consistency/blob_check.rs` — it exercises +/// every axis (both directions, both severities, grace window, cursor, +/// cooperative cancel, blob-level report shape for missing findings). +#[async_trait] +pub trait ConsistencyCheck: Send + Sync { + /// Machine-readable name — appears in the admin endpoint slug and in + /// audit `event` values. Lowercase snake_case, one per check. + fn name(&self) -> &'static str; + + fn grace_window(&self) -> Duration { Duration::from_secs(3600) } + + /// `true` (default) → safe to run against live traffic; the check + /// respects grace window + two-pass re-verify. Only false for a + /// check that genuinely needs a quiescent DB (whole-table snapshot + /// verification of hashes) — not required for anything in v1-v5. + fn concurrent_safe(&self) -> bool { true } + + /// `cursor: None` → fresh run. `Some(bytes)` → resume from last + /// persisted checkpoint. Impls MUST: + /// - call `store.checkpoint(cursor).await` between batches + /// (~1000 items or 30 s, whichever comes first); + /// - call `store.should_cancel().await` between batches — return + /// `RunOutcome::Paused { cursor }` when it returns `true`; + /// - use `store.scan_started_at()` (not `now()`) as the grace + /// window reference. + async fn run_resumable( + &self, + opts: &CheckOptions, + cursor: Option>, + store: &dyn CheckStore, + ) -> Result; +} + +#[derive(Debug)] +pub enum RunOutcome { + Completed, + Paused { cursor: Vec }, + Failed(DomainError), +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Severity { + DataLoss, // user impact — top of triage + Reclaimable, // disk waste, no user impact + Regenerable, // auto-heals on next request + Drift, // accounting mismatch, no user impact +} + +pub struct Inconsistency { + pub kind: &'static str, // "OrphanBlob", "MissingBlob", ... + pub severity: Severity, + pub resource_id: String, // opaque + pub detail: serde_json::Value, +} +``` + +### `CheckStore` + +The framework hands each check a `CheckStore` — the only side effect a +check performs on shared state. + +```rust +#[async_trait] +pub trait CheckStore: Send + Sync { + fn run_id(&self) -> Uuid; + fn scan_started_at(&self) -> chrono::DateTime; + + /// Persist the cursor + last-progress timestamp. Called between + /// batches. If a crash happens after this returns, the next resume + /// starts from `cursor`. + async fn checkpoint(&self, cursor: Vec, scanned_count: u64) + -> Result<(), DomainError>; + + /// Poll the run's `status` column. Returns `true` when an admin + /// requested cancellation. The check MUST return `Paused` with the + /// current cursor. + async fn should_cancel(&self) -> Result; + + /// Upsert a finding. `UNIQUE (run_id, kind, resource_id)` in the + /// schema means re-scanning the same resource on resume is safe. + async fn record_finding(&self, finding: Inconsistency) + -> Result<(), DomainError>; + + /// Delete a previously-recorded finding — used when Pass 2 sees + /// the resource transitioned out of the inconsistent state. + async fn drop_finding(&self, kind: &str, resource_id: &str) + -> Result<(), DomainError>; +} +``` + +### `StatefulAdapter` + +```rust +/// Marker + registration trait for any adapter that persists state OUTSIDE +/// process memory: blobs on disk / S3, DB tables, on-disk caches, message +/// queues you own. +/// +/// Added as a SUPERTRAIT on every state-owning port +/// (`trait BlobStorage: StatefulAdapter`, `trait ThumbnailStore: +/// StatefulAdapter`, `trait FolderRepository: StatefulAdapter`, …), which +/// means: NO NEW ADAPTER CAN COMPILE without declaring its consistency +/// contract. The compiler is the enforcement; these doc-comments are the +/// education. +/// +/// # For implementors adding a new stateful adapter +/// +/// You cannot skip this trait. If you're reading this because your PR +/// won't compile, work through: +/// +/// 1. **Am I actually stateful?** State means "bytes or rows outside +/// process memory that can desync from other subsystems". Config, +/// caches keyed by session, and derived indexes are NOT stateful +/// for this purpose (they can be dropped and rebuilt). If you're +/// not stateful, drop the `StatefulAdapter` impl entirely — but +/// then your port shouldn't have `StatefulAdapter` as a supertrait +/// either, so this compile error means the port author already +/// decided you were. +/// +/// 2. **What are the DIRECTIONS of drift I can detect?** Almost every +/// stateful adapter has both: +/// - Backward (my storage → the DB that references it): orphans. +/// - Forward (the DB → my storage): missing. +/// Return one check that covers both by sequencing phases, OR two +/// checks (one per direction). The former is easier to operate. +/// +/// 3. **What's the SEVERITY of each finding?** See `Severity` in +/// `consistency_check.rs`. Missing user-uploaded data is `DataLoss`; +/// missing server-derived data is `Regenerable`; orphan bytes are +/// `Reclaimable`; accounting drift is `Drift`. +/// +/// 4. **What CURSOR fits my walk?** Content-addressable → hash prefix. +/// UUID-keyed → UUID lex. Path-keyed → path bytes. Whatever you pick, +/// it's opaque `Vec` to the framework — decode inside your check. +/// +/// See `BlobConsistencyCheck` for the canonical impl to copy-adapt. +pub trait StatefulAdapter: Send + Sync { + /// Subsystem slug — appears in the JobRegistry-registered + /// `job_name` (`consistency_`, e.g. `consistency_blobs`) + /// and in audit log `event` values. Lowercase snake_case, unique + /// per adapter. Convention: `"blobs"`, `"thumbnails"`, `"trash"`, + /// `"folder_tree"`, `"used_bytes"`. + fn subsystem(&self) -> &'static str; + + /// REQUIRED (no default impl). Return every consistency check + /// this adapter contributes. Most adapters return exactly one. + /// Multi-keying subsystems return more — `ThumbnailStore` returns + /// FOUR checks (server-generated + user-uploaded, each in both + /// directions). + /// + /// Returning `vec![]` is a red flag. If your adapter has state but + /// no check, either: + /// - Your state is fully covered by another adapter's check + /// (rare — document exactly WHERE in a comment on this method). + /// - You haven't written the check yet — return + /// `vec![]` with a `TODO(consistency): add ConsistencyCheck` + /// comment, ship the trait wiring, add the check in a follow-up PR. + /// + /// Reviewers will grep `TODO(consistency)` and ask when it lands. + fn consistency_checks(&self) -> Vec>; +} +``` + +### `ConsistencyRegistry` + +```rust +/// Collects `StatefulAdapter`s at wire-up time. Instantiated once in +/// `AppServiceFactory`, exposed on `AppState`, consumed by the admin +/// handler + (when JobRegistry lands) the scheduler. +pub struct ConsistencyRegistry { + adapters: Vec>, +} + +impl ConsistencyRegistry { + pub fn register(&mut self, adapter: Arc) { + // Trait bound forces `subsystem()` + `consistency_checks()` to exist. + self.adapters.push(adapter); + } + + /// Every check contributed by every registered adapter, flat. + pub fn all_checks(&self) -> Vec> { + self.adapters + .iter() + .flat_map(|a| a.consistency_checks()) + .collect() + } + + pub fn get(&self, name: &str) -> Option> { + self.all_checks().into_iter().find(|c| c.name() == name) + } +} +``` + +## Admin surface + +Consistency runs are ordinary `RecoverableJob`s (see +`docs/plan/job-registry.md` Part 2), so most operator actions reach +them through the shared scheduler surface: + +``` +GET /api/admin/jobs + → summary list — consistency runs appear as + `job_name = "consistency_"` + +POST /api/admin/jobs/consistency_{name}/trigger + → 200 { ok, outcome: { run_id, status } } + (starts a new run or resumes the latest Paused one — see + Part 2's `run_or_resume`) + +POST /api/admin/jobs/consistency_{name}/cancel + → 200 { run_id, status: "CancelRequested" } + (cooperative — check finishes its current batch and returns Paused) + +GET /api/admin/jobs/consistency_{name}/runs?status= + → 200 [{ id, status, scanned_count, last_progress_at, … }] + +GET /api/admin/jobs/consistency_{name}/runs/{id} + → 200 { run: {...}, findings: [...paginated] } +``` + +Findings enrichment on `runs/{id}` is consistency-specific — read +from `admin.consistency_findings` and joined into the response. +Everything else is generic Part 2 behaviour. + +Production surface — always on, audit-logged. No feature-flag gate. + +## Approach + +### 1. Traits + framework in isolation + +`src/application/ports/consistency.rs` +- Define `ConsistencyCheck`, `RunOutcome`, `Severity`, `Inconsistency`, + `CheckStore`, `StatefulAdapter`, `CheckOptions`. +- Full doc-comments as sketched above — these are the educational + surface, don't cut them. + +`src/infrastructure/services/consistency/mod.rs` +- `ConsistencyRegistry` (data structure only). +- `PgCheckStore` — impl of `CheckStore` reading/writing + `admin.background_runs` (filtered to `job_name LIKE 'consistency_%'`) + + `admin.consistency_findings`. +- `run_check(check, cursor, store)` — the runner that calls + `run_resumable`, applies timeout, records outcome. + +### 2. Schema migration + +`migrations/YYYYMMDDHHMMSS_background_runs_admin_schema.sql` — creates +the merged `admin.background_runs` table shared with the JobRegistry +plan. Consistency checks own the `admin.consistency_findings` table +alone and reference `background_runs.id` via FK. + +```sql +CREATE SCHEMA IF NOT EXISTS admin; + +CREATE TABLE admin.background_runs ( + id UUID PRIMARY KEY, + job_name TEXT NOT NULL, -- 'consistency_blobs', 'storage_migration', 'reextract_audio', ... + status TEXT NOT NULL, -- Running / Paused / Completed / Failed / CancelRequested + started_at TIMESTAMPTZ NOT NULL, + last_progress_at TIMESTAMPTZ NOT NULL, + completed_at TIMESTAMPTZ, + cursor BYTEA, + stats JSONB NOT NULL DEFAULT '{}'::jsonb, -- e.g. {"scanned_count": 12345} + params JSONB NOT NULL DEFAULT '{}'::jsonb, -- e.g. {"grace_window_secs": 3600} + error_message TEXT +); +CREATE UNIQUE INDEX one_active_run_per_job + ON admin.background_runs (job_name) + WHERE status IN ('Running', 'Paused'); +CREATE INDEX ON admin.background_runs (last_progress_at) WHERE status = 'Running'; + +CREATE TABLE admin.consistency_findings ( + id UUID PRIMARY KEY, + run_id UUID NOT NULL REFERENCES admin.background_runs(id) ON DELETE CASCADE, + kind TEXT NOT NULL, + severity TEXT NOT NULL, + resource_id TEXT NOT NULL, + detail JSONB NOT NULL DEFAULT '{}'::jsonb, + found_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + UNIQUE (run_id, kind, resource_id) +); +CREATE INDEX ON admin.consistency_findings (run_id, severity); +``` + +### 3. Supertrait bounds on existing state-owning ports + +Add `StatefulAdapter` as a supertrait on: + +- `src/application/ports/storage_ports.rs::BlobStorage` + (or wherever the blob-storage port lives). +- `src/application/ports/thumbnails.rs::ThumbnailStore` (both server-generated + and user-uploaded paths). +- `src/application/ports/text_extraction.rs::TextExtractionCache`. +- `src/application/ports/audio_metadata.rs::AudioMetadataCache` (if a + distinct port exists). +- `src/domain/repositories/file_blob_read_repository.rs::FileBlobReadRepository` + (via the port trait it exposes to application services). +- `src/domain/repositories/folder_repository.rs::FolderRepository`. +- `src/domain/repositories/trash_repository.rs::TrashRepository`. +- `src/infrastructure/services/webdav_dead_property_store.rs` + (`DeadPropertyStore` — has its own leak class per the deferred-rekey + memory note). + +Each of these will trigger compile errors in its impls. Each impl gets +a two-line stub: + +```rust +impl StatefulAdapter for LocalFsBlobStorage { + fn subsystem(&self) -> &'static str { "blobs" } + fn consistency_checks(&self) -> Vec> { + // TODO(consistency): add BlobConsistencyCheck once impl lands. + vec![] + } +} +``` + +Ship this PR without the actual checks. Grep `TODO(consistency)` = punch list. + +### 4. First real check — `BlobConsistencyCheck` + +`src/infrastructure/services/consistency/blob_check.rs` + +- Depends on `BlobStorage` (storage listing) + `FileBlobReadRepository` + (DB SELECT). +- Phase 1 (orphan direction): walk storage listing, cursor on hash prefix. + Batch of 1000, checkpoint, cancel-poll. For each batch: SELECT ref_count + FROM `storage.file_blobs` WHERE hash IN (…). Pass 2 re-verifies at flag + time. Severity: `Reclaimable`. +- Phase 2 (missing direction): walk `file_blobs` ordered by hash, cursor + on hash. Batch of 1000. For each row: `HEAD` on storage backend. If + missing AND row hasn't disappeared AND row is older than + `grace_window`, flag `MissingInStorage` with `affected_file_ids` from + a JOIN to `file_metadata`. Severity: `DataLoss`. +- Cursor format: `[phase: u8, hash_key: 32 bytes]`. +- `LocalFsBlobStorage::consistency_checks()` returns + `vec![Arc::new(BlobConsistencyCheck::new(self.clone(), ...))]`. + +### 5. Admin handlers + +`src/interfaces/api/handlers/admin_handler.rs` + +- `start_consistency_check(name, force)` — insert an + `admin.background_runs` row with `job_name = 'consistency_'` + and `status = 'Running'`, spawn a tokio task calling `run_check`, + return `run_id`. Concurrent triggers hit the partial unique index + and short-circuit to returning the surviving row. +- `cancel_run(id)` — UPDATE status = 'CancelRequested'. +- `resume_run(id)` — verify status == 'Paused', spawn task with the + persisted cursor. +- `list_runs(filter)` — SELECT with filters + paginate. +- `get_run(id)` — SELECT run + paginated findings. + +Same admin-guard as the JobRegistry surface (`trigger_job`, +`list_jobs`). Production surface — always on, audit-logged, no +feature-flag gate. + +### 6. Boot-time crashed-run recovery + +In `AppServiceFactory` init, after DB pool is up: + +```rust +sqlx::query!( + "UPDATE admin.background_runs + SET status = 'Paused', + error_message = COALESCE(error_message, 'server restart mid-run') + WHERE job_name LIKE 'consistency_%' + AND (status = 'Running' OR status = 'CancelRequested')" +).execute(&pool).await?; +``` + +Filtering on `job_name LIKE 'consistency_%'` scopes the sweep to +consistency runs; other tenants of `background_runs` (storage +migration, reextract-*) run the same auto-Pause sweep from their own +boot-time hook. The JobRegistry supervisor's boot check may +generalise this into a single scheduler-wide sweep — until then, one +per subsystem is fine. + +Do NOT auto-resume — the bug that killed the last run may still be there. +Log a warning if any rows were flipped so operators notice. + +### 7. Hurl regression — `tests/api/consistency_check.hurl` + +- Setup: login admin, seed one file (which creates one blob). +- Trigger `blobs` check with `force=true` (grace_days=0). Poll runs + list until `status='Completed'`. Assert 0 findings. +- Manually orphan a blob (SQL: `DELETE FROM file_metadata WHERE …`, + leave `file_blobs` + storage in place). Trigger check again. + Assert 1 finding with `kind='OrphanInStorage'`, `severity='Reclaimable'`. +- Manually break a blob (SQL: leave `file_blobs` alone, wipe the + storage backend for that hash — actually, use the storage service's + test hook if one exists; otherwise skip this in Hurl and cover in + integration tests). +- Cancel a run mid-scan (large seed, poll for scanned_count > 0, POST + cancel, poll until status='Paused'). Resume. Assert scanned_count + after resume > checkpoint. + +### 8. Follow-up PRs (remaining checks) + +Priority order: + +| # | Check | Direction | Complexity | +|---|---|---|---| +| 1 | `BlobConsistencyCheck` | both | high (canonical) | +| 2 | `ThumbnailConsistencyCheck` | both × 2 subspaces = 4 sub-scans | high | +| 3 | `UsedBytesConsistencyCheck` | pure SQL | low — wrap existing reconciliation diff | +| 4 | `FolderTreeConsistencyCheck` | pure SQL | low — closure over `folders.parent_id` | +| 5 | Deep-hash sub-mode on `BlobConsistencyCheck` | forward | medium — 24 h grace, opt-in | +| 6 | `DeadPropertyConsistencyCheck` | forward | low, blocked on rekey (see `project_webdav_dead_properties_drive_rekey`) | +| 7 | `TrashConsistencyCheck` | both | medium — trash rows vs `file_metadata` soft-delete flags | + +Each is a separate PR against the stable trait. `TODO(consistency)` +count decreases by one per PR. + +## Critical files + +**Create:** +- `src/application/ports/consistency.rs` (~250 lines — traits + doc-comments) +- `src/infrastructure/services/consistency/mod.rs` (~40 lines — pub types) +- `src/infrastructure/services/consistency/registry.rs` (~80 lines) +- `src/infrastructure/services/consistency/pg_check_store.rs` (~150 lines) +- `src/infrastructure/services/consistency/runner.rs` (~100 lines) +- `src/infrastructure/services/consistency/blob_check.rs` (~300 lines — canonical impl) +- `migrations/YYYYMMDDHHMMSS_consistency_check_admin_schema.sql` (~30 lines) +- `tests/api/consistency_check.hurl` (~150 lines) + +**Modify (add supertrait bound):** +- `src/application/ports/storage_ports.rs` — `BlobStorage: StatefulAdapter`. +- `src/application/ports/thumbnails.rs` — `ThumbnailStore: StatefulAdapter`. +- `src/application/ports/text_extraction.rs`. +- `src/application/ports/audio_metadata.rs` (if applicable). +- `src/domain/repositories/file_blob_read_repository.rs`. +- `src/domain/repositories/folder_repository.rs`. +- `src/domain/repositories/trash_repository.rs`. +- `src/infrastructure/services/webdav_dead_property_store.rs`. + +**Modify (add `StatefulAdapter` stubs):** +- Every impl of the above ports. Each gets `subsystem()` + a `vec![]` stub + with `TODO(consistency)`. + +**Modify (wire up admin surface):** +- `src/common/di.rs` — build `Arc`, expose on + `AppState`, register every stateful adapter. +- `src/interfaces/api/handlers/admin_handler.rs` — five handlers. +- `src/interfaces/api/routes.rs` — five routes. +- `src/interfaces/api/mod.rs` — utoipa paths. +- `tests/api/run.sh` — register `consistency_check.hurl`. + +## Reused existing utilities + +- **Admin-guard + audit-log pattern** at + `src/interfaces/api/handlers/admin_handler.rs::trigger_job` — + same shape for the new endpoints (production surface, always-on, + audit-logged; no feature-flag gate). +- **Dedup GC's orphan-detection logic** (`dedup_service.rs`) — the + algorithmic template for `BlobConsistencyCheck`'s orphan phase. + Reference impl, not a callsite — the check needs its own two-pass + discipline; GC currently reap-and-forgets. +- **Reconciliation SQL diff** in `storage_usage_service.rs` — becomes + `UsedBytesConsistencyCheck` almost verbatim, wrapped in report-only mode. +- **`AGENTS.md` audit convention** — every finding double-logs to + `target: "audit"`, `event: "consistency.{check}.finding"`, plus + operational log to `target: "oxicloud::consistency"`. + +## Verification + +1. **Compile**: `cargo check --all-features --all-targets` + + `cargo clippy -- -D warnings` clean. +2. **Schema**: `just fe-nothing … cargo run` starts; migration lands + the `admin` schema; `psql -c "\dt admin.*"` shows the two tables. +3. **Boot line**: `consistency: N adapter(s) registered, M check(s) + available`. Grep `TODO(consistency)` in the source; count should + equal M in v1 minus the shipped `BlobConsistencyCheck`. +4. **Hurl** (`tests/api/consistency_check.hurl`): + - clean state → 0 findings + - forced orphan → 1 `OrphanInStorage` finding, severity `Reclaimable` + - cancel + resume round-trip preserves `scanned_count` +5. **Crash recovery**: kill server mid-scan (`kill -9`); restart; + confirm the row is `Paused` with `error_message='server restart + mid-run'`; POST resume; check completes. +6. **Trait enforcement**: add a new dummy adapter impl of `BlobStorage` + without `StatefulAdapter` — compile MUST fail. Add the stub; compile + succeeds. This is the load-bearing property of the design. +7. **Grace-window sanity**: run against a fresh 10 s window; upload a + file mid-scan; confirm the young blob does NOT surface as + `MissingInStorage` (grace window covers it). +8. **AuthZ gate**: non-admin caller hits `POST /api/admin/jobs/consistency_blobs/trigger` + → 403 from the admin middleware, audit line records the rejection. + +## Out of scope + +- **JobRegistry integration**. Consistency checks are admin-triggered + in v1. When `docs/plan/job-registry.md` lands, JobRegistry will + consume `ConsistencyRegistry::all_checks()` for scheduled execution + — no code change needed here. +- **Auto-repair**. Findings are reported, not fixed. Repair primitives + live in the existing services (dedup GC's reaper, storage_usage + reconciler); a future admin surface could trigger targeted repair + after human review. +- **Distributed scheduling**. Single-process. If OxiCloud ever runs + multi-node, add `SELECT … FOR UPDATE SKIP LOCKED` on the run rows. +- **Byte-exact whole-table snapshot verification**. The `concurrent_safe + = false` case — reserved for a future `DeepBlobConsistencyCheck` that + requires either `pg_export_snapshot` + S3-consistent list OR a + read-only mode. Not needed for v1-v5. +- **Cursor pagination on the `GET /runs/{id}` findings list**. Simple + offset/limit for v1. Add cursor only if operators actually hit a + 10k-findings run. +- **Findings retention**. Runs + findings accumulate forever until an + operator manually deletes. Add a background cleaner once volume + actually matters — most likely alongside JobRegistry. +- **Auto-scheduling in v1**. No `tokio::spawn` interval loop. Admin + triggers only. Every scheduled invocation goes through JobRegistry + when it lands. + +## Related memory notes + +- `feedback_no_abbreviated_env_vars` — full-word env var names if any + land (e.g. `OXICLOUD_CONSISTENCY_BATCH_SIZE`, not + `OXICLOUD_CC_BATCH`). +- `project_consistency_check_trait` — the memory that captures this + design's decisions and the traps that shape the trait. +- `project_dedup_gc_test_trigger` — motivates the check's grace-window + discipline; also the source of the algorithmic template for the + orphan-blob direction. +- `project_webdav_dead_properties_drive_rekey` — `DeadPropertyStore` + will get a check, but only after the rekey lands. +- `bug_thumbnail_dedup`, `bug_folder_cascade_hooks_missing` — surface + the four-sub-scan complexity of `ThumbnailConsistencyCheck`. +- `bug_orphan_seed_null_orphaned_at_flaky` — reminds implementors that + the orphan-blob direction MUST check `orphaned_at`, not just + `ref_count = 0`. diff --git a/docs/plan/job-registry.md b/docs/plan/job-registry.md new file mode 100644 index 00000000..33dc21bd --- /dev/null +++ b/docs/plan/job-registry.md @@ -0,0 +1,916 @@ +# Plan — Job engines (periodic + recoverable) + admin surface + +## Context + +OxiCloud runs several fire-and-forget background daemons today, each +spawned by a service factory in `src/common/di.rs` at startup: + +| Service | Cadence | Shape | +|---|---|---| +| `TrashCleanupService` | every 24 h | Fixed interval, no per-run state | +| `StorageUsageService::start_reconciliation_job` | every 600 s | Fixed interval, no per-run state | +| `db_pool_monitor` | every N s | Fixed interval, no per-run state | +| `dedup_service` GC | on demand + inline | Fixed interval, no per-run state | +| `GrantCleanupService` | every 24 h | Fixed interval, no per-run state | +| `tree_etag_flush_job` | every ~500 ms | Fixed interval, no per-run state | +| `content_index` worker | continuous | Fixed interval, no per-run state | +| Blob storage backend migration | admin-triggered | Long-running, cursor, resumable, in-memory state today | +| `admin/audio/metadata/reextract` | admin-triggered | Long-running, blocks HTTP request today | +| `admin/photos/metadata/reextract` | admin-triggered | Long-running, blocks HTTP request today | +| `ConsistencyCheck` runs (see `docs/plan/consistency-check.md`) | admin-triggered v1 | Long-running, cursor, resumable, needs DB state | + +Two shapes bleed together in the current codebase but shouldn't. Each +daemon reinvents its own env var pattern, admin trigger endpoint, +logging schema, and (for the long-running ones) its own in-memory +progress state that vanishes on restart. + +## Two engines, one file + +This plan is intentionally two plans in one file (Ed 2026-07-27), +because the two engines share an admin URL prefix, a config-var +convention, and a logging target — but nothing else: + +- **Part 1 — Periodic Scheduler.** In-memory registration + tokio + interval loop. Serves fixed-interval jobs an operator might trigger + manually. No DB tables, no cursor, no per-run persistence. +- **Part 2 — Recoverable-Run Engine.** DB-backed cursor persistence + + exclusivity + crash recovery. Serves the four long-running tenants + (storage-migration, reextract-audio, reextract-image, consistency + check runs) and any future work that iterates over a large space + with restart tolerance. + +A recoverable job CAN optionally be periodically-triggered (register +once in each engine; Part 1's tick calls Part 2's `run_or_resume` +instead of a bare handler). Most Layer B tenants are admin-triggered +only. + +Cross-cutting concerns (admin URL taxonomy, env vars, logging target, +plugin future) live in a shared section at the bottom so we're not +duplicating them between parts. + +## Migration criterion — the trigger question + +Not every background loop belongs in JobRegistry. The single question +that decides: + +> **"Would an operator plausibly `POST /api/admin/jobs/{name}/trigger` +> to make it run right now?"** + +**Yes → migrate.** The whole payoff of JobRegistry is a uniform +*operator surface* — list, trigger, last-outcome, log line, config +knobs. If nobody would ever manually trigger the job, the surface +delivers no value; you're paying framework overhead for nothing. +Anything an operator would manually trigger is by definition +periodic + discrete + meaningful. + +**No → leave it as its own loop.** Continuous drains and +event-reactive workers ("core workers") fail this test — "trigger +the content-index worker" makes no sense; it's already running. +Standardise their env var naming and log target as a light +convention (see [Cross-cutting](#cross-cutting) below) but do NOT +wedge them into the scheduler. + +Secondary confirmation questions — if the primary is yes and any of +these is no, migrate anyway but flag the mismatch: + +1. Does each invocation report a meaningful `count` (rows swept, + blobs GC'd, bytes reclaimed)? Continuous workers don't have + discrete invocations to count. +2. Does the operator tune it via env vars beyond enable/disable? +3. Would an operator want a "did this run within the last N?" health + signal? Periodic jobs benefit from `last_outcome`; always-on + workers need liveness signals of a different shape. + +**Cadence is NOT the trigger** — it's a symptom. Sub-second jobs +almost always fail the primary question (nobody manually triggers +something that fires 2× per second), but a hypothetical 1 s periodic +job that operators do want to kick still belongs in JobRegistry. +Cadence tells you "probably no"; the operator-trigger question is +what decides. + +### Applied to the current daemons + +| Service | Operator-trigger? | Destination | +|---|---|---| +| `TrashCleanupService` | Yes — "purge expired trash now" | Part 1 | +| `StorageUsageService::start_reconciliation_job` | Yes — "recompute quotas now" | Part 1 | +| `dedup_service` GC | Yes — already has `trigger-gc` | Part 1 | +| `GrantCleanupService` | Yes — already has `trigger-grant-cleanup` | Part 1 | +| `tree_etag_flush_job` | No — a "flush now" is meaningless (queue drains itself) | Core worker, unchanged | +| `content_index` worker | No — continuous drain, no discrete invocation | Core worker, unchanged | +| `db_pool_monitor` | No — "log stats now" is either grep-existing-logs or attach-a-debugger, not a scheduled job trigger | Core worker, unchanged | +| Blob storage backend migration | Yes — already admin-triggered | Part 2 | +| `admin/audio/metadata/reextract` | Yes — currently admin-triggered (synchronously) | Part 2 | +| `admin/photos/metadata/reextract` | Yes — currently admin-triggered (synchronously) | Part 2 | +| `ConsistencyCheck` runs | Yes — needs a trigger endpoint | Part 2 | + +The `db_pool_monitor` case is illustrative: cadence-wise it *could* +fit Part 1 (10-30 s periodic, bounded work), but the operator-trigger +question kills it. Nobody manually triggers a stats-log because logs +are already there. Keeping it as its own loop is right. + +## Implementation order + +1. **Part 1 lands first** — small, self-contained, unblocks migration + of trash-cleanup + storage-usage + db_pool_monitor + dedup GC + + grant-cleanup + tree-etag flush + content-index. High mechanical + payoff, zero new schema, minimal review surface. +2. **Part 2 lands next** — introduces `admin.background_runs` schema + + `RecoverableJob` trait + `JobStore` port + `run_or_resume`. On + its own PR (schema change deserves independent review). +3. **Consistency-check framework (`docs/plan/consistency-check.md`)** + lands third, consuming Part 2 as its runtime. +4. **Storage-migration and reextract-* migrated to Part 2** as + follow-ups. + +--- + +## Part 1 — Periodic Scheduler + +### Contract — `JobHandler` trait + +The implementor-facing surface for a fixed-interval job: + +```rust +#[async_trait] +pub trait JobHandler: Send + Sync { + /// Stable snake_case identifier. Must be unique across the process. + /// Log lines, admin listing, env vars, and trigger URLs all key on + /// this name. + fn name(&self) -> &str; + + /// One execution. Called by the supervisor at the registered + /// interval and (optionally) on admin trigger. Return `Ok { count, + /// extra }` on success — the count is the primary scalar the job + /// reports (rows swept, ETags flushed, blobs GC'd). Return + /// `Err(msg)` on failure; the supervisor logs it and continues. + /// + /// `args` carries per-dispatch parameters. Periodic ticks pass + /// `JobRunArgs::default()`; admin triggers can set `force: true` + /// to request acceleration semantics (e.g. dedup GC skips its + /// orphan grace window, grant cleanup uses grace = 0). Handlers + /// that don't understand a given arg silently ignore it — no + /// return-error path just because a caller set an unused flag. + async fn run(&self, args: &JobRunArgs) -> JobOutcome; +} + +/// Per-dispatch parameters. Grows over time; today it carries only +/// `force`. Kept as a struct (not `bool`) so we don't have to change +/// signatures the next time a job needs another knob. +#[derive(Debug, Clone, Default)] +pub struct JobRunArgs { + /// Request acceleration semantics. Semantics are per-job: + /// - `dedup_gc`: skip the orphan grace window (grace = 0). + /// - `grant_cleanup`: grace = 0. + /// - Others: silently ignored. + pub force: bool, +} +``` + +Native services implement this trait on an existing service type (no +new wrapper) and register a single `Arc` with the +scheduler. + +### `JobOutcome` + +```rust +pub enum JobOutcome { + Ok { count: u64, extra: serde_json::Value }, + Err(String), +} +``` + +Two variants only. Every reason a run can fail (handler returned an +error, wall-clock timeout, panic caught by the supervisor) collapses +to `Err(String)`, with the *cause* encoded in the message AND in a +`cause` tracing field the supervisor sets when it emits the log line: + +- Handler returned `Err(msg)` → `cause = "handler"`, message = `msg`. +- `tokio::time::timeout` tripped → `cause = "timeout"`. +- `catch_unwind` caught a panic → `cause = "panicked"`, message = the + payload as a string. + +Handlers never construct the cause themselves; they either return +`Ok { count, extra }` or `Err(String)`. Keeping the enum to two +variants prevents every consumer of `match outcome` from having to +distinguish diagnostic sub-cases that behave identically for logging, +persistence, retry, and admin display. + +### Runtime model + +- **One `tokio::spawn`** at startup runs the scheduler main loop. + Sleeps until the earliest due job, dispatches, sleeps again. +- Per-run **panic catching** via `tokio::spawn` inside the dispatch + (or `AssertUnwindSafe` + `catch_unwind`). A bad handler crashes + its own run, not the scheduler. +- **Sequential dispatch within a tick** by default. Two jobs due at + the same instant run one after the other. Parallel dispatch can + layer on later as a per-job toggle if a real need appears — most + handlers touch the DB and don't benefit from concurrency. +- **`ScheduledJob.timeout: Option`** is applied by the + supervisor via `tokio::time::timeout` when set. Optional; use it + when the handler has a real wall-clock bound. None means "let it + run to completion." + +Single supervisor is chosen for **operational** clarity, not runtime +cost: one place to observe, one panic-containment boundary, one +config surface, one plugin-registration hook when plugins land. + +### Exclusivity — one in-flight run per `job_name` + +Mirrors Part 2's exclusivity invariant, enforced in-memory since +Part 1 has no DB row: + +- Each `RegisteredJob` carries an `is_running` flag (an + `AtomicBool` or single-permit `Semaphore`). +- Before dispatching a tick, the supervisor tries to acquire the + flag. If it's already held (the previous run is still executing), + the tick is **skipped, not queued**: + + ```rust + tracing::warn!( + target: "oxicloud::scheduler", + event = "job.tick_skipped", + job = %name, + interval_ms = interval.as_millis(), + running_for_ms = current_run_start.elapsed().as_millis(), + "{name} still running past its interval — tick skipped" + ); + ``` + + `next_run_at` advances by one interval so the schedule stays on + its cadence rather than queueing backlog. +- On completion (or panic caught by the supervisor), the flag is + released. The next tick is free to fire. +- **Diagnostic value.** A `job.tick_skipped` line on every interval + is the operator signal that either the job is chronically slower + than its cadence (retune the interval) or hung (attach a debugger + / set a timeout / kill the process). Without this warning a slow + or hung handler would silently starve. +- **Interaction with timeout.** If a job has a `timeout` configured + and it trips, the supervisor kills the run and releases the flag. + Timeouts prevent hangs from permanently silencing a job. + Handlers without a timeout can, in principle, hang forever — the + repeated `tick_skipped` warning is the only signal. + +Cross-job concurrency is unchanged — different `job_name`s can run +sequentially per tick as described above. Exclusivity is per +job_name, not global. + +### `JobRegistry` + +```rust +pub struct JobRegistry { + jobs: RwLock>, +} + +struct RegisteredJob { + handler: Arc, + /// `None` = on-demand only (admin trigger + programmatic + /// `registry.trigger(name)`), never fires periodically. + /// `Some(dur)` = fires every `dur` AND admin-triggerable. + interval: Option, + timeout: Option, + /// Single-permit gate that enforces the "one in-flight run per + /// `job_name`" invariant (see Exclusivity above). A tick that + /// finds the permit taken emits `job.tick_skipped` and does not + /// spawn. + in_flight: Arc, // capacity = 1 + /// Set when a run starts, cleared when it ends. Used to include + /// `running_for_ms` in the skip warning. + current_run_start: Arc>>, + last_outcome: Option<(chrono::DateTime, JobOutcome)>, + /// Only populated for periodic jobs (`interval = Some(_)`). None + /// for on-demand-only jobs — `pick_next` skips them. + next_run_at: Option>, +} +``` + +`Arc` lives on `AppState`. Native services register +themselves during DI: + +```rust +// Scheduled: fires every N hours AND admin-triggerable. +registry.register( + Arc::clone(&trash_cleanup) as Arc, + Some(Duration::from_secs(interval_hours * 3600)), + None, // no timeout +); + +// On-demand only: no periodic tick, but the job is still catalogued +// so the admin endpoint can trigger it uniformly and callers get the +// same panic-containment + exclusivity guarantees. Used by dedup GC +// (piggybacks on trash cleanup for its main work; admin trigger for +// operator-driven runs). +registry.register( + Arc::clone(&dedup_service) as Arc, + None, // interval — no periodic tick + None, // timeout +); +``` + +**Interval semantics.** +- `Some(dur)` — supervisor fires the job every `dur`. Also admin-triggerable. +- `None` — supervisor never fires the job. Admin-triggerable only. Dispatch still routes through the same `JobRegistry::trigger(name)` path so the job gets the same panic-containment, timeout, exclusivity, and log-line treatment as scheduled ones. + +### Manual dispatch — `JobRegistry::trigger(name, args)` + +```rust +pub async fn trigger(&self, name: &str, args: &JobRunArgs) -> Option; +``` + +The single entry point for running a registered job outside the +scheduler's tick loop. Called by: +- The admin endpoint (`POST /api/admin/jobs/{name}/trigger?force=`). +- Any service that wants a scheduler-uniform dispatch of a peer job + (e.g. an inline call from trash cleanup to `trigger("dedup_gc", &args)`, + if we later route the piggyback through the registry). + +The supervisor's periodic ticks invoke the same underlying dispatch +with `JobRunArgs::default()` — periodic runs never force. + +Returns `None` when the name doesn't exist. Returns `Some(JobOutcome)` +otherwise — even when exclusivity kicks the trigger out (that maps +to `Ok { count: 0, extra: {"skipped": "already_running"} }`, not +`None`). + +### Design boundary — registry is a catalog, not an event system + +Because a job can be triggered from multiple sources (scheduler, +admin, another service), the registry visually resembles an event +system. It is not. The distinction matters so we don't accidentally +extend it into one. + +- **Registry:** *"operator or scheduler wants to run this SPECIFIC + named job right now."* Imperative. Single handler per name. Direct + dispatch. No subscription API. +- **Event system:** *"when SOMETHING happens, notify anyone + interested."* Reactive. Multiple listeners per event type. + Publish + subscribe API. Fan-out semantics. + +Event-reactive work in OxiCloud goes through the existing lifecycle +hooks — `FileLifecycleHook`, `BlobLifecycleHook`, +`UserLifecycleHook`. Those already support multi-subscription and +event-typed dispatch. Never add subscription machinery to +`JobRegistry`; if a "when job A finishes, do B" case appears, +publish a `JobCompleted` lifecycle event and let a hook subscribe. + +### Engine loop + +```rust +async fn run(registry: Arc) { + loop { + let next = registry.pick_next().await; // earliest next_run_at + let sleep = next.deadline().saturating_duration_since(Instant::now()); + tokio::time::sleep(sleep).await; + + let outcome = registry.dispatch(&next.name).await; + registry.record_outcome(&next.name, outcome).await; + } +} +``` + +`dispatch` grabs the handler under a read lock, spawns a task, applies +the timeout, catches panics, and returns the `JobOutcome`. Sequential +dispatch is intentional; two jobs due at the same instant run +one-after-the-other. + +### Native tenants and migration order + +Four services satisfy the operator-trigger criterion above and migrate: + +1. **trash-cleanup** — simplest self-contained loop; reference for the + migration shape. Ships with Part 1's landing PR. +2. **storage-usage reconciliation** — same shape, different service. +3. **dedup GC** — already has `trigger-gc`; the shim forwards to the + new registry-backed trigger. +4. **grant-cleanup** — already has `trigger-grant-cleanup`; same shim + pattern. + +Three services are **core workers** and STAY on their own loops +(fail the operator-trigger question — see the criterion table above): + +- `tree_etag_flush_job` — 500 ms queue-drain, coalescing semantics. +- `content_index` worker — continuous channel drain, event-reactive. +- `db_pool_monitor` — periodic stats-log with no discrete-invocation + count and no operator use for manual trigger. + +Standardise their env var naming (`OXICLOUD_JOB__*`) and +tracing target for uniform operator ergonomics, but do NOT wedge them +into the scheduler. + +### Verification (Part 1) + +1. **Compile**: `cargo check --all-features --all-targets` + + `cargo clippy -- -D warnings` clean. +2. **Boot**: start server; expect `scheduler started, N job(s) registered`. +3. **Admin listing**: + ``` + curl -s http://localhost:8086/api/admin/jobs -H "Authorization: Bearer $TOKEN" + ``` + returns a JSON array with each registered job, its `interval_ms`, + `next_run_at`, and `last_outcome` (null until first tick). +4. **Trigger**: `POST /api/admin/jobs/trash_cleanup/trigger` + invokes the handler immediately, records the outcome. +5. **Panic containment**: unit test a handler that panics; `last_outcome` + records `Err(...)` with `cause = "panicked"` in the log; the scheduler + is still alive (verified by triggering another job); the in-flight + permit is released so the next tick can fire. +6. **Timeout enforcement**: unit test a handler that blocks longer than + its declared timeout; `last_outcome` records `Err(...)` with + `cause = "timeout"`; the in-flight permit is released. +7. **Overrun exclusivity**: unit test a handler with a 100 ms interval + that sleeps 300 ms. Assert exactly ONE run is in flight at any moment + (no parallel dispatch), and that two `job.tick_skipped` log events + fire (one at each missed tick) with `running_for_ms` monotonically + increasing. +8. **Shim compatibility**: existing per-service trigger endpoints + (`trigger-sweep`, `trigger-gc`, `trigger-grant-cleanup`) keep working + as thin forwards. Existing api-test Hurl suites pass unchanged. + +--- + +## Part 2 — Recoverable-Run Engine + +### Contract — `RecoverableJob` trait + +Sibling to `JobHandler`, NOT a subtrait. A stateless job that only +implements `JobHandler` never needs to know Part 2 exists. + +```rust +#[async_trait] +pub trait RecoverableJob: Send + Sync { + /// Stable snake_case identifier — matches the `job_name` column + /// in `admin.background_runs`. + fn name(&self) -> &str; + + /// Long-running, cooperative scan. The store is the job's ONLY + /// side effect: cursor checkpointing, cancel polling, run-state + /// updates all go through it. + /// + /// Between batches the handler MUST poll `store.status()` — a + /// `CancelRequested` return means the operator asked for a pause + /// and the handler should return `Paused { cursor }` at the next + /// safe boundary. A mid-batch `tokio::spawn` abort corrupts the + /// cursor and MUST NEVER happen — that's why the supervisor does + /// not apply `tokio::time::timeout` to recoverable jobs (Part 1's + /// timeout policy does not apply here). + async fn run_resumable(&self, store: &dyn JobStore) -> RunOutcome; +} +``` + +### `RunOutcome` + +```rust +pub enum RunOutcome { + Completed, + Paused { cursor: Vec }, + Failed { message: String }, +} +``` + +- `Completed` — walked the whole space. Engine writes `status = Completed`. +- `Paused { cursor }` — cooperative pause (cancel poll or graceful + shutdown). Engine persists cursor + writes `status = Paused` so a + future resume picks up here. +- `Failed { message }` — irrecoverable error. Cursor NOT advanced; + engine writes `status = Failed` and captures the message. + +### `JobStore` trait + +The port the engine passes to a recoverable job. Backed by +`admin.background_runs` in production; can be mocked for unit tests. + +```rust +#[async_trait] +pub trait JobStore: Send + Sync { + /// The `run_id` this handler was invoked with. Uniquely identifies + /// the row in `admin.background_runs`. + fn run_id(&self) -> Uuid; + + /// Fixed at run start; used by consistency checks (and any other + /// job with a grace boundary) as the reference `NOW()` — NOT + /// `chrono::Utc::now()`, which would drift across a multi-hour + /// scan. See `docs/plan/consistency-check.md` trap #1. + fn started_at(&self) -> chrono::DateTime; + + /// Read the current `status` from the row. Between batches the + /// handler polls this; a return of `CancelRequested` means the + /// operator asked for a pause. + async fn status(&self) -> Result; + + /// The last-persisted cursor (raw bytes, per-job schema), or + /// `None` on a fresh run. The handler decodes into its own key + /// type (blob hash, file_id UUID, ltree path, …). + async fn load_cursor(&self) -> Result>, DomainError>; + + /// Advance cursor + stats, bump `last_progress_at`. Called between + /// batches, typically every ~30 s OR every ~1 000 rows, whichever + /// comes first. See `docs/plan/consistency-check.md` trap #6. + async fn checkpoint(&self, cursor: Vec, delta_count: u64) + -> Result<(), DomainError>; +} +``` + +Domain-specific extensions (consistency-check's finding sink, for +instance) are separate traits the impl composes on top of `JobStore`. +`JobStore` itself carries no findings/severity concept — those are +Layer C in the consistency-check plan, not the engine's concern. + +### Schema — `admin.background_runs` + +```sql +CREATE SCHEMA IF NOT EXISTS admin; + +CREATE TABLE admin.background_runs ( + id UUID PRIMARY KEY, + job_name TEXT NOT NULL, + status TEXT NOT NULL, -- Running / Paused / CancelRequested / Completed / Failed + started_at TIMESTAMPTZ NOT NULL, -- fixed at run start + last_progress_at TIMESTAMPTZ NOT NULL, -- heartbeat + last-checkpoint marker + completed_at TIMESTAMPTZ, + cursor BYTEA, -- opaque, per-job resume key (NULL = fresh) + stats JSONB NOT NULL DEFAULT '{}'::jsonb, -- job-specific counters + params JSONB NOT NULL DEFAULT '{}'::jsonb, -- job-specific params + error_message TEXT +); + +CREATE UNIQUE INDEX one_active_run_per_job + ON admin.background_runs (job_name) + WHERE status IN ('Running', 'Paused', 'CancelRequested'); + +CREATE INDEX ON admin.background_runs (last_progress_at) + WHERE status = 'Running'; +``` + +**The partial unique index is load-bearing.** It enforces the "at +most one non-terminal run per `job_name`" invariant at the DB layer +so it survives concurrent triggers, admin-vs-scheduler races, and +transaction interleavings. The `CancelRequested` inclusion prevents +a second trigger during cancel from spawning a parallel run. + +`admin.*` is a NEW schema — kept distinct from `auth.*` / `storage.*` +so operational tables don't pollute domain schemas. Consistency +checks own their own `admin.consistency_findings` in the same +schema. + +Cursor is `BYTEA`, not JSONB, because per-job cursors are fixed-shape +opaque keys (32-byte BLAKE3, 16-byte UUID, ltree bytes) — JSONB adds +encoding overhead and a keying convention every impl has to agree on. +`stats` and `params` ARE JSONB because they carry human-readable +key/value pairs read by observability code, not compared inside SQL. + +### Cursor semantics + +- **`NULL` cursor** = fresh run, no rows processed yet. Handler + interprets as "start from the beginning." Every keyset-pagination + helper handles this as `WHERE ($1::bytea IS NULL OR key > $1)`. +- **Non-NULL cursor** = last-processed key. On resume, `key > cursor` + in the ORDER BY key ASC iteration. +- **Advance rule** = handler updates its in-memory cursor to the LAST + row it successfully processed at the end of each batch, checkpoints + periodically. On crash: at most one batch of work replays. Idempotent + processing (e.g. `UNIQUE (run_id, kind, resource_id)` on findings) + makes replay a no-op for anything already recorded. + +### Checkpoint mechanics + +One `UPDATE` per checkpoint. Cheap, no row-lock contention (this +process owns the row): + +```sql +UPDATE admin.background_runs + SET cursor = $2, + stats = jsonb_set( + stats, + '{scanned_count}', + ((COALESCE(stats->>'scanned_count','0')::bigint + $3)::text)::jsonb + ), + last_progress_at = NOW() + WHERE id = $1; +``` + +- `cursor` advances to the last row we processed. +- `stats.scanned_count` accumulates the delta — not overwritten. Each + job's handler picks its own key names inside `stats`. There's ONE + convention: a top-level `count` field mirroring the value carried + in `JobOutcome::Ok.count` (see next section) — everything else is + free-form. +- `last_progress_at` doubles as heartbeat. Boot recovery uses it to + spot stale-Running rows. + +### `RunOutcome` → `JobOutcome` bridge + +The supervisor translates so a periodic-triggered recoverable job +records the same `JobOutcome` shape as any other tick: + +- `Completed` → `Ok { count, extra: json!({"completed": true}) }` +- `Paused { cursor }` → `Ok { count, extra: json!({"paused": true, "cursor_hex": …}) }` +- `Failed { message }` → `Err(message)` + +Paused is deliberately NOT an error — the run cooperatively yielded, +that's a success. Log lines stay meaningful (`outcome=ok`, +`extra.paused=true` distinguishes from full completion). Only +`Failed` alerts an operator. + +### `run_or_resume` helper + +The engine module exposes: + +```rust +pub async fn run_or_resume( + job: Arc, + store_factory: &dyn JobStoreFactory, +) -> JobOutcome +``` + +Body: + +1. Look up the latest row for `job.name()`. +2. If `Completed`/`Failed` or nothing → `INSERT` a new `Running` row + with `started_at = NOW()`, cursor NULL. On unique-index conflict + (rare race), read the winning row and continue from step 3. +3. If `Paused` → `UPDATE ... SET status='Running'` on that row. +4. If `Running`/`CancelRequested` → short-circuit + `Ok { count: 0, extra: {"skipped": "already_running"} }`. +5. Build a `JobStore` bound to the row's `run_id` and pass it to + `job.run_resumable(store).await`. +6. Translate the returned `RunOutcome`, write the terminal status + (`Completed` / `Paused` / `Failed`) with the final cursor/stats + snapshot, return the `JobOutcome`. + +### Concurrency policy — exclusive-by-default + +**At most one non-terminal run per `job_name` may exist at any time.** +Non-terminal = `status IN ('Running', 'Paused', 'CancelRequested')`. +This is the default, not opt-in — a job runs to completion, gets +manually paused, or fails; a second trigger while one is active +never spawns a parallel run. + +- A storage-migration cannot run twice at once. Neither can a + reextract-audio, a reextract-image, or a consistency-check. +- The registry's trigger endpoint is idempotent: called while a run + is active it returns the existing `run_id` + status; called while + the latest run is `Paused` it resumes it (same cursor, same stats + accumulator); called when no non-terminal run exists it starts + fresh. +- The DB-level partial unique index makes the invariant impossible + to violate even under concurrent triggers or scheduler-vs-operator + races. +- The scheduler's periodic tick honours the same rule — if the + latest row for a job is non-terminal, the tick does not spawn + another. For long-running jobs "interval" effectively means "check + every N whether a run needs starting", not "start every N." +- Cross-job concurrency is unchanged — different `job_name`s can + run in parallel subject to Part 1's sequential-dispatch default. + Exclusivity is per job_name, not global. + +### Boot-time crashed-run recovery + +At `AppServiceFactory` init, after DB pool is up: + +```rust +sqlx::query!( + "UPDATE admin.background_runs + SET status = 'Paused', + error_message = COALESCE(error_message, 'server restart mid-run') + WHERE status IN ('Running', 'CancelRequested')" +).execute(&pool).await?; +``` + +Do NOT auto-resume — the bug that killed the last run may still be +present. Operators decide. The next scheduler tick (or an explicit +trigger) resumes any `Paused` row per the normal flow. + +Consistency-check.md's existing consistency-scoped sweep collapses +into this general one. + +### Admin surface (recoverable runs) + +Same URL taxonomy as Part 1 — resource-first, action second, all +under `/api/admin/jobs/{name}/*`. Extended for run identity: + +``` +POST /api/admin/jobs/{name}/trigger + → { run_id, status } # starts or resumes; idempotent +POST /api/admin/jobs/{name}/cancel + → { run_id, status: "CancelRequested" } +GET /api/admin/jobs/{name}/runs + → [{ run_id, status, started_at, last_progress_at, stats, ... }] +GET /api/admin/jobs/{name}/runs/{id} + → { run_id, status, cursor_hex, stats, params, error_message, ... } +``` + +### Native tenants (Part 2) + +- **Blob storage backend migration.** `migration_job.rs` becomes a + `RecoverableJob` impl. Cursor = last processed blob hash. Retires + the `Arc>` in-memory struct. +- **Reextract audio metadata.** Currently synchronous inside the + admin HTTP request. Becomes a `RecoverableJob` iterating audio + files by `file_id`. +- **Reextract image/video capture dates.** Same as above. +- **Consistency-check runs.** Every `ConsistencyCheck` impl gets + wrapped by a `RecoverableJob` adapter; the wrapper writes to + `admin.background_runs` via `JobStore`, and separately writes + findings to `admin.consistency_findings` via a check-specific + extension trait. See `docs/plan/consistency-check.md`. + +### Verification (Part 2) + +1. **Compile + schema-migration idempotence.** +2. **Fresh run:** `POST /api/admin/jobs/storage_migration/trigger` → new row with + `status='Running'`, `cursor=NULL`. +3. **Concurrent trigger:** second `POST` while the first is running + returns the SAME `run_id` (idempotent, DB unique index enforces). +4. **Cancel + resume round-trip:** `/api/admin/jobs/…/cancel` flips to + `CancelRequested`; handler polls, returns `Paused { cursor }`; + engine writes `Paused`. `POST /api/admin/jobs/…/trigger` again resumes; cursor + picks up where left off; `stats.count` continues accumulating. +5. **Crash recovery:** stop the server mid-run; restart; boot sweep + flips the row to `Paused` with `error_message = 'server restart mid-run'`; + admin triggers again and it resumes. +6. **Idempotent replay:** for consistency-check specifically, verify + that re-processing the last unpersisted batch does NOT double-record + findings (`UNIQUE (run_id, kind, resource_id)` on + `admin.consistency_findings`). +7. **`RunOutcome` bridge log lines:** completed run logs + `outcome=ok, extra.completed=true`; paused logs + `outcome=ok, extra.paused=true`; failed logs `outcome=err, cause=handler`. + +--- + +## Cross-cutting + +### Admin URL taxonomy + +All scheduler endpoints live on the **production admin surface**: +`/api/admin/jobs/*`. Always on, audit-logged, no feature-flag gate — +these are the operational levers you actually want ops to reach in +prod. See `project_admin_url_taxonomy` for the `/admin` vs +`/admin/internal` split we're honouring here. + +**Resource-first URL taxonomy** for every scheduler-owned endpoint: + +``` +GET /api/admin/jobs # list all +POST /api/admin/jobs/{name}/trigger # one dispatch (Part 1 + 2) +POST /api/admin/jobs/{name}/cancel # cooperative pause (Part 2) +GET /api/admin/jobs/{name}/runs # run history (Part 2) +GET /api/admin/jobs/{name}/runs/{id} # single run detail (Part 2) +``` + +`{name}` is the stable `JobHandler::name()` identifier. `trigger` +accepts an optional `?force=` query param that maps to +`JobRunArgs.force`. + +**Audit logging.** Every `POST` to `/api/admin/jobs/*` emits a +`target: "audit"` line before invoking the registry — bulk-effect +mutations belong on the audit stream. Success/failure outcome fires +its own `oxicloud::scheduler` line via the existing supervisor path. + +**Legacy shim retirement** (Stage 2 — landed): + +The three legacy internal endpoints have been retired in favour of +the JobRegistry surface. Kept here for archaeology / URL migration +reference for any external tool that still expects the old paths: + +| Legacy (retired) | Replacement | +|---|---| +| `POST /admin/internal/trigger-sweep` | `POST /admin/jobs/storage_reconcile/trigger` | +| `POST /admin/internal/trigger-gc?force=X` | `POST /admin/jobs/dedup_gc/trigger?force=X` | +| `POST /admin/internal/trigger-grant-cleanup?force=X` | `POST /admin/jobs/grant_cleanup/trigger?force=X` | + +The `OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS` env var was removed +alongside — its sole purpose was gating those shims. + +Response shape also changed: the old endpoints returned custom fields +(`grants_deleted`, `blobs_deleted`, `bytes_freed`, `forced`); the new +endpoint returns a uniform `{ ok, outcome: JobOutcome }` envelope with +job-specific fields under `outcome.extra`. Any external caller reading +the old fields needs updating. + +### Config surface — env vars + +Canonical form for every job (Part 1 or Part 2 alike, AND for core +workers even though they don't register with the scheduler): + +``` +OXICLOUD_JOB__ENABLED +OXICLOUD_JOB__INTERVAL_HOURS # or _INTERVAL_SECS for sub-hour cadences +OXICLOUD_JOB__... # e.g. _GRACE_HOURS, _BATCH_SIZE +``` + +Core workers reuse this naming purely for uniform operator ergonomics +(e.g. `OXICLOUD_JOB_TREE_ETAG_FLUSH_INTERVAL_MS`) — the convention is +what operators grep for; whether the loop is scheduler-driven or a +dedicated `tokio::spawn` is an implementation detail they don't see. + +Existing per-service env vars keep working as **aliases** during +migration — `OXICLOUD_GRANT_CLEANUP_INTERVAL_HOURS` reads first, falls +back to `OXICLOUD_JOB_GRANT_CLEANUP_INTERVAL_HOURS`. Deprecated aliases +warn once on startup and stay recognised through one minor version. + +### Logging schema + +Uniform structured target across both engines: + +```rust +tracing::info!( + target: "oxicloud::scheduler", + event = "job.run", + job = %name, + outcome = %outcome_kind, // "ok" | "err" + cause = %cause, // omitted on ok; "handler" | "timeout" | "panicked" + count = ..., + elapsed_ms = ..., + // extras from the JobOutcome::Ok.extra map, flattened + ..., + "job {name} ran" +); +``` + +Security-relevant jobs (grant cleanup, authz cache invalidation) still +double-log to `target: "audit"` — the scheduler channel is for +observability; the audit channel is for compliance. + +For Part 2 handlers, the same log line fires at run completion. The +`extra` map surfaces `completed`/`paused`/`cursor_hex` per the +`RunOutcome` bridge above. + +### Composability + +A recoverable job CAN also be periodically-triggered — register with +both engines. Part 1's tick calls Part 2's `run_or_resume(job, store_factory).await` +as its handler. The exclusivity index in Part 2 makes this safe even +if the interval is short enough that a tick fires while a previous +run is still going: the second tick's `run_or_resume` short-circuits +to "already running." + +### Ordering and dependencies (deferred) + +Cross-job dependencies (e.g. "trash cleanup runs before dedup GC") +are not modelled. Every job runs independently. If a real ordering +constraint appears, we add a `depends_on: Vec` field and +topological scheduling then. + +### Shutdown coordination (deferred) + +Matches the existing daemons: no cancellation channel. The scheduler +task dies with the runtime. Recoverable jobs surviving a hard shutdown +land as `Paused` on the next boot via the sweep. If graceful shutdown +lands elsewhere in the codebase, the scheduler and all jobs migrate +together. + +### Future extension — plugins + +Once these engines exist they become the natural place for Extism +plugins to declare scheduled work — manifest `[[jobs]]` entries, +registered on `on_plugin_loaded`, unregistered on unload. Deliberately +deferred: no plugin needs it today, and adding +`JobOwner { Native | Plugin { id } }` + `unregister_by_owner` is a +small type extension the day one does. Nothing in the v1 design +precludes it. + +### Job-history observability + +`admin.background_runs` already carries the latest run per Part 2 job +— "last run time + status" is a `SELECT DISTINCT ON (job_name) …` +query. Deeper history (retention window, per-run drill-down UI) is +deferred; the log stream is the source of truth for older runs. + +Part 1's periodic jobs only carry the last outcome IN MEMORY — no +DB row. If a periodic-only job needs persisted last-run visibility, +either promote it to a "trivial" recoverable job (immediate +`Completed`) or add a small `admin.periodic_runs_last` table later. +No such need today. + +## Out of scope + +- **Cross-job dependencies.** Register-time ordering only, not runtime + graph. +- **Retention pruning of terminal `background_runs` rows.** Deferred + until the volume warrants a policy. +- **Prometheus / OpenMetrics export.** Log-only for now. +- **Distributed scheduling.** Single-process. If OxiCloud ever runs + multi-node, `SELECT … FOR UPDATE SKIP LOCKED` on the runs table is + the pattern; not now. +- **Backfill on startup.** If the process is down when a Part 1 job's + tick was due, we do NOT catch up — the job runs at its next + interval. Matches every existing daemon's behaviour today. +- **Cron expressions.** Fixed intervals only. +- **Rate limiting the admin trigger endpoint.** It's already + admin-gated. + +## Related memory notes + +- `feedback_no_abbreviated_env_vars` — full-word env var names + (`OXICLOUD_JOB_TRASH_CLEANUP_INTERVAL_HOURS`, not + `OXICLOUD_JOB_TC_INTERVAL_H`). +- The grant-cleanup implementation is the closest reference for the + Part 1 daemon → tenant migration shape: three env vars, one impl of + an authz trait method, one daemon service, one admin trigger. +- `project_consistency_check_trait` — the consistency framework + described in `docs/plan/consistency-check.md` is a *consumer* of + Part 2 (the recoverable-run engine), not a peer. It ships after + Part 2 lands. diff --git a/example.env b/example.env index 5004bde0..9c673fe0 100644 --- a/example.env +++ b/example.env @@ -72,19 +72,6 @@ OXICLOUD_SERVER_HOST=127.0.0.1 # higher = less background DB work. Minimum enforced: 30s. #OXICLOUD_STORAGE_USAGE_RECONCILE_SECS=600 -# Test-only sweep triggers under /api/admin/internal/*. -# When true, exposes: -# POST /api/admin/internal/trigger-sweep — run the storage-usage -# reconciliation synchronously -# POST /api/admin/internal/trigger-gc — run the blob garbage collector -# synchronously -# Used by the Hurl / integration suites to assert post-delete quota -# convergence without waiting out the periodic ticker (default 600 s). -# These endpoints short-circuit operator-visible background cadence, so -# leave OFF in production — when disabled, the routes return 404 even -# to an admin token. Default: false. -#OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=false - # Native WebDAV URL segment that returns the drive listing. Sanitized # by trimming leading/trailing `/` so `/@drive/`, `@drive`, and # `@drive/` are equivalent. Three deployment modes: diff --git a/src/application/ports/storage_ports.rs b/src/application/ports/storage_ports.rs index 697fad68..48efcb26 100644 --- a/src/application/ports/storage_ports.rs +++ b/src/application/ports/storage_ports.rs @@ -420,8 +420,13 @@ pub trait StorageUsagePort: Send + Sync + 'static { username: &str, ) -> Result; - /// Updates storage usage statistics for all users - async fn update_all_users_storage_usage(&self) -> Result<(), DomainError>; + /// Reconcile every user's cached `storage_used_bytes` against the + /// authoritative `SUM(drives.used_bytes)` over their personal + /// drives. Returns the number of user rows the sweep actually + /// corrected (`rows_affected` from the `UPDATE … IS DISTINCT + /// FROM …`) — used by the periodic-scheduler `JobHandler` to + /// populate `JobOutcome::Ok.count`. + async fn update_all_users_storage_usage(&self) -> Result; /// Checks if a user has enough quota for an additional upload. /// Returns Ok(()) if the upload is allowed, or Err(QuotaExceeded) with a @@ -455,7 +460,10 @@ pub trait StorageUsagePort: Send + Sync + 'static { /// `GROUP BY drive_id` aggregate, with an `IS DISTINCT FROM` /// guard so idle drives don't churn dead tuples. Runs from the /// same reconciliation ticker. - async fn update_all_drives_storage_usage(&self) -> Result<(), DomainError>; + /// + /// Returns the number of drive rows actually corrected — used by + /// the periodic-scheduler `JobHandler` to populate `JobOutcome::Ok.count`. + async fn update_all_drives_storage_usage(&self) -> Result; /// Pre-upload quota check on a single drive. /// diff --git a/src/application/services/storage_usage_service.rs b/src/application/services/storage_usage_service.rs index 24fadc87..36ab045b 100644 --- a/src/application/services/storage_usage_service.rs +++ b/src/application/services/storage_usage_service.rs @@ -3,7 +3,6 @@ use crate::common::errors::DomainError; use crate::infrastructure::repositories::pg::UserPgRepository; use sqlx::PgPool; use std::sync::Arc; -use tokio::task; use tracing::{debug, error, info}; use uuid::Uuid; @@ -66,8 +65,8 @@ impl StorageUsageService { /// `GET /api/drives` therefore lags by up to the cache TTL (30 s), /// which matches the sibling caches' accepted UX phantom for /// drive-name staleness. Tests / operators that need immediate - /// freshness call `POST /api/admin/internal/trigger-sweep`, which - /// runs `update_all_drives_storage_usage` → this method. + /// freshness call `POST /api/admin/jobs/storage_reconcile/trigger`, + /// which runs `update_all_drives_storage_usage` → this method. /// /// Security posture unaffected: `check_drive_quota` reads /// directly from SQL, bypassing the cache entirely, so quota @@ -569,43 +568,59 @@ impl StorageUsageService { Ok(()) } - /// Spawn a background task that periodically reconciles every user's cached - /// `storage_used_bytes` against the actual sum of their files. + /// Interval helper — same clamping (min 30 s) the retired + /// `start_reconciliation_job` applied, exposed so DI can pass a + /// sanitised `Duration` to `JobRegistry::register`. + pub fn reconciliation_interval(interval_secs: u64) -> std::time::Duration { + std::time::Duration::from_secs(interval_secs.max(30)) + } +} + +pub const STORAGE_RECONCILE_JOB_NAME: &str = "storage_reconcile"; + +use crate::infrastructure::scheduler::{JobHandler, JobOutcome, JobRunArgs}; +use async_trait::async_trait; + +#[async_trait] +impl JobHandler for StorageUsageService { + fn name(&self) -> &str { + STORAGE_RECONCILE_JOB_NAME + } + + /// Runs both reconciliation sweeps — drives first, then users — + /// and reports the total number of rows corrected. /// - /// `GET /api/auth/me` no longer recomputes usage on the request path; this - /// sweep (plus the per-upload update) keeps the cached value current for - /// all mutations — including deletes and trash — without any O(N) work on a - /// hot endpoint. Runs on the maintenance pool. The first sweep is deferred - /// by one interval so it never adds load at boot. - pub fn start_reconciliation_job(&self, interval_secs: u64) { - // Floor the interval so a misconfiguration can't busy-loop the sweep. - let interval_secs = interval_secs.max(30); - let service = self.clone(); - info!( - "Starting storage-usage reconciliation job (every {}s)", - interval_secs - ); - task::spawn(async move { - let mut ticker = tokio::time::interval(std::time::Duration::from_secs(interval_secs)); - // tokio's first `tick()` fires immediately — consume it so the - // first real sweep happens one interval after startup. - ticker.tick().await; - loop { - ticker.tick().await; - debug!("Running scheduled storage-usage reconciliation"); - // Drive sweep runs FIRST: the user-side sweep below - // reads `drives.used_bytes` (the per-drive sum) to - // compute its own counter, so the drive counter must - // be honest first. Failure of one is logged but - // doesn't skip the other or the next tick. - if let Err(e) = service.update_all_drives_storage_usage().await { - error!("Scheduled drive storage-usage reconciliation failed: {}", e); - } - if let Err(e) = service.update_all_users_storage_usage().await { - error!("Scheduled user storage-usage reconciliation failed: {}", e); - } - } - }); + /// **Sweep order matters.** The user envelope is computed from + /// `SUM(drives.used_bytes)` over the caller's personal drives + /// (`docs/plan/drive.md` §7). Running the user sweep before the + /// drive sweep would read a stale per-drive counter and freeze + /// the user counter on the previous tick's numbers — invisible in + /// steady state, breaks any Hurl that trashes + sweeps within one + /// call. See memory note `bug_storage_sweep_order_drive_first`. + /// + /// Failure of one sub-sweep short-circuits the tick to `Err`; + /// operators see `outcome=err, cause=handler` in the scheduler + /// log and the individual sweep's own `error!` line above it. + /// + /// `args.force` is ignored — reconciliation is idempotent and has + /// no acceleration semantics; every run does the same set-based + /// UPDATE regardless. + async fn run(&self, _args: &JobRunArgs) -> JobOutcome { + let drives = match self.update_all_drives_storage_usage().await { + Ok(n) => n, + Err(e) => return JobOutcome::err(format!("drive reconciliation failed: {e}")), + }; + let users = match self.update_all_users_storage_usage().await { + Ok(n) => n, + Err(e) => return JobOutcome::err(format!("user reconciliation failed: {e}")), + }; + JobOutcome::ok_with( + drives + users, + serde_json::json!({ + "drives_corrected": drives, + "users_corrected": users, + }), + ) } } @@ -638,7 +653,7 @@ impl StorageUsagePort for StorageUsageService { /// /// External users are excluded — they carry no storage by construction /// (DB CHECK `users_external_no_storage`). - async fn update_all_users_storage_usage(&self) -> Result<(), DomainError> { + async fn update_all_users_storage_usage(&self) -> Result { debug!("Starting storage-usage reconciliation sweep"); // User envelope = SUM of `drives.used_bytes` across the user's @@ -681,11 +696,12 @@ impl StorageUsagePort for StorageUsageService { DomainError::internal_error("StorageUsage", format!("reconciliation sweep: {e}")) })?; + let corrected = result.rows_affected(); info!( "Storage-usage reconciliation corrected {} user(s)", - result.rows_affected() + corrected ); - Ok(()) + Ok(corrected) } async fn check_storage_quota( @@ -720,7 +736,7 @@ impl StorageUsagePort for StorageUsageService { /// FROM` guard to skip no-op rewrites so idle drives don't churn /// dead tuples. Runs from the same reconciliation ticker as the /// user sweep; failure is logged but doesn't stop the next tick. - async fn update_all_drives_storage_usage(&self) -> Result<(), DomainError> { + async fn update_all_drives_storage_usage(&self) -> Result { debug!("Starting drive storage-usage reconciliation sweep"); let result = sqlx::query( r#" @@ -744,9 +760,10 @@ impl StorageUsagePort for StorageUsageService { DomainError::internal_error("StorageUsage", format!("drive reconciliation sweep: {e}")) })?; + let corrected = result.rows_affected(); info!( "Drive storage-usage reconciliation corrected {} drive(s)", - result.rows_affected() + corrected ); // Unconditional invalidation — do NOT gate on // `rows_affected() > 0`. When a fire-and-forget delta has @@ -762,7 +779,7 @@ impl StorageUsagePort for StorageUsageService { // already right → zero rows → without unconditional // invalidation, cache stays at the previous step's value. self.invalidate_drive_lookup_caches(); - Ok(()) + Ok(corrected) } async fn check_drive_quota( diff --git a/src/common/config.rs b/src/common/config.rs index b0a37d3a..6d5bacfd 100644 --- a/src/common/config.rs +++ b/src/common/config.rs @@ -1078,15 +1078,6 @@ pub struct FeaturesConfig { /// trash/search). OFF by default — opt-in per deployment. /// Env: `OXICLOUD_ENABLE_EXTERNAL_MOUNTS`. pub enable_external_mounts: bool, - /// Expose `/api/admin/internal/*` test-only endpoints that trigger - /// background sweeps on demand (storage-usage reconciliation, blob - /// GC). Intended for Hurl / integration tests that need to wait - /// for these maintenance jobs deterministically rather than - /// polling the cached value. Off by default — these endpoints - /// short-circuit the operator-visible cadence, so production - /// deployments don't want them reachable. Env: - /// `OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS`. - pub enable_admin_internal_endpoints: bool, /// Native WebDAV path segment that lists the caller's drives. /// /// * Default `"@drive"` — bare `/webdav/` addresses the caller's @@ -1163,10 +1154,6 @@ impl Default for FeaturesConfig { expose_system_users: true, // Expose OxiCloud users as address book by default enable_video_thumbnails: true, // Video thumbs via ffmpeg (if detected) enable_external_mounts: false, // External mounts — opt-in, off by default - // Test-only sweep triggers — strictly opt-in. Production - // deployments do NOT need this; the periodic ticker handles - // reconciliation transparently. - enable_admin_internal_endpoints: false, // Back-compat with pre-multi-drive clients — bare `/webdav/` // maps to the caller's default drive; drive listing is // reachable at `/webdav/@drive/`. @@ -1867,16 +1854,6 @@ impl AppConfig { config.features.enable_video_thumbnails = val; } - // `/api/admin/internal/*` test-only triggers. Disabled by - // default; production deployments never need this. The Hurl - // suite flips it on via `OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true`. - if let Ok(enable_internal) = - env::var("OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS").map(|v| v.parse::()) - && let Ok(val) = enable_internal - { - config.features.enable_admin_internal_endpoints = val; - } - // Grant-cleanup daemon. Purges rows from `storage.role_grants` // whose `expires_at` is more than `grace_days` in the past. // See `GrantCleanupConfig` for defaults + rationale. diff --git a/src/common/di.rs b/src/common/di.rs index e6d6dc27..83f5b1cf 100644 --- a/src/common/di.rs +++ b/src/common/di.rs @@ -39,6 +39,7 @@ use crate::infrastructure::repositories::pg::{ FileBlobReadRepository, FileBlobWriteRepository, FileMetadataRepository, FolderDbRepository, TrashDbRepository, }; +use crate::infrastructure::scheduler::{JobRegistry, SchedulerEngine}; use crate::infrastructure::services::file_content_cache::{ FileContentCache, FileContentCacheConfig, }; @@ -429,6 +430,12 @@ impl AppServiceFactory { } let file_lifecycle = Arc::new(fls); + // Empty periodic-job registry; services register themselves + // downstream during their own creation. `SchedulerEngine::start` + // fires at the end of `build_app_state` once all registrations + // have landed. + let job_registry = Arc::new(JobRegistry::new()); + Ok(CoreServices { path_service, file_content_cache, @@ -441,6 +448,7 @@ impl AppServiceFactory { dedup_service, zip_service: None, // Placeholder - replaced after app services init config: self.config.clone(), + job_registry, }) } @@ -865,14 +873,32 @@ impl AppServiceFactory { // Initialize cleanup service (bulk-deletes expired items in 2 SQL // queries, then GCs zero-reference blobs — including chunks orphaned // by aborted streaming uploads). - let cleanup_service = TrashCleanupService::new( + // + // Registers with the periodic-job scheduler + // (`docs/plan/job-registry.md` Part 1) instead of spawning its own + // tokio interval loop. `SchedulerEngine::start` fires the actual + // supervisor task at the end of `build_app_state`. + let cleanup_service = Arc::new(TrashCleanupService::new( trash_repo.clone(), core.dedup_service.clone(), 24, // Run cleanup every 24 hours - ); - - cleanup_service.start_cleanup_job().await; - tracing::info!("Trash service initialized with daily cleanup schedule"); + )); + let interval = cleanup_service.interval(); + if let Err(e) = core + .job_registry + .register(cleanup_service.clone(), Some(interval), None) + .await + { + // Duplicate registration is the only failure mode today and + // shouldn't happen in the normal DI flow. Log + continue so + // trash service still lands even if scheduling didn't. + tracing::error!("Failed to register trash_cleanup job with scheduler: {e}"); + } else { + tracing::info!( + "Trash cleanup registered with scheduler (interval {} h)", + interval.as_secs() / 3600 + ); + } Some(service as Arc) } @@ -1078,12 +1104,16 @@ impl AppServiceFactory { /// /// Uses the `maintenance_pool` for batch operations /// (`update_all_users_storage_usage`) to avoid starving user requests. - pub fn create_storage_usage_service( + /// + /// Note: this is `async` (unlike the pre-migration version) because + /// registration with `core.job_registry` requires an `await`. + pub async fn create_storage_usage_service( &self, _repos: &RepositoryServices, db_pool: &Arc, maintenance_pool: &Arc, drive_repo: Arc, + core: &CoreServices, ) -> Arc { let user_repository = Arc::new( crate::infrastructure::repositories::pg::UserPgRepository::new(db_pool.clone()), @@ -1105,11 +1135,26 @@ impl AppServiceFactory { as Arc, ), ); - // Keep cached storage usage fresh off the request path: GET /api/auth/me - // no longer recomputes the O(N) SUM per call; a periodic sweep does it - // instead (on the maintenance pool). - service.start_reconciliation_job(self.config.storage.usage_reconcile_secs); - tracing::info!("Storage usage service initialized"); + // Keep cached storage usage fresh off the request path: GET + // /api/auth/me no longer recomputes the O(N) SUM per call; a + // periodic sweep does it instead (on the maintenance pool). + // Registered with the periodic-job scheduler + // (`docs/plan/job-registry.md` Part 1); the retired + // `start_reconciliation_job` used to spawn its own interval loop. + let interval = + StorageUsageService::reconciliation_interval(self.config.storage.usage_reconcile_secs); + if let Err(e) = core + .job_registry + .register(service.clone(), Some(interval), None) + .await + { + tracing::error!("Failed to register storage_reconcile job: {e}"); + } else { + tracing::info!( + "Storage-usage reconciliation registered with scheduler (interval {}s)", + interval.as_secs() + ); + } service } @@ -1231,6 +1276,26 @@ impl AppServiceFactory { // 1. Core services (PgPool needed for DedupService index) let core = self.create_core_services(&pool, &maintenance_pool).await?; + // Register on-demand-only jobs whose owning service lives on + // CoreServices. Dedup GC has NO periodic tick — trash cleanup's + // sweep already runs GC as its tail step, so a periodic dedup + // schedule would double the work. Registering with `interval = + // None` keeps it admin-triggerable through the uniform scheduler + // surface (`POST /api/admin/jobs/dedup_gc/trigger`). + if let Err(e) = core + .job_registry + .register( + core.dedup_service.clone() as Arc, + None, // on-demand only + None, // no timeout + ) + .await + { + tracing::error!("Failed to register dedup_gc job with scheduler: {e}"); + } else { + tracing::info!("Dedup GC registered with scheduler (on-demand only)"); + } + // 2. Repository services (requires PgPool for all metadata) let repos = self.create_repository_services(&core, &pool); @@ -1289,8 +1354,15 @@ impl AppServiceFactory { // 3c. Storage usage / quota service (needed by the instant-upload // path inside the application services, and re-exposed on AppState // for the handler-side quota checks of the byte-upload paths). - let storage_usage = - self.create_storage_usage_service(&repos, &pool, &maintenance_pool, drive_repo.clone()); + let storage_usage = self + .create_storage_usage_service( + &repos, + &pool, + &maintenance_pool, + drive_repo.clone(), + &core, + ) + .await; // 3d. Content index (embedded Tantivy) — opened before application // services so SearchService can hold the query port; the feeding @@ -1419,9 +1491,25 @@ impl AppServiceFactory { core.config.features.grant_cleanup.interval_hours, ), ); - // First tick fires immediately inside start_cleanup_job — - // matches the trash/storage-usage daemon shape. - svc.clone().start_cleanup_job().await; + // Registered with the periodic-job scheduler + // (`docs/plan/job-registry.md` Part 1); the retired + // `start_cleanup_job` used to spawn its own interval loop. + // Admin `?force=true` trigger still calls `svc.purge(Some(0))` + // directly — grace override doesn't fit the JobHandler shape. + let interval = svc.interval(); + if let Err(e) = core + .job_registry + .register(svc.clone(), Some(interval), None) + .await + { + tracing::error!("Failed to register grant_cleanup job: {e}"); + } else { + tracing::info!( + "Grant cleanup registered with scheduler (every {}h, grace = {}d)", + interval.as_secs() / 3600, + core.config.features.grant_cleanup.grace_days, + ); + } Some(svc) } else { tracing::info!( @@ -1805,6 +1893,11 @@ impl AppServiceFactory { 50_000, ), ), + // Populated below once every service has finished registering + // with `core.job_registry`. Starting the engine before all + // registrations land would race the first tick against + // late-registered jobs. + scheduler_engine: None, }; let email_bundle = build_email_sender(&self.config.smtp); app_state.email_sender = email_bundle.sender; @@ -2079,6 +2172,21 @@ impl AppServiceFactory { } } + // Start the periodic-job scheduler AFTER every native service has + // finished registering its jobs on `core.job_registry`. Starting + // it earlier would race the first tick against late registrations. + // See `docs/plan/job-registry.md` Part 1. + let registered = app_state.core.job_registry.len().await; + let engine = SchedulerEngine::start(app_state.core.job_registry.clone()); + app_state.scheduler_engine = Some(Arc::new(engine)); + tracing::info!( + target: "oxicloud::scheduler", + event = "scheduler.ready", + registered = registered, + "periodic scheduler ready ({} job(s) registered)", + registered + ); + Ok(app_state) } } @@ -2099,6 +2207,11 @@ pub struct CoreServices { pub dedup_service: Arc, pub zip_service: Option>, pub config: AppConfig, + /// Periodic-job scheduler registry. Services that satisfy the + /// migration criterion (`docs/plan/job-registry.md`) `register()` + /// themselves here during their creation; `SchedulerEngine::start` + /// spins up the supervisor loop at the end of `build_app_state`. + pub job_registry: Arc, } /// Container for repository services @@ -2196,11 +2309,12 @@ pub struct AppState { pub places_service: Option>, pub people_service: Option>, pub storage_usage_service: Option>, - /// Handle to the background daemon that purges expired - /// `storage.role_grants` rows. `None` when the daemon is disabled - /// via `OXICLOUD_GRANT_CLEANUP_ENABLED=false`. The admin - /// `POST /api/admin/internal/trigger-grant-cleanup` handler uses - /// this to invoke the purge on demand (test-only). + /// Handle to the service that purges expired `storage.role_grants` + /// rows. Registered with the periodic-job scheduler on the + /// configured cadence; `None` when disabled via + /// `OXICLOUD_GRANT_CLEANUP_ENABLED=false`. Exposed on `AppState` + /// so the admin trigger endpoint can invoke `purge(Some(0))` for + /// the `?force=true` grace-override path. pub grant_cleanup_service: Option< Arc, >, @@ -2305,6 +2419,14 @@ pub struct AppState { /// Authenticated callers bypass this limit. pub magic_link_send_per_ip_rate_limiter: Arc, + /// Handle to the periodic-job scheduler's supervisor task, spawned + /// at the end of `build_app_state` after every native service has + /// registered its jobs on `core.job_registry`. `Option` because + /// tests that assemble a partial `AppState` (no full DI) skip the + /// scheduler; production always populates it. Held here purely so + /// the tokio task isn't dropped — the supervisor loop runs off its + /// internal `JoinHandle`, not off this reference. + pub scheduler_engine: Option>, } // All AppState construction is done via struct literal in build_app_state(). diff --git a/src/infrastructure/mod.rs b/src/infrastructure/mod.rs index c6383e4d..ee93db7c 100644 --- a/src/infrastructure/mod.rs +++ b/src/infrastructure/mod.rs @@ -2,4 +2,5 @@ pub mod adapters; pub mod auth_factory; pub mod db; pub mod repositories; +pub mod scheduler; pub mod services; diff --git a/src/infrastructure/scheduler/engine.rs b/src/infrastructure/scheduler/engine.rs new file mode 100644 index 00000000..35255d55 --- /dev/null +++ b/src/infrastructure/scheduler/engine.rs @@ -0,0 +1,419 @@ +//! The scheduler supervisor loop. +//! +//! One `tokio::spawn` at startup runs [`SchedulerEngine::run`]. The +//! loop iterates: +//! +//! 1. `pick_next()` — find the job with the earliest `next_run_at`. +//! 2. Sleep until that instant. +//! 3. Dispatch: try-acquire the job's in-flight permit; if held, warn +//! and reschedule; otherwise spawn the handler, apply the timeout, +//! catch panics, record the outcome. +//! +//! Sequential dispatch is intentional. Two jobs due at the same +//! instant run one-after-the-other — the second's `pick_next` fires +//! immediately after the first's dispatch returns, with a zero-length +//! sleep. See `docs/plan/job-registry.md` Part 1 §Runtime model. + +use std::sync::Arc; +use std::time::{Duration, Instant}; + +use chrono::Utc; +use tokio::task::JoinHandle; + +use super::registry::{JobEntry, JobRegistry}; +use super::types::{ErrCause, JobOutcome, JobRunArgs}; + +/// Public handle to the running supervisor. +/// +/// Dropping the handle does NOT cancel the loop (the spawned task +/// runs until the runtime dies). Explicit shutdown is deferred to +/// whenever graceful-shutdown lands globally — matches the shape +/// every other daemon in the codebase has today. See +/// `docs/plan/job-registry.md` Part 1 §Shutdown coordination. +pub struct SchedulerEngine { + _handle: JoinHandle<()>, +} + +impl SchedulerEngine { + /// Spawn the supervisor loop and return a handle. Callers hold + /// the returned `SchedulerEngine` on `AppState` so the task lives + /// as long as the runtime. + pub fn start(registry: Arc) -> Self { + let handle = tokio::spawn(async move { + run(registry).await; + }); + Self { _handle: handle } + } +} + +/// If the registry is empty (no jobs registered yet), sleep this long +/// before rechecking. Registration happens once at boot in the current +/// design, so this only matters as a defensive fallback — in practice +/// the loop enters this branch at most once, right before the first +/// `register()` call completes. +const IDLE_POLL: Duration = Duration::from_secs(60); + +async fn run(registry: Arc) { + tracing::info!( + target: "oxicloud::scheduler", + event = "scheduler.started", + "periodic scheduler supervisor started" + ); + + loop { + // `pick_next` only returns scheduled jobs (interval = Some); + // on-demand jobs never appear here and are only reachable + // through `JobRegistry::trigger`. + let Some((name, next_at)) = registry.pick_next().await else { + tokio::time::sleep(IDLE_POLL).await; + continue; + }; + + // Convert to `Duration`. If `next_at` is in the past (missed + // tick, e.g. very short interval and the previous dispatch + // took longer than the interval), sleep zero and dispatch + // immediately. + let now = Utc::now(); + let sleep_dur = (next_at - now) + .to_std() + .unwrap_or_else(|_| Duration::from_millis(0)); + if !sleep_dur.is_zero() { + tokio::time::sleep(sleep_dur).await; + } + + // The job's `next_run_at` might have changed since `pick_next` + // returned if a concurrent trigger fired — that's fine; the + // dispatch below re-reads via the `JobEntry` snapshot. + let Some(entry) = registry.get(&name).await else { + // Job was unregistered between pick_next and dispatch — + // unreachable in the current design (no unregister), but + // guard defensively. + continue; + }; + + // Fire and forget from the supervisor's perspective — we + // don't care about the outcome, `dispatch` records it on the + // entry and emits the log line itself. Periodic ticks never + // force — that's an admin-trigger-only affordance. + let _ = dispatch(&name, entry, &JobRunArgs::default()).await; + } +} + +/// Dispatch a single run of `name`. Handles: +/// - exclusivity: try-acquire the in-flight permit; skip + warn if held, +/// - spawning under panic containment (via `tokio::spawn` + `JoinHandle`), +/// - timeout enforcement (if `ScheduledJob.timeout` is set), +/// - recording `last_outcome` + advancing `next_run_at` on completion, +/// - emitting the uniform `oxicloud::scheduler::job.run` log line. +/// +/// Returns the [`JobOutcome`] the run produced. The scheduler loop +/// discards this (records-only-via-side-effect); admin/programmatic +/// callers via [`JobRegistry::trigger`](super::registry::JobRegistry::trigger) +/// surface it to the caller. +/// +/// Non-panicking; every failure path resolves to a `JobOutcome::Err` +/// with a `cause` log field. +/// +/// `args` is passed through to `JobHandler::run`. The supervisor's +/// periodic ticks pass `JobRunArgs::default()`; the admin trigger +/// endpoint forwards parsed query params such as `?force=true`. +pub(super) async fn dispatch(name: &str, entry: Arc, args: &JobRunArgs) -> JobOutcome { + // Try to acquire the single-permit gate. `try_acquire` is + // non-blocking — if held, we know the previous run is still + // executing and skip this tick. + let permit = match entry.in_flight.try_acquire() { + Ok(p) => p, + Err(_) => { + // Someone else holds the permit → previous run still in + // flight. Emit the operator-signal warning and reschedule. + let running_for_ms = { + let state = entry.state.lock().expect("JobState mutex poisoned"); + state + .current_run_start + .map(|t| t.elapsed().as_millis()) + .unwrap_or(0) + }; + // On-demand jobs have `interval = None`; log 0 rather than + // fabricate one. Operators reading this line for a scheduled + // job compare `interval_ms` vs `running_for_ms`; the same + // line for an on-demand job just tells them a concurrent + // trigger raced an in-flight run. + let interval_ms = entry.interval.map(|d| d.as_millis()).unwrap_or(0); + tracing::warn!( + target: "oxicloud::scheduler", + event = "job.tick_skipped", + job = %name, + interval_ms = interval_ms, + running_for_ms = running_for_ms, + "{} still running past its interval — tick skipped", + name, + ); + advance_next_run(&entry); + return JobOutcome::ok_with(0, serde_json::json!({ "skipped": "already_running" })); + } + }; + + // We hold the permit. Record run-start, spawn, await, translate. + { + let mut state = entry.state.lock().expect("JobState mutex poisoned"); + state.current_run_start = Some(Instant::now()); + } + let started_wall = Utc::now(); + let start_instant = Instant::now(); + + // Spawn so panics land as `JoinError::is_panic()` instead of + // unwinding into the supervisor loop. Args cloned into the spawn + // scope so the borrow doesn't outlive the caller. + let handler = entry.handler.clone(); + let args_owned = args.clone(); + let join = tokio::spawn(async move { handler.run(&args_owned).await }); + + let (outcome, cause) = match entry.timeout { + Some(dur) => match tokio::time::timeout(dur, join).await { + Ok(res) => translate_join(res), + Err(_elapsed) => { + // Timeout fired. The JoinHandle is dropped, which + // aborts the spawned task cooperatively — but abort + // is best-effort in Rust; a handler that ignores + // yield points may run to completion in the background. + // We still record timeout and release the permit. + ( + JobOutcome::err(format!("wall-clock timeout of {:?} exceeded", dur)), + Some(ErrCause::Timeout), + ) + } + }, + None => translate_join(join.await), + }; + + let elapsed_ms = start_instant.elapsed().as_millis(); + + // Record outcome and advance the schedule. Permit drops naturally + // when `permit` goes out of scope at the end of the function. + { + let mut state = entry.state.lock().expect("JobState mutex poisoned"); + state.current_run_start = None; + state.last_outcome = Some((started_wall, outcome.clone())); + // Only scheduled jobs advance next_run_at. On-demand jobs stay + // at None so `pick_next` never returns them, even after a + // trigger. Same rule as the skip branch — schedule advances + // by one interval, no backlog queueing. + state.next_run_at = entry.interval.map(|dur| { + Utc::now() + + chrono::Duration::from_std(dur).unwrap_or_else(|_| chrono::Duration::seconds(0)) + }); + } + + // Log line. `outcome=ok` runs are informational; `outcome=err` include + // the diagnostic `cause` field. + log_outcome(name, &outcome, cause, elapsed_ms); + + drop(permit); + outcome +} + +/// Advance `next_run_at` by one interval without touching outcome or +/// run-start (skip-path helper). No-op for on-demand jobs — `interval` +/// is `None`, so `next_run_at` stays `None` and `pick_next` continues +/// to skip them. +fn advance_next_run(entry: &JobEntry) { + let mut state = entry.state.lock().expect("JobState mutex poisoned"); + state.next_run_at = entry.interval.map(|dur| { + Utc::now() + + chrono::Duration::from_std(dur).unwrap_or_else(|_| chrono::Duration::seconds(0)) + }); +} + +/// Convert the `Result` returned by the spawned +/// handler into `(JobOutcome, Option)`. `cause` is `None` +/// on Ok, `Some(_)` on Err. +fn translate_join( + res: Result, +) -> (JobOutcome, Option) { + match res { + Ok(outcome) => { + let cause = if outcome.is_ok() { + None + } else { + Some(ErrCause::Handler) + }; + (outcome, cause) + } + Err(join_err) if join_err.is_panic() => { + let payload = join_err.into_panic(); + let msg = if let Some(s) = payload.downcast_ref::<&'static str>() { + (*s).to_string() + } else if let Some(s) = payload.downcast_ref::() { + s.clone() + } else { + "unknown panic payload".to_string() + }; + ( + JobOutcome::err(format!("handler panicked: {msg}")), + Some(ErrCause::Panicked), + ) + } + Err(join_err) => ( + JobOutcome::err(format!("task cancelled: {join_err}")), + Some(ErrCause::Handler), + ), + } +} + +/// Emit the uniform `oxicloud::scheduler` log line for a completed run. +/// Distinct Ok/Err branches so the tracing macros pick up the fields at +/// compile time — `tracing` doesn't expand conditional field lists. +fn log_outcome(name: &str, outcome: &JobOutcome, cause: Option, elapsed_ms: u128) { + match outcome { + JobOutcome::Ok { count, extra } => { + tracing::info!( + target: "oxicloud::scheduler", + event = "job.run", + job = %name, + outcome = "ok", + count = *count, + elapsed_ms = elapsed_ms, + extra = %extra, + "job {} ran", + name, + ); + } + JobOutcome::Err { message: msg } => { + tracing::warn!( + target: "oxicloud::scheduler", + event = "job.run", + job = %name, + outcome = "err", + cause = %cause.unwrap_or(ErrCause::Handler), + elapsed_ms = elapsed_ms, + error = %msg, + "job {} failed", + name, + ); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::infrastructure::scheduler::handler::JobHandler; + use async_trait::async_trait; + use std::sync::atomic::{AtomicU64, Ordering}; + + struct CountingHandler { + name: String, + calls: Arc, + sleep: Duration, + } + + #[async_trait] + impl JobHandler for CountingHandler { + fn name(&self) -> &str { + &self.name + } + async fn run(&self, _args: &JobRunArgs) -> JobOutcome { + self.calls.fetch_add(1, Ordering::SeqCst); + if !self.sleep.is_zero() { + tokio::time::sleep(self.sleep).await; + } + JobOutcome::ok(1) + } + } + + struct PanickingHandler; + + #[async_trait] + impl JobHandler for PanickingHandler { + fn name(&self) -> &str { + "panicker" + } + async fn run(&self, _args: &JobRunArgs) -> JobOutcome { + panic!("intentional test panic"); + } + } + + #[tokio::test] + async fn panic_containment_via_translate_join() { + // Directly exercise translate_join with a spawned panic — the + // supervisor loop's dispatch path uses this same helper. + let handler = Arc::new(PanickingHandler); + let join = tokio::spawn(async move { handler.run(&JobRunArgs::default()).await }); + let (outcome, cause) = translate_join(join.await); + assert!(!outcome.is_ok()); + assert_eq!(cause, Some(ErrCause::Panicked)); + if let JobOutcome::Err { message: msg } = outcome { + assert!(msg.contains("panicked"), "expected panic marker in: {msg}"); + } + } + + #[tokio::test] + async fn overrun_skips_second_tick() { + // Handler that sleeps 200 ms; two dispatches fired back-to-back + // should see the second skip with a `tick_skipped` warning. + let calls = Arc::new(AtomicU64::new(0)); + let handler = Arc::new(CountingHandler { + name: "overrun".to_string(), + calls: calls.clone(), + sleep: Duration::from_millis(200), + }); + + let registry = Arc::new(JobRegistry::new()); + registry + .register(handler, Some(Duration::from_millis(100)), None) + .await + .unwrap(); + let entry = registry.get("overrun").await.unwrap(); + + // Kick off dispatch 1 in the background — it holds the permit + // for ~200 ms. + let entry_bg = entry.clone(); + let bg = + tokio::spawn( + async move { dispatch("overrun", entry_bg, &JobRunArgs::default()).await }, + ); + + // Give dispatch 1 time to grab the permit. + tokio::time::sleep(Duration::from_millis(50)).await; + + // Dispatch 2 should observe the permit taken and skip. + dispatch("overrun", entry.clone(), &JobRunArgs::default()).await; + + // Only dispatch 1's handler should have actually run so far. + assert_eq!(calls.load(Ordering::SeqCst), 1); + + bg.await.unwrap(); + assert_eq!(calls.load(Ordering::SeqCst), 1); + } + + #[tokio::test] + async fn timeout_records_err_and_releases_permit() { + let calls = Arc::new(AtomicU64::new(0)); + let handler = Arc::new(CountingHandler { + name: "slow".to_string(), + calls: calls.clone(), + sleep: Duration::from_millis(500), + }); + + let registry = Arc::new(JobRegistry::new()); + registry + .register( + handler, + Some(Duration::from_millis(100)), + Some(Duration::from_millis(50)), + ) + .await + .unwrap(); + let entry = registry.get("slow").await.unwrap(); + + dispatch("slow", entry.clone(), &JobRunArgs::default()).await; + + // The timeout fired; last_outcome must be Err. + let state = entry.state.lock().unwrap(); + let (_, outcome) = state.last_outcome.as_ref().expect("outcome recorded"); + assert!(!outcome.is_ok(), "expected timeout-Err, got {outcome:?}"); + + // Permit released — another dispatch could acquire it. + assert_eq!(entry.in_flight.available_permits(), 1); + } +} diff --git a/src/infrastructure/scheduler/handler.rs b/src/infrastructure/scheduler/handler.rs new file mode 100644 index 00000000..c90e8087 --- /dev/null +++ b/src/infrastructure/scheduler/handler.rs @@ -0,0 +1,79 @@ +//! The implementor-facing contract for a Part 1 (periodic) job. +//! +//! Everything a native service needs to write to plug into the periodic +//! scheduler is on this page. See `docs/plan/job-registry.md` Part 1 +//! for the design rationale and migration criterion (the "operator +//! trigger" question — if an operator would never +//! `POST /api/admin/jobs/{name}/trigger` for this loop, it doesn't +//! belong here; keep it as a core worker). + +use async_trait::async_trait; + +use super::types::{JobOutcome, JobRunArgs}; + +/// Implemented by every service that wants to run on a fixed interval +/// through the periodic scheduler. +/// +/// # Design shape +/// +/// A single method, `run()`. One tick = one call. The supervisor: +/// - fires it at the registered interval, +/// - catches panics (bad handlers crash their own run, not the scheduler), +/// - enforces the configured wall-clock timeout (if any), +/// - enforces exclusivity — a second tick that fires while a previous +/// run is still executing is **skipped, not queued**, with a +/// `job.tick_skipped` warning emitted (the operator signal that the +/// job is chronically slower than its cadence). +/// +/// Implementors write the body of `run()`. Everything else — logging, +/// timing, panic containment, exclusivity — is the supervisor's job. +/// +/// # `name()` guidance +/// +/// Return a stable, unique snake_case identifier. Log lines +/// (`job = %name`), admin listing, admin trigger URLs +/// (`POST /api/admin/jobs/{name}/trigger`) and env vars +/// (`OXICLOUD_JOB__INTERVAL_HOURS`) all key on this. Renaming +/// after release is a breaking change to operator scripts and log +/// dashboards. +/// +/// # `run()` guidance +/// +/// Return [`JobOutcome::Ok`] with a `count` scalar the operator finds +/// meaningful (rows swept, blobs GC'd, bytes reclaimed) plus optional +/// `extra` JSON. Return [`JobOutcome::Err`] on failure — the +/// supervisor logs it under `outcome=err, cause=handler` and moves +/// on; the next tick fires normally. +/// +/// **Do not** catch panics inside `run()` — the supervisor does it, +/// and hiding one loses the `cause=panicked` diagnostic signal. +/// +/// **Do not** call `tokio::time::sleep` for long durations inside +/// `run()` if you have a `timeout` configured — the timeout fires +/// mid-sleep and kills the run with `cause=timeout`. Use short polling +/// intervals or restructure the work. +/// +/// # Reference implementation +/// +/// See `TrashCleanupService::run` (once migrated) as the canonical +/// example: reads its own configuration, runs bounded work, returns +/// a count. Everything else is boilerplate the scheduler owns. +#[async_trait] +pub trait JobHandler: Send + Sync { + /// Stable, unique snake_case identifier. Must be unique across + /// the process; the registry rejects duplicate registration. + fn name(&self) -> &str; + + /// One execution. Called at the registered interval and (optionally) + /// on admin trigger. + /// + /// `args` carries per-dispatch parameters (`force: bool` today). + /// Periodic ticks pass [`JobRunArgs::default()`]; admin triggers + /// forward query params such as `?force=true`. Handlers that don't + /// understand a given arg silently ignore it — the arg exists to + /// give per-job acceleration semantics without spreading per-job + /// knowledge into every caller. + /// + /// See trait-level docs for guidance on when to return Ok vs Err. + async fn run(&self, args: &JobRunArgs) -> JobOutcome; +} diff --git a/src/infrastructure/scheduler/mod.rs b/src/infrastructure/scheduler/mod.rs new file mode 100644 index 00000000..6eb449cc --- /dev/null +++ b/src/infrastructure/scheduler/mod.rs @@ -0,0 +1,33 @@ +//! Periodic job scheduler (Part 1 of the job-registry plan). +//! +//! In-memory registration + one-supervisor-task dispatch. Fires +//! registered [`JobHandler`] implementations at their configured +//! intervals with panic containment, timeout enforcement, and +//! same-name exclusivity. +//! +//! # For future implementors +//! +//! - **You write a `JobHandler`.** Implement [`JobHandler::name`] + +//! [`JobHandler::run`] on your service. Nothing else. See +//! [`handler`] for the guidance doc-comment. +//! - **DI wires the registration.** In `common/di.rs` (or wherever the +//! composition root lives), build an `Arc` once, register +//! every service that opts in, then call [`SchedulerEngine::start`]. +//! - **Should this loop actually be a scheduler job?** See the migration +//! criterion in `docs/plan/job-registry.md` — the primary question +//! is "would an operator plausibly trigger this manually?". Continuous +//! drains and event-reactive workers stay as their own loops. +//! +//! Part 2 (recoverable-run engine, DB-backed cursor + resume) is +//! designed but not yet implemented. When it lands it will slot in +//! as a sibling module without changing anything here. + +mod engine; +mod handler; +mod registry; +mod types; + +pub use engine::SchedulerEngine; +pub use handler::JobHandler; +pub use registry::{JobEntry, JobRegistry, JobSummary, RegisterError}; +pub use types::{ErrCause, JobOutcome, JobRunArgs}; diff --git a/src/infrastructure/scheduler/registry.rs b/src/infrastructure/scheduler/registry.rs new file mode 100644 index 00000000..4470f047 --- /dev/null +++ b/src/infrastructure/scheduler/registry.rs @@ -0,0 +1,368 @@ +//! In-memory registry of periodic jobs. +//! +//! The [`JobRegistry`] owns a map `name → JobEntry`. Native services +//! `register()` themselves during DI; the [`SchedulerEngine`](super::engine::SchedulerEngine) +//! iterates this map on every tick to pick the next-due job. +//! +//! Per-job state (in-flight semaphore, last outcome, next-run time) +//! lives inside each [`JobEntry`] behind a short-lived `std::sync::Mutex`. +//! The outer map uses a `tokio::sync::RwLock` so `pick_next` and +//! `snapshot` don't block one another and so future dynamic +//! registration (plugin manifests, admin UI) can acquire a write +//! lock without racing readers. + +use std::collections::HashMap; +use std::sync::{Arc, Mutex}; +use std::time::{Duration, Instant}; + +use chrono::{DateTime, Utc}; +use serde::Serialize; +use tokio::sync::{RwLock, Semaphore}; + +use super::handler::JobHandler; +use super::types::{JobOutcome, JobRunArgs}; + +/// A registered job plus its runtime state. Held as `Arc` +/// inside the registry so the engine can hold a snapshot across an +/// `await` without pinning the registry's outer lock. +pub struct JobEntry { + pub(super) handler: Arc, + /// `None` = on-demand only; the supervisor never fires this job + /// (`pick_next` skips it). Admin/programmatic callers reach it + /// via [`JobRegistry::trigger`]. + /// `Some(dur)` = periodic; supervisor dispatches every `dur`. + pub(super) interval: Option, + pub(super) timeout: Option, + /// Single-permit gate enforcing the "one in-flight run per + /// `job_name`" invariant. A tick that finds the permit taken + /// emits `job.tick_skipped` and does not spawn. + pub(super) in_flight: Semaphore, + /// Mutable state — protected by `std::sync::Mutex` because guards + /// are only held for a few statements at a time, never across an + /// `await`. `tokio::sync::Mutex` would add overhead for no benefit. + pub(super) state: Mutex, +} + +pub(super) struct JobState { + /// Set when a run starts, cleared when it ends. Used to include + /// `running_for_ms` in the `job.tick_skipped` warning. + pub current_run_start: Option, + /// Wall-clock time + outcome of the most recent completed run. + /// `None` until the first run finishes. + pub last_outcome: Option<(DateTime, JobOutcome)>, + /// Wall-clock time of the next scheduled dispatch. `None` for + /// on-demand jobs (never fires periodically); `Some(...)` for + /// scheduled jobs, advanced by one interval after every tick. + pub next_run_at: Option>, +} + +/// In-memory job registry. `Arc` lives on `AppState`; +/// native services `register()` during DI wiring. +pub struct JobRegistry { + entries: RwLock>>, +} + +impl JobRegistry { + pub fn new() -> Self { + Self { + entries: RwLock::new(HashMap::new()), + } + } + + /// Register a job. Returns an error if a job with the same name + /// is already registered — names are the primary identifier + /// everywhere (logs, admin URLs, env vars) and collisions would + /// hide bugs. + /// + /// - `interval = Some(dur)` → **scheduled**. The supervisor fires + /// the job every `dur`, starting `now + dur`. Registration does + /// NOT fire the job immediately — callers that want an at-startup + /// run should invoke the service's own initialiser once before + /// registering. + /// - `interval = None` → **on-demand only**. The supervisor never + /// fires this job. Admin endpoint (or programmatic callers) can + /// still invoke it via [`JobRegistry::trigger`] — the dispatch + /// goes through the same panic/timeout/exclusivity gates. + pub async fn register( + &self, + handler: Arc, + interval: Option, + timeout: Option, + ) -> Result<(), RegisterError> { + let name = handler.name().to_string(); + let mut guard = self.entries.write().await; + if guard.contains_key(&name) { + return Err(RegisterError::DuplicateName(name)); + } + let next_run_at = interval.map(|dur| { + Utc::now() + + chrono::Duration::from_std(dur).unwrap_or_else(|_| chrono::Duration::seconds(0)) + }); + let entry = Arc::new(JobEntry { + handler, + interval, + timeout, + in_flight: Semaphore::new(1), + state: Mutex::new(JobState { + current_run_start: None, + last_outcome: None, + next_run_at, + }), + }); + guard.insert(name, entry); + Ok(()) + } + + /// Return the name and next-due timestamp of the earliest-firing + /// **scheduled** job, or `None` if no scheduled jobs are registered. + /// On-demand jobs (registered with `interval = None`) are invisible + /// to `pick_next` — they only run when reached via + /// [`Self::trigger`]. Read-lock only — safe to call frequently from + /// the supervisor loop. + pub async fn pick_next(&self) -> Option<(String, DateTime)> { + let guard = self.entries.read().await; + let mut earliest: Option<(String, DateTime)> = None; + for (name, entry) in guard.iter() { + let Some(next_at) = entry + .state + .lock() + .expect("JobState mutex poisoned") + .next_run_at + else { + continue; // on-demand only — never picked + }; + match &earliest { + None => earliest = Some((name.clone(), next_at)), + Some((_, current)) if next_at < *current => { + earliest = Some((name.clone(), next_at)) + } + _ => {} + } + } + earliest + } + + /// Snapshot handle to a single job. Returns `Arc` so + /// callers can hold across `await` points without pinning the + /// outer read lock. + pub async fn get(&self, name: &str) -> Option> { + let guard = self.entries.read().await; + guard.get(name).cloned() + } + + /// Snapshot every registered job (used by the admin listing + /// endpoint). Returns owned `(name, Arc)` pairs to + /// avoid pinning the outer lock through the HTTP response + /// serialisation. + pub async fn snapshot_all(&self) -> Vec<(String, Arc)> { + let guard = self.entries.read().await; + guard.iter().map(|(k, v)| (k.clone(), v.clone())).collect() + } + + /// Serialisable snapshot for `GET /api/admin/jobs`. Each entry + /// captures the operator-visible state: interval (null for on- + /// demand), next scheduled dispatch (null for on-demand), when + /// the last run started, and its outcome. + pub async fn snapshot(&self) -> Vec { + let entries = self.snapshot_all().await; + entries + .into_iter() + .map(|(name, entry)| { + let state = entry.state.lock().expect("JobState mutex poisoned"); + let (last_run_at, last_outcome) = match &state.last_outcome { + Some((at, outcome)) => (Some(*at), Some(outcome.clone())), + None => (None, None), + }; + JobSummary { + name, + interval_ms: entry.interval.map(|d| d.as_millis() as u64), + next_run_at: state.next_run_at, + last_run_at, + last_outcome, + running: state.current_run_start.is_some(), + } + }) + .collect() + } + + /// Count of registered jobs — used for the startup log line. + pub async fn len(&self) -> usize { + self.entries.read().await.len() + } + + #[allow(dead_code)] + pub async fn is_empty(&self) -> bool { + self.entries.read().await.is_empty() + } + + /// Manual dispatch — the single entry point for running a + /// registered job outside the scheduler's tick loop. Called by: + /// + /// - The admin endpoint `POST /api/admin/jobs/{name}/trigger`. + /// - Any service that wants a scheduler-uniform dispatch of a + /// peer job (uniform log line, exclusivity, panic containment, + /// timeout enforcement). + /// + /// Returns `None` when the name isn't registered. Returns + /// `Some(JobOutcome)` when it is — including the case where + /// exclusivity denied the trigger (previous run still in flight), + /// which surfaces as + /// `Ok { count: 0, extra: { "skipped": "already_running" } }` per + /// the engine's dispatch protocol. + /// + /// Works for BOTH scheduled and on-demand jobs — for on-demand + /// jobs this is the only way they ever run. + /// + /// `args` is forwarded to `JobHandler::run`. Admin trigger routes + /// use `JobRunArgs { force: query.force }`; programmatic callers + /// that just want a plain run pass `JobRunArgs::default()`. + pub async fn trigger(self: &Arc, name: &str, args: &JobRunArgs) -> Option { + let entry = self.get(name).await?; + Some(super::engine::dispatch(name, entry, args).await) + } +} + +impl Default for JobRegistry { + fn default() -> Self { + Self::new() + } +} + +#[derive(Debug, thiserror::Error)] +pub enum RegisterError { + #[error("job name already registered: {0}")] + DuplicateName(String), +} + +/// Per-job row in the `GET /api/admin/jobs` response. +/// +/// - `interval_ms` — periodic cadence; `null` for on-demand jobs. +/// - `next_run_at` — next scheduled dispatch; `null` for on-demand. +/// - `last_run_at` / `last_outcome` — most recent completed run; +/// `null` until the first run finishes. +/// - `running` — true iff the in-flight permit is currently held +/// (either the supervisor tick is in progress or an admin trigger +/// raced in). +#[derive(Debug, Clone, Serialize)] +pub struct JobSummary { + pub name: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub interval_ms: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub next_run_at: Option>, + #[serde(skip_serializing_if = "Option::is_none")] + pub last_run_at: Option>, + #[serde(skip_serializing_if = "Option::is_none")] + pub last_outcome: Option, + pub running: bool, +} + +#[cfg(test)] +mod tests { + use super::*; + use async_trait::async_trait; + + struct DummyHandler { + name: String, + } + + #[async_trait] + impl JobHandler for DummyHandler { + fn name(&self) -> &str { + &self.name + } + async fn run(&self, _args: &JobRunArgs) -> JobOutcome { + JobOutcome::ok(0) + } + } + + fn handler(name: &str) -> Arc { + Arc::new(DummyHandler { + name: name.to_string(), + }) + } + + #[tokio::test] + async fn register_and_pick_next() { + let reg = JobRegistry::new(); + reg.register(handler("job_a"), Some(Duration::from_secs(60)), None) + .await + .unwrap(); + reg.register(handler("job_b"), Some(Duration::from_secs(10)), None) + .await + .unwrap(); + + let (next_name, _) = reg.pick_next().await.expect("expected a due job"); + // job_b has the shorter interval → earlier next_run_at. + assert_eq!(next_name, "job_b"); + } + + #[tokio::test] + async fn duplicate_registration_rejected() { + let reg = JobRegistry::new(); + reg.register(handler("job_x"), Some(Duration::from_secs(60)), None) + .await + .unwrap(); + let err = reg + .register(handler("job_x"), Some(Duration::from_secs(60)), None) + .await + .expect_err("duplicate name must be rejected"); + assert!(matches!(err, RegisterError::DuplicateName(_))); + } + + #[tokio::test] + async fn empty_registry_picks_nothing() { + let reg = JobRegistry::new(); + assert!(reg.pick_next().await.is_none()); + } + + #[tokio::test] + async fn snapshot_all_returns_every_entry() { + let reg = JobRegistry::new(); + reg.register(handler("a"), Some(Duration::from_secs(1)), None) + .await + .unwrap(); + reg.register(handler("b"), Some(Duration::from_secs(1)), None) + .await + .unwrap(); + let all = reg.snapshot_all().await; + assert_eq!(all.len(), 2); + } + + #[tokio::test] + async fn on_demand_job_invisible_to_pick_next() { + let reg = JobRegistry::new(); + // Scheduled job with a long interval. + reg.register(handler("scheduled"), Some(Duration::from_secs(3600)), None) + .await + .unwrap(); + // On-demand job — supervisor must never pick it. + reg.register(handler("on_demand"), None, None) + .await + .unwrap(); + + let (next_name, _) = reg.pick_next().await.expect("scheduled job due"); + assert_eq!( + next_name, "scheduled", + "pick_next must ignore on-demand jobs" + ); + } + + #[tokio::test] + async fn trigger_dispatches_on_demand_job() { + let reg = Arc::new(JobRegistry::new()); + reg.register(handler("gc"), None, None).await.unwrap(); + + let outcome = reg + .trigger("gc", &JobRunArgs::default()) + .await + .expect("job exists"); + assert!(outcome.is_ok()); + } + + #[tokio::test] + async fn trigger_returns_none_for_unknown_job() { + let reg = Arc::new(JobRegistry::new()); + assert!(reg.trigger("nope", &JobRunArgs::default()).await.is_none()); + } +} diff --git a/src/infrastructure/scheduler/types.rs b/src/infrastructure/scheduler/types.rs new file mode 100644 index 00000000..ed56dac6 --- /dev/null +++ b/src/infrastructure/scheduler/types.rs @@ -0,0 +1,150 @@ +//! Common types shared across the scheduler module. +//! +//! Nothing here talks to the DB or the async runtime — pure data +//! definitions so downstream modules (handler, registry, engine) can +//! import without dragging in transitive dependencies. See +//! `docs/plan/job-registry.md` Part 1 for the design rationale. + +use std::fmt; + +use serde::{Deserialize, Serialize}; + +/// Per-dispatch parameters passed from the caller (scheduler tick or +/// admin trigger) into [`JobHandler::run`](super::handler::JobHandler::run). +/// +/// Deliberately a struct — not a bare `bool` — so we don't churn every +/// handler signature the next time a job needs another knob. Grows by +/// addition; renaming a field is a breaking change to admin scripts +/// that pass query params, so treat like SQL columns. +/// +/// **Handlers that don't understand a given arg silently ignore it.** +/// No error path just because a caller set an unused flag — that would +/// leak per-job semantics into callers who don't need to know. +/// +/// Semantics of `force`, per job: +/// - `dedup_gc` — skip the orphan grace window (grace = 0). +/// - `grant_cleanup` — grace = 0. +/// - Others (trash_cleanup, storage_reconcile, …) — ignored. +#[derive(Debug, Clone, Default)] +pub struct JobRunArgs { + pub force: bool, +} + +/// Uniform outcome the supervisor logs and stores for every job dispatch. +/// +/// Two variants, deliberately. Distinguishing *why* a job failed +/// (handler returned Err, `tokio::time::timeout` tripped, +/// `catch_unwind` caught a panic) is a **diagnostic** concern — it +/// belongs in a `cause` tracing field the supervisor sets, not in a +/// control-flow branch every consumer of `match outcome` has to +/// think about. See `docs/plan/job-registry.md` Part 1 §JobOutcome. +/// +/// `Ok::count` is the row/record count the job reports as its primary +/// scalar (rows scanned, blobs migrated, thumbnails checked). `extra` +/// is a free-form JSON blob for job-specific fields the caller wants +/// surfaced to `oxicloud::scheduler` log lines. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(tag = "outcome", rename_all = "snake_case")] +pub enum JobOutcome { + Ok { + count: u64, + #[serde(default, skip_serializing_if = "serde_json::Value::is_null")] + extra: serde_json::Value, + }, + /// `Err` is a struct variant (not tuple-newtype) so it composes + /// with `#[serde(tag = "outcome")]`. Serde's internal tagging + /// refuses to serialise a tuple variant wrapping a bare String + /// — the tag has nowhere to live. The struct form `{ message }` + /// lets serde emit `{"outcome":"err","message":"..."}` cleanly. + Err { message: String }, +} + +impl JobOutcome { + /// Ok with no extras — the common case for jobs that only report a count. + pub fn ok(count: u64) -> Self { + JobOutcome::Ok { + count, + extra: serde_json::Value::Null, + } + } + + /// Ok with a JSON `extra` payload. Use `serde_json::json!({...})` + /// at call sites for readability. + pub fn ok_with(count: u64, extra: serde_json::Value) -> Self { + JobOutcome::Ok { count, extra } + } + + /// Convenience constructor for `Err` — call-site ergonomics + /// match the retired tuple form. + pub fn err(message: impl Into) -> Self { + JobOutcome::Err { + message: message.into(), + } + } + + /// Terse discriminant for logs / metrics: `"ok"` | `"err"`. + pub fn kind(&self) -> &'static str { + match self { + JobOutcome::Ok { .. } => "ok", + JobOutcome::Err { .. } => "err", + } + } + + pub fn is_ok(&self) -> bool { + matches!(self, JobOutcome::Ok { .. }) + } +} + +/// Diagnostic reason the supervisor attaches to the `cause` tracing +/// field when a job's outcome is [`JobOutcome::Err`]. Never persisted +/// as a first-class column — it's a log field only. +/// +/// Handlers never construct this; the supervisor derives it from +/// which failure path fired: +/// - [`ErrCause::Handler`] — the handler returned `Err(_)` itself. +/// - [`ErrCause::Timeout`] — `tokio::time::timeout` tripped on the +/// registered `ScheduledJob.timeout` wall-clock cap. +/// - [`ErrCause::Panicked`] — `JoinHandle` returned a panic error. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ErrCause { + Handler, + Timeout, + Panicked, +} + +impl ErrCause { + /// Stable label for the `cause` tracing field. Log aggregators key + /// on these — renaming here IS a breaking change to any dashboard + /// filtering on `cause = "handler"`. + pub fn as_str(self) -> &'static str { + match self { + ErrCause::Handler => "handler", + ErrCause::Timeout => "timeout", + ErrCause::Panicked => "panicked", + } + } +} + +impl fmt::Display for ErrCause { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(self.as_str()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn joboutcome_kind_label() { + assert_eq!(JobOutcome::ok(0).kind(), "ok"); + assert_eq!(JobOutcome::err("boom").kind(), "err"); + } + + #[test] + fn errcause_labels_stable() { + assert_eq!(ErrCause::Handler.as_str(), "handler"); + assert_eq!(ErrCause::Timeout.as_str(), "timeout"); + assert_eq!(ErrCause::Panicked.as_str(), "panicked"); + } +} diff --git a/src/infrastructure/services/dedup_service.rs b/src/infrastructure/services/dedup_service.rs index a4756086..77bbf417 100644 --- a/src/infrastructure/services/dedup_service.rs +++ b/src/infrastructure/services/dedup_service.rs @@ -2471,15 +2471,15 @@ impl DedupService { .await } - /// Test-only variant that bypasses the orphan grace window — used by - /// `POST /api/admin/internal/trigger-gc?force=true` so the + /// Test-only variant that bypasses the orphan grace window — used + /// by `POST /api/admin/jobs/dedup_gc/trigger?force=true` (via the + /// `JobRunArgs.force` dispatch in `JobHandler::run`) so the /// integration suite can reap just-orphaned blobs synchronously /// (waiting out the production 1 h grace inside a test run is a /// non-starter). Drops the same rows the regular sweep would, just /// without the time floor. Unsafe under concurrent uploads because /// it reopens the TOCTOU window the grace closes — only the - /// admin-internal route, itself gated by - /// `OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS`, may reach here. + /// admin-triggered `?force=true` path reaches here. pub async fn garbage_collect_force(&self) -> Result<(u64, u64), DomainError> { self.garbage_collect_with_grace(0).await } @@ -3120,6 +3120,61 @@ impl DedupPort for DedupService { } } +// ─── JobRegistry integration ──────────────────────────────────────────────── + +/// Registered name for the dedup GC job. Stable identifier used in +/// log lines, `admin.background_runs.job_name` (when Part 2 lands), +/// and admin URLs (`POST /api/admin/jobs/dedup_gc/trigger`). +pub const DEDUP_GC_JOB_NAME: &str = "dedup_gc"; + +#[async_trait::async_trait] +impl crate::infrastructure::scheduler::JobHandler for DedupService { + fn name(&self) -> &str { + DEDUP_GC_JOB_NAME + } + + /// Runs one `garbage_collect` sweep — the same reclamation that + /// `TrashCleanupService` invokes inline as its tail step, exposed + /// through the scheduler so operators can trigger it uniformly via + /// `POST /api/admin/jobs/dedup_gc/trigger`. + /// + /// Registered with `interval = None` (on-demand only): the periodic + /// tick belongs to trash cleanup, whose sweep already runs GC as + /// its final phase. Registering a redundant periodic tick here + /// would double the reclamation work with no benefit; the admin + /// trigger is what the registered entry buys us — uniform log lines, + /// panic containment, exclusivity vs. any concurrent trigger. + /// + /// `count` reports blobs reclaimed; `extra.bytes_reclaimed` reports + /// the freed disk. GC returning `(0, 0)` is normal — it means trash + /// cleanup already reaped everything. + /// + /// `args.force = true` skips the orphan grace window + /// (`garbage_collect_force` — grace_secs = 0). Same semantic as + /// `POST /api/admin/jobs/dedup_gc/trigger?force=true`. Unsafe + /// under concurrent uploads: only reachable through the admin + /// endpoint and only intentionally used by tests + operator + /// diagnostic sessions. + async fn run( + &self, + args: &crate::infrastructure::scheduler::JobRunArgs, + ) -> crate::infrastructure::scheduler::JobOutcome { + use crate::infrastructure::scheduler::JobOutcome; + let result = if args.force { + self.garbage_collect_force().await + } else { + self.garbage_collect().await + }; + match result { + Ok((items, bytes)) => JobOutcome::ok_with( + items, + serde_json::json!({ "bytes_reclaimed": bytes, "forced": args.force }), + ), + Err(e) => JobOutcome::err(format!("dedup GC failed: {e}")), + } + } +} + // ─── Tests ─────────────────────────────────────────────────────────────────── #[cfg(test)] diff --git a/src/infrastructure/services/grant_cleanup_service.rs b/src/infrastructure/services/grant_cleanup_service.rs index 54435606..7dcde2aa 100644 --- a/src/infrastructure/services/grant_cleanup_service.rs +++ b/src/infrastructure/services/grant_cleanup_service.rs @@ -1,36 +1,39 @@ -//! Background daemon that purges expired `storage.role_grants` rows. +//! Service that purges expired `storage.role_grants` rows. //! //! The AuthZ engine already filters expired grants out of every //! permission check at read time (`expires_at IS NULL OR //! expires_at > NOW()` on every `check` / `list_grants_*` path in //! `PgAclEngine`), so expired rows never leak permission. They just -//! accumulate. This daemon garbage-collects them once per -//! [`GrantCleanupService::interval_hours`], with a grace window past -//! `expires_at` that preserves the audit / support answer to "what -//! happened to my access?" for a few weeks. +//! accumulate. This service garbage-collects them, with a grace window +//! past `expires_at` that preserves the audit / support answer to +//! "what happened to my access?" for a few weeks. //! -//! Shape mirrors [`TrashCleanupService`] verbatim (fire-and-forget -//! `tokio::spawn`, `tokio::time::interval`, first-tick-immediate). The -//! authoritative pattern for background daemons in this codebase; see -//! the plan doc `docs/plan/` (deferred future work: fold all daemons -//! into a central `JobRegistry` that plugins can also register into). -//! -//! [`TrashCleanupService`]: crate::infrastructure::services::trash_cleanup_service::TrashCleanupService +//! **Scheduling.** Registered with the periodic-job scheduler +//! (`docs/plan/job-registry.md` Part 1). The retired `start_cleanup_job` +//! used to spawn its own `tokio::interval` loop; the scheduler now +//! dispatches [`GrantCleanupService::purge`] on the configured cadence +//! and handles panic containment + exclusivity + admin trigger routing. +//! Admin trigger with `?force=true` still bypasses the registered +//! job and calls `purge(Some(0))` directly so the grace override reaches +//! the underlying SQL. use std::sync::Arc; use std::time::{Duration, Instant}; -use tokio::time; use tracing::{error, info}; use crate::application::ports::authorization_ports::AuthorizationEngine; +use crate::common::errors::DomainError; +use crate::infrastructure::scheduler::{JobHandler, JobOutcome, JobRunArgs}; use crate::infrastructure::services::pg_acl_engine::PgAclEngine; +use async_trait::async_trait; -/// Daemon that periodically deletes expired grants. +pub const GRANT_CLEANUP_JOB_NAME: &str = "grant_cleanup"; + +/// Service that deletes expired grants. /// /// Owns an `Arc` (not a `dyn AuthorizationEngine`) to avoid -/// the wrapper allocation on every SQL call — the daemon is the sole -/// caller of `purge_expired_grants` outside of the admin trigger -/// endpoint, both statically dispatched. +/// the wrapper allocation on every SQL call — the caller set is small +/// (scheduler tick + admin trigger endpoint), both statically dispatched. pub struct GrantCleanupService { authz: Arc, grace_days: u32, @@ -48,50 +51,35 @@ impl GrantCleanupService { } } - /// Grace period the daemon uses on its scheduled ticks. Exposed + /// Grace period the service uses on its scheduled runs. Exposed /// for the admin trigger's default-response field. pub fn grace_days(&self) -> u32 { self.grace_days } - /// Fire-and-forget the periodic purge. Never joins; killed - /// implicitly at `tokio::runtime::shutdown`. - pub async fn start_cleanup_job(self: Arc) { - let interval_hours = self.interval_hours; - let grace_days = self.grace_days; - info!( - "Starting grant-cleanup daemon: every {}h, grace = {}d", - interval_hours, grace_days - ); - - tokio::spawn(async move { - let mut interval = time::interval(Duration::from_secs(interval_hours * 60 * 60)); - // First tick fires immediately — matches TrashCleanupService. - // Any accumulated backlog at boot gets flushed straight away. - loop { - interval.tick().await; - self.run_once().await; - } - }); + /// Cadence exposed as `Duration` so DI passes a sanitised value + /// (post-`.max(1)`) to `JobRegistry::register`. + pub fn interval(&self) -> Duration { + Duration::from_secs(self.interval_hours * 3600) } - /// One scheduled pass. Also called by the admin trigger endpoint - /// (via a shared `Arc` on `AppState`). + /// Run one purge pass. /// /// `grace_override`: /// - `None` → use the configured grace (`self.grace_days`). /// - `Some(n)` → override with `n`. The admin `?force=true` trigger /// passes `Some(0)` so Hurl regressions can hit expired grants /// without waiting the configured grace out. - pub async fn purge(&self, grace_override: Option) -> u64 { + /// + /// Returns `Ok(count)` on success, `Err(_)` on DB error. Audit-log + /// lines fire on both paths (success + failure) — bulk deletion of + /// authorization rows is security-relevant enough to log even a + /// zero-count run, and failures MUST reach the audit channel. + pub async fn purge(&self, grace_override: Option) -> Result { let grace = grace_override.unwrap_or(self.grace_days); let start = Instant::now(); match self.authz.purge_expired_grants(grace).await { Ok(count) => { - // Audit-channel logging: bulk deletion of authorization - // rows is security-relevant enough to keep it in the - // audit stream even when the count is zero (proves the - // daemon is reachable). info!( target: "audit", event = "grant_cleanup.purged", @@ -102,7 +90,7 @@ impl GrantCleanupService { count, grace, ); - count + Ok(count) } Err(e) => { error!( @@ -112,13 +100,39 @@ impl GrantCleanupService { error = %e, "Grant cleanup failed" ); - 0 + Err(e) } } } +} - /// Convenience for the scheduled loop. - async fn run_once(&self) { - let _ = self.purge(None).await; +#[async_trait] +impl JobHandler for GrantCleanupService { + fn name(&self) -> &str { + GRANT_CLEANUP_JOB_NAME + } + + /// Runs one purge. `count` on the returned `JobOutcome::Ok` is + /// the number of `role_grants` rows physically deleted; + /// `extra.grace_days` records which grace was applied so admin + /// listings can see it without a second lookup. + /// + /// `args.force = true` collapses the grace window to zero for + /// this run only — same semantic as + /// `POST /api/admin/jobs/grant_cleanup/trigger?force=true`. The + /// configured `self.grace_days` is not mutated. + async fn run(&self, args: &JobRunArgs) -> JobOutcome { + let grace_override = if args.force { Some(0) } else { None }; + let effective_grace = grace_override.unwrap_or(self.grace_days); + match self.purge(grace_override).await { + Ok(count) => JobOutcome::ok_with( + count, + serde_json::json!({ + "grace_days": effective_grace, + "forced": args.force, + }), + ), + Err(e) => JobOutcome::err(format!("grant cleanup failed: {e}")), + } } } diff --git a/src/infrastructure/services/trash_cleanup_service.rs b/src/infrastructure/services/trash_cleanup_service.rs index b4dde9cf..035ba992 100644 --- a/src/infrastructure/services/trash_cleanup_service.rs +++ b/src/infrastructure/services/trash_cleanup_service.rs @@ -6,7 +6,9 @@ use tracing::{debug, error, info, instrument}; use crate::common::errors::Result; use crate::domain::repositories::trash_repository::TrashRepository; use crate::infrastructure::repositories::pg::trash_db_repository::TrashDbRepository; +use crate::infrastructure::scheduler::{JobHandler, JobOutcome, JobRunArgs}; use crate::infrastructure::services::dedup_service::DedupService; +use async_trait::async_trait; /// Service for automatic cleanup of expired items in the trash. /// @@ -27,6 +29,8 @@ pub struct TrashCleanupService { } impl TrashCleanupService { + pub const JOB_NAME: &'static str = "trash_cleanup"; + pub fn new( trash_repository: Arc, dedup_service: Arc, @@ -39,6 +43,12 @@ impl TrashCleanupService { } } + /// Registered interval as a `Duration` — helper for DI wiring so + /// the composition root doesn't reinvent the `hours × 3600` cast. + pub fn interval(&self) -> Duration { + Duration::from_secs(self.cleanup_interval_hours * 3600) + } + /// Starts the periodic cleanup job #[instrument(skip(self))] pub async fn start_cleanup_job(&self) { @@ -107,4 +117,73 @@ impl TrashCleanupService { Ok(()) } + + /// One-shot execution used by both the legacy `start_cleanup_job` + /// timer AND the new `JobHandler::run` path. Returns the counts a + /// caller can turn into either a log line (legacy) or a `JobOutcome` + /// (scheduler). + async fn run_once(&self) -> Result { + let (files, folders) = self.trash_repository.delete_expired_bulk().await?; + // GC failure is non-fatal — the expiry itself succeeded. Report + // reclaimed bytes when possible; log + swallow otherwise. + let (gc_items, gc_bytes) = match self.dedup_service.garbage_collect().await { + Ok(pair) => pair, + Err(e) => { + error!("Trash cleanup GC failed: {:?}", e); + (0, 0) + } + }; + Ok(TrashCleanupStats { + files_purged: files, + folders_purged: folders, + gc_items, + gc_bytes, + }) + } +} + +/// Structured counters for one trash-cleanup sweep. Consumed by the +/// scheduler's `JobHandler::run` to shape `JobOutcome::Ok.extra`. +#[derive(Debug, Clone, Copy)] +struct TrashCleanupStats { + files_purged: u64, + folders_purged: u64, + gc_items: u64, + gc_bytes: u64, +} + +#[async_trait] +impl JobHandler for TrashCleanupService { + fn name(&self) -> &str { + Self::JOB_NAME + } + + /// Runs one bulk-delete-expired + GC sweep. `count` on the returned + /// `JobOutcome::Ok` is the total number of rows this tick removed + /// from the trash (files + folders); `extra` carries GC reclaim + /// counts so operators can see "how much did this actually free." + /// + /// Failure of the trash sweep itself → `Err`. GC failure alone is + /// non-fatal and stays logged only. + /// + /// `args.force` is ignored — trash cleanup has no acceleration + /// concept (retention windows are per-item metadata, not a runtime + /// knob). + async fn run(&self, _args: &JobRunArgs) -> JobOutcome { + match self.run_once().await { + Ok(stats) => { + let removed = stats.files_purged + stats.folders_purged; + JobOutcome::ok_with( + removed, + serde_json::json!({ + "files_purged": stats.files_purged, + "folders_purged": stats.folders_purged, + "gc_items": stats.gc_items, + "gc_bytes": stats.gc_bytes, + }), + ) + } + Err(e) => JobOutcome::err(format!("trash cleanup failed: {e}")), + } + } } diff --git a/src/interfaces/api/handlers/admin_handler.rs b/src/interfaces/api/handlers/admin_handler.rs index 51b3f647..475af99a 100644 --- a/src/interfaces/api/handlers/admin_handler.rs +++ b/src/interfaces/api/handlers/admin_handler.rs @@ -24,7 +24,6 @@ use crate::application::dtos::settings_dto::{ use crate::application::dtos::user_dto::{AdminUserSummaryDto, UserDto}; use crate::application::ports::authorization_ports::AuthorizationEngine; use crate::application::ports::plugin_ports::{LogQuery, PluginManagementPort, PluginMgmtError}; -use crate::application::ports::storage_ports::StorageUsagePort; use crate::common::di::AppState; use crate::domain::repositories::drive_repository::DriveRepository; use crate::domain::services::authorization::{Resource, Subject}; @@ -144,17 +143,12 @@ pub fn admin_routes() -> Router> { // when `OXICLOUD_SMTP_MOCK` is off, so production deployments // can route the path freely without leaking inboxes. .route("/smtp/test/captured", get(get_captured_email)) - // Test-only sweep triggers. Routes are always registered; the - // handlers themselves short-circuit to 404 when - // `features.enable_admin_internal_endpoints` is off — matches - // the `/smtp/test/captured` convention so production - // deployments don't need a different route table. - .route("/internal/trigger-sweep", post(internal_trigger_sweep)) - .route("/internal/trigger-gc", post(internal_trigger_gc)) - .route( - "/internal/trigger-grant-cleanup", - post(internal_trigger_grant_cleanup), - ) + // JobRegistry admin surface — production, always-on, + // audit-logged. See `docs/plan/job-registry.md` §Cross-cutting. + // Retired the `/internal/trigger-sweep|gc|grant-cleanup` shims + // that used to sit here (Stage 2 of the job-registry rollout). + .route("/jobs", get(list_jobs)) + .route("/jobs/{name}/trigger", post(trigger_job)) // Drives — admin-wide view (distinct from `/api/drives` which // is filtered to the caller's role grants). .route("/drives", get(list_all_drives)) @@ -2059,237 +2053,96 @@ pub async fn delete_drive_admin( Ok(StatusCode::NO_CONTENT) } -// ════════════════════════════════════════════════════════════════════════════ -// Test-only sweep triggers (`/api/admin/internal/*`) -// -// Wraps the periodic background jobs (storage-usage reconciliation, -// blob garbage collection) behind admin-gated synchronous endpoints -// so Hurl / integration tests can wait for them deterministically -// rather than polling the cached value. Disabled at the handler edge -// when `features.enable_admin_internal_endpoints == false` — match -// the `/smtp/test/captured` convention so production deployments -// don't need a different route table. -// ════════════════════════════════════════════════════════════════════════════ +// ───────────────────────────────────────────────────── +// JobRegistry admin surface (`/api/admin/jobs/*`) +// ───────────────────────────────────────────────────── -/// Refusal when the test-only endpoints are disabled. Returns 404 -/// rather than 403 to avoid leaking the route's existence (and the -/// corresponding config flag) to an unauthenticated probe — the -/// legitimate test runner sets the env explicitly. -fn internal_endpoints_disabled() -> axum::response::Response { - use axum::response::IntoResponse; - ( - StatusCode::NOT_FOUND, - Json(serde_json::json!({ "error": "endpoint not available" })), - ) - .into_response() -} - -/// `POST /api/admin/internal/trigger-sweep` — run the storage-usage -/// reconciliation sweep synchronously. +/// `GET /api/admin/jobs` — enumerate every registered job with its +/// interval, next-run/last-run timestamps, and last outcome. /// -/// Test-only. Recomputes `users.storage_used_bytes` and -/// `drives.used_bytes` from `SUM(size) WHERE NOT is_trashed`, in the -/// same set-based UPDATEs the periodic ticker runs. Used by Hurl -/// suites that need to assert post-delete quota convergence without -/// waiting out the sweep interval (default 600 s). +/// Production endpoint, always on. Read-only, so no audit line — +/// the standard admin-middleware auth check is enough. #[utoipa::path( - post, - path = "/api/admin/internal/trigger-sweep", + get, + path = "/api/admin/jobs", responses( - (status = 200, description = "Sweep ran"), + (status = 200, description = "Jobs listed"), (status = 401, description = "Unauthorized"), (status = 403, description = "Admin required"), - (status = 404, description = "Endpoint disabled (set OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true)"), ), security(("bearerAuth" = [])), tag = "admin" )] -pub async fn internal_trigger_sweep( - State(state): State>, -) -> axum::response::Response { - use axum::response::IntoResponse; - if !state.core.config.features.enable_admin_internal_endpoints { - return internal_endpoints_disabled(); - } - let svc = match state.storage_usage_service.as_ref() { - Some(s) => s, - None => { - return ( - StatusCode::SERVICE_UNAVAILABLE, - Json(serde_json::json!({ - "error": "storage_usage_service not available", - })), - ) - .into_response(); - } - }; - // Order matches the periodic ticker (`start_reconciliation_job`): - // drive sweep first because the user sweep reads `drives.used_bytes` - // (sum-of-personal-drives — `docs/plan/drive.md` §7). Running them - // in the other order makes the user counter freeze on the previous - // tick's drive numbers — invisible in steady state but breaks any - // Hurl that trashes + sweeps within one call. - if let Err(e) = svc.update_all_drives_storage_usage().await { - return AppError::internal_error(format!("drive sweep failed: {e}")).into_response(); - } - if let Err(e) = svc.update_all_users_storage_usage().await { - return AppError::internal_error(format!("user sweep failed: {e}")).into_response(); - } - ( - StatusCode::OK, - Json(serde_json::json!({ "ok": true, "ran": ["drives", "users"] })), - ) - .into_response() +pub async fn list_jobs(State(state): State>) -> impl IntoResponse { + let summary = state.core.job_registry.snapshot().await; + (StatusCode::OK, Json(summary)).into_response() } -/// Query parameters for `POST /api/admin/internal/trigger-gc`. +/// Query parameters for `POST /api/admin/jobs/{name}/trigger`. /// -/// `force=true` bypasses the orphan-grace window so the sweep reaps -/// just-orphaned blobs in the same call. Without this, a blob orphaned -/// less than `GC_ORPHAN_GRACE_SECS` (1 h) ago survives the sweep — the -/// grace exists so a concurrent uploader pinning a just-orphaned chunk -/// can't race the row-delete → file-unlink gap. Integration tests -/// don't have concurrent uploaders, so the test runner sets -/// `force=true` to make the sweep deterministic within a test's -/// runtime. -#[derive(Debug, serde::Deserialize, Default)] -pub struct InternalTriggerGcQuery { +/// `force=true` requests acceleration semantics from handlers that +/// support it (dedup_gc → grace = 0, grant_cleanup → grace = 0). +/// Silently ignored by handlers that don't (trash_cleanup, +/// storage_reconcile). +#[derive(serde::Deserialize)] +pub struct TriggerJobQuery { #[serde(default)] pub force: bool, } -/// `POST /api/admin/internal/trigger-gc` — run the blob garbage -/// collector synchronously. +/// `POST /api/admin/jobs/{name}/trigger` — dispatch one run off-schedule. /// -/// Test-only. Drops `file_blobs` rows with `ref_count = 0` (subject -/// to the orphan-grace window) and their on-disk content. Same call -/// as the inline post-purge GC and the periodic blob-GC sweep — just -/// exposed under an admin route so Hurl can wait for it -/// deterministically. Add `?force=true` to bypass the grace window — -/// see [`InternalTriggerGcQuery`]. +/// Returns the job's `JobOutcome` inline. Idempotent under exclusivity: +/// if the previous run is still in flight, the handler returns +/// `Ok { count: 0, extra: { "skipped": "already_running" } }` rather +/// than spawning a parallel dispatch. +/// +/// Emits an audit line before dispatch — bulk-mutation side effects on +/// operator command belong on the audit stream. #[utoipa::path( post, - path = "/api/admin/internal/trigger-gc", - params(("force" = Option, Query, description = "Bypass the orphan-grace window (test-only)")), + path = "/api/admin/jobs/{name}/trigger", + params(("name" = String, Path, description = "Registered job name")), responses( - (status = 200, description = "GC ran"), + (status = 200, description = "Dispatched; outcome inline"), (status = 401, description = "Unauthorized"), (status = 403, description = "Admin required"), - (status = 404, description = "Endpoint disabled (set OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true)"), + (status = 404, description = "Job not registered"), ), security(("bearerAuth" = [])), tag = "admin" )] -pub async fn internal_trigger_gc( +pub async fn trigger_job( State(state): State>, - Query(query): Query, -) -> axum::response::Response { - use axum::response::IntoResponse; - if !state.core.config.features.enable_admin_internal_endpoints { - return internal_endpoints_disabled(); - } - let result = if query.force { - state.core.dedup_service.garbage_collect_force().await - } else { - state.core.dedup_service.garbage_collect().await - }; - match result { - Ok((blobs_deleted, bytes_freed)) => ( + axum::extract::Path(name): axum::extract::Path, + axum::extract::Query(query): axum::extract::Query, +) -> impl IntoResponse { + use crate::infrastructure::scheduler::JobRunArgs; + // Audit line BEFORE dispatch so an operator triggering something + // that then hangs still leaves a trail. + tracing::info!( + target: "audit", + event = "job.trigger", + job = %name, + force = query.force, + "👮🏻‍♂️ Admin triggered job {} (force={})", + name, + query.force, + ); + let args = JobRunArgs { force: query.force }; + match state.core.job_registry.trigger(&name, &args).await { + Some(outcome) => ( StatusCode::OK, + Json(serde_json::json!({ "ok": true, "outcome": outcome })), + ) + .into_response(), + None => ( + StatusCode::NOT_FOUND, Json(serde_json::json!({ - "ok": true, - "blobs_deleted": blobs_deleted, - "bytes_freed": bytes_freed, - "forced": query.force, + "error": "job not registered", + "name": name, })), ) .into_response(), - Err(e) => AppError::internal_error(format!("gc failed: {e}")).into_response(), } } - -/// Query parameters for `POST /api/admin/internal/trigger-grant-cleanup`. -/// -/// `force=true` sets the grace window to `0` for this call — deletes -/// every row whose `expires_at` is in the past, right now. Enables -/// Hurl regressions to plant a past-dated grant and immediately -/// observe it purged, without waiting the configured -/// `OXICLOUD_GRANT_CLEANUP_GRACE_DAYS` out. -/// -/// Without `force`, the daemon's configured grace applies — the same -/// SQL the daily loop runs. -#[derive(Debug, serde::Deserialize, Default)] -pub struct InternalTriggerGrantCleanupQuery { - #[serde(default)] - pub force: bool, -} - -/// `POST /api/admin/internal/trigger-grant-cleanup` — run the expired- -/// grant purge synchronously. -/// -/// Test-only. Deletes rows from `storage.role_grants` whose -/// `expires_at` is more than `grace_days` in the past (or immediately, -/// with `?force=true`). Same SQL as the periodic `GrantCleanupService` -/// daemon — exposed under an admin route so Hurl can wait for it -/// deterministically. -/// -/// Response fields: -/// `grants_deleted` — count of rows removed by this invocation -/// `grace_days` — the grace window that was applied (0 when -/// `?force=true`, otherwise the config value) -/// `forced` — echoes the query param -#[utoipa::path( - post, - path = "/api/admin/internal/trigger-grant-cleanup", - params(("force" = Option, Query, description = "Force grace = 0 for this run (test-only)")), - responses( - (status = 200, description = "Purge ran"), - (status = 401, description = "Unauthorized"), - (status = 403, description = "Admin required"), - (status = 404, description = "Endpoint disabled (set OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true)"), - (status = 503, description = "Grant-cleanup daemon disabled (OXICLOUD_GRANT_CLEANUP_ENABLED=false)"), - ), - security(("bearerAuth" = [])), - tag = "admin" -)] -pub async fn internal_trigger_grant_cleanup( - State(state): State>, - Query(query): Query, -) -> axum::response::Response { - use axum::response::IntoResponse; - if !state.core.config.features.enable_admin_internal_endpoints { - return internal_endpoints_disabled(); - } - // Daemon may be disabled by config even when the internal-endpoint - // gate is on. Return 503 (rather than 404 or 500) so integration - // tests can distinguish "surface not exposed" from "surface - // exposed but backing service off". - let svc = match state.grant_cleanup_service.as_ref() { - Some(s) => s, - None => { - return ( - StatusCode::SERVICE_UNAVAILABLE, - Json(serde_json::json!({ - "error": "grant_cleanup_service not available (disabled by OXICLOUD_GRANT_CLEANUP_ENABLED=false)", - })), - ) - .into_response(); - } - }; - // `force=true` collapses the grace window to zero for this run - // only — the daemon's configured grace is untouched. Mirrors the - // `trigger-gc?force=true` shape. - let grace_override = if query.force { Some(0) } else { None }; - let grants_deleted = svc.purge(grace_override).await; - let grace_days = grace_override.unwrap_or_else(|| svc.grace_days()); - ( - StatusCode::OK, - Json(serde_json::json!({ - "ok": true, - "grants_deleted": grants_deleted, - "grace_days": grace_days, - "forced": query.force, - })), - ) - .into_response() -} diff --git a/src/interfaces/api/mod.rs b/src/interfaces/api/mod.rs index fdd5e9bc..b98e01ce 100644 --- a/src/interfaces/api/mod.rs +++ b/src/interfaces/api/mod.rs @@ -228,13 +228,12 @@ use crate::interfaces::api::handlers::file_handler::MoveFilePayload; handlers::admin_handler::complete_migration, handlers::admin_handler::verify_migration, handlers::admin_handler::generate_encryption_key, - // Admin internal-trigger handlers — gated by - // OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS (Off by default in - // prod; on for the Hurl suite). Documented in OpenAPI so - // integrators writing test harnesses can discover the surface. - handlers::admin_handler::internal_trigger_sweep, - handlers::admin_handler::internal_trigger_gc, - handlers::admin_handler::internal_trigger_grant_cleanup, + // JobRegistry admin surface — production, always-on, + // audit-logged. Retired the `/internal/trigger-*` handlers in + // favour of `/api/admin/jobs/{name}/trigger` uniform surface + // (docs/plan/job-registry.md §Cross-cutting). + handlers::admin_handler::list_jobs, + handlers::admin_handler::trigger_job, // Grant / ReBAC handlers (free functions) handlers::grant_handler::create_grant, handlers::grant_handler::revoke_grant, diff --git a/tests/api/admin_jobs.hurl b/tests/api/admin_jobs.hurl new file mode 100644 index 00000000..64f6753a --- /dev/null +++ b/tests/api/admin_jobs.hurl @@ -0,0 +1,157 @@ +# ============================================================= +# OxiCloud — JobRegistry admin surface +# ============================================================= +# Pins `GET /api/admin/jobs` and `POST /api/admin/jobs/{name}/trigger` +# — the production admin surface for the periodic-job scheduler +# (`docs/plan/job-registry.md` Part 1). +# +# Coverage: +# * Listing returns the four registered tenants +# (trash_cleanup, storage_reconcile, dedup_gc, grant_cleanup). +# * Scheduled jobs report `interval_ms`; on-demand jobs +# (`dedup_gc`) omit it via `skip_serializing_if=None`. +# * Triggering a job updates its `last_outcome` in the next list. +# * Unknown job → 404 (anti-enum on the trigger URL). +# * Non-admin caller → 403 from the admin middleware layer +# (no bespoke check in `list_jobs`/`trigger_job` itself). +# ============================================================= + + +# ───────────────────────────────────────────────────────────── +# Setup — admin login + jobs_bob (re-)provisioning +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "{{username}}", "password": "{{password}}" } + +HTTP 200 +[Captures] +admin_token: jsonpath "$.access_token" + + +# Anti-enum registration. +POST {{base_url}}/api/auth/register +Content-Type: application/json +{ + "username": "jobs_bob", + "email": "jobs_bob@example.com", + "password": "JobsBobPassword1!" +} + +HTTP 200 + + +POST {{base_url}}/api/auth/login +Content-Type: application/json +{ "username": "jobs_bob", "password": "JobsBobPassword1!" } + +HTTP 200 +[Captures] +bob_token: jsonpath "$.access_token" + + +# ───────────────────────────────────────────────────────────── +# Step 1 — Admin lists jobs. All four Part 1 tenants must appear. +# Scheduled jobs (trash_cleanup, storage_reconcile, grant_cleanup) +# report `interval_ms`; on-demand jobs (`dedup_gc`) omit it via +# serde's `skip_serializing_if = "Option::is_none"`. +# +# JSONPath idiom (per `feedback_hurl_jsonpath_filter_empty`): the +# single-match `[?(...)]` filter unwraps to a scalar so `count` +# fails; `count == 0` also fails on the zero-match case ("no +# value"). Aggregate `$..field` + `contains` is the reliable +# primitive Ed's memory endorses. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/admin/jobs +Authorization: Bearer {{admin_token}} + +HTTP 200 +[Asserts] +# Names — the four scheduler tenants. +jsonpath "$[*].name" contains "trash_cleanup" +jsonpath "$[*].name" contains "storage_reconcile" +jsonpath "$[*].name" contains "dedup_gc" +jsonpath "$[*].name" contains "grant_cleanup" + +# Scheduled jobs' interval_ms values, in whatever order: +# TrashCleanup → 24 h = 86_400_000 ms +# GrantCleanup → 24 h = 86_400_000 ms +# StorageReconcile → 600 s = 600_000 ms +# Recursive descent collects all interval_ms values across the +# array; `contains` doesn't care about order. +jsonpath "$..interval_ms" contains 86400000 +jsonpath "$..interval_ms" contains 600000 + +# On-demand job (`dedup_gc`) has no interval_ms field, so the +# total count of interval_ms values is 3, not 4. Combined with +# the four-name check above, this pins the on-demand-omission +# behaviour without hitting the single-match filter trap. +jsonpath "$..interval_ms" count == 3 + +# Every entry carries a `running` bool — same aggregate primitive. +jsonpath "$..running" count == 4 + + +# ───────────────────────────────────────────────────────────── +# Step 3 — Trigger `trash_cleanup`. Envelope shape: +# `{ ok, outcome: { outcome, count, extra } }`. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/admin/jobs/trash_cleanup/trigger +Authorization: Bearer {{admin_token}} + +HTTP 200 +[Asserts] +jsonpath "$.ok" == true +jsonpath "$.outcome.outcome" == "ok" +# Trash may or may not have expired items; count is a non-negative +# integer either way. `isNumber` on a 1-element JSONPath extract +# needs the filter idiom too — assert exists via the wrapper key. +jsonpath "$.outcome.count" exists +jsonpath "$.outcome.extra.files_purged" exists +jsonpath "$.outcome.extra.folders_purged" exists + + +# ───────────────────────────────────────────────────────────── +# Step 4 — Re-list. At least one job (`trash_cleanup`, just +# triggered above) now has a `last_outcome` populated +# with `outcome=ok`. Aggregate JSONPath — recursive +# descent collects every last_outcome.outcome value +# across the response. +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/admin/jobs +Authorization: Bearer {{admin_token}} + +HTTP 200 +[Asserts] +jsonpath "$..last_outcome.outcome" contains "ok" + + +# ───────────────────────────────────────────────────────────── +# Step 5 — Trigger a job that doesn't exist. 404 anti-enum on +# `JobRegistry::trigger` returning `None`. +# ───────────────────────────────────────────────────────────── +POST {{base_url}}/api/admin/jobs/no_such_job/trigger +Authorization: Bearer {{admin_token}} + +HTTP 404 +[Asserts] +jsonpath "$.error" == "job not registered" +jsonpath "$.name" == "no_such_job" + + +# ───────────────────────────────────────────────────────────── +# Step 6 — Non-admin caller is denied by the `/api/admin/*` +# middleware layer. The handler itself has no bespoke +# role check — reaching it at all means the caller is +# admin (same shape as `dedup_admin_gate.hurl` pins). +# ───────────────────────────────────────────────────────────── +GET {{base_url}}/api/admin/jobs +Authorization: Bearer {{bob_token}} + +HTTP 403 + + +POST {{base_url}}/api/admin/jobs/trash_cleanup/trigger +Authorization: Bearer {{bob_token}} + +HTTP 403 diff --git a/tests/api/cross_drive_copy.hurl b/tests/api/cross_drive_copy.hurl index ee262d3f..7dec25b1 100644 --- a/tests/api/cross_drive_copy.hurl +++ b/tests/api/cross_drive_copy.hurl @@ -30,7 +30,7 @@ # same `v_dest_drive_id` variable, so (a) passing implies # file rows used the same value and (b) cross-checks it. # -# Sweep convergence: `/api/admin/internal/trigger-sweep` is the +# Sweep convergence: `/api/admin/jobs/storage_reconcile/trigger` is the # deterministic synchronisation point — without it the # fire-and-forget delta hook may not yet have updated the cached # `used_bytes` when we read it. @@ -136,7 +136,7 @@ file_id: jsonpath "$.id" # numbers, the late hook adds its delta on top, and used_bytes ends # up high by exactly one file's size. Symptom: expected 32, got 64. # Real fix is await'ing the hook inline server-side. -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} [Options] delay: 200ms @@ -171,7 +171,7 @@ HTTP 200 [Captures] shared_file_id: jsonpath "$.successful[0].id" -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} [Options] delay: 200ms @@ -259,7 +259,7 @@ HTTP 201 # the file's size into the cached counter. 200 ms is well above # the tokio task latency on any reasonable box; the deterministic # fix would be intra-transaction hooks, deferred until D7. -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} [Options] delay: 200ms @@ -348,7 +348,7 @@ jsonpath "$.name" == "dc-subtree-inner" # the Step 6 file copy (32) = 96. Anything other than (96, 96) # would mean the file INSERT in copy_folder_tree used the wrong # drive_id. -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} [Options] delay: 200ms diff --git a/tests/api/cross_drive_move.hurl b/tests/api/cross_drive_move.hurl index 29520e45..f6cbff6a 100644 --- a/tests/api/cross_drive_move.hurl +++ b/tests/api/cross_drive_move.hurl @@ -17,7 +17,7 @@ # a file inside and watching the destination drive's # `used_bytes` jump by the descendant's size (not 0). # -# Sweep convergence: `/api/admin/internal/trigger-sweep` is the +# Sweep convergence: `/api/admin/jobs/storage_reconcile/trigger` is the # deterministic synchronisation point — it recomputes every # drive's cached `used_bytes` from `SUM(file.size) WHERE # drive_id = d.id`. If the file/folder move didn't update @@ -125,7 +125,7 @@ file_id: jsonpath "$.id" # Baseline used_bytes after the upload settles. Trigger-sweep is # the deterministic sync point — but only after the spawn'd hook # has had a chance to land (bug_trigger_sweep_vs_spawn_hook_race.md). -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} [Options] delay: 200ms @@ -161,7 +161,7 @@ Content-Type: application/json HTTP 200 -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} [Options] delay: 200ms @@ -245,7 +245,7 @@ nested_file_id: jsonpath "$.id" # size. Symptom: expected 64, got 96 (one extra hook landed late). # Real fix is await'ing the hook inline server-side; until then this # delay deflakes the test. -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} [Options] delay: 200ms @@ -278,7 +278,7 @@ HTTP 200 # shared: nested hello-copy.txt now charged here (32) # Anything other than (32, 32) means the descendant file's # drive_id wasn't cascaded by the trigger. -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} [Options] delay: 200ms diff --git a/tests/api/drive_quota.hurl b/tests/api/drive_quota.hurl index 38bcf842..a610ba99 100644 --- a/tests/api/drive_quota.hurl +++ b/tests/api/drive_quota.hurl @@ -120,7 +120,7 @@ small_file_id: jsonpath "$.id" # Ed's 2026-07-17 design call: the sweep is the escape hatch # for tests / operators that need immediate cache freshness; # per-write invalidation would nuke the cache on every upload. -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} [Options] delay: 200ms @@ -157,7 +157,7 @@ HTTP 201 # `used_bytes` climbs to 64 (32 + 32). Same trigger-sweep pattern # as the first assertion — the delta is fire-and-forget and the # listing cache lags until the sweep invalidates it. -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} [Options] delay: 200ms @@ -194,7 +194,7 @@ HTTP 507 # consumed by the intervening GET which re-populated the cache # with the pre-refused-write value. Sweep + re-check for # determinism. -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} HTTP 200 @@ -238,7 +238,7 @@ HTTP 201 # Unlimited drive's `used_bytes` climbs to the file's exact size # (5 MiB = 5_242_880 bytes). Trigger-sweep pattern (see above). -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} [Options] delay: 200ms @@ -266,9 +266,7 @@ jsonpath "$[?(@.id=='{{unlimited_drive_id}}')].used_bytes" == 5242880 # tight drive; the 5 MiB is in the unlimited one). # b) Permanently delete via empty-trash. # c) Trigger the reconciliation sweep on demand — -# `/api/admin/internal/trigger-sweep` is gated by -# `OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true` -# (set in `tests/common/server.env`). +# `POST /api/admin/jobs/storage_reconcile/trigger`. # d) `GET /api/drives` now shows the corrected counter. # ───────────────────────────────────────────────────────────── DELETE {{base_url}}/api/files/{{small_file_id}} @@ -286,7 +284,7 @@ HTTP 200 # Sweep is fire-and-forget on a ticker (default 600 s). Run it now # so the assertion below is deterministic instead of polling. -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} HTTP 200 @@ -312,7 +310,7 @@ jsonpath "$[?(@.id=='{{unlimited_drive_id}}')].used_bytes" == 5242880 # the route in production configs; here we just confirm a # non-admin caller is refused even when the feature is on. # ───────────────────────────────────────────────────────────── -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{owner_token}} HTTP 403 @@ -322,14 +320,14 @@ HTTP 403 # summary shape. Hard count is non-deterministic (depends on the # grace window vs the test's elapsed time), so we only check the # response shape. -POST {{base_url}}/api/admin/internal/trigger-gc +POST {{base_url}}/api/admin/jobs/dedup_gc/trigger Authorization: Bearer {{admin_token}} HTTP 200 [Asserts] jsonpath "$.ok" == true -jsonpath "$.blobs_deleted" exists -jsonpath "$.bytes_freed" exists +jsonpath "$.outcome.count" exists +jsonpath "$.outcome.extra.bytes_reclaimed" exists # ───────────────────────────────────────────────────────────── @@ -417,7 +415,7 @@ HTTP 200 # operations above never wrote anything. Trigger-sweep so the # check reads live SQL (see the class doc on the earlier # sweep + GET pair for the design rationale). -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} HTTP 200 @@ -572,7 +570,7 @@ HTTP 201 soft_shrink_file_id: jsonpath "$.id" -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} [Options] delay: 200ms @@ -656,7 +654,7 @@ Authorization: Bearer {{owner_token}} HTTP 200 -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} HTTP 200 diff --git a/tests/api/grant_cleanup.hurl b/tests/api/grant_cleanup.hurl index 2ab47ba8..01d211e2 100644 --- a/tests/api/grant_cleanup.hurl +++ b/tests/api/grant_cleanup.hurl @@ -12,12 +12,10 @@ # assertions here catch it before the daemon runs against real # data. # -# Uses the `POST /api/admin/internal/trigger-grant-cleanup` -# admin endpoint (gated by -# `OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true`, on for the -# api-test suite). `?force=true` collapses the grace window to -# zero for the call so we can plant a past-dated grant and -# immediately observe it purged, without waiting 15+ days. +# Uses the `POST /api/admin/jobs/grant_cleanup/trigger` admin +# endpoint (production surface, always on). `?force=true` collapses +# the grace window to zero for the call so we can plant a past-dated +# grant and immediately observe it purged, without waiting 15+ days. # ============================================================= @@ -150,16 +148,17 @@ permanent_grant_id: jsonpath "$.grants[0].id" # Expect `grants_deleted >= 1` (the past-dated row), # `grace_days == 0`, `forced == true`. # ───────────────────────────────────────────────────────────── -POST {{base_url}}/api/admin/internal/trigger-grant-cleanup?force=true +POST {{base_url}}/api/admin/jobs/grant_cleanup/trigger?force=true Authorization: Bearer {{alice_token}} HTTP 200 [Asserts] jsonpath "$.ok" == true -jsonpath "$.forced" == true -jsonpath "$.grace_days" == 0 +jsonpath "$.outcome.outcome" == "ok" +jsonpath "$.outcome.extra.forced" == true +jsonpath "$.outcome.extra.grace_days" == 0 # At least the expired-fixture row we just planted. -jsonpath "$.grants_deleted" >= 1 +jsonpath "$.outcome.count" >= 1 # ───────────────────────────────────────────────────────────── @@ -194,28 +193,29 @@ jsonpath "$[?(@.id=='{{permanent_grant_id}}')].role" == "viewer" # false in SQL, so it's already correct; but a # mistyped predicate could regress). # ───────────────────────────────────────────────────────────── -POST {{base_url}}/api/admin/internal/trigger-grant-cleanup?force=true +POST {{base_url}}/api/admin/jobs/grant_cleanup/trigger?force=true Authorization: Bearer {{alice_token}} HTTP 200 [Asserts] -jsonpath "$.grants_deleted" == 0 +jsonpath "$.outcome.count" == 0 # ───────────────────────────────────────────────────────────── # Step 9 — Unforced trigger. Grace = configured value (15). # No new expired grants planted, so purge is a no-op. # ───────────────────────────────────────────────────────────── -POST {{base_url}}/api/admin/internal/trigger-grant-cleanup +POST {{base_url}}/api/admin/jobs/grant_cleanup/trigger Authorization: Bearer {{alice_token}} HTTP 200 [Asserts] jsonpath "$.ok" == true -jsonpath "$.forced" == false +jsonpath "$.outcome.outcome" == "ok" +jsonpath "$.outcome.extra.forced" == false # Response echoes the configured grace (15 days by default). -jsonpath "$.grace_days" == 15 -jsonpath "$.grants_deleted" == 0 +jsonpath "$.outcome.extra.grace_days" == 15 +jsonpath "$.outcome.count" == 0 # Permanent grant still there after the unforced call. diff --git a/tests/api/run.sh b/tests/api/run.sh index 20c882ef..b17419d2 100755 --- a/tests/api/run.sh +++ b/tests/api/run.sh @@ -166,6 +166,7 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test "$API_DIR/batch_folder_copy.hurl" \ "$API_DIR/dedup_blob_cleanup.hurl" \ "$API_DIR/dedup_admin_gate.hurl" \ + "$API_DIR/admin_jobs.hurl" \ "$API_DIR/default_caldav_carddav.hurl" \ "$API_DIR/dav_error_mapping.hurl" \ "$API_DIR/carddav_vcard_properties.hurl" \ diff --git a/tests/api/storage_cleanup_check.sh b/tests/api/storage_cleanup_check.sh index 5bed925f..64d23aa3 100755 --- a/tests/api/storage_cleanup_check.sh +++ b/tests/api/storage_cleanup_check.sh @@ -265,34 +265,29 @@ log "API confirms trash is empty." # its `clear_trash_in` path, but that GC honours the 1-hour orphan-grace # window — a blob orphaned seconds ago survives the inline sweep. The # regular periodic sweep would catch it eventually, but tests need the -# disk state to be quiescent NOW. The two admin-internal triggers below -# (gated by `OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true`, set in -# tests/common/server.env) make this deterministic: +# disk state to be quiescent NOW. The two JobRegistry admin triggers +# below (production surface, always on) make this deterministic: # -# 1. trigger-sweep — reconciles users.storage_used_bytes and -# drives.used_bytes from SUM(size) — keeps the -# cached counters honest for any quota -# assertions that follow. -# 2. trigger-gc?force=true — same `garbage_collect()` as the inline -# call, but `force=true` bypasses the orphan -# grace so freshly-orphaned blobs ARE reaped. -# Safe here because the test has no concurrent -# uploaders to race the row-delete → unlink -# window the grace normally protects. -# -# Without `force=true`, the test would have to wait an hour for the -# probe blob's `orphaned_at` timestamp to age past the grace window — -# why this script was disabled until the admin-internal triggers -# landed (commit `74b33744`). +# 1. storage_reconcile — reconciles users.storage_used_bytes and +# drives.used_bytes from SUM(size) — keeps +# the cached counters honest for any quota +# assertions that follow. +# 2. dedup_gc?force=true — same `garbage_collect()` as the inline +# call, but `force=true` bypasses the +# orphan grace so freshly-orphaned blobs +# ARE reaped. Safe here because the test +# has no concurrent uploaders to race the +# row-delete → unlink window the grace +# normally protects. -curl -sf -X POST -H "$AUTH" "$base_url/api/admin/internal/trigger-sweep" >/dev/null \ - || fail "trigger-sweep failed (is OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true?)" +curl -sf -X POST -H "$AUTH" "$base_url/api/admin/jobs/storage_reconcile/trigger" >/dev/null \ + || fail "storage_reconcile trigger failed" log "Reconciliation sweep triggered." -GC_RESULT=$(curl -sf -X POST -H "$AUTH" "$base_url/api/admin/internal/trigger-gc?force=true") +GC_RESULT=$(curl -sf -X POST -H "$AUTH" "$base_url/api/admin/jobs/dedup_gc/trigger?force=true") [[ -z "$GC_RESULT" ]] && fail "trigger-gc returned an empty body" -GC_BLOBS=$(echo "$GC_RESULT" | jq -r '.blobs_deleted') -GC_BYTES=$(echo "$GC_RESULT" | jq -r '.bytes_freed') +GC_BLOBS=$(echo "$GC_RESULT" | jq -r '.outcome.count') +GC_BYTES=$(echo "$GC_RESULT" | jq -r '.outcome.extra.bytes_reclaimed') log "GC reaped $GC_BLOBS blob(s), $GC_BYTES byte(s) freed." # ── 4. Disk verification ────────────────────────────────────────────────────── diff --git a/tests/api/user_envelope_quota.hurl b/tests/api/user_envelope_quota.hurl index 7456cb58..b1971c4a 100644 --- a/tests/api/user_envelope_quota.hurl +++ b/tests/api/user_envelope_quota.hurl @@ -19,13 +19,11 @@ # 4. Sweep self-heals — after trashing the personal file and # `trigger-sweep`, `/me.storage_used_bytes` returns to 0. # -# `trigger-sweep` is the deterministic synchronisation point: -# it runs the drive-side sweep then the user-side sweep -# (`StorageUsageService::start_reconciliation_job`), so both -# cached counters are authoritative ground-truth by the time -# the assertion fires. Gated by -# `OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true` -# (set in `tests/common/server.env`). +# `POST /api/admin/jobs/storage_reconcile/trigger` is the +# deterministic synchronisation point: it runs the drive-side sweep +# then the user-side sweep (both under the periodic scheduler), so +# both cached counters are authoritative ground-truth by the time +# the assertion fires. Production surface — always on, no gate. # # Self-contained: provisions `ue_owner` so it can run alongside # the rest of the suite. @@ -141,7 +139,7 @@ HTTP 201 # acts as the synchronisation point for the user-envelope # assertion below — the sweep is the authoritative # ground-truth for both drive- and user-side counters. -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} [Options] delay: 200ms @@ -161,7 +159,7 @@ jsonpath "$[?(@.id=='{{shared_drive_id}}')].used_bytes" == 32 # If the delta path incorrectly fired the user counter, the sweep # would still correct it back to 0 (the new SQL excludes shared # drives) — this also validates the sweep formula. -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} HTTP 200 @@ -209,7 +207,7 @@ jsonpath "$.storage_used_bytes" == 32 # Confirm the sweep agrees with the delta — both code paths must # give the same number. -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} HTTP 200 @@ -239,7 +237,7 @@ Authorization: Bearer {{owner_token}} HTTP 200 -POST {{base_url}}/api/admin/internal/trigger-sweep +POST {{base_url}}/api/admin/jobs/storage_reconcile/trigger Authorization: Bearer {{admin_token}} HTTP 200 diff --git a/tests/common/server-webdav-drive-root.env b/tests/common/server-webdav-drive-root.env index 3fafb75b..9df3e1d1 100644 --- a/tests/common/server-webdav-drive-root.env +++ b/tests/common/server-webdav-drive-root.env @@ -28,12 +28,6 @@ OXICLOUD_OIDC_ENABLED=false OXICLOUD_NEXTCLOUD_ENABLED=true -# Test-only sweep triggers (`/api/admin/internal/trigger-sweep`, -# `/api/admin/internal/trigger-gc`). Off by default in production; -# the Hurl suite needs them to assert post-delete quota convergence -# without waiting out the 600 s reconciliation tick. -OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true - RUST_LOG="warn,audit=info,sqlx::migrate=info" #RUST_LOG="warn,audit=info,oxicloud::quota=debug" #RUST_LOG=debug @@ -79,7 +73,6 @@ OXICLOUD_MAGIC_LINK_SEND_PER_IP_PER_HOUR=50 # permits IP spoofing for tests OXICLOUD_TRUST_PROXY_CIDR=0.0.0.0/0 -OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true # /webdav/ will points directly to list of drives OXICLOUD_WEBDAV_DRIVE_LISTING_PREFIX="" diff --git a/tests/common/server-with-oidc-only.env b/tests/common/server-with-oidc-only.env index 47f04d5a..7a198000 100644 --- a/tests/common/server-with-oidc-only.env +++ b/tests/common/server-with-oidc-only.env @@ -31,7 +31,6 @@ OXICLOUD_ENABLE_MUSIC=true OXICLOUD_EXPOSE_SYSTEM_USERS=true OXICLOUD_WOPI_ENABLED=false OXICLOUD_NEXTCLOUD_ENABLED=true -OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true RUST_LOG="warn,audit=info,oxicloud::infrastructure::services::oidc_service=info,oxicloud::application::services::auth_application_service=info" diff --git a/tests/common/server-with-oidc.env b/tests/common/server-with-oidc.env index db424be6..a0931733 100644 --- a/tests/common/server-with-oidc.env +++ b/tests/common/server-with-oidc.env @@ -30,7 +30,6 @@ OXICLOUD_ENABLE_MUSIC=true OXICLOUD_EXPOSE_SYSTEM_USERS=true OXICLOUD_WOPI_ENABLED=false OXICLOUD_NEXTCLOUD_ENABLED=true -OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true RUST_LOG="warn,audit=info,oxicloud::infrastructure::services::oidc_service=info,oxicloud::application::services::auth_application_service=info" diff --git a/tests/common/server.env b/tests/common/server.env index 15d63285..0cb35cac 100644 --- a/tests/common/server.env +++ b/tests/common/server.env @@ -31,12 +31,6 @@ OXICLOUD_OIDC_ENABLED=false OXICLOUD_NEXTCLOUD_ENABLED=true -# Test-only sweep triggers (`/api/admin/internal/trigger-sweep`, -# `/api/admin/internal/trigger-gc`). Off by default in production; -# the Hurl suite needs them to assert post-delete quota convergence -# without waiting out the 600 s reconciliation tick. -OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true - RUST_LOG="warn,audit=info,sqlx::migrate=info" #RUST_LOG="warn,audit=info,oxicloud::quota=debug" #RUST_LOG=debug @@ -107,4 +101,3 @@ OXICLOUD_MAGIC_LINK_SEND_PER_IP_PER_HOUR=50 # permits IP spoofing for tests OXICLOUD_TRUST_PROXY_CIDR=0.0.0.0/0 -OXICLOUD_ENABLE_ADMIN_INTERNAL_ENDPOINTS=true