[aliev]

Why

A call through a customer's own SIP trunk is held by the router node that dialled it, and only that node. So every deploy hangs up every trunk call on the node. A crash is worse: the audio keeps flowing, but when one side hangs up, the BYE has nowhere to go, and the other side stays connected to nothing. Now another node continues the call, and the people keep talking while it moves.

How it works

Each connected call on a shared Stream line has a record in Redis: both dialogs (tags, route set, remote target, CSeq, last SDP), never a password. The node holding it renews its claim every 10 s. The audio never passes through a node, so only the SIP dialogs move.

Deploy

sequenceDiagram
    participant C as Customer trunk
    participant A as Node A (stopping)
    participant R as Redis
    participant B as Node B
    participant S as Stream SIP
    Note over A: drain starts: A stops taking other nodes' calls,<br/>keeps renewing its own
    A->>R: is another node taking calls?
    R-->>A: yes, B
    A->>R: write the record, drop the claim, publish "free"
    R-->>B: "free"
    B->>R: claim it (SET NX)
    Note over A: A sees B holds it and lets go, no BYE
    B->>R: read the record, save CSeq + 1000
    Note over B: trunk and line passwords read from Postgres
    B->>C: OPTIONS, to learn B's public address
    B->>C: re-INVITE, same dialog, Contact B, stored SDP
    C-->>B: 200, then ACK
    B->>S: re-INVITE, same dialog, Contact B
    S-->>B: 401
    B->>S: re-INVITE with credentials
    S-->>B: 200, then ACK
    B->>R: record and claim as B's
    C->>B: BYE, later
    B->>S: BYE

Loading

A lets go as soon as B wins the claim, before B's re-INVITEs: once a peer has the re-INVITE at CSeq + 1000, a keep-alive from A with the old CSeq would be refused and would end the call.

Crash

sequenceDiagram
    participant A as Node A
    participant R as Redis
    participant B as Node B
    A->>R: renew claim, every 10 s
    Note over A: crashes
    Note over R: A's claim lapses within 30 s
    loop every 10 s
        B->>R: calls whose claim lapsed?
    end
    R-->>B: this call
    B->>R: claim it (SET NX)
    Note over B: same takeover as on a deploy

Loading

A node takes calls one at a time, so when many move at once the last ones wait longer. If Redis loses a call's keys, the node holding it claims the call back while its own claim still runs.

What still gets a BYE

flowchart TD
    S[node stops] --> T{call on a shared<br/>Stream line, with Redis?}
    T -- "no: own Stream trunk,<br/>transfer leg, no Redis" --> BYE[BYE to both sides, as before]
    T -- yes --> N{another node<br/>taking calls?}
    N -- "no: lone node,<br/>rollback to an older version" --> BYE
    N -- yes --> H[hand over]
    H --> K{taken?}
    K -- yes --> G[let go, no BYE]
    K -- "no, and no node left" --> BYE
    K -- "no taker by the deadline" --> L[let go, left for the next scan]

Loading

Testing

• Live, on a throwaway build: a second process continued a real call through a Twilio trunk and Stream. Both answered the re-INVITE with 200 on the new connection (Stream after a 401), then sent keep-alives and BYE there, and the audio kept flowing.
• TakeoverSuite (Redis, now in CI) covers deploy and crash takeovers, two nodes racing for one call, lost keys, no other node, nodes stopping together, and draining. sipbridge loopback tests cover the rebuilt dialogs.

[gitguardian]

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

---

^{_{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}}

[kanat]

Thanks for this one, Ali. It's a careful piece of SIP work, and the tests already pin most of it. This is round 1 from me: one review, findings by id.

Reviewed da33d313 merged onto accelerate at bf1ee9ce, in a scratch worktree. The merge is clean locally. GitHub shows CONFLICTING only because of CHANGELOG.md merge=union, which GitHub ignores, so that is a note, not a finding.

Checks run: build, vet and gofmt are clean. ./internal/phone/... passes with -race. sipbridge and siptrunk pass 50 runs with -race and 50 with -cpu 1. TakeoverSuite (against Redis) passes 10 runs with -race. The phone integration suite and the store trunk tests pass. 41 mutations: 26 killed, 3 equivalent, 12 survived (in R1.3, R1.4 and R1.5).

[Should fix] R1.6 CI never runs TakeoverSuite. .github/workflows/integration.yml runs ./internal/phone but not ./internal/phone/siptrunk, and it sets no ROUTER_REDIS_ADDR, so the suite would skip anyway. The handoff, the crash takeover and the NX race, the core of this PR, are covered only on a laptop. Add internal/phone/siptrunk to the matrix and pass ROUTER_REDIS_ADDR: localhost:${{ job.services.redis.ports['6379'] }}, or have the suite read the Redis address from testing.yaml as the other suites do.

Checked and sound:

• Nothing changes when the feature is off. Without Redis, adopt claims nothing, Start returns at once, and hold hangs up on stop as before (TestWithoutRedisCloseStillHangsUp). Other vendors are untouched: the service.go change sits inside if trunk != nil. A call on a per-call Stream trunk carries no Resume, which a test pins.
• What Redis holds. The record carries no password. It does carry the outcome token, but /v1/phone/status/{token} takes posts only for a StatusReader vendor, and sip_trunk is not one.
• Tenant isolation. ResumeTrunkCall reads both trunks scoped to the customer.
• CSeq. The +1000 jump, the auth retry (+1) and recording the CSeqs before the re-INVITE are all pinned by tests.
• BYE handling. A resumed leg's BYE and readBye bypass sipgo's INVITE transaction correctly; with sipgo's versions, the tests panic.
• Shutdown order. closePhone runs before the deferred Redis close.

Verdict: request changes, for R1.1 (Redis key loss releases live calls), R1.2 (rollback leaves calls with no relay) and the test gaps in R1.3 to R1.6. R1.7 to R1.13 are nits and R1.14 is a question; none of them blocks the merge.

[kanat]

[Should fix] R1.1 kept == 0 means two different things here: another node holds the claim, or the claim (and maybe the record) is simply gone from Redis. Both release the call. When Redis loses the keys (a restart or failover that drops recent writes, eviction, or an outage longer than holdFor so the owner key expires), every node releases every trunk call it relays. If the record went too, no node can ever take the call, so the BYE has nowhere to go: the crash case this PR fixes, now caused by Redis alone. On the base, Redis state had no effect on trunk calls.

Checked with a probe in TakeoverSuite that fails on this head: dial on a, then DEL call:<id>, DEL owner:<id> and ZREM calls <id>, then s.Never(a.dialer.call.wasReleased, 5*testRenewEvery, ...). Head: "a let go a call no other node holds". The same happens with only owner:<id> deleted; there a's own scan takes the call back, at the cost of a pair of re-INVITEs.

Fix: let renewScript tell "held by another node" (release) apart from "no owner" (claim again with claimScript and the current State(), which writes the record back). Keep the two probes above as tests.

[aliev]

Fixed in 83ac6a6 and 047386a. renewScript now tells the cases apart: held by another node → release; no owner and score 0 (given up by a handover) → leave it; no owner otherwise → claim it again and write the record back, but only while this node's own lease still runs. Past its own lease a node can't tell "Redis lost the keys" from "another node took the call and it ended there", so it releases as before. Inside its lease no scan can have taken the call, so claiming it back is safe. Your two probes are tests (TestARedisThatLostACallsKeysDoesNotEndIt, TestANodeWhoseClaimWentMissingKeepsTheCall), plus one for a node cut off past its lease.

[kanat]

R1.1: fixed in 83ac6a6 and 047386a. Removing the reclaim, the lease check or the handingOver skip fails TestARedisThatLostACallsKeysDoesNotEndIt, TestANodeCutOffPastItsLeaseDoesNotBringBackACallThatEndedElsewhere and TestRenewDoesNotClaimBackACallBeingHandedOverOnceItsScoreMoved.

[kanat]

[Should fix] R1.2 Rollback safety. With no node to take the call, Close releases it without a BYE and leaves the record for "the next to scan" (TestCloseWithNoOtherNodeLeavesTheCallForLater asserts this). The previous release has no scan, so on a rollback (new to old) the last new-build node to stop leaves every trunk call it held with no relay: when one side hangs up, the other side stays connected to nothing. Today's build hangs those calls up cleanly. A deployment with a single replica and no surge is in the same spot until the new pod starts and scans.

Fix: hand a call over only when a node that can take it exists, and otherwise hang up as before. For example, check PUBSUB NUMSUB on the free channel, or a short-lived presence key that each node with a Resumer renews. Then a rollback or a lone node gets today's BYE. Test row: one node with no other node listening; Close → wasHungUp() is true and the call is reported.

[aliev]

Fixed in 7d7ff39, with follow-ups in 354ba13, 835388b and c90c1ed. Each node that can take calls keeps an entry in {siptrunk}:nodes (scored by when it lapses, renewed every scan tick). Close removes its own entry, then hands calls over only if another live entry exists. Otherwise it hangs them up with a BYE and reports them, as the old build does. So a rollback or a lone node gets today's behaviour. Nodes stopping together are covered too: a closing node whose call is still unowned, still given up, with no node listed, wins it back atomically and hangs it up. A node stopping past its own lease checks the owner key: if the owner is itself it hangs up; otherwise it lets go and Close returns an error so the drop is logged. Your test row is TestCloseWithNoOtherNodeHangsTheCallUp. Known limit: a node that crashed stays listed for up to holdFor, so a node closing in that window hands over to nobody and the call waits for the next scan.

[kanat]

R1.2: fixed in 7d7ff39, 354ba13, 835388b and c90c1ed. Making withdraw always say a node is left, or always handing over, fails the suite; TestCloseWithNoOtherNodeHangsTheCallUp pins the lone-node BYE.

[kanat]

[Should fix] R1.3 Four rules in this file survive mutation: the suite stays green with each one removed. Each test row below passes on this head (10 runs, -race) and fails under its mutant. All of them belong in TakeoverSuite:

• claimScript owner check (if owner and owner ~= ARGV[1] removed): b's resume hook sets owner:<id> to "z" (another node won after a lapse). Expect b.takeover.call.wasReleased(), not wasHungUp(), and owner == "z". Under the mutant, b overwrites z's claim, and two nodes relay.
• writeScript owner check (line 84): s.holdFor = time.Minute. b's resume hook calls a's OnChange with callState("stale"), waits 200 ms, then reads the record. Expect LastSDP == "v=0 a". Under the mutant, a overwrites the record b took over.
• renewScript's ZADD (line 112): dial on a, sleep 2*testHoldFor. Expect lapse(id) to be after now. Under the mutant, every live call looks lapsed to every scan.
• forget on the written == -1 path (line 354): TestAByeDuringTheHandoffEndsTheCallOnce passes only because b's 600 ms claim expires inside its 1 s Eventually. With s.holdFor = time.Minute, gone(id) catches the mutant.

[aliev]

Fixed in 7bb449b. All four rows are in TakeoverSuite, and each was run red under its mutant and green after reverting. Row 2 differs a little from your sketch: b's own claim rewrites the record from b's fake state, so the test presets b's state, stops a's renewal so a keeps relaying, and fires a's stale OnChange only once b owns the call.

[kanat]

R1.3: fixed in 7bb449b. All four mutants (claim owner check, write owner check, renew ZADD, forget on -1) now fail a test.

[kanat]

[Should fix] R1.4 Six rules in sipbridge survive mutation, because every fixture has a single route, no NAT, and no re-INVITE from a peer. Each test row below passes on this head (20 runs, -race) and fails under its mutant:

• Route order (this line, slices.Backward → slices.All): give RouteSet two entries, the stream peer first and then <sip:192.0.2.9;lr>. Expect the re-INVITE's Route headers in that order, and call.State().Stream.RouteSet equal to the input.
• natContact in learnContact (line 228): a customer peer whose OPTIONS answer adds received=192.0.2.44;rport=40404 to the Via. Expect the re-INVITE's Contact to be 192.0.2.44:40404.
• call.media = state.Media (line 108): set state.Media. Expect call.Media() and call.State().Media to equal it.
• call.onChange = cfg.OnChange here (line 107), and the second call.changed() in handle (dial.go:200): an OnChange that snapshots call.State(). The customer peer sends a re-INVITE with SDP. Expect the last snapshot's Customer.LastSDP to be the answer the peer got, and Stream.LastSDP to be the offer. Without the second changed(), the only snapshot is taken before the customer leg stored its answer.
• call.onChange = cfg.OnChange in Dial (dial.go:48): Dial against the two loopback peers (with real SDP), then a re-INVITE from the customer peer. Expect OnChange to be called. Without it, no change made in production ever rewrites the record.

[aliev]

Fixed in 3e32290. All six mutants (route order, natContact, call.media, onChange in Resume, the second changed() in handle, onChange in Dial) now fail a test. The new tests are in the sipbridge suite, with small extensions to the loopback peer. Under the route-order mutant, the OPTIONS goes to 192.0.2.9, so the test fails on a 3 s timeout rather than on the header assertion. It still fails.

[kanat]

R1.4: fixed in 3e32290. All six mutants fail a test in the resume and dial suites.

[kanat]

[Should fix] R1.5 The wiring is untested. Swap context.WithoutCancel(ctx) for ctx, or drop the SetResumer line above, and every test still passes: nothing in cmd/router/*_test.go calls buildPhone. The first mutant stops renewal at the signal. During a drain the claims would then lapse while this node still relays its calls, and another node would take them over, so two nodes relay one call. The second mutant turns takeover off in production. Test row (not checked): pull the provider wiring out of buildPhone so a test can cancel the run context and still see the claim's lapse in calls move forward, and see a lapsed record taken.

[Nit] R1.11 (same line) A draining node keeps its scan loop and can take over other nodes' calls during the drain, only to hand them on again at closePhone. Keep renewal through the drain, but stop scanning when the drain starts.

[aliev]

R1.5, fixed in 81a0349 and 3742c4a. Start(r Resumer) replaces SetResumer + Start(ctx) and takes no context, so a signal context can no longer end renewal: that mutant can't be written any more. RouterStartSuite.TestARunningRouterSaysItTakesTrunkCallsOverUntilItStops checks the router shows in {siptrunk}:nodes while it runs, which fails if main drops Start. R1.11, fixed in 81a0349: StopTaking() runs when a drain starts. It stops scanning and listening and removes the node from the nodes set, and keeps renewing. DrainSuite.TestADrainingRouterStopsSayingItTakesTrunkCallsOver fails if run() stops passing it.

[kanat]

R1.5 and R1.11: fixed in 81a0349 and 3742c4a. Dropping Start, Start(nil), the stopTaking field in run, or the call in stopServing each fails a test.

[kanat]

[Nit] R1.9 A record whose takeover keeps failing with an error other than ErrNoSIPTrunk/ErrNoStreamTrunk (a sealed password that no longer opens, for example) keeps its old score. Every node retries it every holdFor, for up to recordFor (5 h). The scan takes the 100 lowest scores, so 100 such records starve every newly lapsed call behind them. Fix: on a failed take, raise the score (backoff) or count the attempts.

[aliev]

Fixed in 7d7ff39. A take that fails before any re-INVITE gives its claim back (owner-checked) and moves the score to now + renewEvery, without publishing. Newly lapsed calls sort ahead of it, so a broken record can no longer starve them. It still retries until the record expires, one Warn each time. I left that alone: a credential error may clear, and the record expiring bounds it.

[kanat]

R1.9: fixed in 7d7ff39. Not calling giveBack, or giving back with no delay, fails TestATakeThatFailsBeforeResumingGivesTheCallBackForLater. The XX in giveBackScript is not pinned (R2.2, nit).

[kanat]

[Nit] R1.10 scan takes calls one after another, and each take can run up to holdFor-renewEvery (20 s). Calls from a crashed node therefore move at the pace of one takeover at a time on each scanning node. The "within about 40 s" in the PR body and in CHANGELOG holds only for the first few calls. Either qualify the claim, or take calls with a small bounded worker pool.

[aliev]

Pushed back on the worker pool; the wording is fixed in 61f9b5e and 1738f99. A take is normally two Redis round trips plus two re-INVITEs, well under a second. The 20 s is only the bound for a peer that doesn't answer. So a pool buys little for most crashes, and it would add concurrency to the code that most needs to stay simple. CHANGELOG now says a node takes calls one at a time on a deploy and a crash alike, so when many calls move the last ones wait longer. If a node with hundreds of trunk calls turns up, a small pool is an easy follow-up.

[kanat]

R1.10: closed. I accept the reason for no pool; the wording in 61f9b5e and 1738f99 says the claim as it is.

[kanat]

[Nit] R1.12 The record is rewritten only when the SDP changes. In-dialog keepalive OPTIONS (every 20 s, so 720 in a 4 h call) and every INFO passed on raise the CSeq without being recorded. A jump of 1000 covers the keepalives, but not a long call with many INFOs. Fix: rewrite the record from renew when either leg's CSeq has moved, or size the jump from the call's age.

[aliev]

Fixed in 83ac6a6, together with R1.1: renew now writes the call's current record every tick (owner-checked), so a CSeq moved by keepalives or INFOs is at most renewEvery old. It costs one record write per call every 10 s. TestACSeqThatMovedWithoutAnSDPChangeIsRecorded covers it.

[kanat]

R1.12: fixed in 83ac6a6. Dropping the record write from renewScript fails TestACSeqThatMovedWithoutAnSDPChangeIsRecorded.

[kanat]

[Nit] R1.13 GitGuardian 40040754 flags this fixture password (commit 7189d7f). It is a test value next to an example.com host, not a real secret. Mark the incident as a false positive in GitGuardian so the check goes green, or use a value its detector skips.

[aliev]

Fixed in e60f8a4 and e185672. Fixture passwords in this suite and in the branch's other new tests are now built at runtime from repeated characters, so no literal looks like a credential. The old commit 7189d7f is still in the PR's history, so the GitGuardian incident may need marking as a false positive.

[kanat]

R1.13: fixed; the GitGuardian check is green on c90c1ed.

[kanat]

[Question] R1.14 The old node releases as soon as owner != p.node, which is right after the taker's SET NX and before its re-INVITEs go out. If the taker then fails before resuming (for example the transient ResumeTrunkCall error at line 315: "the claim lapses unrenewed"), the call has no relay for up to holdFor, although the old node was still up and could have kept it. Would it be safer to wait until the taker has adopted the call (its claimScript rewrites the record), or until a resumed marker appears? You know the timing on a live trunk best.

[aliev]

Answered here; no change to when the old node lets go. Waiting until the taker has adopted the call is less safe. The taker re-INVITEs at CSeq + 1000. If the old node is still relaying, its next keepalive OPTIONS carries the old CSeq, the peer refuses it, and a failed keepalive ends the call with a BYE to the other side (keepalive → call.lost in dial.go). So the old node must stop relaying before the re-INVITEs go out, which is what releasing on the NX does. The gap you describe is shorter now: a take that fails before any re-INVITE gives its claim back at once (R1.9), so the next scan retries it within about renewEvery instead of after holdFor.

[kanat]

R1.14: answered, thanks. A taker's re-INVITE is at CSeq+1000, so an old node still relaying would fail its next keepalive and BYE the call; releasing on the NX is right.

[aliev]

R1.6 (in the review body): fixed in 2bdf945. internal/phone/siptrunk is now in the integration matrix. The suite needs no extra env: it imports internal/testenv, which exports ROUTER_REDIS_ADDR from testing.yaml, and the workflow already rewrites that file's Redis port, so the skip in SetupSuite can't fire in CI.

[aliev]

Tested end to end on a real trunk call

Two router nodes built from c90c1ed4, sharing one Redis and Postgres. Each run placed a real outbound call through a customer's Twilio Elastic SIP trunk to a mobile phone, which answered. The call was set up on node A.

Run	What happened to node A	Node B took the call	After the takeover
Deploy	SIGTERM	0.46 s after node A gave it up	The phone hung up 27 s later. The BYE reached node B, which ended the Stream side. Outcome completed.
Crash	kill -9	29 s later, when node A's claim lapsed	The call stayed up 34 s more, past a keep-alive from node B. The phone hung up, and the BYE reached node B. Outcome completed.

In the deploy run node A sent no BYE and exited by itself once node B held the call. After each call, Redis held no record of it.

Not covered here: a takeover from a different public IP (both nodes ran on one machine).

[kanat]

Round 2: NO-GO

Thanks Ali, this is a big step: every round-1 Should fix is closed with a test that fails under its mutant, and the rollback case now behaves like today's build. One small gap is left (R2.1), three lines of test.

Reviewed c90c1ed4 merged onto accelerate at 03994a6f (your base merge is already in; git show --remerge-diff of it is empty, no conflict edits).

Round 1	Status	Proof
R1.1 Redis loses keys	fixed	83ac6a647, 047386a57. Removing the reclaim, the lease check (both directions) or the handingOver skip fails TestARedisThatLostACallsKeysDoesNotEndIt, TestANodeCutOffPastItsLeaseDoesNotBringBackACallThatEndedElsewhere, TestRenewDoesNotClaimBackACallBeingHandedOverOnceItsScoreMoved
R1.2 rollback / lone node	fixed	7d7ff399d, 354ba13cf, 835388b88, c90c1ed40. TestCloseWithNoOtherNodeHangsTheCallUp; making withdraw always say "someone is left", or always handing over, fails the suite. Known limit (a crashed node stays listed up to holdFor) is fair and documented
R1.3 four unpinned script rules	fixed	7bb449bdc. claimScript owner check, writeScript owner check, renewScript ZADD, forget on -1: all four mutants fail a test
R1.4 six unpinned sipbridge rules	fixed	3e322903f. Route order, natContact, call.media, both onChange sites and the second changed(): all six fail a test
R1.5 wiring	fixed	81a03494e, 3742c4a3c. Dropping Start, Start(nil), dropping stopTaking in run, dropping the call in stopServing: each fails a test. A signal context can no longer end renewal, as you say
R1.6 CI never ran the suite	fixed	2bdf945e4; integration (internal/phone/siptrunk) ran 4m12s and is green
R1.7 BridgeURI	fixed	fcf958f64, no reader left
R1.8 post-dial delay	fixed	7d7ff399d; dropping it fails TestATakenOverCallReportsItsPostDialDelay
R1.9 retry starvation	fixed	7d7ff399d; no giveBack call, or no delay, fails TestATakeThatFailsBeforeResumingGivesTheCallBackForLater
R1.10 one take at a time	closed	pool declined with a reason I accept; the changelog now says it
R1.11 draining node scans	fixed	81a03494e; see R1.5
R1.12 CSeq without SDP change	fixed	83ac6a647; dropping the record write in renewScript fails TestACSeqThatMovedWithoutAnSDPChangeIsRecorded
R1.13 GitGuardian	fixed	check is green
R1.14 question	answered	Your point holds: the taker's re-INVITE is at CSeq+1000, so an old node that keeps relaying would fail its next keepalive and BYE the call. Releasing on the NX is right

New findings (inline, same ids): R2.1 Should fix, R2.2 Nit, R2.3 Nit.

Checks run: build, vet, gofmt (only a file this PR does not touch is unformatted) clean. ./internal/phone/... passes with -race. sipbridge and siptrunk unit: 20 runs with -race and 20 with -cpu 1. TakeoverSuite against Redis: 3 runs with -race and 3 with -cpu 1. internal/phone and the store trunk tests (integration) pass. cmd/router: the new start and drain tests and the closephone tests pass (without -race, as CI runs them; the start suite reports a data race around the default HTTP transport under -race, on the base too, so it is not from this PR). gh pr checks: all green.

Mutations: 46 checked this round (including the 12 round-1 survivors); 10 survived: 3 are R2.1, 5 are R2.2, 2 are equivalent (renewScript == '0' branch, unreachable behind the handingOver skip; the withdraw count-parse error path).

Verdict: NO-GO until R2.1 has its three test rows. The Nits do not block.

[kanat]

[Should fix] R2.1 Three rules of the new node set survive mutation: the suite stays green with each removed. Each row below passes on this head (5 runs, -race) and fails under its mutant; all belong in TakeoverSuite:

• Line 357, p.advertise(ctx) in scanLoop removed: the node says it takes calls only once, at Start, and after holdFor (30 s) it drops out of {siptrunk}:nodes. Every later deploy then hangs calls up instead of handing them over, for as long as that node lives. Row: s.node("a"), time.Sleep(3 * testHoldFor), then advertised("a") is true and after now.
• Line 377, the ZREMRANGEBYSCORE in advertise removed: the words of dead nodes pile up (each process is a new node). Row: ZADD <prefix>:nodes <now-1m> gone before s.node("a"), then Eventually advertised("gone") is false.
• Line 402, withdraw returning true when its ZREM fails: the comment promises "false when Redis cannot tell, so the calls are hung up". Row: dial on a, a.stop() and a.provider.loops.Wait(), swap a.provider.redis for a client already closed, then Close → a.dialer.call.wasHungUp() is true. Under the mutant handOver fails too and the call is released with no BYE.

[kanat]

[Nit] R2.2 Five more guards are not pinned, all small and defensive. For each, either add a row or drop the guard:

• Line 177, ZSCORE ~= '0' in reclaimScript: removing it changes no test. It only matters for a call whose score moved (a failed take, or a call forgotten meanwhile).
• Line 368, the p.ctx.Err() != nil || ctx.Err() != nil check in advertise: it guards a race between a scan tick and Close, so a row needs a hook; fine to leave with a comment saying so.
• Line 161, XX in giveBackScript: without it a call forgotten meanwhile comes back into calls (it heals at the next scan).
• Lines 656-667, the ctx.Done() branch of handOver: the last reclaim and the notTaken mark both survive; the polling branch covers all but the last 100 ms.

[kanat]

[Nit] R2.3 withdraw is the first thing Close does, and it waits up to redisTimeout (5 s). phoneHangupGrace is also 5 s (main.go), and ctx here is the grace. So if Redis stalls instead of refusing the connection at shutdown, withdraw uses the whole grace, and the BYEs that follow have none left, so the node exits with the calls up. The base never touched Redis at stop. I read this from the code, I did not reproduce a stall. Fix: bound this call by a short time (a second, say), so a Redis that does not answer still leaves time for the BYEs.