Skip to content

fix: make bridge rpc text sends idempotent#159

Open
steipete wants to merge 3 commits into
mainfrom
fix/rpc-bridge-send-idempotence
Open

fix: make bridge rpc text sends idempotent#159
steipete wants to merge 3 commits into
mainfrom
fix/rpc-bridge-send-idempotence

Conversation

@steipete

@steipete steipete commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • atomically claim each v2 bridge inbox file before reading or dispatching it, making request ownership cross-process instead of process-local
  • bootstrap the bridge only inside the com.apple.MobileSMS process and keep skipped injected processes from deleting the real bridge-ready lock during teardown
  • discard dead/stale claim files without replaying a possibly delivered side effect
  • add focused ordering, cleanup, and bootstrap regression coverage plus a 0.12.4 changelog entry

Refs #158.

Why

The reported failure produces two physical rows with distinct GUIDs from one logical bridge request at a stable ~60–64 ms gap, while the single response can identify either row. That matches two injected inbox watchers reading and dispatching the same finalized request before either removes it.

The previous candidate pinned a client-generated IMMessage GUID. Current macOS 26 plain-text construction already assigns one GUID before its single dispatch, so that approach did not prevent two sendMessage: calls. This rewrite fixes ownership at the queue boundary: a same-directory atomic rename gives exactly one process the request before any side effect.

Verification

  • rebased onto 2ea890f (main)
  • DEVELOPER_DIR=/Applications/Xcode-26.6.app/Contents/Developer make test — 375 tests passed
  • DEVELOPER_DIR=/Applications/Xcode-26.6.app/Contents/Developer make lint — passed; 7 existing warnings, 0 serious violations
  • DEVELOPER_DIR=/Applications/Xcode-26.6.app/Contents/Developer make build — universal CLI (arm64, x86_64) and helper (arm64e, arm64, x86_64) built successfully
  • focused helper compilation and claim-ordering test passed after the review fix
  • Codex autoreview — first pass found missing orphan-claim cleanup; fixed; local rerun and final branch review both clean with no accepted/actionable findings

Live validation gate

Not merge-ready yet. The available test host has SIP enabled and reports v2_ready: false, so it cannot exercise injected IMCore delivery.

@lowcode-garage please run the issue's fresh-process probe against this head on the reproducing SIP-disabled Mac for both direct to and chat_guid sends. Please include request count, response count, sender-row count, unique text/GUID count, response-GUID-to-row mapping, duplicate/missing texts, and recipient-side confirmation that each request delivered exactly once. The bridge log now records request UUID, PID, and process name so we can also confirm one handler claimed each request.

@clawsweeper

clawsweeper Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codex review: found issues before merge. Reviewed July 9, 2026, 7:16 PM ET / 23:16 UTC.

Summary
The branch changes the injected v2 RPC bridge inbox watcher to claim request files atomically, guard bridge bootstrap and cleanup to Messages.app, add source-level regression tests, and edit the unreleased changelog.

Reproducibility: yes. for the linked bug: the canonical issue gives a concrete fresh-process imsg rpc loop and chat.db grouping checks on a macOS bridge host. I did not run live Messages delivery from this Linux checkout.

Review metrics: 3 noteworthy metrics.

  • Changed surface: 4 files changed, +134/-24. The diff is focused but changes the injected helper queue path that controls physical message delivery.
  • Live delivery proof: 0 after-fix SIP-disabled bridge runs attached. The PR itself says the available host cannot exercise injected IMCore delivery, which is the behavior under review.
  • Release-owned files: 1 CHANGELOG.md edit. Release notes are owned outside normal PR branches, so maintainers should notice this before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #158
Summary: This PR is the active candidate fix for the canonical duplicate bridge-backed RPC text-send issue.

Members:

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

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Attach redacted live proof from the reproducing SIP-disabled Mac for both direct to and chat_guid sends, including request/response counts, outbound-row counts, unique GUID counts, response mapping, duplicate/missing texts, and recipient confirmation.
  • Remove the release-owned CHANGELOG.md edit and keep release-note context in the PR body or commit message.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Missing: the PR lists tests, lint, build, and source-level checks but explicitly says the available host cannot exercise injected IMCore delivery; it needs redacted live output or logs from the reproducing SIP-disabled Mac. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] Without live SIP-disabled proof, source tests cannot prove that Messages/IMCore now delivers exactly one physical outbound row per bridge RPC request.
  • [P1] The patch changes bridge request ownership and orphan-claim cleanup in the message-delivery path; a handler death after claim is intentionally not replayed, so live validation should cover duplicate, missing, and response-mapping outcomes.
  • [P1] The PR edits release-owned CHANGELOG.md; release-note context should stay in the PR body or commit message unless the release owner explicitly accepts it.

Maintainer options:

  1. Collect live bridge proof first (recommended)
    Have the reproducing SIP-disabled Mac run direct to and chat_guid probes and attach redacted counts/logs proving one physical send per request before merge.
  2. Merge on source confidence
    Maintainers can accept the remaining IMCore-delivery uncertainty if urgency outweighs waiting for the reproducing host.
  3. Hold the branch
    Keep this PR paused until the SIP-disabled validation environment is available rather than landing an unproven delivery-path change.

Next step before merge

  • [P2] Human review is needed for proof sufficiency because automation cannot produce SIP-disabled Messages delivery proof from this checkout.

Maintainer decision needed

  • Question: Should this message-delivery fix wait for SIP-disabled live bridge proof before merge, or may maintainers accept source and unit-test confidence only?
  • Rationale: The source repair is plausible, but the reported failure occurs in private Messages/IMCore physical delivery that this checkout cannot exercise and the PR body still marks as unproven.
  • Likely owner: steipete — steipete owns the current fix branch, has current-main history in the watcher, and already asked for the exact live validation gate.
  • Options:
    • Wait for reproducer proof (recommended): Require the reproducing Mac to attach redacted request, response, outbound-row, unique-GUID, response-mapping, duplicate/missing-text, and recipient-confirmation results before merge.
    • Accept source-only risk: Merge based on the atomic-claim implementation and tests, with maintainers owning any remaining duplicate-send or missing-send behavior found only in live IMCore.
    • Pause until a Mac is available: Leave the PR open but blocked if the reproducing SIP-disabled environment cannot be scheduled soon.

Security
Cleared: No concrete security or supply-chain regression was found; the diff adds no dependencies, workflows, secrets handling, or broader permissions.

Review findings

  • [P3] Remove the release-owned changelog entry — CHANGELOG.md:9
Review details

Best possible solution:

Land the atomic queue-claim fix after removing the release-owned changelog edit and attaching redacted live proof that direct to and chat_guid bridge sends produce one response, one outbound row, one unique GUID/text, and one recipient-visible delivery per request.

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

Yes, for the linked bug: the canonical issue gives a concrete fresh-process imsg rpc loop and chat.db grouping checks on a macOS bridge host. I did not run live Messages delivery from this Linux checkout.

Is this the best way to solve the issue?

Yes, conditionally: claiming the inbox file before reading or dispatch is the narrowest source-level fix for a cross-process duplicate consumer race, but it needs live delivery proof and the release-owned changelog edit removed before merge.

Full review comments:

  • [P3] Remove the release-owned changelog entry — CHANGELOG.md:9
    CHANGELOG.md is release-owned in this review workflow, so normal PR branches should keep release-note context in the PR body or commit message instead. This entry was already visible on an earlier reviewed head and remains unchanged; please drop the changelog edit before merge.
    Confidence: 0.89
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is correct
Overall confidence: 0.84

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P1: The PR targets an urgent duplicate outbound iMessage delivery bug in a bridge-backed send workflow used by real channel automation.
  • merge-risk: 🚨 message-delivery: Merging changes bridge request ownership and delivery cleanup behavior, and only live IMCore proof can show it does not duplicate, suppress, or mis-identify messages.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Missing: the PR lists tests, lint, build, and source-level checks but explicitly says the available host cannot exercise injected IMCore delivery; it needs redacted live output or logs from the reproducing SIP-disabled Mac. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read fully; its test guidance and macOS Messages permission notes informed the proof expectations for this bridge delivery change. (AGENTS.md:19, d9bd9c754d50)
  • Current main uses process-local inbox dedupe: Current main reads the finalized inbox JSON directly, dispatches the envelope, then removes the inbox file while using process-local processedRpcIds, so the cross-process claim behavior is not already on main. (Sources/IMsgHelper/IMsgInjected.m:4518, d9bd9c754d50)
  • PR claims before dispatch: The PR head renames the inbox request to a per-PID claim path before reading it or calling processV2Envelope, which is the intended queue-ownership fix. (Sources/IMsgHelper/IMsgInjected.m:4530, 9f2bcf2533ea)
  • PR avoids replaying orphaned claims: The PR removes claims owned by the current process, dead PIDs, or claims older than kV2ClaimMaxAge before scanning finalized JSON files, which deliberately avoids replaying a possibly delivered side effect. (Sources/IMsgHelper/IMsgInjected.m:4592, 9f2bcf2533ea)
  • PR guards injected bootstrap and teardown: The PR only bootstraps inside com.apple.MobileSMS and skips destructor cleanup when bootstrap did not complete, matching the stated skipped-process lock-file concern. (Sources/IMsgHelper/IMsgInjected.m:4678, 9f2bcf2533ea)
  • Focused source-level regression coverage: The added test asserts claim-before-read-before-dispatch ordering, removal of processedRpcIds, orphan cleanup, and the skipped-process cleanup guard. (Tests/imsgTests/BridgeCommandRegistrationTests.swift:107, 9f2bcf2533ea)

Likely related people:

  • steipete: Current-main blame for the v2 inbox watcher points to Peter Steinberger's v0.12.3 work, and steipete authored this atomic-claim branch and requested the live proof gate. (role: recent area contributor and active fix-candidate owner; confidence: high; commits: dea78a9e9c49, 2ea890ff8172, 9f2bcf2533ea; files: Sources/IMsgHelper/IMsgInjected.m, Sources/imsg/RPCServer+Support.swift, Tests/imsgTests/BridgeCommandRegistrationTests.swift)
  • omarshahine: Repository history shows Omar Shahine introduced the BlueBubbles private-API bridge and made substantial macOS 26 bridge helper repairs in the same injected-helper area. (role: original bridge contributor and adjacent send-path contributor; confidence: medium; commits: c56c24d488ef, 2d7b506d1736, 1560a678ce0f; files: Sources/IMsgHelper/IMsgInjected.m, Sources/IMsgCore/IMsgBridgeClient.swift, Sources/IMsgCore/IMsgBridgeProtocol.swift)
  • TurboTheTurtle: The merged reply-routing work changed the shared plain RPC send bridge route and tests adjacent to this duplicate-send path. (role: recent adjacent RPC send contributor; confidence: medium; commits: f4cde96cb9eb; files: Sources/imsg/RPCServer+Support.swift, Tests/imsgTests/RPCServerBridgeTests.swift, Sources/IMsgHelper/IMsgInjected.m)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.
Review history (4 earlier review cycles)
  • reviewed 2026-07-06T15:24:36.081Z sha ac27a79 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-09T21:06:49.000Z sha 41481e4 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-09T21:11:32.678Z sha 41481e4 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-09T21:21:21.913Z sha 9f2bcf2 :: found issues before merge. :: [P3] Remove the release-owned changelog entry

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. labels Jul 6, 2026
@steipete steipete force-pushed the fix/rpc-bridge-send-idempotence branch from ac27a79 to 41481e4 Compare July 9, 2026 21:02
@steipete steipete force-pushed the fix/rpc-bridge-send-idempotence branch from 41481e4 to 9f2bcf2 Compare July 9, 2026 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 message-delivery 🚨 Merging this PR could drop, duplicate, misroute, suppress, or wrongly target messages. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant