You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
uploadDataset silently skips a series manifest's bytes (uploaded doc references a nonexistent tempfile after DID ingests) #306
Surfaced by the round-trip test PR #305 added for #304, in scheduled cloud run 34924762494 against TEST_USER_1. Both test_members_survive_a_download_from_the_cloud and ..._with_sync_files_false fail on the download side with:
Refusing to add document "...": series "chunkdata.bin" declares 4 members but records no ingest_locations.
Downstream of a real upload-side bug the round-trip test caught for the first time.
What is going wrong
The captured cloud-side signal in the failed run:
CloudAPIError: HTTP 409 Conflict - Manifest for file series "chunkdata.bin" (uid <manifest_uid>) is not a file of this dataset
No file details for <manifest_uid> in dataset <cloud_id>: Not found (HTTP 404): {"message":"File not found"}
Cannot fetch manifest for series "chunkdata.bin" (uid <manifest_uid>): Not found (HTTP 404)
Local file does not exist for uid <manifest_uid> at .../download/files/<manifest_uid>
The server has no record of the manifest uid on the freshly uploaded dataset. Neither reconstruction path can succeed:
SyncFiles=true: downloadDatasetFiles 404s on the manifest uid, so staging/<manifest_uid> never gets written, and updateFileInfoForLocalFiles -> reconstructSeriesIngestLocations silently skips (not os.path.isfile(manifest_path)), leaving ingest_locations empty.
DID's MembersNotLocatable guard then refuses the doc on add_docs. The bytes were never on the cloud; they never had a chance.
Root cause
ndi.cloud.upload.file_uploads_for_document(doc) walks files.file_info[].locations[] and yields (uid, local_path) pairs to upload, requiring os.path.exists(candidate) on the recorded location.
did.document.Document.add_file_series(name, member_paths) writes the manifest to tempfile.NamedTemporaryFile(prefix="did_manifest_", suffix=".manifest", delete=False), and calls add_file(name, manifest_path) — which stores manifest_path as the location and, for a file-type location, defaults delete_original=True. database_add runs the ingest, DID copies the manifest into <FileDir>/<uid> and deletes the source. The file_info.locations[0].location still holds the (now-gone) tempfile path.
By the time uploadFilesForDatasetDocuments runs, that tempfile is gone. file_uploads_for_document returns no pair for the manifest slot, the upload silently skips it, and only the metadata reaches the cloud. The next download 404s.
Reproduced locally:
=== BEFORE database_add ===
name: chunkdata.bin
uid: 41269720e0a8f771_...
location: /tmp/did_manifest_kgwlwr83.manifest
exists: True
=== AFTER database_add ===
location: /tmp/did_manifest_kgwlwr83.manifest
exists: False <-- gone
=== check DID FileDir ===
cached file: 41269720e0a8f771_... 189 bytes <-- bytes still on disk under the uid
This is also a latent bug for any ordinary file where the user-provided source path is later moved or deleted before the upload: the ingest already copied the bytes into DID's file store, but the upload can't find them again.
What to fix
Teach the upload path to fall back to DID's file cache when the recorded location no longer resolves. The bytes are keyed by uid inside <FileDir>/<uid> (and DID's global filecachepath), and did.file.cached_path_for_uid(uid, additional_roots=[...]) already does exactly this lookup for the read path.
Extend file_uploads_for_document(doc, additional_roots=None): after the recorded-location probe, if the location's file does not exist, look the uid up in the given roots via cached_path_for_uid.
Extend uploadFilesForDatasetDocuments(..., additional_roots=None) to forward it.
Callers (orchestration.uploadDataset and sync.operations._upload_binaries) resolve the local dataset's binary_path — DID's FileDir — and pass it in.
Not proposed: changing DID's add_file_series to use a persistent path instead of a tempfile. That would change DID semantics for a symptom this fix already covers.
Not proposed: rewriting file_info.locations[].location to the ingested path on the way through DID. That is a broader change with more ripples.
Regression tests
Offline unit tests in tests/test_cloud_upload_files_from_did_cache.py (or similar):
file_uploads_for_document returns a (uid, path) pair when the recorded location is gone but the file is at <additional_roots[0]>/<uid>.
Same for a MATLAB-shaped file_info (one-element locations as a bare dict).
If neither the recorded location nor the cached one exists, no pair is yielded (nothing to upload; the caller records the doc as failed_document_ids as it does today).
If the recorded location DOES exist, it is preferred and the cache is not consulted (test data is separable, so preferring the recorded path avoids reading from DID's cache when the source is still there).
Cross-checked against a real database_add of a demoNDISeries doc: after add, file_uploads_for_document(doc, additional_roots=[binary_path]) yields the manifest's (uid, cached_path).
The end-to-end coverage is PR #305's live-cloud test — which will pass once this fix lands.
Surfaced by the round-trip test PR #305 added for #304, in scheduled cloud run 34924762494 against TEST_USER_1. Both
test_members_survive_a_download_from_the_cloudand..._with_sync_files_falsefail on the download side with:Downstream of a real upload-side bug the round-trip test caught for the first time.
What is going wrong
The captured cloud-side signal in the failed run:
The server has no record of the manifest uid on the freshly uploaded dataset. Neither reconstruction path can succeed:
downloadDatasetFiles404s on the manifest uid, sostaging/<manifest_uid>never gets written, andupdateFileInfoForLocalFiles->reconstructSeriesIngestLocationssilently skips (not os.path.isfile(manifest_path)), leavingingest_locationsempty.ingest_locationsstays empty.DID's
MembersNotLocatableguard then refuses the doc onadd_docs. The bytes were never on the cloud; they never had a chance.Root cause
ndi.cloud.upload.file_uploads_for_document(doc)walksfiles.file_info[].locations[]and yields(uid, local_path)pairs to upload, requiringos.path.exists(candidate)on the recordedlocation.did.document.Document.add_file_series(name, member_paths)writes the manifest totempfile.NamedTemporaryFile(prefix="did_manifest_", suffix=".manifest", delete=False), and callsadd_file(name, manifest_path)— which storesmanifest_pathas the location and, for afile-type location, defaultsdelete_original=True.database_addruns the ingest, DID copies the manifest into<FileDir>/<uid>and deletes the source. Thefile_info.locations[0].locationstill holds the (now-gone) tempfile path.By the time
uploadFilesForDatasetDocumentsruns, that tempfile is gone.file_uploads_for_documentreturns no pair for the manifest slot, the upload silently skips it, and only the metadata reaches the cloud. The next download 404s.Reproduced locally:
This is also a latent bug for any ordinary file where the user-provided source path is later moved or deleted before the upload: the ingest already copied the bytes into DID's file store, but the upload can't find them again.
What to fix
Teach the upload path to fall back to DID's file cache when the recorded
locationno longer resolves. The bytes are keyed by uid inside<FileDir>/<uid>(and DID's globalfilecachepath), anddid.file.cached_path_for_uid(uid, additional_roots=[...])already does exactly this lookup for the read path.file_uploads_for_document(doc, additional_roots=None): after the recorded-location probe, if the location's file does not exist, look the uid up in the given roots viacached_path_for_uid.uploadFilesForDatasetDocuments(..., additional_roots=None)to forward it.orchestration.uploadDatasetandsync.operations._upload_binaries) resolve the local dataset'sbinary_path— DID'sFileDir— and pass it in.Not proposed: changing DID's
add_file_seriesto use a persistent path instead of a tempfile. That would change DID semantics for a symptom this fix already covers.Not proposed: rewriting
file_info.locations[].locationto the ingested path on the way through DID. That is a broader change with more ripples.Regression tests
Offline unit tests in
tests/test_cloud_upload_files_from_did_cache.py(or similar):file_uploads_for_documentreturns a(uid, path)pair when the recorded location is gone but the file is at<additional_roots[0]>/<uid>.locationsas a bare dict).failed_document_idsas it does today).database_addof ademoNDISeriesdoc: after add,file_uploads_for_document(doc, additional_roots=[binary_path])yields the manifest's(uid, cached_path).The end-to-end coverage is PR #305's live-cloud test — which will pass once this fix lands.
References