[neighorch]: Avoid inserting missing bulk neighbors#4772
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6b568a1-5d2a-4309-b1ac-ef21ca155079 Signed-off-by: Xichen96 <lukelin0907@gmail.com>
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR fixes a subtle cache-mutation bug in NeighOrch::enableNeighbors() where reading m_syncdNeighbors[neighborEntry] could insert a default-constructed neighbor entry, making subsequent “not found” handling ineffective. The change ensures missing neighbors are skipped without altering the cache and adds a mock regression test to prevent reintroduction.
Changes:
- Replace
operator[]access with afind()-based lookup before reading a neighbor’s cached MAC inenableNeighbors(). - Skip processing for missing neighbor entries without mutating
m_syncdNeighbors. - Add a mock unit test verifying
enableNeighbors()does not insert a missing neighbor entry.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| orchagent/neighorch.cpp | Avoids unintended insertion into m_syncdNeighbors by using find() before reading cached neighbor data in enableNeighbors(). |
| tests/mock_tests/neighorch_ut.cpp | Adds regression coverage ensuring a missing neighbor in a bulk enable request does not create a cache entry. |
|
/azpw run |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1170080: ✅Stage TestAsan:
✅Stage Test:
|
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
This PR has backport request label(s) for branch(es): 202605, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202605: <test result>) in the Test result section as well in your PR description. ---Powered by SONiC BuildBot
|
What I did
Prevented
NeighOrch::enableNeighbors()from inserting a default cache entry while checking whether a bulk-enable neighbor exists.Root cause
The old code read the map before checking it:
std::map::operator[]inserts a default value when the key is missing. The intended absence check was therefore unreachable:The fix uses one non-mutating lookup and reads the MAC only after the entry is confirmed:
Why I did it
MuxNbrHandler::enable()can hand NeighOrch a context for a neighbor that disappeared fromm_syncdNeighborsbefore bulk processing. A read-side guard must not manufacture the state it is trying to detect.This defect was found while reviewing PR #4746, but it predates that change and belongs in a separate PR because it affects NeighOrch cache correctness and MUX bulk-enable behavior, not the IntfsOrch removal fence.
The fix has no standalone master merge dependency and remains independently reviewable.
How I verified it
Added
EnableNeighborsDoesNotInsertMissingNeighborintests/mock_tests/neighorch_ut.cpp.The regression:
m_syncdNeighbors;enableNeighbors();Current exact-head master PR build
faa3e6df99028137237ea70c9fa99f5a8b8c097cTest vstest, andTestAsan vstestlanes passed.202605 PR-build validation
operator[]-before-find()defect exists on202605.d4b0502ecdac76d56fdf063a633045c0fb0de977Tested for 202605 branchlabel must remain absent until that exact result is complete.Details if related
orchagent/neighorch.cpptests/mock_tests/neighorch_ut.cpp