Use the requested pid in the Linux memory grabber by hammadxcm · Pull Request #7011 · sidekiq/sidekiq · GitHub
Skip to content

Use the requested pid in the Linux memory grabber - #7011

Merged
mperham merged 1 commit into
sidekiq:mainfrom
hammadxcm:fix-launcher-memory-grabber-pid
May 28, 2026
Merged

mperham merged 1 commit into
sidekiq:mainfrom
hammadxcm:fix-launcher-memory-grabber-pid

Conversation

@hammadxcm

Copy link
Copy Markdown
Contributor

Summary

The Linux branch of MEMORY_GRABBER (lib/sidekiq/launcher.rb) takes a pid argument but ignores it, reading the current process instead:

when /linux/
  ->(pid) {
    IO.readlines("/proc/#{$$}/status").each do |line|   # reads $$ , not pid
      ...

The darwin|bsd branch correctly uses pid (ps -o pid,rss -p #{pid}). The bug has been latent since #4717 because the only caller is memory_usage(::Process.pid), so $$ and pid are always equal in practice — but the lambda is parameterized by pid and should honor it (and stay consistent across platforms).

Fix: read /proc/#{pid}/status.

Testing

Adds a Linux-guarded regression test: calling the grabber with a non-existent pid must raise Errno::ENOENT (it now reads that pid's /proc entry). This fails against the old $$ behavior (which silently returns the current process's RSS) and passes after the fix. It skips on non-Linux platforms where this lambda isn't used.

bundle exec rake is green (standard + lint:herb + full suite; the new test skips locally on macOS and runs on Linux CI).

Note: I develop on macOS, so the Linux /proc path can't execute locally — this fix is by inspection/consistency with the darwin|bsd branch, and the regression test exercises it on Linux CI.

The Linux MEMORY_GRABBER lambda takes a pid argument but read
/proc/#{$$}/status (the current process) instead of /proc/#{pid}/status,
ignoring its argument. The darwin/bsd branch correctly uses pid. The bug
has been latent since sidekiq#4717 because the only caller passes Process.pid,
so $$ and pid are always equal in practice. Use pid so the grabber
honors its argument and matches the other platforms.

Adds a Linux-guarded regression test: calling the grabber with a
non-existent pid must raise Errno::ENOENT (it now reads that pid's
/proc entry), which fails against the old $$ behavior.
@mperham
mperham merged commit 6b8e894 into sidekiq:main May 28, 2026
17 checks passed
@hammadxcm

Copy link
Copy Markdown
Contributor Author

mperham pushed a commit that referenced this pull request May 29, 2026
Follow-up coverage for the memory grabber fixed in #7011. The existing
tests only call MEMORY_GRABBER with $$ (plus a Linux-only check), and
the memory_usage wrapper was untested. Add cross-platform coverage that
the grabber reads a different process by pid (Process.ppid) and that
memory_usage returns a positive integer for a given pid.
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