Eagerly initialize last_id to avoid skipping messages

[xrav3nz]

What does this do?

This change eagerly initializes @last_id to avoid skipping messages.

Why?

Currently, @last_id is lazily initialized as MAX(id) on the listener thread's first call to broadcast_messages.

solid_cable/lib/action_cable/subscription_adapter/solid_cable.rb

Lines 119 to 121 in af98678

	def last_id
	@last_id ||= last_message_id
	end

However, if a broadcast inserts a message between a subscription and that first call, MAX(id) returns a value that includes that inserted message message, causing the query WHERE id > last_id to miss it entirely.

For example:

Main thread                           Listener thread
──────────                            ───────────────
subscribe()
  Listener.new → starts thread →      thread starts, enters listen loop
  add_channel:
    channels["test"] = MAX(id) = 5
    event_loop.post(on_success)
                                      interruptible { executor.run! }
on_success fires → subscribed.set
subscribed.wait returns
                                      ↑ still hasn't reached broadcast_messages
broadcast("hello") → inserts id=6
                                      broadcast_messages:
                                        @last_id ||= MAX(id) → 6  ← INCLUDES THE MESSAGE
                                        broadcastable(["test"], 6)
                                          → WHERE id > 6 → nothing returned!
                                        message 6 is SKIPPED

The per-channel cursor (channels["test"] = 5) doesn't help, because it's only checked as a secondary filter inside the loop over query results, which already excluded the message.

solid_cable/lib/action_cable/subscription_adapter/solid_cable.rb

Lines 134 to 148 in af98678

	::SolidCable::Message.
	broadcastable(current_channels.keys, last_id).
	each do |message|
	should_broadcast_message = false
	channels.compute_if_present(message.channel) do |channel_last_id|
	break if channel_last_id >= message.id
	should_broadcast_message = true
	message.id
	end
	broadcast(message.channel, message.payload) if should_broadcast_message
	self.last_id = message.id
	end
	end

How to repro?

This is evident in the existing test suite, where sleep is required after the first subscribe to give the listener thread time to complete the first poll cycle and evaluate @last_id to a pre-broadcast max.

solid_cable/test/lib/action_cable/subscription_adapter/solid_cable_test.rb

Line 215 in af98678

sleep WAIT_WHEN_EXPECTING_EVENT

Removing this sleep causes tests to fail without this change.

What do I NOT like about this change?

There is now a DB read in the constructor... Though:

• Listeners are already lazily initialized so this is not a boot-time issue
• DB connection is ready: both the listener thread & #subscribe call perform DB read/writes immediately

---

Thanks and open to suggestions!

[xrav3nz]

I can't think of a better way to add a regression test for the race condition. This intentionally does NOT use subscribe_as_queue, to make the whole process as transparent as possible. Happy to change / remove if it feels redundant.

[npezza93]

Yea lets go ahead and remove this i think the removal of line 215 should cover us.

[xrav3nz]

Removed and thanks!

[npezza93]

Thanks for this. Ill take a look this weekend

[npezza93]

Otherwise looks good to me

[npezza93]

Yea lets go ahead and remove this i think the removal of line 215 should cover us.