Select failed errors in Syncd due to PR#1906

[sivat6]

Description of PR
Noticed the following error logs in syncd.

2026 Jun 15 06:51:58.579634 vlab-01 ERR syncd#syncd: :- run: select failed: 2

Summary:
Fixes #
processPendingDampingSync() was directly sending notifications via m_notifications->send() which might affect the fd that select() is managing resulted in inconsistent state.

Introduced a queue with mutex protection. processPendingDampingSync() enqueues the notifications and flushPendingDampingNotifications() dequeues and sends in main thread.

There were few review comments given after the marge of PR#1906. Addressed those review comments.

Type of change
Bug fix
Approach
What is the motivation for this PR?
Build issue in sonic-buildimage

Work item tracking
None

How did you do it?
The processPendingDampingSync() just enqueues the notifications. Later flushPendingDampingNotifications() dequeues and sends in main thread.

How did you verify/test it?
Unit Testing done. Without the fix, I can see the run:select_failed messages every 1 sec. After the fix, I am not seeing this error logs.

Any platform specific information?
None

Documentation
None

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

This PR updates syncd’s link-event damping flow to avoid interfering with the main select() loop by decoupling “detect pending damping sync” from “send PORT_STATE_CHANGE notifications”, and adds unit tests to validate the new queueing/flush behavior.

Changes:

• Queue port-state notifications generated by processPendingDampingSync() and flush them via a dedicated flushPendingDampingNotifications() path from the main thread.
• Add hasAnyValidDampingConfig() and update the Syncd::run() loop to handle Select::TIMEOUT explicitly and run damping processing on timeouts.
• Extend syncd unit tests for damping notification queueing/overflow/flush, and enable -fno-access-control for unittest access to private members.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
unittest/syncd/TestSyncd.cpp	Adds tests that validate notification queueing, flushing, and overflow behavior for damping sync.
unittest/syncd/Makefile.am	Adds -fno-access-control to allow unit tests to access private/protected members.
syncd/Syncd.h	Moves LinkEventDampingPortState to a new header and adds pending-notification queue/mutex + new helper declarations.
syncd/Syncd.cpp	Implements notification enqueue/flush, adds Select::TIMEOUT handling, and gates damping processing via hasAnyValidDampingConfig().
syncd/LinkEventDamping.h	New header containing the LinkEventDampingPortState struct previously embedded in Syncd.h.

[prsunny]

Can you check build failure?

[lolyu]

Review: #1948

Reviewed the full Syncd.cpp (not just the diff) to trace the threading model. The core fix is correct: m_notifications->send now has exactly two call sites — flushPendingDampingNotifications and the shutdown path — both on the main thread, while the timer thread (checkDampedPortsTimeout) only sets pending_state_sync and writes STATE_DB counters under m_linkEventDampingMutex. That's precisely the select failed: 2 race, correctly resolved. Nice work decoupling the send.

Requesting changes on one correctness issue (inline #1): because the in-memory advertised_status is committed at enqueue time but the send happens later, a batch that is dropped by the overflow guard — or stranded by the hasAnyValidDampingConfig() gate the Copilot reviewer already flagged — leaves the port's advertised/physical state permanently mismatched with no retry, since pending_state_sync was already cleared. Inline #2 proposes a single change (make flush unconditional) that resolves both the stranding and a redundant hot-path scan. #3 (-fno-access-control) is a question; #4 a minor DRY nit. The central fix is sound — these are about hardening the failure paths around it.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[yijingyan2]

Hi @sivat6, could you please help check the test failure? if it takes some time to fix, can I revert your change to unblock the submodule advance PR? Thanks!

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[sivat6]

Hi @sivat6, could you please help check the test failure? if it takes some time to fix, can I revert your change to unblock the submodule advance PR? Thanks!

Hi, This PR is pending due to baseline issue. It will be merged once the baseline issue resolved.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[prsunny]

@lolyu , could you rereview and merge?

@sivat6 , please change the title of PR to reflect what is done for the PR instead of "fix elastic failure"

[sivat6]

@lolyu , could you rereview and merge?

@sivat6 , please change the title of PR to reflect what is done for the PR instead of "fix elastic failure"

Changed the title.

[lolyu]

Review: #1948 (re-review at 6fcb8c8)

Thanks for the revision — verified all the changes against the full source, not just the diff. Everything I flagged is genuinely resolved, and the fixes are correct (not just relocated). Dismissing my earlier CHANGES_REQUESTED and approving.

#1 (blocking correctness) — fixed correctly. processPendingDampingSync now only enqueues; it no longer touches pending_state_sync or advertised_status. The in-memory commit moved into flushPendingDampingNotifications (lines 1616-1617) and — crucially — sits inside the success path, after m_notifications->send() returns. So a batch dropped by the overflow guard or failed in the try/catch leaves pending_state_sync = true, and the next pass re-enqueues it. The permanent advertised/physical mismatch (and the strictly-worse overflow-drop case) is now self-healing. Exactly the retry semantics I was asking for.

#2 / the gating issue — fixed. processPendingDampingSync() + flushPendingDampingNotifications() are now called unconditionally on both the TIMEOUT and unknown-selectable paths; the hasAnyValidDampingConfig() gate is gone, so a stranded queue always drains on the next loop iteration. This also removes the redundant double O(ports) scan.

#4 (DRY) — fixed via the new serializePortVids() helper, used by both paths. The Copilot swap-vs-copy nit is also addressed (emplace() + swap at enqueue).

New surface checked: moving the commit into the send path means flush now touches both m_pendingNotificationsMutex and m_linkEventDampingMutex. Confirmed there's no lock-ordering inversion — both processPendingDampingSync and flush acquire one mutex, release it, then acquire the other; neither ever holds both simultaneously. The try/catch around the send (one bad batch can't strand the rest) is a nice touch.

The core decoupling remains correct: m_notifications->send is main-thread-only, the timer thread only sets flags under the damping mutex. This is a sound fix for the select failed: 2 race. LGTM.

[sivat6]

Review: #1948 (re-review at 6fcb8c8)

Thanks for the revision — verified all the changes against the full source, not just the diff. Everything I flagged is genuinely resolved, and the fixes are correct (not just relocated). Dismissing my earlier CHANGES_REQUESTED and approving.

#1 (blocking correctness) — fixed correctly. processPendingDampingSync now only enqueues; it no longer touches pending_state_sync or advertised_status. The in-memory commit moved into flushPendingDampingNotifications (lines 1616-1617) and — crucially — sits inside the success path, after m_notifications->send() returns. So a batch dropped by the overflow guard or failed in the try/catch leaves pending_state_sync = true, and the next pass re-enqueues it. The permanent advertised/physical mismatch (and the strictly-worse overflow-drop case) is now self-healing. Exactly the retry semantics I was asking for.

#2 / the gating issue — fixed. processPendingDampingSync() + flushPendingDampingNotifications() are now called unconditionally on both the TIMEOUT and unknown-selectable paths; the hasAnyValidDampingConfig() gate is gone, so a stranded queue always drains on the next loop iteration. This also removes the redundant double O(ports) scan.

#4 (DRY) — fixed via the new serializePortVids() helper, used by both paths. The Copilot swap-vs-copy nit is also addressed (emplace() + swap at enqueue).

New surface checked: moving the commit into the send path means flush now touches both m_pendingNotificationsMutex and m_linkEventDampingMutex. Confirmed there's no lock-ordering inversion — both processPendingDampingSync and flush acquire one mutex, release it, then acquire the other; neither ever holds both simultaneously. The try/catch around the send (one bad batch can't strand the rest) is a nice touch.

The core decoupling remains correct: m_notifications->send is main-thread-only, the timer thread only sets flags under the damping mutex. This is a sound fix for the select failed: 2 race. LGTM.

Thanks lolyu

[prsunny]

Original PR - #1906