Address collaborative editing channel review follow-ups

Lee Tickettrequested to merge
gitlab-community/gitlab-org/gitlab:628402-collaborative-editing-channel-follow-ups into master

What does this MR do and why?

Addresses four non-blocking review follow-ups from !253403 (merged), raised by Asherah Connor (@kivikakk).

This is part of the feature that relays Yjs collaborative-editing updates between browsers editing the same wiki page, over ActionCable. The server is not a CRDT peer. It authorises the subscriber, appends opaque base64 updates to a Redis log and rebroadcasts them. The whole feature sits behind the default-off WIP feature flag wiki_collaborative_editing and nothing in the UI reaches it yet, so there is nothing user-facing and no changelog entry.

Two commits, one per concern.

Make DocumentStore reject bad arguments consistently

DocumentStore#append raised ArgumentError while #replace returned false, and the review asked which should win.

The store now raises ArgumentError for contract violations (wrong argument type) in both methods. Return values are reserved for runtime outcomes: no outstanding compaction claim, a stale token, or a lost race.

To make that safe, the channel type-checks the snapshot token in #handle_snapshot. That token arrives straight from the client and was the only argument the store received without a guard upstream; the payload was already validated by #valid_payload?. Behaviour seen by a client is unchanged — a snapshot with a missing or non-String token is still ignored.

Close collaborative editing writes on every unsubscribe path

This follow-up uncovered a bug. ApplicationCable::Channel#handle_authentication_error calls unsubscribe_from_channel but does not remove the subscription from connection.subscriptions, so messages keep being dispatched to the channel. A subscriber who failed the periodic authorization re-check could still append to the Redis log and broadcast to every other peer in the session.

The fix follows the reviewer's suggestion: move the three instance-variable resets out of #revalidate_access into a new #unsubscribed hook. Every unsubscribe path now clears the stream key, and #receive returns early.

Two further notes said the return unless @container guard in #revalidate_access was unreachable, and that the spec covering it implied otherwise. Both were correct and both were removed. ActionCable::Channel::Base#subscribe_to_channel calls reject_subscription, which calls remove_subscription, which calls unsubscribe_from_channel, which stops the periodic timers. A rejected subscription never has a live timer.

Adding #unsubscribed makes a nil container reachable for a different reason, so the guard is reinstated with a comment explaining that reason: the revalidate_access timer (every 1 minute) and validate_user_authorization (every 10 minutes) coincide every 10 minutes, and ActionCable::Server::Worker runs callbacks on a 5-thread pool with no per-connection lock. An authentication failure on one thread can clear the container while a revalidation runs on another.

Worth reviewer attention: the idiomatic guard, unsubscribed?, cannot be used here. The lograge gem (0.11.2) reopens ActionCable::Channel::Base and replaces unsubscribe_from_channel, dropping its @unsubscribed = true assignment, so the flag never flips anywhere in the application. This also silently disables ActionCable's own return if unsubscribed? guard in ActionCable::Channel::Streams. Out of scope here; it needs its own issue.

Files touched

  • app/channels/collaborative_editing/base_channel.rb
  • lib/gitlab/collaborative_editing/document_store.rb
  • spec/channels/collaborative_editing/wiki_page_channel_spec.rb
  • spec/lib/gitlab/collaborative_editing/document_store_spec.rb

References

Screenshots or screen recordings

Not applicable. There is no UI change.

How to set up and validate locally

Nothing to validate through the UI. No client subscribes to the channel yet.

Run the specs:

bundle exec rspec spec/lib/gitlab/collaborative_editing/document_store_spec.rb spec/channels/

145 examples, 0 failures. RuboCop is clean on all four files.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Merge request reports