Code Review

Overview

Clean, targeted fix: propagate OTel context across Ki.fork boundaries and establish root spans at the three headless entry points (OTLP batch, gRPC, PubSub). Documentation additions are detailed and useful. Core logic is correct.

Issues

1. forkWithCtx — detach token silently dropped

Context.attachContext returns IO (IO ()) — the inner IO () is the detach/restore action. void discards it. Since Ki creates a fresh OS thread per fork (not a pooled executor), the thread-local context disappears when the thread exits, so this is practically harmless today. But it is imprecise and would silently break if the OTel backend changes storage strategy or threads are pooled.

Prefer bracket-style cleanup, which is also shorter since it avoids the nested void:

forkWithCtx scope action = do
  ctx <- liftIO Context.getContext
  Ki.fork scope do
    detach <- liftIO $ Context.attachContext ctx
    action `finally` liftIO detach

(finally from UnliftIO.Exception is already used throughout the codebase.)

2. Telemetry.hs Ki.fork calls not converted — incomplete sweep

bulkInsertOtelLogsAndSpansTF (lines 866–867) still uses plain Ki.fork for its PG and TF writer threads:

pgThread <- Ki.fork scope $ tryAny $ void $ bulkInsertOtelLogsAndSpans records
tfThread <- Ki.fork scope do
  res <- tryAny (retryTimefusion 10 records)

These are the Hasql-intensive paths — the exact calls that produce orphan spans. The PR swept the 5 Ki.fork sites in BackgroundJobs.hs, but these two inner forks in Telemetry.hs are a critical hot path. Without converting them, hasql spans from bulkInsertOtelLogsAndSpans / retryTimefusion will remain orphaned even after the outer BackgroundJobs forks are fixed.

bulkInsertOtelLogsAndSpansTF already has IOE and Ki.StructuredConcurrency constraints — swapping to forkWithCtx is the same one-liner change as in BackgroundJobs.hs.

3. grpcRunBackground — confirm interpreter order

withSpan_ requires Tracing :> es. Since it is nested inside runBackground (which calls runTracing), the constraint is satisfied at runtime — just worth confirming runTracing is an outer interpreter in runBackground so the effect is in scope when withSpan_ runs.

Nits

• liftIO (void $ Context.attachContext ctx) — moving void outside liftIO (void $ liftIO $ Context.attachContext ctx) is more idiomatic; makes the discard explicit at the effectful layer rather than inside IO.

• processList / processMessages now carry 9-item constraint tuples. The project has DB as a constraint-alias precedent; a type BackgroundEs es = (Concurrent :> es, DB es, …) alias in System.Types would help readability as the list grows. Not blocking.

• Docs (incident reports, plans) are high quality and worth keeping.

Summary

Two things worth fixing before merge:

1. Bracket the detach token in forkWithCtx (one extra line, correctness hygiene).
2. Convert the Ki.fork calls in bulkInsertOtelLogsAndSpansTF — those are the highest-value Hasql call sites and their omission leaves the dual-write path producing orphan spans.

Code Review

Good fix overall — the OTel context-propagation problem is real and the forkWithCtx abstraction is the right shape for it. A few things worth addressing:

1. Incomplete sweep — Ki.fork in Pages/LogExplorer/Log.hs

Six Ki.fork calls in Log.hs (lines 738–746) were not converted. These run inside HTTP request handlers, so they start with OTel context from newOpenTelemetryWaiMiddleware — but that context lives on the request-handler ThreadId. Any DB or tracing call inside those child fibers will produce orphan spans for the same reason this PR fixes. The HTTP path isn't "already correct" at the fork level, only at the middleware entry point.

Discord.hs:466-467 is similar.

2. forkWithCtx — use bracket over manual finally

The current form:

prevCtx <- liftIO $ Context.attachContext ctx
action `finally` liftIO (Context.adjustContext (const prevCtx))

is fine, but the acquire/release is split across two statements. A bracket makes the resource lifecycle explicit and slightly harder to accidentally break:

forkWithCtx scope action = do
  ctx <- liftIO Context.getContext
  Ki.fork scope $
    bracket
      (liftIO $ Context.attachContext ctx)
      (\prevCtx -> liftIO $ Context.adjustContext (const prevCtx))
      (const action)

(I note runTracing uses the same non-bracketed style, but runTracing also has a latent gap: if the spanned action throws, endSpan is never called and the modified context leaks — worth fixing separately.)

3. New spans have no attributes

All three new root spans pass []:

withSpan_ "otlp.processList" [] ...
withSpan_ ("otlp.grpc." <> label) [] ...
withSpan_ "pubsub.processMessages" [] ...

Even one or two attributes (e.g. messaging.batch.message_count, tenant/project ID) would make these spans significantly more useful in production. The test plan item "confirm hasql spans show under an otlp.processList parent" is verifiable without attributes, but diagnosing latency outliers or per-tenant issues requires them.

4. No tests for forkWithCtx

forkWithCtx is the core correctness fix. A unit test that forks with a context containing a known span, then asserts the child thread sees that span as current, would prevent regressions. The function is pure enough to test cheaply with runTracing + a no-op TracerProvider.

Minor

• The Tracing :> es constraint is intentionally absent from forkWithCtx (it works via raw IO thread-locals). Worth a one-line note in the Haddock so future readers don't wonder if it's missing.
• TestUtils.hs forks don't need conversion (test infra, no tracing assertions there), but worth an explicit comment that they're intentionally left as Ki.fork.

Code Review: otel context propagation + root spans

Correct fix for orphan spans. hs-opentelemetry-api keys its Context on ThreadId, so every Ki.fork silently loses the parent trace. forkWithCtx solves this by capturing the parent context before forking and re-attaching it in the child fiber. Root-span additions at processList, grpcRunBackground, and processMessages give those entry points something to attach to.

forkWithCtx — correctness: attachContext returns the child thread's previous (empty) context; finally restores it. Correct and exception-safe. Suggest switching to bracket to make the acquire/release structure more explicit and eliminate the intermediate binding — needs bracket added to the UnliftIO.Exception import alongside finally:

forkWithCtx scope action = do
  ctx <- liftIO Context.getContext
  Ki.fork scope $
    bracket
      (liftIO $ Context.attachContext ctx)
      (\prev -> liftIO $ Context.adjustContext (const prev))
      (const action)

Pre-existing exception-safety gap in runTracing: Context.adjustContext and Trace.endSpan after unlift (f sp) never run if the action throws, so spans are silently left open. Pre-dates this PR — forkWithCtx gets it right with finally; runTracing should mirror it. Not a blocker here.

Span naming: OTel semantic conventions use snake_case operation names. "otlp.processList" and "pubsub.processMessages" should be "otlp.process_list" and "pubsub.process_messages". The "otlp.grpc.<label>" names are fine since labels are already traces/logs/metrics.

Constraint propagation: Adding Tracing :> es to processList and processMessages is correct — ATBackgroundCtx includes Tracing (confirmed in System.Types) and runBackground wires runTracing, so callers are unaffected. forkWithCtx intentionally omits the Tracing constraint since it propagates context at the IO level — right design.

Test coverage: The test plan is manual. A small unit test forking under forkWithCtx and asserting the child span's parentSpanId matches the parent would give automated confidence. Worth landing before merge rather than as post-deploy verification.

Summary:

• Core logic: correct
• Child exception safety: finally handles it
• runTracing exception safety: pre-existing gap worth a follow-up
• Span naming: camelCase inconsistency with OTel conventions
• Tests: manual only — add a parentage assertion
• Code size: minimal — 5-line core, 5 fork sites swept cleanly

Code Review PR 414 otel context propagation - Overview: Clean well-scoped fix. The core mechanism forkWithCtx is correct: capture ThreadId-keyed context before the fork, attach it inside the child, and restore on exit. Root spans at processList, grpcRunBackground, and processMessages are the right entry points. Issues: (1) Use HM.findWithDefault instead of fromMaybe lookup at the two ce-type attribute sites in processList and processMessages. (2) Trailing do without dollar in processMessages - should match processList style with ] dollar do. (3) forkWithCtx bracket cleanup is a no-op in practice - GHC RTS does not pool Ki/forkIO threads, each gets a fresh ThreadId, the restore is dead code. (4) const action in bracket - minor style nit, backslash_ -> action is clearer. (5) Discord.hs forks propagate context to plain HTTP calls that do not emit OTel spans - harmless overhead, worth a note. (6) Log.hs forks (6 sites) not mentioned in PR description - Pages/LogExplorer/Log.hs has 6 Ki.fork -> forkWithCtx changes absent from the summary, please add a line to the PR body. (7) length msgs in span attributes is O(n) - low severity but worth noting for high-throughput ingestion paths. (8) Data.Effectful.Hasql import style change is unrelated and belongs in its own commit. Correctness looks good: bracket handles async cancellation correctly, processList short-circuit before withSpan_ is correct, grpcRunBackground withSpan_ wraps correctly, awaitAll scope semantics preserved. Summary: mechanism is sound, main asks before merging are (1) HM.findWithDefault at the two ce-type sites, (2) add Log.hs forks to PR description, (3) clarify or drop the bracket restore comment in forkWithCtx.

Code Review

Overall: Well-targeted fix. forkWithCtx is the right abstraction and the root-span placement is correct. A few things worth addressing before merge.

Correctness

forkWithCtx — looks correct. Capturing ctx before the fork, attaching in the child with bracket, detaching on exit is the right OTel context propagation pattern. The bracket guarantees cleanup even if the action throws.

Missed Ki.fork site in Discord.hs (not mentioned in PR description, but the fix is good).

Potential missed sites: The PR description says "swept the 5 Ki.fork sites in BackgroundJobs.hs", but the diff also sweeps Log.hs (6 sites), Discord.hs (2 sites), and Telemetry.hs (2 sites). Are there any remaining Ki.fork calls elsewhere in the codebase that still orphan spans? Worth a quick:

grep -rn "Ki\.fork" src/

to confirm the sweep is complete.

Code quality

fromMaybe "" for absent span attributes — in two places:

-- OtlpServer.hs and ProcessMessage.hs
("ce.type", OA.toAttribute (fromMaybe "" (HM.lookup "ce-type" attrs)))

Setting an attribute to "" when absent is noise in the trace. Prefer omitting it:

let baseAttrs = [("messaging.batch.message_count", OA.toAttribute (length msgs))]
    ceAttr    = [("ce.type", OA.toAttribute t) | t <- maybeToList (HM.lookup "ce-type" attrs)]
withSpan_ "..." (baseAttrs <> ceAttr) ...

Span name inconsistency — the PR description calls the span "otlp.processList" but the code emits "otlp.process_list". Sibling spans use "pubsub.process_messages" (snake_case) and "otlp.grpc.<label>" (no separator). Pick one convention and apply it to all three entry-point spans.

grpcRunBackground line length — the wrapped call is quite long:

tryAny (runBackground appLogger appCtx tp $ withSpan_ ("otlp.grpc." <> label) [...] task) >>= \case

Extracting the wrapped task into a let binding would improve readability without adding lines.

Data/Effectful/Hasql.hs — HashMap → HM.HashMap is a good consistency cleanup but unrelated to the OTel work; fine to include but could be a separate commit.

Succinctness / packages

forkWithCtx could use <$ or void instead of const — minor:

-- current
bracket acquire release (const action)
-- slightly cleaner (matches local style)
bracket acquire release \_ -> action

Both are fine; this is style-only.

No reimplementation concerns — Context.getContext / attachContext / detachContext are the correct upstream API surface; nothing is being hand-rolled.

Test coverage

No tests added for forkWithCtx. A minimal unit test that:

1. Sets a parent span on the current thread
2. Forks with forkWithCtx
3. Asserts the child's active context matches the parent's

…would prevent silent regressions if the OTel API changes its thread-local semantics. Not blocking, but worth a TODO.

Performance / security

No concerns. Context capture is a single IORef read; bracket overhead is negligible per fork.

Summary

• Sweep completeness: verify no remaining Ki.fork sites with grep -rn "Ki\.fork" src/
• Fix fromMaybe "" attribute noise in both processList and processMessages
• Normalise span naming convention across the three new root spans
• Optional: extract let wrappedTask = withSpan_ ... in grpcRunBackground for readability

Code Review

Summary: Fixes orphan OTel spans from forked fibers by capturing/restoring thread-local context in forkWithCtx, and adds root spans at Kafka/Pubsub/gRPC entry points. The core fix is correct and the approach is sound.

System/Tracing.hs — forkWithCtx

Context management API mismatch. forkWithCtx uses attachContext/detachContext (stack-based) while runTracing's WithSpan case uses adjustContext (const ...) (direct set, bypasses the stack). Mixing these two APIs on the same thread risks corrupting the context stack if both are exercised within a forked child. Since forked threads have fresh ThreadIds this currently doesn't blow up in practice, but the WithSpan impl should ideally also use attach/detach for consistency.

WithSpan lacks exception safety (pre-existing, now more impactful). In runTracing:

Context.adjustContext (const newCtx)
result <- unlift (f sp)          -- throws → lines below never run
Context.adjustContext (const ctx) -- context left dirty
Trace.endSpan sp Nothing          -- span never closed

Now that root spans exist at entry points, leaked/unclosed spans on exception will be more visible in traces. Worth fixing with bracket/onException.

forkWithCtx doesn't require Tracing :> es — this is correct and intentional, but the export placement under -- * Cross-thread context propagation hints users might want a note that it works even with tracing disabled.

Duplicate attribute pattern

Both OtlpServer.hs:246 and ProcessMessage.hs independently write:

("ce.type", OA.toAttribute (fromMaybe "" (HM.lookup "ce-type" attrs)))

This is repeated logic. A small helper (or simply inlining attrs ^. at "ce-type" . non "" if lens is in scope) would reduce the duplication. More importantly, fromMaybe "" emits an empty-string attribute when the key is absent — it's better to omit the attribute entirely when the key doesn't exist:

-- only include if present
let ceTypeAttrs = maybe [] (\v -> [("ce.type", OA.toAttribute v)]) (HM.lookup "ce-type" attrs)
withSpan_ "otlp.process_list" (("messaging.batch.message_count", ...) : ceTypeAttrs) ...

Data/Effectful/Hasql.hs

Renaming import Data.HashMap.Strict (HashMap) → import Data.HashMap.Strict qualified as HM just to use HM.HashMap is a net negative for readability — HashMap as an unqualified type name in a signature is clearer than HM.HashMap. If the goal is import style consistency, that's fine, but HM.HashMap in type signatures is unusual.

Unrelated change

docs/plans/refactor-placeholder-expansion.md deletion (218 lines, -218/+0) is unrelated to OTel context propagation. This should be in a separate commit or PR to keep this one reviewable and revertable cleanly.

Minor

• length msgs in span attributes is O(n) on a list. Fine here since the list is already fully evaluated, but fromIntegral (length msgs) should produce Int which OA.toAttribute handles — just making sure the instance resolves correctly.
• Span name "otlp.process_list" (snake_case) vs "pubsub.process_messages" (snake_case) are consistent with each other, but differ from existing spans like "processList:exception" (camelCase). Not blocking, but worth noting for searchability.

Summary

test

Code Review: otel context propagation

Overview: Clean, well-scoped fix. forkWithCtx is minimal and correct; root spans at processList/processMessages/grpcRunBackground give downstream Hasql spans a parent. The Ki.fork sweep looks comprehensive.

Issues:

1. Misleading comment in forkWithCtx (System/Tracing.hs) -- comment says "The detach token returned by attachContext is restored on exit" but Context.attachContext returns the previous Context, not a detach token. The adjustContext (const prevCtx) release is correct; the comment is not. Suggest: "The previous OTel context saved by attachContext is restored in the bracket release so the per-thread entry does not linger if GHC pools threads."

2. Discord.hs forkWithCtx may be premature -- the two forkWithCtx calls fork getWith opts url (Wreq HTTP). Those child threads emit no OTel spans today so propagation is a no-op. Worth a comment clarifying intentional forward-compat, or defer until HTTP instrumentation lands.

3. Unrelated style change in Hasql.hs -- HashMap import -> qualified HM is a pure style cleanup. Fine to carry, but muddies the diff.

4. Missing Ki.fork audit in Queue.hs and OtlpServer.hs -- PR sweeps BackgroundJobs, Telemetry, Discord, Log. Are there Ki.fork calls in Pkg/Queue.hs (per-topic worker, DLQ fork) or gRPC mkRpcHandler bodies that also need forkWithCtx? A grep before merge is worth it.

5. OA.toAttribute (length msgs) Int vs Int64 -- length returns Int; OTel integer attributes are Int64. Compile-safe if toAttribute has an Int instance, but fromIntegral (length msgs) :: Int64 is more explicit and consistent.

Minor: grpcRunBackground puts withSpan_ inside runBackground effect stack (correct, runTracing is wired there). A one-line comment helps future readers. Span name otlp.process_list in code vs otlp.processList in description -- align for dashboard clarity.

What is good: forkWithCtx is the right abstraction. bracket for context restore is correct. Root span placement is correct. Attribute choice follows OTel conventions. Incident/plan docs are useful.

Verdict: Approve with the comment fix and a Ki.fork grep before merge.

Code Review

Good fix for a real correctness problem — OTel orphan spans from forked threads are genuinely hard to debug. The approach (propagate context at fork time via bracket) is idiomatic and the sweep across all Ki.fork sites is complete. A few things worth addressing:

Bug: runTracing leaks spans on exception (pre-existing, but now more visible)

src/System/Tracing.hs — the WithSpan handler calls Trace.endSpan sp Nothing after unlift (f sp), with no exception safety:

result <- unlift (f sp)
Context.adjustContext (const ctx)
Trace.endSpan sp Nothing   -- never reached if unlift throws
pure result

Every withSpan_ call added in this PR (three new root spans) now silently leaks an unclosed span whenever the body throws. Consider wrapping with onException or bracket:

result <- unlift (f sp) `onException` liftIO (Trace.endSpan sp (Just $ ... Error))

This is not new to this PR, but the PR adds three high-traffic root spans so the blast radius has grown.

fromMaybe "" — use fold instead

Two identical calls in processList and processMessages:

fromMaybe "" (HM.lookup "ce-type" attrs)

Text is a Monoid, so:

fold (HM.lookup "ce-type" attrs)

Does the same thing with less noise, and is consistent with the pattern elsewhere in the codebase.

OA.toAttribute (length msgs) — Int vs Int64

length returns Int. The hs-opentelemetry library's IsAttribute instance for Int exists, but OTel spec integer attributes are 64-bit. At high message volumes length values are small but the type is technically wrong. Prefer:

OA.toAttribute (fromIntegral @Int @Int64 (length msgs))

Same issue in both processList and processMessages.

IOE constraint leaking into getThreadStarterMessage (Discord.hs)

forkWithCtx requires IOE :> es (because Context.getContext is IO), which forces IOE onto getThreadStarterMessage's public signature. The two HTTP fetches here (getWith) don't create OTel spans, so they don't actually need context propagation — the Ki.fork is just parallelizing two HTTP calls, not tracing anything. Consider whether Ki.fork is appropriate here or whether forkWithCtx should only be used where the child action uses tracing-aware code.

Minor: span name in PR description vs code

PR description says "otlp.processList" / "pubsub.processMessages" but the code emits "otlp.process_list" / "pubsub.process_messages". The snake_case form is more conventional for OTel span names, so the code is fine — just update the PR description to match.

Positive notes

• forkWithCtx implementation is correct: captures context before the fork, attaches it in the child via bracket so it's cleaned up even on exception. The bracket (liftIO $ Context.attachContext ctx) (liftIO . Context.detachContext) (const action) pattern is clean.
• Sweeping all 5 Ki.fork sites (including Telemetry.hs) is thorough.
• grpcRunBackground wrapping is a one-liner that composes cleanly with existing runBackground.
• Deleting docs/plans/refactor-placeholder-expansion.md is good housekeeping.
• HM.HashMap qualified import in Data.Effectful.Hasql is a style improvement.

Code Review

Summary

Solid fix for a real OTel correctness problem. hs-opentelemetry-api keys context on ThreadId, so every Ki.fork previously created orphan spans. The approach (capture context in parent, attach/detach in child via bracket) is the correct pattern.

Positives

• forkWithCtx implementation is correct. Context.getContext is called eagerly in the parent thread, attachContext / detachContext via bracket handles the child correctly — including the thread-pool reuse edge case (restores the pre-attach empty context on exit).
• runTracing finally fix is a bonus improvement. The original code leaked open spans on exception; the finally cleanup correctly ends the span and restores context even if the body throws. The simplified Context.adjustContext (Context.insertSpan sp) (applying to current context atomically) is also more correct than the old const newCtx approach.
• Root spans at entry points (otlp.process_list, pubsub.process_messages, otlp.grpc.<label>) are the right call — without them, the propagated child contexts had nowhere to attach.
• Good housekeeping: stale plan doc removed, HashMap import qualified consistently with project style.

Issues

1. Redundant fromIntegral @Int @Int64 (both OtlpServer.hs:245 and ProcessMessage.hs:145)

OA.toAttribute (fromIntegral @Int @Int64 $ length msgs)

hs-opentelemetry-api defines ToAttribute Int (which maps to IntAttribute via fromIntegral internally), so the explicit conversion is unnecessary. The rest of the codebase agrees — e.g. BackgroundJobs.hs:939:

("batch_size", OA.toAttribute $ V.length projectIds)

Both call sites can just write OA.toAttribute (length msgs).

2. Duplicated attribute pair

processList and processMessages both construct the identical attribute list:

[ ("messaging.batch.message_count", OA.toAttribute ...)
, ("ce.type", OA.toAttribute @Text $ fold (HM.lookup "ce-type" attrs))
]

Consider extracting a small helper or at least sharing the definition. Given these two functions sit in different modules this is borderline, but the duplication is verbatim.

3. Log.hs forks get context propagation but no root span

apiLogH now uses forkWithCtx for its 6 parallel DB forks — good. But the handler itself doesn't open a root span. If it's reached via WAI middleware (which already creates a root span), that's fine and intentional. Just worth a comment, or a confirmation that the HTTP path is already covered (PR description says it is — perhaps add an inline note at the forkWithCtx callsite for clarity).

Minor Nits

• withSpan_ "otlp.process_list" [...] $ checkpoint "processList" do — the $ chains make indentation deep. Could flatten with do + let binding or just accept it. Pure style.
• The comment on forkWithCtx mentions "detach token" but Context.detachContext takes the previous context (returned by attachContext), not a token. Slightly misleading — worth rephrasing to "the previous context is restored on exit".

Test Coverage

No automated tests — acknowledged in the PR description with a manual verification plan. Given this is IO/thread-local behaviour, a property test that spawns child fibers and asserts getContext matches the parent's would catch regressions cheaply. Worth a follow-up.

Overall this is a well-reasoned fix with good rollout (sweeping all relevant Ki.fork sites). The fromIntegral cleanup and the doc comment wording are the only changes I'd ask for before merging.

Code Review

Overview

This PR fixes orphan OTel spans in background/async work by: (1) introducing forkWithCtx that snapshots the parent thread's OTel context into child fibers, and (2) adding root spans at Kafka/Pub-Sub (processList, processMessages) and gRPC (grpcRunBackground) entry points so downstream spans have something to attach to. The HTTP path was already handled by newOpenTelemetryWaiMiddleware. The approach is sound.

Issues

Missed Ki.fork site — Discord.hs

src/Pages/Bots/Discord.hs:466–467

Two Ki.fork calls fetching Discord messages were not converted. If this code runs inside a span (e.g. an HTTP handler span), those child threads will produce orphan spans just like the sites this PR fixed. Either convert them to forkWithCtx or leave a comment explaining why they're intentionally left as Ki.fork.

Unrelated file deletion

docs/plans/refactor-placeholder-expansion.md is deleted. It has nothing to do with OTel context propagation. If the refactor is done or abandoned, this belongs in a separate commit (or PR) with its own context.

runTracing: exception safety fix is correct, but Trace.endSpan isn't setting error status

The finally refactor correctly ensures spans are ended even when the body throws. However, when an exception escapes, endSpan is called with Nothing — the span status won't be marked as Error. Consider:

unlift (f sp) `onException` liftIO (Trace.setStatus sp (Error ""))
  `finally` liftIO do
    Context.adjustContext (const ctx)
    Trace.endSpan sp Nothing

Without this, failed operations produce green spans, making errors invisible in the trace UI. This was pre-existing but this PR touches the code, making it the right moment to fix.

Empty attribute for missing ce-type

Both processList and processMessages set:

("ce.type", OA.toAttribute @Text $ fold (HM.lookup "ce-type" attrs))

fold on Maybe Text gives "" when absent, so every batch without a ce-type header gets an empty-string attribute in OTel. This is noisy in backends like Jaeger/Tempo. Prefer omitting the attribute entirely when the key is missing:

foldMap (\v -> [("ce.type", OA.toAttribute @Text v)]) (HM.lookup "ce-type" attrs)
-- spread into the outer attribute list with (<>)

Span name discrepancy between PR description and code

The PR description says "otlp.processList" and "pubsub.processMessages"; the code uses "otlp.process_list" and "pubsub.process_messages". The code is correct (snake_case is OTel convention) — the description is just misleading for anyone searching traces by name later.

Positives

• forkWithCtx is well-implemented: captures context in the parent thread (correct timing), uses bracket for attachContext/detachContext (correct cleanup even on exception), and the comment explains the ThreadId keying problem clearly.
• The runTracing simplification (collapsing 6 lines to Context.adjustContext (Context.insertSpan sp) + finally) is cleaner and strictly correct.
• TestUtils.hs Ki.fork sites are correctly left unconverted — they run under runEff without runTracing, so there's no OTel context to propagate there.
• The Hasql.hs qualified-import cleanup (HashMap → HM.HashMap) is a good consistency fix.

Code Review

Overview

This PR fixes two real problems: orphan OTel spans from background fibers (context is ThreadId-keyed and isn't inherited across Ki.fork), and a span-leak bug in runTracing where exceptions silently swallowed unclosed spans. The approach is sound.

Bug fix in runTracing — worth calling out explicitly

The old WithSpan handler had a latent bug: if unlift (f sp) threw, endSpan was never called and the thread-local context was never restored — spans silently leaked and the parent context stayed polluted. The new withException/finally combo fixes both. This is a correctness fix, not just a cleanup.

-- old: exception → endSpan never called, context never restored
result <- unlift (f sp)
Context.adjustContext (const ctx)
Trace.endSpan sp Nothing

-- new: always closes + restores; marks Error on exception
withException (unlift (f sp)) (\(e :: SomeException) -> Trace.setStatus sp (Error ...))
  `finally` do
    Context.adjustContext (const ctx)
    Trace.endSpan sp Nothing

forkWithCtx — clean and well-scoped

The bracket attachContext detachContext pattern is correct. No Tracing constraint needed (right — context propagation works at the IO level through Context.ThreadLocal). The doc comment explains the invariant clearly.

Suggestions

1. Duplicated attribute-building expression (two sites, same code)

processList and processMessages have identical attribute list construction. Given the project's priority on code size, extract it:

-- in a shared module, or locally in each file
batchSpanAttrs :: Int -> HM.HashMap Text Text -> [(Text, OA.Attribute)]
batchSpanAttrs n attrs =
  ("messaging.batch.message_count", OA.toAttribute n)
    : toList (("ce.type",) . OA.toAttribute @Text <$> HM.lookup "ce-type" attrs)

toList (f <$> mx) on Maybe is also more idiomatic than foldMap (\v -> [f v]) mx for this pattern (same semantics, clearer intent).

2. Span name inconsistency with PR description

The PR description says "otlp.processList" / "pubsub.processMessages" but the code uses "otlp.process_list" / "pubsub.process_messages" (snake_case). The code is internally consistent, but the description is misleading for anyone searching traces by name.

3. grpcRunBackground — span wraps task but runBackground sets up the effectful stack around it

tryAny (runBackground appLogger appCtx tp $ withSpan_ ("otlp.grpc." <> label) [...] task)

withSpan_ is called inside runBackground, which is correct — Tracing is wired by runBackground/runTracing before the task runs. Just confirming the ordering is intentional, not accidental.

4. Discord.hs comment

The comment justifying plain Ki.fork is good. One small note: even without child spans today, propagating context is free and future-proofs the site if instrumentation is added later. Not a blocker — the current reasoning is reasonable given Wreq isn't OTel-instrumented.

No concerns on

• Security: pure observability plumbing
• Test coverage: hard to unit-test OTel wiring; the manual deploy verification plan in the PR is appropriate
• Data.Effectful.Hasql import cleanup (HashMap → HM) is correct style
• Sweep of all 5 Ki.fork sites in BackgroundJobs.hs looks complete