Repository navigation
Avoid PostHog flush deadlock during quit - #6232
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesPostHogAnalytics dependency injection refactor and test migration
CI workflow sudo availability check
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Greptile SummaryFixes a quit/update hang by removing the synchronous PostHog flush from
Confidence Score: 5/5Safe to merge. The deadlock path is cleanly removed and active-event flushing continues at the right point in the app lifecycle. The change removes a synchronous cross-queue wait from the termination path — the root cause of the reported hang — and replaces it with nothing, which is correct because active events are already flushed at capture time. The @unchecked Sendable invariants (didStart confined to the work queue, activeCheckTimer confined to the main queue) hold throughout the file. Dependency injection is #if DEBUG-gated and the init stays private, preserving the singleton contract in production. The test coverage is deterministic and exercises the capture+flush ordering end-to-end. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Main as Main Thread
participant WQ as workQueue
participant SDK as PostHogSDK
Note over Main,SDK: BEFORE (deadlock path)
Main->>Main: applicationWillTerminate
Main->>WQ: "workQueue.sync { flush() }"
WQ-->>Main: (blocked waiting for main-thread notification work)
Main-->>WQ: (blocked waiting for workQueue)
Note over Main,WQ: DEADLOCK
Note over Main,SDK: AFTER (this PR)
Main->>Main: applicationWillTerminate
Note over Main: PostHog flush removed entirely
Main->>Main: cancel breadcrumb task, clearAll, markCleanExit
Note over Main,SDK: Active-event flush (while alive)
Main->>WQ: trackActive() [async]
WQ->>SDK: capturePostHog(daily_active)
WQ->>SDK: capturePostHog(hourly_active)
WQ->>SDK: flushPostHog() [once, if anything captured]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Main as Main Thread
participant WQ as workQueue
participant SDK as PostHogSDK
Note over Main,SDK: BEFORE (deadlock path)
Main->>Main: applicationWillTerminate
Main->>WQ: "workQueue.sync { flush() }"
WQ-->>Main: (blocked waiting for main-thread notification work)
Main-->>WQ: (blocked waiting for workQueue)
Note over Main,WQ: DEADLOCK
Note over Main,SDK: AFTER (this PR)
Main->>Main: applicationWillTerminate
Note over Main: PostHog flush removed entirely
Main->>Main: cancel breadcrumb task, clearAll, markCleanExit
Note over Main,SDK: Active-event flush (while alive)
Main->>WQ: trackActive() [async]
WQ->>SDK: capturePostHog(daily_active)
WQ->>SDK: capturePostHog(hourly_active)
WQ->>SDK: flushPostHog() [once, if anything captured]
Reviews (7): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/PostHogAnalytics.swift (1)
14-15: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider making event name constants static to eliminate duplication.
The event names
"cmux_daily_active"and"cmux_hourly_active"appear as instance properties (lines 14-15) and as hardcoded literals in the staticshouldFlushAfterCapture(lines 273). If someone changes one location but not the other, the flush-after-capture behavior would silently break.♻️ Suggested refactor
- private let dailyActiveEvent = "cmux_daily_active" - private let hourlyActiveEvent = "cmux_hourly_active" + private static let dailyActiveEvent = "cmux_daily_active" + private static let hourlyActiveEvent = "cmux_hourly_active"Then update
shouldFlushAfterCapture:nonisolated static func shouldFlushAfterCapture(event: String) -> Bool { switch event { - case "cmux_daily_active", "cmux_hourly_active": + case dailyActiveEvent, hourlyActiveEvent: return true default: return false } }And update references in instance methods (e.g.,
let event = Self.dailyActiveEvent).Also applies to: 271-278
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/PostHogAnalytics.swift` around lines 14 - 15, Convert the event name constants dailyActiveEvent and hourlyActiveEvent from instance properties to static properties at lines 14-15. Then update the static method shouldFlushAfterCapture (lines 271-278) to reference these static properties instead of using hardcoded string literals. Finally, update any instance method references to use Self.dailyActiveEvent and Self.hourlyActiveEvent instead of accessing them as instance properties, ensuring the event names are defined and referenced from a single source.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/PostHogAnalytics.swift`:
- Around line 14-15: Convert the event name constants dailyActiveEvent and
hourlyActiveEvent from instance properties to static properties at lines 14-15.
Then update the static method shouldFlushAfterCapture (lines 271-278) to
reference these static properties instead of using hardcoded string literals.
Finally, update any instance method references to use Self.dailyActiveEvent and
Self.hourlyActiveEvent instead of accessing them as instance properties,
ensuring the event names are defined and referenced from a single source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ca43ffa-d887-4ade-8b41-be4b65ca6cf0
📒 Files selected for processing (2)
Sources/PostHogAnalytics.swiftcmuxTests/GhosttyConfigTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/PostHogAnalyticsPropertiesTests.swift`:
- Around line 130-132: The test waits for only one flushCalled signal on line
130 before asserting both daily and hourly active events on line 132, creating a
race condition. Modify the wait logic to account for both flush events: either
call flushCalled.wait() twice consecutively with appropriate timeouts, or
restructure the synchronization to ensure both flushes are captured before the
assertion that validates the captured events contain both "cmux_daily_active"
and "cmux_hourly_active" entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc57c362-7f36-489d-b6f6-acdc343d62a9
📒 Files selected for processing (4)
Sources/PostHogAnalytics.swiftcmux.xcodeproj/project.pbxprojcmuxTests/GhosttyConfigTests.swiftcmuxTests/PostHogAnalyticsPropertiesTests.swift
💤 Files with no reviewable changes (1)
- cmuxTests/GhosttyConfigTests.swift
…dlock # Conflicts: # .github/workflows/perf-activation.yml
8 tasks done
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
applicationWillTerminatesynchronously waited insidePostHogAnalytics.flush()while the analytics queue was waiting on main-thread notification work.Verification
PostHogAnalyticsPropertiesTests.flushReturnsWithoutWaitingForBusyWorkQueue()failed withflushReturned.wait(...) == .timedOut.xcodebuild -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-phquit-final test-without-building -only-testing:cmuxTests/PostHogAnalyticsPropertiesTestspassed 7 Swift Testing tests, includingactiveEventCaptureFlushesBeforeShutdown().\n- Previous tagged preflight: launchedcmux DEV phquit, moved it to LG HDR 4K, confirmed the debug socket, captured rendered UI with Computer Use, then terminated bundlecom.cmuxterm.app.debug.phquitthroughNSRunningApplication.terminate(); pid 16056 reportedterminated truewithin 10 seconds.\n\n## Risk\nTermination no longer makes a last-chance analytics flush. Active events still flush when captured, and quit/update correctness is no longer coupled to telemetry delivery.\nSummary by CodeRabbit