fix: Read all items when a tombstone has no key - #443
Merged
Merged
Conversation
jsonbailey
added this pull request to stack #445
September 18, 2026 16:06
jsonbailey
marked this pull request as ready for review
September 18, 2026 16:14
The keyless-tombstone spec already requires the store to key its all-items map by the key each item is stored under, because such an item carries no key of its own. A record whose body key names a different item is a shape no SDK writes, so the spec only covered data that cannot occur.
keelerm84
approved these changes
Sep 21, 2026
jsonbailey
added a commit
that referenced
this pull request
Sep 22, 2026
> **Stacked on #443.** The base of this PR is `jb/sdk-2995/tombstone-store-keys`, not `main`. > **Do not merge this until #443 merges.** GitHub retargets this PR to `main` automatically when #443 lands. ## Symptom When a data kind holds exactly one key in Consul, reading all items of that kind raises: ``` NoMethodError: undefined method 'each' for an instance of String ``` The error propagates out of the store, so the whole all-items read fails rather than one item. `all_flags_state` degrades to `{"$flagsState":{},"$valid":false}` and every flag falls back to its default. ## Root cause `ConsulFeatureStoreCore#get_all_internal` read the collection with a recursive `Diplomat::Kv.get`. `Kv.get` calls `return_value(return_nil_values, transformation)` and leaves `return_hash` at its default `false` (`diplomat-2.6.6/lib/diplomat/kv.rb:28`, the `found == :return` branch). In `return_value` (`diplomat-2.6.6/lib/diplomat/rest_client.rb:189-203`): ```ruby if @value.count == 1 && !return_hash @value = @value.first['Value'] # a bare String, not [{key:, value:}] return @value ``` So a recursive get returns the bare decoded value String when exactly one key matches the prefix. `.each` on a String then raises. Verified against a live Consul (dev agent, diplomat 2.6.6): | keys under prefix | `Kv.get(prefix, {recurse: true}, :return)` | `Kv.get_all(prefix, {}, :return)` | | --- | --- | --- | | 0 | `""` (String) | `[]` | | 1 | **bare value String** | `[{key:, value:}]` | | 2+ | `[{key:, value:}]` | `[{key:, value:}]` | ## The fix Read with `Kv.get_all`, which passes `return_hash = true` and so always returns key/value pairs, and which returns `[]` for a 404 when given `not_found = :return` (`diplomat-2.6.6/lib/diplomat/kv.rb:106-132`). `get_all` sets `:recurse` itself. It returns full keys, so the existing prefix-stripping is unchanged. The `results == ""` guard for the empty case becomes dead and is removed, because `get_all` gives `[]` instead of `""`. ## Why this matters for #443 The `.each` line is not changed by #443, so the bug is pre-existing. But it defeats #443's own fix at n=1: a store whose only `features` row is a keyless tombstone — a single-flag project, or every flag deleted — still raises instead of reading the tombstone and filtering it. #443's new spec does not catch this because it seeds a live item *plus* a tombstone, which is two keys. ## Test evidence Two specs added to the shared `persistent_feature_store` examples in `spec/feature_store_spec_base.rb`: - `can read all items when a kind holds a single item` - `can read all items when the single item is a tombstone with no key` **Before** (on `jb/sdk-2995/tombstone-store-keys`, 2 specs x 4 permutations): ``` 8 examples, 8 failures NoMethodError: undefined method 'each' for an instance of String # ./lib/ldclient-rb/impl/integrations/consul_impl.rb:73:in 'ConsulFeatureStoreCore#get_all_internal' # ./lib/ldclient-rb/integrations/util/store_wrapper.rb:98:in 'CachingStoreWrapper#all' ``` **After**, the Consul suite is green: ``` 92 examples, 0 failures ``` Full suite against live Redis, Consul and DynamoDB (`LD_SKIP_DATABASE_TESTS=0`): ``` 1394 examples, 0 failures ``` `bundle exec rubocop`: 187 files inspected, no offenses detected. ## Note on spec placement The specs go in `persistent_feature_store` rather than `any_feature_store`, so they also cover Redis and DynamoDB. Both already pass there (16 examples), which confirms the defect is specific to the Consul client. They deliberately read through a **second** store instance. `CachingStoreWrapper#init` warms the all-items cache (`store_wrapper.rb:60-74`) and caching is on by default at a 15s TTL, so a single-instance `all` after `init` is served from cache and never calls `get_all_internal` — it passes even on the broken code. An `any_feature_store` placement would therefore not have caught this, and would not work for the in-memory store, which has no shared backing for a second instance and no `write_raw_item`. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Fixes Consul feature store **`all`** reads when a data kind has **zero or one** key under its prefix. Listing used recursive **`Diplomat::Kv.get`**, which returns a bare value **String** (or **`""`**) instead of key/value pairs when only one match exists—**`each`** then raised and **`all_flags_state`** could degrade to invalid defaults. > > **`get_all_internal`** now uses **`Diplomat::Kv.get_all`**, which always returns an enumerable list (including **`[]`** when nothing matches), so prefix stripping and tombstone handling stay the same without the old empty-string guard. > > Shared **`persistent_feature_store`** examples add coverage for single-item, empty-kind, and single keyless-tombstone cases, using a **second store instance** so reads hit the database rather than the warmed all-items cache. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 2734305. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Symptom
With Consul or DynamoDB, a single deleted item whose stored JSON carries no
keybreaks the entire all-flags read.
all_flags_statereturns{"$flagsState":{},"$valid":false}, so every flag falls back to its default.Individual
variationcalls keep working — only the all-items read breaks.Root cause
Both stores rebuilt the all-items map from the
keyinside the record body,discarding the store key they had just read:
consul_impl.rb:76—items_out[item[:key].to_sym] = itemdynamodb_impl.rb:116—items_out[item_out[:key].to_sym] = item_outFor a keyless tombstone
item[:key]isnil, andnil.to_symraisesNoMethodError, which propagates out of the store and fails the whole read.Keyless tombstones are the norm, not a corruption. .NET, Java, Node (Redis
upsert path) and Haskell all write
{"version":N,"deleted":true}, and any SDKcan be pointed at a store another SDK wrote. The inner key is redundant anyway:
the store already addresses the record by key — the Consul KV path, the DynamoDB
sort key.
Redis was never affected because it already keys the map by the outer hash
field. Segments were unaffected on all stores because that path goes through
get(kind, key), which receives the key as a parameter.Fix
Key the output map by the store's own key, which both implementations already
have in hand:
result[:key], strippingthe
kind_key(kind)prefix. Entries outside that prefix are skipped ratherthan mis-keyed.
unmarshal_item, and skip an item that unmarshals tonil.No change was needed in the model layer: Ruby's
FeatureFlagandSegmentconstructors read
data[:key]without requiring it, so a keyless tombstonealready decoded. (This is where the equivalent Python fix also had to change the
model.)
Tests
spec/feature_store_spec_base.rbgains a shared example, run for everypersistent store under all four caching/prefix permutations: a keyless tombstone
written straight to the database alongside a valid item must not spoil the
all-items read, and must read back as absent. Each store tester gains a
write_raw_itemhelper to set up data in a shape the store itself never writes.Verified against the tombstone contract tests in
launchdarkly/sdk-test-harness#438, run locally with Redis, Consul and
DynamoDB-local:
persistent data store(v2,-enable-persistence-tests)The two pre-fix failures were exactly
consul/daemon mode/tombstones/flags/body has no keyanddynamodb/daemon mode/tombstones/flags/body has no key.New rspec examples reproduce the same split: 8 of 12 fail without the fix
(Consul and DynamoDB), Redis's 4 pass either way.
Full suite:
rspec spec1354 examples / 0 failures,rubocopclean.Related
Note
Overview
Fixes Consul and DynamoDB bulk reads (
get_all_internal) so the returned map is keyed by each record’s storage key (Consul KV path suffix, DynamoDB sort key) instead of the optionalkeyfield inside the JSON. Keyless deletion tombstones ({version, deleted}) from other SDKs no longer triggernil.to_symand abort the wholeall/ all-flags read.Adds shared persistent-store coverage that writes a raw keyless tombstone beside a valid item and asserts
allstill returns only live data, pluswrite_raw_itemon Consul/DynamoDB/Redis testers and model deserialization examples for tombstones with and without a key.Reviewed by Cursor Bugbot for commit 8010c8f. Bugbot is set up for automated code reviews on this repo. Configure here.