Fix order-dependent slug collisions in deployment schedule replaces handling#20669
Fix order-dependent slug collisions in deployment schedule replaces handling#20669chrisguidry wants to merge 4 commits intomainfrom
replaces handling#20669Conversation
… handling When updating deployment schedules with `replaces`, the server was patching schedules one at a time in list order. Chain renames like `a -> b` and `b -> c` would hit the unique index on `(deployment_id, slug)` because `b` still existed when `a -> b` was applied. Slug swaps (`x -> y`, `y -> x`) had the same problem. The previous workaround rejected chained replacements with a 422, but that was unnecessarily restrictive. The fix uses a two-phase approach: first NULL out the slugs being replaced (NULLs are distinct in the unique index), then apply all updates by row ID. This handles chains, swaps, and simple renames uniformly. We addressed the same gap in Prefect Cloud (PrefectHQ/nebula#11081). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chrisguidry
requested review from
aaazzam,
cicdw,
desertaxle,
devinvillarosa and
zzstoatzz
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5188593a4e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| slugs_being_replaced = set(replaces_targets.keys()) | ||
| for schedule in deployment.schedules: | ||
| if ( | ||
| schedule.slug | ||
| and schedule.slug in current_slugs | ||
| and schedule.slug not in slugs_being_replaced |
There was a problem hiding this comment.
The new collision guard allows any rename whose destination slug is also in slugs_being_replaced, but this admits non-bijective chains like a -> b and b -> b that cannot satisfy the unique (deployment_id, slug) constraint. In update_deployment, both updates pass pre-validation, phase 1 nulls both rows, and phase 2 raises an IntegrityError on the second update (surfacing as a generic 409) instead of returning a deterministic 422 validation error. This regression was introduced when chained replacements stopped being rejected wholesale, so the replacement graph now needs an explicit uniqueness check on final destination slugs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Considering now we're on an edge case to the edge case, I think we're okay here
There was a problem hiding this comment.
Also to me the 409 is the correct response code if that happens
There was a problem hiding this comment.
Reverting the changes to this file should fix the CI failures on this PR
There was a problem hiding this comment.
😡 thank you, definitely not intended
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This PR should only touch Python files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
When updating deployment schedules with
replaces, the server was patchingschedules one at a time in list order. Chain renames like
a -> bandb -> cwould hit the unique index on
(deployment_id, slug)becausebstill existedwhen
a -> bwas applied. Slug swaps (x -> y,y -> x) had the same problem.The previous workaround rejected chained replacements with a 422, but that's
unnecessarily restrictive. The fix uses a two-phase approach: first NULL out the
slugs being replaced (NULLs are distinct in the unique index), then apply all
updates by row ID. This handles chains, swaps, and simple renames uniformly.
Aligns the OSS server with the behavior we've shipped on Prefect Cloud
and supports these more complex rename patterns that
the
replacesfeature was meant to handle.Session context
The
replacesfeature was initially added with a workaround that rejectedchained replacements entirely. We hit this in Cloud and fixed it there first
with a two-phase NULL-then-update approach that sidesteps the unique index
constraint. This ports the same fix to OSS, replacing the rejection with proper
handling. Both chain renames and slug swaps now work as expected.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com