fix(Environments): Environment document can sync to DynamoDB before feature states commit - #8244
Conversation
… feature states The environment document write is driven by the audit log post_save signal, which is enqueued before `create_feature_states` has seeded the initial feature states for a new environment or feature. When the write won the race, edge-api served a document missing those states. For environments using v2 feature versioning the document never recovered on its own: seeded feature states are excluded from the audit log, so no later audit record triggered a corrective write, and the document stayed partial until it was rebuilt by hand. Mark that window with `Environment.is_creating` (extended from clones to standard creation) and a new `Feature.is_creating`, and have `process_environment_update` re-enqueue itself with exponential backoff while either is set, giving up after three attempts so a stuck flag cannot block an environment's updates forever. This replaces the one second `delay_until` shim, which was a timer rather than a guarantee. Both flags are cleared with a direct row update, so their historical records are always stale — keep `is_creating` out of the feature's history, and out of the environment's audit log change details, where a spurious entry was already shown for cloned environments. Partial writes were also invisible: add a `flagsmith_environment_document_writes_total` counter and an `environment_document.written` event carrying the feature state count, so a truncated document can be alerted on rather than reported through support.
|
@bardock-2393 is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe change adds lifecycle flags for environments and features during initial feature-state creation. Task-based environment document updates now defer writes while seeding is active, with exponential backoff and a maximum deferral count. DynamoDB document writes now emit success or failure metrics and structured metadata. Compressed documents include feature-state counts. Tests and observability catalogues cover the new lifecycle, retry, logging, and metric behaviour. Estimated code review effort: 4 (Complex) | ~45 minutes ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
github-actions
Bot
added
api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/core/signals.py (1)
3-4: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWrap the audit-background enqueue in
transaction.on_commit.
post_create_historical_recordruns before the outer transaction commits, sotasks.create_audit_log_from_historical_record.delay(...)atapi/core/signals.py:45can send the task immediately when no transaction is active or beforehistory_instance.history_idis visible. Addtransaction.on_commitaround this call, or otherwise use a commit-after enqueue path, so the worker cannot fail the lookup withObjectDoesNotExistand lose the audit log.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5bbfae29-3dd8-40a0-97f0-0405572ed096
📒 Files selected for processing (17)
api/core/signals.pyapi/environments/constants.pyapi/environments/dynamodb/wrappers/environment_wrapper.pyapi/environments/metrics.pyapi/environments/models.pyapi/environments/tasks.pyapi/features/migrations/0068_add_feature_is_creating.pyapi/features/models.pyapi/tests/unit/environments/dynamodb/wrappers/test_unit_dynamo_environment_wrapper.pyapi/tests/unit/environments/dynamodb/wrappers/test_unit_dynamodb_environment_v2_wrapper.pyapi/tests/unit/environments/test_unit_environments_models.pyapi/tests/unit/environments/test_unit_environments_tasks.pyapi/tests/unit/features/test_unit_features_models.pyapi/util/dataclasses.pyapi/util/mappers/dynamodb.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mddocs/docs/deployment-self-hosting/observability/_metrics-catalogue.md
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8244 +/- ##
==========================================
- Coverage 98.72% 98.59% -0.14%
==========================================
Files 1558 1559 +1
Lines 61948 62078 +130
==========================================
+ Hits 61160 61203 +43
- Misses 788 875 +87 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… is flushed `batch_writer` only buffers documents, flushing them when it fills up and when the context exits, so `put_item` returning says nothing about whether a document was written. Since these writes carry a single environment, or a project's handful, nothing was flushed before the success event was logged. Hold the events until the writer's context has exited, and count the documents a failed batch carried rather than only those already handed over. A failed batch does not report which of its documents were persisted, so the counter's description now says the failure count includes them.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/docs/deployment-self-hosting/observability/_metrics-catalogue.md (1)
49-53: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the exported counter name.
The counter is declared as
flagsmith_environment_document_writes_total, but the metric catalogue documentsflagsmith_environment_document_writes. Use the_totalname or users will query a metric that is not exported.Proposed fix
-### `flagsmith_environment_document_writes` +### `flagsmith_environment_document_writes_total`api/environments/dynamodb/wrappers/environment_wrapper.py (1)
77-127: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftCount failures against documents supplied to the batch writer.
Line 83 increments
attemptedbefore OpenFeature evaluation and document mapping. If either operation raises,flagsmith_environment_document_writes_total.labels(result="failure").inc(attempted)records a document that was not supplied towriter.put_item(). Also, if the second document mapping raises after the first one is buffered,batch_writer.__exit__flushes the first document and can propagate that flush failure; the current handler then labels that earlier document as failed. Incrementattemptedonly after a document is mapped and supplied towriter.put_item(), and record failures only for exceptions fromwriter.put_item()or the flush path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a7d1df10-6b0d-4503-8463-3d72c6a2b9c0
📒 Files selected for processing (5)
api/environments/dynamodb/wrappers/environment_wrapper.pyapi/environments/metrics.pyapi/tests/unit/environments/dynamodb/wrappers/test_unit_dynamo_environment_wrapper.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mddocs/docs/deployment-self-hosting/observability/_metrics-catalogue.md
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #7281
A newly created environment or feature can have its document written to DynamoDB before its initial feature states exist, leaving edge-api serving an environment whose flags are missing. The write is triggered by the audit log, which is enqueued while those feature states are still being created. Until now the only thing standing between the two was a one second delay on the audit log task, which is a timer rather than a guarantee.
For environments using v2 feature versioning, nothing corrects this afterwards. Seeded feature states are deliberately excluded from the audit log, so no later record triggers another write and the document stays incomplete until somebody rebuilds it by hand. That matches the reported incident, which reached us through two customer tickets rather than an alert.
Two decisions worth a reviewer's attention:
delay_until, which only does anything under the task processor. Under the other task run methods the check is skipped entirely and behaviour is unchanged, since re-enqueueing there would silently drop the write.Both
is_creatingflags are cleared with a direct row update and so never appear in a historical record. The feature flag is therefore excluded from the feature's history, and the environment flag from the audit log's change details, where it was already producing a spurious entry for cloned environments.Review effort: 3/5
How did you test this code?
Added unit tests covering the new behaviour, each confirmed to fail without the change:
process_environment_updatedefers the write while an environment or a feature in the project is being created, and re-enqueues itself with a growing delay.Existing tests were updated where they assert on the exact set of log events emitted during a write.
Ran the full backend suite against Postgres locally: 4513 passed, 48 skipped. Two feature lifecycle tests error during setup in my environment because they connect to InfluxDB, which I am not running locally.
make typecheckis clean, as are the pre-commit hooks, including the docs catalogue generation.