Skip to content

Commit 40a5bf3

Browse files
committed
Let the backends own their teardown budget and no-input story
Each backend now publishes the teardown budget its own constants add up to and the CLI waits out the longest one reachable on this platform, so a ConPTY teardown is no longer reported as a render that did not stop. The absent-input note comes from the backend that ran rather than from sys.platform, which was wrong under the escape hatch on Windows. The POSIX grace probes the process group each tick and ends early once it is spent, and the precondition check asks each repository once for the whole list of previous frids.
1 parent 90c9ab1 commit 40a5bf3

12 files changed

Lines changed: 237 additions & 75 deletions

git_utils.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -331,6 +331,19 @@ def has_commit_for_frid(repo_path: Union[str, os.PathLike], frid: str, module_na
331331
return bool(_get_commit_with_frid(repo, frid, module_name))
332332

333333

334+
def frids_missing_commits(
335+
repo_path: Union[str, os.PathLike], frids: list[str], module_name: Optional[str] = None
336+
) -> list[str]:
337+
"""The frids from `frids` with no commit in the repository, in the order given.
338+
339+
One Repo answers for the whole list. Asking per frid instead opens and closes a Repo
340+
each time, and close() runs gc.collect() twice on win32, so a render resumed late paid
341+
that for every functionality before it.
342+
"""
343+
with Repo(repo_path) as repo:
344+
return [frid for frid in frids if not _get_commit_with_frid(repo, frid, module_name)]
345+
346+
334347
def _get_base_folder_commit(repo: Repo) -> str:
335348
"""Finds commit related to copy of the base folder."""
336349
return _get_commit_with_message(repo, BASE_FOLDER_COMMIT_MESSAGE)

plain2code.py

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -51,30 +51,20 @@
5151
)
5252
from plain2code_state import RunState
5353
from plain2code_telemetry import capture_crash, initialize_telemetry
54-
from render_machine.terminal_process import (
55-
DRAIN_DEADLINE_SECONDS,
56-
REAP_DEADLINE_SECONDS,
57-
SIGTERM_GRACE_PERIOD_SECONDS,
58-
)
54+
from render_machine.terminal_process import teardown_budget_seconds
5955
from system_config import system_config
6056
from tui.plain2code_tui import Plain2CodeTUI
6157
from tui.plain_module_render_choice_tui import PlainModuleRenderChoiceTUI
6258

6359
DEFAULT_TEMPLATE_DIRS = "standard_template_library"
6460

6561
# The render thread is cancelled, never killed, so the wait after cancellation has to
66-
# outlast the teardown a script execution is entitled to: the SIGTERM grace runs to its
67-
# end before the SIGKILL, the killed group is then reaped, and the output reader is joined
68-
# on its own drain and reap budgets. A shorter wait lets the CLI exit mid-escalation and
69-
# leave a descendant that ignores TERM alive, so the bound is those budgets in sequence.
62+
# outlast the teardown a script execution is entitled to. Each backend adds its own phases
63+
# up and publishes the total, and the longest one reachable on this platform is what has to
64+
# be waited out: a shorter wait lets the CLI exit mid-escalation and leave a descendant that
65+
# ignores TERM alive.
7066
RENDER_THREAD_UNWIND_MARGIN_SECONDS = 1.0
71-
RENDER_THREAD_SHUTDOWN_TIMEOUT = (
72-
SIGTERM_GRACE_PERIOD_SECONDS
73-
+ REAP_DEADLINE_SECONDS
74-
+ DRAIN_DEADLINE_SECONDS
75-
+ REAP_DEADLINE_SECONDS
76-
+ RENDER_THREAD_UNWIND_MARGIN_SECONDS
77-
)
67+
RENDER_THREAD_SHUTDOWN_TIMEOUT = teardown_budget_seconds() + RENDER_THREAD_UNWIND_MARGIN_SECONDS
7868

7969
# Exceptions that represent expected, user-facing error conditions. They are
8070
# reported to the user directly and must never be sent to Sentry as crashes.

plain_modules.py

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -283,34 +283,44 @@ def _ensure_module_folders_exist(self, first_render_frid: str, render_conformanc
283283
f" codeplain {self.module_name}{plain_file.PLAIN_SOURCE_FILE_EXTENSION}"
284284
)
285285

286-
def _ensure_frid_commit_exists(
286+
def _raise_for_missing_frid_commits(
287287
self,
288-
frid: str,
288+
previous_frids: list[str],
289289
first_render_frid: str,
290290
render_conformance_tests: bool,
291291
) -> None:
292292
"""
293-
Ensure commit exists for a single FRID in both repositories.
293+
Ensure commits exist for every previous FRID in both repositories.
294+
295+
Each repository is asked once for the whole list rather than once per FRID, and the
296+
first FRID that is missing anywhere decides the error, in the order given.
294297
295298
Args:
296-
frid: The FRID to check
299+
previous_frids: The FRIDs that should already have been rendered
297300
first_render_frid: The first FRID in the render range (for error messages)
298301
render_conformance_tests: Whether to check for conformance tests
299302
300303
Raises:
301-
MissingPreviousFridCommitsError: If the commit is missing
304+
MissingPreviousFunctionalitiesError: If any commit is missing
302305
"""
303-
# Check in build folder
304-
if not git_utils.has_commit_for_frid(self.module_build_folder, frid, self.module_name):
305-
raise MissingPreviousFunctionalitiesError(
306-
f"Cannot start rendering from functionality {first_render_frid} for module '{self.module_name}' because the implementation of the previous functionality ({frid}) hasn't been completed yet.\n\n"
307-
f"To fix this, please render the missing functionality ({frid}) first by running:\n"
308-
f" codeplain {self.module_name}{plain_file.PLAIN_SOURCE_FILE_EXTENSION} --render-from {frid}"
306+
missing_in_build = set(
307+
git_utils.frids_missing_commits(self.module_build_folder, previous_frids, self.module_name)
308+
)
309+
missing_in_tests = set()
310+
if render_conformance_tests:
311+
missing_in_tests = set(
312+
git_utils.frids_missing_commits(self.module_conformance_tests_folder, previous_frids, self.module_name)
309313
)
310314

311-
# Check in conformance tests folder (only if conformance tests are enabled)
312-
if render_conformance_tests:
313-
if not git_utils.has_commit_for_frid(self.module_conformance_tests_folder, frid, self.module_name):
315+
for frid in previous_frids:
316+
if frid in missing_in_build:
317+
raise MissingPreviousFunctionalitiesError(
318+
f"Cannot start rendering from functionality {first_render_frid} for module '{self.module_name}' because the implementation of the previous functionality ({frid}) hasn't been completed yet.\n\n"
319+
f"To fix this, please render the missing functionality ({frid}) first by running:\n"
320+
f" codeplain {self.module_name}{plain_file.PLAIN_SOURCE_FILE_EXTENSION} --render-from {frid}"
321+
)
322+
323+
if frid in missing_in_tests:
314324
raise MissingPreviousFunctionalitiesError(
315325
f"Cannot start rendering from functionality {first_render_frid} for module '{self.module_name}' because the conformance tests for the previous functionality ({frid}) haven't been completed yet.\n\n"
316326
f"To fix this, please render the missing functionality ({frid}) first by running:\n"
@@ -342,8 +352,7 @@ def ensure_previous_frid_commits_exist(self, render_range: list[str], render_con
342352
self._ensure_module_folders_exist(first_render_frid, render_conformance_tests)
343353

344354
# Verify commits exist for all previous FRIDs
345-
for prev_frid in previous_frids:
346-
self._ensure_frid_commit_exists(prev_frid, first_render_frid, render_conformance_tests)
355+
self._raise_for_missing_frid_commits(previous_frids, first_render_frid, render_conformance_tests)
347356

348357
def get_required_module_by_name(self, module_name: str) -> PlainModule:
349358
for module in self.all_required_modules:

render_machine/_conpty.py

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,9 @@
5656
DRAIN_DEADLINE_SECONDS,
5757
GRACE_TICK_SECONDS,
5858
HANDSHAKE_TIMEOUT_SECONDS,
59+
)
60+
from render_machine.terminal_process import NO_INPUT_NOTE as DEFAULT_NO_INPUT_NOTE
61+
from render_machine.terminal_process import (
5962
OWNER_PARENT,
6063
OWNER_READER,
6164
POLL_INTERVAL_SECONDS,
@@ -139,11 +142,33 @@
139142
FINALIZER_DEADLINE_SECONDS = 60.0
140143
FINALIZER_TICK_SECONDS = 0.5
141144

145+
# What one full teardown of this backend may spend, phase by phase and in sequence. The
146+
# pipeline is longer than the POSIX one — a control byte has to be delivered before the
147+
# grace it earns, the job's membership is waited out, the writer is stopped, and each
148+
# join_reader() round is bounded twice: a join on the bound, then a cancel-and-join loop
149+
# under the same bound again. A caller waiting on a render derives its own bound from this,
150+
# so it cannot report a stuck teardown while the backend is still inside its own budget.
151+
TEARDOWN_BUDGET_SECONDS = (
152+
CONTROL_DELIVERY_DEADLINE_SECONDS # teardown(): delivering the graceful control byte
153+
+ SIGTERM_GRACE_PERIOD_SECONDS # teardown(): the grace a delivered byte earns
154+
+ REAP_DEADLINE_SECONDS # teardown(): waiting for the job's membership to reach zero
155+
+ WRITER_JOIN_DEADLINE_SECONDS # teardown(): stopping the input writer
156+
+ 2 * DRAIN_DEADLINE_SECONDS # teardown(): join_reader() inside _close_pseudoconsole()
157+
+ 2 * DRAIN_DEADLINE_SECONDS # close(): the join_reader() that follows the stack close
158+
)
159+
142160
# The graceful signal: writing 0x03 into the pseudoconsole input is how terminal emulators
143161
# deliver Ctrl-C to a ConPTY client. `GenerateConsoleCtrlEvent` cannot be used, because it
144162
# reaches only processes sharing the caller's console and the target is on the pseudoconsole.
145163
CONTROL_C_BYTE = b"\x03"
146164

165+
# The absent-input note this backend adds to, stated where the asymmetry is documented: a
166+
# script that reads input blocks until the execution timeout rather than seeing end-of-file.
167+
NO_INPUT_NOTE = DEFAULT_NO_INPUT_NOTE + (
168+
" On Windows the terminal carries no synthetic end-of-file, so such a script blocks until the "
169+
"timeout instead of reading end-of-file."
170+
)
171+
147172

148173
class COORD(ctypes.Structure):
149174
_fields_ = [("X", ctypes.c_short), ("Y", ctypes.c_short)]
@@ -1077,6 +1102,9 @@ def write_input(self, data: bytes) -> InputWriteResult:
10771102
result, _ = self._input_queue.submit(data)
10781103
return result
10791104

1105+
def no_input_note(self) -> str:
1106+
return NO_INPUT_NOTE
1107+
10801108
def infrastructure_failure(self) -> Optional[str]:
10811109
detail = super().infrastructure_failure()
10821110
if detail is not None:

render_machine/_legacy_pipe.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,16 @@
5858
# that inherited the write end keeps the pipe open past the leader's exit.
5959
CLOSE_JOIN_SECONDS = 1.0
6060

61+
# What one full teardown of this backend may spend, phase by phase and in sequence. A
62+
# caller waiting on a render derives its own bound from this, so it cannot report a stuck
63+
# teardown while the backend is still inside the budget its own constants grant it.
64+
TEARDOWN_BUDGET_SECONDS = (
65+
SIGTERM_GRACE_PERIOD_SECONDS # terminate_tree(): the grace before the SIGKILL
66+
+ REAP_DEADLINE_SECONDS # terminate_tree(): reaping the killed process
67+
+ DRAIN_DEADLINE_SECONDS # close(): the first join, while the pipe is still open
68+
+ CLOSE_JOIN_SECONDS # close(): the second, after the pipe is closed under the reader
69+
)
70+
6171
# Windows gives a child the parent's console unless told otherwise, and a child on that
6272
# console can read the renderer's keystrokes through CONIN$ regardless of where its
6373
# standard input handle points. CREATE_NO_WINDOW gives it a console of its own instead.

render_machine/_posix_pty.py

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,16 @@
6868
# forks, so termination is immediate and the full grace would only slow failures down.
6969
ROLLBACK_GRACE_SECONDS = 0.1
7070

71+
# What one full teardown of this backend may spend, phase by phase and in sequence. A
72+
# caller waiting on a render derives its own bound from this, so it cannot report a stuck
73+
# teardown while the backend is still inside the budget its own constants grant it.
74+
TEARDOWN_BUDGET_SECONDS = (
75+
SIGTERM_GRACE_PERIOD_SECONDS # terminate_tree(): the grace before the SIGKILL
76+
+ REAP_DEADLINE_SECONDS # terminate_tree(): reaping the killed group
77+
+ DRAIN_DEADLINE_SECONDS # close(): the reader's final drain
78+
+ REAP_DEADLINE_SECONDS # close(): the rest of the same reader join
79+
)
80+
7181

7282
class _ProtocolError(Exception):
7383
"""The launcher's status stream did not follow the handshake protocol."""
@@ -82,20 +92,25 @@ def _close_quietly(fd: Optional[int]) -> None:
8292
pass
8393

8494

85-
def _signal_group(pgid: int, sig: int) -> None:
86-
"""The only killpg site in this module.
95+
def _signal_group(pgid: int, sig: int) -> bool:
96+
"""The only killpg site in this module. False once the group has nothing left to signal.
8797
8898
ESRCH: the group is gone.
8999
EPERM: verified on macOS — killpg() returns EPERM, not ESRCH, when the group's only
90100
remaining member is our own unreaped zombie leader. That is the NORMAL state after a
91101
graceful exit, so it must not raise.
102+
103+
Both are terminal for the group, which is what lets signal 0 serve as a liveness probe
104+
without a second killpg site.
92105
"""
93106
try:
94107
os.killpg(pgid, sig)
95108
except ProcessLookupError: # ESRCH — nothing left
96-
return
109+
return False
97110
except PermissionError: # EPERM — zombie-only group
98111
console.debug(f"killpg({pgid}, {sig}): EPERM, treating as terminal")
112+
return False
113+
return True
99114

100115

101116
def _background_reap(proc: subprocess.Popen) -> None:
@@ -548,6 +563,8 @@ def terminate_tree(self, grace: float = SIGTERM_GRACE_PERIOD_SECONDS) -> None:
548563
self._deliver(proc, pgid, signal.SIGCONT)
549564
deadline = time.monotonic() + grace # independent clock — NOT stop_event
550565
while time.monotonic() < deadline: # never waits on the leader either
566+
if self._group_spent(pgid):
567+
break # the tree handled the SIGTERM; the escalation still follows
551568
self._grace_tick()
552569
finally:
553570
# Unconditional: an interruption mid-grace must still escalate.
@@ -998,6 +1015,17 @@ def _deliver(self, proc: subprocess.Popen, pgid: Optional[int], sig: int) -> Non
9981015
def _grace_tick(self) -> None:
9991016
time.sleep(GRACE_TICK_SECONDS)
10001017

1018+
def _group_spent(self, pgid: Optional[int]) -> bool:
1019+
"""Signal 0 as a liveness probe: True once the group can no longer be signalled.
1020+
1021+
Only the grace loop uses it, and only to stop waiting early. Nothing is reaped here
1022+
— the SIGKILL and the reap that follow are unconditional — because reaping before
1023+
the escalation would recycle the group the escalation still has to reach.
1024+
"""
1025+
if pgid is None: # pre-ack: no group recorded, so the grace runs to its end
1026+
return False
1027+
return not _signal_group(pgid, 0)
1028+
10011029
def _rollback(self) -> None:
10021030
try:
10031031
if self._proc is not None:

render_machine/render_utils.py

Lines changed: 14 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
from plain2code_exceptions import RenderCancelledError
1111
from render_machine.terminal_process import (
1212
ENVIRONMENT_ERROR_EXIT_CODE,
13+
NO_INPUT_NOTE,
1314
TerminalProcess,
1415
TerminalProcessError,
1516
create_terminal_process,
@@ -28,30 +29,6 @@
2829
# than on bytes written: a script that blocks on input has written nothing either way.
2930
INPUT_DRIVER: Optional[object] = None
3031

31-
NO_INPUT_DIAGNOSTIC_BASE = (
32-
" No input driver was attached to the script's terminal, so a script that waits for input "
33-
"never receives any and runs to the timeout."
34-
)
35-
36-
# A documented platform asymmetry, not an implementation detail. POSIX injects the
37-
# terminal's EOF byte at spawn when no input driver is attached, so a script that reads
38-
# input sees end-of-file at once. ConPTY has no parent-side equivalent that leaves the input
39-
# channel open, and the channel has to stay open for the graceful control byte and for
40-
# terminal-query replies, so the same script blocks until the timeout.
41-
WINDOWS_NO_EOF_DIAGNOSTIC = (
42-
" On Windows the terminal carries no synthetic end-of-file, so such a script blocks until the "
43-
"timeout instead of reading end-of-file."
44-
)
45-
46-
47-
def no_input_diagnostic(platform: str) -> str:
48-
if platform == "win32":
49-
return NO_INPUT_DIAGNOSTIC_BASE + WINDOWS_NO_EOF_DIAGNOSTIC
50-
return NO_INPUT_DIAGNOSTIC_BASE
51-
52-
53-
NO_INPUT_DIAGNOSTIC = no_input_diagnostic(sys.platform)
54-
5532
# Conditions the arbiter chooses between, highest precedence last.
5633
CONDITION_EXIT = "exit"
5734
CONDITION_TIMEOUT = "timeout"
@@ -148,6 +125,9 @@ def __init__(self) -> None:
148125
self.raw_output = b""
149126
self.reply_failed = False
150127
self.reply_detail = ""
128+
# The backend that ran states this itself. Keyed on the platform it would describe
129+
# the wrong backend whenever the escape hatch selected another one.
130+
self.no_input_note = NO_INPUT_NOTE
151131

152132

153133
def _await_target(
@@ -221,6 +201,7 @@ def _collect_backend_state(process: TerminalProcess, execution: _ScriptExecution
221201
execution.raw_output = process.read_raw_output()
222202
execution.reply_failed = process.terminal_reply_failed
223203
execution.reply_detail = process.terminal_reply_detail()
204+
execution.no_input_note = process.no_input_note()
224205
except Exception as exc:
225206
_record_backend_failure(execution.outcome, exc, "while reporting its result")
226207

@@ -340,8 +321,9 @@ def _publish_timeout(
340321
output: str,
341322
reply_failed: bool,
342323
reply_detail: str,
324+
no_input_note: str,
343325
) -> tuple[int, str, Optional[str]]:
344-
diagnostics = NO_INPUT_DIAGNOSTIC if INPUT_DRIVER is None else ""
326+
diagnostics = no_input_note if INPUT_DRIVER is None else ""
345327
if reply_failed:
346328
diagnostics += f" Terminal replies the script asked for could not be delivered: {reply_detail}."
347329

@@ -398,7 +380,13 @@ def execute_script(
398380
raise RenderCancelledError()
399381
elif outcome.condition == CONDITION_TIMEOUT:
400382
result = _publish_timeout(
401-
script, script_type, script_timeout, execution.output, execution.reply_failed, execution.reply_detail
383+
script,
384+
script_type,
385+
script_timeout,
386+
execution.output,
387+
execution.reply_failed,
388+
execution.reply_detail,
389+
execution.no_input_note,
402390
)
403391
elif outcome.exit_code is None:
404392
result = _publish_environment_error(

0 commit comments

Comments
 (0)