[orchagent]: _exit() on graceful shutdown to avoid destructor-chain memory corruption

[inder-nexthop]

What I did

Made orchagent exit through _exit(0) when a graceful shutdown is requested, instead of falling through to return 0 and running the full ~OrchDaemon destructor chain. Before exiting it drains the async swss recorder so any pending records are flushed first.

Why I did it

Fixes #4600.

PR #4400 added graceful shutdown handling. On SIGTERM or SIGINT the signal handler sets gOrchShutdownRequested, OrchDaemon::start() returns, and main() falls through to return 0. That runs the full ~OrchDaemon destructor chain, which is not safe to run on shutdown.

During that teardown, FlexCounterManager destruction makes blocking SAI calls (stopFlexCounterPolling, which calls set_switch_attribute). Those calls go through sairedis's ZMQ channel to syncd and park the main thread in zmq_poll while several libzmq IO threads are still running. Orchs that were deleted earlier in the reverse order delete loop have freed buffers that those IO threads still reference, so the process corrupts its own heap. We saw orchagent crash in a libzmq IO thread when METADATA configuration was changed and the containers were restarted.

How I verified it

Configure the route state async publish and ZMQ options in METADATA, then restart the containers so orchagent receives SIGTERM. Before this change orchagent crashes fairly consistently during teardown. With this change orchagent shuts down cleanly and the crash no longer reproduces.

Details if related

Draining the async swss recorder through setAsync(false) joins the recorder worker so pending records are flushed, which was the reason PR #4400 wanted start() to return cleanly.

[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).

[deepak-singhal0408]

Reviewed the fix and the root cause in #4600. The analysis holds up:

Letting start() return and running ~OrchDaemon on graceful shutdown is unsafe because the reverse-order teardown reaches ~FlexCounterManager, which issues stopFlexCounterPolling() → set_switch_attribute(). That's a synchronous SAI call over sairedis' ZMQ channel, so the main thread parks in zmq_poll while libzmq I/O threads are still live — but orchs destroyed earlier in the loop have already freed buffers those threads reference, corrupting the heap. This is a structural ordering problem, not something easily fixed by reordering destructors. Before #4400 orchagent always _exit()-ed on SIGTERM, so this path never ran.

Reverting to _exit() while draining the async swss recorder first is the right, minimal fix — it sidesteps the whole destructor chain and is robust to any future unsafe teardown. The drain path also looks sound: setAsync(false) → stopAsyncWorker() joins the worker, drain() only exits once the queue is empty, and records are written with std::endl so they're flushed before _exit(); setAsync(false) is a safe no-op if the worker never started.

Two optional nits, neither blocking:

• After this change start() only returns on shutdown, so the trailing return 0; is now unreachable — a one-line comment noting that would help future readers.
• _exit(0) reports a clean exit on signal termination; that matches the prior return 0 intent, just flagging in case exit-status conventions ever matter.

LGTM.

[Copilot]

Pull request overview

This PR changes orchagent shutdown behavior so that when a graceful shutdown is requested it drains the async swss.rec recorder queue and terminates via _exit(0) to avoid running the ~OrchDaemon destructor chain, which the PR description notes can trigger teardown-time memory corruption due to unsafe shutdown ordering.

Changes:

• Add a post-OrchDaemon::start() shutdown path that disables async recording (drain/flush) and calls _exit(0) instead of returning normally.

[venkit-nexthop]

tagging @vpandian-nokia @prsunny to approve.

[prsunny]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[vpandian-nokia]

tagging @vpandian-nokia @prsunny to approve.

Thanks for tagging me. The proposed fix in PR #4699 looks like the right minimal approach: drain the async recorder first, then use _exit(0) to avoid the destructor-chain teardown.

I see the required coverage check is failing because the new shutdown branch is not covered. A small UT for the shutdown decision path would be useful if practical, but I would not expect the UT to reproduce the heap corruption itself.

I am not currently on the reviewer list, so I cannot formally approve, but from my review, the fix looks reasonable.

[prsunny]

@inder-nexthop , can you fix the coverage?

[deepak-singhal0408]

@inder-nexthop , can you fix the coverage?

@inder-nexthop, gentle ping on this. JFYI, there are some Ci failures observed in sonic-swss-common repo without this change.
cc: @venkit-nexthop as well.

[prsunny]

@deepak-singhal0408 , suggest revert the original PR since this fix is getting delayed.

@inder-nexthop , @prabhataravind , @venkit-nexthop , @vpandian-nokia for viz.

[deepak-singhal0408]

@deepak-singhal0408 , suggest revert the original PR since this fix is getting delayed.

@inder-nexthop , @prabhataravind , @venkit-nexthop , @vpandian-nokia for viz.

@prsunny had a teams chat with @inder-nexthop on this today. He is working on the code coverage part. lets wait till tomorrow? thanks,

[prsunny]

@deepak-singhal0408 , suggest revert the original PR since this fix is getting delayed.
@inder-nexthop , @prabhataravind , @venkit-nexthop , @vpandian-nokia for viz.

@prsunny had a teams chat with @inder-nexthop on this today. He is working on the code coverage part. lets wait till tomorrow? thanks,

Thanks. May I know why fixing coverage is taking more than a week? I would like to understand submitter feedback and if something can be improved on infra. @theasianpianist for viz.

[inder-nexthop]

@deepak-singhal0408 , suggest revert the original PR since this fix is getting delayed.
@inder-nexthop , @prabhataravind , @venkit-nexthop , @vpandian-nokia for viz.

@prsunny had a teams chat with @inder-nexthop on this today. He is working on the code coverage part. lets wait till tomorrow? thanks,

Thanks. May I know why fixing coverage is taking more than a week? I would like to understand submitter feedback and if something can be improved on infra. @theasianpianist for viz.

Hi @prsunny , apologies for missing your initial comment. I was on PTO, working on resolving the comments mentioned in the review.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[deepak-singhal0408]

LGTM.

[rookie-who]

This PR has the "Request for 202605 Branch" label but is missing 202605-specific test results. Please:

1. Tick the 202605 checkbox in the "Tested branch" section of the PR description
2. Provide 202605 branch test evidence in the "Test result" section
3. Add the label "Tested for 202605 branch" once done

Master branch test results alone are not sufficient for 202605 cherry-pick approval.

Comment on behalf of @vaibhavhd
Review by @vaibhavhd via automated tooling

[mssonicbld]

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}

[deepak-singhal0408]

This is live on the 202605 nightly. We root-caused a recurring 202605 nightly failure to the same shutdown-teardown hazard as #4600, but a different casualty.

Surfaces as: the pre-test sanity of tests/test_pretest.py (check_processes/check_bgp) reports the bgp container down, docker exec bgp → rc=127. That's a detector, not the cause — a config reload/container restart sends orchagent SIGTERM, it crashes on graceful shutdown, and the whole swss group restarts; the sanity gate just probes bgp mid-restart.

Root cause (symbolized core), same enabler (#4400 → ~OrchDaemon on SIGTERM):

~OrchDaemon → ~MuxOrch → ~MuxCable → ~MuxAclHandler (muxorch.cpp)
  → gAclOrch->getAclRule() → AclOrch::getTableById() (aclorch.cpp)
    on an already-freed AclOrch → use-after-free → SIGSEGV

~MuxAclHandler calls into gAclOrch from a destructor after the reverse-order teardown already freed AclOrch; getTableById's for (auto it : m_AclTables) by-value copy then destroys an AclRule control block in freed memory. Reproduces only on a mux/dual-ToR config.

Why the fix is right: it skips the entire destructor chain (robust to any unsafe teardown, not a per-orch patch), drains the async recorder first so nothing is lost, and _exit(0) restores the exact pre-#4400 SIGTERM behavior — a minimal revert-to-known-good.

[saiarcot895]

Orchs that were deleted earlier in the reverse order delete loop have freed buffers that those IO threads still reference, so the process corrupts its own heap.

Not necessarily related to the changes in this PR or the fix, but this statement sounds like there's an object ownership problem in orchagent, with regards to who owns the buffers and is thus responsible for freeing those buffers.

[deepak-singhal0408]

@inder-nexthop @venkit-nexthop could you help provide testing result from 202605 branch proving the fix is safe/addresses the problem? we should be able to repro the issue #4699 (comment)?

[venkit-nexthop]

Orchs that were deleted earlier in the reverse order delete loop have freed buffers that those IO threads still reference, so the process corrupts its own heap.

Not necessarily related to the changes in this PR or the fix, but this statement sounds like there's an object ownership problem in orchagent, with regards to who owns the buffers and is thus responsible for freeing those buffers.

Yes, you are right that there is an object ownership problem. If orchagent had been strictly implemented with object oriented concepts, it should have had classes that strictly own some resources and object of these classes should be referenced (and held in place) by other objects that use the resources. This would have created a clean destructor order that would avoid this kind of issue.
But realistically, fixing this problem may be harder at this time. It would involve a lot of code refactoring for little benefit. Since we almost never expect a destructor of this form to get called in production (this destructor call got introduced due to some recent code change).

[inder-nexthop]

Hey, sending over a patch with the test work backing the orchagent graceful shutdown fix (sonic-swss PR 4699). What it contains:

1. Two tests in tests/mock_tests/mux_rollback_ut.cpp. Each forks a child that replays ~OrchDaemon's deletion order, where AclOrch is freed before MuxOrch:

• OrchDaemonTeardownOrderHeapUseAfterFree (ASAN builds only): child runs delete gAclOrch; delete m_MuxOrch; and must die with a heap use after free. The frames match the 202605 nightly core exactly: ~MuxAclHandler -> getAclRule -> getTableById on the freed AclOrch.
• GracefulShutdownExitsBeforeOrchDaemonTeardown (all builds): child requests shutdown, calls exit_if_graceful_shutdown_requested(), then falls through to the same deletes. It must exit 0. On any tree without the 4699 fix this fails, so it doubles as a permanent regression guard.

2. ASAN wiring for the mock test binary in tests/mock_tests/Makefile.am (--enable-asan now instruments it, same pattern as p4orch_tests).

orchagent_teardown_ut_asan_fixes.patch

Test matrix:

  ┌──────────────────────────────┬───────────────────────┬─────────────┬───────────────────────────────────────────────────────────────┬────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
  │            Target            │         Build         │  Fix state  │                           Test case                           │                                                   Result                                                   │
  ├──────────────────────────────┼───────────────────────┼─────────────┼───────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ master                       │ ASAN                  │ fix present │ MuxRollbackTest.OrchDaemonTeardownOrderHeapUseAfterFree       │ PASS — child dies of heap-use-after-free, as asserted                                                      │
  ├──────────────────────────────┼───────────────────────┼─────────────┼───────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ master                       │ ASAN                  │ fix present │ MuxRollbackTest.GracefulShutdownExitsBeforeOrchDaemonTeardown │ PASS — child exits 0 before teardown                                                                       │
  ├──────────────────────────────┼───────────────────────┼─────────────┼───────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ master                       │ ASAN                  │ fix removed │ MuxRollbackTest.OrchDaemonTeardownOrderHeapUseAfterFree       │ PASS — teardown order is lethal either way                                                                 │
  ├──────────────────────────────┼───────────────────────┼─────────────┼───────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ master                       │ ASAN                  │ fix removed │ MuxRollbackTest.GracefulShutdownExitsBeforeOrchDaemonTeardown │ FAIL — heap-use-after-free in ~MuxAclHandler → getAclRule → getTableById, freed by ~AclOrch                │
  ├──────────────────────────────┼───────────────────────┼─────────────┼───────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ master                       │ ASAN                  │ fix present │ full mock suite                                               │ 913 tests / 91 suites pass, zero ASAN reports                                                              │
  ├──────────────────────────────┼───────────────────────┼─────────────┼───────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ master                       │ non-ASAN (CI default) │ fix present │ all touched suites                                            │ 140 tests pass; ASAN-only repro compiled out; guard test passes                                            │
  ├──────────────────────────────┼───────────────────────┼─────────────┼───────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ 202605 + #4699 cherry-picked │ ASAN                  │ fix present │ MuxRollbackTest.OrchDaemonTeardownOrderHeapUseAfterFree       │ PASS (772 ms)                                                                                              │
  ├──────────────────────────────┼───────────────────────┼─────────────┼───────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ 202605 + #4699 cherry-picked │ ASAN                  │ fix present │ MuxRollbackTest.GracefulShutdownExitsBeforeOrchDaemonTeardown │ PASS (413 ms)                                                                                              │
  ├──────────────────────────────┼───────────────────────┼─────────────┼───────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ 202605 as-is (no backport)   │ ASAN                  │ fix absent  │ MuxRollbackTest.OrchDaemonTeardownOrderHeapUseAfterFree       │ PASS (697 ms)                                                                                              │
  ├──────────────────────────────┼───────────────────────┼─────────────┼───────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ 202605 as-is (no backport)   │ ASAN                  │ fix absent  │ MuxRollbackTest.GracefulShutdownExitsBeforeOrchDaemonTeardown │ FAIL (649 ms) — heap-use-after-free at ~MuxAclHandler muxorch.cpp:1486, freed by ~AclOrch aclorch.cpp:4242 │
  └──────────────────────────────┴───────────────────────┴─────────────┴───────────────────────────────────────────────────────────────┴────────────────────────────────────────────────────────────────────────────────────────────────────────────┘

CC @deepak-singhal0408 @venkit-nexthop @vaibhavhd

[prsunny]

Approving for 202605

[mssonicbld]

Cherry-pick PR to 202605: #4788