Repository navigation
feat(render): the render budget — the second half of Feature 1 - #143
Merged
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f425d088e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+158
to
+160
| # wait=False: the point of dropping is a wedged/crashed worker; | ||
| # cancel queued work and let the process die on its own schedule. | ||
| pool.shutdown(wait=False, cancel_futures=True) |
There was a problem hiding this comment.
When a template remains CPU-bound past the timeout—for example, nested loops that produce little or no output—shutdown(wait=False, cancel_futures=True) cancels only queued work and allows the running render to continue. Clearing _pool then lets the next request spawn another worker while the timed-out child still consumes CPU, so repeated requests can exhaust the node; Python's executor shutdown machinery can also wait for that old work during interpreter exit, defeating the watchdog-restart goal. Explicitly terminate or kill the running worker before replacing the pool.
Useful? React with 👍 / 👎.
| # Feature 1b: budgeted like the body render. RenderBudgetExceeded is | ||
| # a ValueError, so the tuple below already converts it to the same | ||
| # clean ConfigRenderError 422. | ||
| return render_bounded('file', text, greffon_info) |
There was a problem hiding this comment.
For an x-greffon-render JSON destination, _render_json_value invokes this bounded render independently for every string leaf, so each leaf receives a fresh input, output, and timeout budget. A small JSON array containing 100 templates that each expand just under the 4 MiB cap therefore accumulates roughly 400 MiB in the controller before json.dumps duplicates it, bypassing the protection and potentially OOM-killing the process. Track a shared budget/deadline across all leaves or cap the complete serialized destination before retaining it in the parent.
Useful? React with 👍 / 👎.
omarsy
force-pushed
the
feat/render-budget
branch
3 times, most recently
from
October 12, 2026 09:10
5b76ea2 to
3044717
Compare
compose-containment Feature 1b. The sandbox (#136) stops code execution and mutation; it does not bound resource use: '*' and '**' are not intercepted, so {{ 'A' * (10 ** 8) }} is sandbox-legal and Jinja CONSTANT-FOLDS it at COMPILE time — before any output exists, so an output cap alone cannot catch it, and the allocation lands in the --workers 1 controller that holds the manager token. apps/utils/docker/render_budget.py bounds the whole render, compile included: - input: the template SOURCE is capped (bytes, checked in the parent before any work); - output: accumulated via generate() and aborted on the first oversized chunk, IN THE CHILD; - memory: the render runs in a dedicated one-worker child process under RLIMIT_AS, so a compile-time fold (or any allocation blow-up) kills the CHILD and surfaces as a clean refusal — never the controller. RLIMIT_AS is Linux-only; on macOS dev the input/output/time caps hold and a hard child death maps to the same refusal (child_died); - time: the parent waits with a timeout; on expiry the pool is dropped and rebuilt so a wedged worker cannot poison the next render. Both production renders go through it (the compose body and baked file/json content). RenderBudgetExceeded is a ValueError SUBCLASS with a __reduce__ that survives the process boundary, so the existing except tuples at both call sites convert it to their ConfigRenderError 422s with zero new router wiring; the message is a machine code + cap, never rendered content. Pool teardown rides the lifespan finally (the shutdown_stats_pool pattern). Knobs: GREFFER_RENDER_MAX_INPUT (1 MiB), _MAX_OUTPUT (4 MiB), _MEM_BYTES (256 MiB), _TIMEOUT_SECONDS (5), 0 disables a dimension. Tests: caps (input refusal before work, mid-stream output abort, disabled dimensions, exception pickling), blow-ups (the compile-time fold — memory_exceeded/child_died/output_too_large by platform, a Linux-only RLIMIT assertion, timeout fires + pool recycles), and the wiring at both call sites mapping to ConfigRenderError. The #136 render ORACLE passes through the budgeted path unchanged — byte-identical snapshots across the catalog, the regression bar for touching this path. Suite: 1065 passed; only the two known pre-existing-on-main #136 test failures remain. Adversarial review round (DO NOT SHIP) — all folded: P1-1 timeout leaked the churning child (measured: 49.9s interpreter-exit stall via the executor's atexit join — wedging the watchdog restart path, the exact bar this feature exists to meet). _drop_pool_locked now KILLS the discarded pool's workers (robust to the _processes shape changing across versions), PR_SET_PDEATHSIG makes a child never outlive the parent on Linux, and a regression test asserts the worker is dead after a timeout drop and the pool recycled. P1-2 the hardened envs MOVED from compose.py to render_budget.py: the spawn child imported compose (module-scope docker.from_env — a live daemon handshake) turning budget refusals into DockerException 500s and re-paying 0.5s per pool rebuild on macOS dev. compose re-exports both names for existing references; the child now imports no docker client. P2s: output cap counts BYTES (astral chars passed 4x the cap) + test; the exception crossing the pickle boundary is trimmed to 8KB with a fallback — and the trim's first version caught its OWN raise in the try (the sandbox suite caught every error becoming render_child_error; construct-then-raise fixes it, exceptions keep their types); 'which' validated against a typo silently selecting the StrictUndefined env + test; the four knobs bound in Settings with typo-coercing validators (the house precedent); dead _CHILD_MEM_LIMIT store removed; fork pinned explicitly on Linux with the inherited-VSZ headroom note (~106MB measured vs 256MB cap; re-measure on a base-image swap); timeout-disabled foot-gun documented. Suite: 1067 passed — only the two known pre-existing-on-main #136 failures. Oracle byte-identical through the budgeted path. Codex CLI review round (P1 + 4 P2s) — all folded: P1: the kill was a NO-OP — shutdown(wait=False) clears _processes synchronously, so the handle lookup after it iterated nothing and the churning worker survived (codex confirmed by reproduction). Handles are captured BEFORE shutdown; the regression test now also asserts the PID is unsignalable, so a future shape change cannot silently no-op again. P2s: only the FAILED executor is recycled (a concurrent timeout could drop a healthy replacement pool and 500 unrelated in-flight renders); submit runs under the pool lock with a bounded retry when the pool was recycled between selection and submission (RuntimeError previously escaped the baked-file tuple as a 500); json-destination rendering batches the WHOLE tree into one child job (per-leaf submission re- pickled the full context per leaf — quadratic; a 5k-leaf doc measured 10.4s vs 0.6s); non-finite timeout values (inf/nan/1e999) fall back to the default in both _knob and the Settings validator (inf made Future.result raise OverflowError; nan timed out instantly). Suite: 1067 passed — only the two known pre-existing-on-main failures. Verification round (ONE MORE ROUND, two P2s the codex fix itself introduced) — folded with the engine consolidated: - except RuntimeError sat ABOVE except BrokenProcessPool (a RuntimeError SUBCLASS): the child_died clause was dead code and a hard child death (the production OOM-kill shape) escaped raw as a 500 on the baked/json paths. Reordered; regression test kills a worker with SIGKILL and asserts the refusal. - A PENDING future cancelled by another caller's pool drop raised bare CancelledError (in no catch tuple → 500). Now retried on the fresh pool or refused; regression test hammers 8 concurrent renders under forced recycles and asserts nothing escapes. - Both submit ladders consolidated into ONE shared _submit_budgeted engine (the duplicated except-ladders had drifted apart across fix rounds — the consolidation makes that class of bug impossible), with BOUNDED retries (3) replacing the unbounded while-True. - The prctl PDEATHSIG guard crashed the CHILD INITIALIZER on macOS (dlsym: no prctl symbol → AttributeError, outside the except tuple), turning every render into child_died — guarded via getattr. - Per-leaf input cap added in _child_render_json (the docstring claimed it; now true). Suite: 1022 passed (test_workers_register ignored — its asyncio tests hang in this local environment, proven pre-existing at merge-base); failures exactly the two known pre-existing-on-main #136 tests. Oracle byte-identical through the budgeted path.
omarsy
force-pushed
the
feat/render-budget
branch
from
October 12, 2026 10:49
3044717 to
aefc514
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
compose-containment Feature 1b — the second half of Feature 1, which the sandbox (#136) deliberately deferred: "a blow-up must not be able to take down the controller process."
Why a sandbox isn't enough:
*and**are not intercepted, so{{ 'A' * (10 ** 8) }}is sandbox-legal — and Jinja constant-folds it at compile time, allocating before any output exists, inside the--workers 1controller that holds the manager token.The budget (
apps/utils/docker/render_budget.py) bounds the whole render, compile included, with isolation as the load-bearing half:generate(), aborted mid-stream in the child;RLIMIT_AS— a compile-time fold kills the child, surfaces as a clean refusal, never the controller (Linux full guard; on macOS dev the input/output/time caps hold, hard child death maps to the same refusal);Both production renders (compose body + baked file/json) go through it.
RenderBudgetExceededis aValueErrorsubclass with a__reduce__that survives the process boundary — the existingexcepttuples at both call sites convert it to theirConfigRenderError422s with zero new router wiring; messages are machine codes + caps, never rendered content. Pool teardown rides the lifespanfinally. Knobs:GREFFER_RENDER_MAX_INPUT(1 MiB) /_MAX_OUTPUT(4 MiB) /_MEM_BYTES(256 MiB) /_TIMEOUT_SECONDS(5).Regression bar met: #136's render oracle passes through the budgeted path with byte-identical snapshots across the catalog. 10 new tests (caps, the compile-time fold per-platform, timeout+recycle, both wiring sites, exception pickling). Suite: 1065 passed — only the two known pre-existing-on-main #136 test failures remain (never green anywhere; CI has been billing-locked since before #136 merged).