From 23a7963b9ec8f515527223fa74e217522737f584 Mon Sep 17 00:00:00 2001 From: "Doucette, Jarrod S" Date: Tue, 4 Aug 2026 12:17:39 -0400 Subject: [PATCH] fix(stall): bound the verify retrieve separately from the create (X1D re-ship loop) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ship_object's post-create round-trip verify retrieve shared the create-side HTAR_STALL_SECONDS (2h) silence budget. A create streams per-file progress, so silence there is a genuine hang; a retrieve is silent BY DESIGN while HPSS queues and stages the tape. On a large routed shard the shared bound was killing healthy retrieves — and because verify never passed, ship_object wrote no success record, so the wrapper re-shipped the whole object on every scheduled run. Observed (repository_X1D_3_metabolomics_rawspectra, one 6,624,961,024-byte shard): htar -cvf succeeded every run; the retrieve was killed at exactly 7200s on both attempts (~8h/job, exit 1), and its own stall log records "activity resumed after 7200.1s silence" — 0.1s past the kill line. Result: 48 identical 6.62 GB tars (~318 GB of duplicate tape) between 2026-07-17 and 2026-08-03. - new stall_defaults.json holds the default retrieve budget (21600s = 6h, ~3x the one observed gap, well inside the 48h Slurm walltime even if both retrieve attempts burn it). Defaults live in DATA, not in a function signature. - load_stall_defaults() fails loud on a missing/malformed/invalid file; load_config reads it LAZILY so a config that pins retrieve_stall_seconds needs no defaults file, and validates a per-asset override. - ship_object takes retrieve_stall_seconds and REQUIRES it (raises before any htar/hsi call) — no code-side fallback that could silently reinstate 2h. - create keeps HTAR_STALL_SECONDS, documented at both constants and in CLAUDE.md invariant #7. Tests: new tests/test_retrieve_stall_config.py (defaults-file contract incl. the default must exceed the create bound, load_config default/override/validation, the retrieve calls carry the configured bound while the create does not, and the absent/invalid-value raises land before any tape work). Existing ship_object callers in three test files updated. Full suite: 423 passed. Note: this does NOT by itself stop a re-ship loop — a repeatedly-failing verify still re-ships. The per-target circuit breaker for that lives in florasense-tools (fortress/repo_backup.py), shipped alongside this. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 18 +- archive.py | 115 ++++++++++- config.example.json | 3 + stall_defaults.json | 6 + tests/test_retrieve_stall_config.py | 233 +++++++++++++++++++++++ tests/test_ship_object_htar_rc.py | 9 +- tests/test_ship_object_retrieve_retry.py | 9 +- tests/test_stall_detection.py | 3 +- 8 files changed, 381 insertions(+), 15 deletions(-) create mode 100644 stall_defaults.json create mode 100644 tests/test_retrieve_stall_config.py diff --git a/CLAUDE.md b/CLAUDE.md index 77dcdc8..3552994 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -230,9 +230,23 @@ Slurm's own wall-clock (up to 48h) would ever stop it. Long, output-streaming ca (create, round-trip retrieve) go through `_run_with_stall_watch()` (`ship_object`) or its self-contained inline twin `run_watched()` (`send_to_fortress`, invariant #1 — imports inside, no reference to the module-level copy), which kill the process and -raise if `HTAR_STALL_SECONDS` (2h) passes with no new output — silence, not elapsed +raise if the call's silence budget passes with no new output — silence, not elapsed time, is the signal, since legitimate transfers range from minutes to 22+ hours (see -Known issues below). Short, no-progress calls (`hsi mkdir`/`hsi ls`) just need +Known issues below). + +The budget is **not one number**: a *create* streams per-file progress, so silence there +really is a hang and it keeps `HTAR_STALL_SECONDS` (2h); the post-create *round-trip +verify retrieve* is silent **by design** while HPSS queues and stages the tape, so +`ship_object` bounds it with the config's **`retrieve_stall_seconds`** instead (default +in `stall_defaults.json` — data, not code; a per-asset config may override it; there is +deliberately no code-side fallback, so a caller that forgets to thread it down raises). +Conflating the two is a real, expensive bug, not a theoretical one: on 2026-08-03 a +single 6.62 GB `X1D_3_metabolomics_rawspectra` shard had its healthy retrieve killed at +exactly 2h twice per job (its stall log reads `activity resumed after 7200.1s silence`), +so verify never passed, no success record was written, and every scheduled run re-shipped +the whole shard — 48 identical tars, ~318 GB of duplicate tape, over 2.5 weeks. + +Short, no-progress calls (`hsi mkdir`/`hsi ls`) just need `timeout=HSI_SHORT_CALL_TIMEOUT_SECONDS` (5 min) on a plain `subprocess.run()`. A new call site that skips this reintroduces the exact hang this fixes. diff --git a/archive.py b/archive.py index 2a56a60..829786c 100644 --- a/archive.py +++ b/archive.py @@ -49,6 +49,51 @@ # that --local can run in a plain stdlib Python with no Globus install at all. +# Engine-wide stall-watch defaults live in DATA, not code: stall_defaults.json beside +# this module (override the path with FORTRESS_STALL_DEFAULTS). The file is REQUIRED — +# load_stall_defaults raises on a missing/malformed/invalid file rather than +# substituting a value buried in a function signature, so the operative timeout for +# every run is always readable in one inspectable place. A per-asset config may still +# override any key by name (load_config's setdefault below). +STALL_DEFAULTS_PATH = os.environ.get( + "FORTRESS_STALL_DEFAULTS", + os.path.join(os.path.dirname(os.path.realpath(__file__)), "stall_defaults.json")) + +# Keys load_stall_defaults requires, each a positive integer number of seconds. +STALL_DEFAULT_KEYS = ("retrieve_stall_seconds",) + + +def load_stall_defaults(path=None): + """Load stall_defaults.json (see STALL_DEFAULTS_PATH). Returns a dict of + {key: positive int seconds} for every key in STALL_DEFAULT_KEYS. Raises + ValueError if the file is missing, unparseable, or any required key is absent + or not a positive integer — fail loud, never a silent code-side fallback.""" + path = path or STALL_DEFAULTS_PATH + try: + with open(path) as fh: + data = json.load(fh) + except OSError as exc: + raise ValueError( + f"stall defaults file is required but could not be read: {path} ({exc}). " + f"Restore it from the fortress-archive checkout, or point " + f"FORTRESS_STALL_DEFAULTS at a valid one.") + except json.JSONDecodeError as exc: + raise ValueError(f"stall defaults file {path} is not valid JSON: {exc}") + if not isinstance(data, dict): + raise ValueError(f"stall defaults file {path} must contain a JSON object") + out = {} + for key in STALL_DEFAULT_KEYS: + if key not in data: + raise ValueError(f"stall defaults file {path} is missing required key {key!r}") + val = data[key] + if not isinstance(val, int) or isinstance(val, bool) or val <= 0: + raise ValueError( + f"stall defaults file {path}: {key!r} must be a positive integer " + f"(seconds), got: {val!r}") + out[key] = val + return out + + def load_config(config_path): """Load and validate the JSON config file.""" fh = open(config_path, 'r') @@ -182,6 +227,24 @@ def load_config(config_path): "(size-routing) and Phase 3 (leveled incremental) are mutually exclusive " "schemes for a target (docs/RFC_incremental_v2.md §3). Pick one.") + # retrieve_stall_seconds: how long ship_object's round-trip verify retrieve may go + # SILENT before it is killed as a hung HPSS connection. Default comes from + # stall_defaults.json (fail-loud if that file is gone); a per-asset config may + # override it for a target whose tape-stage latency is legitimately longer. Kept + # separate from the create-side HTAR_STALL_SECONDS on purpose — see that constant + # and stall_defaults.json for the X1D shard incident that split the two. + # Read LAZILY (not via setdefault's eagerly-evaluated argument): a config that + # pins the value itself must not need the defaults file at all. + if "retrieve_stall_seconds" not in config: + config["retrieve_stall_seconds"] = \ + load_stall_defaults()["retrieve_stall_seconds"] + if not isinstance(config["retrieve_stall_seconds"], int) \ + or isinstance(config["retrieve_stall_seconds"], bool) \ + or config["retrieve_stall_seconds"] <= 0: + raise ValueError( + f"config 'retrieve_stall_seconds' must be a positive integer (seconds), " + f"got: {config['retrieve_stall_seconds']!r}") + # Durability guard (RFC §2.4 / §3.2): EITHER routing scheme keeps a per-target # manifest at {log_dir}/_vault/ that MUST outlive runs and be shared, so reclaim # can find it. Refuse a log_dir under $HOME / scratch / the home default — home @@ -888,9 +951,21 @@ def decode_blonde(blonde): # it's silence: a healthy htar/htar_large/hsi call streams per-file progress # continuously, a stalled one stops producing output entirely. # -# HTAR_STALL_SECONDS bounds how long a create/retrieve call may go without any new -# stdout/stderr before it's killed as a hung HPSS connection (see -# _run_with_stall_watch). Generous relative to any per-file gap observed in practice. +# HTAR_STALL_SECONDS bounds how long a CREATE call (and any other stall-watched call +# that has no bound of its own) may go without any new stdout/stderr before it's killed +# as a hung HPSS connection (see _run_with_stall_watch). Generous relative to any +# per-file gap observed in practice for a create, which streams per-file progress +# continuously. +# +# The post-create round-trip VERIFY RETRIEVE is bounded separately, by the config's +# `retrieve_stall_seconds` (default in stall_defaults.json, threaded into ship_object) +# — 2 hours is wrong there. A retrieve is silent BY DESIGN while HPSS queues and +# stages the tape, so on a large routed shard this bound was killing a healthy +# retrieve: 2026-08-03, repository_X1D_3_metabolomics_rawspectra (one 6,624,961,024-byte +# shard) was killed at exactly the 2h bound on both attempts, and its own stall log +# shows "activity resumed after 7200.1s silence" — 0.1s past the kill line. Verify +# never passed, so no success record was written and every scheduled run re-shipped +# the whole shard: 48 identical 6.62 GB tars (~318 GB of tape) over 2026-07-17..08-03. HTAR_STALL_SECONDS = 2 * 60 * 60 # 2 hours # mkdir / ls existence checks return in seconds to low minutes when healthy and carry @@ -3760,7 +3835,8 @@ def _verify_members(heartbeat): def ship_object(source_folder, arcnames, object_stem, catalog, fortress_base_dir, - tmp_dir, log_dir, emails, cleanup_on_success=False): + tmp_dir, log_dir, emails, cleanup_on_success=False, + retrieve_stall_seconds=None): """ Ship ONE routed object (Phase 2, RFC §2.5) — LOCAL only. Tars the object's members DIRECTLY from source (no zip), choosing the transport via choose_transport: real @@ -3802,6 +3878,15 @@ def ship_object(source_folder, arcnames, object_stem, catalog, fortress_base_dir htar_large: cd ; htar_large -cvf /_.tar verify htar: cd ; htar -xvf ; md5 each member vs source verify _lg: hsi get -> ; tar xf ; md5 + + retrieve_stall_seconds is REQUIRED (no code-side default): the silence budget for + the round-trip verify retrieve only, threaded down from the config + (`retrieve_stall_seconds`, whose own default lives in stall_defaults.json). Passing + None raises rather than quietly reinstating the create-side HTAR_STALL_SECONDS, + which is far too tight for a large shard's tape stage — the exact regression that + burned ~318 GB of duplicate tape on repository_X1D_3_metabolomics_rawspectra (see + HTAR_STALL_SECONDS). The create call keeps HTAR_STALL_SECONDS: a create streams + per-file progress, so silence there really is a hang. """ import os import re @@ -3821,6 +3906,13 @@ def ship_object(source_folder, arcnames, object_stem, catalog, fortress_base_dir arcnames = list(arcnames) if not arcnames: raise RuntimeError(f"ship_object: empty member set for {object_stem}") + if not isinstance(retrieve_stall_seconds, int) \ + or isinstance(retrieve_stall_seconds, bool) or retrieve_stall_seconds <= 0: + raise RuntimeError( + f"ship_object: retrieve_stall_seconds must be a positive integer " + f"(seconds), got {retrieve_stall_seconds!r} — thread it down from the " + f"config (default in stall_defaults.json); there is deliberately no " + f"code-side fallback.") transport = choose_transport(arcnames, catalog) timestamp = datetime.datetime.now().strftime("%Y%m%d_%H%M%S") @@ -3957,7 +4049,9 @@ def _hsi_ls_ok(path): print(f" WARNING: {create_warning}") # Round-trip verify: pull the members back and re-check per-file MD5. - # See RETRIEVE_STALL_MAX_ATTEMPTS above for why this step gets one retry. + # See RETRIEVE_STALL_MAX_ATTEMPTS above for why this step gets one retry, and + # HTAR_STALL_SECONDS for why the silence budget here is the config's + # retrieve_stall_seconds rather than the (create-sized) module default. retrieve_dir = os.path.join(tmp_dir, f"{run_timestamp}.verify") retrieve_stall_log = os.path.join(log_dir, f"{run_timestamp}.retrieve.stall.log") @@ -3967,16 +4061,19 @@ def _attempt_retrieve(): if transport == "htar": vr = _run_with_stall_watch(["htar", "-xvf", fortress_tar], cwd=retrieve_dir, + stall_seconds=retrieve_stall_seconds, activity_log=retrieve_stall_log) else: local_tar = os.path.join(retrieve_dir, "obj.tar") g = _run_with_stall_watch( f'hsi -q "get {local_tar} : {fortress_tar}"', + stall_seconds=retrieve_stall_seconds, activity_log=retrieve_stall_log) if g.returncode != 0: raise RuntimeError("hsi get failed:\n" + g.stdout.decode("utf-8", "replace")[-4000:]) vr = _run_with_stall_watch(["tar", "xf", local_tar], cwd=retrieve_dir, + stall_seconds=retrieve_stall_seconds, activity_log=retrieve_stall_log) if vr.returncode != 0: raise RuntimeError( @@ -4123,6 +4220,9 @@ def _verify_md5s(heartbeat): shard_count_cfg = config["shard_count"] shard_target = config["shard_target"] incremental = config["incremental"] + # Silence budget for ship_object's round-trip verify retrieve (size-routing path). + # load_config guarantees this key: its default lives in stall_defaults.json. + retrieve_stall_seconds = config["retrieve_stall_seconds"] # Size-routing runs LOCAL only (RFC §2.8): the per-target manifest lives on a # shared durable log_dir that __main__ reads/writes, and reclaim later reads. Refuse @@ -4296,8 +4396,11 @@ def run(fn, *fn_args): def ship_one(obj, stem): # Tar the object's members directly from source (no zip); ship_object picks # htar (indexed) vs htar_large by max member size, verifies, and logs. + # retrieve_stall_seconds is passed POSITIONALLY: `run` is the local/globus + # dispatcher (def run(fn, *fn_args)) and forwards positional args only. return run(ship_object, source_folder, obj["arcnames"], stem, survivors, - fortress_base, tmp_dir, log_dir, emails, cleanup_zip_on_success) + fortress_base, tmp_dir, log_dir, emails, cleanup_zip_on_success, + retrieve_stall_seconds) try: summary = route_and_ship( diff --git a/config.example.json b/config.example.json index 60a66cf..4a4ea1b 100644 --- a/config.example.json +++ b/config.example.json @@ -35,6 +35,9 @@ "comment_exclude_optional": "Optional (default []). Per-asset opt-out list of Tier-2 (OPTIONAL) rule ids from the exclude_spec (e.g. omics provenance/QC: aux_info, fastp, checksums). Honored only if this asset's file_pattern also ships the raw inputs those artifacts are regenerable from. Omit to back up all OPTIONAL artifacts (safe default).", "exclude_optional": [], + "comment_retrieve_stall_seconds": "Optional override (default: stall_defaults.json's retrieve_stall_seconds, currently 21600 = 6h). How long the post-create round-trip VERIFY RETRIEVE may go without producing any output before it is killed as a hung HPSS connection. Much larger than the create-side bound on purpose: a retrieve is silent while HPSS queues and stages the tape, and a too-tight bound kills a healthy retrieve — which, because verify never passes, makes the next run re-ship the whole object (the 2026-08 X1D shard loop; see stall_defaults.json). Raise it for a target whose tape-stage latency is legitimately longer.", + "retrieve_stall_seconds": 21600, + "comment_size_routing": "Optional (default false). Phase-2 size-routing (docs/RFC_incremental_v2.md §2): instead of one whole-target tar, ship each file >= t_small as its own 'solo' object and bundle smaller files into content-addressed 'shards' (shard = hash(relpath) mod K). Unchanged objects skip, so a GROWING target ships only its new files (the append win). LOCAL mode only — refused under --globus. When true, log_dir MUST be shared+durable (a Depot logs dir), NOT home or scratch: the per-target manifest lives at {log_dir}/_vault/ and reclaim must find it. Omit for the default whole-target behavior.", "size_routing": false, diff --git a/stall_defaults.json b/stall_defaults.json new file mode 100644 index 0000000..a22853e --- /dev/null +++ b/stall_defaults.json @@ -0,0 +1,6 @@ +{ + "comment": "Engine-wide stall-watch defaults. Read by archive.load_config() for EVERY config (per-asset configs may override a key by name). This file is REQUIRED: a missing/malformed/invalid file raises rather than falling back to a value hidden in code, so the operative timeout is always inspectable here.", + + "comment_retrieve_stall_seconds": "How long ship_object's post-create round-trip verify retrieve (htar -xvf, or hsi get + tar xf) may go without producing ANY output before it is killed as a hung HPSS connection. Deliberately much larger than the create-side HTAR_STALL_SECONDS (2h): a create streams per-file progress continuously, so silence there really is a hang, whereas a retrieve of a large routed shard is silent BY DESIGN while HPSS queues and stages the tape. 2026-08-03 (repository_X1D_3_metabolomics_rawspectra, a single 6,624,961,024-byte shard): the retrieve went quiet, was killed at exactly the 2h bound on both attempts, and its own stall log shows activity resuming 0.1s past the kill line ('activity resumed after 7200.1s silence') — legitimate tape-stage latency, misread as a hang. Verify never passed, so no success record was written and the next scheduled run re-shipped the whole shard from scratch: 48 identical 6.62 GB tars (~318 GB of duplicate tape) over 2026-07-17..08-03. 6h gives ~3x margin over the one observed gap while staying well inside the 48h Slurm walltime even if both retrieve attempts burn their full budget.", + "retrieve_stall_seconds": 21600 +} diff --git a/tests/test_retrieve_stall_config.py b/tests/test_retrieve_stall_config.py new file mode 100644 index 0000000..4df945c --- /dev/null +++ b/tests/test_retrieve_stall_config.py @@ -0,0 +1,233 @@ +""" +Unit tests for the CONFIGURABLE round-trip-verify retrieve stall bound. + +2026-08-03/04 incident (repository_X1D_3_metabolomics_rawspectra, a single +6,624,961,024-byte routed shard): `htar -cvf` succeeded every run, then the post-create +verify retrieve went silent while HPSS staged the tape and the stall watch killed it at +exactly the shared 2h HTAR_STALL_SECONDS bound — on BOTH attempts, ~8h per job. Its own +stall log recorded "activity resumed after 7200.1s silence", i.e. the retrieve was +healthy and 0.1s past the kill line. Because verify never passed, ship_object wrote no +success record, so every scheduled run re-shipped the whole shard from scratch: 48 +identical 6.62 GB tars (~318 GB of duplicate tape) between 2026-07-17 and 2026-08-03. + +The fix separates the two silence budgets. A CREATE streams per-file progress, so +silence there really is a hang and it keeps HTAR_STALL_SECONDS. A RETRIEVE is silent by +design during a tape stage, so it gets its own, larger `retrieve_stall_seconds`, whose +default lives in DATA (stall_defaults.json) rather than in a function signature, and +which a per-asset config may override. There is deliberately NO code-side fallback: a +missing defaults file, or a caller that forgets to thread the value down, must fail +loud rather than silently reinstate the 2h bound that caused the loop. +""" +import json +import os +import shutil +import sys +import tempfile +import unittest +from unittest import mock + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +import archive # noqa: E402 +from test_ship_object_htar_rc import FakeHsiHtarEnv # noqa: E402 + +REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + + +class TestStallDefaultsFile(unittest.TestCase): + """The shipped defaults file is the single inspectable home of the default.""" + + def test_repo_ships_a_valid_defaults_file(self): + defaults = archive.load_stall_defaults( + os.path.join(REPO_ROOT, "stall_defaults.json")) + self.assertIn("retrieve_stall_seconds", defaults) + # Must be strictly greater than the create-side bound — the whole point of + # splitting them. A value <= HTAR_STALL_SECONDS would silently re-open the + # X1D loop while looking configured. + self.assertGreater(defaults["retrieve_stall_seconds"], + archive.HTAR_STALL_SECONDS) + + def test_module_default_path_resolves_to_the_shipped_file(self): + self.assertEqual(os.path.realpath(archive.STALL_DEFAULTS_PATH), + os.path.realpath(os.path.join(REPO_ROOT, + "stall_defaults.json"))) + + def test_missing_file_raises_instead_of_falling_back(self): + with tempfile.TemporaryDirectory() as d: + with self.assertRaises(ValueError) as ctx: + archive.load_stall_defaults(os.path.join(d, "nope.json")) + self.assertIn("required", str(ctx.exception)) + + def test_malformed_json_raises(self): + with tempfile.TemporaryDirectory() as d: + p = os.path.join(d, "stall_defaults.json") + with open(p, "w") as fh: + fh.write("{not json") + with self.assertRaises(ValueError) as ctx: + archive.load_stall_defaults(p) + self.assertIn("not valid JSON", str(ctx.exception)) + + def test_missing_key_raises(self): + with tempfile.TemporaryDirectory() as d: + p = os.path.join(d, "stall_defaults.json") + with open(p, "w") as fh: + json.dump({"something_else": 5}, fh) + with self.assertRaises(ValueError) as ctx: + archive.load_stall_defaults(p) + self.assertIn("retrieve_stall_seconds", str(ctx.exception)) + + def test_nonpositive_or_wrongly_typed_value_raises(self): + for bad in (0, -1, 1.5, "7200", True, None): + with tempfile.TemporaryDirectory() as d: + p = os.path.join(d, "stall_defaults.json") + with open(p, "w") as fh: + json.dump({"retrieve_stall_seconds": bad}, fh) + with self.assertRaises(ValueError, msg=f"accepted {bad!r}"): + archive.load_stall_defaults(p) + + +def _min_config(**extra): + cfg = { + "project_name": "testproj", + "source_folder": "/depot/x", + "file_pattern": "2026", + "fortress_base_dir": "/group/x", + "emails": ["ops@example.com"], + } + cfg.update(extra) + return cfg + + +class TestLoadConfigRetrieveStall(unittest.TestCase): + def setUp(self): + self.d = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.d, ignore_errors=True) + + def _write(self, cfg): + p = os.path.join(self.d, "config.json") + with open(p, "w") as fh: + json.dump(cfg, fh) + return p + + def test_default_comes_from_the_defaults_file(self): + cfg = archive.load_config(self._write(_min_config())) + self.assertEqual( + cfg["retrieve_stall_seconds"], + archive.load_stall_defaults()["retrieve_stall_seconds"]) + + def test_per_asset_override_wins(self): + cfg = archive.load_config( + self._write(_min_config(retrieve_stall_seconds=43200))) + self.assertEqual(cfg["retrieve_stall_seconds"], 43200) + + def test_invalid_override_raises(self): + for bad in (0, -5, "6h", 1.5, True, None): + with self.assertRaises(ValueError, msg=f"accepted {bad!r}"): + archive.load_config( + self._write(_min_config(retrieve_stall_seconds=bad))) + + def test_missing_defaults_file_fails_the_whole_load(self): + """No defaults file + no per-asset key = loud failure, never a 2h fallback.""" + with mock.patch.object(archive, "STALL_DEFAULTS_PATH", + os.path.join(self.d, "gone.json")): + with self.assertRaises(ValueError): + archive.load_config(self._write(_min_config())) + + def test_missing_defaults_file_is_survivable_via_explicit_config(self): + """An explicit per-asset value needs no defaults file (setdefault short- + circuits) — an operator can always pin the value in the config itself.""" + with mock.patch.object(archive, "STALL_DEFAULTS_PATH", + os.path.join(self.d, "gone.json")): + cfg = archive.load_config( + self._write(_min_config(retrieve_stall_seconds=21600))) + self.assertEqual(cfg["retrieve_stall_seconds"], 21600) + + +class _CapturingStallWatch: + """Wraps the real archive._run_with_stall_watch, recording the stall_seconds each + call was given (and forcing a small effective bound so tests stay fast). Drives + the REAL function — real Popen/select plumbing against the fake hsi/htar bins — + matching this suite's no-subprocess-mocking philosophy.""" + + def __init__(self, fast_seconds=0.5, poll_interval=0.05): + self.real = archive._run_with_stall_watch + self.fast_seconds = fast_seconds + self.poll_interval = poll_interval + self.seen = [] + + def __call__(self, cmd, *, cwd=None, watch_paths=None, stall_seconds=None, + poll_interval=None, activity_log=None): + self.seen.append((cmd, stall_seconds)) + return self.real(cmd, cwd=cwd, watch_paths=watch_paths, + stall_seconds=self.fast_seconds, + poll_interval=self.poll_interval, + activity_log=activity_log) + + +class TestShipObjectUsesConfiguredRetrieveBound(unittest.TestCase): + def setUp(self): + self.d = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.d, ignore_errors=True) + self.source_folder = os.path.join(self.d, "src") + os.makedirs(os.path.join(self.source_folder, "sub")) + with open(os.path.join(self.source_folder, "a.txt"), "wb") as fh: + fh.write(b"hello world") + with open(os.path.join(self.source_folder, "sub", "b.txt"), "wb") as fh: + fh.write(b"second member") + self.arcnames = ["a.txt", "sub/b.txt"] + self.catalog = {"a.txt": (0, 11), "sub/b.txt": (0, 13)} + self.tmp_dir = os.path.join(self.d, "tmp") + self.log_dir = os.path.join(self.d, "logs") + self.fortress_base_dir = "/tape/base" + + def test_retrieve_calls_get_the_configured_bound_create_keeps_the_default(self): + watch = _CapturingStallWatch() + env = FakeHsiHtarEnv(create_rc=0, create_stdout=b"HTAR: HTAR SUCCESSFUL\n", + ls_rc=0, source_folder=self.source_folder, + arcnames=self.arcnames) + with env: + with mock.patch.object(archive, "_run_with_stall_watch", watch): + archive.ship_object( + self.source_folder, self.arcnames, "testproj__shard-0of1", + self.catalog, self.fortress_base_dir, self.tmp_dir, + self.log_dir, ["ops@example.com"], + retrieve_stall_seconds=21600) + + def _cmd_str(c): + return " ".join(c) if isinstance(c, (list, tuple)) else str(c) + + create = [s for c, s in watch.seen if _cmd_str(c).startswith("htar -P -cvf")] + retrieve = [s for c, s in watch.seen if _cmd_str(c).startswith("htar -xvf")] + self.assertTrue(create, "no create call observed") + self.assertTrue(retrieve, "no retrieve call observed") + # The create keeps the module default (passed as None -> signature default). + self.assertTrue(all(s is None for s in create), + f"create must not be re-bounded: {create}") + # Every retrieve call carries the configured bound, NOT the 2h create bound. + self.assertTrue(all(s == 21600 for s in retrieve), + f"retrieve bound not threaded through: {retrieve}") + + def test_absent_bound_raises_before_any_tape_work(self): + """A caller that forgets the value must fail loud, and BEFORE creating a tar + — reinstating the 2h bound here is exactly what burned 318 GB of tape.""" + watch = _CapturingStallWatch() + with mock.patch.object(archive, "_run_with_stall_watch", watch): + with self.assertRaises(RuntimeError) as ctx: + archive.ship_object( + self.source_folder, self.arcnames, "testproj__shard-0of1", + self.catalog, self.fortress_base_dir, self.tmp_dir, + self.log_dir, ["ops@example.com"]) + self.assertIn("retrieve_stall_seconds", str(ctx.exception)) + self.assertEqual(watch.seen, [], "must raise before any htar/hsi call") + + def test_nonpositive_bound_raises(self): + for bad in (0, -1, "21600", 1.5, True, None): + with self.assertRaises(RuntimeError, msg=f"accepted {bad!r}"): + archive.ship_object( + self.source_folder, self.arcnames, "testproj__shard-0of1", + self.catalog, self.fortress_base_dir, self.tmp_dir, + self.log_dir, ["ops@example.com"], + retrieve_stall_seconds=bad) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_ship_object_htar_rc.py b/tests/test_ship_object_htar_rc.py index 104e1cf..edbac3e 100644 --- a/tests/test_ship_object_htar_rc.py +++ b/tests/test_ship_object_htar_rc.py @@ -153,7 +153,8 @@ def _ship(self, env): result = archive.ship_object( self.source_folder, self.arcnames, "testproj__shard-0of1", self.catalog, self.fortress_base_dir, self.tmp_dir, self.log_dir, - ["ops@example.com"]) + ["ops@example.com"], + retrieve_stall_seconds=7200) calls = env.calls() return result, calls @@ -286,7 +287,8 @@ def _ship(self, env): result = archive.ship_object( self.source_folder, self.arcnames, "testproj__shard-0of1", self.catalog, self.fortress_base_dir, self.tmp_dir, self.log_dir, - ["ops@example.com"]) + ["ops@example.com"], + retrieve_stall_seconds=7200) calls = env.calls() return result, calls @@ -298,7 +300,8 @@ def _ship_raises(self, env): archive.ship_object( self.source_folder, self.arcnames, "testproj__shard-0of1", self.catalog, self.fortress_base_dir, self.tmp_dir, self.log_dir, - ["ops@example.com"]) + ["ops@example.com"], + retrieve_stall_seconds=7200) calls = env.calls() return ctx.exception, calls diff --git a/tests/test_ship_object_retrieve_retry.py b/tests/test_ship_object_retrieve_retry.py index daca43a..a051029 100644 --- a/tests/test_ship_object_retrieve_retry.py +++ b/tests/test_ship_object_retrieve_retry.py @@ -120,7 +120,8 @@ def test_retrieve_stall_then_retry_succeeds(self): result = archive.ship_object( self.source_folder, self.arcnames, "testproj__shard-0of1", self.catalog, self.fortress_base_dir, self.tmp_dir, - self.log_dir, ["ops@example.com"]) + self.log_dir, ["ops@example.com"], + retrieve_stall_seconds=7200) calls = env.calls() finally: env.__exit__(None, None, None) @@ -164,7 +165,8 @@ def test_retrieve_stalls_twice_raises_after_max_attempts(self): archive.ship_object( self.source_folder, self.arcnames, "testproj__shard-0of1", self.catalog, self.fortress_base_dir, self.tmp_dir, - self.log_dir, ["ops@example.com"]) + self.log_dir, ["ops@example.com"], + retrieve_stall_seconds=7200) calls = env.calls() finally: env.__exit__(None, None, None) @@ -189,7 +191,8 @@ def test_nonstall_extract_failure_is_not_retried(self): archive.ship_object( self.source_folder, self.arcnames, "testproj__shard-0of1", self.catalog, self.fortress_base_dir, self.tmp_dir, - self.log_dir, ["ops@example.com"]) + self.log_dir, ["ops@example.com"], + retrieve_stall_seconds=7200) calls = env.calls() finally: env.__exit__(None, None, None) diff --git a/tests/test_stall_detection.py b/tests/test_stall_detection.py index 76536c7..a6c6228 100644 --- a/tests/test_stall_detection.py +++ b/tests/test_stall_detection.py @@ -204,7 +204,8 @@ def test_hsi_ls_timeout_treated_as_not_landed(self): archive.ship_object( self.source_folder, self.arcnames, "testproj__shard-0of1", self.catalog, self.fortress_base_dir, self.tmp_dir, - self.log_dir, ["ops@example.com"]) + self.log_dir, ["ops@example.com"], + retrieve_stall_seconds=7200) elapsed = time.monotonic() - start finally: os.environ["PATH"] = old_path