[xcvrd]: Fast-path wait_for_port_config_done() via APPL_DB existence check

[gs1571]

Description

wait_for_port_config_done() (present in both DaemonXcvrd and
CmisManagerTask, identical logic in each) subscribes to APPL_DB
PORT_TABLE via SubscriberStateTable and blocks until it observes a
live pub/sub SET notification with key PortConfigDone or
PortInitDone. portsyncd emits this notification exactly once, when
swss finishes initial port bring-up.

If xcvrd (re)starts after that one-time notification already fired — e.g.
a crash-restart, or any restart of xcvrd alone without a corresponding
swss restart/config reload — the subscription starts too late: the
notification itself is gone, but the underlying key is still a normal,
queryable entry in APPL_DB. The daemon then blocks in this wait loop
forever and never processes any port on the switch.

Added a fast-path check via appl_db.exists() for either key before
subscribing to the pub/sub notification; if either is already present,
return immediately.

Motivation and Context

Found while debugging why CmisManagerTask never progressed past module
insertion on a switch where xcvrd had been restarted (without a full
config reload) during iterative bring-up of an 800G ZR+ coherent module.
Confirmed via debug logging that the daemon printed "Waiting for
PortConfigDone..." once at startup and then produced no further output —
the notification had already occurred (PORT_TABLE:PortConfigDone existed
in APPL_DB the whole time), but the daemon had no way to notice.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[gs1571]

/azp run

[mssonicbld]

/azp run

[azure-pipelines]

Commenter does not have sufficient privileges for PR 857 in repo sonic-net/sonic-platform-daemons

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[mssonicbld]

Hi, there are workflow run(s) waiting for approval, you may be first-time contributor. I will notify maintainers to help approve once PR is approved. Thanks!

_{---Powered by SONiC BuildBot}

[bgallagher-nexthop]

I am not confident the premise of "the subscription starts too late: the notification itself is gone" -> we now get stuck forever is actually true here.

Independent restart of xcvrd typically works today in my testing via systemctl restart pmon. In the SubscriberStateTable constructor, existing keys are pushed into the buffer here which means those keys should be notified to xcvrd when select() is called.

Is it possible that there was a downstream issue here in the likes of the CmisManagerTask instead?

[gs1571]

I am not confident the premise of "the subscription starts too late: the notification itself is gone" -> we now get stuck forever is actually true here.

Independent restart of xcvrd typically works today in my testing via systemctl restart pmon. In the SubscriberStateTable constructor, existing keys are pushed into the buffer here which means those keys should be notified to xcvrd when select() is called.

Is it possible that there was a downstream issue here in the likes of the CmisManagerTask instead?

Good catch on the constructor snapshot — you're right that SubscriberStateTable's constructor pre-loads existing keys into m_buffer (subscriberstatetable.cpp), so in isolation this should just work. But there's a second bug underneath that makes it not work in practice, and my original description was imprecise — "the notification is gone forever" isn't quite what happens.

wait_for_port_config_done() calls port_tbl.pop() (singular), not pops(). ConsumerTableBase::pop() (consumertablebase.cpp:22-44) does:

if (m_buffer.empty()) {
    pops(m_buffer, prefix);   // drains ALL of SubscriberStateTable::m_buffer into ConsumerTableBase::m_buffer
}

kco = m_buffer.front(); ... m_buffer.pop_front();  // returns only one

The first pop() call drains the entire constructor-time snapshot — every key currently in PORT_TABLE on a real switch, not just PortConfigDone/PortInitDone — into a second, separate buffer, and hands back only the front item. That's essentially never PortConfigDone.

SubscriberStateTable::hasData()/hasCachedData() (subscriberstatetable.cpp:85-93) only look at its own m_buffer/m_keyspace_event_buffer, which is now empty after that drain — not at ConsumerTableBase::m_buffer, where the rest of the snapshot (including PortConfigDone) is sitting. So select() reports TIMEOUT on the next iteration, and the loop's if state == TIMEOUT: continue means it never calls pop() again. The remaining data is one pop() call away and nothing tells the loop to make that call.

So the actual mechanism isn't "the pub/sub event is lost" — it's a cache-visibility gap between ConsumerTableBase's own buffering and Select's per-selectable cache tracking, and it triggers deterministically whenever the snapshot has more than one key before the target key, which is always true in production (PORT_TABLE has one entry per port plus the two control keys).

Two ways to fix it:

1. Keep the appl_db.exists() fast-path — sidesteps Select/SubscriberStateTable entirely when the key's already there.
2. Fix the drain pattern itself: loop pop() until it returns an empty key before going back to select() (same fix needed in CmisManagerTask's copy).

Happy to add (2) alongside (1), or drop (1) in favor of (2).

[bgallagher-nexthop]

I suspect you may be on some older version of the codebase or something?

• SubscriberStateTable::m_buffer and ConsumerTableBase::m_buffer are the same buffer, the latter is a superclass of the former.
• pop only calls pops when m_buffer is empty, which it won't be in the described scenario.
• The loop in xcvrd to keep poping until either "PortConfigDone" or "PortInitDone" looks fully valid to me as-is.

[gs1571]

You're right, and thanks for pushing on this — I went through the swss-common source and reproduced it on hardware; the premise doesn't hold.

Walking the actual code (master):

• SubscriberStateTable : public ConsumerTableBase, and m_buffer is declared only in ConsumerTableBase — it's a single inherited buffer, so there's no two-buffer desync.
• The constructor snapshots all existing PORT_TABLE keys into m_buffer via getKeys(), including PortConfigDone/PortInitDone when they already exist.
• pop() calls pops() only when m_buffer is empty and otherwise returns one element, leaving the rest; Select::poll_descriptors reinserts the selectable while hasCachedData() is true, so successive select()→pop() drains the whole snapshot.

I also verified on the switch: creating a SubscriberStateTable(APPL_DB, "PORT_TABLE") and draining it with select()/pop() returned all 131 keys, with PortInitDone at position 3 and PortConfigDone at 128 — the loop would exit after a few iterations, not hang.

The hang I attributed to this was a different, downstream issue on our (vendor-forked) build. Closing this PR — thanks again for the careful review.