Repository navigation
fix(registry): make artifact registration idempotent - #9
Merged
Merged
Conversation
Artifact identity is the canonical output path — `Store::insert_artifact`
derives `id = sha256(canonical_path)[..16]`, and migration 0001 states the
invariant outright ("path collisions are impossible by construction") and
backs it with `artifacts_path_unique`. Re-producing a path is therefore the
same artifact, not a new one.
`PgStore::insert_artifact` was nonetheless a plain INSERT, the only writer in
this module that doesn't handle a conflict (runs, aliases, users, evals and
tracking rows all upsert). Any recipe whose output path isn't run-scoped
raises `artifacts_pkey` on its second run, and the artifact can then never be
registered.
In the CLI that error also aborted the whole reconcile pass, so every run
behind the offending one was stranded — status frozen at `submitted`, outputs
unregistered, nothing visible in the UI. Observed on a live registry: a June
run of a recipe writing to `datasets/<user>/<recipe_name>` wedged that user's
reconcile for two months; 14 runs and 12 artifacts landed once unblocked.
Two changes:
- `insert_artifact` upserts on conflict, refreshing the columns that describe
the latest production (kind, producer_run_id, metadata_json). `path`,
`user` and `created_at` are identity/first-seen and are left alone.
- `runner::reconcile` isolates per-run failures behind a log line and a
`runs_failed` counter, the way `agent.rs`'s dispatch loop already does, so
one unreconcilable run can't strand the rest.
Covered by a PG-backed regression test: registering the same path twice must
succeed and refresh the mutable columns. It reproduces the exact production
error (`duplicate key value violates unique constraint "artifacts_pkey"`)
when the upsert is reverted.
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
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.
What was wrong
Artifact identity is the canonical output path.
Store::insert_artifactderivesid = sha256(canonical_path)[..16], and migration 0001 states the invariant outright — "Identity is path-canonical … so path collisions are impossible by construction" — backing it withartifacts_path_unique. Re-producing a path is therefore the same artifact, not a new one.PgStore::insert_artifactwas nonetheless a plainINSERT, the only writer in that module without conflict handling (runs, aliases, users, evals and tracking rows all upsert). So any recipe whose output path isn't run-scoped raisesartifacts_pkeyon its second run, and that artifact can never be registered.In the CLI the error was worse than a skipped artifact:
runner::reconcileused?onreconcile_one, so one bad run aborted the entire pass. Every run behind it was stranded — status frozen atsubmitted, outputs unregistered, nothing in the UI.Found on the live berlin registry. A June run of a recipe writing to
datasets/<user>/<recipe_name>(no{run.id}in the path) had wedged my reconcile for two months. Recipes that embed{run.id}in the alias never hit it, which is why it went unnoticed.The fix
insert_artifactupserts on conflict, refreshing the columns that describe the latest production (kind,producer_run_id,metadata_json).path,userandcreated_atare identity / first-seen and are left alone.runner::reconcileisolates per-run failures behind a log line and a newruns_failedcounter — exactly whatagent.rs's dispatch loop already does. One unreconcilable run can no longer strand the rest. The counter serialises into the CLI's JSON output for free.Verification
New PG-backed regression test
insert_artifact_is_idempotent_on_repeat_registration, in the style of the existing#[ignore = "requires running PG"]tests: register the same path under two different producer runs, assert the second succeeds and thatproducer_run_id/metadata_jsonare refreshed whilecreated_atsurvives.duplicate key value violates unique constraint "artifacts_pkey".cargo test --all-features: 9 passed, 0 failed.Against the live registry, the previously-wedged reconcile then completed cleanly:
and a run that had been stuck at
submittedsince aCOMPLETEDSLURM job resolved tosucceededwith itseval_resultartifact and alias registered.Note for reviewers
This is the mechanism behind "my runs never show up in the UI". Worth knowing that a user whose registry is already wedged needs one
labctl reconcilewith this build to catch up — the backlog isn't lost, just unprocessed.