Skip to content

Use Puma::Server instead of Puma::Launcher to fix Ctrl+C handling #21661

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kx7m2qd wants to merge 2 commits into
base: master
Choose a base branch
from
+29 −13
Open

Use Puma::Server instead of Puma::Launcher to fix Ctrl+C handling #21661

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e4b2d42
Use Puma::Server instead of Puma::Launcher to fix Ctrl+C handling
kx7m2qd Jul 8, 2026
85f3312
Preserve Puma::Launcher clustering when workers > 0
kx7m2qd Jul 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 14 additions & 16 deletions lib/msf/core/mcp/server.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,12 @@ def start(transport: :stdio, host: 'localhost', port: 3000, auth_token: nil, min
# Shutdown the MCP server and cleanup resources
#
def shutdown
@puma_launcher&.stop
@puma_server&.stop(true)
rescue StandardError => e
elog({ message: 'Error stopping Puma', exception: e }, LOG_SOURCE, LOG_ERROR)
ensure
@puma_log_io&.close
@puma_launcher = nil
@puma_server = nil
@puma_log_io = nil
@msf_client&.shutdown
@mcp_server = nil
Expand Down Expand Up @@ -130,8 +130,7 @@ def start_stdio
def start_http(host, port, auth_token, min_threads: PUMA_MIN_THREADS, max_threads: PUMA_MAX_THREADS, workers: PUMA_WORKERS)
require 'rack'
require 'puma'
require 'puma/configuration'
require 'puma/launcher'
require 'puma/server'
require 'puma/log_writer'

transport = ::MCP::Server::Transports::StreamableHTTPTransport.new(@mcp_server)
Expand All @@ -147,26 +146,25 @@ def start_http(host, port, auth_token, min_threads: PUMA_MIN_THREADS, max_thread
# Use Puma's server API directly so we can stop it gracefully on shutdown.
bind_host = host.include?(':') ? "[#{host}]" : host
@puma_log_io = File.open(File::NULL, 'w')
begin
puma_config = Puma::Configuration.new do |config|
config.bind "tcp://#{bind_host}:#{port}"
config.threads min_threads, max_threads
config.workers workers

@dwelch-r7 dwelch-r7 Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does Puma::Launcher not have support for workers? if it does let's add it in, otherwise we should remove all references to workers if they aren't used anymore

@kx7m2qd kx7m2qd Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good question turns out workers goes a bit deeper than server.rb. It's actually a supported, validated config option (MSF_MCP_WORKERS env var / mcp.workers in the config file, validated in config/validator.rb, loaded in config/loader.rb) that's wired through to the standalone msfmcpd binary via application.rb.
Since Puma::Server doesn't support clustering (that's Puma::Cluster, which Puma::Launcher used internally), workers is currently a no-op wherever it flows through start_http. Given the plugin path always uses the default (PUMA_WORKERS = 0), this PR's fix doesn't change plugin behavior either way but I don't want to silently strip a documented option that might affect the standalone msfmcpd binary without testing that path too.

Given the scope, I'd propose: keep workers in this PR as-is (still a no-op, same as before my change — not a regression I'm introducing), and I'll follow up with a separate PR/issue to properly deprecate or reintroduce cluster support for workers, after testing the msfmcpd binary path specifically. Let me know if you'd rather I just strip it now instead happy to do either, just flagging the wider surface first.

@dwelch-r7 dwelch-r7 Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The plugin only uses 0 sure but there are other ways of starting up the MCP server using msfmcpd where if you set MSF_MCP_WORKERS=4 it would work yes?

@kx7m2qd kx7m2qd Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right, confirmed MSF_MCP_WORKERS=4 via msfmcpd would genuinely lose clustering with this change, that's a real regression I missed since I only tested the plugin path (always workers: 0).

Digging into why: Puma::Launcher 's signal trapping is largely there to support clustering coordinating worker processes needs the parent to own SIGTERM/SIGINT. That's actually fine for msfmcpd (it's a standalone process, same as any CLI daemon), it's only a problem for the plugin case where we're embedded inside msfconsole.

Proposing a conditional: use Puma::Server when workers == 0 (the plugin's case, fixes the Ctrl+C bug), and keep Puma::Launcher when workers > 0 (preserves clustering for msfmcpd, and signal ownership there isn't an issue). Will push an update and test both paths (plugin + msfmcpd with MSF_MCP_WORKERS set) before pinging you again.

@kx7m2qd kx7m2qd Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Pushed the fix verified both paths:

Plugin (mcp start, workers=0): Ctrl+C still shows the normal Interrupt: use the 'exit' command to quit, and mcp stop/mcp restart work cleanly.
Standalone (MSF_MCP_WORKERS=2 ./msfmcpd): confirmed clustering is genuinely preserved —ps -ef --forestshows the parent Puma process with 2 real cluster worker child processes underneath it, same as before this change.

Confirmed with your exact example MSF_MCP_WORKERS=4 ./msfmcpd now spawns 4 real cluster workers:

puma 6.6.0 (tcp://localhost:3000) [metasploit-framework]
  \_ msfrpcd
  \_ puma: cluster worker 0: 151501
  \_ puma: cluster worker 1: 151501
  \_ puma: cluster worker 2: 151501
  \_ puma: cluster worker 3: 151501

And the plugin path (workers=0) still fixes the original Ctrl+C issue
mcp start/Ctrl+C/mcp stop/mcp restart all confirmed working cleanly.

config.log_requests false
config.app rack_app
end

begin
# Suppress Puma's startup banner by providing a silent log writer
log_writer = Puma::LogWriter.new(@puma_log_io, @puma_log_io)
@puma_launcher = Puma::Launcher.new(puma_config, log_writer: log_writer)
@puma_launcher.run

@puma_server = Puma::Server.new(rack_app, nil, {
log_writer: log_writer,
min_threads: min_threads,
max_threads: max_threads
})
@puma_server.add_tcp_listener(bind_host, port)
@puma_server.run.join
rescue StandardError
begin
@puma_launcher&.stop
@puma_server&.stop(true)
rescue StandardError
nil
end
@puma_launcher = nil
@puma_server = nil
@puma_log_io&.close
@puma_log_io = nil
raise
Expand Down
Loading