fix: drain remaining PTY data after reader stop on macOS#561
fix: drain remaining PTY data after reader stop on macOS#561mangelajo merged 6 commits intojumpstarter-dev:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 36 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds bounded PTY drain logic and a Changes
Sequence DiagramsequenceDiagram
participant Reader as Reader Task
participant PTY as PTY fd
participant Finally as Finally/Drain
participant Flush as _flush_lines()
participant Logger as Logger
Reader->>PTY: non-blocking reads in loop
PTY-->>Reader: bytes
Reader->>Flush: accumulate & flush complete lines
Note over Reader: Reader task stops/cancelled
Reader->>Finally: enter finally block
Finally->>PTY: synchronous os.read() until bytes or timeout
PTY-->>Finally: remaining bytes / EOF / OSError
Finally->>Flush: pass accumulated bytes
Flush->>Logger: decode and log complete non-empty lines
Finally->>Logger: decode/log any remaining remainder bytes
Finally-->>Reader: drain complete, close fd
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/exporter/hooks_test.py (1)
500-548: Test may not exercise byte limit on typical systems.The test fills the pipe buffer until
BlockingIOError, which on most Linux systems is around 64KB (less thanMAX_DRAIN_BYTESof 256KB). This means the drain will complete before hitting the byte limit, so the assertiondrained <= MAX_DRAIN_BYTESpasses trivially.To actually test the byte limit enforcement, consider using
fcntl.F_SETPIPE_SZto increase the pipe buffer size beyond 256KB, or mocking the drain loop directly.That said, the test still validates that the drain logic works correctly with bounded reads, which provides value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/exporter/hooks_test.py` around lines 500 - 548, The current test (test_drain_respects_byte_limit) likely never reaches MAX_DRAIN_BYTES because the OS pipe buffer is smaller; modify the test to force the drain loop to hit the byte limit by enlarging the pipe buffer before writing (use fcntl.F_SETPIPE_SZ when available to set the pipe size > MAX_DRAIN_BYTES), falling back to an explicit mock of the drain loop if the fcntl constant isn't present; keep references to MAX_DRAIN_BYTES, DRAIN_TIMEOUT_SECONDS, and _flush_lines and ensure the rest of the test logic (non-blocking fds, write loop, reading loop and assertions) remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/exporter/hooks_test.py`:
- Around line 500-548: The current test (test_drain_respects_byte_limit) likely
never reaches MAX_DRAIN_BYTES because the OS pipe buffer is smaller; modify the
test to force the drain loop to hit the byte limit by enlarging the pipe buffer
before writing (use fcntl.F_SETPIPE_SZ when available to set the pipe size >
MAX_DRAIN_BYTES), falling back to an explicit mock of the drain loop if the
fcntl constant isn't present; keep references to MAX_DRAIN_BYTES,
DRAIN_TIMEOUT_SECONDS, and _flush_lines and ensure the rest of the test logic
(non-blocking fds, write loop, reading loop and assertions) remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71bfa89b-3845-42ee-b2bd-aa69ad0f9d16
📒 Files selected for processing (2)
python/packages/jumpstarter/jumpstarter/exporter/hooks.pypython/packages/jumpstarter/jumpstarter/exporter/hooks_test.py
…apture The read_pty_output function exited immediately when the reader_stop flag was set, without draining remaining data from the PTY kernel buffer. On macOS, PTY output delivery timing differs from Linux, causing data to still be in the kernel buffer after the subprocess exits and the 0.2s grace period elapses. This resulted in test_exec_bash failing on macOS CI runners because the mocked logger never received the expected output. The fix adds a non-blocking drain loop in the finally block of read_pty_output that reads and processes all remaining PTY data before the function returns. Generated-By: Forge/20260415_214923_3139216_59edcfbb Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…logic The line-processing pattern (split on newline, decode, rstrip, append, log) was duplicated in three places within read_pty_output(). Extracting it into a single _flush_lines() function ensures the contract is maintained in one place and reduces the risk of inconsistent changes. Generated-By: Forge/20260415_214923_3139216_59edcfbb
…ppression Add MAX_DRAIN_BYTES (256KB) and DRAIN_TIMEOUT_SECONDS (2s) limits to the PTY drain loop in read_pty_output(). Without bounds, a grandchild process holding the PTY slave fd open could cause the drain to spin indefinitely after the hook timeout fires, effectively bypassing the configurable timeout. Also wrap drain + line-processing in try/except to prevent suppressing the original exception from the finally block. Generated-By: Forge/20260415_214923_3139216_59edcfbb
The previous test_pty_output_drained_after_process_exits used a fast subprocess whose output was always captured by the main read loop before the stop flag was set, so the drain path was never exercised. Replace it with test_pty_output_drained_after_stop_flag_set which uses a pipe to inject data, skips the main loop entirely, and verifies the drain logic captures all lines. Generated-By: Forge/20260415_214923_3139216_59edcfbb
…iling newline Cover the three edge cases specified but not tested: - Empty PTY buffer after stop flag: drain completes immediately - OSError during drain (e.g. closed fd): exits gracefully - Output without trailing newline: captured by the trailing buffer handler Generated-By: Forge/20260415_214923_3139216_59edcfbb
raballew
force-pushed
the
046-fix-hook-pty-macos
branch
from
e09b6cf to
ffaf152
Compare
Add two tests that exercise the drain code path inside read_pty_output's finally block, raising diff-cover from 11% to 100% on the changed lines: - test_drain_reads_data_remaining_in_pty_buffer: patches os.read to inject data after EOF, simulating the macOS PTY buffering scenario - test_drain_exception_is_suppressed: patches _flush_lines to raise during drain, verifying the except-Exception handler suppresses it Generated-By: Forge/20260416_144213_369510_282d5eaf Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merge latest main to include the PTY drain fix (jumpstarter-dev#561) and other recent changes. Generated-By: Forge/20260416_202053_681470_c94294b7_i240 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
read_pty_output_flush_lineshelper to deduplicate line-processing logic across main loop and drainMAX_DRAIN_BYTES(256KB) andDRAIN_TIMEOUT_SECONDS(2s) to prevent indefinite blocking when grandchild processes hold the PTY slave fd openContext
On macOS, PTY output delivery timing differs from Linux -- data can still be in the kernel buffer after the subprocess exits and the 0.2s grace period elapses. This caused
test_exec_bashto fail on macOS CI runners because the mocked logger never received the expected output. The fix adds a bounded, non-blocking drain loop in thefinallyblock ofread_pty_output.Test plan
_flush_lineshelper (complete lines, empty buffer, no newlines, empty lines)MAX_DRAIN_BYTESis respectedCloses #560