Skip to content

fix: Allow tombstones without a key property - #502

Merged
jsonbailey merged 3 commits into
mainfrom
jb/sdk-2941/tombstone-key-check
Aug 26, 2026
Merged

fix: Allow tombstones without a key property#502
jsonbailey merged 3 commits into
mainfrom
jb/sdk-2941/tombstone-key-check

Conversation

@jsonbailey

@jsonbailey jsonbailey commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Symptom

A customer running the Python SDK and the Node server SDK against one shared Redis store saw all_flags_state() start returning FeatureFlagsState(valid=False) with zero flags — the first time any flag was deleted, and for every subsequent read. Individual variation() calls kept working; only the all-flags read broke.

Root cause

FeatureFlag.__init__ (ldclient/impl/model/feature_flag.py:107) and Segment.__init__ (ldclient/impl/model/segment.py:76) validated the required key property before the deleted-item early-out:

self._key = req_str(data, 'key')      # runs first
self._version = req_int(data, 'version')
self._deleted = opt_bool(data, 'deleted')
if self._deleted:
    return                             # tombstone exemption started here

A persisted tombstone with no inner key therefore raised ValueError: error in flag/segment data: required property "key" is missing.

CachingStoreWrapper._cache_put_all (ldclient/feature_store_helpers.py:130-138) decodes every item in the set with no per-item error handling, so one such record fails the entire all-flags read, and all_flags_state() returns an invalid state with no flags (ldclient/client.py:629-635).

Cross-SDK context

Keyless tombstones are the norm, not a corruption:

  • The Node server SDK's Redis store writes deleted items as {"version":N,"deleted":true} with no key (js-core, packages/store/node-server-sdk-redis/src/RedisCore.ts:163-168). .NET and Java do the same.
  • Haskell and C++ short-circuit on deleted before validating the key. .NET's guard is literally if (key is null && !deleted).
  • Go and the Relay Proxy write a placeholder key ($deleted), which already decoded fine here.

Python was the only SDK that required the inner key on a tombstone. The key is redundant in a persistent store anyway: the store already knows it, because it is the Redis hash field the item is stored under.

Fix

The key requirement now applies only to items that are not deleted. A tombstone:

  • still requires version;
  • still validates key's type if one is present, so the Go/Relay shape keeps working;
  • sets .key to an empty string when no key is present, so callers reading .key never hit an AttributeError (__slots__ is declared on these classes).

The version check moves above the deleted branch so it stays a single check. One knock-on: a live record missing both key and version now reports version as the missing property instead of key. Both are still required, only the property named in the error changes, and no test asserted the old wording.

No key is synthesized into the stored dict. ModelEntity.to_json_dict() returns self._data and the FDv2 Store.commit() path depends on that, so the original data still round-trips verbatim — a test asserts this.

Tests

  • ldclient/testing/impl/test_model_decode.py — for both FEATURES and SEGMENTS: a keyless tombstone decodes and round-trips unchanged; the Go/Relay $deleted shape decodes (regression guard); a tombstone still requires version; a live record with no key still raises ValueError, including the explicit "deleted": False case.
  • ldclient/testing/test_feature_store_helpers.py and test_async_feature_store_helpers.py — a wrapper whose core holds a valid flag plus a keyless tombstone returns the valid flag from all() without raising, the tombstone is filtered out, and get() on it returns None.

No existing test asserted the old strictness for tombstones, so nothing had to be updated. make test (1434 passed, 276 skipped) and make lint both pass.

Out of scope

Deliberately kept small and single-purpose. These are tracked separately and are not in this PR:

  • per-item error isolation in _cache_put_all, so one bad record cannot fail a whole set;
  • validate-before-write reordering;
  • json.loads guards in the store cores;
  • the store-outage misclassification fix.

Related

  • SDK-2941
  • Customer escalation FROPS-511

Note

Overview
Fixes shared persistent-store setups (e.g. Python + Node on Redis) where keyless deletion tombstones ({"version":N,"deleted":true}) caused all_flags_state() to return invalid empty state because decoding required an inner key.

Model: FeatureFlag and Segment now validate version first; deleted items no longer require key (optional placeholder like $deleted still works). .key is '' when absent, and to_json_dict() still round-trips the original payload without synthesizing a key.

Stores: Consul and DynamoDB get_all_internal index results by the database/storage key (hash field / KV path / sort key), not item['key'], matching Redis and other SDKs.

Tests: Decode/round-trip cases for FEATURES and SEGMENTS; caching wrapper tolerance; integration write_raw_item + test_all_reads_tombstone_with_no_key for Consul, DynamoDB, and Redis.

Reviewed by Cursor Bugbot for commit 37bddbb. Bugbot is set up for automated code reviews on this repo. Configure here.

@jsonbailey
jsonbailey requested a review from a team as a code owner August 19, 2026 16:48
@jsonbailey
jsonbailey force-pushed the jb/sdk-2941/tombstone-key-check branch 2 times, most recently from 030c519 to a245d0e Compare August 19, 2026 17:27
FeatureFlag and Segment validated the required "key" property before the
deleted-item early-out, so a persisted tombstone written with only a
version could not be decoded. Other LaunchDarkly SDKs (Node, .NET, Java)
write keyless tombstones to a persistent store, so a shared store broke
Python's all-flags read.

The key requirement now applies only to items that are not deleted. A
tombstone still requires a version, and .key falls back to an empty
string so callers never fail. The stored data still round-trips verbatim.
@jsonbailey
jsonbailey force-pushed the jb/sdk-2941/tombstone-key-check branch from a245d0e to d99edf5 Compare August 19, 2026 17:36
The Consul and DynamoDB cores rebuilt the all-items map from the key
inside each item, and threw away the key the item was stored under. A
deleted item written by another SDK has no key of its own, so the read
failed with KeyError('key') and broke all_flag_state.

Both cores now use the key they already have: the Consul KV path and the
DynamoDB sort key. The Redis cores were already correct.
@jsonbailey
jsonbailey requested a review from keelerm84 August 26, 2026 14:56
@jsonbailey
jsonbailey merged commit 5f44e61 into main Aug 26, 2026
15 checks passed
@jsonbailey
jsonbailey deleted the jb/sdk-2941/tombstone-key-check branch August 26, 2026 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants