Skip to content

ActivityService: concurrent cold reads of the boostTxIds cache each run their own full rebuild #650

Description

@jvsena42

Problem

ActivityService.getTxIdsInBoostTxIds is check-then-refresh, with a suspension point between the check and the write:

func getTxIdsInBoostTxIds(walletId: String = WalletScope.default) async -> Set<String> {
    if cachedTxIdsInBoostTxIds[walletId] == nil {   // check
        await refreshBoostTxIdsCache(walletId: walletId)   // suspends; runs a full get(filter: .onchain)
    }
    return cachedTxIdsInBoostTxIds[walletId] ?? []
}

Nothing records that a refresh is already in flight, so every caller that arrives while one is running sees a cold cache and starts its own. Each rebuild is a full get(filter: .onchain, walletId:) scan of that wallet, so N concurrent callers means N identical scans on the core queue.

Reaching it needs a cold cache plus concurrency, which the activity list supplies on both counts. ActivityListViewModel.init wires four separate Combine subscriptions (search text, date range, tags, tab), each spawning an unstructured Task { await updateFilteredActivities() }. Two firing close together — routine on the first render after launch — gives two concurrent passes, and filterOutReplacedSentTransactions calls the getter once per distinct wallet id, so the fan-out multiplies by the number of paired hardware wallets.

Impact

Wasted work only — duplicate reads of the same rows, converging on the same value. Not a correctness or safety problem:

  • Concurrent access to the cache dictionary itself is a separate matter, fixed under feat: persist hardware wallet activities #648 by putting it behind a lock. This issue is about the redundant rebuilds, which the lock does not and should not prevent.
  • It is bounded to a cold cache. Once a wallet is warmed the check short-circuits, so this is a startup cost, not a per-call one.

Filing it as a known rough edge rather than something urgent.

History — pre-existing, and #648 improved it

The pattern is unchanged from master; #648 only added the walletId parameter. Worth recording that the branch made the surrounding behaviour strictly better, so this should not be read as a regression:

master tested cachedTxIdsInBoostTxIds.isEmpty, which cannot distinguish "not loaded yet" from "loaded, and this wallet genuinely has no boosted transactions". For any user with no boosts — most users, most of the time — that meant a full get(filter: .onchain) scan on every call, forever. #648's nil check caches the empty set, so it now refreshes once.

Net direction per updateFilteredActivities, accounting for the caller change (master called the getter once; #648 calls it once per distinct wallet):

scenario master after #648
no boosts, 1 wallet 1 scan every call 1 scan ever
no boosts, 3 hardware wallets 1 scan every call 3 scans on first call, then 0

Suggested fix

Single-flight the refresh: keep a [String: Task<Void, Never>] of in-flight rebuilds keyed by wallet id, so a second caller awaits the running task instead of starting another. AddressSearchCoordinator at the bottom of CoreService.swift is the existing precedent in this file for that shape.

Two things to keep in mind when doing it:

  • The failure path matters. refreshBoostTxIdsCache swallows its error and leaves the key unset, so the entry must be cleared on completion either way or a failed rebuild would be cached as "in flight" forever.
  • updateBoostTxIdsCache only merges into an already-warmed wallet (deliberately — see its comment). A single-flight rewrite needs to preserve that, or a merge landing mid-rebuild could be treated as the whole set.

References

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions