internal/infrastructure/live-store/redis/tree_signing_session.go (4)

26-33: Per‑round maps + RWMutex look correct, but rely on strict New/Delete discipline

The move to lock + {nonce,sigs}Chs/ctxs maps is sound and removes the previous global‑state coupling; map access is properly guarded with RWMutex.

However, this design assumes:

• Every New(roundId, …) is eventually paired with Delete(roundId); otherwise the maps (and cancel funcs/channels) grow without bound.
• New is not called twice for the same roundId without an intervening Delete; currently a second New overwrites ctxs[roundId] and channels without cancelling/closing the previous ones, which is a leak and can leave stale watchers around.

Consider at least:

• Documenting these invariants at the interface level, and/or
• Adding a defensive check in New that logs (or even rejects) if a session already exists for roundId before overwriting.

Also applies to: 38-41

---

45-49: Lock scope in New is broader than needed and includes Redis I/O

New takes s.lock.Lock() around:

• Redis writes (HSet), and
• Per‑round map setup + watcher goroutine spawning.

Only the map mutations and the hand‑off of per‑round channels/ctxs actually require the mutex; holding the lock across HSet can unnecessarily serialize creation of independent sessions and block Delete/NoncesCollected/SignaturesCollected for other rounds if Redis is slow.

You could narrow the critical section roughly like:

 func (s *treeSigningSessionsStore) New(
   roundId string, uniqueSignersPubKeys map[string]struct{},
 ) *ports.MusigSigningSession {
-  s.lock.Lock()
-  defer s.lock.Unlock()
-
-  ctx := context.Background()
+  ctx := context.Background()
   metaKey := fmt.Sprintf(treeSessMetaKeyFmt, roundId)
   cosignersBytes, _ := json.Marshal(uniqueSignersPubKeys)
   meta := map[string]interface{}{
     "Cosigners":   cosignersBytes,
     "NbCosigners": len(uniqueSignersPubKeys) + 1, // operator included
   }
   s.rdb.HSet(ctx, metaKey, meta)
 
-  watchCtx, cancel := context.WithCancel(context.Background())
-  s.ctxs[roundId] = cancel
-  s.nonceChs[roundId] = make(chan struct{})
-  s.sigsChs[roundId] = make(chan struct{})
-
-  go s.watchNoncesCollected(watchCtx, roundId)
-  go s.watchSigsCollected(watchCtx, roundId)
+  watchCtx, cancel := context.WithCancel(context.Background())
+  s.lock.Lock()
+  s.ctxs[roundId] = cancel
+  s.nonceChs[roundId] = make(chan struct{})
+  s.sigsChs[roundId] = make(chan struct{})
+  s.lock.Unlock()
+
+  go s.watchNoncesCollected(watchCtx, roundId)
+  go s.watchSigsCollected(watchCtx, roundId)

This keeps correctness while avoiding holding the lock during Redis I/O.

Also applies to: 60-66

---

127-149: Delete semantics are safe, but round cleanup depends entirely on callers

The Delete implementation looks race‑safe:

• You take the write lock while cancelling the ctx and closing/removing per‑round channels, so map access is well‑synchronized.
• Closing channels under the lock and accessing them via RLock in watchers avoids concurrent map writes.

Two behavioral points to be aware of:

1. If Delete(roundId) is never called for a completed/aborted round, the per‑round ctx cancel func and channels remain in the maps indefinitely. Over time this is a memory/leak risk on a long‑lived node.

2. If Delete is called before the watcher has signaled, it will:

   • Cancel the watcher via ctx, causing it to return without signaling, and
   • Close the per‑round channel; any goroutine blocked on <-NoncesCollected(roundId)/<-SignaturesCollected(roundId) will then unblock due to close rather than an explicit send.

Both behaviors may be acceptable, but they are observable. I’d recommend:

• Explicitly documenting that Delete both tears down the watcher and “unblocks waiters” via channel close, and
• Ensuring call‑sites always pair New with Delete once the session is done.

---

186-231: Watcher loops busy‑poll Redis and ignore some errors; use pollInterval and improve logging

Both watchNoncesCollected and watchSigsCollected:

• Run a tight for loop with no sleep/backoff; on “not ready yet” they just continue, effectively hammering Redis and burning CPU.
• Treat HGetAll errors the same as “missing meta” (len(meta) == 0) and just spin without logging or backoff.
• Log parse errors and recovered panics without including roundId, which makes multi‑round debugging harder.
• Duplicate almost identical logic between the two functions.

Given pollInterval already exists, you can introduce inexpensive backoff and better error logging with a small change. For example, in watchNoncesCollected:

func (s *treeSigningSessionsStore) watchNoncesCollected(ctx context.Context, roundId string) {
  metaKey := fmt.Sprintf(treeSessMetaKeyFmt, roundId)
  noncesKey := fmt.Sprintf(treeSessNoncesKeyFmt, roundId)
  for {
    select {
    case <-ctx.Done():
      return
    default:
-      meta, err := s.rdb.HGetAll(ctx, metaKey).Result()
-      if err != nil || len(meta) == 0 {
-        continue
-      }
+      meta, err := s.rdb.HGetAll(ctx, metaKey).Result()
+      if err != nil {
+        log.Warnf("watchNoncesCollected: failed to fetch meta for round %s: %v", roundId, err)
+        time.Sleep(s.pollInterval)
+        continue
+      }
+      if len(meta) == 0 {
+        time.Sleep(s.pollInterval)
+        continue
+      }
       nbCosigners := 0
       if _, err := fmt.Sscanf(meta["NbCosigners"], "%d", &nbCosigners); err != nil {
-        log.Warnf("watchNoncesCollected:failed to parse NbCosigners: %v", err)
+        log.Warnf("watchNoncesCollected: failed to parse NbCosigners for round %s: %v", roundId, err)
+        time.Sleep(s.pollInterval)
         continue
       }
       noncesMap, _ := s.rdb.HGetAll(ctx, noncesKey).Result()
       if len(noncesMap) == nbCosigners-1 {
         // unchanged send logic...
         return
       }
+      time.Sleep(s.pollInterval)
    }
  }
}

And mirror the same changes (including roundId in log messages and a time.Sleep(s.pollInterval) on “not ready / error” paths) in watchSigsCollected.

Optionally, you could also factor the common watcher logic into a shared helper that takes the key suffix and log prefix, to avoid duplication.

Also applies to: 233-278

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/infrastructure/live-store/redis/confirmation_sessions.go (1)

191-212: Consider simplifying the signaling logic.

The nested selects and panic recovery add complexity:

• The outer select (line 192) and inner select (line 205) both check ctx.Done(), which is redundant protection against a TOCTOU issue that the current lock pattern doesn't fully prevent anyway
• The panic recovery (lines 197-204) guards against nil channel sends, but the nil check at line 190 already covers this

While this defensive coding isn't harmful, consider simplifying to improve maintainability:

if ch != nil {
    chOnce.Do(func() {
        select {
        case ch <- struct{}{}:
        case <-ctx.Done():
        }
    })
}
return

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

internal/infrastructure/live-store/redis/confirmation_sessions.go (1)

177-219: Watcher loop now uses pollInterval; consider minor context and error‑handling tweaks.

The reworked watchSessionCompletion correctly:

• Uses a time.NewTicker(s.pollInterval) instead of busy-waiting,
• Is cancelable via ctx.Done(),
• Uses RWMutex to safely snapshot sessionCompleteCh,
• Guards the notification with sync.Once and panic recovery.

A few potential improvements:

1. Error handling on Redis reads.
   Both Get calls ignore errors:

   numIntents, _ := s.rdb.Get(ctx, confirmationNumIntentsKey).Int()
   numConfirmed, _ := s.rdb.Get(ctx, confirmationNumConfirmedKey).Int()

   This can mask Redis connectivity or serialization issues and silently stall completion detection. Even a lightweight if err != nil { log.Warnf(...) ; continue } would improve observability.

   # Check for other places where Redis errors are dropped in this package.
   rg -n "_, _ := s\.rdb\." internal/infrastructure/live-store/redis -C2

2. Context choice for Redis Get calls.
   You’re currently using the watcher’s cancelable ctx for the Get operations. A previous review suggested using context.Background() for these reads so they aren’t aborted by watcher cancellation, relying only on the select around the loop for termination. Whether that’s preferable depends on your desired shutdown behavior (fast cancellation vs. completing the last read). Because this mirrors past feedback, treat it as an optional design choice rather than a bug.

   In go-redis/v9, what are the semantics of using a canceled context vs. context.Background()
   for simple GET operations in long-running polling loops? Are there recommended patterns
   for shutdown behavior in such watchers?
   

3. Docs vs implementation for completion signaling.
   The header comment talks about “closing the local sessionCompleteCh channel when the session is complete”, but the implementation sends a single struct{}{} instead of closing. If multiple goroutines may wait on SessionCompleted(), closing is a broadcast whereas a single send only wakes one waiter. Either is valid, but it would be good to align the comment with the actual behavior or vice versa, depending on how call sites are written.

Overall, the loop changes themselves are a solid improvement over the previous busy-waiting implementation.
Based on learnings

internal/infrastructure/live-store/redis/confirmation_sessions.go (2)

39-47: New fields look appropriate; ctx field currently unused outside Reset/New.

lock, cancel, and pollInterval make sense for coordinating the watcher and guarding sessionCompleteCh. The stored ctx field, however, is not read anywhere in this file; if it is not used by other packages via unsafe reflection/type assertions, you could drop it to reduce state surface area.

---

142-163: Race condition is confirmed, but callers have defensive timeout handling; refactoring suggestions remain valid but not critical.

The race condition you identified is real: if Reset() is called while watchSessionCompletion() is in the nested select (lines 208–212), the ctx.Done() case can win before the signal send, causing the completion to go unnoticed.

However, examination of the callers shows defensive handling: both the test (line 315–318) and the application (service.go ~2240) use select statements with timeout fallbacks. If the completion signal is missed, the timeout fires and execution continues—making this a timing inefficiency rather than a correctness failure.

The suggested refactors (delaying cancel(), using generation IDs in Redis) are sound improvements that eliminate the race window, but the current implementation is safe in practice due to these defensive timeouts.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: louisinger
Repo: arkade-os/arkd PR: 686
File: internal/core/application/fraud.go:47-61
Timestamp: 2025-08-28T08:21:01.170Z
Learning: In reactToFraud function in internal/core/application/fraud.go, the goroutine that waits for confirmation and schedules checkpoint sweep should use context.Background() instead of the request context, as this is intentional design to decouple the checkpoint sweep scheduling from the request lifetime.