Skip to content

[REG-10] W0.5 · Async context propagation — workers + recoverAsyncPanic (partial) - #167

Merged
singret merged 1 commit into
mainfrom
reg-10-async-context-propagation
Sep 1, 2026
Merged

singret merged 1 commit into
mainfrom
reg-10-async-context-propagation

Conversation

@singret

@singret singret commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Partial REG-10 — ships the independent half (no blockers). The request-to-goroutine propagation, Redis traceparent handoff, and full alert-to-push trace verification are folded into REG-157, since they need IncidentService/AlertService to carry a context parameter first (currently zero of IncidentService's 30 methods do).

What

Every standalone background worker now opens a real per-iteration root span. recoverAsyncPanic is extended to accept a context and record recovered panics as a linked (not parented) span, ready for REG-157 to feed it a real request context.

Acceptance criteria — what's done, what's deferred

  • recoverAsyncPanic helper extended to accept and propagate a context, opening a linked span
  • Goroutines started from a request use context.WithoutCancel(ctx)deferred to REG-157: needs IncidentService to have a ctx parameter first; all 16 call sites currently pass context.Background() as an honest placeholder
  • Worker jobs that originate from a request carry trace context through Redis — deferred to REG-157: publishResolved hardcodes context.Background() and is called from ResolveIncident, which has no ctx param yet
  • Workers with no originating request start their own root span per job iteration — all 5 standalone workers (EscalationWorker, ShiftNotifier, HolidayWorker, TelemetryWorker's two ticks, PushCleanupWorker)
  • Span links used where a causal-but-not-parent relationship is more accurate than nesting — recoverAsyncPanic's span links to (never parents under) the originating context's span
  • Verify: one alert webhook produces a single connected trace spanning ingest → incident create → Slack post → escalation → push — deferred to REG-157: not achievable until the repository layer (REG-9's own gap) and the goroutine/Redis threading above both land

Why split it this way

The 5 standalone workers already receive ctx in Run(ctx) — a real lifecycle context, just not a per-request one. Opening a root span per tick needed zero new plumbing there. recoverAsyncPanic's 16 call sites live entirely inside IncidentService, which has no ctx parameter on any of its 30 methods — the exact same gap REG-9 found in the repository layer one layer down. Threading it through is a real, cascading interface change, not something to force through this ticket alongside everything else. Extended the helper's signature now (so the link-based span-recording logic is built and tested), left every call site honestly passing context.Background() until REG-157 does the threading.

A design note worth flagging

StartWorkerTick deliberately roots each tick at context.Background(), not the worker's own Run(ctx) lifecycle context. Parenting every tick under Run's context would produce one unbounded "trace" per worker spanning the server's entire uptime — the wrong shape for "what happened on this particular tick" queries, which is what actually matters when debugging why an escalation didn't fire.

Testing

  • StartWorkerTick/EndWorkerTick: fresh trace ID per call (not one shared trace), error status recorded only when an error is given
  • recoverAsyncPanic: logs correctly, never lets a panic escape, no-ops cleanly when nothing panics, records a linked (not parent) span when given a real originating context, and degrades gracefully when given context.Background() (today's actual state at every call site)
  • Tests never touch otel's process-global TracerProvider (the REG-7 finding) — StartWorkerTick takes an injectable tracer; recoverAsyncPanic's tracer lives behind a package-level var tests can swap
  • go test ./... -shuffle=on -count=3 clean on touched packages
  • golangci-lint run ./... clean (0 issues)
  • Live-verified against a real Jaeger collector, not just unit tests: booted the real binary, and on first boot escalation_worker.tick, holiday_worker.tick, shift_notifier.tick, telemetry_worker.heartbeat, and telemetry_worker.fetch_announcements all appeared as real span operations. Watched the escalation worker tick twice (its 30s poll interval) and confirmed the two ticks produced genuinely independent trace IDs — the actual design goal, proven live, not asserted.

…syncPanic

Ships the independent half of REG-10 (the piece with no blockers) and
extends recoverAsyncPanic ahead of REG-157 threading real context into
it. The request-to-goroutine propagation, Redis traceparent handoff,
and full end-to-end alert-to-push trace verification need
IncidentService/AlertService to carry a context parameter first —
that work, and why, is documented in REG-157.

- internal/observability/worker.go: StartWorkerTick/EndWorkerTick open
  a fresh root span per worker-loop iteration, rooted at
  context.Background() rather than the worker's long-lived Run(ctx) —
  parenting every tick under that lifecycle context would produce one
  unbounded 'trace' per worker for the server's entire lifetime, not
  one span per iteration.
- Wired into all 5 standalone workers (EscalationWorker, ShiftNotifier,
  HolidayWorker, TelemetryWorker's two ticks, PushCleanupWorker) —
  each already receives ctx in Run(), so this needed no new plumbing.
- recoverAsyncPanic extended to accept a context.Context and record
  the recovered panic as an error on a span *linked to* (not parented
  by) that context's span — a link, not nesting, because the async
  goroutine commonly outlives the request that spawned it and may
  already have ended.
- All 16 recoverAsyncPanic call sites in incident_service.go updated
  to pass context.Background() — an honest placeholder until REG-157
  threads a real request context through IncidentService's 30 methods
  (currently zero of which take a ctx parameter).

Testing note: tests never touch otel's process-global TracerProvider
(the REG-7 finding — it's a shared, order-dependent singleton, unsafe
to save/restore per-test). StartWorkerTick takes tracer as a parameter
so tests inject an isolated one; recoverAsyncPanic's tracer lives
behind a package-level var (asyncSpanTracer) tests can swap, since its
signature can't grow another parameter without touching all 16 call
sites again.

Live-verified against a real Jaeger collector, not just unit tests:
escalation_worker.tick, holiday_worker.tick, shift_notifier.tick,
telemetry_worker.heartbeat, and telemetry_worker.fetch_announcements
all appear as real span operations on first boot. Watched the
escalation worker tick twice (30s interval) and confirmed the two
ticks produced genuinely independent trace IDs, not one shared trace
for the worker's lifetime.

Part of REG-10 (the independent worker + recoverAsyncPanic signature
half); the request/goroutine/Redis threading half is REG-157
@mintlify

mintlify Bot commented Sep 1, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
fluidify 🟢 Ready View Preview Sep 1, 2026, 7:47 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@singret
singret merged commit 664dc27 into main Sep 1, 2026
5 checks passed
@singret
singret deleted the reg-10-async-context-propagation branch September 1, 2026 07:59
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.

1 participant