refactor(macos): lock and unify PortGuardian tunnel record persistence so concurrent app instances cannot lose orphan records#100601
Conversation
openclaw-barnacle
Bot
added
app: macos
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b22ed2a146
ℹ️ 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".
| return | ||
| } | ||
| defer { close(fd) } | ||
| _ = flock(fd, LOCK_EX) |
There was a problem hiding this comment.
When another app instance is using port-guard.json, an interrupted or failed flock returns failure, but this code discards the result and still runs the read-merge-write body unlocked. In that failure path the new persistence path can again clobber a sibling instance's tunnel records—the exact concurrent prod+dev scenario this change is meant to protect—so the lock acquisition should be checked/retried or the mutation should fail closed instead of proceeding.
Useful? React with 👍 / 👎.
|
Codex review: needs real behavior proof before merge. Reviewed July 6, 2026, 2:08 AM ET / 06:08 UTC. Summary PR surface: Other +26. Total +26 across 4 files. Reproducibility: yes. source-level. Current main rewrites the shared record file from one actor-local view, and the PR head still has source-visible record-loss paths through failed locking and no-op reaps; I did not run a live macOS multi-instance reproduction. Review metrics: 1 noteworthy metric.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the single shared record-store direction, but require a successful lock, restore kept owned records when disk is missing or stale, and add redacted multi-instance macOS proof before merge. Do we have a high-confidence way to reproduce the issue? Yes, source-level. Current main rewrites the shared record file from one actor-local view, and the PR head still has source-visible record-loss paths through failed locking and no-op reaps; I did not run a live macOS multi-instance reproduction. Is this the best way to solve the issue? No. A single locked read-merge-write path with explicit own-record ownership is the right fix shape, but this implementation must enforce the lock and restore kept owned records before it reliably solves the issue. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 241c761d0036. Label changesLabel justifications:
Evidence reviewedPR surface: Other +26. Total +26 across 4 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
Review history (2 earlier review cycles)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
d8a0044 to
278fa5d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 278fa5d79b
ℹ️ 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".
| if kept != self.records.sorted(by: byTime) || kept != disk.sorted(by: byTime) { | ||
| self.records = kept | ||
| self.save() | ||
| guard !removals.isEmpty else { return } |
There was a problem hiding this comment.
When port-guard.json is missing or has been clobbered while this instance's SSH tunnel is still alive, plan.keep still contains the ownRecords entry but removals is empty, so this early return leaves the disk file without the live tunnel record. The previous sweep path rewrote kept in-memory records whenever disk differed; without that repair, a later app crash leaves the still-running tunnel unrecorded and therefore unreapable by the next launch.
Useful? React with 👍 / 👎.
278fa5d to
2c063c0
Compare
openclaw-barnacle
Bot
added
docs
* origin/main: (1287 commits) fix(android): block loopback canvas navigation (openclaw#99874) fix(hooks): suppress unhandled stdout/stderr stream errors in gmail watcher (openclaw#100519) fix(memory-core): guard qmd mcporter JSON.parse against non-JSON stdout (openclaw#98381) fix(build): fall back to tsx for build TypeScript scripts (openclaw#91262) feat(skills): suggest saving detected reusable workflows by default (openclaw#95477) (openclaw#100692) docs(changelog): remove generated release-note entries feat(telegram): offer BotFather web app flow in setup help and docs (openclaw#100540) fix(ios): unify Talk and Settings row typography on one branded detail row (openclaw#100515) feat(gateway): add persisted crash-loop breaker and fatal-config exit contract refactor(macos): lock and unify PortGuardian tunnel record persistence so concurrent app instances cannot lose orphan records (openclaw#100601) fix: stop reconnecting on protocol mismatch (openclaw#98414) fix(maint): reuse recent hosted gates after rebase (openclaw#100663) fix(ui): reopen web terminals without stale content (openclaw#100665) fix(browser): diagnose empty WSL2 Chrome replies (openclaw#100590) fix(ios): chat snaps back to bottom when scrolling to top via status-bar tap (openclaw#100502) Treat already-compacted CLI compaction as no-op (openclaw#99136) docs(changelog): remove direct main fix entry fix(feishu): strip internal tool-trace banners from outbound text (openclaw#98705) fix(message): thread --limit through to CLI formatter and surface provider pagination hints (openclaw#99089) fix(voyage): close response body stream when batch output JSONL parsing throws (openclaw#98840) ... # Conflicts: # extensions/memory-wiki/package.json
…e so concurrent app instances cannot lose orphan records (openclaw#100601) * refactor(macos): single locked write path and per-instance ownership for PortGuardian tunnel records * chore(i18n): sync native app string inventory for PortGuardian refactor * chore(i18n): sync native app string inventory for PortGuardian refactor
…e so concurrent app instances cannot lose orphan records (openclaw#100601) * refactor(macos): single locked write path and per-instance ownership for PortGuardian tunnel records * chore(i18n): sync native app string inventory for PortGuardian refactor * chore(i18n): sync native app string inventory for PortGuardian refactor
1 participant
Related: #100477
What Problem This Solves
Follow-up hardening and cleanup of the PortGuardian orphan-tunnel work from #100489. The record store had a remaining bug class:
record()/removeRecord()rewroteport-guard.jsonfrom one app instance's in-memory view, so concurrent instances (prod + dev on one machine — the exact scenario from #100477) silently dropped each other's records. A lost record is an unreapable orphan after a crash, which undermines the reaper the file exists to feed. There were also two overlapping kill helpers (both spawning/bin/killsubprocesses, one without exit confirmation), a fire-and-forget record write that could leave a just-spawned tunnel untracked, and assorted vestigial structure.Why This Change Was Made
The record store now has one canonical write path: every mutation is a read-merge-write of the shared file under an
flockon a sidecar lock file (the kernel releases it if the holder dies; the sidecar is needed because atomic writes swap the json inode). Ownership is explicit —ownRecordsholds only pids this process spawned; disk entries from other instances are reap-plan input, never adopted, which kills the resurrection hazard where one instance re-persisted records a sibling had deliberately removed. Removals are content-guarded (reap,removeRecord): a record is deleted only if the file still holds the exact record that was classified, so a sibling's fresh record on a recycled pid always survives. The two kill helpers collapsed into oneDarwin.kill-based TERM→KILL helper with confirmed-exit semantics, which also upgrades the local-mode sweep kill (previously fire-and-forget signal delivery).RemotePortTunnelnow awaits the record write before returning (no untracked-orphan window), shares one teardown path betweendeinitandterminate(), and drops a test-only overload. Vestigial single-element port loops insweep/diagnosewere flattened and the duplicate DEBUG extensions merged.User Impact
Multi-instance dev machines get reliable orphan reaping: tunnel records can no longer be lost to cross-instance clobbering, so every crashed instance's tunnel stays reapable. Local-mode port takeover now waits for the squatter to actually exit before the gateway starts. No user-visible surface changes.
Evidence
swift test --package-path apps/macos --parallel: 639 tests in 113 suites, all passed.swiftformat --lintclean; Codex autoreview clean on the first pass ("patch is correct", no actionable findings).