Harden backfill_original_filenames against per-row failures (#1391)#1402
Merged
Conversation
The backfill runs on every container start over the whole artifact dataset. It had no per-row error isolation, so a single malformed row would propagate its exception out of the command and abort the entire run — and since docker-entrypoint.sh has no `set -e`, that would fail silently (traceback prints, startup continues to runserver), leaving every row's original_*_filename unset. The most likely trigger is a null `date` or `title`, which makes Artifact.generate_filename() raise (date.year / title.title()). This is defensive insurance, not a fix for an observed failure — the 2.25.0 prod backfill did populate the legacy rows correctly. - Process each (artifact, file field) pair inside its own try/except; log and count the row, then continue. Extract the row logic into _backfill_row(). - Surface an errored-row count in the summary log line. - Regression test: a row with a null date (generate_filename raises) no longer aborts the batch; a good legacy row in the same run is still backfilled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Follow-up hardening for #1391 (no observed prod failure — defensive insurance).
Why
backfill_original_filenamesruns on every container start over the whole artifact dataset, but had no per-row error isolation. A single malformed row would propagate its exception out of the command and abort the entire run — and becausedocker-entrypoint.shhas noset -e, that fails silently (the traceback prints, but startup continues torunserver), leaving every row'soriginal_*_filenameunset. The most likely trigger is a nulldateortitle, which makesArtifact.generate_filename()raise (date.year/title.title()).To be clear: the 2.25.0 prod backfill worked — legacy rows (e.g. the Mauriello talk) are populated correctly. This just makes a deploy-time batch job robust against future bad data.
Changes
(artifact, file field)pair is processed inside its owntry/except; on error it logs (with_logger.exception) and counts the row, then continues. Row logic extracted into_backfill_row().test_one_bad_row_does_not_abort_the_batch): a row with a nulldate(sogenerate_filenameraises) no longer aborts the run, and a good legacy row in the same batch is still backfilled.Tests
Full suite green locally: 552 tests, OK (8 skipped).
Note
The underlying fragility is in
get_filename_without_ext_for_artifact(no None-guard ondate/title), which the rename path also calls. Hardening that shared helper is better scoped to #1401 (the legacy re-rename work); this PR isolates the batch backfill instead.🤖 Generated with Claude Code