Skip to content

Adj-Rib: fix leak memory on withdraw unknown path#3310

Closed
taitov wants to merge 1 commit intoosrg:masterfrom
taitov:adj-rib-leak
Closed

Adj-Rib: fix leak memory on withdraw unknown path#3310
taitov wants to merge 1 commit intoosrg:masterfrom
taitov:adj-rib-leak

Conversation

@taitov
Copy link
Copy Markdown
Contributor

@taitov taitov commented Feb 4, 2026

If you try to delete an unknown path from adj-rib, a key with an empty slice will be created, which will live until you delete adj-rib.

@taitov
Copy link
Copy Markdown
Contributor Author

taitov commented Feb 4, 2026

Hi @fujita ! Please take a look at this little fix.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes an Adj-RIB destination retention issue where processing a withdraw for an unknown path could create/retain an empty destination entry until the Adj-RIB is cleared.

Changes:

  • Delete the destination when handling a withdraw that results in an empty knownPathList, even if the withdrawn path wasn’t previously present.
  • Add a regression test covering “withdraw unknown path” behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
internal/pkg/table/adj.go Ensures empty destinations are removed after withdraw processing regardless of whether a matching path was found.
internal/pkg/table/adj_test.go Adds a test to confirm no destination entry remains after withdrawing an unknown path.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


adj := NewAdjRib(logger, families)
adj.Update([]*Path{p1})
assert.Equal(t, len(adj.table[family].destinations), 0)
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test reaches into the Table's unexported destinations field. Since the behavior being verified is “no destination was created/retained”, this can be asserted via the existing GetDestinations() accessor instead, which keeps the test resilient to internal representation changes while still failing in the pre-fix case (destination leaked with 0 paths).

Suggested change
assert.Equal(t, len(adj.table[family].destinations), 0)
assert.Equal(t, len(adj.table[family].GetDestinations()), 0)

Copilot uses AI. Check for mistakes.
@fujita
Copy link
Copy Markdown
Member

fujita commented Feb 17, 2026

Nice catch! The Copilot comment is reasonable but trivial. So I pushed this fix.

@fujita fujita closed this Feb 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants