fix: remove lock-held file I/O and nested lock from userdata registry - #263
Merged
Conversation
…g pointer GhosttySurfaceUserdataRegistry (added in #262 to stop ghostty callbacks resolving freed GhosttySurfaceCallbackContext pointers) held its lock across a DEBUG dlog() call on the stale-pointer path, and resolve(from:) nested the context's own tabIdLock inside the registry lock while dereferencing the pointer. Both are gone: the registry now stores the (surfaceId, tabId) snapshot directly in a dictionary keyed by pointer, so resolve(from:) is a pure lookup under one lock, and the stale dlog fires after the lock is released. - GhosttySurfaceCallbackContext no longer stores tabId; it's now just an identity/lifetime token for Unmanaged retain/release bookkeeping (its tabIdLock/_tabId/tabId getter/updateTabId were only read by the old resolve(from:), which no longer dereferences the class at all). - TerminalSurface.updateWorkspaceId now propagates a tabId reassignment into the registry via GhosttySurfaceUserdataRegistry.updateTabId(pointer:tabId:), the same snapshot callbacks resolve. - Regression test testStaleSurfaceUserdataResolvesToNilInsteadOfCrashing still passes unmodified (comment updated to note the mechanism change); added testSurfaceUserdataResolvesUpdatedTabIdAfterWorkspaceReassignment to pin the new tabId-propagation behavior.
4 tasks
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.
What this does
#262 (just merged) stopped ghostty's callback threads from dereferencing a
freed Swift object — that crash-loop is gone (0 minidumps on the re-land
branch, down from 6/run). This PR removes two rough edges in how it did
that: the stale-pointer debug log used to run while holding the global
registry lock (synchronous file I/O, on the frequent path during rapid
surface teardown), and resolving a live pointer nested the object's own
lock inside that registry lock. Neither is a proven cause of anything yet,
but a socket-integration test and a focus test that had never failed
before both started timing out ("Socket closed" / hang) in the same test
run that added #262 — this is a plausible explanation, not a confirmed
fix, and the re-land gate will tell us if it holds.
Summary
GhosttySurfaceUserdataRegistrynow owns the callback data directly: itsstorage changed from
Set<UnsafeMutableRawPointer>to[UnsafeMutableRawPointer: GhosttySurfaceCallbackSnapshot].resolve(from:)is now a pure dictionary lookup under one lock — it never calls
Unmanaged.fromOpaque(_:).takeUnretainedValue()and never touches theGhosttySurfaceCallbackContextinstance.dlog(...)now fires after the lock is released,not while held.
GhosttySurfaceCallbackContextno longer storestabId— itstabIdLock/_tabId/tabIdgetter/updateTabIdwere only ever read bythe old
resolve(from:), which no longer dereferences the class at all.The class is now just an identity/lifetime token for
Unmanagedretain/release bookkeeping.TerminalSurface.updateWorkspaceIdnow propagates atabIdreassignmentinto the registry via
GhosttySurfaceUserdataRegistry.updateTabId(pointer:tabId:)— the samesnapshot callbacks resolve — instead of only updating the (no-longer-read)
context instance.
Sources/for any otherdlog(call made while a lock is held ona callback-frequency path introduced by fix: reject stale ghostty surface userdata pointers before ARC retain #262: none found. The lock-held
dlog fixed here was the only instance.
Test Plan
xcodebuild -project GhosttyTabs.xcodeproj -scheme programa -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/programa-registry build→ BUILD SUCCEEDED-only-testing:programaTests/WorkspaceSplitWorkingDirectoryTests→ 5/5 passed, including the existingtestStaleSurfaceUserdataResolvesToNilInsteadOfCrashing(unmodified assertions, comment updated) and the newtestSurfaceUserdataResolvesUpdatedTabIdAfterWorkspaceReassignment