graphql-pro: RedisStorage clean_inactive_subs leaves dead subscription IDs in mixed fingerprint sets under TTL expiry

[sealabcore]

This is a graphql-pro bug (closed-source), but Pro has no public issue tracker, so I'm filing here to reach you — happy to move it to support@graphql.pro if you'd prefer. We're a Pro license holder.

Gem / location

• graphql-pro 1.30.0
• GraphQL::Pro::Subscriptions::RedisStorage, in the clean_inactive_subs Lua script used by each_fingerprint_and_subscription_ids.

Summary

When a subscription's query record is removed by Redis TTL (stale_ttl_s) rather than by an explicit delete_subscription, the cleanup that runs while iterating a topic's fingerprints fails to remove that subscription id from its fingerprintsubscriptions:<fingerprint> set — unless every member of that set is dead. Any fingerprint set that still has at least one live member accumulates dead ids indefinitely, so fingerprintsubscriptions:* grows without bound.

Root cause

The fingerprintsubscriptions:<fingerprint> set is populated with the bare subscription id (via sadd(fingerprint_subscriptions_key(f), [sub_id]) in the write path). But in clean_inactive_subs, the SREM against that set is issued with the prefixed query key (<prefix>query:<id>) — the same variable used for the preceding DEL — instead of the bare id. Since that value is never a member of the set, the SREM is a silent no-op and the dead id is left behind.

The leak is masked for fully-dead sets only because run_fingerprint_batch separately DELs the whole set when no active ids remain. Mixed (live + dead) sets are where it leaks.

Strong signal that this is the bug and not intended: the legacy path (filter_active_subscriptions) does it correctly — it SREMs the bare inactive_subscription_ids. Only the fingerprint path uses the prefixed key.

Suggested fix

In clean_inactive_subs, SREM the bare subscription id from fingerprintsubscriptions:<fingerprint> (the bare ids are already available 1:1 alongside the prefixed query keys), matching what the legacy filter_active_subscriptions path does.

Reproduction

1. Configure RedisStorage with a short stale_ttl_s.
2. Create two subscriptions whose queries hash to the same fingerprint (same query + variables), so both ids land in one fingerprintsubscriptions:<fp> set.
3. Let one subscription's query record expire via TTL while the other stays live.
4. Trigger the topic (runs each_fingerprint_and_subscription_ids).
5. Observe: the expired id is still a member of fingerprintsubscriptions:<fp> (SMEMBERS lists it) and is never reaped.

Impact (our production)

Redis backing GraphQL subscriptions roughly doubled in memory over a few months. ~96% of keys were gql:sub:*, and a sampled ~99% of fingerprintsubscriptions members pointed at query records that had already expired. The affected pattern is recurring per-user fingerprints (e.g. mobile clients that reconnect with a fresh subscription id but an identical query+variables fingerprint): the set always keeps one live member, so the bug applies and dead ids pile up.

Thanks for all your work on graphql-ruby/pro!

[rmosolgo]

Hey, thanks for the detailed report and sorry for the trouble with this. That's a lot of dead data in there 😖 I'm going to work up a new release of graphql-pro which fixes the bug and includes a method for cleaning this stale data. I'll follow up here when it's ready!

[sealabcore]

Update after shipping our fix to production — correcting the impact I described above, for accuracy.

We swept all topics and ran the cleanup against our full production keyspace. Results: ~338K orphaned fingerprintsubscriptions sets → ~4K; Redis 105 MB → 18 MB; orphan ratio 98% → ~2%; ~23.5K topics reconciled. But the bare-id removal for mixed sets reported zero removals.

That zero is the important part. It means the bulk of our orphans were fully-dead sets (every member's query record expired), which run_fingerprint_batch already DELs correctly once a topic is visited — our accumulation was actually a coverage gap on our own side (our sweep only visited the first N topics), not this bug. The mixed-set case this issue is about — a set retaining a live member while never shedding its dead ids — was essentially absent in our data, because our fingerprints are near-universally single-member (per-user subscriptions → unique variables), so sets end up either fully-dead or fully-live.

So in the interest of accuracy: I can't claim this bug caused observable impact for us, and my earlier 'recurring per-user fingerprints' scenario was wrong for our deployment. The code defect itself is still real — clean_inactive_subs SREMs the prefixed <prefix>query:<id> key rather than the bare sub_id, so it's a no-op against the bare-id set — and it would leak for deployments with genuinely shared fingerprints (multiple subscription ids hashing to one query+variables fingerprint) under TTL-based expiry. Just reframing it as a latent correctness defect rather than a high-impact leak.

[rmosolgo]

Hey, thanks for sharing what you found when cleaning this up. I want to make sure I understand: there was stale data in there, but it turned out to be from a different origin than the real bug described in the OP. Is that right?

I'm not quite sure where the stale data actually came from. You mentioned "our sweep only visited ...", but shouldn't GraphQL-Pro have cleaned up its own stale data? Or did I misunderstand something along the way?

[sealabcore]

Yes, that's right — and sorry, my "recurring per-user fingerprints" framing in the original report sent you down the wrong path. Let me untangle the two things, because they really are separate:

1. The OP bug (mixed-set bare-id SREM). Real, but it contributed ~0 for us — our bare-id prune reported zero removals because our fingerprints are near-universally single-member, so sets end up fully-dead or fully-live, never mixed. This bug only bites deployments with genuinely shared fingerprints.

2. Where our stale data actually came from. This is on our side, and it's a lifecycle interaction worth flagging for the cleanup method you're shipping. We run with stale_ttl_s set, and we've intentionally disabled the eager-delete paths (channel_vacated webhook off, plus an override forcing still_subscribed? => true) because Pusher reports a channel vacant ~60s after the last socket drops, which was nuking mobile clients that backgrounded. The side effect: subscriptions now expire almost exclusively via TTL, and TTL expiry drops the query:{id} record without ever calling delete_subscription — so the index sets (fingerprintsubscriptions:*, fingerprints:*, alltopics) keep the dead id. And delete_subscription couldn't clean them even if it ran, since it reads the topic/fingerprint list out of the record that just expired.

graphql-pro does recover from this — each_subscription_id runs clean_inactive_subs and correctly DELs fully-dead sets — but only lazily, when a topic is next triggered or swept. The gap for us was the long tail of quiet topics: they're never triggered again, and remove_unsubscribed only sweeps the first limit: topics at offset: 0, so they're never revisited. That's the "our sweep only visited the first N topics" coverage gap — fully-dead sets piling up on topics nothing ever returns to.

So the takeaway for the gem fix: I think any stale_ttl_s deployment that lets records expire via TTL rather than explicit delete will accumulate orphaned index entries on topics that go quiet, independent of the mixed-set bug. A cleanup method that walks every topic (not just an offset window) and reuses the existing each_subscription_id cleanup would cover the bulk of it.

Here's the reconciliation we run hourly, in case it's a useful reference. It snapshots topic names up front (the remove_topic SREMs shrink alltopics mid-sweep, so paging by offset would skip topics), drives the gem's own per-topic cleanup across all of them, then prunes dead bare ids from any surviving mixed sets to cover the OP bug too:

def reconcile_orphaned_subscriptions
  topic_names = topics(limit: nil, offset: nil).first.map(&:name)
  topic_names.each do |name|
    each_subscription_id(name) { |_sub_id| nil }  # reuse gem's atomic Lua cleanup
    prune_dead_fingerprint_members(name)           # mixed-set bare-id prune (OP bug)
  end
  topic_names.size
end

-- prune dead bare ids from a topic's fingerprintsubscriptions s
-- (clean_inactive_subs SREMs the prefixed query key, which is at)
local query_prefix = ARGV[1]
local removed = 0
for _, fp_key in ipairs(KEYS) do
  for _, sub_id in ipairs(redis.call('smembers', fp_key)) do
    if redis.call('exists', query_prefix .. sub_id) == 0 then
      removed = removed + redis.call('srem', fp_key, sub_id)
    end
  end
end                                                                                                                                                    return removed

(We SSCAN the fingerprints in batches and run the Lua per batch so a hot topic can't block single-threaded Redis — happy to share the full method if useful.) After running this across the full keyspace: ~338K orph → 18 MB.