Skip to content

FE-1220: Harden and document Petrinaut optimization execution isolation - #9058

Draft
kube wants to merge 1 commit into
cf/fe-1226-petrinaut-optimization-reconnectfrom
cf/fe-1220-petrinaut-optimization-execution-isolation
Draft

FE-1220: Harden and document Petrinaut optimization execution isolation#9058
kube wants to merge 1 commit into
cf/fe-1226-petrinaut-optimization-reconnectfrom
cf/fe-1220-petrinaut-optimization-execution-isolation

Conversation

@kube

@kube kube commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

🌟 What is the purpose of this PR?

Bounds what the Petrinaut CLI subprocess can do, and writes down the isolation model the optimizer service depends on. A malicious optimization manifest can no longer leave descendants running after its study ends, and each CLI process is capped on CPU time, address space, and process count.

🔗 Related links

  • FE-1220 — this issue
  • FE-1217 — parent hardening issue
  • SRE-830 — the out-of-repo ECS task settings this documents as required

🚫 Blocked by

🔍 What does this change?

  • petrinaut_client.py: a SIGKILL sweep of the CLI's process group on every close path, so descendants cannot outlive the study; resource.prlimit (Linux) bounds each CLI process via RLIMIT_CPU / RLIMIT_AS / RLIMIT_NPROC, configurable through HASH_PETRINAUT_OPT_CLI_* (0 disables).
  • docker/Dockerfile: tini as PID 1, so any orphan that escapes the sweep is reaped instead of becoming a zombie under uvicorn.
  • docs/threat-model.md (new): the execution path, per-layer mitigations, the required out-of-repo deployment configuration, and the documented decision that container/process isolation is sufficient — no additional V8 sandbox.
  • README.md: the security/isolation section and the CLI limit environment variables.
  • OPTIMIZATION_INTEGRATION.md: the CLI cancellation/shutdown contract that parents must implement.

Dropped during the rebase: this branch originally added a study wall-clock ceiling to the legacy stream_all/stream_best generators. main now bounds detached runs via HASH_PETRINAUT_OPT_MAX_STUDY_SECONDS (FE-1224), and #9066 below this in the stack deletes those generators entirely — so that part would have been written and then deleted.

The threat model is written against the current architecture rather than the one this branch was first drafted on: runs are detached and outlive their connections, ownership is enforced in the optimizer by account tag (a foreign run answers 404 so ids cannot be probed), and study-slot occupancy is bounded by the wall-clock ceiling plus detach-grace reaping instead of by connection deadlines. Each claim was checked against the code.

Pre-Merge Checklist 🚀

🚢 Has this modified a publishable library?

This PR:

  • does not modify any publishable blocks or libraries, or modifications do not need publishing

📜 Does this require a change to the docs?

The changes in this PR:

  • require changes to docs which are made as part of this PR

🕸️ Does this require a change to the Turbo Graph?

The changes in this PR:

  • do not affect the execution graph

⚠️ Known issues

  • RLIMIT_NPROC is enforced by Linux against the service user's total task count, not this child's descendants; container-level pidsLimit (SRE-830) is the real backstop.
  • prlimit is applied from the parent immediately after spawn, so the child is briefly unbounded; container limits remain the outer bound.
  • A descendant that calls setsid()/setpgid() leaves the CLI's process group and escapes the sweep — documented in the threat model's limitations.
  • Task-level settings (read-only rootfs, memory sizing, deny-all egress, pidsLimit) live in the out-of-repo infra and are tracked in SRE-830.

🛡 What tests cover this?

apps/petrinaut-opt: 93 pytest green on this layer — including the process-group sweep verified with a real grandchild process (mutation-checked: removing the sweep fails the test), prlimit application and env parsing, and the prompt-termination paths.

❓ How to test this?

  1. cd apps/petrinaut-opt && uv run pytest
  2. Build the image and confirm tini is PID 1: docker build --file apps/petrinaut-opt/docker/Dockerfile --tag petrinaut-opt:local . then docker run --rm petrinaut-opt:local ps -o pid,comm (or inspect the entrypoint).
  3. Start a run, note the CLI's pid, kill the study, and confirm no descendant of that process group survives.

🤖 Generated with Claude Code

@kube kube self-assigned this Jul 19, 2026
@vercel

vercel Bot commented Jul 19, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hash Ready Ready Preview Aug 3, 2026 5:42pm
petrinaut Ready Ready Preview Aug 3, 2026 5:42pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
hashdotdesign-tokens Ignored Ignored Preview Aug 3, 2026 5:42pm

@github-actions github-actions Bot added area/infra Relates to version control, CI, CD or IaC (area) area/libs Relates to first-party libraries/crates/packages (area) type/eng > frontend Owned by the @frontend team area/tests New or updated tests area/apps labels Jul 19, 2026
@vercel
vercel Bot temporarily deployed to Preview – petrinaut July 19, 2026 02:53 Inactive
@kube
kube marked this pull request as ready for review July 19, 2026 03:14
@kube
kube requested a review from YannisZa July 19, 2026 03:14
@cursor

cursor Bot commented Jul 19, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches subprocess lifecycle and resource limits on a security-sensitive optimization path; behavior is well-tested but mis-tuned RLIMIT_NPROC or memory limits could affect availability under load.

Overview
Hardens Petrinaut optimization execution isolation and documents the security model for reviewers and operators.

Runtime hardening (petrinaut_client, Dockerfile): every CLI shutdown now ends with a SIGKILL process-group sweep so grandchildren cannot outlive the study; tini is PID 1 to reap orphans. Linux prlimit on each spawned CLI bounds CPU, address space, and process count via HASH_PETRINAUT_OPT_CLI_* env vars. petrinaut_optimizer enforces a per-study wall-clock ceiling (HASH_PETRINAUT_OPT_MAX_STUDY_SECONDS, default 900s) on SSE streams, emitting a terminal error and tearing down the CLI without mislabeling a study that already queued its completion sentinel.

Documentation: new docs/threat-model.md (execution path, mitigations, limitations, required ECS settings, no extra V8 sandbox rationale); README lists isolation env vars; OPTIMIZATION_INTEGRATION.md documents parent cancellation/shutdown expectations.

Tests cover group sweep, rlimit application/parsing, study ceiling vs. boundary completion, and disabling limits in real-subprocess tests.

Reviewed by Cursor Bugbot for commit 47f2fea. Bugbot is set up for automated code reviews on this repo. Configure here.

@cursor cursor 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.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 47f2fea. Configure here.

f"{json.dumps({'state': 'ERROR', 'message': message})}"
f"\n\n"
)
break

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pending sentinel misclassified as timeout

Medium Severity

When the study wall-clock deadline is reached, timeout handling treats an empty events queue as proof the study is still running. The worker always enqueues the completion sentinel via call_soon_threadsafe, so a study that just finished can look idle until that callback runs, triggering a false execution-limit error instead of the normal done path.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 47f2fea. Configure here.

YannisZa
YannisZa previously approved these changes Jul 19, 2026
@kube
kube marked this pull request as draft July 19, 2026 15:23
@kube

kube commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator Author

Converted back to draft: the stack was reordered so the streaming fixes (#9057) and correlated logging (#9059) can merge first to debug the staging network error (FE-1217). #9059 no longer stacks on this branch; once it merges, this branch needs a rebase over it (reintroducing the sweep/ceiling log events and the threat-model rows that reference logging).

Base automatically changed from cf/fe-1218-petrinaut-optimization-streaming-cancellation to main July 19, 2026 21:00
@kube
kube dismissed YannisZa’s stale review July 19, 2026 21:00

The base branch was changed.

@github-actions github-actions Bot added area/apps > hash* Affects HASH (a `hash-*` app) area/apps > hash-api Affects the HASH API (app) type/eng > backend Owned by the @backend team labels Jul 19, 2026
@kube
kube force-pushed the cf/fe-1220-petrinaut-optimization-execution-isolation branch from 47f2fea to a38af6e Compare July 23, 2026 16:24
@github-actions github-actions Bot removed area/apps > hash* Affects HASH (a `hash-*` app) area/apps > hash-api Affects the HASH API (app) type/eng > backend Owned by the @backend team labels Jul 23, 2026
YannisZa
YannisZa previously approved these changes Jul 24, 2026
@kube
kube force-pushed the cf/fe-1220-petrinaut-optimization-execution-isolation branch from a38af6e to e48cebd Compare August 3, 2026 14:08
@github-actions github-actions Bot added area/apps > hash* Affects HASH (a `hash-*` app) type/eng > backend Owned by the @backend team area/apps > hash.design Affects the `hash.design` design site (app) labels Aug 3, 2026
@kube
kube changed the base branch from main to cf/fe-1226-petrinaut-optimization-reconnect August 3, 2026 14:09
@github-actions github-actions Bot removed area/apps > hash* Affects HASH (a `hash-*` app) type/eng > backend Owned by the @backend team area/apps > hash.design Affects the `hash.design` design site (app) labels Aug 3, 2026
@kube
kube force-pushed the cf/fe-1226-petrinaut-optimization-reconnect branch from cf9d973 to 633185f Compare August 3, 2026 15:51
@kube
kube force-pushed the cf/fe-1220-petrinaut-optimization-execution-isolation branch from e48cebd to a72b3b5 Compare August 3, 2026 15:54
@kube
kube force-pushed the cf/fe-1226-petrinaut-optimization-reconnect branch 2 times, most recently from 74f438d to af8d841 Compare August 3, 2026 17:32
Bounds and documents what the Petrinaut CLI subprocess can do, and records
the deployment isolation model the service relies on.

- petrinaut_client.py: a SIGKILL sweep of the CLI's process group on every
  close path, so descendants cannot outlive the study; resource.prlimit
  (Linux) bounds each CLI process via RLIMIT_CPU / RLIMIT_AS / RLIMIT_NPROC,
  configurable through HASH_PETRINAUT_OPT_CLI_* (0 disables).
- Dockerfile: tini as PID 1 to reap any orphan that escapes the sweep.
- docs/threat-model.md: the execution path, per-layer mitigations, required
  out-of-repo deployment configuration, and the documented decision that
  container/process isolation is sufficient (no additional V8 sandbox).
- README: the isolation section and the CLI limit environment variables.
- OPTIMIZATION_INTEGRATION.md: the CLI cancellation/shutdown contract.

The wall-clock ceiling this branch originally added to the legacy stream_all
and stream_best generators is dropped: main already bounds detached runs via
HASH_PETRINAUT_OPT_MAX_STUDY_SECONDS, and the legacy generators are removed
by the frontend cutover below this in the stack. The threat model is updated
for the detached-run architecture — runs outlive connections, ownership is
enforced in the optimizer by account tag, and slot occupancy is bounded by
the ceiling and detach-grace reaping rather than by connection deadlines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/apps area/infra Relates to version control, CI, CD or IaC (area) area/libs Relates to first-party libraries/crates/packages (area) area/tests New or updated tests type/eng > frontend Owned by the @frontend team

Development

Successfully merging this pull request may close these issues.

2 participants