Fix subscription event delivery when some subscriptions expire

[prog-supdex]

Fix #52

Why

A subscription can expire after execute_grouped selects it but before read_subscription reads its data.

When that happens, the event is not executed and other active subscriptions with the same fingerprint do not receive the update.

What we do

If the selected subscription is no longer stored in Redis, try the next subscription from the same group.

read_subscription now raises GraphQL::AnyCable::SubscriptionExpiredError when the Redis hash is missing, and execute_grouped retries only in that case.

If GraphQL returns NO_UPDATE or unsubscribes, stop and do not run the same update again.

Release the Redis connection before deserializing subscription data.

Use the configured Redis connector when deleting a subscription instead of the deprecated GraphQL::AnyCable.redis accessor.

Added specs for the race condition, NO_UPDATE, unsubscribe, missing subscriptions, Redis connection re-entrancy, and the case when all subscriptions have expired.

[prog-supdex]

GitHub workflow installs graphql 2.5 (here GraphQL-Ruby ~> 2.3), which brings stricter validation for subscriptions here and here

According to the validations, subscription operations must have only one root field
This means we can not use multiple subscription fields in a single query like:

subscription SomeSubscription {
  productCreated { id title }
  productUpdated { id }
}

I have updated the tests (stat_spec and anycable_spec) to follow this limitation

[bunnybilou]

Hi @prog-supdex — thanks for this, matches issue #52 for us too. We've patched read_subscription locally (return nil on missing query_string) to stop the bad broadcasts, but still exposed to the execute_grouped race since it only tries one subscription_id. Any status on getting this merged? Would like to drop our workaround once it lands.

[Envek]

Thanks for digging into this, @prog-supdex! I ran the suite on both master and this branch (46 examples, 0 failures here; the branch is up to date with master and merges cleanly) and went through it in detail.

Verdict: the core fix is correct and worth merging, but there is one behavioral regression I reproduced empirically that should be addressed first.

What it gets right (verified)

1. The race in execute_grouped is genuinely fixed. The old code picked one id via exists? and gave up if read_subscription then came back empty — the whole fingerprint group lost the update. The retry loop is the right shape.

2. read_subscription returning nil is safe across the whole supported graphql-ruby range. I checked this because the gemspec allows graphql >= 1.11, < 3 and returning nil where a Hash was returned before is an interface change. Subscriptions#execute_update guards with if query_data.nil? → delete_subscription → return nil in v1.11.6, v1.13.24, v2.0.31, v2.5.4 and current master, and read_subscription has no other caller anywhere in graphql-ruby. So this is compatible.

This also fixes the second half of #52 directly: previously .tap returned {query_string: nil, variables: nil, …} — non-nil, so graphql-ruby proceeded and emitted "No query string was present" to the client.

3. The delete_subscription(subscription_id, redis: AnyCable.redis) change is a real bug fix, and a bigger one than the PR describes. Inside GraphQL::Subscriptions::AnyCableSubscriptions, constant lookup resolves bare AnyCable to GraphQL::AnyCable (note the class deliberately uses ::AnyCable for broadcast). So the default arg called the deprecated GraphQL::AnyCable.redis, which does @redis ||= with_redis { |conn| conn } — it memoizes a connection after returning it to the pool. Confirmed on master:

$ bundle exec rspec   # calling subs.delete_subscription("does-not-exist")
Usage of `GraphQL::AnyCable.redis` is deprecated. …

For anyone using the ConnectionPool proc from the README, that is a connection used outside its checkout, permanently. Worth its own CHANGELOG line.

Issues

1. Blocking — the retry loop re-executes the query N times when subscribers unsubscribe

break if result || subscription_exists?(subscription_id) treats "subscription gone after execute" as "it expired, retry". But graphql-ruby has a second path that produces exactly that state (lib/graphql/subscriptions.rb#L136):

if subscriptions_context[:unsubscribed] && !subscriptions_context[:final_update]
  delete_subscription(subscription_id)
  result = nil
end

Subscription#unsubscribe (no arg) sets unsubscribed = true with no final update — so calling unsubscribe inside update deletes the row and returns nil, and the loop retries with the next subscriber in the group. Since fingerprint groups exist precisely to hold many identical subscribers, every one of them runs the full query.

Measured with a 5-subscriber group whose update calls unsubscribe:

	GraphQL executions per trigger
master	1
this PR	5

"Unsubscribe when the thing finishes" is a common idiom, and it pairs badly with broadcast: true: a group of 5k subscribers turns one trigger into 5k executions and still delivers nothing.

2. Same root cause — NO_UPDATE detection is racy

The post-hoc subscription_exists? probe is a second round trip, so between execute_update returning NO_UPDATE and the probe, the key can expire. Then a skip that GraphQL explicitly requested becomes an update broadcast to the whole group. Low probability, but it is the exact condition (high load + subscription_expiration_seconds) that #52 is filed under.

Fix for both: take the "it vanished" signal from read_subscription itself instead of re-probing Redis. Keep the cheap subscription_exists? pre-check (it preserves master's cost profile by skipping stale ids without triggering delete_subscription churn) and replace only the post-check:

MISSING_SUBSCRIPTION = :__graphql_anycable_missing_subscription__

def execute_grouped(fingerprint, subscription_ids, event, object)
  return if subscription_ids.empty?

  result = nil

  subscription_ids.each do |subscription_id|
    next unless subscription_exists?(subscription_id)

    Thread.current[MISSING_SUBSCRIPTION] = false
    result = execute_update(subscription_id, event, object)

    # Retry with another subscription of the group only if this one vanished
    # between the check and the read. A nil result from GraphQL itself
    # (NO_UPDATE, or unsubscribe without a final update) applies to the
    # whole group, since they all share the same fingerprint.
    break unless Thread.current[MISSING_SUBSCRIPTION]
  end

  return unless result

  deliver(redis_key(SUBSCRIPTIONS_PREFIX) + fingerprint, result)
ensure
  Thread.current[MISSING_SUBSCRIPTION] = nil
end

def read_subscription(subscription_id)
  with_redis do |redis|
    subscription = redis.mapped_hmget(
      "#{redis_key(SUBSCRIPTION_PREFIX)}#{subscription_id}",
      :query_string, :variables, :context, :operation_name
    )

    if subscription[:query_string].nil? # Redis returns a hash of nils for a missing key
      Thread.current[MISSING_SUBSCRIPTION] = true
      next
    end

    subscription[:context] = @serializer.load(subscription[:context])
    subscription[:variables] = JSON.parse(subscription[:variables])
    subscription[:operation_name] = nil if subscription[:operation_name].to_s.strip == ""
    subscription
  end
end

Thread.current rather than an ivar: there is one AnyCableSubscriptions instance per schema and trigger is called concurrently. This also drops the .then chain, which reads better as a plain local — then purely for control flow is a bit obscure here.

3. The PR description's connection claim is inaccurate for execute_grouped

Redis connections are also no longer held while GraphQL executes.

master's execute_grouped already closed its with_redis block before calling execute_update. Where the claim is true is delete_subscription (point 3 above). Worth correcting so the rationale matches the change.

4. Minor — pool checkouts, one per candidate

master did all the exists? calls inside a single with_redis; this PR opens one per id. Commands are unchanged, but for ConnectionPool users (the pattern the README documents) a mostly-expired group of N ids now costs N checkouts instead of 1. Negligible with the default plain Redis client. Given #56/#57 went the other way on this, a pipelined EXISTS over the candidate list would fit the codebase's direction — but it trades laziness for eagerness, so I'd leave it unless profiling says otherwise.

5. Minor — stricter missing-hash check is a silent behavior change

subscription.values.all?(&:nil?) → subscription[:query_string].nil? also captures the partially-written/corrupt-hash case, which now silently deletes the subscription and drops the client instead of surfacing a GraphQL error. I think that is the better default, but it is undocumented. The added .to_s on operation_name is fine.

Test gaps

The three new specs are well targeted, and the redis_checked_out re-entrancy guard is a nice regression net for pool deadlocks. Gaps:

• No spec for the unsubscribe-during-update path — the one that regresses. This is the missing case.
• No spec for delete_subscription called without redis:, which is the whole point of that hunk. An expect(GraphQL::AnyCable).not_to receive(:redis) assertion would lock it in.
• redis.del("graphql-subscription:#{subscription_id}") hardcodes the prefix in two places instead of deriving it from config.redis_prefix + SUBSCRIPTION_PREFIX.
• The re-entrancy assertion lives in a before hook of an example described as "broadcasts the result using another subscription" — failures there will point at the wrong thing. Better as its own example.
• let(:subscription_ids) { redis.smembers(...) } — SMEMBERS is unordered, so "the first subscription expires" is really "an arbitrary one". It is self-consistent because the let is memoized before subject runs, so the test is not flaky, but the naming is misleading.
• allow_any_instance_of(GraphQL::Subscriptions::Event).to receive(:fingerprint) with a literal string couples the spec to internals; the real fingerprint would work.

Lint-wise these are all fine — RSpec/AnyInstance and RSpec/ExpectInHook are not enabled in .rubocop/rspec.yml, and RSpec/ReceiveCounts only matches receive(...) with counts 1–2, so have_received(:execute_update).exactly(3).times is not an offense.

CHANGELOG

The entry is correctly placed under ## Unreleased → ### Fixed, and [@prog-supdex] is already defined at the bottom of the file. Consider a second line for the GraphQL::AnyCable.redis deprecation/pooled-connection fix — it is a user-visible improvement that the current entry hides.

---

Summary: merge after fixing 1 (and 2, which the same change handles). Everything else is polish. I will push the fixes to this branch shortly so you do not have to — please review them.

[Envek]

I pushed three commits addressing the review above. Summary of what changed and why:

1. Tell a vanished subscription from a deliberately skipped update

The retry condition break if result || subscription_exists?(subscription_id) reads "the subscription is gone, so it must have expired — try the next one". graphql-ruby produces that same state on a second path: #unsubscribe without a final update deletes the subscription and returns nil (:unsubscribed && !:final_update). So one subscriber unsubscribing on a trigger made the loop re-run the whole query for every remaining subscriber of the group — 5 executions for a group of 5, against 1 before — and then deliver nothing to anybody.

The probe was also racy in the other direction: a subscription could expire between #execute_update returning NO_UPDATE and the exists? round trip, turning a skip GraphQL asked for into a broadcast to the whole group.

Only #read_subscription can tell those two apart, so it now says so directly by raising GraphQL::AnyCable::SubscriptionExpiredError instead of returning nothing. That let #subscription_exists? go away entirely — a read already tells us whether the subscription is there — so a healthy group costs 3 Redis round trips per fingerprint instead of 4, and a group with 3 of 5 expired costs 4 instead of 5.

The raise and the deserialization also moved outside the with_redis block, so neither happens while a pooled connection is checked out.

2. Cover unsubscribing, deletion and connection re-entrancy

A spec for the case that regressed, which fails against the previous retry condition and passes now. Plus one for #delete_subscription without an explicit connection, since that is the whole point of dropping its AnyCable.redis default. The re-entrancy check became an example of its own — it was asserting from inside a stub in a hook, so a failure would have been reported against an unrelated expectation.

The stub on Event#fingerprint is gone: a hand-built event cannot compute one (it has no query), but nothing on this path asks it to, since #execute_grouped receives the fingerprint as an argument. The specs read the real one from Redis instead.

3. Note the connection and read_subscription changes in the changelog

The AnyCable.redis default was a user-visible fix of its own — bare AnyCable resolves to GraphQL::AnyCable inside the adapter, so it went through the deprecated accessor, which memoizes a connection after returning it to the pool and prints a warning on every stale subscription. And #read_subscription raising is a contract change worth recording for anyone calling it.

On raising from #read_subscription

This was the part worth checking carefully, so here is the evidence rather than an assertion.

Across the whole supported range, whole gem trees, not just subscriptions.rb:

graphql-ruby	callers of read_subscription
1.11.0 … 2.5.4	Subscriptions#execute_update only
2.6.0 … 2.6.9	execute_update + Dashboard::Subscriptions::SubscriptionsController#show

execute_update in turn has exactly one caller in every version, Subscriptions#execute — which this adapter overrides to raise NotImplementedError. That is the proof that #execute_grouped is the only live path: anything else reaching execute_update would already be failing today.

The Dashboard call site is gated by before_action :check_installed, whose feature_installed? requires GraphQL::Pro::Subscriptions; AnyCableSubscriptions is not one, so the action never runs. That controller also needs broadcast_subscription_id?, still_subscribed?, read_subscriptions, topics and topic_last_triggered_at, none of which exist in the OSS gem at all.

As a runtime check rather than a reading of the source, forcing read_subscription to raise for every id and then exercising subscribe, trigger, delete_channel_subscriptions, delete_subscription, all five Cleaner entry points and Stats leaks nothing.

Worth knowing that graphql-ruby's own delete_subscription call for a missing subscription, which is now skipped, was a no-op anyway: it reads the events map out of the very hash that expired, so it only ever DELs an absent key. Redis state left behind is byte-identical, and the stale id waits for Cleaner exactly as before.

Verification

50 examples, 0 failures and RuboCop clean, against graphql-ruby 1.13, 2.0 and 2.3.

---

🤖 Investigated and written with Claude Code; the pushed commits are co-authored by Claude.

[prog-supdex]

Thanks, I went through the latest changes. The exception-based approach looks good to me and fixes both the unsubscribe regression and the NO_UPDATE race.

I added two small regression specs: one for the subscription disappearing after execute_update has already returned NO_UPDATE, and one for read_subscription raising when the Redis hash is gone. The first one fails against the previous implementation and passes with the current code.

I also updated the PR description. The full suite passes on graphql-ruby 1.13 and 2.5.

[Envek]

Thank you very much for the contribution and for your patience!

Released in 1.3.4, please enjoy!