Reconcile S3 manifests with DB + fix publish-time DOI race - #2862
Draft
yarikoptic wants to merge 4 commits into
Draft
Reconcile S3 manifests with DB + fix publish-time DOI race#2862yarikoptic wants to merge 4 commits into
yarikoptic wants to merge 4 commits into
Conversation
Summarize the issue raised in #2759 (missing DOIs / drifted manifests on S3), the root-cause analysis of the publish-time race between write_manifest_files and DOI minting, the design of the new sync_manifest_files management command, and the testing plan. Refs: #2759 Co-Authored-By: Claude Code 2.1.146 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reconcile S3 manifest files (dandiset.jsonld / assets.jsonld) against the
database. For each selected Version the command parses the on-S3 file,
compares it to what would be rendered from current Version.metadata /
Version.assets, and if they differ (or the S3 object is missing) triggers
write_manifest_files to rewrite all five manifest files. By default the
regeneration is enqueued via Celery (--async); use --sync for inline runs.
Optionally (on by default, --no-fix-doi to disable) re-mint DOIs for
published versions whose Version.doi is NULL or equals the publish-time
placeholder (...123456/0.123456.1234). DOI minting is synchronous so
operator-visible failures are reported, not silently swallowed.
Surface:
./manage.py sync_manifest_files [DANDISET_ID ...] [-a/--all]
[--version draft|published|all]
[--specific-version VER]
[--targets dandiset|assets|both]
[--dry-run/--check]
[--sync/--async]
[--fix-doi/--no-fix-doi]
[--show-diff]
Comparison is semantic and defensive: both sides flow through
JSONRenderer + json.loads (via the _renormalize helper) so future DRF
type coercions (Decimal, UUID, datetime) don't trip one-sided
mismatches.
If doi.create_doi raises mid-call, version.refresh_from_db() rolls back
the in-memory metadata['doi'] mutation _generate_doi_data leaves behind,
so the subsequent S3 compare doesn't see a polluted version.
Refs: #2759
Co-Authored-By: Claude Code 2.1.146 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cover the matrix from doc/design/manifests-consistency.md against the existing pytest harness (MinIO via default_storage, eager Celery, DOI creation monkeypatched because to_datacite() runs PublishedDandiset pydantic validation that faker-generated factory metadata cannot satisfy): Unit: - _is_dummy_doi truth table (placeholder vs. real vs. None) Integration (MinIO + Django DB + eager Celery): - in-sync: command is a no-op, summary reports 0 mismatches - drift in dandiset.jsonld is detected and repaired - --dry-run does not write but reports "would regenerate" - missing S3 object is treated as a mismatch - --targets=assets compares assets.jsonld and regenerates on drift - --targets=both regenerates if either side differs - DOI remint fills NULL doi on published versions (monkeypatched create_doi mirrors the real function's metadata['doi'] side effect) - DOI remint replaces placeholder doi - --no-fix-doi skips DOI repair - DOI minting failure is logged, not fatal; version.doi stays NULL - --version=published filter skips draft versions - --version=draft filter skips published versions - --specific-version overrides --version filter - positional dandiset filter only touches that dandiset - --all happy path processes every dandiset, only stale ones regenerate - --show-diff prints a unified diff with the expected headers - mutually-exclusive arg validation raises ClickException - embargoed manifest retains its embargoed tag after regeneration Follows the existing per-command pattern (test_create_dev_dandiset.py, test_correct_metadata.py): import the click-decorated function and call directly with argv-style positional args. Refs: #2759 Co-Authored-By: Claude Code 2.1.146 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
_publish_dandiset previously scheduled two independent transaction.on_commit callbacks: transaction.on_commit(lambda: write_manifest_files.delay(new_version.id)) transaction.on_commit(lambda: _create_doi(new_version.id)) The manifest task was enqueued to Celery and could run on a worker before _create_doi finished its synchronous DataCite request and version.save(), so dandiset.jsonld got written from a Version whose metadata still lacked the freshly-minted DOI (and whose citation therefore used the archive URL rather than the DOI URL). Nothing re-ran write_manifest_files after the DOI was saved, so the stale manifest persisted indefinitely. This is the root cause of the "DOI missing from S3 manifests" symptom reported in #2759. Replace the two callbacks with a single chained one that mints the DOI, saves it (Version.save() repopulates metadata['doi'] and the DOI-based citation via _populate_metadata), and then enqueues write_manifest_files. The two substeps live in separate try blocks with distinct log messages so it is clear whether the DOI was minted-but-not-saved or never minted at all. Neither failure aborts the manifest write — even a DOI-less published version is better represented by an on-S3 manifest than by nothing; sync_manifest_files --fix-doi can later remint the DOI and rewrite the manifest. Refs: #2759 Co-Authored-By: Claude Code 2.1.146 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
yarikoptic
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sync_manifest_filesmanagement command to reconcile S3 manifest files (dandiset.jsonld/assets.jsonld) against the current database state, regenerate viawrite_manifest_fileson mismatch, and (by default) re-mint NULL/placeholder DOIs on published versions.dandiset.jsonldfiles to be written to S3 before the DOI was minted and saved, leaving them withdoi: nulland a non-DOI citation forever.doc/design/manifests-consistency.md.The four commits are independently reviewable:
doc:add design notesync_manifest_filesmanagement commandsync_manifest_filesRoot cause (race)
dandiapi/api/services/publish/__init__.pyregistered two independenttransaction.on_commitcallbacks — one towrite_manifest_files.delay()and one to mint the DOI synchronously. The manifest task could pick up before the DOI was committed, and nothing re-ran the manifest task afterversion.doiwas saved.The fix chains them so the DOI is minted and saved before the manifest write is enqueued. A DataCite failure no longer leaves the published version without an S3 manifest — the exception is logged and the manifest is still enqueued;
sync_manifest_files --fix-doican repair later.sync_manifest_filessurfaceComparison is semantic (parsed JSON, list of dicts for
assets.jsonld) so it doesn't trip on whitespace / key ordering.Suggested operational sweep (post-deploy)
Test plan
tox -e lintpasses on the new files.docker compose up -d minio postgres rabbitmq && tox -e test -- dandiapi/api/tests/test_sync_manifest_files.py dandiapi/api/tests/test_tasks.pyis green locally; the latter is included to verify the publish fix doesn't breaktest_publish_task.sync_manifest_files --all --version published --dry-run --show-diffagainst staging and review the diff before applying.