[MAJOR] §14: NatsConnection::fetch() returns raw natsMsg* with manual ownership

[mvillmow]

Finding

Severity: MAJOR
Section: 14
Evidence: include/transport/nats_connection.hpp:302-303
Principle: POLA

fetch() returns a raw natsMsg* that the caller must destroy via natsMsg_Destroy(). This is an unsafe ownership transfer pattern in modern C++. A std::unique_ptr<natsMsg, decltype(&natsMsg_Destroy)> with natsMsg_Destroy as the deleter would make ownership explicit and prevent leaks. Raw pointer return in a C++20 codebase is a POLA violation.

---

Part of #504

[mvillmow]

Implementation Plan

I now have everything needed to write a comprehensive implementation plan.

---

Implementation Plan: Issue #514 — NatsConnection::fetch() RAII Ownership for natsMsg*

1. Objective

Replace the raw natsMsg* return type of NatsConnection::fetch() with std::unique_ptr<natsMsg, decltype(&natsMsg_Destroy)> so ownership is explicit, compiler-enforced, and leak-proof without any manual natsMsg_Destroy() call at the callsite.

---

2. Approach

Key decisions:

• Define a type alias NatsMsgPtr in nats_connection.hpp using std::unique_ptr<natsMsg, decltype(&natsMsg_Destroy)>. This is declared once and reused by all callers — consistent with the team guidance to keep the deleter in one place.
• Change fetch() to return NatsMsgPtr instead of natsMsg*. A nullptr result (timeout) is expressed as a default-constructed NatsMsgPtr{nullptr, &natsMsg_Destroy}.
• Update nats_listener.cpp (pull_loop / handle_message) to accept NatsMsgPtr instead of natsMsg*. handle_message receives ownership by value and uses msg.get() to call the nats.c API functions; the destructor handles cleanup instead of the explicit natsMsg_Destroy(msg) call.
• All existing ack/nak logic in handle_message is preserved; the only change is removing the manual natsMsg_Destroy(msg) call at the end (the unique_ptr destructor takes care of it).
• No callers outside these two translation units use fetch() directly (confirmed by grep).

Why not std::optional<NatsMsgPtr>? A null unique_ptr already represents "no message" (timeout path), which is the existing contract. Adding optional would be an unnecessary wrapper.

---

3. Files to Create

None — all changes are to existing files.

---

4. Files to Modify

include/transport/nats_connection.hpp

• Add type alias (after the #include <nats.h> line, inside namespace keystone::transport):

  using NatsMsgPtr = std::unique_ptr<natsMsg, decltype(&natsMsg_Destroy)>;

• Change signature of fetch() at line 302:

  // Before:
  natsMsg* fetch(std::string_view subject,
                 std::string_view consumer_name,
                 int64_t timeout_ms = 30000);
  // After:
  NatsMsgPtr fetch(std::string_view subject,
                   std::string_view consumer_name,
                   int64_t timeout_ms = 30000);

• Update the doc comment above fetch() (lines 285–304): remove the sentence "caller owns it and must call natsMsg_Destroy()" and replace it with "ownership is transferred via NatsMsgPtr; the caller need not call natsMsg_Destroy()".

src/transport/nats_connection.cpp

• Add <memory> to includes (already needed for std::unique_ptr).
• Change fetch() return type (line 405) to NatsMsgPtr.
• Timeout path (line 448): return NatsMsgPtr{nullptr, &natsMsg_Destroy} instead of return nullptr.
• Empty-list path (line 453–455): same — return NatsMsgPtr{nullptr, &natsMsg_Destroy}.
• Success path (lines 459–463):

  // Before:
  natsMsg* msg = list.Msgs[0];
  list.Msgs[0] = nullptr;
  list.Count = 0;
  natsMsgList_Destroy(&list);
  return msg;
  // After:
  NatsMsgPtr msg{list.Msgs[0], &natsMsg_Destroy};
  list.Msgs[0] = nullptr;
  list.Count = 0;
  natsMsgList_Destroy(&list);
  return msg;

include/network/nats_listener.hpp

• Add #include "transport/nats_connection.hpp" if not already transitively present (it is not currently included; nats_listener.hpp only includes <nats.h> directly).
• Change handle_message private signature (line 80):

  // Before:
  void handle_message(natsMsg* msg) noexcept;
  // After:
  void handle_message(keystone::transport::NatsMsgPtr msg) noexcept;

src/network/nats_listener.cpp

• pull_loop (lines 194–227): Replace the natsMsg* msg = list.Msgs[0] section with NatsMsgPtr:

  // Before:
  natsMsg* msg = list.Msgs[0];
  list.Msgs[0] = nullptr;
  list.Count = 0;
  natsMsgList_Destroy(&list);
  handle_message(msg);
  
  // After:
  keystone::transport::NatsMsgPtr msg{list.Msgs[0], &natsMsg_Destroy};
  list.Msgs[0] = nullptr;
  list.Count = 0;
  natsMsgList_Destroy(&list);
  handle_message(std::move(msg));

• handle_message (lines 229–298): Change parameter to NatsMsgPtr msg (by value). Update the body:
  • Replace if (msg != nullptr) guard in the finish lambda with if (msg).
  • Replace natsMsg_Ack(msg, ...) / natsMsg_Nak(msg, ...) / natsMsg_GetSubject(msg) etc. with msg.get() where a raw pointer is required by the C API.
  • Remove the explicit natsMsg_Destroy(msg) call on line 241 — the unique_ptr destructor does this automatically when finish() runs.
  • The finish lambda should call msg.reset() (or just let the lambda's captured msg fall out of scope) after ack/nak to make the cleanup point explicit.

---

5. Implementation Order

1. Add NatsMsgPtr type alias to include/transport/nats_connection.hpp and update the fetch() declaration and doc comment.
2. Update src/transport/nats_connection.cpp: change return type, add <memory>, fix all three return sites (timeout, empty-list, success).
3. Update include/network/nats_listener.hpp: add the include for nats_connection.hpp, update handle_message signature.
4. Update src/network/nats_listener.cpp: fix pull_loop to construct NatsMsgPtr, fix handle_message to take NatsMsgPtr by value, remove manual natsMsg_Destroy, switch raw pointer calls to .get().
5. Update tests/unit/test_nats_connection.cpp: add a new test section NatsFetchOwnershipTest (see §6 below).
6. Build and run tests under ASan (cmake --preset asan && ctest --preset asan) to verify no use-after-free.

---

6. Verification

New Unit Tests — tests/unit/test_nats_connection.cpp

Add a NatsFetchOwnershipTest test fixture. Since fetch() requires a live NATS server, the tests validate the type and ownership contract without live calls:

FetchReturnTypeIsUniquePtr
  — Verify NatsMsgPtr is a unique_ptr<natsMsg, ...> (static_assert / type trait check).

FetchWithNullJsContextThrows
  — Call fetch() on a disconnected NatsConnection; expect std::runtime_error.
  — Confirm no resource leak (ASan validates this on its own).

FetchWithEmptySubjectThrowsDomainError
  — Construct a NatsMsgPtr = {nullptr, &natsMsg_Destroy}; verify it compiles and
    that .get() == nullptr and the destructor runs without crashing.

NatsMsgPtrDefaultNullIsValid
  — NatsMsgPtr p{nullptr, &natsMsg_Destroy}; EXPECT_EQ(p.get(), nullptr);
  — Verifies the null-message (timeout) return path type is usable.

Build + Sanitizer Validation

cmake --preset asan && cmake --build --preset asan
ctest --preset asan --output-on-failure   # Catches use-after-free from raw pointer copies

cmake --preset ubsan && cmake --build --preset ubsan
ctest --preset ubsan --output-on-failure  # Catches any UB in the unique_ptr path

clang-tidy

just lint

Confirm no cppcoreguidelines-owning-memory or modernize-use-owning-memory warnings remain on fetch().

Code Review Checklist

• natsMsg_Destroy is never called explicitly in any .cpp file (grep passes).
• fetch() return type in header and impl match.
• handle_message never calls .release() (which would defeat the RAII).
• NatsMsgPtr is the single definition site for the deleter.

---

7. Skills Used

Invoked during planning:

• None explicitly invoked via Skill tool — research was performed directly via Read and Grep on the codebase.

Team knowledge base skills referenced (from Prior Learnings):

• natsc-fetchcontent-cpp20-integration — Ownership patterns for nats.c structs (jsStreamInfo, jsPubAck, natsMsg); informed the NatsMsgPtr type alias placement and the deleter selection.
• check-memory-safety — RAII/ownership transfer checklist; confirmed the unique_ptr-with-custom-deleter pattern and the rule against calling .release().
• resource-manager-pattern — Guaranteed cleanup on exception via destructor; confirmed removing the explicit natsMsg_Destroy in handle_message's finish lambda.
• cpp-ci-sanitizer-tsan-data-race-fixes — Verified ASan is the correct sanitizer to run post-refactor to catch any leftover raw-pointer copies that would become dangling.

---

Generated by Claude Code Planner