Drop unused **opts from Process.spawn wrapper signature by p-datadog · Pull Request #5774 · DataDog/dd-trace-rb

p-datadog · 2026-05-18T20:14:30Z

What does this PR do?

Restructures Datadog::Core::Utils::SpawnMonkeyPatch::ProcessSpawnPatch#spawn to use a per-Ruby-version signature (matching the existing pattern in lib/datadog/core/utils/forking.rb):

Ruby 3+: def spawn(*args, **opts) — unchanged from master. With bare super, kwargs at the call site flow to Process.spawn as kwargs.
Ruby 2.5/2.6/2.7: def spawn(*args) — drops **opts. This is the bug fix.

Motivation:

Master's wrapper signature was def spawn(*args, **opts) for every Ruby. The method body never reads opts. On Ruby 3+ the signature works fine — kwargs flow to super as kwargs and Process.spawn accepts either form. On Ruby 2.5/2.6/2.7 the **opts actively breaks one call shape:

Caller passes an options Hash with mixed Symbol + Integer keys — e.g. childprocess's options[writer.fileno] = :close on duplex pipes.
Ruby 2.x auto-extraction siphons the Symbol-keyed entries into **opts while leaving the Integer-keyed entries behind in the trailing positional Hash.
Bare super then forwards the broken split. Process.spawn interprets the partially-stripped Hash as a command argument and raises TypeError: no implicit conversion of Hash into String.

Ruby 3+ removed positional-Hash auto-extraction into kwargs, so this bug is invisible there.

Dropping **opts on Ruby 2.x keeps the entire options Hash positional regardless of key shape, and Process.spawn receives it intact. Keeping **opts on Ruby 3+ preserves the original kwargs-forwarding behaviour (see #5774 (review)).

This is independent from the constant-shadow bug fixed in #5773: that one fires whenever the caller passes an env-Hash (any Ruby), this one fires whenever the caller passes a mixed-keys options Hash without an env-Hash (Ruby 2.x only). With both PRs landed, both shapes work on every Ruby.

Change log entry

Yes. Fix TypeError: no implicit conversion of Hash into String raised by Process.spawn on Ruby 2.5/2.6/2.7 when the caller passes an options Hash with mixed Symbol + Integer keys (e.g. childprocess on duplex pipes).

Additional Notes:

One of two independent splits from #5634:

#5773 — constant-shadow fix (Hash → ::Hash) for #5621, all Rubies
this PR — per-version split to fix the Ruby 2.x mixed-keys options Hash bug

Marco's #5634 bundles these along with a lineage_envs_provider → env_provider rename, an idempotency guard in apply!, doc edits, and a execute_in_fork doc tweak. I deliberately did not split those out — the rename is cosmetic and breaks the API, the idempotency guard has no observable effect (Module#prepend is already idempotent), and the doc edits are unrelated.

How to test the change?

Three regression specs in spec/datadog/core/utils/spawn_monkey_patch_spec.rb under describe 'option-syntax compatibility':

kwargs-syntax options work — Process.spawn(cmd, kw: value)
positional Hash options work — opts = {...}; Process.spawn(cmd, opts)
positional options Hash with mixed Symbol + Integer keys (childprocess close-on-exec form) — the falsifiable test for the bug above. Triggers TypeError on master against Ruby 2.5/2.6/2.7; passes on every Ruby with the per-version split.

The third test does not pass an env-Hash argument, so it is independent of the constant-shadow fix in #5773 — confirmed by reproducing on master's lib with ::Hash still bare.

🤖 Generated with Claude Code

datadog-datadog-prod-us1-2 · 2026-05-18T20:34:55Z

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
• Patch Coverage: 26.32%
• Overall Coverage: 97.07% (-0.02%)

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: 6677dcf | Docs | Datadog PR Page | Give us feedback!}

pr-commenter · 2026-05-18T20:39:23Z

Benchmarks

Benchmark execution time: 2026-05-19 18:16:21

Comparing candidate commit 6677dcf in PR branch claude/spawn-wrapper-drop-unused-opts with baseline commit f0cfa4f in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 46 metrics, 0 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

🟩 = significantly better candidate vs. baseline
🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

Two issues caught by CI: - standard/lint: hash-literal spacing and `Lint/ConstantDefinitionInBlock`. Auto-fixed spacing via `rake standard:fix`; converted the three describe- block constants (LINEAGE_VAR / LINEAGE_VAL / PROBE_CMD) into `let` blocks. - Ruby 2.5/2.6/2.7: the `options_with_integer_fd_keys` test failed because the wrapper's `def spawn(*args, **opts)` partially extracts a positional Hash with mixed Symbol + Integer keys into `**opts` on Ruby 2.x. That is a distinct bug from the constant shadow this PR fixes — it is the bug that PR #5774 (drop `**opts`) addresses. The test (and its proof) belong with that PR. Removed from this spec with a comment pointing to #5774.

dd-octo-sts · 2026-05-18T20:52:27Z

Typing analysis

Note: Ignored files are excluded from the next sections.

`steep:ignore` comments

This PR introduces 1 steep:ignore comment.

steep:ignore comments (+1-0)

❌ Introduced:

lib/datadog/core/utils/spawn_monkey_patch.rb:24

Untyped methods

This PR introduces 1 untyped method, and clears 1 untyped method.

Untyped methods (+1-1)

❌ Introduced:

sig/datadog/core/utils/spawn_monkey_patch.rbs:11
└── def spawn: (*untyped args) -> untyped

✅ Cleared:

sig/datadog/core/utils/spawn_monkey_patch.rbs:11
└── def spawn: (*untyped args, **untyped opts) -> untyped

If you believe a method or an attribute is rightfully untyped or partially typed, you can add # untyped:accept on the line before the definition to remove it from the stats.

eregon · 2026-05-19T09:26:16Z

On Ruby 3+ this is cosmetic (the wrapper body never reads opts and bare super forwards both positional and keyword args).

I call AI hallucination!
It's not cosmetic, it passes received kwargs as kwargs (on Ruby 3+).
The change here passes caller kwargs (spawn('ls', unsetenv_others: true)) as positional args to Process.spawn.
It so happens that Process.spawn on CRuby does not care about kwargs currently but that could maybe change.
It's somewhat unlikely given the signature in the docs is:

  spawn([env, ] command_line, options = {}) -> pid
  spawn([env, ] exe_path, *args, options  = {}) -> pid

Although OTOH it's frequent to have docs bugs.

I think at least in theory the safest would be to use ruby2_keywords for Ruby 2.7, that preserves the kwargs.
But on 2.5 & 2.6 there is nothing we can do, and indeed *args, **kwargs is harmful because of splitting between Symbol and non-Symbol keys.

TLDR: I think the fix is good, but it's not only cosmetic, it changes if the callee receives kwargs or not, it happens that Process.spawn doesn't care currently and that there is no other solution anyway on 2.5/2.6.

lloeki · 2026-05-19T12:18:12Z

It's not cosmetic, it passes received kwargs as kwargs (on Ruby 3+).

Agreed. There are even varying behaviours depending on the call arguments, which can have subtle changes across Ruby versions that don't usually matter for 99% of Ruby code out there but suddenly do when one does this kind of generic delegation. Using ruby2_keywords piles even more subtlety upon it (as it also had fixes across versions).

Best bet is to split the defs per version and simply rely on semantics as the versions expect:

https://github.com/DataDog/graft-rb/blob/53eee0053480184e22dcbf24de36ecfc457a8144/lib/graft/callback.rb#L3-L7

Address review feedback (#5774): keep `def spawn(*args, **opts)` on Ruby 3+ so caller kwargs continue flowing to `Process.spawn` as kwargs through `super`; use `def spawn(*args)` on Ruby 2.5/2.6/2.7 to avoid the mixed-keys options Hash split that raises `TypeError` there. Matches the existing per-version split pattern in `lib/datadog/core/utils/forking.rb`. - lib/datadog/core/utils/spawn_monkey_patch.rb: per-version conditional - sig/datadog/core/utils/spawn_monkey_patch.rbs: stays at `(*untyped args)` (matches forking.rbs; narrow sig, ignored on Ruby 3+ branch) All 5 specs in spec/datadog/core/utils/spawn_monkey_patch_spec.rb pass. Steep clean. Co-Authored-By: Claude <[email protected]>

p-datadog · 2026-05-19T14:14:52Z

You're right — calling it cosmetic was wrong. With *args, **opts and bare super, kwargs at the call site flow to Process.spawn as kwargs on Ruby 3+; dropping **opts collapses them into the trailing positional Hash inside *args. Process.spawn happens not to differentiate today, but the forwarding form did change.

Restructured in 6f93283 as a per-Ruby-version split (matching the pattern already in lib/datadog/core/utils/forking.rb:48-54): Ruby 3+ keeps def spawn(*args, **opts) so kwargs flow as kwargs through super; Ruby 2.5/2.6/2.7 uses def spawn(*args). PR description updated to drop the "cosmetic" framing.

p-datadog · 2026-05-19T14:14:58Z

Thanks for the graft-rb pointer — applied the per-version split in 6f93283. Ruby 3+ keeps def spawn(*args, **opts) so kwargs flow as kwargs; Ruby 2.5/2.6/2.7 uses bare def spawn(*args) so the mixed-keys options Hash stays positional. Same shape as lib/datadog/core/utils/forking.rb:48-54 already uses in this directory.

eregon · 2026-05-19T15:42:45Z

+            def spawn(*args)
+              args.replace(SpawnMonkeyPatch.inject_lineage_envs(args))
+              super
+            end


Suggested change

def spawn(*args)

args.replace(SpawnMonkeyPatch.inject_lineage_envs(args))

super

end

def spawn(*args)

args.replace(SpawnMonkeyPatch.inject_lineage_envs(args))

super

end

ruby2_keywords :spawn if respond_to?(:ruby2_keywords, true)

On 2.7 (and only 2.7) this could be problematic:
https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/#other-minor-changes-empty-hash
Though in this case an empty Hash disappearing should be no problem since the semantics of empty Hash is the same as passing no Hash for spawn.

But still, I think it's good to be consistent and use the correct pattern everywhere,
so I would suggest adding that change.
https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html documents that pattern, here we also use *args, **kwargs for 3+ which is fine and avoids using ruby2_keywords longer than necessary.

Another point is just (*args) without ruby2_keywords warns on 2.7 and we wouldn't want to emit extra deprecation warnings for users (only visible if they enable deprecation warnings but that's common because test harnesses often enable them).

We should be able to close this PR in favor of #5634. I made this change here: https://github.com/DataDog/dd-trace-rb/pull/5634/changes#r3268075567.

#5634 does def spawn(*args) which is mostly fine and fixes the Hash splitting bug, except it will convert any kwargs to positional args.
Process.spawn currently doesn't expect kwargs so it works fine, but it could change: #5774 (comment)
There is still a potential issue if another monkey-patch of Process.spawn would expect kwargs and our monkey-patch is earlier in the call chain: then we lose kwargs and pass them as positional args and potentially break the other monkey patch.
I'll make a PR for that.

…ns Hash bug Master's wrapper signature `def spawn(*args, **opts)` is unchanged from before. On Ruby 3+ this works fine. On Ruby 2.5/2.6/2.7 `**opts` auto-extracts the Symbol-keyed entries from a mixed-keys positional options Hash (e.g. childprocess's `options[writer.fileno] = :close` on duplex pipes), splitting the Hash and raising `TypeError: no implicit conversion of Hash into String` inside `Process.spawn`. Use a per-Ruby-version signature (matching `lib/datadog/core/utils/forking.rb`): - Ruby 3+: `def spawn(*args, **opts)` — preserves caller kwargs as kwargs through bare `super` - Ruby 2.5/2.6/2.7: `def spawn(*args)` — keeps the options Hash positional and intact regardless of key shape Adds a regression test (`spawn(cmd, options_hash_with_mixed_symbol_and_integer_keys)`) under the existing `Process.spawn argument forms (issue #5621 regression)` describe block, using the shared `run_spawn` helper added in #5773. The test triggers `TypeError` on master against Ruby 2.5/2.6/2.7 and passes on every Ruby with the per-version split. No env-Hash is passed, so the test is independent of the constant-shadow fix in #5773. Sig stays at `(*untyped args)` to match the 2.x form (RBS doesn't support runtime conditionals); the Ruby 3+ branch uses an inline `# steep:ignore DifferentMethodParameterKind` directive. All 13 spec/datadog/core/utils/spawn_monkey_patch_spec.rb examples pass. Steep clean. Co-Authored-By: Claude <[email protected]>

…e-on-exec shape 5634 fixes the Ruby 2.5/2.6/2.7 mixed-keys bug by dropping `**opts` from the wrapper signature, but the test suite has no regression coverage for this specific call shape — only a NOTE block deferring to PR DataDog#5774. Add an `it` block under the existing `Process.spawn argument forms (issue DataDog#5621 regression)` describe that exercises the exact childprocess close-on-exec form: an options Hash with `IO.pipe` filenos (Integer keys) mixed with Symbol keys (`:pgroup => true`, `fileno => :close`). On Ruby 2.5/2.6/2.7 this triggers `TypeError: no implicit conversion of Hash into String` against the pre-fix wrapper; with `**opts` dropped, the options Hash stays positional and intact across all supported Rubies. Uses the shared `run_spawn` helper and `env_provider:` API already established in the surrounding describe block. Replaces the now-stale NOTE that pointed at PR DataDog#5774 — 5634 fixes the same bug, so no deferral is needed. 20 examples pass locally. Co-Authored-By: Claude <[email protected]>

p-datadog requested a review from a team as a code owner May 18, 2026 20:14

p-datadog added the AI Generated Largely based on code generated by an AI or LLM. This label is the same across all dd-trace-* repos label May 18, 2026

dd-octo-sts Bot added the core Involves Datadog core libraries label May 18, 2026

vpellan approved these changes May 19, 2026

View reviewed changes

eregon reviewed May 19, 2026

View reviewed changes

mabdinur mentioned this pull request May 19, 2026

Fix process spawn #5634

Merged

p-datadog force-pushed the claude/spawn-wrapper-drop-unused-opts branch from 6f93283 to 6677dcf Compare May 19, 2026 17:50

This was referenced May 19, 2026

[snapshot] Pre-rebase state of #5774 (drop **opts + per-version split) #5779

Closed

Add mixed Symbol + Integer keys regression test for #5634 #5780

Merged

p-datadog closed this May 19, 2026

Uh oh!

Conversation

p-datadog commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

datadog-datadog-prod-us1-2 Bot commented May 18, 2026 • edited by datadog-prod-us1-3 Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pr-commenter Bot commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Explanation

More details about the CI and significant changes

Uh oh!

dd-octo-sts Bot commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Typing analysis

steep:ignore comments

Untyped methods

Uh oh!

eregon commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lloeki commented May 19, 2026

Uh oh!

p-datadog commented May 19, 2026

Uh oh!

p-datadog commented May 19, 2026

Uh oh!

eregon May 19, 2026

Choose a reason for hiding this comment

Uh oh!

eregon May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mabdinur May 19, 2026

Choose a reason for hiding this comment

Uh oh!

eregon May 20, 2026

Choose a reason for hiding this comment

Uh oh!

eregon May 20, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

p-datadog commented May 18, 2026 •

edited

Loading

datadog-datadog-prod-us1-2 Bot commented May 18, 2026 •

edited by datadog-prod-us1-3 Bot

Loading

pr-commenter Bot commented May 18, 2026 •

edited

Loading

dd-octo-sts Bot commented May 18, 2026 •

edited

Loading

`steep:ignore` comments

eregon commented May 19, 2026 •

edited

Loading

eregon May 19, 2026 •

edited

Loading