Skip to content

fix: give the injected bridge helper a single owner per container - #284

Merged
steipete merged 3 commits into
openclaw:mainfrom
omarshahine:fix/helper-single-claimer-guard
Sep 7, 2026
Merged

fix: give the injected bridge helper a single owner per container#284
steipete merged 3 commits into
openclaw:mainfrom
omarshahine:fix/helper-single-claimer-guard

Conversation

@omarshahine

@omarshahine omarshahine commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

A second injected Messages process could clear the active helper's legacy IPC, overwrite its ready PID, and remove the surviving helper's ready marker when it exited. The launcher lock only serializes launcher calls; it does not establish helper ownership.

This change holds a dedicated kernel lock for the helper's lifetime. Standbys leave shared IPC and readiness alone, retry ownership once per second, and activate after the owner exits. The active owner restores a removed ready marker. The lock file remains in place so ownership always refers to the same inode.

Fixes #283. The reported vanished-response symptom is not independently attributed to duplicate claiming; this change repairs the verified ownership and readiness interference.

Validation includes all four native helper suites, all 754 Swift tests, lint (17 existing warnings, no serious violations), helper compilation, and an independent autoreview. A new regression runs separate helper-host processes against an isolated container and uses the production Swift bridge client. It checks exclusive request claims, standby destructor behavior, readiness restoration, SIGKILL takeover, and graceful final cleanup.

For the before case, separate hosts running the main-branch watcher and cleanup functions reproduced both defects: the second host overwrote the first live host’s ready PID, then removed readiness on exit while the first host remained alive.

A separately compiled, Developer-ID-signed consumer also called the built IMsgCore library against those real helper processes:

fresh startup: real IMsgBridgeClient ping -> pong
two live helper processes: 11/11 requests claimed exclusively by owner
removed readiness marker: restored by owner; ping -> pong
owner SIGKILL: standby takes ownership; 10/10 subsequent requests -> pong
graceful final exit: readiness removed; permanent ownership lock retained

The newly built, matching-Developer-ID-signed CLI successfully read one row from the real Messages database; private fields were withheld. These are process-lifecycle and protocol proofs, not Messages injection or delivery tests. SIP remains enabled, no private frameworks were loaded in the isolated hosts, and no messages were sent.

Upgrade boundary: old injected helpers do not acquire this lock. Stop the old injected Messages instance with imsg launch --kill-only before launching the updated helper. A patched helper cannot exclude an older helper that remains running. The docs now state that boundary explicitly.

Release notes are prepared in the separate final notes PR to avoid sibling changelog conflicts. Credit: @omarshahine.

Only one injected Messages.app may service the bridge queues. Take a kernel
flock on a dedicated .imsg-bridge-owner.lock before touching any shared state;
a second injected instance stands by instead of clearing the legacy IPC files,
overwriting the ready marker, claiming v2 requests, or removing the owner's
ready marker on exit. The standby retries once a second and takes over when the
owner's flock is released, so a launch that overlaps a still-terminating owner
still ends with a working bridge and no relaunch.

Follow-up to openclaw#272 / openclaw#274, which serialized launches but left the helper itself
unguarded. Fixes openclaw#283.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0131k87VJuatKyj7U1XJuP3p
@clawsweeper

clawsweeper Bot commented Sep 7, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@omarshahine

Copy link
Copy Markdown
Contributor Author

@clawsweeper review

@clawsweeper

clawsweeper Bot commented Sep 7, 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.

Re-review progress:

@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 7, 2026
@clawsweeper

clawsweeper Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed September 7, 2026, 1:26 PM ET / 17:26 UTC.

ClawSweeper review

What this changes

The PR gives each Messages container one active injected helper, adds standby takeover and readiness restoration, and covers that lifecycle with native process tests and upgrade documentation.

Merge readiness

Needs changes before merge - 1 item remains

This remains a useful, distinct fix beyond the merged launcher lock. The updated production-path lifecycle evidence and upgrade documentation address both previous review requests; no blocking patch defect was found.

Priority: P1
Reviewed head: ddccd1d6a275879b50eb7b4a0c1b1c127047b9fc

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused implementation with sufficient production-path lifecycle evidence and both prior review requests addressed.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The captured after-fix transcript exercises the production helper and real IMsgCore transport in separate macOS processes, showing exclusive claims, readiness restoration, SIGKILL takeover, subsequent ping/pong recovery, and final cleanup; this satisfies the changed internal lifecycle behavior.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The captured after-fix transcript exercises the production helper and real IMsgCore transport in separate macOS processes, showing exclusive claims, readiness restoration, SIGKILL takeover, subsequent ping/pong recovery, and final cleanup; this satisfies the changed internal lifecycle behavior.
Evidence reviewed 7 items Repository policy and patch scope: Read the complete root AGENTS.md; no nested AGENTS.md or maintainer-note files were found. Applied its focused-change, deterministic-fixture, validation, and macOS-permission guidance. The checkout was clean.
Introduced ownership and cleanup behavior: The complete local production diff acquires a persistent-inode, nonblocking flock before activation, leaves competing helpers retrying without starting watchers, and releases ownership after readiness cleanup. File validation rejects symlinks, foreign ownership, extra links, and shared permissions.
Current main and release still lack helper ownership: Current main starts helper watchers without acquiring a lifetime ownership lock. Its helper source is unchanged from the supplied latest release v0.15.2. The merged launcher fix at #274 serializes launcher operations, not independently injected helper lifetimes.
Findings None None.
Security None None.

How this fits together

The injected Messages helper receives file-based requests from imsg clients and dispatches them through the Messages bridge. Helper ownership determines which process consumes requests and publishes readiness.

flowchart TD
  A[imsg bridge client] --> B[Container request queue]
  C[Injected helper processes] --> D{Container ownership lock}
  D -->|Acquired| E[Active helper]
  D -->|Busy| F[Standby and retry]
  F --> D
  B --> E
  E --> G[Responses and readiness]
Loading

Before merge

  • Complete next step (P2) - Mark the draft ready for review when the contributor considers it complete.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +179/-8; tests +341/-5 Production growth implements ownership and readiness recovery, supported by native locking and real-client process lifecycle coverage.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #283
Summary: This PR implements the helper-ownership portion tracked by the paired issue; launcher serialization and release notes are separate related work.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Technical review

Best possible solution:

Use the dedicated lifetime lock alongside launcher serialization, retaining the documented stop-and-relaunch procedure for older injected helpers.

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

Yes: current main unconditionally initializes shared IPC and removes readiness on helper exit, so two injected helpers can interfere. The supplied before-case process run corroborates that path; this review did not execute it.

Is this the best way to solve the issue?

Yes: a separate persistent lock avoids ready-marker inode replacement and complements the existing launcher lock; retrying contenders preserves takeover after owner exit.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 34aa2b26afce.

Labels

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The captured after-fix transcript exercises the production helper and real IMsgCore transport in separate macOS processes, showing exclusive claims, readiness restoration, SIGKILL takeover, subsequent ping/pong recovery, and final cleanup; this satisfies the changed internal lifecycle behavior.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The captured after-fix transcript exercises the production helper and real IMsgCore transport in separate macOS processes, showing exclusive claims, readiness restoration, SIGKILL takeover, subsequent ping/pong recovery, and final cleanup; this satisfies the changed internal lifecycle behavior.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

  • P1: The patch addresses active bridge readiness and IPC interference when multiple injected helpers coexist.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The captured after-fix transcript exercises the production helper and real IMsgCore transport in separate macOS processes, showing exclusive claims, readiness restoration, SIGKILL takeover, subsequent ping/pong recovery, and final cleanup; this satisfies the changed internal lifecycle behavior.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured after-fix transcript exercises the production helper and real IMsgCore transport in separate macOS processes, showing exclusive claims, readiness restoration, SIGKILL takeover, subsequent ping/pong recovery, and final cleanup; this satisfies the changed internal lifecycle behavior.

Evidence

What I checked:

  • Repository policy and patch scope: Read the complete root AGENTS.md; no nested AGENTS.md or maintainer-note files were found. Applied its focused-change, deterministic-fixture, validation, and macOS-permission guidance. The checkout was clean. (AGENTS.md:1, ddccd1d6a275)
  • Introduced ownership and cleanup behavior: The complete local production diff acquires a persistent-inode, nonblocking flock before activation, leaves competing helpers retrying without starting watchers, and releases ownership after readiness cleanup. File validation rejects symlinks, foreign ownership, extra links, and shared permissions. (Sources/IMsgHelper/IMsgInjected.m:7011, ddccd1d6a275)
  • Current main and release still lack helper ownership: Current main starts helper watchers without acquiring a lifetime ownership lock. Its helper source is unchanged from the supplied latest release v0.15.2. The merged launcher fix at fix: serialize Messages bridge launches across processes #274 serializes launcher operations, not independently injected helper lifetimes. (Sources/IMsgHelper/IMsgInjected.m:6969, 34aa2b26afce)
  • After-fix production transport evidence: The captured PR body, sourceRevision 563957712f075647c92fc523ded7223732c9c2b238e0032ebf44abe22a5ad451, reports a separately compiled consumer using the built IMsgCore client against real helper-host processes: startup ping/pong, 11 exclusively owned requests, readiness restoration, SIGKILL takeover followed by 10 successful requests, and graceful cleanup. The host directly includes the production helper and uses its ownership, activation, queue, and dispatcher paths. This establishes lifecycle recovery without claiming Messages injection or delivery proof; signing and the separate database-read smoke are supplemental. (Tests/IMsgHelperTests/BridgeOwnershipHost.m:4, ddccd1d6a275)
  • Previous review requests addressed: The previous completed review had no findings and requested production-client lifecycle evidence plus correction of mixed-version standby claims. The latest commit adds the real-client process regression and documents that older injected helpers must be stopped with imsg launch --kill-only before replacement. (docs/advanced-imcore.md:74, ddccd1d6a275)
  • Related work remains distinct: The open same-author issue bridge: injected helper has no single-claimer guard, so any second injected Messages.app silently races the v2 queue (follow-up to #272) #283 is explicitly paired with this implementation. The open notes-only PR docs: prepare bridge ownership patch notes #285 prepares release notes after this fix and does not supersede it. The body correctly avoids attributing every vanished-response symptom to duplicate claiming.

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • goutamadwant: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

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 (2 earlier review cycles)
  • reviewed 2026-09-07T16:42:52.192Z sha 4a0969d :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-07T16:51:17.059Z sha 68ee035 :: needs real behavior proof before merge. :: none

If the launcher's killall misses a live owner, its cleanup deletes
.imsg-bridge-ready and the replacement stands by, so nothing would write a
marker again and imsg launch would time out against a bridge that is serving.
The owner now restores its marker once a second while it holds the lock, so
readiness stays truthful and the standby still takes over on exit.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0131k87VJuatKyj7U1XJuP3p
@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. label Sep 7, 2026
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. 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. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Sep 7, 2026
@steipete

steipete commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Maintainer verification of ddccd1d6a275879b50eb7b4a0c1b1c127047b9fc:

  • Exact-head macOS and Linux CI passed: https://github.com/openclaw/imsg/actions/runs/34147353108.
  • Independent committed-branch autoreview against origin/main is scoped-clean at P0–P2.
  • All 754 Swift tests and all four native helper suites pass. make lint reports 17 existing warnings and no serious violations. make build-dylib succeeds.
  • The new process regression uses the production IMsgBridgeClient and helper dispatcher/queue. It checks exclusive claims, a standby exiting without touching owner readiness, restored readiness, takeover after SIGKILL, and graceful final cleanup.
  • An independently compiled, Developer-ID-signed library consumer completed 22 real queue ping/pong round trips: 11 while two helper processes coexisted, one after readiness restoration, and ten after killing the owner and observing standby takeover. Claimed-request PIDs matched the expected owner before and after handoff.
  • The before case runs the main-branch watcher/cleanup functions in separate isolated hosts: the second host overwrites the live owner's readiness PID, then deletes readiness when it exits while the first remains alive. Both defects are prevented by the patched lifecycle.
  • The newly built, matching-Developer-ID-signed CLI ran and read one row from the actual Messages database; private fields were withheld.

The isolated hosts load no private Messages frameworks, and no messages were sent. This establishes the changed ownership and request-servicing behavior; it does not claim live Messages injection, delivery, or mixed-version exclusion. The docs explicitly require stopping an older injected helper before upgrading.

Prepared for squash landing by the orchestrator. Merge this PR before #285, which carries the sole changelog entry and thanks @omarshahine. No merge or release was performed in this verification pass.

@steipete
steipete marked this pull request as ready for review September 7, 2026 17:27
@clawsweeper

clawsweeper Bot commented Sep 7, 2026

Copy link
Copy Markdown

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix: give the injected bridge helper a single owner per container This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

@steipete
steipete merged commit 56686c3 into openclaw:main Sep 7, 2026
8 of 10 checks passed
steipete added a commit that referenced this pull request Sep 7, 2026
Document the bridge ownership fix from #284 under Unreleased, with a user-facing highlight and contributor credit.

Keep the native-audio change outside this release pending its existing playback-proof gate.
@omarshahine
omarshahine deleted the fix/helper-single-claimer-guard branch September 7, 2026 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. 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.

bridge: injected helper has no single-claimer guard, so any second injected Messages.app silently races the v2 queue (follow-up to #272)

2 participants