Skip to content

Implement async_job_wrapper functionality in the concurrent_thread_pool extension to allow custom job wrapping - #49

Open
avoleba wants to merge 2 commits into
umbrellio:masterfrom
avoleba:implement-async-job-wrapper
Open

Implement async_job_wrapper functionality in the concurrent_thread_pool extension to allow custom job wrapping#49
avoleba wants to merge 2 commits into
umbrellio:masterfrom
avoleba:implement-async-job-wrapper

Conversation

@avoleba

@avoleba avoleba commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Hi @tycooon!

Following up on your longer-term architectural suggestion from PR #46, I've added a proper opts surface for context propagation instead of relying on per-vendor defined? 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 via async_job_wrapper.

⚠️ Migration Note (0.19.0 → 0.20.0)

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_wrapper option.

If you relied on automatic OTel propagation in 0.19.0, you must now explicitly pass the wrapper when connecting:

otel_wrapper = lambda do |&block|
  ctx = OpenTelemetry::Context.current
  if ctx.equal?(OpenTelemetry::Context::ROOT)
    block
  else
    -> { OpenTelemetry::Context.with_current(ctx, &block) }
  end
end

DB = Sequel.connect(ENV["DATABASE_URL"], async_job_wrapper: otel_wrapper)
DB.extension(:concurrent_thread_pool)

Let me know if it looks ok now after refactoring!

@tycooon-review-bot tycooon-review-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
  • P2async_job_wrapper isn't validated at extension load, so a non-callable value only explodes at the first async call; suggest a one-line respond_to?(:call) check (+ optional async_run micro-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]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. release.yml skips 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.
  2. 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(&)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
end

While 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)
end

Nit territory — feel free to resolve the thread yourself if you don't think it's worth fixing.

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.

2 participants