Fix specs, update changelog, cleanup rbnext (#143)

Author: DmitryTsepelev

.gitignore

@@ -8,7 +8,6 @@
 /tmp/
 Gemfile.lock
 .DS_Store
-.rbnext/
 *.sqlite3
 *.sqlite3-*
 spec/internal/log/*.log

.rubocop.yml

@@ -18,7 +18,6 @@ AllCops:
     - 'Rakefile'
     - 'Gemfile'
     - '*.gemspec'
-    - 'lib/.rbnext/**/*'
 
 Lint/DuplicateMethods:
   Exclude:

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## master
 
+- [PR#99](https://github.com/DmitryTsepelev/graphql-ruby-fragment_cache/pull/99) Fix for race condition when reading from cache
+ ([@gsdean][])
+
 ## 1.22.1 (2025-06-07)
 
 - [PR#137](https://github.com/DmitryTsepelev/graphql-ruby-fragment_cache/pull/137) Remove new_connections? check for gql > 2.3.10 in Tracer ([@DmitryTsepelev][])
@@ -226,3 +229,4 @@
 [@Drowze]: https://github.com/Drowze
 [@danielhartnell]: https://github.com/danielhartnell
 [@mgruner]: https://github.com/mgruner
+[@gsdean]: https://github.com/gsdean

Gemfile

@@ -1,6 +1,9 @@
 source "https://rubygems.org"
 
 gem "pry-byebug", platform: :mri
+# byebug requires "readline", which is a bundled gem on Ruby 3.5+/4.0 and must
+# be declared explicitly so it loads under `bundle exec` (e.g. via Combustion).
+gem "readline", platform: :mri
 
 eval_gemfile "gemfiles/rubocop.gemfile"
 

graphql-ruby-fragment_cache.gemspec

@@ -17,7 +17,7 @@ Gem::Specification.new do |spec|
   spec.metadata["source_code_uri"] = spec.homepage
   spec.metadata["changelog_uri"] = "#{spec.homepage}/CHANGELOG.md"
 
-  spec.files = Dir.glob("lib/**/*") + Dir.glob("lib/.rbnext/**/*") + Dir.glob("bin/**/*") + %w[README.md LICENSE.txt CHANGELOG.md]
+  spec.files = Dir.glob("lib/**/*") + Dir.glob("bin/**/*") + %w[README.md LICENSE.txt CHANGELOG.md]
   spec.require_paths = ["lib"]
 
   spec.required_ruby_version = ">= 3.1"

spec/graphql/fragment_cache/fragment_spec.rb

@@ -52,4 +52,61 @@ def fragment_doubles_factory(count:, ctx: {})
       end
     end
   end
+
+  describe "#read" do
+    let(:cache_key) { "fragment-cache-key" }
+    let(:cache_store) { double("cache_store") }
+
+    let(:fragment) do
+      context = double("context")
+      allow(context).to receive(:namespace).with(:interpreter).and_return(current_path: ["post"])
+      allow(context).to receive(:[]).with(:renew_cache).and_return(nil)
+
+      described_class.new(context).tap do |fragment|
+        allow(fragment).to receive(:cache_key).and_return(cache_key)
+      end
+    end
+
+    before do
+      allow(GraphQL::FragmentCache).to receive(:cache_store).and_return(cache_store)
+    end
+
+    # Regression test for the race condition fixed in #99: existence is checked
+    # with #exist? *before* #read, so a key that is absent at lookup time is
+    # reported as a cache miss without being read — instead of being read first
+    # and mistaken for a cached nil if another request populates it in between.
+    context "when the cache key does not exist" do
+      before do
+        allow(cache_store).to receive(:exist?).with(cache_key).and_return(false)
+        allow(cache_store).to receive(:read).with(cache_key)
+      end
+
+      it "returns a cache miss without reading the absent key" do
+        expect(fragment.read).to be_nil
+        expect(cache_store).not_to have_received(:read)
+      end
+    end
+
+    context "when a nil value is stored in the cache" do
+      before do
+        allow(cache_store).to receive(:exist?).with(cache_key).and_return(true)
+        allow(cache_store).to receive(:read).with(cache_key).and_return(nil)
+      end
+
+      it "returns the sentinel for a cached nil" do
+        expect(fragment.read).to be(described_class::NIL_IN_CACHE)
+      end
+    end
+
+    context "when a value is stored in the cache" do
+      before do
+        allow(cache_store).to receive(:exist?).with(cache_key).and_return(true)
+        allow(cache_store).to receive(:read).with(cache_key).and_return("cached value")
+      end
+
+      it "returns the cached value" do
+        expect(fragment.read).to eq("cached value")
+      end
+    end
+  end
 end

spec/graphql/fragment_cache/object_helpers_spec.rb

@@ -700,13 +700,13 @@ def post(id:, expires_in: nil)
     end
 
     it "calls #read for each entry" do
-      # warmup calls
-      expect(GraphQL::FragmentCache.cache_store).to have_received(:read).exactly(3)
+      # cache was cold during warmup, so #exist? short-circuits before #read
+      expect(GraphQL::FragmentCache.cache_store).not_to have_received(:read)
 
       execute_query
 
-      # read key once
-      expect(GraphQL::FragmentCache.cache_store).to have_received(:read).exactly(6)
+      # cache is warm now: one #read per entry
+      expect(GraphQL::FragmentCache.cache_store).to have_received(:read).exactly(3)
     end
 
     context "when keep_in_context is true" do
@@ -717,13 +717,13 @@ def post(id:, expires_in: nil)
       end
 
       it "calls #read once" do
-        # warmup calls
-        expect(GraphQL::FragmentCache.cache_store).to have_received(:read).exactly(3)
+        # cache was cold during warmup, so #exist? short-circuits before #read
+        expect(GraphQL::FragmentCache.cache_store).not_to have_received(:read)
 
         execute_query
 
-        # read key once
-        expect(GraphQL::FragmentCache.cache_store).to have_received(:read).exactly(4)
+        # cache is warm now: read once, then reused from context for the other entries
+        expect(GraphQL::FragmentCache.cache_store).to have_received(:read).exactly(1)
       end
     end
   end