Implement async_job_wrapper functionality in the concurrent_thread_pool extension to allow custom job wrapping - #49
Conversation
…ol extension to allow custom job wrapping
tycooon-review-bot
left a comment
There was a problem hiding this comment.
Code review — Claude Code
Nice follow-up to #46: the generic async_job_wrapper hook has the right shape — invoked on the calling thread at schedule time (exactly what context capture needs), returning the callable that runs on the worker — lib/ no longer references any vendor, and the specs pin the contract well (schedule-time capture, the dataset path, exception propagation, the no-wrapper default). The new README section is accurate, including the pool-type restriction and option defaults.
Two findings, severity-ordered (inline threads):
- P1 — the automatic OpenTelemetry propagation this removes already shipped in 0.19.0 (released 2026-07-08), and the gemspec still says 0.19.0: the release workflow won't publish after merge (a v0.19.0 release already exists), and 0.19.0 users eventually lose span propagation silently. Bump to 0.20.0 in this PR and call out the migration.
- P2 —
async_job_wrapperisn't validated at extension load, so a non-callable value only explodes at the first async call; suggest a one-linerespond_to?(:call)check (+ optionalasync_runmicro-simplification). Self-resolvable.
Also verified: every dataset async method funnels through async_run, so the wrapper covers all scheduling paths; the removed defined?-stub spec correctly went away with the guard it tested; CI is green on 3.3/3.4.
| async_job_class.new(async_thread_executor) do | ||
| OpenTelemetry::Context.with_current(otel_context, &) | ||
| end | ||
| if opts[:async_job_wrapper] |
There was a problem hiding this comment.
P1 · Claude Code — this needs a version bump to ship correctly.
Automatic OTel propagation isn't just on master — it's released: 0.19.0 was published from master on 2026-07-08, right after #46 merged, so this PR removes released behavior while the gemspec still says 0.19.0. Two consequences:
release.ymlskips publishing when the version's release already exists — so after merge, this change sits unreleased on master until a future unrelated bump silently carries it out.- Anyone on 0.19.0 who relies on the automatic propagation upgrades and their async spans silently detach — no error, traces just lose their parent — unless they've added
:async_job_wrapper.
Making it opt-in is the right call (it's what the #46 review asked for), but suggest bumping to 0.20.0 in this PR — repo precedent is a minor bump for behavior changes, done in the PR itself (#46 shipped the 0.19.0 bump) — and calling out the 0.19 → 0.20 migration (add the README's otel_wrapper) in the PR description, since the auto-generated release notes will point here.
| OpenTelemetry::Context.with_current(otel_context, &) | ||
| end | ||
| if opts[:async_job_wrapper] | ||
| wrapped_block = opts[:async_job_wrapper].call(&) |
There was a problem hiding this comment.
P2 · Claude Code — fail fast on a non-callable wrapper.
The boolean/integer options get typecast and validated when the extension loads (typecast_value_boolean(opts[:preempt_async_thread]), the positive-num_async_threads raise), but async_job_wrapper is first touched here — so a non-callable value (a typo, or a string that arrived via connection-URL query params) surfaces as NoMethodError: undefined method 'call' at the first .async call instead of a clear error at load time. A one-liner in extended next to the other opts handling:
if (wrapper = opts[:async_job_wrapper]) && !wrapper.respond_to?(:call)
raise Error, "async_job_wrapper must respond to #call"
endWhile in here, async_run can optionally collapse to a single instantiation site:
def async_run(&block)
block = opts[:async_job_wrapper].call(&block) if opts[:async_job_wrapper]
async_job_class.new(async_thread_executor, &block)
endNit territory — feel free to resolve the thread yourself if you don't think it's worth fixing.
…ncurrent_thread_pool extension
Hi @tycooon!
Following up on your longer-term architectural suggestion from PR #46, I've added a proper
optssurface for context propagation instead of relying on per-vendordefined?checks.I added
opts[:async_job_wrapper]— a generic callable hook that users can configure to propagate ANY context (OpenTelemetry, Sentry, Datadog, etc.) without modifying the gem.This approach gives users full flexibility while preserving sensible defaults. The gem no longer needs a per-vendor
defined?ladder — any future integrations can be handled at the application level viaasync_job_wrapper.This PR removes the automatic OpenTelemetry context propagation that was briefly introduced in
0.19.0.To prevent silent trace detachment and keep the gem vendor-agnostic, context propagation is now opt-in via the new
:async_job_wrapperoption.If you relied on automatic OTel propagation in
0.19.0, you must now explicitly pass the wrapper when connecting: