fix(stream): retire the frozen-channel ledger on the union path too - #1308
Merged
Conversation
✅ Deploy Preview for familiarise ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Review limit reachedNext included review available in 19 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Your 53 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Comment |
#1302 review. `source` was doing two jobs: reporting how the unfreeze set was determined, and gating ledger retirement. On the union path it reads "derived" because the set is only best-effort complete — but the set still CONTAINS the ledger entries, so `result.source === "ledger"` skipped `retireFrozenChannels` and `FROZEN_CHANNELS` never shrank. Left alone that is not just untidy. The stale set is re-unfrozen on every later OFF transition, spending the 300/min UpdateChannelPartial budget on it, and — since #1303 landed — reopening DM channels the dormancy sweep froze on purpose. Maintenance would silently undo another subsystem's decision. Split the two: `usedLedger` tracks participation, `source` keeps reporting provenance. `srem` on an id the set never held is a no-op, so passing the derived ids through with the ledger ids costs nothing. Also corrects two `withStreamCircuitBreaker` fixtures that simulated Redis's breaker message. They passed only because the guard matches the substring "circuit breaker is OPEN", so they were asserting against a string this path cannot emit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
teetangh
force-pushed
the
fix/drain-ledger-retire-gate
branch
from
September 1, 2026 08:05
7bd595e to
7bc6c3e
Compare
|
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1302, from its own unresolved review threads. Two findings, one of them a real cross-subsystem bug.
1 · The frozen-channel ledger never shrank on the union path (🟠 Major)
unfreezeChannelsAfterMaintenanceusedresult.sourcefor two different jobs — reporting how the unfreeze set was determined, and gating ledger retirement:On the union path (
ledger.length > 0 && incomplete)sourceis set to"derived", because the set is only best-effort complete. But the returned set still contains every ledger entry. So retirement was skipped andFROZEN_CHANNELSnever shrank.Why this is more than untidy. The stale set is re-unfrozen on every later OFF transition, which:
UpdateChannelPartialbudget on channels that are already unfrozen, andFix. Split the two concerns:
usedLedgertracks participation,sourcekeeps reporting provenance. Both ledger-reading paths set it.sremon an id the set never held is a no-op, so passing the derived ids through alongside the ledger ids costs nothing.2 · Two breaker fixtures asserted a message their path cannot emit (🔵 Trivial)
Since the breaker split,
createCircuitBreaker("stream")throws"stream circuit breaker is OPEN - service unavailable". BothwithStreamCircuitBreakerfixtures still simulated"Redis ...". They passed only because the guard matches the substring"circuit breaker is OPEN"— so they were green against a string this code path can never produce.Verification
Proof-by-revert on the new assertion — restore the
result.source === "ledger"gate and it goes red on exactly the right thing:Restored: 13 passed.
npx tsc --noEmit→ exit 0npx eslinton the three changed files → exit 0npx jest __tests__/stream→ 41 suites, 514 tests, all passThreads deliberately not actioned
result.errors— ledger read, unfreeze, retire — genuinely does make the ledger suspect, which is what the marker means. The condition is already the intended one.loadOrgChatRetentionshould filter to seen orgschatRetentionDays, so narrowing the query would make the window depend on which page loaded — the same incomplete-input-drives-a-destructive-decision shape the truncation guard exists to prevent.deletedDmsinflateddmDeleteRequestsand its docblock says it counts requests, not deletions; the re-send is bounded by the scan window (largest retention + 60 days).2026-09-01Last Updated: 2025-01-22the comment cites is not in the file.🤖 Generated with Claude Code