feat: add remote Deadpool executor - #429
Conversation
## Big picture Add an experimental remote executor so independent processes can share one server-owned Deadpool over Unix-domain or TCP sockets while retaining the standard `concurrent.futures.Executor` programming model. The implementation keeps application payloads opaque in the broker, makes distributed ownership and cancellation outcomes explicit, and bounds network-facing queues, frames, connections, and retained results. The existing single-file module is migrated into a package so the remote client/server implementation can be separated behind focused internal modules without breaking current `import deadpool` users. ## Changes - Preserve the existing local executor API through package-level re-exports and compatibility forwarding while moving the pool implementation to `deadpool/_pool.py`. - Add `DeadpoolClient`, `DeadpoolServer`, and `RemoteFuture`, including callable and registered-task submission, ordered mapping, health/status/statistics, request identity, fork protection, and explicit client/server lifecycle handling. - Add a private experimental wire protocol with safe JSON control headers, opaque chunked payloads, digest validation, bounded framing, typed failures, and ordered full-duplex transport handling. - Support protected Unix-domain sockets, explicit loopback TCP, and caller-configured mutual TLS with bounded authentication and connection admission. - Add broker-owned priority and principal-fair scheduling, queue and execution timeouts, ordinary/group/hard cancellation, terminal arbitration, and bounded result retention with acknowledgement cleanup. - Add serializer limits, worker-side opaque invocation decoding, remote traceback fallbacks, and distinct transport, compatibility, queue, process, cancellation, and result exceptions. - Document the remote API, pickle trust boundary, experimental wire status, and intentionally deferred protocol extensions. - Add package compatibility, protocol, transport, cancellation, timeout, security, retention, callback, fork, and end-to-end integration coverage. ## Validation - `nox -s test-3.14` — 103 passed. - Ruff checks for the new package and tests passed. - Wheel construction succeeded and includes all remote executor modules.
## Big picture Strengthen the experimental remote executor with realistic behavioral coverage across its public client, server, and Future interfaces. The expanded suite exercises process, network, TLS, scheduling, cancellation, shutdown, and Unix socket lifecycles rather than targeting individual implementation lines merely to improve the percentage. The new scenarios exposed several real races and lifecycle defects. Fix those issues alongside the tests that reproduce them, and configure Coverage.py to attribute work performed by multiprocessing, forked, and spawned children correctly. Branch-aware coverage rises from roughly 79% to 88%, while statement coverage rises above 90%. ## Changes - Make submission registration atomic with transport failure and shutdown so a Future cannot be stranded after sender exit or concurrent client closure. - Resolve control RPCs exactly once so a valid response cannot be overwritten by a subsequent disconnect, and ensure failed queueing releases RPC bookkeeping. - Make all concurrent server startup callers observe the same readiness or startup failure, preserve failed startup state, and avoid request-table mutation while applying disconnect cancellation. - Correct signed 64-bit protocol validation and reject inconsistent empty group identifiers before submission. - Harden Unix listener cleanup with inode/device identity checks so failed bind and shutdown paths preserve visible replacement sockets and unrelated files. - Configure coverage collection through `.coveragerc` for threads, multiprocessing, raw forks, subprocesses, and `_exit`, erase stale data before collection, and require a Coverage.py version that supports process patches. - Add public end-to-end scenarios for spawned clients sharing one server pool, mutual TLS authentication and certificate rejection, chunked payloads, graceful draining, worker death and recovery, admission limits, scheduling fairness, disconnect policies, callbacks, and control/status operations. - Add focused protocol, serializer, scheduler, configuration, and Future tests where low-level resource and state-machine invariants cannot be exercised reliably through the public API. - Add test-only TLS credentials and document that they are fixtures with no production trust. - Replace timing-sensitive cancellation assertions with explicit barriers, bounded polling, and robust child-process cleanup. ## Validation - `nox -s test-3.14` — 172 passed. - `nox -s testcov-3.14` — 172 passed; 88% branch-aware coverage. - `nox -s test-3.10 -- tests/remote -q` — 105 passed. - Ruff, Black, and `git diff --check` passed. - Independent concurrency and test-quality review approved the final changes.
cjrh
left a comment
There was a problem hiding this comment.
Intended decision: Request Changes
Review scope
Reviewed PR #429 (feat: add remote Deadpool executor) at 9c62ae5d194069144809617570bfbbb926d60abd against main at 7412f7a64f8806dd6ebb6e68720529c2c7478ef5. I inspected the full 40-file diff plus the surrounding pool, client/server, protocol, transport, serializer, and test code. The untracked REMOTE_EXECUTOR_SPEC.md is not part of the PR and was excluded. There is no linked Shortcut story/issue and there were no existing PR conversation, review, or inline comments.
Blocking findings
I left line-level comments for each issue:
- Live-session removal leaks global admission capacity —
deadpool/remote/server.py:1159. After the first request on a still-connected client is acknowledged, the session is removed from the server indexes. Later ACKs cannot find that session, so requests and worst-case result reservations leak permanently. This is a common sequential-use path and eventually producesRemoteQueueFullfor valid clients. - Startup can report failure or shutdown, then publish a live listener —
deadpool/remote/server.py:300.start()uses the handshake timeout for pool/listener initialization, while_initialize()continues and later writesRUNNINGwithout checking whether timeout handling or concurrent shutdown already won. - The client permits non-loopback plaintext TCP —
deadpool/remote/config.py:37.TcpAddress("example.com", ..., insecure=True)is accepted, contrary to the documented loopback-only plaintext policy, exposing authentication and pickle payloads on the network. - Cancelled
RemoteFutures do not complete stdlib waiters —deadpool/remote/_future.py:151.done()becomes true, butconcurrent.futures.wait()keeps the future innot_doneandas_completed()times out because the base state never reachesCANCELLED_AND_NOTIFIED. - A large task exception tears down the entire session —
deadpool/remote/_worker.py:72. Traceback strings are unbounded even though protocol control strings are bounded; a 20 KiB exception message producedRemoteConnectionLostinstead of a task failure and also fails unrelated in-flight work. - Framing limits are neither negotiated nor rejected —
deadpool/remote/server.py:566. A valid 8 KiB-frame server and default client completed the handshake, then a 20 KiB submission inside the advertised invocation limit failed asSubmissionOutcomeUnknown.
Important follow-up
RemoteFuture.add_done_callback()reserves callback capacity synchronously (deadpool/remote/_future.py:74). Withcallback_queue_size=1, a second callback registration blocks until the future completes and can deadlock the thread needed to let the task finish. Bound dispatch rather than registration, or provide a nonblocking/inline fallback.- The primary README remote example starts a non-daemon server without shutting it down, then creates a second unstarted server and reuses
executorafter its context has closed (README.rst:785-805). Please replace it with one nested server/client context that demonstrates both modes and exits cleanly. - Registered mode still unpickles client-controlled arguments before invoking the registered function. The trust warning should say that the default
PickleSerializergrants worker-account code execution in both callable and registered modes; operation authorization is not a sandbox.
Validation evidence
nox -s test-3.14— passed, 172 tests; one existingos.fork()deprecation warning.nox -s testcov-3.14— passed, 172 tests; local report: 85% total branch-aware coverage.ruff check deadpool/remote deadpool/__init__.py deadpool/_pool.py tests/remote tests/test_package_api.py noxfile.py— passed.black --check deadpool tests noxfile.py— passed, 32 files unchanged.git diff --check origin/main...HEAD— passed.python -m pip wheel . --no-deps --wheel-dir /tmp/deadpool-review-wheel— passed; the wheel contains alldeadpool.remotemodules.nox -s lint— could not run with the installed Ruff because this nox session invokes the removedruff .CLI form; the focusedruff checkcommand above passed. This is a local tool-version mismatch, not evidence of a changed-file lint defect.python -m build --wheel --sdist --outdir /tmp/deadpool-review-dist— could not run because the active environment lacks thebuildmodule;pip wheelprovided alternate package-build evidence.- Focused inline Python probes reproduced all six blocking behaviors and the callback-registration deadlock described above.
- GitHub's Python 3.10, 3.11, 3.12, 3.13, 3.14, and 3.14t jobs, lint, dependency review/scan, Semgrep, CodeQL, and Trivy are green. Coveralls is failing: “Coverage decreased (-5.6%) to 88.172%.”
The broad automated suite is strong, but it does not exercise these lifecycle, stdlib interoperability, asymmetric-limit, and bounded-error paths. They are material correctness/security risks in the new remote executor and should be fixed with focused regressions before merge.
cjrh
left a comment
There was a problem hiding this comment.
Intended decision: Request Changes
I reviewed PR #429 (main at 7412f7a → feat-ipc-submission at 0e9c738), including the complete 40-file diff, remote protocol/client/server/package context, tests, prior threads, and CI. There is no linked Shortcut story or issue. The seven earlier inline threads each have a follow-up reply and their original issues appear addressed.
Blocking findings
-
deadpool/remote/server.py:835— local backlog backpressure disables remote queue deadlines.
_dispatch_loop()calls the local pool's blocking private_submit_future()from the same thread that runs_expire_queued_locked().Deadpool._submit_future()performs a blockingPriorityQueue.put(). Once the local pool'smax_backlogis full, this thread cannot revisit expiration, so an accepted remote request can remainACCEPTED_QUEUEDpastdeadpool_queue_timeoutuntil unrelated local work completes.I reproduced this with
Deadpool(max_workers=1, max_backlog=1): after a running task and one local-backlog task, a third request withdeadpool_queue_timeout=0.05was stillACCEPTED_QUEUEDafter 0.25 seconds and becameRemoteQueueTimeoutonly after releasing the unrelated running task. Use nonblocking/timed local admission with requeue/defer semantics, or separate deadline expiration from the blocking local-pool admission path. Add a regression that requires timeout completion before the saturated local backlog is released. -
deadpool/remote/config.py:11,:43-44,:98-99— plaintext TCP accepts an unresolved hostname as “loopback.”
The allowlist accepts the stringlocalhost, butsocket.create_connection()andsocket.bind()resolve it later. A modified hosts/DNS resolver can map that name to a LAN address, making an explicitly insecure listener/client non-loopback despite the advertised restriction. With pickle invocation this can expose unencrypted, unauthenticated worker-account code execution. Require numeric loopback addresses for insecure TCP, or resolve every selected sockaddr and requireipaddress.ip_address(address).is_loopback; add a hostile-resolution regression. -
deadpool/remote/_protocol.py:374— incomplete chunk buffering has no aggregate byte limit.
max_incomplete_messageslimits only IDs; every payload is retained instate.partsuntil the final chunk. An authenticated peer can open 16 logical 64-MiB default messages and withhold one final 1-MiB chunk from each, retaining about 0.984 GiB on one connection. Atmax_connections_global=256, that is roughly 252 GiB. This happens during framing, before operation authorization. Track and cap aggregate incomplete bytes per reader/connection (with aRemoteLimitssetting), releasing the count on completion/error; add an interleaved-partial-message limit test. -
deadpool/remote/_future.py:70-76— callbacks registered after completion do not followconcurrent.futures.Futuresemantics.
A completed Future queues the callback through the dispatcher rather than invoking it beforeadd_done_callback()returns. If the dispatcher is waiting on callback capacity, it can be deferred indefinitely.concurrent.futures.Futurerequires callbacks added after terminal completion to be called immediately. Invoke terminal callbacks inline with the existing exception logging; unfinished-Future registration can remain nonblocking. Add a regression with a stalled dispatcher that asserts a late callback has run whenadd_done_callback()returns. -
README.rst:823-825— the pickle trust-boundary warning incorrectly scopes RCE to callable mode.
Registered task arguments are also processed byPickleSerializer.loads()indeadpool/remote/_worker.py:35, before the registered function runs. A registry restricts handler names, not hostile pickle execution. State that the default serializer deserializes callable and registered-task payloads (and result/exception payloads), and that every peer permitted to submit either mode must be mutually trusted.
Validation
Passed locally:
python3 -m pytest -q tests/remote tests/test_package_api.py— 120 passed; one existingos.fork()deprecation warning.nox -s testcov-3.14— 185 passed; one existing warning; 87% branch-aware total coverage.black --check deadpool tests/remote tests/test_package_api.py && ruff check deadpool tests/remote tests/test_package_api.py && git diff --check 7412f7a64f8806dd6ebb6e68720529c2c7478ef5..HEAD— passed (Black emitted only its Python-3.15 parsing warning).uv build --wheel --out-dir /tmp/pr429-review-wheel— passed; wheel includesdeadpool/__init__.pyand all 11 remote modules.
GitHub's Python 3.10–3.14t test matrix, lint, CodeQL, Semgrep, Trivy, and dependency checks pass. coverage/coveralls currently fails because coverage decreased 6.0% to 87.78%; restore the required check before merging.
Review — remote Deadpool executorSolid PR overall. CI is green, the 1. RESULT that fails client-side deserialization leaks the future and the server reservation (bug)
Reproduced with a client serializer whose
The 2. Full invocation payload is sent over the worker pipe while the global lock is held (liveness)
Suggest committing RUNNING state under the lock but performing 3.
|
## Big picture Keep the remote executor's public timing, scheduling, and trust guarantees intact under real backpressure and completion races. Remote queue deadlines must remain enforceable when the local pool is saturated, `RemoteFuture` must retain the standard `Future` callback contract, and the default pickle transport must clearly communicate that trust is bidirectional in every submission mode. ## Detailed changes - make broker-to-pool admission nonblocking without changing ordinary local `Deadpool.submit()` backpressure - requeue rejected admissions without losing strict priority, principal fairness, or FIFO ordering within a scheduling stream - arbitrate queue deadlines immediately before worker dispatch so expired work cannot start in an admission race - invoke callbacks registered after terminal completion synchronously while retaining isolated asynchronous dispatch for callbacks registered earlier - centralize callback exception handling and the default `PickleSerializer` trust warning - document that callable and registered-task invocations, results, and exceptions require mutually trusted clients and servers - add regressions for saturated-backlog queue timeouts, scheduler requeue ordering, and late callback timing and logging
|
@wallies Re item 1 in your review: fixed in A terminal result is now acknowledged and removed from the client tables even when client-side deserialization fails. If the mandatory Verified:
|
|
@wallies Re item 2 in your review: fixed in The broker now atomically claims the queue/worker boundary and publishes the selected PID under its condition, then releases the condition before Verified:
|
|
@wallies Re item 3 in your review: fixed in
Verified:
|
|
@wallies Re item 4 in your review: no change in this PR. I confirmed the three costs, but they are bounded scaling ceilings rather than correctness failures: expiry and per-principal admission scan at most Without latency evidence at the configured ceilings, that added state and race surface is not justified for this experimental implementation. I’m deferring these optimizations to benchmark-driven follow-up work rather than expanding this correctness/security feedback pass. |
|
@wallies Re item 5 in your review: the material memory-amplification issue is fixed in
I did not add a general post-handshake idle timeout: that would require keepalive/liveness semantics for legitimate long-running tasks and is a separate lifecycle feature. The aggregate cap provides the needed finite memory bound without introducing that policy or new configuration. Verified:
|
|
@wallies Re the blocking Once work is submitted, a truthful The README and method contract now state that submitted cancellation can block up to Verified:
|
|
@wallies Re the recursive control-JSON minor in your review: fixed in
Verified:
|
|
@wallies Re the duplicate invocation SHA-256 minor in your review: no change in this PR. The two digests protect different layers: the submission digest is broker request-identity metadata, while |
|
Follow-up on the second owner review’s insecure- Rechecking the full feedback inventory showed this item was not part of the earlier consolidated Verified:
|
Big picture
Add an experimental remote executor so independent processes can share one
server-owned Deadpool over Unix-domain or TCP sockets while retaining the
standard
concurrent.futures.Executorprogramming model. The implementationkeeps application payloads opaque in the broker, makes distributed ownership
and cancellation outcomes explicit, and bounds network-facing queues, frames,
connections, and retained results.
The existing single-file module is migrated into a package so the remote
client/server implementation can be separated behind focused internal modules
without breaking current
import deadpoolusers.Changes
compatibility forwarding while moving the pool implementation to
deadpool/_pool.py.DeadpoolClient,DeadpoolServer, andRemoteFuture, including callableand registered-task submission, ordered mapping, health/status/statistics,
request identity, fork protection, and explicit client/server lifecycle
handling.
opaque chunked payloads, digest validation, bounded framing, typed failures,
and ordered full-duplex transport handling.
caller-configured mutual TLS with bounded authentication and connection
admission.
timeouts, ordinary/group/hard cancellation, terminal arbitration, and bounded
result retention with acknowledgement cleanup.
traceback fallbacks, and distinct transport, compatibility, queue, process,
cancellation, and result exceptions.
intentionally deferred protocol extensions.
security, retention, callback, fork, and end-to-end integration coverage.
Validation
nox -s test-3.14— 103 passed.