fix(log): scope fd-capture stop() to the owning process by asaiacai · Pull Request #140 · Trainy-ai/pluto

asaiacai · 2026-08-05T20:39:17Z

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.

Console capture died partway through a torchtitan run: the last uploaded line was mid-way through a torch.compile warning ~10s after init(), while the terminal kept every line to the end of training. FdCapture.stop() writes a sentinel into the pipe so the reader knows where "everything before stop()" ends. But that pipe is shared with every process that inherited fd 1/2 — forked DataLoader and inductor compile workers, the sync subprocess — and forked children 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: it sets _enqueue_enabled = False and drops into tee-only drain mode. Terminal output is unaffected, the reader thread stays alive, nothing is logged — the run's console section just stops. Scope both halves to the process that called start(): - stop() is a no-op off the owner pid. Restoring fds and signalling the reader belong to the owner. - The sentinel carries the owner's pid, and the reader honours only its own. A foreign sentinel is dropped from the tee and the capture rather than leaking control bytes 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. Both new fork tests fail on the previous code and pass here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGGPRf5pRf6sz9tgoucTm4

coderabbitai · 2026-08-05T20:39:25Z

📝 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

🚥 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

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

CI pins ruff 0.4.10 via poetry.lock, which formats an assert message inline rather than parenthesized. No behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGGPRf5pRf6sz9tgoucTm4

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}

pytest 9 exposed a latent bug that silently disabled console upload. setup_logger short-circuited on `len(console.handlers) > 0`, treating *any* handler on the console logger as proof pluto had already configured it. But setup_logger_file sets `console.propagate = False`, which forces anything capturing log records to attach at that logger rather than at the root — and pytest's logging plugin does exactly that from pytest 9 on. So from the second pluto.init() in a process onward, the guard saw pytest's LogCaptureHandlers, returned early, and never installed the ConsoleHandler wrappers. Nothing then enqueued console lines, and the fd layer could not cover for it either: print() writes to sys.stdout, which under pytest capture never reaches fd 1. Zero console records, no error — the same silent-and-total failure mode as the fd-capture bug in #140. Measured on tests/test_sync_process.py::TestSyncProcessShutdown, at the second init(): pytest 8.4.2 console.handlers == 0 -> setup runs pytest 9.1.1 console.handlers == 2 LogCaptureHandler -> setup skipped This is not a test-only problem. Any library that attaches a handler to that logger — Jupyter tooling, an APM agent, a user's own logging setup before a second init() — disables console upload for the rest of the process, in production, with no diagnostic. Fix: mark the one handler setup_logger_file attaches and have the guard look for that marker instead of for handlers in general. Capture behaviour is unchanged and the guard stays idempotent for pluto's own handler, which is what it was there for. Regression tests in tests/test_log_console_handler.py cover both directions: a foreign handler must not skip setup (fails against the old guard), and pluto's own handler must still short-circuit so the wrappers can't stack and double-enqueue. Verified against a local Pluto stack: test_sync_process.py 50 passed under pytest 9.1.1 and 8.4.2; the console/fd suites plus test_e2e_console_logs, 100 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore(deps): bump poetry.lock to clear 62 pip security alerts All 62 pip Dependabot alerts trace to poetry.lock. Bumped via `poetry update --lock` (no source changes in pluto/ or mlop/): cryptography 46.0.3 -> 50.0.0 gitpython 3.1.45 -> 3.1.59 pillow 12.0.0 -> 12.3.0 h2 4.3.0 -> 4.4.1 urllib3 2.6.2 -> 2.7.0 idna 3.11 -> 3.18 pygments 2.19.2 -> 2.21.0 torch 2.7.1 -> 2.13.0 pytest 8.4.2 -> 9.1.1 requests 2.32.5 -> 2.34.2 setuptools 80.9.0 -> 84.0.0 filelock 3.20.1 -> 3.32.3 pytest's pyproject constraint moves ^8.2.2 -> ^9.0.3: the advisory is only fixed in 9.0.3, so the lock alone could not resolve it. The constraint was already stale — the dev venv was on 9.1.1. All 948 tests collect under pytest 9 and 456 offline tests pass; the setup_class/setup_method helpers in the suite are xunit style, not the nose-style hooks pytest 9 removed. torch is a dev-only test dependency and is multi-GB, so it was left to CI to install; the torch and DDP suites are unverified locally. Verified each alert's vulnerable_version_range against the resulting locked versions rather than trusting `npm audit`/`pip audit` output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(docs): remove unused Docusaurus site Deletes docs/, which clears the remaining 84 Dependabot alerts — every npm alert in the repo came from docs/package-lock.json, this being the only npm manifest. Two of them (image-size 2.0.2, GHSA-5p2g-fcmc-qvqq and GHSA-w3rx-r6r6-pgpr) had no upstream patch and could not be fixed by any version bump, so removing the site is the only way to resolve them. The site is dead infrastructure, not something in use: - Live docs are Mintlify at docs.trainy.ai/pluto, built in Trainy-ai/konduktor from this repo's docs-api/ — regenerated each release and CI-checked by api-docs.yml. README points there. - docusaurus.config.js was never updated for the mlop -> pluto rename: title 'MLOP', url mlop-ai.github.io, org mlop-ai. - No gh-pages branch exists and GitHub Pages is not enabled. - docs.yml never ran once, and could not: it triggers only on push to the nonexistent gh-pages branch, then runs `npm install && npm run build` at the repo root, which has no package.json. - No content change since #68 (Discord links). Also removed, all Docusaurus-only: - .github/workflows/docs.yml - scripts/docs-init.sh, scripts/docs-pub.sh (the latter deploys to git@github.com:mlop-ai/docs.git, the pre-rename org) Kept: docs/SYNC_PROCESS_V2_ARCHITECTURE.md moved to the repo root. It is 440 lines of internal architecture documentation that was never part of the site (Docusaurus served only docs/docs and docs/blog). Site branding assets are not lost — design/ holds the sources. The 13 markdown pages and 2 blog posts are pre-rename and superseded by the Mintlify docs; they remain recoverable from git history if any prose still needs migrating. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(deps): pin torchvision to 0.28.0 to match torch 2.13.0 CI caught a regression in the previous commit: torchvision silently resolved *backwards*, 0.22.1 -> 0.12.0 (a 2022 release). 0.12.0 ships only cp37-cp310 wheels, so `poetry install` failed with "Unable to find installation candidates for torchvision (0.12.0)" on Python 3.11/3.12/3.13 — breaking the test matrix and both contract jobs. 3.10 was unaffected, which is what made the wheel-tag gap obvious. Cause: torchvision pins torch exactly (0.28.0 -> torch==2.13.0), and its requires_python is "!=3.14.1,>=3.10". The project's python range is ^3.10, which *includes* 3.14.1, so no modern torchvision could satisfy the full range. Rather than fail, poetry fell back to the newest release whose requires_python covers everything — 0.12.0 (>=3.7). Bumping torch without torchvision in the same `poetry update` is what exposed this. Fix: mirror torchvision's own requires_python as a dependency marker so poetry can select 0.28.0, and give both packages explicit floors at the security minimums so neither can resolve backwards again. torch = ">=2.13.0" torchvision = { version = ">=0.28.0", python = "!=3.14.1,>=3.10" } Verified: torchvision 0.28.0 / torch 2.13.0, wheels present for cp310-cp314, zero downgrades anywhere in the lock vs main, and all 62 pip alerts still resolved. The 14 nvidia-*-cu12 packages are replaced by their cu13 equivalents (plus cuda-toolkit/bindings/pathfinder and nvshmem): torch 2.13.0 moves to CUDA 13 wheels. Expected, not a dropped dependency. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(log): key the console-setup guard on pluto's own handler pytest 9 exposed a latent bug that silently disabled console upload. setup_logger short-circuited on `len(console.handlers) > 0`, treating *any* handler on the console logger as proof pluto had already configured it. But setup_logger_file sets `console.propagate = False`, which forces anything capturing log records to attach at that logger rather than at the root — and pytest's logging plugin does exactly that from pytest 9 on. So from the second pluto.init() in a process onward, the guard saw pytest's LogCaptureHandlers, returned early, and never installed the ConsoleHandler wrappers. Nothing then enqueued console lines, and the fd layer could not cover for it either: print() writes to sys.stdout, which under pytest capture never reaches fd 1. Zero console records, no error — the same silent-and-total failure mode as the fd-capture bug in #140. Measured on tests/test_sync_process.py::TestSyncProcessShutdown, at the second init(): pytest 8.4.2 console.handlers == 0 -> setup runs pytest 9.1.1 console.handlers == 2 LogCaptureHandler -> setup skipped This is not a test-only problem. Any library that attaches a handler to that logger — Jupyter tooling, an APM agent, a user's own logging setup before a second init() — disables console upload for the rest of the process, in production, with no diagnostic. Fix: mark the one handler setup_logger_file attaches and have the guard look for that marker instead of for handlers in general. Capture behaviour is unchanged and the guard stays idempotent for pluto's own handler, which is what it was there for. Regression tests in tests/test_log_console_handler.py cover both directions: a foreign handler must not skip setup (fails against the old guard), and pluto's own handler must still short-circuit so the wrappers can't stack and double-enqueue. Verified against a local Pluto stack: test_sync_process.py 50 passed under pytest 9.1.1 and 8.4.2; the console/fd suites plus test_e2e_console_logs, 100 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(deps): cap torch/torchvision dev markers below Python 3.15 Per CodeRabbit review on #145. torch 2.13.0 (requires_python >=3.10) and torchvision 0.28.0 (!=3.14.1,>=3.10) both claim Python 3.15 in their metadata, but neither publishes a cp315 wheel — highest is cp314. The project constraint is ^3.10, which includes 3.15. So resolution succeeds and `poetry install --with dev` then fails on 3.15 with "Unable to find installation candidates" — the same failure this PR already hit once, when torchvision resolved back to 0.12.0 and broke the 3.11/3.12/3.13 matrix. Capping the markers makes both simply not apply on 3.15 instead of breaking the install. The review flagged torchvision only, since that was the line carrying a marker; torch has the identical wheel gap and no marker at all, so capping torchvision alone would have just moved the failure one package over. Both are capped. These are dev-group test deps, so this does not constrain what end users can install pluto-ml on — the project's own ^3.15-inclusive range is untouched deliberately. Capping *that* instead (the review's alternative suggestion) would block real users from installing on 3.15 to work around a test dependency. Lift the cap when upstream ships cp315 wheels. Marker-only change: no package version moves in the lock, and no downgrades against main. All 146 alerts still resolve; the console/fd suites still pass (99 passed). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Ubuntu <azureuser@ryansSandbox.iqigxx5hkb2uxpbdcmttsvsjcc.cx.internal.cloudapp.net> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

asaiacai temporarily deployed to integration August 5, 2026 20:39 — with GitHub Actions Inactive

asaiacai temporarily deployed to integration August 5, 2026 20:44 — with GitHub Actions Inactive

asaiacai temporarily deployed to integration August 5, 2026 21:00 — with GitHub Actions Inactive

asaiacai marked this pull request as ready for review August 6, 2026 17:53

ryanhayame approved these changes Aug 6, 2026

View reviewed changes

coderabbitai Bot reviewed Aug 6, 2026

View reviewed changes

Comment thread tests/test_fd_capture.py

asaiacai merged commit 2505206 into main Aug 6, 2026
18 checks passed

asaiacai deleted the claude/torchtitan-logs-partial-persist-cica4h branch August 6, 2026 17:55

cursor Bot reviewed Aug 6, 2026

View reviewed changes

Comment thread tests/test_fd_capture.py

cursor Bot mentioned this pull request Aug 6, 2026

test(log): exercise foreign fd-capture sentinel in reader path #141

Closed

2 tasks

ryanhayame mentioned this pull request Aug 17, 2026

chore(deps): resolve all 146 Dependabot security alerts #145

Merged

2 tasks

Navigation Menu

Uh oh!

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

fix(log): scope fd-capture stop() to the owning process#140
asaiacai merged 2 commits into
mainfrom
claude/torchtitan-logs-partial-persist-cica4h

asaiacai commented Aug 5, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 5, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

ryanhayame left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

cursor Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

asaiacai commented Aug 5, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The bug

The fix

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

ryanhayame left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

asaiacai commented Aug 5, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 5, 2026 •

edited

Loading