fix(LaunchDarkly): Import processing not idempotent for completed requests#7954
Conversation
…d requests process_import_request() clears an import request's ld_token once it finishes (success or failure). If the task is delivered again for the same request (e.g. a stale/duplicate task-processor retry), it tried to unsign that now-empty token and crashed with django.core.signing.BadSignature: No ":" found in value, instead of being a harmless no-op. Guard against reprocessing an already-completed request. Signed-off-by: Zain Asif <zainasif.jutt1@gmail.com>
|
@zain-asif-dev is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Related PRs: None identified. Suggested labels: bug, backend, tests Suggested reviewers: None identified. 🐰 A guard now stands where imports once looped, Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b6cfc865-b39a-4f55-8716-58da1e75f887
📒 Files selected for processing (2)
api/integrations/launch_darkly/services.pyapi/tests/unit/integrations/launch_darkly/test_services.py
khvn26
left a comment
There was a problem hiding this comment.
just a couple edits away from merging this 👍
Signed-off-by: Zain Asif <zainasif.jutt1@gmail.com>
|
Removed those comments. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7954 +/- ##
==========================================
- Coverage 98.63% 98.49% -0.15%
==========================================
Files 1497 1497
Lines 59189 59198 +9
==========================================
- Hits 58383 58305 -78
- Misses 806 893 +87 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
docs/if required so people know about the feature.Changes
Closes #7950
The Sentry trace attached to the issue shows
process_import_requestcrashing with:while unsigning
import_request.ld_token. This isn't actually about the create endpoint accepting an empty token:CreateLaunchDarklyImportRequestSerializer.tokenis a plainCharField, andallow_blankdefaults toFalse, so the create endpoint already rejects a blank token with a 400 (verified locally, see test plan below).The real issue is in
process_import_requestitself:_complete_import_requestclearsimport_request.ld_tokento""once the request finishes (success or failure), by design, so the token isn't kept around longer than needed. Ifprocess_import_requestruns again for that same, already-completed request (for example a stale/duplicate task-processor delivery, which the task'stimeoutand failure-retry behavior make possible), it tries to unsign that now-blank token and blows up with the exactBadSignatureerror from the Sentry trace, rather than being a harmless no-op.This adds a guard at the top of
process_import_requestthat returns early (with a log message) ifimport_request.completed_atis already set, making reprocessing idempotent instead of crashing.How did you test this code?
Added
test_process_import_request__already_completed__does_not_reprocess, which marks an import request as completed with a blankld_token(mirroring what_complete_import_requestleaves behind) and asserts that callingprocess_import_requeston it again is a no-op (the LaunchDarkly client is never re-instantiated). I confirmed this test reproduces the exactBadSignature: No ":" found in valuecrash from the Sentry trace when the guard is removed, and passes with the fix.Also ran the full
tests/unit/integrations/launch_darkly/suite (47 passed),ruff check/ruff format --check, andmypyon the changed files.