Skip to content

fix: Read all items when a kind holds a single key - #444

Open
jsonbailey wants to merge 2 commits into
jb/sdk-2995/tombstone-store-keysfrom
jb/sdk-2995/consul-single-key
Open

jsonbailey wants to merge 2 commits into
jb/sdk-2995/tombstone-store-keysfrom
jb/sdk-2995/consul-single-key

Conversation

@jsonbailey

@jsonbailey jsonbailey commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

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):

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.


Note

Overview
Fixes Consul feature store failures when a data kind has exactly one key under its prefix. all / get_all_internal used recursive Diplomat::Kv.get, which returns a bare value String for a single match instead of key/value pairs, so .each raised and flag reads degraded to invalid $flagsState.

get_all_internal now uses Diplomat::Kv.get_all, which always returns an enumerable list of {key:, value:} (and [] when empty), so prefix stripping and tombstone handling stay the same without the old results == "" guard.

Two persistent_feature_store examples assert single-item and single keyless-tombstone all reads via a second store instance (bypassing warm cache), covering the n=1 case that multi-key tombstone tests missed.

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

@jsonbailey
jsonbailey added this pull request to stack #445 September 18, 2026 16:06
A recursive Consul get returns the bare value string when only one key
matches the prefix, instead of a list of key/value pairs. Reading all
items of that kind then raised NoMethodError and failed the whole read,
so every flag fell back to its default.

Read with get_all, which always returns key/value pairs and gives back
an empty list when no key matches.
@jsonbailey
jsonbailey force-pushed the jb/sdk-2995/consul-single-key branch from 19df746 to 6643e7f Compare September 18, 2026 22:27
@jsonbailey

Copy link
Copy Markdown
Contributor Author

Checked get_all availability across every diplomat version we allow, since the switch depends on it.

The pin is ~> 2.6 (launchdarkly-server-sdk.gemspec:31, and contract-tests/Gemfile:17), so the range is >= 2.6.0, < 3.0 — in practice 2.6.0 through 2.6.6, as 2.6.6 is the newest 2.x. All seven are identical on every property this fix relies on:

def get_all passes return_hash = true [] on 404 count == 1 && !return_hash guard
2.6.0 – 2.6.6 (all 7) yes yes yes yes
upstream master yes yes yes yes

Two notes:

  • get_all was added in 2.3.0 (absent in 2.0–2.2, unchanged since), so the pin has three minor versions of headroom. Loosening it to ~> 2.3 would still resolve.
  • master still has get omitting the third argument and the collapse guard intact, so this is current upstream behavior rather than a quirk of the pinned release — the n=1 collapse is not something already fixed upstream that we would be working around.

Out of scope for this PR, but worth recording: diplomat is only a development dependency, so Consul users supply their own. Someone on 2.0–2.2 would hit NoMethodError on get_all. That exposure is not new — the gemspec has never constrained a user's diplomat at runtime, and the same applies to every other Diplomat::Kv call in the file — but a documented minimum or a runtime dependency would close it.

@jsonbailey
jsonbailey marked this pull request as ready for review September 18, 2026 22:35
@jsonbailey
jsonbailey requested a review from a team as a code owner September 18, 2026 22:35
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.

1 participant