fix: enforce a 24 hour floor on the deletion age window

CHANGELOG.md

@@ -15,6 +15,45 @@ expose.
 
 ## [Unreleased]
 
+### Changed
+
+- **`--useage` now has a hard floor of 24 hours, and defaults to 24** instead
+  of 0. Below the floor a run is refused outright rather than warned about, for
+  `--dry-run` as well as delete.
+
+  Why a floor and not a warning: the age window is the *only* thing standing
+  between a run and live data. ClickHouse uploads a part's blobs to S3 and
+  registers them in `system.remote_data_paths` a moment later; in that window a
+  live blob is absent from the reference table and looks orphaned, and there is
+  no per-object re-check before the S3 delete. `--useage 0` silently removed
+  that protection, the default *was* 0, and `render.py` accepted it — so the
+  dangerous configuration was also the out-of-the-box one for anyone who did
+  not set it.
+
+  Why a floor and not a hardcoded constant: the parameter is only dangerous
+  downward. Upward it is the "be more careful" lever — a cluster with slow
+  merges or long-running mutations may legitimately want 72 hours or a week.
+  Removing it would forfeit that and buy nothing the floor does not already
+  give.
+
+  Refused for `--dry-run` too, deliberately: a preview computed over a wider
+  set than the delete would honour is worse than no preview, because the
+  reviewed number is the one the customer approves.
+
+  The floor is enforced in **both** `s3gc.py` and `render.py`. The renderer
+  catches it before a Job is applied; the tool catches a direct CLI run, which
+  never passes through the renderer at all.
+
+  **Development escape hatch, scoped to the one non-production phase.**
+  `PHASE=dev-automation` seeds and deletes its own fixtures within minutes, so
+  a 24 hour window would make it find nothing and "succeed" vacuously — worse
+  than failing. That phase, and only that phase, passes
+  `--dev-allow-short-useage`; the `collect`, `dry-run` and `delete` branches of
+  the entrypoint never do, and a test asserts it. A run that uses it logs a
+  warning and writes a `warning` row to the durable run log, so it can never be
+  mistaken for a normal one. The entrypoint passes the explicit
+  `--dev-allow-short-useage=true` value required by the boolean parser.
+
 ### Added
 
 - **A durable run log in ClickHouse**, `<COLLECTTABLEPREFIX><disk>_log`, written

README.md

@@ -104,7 +104,7 @@ export S3GC_CLUSTERNAME='<clickhouse-cluster>'
 export S3GC_EXPECTED_REPLICAS=2
 export S3GC_COLLECTTABLEPREFIX='s3gc_example_'
 export S3GC_AGE=24
-export S3GC_USEAGE=24
+export S3GC_USEAGE=24   # minimum 24; raising is fine, lowering is refused
 ```
 
 ### S3 authentication modes

TODO.md

@@ -13,11 +13,6 @@ forward-looking backlog.
   fixtures, and remain excluded from CI.
 - [ ] Require the `Container / test` GitHub Actions check before pull-request
   merges in the repository branch-protection settings.
-- [ ] Decide what `USEAGE_HOURS=0` should do. It disables the age window that
-  is the only guard against deleting a part between its blob upload and its
-  registration in `system.remote_data_paths`. Options: reject it in
-  `render.py`, warn loudly in `s3gc.py`, or leave it and document it. Covered
-  today only by a test that documents the hazard.
 - [ ] Quote `--useafter` as a SQL string literal (strict `xfail` in the suite).
 - [ ] Consider re-checking cluster topology per sample, not once per run, so a
   replica lost mid-run cannot widen the deletion scope.

deploy/kubernetes/README.md

@@ -100,6 +100,15 @@ Fill `s3gc.env` from `example.env`. For production, start with
 `SAMPLES=4`, `USEAGE_HOURS=24`, `ORDER_BY_OBJPATH=false`, and a 12-hour
 deadline.
 
+> **`USEAGE_HOURS` has a hard floor of 24 and the renderer enforces it.**
+> ClickHouse uploads a part's blobs to S3 and registers them in
+> `system.remote_data_paths` a moment later. In that window a live blob looks
+> orphaned, and nothing re-checks it before the delete — so this window is the
+> only thing protecting a part that is still being written. Raise it if a
+> cluster has slow merges or long mutations; you cannot lower it. The one
+> exception is `PHASE=dev-automation`, which seeds and deletes its own fixtures
+> and is already documented as non-production.
+
 Before the first *full* delete against a newly published image or a cluster you
 have not deleted from before, run one bounded delete with `USETOTAL` set to a
 few thousand. It exercises the whole path — anti-join, S3 deletion, tombstone

deploy/kubernetes/example.env

@@ -35,6 +35,10 @@ S3PROFILE=
 
 SAMPLES=4
 DELETE_BATCH_SIZE=1000
+# Minimum 24, and the renderer refuses less for every phase except
+# dev-automation. This window is the only protection against deleting a part
+# between its upload to S3 and its registration in system.remote_data_paths.
+# Raising it is fine and sometimes wise; lowering it is not.
 USEAGE_HOURS=24
 # Optional cap on how many collected objects a use phase processes. Leave empty
 # for a full run. Set it to bound the first delete against a new image or a new

deploy/kubernetes/render.py

@@ -42,6 +42,9 @@
     "VERBOSE",
 }
 DELETE_CONFIRMATION = "DELETE_ORPHANS"
+# Mirrors MINIMUM_USEAGE_HOURS in s3gc.py. Enforced here as well so a bad value
+# fails at render time rather than after a Job has been applied to a cluster.
+MINIMUM_USEAGE_HOURS = 24
 # Optional keys and their defaults. An empty value renders no environment
 # variable at all, because s3gc parses S3GC_USETOTAL as an integer and would
 # reject an empty string.
@@ -90,8 +93,19 @@ def validate(values: dict[str, str]) -> None:
     for numeric_key in ("DELETE_BATCH_SIZE", "EXPECTED_REPLICAS", "SAMPLES", "ACTIVE_DEADLINE_SECONDS", "TTL_SECONDS_AFTER_FINISHED"):
         if not values[numeric_key].isdigit() or int(values[numeric_key]) < 1:
             raise ValueError(f"{numeric_key} must be a positive integer")
-    if not values["USEAGE_HOURS"].isdigit() or int(values["USEAGE_HOURS"]) < 0:
+    if not values["USEAGE_HOURS"].isdigit():
         raise ValueError("USEAGE_HOURS must be a non-negative integer")
+    # dev-automation seeds and deletes its own fixtures within minutes, and is
+    # already documented as non-production. Every other phase gets the floor.
+    if (
+        int(values["USEAGE_HOURS"]) < MINIMUM_USEAGE_HOURS
+        and values["PHASE"] != "dev-automation"
+    ):
+        raise ValueError(
+            f"USEAGE_HOURS must be at least {MINIMUM_USEAGE_HOURS} for PHASE={values['PHASE']}: "
+            "the age window is the only protection against deleting a part between "
+            "its upload to S3 and its registration in system.remote_data_paths"
+        )
     if values["S3AUTH"] not in {"static", "aws", "iam"}:
         raise ValueError("S3AUTH must be static, aws or iam")
     if values["S3PROFILE"] and values["S3AUTH"] != "aws":

docker/kubernetes-entrypoint.sh

@@ -36,12 +36,16 @@ case "${phase}" in
 
     # A fresh collection avoids mixing prior runs and their tombstones into an
     # automated development run. `set -e` stops subsequent stages on error.
+    # Development fixtures are seeded and deleted within minutes, so this phase
+    # -- and ONLY this phase -- may run below the 24 hour age minimum that
+    # protects a part between its upload to S3 and its registration in
+    # system.remote_data_paths. The prod phases above never pass this flag.
     echo "s3gc dev automation: collect"
     python /app/s3gc.py --collectonly --keepdata --drop-collecttable
     echo "s3gc dev automation: dry-run"
-    python /app/s3gc.py --usecollected --dry-run
+    python /app/s3gc.py --usecollected --dry-run --dev-allow-short-useage=true
     echo "s3gc dev automation: delete"
-    exec python /app/s3gc.py --usecollected --keepdata --non-interactive
+    exec python /app/s3gc.py --usecollected --keepdata --non-interactive --dev-allow-short-useage=true
     ;;
   *)
     echo "Invalid S3GC_PHASE=${phase}; use collect, dry-run, delete, or dev-automation" >&2

s3gc.py

@@ -335,8 +335,12 @@ def coerce_bool(value):
     "--useage-hours",
     dest="useage",
     type=int,
-    default=0,
-    help="Process only already collected objects older than specified number of hours",
+    default=24,
+    help=(
+        "Process only already collected objects older than specified number of "
+        "hours. Minimum 24: below that a run can delete a part between its blob "
+        "upload and its registration in system.remote_data_paths"
+    ),
 )
 parser.add_argument(
     "--samples",
@@ -503,6 +507,17 @@ def coerce_bool(value):
     help="list all command line options for internal purposes",
 )
 
+parser.add_argument(
+    "--dev-allow-short-useage",
+    dest="dev_allow_short_useage",
+    type=coerce_bool,
+    default=False,
+    help=(
+        "development only: permit --useage below the 24 hour minimum. Passed only "
+        "by the dev-automation entrypoint phase, which seeds and deletes its own "
+        "fixtures within minutes. Never set this for customer or production work"
+    ),
+)
 parser.add_argument(
     "--runlog",
     "--run-log",
@@ -542,6 +557,7 @@ def coerce_bool(value):
 BOOLEAN_DESTS = (
     "s3secure_flag",
     "runlog_flag",
+    "dev_allow_short_useage",
     "use_remove_objects",
     "keepdata_flag",
     "collectonly_flag",
@@ -678,6 +694,18 @@ def graceful_exit():
 ch_writer = None
 
 
+# ClickHouse uploads a part's blobs to S3 and registers them in
+# system.remote_data_paths a moment later. In that window a live blob is absent
+# from the reference table and looks orphaned, and there is no per-object
+# re-check before the S3 delete — so the age window is the ONLY thing standing
+# between a run and live data.
+#
+# 24 hours is far longer than any part write, and short enough to stay useful.
+# It is a floor, not a default to be talked down: a run configured below it is
+# refused rather than warned about.
+MINIMUM_USEAGE_HOURS = 24
+
+
 class S3DeletionError(RuntimeError):
     """A delete failed after successful deletions were checkpointed."""
 
@@ -1186,6 +1214,33 @@ def check_samples_match_partitioning():
 
 
 def do_use():
+    if args.useage < MINIMUM_USEAGE_HOURS:
+        if not args.dev_allow_short_useage:
+            # Refused for --dry-run too, so the reviewed preview is exactly the
+            # set a delete would remove. A dry run that previews a wider set
+            # than the delete honours is worse than no preview at all.
+            raise UserVisibleError(
+                f"--useage {args.useage} is below the {MINIMUM_USEAGE_HOURS} hour minimum. "
+                "The age window is the only protection against deleting a part between "
+                "its upload to S3 and its registration in system.remote_data_paths; "
+                f"use --useage {MINIMUM_USEAGE_HOURS} or greater."
+            )
+        # Reachable only through the dev-automation phase, which seeds and
+        # deletes its own fixtures. Say so loudly and put it in the durable run
+        # log, so a run that did this can never be mistaken for a normal one.
+        logger.warning(
+            f"--useage {args.useage} is below the {MINIMUM_USEAGE_HOURS} hour minimum and is "
+            "permitted ONLY because --dev-allow-short-useage is set. This run can "
+            "delete a part that is still being written. Never use this against "
+            "customer or production data."
+        )
+        run_log(
+            "warning",
+            f"useage {args.useage} below the {MINIMUM_USEAGE_HOURS}h minimum, "
+            "permitted by --dev-allow-short-useage",
+            phase="use",
+        )
+
     if not args.dryrun_flag:
         preflight_cluster()