4.1 batched writer discards accepted broadcasts after a failed insert #97

Summary

Since 4.1.0's batched writer (#87), broadcast returns after enqueueing, before the database insert. If BatchedBroadcaster#flush encounters an insert error, it reports the exception through Rails.error and discards that batch. A subsequent healthy write succeeds, but the earlier accepted broadcast is never retried, including during graceful shutdown.

This is not a request for exactly-once delivery. The concern is the changed failure boundary: in 4.0.2, the adapter's synchronous write could raise to its caller; in 4.1.0, that caller has already returned and cannot react to this failure.

Reproduction

Verified on upstream main bd0442f84b10c44cbd915d919c74fcd698e02b11 (version 4.1.0), Ruby 4.0.6 / Linux. Save the following as test/lib/solid_cable/failed_batch_reproduction_test.rb and run:

bundle exec bin/test test/lib/solid_cable/failed_batch_reproduction_test.rb

This is a deterministic unit-level fault injection: the first database write raises ActiveRecord::ConnectionFailed; later writes are healthy. The two arrays record attempted and successful writes, not browser delivery.

# frozen_string_literal: true
require "test_helper"
require "config_stubs"
require "timeout"

class SolidCable::FailedBatchReproductionTest < ActiveSupport::TestCase
  include ConfigStubs

  test "one failed write is reported but its accepted payload is never retried" do
    attempted = []
    persisted = []
    errors = Queue.new
    attempts = 0

    with_cable_config autotrim: false do
      writer = SolidCable::BatchedBroadcaster.new(batch_size: 1, batch_delay: 0)
      Rails.error.stub(:report, ->(error, **) { errors << error }) do
        write = lambda do |batch|
          attempted.concat(batch.map(&:payload))
          attempts += 1
          raise ActiveRecord::ConnectionFailed, "one injected connection failure" if attempts == 1
          persisted.concat(batch.map(&:payload))
        end
        SolidCable::Message.stub(:broadcast_batch, write) do
          writer.broadcast("channel", "first")
          assert_instance_of ActiveRecord::ConnectionFailed, Timeout.timeout(5) { errors.pop }
          writer.broadcast("channel", "second")
          writer.shutdown
        end
      end
    ensure
      writer&.shutdown
    end

    assert_equal [ "first", "second" ], attempted
    assert_equal [ "second" ], persisted
  end
end

Result: 1 run, 3 assertions, 0 failures/errors. The passing assertions demonstrate the loss, not desired recovery.

We also observed the same discard behavior with a real PostgreSQL CHECK-constraint rejection: the broadcast returned normally, the background insert reported ActiveRecord::CheckViolation, the rejected payload remained absent after removing the constraint and shutting down gracefully, and a later healthy broadcast was stored. That permanent-error probe is not an argument for retrying constraint violations.

Possible direction

Is dropping failed batches the intended delivery contract? The existing reports write errors test covers reporting, so I do not want to assume a guaranteed-delivery contract the project has not made.

A small opt-in synchronous-writing mode could preserve the caller-visible write failure behavior for applications that need it, while retaining default batching. Alternatively, bounded retries of specifically transient write errors could help, but ambiguous commit outcomes, duplicates, permanent errors and shutdown would need an explicit policy.

Happy to contribute a focused PR once the preferred boundary is clear. At minimum, documenting that a successful broadcast return does not establish persistence, and that failed batches are reported and discarded without retry, would make upgrades easier to assess.