Skip to content

uploadDataset silently skips a series manifest's bytes (uploaded doc references a nonexistent tempfile after DID ingests) #306

Description

@stevevanhooser

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.
  • SyncFiles=false: my Rebuild series ingest_locations on SyncFiles=false download (companion to VH-Lab/NDI-matlab#988) #302 rebuild path tries to fetch the manifest through the customFileHandler; the batch signed-URL scope returns a 409 that names the exact server-side complaint. Silently skips, ingest_locations stays 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):

  1. file_uploads_for_document returns a (uid, path) pair when the recorded location is gone but the file is at <additional_roots[0]>/<uid>.
  2. Same for a MATLAB-shaped file_info (one-element locations as a bare dict).
  3. 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).
  4. 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).
  5. 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.

References

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions