feat(api): Add command to remove identity overrides that no longer exist - #8557
matthewelwell wants to merge 1 commit into
Conversation
Identity overrides live in two places: `identity_features` on the identity document, and one document per (feature, identity) in `environments_v2`. A write landing between another request's read and its write is invisible to that request's changeset, so the `environments_v2` document survives the identity being deleted. Once the identity is gone, or its uuid has changed, nothing can reach the stale document — it is keyed on an identity uuid that no longer resolves. `delete_orphaned_identity_overrides` reconciles an environment against its identity documents, which are the source of truth for both remote evaluation and the identity page. Deletes are conditional on `identity_uuid` so an override recreated between being read and being deleted is left alone, rather than the repair carrying the same race as the bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
github-actions
Bot
added
api
Docker builds report
|
📝 WalkthroughWalkthroughThe change adds orphaned identity-override data types and service functions. DynamoDB access now supports batched identity reads, cached resources, and conditional deletion. A management command reconciles overrides for an environment, supports dry runs, reports orphan reasons, and records skipped deletions. Unit tests cover deletion, classification, dry-run behaviour, missing environments, and concurrent changes. One observability documentation line is updated. Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟠 High · up to The command can delete valid overrides during stale reads or concurrent recreation, and the resource-caching change risks request failures in threaded production deployments. These issues should be resolved before merge. 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 |
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20519 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #20519 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20519 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20519 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8557 +/- ##
==========================================
- Coverage 98.82% 98.81% -0.01%
==========================================
Files 1622 1623 +1
Lines 66727 66415 -312
==========================================
- Hits 65942 65629 -313
- Misses 785 786 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: ed3cb32f-bc04-4292-a628-1db20aa3aebd
📒 Files selected for processing (10)
api/edge_api/identities/dataclasses.pyapi/edge_api/identities/edge_identity_service.pyapi/edge_api/management/commands/delete_orphaned_identity_overrides.pyapi/environments/dynamodb/constants.pyapi/environments/dynamodb/wrappers/base.pyapi/environments/dynamodb/wrappers/environment_wrapper.pyapi/environments/dynamodb/wrappers/identity_wrapper.pyapi/tests/unit/edge_api/test_unit_delete_orphaned_identity_overrides.pyapi/util/util.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.md
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| if not self._resource: | ||
| self._resource = self.get_resource() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
sed -n '20,65p' api/environments/dynamodb/wrappers/base.py
rg -n 'DynamoIdentityWrapper\(|DynamoEnvironmentV2Wrapper\(|dynamo_wrapper|environment_v2_wrapper|gunicorn|threads|worker_class|celery' api docker* compose* pyproject.toml .github 2>/dev/null | head -250Repository: Flagsmith/flagsmith
Length of output: 33319
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- api/Procfile ---'
cat -n api/Procfile
printf '%s\n' '--- wrapper declarations and relevant request paths ---'
cat -n api/edge_api/identities/models.py | sed -n '1,115p'
cat -n api/edge_api/identities/views.py | sed -n '80,135p'
cat -n api/edge_api/identities/views.py | sed -n '200,225p'
cat -n api/edge_api/identities/views.py | sed -n '330,350p'
cat -n api/environments/models.py | sed -n '65,85p'
cat -n api/edge_api/identities/edge_identity_service.py | sed -n '1,40p'
printf '%s\n' '--- deployment/configuration references ---'
rg -n --glob '!api/uv.lock' --glob '!api/tests/**' 'GUNICORN_(WORKERS|THREADS)|gunicorn|worker_class|threads:|threads[[:space:]]*:|WEB_CONCURRENCY|preload_app|--threads|--workers' . | head -200Repository: Flagsmith/flagsmith
Length of output: 19694
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Dockerfile runtime command and Gunicorn environment ---'
cat -n Dockerfile | sed -n '125,155p'
rg -n -C 4 'GUNICORN_(WORKERS|THREADS)|GUNICORN_CMD_ARGS|CMD|ENTRYPOINT|Procfile' Dockerfile docker-compose.yml docker-compose.*.yml 2>/dev/null || true
printf '%s\n' '--- Fly deployment ---'
cat -n fly.toml | sed -n '1,45p'
printf '%s\n' '--- ECS production Gunicorn environment ---'
cat -n infrastructure/aws/production/ecs-task-definition-admin-api.json | sed -n '98,125p'
cat -n infrastructure/aws/production/ecs-task-definition-sdk-api.json | sed -n '103,130p'
printf '%s\n' '--- repository Gunicorn deployment documentation ---'
cat -n docs/docs/deployment-self-hosting/hosting-guides/docker.md | sed -n '15,32p'
cat -n docs/docs/deployment-self-hosting/scaling-and-performance/sizing-and-scaling.md | sed -n '345,360p'Repository: Flagsmith/flagsmith
Length of output: 10743
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- migrate-and-serve implementation ---'
rg -n -C 8 'migrate-and-serve|migrate_and_serve|GUNICORN_THREADS|GUNICORN_WORKERS' api . --glob '*.py' --glob '*.js' --glob '*.toml' --glob 'Makefile*' | head -240Repository: Flagsmith/flagsmith
Length of output: 735
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- package and build command definitions ---'
rg -n -C 5 'migrate|serve|gunicorn|GUNICORN' api/pyproject.toml pyproject.toml Makefile* api/Makefile* scripts .github Dockerfile 2>/dev/null | head -300
printf '%s\n' '--- likely entrypoint files ---'
fd -HI -t f 'flagsmith|entrypoint|manage|gunicorn' . | head -120Repository: Flagsmith/flagsmith
Length of output: 14462
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Dockerfile build ancestry and install stages ---'
cat -n Dockerfile | sed -n '1,125p'
printf '%s\n' '--- project metadata and scripts ---'
cat -n api/pyproject.toml | sed -n '1,90p'
cat -n pyproject.toml | sed -n '1,100p' 2>/dev/null || trueRepository: Flagsmith/flagsmith
Length of output: 9237
🏁 Script executed:
#!/bin/bash
set -eu
cat -n api/environments/dynamodb/wrappers/base.py | sed -n '1,55p'Repository: Flagsmith/flagsmith
Length of output: 2276
Use a thread-local or request-local DynamoDB resource.
BaseDynamoWrapper.resource caches the boto3.resource("dynamodb", ...) result, and table caches a table derived from it. EdgeIdentity.dynamo_wrapper is a class-level instance used by request views. Each Gunicorn worker therefore shares these objects across its request threads; production ECS configuration uses 15 threads per worker. Boto3 resources are not thread-safe, so concurrent requests can cause unsafe access and request failures.
Use a separate wrapper, resource, and table per thread or request. If you replace the resource with a client, refactor the table operations to use the client API.
| ENVIRONMENTS_V2_PARTITION_KEY: str(environment_id), | ||
| ENVIRONMENTS_V2_SORT_KEY: document_key, | ||
| }, | ||
| ConditionExpression=Attr("identity_uuid").eq(identity_uuid), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Revalidate the exact override instance before deletion.
identity_uuid cannot distinguish a stale override from an override recreated for the same identity. If the identity re-adds the feature before deletion, the condition still succeeds and removes the valid override.
api/environments/dynamodb/wrappers/environment_wrapper.py#L241-L241: condition deletion on an immutable revision or write token captured during classification.api/edge_api/identities/edge_identity_service.py#L130-L134: pass the captured revision or write token to the conditional-delete operation.
Based on learnings: stale cleanup must revalidate both the resource identity and the exact observed record instance before deletion.
📍 Affects 2 files
api/environments/dynamodb/wrappers/environment_wrapper.py#L241-L241(this comment)api/edge_api/identities/edge_identity_service.py#L130-L134
Source: Learnings
| keys_and_attributes: "KeysAndAttributesServiceResourceTypeDef" = { | ||
| "Keys": [{"composite_key": composite_key} for composite_key in chunk] | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use strongly consistent reads before deleting overrides.
BatchGetItem uses eventually consistent reads by default. A recent identity write can therefore appear missing or stale. The reconciliation path then classifies its valid override as orphaned and deletes it. (docs.aws.amazon.com)
Set "ConsistentRead": True in keys_and_attributes.
Proposed fix
keys_and_attributes: "KeysAndAttributesServiceResourceTypeDef" = {
- "Keys": [{"composite_key": composite_key} for composite_key in chunk]
+ "Keys": [{"composite_key": composite_key} for composite_key in chunk],
+ "ConsistentRead": True,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| keys_and_attributes: "KeysAndAttributesServiceResourceTypeDef" = { | |
| "Keys": [{"composite_key": composite_key} for composite_key in chunk] | |
| } | |
| keys_and_attributes: "KeysAndAttributesServiceResourceTypeDef" = { | |
| "Keys": [{"composite_key": composite_key} for composite_key in chunk], | |
| "ConsistentRead": True, | |
| } |
Source: MCP tools
| yield from response["Responses"].get(table_name, []) | ||
| # DynamoDB returns keys it declined to read — e.g. when the | ||
| # response would exceed 16MB — and expects them to be retried. | ||
| request_items = response.get("UnprocessedKeys") # type: ignore[assignment] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add bounded exponential backoff for UnprocessedKeys.
This loop resubmits unprocessed keys immediately and has no attempt limit. Sustained throttling can create a tight request loop and prevent the command from completing. AWS recommends exponential backoff for this response path. (docs.aws.amazon.com)
Apply jittered exponential backoff and stop after a configured attempt limit.
Source: MCP tools
| def iter_chunks( | ||
| iterable: Iterable[T], | ||
| *, | ||
| chunk_size: int, | ||
| ) -> Generator[list[T], None, None]: | ||
| """ | ||
| Iterate over `iterable`, yielding lists of at most `chunk_size` items. | ||
| """ | ||
| iterator = iter(iterable) | ||
| while chunk := list(islice(iterator, chunk_size)): | ||
| yield chunk |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,145p' api/util/util.py
rg -n '\bbatched\b|\biter_chunks\b' api --glob '*.py'Repository: Flagsmith/flagsmith
Length of output: 5029
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- edge_identity_service.py ---'
sed -n '1,105p' api/edge_api/identities/edge_identity_service.py
printf '%s\n' '--- identity_wrapper.py ---'
sed -n '1,125p' api/environments/dynamodb/wrappers/identity_wrapper.py
printf '%s\n' '--- segment_membership/tasks.py ---'
sed -n '20,115p' api/segment_membership/tasks.py
printf '%s\n' '--- util tests around batched ---'
sed -n '115,155p' api/tests/unit/util/test_util.pyRepository: Flagsmith/flagsmith
Length of output: 13646
🤖 get_repo_knowledge executed:
get_repo_knowledge Flagsmith/flagsmith /tmp/coderabbit-repo-knowledge/flagsmith-flagsmith-a74d157a/learnings
Length of output: 6363
Reuse the existing batched helper.
Both helpers lazily yield identical list batches. Update the callers to pass size= because batched does not use the keyword-only chunk_size argument. Keeping both implementations creates two batching paths that can diverge.
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #8397
Identity overrides are held in two places —
identity_featureson the identity document, and one document per (feature, identity) inenvironments_v2— and a write landing between another request's read and its write is invisible to that request's changeset. Theenvironments_v2document then survives the identity being deleted, and once the identity is gone or its uuid has changed nothing can reach it: it is keyed on an identity uuid that no longer resolves, so it cannot be removed through the API or the dashboard. It is still listed on the feature's identity-overrides tab and still served to local-evaluation SDKs.delete_orphaned_identity_overrides --environment-id <id> [--dry-run]reconciles an environment against its identity documents, which are the source of truth for both remote evaluation and the identity page. An override is removed when the identity no longer exists, when the identifier resolves to a different identity, or when the identity no longer overrides that feature. Deletes are conditional onidentity_uuid, so an override recreated between being read and being deleted is left alone rather than the repair carrying the same race as the bug it repairs.--dry-runreports what would go without touching anything.This only repairs data. It does not stop new orphans appearing — that needs the optimistic concurrency work in #8397, and until that ships this wants running periodically rather than once.
Supporting changes:
iter_items_by_composite_keyson the identity wrapper for batched reads,delete_identity_override_if_unchangedon the environments v2 wrapper, aresourceaccessor on the base wrapper (batch_get_itemis not available on aTable), and a genericiter_chunkshelper.How did you test this code?
Unit tests against moto covering each way an override goes stale, that a valid override is untouched, that
--dry-rundeletes nothing, that a recreated override survives the conditional delete, and that an unknown environment fails cleanly. 100% coverage on the new command.Also exercised against a production environment's data read-only, reproducing the audit the command performs and confirming it classifies the same documents as stale.