Skip to content

fix(supervisor): preserve siblings during targeted lifecycle - #106

Merged
jalehman merged 6 commits into
openclaw:mainfrom
hannesrudolph:fix/isolate-gateway-restarts-20260822
Aug 24, 2026
Merged

fix(supervisor): preserve siblings during targeted lifecycle#106
jalehman merged 6 commits into
openclaw:mainfrom
hannesrudolph:fix/isolate-gateway-restarts-20260822

Conversation

@hannesrudolph

@hannesrudolph hannesrudolph commented Aug 22, 2026

Copy link
Copy Markdown
Member

Summary

  • preserve unrelated running gateway specs and PIDs during targeted service start
  • fully reconcile fleet state before activating a stopped daemon
  • reconcile only the new environment during clone, simulation-clone, and import
  • add regressions for running-daemon, stopped-daemon, and clone isolation

Incidents

Two related fleet-isolation failures were reproduced:

  1. A targeted ocm service start openclaw rebuilt complete fleet state and
    relaunched unrelated gateways when latent sibling drift was present.
  2. ocm env clone clawpatrol <canary> rebuilt complete fleet state and
    reloaded production OpenClaw during supervisor convergence.

Both failures came from target-scoped commands invoking fleet-wide supervisor
synchronization.

Behavior

  • When the supervisor daemon is already running, service start synchronizes
    only the named environment.
  • When the daemon is stopped, service start fully reconciles fleet state before
    activation so stale persisted sibling definitions are not launched.
  • Clone/import paths synchronize only the newly created environment and do not
    regenerate sibling specs.

Validation

  • cargo fmt --all -- --check
  • cargo check --workspace --all-targets --locked
  • focused regressions:
    • service_start_preserves_running_siblings_despite_unrelated_drift
    • service_start_reconciles_siblings_before_activating_stopped_daemon
    • env_clone_preserves_unrelated_supervisor_child_specs
    • env_import_preserves_unrelated_supervisor_child_specs
  • cargo test --test daemon_runtime_tests -- --test-threads=1
  • cargo test --all-targets --locked -- --test-threads=1 --skip bin_wrapper_runs_with_an_overridden_home
  • cargo build --release --locked

The skipped wrapper test requires rustup, which is not installed on the
deployment host. Every executed test passed.

@clawsweeper

clawsweeper Bot commented Aug 22, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 22, 2026
@clawsweeper

clawsweeper Bot commented Aug 22, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 24, 2026, 4:51 PM ET / 20:51 UTC.

ClawSweeper review

What this changes

The PR reconciles only the target environment during service start and environment create/clone/import, while fully reconciling the fleet before activating a stopped supervisor daemon.

Regression provenance

Possible regression — probable (reproduction; reviewed change). No predecessor PR is attributed.

Merge readiness

⚠️ Ready for maintainer review - 2 items remain

Keep open for maintainer review. The current head narrowly fixes target-scoped supervisor synchronization and adds focused regression coverage; it remains a MEMBER-authored PR with pending platform checks.

Priority: P2
Reviewed head: d8c23090550128aee9c91b1d91b979d2073f560c

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) The patch is narrow and well-covered, with final platform validation still pending.
Proof confidence 🌊 off-meta tidepool Not applicable: This MEMBER-authored PR is exempt from the external-contributor proof gate; its reported focused regression runs remain useful supplemental evidence.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This MEMBER-authored PR is exempt from the external-contributor proof gate; its reported focused regression runs remain useful supplemental evidence.
Evidence reviewed 5 items Current-main behavior: Current main performs a full supervisor sync before checking whether the daemon is already running, allowing target service start to rebuild sibling state.
Targeted lifecycle repair: The head changes environment creation, clone, simulation clone, and import to use target-only synchronization.
Stopped-daemon safety: The head checks daemon status first and executes a full sync only before activating a stopped daemon.
Findings None None.
Security None None.

Live Verification

Command: cargo test --test daemon_runtime_tests service_start_preserves_running_siblings_despite_unrelated_drift -- --test-threads=1

Result: FAIL (failed) — execution before step 1 run: sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.23.0.tgz

sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.23.0.tgz

Assertions:

  • FAIL expect_output: test service_start_preserves_running_siblings_despite_unrelated_drift ... ok

How this fits together

OCM lifecycle commands persist desired environment service state for a shared supervisor daemon. The daemon consumes that state to run OpenClaw gateway child processes.

flowchart LR
A[Lifecycle command] --> B[Environment metadata]
B --> C[Targeted supervisor sync]
C --> D[Persisted child state]
D --> E[Gateway child processes]
B --> F[Stopped daemon check]
F --> G[Full fleet sync]
G --> D
Loading

Before merge

  • Resolve merge risk (P1) - Supervisor state controls running gateway processes; retain the sibling-isolation and stopped-daemon regressions as merge validation.
  • Complete next step (P2) - This MEMBER-authored PR needs normal maintainer approval after its pending platform checks complete.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Changed surface 4 files affected; production +10/-10, tests +336/-4 A small lifecycle implementation change is accompanied by substantial targeted regression coverage.

Merge-risk options

Maintainer options:

  1. Complete lifecycle validation (recommended)
    Require the current macOS and Ubuntu test runs to pass while retaining the focused sibling-isolation regressions.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Require successful macOS and Ubuntu tests for the current head and retain the daemon lifecycle isolation regressions.

Technical review

Best possible solution:

Merge the target-only reconciliation after the latest macOS and Ubuntu checks pass, preserving the focused lifecycle regressions.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: focused daemon-runtime tests create sibling services, introduce drift, and assert sibling specifications and PIDs remain unchanged.

Is this the best way to solve the issue?

Yes. Reusing existing targeted synchronization and reserving fleet sync for stopped-daemon activation is the narrowest maintainable repair.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 6044dbaf5e55.

Labels

Label changes:

  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🦞 diamond lobster.
  • remove rating: 🦞 diamond lobster: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

  • P2: The PR prevents unintended restarts or state changes for unrelated managed gateways.
  • merge-risk: 🚨 availability: Supervisor synchronization and daemon activation directly control gateway child-process continuity.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This MEMBER-authored PR is exempt from the external-contributor proof gate; its reported focused regression runs remain useful supplemental evidence.

Evidence

What I checked:

  • Current-main behavior: Current main performs a full supervisor sync before checking whether the daemon is already running, allowing target service start to rebuild sibling state. (src/supervisor/mod.rs:480, 6044dbaf5e55)
  • Targeted lifecycle repair: The head changes environment creation, clone, simulation clone, and import to use target-only synchronization. (src/env/lifecycle.rs:229, d8c230905501)
  • Stopped-daemon safety: The head checks daemon status first and executes a full sync only before activating a stopped daemon. (src/supervisor/mod.rs:480, d8c230905501)
  • Regression coverage: Added daemon-runtime cases assert unchanged sibling PIDs under drift and refreshed sibling state before stopped-daemon activation. (tests/daemon_runtime_tests.rs:1356, d8c230905501)
  • Feature-history provenance: Available local history is shallow/grafted, but blame attributes the current changed lifecycle lines to the PR-head commit by Josh Lehman. (src/service/manage.rs:469, d8c230905501)

Likely related people:

  • jalehman: The current head commit and blame attribute the completed creation-path extension and lifecycle lines to Josh Lehman. (role: recent area contributor; confidence: medium; commits: d8c230905501; files: src/env/lifecycle.rs, src/service/manage.rs, src/supervisor/mod.rs)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Let the current macOS and Ubuntu checks finish on the latest head.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (12 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-22T19:57:18.679Z sha 39f28ec :: needs changes before merge. :: [P1] Synchronize all state before activating a stopped daemon
  • reviewed 2026-08-22T22:56:08.186Z sha 39f28ec :: needs changes before merge. :: [P1] Synchronize all state before activating a stopped daemon
  • reviewed 2026-08-23T09:01:24.325Z sha 39f28ec :: needs changes before merge. :: [P1] Synchronize the fleet before activating a stopped daemon
  • reviewed 2026-08-23T16:26:35.908Z sha 54339c2 :: needs changes before merge. :: [P1] Scope supervisor synchronization for environment imports
  • reviewed 2026-08-23T16:44:44.208Z sha 7867050 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-23T21:23:02.403Z sha 7867050 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-24T11:02:44.436Z sha 7867050 :: needs maintainer review before merge. :: none
  • reviewed 2026-08-24T16:09:31.501Z sha 7867050 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Aug 22, 2026
@hannesrudolph hannesrudolph changed the title fix(service): preserve sibling gateways on start fix(supervisor): preserve siblings during targeted lifecycle Aug 23, 2026
@hannesrudolph

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

Addressed the stopped-daemon finding and added the clone-isolation fix exposed
by the August 23 live incident. Running-daemon, stopped-daemon, and clone
regressions pass, as does the complete deterministic all-target suite.

@clawsweeper

clawsweeper Bot commented Aug 23, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

@hannesrudolph

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

Addressed the import-isolation finding on head 7867050. Running-daemon,
stopped-daemon, clone, and import regressions pass, as does the complete
deterministic all-target suite.

@clawsweeper

clawsweeper Bot commented Aug 23, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Aug 23, 2026
@jalehman

Copy link
Copy Markdown
Contributor

Extended the creation path at d8c23090550128aee9c91b1d91b979d2073f560c.

  • EnvironmentService::create now uses the target-only supervisor reconciliation already introduced by this PR.
  • This covers missing-environment ocm start, env create, new dev environments, and migration/adoption creation through their shared service boundary.
  • The incident-shaped regression runs ocm start pr121799-macos-proof --command 'node openclaw.mjs' --cwd <isolated-test-dir> --port 19079 --no-service --json with caller NODE_OPTIONS drift and asserts that both unrelated child specs and PIDs remain unchanged.

Evidence:

  • Pre-fix at 786705082866057b3da0c8840d16cb89a6d2dccf: the regression fails because both sibling specs absorb caller-environment drift.
  • Post-fix: the regression passes and both sibling PIDs remain unchanged.
  • Production/test delta: src/env/lifecycle.rs +1/-1; tests/daemon_runtime_tests.rs +74/-0.
  • Passed: cargo fmt --all -- --check, cargo check --workspace --all-targets --locked, focused regression, 30-test daemon suite excluding the named baseline failure below, full locked all-target suite with the same baseline exclusion and the documented rustup-dependent wrapper exclusion, and cargo build --release --locked.
  • Independent baseline: env_destroy_removes_only_target_child_despite_unrelated_drift fails with environment process state changed during inspection at both 7867050 and this head. Strict Clippy is also blocked by the pre-existing clippy::too_many_arguments finding at src/migrate.rs:372.
  • Autoreview ran. Its only finding claimed conflicting source-backed children could both become active; the runtime rejects a pending child when a running child owns the same run directory, and this regression creates a disabled target recorded in skippedEnvs, so no patch expansion was made.

No live OCM environment or Gateway was used during proof.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Aug 24, 2026
@jalehman
jalehman merged commit 0356869 into openclaw:main Aug 24, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants