Repository navigation
Fix Cmd+N nightly crash: avoid local Workspace refs in ARC hotpath - #2204
Conversation
The snapshot approach (c1998e3) navigated workspace → panel → surface through local variables. Xcode 16.4's -O ARC optimizer aggressively elides retains on these locals through inlined call chains, causing use-after-free on every Cmd+N in CI-built nightlies. Fix: extract preferredWorkingDirectory and inheritedTerminalFontPoints through self (always retained) BEFORE capturing locals. The snapshot is now purely value-typed with no Workspace references held in locals. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe workspace creation snapshot logic in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a use-after-free crash on every Cmd+N in CI-built nightlies (Xcode 16.4, macOS 15) by eliminating local Confidence Score: 5/5Safe to merge — targeted ARC fix with no behavioral regressions and new behavioral test coverage. The fix is logically sound: extracting values through retained No files require special attention. Both changed files are clean. Important Files Changed
Sequence DiagramsequenceDiagram
participant C as Caller (Cmd+N)
participant A as addWorkspace()
participant S as self (retained)
participant L as workspaceCreationSnapshotLite()
participant D as didCaptureWorkspaceCreationSnapshot()
participant N as newTabInsertIndex()
C->>A: addWorkspace(placementOverride:)
Note over A: self is always retained for duration
A->>S: preferredWorkingDirectoryForNewTab()
S-->>A: preferredDir (String?)
A->>S: inheritedTerminalFontPointsForNewWorkspace()
S-->>A: inheritedFontPoints (Float?)
Note over A: Capture value types: tabs, selectedTabId
A->>L: workspaceCreationSnapshotLite(currentTabs, currentSelectedTabId, preferredDir, inheritedFontPoints)
L-->>A: WorkspaceCreationSnapshot (pure value type)
A->>D: didCaptureWorkspaceCreationSnapshot()
Note over D: Mid-creation mutations allowed here (close/reorder)
A->>N: newTabInsertIndex(snapshot, placementOverride)
N-->>A: insertIndex
A->>A: insert newWorkspace at insertIndex into live tabs
A-->>C: Workspace
Reviews (1): Last reviewed commit: "fix: extract workspace config through se..." | Re-trigger Greptile |
Resolve conflicts in TabManager.swift and WorkspaceUnitTests.swift, keeping the hardened addWorkspace() that extracts config data through self before capturing locals (Xcode 16.x ARC optimizer fix). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func testAddWorkspaceSurvivesMidCreationClose() { | ||
| let manager = SnapshotMutatingTabManager() | ||
| guard let first = manager.tabs.first else { | ||
| XCTFail("Expected initial workspace") | ||
| return | ||
| } | ||
|
|
||
| let closingWorkspace = manager.addWorkspace() | ||
| let third = manager.addWorkspace() | ||
| manager.selectWorkspace(third) | ||
|
|
||
| let closingWorkspaceId = closingWorkspace.id | ||
| XCTAssertEqual(manager.tabs.map(\.id), [first.id, closingWorkspaceId, third.id]) | ||
|
|
||
| manager.afterCaptureWorkspaceCreationSnapshot = { | ||
| guard let liveWorkspace = manager.tabs.first(where: { $0.id == closingWorkspaceId }) else { | ||
| XCTFail("Expected captured workspace to still be present when closing after snapshot") | ||
| return | ||
| } | ||
| manager.closeWorkspace(liveWorkspace) | ||
| } | ||
|
|
||
| let inserted = manager.addWorkspace(placementOverride: .afterCurrent) | ||
|
|
||
| XCTAssertFalse(manager.tabs.contains(where: { $0.id == closingWorkspaceId })) | ||
| XCTAssertEqual(manager.tabs.map(\.id), [first.id, third.id, inserted.id]) | ||
| XCTAssertEqual(manager.selectedTabId, inserted.id) | ||
| } |
There was a problem hiding this comment.
Per the CLAUDE.md regression test policy, new tests for a bug fix should be committed separately before the fix so that CI goes red first (proving the test catches the bug), then green after the fix lands.
This new test (testAddWorkspaceSurvivesMidCreationClose) and the production fix were committed together as c5837dbd. If the test actually fails on the unfixed code, it should have been split into two commits to demonstrate that on CI.
That said, because the underlying bug is an ARC optimizer use-after-free that manifests only in Release/CI builds (not easily provable in unit tests), and this test appears to exercise the behavioral outcome of a mid-creation close of a non-selected workspace (rather than the crash path directly), it may well pass on the unfixed code — in which case there is nothing to "prove red" and the single-commit structure is fine. If this test doesn't actually fail on the pre-fix code, a short comment on the test explaining that would be helpful context for future readers.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…anaflow-ai#2204) * test: reproduce Cmd+N snapshot workspace lifetime race * fix: retain snapshot workspaces through Cmd+N creation * fix: repair workspace lifetime regression test * fix: extract workspace config through self to avoid Xcode 16.x ARC crash The snapshot approach (f2e5091) navigated workspace → panel → surface through local variables. Xcode 16.4's -O ARC optimizer aggressively elides retains on these locals through inlined call chains, causing use-after-free on every Cmd+N in CI-built nightlies. Fix: extract preferredWorkingDirectory and inheritedTerminalFontPoints through self (always retained) BEFORE capturing locals. The snapshot is now purely value-typed with no Workspace references held in locals. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Summary
preferredWorkingDirectoryandinheritedTerminalFontPointsthroughself(always retained) before capturing locals, instead of navigating workspace → panel → surface through local variables inside the snapshot-OARC optimizer aggressively elides retains on local Workspace references through inlined call chains, causing use-after-free on every Cmd+N in CI-built nightlieswithExtendedLifetimewrapper which was insufficient against the inlining optimizerContext
This crash reproduced on every Cmd+N in the CI nightly (Xcode 16.4, macOS 15) but not in local builds (Xcode 26.4, macOS 26). The root cause was
c1998e34introducingworkspaceCreationSnapshot()which held local Workspace refs across deep call chains.Test plan
🤖 Generated with Claude Code
Summary by cubic
Fixes Cmd+N crashes in CI nightlies by removing local Workspace refs from the creation path and making the snapshot value-only. Addresses issue 2180; config is pulled via self to avoid Xcode 16.4 ARC retain elision.
Written for commit 71d89b2. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Tests