Skip to content

fix(migrations): reset failed 0024 migration DEV-2094#7056

Merged
rgraber merged 2 commits into
release/2.026.12from
beccagraber/reset-lrm-0024
May 14, 2026
Merged

fix(migrations): reset failed 0024 migration DEV-2094#7056
rgraber merged 2 commits into
release/2.026.12from
beccagraber/reset-lrm-0024

Conversation

@rgraber
Copy link
Copy Markdown
Contributor

@rgraber rgraber commented May 14, 2026

🗒️ Checklist

  1. run linter locally
  2. update developer docs (API, README, inline, etc.), if any
  3. for user-facing doc changes create a Zulip thread at #Support Docs Updates, if any
  4. draft PR with a title <type>(<scope>)<!>: <title> DEV-1234
  5. assign yourself, tag PR: at least Front end and/or Back end or workflow
  6. fill in the template below and delete template comments
  7. review thyself: read the diff and repro the preview as written
  8. open PR & confirm that CI passes & request reviewers, if needed
  9. delete this section before merging

👷 Description for instance maintainers

Force servers to rerun migration 0024 if it failed due to the bug addressed in #7051

@rgraber rgraber requested review from jnm and noliveleger as code owners May 14, 2026 12:59
@rgraber rgraber self-assigned this May 14, 2026
@rgraber rgraber removed request for jnm and noliveleger May 14, 2026 12:59
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 14, 2026

Greptile Summary

This PR adds a new Django data migration (0025) that resets LongRunningMigration record 0024_migrate_submission_supplement_qpaths from failed back to created so servers can automatically retry it, following a bug fix in PR #7051. The error message is preserved (prefixed with 'previous error: ') so the failure history is not lost.

  • Fetches the named LRM record and resets its status to 'created' only if it is currently 'failed', leaving records in any other state untouched.
  • Preserves the failure message by prefixing it with 'previous error: ', addressing reviewer feedback from the prior thread.
  • Uses a noop reverse function, which is correct since rolling back this migration would leave the record in a confusing state.

Confidence Score: 5/5

Safe to merge — the migration is narrow, idempotent for non-failed records, and handles the DoesNotExist case cleanly.

The change touches only one database row under a specific condition (status == 'failed'). Records in any other state are left untouched, and a missing record is handled with a no-op. The reverse function intentionally does nothing, which is appropriate since re-failing a reset record would be confusing. No schema changes, no broad queries, no risk of data loss.

No files require special attention.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Migration 0025 runs] --> B[Fetch LRM record named 0024]
    B --> C{Record exists?}
    C -- No --> D[DoesNotExist caught - skip]
    C -- Yes --> E{status is failed?}
    E -- No --> F[No-op - leave record alone]
    E -- Yes --> G[Set status to created]
    G --> H[Prefix error with previous error text]
    H --> I[Save record]
    I --> J[0024 will be retried on next run]
Loading

Reviews (2): Last reviewed commit: "fixup!: clarify previous error" | Re-trigger Greptile

Comment on lines +14 to +16
if lrm_0024.status == 'failed':
lrm_0024.status = 'created'
lrm_0024.save()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 When resetting the migration to 'created', the error field retains the old failure message, so the record will briefly show an error even though it's in a clean "ready to retry" state. Scoping the save to update_fields (as execute() does elsewhere in the model) and clearing error at reset time makes the record's state unambiguous for any tooling or admin page that reads it.

Suggested change
if lrm_0024.status == 'failed':
lrm_0024.status = 'created'
lrm_0024.save()
if lrm_0024.status == 'failed':
lrm_0024.status = 'created'
lrm_0024.error = None
lrm_0024.save(update_fields=['status', 'error', 'date_modified'])

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should actually keep the error around so we know that this was a rerun. It might be a little ambiguous but I think it's better than losing that data.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with you but maybe we can alter the error message with lrm_0024.error = f'previous error: {lrm_0024.error}'.

Not required, just an idea.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me. Updating

@@ -0,0 +1,35 @@
# Generated by Django 4.2.28 on 2026-05-14 12:47
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 PR description doesn't follow the repository template

The checklist section in the PR description includes the instruction "delete this section before merging" and all items remain unchecked. The 📣 Summary, 📖 Description, and 💭 Notes sections still show TODO or are absent without being explicitly deleted. Per the repository's PR template, the checklist block must be removed before merge, and any inapplicable sections should be explicitly deleted (rather than left as TODO).

Rule Used: What: PR descriptions must follow the repository's... (source)

Copy link
Copy Markdown
Contributor

@noliveleger noliveleger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Look at my comment and adapt it if you want.

@rgraber rgraber merged commit 8173f32 into release/2.026.12 May 14, 2026
17 checks passed
@rgraber rgraber deleted the beccagraber/reset-lrm-0024 branch May 14, 2026 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants