Fix missing real-time notes by pinning the primary read before the note resolves

What does this MR do and why?

Notes created on a work item sometimes never reach people watching it in real time. A page reload always fixes it, which is what pointed at replica lag rather than a broken feature.

Related to #630752.

This MR pins the subscription's database read to the primary at the point in the flow where it actually matters: before graphql-ruby turns the broadcast payload back into a Note, not after. It ships behind graphql_subscription_primary_fallback_on_load, off by default.

The flow, and where it breaks

When someone posts a note, here is what happens on the way to a subscriber:

1. The note is inserted on the primary.
2. GitLab broadcasts an ActionCable message. To keep it small, the payload carries no note, only a global ID (gid://gitlab/Note/24) plus the WAL locations recording how far the primary had gotten.
3. The receiving process turns that global ID back into a Note. graphql-ruby does this in load_action_cable_message.
4. Only after that does graphql-ruby call execute_update, where our own Subscriptions::Notes::Created runs.

Steps 3 and 4 sit on adjacent lines inside the gem (graphql 2.6.10, lib/graphql/subscriptions/action_cable_subscriptions.rb:180-181).

Step 3 reads from a replica. If the note has not replicated yet, GlobalID::Locator.locate raises ActiveRecord::RecordNotFound. Nothing on this path rescues it (app/channels/graphql_channel.rb only rescues Gitlab::Graphql::Variables::Invalid), so the whole update is dropped and the subscriber gets nothing. That matches the report exactly: not a stale note, no note at all.

GitLab's existing primary fallback lives inside execute_update, step 4. By the time it runs, step 3 has already failed, so pinning there is too late no matter which subscription class does it.

Why the existing edited-note fix (!199101 (merged)) does not carry over

!199101 (merged) fixed a different symptom (#535280 (closed)): an edited note reverting to its old text in real time. It pinned the re-read to the primary inside Subscriptions::Notes::Updated, and that works because for an edit the row already exists on the replica, just with stale column values. Step 3 succeeds, and step 4's re-read on the primary repairs it.

For a create, step 3 itself is what fails. There is nothing left on the replica for step 4 to repair. Wrapping Subscriptions::Notes::Created the same way, which an earlier revision of this MR did and which is now reverted, cannot help, because the code never reaches it.

Also worth noting: the load balancer session is cleared per ActionCable work unit (lib/gitlab/database/load_balancing/action_cable_callbacks.rb:15-16). Every message starts out reading from a replica, so there is no leftover pin from an earlier message that could rescue this one by accident.

The fix

lib/gitlab/graphql/subscriptions/action_cable_with_load_balancing.rb overrides load_action_cable_message. It reads the WAL locations straight out of the raw broadcast JSON, which needs no query, since they are plain strings sitting next to the global ID. If the replicas are behind, it pins the session to the primary before letting the gem resolve the global ID. execute_update still makes the same call it always did when the flag is off.

This transport backs every GraphQL subscription in the schema, not only notes, which is why it is behind a flag: graphql_subscription_primary_fallback_on_load, type gitlab_com_derisk, default off. With the flag off the ordering is unchanged.

Evidence: measured ordering and reproduced failure

Ordering from a real round trip through the transport:

load_action_cable_message :before
load_action_cable_message :after   payload_class=DiscussionNote payload_id=21
execute_update :before
session_map_with_sessions
use_primary_bang
execute_update :after

Failure mode, reproduced by deleting the row to stand in for "not replicated yet", since the RSpec suite has no real replica:

DUMPED: {"wal_locations":{},"gql_payload":{"__gid__":"Z2lkOi8vZ2l0bGFiL05vdGUvMjQ"}}
LOADED OK: Note id=24 same_object=false
AFTER DELETE: ActiveRecord::RecordNotFound: Couldn't find Note with 'id'="24"

Tests

• 20 examples in spec/lib/gitlab/graphql/subscriptions/action_cable_with_load_balancing_spec.rb, covering both flag states. The broadcast ones spy on the serializer to confirm the session is already pinned when the payload is resolved.
• Two of those are new in this revision. They cover an envelope the safe parser cannot read: one valid JSON but over the parser's size limit, one malformed. Both assert the primary is pinned anyway, and both failed against the previous behavior.
• 53 green across that spec plus spec/graphql/subscriptions/notes/created_spec.rb, spec/graphql/subscriptions/notes/updated_spec.rb and spec/requests/api/graphql/subscriptions/notes/.
• Four more cover the log line: one per reason, plus one asserting nothing is logged when the replicas are in sync.
• created_spec.rb is new. It is the first coverage that class has had.

Rollout

The flag actor is Feature.current_request. Its flipper_id is a fresh SecureRandom.uuid memoised in the request store (lib/feature.rb:50-56), and each ActionCable work unit gets its own store (lib/gitlab/action_cable/request_store_callbacks.rb:12). One actor is one work unit.

Correction to an earlier version of this description: this schema is not broadcastable. app/graphql/gitlab_schema.rb:17 uses the transport with no broadcast: true, so BroadcastAnalyzer is never installed, :subscription_broadcastable is never set, and Event#fingerprint falls back to a per-event UUID. Each subscriber resolves its own payload in its own work unit. Two comments in the repo say the same: app/graphql/subscriptions/work_items/namespace_work_item_changes.rb:5 and app/services/work_items/namespace_changes/broadcast_service.rb:41.

That changes what a percentage rollout buys. A percentage of actors samples work units, not users. At 10 percent, roughly one subscriber in ten of a given note takes the new path. That is useful for measuring load and poor for validating the fix, since nobody's notes become reliable until the flag is near 100 percent.

Suggested rollout: a short soak at 1 percent and 10 percent, watching primary queries per second on the websockets fleet, then straight to 100. Skip percentage_of_time; doc/development/feature_flags/_index.md:745 discourages it.

Merging is inert on its own because the flag defaults off.

Database review

Done. There is no new SQL, no scopes, no finders, no migrations and no schema change, so no EXPLAIN plans apply here. The only database-relevant change is that an existing read-routing decision moves one step earlier inside the same work unit.

The pin condition is evaluated the same number of times as before, once per work unit. What newly falls inside the pinned window is the global ID lookup that resolves the payload: one primary key SELECT per global ID, or one IN query per model class when the payload is an array. Everything downstream of that, the whole subscription re-execution, was already inside the pin. For workItemNoteCreated the shipped client document expands the full discussion thread, which is tens of queries. So the increment is about one read against a denominator of tens. When the replicas are in sync, nothing pins and the delta is zero.

No blockers found.

The review asked for a way to tell "pinned because the replicas lagged" apart from "pinned because the WAL locations were absent" apart from "did not pin", since without it the soak produces a load number and no evidence the fix fired. That is now in. Each fallback writes one line to database_load_balancing.log with event: graphql_subscription_primary_fallback and a fallback_reason of replicas_behind, wal_locations_absent or envelope_unreadable. Nothing is written when the replicas are in sync, so the volume tracks the lag window rather than the broadcast rate.

The line carries a correlation_id, and the broadcast envelope now carries it so that it can. An ActionCable work unit has no Labkit context of its own: nothing pushes one per work unit, so Labkit::Context.current is empty there and a log line written from it has no correlation id at all. Taking the id from the publishing request and putting it in the envelope next to the WAL locations is what makes the decision findable in Kibana from the request that created the note.

For the record, and unchanged by this MR: the pin applies to every load balancer, main, ci and sec, even when only one database's WAL is behind. That call was already in execute_update.

What this does not prove

The RSpec suite has no real replica. The :database_replica tag points the replica load balancer at the same host as the primary, so the tests prove the pin is active at the right moment, not that the production symptom goes away. That still needs a staged rollout with monitoring.

There are no production numbers here for broadcast rate, subscribers per event, or how often databases_in_sync? is false. The load argument in the database review section is a bound and a ratio, not a measurement.

Generated with Claude Code