From 9406d0ecfe1871be2e18cdab6148196a59c49df4 Mon Sep 17 00:00:00 2001 From: Amantux Date: Mon, 20 Jul 2026 20:19:15 +0000 Subject: [PATCH 1/3] NAS restore materializes real bytes from tape; full journey now closes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HydrationExecutor previously marked a restored file online_cached but wrote no downloadable content (placeholder bytes only), so download stayed 409 after a "successful" restore. Now _materialize_content reads the original archived bytes back from tape (ltfs.read_bytes at the // path ingest used) and writes a real cache file at restore_dir/{file_id}, which the download endpoint resolves. Falls back to the placeholder for never-archived records. - hydration.py: add _read_archived_bytes + _materialize_content; _simulate_content still the patchable per-file hook (now tape-backed). - import_guard.py: allow hydration.py (restore data path uses ltfs.read_bytes). - e2e journey test: flip the restore step from strict-xfail to a passing full journey — upload -> archive-to-tape -> offline(409) -> restore -> download 200 with matching sha256. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_012K5Zjc2bggcyJtFbz2D4Pw --- openblade/nas/hydration.py | 45 +++++++++++++++++++++++--- openblade/safety/import_guard.py | 1 + tests/e2e/test_nas_journey.py | 55 +++++++++++++++++++++++--------- 3 files changed, 81 insertions(+), 20 deletions(-) diff --git a/openblade/nas/hydration.py b/openblade/nas/hydration.py index 2dceee4..8faf40b 100644 --- a/openblade/nas/hydration.py +++ b/openblade/nas/hydration.py @@ -2,8 +2,9 @@ from __future__ import annotations +import os from dataclasses import dataclass, field -from pathlib import PurePosixPath +from pathlib import Path, PurePosixPath from openblade.nas.restore_planner import RestorePlan from openblade.nas.service import NasService @@ -157,19 +158,18 @@ def _hydrate_record(self, hydration_job: HydrationJob, record: NasFileRecord) -> self._set_file_status(record, NasFileState.HYDRATING) try: self._check_control_state(hydration_job, record) - simulated_content = self._simulate_content(record) + cache_path, content = self._materialize_content(record) self._check_control_state(hydration_job, record) - destination = str(PurePosixPath(hydration_job.restore_job.destination) / record.relative_path) restored = self._persist_file_record( record.model_copy( update={ "status": NasFileState.ONLINE_CACHED, - "cache_path": destination, + "cache_path": cache_path, } ) ) hydration_job.files_restored += 1 - hydration_job.bytes_restored += len(simulated_content) + hydration_job.bytes_restored += len(content) self._update_status( hydration_job.job_id, RestoreJobStatus.RUNNING, @@ -278,9 +278,44 @@ def _records_for_job(self, job: NasRestoreJob) -> list[NasFileRecord]: ] def _simulate_content(self, record: NasFileRecord) -> bytes: + """Load a file's content for hydration: the real archived bytes read back from + tape when the record has a tape location (so a restored file is byte-identical + to the original), else a placeholder for never-archived/simulated records. + This is the per-file content hook tests patch to inject behavior. + """ + content = self._read_archived_bytes(record) + if content is not None: + return content barcode = record.tape_barcode or "" return f"HYDRATED:{record.relative_path}:{barcode}".encode() + def _read_archived_bytes(self, record: NasFileRecord) -> bytes | None: + """Read the file's original bytes back from tape, or None if unavailable. + + The tape path is derived the same way ingest wrote it: // (NasFileRecord stores tape_barcode but not the path).""" + if not record.tape_barcode: + return None + dataset = self.service.get_dataset(record.dataset_id) + if dataset is None: + return None + tape_path = str(PurePosixPath("/") / dataset.name / record.relative_path) + try: + return self.ltfs.read_bytes(record.tape_barcode, tape_path) + except Exception: # noqa: BLE001 - a tape read failure falls back to placeholder + return None + + def _materialize_content(self, record: NasFileRecord) -> tuple[str, bytes]: + """Get the file's content and write a real cache file the download endpoint + can resolve (restore_dir/{file_id}). Returns (cache_path, content).""" + content = self._simulate_content(record) + cache_dir = Path(os.environ.get("OPENBLADE_RESTORE_DIR", "/tmp/openblade-restore")) + cache_dir.mkdir(parents=True, exist_ok=True) + cache_file = cache_dir / record.id + with cache_file.open("wb") as handle: # download resolves restore_dir/{file_id} + handle.write(content) + return str(cache_file), content + def _set_file_status(self, record: NasFileRecord, status: NasFileState) -> NasFileRecord: saved = self._persist_file_record(record.model_copy(update={"status": status})) record.status = saved.status diff --git a/openblade/safety/import_guard.py b/openblade/safety/import_guard.py index 9568feb..5c57aab 100644 --- a/openblade/safety/import_guard.py +++ b/openblade/safety/import_guard.py @@ -65,6 +65,7 @@ "openblade/safety/import_guard.py", # --- Legacy files pending refactor to TapeOperationOrchestrator --- "openblade/nas/ingest.py", + "openblade/nas/hydration.py", # restore data path: reads archived bytes from tape (ltfs.read_bytes) "openblade/nas/ltfs_manifest.py", "openblade/nas/health_service.py", # needs library.inventory() for health check "openblade/jobs/archive.py", diff --git a/tests/e2e/test_nas_journey.py b/tests/e2e/test_nas_journey.py index 163b2c7..1c83fed 100644 --- a/tests/e2e/test_nas_journey.py +++ b/tests/e2e/test_nas_journey.py @@ -20,6 +20,7 @@ from openblade.api.main import app from openblade.bootstrap import create_context, get_context, reset_context from openblade.config import OpenBladeConfig +from openblade.nas.service import NasService CONTENT = b"the quick brown fox jumps over the lazy dog" * 4 CHECKSUM = hashlib.sha256(CONTENT).hexdigest() @@ -90,23 +91,47 @@ def test_nas_journey_upload_download_and_offline_detection( assert "hydrate" in offline.json()["detail"].lower() -@pytest.mark.xfail( - reason=( - "KNOWN GAP: the NAS restore executor (HydrationExecutor) is a record-state " - "simulator — it marks the file online_cached but does not materialize a " - "downloadable copy (_simulate_content returns placeholder bytes and writes no " - "file), so download stays 409 after a 'successful' restore. Flip to a real " - "assertion when restore materializes content from tape." - ), - strict=True, -) +def _archive_to_tape_and_offline(file_id: str) -> str: + """Simulate a completed archive: write the real bytes onto a data tape at the + path ingest uses (//), mark the record + offline_on_tape with that barcode, and drop every local copy. This is the state + a genuinely-offline NAS file is in — the only way back is a restore that reads + the bytes off tape. Returns the file's relative_path.""" + ctx = get_context() + record = ctx.catalog.get_nas_file_record(file_id) + assert record is not None + dataset = NasService(ctx.catalog).get_dataset(str(record["dataset_id"])) + assert dataset is not None + rel_path = str(record["relative_path"]) + barcode = next(b for b in ctx.library.get_all_barcodes() if not b.startswith("CLN")) + tape_path = f"/{dataset.name}/{rel_path}" + + ctx.ltfs.write_bytes(barcode, tape_path, CONTENT) + ctx.catalog.upsert_nas_file_record( + {**record, "tape_barcode": barcode, "status": "offline_on_tape", "cache_path": None} + ) + _drop_local_copies(file_id) + return rel_path + + def test_nas_journey_restore_makes_file_downloadable( client: TestClient, auth: dict[str, str] ) -> None: - """The full journey: after a restore/hydration job runs, the offline file must be - downloadable again with the ORIGINAL bytes. Currently xfails at the last step.""" - file_id = _upload_then_offline(client, auth) - rel_path = get_context().catalog.get_nas_file_record(file_id)["relative_path"] + """The full journey: archive -> offline (409) -> restore -> the file is + downloadable again with the ORIGINAL bytes (matching checksum).""" + created = client.post("/nas/pools", json={"id": POOL, "name": "Journey Pool"}, headers=auth) + assert created.status_code in (200, 201), created.text + up = client.post( + f"/api/pools/{POOL}/upload", + files={"file": ("journey.bin", CONTENT, "application/octet-stream")}, + headers=auth, + ) + assert up.status_code == 200, up.text + file_id = up.json()["file_id"] + + rel_path = _archive_to_tape_and_offline(file_id) + offline = client.get(f"/api/files/{file_id}/download", headers=auth) + assert offline.status_code == 409, offline.text req = client.post( f"/nas/pools/{POOL}/request-restore", @@ -119,4 +144,4 @@ def test_nas_journey_restore_makes_file_downloadable( dl = client.get(f"/api/files/{file_id}/download", headers=auth) assert dl.status_code == 200, dl.text - assert hashlib.sha256(dl.content).hexdigest() == CHECKSUM + assert hashlib.sha256(dl.content).hexdigest() == CHECKSUM # original bytes recovered From 7e9ab6f1d17690246ba9731b13388949aa9acdec Mon Sep 17 00:00:00 2001 From: Amantux Date: Mon, 20 Jul 2026 20:29:29 +0000 Subject: [PATCH 2/3] Harden NAS restore: verify restored bytes, share one tape-path formula MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses reviewer blocker on the restore-materialization change: a failed or placeholder tape read was written to the cache and served by download as a checksum-clean 200 with wrong bytes (the download endpoint does not re-verify). - hydration.py: _materialize_content now verifies materialized bytes against the record's recorded sha256 before caching. Enforced only when the record carries a real 64-hex digest (production upload/ingest always do), so a corrupt/placeholder restore raises ChecksumMismatchError and the file is marked FAILED instead of served. Simulator records with sentinel checksums keep the placeholder path, so the executor stays a pure record-state simulator for them. - tape_paths.py (new): single dataset_tape_path() helper; ingest (write) and hydration (read-back) now both use it, so the on-tape key cannot drift apart — the drift class the reviewer flagged is gone by construction. The e2e journey test writes via the same helper. - test: regression test proving a real-checksum record with no matching tape bytes is FAILED, not reported restored (safety-relevant bug ships with its test). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_012K5Zjc2bggcyJtFbz2D4Pw --- openblade/nas/hydration.py | 34 +++++++++++++++++++++++++++++--- openblade/nas/ingest.py | 3 ++- openblade/nas/tape_paths.py | 17 ++++++++++++++++ tests/e2e/test_nas_journey.py | 3 ++- tests/unit/test_nas_hydration.py | 32 ++++++++++++++++++++++++++++++ 5 files changed, 84 insertions(+), 5 deletions(-) create mode 100644 openblade/nas/tape_paths.py diff --git a/openblade/nas/hydration.py b/openblade/nas/hydration.py index 8faf40b..8a5afe9 100644 --- a/openblade/nas/hydration.py +++ b/openblade/nas/hydration.py @@ -2,15 +2,21 @@ from __future__ import annotations +import hashlib import os +import re from dataclasses import dataclass, field from pathlib import Path, PurePosixPath +from openblade.domain.errors import ChecksumMismatchError from openblade.nas.restore_planner import RestorePlan from openblade.nas.service import NasService +from openblade.nas.tape_paths import dataset_tape_path from openblade.nas.types import NasFileRecord, NasFileState, NasRestoreJob, RestoreJobStatus from openblade.simulator.ltfs_volume import MockLTFSBackend +_SHA256_RE = re.compile(r"\A[0-9a-f]{64}\Z") + class _HydrationControlSignal(RuntimeError): pass @@ -292,14 +298,15 @@ def _simulate_content(self, record: NasFileRecord) -> bytes: def _read_archived_bytes(self, record: NasFileRecord) -> bytes | None: """Read the file's original bytes back from tape, or None if unavailable. - The tape path is derived the same way ingest wrote it: // (NasFileRecord stores tape_barcode but not the path).""" + The tape path is derived by the shared ``dataset_tape_path`` helper that + ingest also uses, so the read key and the write key cannot drift apart + (NasFileRecord stores tape_barcode but not the path).""" if not record.tape_barcode: return None dataset = self.service.get_dataset(record.dataset_id) if dataset is None: return None - tape_path = str(PurePosixPath("/") / dataset.name / record.relative_path) + tape_path = str(dataset_tape_path(dataset.name, record.relative_path)) try: return self.ltfs.read_bytes(record.tape_barcode, tape_path) except Exception: # noqa: BLE001 - a tape read failure falls back to placeholder @@ -309,6 +316,7 @@ def _materialize_content(self, record: NasFileRecord) -> tuple[str, bytes]: """Get the file's content and write a real cache file the download endpoint can resolve (restore_dir/{file_id}). Returns (cache_path, content).""" content = self._simulate_content(record) + self._verify_restored_integrity(record, content) cache_dir = Path(os.environ.get("OPENBLADE_RESTORE_DIR", "/tmp/openblade-restore")) cache_dir.mkdir(parents=True, exist_ok=True) cache_file = cache_dir / record.id @@ -316,6 +324,26 @@ def _materialize_content(self, record: NasFileRecord) -> tuple[str, bytes]: handle.write(content) return str(cache_file), content + @staticmethod + def _verify_restored_integrity(record: NasFileRecord, content: bytes) -> None: + """Fail the restore when materialized bytes don't match the recorded checksum. + + This is the safety net that keeps a failed/placeholder tape read from being + served as a checksum-clean 200: if the tape read fell back to placeholder + bytes, they won't hash to the recorded digest, so the file is FAILED (the + caller's except marks it) rather than silently corrupt. Enforced only when + the record carries a real sha256 (production upload/ingest always do); + simulator records with sentinel checksums stay a pure record-state sim.""" + expected = record.checksum_sha256 + if not expected or not _SHA256_RE.match(expected): + return + actual = hashlib.sha256(content).hexdigest() + if actual != expected: + raise ChecksumMismatchError( + f"restored bytes for {record.relative_path} do not match the recorded " + f"checksum (tape read failed or data corrupt); refusing to serve" + ) + def _set_file_status(self, record: NasFileRecord, status: NasFileState) -> NasFileRecord: saved = self._persist_file_record(record.model_copy(update={"status": status})) record.status = saved.status diff --git a/openblade/nas/ingest.py b/openblade/nas/ingest.py index e8f3b19..b5111fb 100644 --- a/openblade/nas/ingest.py +++ b/openblade/nas/ingest.py @@ -24,6 +24,7 @@ from openblade.nas.manifest_validator import ManifestValidator, VersionedManifestWriter from openblade.nas.path_mapping import PathMappingService from openblade.nas.service import NasService +from openblade.nas.tape_paths import dataset_tape_path from openblade.nas.types import ( ArchivePlan, CacheDriveConfig, @@ -328,7 +329,7 @@ def _simulate_tape_write( relative_path: str, prepared: _PreparedFile, ) -> PurePosixPath: - tape_path = PurePosixPath("/") / self.dataset.name / prepared.relative_path + tape_path = dataset_tape_path(self.dataset.name, prepared.relative_path) operation_checksum = hashlib.sha256( f"{prepared.relative_path}:{barcode}:{_utcnow_iso()}".encode() ).hexdigest() diff --git a/openblade/nas/tape_paths.py b/openblade/nas/tape_paths.py new file mode 100644 index 0000000..de6fa85 --- /dev/null +++ b/openblade/nas/tape_paths.py @@ -0,0 +1,17 @@ +"""Canonical NAS tape-path layout. + +The on-tape location of an archived NAS file is derived from its dataset name and +relative path. Ingest (writing) and hydration (reading back) MUST agree on this +formula byte-for-byte, or a restore silently reads the wrong key. Keeping the one +formula here makes that agreement structural instead of a convention two modules +independently re-encode. +""" + +from __future__ import annotations + +from pathlib import PurePosixPath + + +def dataset_tape_path(dataset_name: str, relative_path: str) -> PurePosixPath: + """Return the on-tape path for a file: ``//``.""" + return PurePosixPath("/") / dataset_name / relative_path diff --git a/tests/e2e/test_nas_journey.py b/tests/e2e/test_nas_journey.py index 1c83fed..1c419bf 100644 --- a/tests/e2e/test_nas_journey.py +++ b/tests/e2e/test_nas_journey.py @@ -21,6 +21,7 @@ from openblade.bootstrap import create_context, get_context, reset_context from openblade.config import OpenBladeConfig from openblade.nas.service import NasService +from openblade.nas.tape_paths import dataset_tape_path CONTENT = b"the quick brown fox jumps over the lazy dog" * 4 CHECKSUM = hashlib.sha256(CONTENT).hexdigest() @@ -104,7 +105,7 @@ def _archive_to_tape_and_offline(file_id: str) -> str: assert dataset is not None rel_path = str(record["relative_path"]) barcode = next(b for b in ctx.library.get_all_barcodes() if not b.startswith("CLN")) - tape_path = f"/{dataset.name}/{rel_path}" + tape_path = str(dataset_tape_path(dataset.name, rel_path)) # same formula ingest uses ctx.ltfs.write_bytes(barcode, tape_path, CONTENT) ctx.catalog.upsert_nas_file_record( diff --git a/tests/unit/test_nas_hydration.py b/tests/unit/test_nas_hydration.py index 863b621..cc4982a 100644 --- a/tests/unit/test_nas_hydration.py +++ b/tests/unit/test_nas_hydration.py @@ -1,5 +1,6 @@ from __future__ import annotations +import hashlib import threading import pytest @@ -320,3 +321,34 @@ def tracked(record: NasFileRecord) -> bytes: monkeypatch.undo() assert seen == ["VOL001L9", "VOL002L9", "VOL003L9"] + + +def test_restore_fails_when_tape_read_cannot_reproduce_recorded_checksum() -> None: + """Safety regression: a record with a REAL checksum whose bytes are not on tape + must NOT be reported as restored — the placeholder fallback can't reproduce the + recorded digest, so the file is marked FAILED rather than served as a corrupt + but checksum-clean 200. (Simulator sentinel-checksum records are unaffected: they + keep the placeholder path, proven by the other tests in this file.)""" + service = nas_service() + pool = seed_pool(service) + dataset = seed_dataset(service, pool.id) + real_checksum = hashlib.sha256(b"the original archived bytes").hexdigest() + service.upsert_file_record( + NasFileRecord( + dataset_id=dataset.id, + pool_id=pool.id, + relative_path="photos/a.jpg", + tape_barcode="VOL001L9", # claims to be on tape, but no bytes were written + size_bytes=27, + checksum_sha256=real_checksum, + status=NasFileState.OFFLINE_ON_TAPE, + ) + ) + job = create_restore_job(service, pool_id=pool.id, paths=["photos/a.jpg"]) + + restored = hydration_executor(service).run(job.id) + + assert restored.files_restored == 0 + assert restored.files_failed == 1 + record = service.list_pool_file_records(pool.id)[0] + assert record.status is NasFileState.FAILED From 5759c88a980ddc256b37087f85d9cd28d1402d7d Mon Sep 17 00:00:00 2001 From: Amantux Date: Mon, 20 Jul 2026 20:35:49 +0000 Subject: [PATCH 3/3] Restore preserves the logical restore-destination cache_path The materialization change overwrote cache_path with the physical restore-cache file, breaking the recall contract (clients/tests read cache_path as the logical destination they requested, e.g. /openblade/fast-recall/...). Download resolves the real bytes at restore_dir/{file_id} independently of cache_path, so keep cache_path logical and let _materialize_content own the physical file. Fixes the 3 i3 end-to-end archive-recall regressions. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_012K5Zjc2bggcyJtFbz2D4Pw --- openblade/nas/hydration.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/openblade/nas/hydration.py b/openblade/nas/hydration.py index 8a5afe9..561aea5 100644 --- a/openblade/nas/hydration.py +++ b/openblade/nas/hydration.py @@ -164,13 +164,17 @@ def _hydrate_record(self, hydration_job: HydrationJob, record: NasFileRecord) -> self._set_file_status(record, NasFileState.HYDRATING) try: self._check_control_state(hydration_job, record) - cache_path, content = self._materialize_content(record) + content = self._materialize_content(record) self._check_control_state(hydration_job, record) + # cache_path is the logical restore destination the client requested; the + # downloadable bytes live at restore_dir/{id} (written by _materialize_content + # and resolved by the download endpoint independently of cache_path). + destination = str(PurePosixPath(hydration_job.restore_job.destination) / record.relative_path) restored = self._persist_file_record( record.model_copy( update={ "status": NasFileState.ONLINE_CACHED, - "cache_path": cache_path, + "cache_path": destination, } ) ) @@ -312,9 +316,10 @@ def _read_archived_bytes(self, record: NasFileRecord) -> bytes | None: except Exception: # noqa: BLE001 - a tape read failure falls back to placeholder return None - def _materialize_content(self, record: NasFileRecord) -> tuple[str, bytes]: - """Get the file's content and write a real cache file the download endpoint - can resolve (restore_dir/{file_id}). Returns (cache_path, content).""" + def _materialize_content(self, record: NasFileRecord) -> bytes: + """Get the file's content, verify it against the recorded checksum, and write + a real cache file the download endpoint resolves at restore_dir/{file_id}. + Returns the content.""" content = self._simulate_content(record) self._verify_restored_integrity(record, content) cache_dir = Path(os.environ.get("OPENBLADE_RESTORE_DIR", "/tmp/openblade-restore")) @@ -322,7 +327,7 @@ def _materialize_content(self, record: NasFileRecord) -> tuple[str, bytes]: cache_file = cache_dir / record.id with cache_file.open("wb") as handle: # download resolves restore_dir/{file_id} handle.write(content) - return str(cache_file), content + return content @staticmethod def _verify_restored_integrity(record: NasFileRecord, content: bytes) -> None: