Avoid mutating global STDOUT & STDERR (#1837)#1
Avoid mutating global STDOUT & STDERR (#1837)#1MitchLewis930 wants to merge 1 commit intopr_051_beforefrom
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| events = Puma::Events.stdio | ||
|
|
||
| assert !STDOUT.sync | ||
| assert events.stdout.sync |
There was a problem hiding this comment.
Test does not restore STDOUT.sync global state
Low Severity
The new test_stdio_respects_sync test sets STDOUT.sync = false but never restores it to its original value. Other tests in this file use ensure blocks to restore global state after modification (e.g., ENV["PUMA_DEBUG"]). This missing cleanup causes test pollution and could lead to flaky behavior in other tests that depend on STDOUT.sync being in its default state.
| assert_equal STDERR, events.stderr | ||
| # events.stdout is a dup, so same file handle, different ruby object, but inspect should show the same file handle | ||
| assert_equal STDOUT.inspect, events.stdout.inspect | ||
| assert_equal STDERR.inspect, events.stderr.inspect |
There was a problem hiding this comment.
Test compares inspect strings that have different formats
Medium Severity
The test_stdio test compares STDOUT.inspect with events.stdout.inspect, but events.stdout is a dup of $stdout. In Ruby, STDOUT.inspect typically returns #<IO:<STDOUT>> while STDOUT.dup.inspect returns #<IO:fd 1> - different string formats that won't be equal. The test's approach of comparing inspect strings is unreliable for verifying the same file handle; comparing fileno values would be more robust.


PR_051
Note
Low Risk
Behavior change is limited to how
Puma::Eventshandles IO objects (dup + sync), with test-only adjustments elsewhere; low risk unless callers relied onEvents.stdoutbeing the exact same Ruby object asSTDOUT/STDERR.Overview
Puma
Eventsnow duplicates the passedstdout/stderrIO objects on initialization before settingsync, preventing Puma from mutating the globalSTDOUT/STDERRstate.Tests are updated to reflect the new semantics (stdio no longer equals the global constants, but points at the same underlying file descriptor) and add coverage that
Events.stdioforcessynctrue without changingSTDOUT.sync;TCPLoggertests are adjusted to ensure subprocess IO capture still observes logger output.Written by Cursor Bugbot for commit 70b28bb. This will update automatically on new commits. Configure here.