fix(tesla): prevent RSA key generation from blocking asyncio - #105
Conversation
get_rsa_private_key generated a 4096-bit RSA key synchronously in an async method, blocking the event loop for the duration of the CPU-bound prime search. Delegate it to a worker thread via asyncio.to_thread, matching the async contract callers (e.g. teslemetry's local-Powerwall pairing flow) already assume.
![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: 610e0d85da
ℹ️ 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".
| value = serialization.load_pem_private_key( | ||
| pem, password=None, backend=default_backend() | ||
| ) |
There was a problem hiding this comment.
On first-time key creation with the default 4096-bit size, load_pem_private_key performs synchronous RSA private-key validation on the event-loop thread; that validation can take hundreds of milliseconds, so this new reload leaves a substantial event-loop stall after process-based generation completes. The heartbeat test does not detect this contiguous stall because it only requires a few ticks during the entire subprocess operation. Deserialize without redundant validation for this freshly generated, trusted PEM, or otherwise move deserialization off the loop as well.
AGENTS.md reference: AGENTS.md:L114-L116
Useful? React with 👍 / 👎.
| _RSA_KEY_GENERATION_EXECUTOR = ProcessPoolExecutor( | ||
| max_workers=1, mp_context=get_context("spawn") | ||
| ) |
There was a problem hiding this comment.
When a new key is requested from an interactive environment such as Jupyter, python -c, or stdin, this forced spawn pool cannot import an actual __main__ file and the await fails with BrokenProcessPool; an ordinary script that invokes the async API at module scope similarly re-enters itself in the child and hits multiprocessing's bootstrapping error unless the caller adds an if __name__ == "__main__" guard. These contexts previously generated keys successfully, so the library should use an isolation mechanism that does not impose multiprocessing entry-point requirements on callers.
Useful? React with 👍 / 👎.
Codex flagged two real issues on the initial fix: the post-generation load_pem_private_key ran synchronously on the loop, and the spawn-based ProcessPoolExecutor was created at import time, breaking any interactive caller (REPL, `python -c`, notebooks) that imports this module. Verified directly (not assumed) that switching generation itself to asyncio.to_thread would not actually fix the original bug: cryptography's RSA keygen holds the GIL for its full duration in this environment, so a worker thread still stalls the caller's event loop just as much as an inline call - measured 0 heartbeat ticks either way, vs ~385 ticks for an equivalent pure-Python CPU-bound call over the same span. Process isolation is therefore kept for generation (the only approach that actually decouples it from the caller's event loop); the pool is now created lazily per call instead of at import time, and PEM deserialization now runs via asyncio.to_thread instead of inline on the loop.
Bre77
changed the title
|
Fixed the Codex P2 finding ( Before applying the literally-suggested fix (drop multiprocessing, run generation itself via Full suite (512 tests), pyright strict, and ruff are all clean; CI is green. |
|
Fixed the Codex P2 finding ( The pool is now created lazily inside While verifying this end-to-end I also found the existing Full suite (512 tests), pyright strict, and ruff are all clean; CI is green. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24d3e4e4c8
ℹ️ 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".
| `python -c`, or a notebook cell - surfaced here as a clear error instead of | ||
| an opaque BrokenProcessPool. | ||
| """ | ||
| pool = ProcessPoolExecutor(max_workers=1, mp_context=get_context("spawn")) |
There was a problem hiding this comment.
When a caller runs with umask(0o777), the spawned pool cannot create a usable multiprocessing semaphore, so first-time RSA key generation now fails before the key file is written. This configuration was explicitly supported by the pre-change test_new_key_file_is_owner_only_with_restrictive_umask; changing that test to 0o077 only hides the regression rather than preserving the previous behavior for hardened processes.
Useful? React with 👍 / 👎.
Codex correctly flagged that changing the restrictive-umask test to 0o077 hid a real regression: under umask 0o777 (and similarly, any environment that can't start a spawn-based worker process, such as a REPL/`python -c`/ notebook caller) generation now failed outright, where it previously succeeded (just by blocking the loop). Broadened the existing BrokenProcessPool handling into a general fallback: any failure to acquire/start the isolated worker process (BrokenProcessPool, or an OSError such as the semaphore PermissionError under a restrictive umask) now falls back to in-process generation with a logged warning, restoring the legacy works-but-blocks behavior for these environments instead of raising. Restored the umask test to 0o777, now asserting success via the fallback and that the key file still lands owner-only.
Bre77
changed the title
|
Fixed the Codex P2 finding ( Rather than another carve-out, broadened the existing Full suite (513 tests), pyright strict, and ruff are all clean; CI is green. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e6893bf0f
ℹ️ 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".
| """ | ||
| try: | ||
| pem = await _generate_rsa_private_key_pem_isolated(key_size) | ||
| except (BrokenProcessPool, OSError) as err: |
There was a problem hiding this comment.
When get_rsa_private_key() is called inside a daemonic multiprocessing worker, ProcessPoolExecutor.submit() raises AssertionError("daemonic processes are not allowed to have children"), not BrokenProcessPool or OSError. Because this catch excludes that startup failure, key generation now aborts before writing the key even though the previous inline implementation worked in such workers; detect the daemon context or treat this assertion as unavailable process isolation and use the existing fallback.
Useful? React with 👍 / 👎.
…stic
Codex found a third pool-mechanics failure outside the prior round's
enumerated (BrokenProcessPool, OSError) catch: a caller already running as
a daemonic multiprocessing worker gets a bare AssertionError ("daemonic
processes are not allowed to have children") from Process.start(), which
was neither caught nor fell back.
Rather than adding a third type to the list, broadened the catch to
`except Exception` around pool creation and submission only - process
isolation is best-effort by design, so any way it can fail, present or
future, should degrade to the working (if blocking) in-process path
instead of needing another round to enumerate. The catch stays scoped to
_generate_rsa_private_key_pem_isolated's call; PEM deserialization
afterward is untouched. The warning now logs the exception's class name
alongside its message. Added a daemonic-worker test case.
Bre77
changed the title
|
Fixed the Codex P2 finding ( Per the request, this is now failure-mode-agnostic instead of another enumerated type: the catch around pool creation/submission is a blanket Full suite (514 tests), pyright strict, and ruff are all clean; CI is green. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2ec327de6
ℹ️ 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".
| environment that can't host a worker process - so the caller catches | ||
| broadly and falls back to in-process generation. | ||
| """ | ||
| pool = ProcessPoolExecutor(max_workers=1, mp_context=get_context("spawn")) |
There was a problem hiding this comment.
When a normal script calls get_rsa_private_key() at module scope without an if __name__ == "__main__" guard, forcing the spawn context makes the worker execute the caller's entire main module again, so unrelated startup side effects run twice and the nested key-generation call can also write the key before the parent does. Fresh evidence in this revision is that the broad fallback at lines 82–92 occurs only after spawn has already re-imported and executed the caller, so it restores a successful return but does not address the original re-entry regression; use isolation that does not import __main__, or detect this context before spawning.
Useful? React with 👍 / 👎.
…cess Codex identified the actual root cause behind rounds 2-5: spawn re-executes an unguarded caller's __main__ module, so side effects can run twice before any exception ever reaches the round-4 catch-all - the harm precedes the catch. multiprocessing's own machinery (spawn's __main__ re-import, pickling, semaphores, the daemonic-process restriction) was the entire class of finding, not any one exception type within it. Replaced ProcessPoolExecutor with a plain `sys.executable -c <script>` subprocess via asyncio.create_subprocess_exec: its -c script is its own __main__, so it never re-imports the caller's real entry point - nothing to guard, nothing to pickle, no semaphore, no daemonic restriction, and (confirmed live) no umask-sensitive IPC either. Kept the catch-all fallback to in-process generation on any launch/exit failure, off-loop deserialization, and file permission handling unchanged. Added explicit cancellation handling (kill + await wait, both async) so a cancelled caller can't leave an orphaned subprocess or block the loop either. Simplified the test matrix: the three separate multiprocessing-specific fallback tests collapse into two generic ones (launch failure, non-zero exit), and the cancellation test now exercises the subprocess protocol directly.
Bre77
changed the title
|
Credit to Codex here - this finding identified the actual root cause behind rounds 2-4's back-and-forth, not just another symptom of it: Replaced Review caught one more real issue on this round: Test matrix simplified as requested: three multiprocessing-specific fallback tests collapse into two generic ones (launch failure, non-zero exit). Full suite (514 tests), pyright strict, and ruff are all clean; CI is green. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 794a12e750
ℹ️ 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".
| proc = await asyncio.create_subprocess_exec( | ||
| sys.executable, | ||
| "-c", | ||
| _RSA_KEYGEN_SUBPROCESS_SCRIPT, |
There was a problem hiding this comment.
sys.executable -c
When this library runs from a PyInstaller/cx_Freeze-style bundle, sys.executable points to the frozen application rather than a standalone Python interpreter, so this command restarts the application instead of executing _RSA_KEYGEN_SUBPROCESS_SCRIPT. If key creation occurs during application startup, the child can recursively spawn more copies; if it exits successfully without producing a PEM, deserialization later fails outside the fallback block. Fresh evidence relative to the earlier caller-module concern is the new direct reliance on sys.executable being an interpreter; detect frozen runtimes and use the fallback before spawning, or invoke an isolation mechanism that does not re-enter the bundled application.
Useful? React with 👍 / 👎.
… output Codex found a real gap in the plain-subprocess design: in a PyInstaller/ cx_Freeze/py2exe bundle, sys.executable is the application itself, not a Python interpreter. Launching it with `-c <script>` would relaunch the whole application (a recursive-spawn risk at startup), and that relaunch can exit 0 without ever emitting a PEM - dodging the catch-all fallback entirely since nothing raises. Added a surgical pre-flight check: if sys.frozen (the de facto marker PyInstaller/cx_Freeze/py2exe all set), skip subprocess isolation entirely and go straight to the in-thread fallback with the usual warning - consistent with the existing best-effort-isolation contract, no special casing needed at the call site. Also hardened the success path per the same finding: an empty stdout (0 exit, no output) now raises before reaching deserialization, and deserialization itself (moved into a shared _deserialize_rsa_pem helper) is now inside the fallback's try/except, so a subprocess that exits 0 with non-empty but malformed output also triggers the fallback instead of propagating a ValueError or, worse, being trusted as real key material. Added tests for the frozen-bundle skip, empty-output, and invalid-PEM cases.
Bre77
changed the title
|
Fixed the Codex P2 finding ( Added a surgical pre-flight check: if Also hardened the success path per the same finding: an empty stdout now raises before ever reaching deserialization, and deserialization itself moved inside the fallback's Full suite (517 tests), pyright strict, and ruff are all clean; CI is green. |
1 participant
Intent
Address Codex round 6 on PR #105: a real gap in the round-5 plain-subprocess design. In a frozen bundle (PyInstaller, cx_Freeze, py2exe), sys.executable is the application itself, not a Python interpreter, so launching it with -c <script> would relaunch the whole application - a recursive-spawn risk at startup - and that relaunch can exit 0 without ever emitting a PEM, which dodges the existing catch-all fallback entirely since nothing raises to trigger it. Added a surgical pre-flight check in _generate_rsa_private_key_pem_isolated: if getattr(sys, 'frozen', False) (the de facto marker PyInstaller/cx_Freeze/py2exe all set - there is no other stdlib-documented equivalent beyond this one attribute), raise before ever attempting the subprocess, which flows through the exact same except-Exception-then-fallback path already in _generate_rsa_private_key - consistent with the best-effort-isolation contract, no special-casing needed at the call site. Also hardened the success path per the same finding, per instruction to treat an empty or invalid PEM from the child as failure rather than letting a wrong-but-zero-exit child poison deserialization: an empty stdout (returncode 0, no output) now raises immediately in the isolated helper before ever reaching deserialization; and deserialization itself (extracted into a new shared _deserialize_rsa_pem helper used by both the isolated and fallback paths) now happens INSIDE the try/except in _generate_rsa_private_key rather than after it, so a subprocess that exits 0 with non-empty but malformed/garbage output also triggers the warning+fallback instead of letting a ValueError from load_pem_private_key propagate uncaught. This keeps the happy path's cost unchanged (one deserialization) and only pays a second deserialization in the rare case where the isolated path's output turns out to be unusable. Added three tests: test_frozen_bundle_falls_back_to_in_process_generation (mocks sys.frozen=True, asserts create_subprocess_exec is never called and generation still succeeds via fallback with the warning logged), test_empty_subprocess_output_falls_back_to_in_process_generation (a fake process with returncode 0 and empty stdout/stderr), and test_invalid_subprocess_pem_falls_back_to_in_process_generation (returncode 0 with non-empty but non-PEM stdout). All three assert success via fallback, the key file lands owner-only, and the warning log mentions the isolated subprocess. Full suite (517 tests), pyright strict, and ruff all pass locally; manually verified end-to-end that setting sys.frozen=True triggers the warning and successful fallback.
What Changed
Risk Assessment
✅ Low: The frozen-runtime preflight and shared deserialization path close the stated recursive-spawn and invalid-output gaps while preserving cancellation and fallback behavior.
Testing
Targeted tests exercised frozen-app subprocess avoidance, zero-exit empty output, and malformed non-empty output; all successfully fell back. An end-to-end transcript confirms the frozen path logs the expected warning, persists a valid matching RSA key, and creates it with owner-only mode
0600; transient worktree test artifacts were removed.Evidence: Frozen-bundle fallback transcript
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
uv run pytest -q tests/test_tesla_private_key.py::GetRsaPrivateKeyPermissionsTests::test_frozen_bundle_falls_back_to_in_process_generation tests/test_tesla_private_key.py::GetRsaPrivateKeyPermissionsTests::test_empty_subprocess_output_falls_back_to_in_process_generation tests/test_tesla_private_key.py::GetRsaPrivateKeyPermissionsTests::test_invalid_subprocess_pem_falls_back_to_in_process_generationManualsys.frozen=Trueinvocation ofTesla.get_rsa_private_key, followed by persisted-PEM deserialization, generated/persisted key comparison, warning inspection, and file-mode verification✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.