feat: Push events on their own goroutines instead of blocking the drain

[keelerm84]

Summary

eventLoop drained a batch and pushed it inline, so the push held up the next drain. With
HTTP_TIMEOUT at its 30s default and two attempts a second apart, one degraded cycle could
keep the loop for around two minutes:

Phase	Worst case
Drain from Salesforce	30s
Push attempt 1	30s
Retry delay	1s
Push attempt 2	30s
Interval wait	30s
Drain to drain	~121s

The interval is time.After measured after the work rather than a ticker, so the cycle
period is always work plus 30s. Every second the push holds is a second EventData__c is
not drained -- and the default EventSink caps that table at 1000 rows and then stops
recording silently, so a slow events endpoint turns into dropped events in the org.

The push now runs on its own goroutine, bounded at MAX_EVENT_FLUSH_WORKERS = 5, matching
maxFlushWorkers in go-sdk-events. The retry sequence moves with it unchanged.

The slot is taken before the drain, and that ordering is the design

EventREST.prepareEvents deletes the rows as it hands them over, so a batch drained with no
free push is a batch with nowhere to go. A non-blocking hand-off after draining would leave
only "drop the batch" as the fallback -- real data loss.

Taking the slot first means that when all five are busy the bridge declines to drain at all,
and this cycle's events stay in EventData__c. That is the same choice the other SDKs make:
triggerFlush takes its default branch and does not clear the outbox, pinned there by
TestEventsAreKeptInBufferIfAllFlushWorkersAreBusy. The bridge has no in-memory outbox --
the org's table is the outbox.

TestDrainIsSkippedWhenEveryFlushSlotIsBusy asserts on the drain count rather than the
push count for exactly this reason: a bridge that drained and then dropped the batch looks
identical from the push side.

Two consequences that needed handling

A push goroutine cannot return from eventLoop. A rejected SDK key is reported through
flushFatal and the loop answers with it, preserving both the existing message and the
non-zero exit from #57. It may surface up to one poll interval later than before, since the
loop notices at its next wake.

A push still running at shutdown holds events Salesforce has already deleted. The loop
waits FLUSH_DRAIN_GRACE (5s) for in-flight pushes and logs how many it abandoned. The wait
is capped rather than open-ended because HTTP_TIMEOUT alone allows 30s per attempt, and a
shutdown that takes that long is indistinguishable from a hang.

Two test-side problems the change forced

newEventPushBridge set the poll interval and the retry delay both to one millisecond. That
was harmless while the push was inline, because the loop could not reach the next drain until
the push finished. Now they are concurrent, so the drain that cancels the bridge could land
inside a retry delay and abandon it -- the existing retry tests went intermittent under
-count=20. The interval is now well clear of the delay, keeping the shape production has at
1s against 30s.

captureLog handed back a bytes.Buffer that a test could read while the loop was still
writing to it. log.Logger serialises its own writes, so this was safe until a push ran on
another goroutine; the race detector caught it immediately after. It is mutex-guarded now,
which also closes the hazard for the existing tests that assert on log content.

Verification

Five new tests, each checked by mutation:

• draining before taking a slot -> 19 extra drains, batches deleted with nowhere to go
• never releasing a slot -> the loop goes silent permanently
• skipping the shutdown wait -> an in-flight push is abandoned

Slot release is covered on all four push outcomes (success, exhausted retries,
non-recoverable status, dropped connection) by driving more batches through than there are
slots, since a leak is invisible below that threshold.

Two of the new tests originally hung on their failure path rather than reporting -- blocked
handlers deadlocking httptest.Server.Close -- so their gates are now released before the
server closes and a real failure reports in milliseconds instead of timing out.

Green on Go 1.15 (CI) and 1.26, under -race -count=10, and -shuffle=on.

Not addressed

Delivery is still not durable. Salesforce deletes on read, so a batch that exhausts both
attempts is gone. This changes where a batch waits, not whether it can be lost. Batches can
also reach LaunchDarkly out of order now, which is acceptable: the SDKs already flush in
parallel, events carry their own timestamps, and each batch keeps its own payload ID so
per-batch retry dedup is unaffected.

---

Note

Overview
Event delivery no longer blocks Salesforce drains. eventLoop used to push each batch to LaunchDarkly inline, so a slow events API could stall the next drain for minutes and back up EventData__c. Pushes now run in flushEvents on separate goroutines, capped at MAX_EVENT_FLUSH_WORKERS (5) via a flushSlots token pool, aligned with other LaunchDarkly server SDKs.

Backpressure avoids silent batch loss. Because drains delete rows in Salesforce, the bridge acquires a flush slot before draining. When all workers are busy it skips the drain and leaves events in the org (same pattern as SDK outbox behavior), with logging when it declines.

Shutdown and fatal errors from workers. eventLoop awaitFlushes for up to FLUSH_DRAIN_GRACE (5s) so in-flight pushes can finish after cancel. Push goroutines report 401/403 and similar stop-the-daemon failures through flushFatal so eventLoop still exits with the right error.

Tests and harness fixes. New flush_workers_test.go covers skip-drain under saturation, slot release on all outcomes, non-blocking drains, distinct payload IDs under concurrency, and shutdown waiting. Retry tests use a poll interval well above the retry delay to avoid races; captureLog uses a mutex-backed buffer for concurrent push logging.

^{Reviewed by Cursor Bugbot for commit 2d5040b. Bugbot is set up for automated code reviews on this repo. Configure here.}

[kinyoklion]

Suggested change

	// batch drained with nowhere to send it is a batch lost. The bridge therefore declines to
	// drain at all, which leaves the events in the org -- the same choice the other
	// LaunchDarkly SDKs make by keeping events in their outbox when every flush worker is
	// busy. TestDrainIsSkippedWhenEveryFlushSlotIsBusy is that assertion.
	// batch drained with nowhere to send it is a batch lost. The bridge therefore declines to
	// drain at all, which leaves the events in the org.
	// TestDrainIsSkippedWhenEveryFlushSlotIsBusy is that assertion.

I wouldn't say the situation is directly analogous. They would leave the buffer alone, but the buffer would be bounded. So eventually they just stop processing events.

Is there any protection about how many events will accumulate if something goes wrong with the flushers?

[kinyoklion]

Typically, for events, we have a locally recoverable function which is a bit different than the standard recoverable statuses.

[kinyoklion]

So that we don't retry a 413 with the same too large payload.