fix(backend): close SSE reconcile race that could permanently drop a user's event triggers#32
Open
LukasHirt wants to merge 1 commit into
Open
fix(backend): close SSE reconcile race that could permanently drop a user's event triggers#32LukasHirt wants to merge 1 commit into
LukasHirt wants to merge 1 commit into
Conversation
…user's event triggers Two related bugs in the per-user SSE consumer manager (pkg/sse/manager.go) combined to cause an up-to-30s or, in the worst case, permanent gap between a workflow's event trigger becoming active and its uploads actually firing: 1. consumeForUser inserted its m.active[userID] entry *before* calling GetAutomation, but never removed it if GetAutomation failed (e.g. the user has an event trigger but hasn't connected automation yet, or their app-password was revoked). Because the entry never left m.active, every later reconcile() pass treated that user as "already active" and never retried the consumer — even once automation was connected. Fixed by having consumeForUser remove its own entry on that one failure path. The other return paths (ctx cancelled by reconcile's removal logic or by shutdown) already have their entries removed by the canceller, so self-removal there is skipped to avoid racing with a fresher entry a later reconcile pass may have already installed for the same userID — guarded with a per-entry generation id (activeConsumer.id) so the cleanup only ever deletes the entry it actually owns. 2. reconcile() only ran on a fixed 30s ticker, so a workflow's event trigger being added/enabled, or a user's automation being connected, could miss an upload that happened in the blind window before the next tick opened that user's SSE stream. Added Manager.Kick(), a non-blocking, drop-if-pending signal that Start's select loop also listens on, and wired it into the two write paths that can affect the wanted-consumer set: WorkflowsHandler.syncTriggerIndex (workflow create/update) and automation.Service.Connect. Tests added to pkg/sse/manager_test.go under TDD, watched failing against the old code before the fix: - TestReconcileRetriesConsumerAfterTransientGetAutomationFailure - TestKickTriggersImmediateReconcile Full backend suite (go build, go vet, go test ./... -race) passes. Signed-off-by: Lukas Hirt <info@hirt.cz>
LukasHirt
force-pushed
the
fix/sse-reconcile-race
branch
from
July 24, 2026 21:08
ac26ece to
f714fe8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The per-user SSE consumer manager (
backend/pkg/sse/manager.go) had two related timing/race bugs that could cause event-triggered workflows to silently never fire uploads:Dead consumer never retried.
consumeForUserinserted itsm.active[userID]entry before callingGetAutomation, but never removed it ifGetAutomationfailed (e.g. the user has an event trigger but hasn't connected automation yet, or their app-password was revoked). Since the entry never leftm.active, every laterreconcile()pass saw the user as "already active" and never retried the consumer, even after automation was later connected — the consumer was dead forever.Fix:
consumeForUsernow removes its ownm.activeentry when it exits due to aGetAutomationfailure. All the other return paths happen becausectxwas cancelled byreconcile's removal logic or by shutdown — those already remove the entry themselves, soconsumeForUsermust not also delete it there (that would race with a fresher entry a laterreconcilepass may have already installed for the sameuserID). This is guarded with a small per-entry generation id (activeConsumer.id) so self-cleanup only ever deletes the entry it actually owns.Up-to-30s blind window.
reconcile()only ran on a fixed 30s ticker, so a workflow's event trigger being created/enabled, or a user's automation being connected, could miss an upload that happened before the next tick opened that user's SSE stream.Fix: added
Manager.Kick()— a non-blocking, drop-if-pending signal — thatStart's select loop also listens on alongside the ticker. Wired it into the two write paths that can affect the wanted-consumer set:WorkflowsHandler.syncTriggerIndex(workflow create/update, inpkg/service/workflows.go) andautomation.Service.Connect(inpkg/automation/automation.go). Both take an optionalKick()-satisfying interface so they stay decoupled from the concrete*sse.Managertype;backend/pkg/command/server.gowas reordered to construct thesse.Managerfirst and wire it into both.Test plan
TDD: wrote the following tests against the old code first and watched them fail for the right reason, then implemented the fix and watched them pass.
TestReconcileRetriesConsumerAfterTransientGetAutomationFailure(new,pkg/sse/manager_test.go) — reproduces bug chore(deps): Bump actions/setup-node from 6.3.0 to 7.0.0 #1: automation missing on first reconcile, connected later, asserts the consumer is retried.TestKickTriggersImmediateReconcile(new,pkg/sse/manager_test.go) — reproduces bug chore(deps): Bump actions/checkout from 6.0.2 to 7.0.0 #2: uses a "warm-up" trigger to proveStart's select loop is actually running (avoiding a scheduling race), then assertsKick()picks up a newly-added trigger well within the (1-hour, in the test) ticker interval.pkg/ssetests continue to pass, includingTestReconcileStartsAndStopsConsumersAsTriggersChange(removal-path coverage).Ran locally, matching CI's backend job exactly:
go build ./...go vet ./...go test ./...go test ./... -race(extra, not in CI but run to validate the new mutex/generation-id logic)go build -tags=e2e ./.../go vet -tags=e2e ./...(constructor signature changes didn't break e2e compilation)No unrelated code touched — the share-file action node commits on this branch's ancestry were left untouched; this PR only modifies
pkg/sse/manager.go(+tests),pkg/service/workflows.go,pkg/automation/automation.go, andpkg/command/server.gowiring.