Repair automation defaults to destructive enforce/Full mode with no environment variable set

[ethan-tyler]

Severity: Medium · Category: reliability

Affected code

• crates/arco-compactor/src/main.rs:124-137
• crates/arco-compactor/src/main.rs:1384-1411
• crates/arco-compactor/src/main.rs:1250-1258
• crates/arco-compactor/src/main.rs:762-852
• crates/arco-catalog/src/reconciler.rs:361-437

Problem

With no env vars set at all, RepairAutomationConfig::default() is:

Self {
    mode: RepairAutomationMode::Enforce,      // not Disabled, not DryRun
    interval: Duration::from_secs(300),
    scope: RepairScope::Full,
    domains: vec![Catalog, Lineage, Search],
}

Note RepairAutomationMode's own #[derive(Default)] marks Disabled as the default (line 89-90), but RepairAutomationConfig::default() overrides it to Enforce, and from_env_reader starts from that (line 148). main unconditionally spawns run_repair_automation_loop when mode != Disabled (line 2237-2242), and a test pins this behaviour in place (test_repair_automation_config_defaults_to_enforce_full_scope, line 1669).

Every 300s this calls Reconciler::repair_with_scope(.., RepairScope::Full), which storage.delete(&issue.path) for every OrphanedSnapshot/OldSnapshotVersion issue (reconciler.rs:415) — i.e. every object under snapshots/{catalog,lineage,search}/ that is not one of the file paths listed in the currently visible manifest's SnapshotInfo. There is no age guard, no retention window and no dry-run gate; the only protections are protected_paths (the current snapshot's files) and version > visible_snapshot_version (reconciler.rs:380-414).

The mutual exclusion this relies on is also incomplete: run_repair_automation_once guards itself with state.compactor.compaction_lock.try_lock() (line 1250), but notify_handler — the production compaction trigger, and itself unauthenticated — takes only the notification_consumer mutex (line 776) and never touches compaction_lock before calling consumer.flush(), which publishes new Tier-1 snapshots. So deletion and snapshot publication can and do run concurrently.

Why it matters

A service whose stated purpose is compaction ships with unattended object deletion turned on by default across three domains, at Full scope, in a single-tenant-scoped bucket path. Every prior snapshot version's Parquet is removed within 300s of a new version being published, with zero grace period for readers that have already resolved the older pointer/manifest but not yet fetched its files. Because load_catalog_state treats a missing snapshot Parquet as an empty table rather than an error (see the tier1_state finding), any deletion that races a reader or a compaction base-state load degrades from "read fails" to "catalog silently republished empty". Destructive automation should be opt-in; here an operator must know to set ARCO_COMPACTOR_REPAIR_AUTOMATION_MODE=disabled to turn it off.

Failure scenario

Compactor starts with no ARCO_COMPACTOR_REPAIR_AUTOMATION_* env vars. arco-api resolves manifests/catalog.pointer.json -> manifest at v42 -> snapshots/catalog/v42/attempts/M42/tables.parquet and begins the GET. Meanwhile an unauthenticated /internal/notify flush publishes v43 (notify_handler never takes compaction_lock, so it is not serialized against the repair loop). The 300s repair tick fires, reloads expected paths (now v43), classifies all v42 attempt files as OldSnapshotVersion at version 42 <= 43, and deletes them. The in-flight reader's GET 404s. If the same 404 happens on the compaction base-state load instead, load_catalog_state returns empty vectors and the next publish wipes the domain.

Suggested fix

Flip the default to safe: make RepairAutomationConfig::default() return mode: RepairAutomationMode::Disabled (or at most DryRun) so enforcement requires an explicit ARCO_COMPACTOR_REPAIR_AUTOMATION_MODE=enforce, and update test_repair_automation_config_defaults_to_enforce_full_scope accordingly. Independently, add a minimum-age guard in Reconciler::repair_with_scope — skip deletion unless the object's last_modified is older than a configurable retention window (e.g. 24h) — and have notify_handler acquire state.compactor.compaction_lock (or route through run_compaction_cycle_guarded) so publication and repair are actually mutually exclusive in-process.

Verification notes

Existing mitigations found (do not fully close it): reconciler.rs:351-360 reloads protected_paths at repair time (current snapshot never deleted); reconciler.rs:392-408 skips any version newer than the visible manifest (in-flight publishes safe); notification_consumer.rs:471-505 holds a fenced per-domain DistributedLock across the entire Tier-1 read-modify-publish, so base-state corruption is unreachable; infra/terraform/main.tf:81-84 enables GCS object versioning so deletes are recoverable; reader.rs:326-400 re-reads the pointer per request with no stale-pointer cache, keeping the reader race window short; run_repair_automation_once (main.rs:1250) does try_lock the in-process compaction_lock, serializing it against the compaction loop (main.rs:1037) and the anti-entropy handler (main.rs:700).

Caveat / scope: Two things I could not establish. (1) I did not find any consumer that reads a non-current snapshot version (no time-travel/as-of path in arco-catalog), so the practical loss from immediate deletion is rollback capability plus a short reader race, not broken queries — if such a consumer exists elsewhere (arco-iceberg/arco-uc/arco-delta facades, or the workspace snapshot restore added in #322) the severity would rise. (2) The claim's framing "with no environment variable set" is not the deployed path — Cloud Run always sets the var — but since the terraform variable default is also "enforce"/"full" with no environment override, the effective behavior is identical, so this is a framing correction rather than a refutation.

Found during a staff-level audit of origin/main @ c3c0867. Mechanism confirmed by an independent adversarial verifier.

[ethan-tyler]

Deployed-posture reviewer notes, for accurate prioritization: the code facts are all confirmed — RepairAutomationConfig::default() really is mode: Enforce, interval 300s, scope: Full, domains [Catalog, Lineage, Search], main spawns the loop whenever mode != Disabled, and a test pins that default in place.

Two guards do exist in repair_with_scope (crates/arco-catalog/src/reconciler.rs:349-414) and they matter: protected_paths is re-read from the currently-visible manifest at repair time, and any snapshot version above the visible head is skipped. Together these mean the current snapshot is never deleted and an in-flight publish is safe — which is the corruption scenario from #251, and it is genuinely fixed.

What remains is the narrower defect in #343: there is no age or grace window, so the previous version is deleted within 300s of a new publish, which is what invalidates outstanding signed URLs (900s default TTL, 3600s max). Read #357 and #343 together — the fix for both is a minimum-age guard plus a safe default mode.