Skip to content

fix(consumer): derive controllerCtx from Background, not caller context#368

Merged
kevinlnew merged 1 commit into
mainfrom
kevin.new/consumer-context-fix
Jul 15, 2026
Merged

fix(consumer): derive controllerCtx from Background, not caller context#368
kevinlnew merged 1 commit into
mainfrom
kevin.new/consumer-context-fix

Conversation

@kevinlnew

@kevinlnew kevinlnew commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • consumer.subscribe created controllerCtx from the caller's ctx, which inherits any deadline (e.g. Fx OnStart's 15-second timeout). The consume loop selects on controllerCtx.Done() and exits when the deadline fires, silently dropping all subsequent messages.
  • The subscriber already uses context.Background(); this aligns the consumer to match. Stop() is the shutdown mechanism, not caller context cancellation.

Test plan

  • New test TestConsumer_ConsumeLoopSurvivesCallerDeadline: starts the consumer with a 50ms deadline context, waits for expiry, delivers a message, asserts it is processed
  • All existing consumer tests pass

@kevinlnew kevinlnew force-pushed the kevin.new/consumer-context-fix branch from c8ad4fe to dfc9e0a Compare July 14, 2026 21:45
@kevinlnew kevinlnew changed the base branch from kevin.new/runway-merge-target-terminology to main July 14, 2026 21:45
@CLAassistant

CLAassistant commented Jul 14, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@kevinlnew kevinlnew marked this pull request as ready for review July 14, 2026 21:46
@kevinlnew kevinlnew requested review from a team, behinddwalls and sbalabanov as code owners July 14, 2026 21:46
@kevinlnew kevinlnew force-pushed the kevin.new/consumer-context-fix branch from dfc9e0a to 815e2b5 Compare July 14, 2026 21:49
Comment thread platform/consumer/consumer.go Outdated
@behinddwalls

Copy link
Copy Markdown
Collaborator

Review

Verdict: ✅ LGTM — the fix is correct, the bug is real, and the test is well-constructed. Two minor doc-level suggestions below.

Correctness — verified

  • The bug is real and the fix is complete. Traced the full path: consumeLoop, processPartition, and processDelivery all hang off controllerCtx; with a deadline-bearing parent (e.g. Fx OnStart's 15s timeout), the loop exits at the ctx.Done() select and silently drops all subsequent messages.
  • The claim about the subscriber checks out. The MySQL subscriber's Subscribe creates its supervisor context from context.Background() and ignores the caller ctx for lifecycle, so only the consumer side was affected by the deadline. This fix aligns the two halves.
  • No caller relies on parent-context cancellation for shutdown. All four service wirings (orchestrator, gateway, runway, stovepipe) explicitly call Stop(30000), including the orchestrator's partial-start rollback path. Stop()unsubscribeAllsub.cancelFunc still cancels the loop correctly.
  • No existing test depended on the old behavior — all existing tests shut down via Stop(); their defer cancel() on parent contexts is now inert but harmless.

Test quality — good

  • Deterministic: waits on <-startCtx.Done() rather than sleeping, then synchronizes on the Ack-driven done channel from setupDelivery.
  • Genuinely fails on the old code (the loop exits at the 50ms deadline, so the delivery is never processed).

Suggestions (minor, non-blocking)

  1. Stale doc comment. The consumeLoop shutdown-sequence comment still says "ctx is cancelled (by Stop or parent context)" — the parent-context path no longer exists after this change. Update to just "by Stop()".
  2. Consider context.WithoutCancel(ctx) instead of context.Background(). context.WithCancel(context.WithoutCancel(ctx)) detaches deadline/cancellation identically but preserves context values (tracing baggage, logger metadata) from the caller. Probably no values matter today, and Background() matches the subscriber's convention — judgment call, but WithoutCancel states the intent ("detach lifetime, keep values") more precisely.
  3. Document the narrowed role of Start(ctx). After this change, the ctx passed to Start governs only the initial Subscribe calls, not loop lifetime. A one-line note on the Consumer.Start interface doc would prevent a future caller from expecting cancellation to stop consumption (and leaking goroutines by never calling Stop).

Risk

Low. The only behavioral change is that consume loops no longer die on parent-context cancellation — which is exactly the bug being fixed, and Stop() is universally used for shutdown. The change makes the consumer's contract ("Stop() is the shutdown mechanism") actually hold.

@kevinlnew kevinlnew force-pushed the kevin.new/consumer-context-fix branch from 815e2b5 to acf4e77 Compare July 14, 2026 22:13
consumer.subscribe created controllerCtx from the caller's ctx, which
inherits any deadline (e.g. Fx OnStart's 15-second timeout). The consume
loop selects on controllerCtx.Done() and exits when the deadline fires,
silently dropping all subsequent messages. The subscriber already uses
context.Background(); this aligns the consumer to match.
@kevinlnew kevinlnew force-pushed the kevin.new/consumer-context-fix branch from acf4e77 to 345a856 Compare July 15, 2026 04:55
@kevinlnew kevinlnew added this pull request to the merge queue Jul 15, 2026
Merged via the queue into main with commit 5063ddd Jul 15, 2026
15 checks passed
@kevinlnew kevinlnew deployed to stack-rebase July 15, 2026 05:03 — with GitHub Actions Active
@behinddwalls behinddwalls deleted the kevin.new/consumer-context-fix branch July 15, 2026 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants