Coverity: handle uncaught-exception reports in test code

[bryancall]

Coverity reports uncaught-exception escapes from test destructors and test main() functions. Part of #13682.

The one thing to decide

src/iocore/eventsystem/unit_tests/test_MIOBufferWriter.cc redefines _ink_assert to throw InkAssertExcept(). It already carries its own // coverity[UNCAUGHT_EXCEPT:FALSE] noting that "Coverity is confused and thinks this _ink_assert is the one used in traffic_server." Coverity merges symbol definitions across the whole analysis, so every ink_assert() in the tree resolves to that throwing definition.

That is why these destructors look like they can throw. They reach an assert one of three ways:

• directly (~CacheTestSM)
• via THREAD_FREE() → thread_freeup(), which asserts on its postcondition (~EasyURL through HdrHeap::destroy(); ~CacheReadTest / ~CacheWriteTest through free_MIOBuffer → dealloc_reader)
• via EThread::schedule() (~TestContChain)

The aborting _ink_assert from unit_tests/stub.cc is what actually links into these binaries, so the exception cannot occur.

Important for anyone fixing this family

Adding noexcept to a destructor is a no-op. A user-provided destructor with no exception-specification is already implicitly noexcept(true) in C++20, even when its body calls a potentially-throwing function (std::is_nothrow_destructible_v is 1). An explicit noexcept changes nothing and cannot clear the finding.

Two treatments, chosen per site

Suppression where the exception provably cannot occur and there is nothing to handle — // coverity[UNCAUGHT_EXCEPT:FALSE] with the trace in a comment: CIDs 1528624, 1528646, 1528704, 1528771, 1591506.

try/catch + ink_abort where a guard is strictly better than a silent std::terminate, so a genuine escape still fails loudly with context: CIDs 1518135, 1559190 (~ParentTest), 1686062 (~HoldOnEThread), 1518878, 1528569 (~NetVCTest). ci/coverity-model.cpp already teaches Coverity that ink_abort panics.

try/catch returning a non-zero exit status for the main() findings, so a test failure is still reported rather than terminating: CIDs 1523670, 1528654, 1644283 (unit_test_main.cc), 1523686, 1528590, 1644327 (test_AIO.cc), 1686026, 1587256, 1587267 (test_RefCountCache.cc), 1528601 (test_HPACK.cc). main is deliberately not marked noexcept.

Several commits close three CIDs, because Coverity reports one per throwing call site and a single handler covers them all.

Alternative worth considering

The project also has precedent for // coverity[exn_spec_violation] annotations (Stripe::~Stripe(), HttpSM::~HttpSM()). If a reviewer prefers one uniform treatment across the whole family rather than suppression-where-provable and guard-where-useful, the destructor commits are the ones to swap.

Verification

test_records, test_tsutil, test_proxy_hdrs, test_proxy_hdrs_xpack, test_cache, test_hostdb, test_tscore, test_tsconfig — 321 tests, all passing. Every touched file compiles independently on this branch.

[Copilot]

🟢 Approval recommended

The reviewed changes address the reported Coverity findings with targeted handling and suppressions.

Pull request overview

This pull request addresses Coverity uncaught-exception reports in test destructors and entry points without changing production behavior.

Changes:

• Adds exception handling to test runners.
• Guards destructor failures with ink_abort.
• Documents verified Coverity false-positive suppressions.

File summaries

File	Reviewed change
src/records/unit_tests/unit_test_main.cc	Handles test initialization and execution exceptions.
src/proxy/http2/test_HPACK.cc	Handles HPACK test-runner exceptions.
src/proxy/http/remap/unit-tests/test_RemapRulesYaml.cc	Suppresses a verified teardown false positive.
src/iocore/net/NetVCTest.cc	Guards buffer teardown failures.
src/iocore/hostdb/unit_tests/test_RefCountCache.cc	Reports test exceptions as failures.
src/iocore/eventsystem/unit_tests/test_Lock.cc	Guards continuation teardown.
src/iocore/cache/unit_tests/main.h	Suppresses cache-test destructor false positives.
src/iocore/cache/unit_tests/CacheTestHandler.h	Suppresses a continuation-chain false positive.
src/iocore/cache/CacheTest.cc	Suppresses a cache destructor false positive.
src/iocore/aio/test_AIO.cc	Handles AIO test-runner exceptions.
src/api/InkAPITest.cc	Guards ParentTest teardown failures.

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[brbzull0]

LGTM. I verified the FALSE classification independently rather than taking it on faith, and it holds: the throwing _ink_assert (throw InkAssertExcept(), test_MIOBufferWriter.cc:196) is linked into exactly one executable — add_executable(test_MIOBufferWriter unit_tests/test_MIOBufferWriter.cc) with target_link_libraries(... libswoc::libswoc Catch2::Catch2WithMain), no ts::tscore. Every binary containing an annotated destructor links ts::tscore, whose _ink_assert is TS_NORETURN and calls abort(). So nothing on those teardown paths can throw and no real failure is being hidden. A repo-wide grep turns up only three _ink_assert definitions — the aborting one in libtscore, the throwing one in test_MIOBufferWriter.cc, and an std::exit(1) one in test_LogUtils.cc:124 — and neither override links with any annotated file.

One factual fix for the PR description (not the code): it says the aborting _ink_assert comes from unit_tests/stub.cc, but no stub.cc in the tree defines it. It's src/tscore/ink_assert.cc.

On the question the PR body asks the reviewer to decide — three destructors get a runtime try/catch while five structurally identical ones get a comment-only annotation, even though the analysis establishes the exception is impossible in all eight. I'd go one way or the other: either drop the three guards in favour of annotations matching the other five (the Stripe.cc:316 comment is a good model), or keep them as defence-in-depth with a one-line note saying why those three differ.

If you keep the guards, consider adding a catch (std::exception const &e) arm ahead of each catch (...) and passing e.what() through ink_abort. As written they abort with a fixed string, discarding the exception type and message — strictly less information than the default terminate handler they replace, and inconsistent with the main() guards added in the same PR, which do print e.what().

One alternative worth considering: the root cause is a Coverity modelling gap, and the repo already owns the mechanism for it. ci/coverity-model.cpp models _TSReleaseAssert as __coverity_panic__() for exactly this reason but has no entry for _ink_assert. Adding one there would fix all eleven files at the source rather than annotating each site, and would keep future ink_assert() calls from re-triggering this.