Use Puma::Server instead of Puma::Launcher to fix Ctrl+C handling#21661
Use Puma::Server instead of Puma::Launcher to fix Ctrl+C handling#21661kx7m2qd wants to merge 2 commits into
Conversation
Puma::Launcher installs process-wide SIGINT/SIGTERM signal traps as part of its startup, intended for when Puma owns the entire process. Since the MCP server runs on a background thread inside msfconsole, these traps are process-global and silently override msfconsole's own Ctrl+C handling. Puma::Server is the lower-level, embeddable API and does not install any signal traps, making it safe to run inside a host process like msfconsole. Fixes rapid7#21650
dwelch-r7
left a comment
dwelch-r7
left a comment
There was a problem hiding this comment.
Tested and seems to work well, one minor comment but once that's addressed I think this is good to go
| puma_config = Puma::Configuration.new do |config| | ||
| config.bind "tcp://#{bind_host}:#{port}" | ||
| config.threads min_threads, max_threads | ||
| config.workers workers |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sorry about the delay @kx7m2qd taking a look right now
There was a problem hiding this comment.
LGTM manual testing seemed good, just waiting on the automated tests to finish up
There was a problem hiding this comment.
@kx7m2qd there seems to be some issues with the rspec tests, they're hanging on the Pr and locally I'm getting failures and it's hanging, did they pass for you locally?
Puma::Server does not support clustering, so falling back to Puma::Launcher when workers > 0 preserves the existing MSF_MCP_WORKERS behavior for the standalone msfmcpd binary, while still using Puma::Server (no signal trapping) for the plugin's embedded case where workers defaults to 0.
3 participants
Puma::Launcher installs process-wide SIGINT/SIGTERM signal traps as part of its startup, intended for when Puma owns the entire process (e.g. run standalone via the
pumaCLI). Since the MCP server runs on a background thread inside msfconsole (spawned viaframework.threads.spawn), these traps are process-global and silently override msfconsole's own Ctrl+C handling, which relies on Ruby's defaultInterruptbehavior.This swaps
Puma::LauncherforPuma::Server, the lower-level, embeddable API.Puma::Serverdoes not install any signal traps, making it safe to run inside a host process like msfconsole. Also updatedshutdownto call.stop(true)directly on the server for graceful shutdown.Related Issue:
Fixes #21650
Verification Steps:
mcpplugin, runmcp start, then press Ctrl+C. Expected: console printsInterrupt: use the 'exit' command to quit(matching normal msfconsole behavior), rather than doing nothing.mcp stop. Expected: printsMCP server stoppedcleanly with no hang or error.mcp startfollowed bymcp restart. Expected: server restarts cleanly without error.Test Evidence:
msf > load mcp
[] MCP plugin loaded. Use mcp start to start the server.
[] Successfully loaded plugin: mcp
msf > mcp start
[] Auto-started msgrpc - User: msf, Pass: [redacted]
[] MCP server started on localhost:3000 (transport: http)
msf > Interrupt: use the 'exit' command to quit
msf > mcp stop
[] MCP server stopped
msf > mcp start
[] MCP server started on localhost:3000 (transport: http)
msf > mcp restart
[*] MCP server started on localhost:3000 (transport: http)
Environment:
| Operating System | Ubuntu 24.04 |
| Target Software/Hardware | N/A |
AI Usage Disclosure:
Used AI to help identify the root cause and draft the fix. All testing and verification was done manually by me.
Pre-Submission Checklist:
Reviewer Notes
Wasn't able to get the full local RSpec suite passing due to an unrelated local Postgres/msfdb environment setup issue on my machine verified manually instead (see Test Evidence). Happy to dig further if CI surfaces anything.