From 68b2876e17dc96901f6dd678675a1722d9133c08 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 15 Sep 2026 10:39:13 +0000 Subject: [PATCH] Upload a series manifest from DID's file cache after ingest The round-trip test in tests/test_cloud_file_series_round_trip.py caught a real bug on TEST_USER_1 last night: uploadDataset silently sent no bytes for a series manifest, and the following download 404'd on the manifest uid ("Manifest for file series 'chunkdata.bin' (uid ...) is not a file of this dataset"). The reconstruction paths on the download side both land on ingest_locations empty, and DID's MembersNotLocatable guard refuses the doc. Root cause: file_uploads_for_document walks files.file_info[].locations[].location and requires os.path.exists on the recorded path. did.document.Document.add_file_series writes the manifest to a tempfile with delete_original=True by default, so database_add copies it into DID's FileDir/ and removes the source. By the time the file pass runs, the recorded location is gone; the bytes are still on disk under the uid, but the upload never sees them. Fix: teach file_uploads_for_document to fall back to did.file.cached_path_for_uid(uid, additional_roots=[...]) when the recorded path is gone. Callers pass the local dataset's DID FileDir via _did_file_store_roots(dataset). Threaded through uploadFilesForDatasetDocuments -> uploadDataset and every _upload_binaries call site in sync.operations (uploadNew, mirrorToRemote, twoWaySync). The recorded location is still preferred when it resolves, so a doc whose user-provided source path is still on disk uploads from there -- this fix only fires on ingested bytes whose source has moved. Also a latent safety net: an ordinary file whose source path was deleted or moved after add_file gets picked up from DID's cache too, rather than being silently missing from the remote. Offline coverage in tests/test_cloud_upload_uses_did_cache.py: - fallback finds the file by uid in additional_roots; - without roots, the fallback does not fire (backwards compatible); - a live recorded path is preferred over the cache; - no uid, or neither location resolves, yields nothing (no upload); - a MATLAB-shaped bare-dict locations still works; - end-to-end via ndi_dataset_dir + database_add on a demoNDISeries doc, proving the recorded location IS gone post-ingest and the manifest is recoverable from the FileDir root. Closes #306. Refs: Waltham-Data-Science/NDI-python#304, #305 (round-trip test that caught this). Co-Authored-By: Claude Opus 4.7 Claude-Session: https://claude.ai/code/session_01AdGZ8fRPeY9JSpUHgXm9ao --- src/ndi/cloud/orchestration.py | 24 +++ src/ndi/cloud/sync/operations.py | 11 + src/ndi/cloud/upload.py | 53 ++++- tests/test_cloud_upload_uses_did_cache.py | 238 ++++++++++++++++++++++ 4 files changed, 321 insertions(+), 5 deletions(-) create mode 100644 tests/test_cloud_upload_uses_did_cache.py diff --git a/src/ndi/cloud/orchestration.py b/src/ndi/cloud/orchestration.py index 5d73f0a1..0b01d5ab 100644 --- a/src/ndi/cloud/orchestration.py +++ b/src/ndi/cloud/orchestration.py @@ -24,6 +24,29 @@ logger = logging.getLogger(__name__) +def _did_file_store_roots(dataset: Any) -> list[str]: + """Directories to search for a document's binaries when the recorded + location is gone. + + DID ingests a file with ``delete_original=True`` by default -- it copies + the bytes into ``/`` and removes the source path -- so the + ``file_info`` location the document holds no longer resolves on disk. + Returning the local dataset's DID ``FileDir`` here lets + :func:`ndi.cloud.upload.file_uploads_for_document` recover the bytes by + uid. See Waltham-Data-Science/NDI-python#306. + + Reads defensively: an unusual dataset shape or a DID rename must not + stop an upload of the doc whose location is still resolvable on its own. + """ + try: + binary_path = dataset._session._database.binary_path + except Exception: # noqa: BLE001 - a stand-in dataset is not ours to fix + return [] + if not binary_path: + return [] + return [str(binary_path)] + + def _remove_empty_staging(staging: Path) -> None: """Remove the staging tree, if ingestion emptied it. @@ -539,6 +562,7 @@ def uploadDataset( client.config.org_id, cloud_id, doc_jsons, + additional_roots=_did_file_store_roots(dataset), client=client, ) if verbose: diff --git a/src/ndi/cloud/sync/operations.py b/src/ndi/cloud/sync/operations.py index 36c0ac77..6dd3c332 100644 --- a/src/ndi/cloud/sync/operations.py +++ b/src/ndi/cloud/sync/operations.py @@ -332,6 +332,7 @@ def _upload_binaries( documents: list[dict[str, Any]], options: SyncOptions, *, + dataset: Any = None, client: CloudClient | None, ) -> set[str]: """Upload each document's binaries; return the ids whose binaries failed. @@ -341,9 +342,15 @@ def _upload_binaries( gets a 404 (NDI-matlab#805). uploadFilesForDatasetDocuments does not raise on a per-file failure -- it reports one -- so catching an exception was never going to see the ordinary case. + + *dataset* threads the local dataset's DID ``FileDir`` down to + :func:`ndi.cloud.upload.file_uploads_for_document` so a doc whose + recorded location is gone (a series manifest tempfile after ingest) + is still uploaded from its bytes in the file store. See #306. """ if not options.sync_files or not documents: return set() + from ..orchestration import _did_file_store_roots from ..upload import document_id, uploadFilesForDatasetDocuments try: @@ -351,6 +358,7 @@ def _upload_binaries( getattr(getattr(client, "config", None), "org_id", ""), cloud_dataset_id, documents, + additional_roots=_did_file_store_roots(dataset) if dataset is not None else None, client=client, ) except Exception as exc: # noqa: BLE001 @@ -621,6 +629,7 @@ def uploadNew( cloud_dataset_id, [documents[i] for i in uploaded if i in documents], options, + dataset=dataset, client=client, ) if binaries_failed: @@ -798,6 +807,7 @@ def mirrorToRemote( cloud_dataset_id, [documents[i] for i in uploaded if i in documents], options, + dataset=dataset, client=client, ) if binaries_failed: @@ -1062,6 +1072,7 @@ def twoWaySync( cloud_dataset_id, [documents[i] for i in uploaded if i in documents], options, + dataset=dataset, client=client, ) if binaries_failed: diff --git a/src/ndi/cloud/upload.py b/src/ndi/cloud/upload.py index 2d4e28ed..c79a447c 100644 --- a/src/ndi/cloud/upload.py +++ b/src/ndi/cloud/upload.py @@ -157,6 +157,7 @@ def uploadFilesForDatasetDocuments( dataset_id: str, documents: list[dict[str, Any]], *, + additional_roots: list[str] | None = None, client: CloudClient | None = None, ) -> dict[str, Any]: """Upload associated binary files for a list of documents. @@ -168,6 +169,10 @@ def uploadFilesForDatasetDocuments( org_id: Organisation ID. dataset_id: Cloud dataset ID. documents: List of document property dicts. + additional_roots: Extra directories to search for a file whose + recorded ``location`` no longer exists on disk -- typically + the dataset's DID ``FileDir``. Passed through to + :func:`file_uploads_for_document`. client: Authenticated cloud client (auto-created if omitted). Returns: @@ -188,7 +193,9 @@ def uploadFilesForDatasetDocuments( for doc in documents: doc_id = document_id(doc) - for file_uid, file_path in file_uploads_for_document(doc): + for file_uid, file_path in file_uploads_for_document( + doc, additional_roots=additional_roots + ): try: url = files_api.getFileUploadURL(org_id, dataset_id, file_uid, client=client) files_api.putFiles(url, file_path) @@ -202,7 +209,11 @@ def uploadFilesForDatasetDocuments( return report -def file_uploads_for_document(doc: dict[str, Any]) -> list[tuple[str, str]]: +def file_uploads_for_document( + doc: dict[str, Any], + *, + additional_roots: list[str] | None = None, +) -> list[tuple[str, str]]: """The ``(file_uid, local_path)`` pairs to upload for one document. TWO SHAPES REACH THIS, AND ONLY ONE USED TO BE READ. @@ -219,9 +230,19 @@ def file_uploads_for_document(doc: dict[str, Any]) -> list[tuple[str, str]]: Locations already rewritten to ``ndic://`` are skipped: that scheme means the file is on the cloud, which is the opposite of something to upload. - A location that no longer exists on disk is skipped too -- there is - nothing to send -- and the caller learns of it as a document whose - binaries did not arrive. + + A location that no longer resolves on disk is looked up by uid in + *additional_roots* (typically the dataset's DID ``FileDir``, plus DID's + global file cache via :func:`did.file.cached_path_for_uid`). The + recorded location is the source path the caller passed to ``add_file``, + and DID's ingest moves the bytes into ``/`` with + ``delete_original=True`` by default -- so a series manifest, which + ``did.document.add_file_series`` writes to a tempfile, is gone from its + original path by the time the upload runs. The bytes are still there, + keyed by uid, and this fallback finds them. Without it, a series + document's manifest is silently never uploaded and every reader on the + cloud side sees "manifest not a file of this dataset" and a series it + cannot open. See Waltham-Data-Science/NDI-python#306. """ import os @@ -253,11 +274,33 @@ def file_uploads_for_document(doc: dict[str, Any]) -> list[tuple[str, str]]: if not local_path and candidate and "://" not in candidate: if os.path.exists(candidate): local_path = candidate + if uid and not local_path: + local_path = _cached_path_for_uid(uid, additional_roots) if uid and local_path: pairs.append((uid, local_path)) return pairs +def _cached_path_for_uid(uid: str, additional_roots: list[str] | None) -> str: + """Look ``uid`` up in DID's on-disk stores, or return ``""``. + + Isolated so a did-python import failure never breaks the ordinary + recorded-location path: DID is a hard dependency at runtime, but this + helper is on the fallback path and swallowing an ImportError keeps the + upload behaviour of a doc with a live location identical. + """ + if not uid: + return "" + try: + from did.file import cached_path_for_uid + except ImportError: # pragma: no cover - DID is a hard runtime dep + return "" + try: + return str(cached_path_for_uid(uid, additional_roots=additional_roots) or "") + except Exception: # noqa: BLE001 - a malformed uid must not fail the upload + return "" + + @_auto_client def uploadSingleFile( dataset_id: str, diff --git a/tests/test_cloud_upload_uses_did_cache.py b/tests/test_cloud_upload_uses_did_cache.py new file mode 100644 index 00000000..4668ca83 --- /dev/null +++ b/tests/test_cloud_upload_uses_did_cache.py @@ -0,0 +1,238 @@ +"""``file_uploads_for_document`` finds an ingested binary in DID's file cache. + +The recorded ``files.file_info[].locations[].location`` is the source path +the caller passed to ``add_file``. For a series manifest, +``did.document.Document.add_file_series`` writes that source path to a +``tempfile.NamedTemporaryFile(prefix="did_manifest_")`` -- and add_file +defaults ``delete_original=True`` on a ``file`` location, so +``did.database.add_docs`` copies the manifest into DID's ``FileDir/`` +and removes the source. By the time :func:`ndi.cloud.upload.uploadDataset`'s +file pass runs, the recorded location no longer resolves and the manifest +would be silently missing from the upload -- which is what run 227 caught +on TEST_USER_1 (Waltham-Data-Science/NDI-python#306). + +The fix: teach :func:`ndi.cloud.upload.file_uploads_for_document` to fall +back to ``did.file.cached_path_for_uid(uid, additional_roots=[...])`` when +the recorded location is gone. Callers pass the local dataset's +``binary_path`` in *additional_roots*. + +Live-cloud coverage of the whole round trip is +``tests/test_cloud_file_series_round_trip.py``; this file is the offline +half, no credentials required. +""" + +from __future__ import annotations + +import os +import uuid +from pathlib import Path + +import pytest +from did.document import Document as DIDDocument + +from ndi.cloud.orchestration import _did_file_store_roots +from ndi.cloud.upload import file_uploads_for_document +from ndi.dataset import ndi_dataset_dir +from ndi.document import ndi_document + +SERIES_NAME = "chunkdata.bin" + + +@pytest.fixture +def cache_dir(tmp_path): + """A directory that stands in for DID's FileDir. + + Just a folder holding ```` blobs. Nothing else about DID is + reached in these tests -- ``file_uploads_for_document``'s fallback is + :func:`did.file.cached_path_for_uid`, which is a pure function of the + uid and the roots. + """ + d = tmp_path / "file_cache" + d.mkdir() + return d + + +class TestTheFallbackFindsAnIngestedManifest: + """The MATLAB-style file_info shape (one entry, one location) and the + minimum every caller of ``file_uploads_for_document`` promises. + + The recorded location has vanished (as it does after DID ingests a + manifest tempfile), so this pins the shape of the fallback rather than + driving it through a real ``database_add``. That case has its own test + below; here the minimum is a ``file_info`` doc with one uid and a + missing location. + """ + + @staticmethod + def _doc(uid, gone_location): + return { + "base": {"id": "doc1"}, + "files": { + "file_info": [ + { + "name": SERIES_NAME, + "locations": [ + { + "uid": uid, + "location": str(gone_location), + "location_type": "file", + "ingest": 1, + "delete_original": 0, + "parameters": "", + } + ], + } + ] + }, + } + + def test_the_manifest_is_found_by_uid_in_the_file_cache(self, tmp_path, cache_dir): + uid = "u" + uuid.uuid4().hex + (cache_dir / uid).write_bytes(b"manifest bytes") + doc = self._doc(uid, tmp_path / "gone.manifest") + + pairs = file_uploads_for_document(doc, additional_roots=[str(cache_dir)]) + + assert pairs == [(uid, str(cache_dir / uid))] + + def test_without_roots_the_fallback_does_not_fire(self, tmp_path, cache_dir): + """The default remains what it was: a doc whose location is gone + produces no pair. Callers that had this working before this fix + still do.""" + uid = "u" + uuid.uuid4().hex + (cache_dir / uid).write_bytes(b"manifest bytes") + doc = self._doc(uid, tmp_path / "gone.manifest") + + assert file_uploads_for_document(doc) == [] + + def test_a_live_recorded_location_is_still_preferred(self, tmp_path, cache_dir): + """When the recorded path resolves, use it. Test data is separable + from DID's cache, and preferring the recorded path avoids reading + from the cache while the source is still there. + """ + uid = "u" + uuid.uuid4().hex + live = tmp_path / "live.bin" + live.write_bytes(b"live source bytes") + (cache_dir / uid).write_bytes(b"cached bytes") + doc = self._doc(uid, live) + + pairs = file_uploads_for_document(doc, additional_roots=[str(cache_dir)]) + + assert pairs == [(uid, str(live))] + + def test_a_missing_uid_produces_nothing(self, tmp_path, cache_dir): + """No uid means nothing to look up; the fallback stays quiet.""" + doc = self._doc("", tmp_path / "gone") + + assert file_uploads_for_document(doc, additional_roots=[str(cache_dir)]) == [] + + def test_neither_recorded_nor_cached_produces_nothing(self, tmp_path, cache_dir): + """The caller learns of it as a failed document, not as a silent + skip. Same shape as before this fix: no pair, no upload, and the + report at :func:`uploadFilesForDatasetDocuments` marks the doc's + binary as failed.""" + uid = "u" + uuid.uuid4().hex + # cache_dir has no file this time. + doc = self._doc(uid, tmp_path / "gone.manifest") + + assert file_uploads_for_document(doc, additional_roots=[str(cache_dir)]) == [] + + def test_a_matlab_shaped_locations_dict_still_works(self, tmp_path, cache_dir): + """MATLAB's jsonencode writes a one-element locations struct as a + bare object. Same shape guard :func:`file_uploads_for_document` + already had for the recorded-path case; it has to apply to the + fallback too. + """ + uid = "u" + uuid.uuid4().hex + (cache_dir / uid).write_bytes(b"manifest bytes") + doc = self._doc(uid, tmp_path / "gone.manifest") + # Collapse the single-element list into a bare dict, MATLAB-style. + doc["files"]["file_info"][0]["locations"] = doc["files"]["file_info"][0]["locations"][0] + + pairs = file_uploads_for_document(doc, additional_roots=[str(cache_dir)]) + + assert pairs == [(uid, str(cache_dir / uid))] + + +class TestEndToEndAgainstARealDatabase: + """The actual failure mode #306 names, driven through the same code + path a live upload uses. + + ``add_file_series`` writes the manifest to a tempfile; ``database_add`` + copies it into DID's FileDir and removes the source. The recorded + location is then gone. ``file_uploads_for_document`` with the + dataset's FileDir in *additional_roots* recovers the manifest by uid + and yields a ``(uid, cached_path)`` pair the upload can send. + """ + + def test_the_manifest_bytes_are_uploadable_after_ingest(self, tmp_path): + local = ndi_dataset_dir("test_series_ds", str(tmp_path)) + members_dir = tmp_path / "members" + members_dir.mkdir() + member_paths = [] + for i in range(3): + p = members_dir / f"m_{i + 1}.bin" + p.write_bytes(bytes((k * (i + 1)) % 251 for k in range(1, 65))) + member_paths.append(str(p)) + + seed = ndi_document( + "demoNDISeries", + **{ + "base.name": f"series_doc_{uuid.uuid4().hex[:8]}", + "demoNDISeries.value": 1, + }, + ) + did_doc = DIDDocument(seed.document_properties) + did_doc.add_file_series(SERIES_NAME, member_paths) + doc = ndi_document(did_doc.document_properties) + + local.database_add(doc) + + # Pull the manifest's uid off the doc so we can prove the cache + # lookup is what recovered it (rather than an unnoticed leftover + # at the tempfile path). + fi = doc.document_properties["files"]["file_info"] + if isinstance(fi, dict): + fi = [fi] + entry = next(e for e in fi if e.get("name") == SERIES_NAME) + locs = entry.get("locations") + if isinstance(locs, dict): + locs = [locs] + manifest_uid = locs[0]["uid"] + recorded_path = locs[0]["location"] + + # The recorded location must be gone -- that IS the shape this + # test exists to prove the fallback survives. + assert not os.path.exists(recorded_path), ( + "fixture: DID ingest should have removed the manifest tempfile; " + "if this fires, the failure mode this test targets no longer " + "reproduces and the test is testing something else." + ) + + roots = _did_file_store_roots(local) + assert roots, "the local dataset should expose its DID FileDir as a root" + cached_path = Path(roots[0]) / manifest_uid + assert cached_path.is_file(), "DID should have copied the manifest into its FileDir" + + pairs = file_uploads_for_document(doc.document_properties, additional_roots=roots) + + # Exactly one pair, and it names the cached path -- so a live + # ``uploadFilesForDatasetDocuments`` call would send the manifest + # bytes and the server would have a record of that uid on the + # dataset. Without this pair, the download side sees "manifest + # not a file of this dataset" and every member reads as absent. + assert (manifest_uid, str(cached_path)) in pairs, ( + f"the manifest uid must be uploaded from DID's file cache; " f"pairs were {pairs!r}" + ) + + def test_a_stand_in_dataset_still_uploads_what_it_can(self, tmp_path): + """``_did_file_store_roots`` reads defensively. + + A caller passing an object that is not a real ``ndi_dataset`` + should not stop the upload of a doc whose recorded location is + still on disk. Recorded elsewhere in this file that a live + recorded location is preferred; here we prove the roots resolver + does not raise on the way to that call. + """ + assert _did_file_store_roots(None) == [] + assert _did_file_store_roots(object()) == []