fix(log): scope fd-capture stop() to the owning process

[asaiacai]

Console capture died partway through a torchtitan run (TOR-9): the last uploaded line was mid-way through a torch.compile warning ~10s after init(), while the terminal kept every line through to the end of a 20+ minute run. Metrics were unaffected.

The bug

FdCapture.stop() writes a sentinel into the pipe so the reader knows where "everything written before stop()" ends. That pipe is shared with every process that inherited fd 1/2 — forked DataLoader and inductor compile workers, the sync subprocess, anything spawned during training. Forked children also inherit pluto's atexit handlers, so a child exiting through the normal interpreter path runs Op.finish() → flush_console_buffers() → FdCapture.stop().

The child's sentinel lands in the shared pipe and the parent's reader treats it as its own: _enqueue_enabled = False, and the reader drops into tee-only drain mode. Terminal output is unaffected, the reader thread stays alive, nothing is logged at any level — the run's console section just stops, mid-batch. Timing on the reported run lines up with inductor spinning up its compile workers.

The fix

Scope both halves of stop() to the process that called start():

• stop() is a no-op off the owner pid — restoring fds and signalling the reader are the owner's business.
• The sentinel carries the owner's pid (_stop_sentinel()), and the reader honours only its own. A foreign sentinel is dropped from both the tee and the capture, so no control bytes leak into the terminal or the uploaded lines.

Split-across-reads handling grows with the variable-length pid field: _partial_sentinel_suffix now also holds back a complete prefix whose pid (and closing suffix) is still arriving.

Also documents the constraint in CLAUDE.md so stop() doesn't grow another shared-pipe side effect.

Tested (run the relevant ones):

• Code formatting: bash format.sh — ruff check, ruff format --check, mypy pluto/_fd_capture.py all clean
• Any manual or new tests for this PR (please specify below)

New in tests/test_fd_capture.py:

• TestForkedChildCannotDisableParentCapture — a forked child calling stop() must not mute the parent, and must not leak sentinel bytes into captured output; owner stop() still flushes the partial line and mutes.
• TestSentinelSplitAcrossReads — _partial_sentinel_suffix unit cases for the variable-length pid, plus an end-to-end split-write flush.

Both fork tests fail on the previous code (parent captures 0 lines after the child exits) and pass here. Full file: 26 passed; with test_log_console_handler.py + test_sanitize.py: 93 passed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HGGPRf5pRf6sz9tgoucTm4

---

Generated by Claude Code

---

Note

Medium Risk
Changes low-level fd/pipe teardown behavior on a shared resource; incorrect scoping could still mute capture or leak control bytes, but the fork and split-sentinel tests target the reported production failure mode.

Overview
Fixes silent loss of uploaded console logs when forked workers (e.g. torch.compile / DataLoader) tear down inherited Pluto state: a child’s FdCapture.stop() used to write the shared pipe’s flush sentinel and the parent reader would stop enqueueing for the rest of the run while the terminal still showed everything.

FdCapture now records the owner pid in start() and makes stop() a no-op in any other process (no fd restore, no sentinel). The flush sentinel embeds the owner pid; the reader only treats its own sentinel as the flush boundary and drops foreign sentinels from tee and capture so control bytes never appear in logs.

_partial_sentinel_suffix is updated for the variable-length pid field so sentinels split across pipe reads still flush correctly.

CLAUDE.md documents the shared-pipe / fork constraint. tests/test_fd_capture.py adds fork regression tests and sentinel-split coverage.

^{Reviewed by Cursor Bugbot for commit 664494c. Configure here.}

Summary by CodeRabbit

• Bug Fixes

  • Improved console capture across forked processes.
  • Prevented child-process shutdown from interrupting the parent process’s console capture.
  • Ensured control signals remain hidden from captured output.
  • Improved handling of split or partial capture signals.

• Tests

  • Added regression coverage for forked-child behavior and signal handling.

[coderabbitai]

📝 Walkthrough

Walkthrough

FdCapture now scopes descriptor restoration and flush sentinels to the process that started capture. The reader parses PID-tagged sentinels, ignores foreign markers, handles split reads, and preserves output. Tests and documentation cover forked-child teardown.

Changes

Fork-safe FdCapture

Layer / File(s)	Summary
Sentinel ownership and capture state pluto/_fd_capture.py	FdCapture records the owner PID and creates PID-tagged flush sentinels.
Owner-scoped shutdown and reader filtering pluto/_fd_capture.py	Non-owner stop() calls do not alter capture. The reader filters foreign sentinels and handles partial markers.
Fork and split-sentinel regression coverage tests/test_fd_capture.py, CLAUDE.md	Tests cover child teardown, owner shutdown, partial sentinels, and split reads. Documentation records the fork-safe behavior.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ParentProcess
  participant ChildProcess
  participant SharedPipeReader
  ParentProcess->>SharedPipeReader: start capture with owner PID
  ChildProcess->>ChildProcess: call FdCapture.stop()
  ChildProcess-->>SharedPipeReader: no sentinel and no descriptor restore
  ParentProcess->>SharedPipeReader: write owner-tagged flush sentinel
  SharedPipeReader->>SharedPipeReader: flush preceding bytes and stop ingestion

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description explains the bug, fix, and test results, and it includes the required formatting and test sections.
Title check	✅ Passed	The title clearly and concisely describes the main fix: restricting fd-capture stop behavior to the owning process.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/torchtitan-logs-partial-persist-cica4h

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[ryanhayame]

lgtm

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@tests/test_fd_capture.py`:
- Around line 169-187: Update test_child_stop_leaks_no_control_bytes to write
_stop_sentinel(pid) to file descriptor 2 after waitpid(), exercising
_reader_loop’s foreign PID-tagged sentinel handling. Keep the existing
assertions verifying the sentinel is not logged or emitted as output and that
subsequent “still here” output remains captured.

🪄 Autofix

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: fbd10dff-8ebc-4efb-8a6c-be28ef85c2f6

📥 Commits

Reviewing files that changed from the base of the PR and between 1439d2f and 8861d30.

📒 Files selected for processing (3)

• CLAUDE.md
• pluto/_fd_capture.py
• tests/test_fd_capture.py

[cursor]

Addressed the CodeRabbit note on test_child_stop_leaks_no_control_bytes: after waitpid(), the test now writes _stop_sentinel(pid) to fd 2 so _reader_loop’s foreign PID-tagged sentinel path is exercised (child stop() is a no-op off the owner pid and never wrote one). Existing assertions unchanged; TestForkedChildCannotDisableParentCapture passes.

PR #140 had already been merged and its head branch deleted, so the fix lands in a follow-up PR from cursor/pull-request-comment-fixes-c0ca. The original thread is resolved.

  

_{Sent by Cursor Automation: Autofix PR review comments}