Skip to content

Delete stale CloudFront redirect KVS entries#3409

Open
cotti wants to merge 2 commits into
mainfrom
fix/redirect-kvs-deletion-inverted
Open

Delete stale CloudFront redirect KVS entries#3409
cotti wants to merge 2 commits into
mainfrom
fix/redirect-kvs-deletion-inverted

Conversation

@cotti
Copy link
Copy Markdown
Contributor

@cotti cotti commented May 27, 2026

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 Except was 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 via redirects.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.yml actually propagate to CloudFront.

What

  • Move the put/delete diff calculation into RedirectKvsDiff, a pure static helper with no AWS dependencies, and swap the Except operands so stale KVS keys are flagged for deletion.
  • Add a wipe guard (WouldWipeAllExisting): refuse to push an empty redirects.json against 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.
  • Cover the regression with RedirectKvsDiffTests — including ComputeBatchUpdates_AzureIsvRegression_ScenarioEndToEnd, which pins the exact "stale key in KVS, dropped from sourced file, must end up in toDelete" semantics that broke here.
  • Log the computed batch sizes at info level so future deploys make the diff visible in CI output.

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.

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.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 27, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 1ab53a07-2e55-48c6-9e41-50491405293e

📥 Commits

Reviewing files that changed from the base of the PR and between fb76f8d and b50f4fa.

📒 Files selected for processing (3)
  • src/services/Elastic.Documentation.Assembler/Deploying/Redirects/AwsCloudFrontKeyValueStoreProxy.cs
  • src/services/Elastic.Documentation.Assembler/Deploying/Redirects/RedirectKvsDiff.cs
  • tests/Elastic.Documentation.Build.Tests/RedirectKvsDiffTests.cs

📝 Walkthrough

Walkthrough

The PR refactors CloudFront KeyValueStore redirect deployment by extracting batch diffing logic into a new RedirectKvsDiff utility class. The utility provides ComputeBatchUpdates, which transforms sourced redirects into PUT requests and computes DELETE requests from keys present in the live KVS but absent from sourced entries. A second helper, WouldWipeAllExisting, guards against accidentally deleting all existing entries when the sourced redirects dictionary is empty. The existing UpdateRedirects method now calls these utilities and explicitly aborts if a dangerous wipe condition is detected. Comprehensive unit tests validate both the diffing and guard behaviors across edge cases and regression scenarios.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and specifically describes the main change: fixing the deletion of stale CloudFront redirect KVS entries, which is the core bug fix in this PR.
Description check ✅ Passed The description provides comprehensive context about the bug, its impact, the fix, and test coverage—all clearly related to the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch fix/redirect-kvs-deletion-inverted

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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.

1 participant