Skip to content

Add process-isolated philosopher executor with hard timeout mode - #470

Open
hiroshitanaka-creator wants to merge 1 commit into
mainfrom
codex/add-hard-timeout-execution-mode-for-philosophers
Open

Add process-isolated philosopher executor with hard timeout mode#470
hiroshitanaka-creator wants to merge 1 commit into
mainfrom
codex/add-hard-timeout-execution-mode-for-philosophers

Conversation

@hiroshitanaka-creator

Copy link
Copy Markdown
Owner

Motivation

  • Thread-based timeouts can only signal a fallback while the underlying work may continue running in the background, which is unacceptable for production safety; a hard-stop process mode is required to guarantee work is stopped.
  • Make the execution backend configurable so callers keep the same sync/async APIs while operators can select a production-safe process mode.

Description

  • Introduce a runtime toggle PO_PHILOSOPHER_EXECUTION_MODE (supported values: thread, process) and expose it via Settings.from_env() and the API APISettings model, defaulting to thread for backward compatibility.
  • Add src/po_core/runtime/philosopher_executor.py implementing a PhilosopherExecutor abstraction with ThreadPhilosopherExecutor (compat soft-timeout behavior) and ProcessPhilosopherExecutor (process-isolated execution using multiprocessing.Process + queue), including a deterministic indexing/ordering contract and authoritative timeout semantics (worker teardown and recycle on timeout).
  • Route run_philosophers() and the AsyncPartyMachine synchronous fallback through the new executor abstraction (build_executor / run_in_process_async) so behavior selection is centralized while keeping public APIs unchanged.
  • Add a concise explanatory code comment in the thread helper noting that thread timeouts cannot guarantee the underlying work is stopped, hence process mode for production hard-stop safety.
  • Update .env.example to document the new PO_PHILOSOPHER_EXECUTION_MODE variable and add tests covering the new behavior (tests/execution/test_process_executor_timeout.py and tests/execution/test_process_executor_ordering.py).

Testing

  • Ran pytest tests/execution/test_process_executor_timeout.py -q and it passed (verifies blocking philosopher times out authoritatively and does not contribute proposals).
  • Ran pytest tests/execution/test_process_executor_ordering.py -q and it passed (verifies deterministic proposal ordering in process mode).
  • Ran pytest tests/runtime/test_settings_from_env.py -q, pytest tests/execution/test_timeout_contract.py -q, and pytest tests/execution/test_async_timeout_contract.py -q and they all passed to ensure backward-compatible timeout contracts and settings parsing remain stable.

Codex Task

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5e3d0a9849

ℹ️ 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 +154 to +157
if elapsed_ms > timeout_s * 1000 and outcome.error is None:
# A thread future can time out while the underlying work keeps running,
# so thread mode cannot guarantee production-safe cancellation.
return ExecOutcome([], 0, True, _soft_timeout_error(timeout_s, "thread"), elapsed_ms, pid)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve timed_out for cooperative budget expiry

When a thread-mode philosopher polls ExecutionBudget and stops cooperatively after the deadline, run_one_philosopher() returns an ExecOutcome with error="Execution stopped cooperatively …". This branch now converts only the error is None case into a timeout, so budget-aware philosophers are reported as generic failures (timed_out=False) instead of timeouts. That breaks the timeout contract for cooperative cancellation and regresses tests/unit/test_party_machine_cancellation_budget.py::test_budget_aware_philosopher_stops_promptly_when_cancelled.

Useful? React with 👍 / 👎.

Comment on lines +151 to +154
settings = Settings.from_env()
executor = build_executor(
ExecutorConfig(
mode=settings.philosopher_execution_mode,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use the caller's configured execution mode

run_turn() and app.api.run(settings=...) already pass a populated Settings object through the pipeline, but this code ignores it and re-reads Settings.from_env() instead. As a result, Settings(philosopher_execution_mode="process") and create_app(settings=APISettings(...)) still select the thread executor unless PO_PHILOSOPHER_EXECUTION_MODE is also exported, so the new toggle is effectively a no-op for programmatic configuration. The async fallback path repeats the same mistake in AsyncPartyMachine._dispatch_one().

Useful? React with 👍 / 👎.

Comment on lines +314 to +315
async def run_in_process_async(job: SerializedJob) -> ExecOutcome:
return await asyncio.to_thread(_run_one_in_subprocess, job)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Respect max_workers in async process mode

In process mode, each _dispatch_one() task awaits run_in_process_async(), and this helper uses asyncio.to_thread() rather than the machine's bounded executor. Because AsyncPartyMachine.run() creates one task per philosopher, max_workers no longer limits concurrency here: even with max_workers=1, multiple synchronous philosophers can launch subprocesses at once, which can oversubscribe the host and invalidate the async API's latency/resource budgeting.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant