reconciler: drain stdout to avoid deadlock

[ideaship]

Problem

run_on_change() starts /run.sh with stdout=subprocess.PIPE but never reads the pipe and calls p.wait() with no timeout. run() has the same undrained pipe on the publish=False path. Because the pipe is never consumed, a /run.sh that writes more than the OS pipe buffer (~64 KB) blocks on write while the parent blocks forever on wait() — deadlocking the reconciler worker (run_on_change unbounded; run(publish=False) bounded only by its wait(timeout=60), so it errors rather than hangs).

Fix

• run() — always drain stdout by moving the read loop back outside the publish guard; forwarding via push_task_output/finish_task_output stays gated on publish. This matches the always-drain pattern used by run_ansible_in_environment and run_command.
• run_on_change() — drain stdout into the log (the task publishes nowhere) and add wait(timeout=60) as a reap backstop, matching run().

stdout=subprocess.PIPE is retained on both paths — the pipe is required in order to drain it.

Tests

The two affected unit tests now feed real byte streams through the production TextIOWrapper drain loop and assert the pipe is consumed, rather than pinning the previous undrained call shape.

• Target module: 47 passed
• Full suite: 1572 passed
• black --check and flake8: clean

🤖 Generated with Claude Code

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• In both run and run_on_change, consider wrapping io.TextIOWrapper(p.stdout, ...) in a context manager so the wrapper (and underlying pipe) are explicitly closed after draining to avoid relying on GC for cleanup.
• In run_on_change, logger.info(line.rstrip()) will strip all trailing whitespace, not just the newline; if preserving message spacing matters, use rstrip("\n") instead.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In both `run` and `run_on_change`, consider wrapping `io.TextIOWrapper(p.stdout, ...)` in a context manager so the wrapper (and underlying pipe) are explicitly closed after draining to avoid relying on GC for cleanup.
- In `run_on_change`, `logger.info(line.rstrip())` will strip all trailing whitespace, not just the newline; if preserving message spacing matters, use `rstrip("\n")` instead.

## Individual Comments

### Comment 1
<location path="tests/unit/tasks/test_task_wrappers.py" line_range="545" />
<code_context>
     )
     mocker.patch("osism.tasks.reconciler.utils.create_redlock", return_value=lock)
     proc = mocker.MagicMock()
+    proc.stdout = io.BytesIO(b"")
     mocker.patch("osism.tasks.reconciler.subprocess.Popen", return_value=proc)

</code_context>
<issue_to_address>
**issue (testing):** Extend the lock-already-released test to assert stdout is drained and wait uses the timeout, matching the new behavior.

The test now sets `proc.stdout = io.BytesIO(b"")`, but it doesn’t verify stdout is drained or that `wait` uses the timeout. Since `run_on_change` always drains stdout and calls `p.wait(timeout=60)`, please add assertions like `assert proc.stdout.closed` and `proc.wait.assert_called_once_with(timeout=60)` so the lock-already-released path also validates this behavior.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (testing): Extend the lock-already-released test to assert stdout is drained and wait uses the timeout, matching the new behavior.

The test now sets proc.stdout = io.BytesIO(b""), but it doesn’t verify stdout is drained or that wait uses the timeout. Since run_on_change always drains stdout and calls p.wait(timeout=60), please add assertions like assert proc.stdout.closed and proc.wait.assert_called_once_with(timeout=60) so the lock-already-released path also validates this behavior.