The periodic cleanup in SubscribersMap.cleanUp deleted subscribers whose
elapsed time since last activity was *less* than the idle timeout, and kept
those that had gone idle. This is backwards: active subscribers (that keep
their subscription alive via pings, which call Refresh) were being dropped
within a cleanup cycle, while genuinely idle subscribers were never reaped
and leaked forever.
Flip the comparison so a subscriber is removed only once it has been idle
for longer than the timeout.
Also rewrite TestCleanup (which previously encoded the buggy behavior) and
add TestCleanupKeepsActiveSubscribers, which fails on the old inverted check.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sustained subscribe failures saturated CPU, leaked 600+ subscriptionLoop
goroutines, and twice panicked with `strings: Join output length overflow`.
Five independent issues:
- api/filter: errcnt budget was gated on `possibleRecursiveError`, which
matched only `ErrNoPeersAvailable` / `swarm.ErrDialBackoff`. The dominant
error class never incremented errcnt, so the 3-error-per-5s budget was
dead code. Replaced gate with `shouldIncrementErrCnt(err)`: counts every
non-nil error.
- protocol/filter: WakuFilterLightNode.Subscribe flattened per-peer errors
via `fmt.Errorf+strings.Join`, losing typed *FilterError and growing
unboundedly. Replaced with typed `*SubscribeError` (PeerID, ContentTopics,
Err) plus `HasRateLimitError()`; `Error()` is hard-capped. Concurrent
per-peer appends now mutex-guarded.
- api/filter: 60-s rate-limit backoff on `*SubscribeError.HasRateLimitError()`.
`shouldHonourRateLimitBackoff(rateLimitedUntil, now)` gates ticker push and
closing-channel checkAndResubscribe. Cleared on subscribe success.
- api/filter: FilterManager.waitingToSubQueue was a cap-100 chan written and
drained under the same lock, deadlocking the manager once full. Replaced
with mutex-guarded slice.
- api/filter: Sub.cleanup closed DataCh while multiplex forwarders could
still be sending. Added multiplexWG awaited in cleanup; forwarder send is
in a select with apiSub.ctx.Done() so it can't deadlock when
subDetails.C is never closed (node-stop transitions).
Tests (all under -race):
- TestSub_CleanupRaceWithMultiplex (50 iter)
- TestSub_CleanupDoesNotDeadlockWhenSubChannelStaysOpen
- TestFilterManager_SubscribeFilter_DoesNotDeadlockWhenQueueFull
- TestShouldIncrementErrCnt
