bgpd: migrate timers during peer_xfer_conn to fix stale route cleanup

[shashanka-ks]

When a BGP session goes down on a GR-helper, bgp_stop() arms the stalepath timer (t_gr_stale) and restart timer (t_gr_restart) on the config peer's connection. If the restarting peer reconnects and a connection collision occurs (common in large-scale topologies like Fairwater with 256 sessions per leaf), peer_xfer_conn() swaps the connection pointers: the doppelganger's winning connection (keeper) is given to the config peer, and the config peer's old connection (going_away) is handed to the doppelganger for deletion.

The GR timers armed on going_away were not migrated during this swap. When the doppelganger was subsequently deleted, going_away was freed and the timers were silently lost. Without the stalepath timer safety net, stale routes were never removed even after the configured stalepath-time had long elapsed.

Fix:
Cancel the GR timers on going_away after the swap and re-arm them on keeper with the remaining time. A simple pointer copy is insufficient because EVENT_ARG (the callback context) must reference keeper, not the going_away connection that will be freed.

Custom debug logs:

Before fix - timer lost during xfer, stale routes never removed

bgp_stop: start stalepath timer 120 sec
peer_xfer_conn: going_away t_gr_stale=armed rem_sec=98
peer_xfer_conn: after xfer config_conn t_gr_stale=NULL    <-- LOST
bgp_establish: t_gr_stale=NULL                            <-- never fires

After fix - timer migrated, stale routes removed on expiry

bgp_stop: start stalepath timer 120 sec
peer_xfer_conn: going_away t_gr_stale=armed rem_sec=98
peer_xfer_conn: migrated stalepath timer (98 sec remain)  <-- MIGRATED
peer_xfer_conn: after xfer config_conn t_gr_stale=armed rem_sec=97
bgp_establish: t_gr_stale=armed gr_stale_rem_sec=97
stalepath timer expired                                   <-- FIRES
bgp_clear_stale_route: entered (stale routes removed)

[greptile-apps]

Greptile Summary

This PR fixes a stale-route cleanup bug in BGP Graceful Restart helper mode: when a connection collision occurs during peer reconnection, peer_xfer_conn() swaps the connection pointers but the GR timers (t_gr_stale, t_gr_restart) armed on going_away were silently lost with the doppelganger, preventing stale route removal. The fix cancels those timers on going_away and re-arms them on keeper with the remaining time, correctly passing keeper as the event arg to match what the timer callbacks (bgp_graceful_stale_timer_expire/bgp_graceful_restart_timer_expire) expect via EVENT_ARG.

Confidence Score: 5/5

Safe to merge — the fix is logically correct, well-scoped, and addresses a real stale-route cleanup regression in GR-helper mode.

No P0 or P1 issues found. The timer migration is placed correctly (after the connection-pointer swap so keeper->peer already points to the config peer), the forward declarations are necessary and correct, and the EVENT_ARG passed to each re-armed timer matches what both callbacks cast it to (struct peer_connection *). All remaining observations are P2 or below.

No files require special attention.

Important Files Changed

Filename	Overview
bgpd/bgp_fsm.c	Adds GR timer migration in peer_xfer_conn() — cancels t_gr_stale/t_gr_restart on the old connection (going_away) and re-arms them on keeper with the remaining time; forward declarations added correctly; EVENT_ARG matches what both callbacks expect.

Sequence Diagram

sequenceDiagram
    participant BP as BGP Session
    participant CFG as Config Peer (peer)
    participant DOP as Doppelganger (from_peer)
    participant KP as keeper (doppelganger conn)
    participant GA as going_away (old config conn)

    BP->>CFG: Session drops
    CFG->>GA: bgp_stop() arms t_gr_stale + t_gr_restart on going_away

    Note over DOP: Peer reconnects → collision

    DOP->>KP: New connection (doppelganger wins)
    Note over CFG,GA: peer_xfer_conn() called

    CFG->>KP: peer->connection = keeper
    GA->>DOP: from_peer->connection = going_away

    alt Before fix
        Note over GA: t_gr_stale/t_gr_restart stay on going_away
        DOP->>GA: Doppelganger freed → timers LOST
        Note over CFG: Stale routes never removed
    else After fix (this PR)
        GA->>KP: Migrate t_gr_stale with remaining time
        GA->>KP: Migrate t_gr_restart with remaining time
        Note over GA: going_away timers cancelled
        DOP->>GA: Doppelganger freed (no timers)
        KP->>CFG: t_gr_stale fires → bgp_clear_stale_route()
    end

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: bgpd/bgp_fsm.c
Line: 228-234

Comment:
**Sub-second truncation may fire timer immediately**

`event_timer_remain_second()` does integer division (`remain_msec / 1000`), so if fewer than 1 second remains on the stalepath timer the result is `0`. Passing `0` to `event_add_timer` arms the timer to fire at the very next event-loop iteration — semantically correct (the time has nearly elapsed), but worth noting. If sub-second precision matters here, consider using `event_timer_remain_msec()` together with `event_add_timer_msec()` to preserve the fractional remainder.

```suggestion
		unsigned long remain_ms =
			event_timer_remain_msec(going_away->t_gr_stale);
		event_cancel(&going_away->t_gr_stale);
		event_add_timer_msec(bm->master,
				bgp_graceful_stale_timer_expire,
				keeper, remain_ms,
				&keeper->t_gr_stale);
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "bgpd: migrate timers during peer_xfer_co..." | Re-trigger Greptile}

[shashanka-ks]

ci:rerun

[shashanka-ks]

ci:rerun

[shashanka-ks]

Hi @donaldsharp @ton31337 , could you please rerun the cancelled job: Ubuntu 24.04 amd64 Build? It timed out at 45 minutes due to slow Ubuntu package mirrors during the Docker build — not related to the PR changes.

[ton31337]

LGTM

[ton31337]

@Mergifyio backport stable/10.6 stable/10.5

[mergify]

backport stable/10.6 stable/10.5

✅ Backports have been created

Details

• #21662 bgpd: migrate timers during peer_xfer_conn to fix stale route cleanup (backport #21558) has been created for branch stable/10.6
• #21663 bgpd: migrate timers during peer_xfer_conn to fix stale route cleanup (backport #21558) has been created for branch stable/10.5