fix: reject nested run_app invocations - #370
codeforester wants to merge 1 commit into
Conversation
| "invoke each CLI in a separate process or serialize calls.", | ||
| file=sys.stderr, | ||
| ) | ||
| return ExitCode.FAILURE |
There was a problem hiding this comment.
Correctness: nested/concurrent run_app() rejection bypasses the JSON output contract
Every other exit path in run_app honors state.json_output: when JSON output is requested, errors are written as a JSON envelope on stdout via _emit_json_error (see the click.Abort, click.ClickException, KeyboardInterrupt, SystemExit, JsonCaptureLimitError, and generic Exception branches below). This new early-return path (lines 114-129) is the only exit that unconditionally prints plain text to sys.stderr and returns ExitCode.FAILURE with nothing written to stdout.
Failure scenario: a consumer CLI configured for --json output hits a nested or concurrent run_app() call (e.g. a callback that recursively invokes run_app, or two threads racing). The rejected call returns ExitCode.FAILURE with empty stdout and an unstructured ERROR: ... line on stderr instead of a base-cli.output envelope. A machine consumer that always expects a JSON envelope on stdout (per docs/strict-json-consumer.md) gets nothing to parse and must special-case this one failure mode.
Note this is knowable for the nested branch at least: active_state.json_output is already available at line 118 (from the outer invocation's _InvocationState), but isn't consulted before choosing the plain-text print.
| from .redaction import option_aliases_from_decls | ||
|
|
||
| _MAX_JSON_CAPTURE_BYTES = 8 * 1_048_576 | ||
| _RUN_APP_LOCK = Lock() |
There was a problem hiding this comment.
Altitude: the guard is a single process-wide lock, broader than the root cause it fixes
Issue #341's actual defect is that configure_logger()/cleanup() key off one process-global logger keyed only by CLI name, so only a same-identity nested/concurrent call can corrupt the outer invocation's handlers. This lock (_RUN_APP_LOCK) is a single global mutex shared by every App instance, so it also rejects two different, unrelated CLI identities that try to run concurrently or nested in the same process — a combination that wouldn't actually collide on the shared logger state described in the issue.
Since _INVOCATION_STATE already carries owner_app, the narrower/root-cause fix would key the guard (or at least the message logic) by app.name/identity rather than blocking all invocations process-wide. As written, an embedder that legitimately runs two independent, differently-named Apps concurrently on separate threads (previously safe, since they don't share a logger key) now gets one of them rejected with ExitCode.FAILURE for no collision-related reason. This is called out as intentional in the README ("only one active invocation per process"), but it goes beyond what the linked issue's acceptance criteria required and forecloses a previously-safe usage pattern.
Summary
run_app()calls before they enter Click or alter stdout/logging state.Fixes #341.
Validation
git diff --check.