Delete stale CloudFront redirect KVS entries#3409
Conversation
Operand order on `Except` in the redirect KVS sync was flipped, so the delete batch resolved to "keys we are about to PUT" instead of "keys already in the KVS that no longer appear in the new redirects file". Result: nothing has ever been deleted from the prod/staging/preview redirect KVS since the regression landed, and stale redirects stick forever (e.g. elastic/docs-content#6716 could not undo the broken azure-native-isv-service redirect via redirects.yml). Fix the operand order, lift the diff into a pure helper so we can unit-test it without mocking the AWS CLI, and add a wipe guard that refuses to deploy an empty redirects.json against a populated KVS.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR refactors CloudFront KeyValueStore redirect deployment by extracting batch diffing logic into a new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
I feel like we should get a list of redirects that will be deleted, and check if these URLs are in use. Can you do an analysis of that? |
Mpdreamz
left a comment
Mpdreamz
left a comment
There was a problem hiding this comment.
LGTM. The operand fix is clearly correct, the wipe guard adds a solid safety layer, and the test suite thoroughly pins the regression.
One minor follow-up worth tracking: ComputeBatchUpdates currently re-puts every sourced entry on every deploy, even entries whose value hasn't changed. This burns unnecessary CloudFront KVS API quota on large redirect sets. A future optimisation could skip the no-op puts by filtering sourcedRedirects down to entries where the key is absent from existingRedirects or the value differs. The ComputeBatchUpdates_KeyInBoth_IsPutAndNotDeleted test already pins this behavior so any change there would be caught.
3 participants
Why
The redirect KVS sync has had a one-line bug since #1503 (July 2 2025) that prevented any redirect from ever being deleted from the prod/staging/preview/edge CloudFront KeyValueStores. Operand order on
Exceptwas flipped so the delete batch resolved to "keys we are about to PUT" (always a no-op) instead of "keys already in the KVS that no longer appear in the new redirects file".This surfaced as a writer-visible bug today: elastic/docs-content#5818 added a bad redirect for
/.../azure-native-isv-service, and two follow-up content PRs (#6667, #6716) could not undo it viaredirects.yml. Every redirect ever pushed since the regression has accumulated; the prod KVS had 806 keys at the time of investigation.The bad Azure ISV entry was manually purged from the prod KVS to unblock the writer, but the underlying bug needs fixing so future removals from
redirects.ymlactually propagate to CloudFront.What
RedirectKvsDiff, a pure static helper with no AWS dependencies, and swap theExceptoperands so stale KVS keys are flagged for deletion.WouldWipeAllExisting): refuse to push an emptyredirects.jsonagainst a populated KVS. Before this fix the bug masked this risk; after the fix an empty sourced file would wipe every redirect, which is almost certainly a broken build rather than intent.RedirectKvsDiffTests— includingComputeBatchUpdates_AzureIsvRegression_ScenarioEndToEnd, which pins the exact "stale key in KVS, dropped from sourced file, must end up intoDelete" semantics that broke here.After this lands, the next prod deploy will also clean up other stale entries that have accumulated since the regression. A read-only audit of those is being shared separately.