fix(render): stop at end of JSON payload instead of waiting for EOF - #39
Merged
Merged
Conversation
`_consume_stdin()` calls `sys.stdin.buffer.read()`, which returns only at EOF — and EOF requires *every* write handle on the pipe to close, not just the one Claude Code wrote through. Claude Code spawns the statusLine via a shell (`bash -c "cs render"`); on Windows a sibling process that inherited the write handle keeps the pipe open after that shell exits. Result: the payload has arrived in full, the render never happens, and the process lives forever. Observed on Windows 11 / v3.32.0 as a steady drip of `cs render` processes — parent gone, still resident minutes later, one every 20-40s across concurrent sessions: 16200 age= 281s parent=27156 GONE 19044 age= 277s parent=8356 GONE 25480 age= 266s parent=35288 GONE Confirmed directly: write the payload, never close stdin → the process is still alive after 4s; close stdin → it exits immediately. The payload is one JSON document, so read until it parses and stop there. Falls back to reading to EOF when the input is not JSON, so nothing is truncated. One extra json.loads per chunk (~0.05ms — the payload is a couple of KB and arrives in a single read). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesThe thin renderer now consumes stdin in chunks and stops when a complete JSON document is available. Tests cover split payloads, non-JSON input, interactive and empty stdin, session extraction, and JSON completeness detection. Bounded stdin consumption
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant stdin.buffer
participant _consume_stdin
participant _payload_is_complete
_consume_stdin->>stdin.buffer: read1(_STDIN_CHUNK)
_consume_stdin->>_payload_is_complete: check accumulated bytes
_payload_is_complete->>_payload_is_complete: call json.loads
_payload_is_complete-->>_consume_stdin: complete or incomplete
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_render_thin_stdin.py (1)
27-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the no-
read1fallback path.The PR objectives call out that "stdin replacements without
read1remain supported," but no test exercisesrender_thin.py'sread_some is Nonebranch (the old blocking-read()path) or theexcept (OSError, AttributeError)branch. Given_FakeBufferalready exists, a variant withoutread1would close this gap cheaply.As per coding guidelines, "Add targeted pytest tests around
core.main, calculation helpers, and file parsing when modifying logic."class _FakeBufferNoRead1: """Stands in for stdin replacements that only implement read().""" def read(self): return PAYLOAD def test_falls_back_to_read_when_read1_is_absent(monkeypatch): stdin = _FakeStdin([PAYLOAD]) stdin.buffer = _FakeBufferNoRead1() monkeypatch.setattr(sys, "stdin", stdin) assert render_thin._consume_stdin() == PAYLOAD🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_render_thin_stdin.py` around lines 27 - 44, Add targeted pytest coverage for render_thin._consume_stdin when stdin.buffer lacks read1: introduce a fake buffer implementing only read() and assert the payload is returned. Also exercise the (OSError, AttributeError) fallback path if needed to cover both supported fallback branches, while preserving the existing _FakeBuffer behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/claude_statusbar/render_thin.py`:
- Around line 190-196: Update _payload_is_complete to catch RecursionError
alongside ValueError from json.loads, returning False so deeply nested JSON
follows the same incomplete/malformed-payload fallback path without propagating
through _consume_stdin or render.
---
Nitpick comments:
In `@tests/test_render_thin_stdin.py`:
- Around line 27-44: Add targeted pytest coverage for render_thin._consume_stdin
when stdin.buffer lacks read1: introduce a fake buffer implementing only read()
and assert the payload is returned. Also exercise the (OSError, AttributeError)
fallback path if needed to cover both supported fallback branches, while
preserving the existing _FakeBuffer behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8faea5aa-9d1b-4a2d-9adc-402041c5c5d4
📒 Files selected for processing (2)
src/claude_statusbar/render_thin.pytests/test_render_thin_stdin.py
Comment on lines
+190
to
+196
| def _payload_is_complete(buf: bytearray) -> bool: | ||
| """True once `buf` holds a whole JSON document.""" | ||
| try: | ||
| json.loads(buf.decode("utf-8", errors="replace")) | ||
| except ValueError: # JSONDecodeError ⊂ ValueError | ||
| return False | ||
| return True |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
_payload_is_complete doesn't catch RecursionError from deeply-nested JSON.
json.loads raises RecursionError (a RuntimeError, not ValueError) on pathologically nested documents. That exception isn't caught here, nor by _consume_stdin's except (OSError, AttributeError): (Line 237), so it propagates out of _consume_stdin() and crashes render() entirely instead of falling back gracefully like malformed JSON does.
🛡️ Proposed fix
def _payload_is_complete(buf: bytearray) -> bool:
"""True once `buf` holds a whole JSON document."""
try:
json.loads(buf.decode("utf-8", errors="replace"))
- except ValueError: # JSONDecodeError ⊂ ValueError
+ except (ValueError, RecursionError): # JSONDecodeError ⊂ ValueError
return False
return True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _payload_is_complete(buf: bytearray) -> bool: | |
| """True once `buf` holds a whole JSON document.""" | |
| try: | |
| json.loads(buf.decode("utf-8", errors="replace")) | |
| except ValueError: # JSONDecodeError ⊂ ValueError | |
| return False | |
| return True | |
| def _payload_is_complete(buf: bytearray) -> bool: | |
| """True once `buf` holds a whole JSON document.""" | |
| try: | |
| json.loads(buf.decode("utf-8", errors="replace")) | |
| except (ValueError, RecursionError): # JSONDecodeError ⊂ ValueError | |
| return False | |
| return True |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/claude_statusbar/render_thin.py` around lines 190 - 196, Update
_payload_is_complete to catch RecursionError alongside ValueError from
json.loads, returning False so deeply nested JSON follows the same
incomplete/malformed-payload fallback path without propagating through
_consume_stdin or render.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
_consume_stdin()callssys.stdin.buffer.read(), which returns only at EOF.EOF requires every write handle on the pipe to close — not just the one
Claude Code wrote through. Claude Code spawns the statusLine via a shell
(
bash -c "cs render"on Windows), and a sibling process that inherited thewrite handle keeps the pipe open after that shell exits.
So the payload arrives in full, the render never happens, and the process
lives forever. On this machine that is a steady drip of
cs renderprocesses— parent gone, still resident minutes later, roughly one every 20-40s across
concurrent sessions:
Confirmed directly — write the payload, never close stdin:
Fix
The payload is a single JSON document, so read until it parses and stop there
instead of waiting on the pipe. Falls back to reading to EOF when the input
is not JSON, so nothing is truncated. Keeps
read()for any stdinreplacement that lacks
read1.Cost: one extra
json.loadsper chunk. The payload is a couple of KB andarrives in a single read — ~0.05ms on the render path.
Tests
New
tests/test_render_thin_stdin.py: the fake pipe raises if read againafter the payload is complete, which is exactly the call that hangs in
production. Covers single-chunk, split-chunk, non-JSON→EOF, tty, empty, and
session-id routing after the bounded read.
tests/test_daemon.py+ the new file: 65 passed, 1 skipped, 1 pre-existingunrelated failure (
test_release_pidfile_leaves_someone_elses_file_alone).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests