Skip to content

Fix Penalties popup data + concurrent Nostr relay callback corruption#909

Open
dangershony wants to merge 2 commits into
mainfrom
fix/penalties-popup-and-relay-race
Open

Fix Penalties popup data + concurrent Nostr relay callback corruption#909
dangershony wants to merge 2 commits into
mainfrom
fix/penalties-popup-and-relay-race

Conversation

@dangershony

Copy link
Copy Markdown
Member

Summary

Two related fixes from the same investigation, kept as separate commits:

1. Penalties popup showed stale/incorrect data

The Funded > Penalties button opened a modal that filtered the already-loaded Investments list by a per-investment \RecoveryState\ flag only populated lazily when a user opens that specific investment's detail view (Manage). If you never drilled into a project with a penalty, it silently never showed up in the popup. Separately, once wired to the correct SDK operation, the displayed 'days remaining' was always a hard-coded 365-day placeholder due to an unrelated SDK bug.

  • \PortfolioViewModel: added \LoadPenaltiesAsync(), calling the SDK's wallet-wide \GetPenalties\ operation (previously only used by the CLI) into a new \Penalties\ collection, replacing \PenaltyInvestments/\HasPenaltyInvestments.
  • \PortfolioView: Penalties button now triggers a fresh scan instead of reading stale in-memory state.
  • \PenaltiesModal: bound to the new collection, added a loading state, now shows the resolved project name alongside the project ID.
  • \GetPenalties\ (SDK): fixed \LookupInvestment.Project\ never being populated (causing the 365-day placeholder), and excluded penalties that were already fully released from the results.
  • Updated the two \App.Test.Integration\ tests that asserted on the removed properties.

2. Concurrent Nostr relay callbacks corrupting consumer state

\RelayService\ connects to multiple relays simultaneously and merges their events into a single shared Rx subject per client, with nothing serializing delivery to subscribers. When two relays responded around the same instant, a subscriber's callback could be invoked concurrently from two different relay receive threads.

This surfaced while running the UAT suite: concurrent writes to a plain \Dictionary/\HashSet/\List\ in \DocumentProjectService\ threw 'Operations that change non-concurrent collections must have exclusive access', corrupting the collection and causing 'No projects found in Nostr relays' for the rest of that process's life — blocking project discovery for that investor.

Rather than patch every consumer that mutates shared state inside a relay callback (a scan turned up several more instances of the same bug shape, including in the legacy webapp), this fixes it once at the source: every \RelayService\ \EventStream\ subscription and every \RelaySubscriptionsHandling\ \OkStream/\EoseStream\ subscription is now wrapped with Rx's \Synchronize()\ operator, guaranteeing non-overlapping delivery to each subscriber's callback regardless of how many relays respond at once. \DocumentProjectService\ itself needed no changes since the guarantee now holds upstream.

Testing

  • \dotnet build\ on \Angor.Sdk, \Angor.Shared, \Angor.Client\ (webapp), \App\ (Avalonia class library), and \App.Test.Integration\ — all green.
  • Ran the UAT suite (\CreateProjectTest, \MultiFundClaimAndRecoverTest, \MultiInvestClaimAndRecoverTest): \CreateProjectTest\ passed; the other two hit the relay race (now fixed) on the first run, and on retry were blocked by the shared signet faucet returning 503/no UTXOs available — external infrastructure, unrelated to this change.
  • Manually verified the Penalties popup fix against a real signet penalty.

The Penalties button opened a modal that filtered the already-loaded
Investments list by a per-investment RecoveryState flag that is only
populated lazily when a user opens that specific investment's detail
view (Manage). Investments the user never drilled into always showed
as having no penalty, even when one genuinely existed on-chain.

- PortfolioViewModel: add LoadPenaltiesAsync(), which calls the SDK's
  wallet-wide GetPenalties operation (previously wired up for the CLI
  only) and populates a new Penalties collection, replacing the old
  PenaltyInvestments/HasPenaltyInvestments properties.
- PortfolioView: trigger LoadPenaltiesAsync() when the Penalties button
  is clicked so the popup always reflects a fresh scan.
- PenaltiesModal: bind to the new Penalties collection, add a loading
  state, and show the resolved project name alongside the project ID.
- GetPenalties (SDK): fix two bugs found while wiring this up -
  LookupInvestment.Project was never populated, so penalty day counts
  always fell back to a hard-coded 365-day placeholder; and penalties
  that had already been fully released were never excluded from the
  results.
- Update the two App.Test.Integration tests that asserted on the old
  PenaltyInvestments/HasPenaltyInvestments properties to match.
RelayService connects to multiple relays simultaneously and merges
their events into a single shared Rx subject per client. Nothing
serialized delivery of that subject to subscribers, so when two
relays responded around the same instant, a subscriber's callback
could be invoked concurrently from two different relay receive
threads.

This surfaced while running the UAT suite: two relays racing while
adding to a plain Dictionary/HashSet/List in DocumentProjectService
threw 'Operations that change non-concurrent collections must have
exclusive access', corrupting the collection and causing
'No projects found in Nostr relays' for the rest of the process's
life - blocking project discovery for that investor.

Rather than patch every consumer that mutates shared state in a relay
callback (a scan found several more instances of the same bug shape,
including in the legacy webapp), fix it once at the source: wrap each
of RelayService's EventStream subscriptions, and
RelaySubscriptionsHandling's OkStream/EoseStream subscriptions, with
Rx's Synchronize() operator so every subscriber's callback is
guaranteed non-overlapping delivery, regardless of how many relays
respond at once.
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.

1 participant