Skip to content

fix(mac): avoid provider presence deadlock - #1212

Merged
iamtoruk merged 2 commits into
getagentseal:mainfrom
MiguelMachado-dev:fix/menubar-presence-deadlock
Sep 1, 2026
Merged

fix(mac): avoid provider presence deadlock#1212
iamtoruk merged 2 commits into
getagentseal:mainfrom
MiguelMachado-dev:fix/menubar-presence-deadlock

Conversation

@MiguelMachado-dev

Copy link
Copy Markdown
Contributor

Problem

Checking whether a provider's credentials are present could deadlock the menubar app — the presence check and the credential store's mutation path took the same lock in opposite order (reproducer: presenceWritesDoNotDeadlockSwiftUICallbacks).

Fix

Reorder the lock acquisition in CapacityDockProviderCredential so presence reads never wait on the write path.

Tests

  • New ProviderSettingsEditorStateTests regression coverage (37 lines)
  • Full menubar test suite passes locally (352 tests)

A background credential read held the provider-presence lock while UserDefaults synchronously notified SwiftUI. SwiftUI could re-read presence while holding its own update lock, leaving each thread waiting on the other. Serialize presence writes on the main queue and release the presence lock before publishing defaults changes.
@MiguelMachado-dev

Copy link
Copy Markdown
Contributor Author

The test failures are unrelated to this PR — pre-existing date-dependent bug in tests/cli-budget.test.ts (seeds at today 12:00 UTC, which is in the future before noon UTC, so the overview shows no usage). All four of these PRs fail on the identical test. Reproduced and fixed in #1216; a re-run after that merges (or after 12:00 UTC) goes green.

…ck test cleanup

The lock no longer covers the defaults write, so the main-queue hop is
what keeps the read-modify-write atomic; say so where the next reader
would be tempted to make it async. The regression test now cleans up its
observer and defaults domain on every exit and waits up to 5 seconds, so
a loaded runner cannot fake a deadlock.
@iamtoruk

iamtoruk commented Sep 1, 2026

Copy link
Copy Markdown
Member

Reviewed in depth and merging. Verified independently: your regression test fails on origin/main (both flags false, the writing thread is wedged) and passes on this branch, so the deadlock is real and the fix is at the root: the reentrant contains() from the synchronous UserDefaults notification now finds the lock free. Full suite 349 tests in 44 suites green. Two small things patched on top rather than sent back: a comment explaining that the main-queue hop is now what keeps the read-modify-write atomic (so nobody flips it to async later), and the test cleanup moved into defer with a 5 second bound so a loaded runner cannot fake a deadlock. One note on the description: the mechanism is single-thread reentrancy on a non-recursive NSLock, not two locks taken in opposite order.

@iamtoruk
iamtoruk merged commit fecbcd6 into getagentseal:main Sep 1, 2026
5 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants