Fix: Invalid JSON on gc-stats (#1801)

[MitchLewis930]

PR_054

Summary by CodeRabbit

• Bug Fixes
  • Enhanced garbage collection statistics endpoint to use proper JSON formatting, improving parsing reliability and compatibility with standard JSON processing tools.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

This change refactors GC statistics serialization in Puma's status endpoint and corresponding tests. The codebase replaces manual JSON string construction with standard JSON.to_json() and JSON.parse() methods, simplifying both production code and test logic.

Changes

Cohort / File(s)	Summary
GC Stats Serialization lib/puma/app/status.rb	Added require 'json' and replaced manual JSON string building with GC.stat.to_json for proper GC statistics serialization in the /gc-stats endpoint.
GC Stats Test Parsing test/test_cli.rb	Added require "json" and replaced regex-based JSON parsing with standard JSON.parse() for simplified GC statistics extraction in control tests.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 JSON strings once built by hand,
Now to_json() takes a stand,
Parsing cleaner, testing bright,
Standard methods set things right! ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is largely incomplete, containing only 'PR_054' without addressing required template sections like problem statement, alternatives, design decisions, or checklist items.	Expand the description to include the original problem, related issues, design rationale, and complete the contributor checklist as specified in the template.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the fix (Invalid JSON on gc-stats) with a specific issue reference (#1801), accurately reflecting the main change in the pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch pr_054_after

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 RuboCop (1.84.0)

lib/puma/app/status.rb

Error: The Layout/Tab cop has been renamed to Layout/IndentationStyle.
(obsolete configuration found in .rubocop.yml, please update it)
The Layout/TrailingBlankLines cop has been renamed to Layout/TrailingEmptyLines.
(obsolete configuration found in .rubocop.yml, please update it)
Performance/* has been extracted to the rubocop-performance gem.
(obsolete configuration found in .rubocop.yml, please update it)

test/test_cli.rb

Error: The Layout/Tab cop has been renamed to Layout/IndentationStyle.
(obsolete configuration found in .rubocop.yml, please update it)
The Layout/TrailingBlankLines cop has been renamed to Layout/TrailingEmptyLines.
(obsolete configuration found in .rubocop.yml, please update it)
Performance/* has been extracted to the rubocop-performance gem.
(obsolete configuration found in .rubocop.yml, please update it)

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

test/test_cli.rb (1)

177-185: ⚠️ Potential issue | 🟠 Major

Incomplete fix: First gc-stats parsing still uses manual regex extraction.

The PR objective is to fix JSON parsing issues, but only the second gc-stats call (line 199) was updated to use JSON.parse. The first gc-stats parsing (lines 177-185) still uses the fragile manual regex approach that was the source of the original bug.

This inconsistency means:

1. The first gc-stats call may still fail with certain JSON formats
2. The test logic is unnecessarily complex and harder to maintain

🐛 Proposed fix to use JSON.parse consistently

     lines = body.split("\r\n")
     json_line = lines.detect { |l| l[0] == "{" }
-    pairs = json_line.scan(/\"[^\"]+\": [^,]+/)
-    gc_stats = {}
-    pairs.each do |p|
-      p =~ /\"([^\"]+)\": ([^,]+)/ || raise("Can't parse #{p.inspect}!")
-      gc_stats[$1] = $2
-    end
-    gc_count_before = gc_stats["count"].to_i
+    gc_stats = JSON.parse(json_line)
+    gc_count_before = gc_stats["count"]

     s = yield
     s << "GET /gc HTTP/1.0\r\n\r\n"

🧹 Nitpick comments (1)

test/test_cli.rb (1)

96-97: Redundant require 'json' statement.

This require 'json' inside test_control_clustered is now redundant since json is already required at the top of the file (line 4).

🧹 Suggested cleanup

     sleep 2

     s = UNIXSocket.new `@tmp_path`
     s << "GET /stats HTTP/1.0\r\n\r\n"
     body = s.read

-    require 'json'
     status = JSON.parse(body.split("\n").last)

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 73c2026 and 2db1ef1.

📒 Files selected for processing (2)

• lib/puma/app/status.rb
• test/test_cli.rb

🔇 Additional comments (4)

lib/puma/app/status.rb (2)

1-2: LGTM!

Adding require 'json' at the top level is the correct approach to enable JSON serialization for GC.stat.

---

64-65: Clean fix for the JSON serialization issue.

Using GC.stat.to_json instead of manual string construction correctly addresses the whitespace-sensitive JSON parsing issue. The standard library handles all edge cases in key-value formatting.

test/test_cli.rb (2)

4-4: LGTM!

Adding require "json" at the top level is appropriate for the test file.

---

199-200: Good use of JSON.parse for the second gc-stats call.

This correctly parses the JSON response. Note that with JSON.parse, gc_stats["count"] will already be an integer, so the .to_i call on line 200 is technically redundant but harmless.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}