fix(pty): stop the non-blocking write losing input silently (#17381)

[mrveiss]

Thinking Path

master_fd is non-blocking — os.set_blocking(self.master_fd, False) in
_spawn. The write loop then called os.write(self.master_fd, data) once,
discarded the return value, under a broad except Exception: log; break.

A non-blocking write expresses two outcomes that a single discarded call cannot:

What os.write does	What the old code did
returns how many bytes it took, which may be fewer than given	dropped the remainder
raises BlockingIOError meaning buffer full, not now	caught it, logged, break — ending the write thread for the session's life

Both lose user input silently. The second is the more visible failure and
the first is the worse one: the read loop keeps streaming output either way, so
the terminal looks healthy — but a truncated write corrupts quietly, at a byte
boundary that can split a multi-byte UTF-8 character, while a dead write thread
at least stops responding.

What Changed

File	Change
autobot-backend/services/simple_pty.py	new _write_all(); cleanup() fd-close ordering; _PROC_TERM_GRACE_S split out
autobot-backend/services/simple_pty_write_all_17381_test.py	new, 5 tests against a real non-blocking pipe

_write_all loops on the remainder and treats EAGAIN as backpressure.

The bound is time since last PROGRESS, and that distinction was a bug I shipped

The first version of this fix bounded retrying by total elapsed time. That
looks equivalent to a stall bound, passes every small-input test, and truncates
exactly the large write the method exists to deliver — a 1 MiB paste needs many
buffer-fills and blows a 5s cap, so _write_all re-raised and the data was
lost. I would have reintroduced this very bug, for large inputs only.

It surfaced because one test uses a payload large enough to span multiple
buffer-fills; a 4 KB payload passes against both designs. That requirement is
now pinned in the test's docstring, because the test is worthless if someone
later shrinks the payload for speed.

_WRITE_STALL_BUDGET_S now resets on every byte accepted, so it detects a child
that has stopped reading rather than capping how long a legitimate write may
take. On exhaustion it re-raises deliberately: a child that has not accepted
a byte in that long is wedged, and failing loudly beats retrying forever.

Two smaller defects in the same file, from the same review that found the first:

• TOCTOU in cleanup() — it closed the descriptor and set master_fd = None
  afterwards, leaving a window where the write loop could use a closed fd and,
  once the number was reused, one belonging to something else. It now publishes
  None first and closes the fd it took.
• MICRO_DELAY served two unrelated timings — the write loop's idle bound
  and the post-SIGTERM grace period. One constant for two purposes means
  tuning either silently moves the other. The grace period is now
  _PROC_TERM_GRACE_S; same SSOT value today, separate names.

Verification

Tests run against a real non-blocking pipe, not a patched os.write. A fake
can only reproduce the semantics someone believed; a pipe whose buffer is
genuinely full produces the real short write and the real EAGAIN and cannot
drift from what the kernel does. It also avoids patching a global that pytest
itself writes through.

Mutation checked — replace _write_all's body with a single os.write(fd, data):

Test	Result under that mutation
every byte of a 1 MiB write arrives	fails — tail lost to the short write
a full buffer is backpressure, not failure	fails — raises BlockingIOError

Pre-push hook output is the evidence for the suite passing; a local run is not
CI's run, and CI is the authority.

Acceptance criteria

Criterion	Status
BlockingIOError does not terminate the loop; data is retried and delivered	met — test_a_full_buffer_is_backpressure_not_failure
a short write is completed, verified with input larger than the pipe buffer	met — 1 MiB payload, not a short string
a multi-byte UTF-8 sequence spanning a partial write arrives intact	met — test_a_multibyte_character_survives_a_split_write
the broad except no longer catches retryable I/O; a fatal error still ends the loop	met with a stated caveat, below
tests fail against the current code	met — mutation table above

The caveat, stated rather than ticked silently. A BlockingIOError that
survives a full _WRITE_STALL_BUDGET_S with no byte accepted is re-raised and
does reach the broad handler, which still breaks. That is deliberate: after
a full budget during which the child accepted nothing at all, the condition is
no longer retryable in any useful sense, and the alternative is a writer thread
that never returns. So the criterion is
met in the sense that matters — transient backpressure no longer kills the
session — but it is not literally true that the handler can never see an
EAGAIN. Reading it the literal way would require an unbounded retry, which is
a worse bug than the one being fixed.

Closes #17381

Model Used

Claude Opus 5 (1M context).

[coderabbitai]

Warning

Review limit reached

Next included review available in 34 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: b744d3f3-096c-46c0-a811-84fb03fde60e

📥 Commits

Reviewing files that changed from the base of the PR and between f277463 and cb98ed3.

📒 Files selected for processing (2)

• autobot-backend/services/simple_pty.py
• autobot-backend/services/simple_pty_write_all_17381_test.py

---

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.}

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.