Skip to content

ServiceQueue: the async overload silently defeats the serialization its callers assume #649

Description

@jvsena42

Problem

ServiceQueue.background has two overloads. The synchronous one really does serialize:

service.queue.async {
    let res = try blocking()      // runs on the queue thread
    continuation.resume(...)
}

The async one does not:

service.queue.async {
    Task {                         // hops straight off the DispatchQueue
        let result = try await execute()
        ...
    }
}

The Task detaches immediately, so the queue block returns before any work happens and the body runs on the cooperative pool. Overload resolution is by closure shape, so adding a single await inside a ServiceQueue.background block silently converts it from serialized to unserialized — no diagnostic, no visible change at the call site.

Found while reviewing #648, where replaceHwSnapshot's read → delete → upsert sequence had picked up the async overload this way and was racing markOnchainActivityAsTransfer. Fixed there by keeping the closure synchronous; the general trap is still armed.

Confirmed scope

Not affected — LDK. All 41 ServiceQueue.background(.ldk) call sites pass synchronous closures and are genuinely serialized on ldkQueue. Nothing on the Lightning path depends on the async overload's current behaviour.

Two read-modify-write cases on .core (both pre-existing, neither on a hardware-wallet path, both a ~3-line fix — move refreshBoostTxIdsCache after the await):

  • ActivityService.upsertListBitkit/Services/CoreService.swift:380 (callers: BackupService restore, MigrationsService)
  • ActivityService.delete(id:walletId:)Bitkit/Services/CoreService.swift:1385 (no production caller; only BitkitTests/ActivityListTest.swift)

While these stand, markOnchainActivityAsTransfer's documented "single core-queue transaction so a concurrent watcher sync can't clobber it" is only partly true.

The larger finding. OnChainHwService (8 call sites) and TrezorService (15) both carry the class comment:

All operations run on ServiceQueue.background(.core) to ensure thread safety

Every one of those call sites uses the async overload, so none of them are actually serialized and the documented guarantee does not hold. Unlike the two above, these genuinely await async FFI (Electrum queries, device I/O), so the synchronous overload is not the fix — blocking a queue thread on network I/O would be worse.

Suggested fix

  1. Fix the two .core read-modify-write cases by moving the awaited work outside the queue block.
  2. Give the async path real serialization — an actor per service domain, so await-ing work can queue without blocking a thread — and either adopt it in OnChainHwService/TrezorService or correct their class comments to state what they actually guarantee.
  3. Rename the async overload (e.g. ServiceQueue.backgroundAsync) so the two can never be selected by inference again. This is the part that stops the bug recurring; it touches ~40 call sites across CoreService/LightningService/BackupService, which is why it was kept out of feat: persist hardware wallet activities #648.

References

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions