Repository navigation
iroh-diag relay profile lines + socket cmd+q terminateLater deadlock fix - #10814
Conversation
The verb printed only the DiagnosticLog timeline; proving which relay the endpoint used required netstat. Append an active-relay section (managed catalog / custom / CMUX_IROH_RELAY_URL_OVERRIDE debug override, plus URLs) from a nonisolated mirror of the installed policy so the verb stays usable while the main thread is wedged. Relay URLs stay out of the privacy-safe DiagnosticLog report itself.
…'s block Regression test for #10788. Routes the quit shortcut path's NSApp.terminate through one shared AppTerminationRequest seam (still synchronous here, so this commit stays red) and asserts the scheduled terminate does not run inside the requesting main-queue block.
…Later deadlock AppTerminationRequest.schedule now defers NSApp.terminate to a main-run-loop callout (RunLoop.main.perform in common modes) instead of calling it inside the requesting block. A simulate_shortcut cmd+q handler runs inside v2MainSync's DispatchQueue.main.sync block; terminating there left the main queue occupied while applicationShouldTerminate's .terminateLater cleanup task waited for it, hanging the app forever. Fixes #10788.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 91799653f7
ℹ️ 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".
| Self.relayDiagMirror.withLock { [relayPolicyEffective] state in | ||
| state = relayPolicyEffective.map { | ||
| RelayDiagState( | ||
| source: $0.source, | ||
| usedCachedPolicy: $0.usedCachedPolicy, | ||
| relayURLs: $0.endpointRelayProfile.allowedRelayURLs.sorted() | ||
| ) |
There was a problem hiding this comment.
When a live policy refresh assigns relayPolicyEffective and runtime.replaceRelayPolicy(effective) then throws (for example because the runtime is inactive or the relay fleet no longer matches), this observer updates the diagnostic mirror before installation succeeds. The endpoint can therefore continue using its previous profile while iroh_diag reports the rejected policy's URLs as active, and the catch path never restores the old mirror. Update the mirror only after replacement succeeds or roll it back on failure.
AGENTS.md reference: AGENTS.md:L117-L121
Useful? React with 👍 / 👎.
…tor mirror CmxIrohDebugRelayOverride keeps no new static members; the diag URL is read through an injectable CmxIrohDebugRelayOverrideDiagnostics struct in its own package file. The relay diag mirror moves to MobileHostIrohRuntime+RelayDiag.swift and becomes a revision-ordered actor instead of an OSAllocatedUnfairLock, read from the iroh_diag Task off-main so the wedged-main-thread guarantee is unchanged. AppTerminationRequest moves to its own Sources file.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d3a58b38d
ℹ️ 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".
| guard let policy else { | ||
| lines.append("Source: none installed (no relay policy this launch)") | ||
| return lines.joined(separator: "\n") |
There was a problem hiding this comment.
When no relay-policy trust root is available, activation still installs a selected custom profile into endpointRelayProfile, but it deliberately leaves resolvedEffectivePolicy (and therefore relayDiagMirror) as nil in MobileHostIrohRuntime+Activation.swift. As a result, iroh_diag reaches this guard and reports none installed even though the endpoint is actively using the custom relay profile, making the new diagnostics misleading for that configuration. Populate the mirror from the actual endpoint profile (including unavailable custom selections) or otherwise preserve the selected custom source and URLs.
Useful? React with 👍 / 👎.
…ility The actor mirror published through a detached Task, so an iroh_diag read racing a policy installation could report the previous or missing profile (second review round's finding). Restore the OSAllocatedUnfairLock mirror, now in MobileHostIrohRuntime+RelayDiag.swift: the didSet writer publishes synchronously before returning, and the reader stays off the main actor so the verb still works while the main thread is wedged. This matches the in-tree AgentChatThemeSync nonisolated-static-lock precedent; the Aziz lock-vs-actor lint intentionally stays flagged because an actor cannot give a synchronous writer read-after-write visibility here.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
1 participant
Two small debug-workflow fixes, found during iroh preflights.
iroh-diag relay URL. The socket verb printed only the DiagnosticLog timeline; proving which relay the endpoint used required netstat. The verb now appends an active-relay section: profile source (managed catalog, cached catalog, custom, unavailable, or the CMUX_IROH_RELAY_URL_OVERRIDE debug override) and the relay URLs. The data comes from a nonisolated OSAllocatedUnfairLock mirror in
MobileHostIrohRuntime+RelayDiag.swift, written synchronously by therelayPolicyEffectivedidSet funnel, so the verb keeps its no-main-actor-hop guarantee and still works while the main thread is wedged, and a read immediately after installation sees the new policy. Relay URLs stay out of DiagnosticLog and the Settings export, which remain privacy-safe; only the local debug socket prints them. The override is consulted first (via the injectableCmxIrohDebugRelayOverrideDiagnostics) because every profile installation funnel substitutes it while active.Socket cmd+q deadlock, fixes #10788.
simulate_shortcut cmd+qruns the quit path insidev2MainSync'sDispatchQueue.main.syncblock. CallingNSApp.terminatethere deadlocked:applicationShouldTerminatereturns.terminateLater, AppKit spins the run loop waiting forreplyToApplicationShouldTerminate, and the deferred@MainActorcleanup task can never start while the main queue is still inside the socket-command block. The quit shortcut path now routes through one sharedAppTerminationRequest.schedule(Sources/AppTerminationRequest.swift), which defersNSApp.terminateto aRunLoop.main.perform(inModes: [.common])callout, so the handler replies and releases the main queue first. This matches how a real keyboard Cmd+Q arrives (run-loop event callout with an idle main queue).DispatchQueue.main.asyncwould not fix it: terminate would still spin its wait loop from inside a main-queue block, and the cleanup task still could not run.Regression test commits follow the red/green convention: the test commit routes the quit path through the seam while keeping the synchronous (deadlocking) dispatch, so
AppTerminationRequestDispatchTestsfails there; the fix commit turns the seam into the run-loop deferral.Base is
feat-iroh-integration-test, notmain: the diag override reporting depends onCmxIrohDebugRelayOverride, which only exists on that branch.Pinned local review (gpt-5.6-sol, high): final verdict "patch is correct", no actionable findings. One Aziz P2 lint remains open by design: the lock-vs-actor rule conflicts with the reviewer's read-after-write requirement here (an actor write from the synchronous didSet would be a detached hop and reintroduce the stale-read race the second review round flagged); the mirror follows the in-tree
AgentChatThemeSyncnonisolated-static-lock precedent and documents the reasoning inline.Verified:
swift test --filter CmxIrohDebugRelayOverrideTestsin CmuxIrohTransport passes; taggediwrtbuild of the final head launched twice,simulate_shortcut cmd+qover/tmp/cmux-debug-iwrt.sockreturnedOKand the app exited cleanly within 1 s both times (previously hung forever, 2/2);iroh-diagprinted the relay section on both builds (Source: managed catalogwith the seven managed relay URLs once the account policy restored,Source: none installedbefore restore).