More cleanup

design.md

@@ -393,7 +393,7 @@ Only Server can create connections (using private constructor via friend access)
 #### Simple Synchronous Handler
 
 ```cpp
-class HttpHandler : public ConnectionHandler {
+class HttpHandler : ConnectionHandler {
 public:
   void on_data_arrived(std::string_view data, Connection& conn) override {
     // Parse HTTP request using connection's arena
@@ -410,7 +410,7 @@ public:
 #### Async Handler with WeakRef
 
 ```cpp
-class AsyncHandler : public ConnectionHandler {
+class AsyncHandler : ConnectionHandler {
 public:
   void on_data_arrived(std::string_view data, Connection& conn) override {
     // Get weak reference for async processing
@@ -429,7 +429,7 @@ public:
 #### Batching Handler with User Data
 
 ```cpp
-class BatchingHandler : public ConnectionHandler {
+class BatchingHandler : ConnectionHandler {
 public:
   void on_connection_established(Connection &conn) override {
     // Allocate some protocol-specific data and attach it to the connection
@@ -468,7 +468,7 @@ private:
 #### Streaming "yes" Handler
 
 ```cpp
-class YesHandler : public ConnectionHandler {
+class YesHandler : ConnectionHandler {
 public:
   void on_connection_established(Connection &conn) override {
     // Write an initial "y\n"

src/arena.cpp

@@ -1,4 +1,5 @@
 #include "arena.hpp"
+
 #include <cassert>
 #include <iomanip>
 #include <limits>

src/arena.hpp

@@ -59,10 +59,9 @@
  *
  * ### Safe Usage Patterns in WeaselDB:
  * - **Per-Connection Instances**: Each Connection owns its own Arena
- *   instance, accessed only by the thread that currently owns the connection
+ *   instance, accessed by its io thread
- * - **Single Owner Principle**: Connection ownership transfers atomically
+ * - **Server Ownership**: Server retains connection ownership, handlers access
- * between threads using unique_ptr, ensuring only one thread accesses the arena
+ * arenas through Connection& references with proper mutex protection
- * at a time
  *
  * ### Thread Ownership Model:
  * 1. **I/O Thread**: Server owns connections, processes socket I/O events

src/connection.cpp

@@ -45,8 +45,7 @@ Connection::Connection(struct sockaddr_storage addr, int fd, int64_t id,
     : fd_(fd), id_(id), epoll_index_(epoll_index), addr_(addr),
       handler_(handler), server_(std::move(server)) {
   auto server_ref = server_.lock();
-  // This should only be called from a member of Server itself, so I should
+  // Should only be called from the io thread
-  // hope it's alive.
   assert(server_ref);
   server_ref->active_connections_.fetch_add(1, std::memory_order_relaxed);
 
@@ -96,19 +95,23 @@ void Connection::append_message(std::span<std::string_view> data_parts,
   bool was_empty = message_queue_.empty();
 
   // Add message to queue
+  // TODO this allocates while holding the connection lock
   message_queue_.emplace_back(
       Message{std::move(arena), data_parts, close_after_send});
   outgoing_bytes_queued_ += total_bytes;
 
   // If queue was empty, we need to add EPOLLOUT interest.
-  if (was_empty && fd_ >= 0) {
+  if (was_empty) {
     auto server = server_.lock();
-    if (server) {
+    if (fd_ >= 0 && server) {
       // Add EPOLLOUT interest - pipeline thread manages epoll
       struct epoll_event event;
       event.data.fd = fd_;
       event.events = EPOLLIN | EPOLLOUT;
       tsan_release();
+      // I think we have to call epoll_ctl while holding mutex_. Otherwise a
+      // call that clears the write interest could get reordered with one that
+      // sets it and we would hang.
       epoll_ctl(server->epoll_fds_[epoll_index_], EPOLL_CTL_MOD, fd_, &event);
     }
   }
@@ -133,9 +136,8 @@ int Connection::readBytes(char *buf, size_t buffer_size) {
     }
 
     // Increment bytes read metric
-    if (r > 0) {
+    assert(r > 0);
-      bytes_read.inc(r);
+    bytes_read.inc(r);
-    }
 
     return r;
   }
@@ -196,9 +198,7 @@ uint32_t Connection::write_bytes() {
         if (errno == EAGAIN) {
           // Increment EAGAIN failure metric
           write_eagain_failures.inc();
-          if (total_bytes_written > 0) {
+          bytes_written.inc(total_bytes_written);
-            bytes_written.inc(total_bytes_written);
-          }
           return result;
         }
         perror("sendmsg");
@@ -221,7 +221,6 @@ uint32_t Connection::write_bytes() {
 
       while (bytes_remaining > 0 && !message_queue_.empty()) {
         auto &front_message = message_queue_.front();
-        bool message_complete = true;
 
         for (auto &part : front_message.data_parts) {
           if (part.empty())
@@ -236,22 +235,17 @@ uint32_t Connection::write_bytes() {
             part = std::string_view(part.data() + bytes_remaining,
                                     part.size() - bytes_remaining);
             bytes_remaining = 0;
-            message_complete = false;
             break;
           }
         }
 
-        if (message_complete) {
+        if (front_message.close_after_send) {
-          if (front_message.close_after_send) {
+          result |= Close;
-            result |= Close;
+        }
-          }
+        // Move arena to thread-local vector for deferred cleanup
-          // Move arena to thread-local vector for deferred cleanup
+        g_arenas_to_free.emplace_back(std::move(front_message.arena));
-          g_arenas_to_free.emplace_back(std::move(front_message.arena));
+        message_queue_.pop_front();
-          message_queue_.pop_front();
+        if (result & Close) {
-          if (result & Close) {
-            break;
-          }
-        } else {
           break;
         }
       }
@@ -269,18 +263,19 @@ uint32_t Connection::write_bytes() {
         event.data.fd = fd_;
         event.events = EPOLLIN; // Remove EPOLLOUT
         tsan_release();
+        // I think we have to call epoll_ctl while holding mutex_. Otherwise a
+        // call that clears the write interest could get reordered with one that
+        // sets it and we would hang.
         epoll_ctl(server->epoll_fds_[epoll_index_], EPOLL_CTL_MOD, fd_, &event);
       }
     }
   }
 
   // Increment bytes written metric
-  if (total_bytes_written > 0) {
+  bytes_written.inc(total_bytes_written);
-    bytes_written.inc(total_bytes_written);
-  }
 
   // Clean up arenas after all mutex operations are complete
-  // This avoids holding the connection mutex while free() potentially contends
+  // This avoids holding the connection mutex while calling free()
   g_arenas_to_free.clear();
 
   return result;

src/connection.hpp

@@ -34,7 +34,9 @@ struct MessageSender {
    *
    * Thread-safe method that can be called from any thread, including
    * pipeline processing threads. The arena is moved into the connection
-   * to maintain data lifetime until the message is sent.
+   * to maintain data lifetime until the message is sent. Messages appended
+   * concurrently may be written in either order, but they will not be
+   * interleaved.
    *
    * @param data_parts Span of string_view parts to send (arena-allocated)
    * @param arena Arena containing the memory for data_parts string_views
@@ -55,20 +57,21 @@ struct MessageSender {
 };
 
 /**
- * Represents a single client connection with thread-safe concurrent access.
+ * Represents a single client connection - the full interface available to the
+ * io thread and connection handler.
  *
  * Connection ownership model:
  * - Server owns all connections
  * - Handlers receive Connection& references, and can keep a WeakRef to
- * Connection for async responses.
+ * MessageSender for async responses.
- * - Multiple pipeline threads can safely access connection concurrently
+ * - Multiple pipeline threads can safely access the MessageSender concurrently
  * - I/O thread has exclusive access to socket operations
  *
  * Threading model:
- * - Single mutex protects all connection state
+ * - Single mutex protects state shared with pipeline threads
  * - Pipeline threads call Connection methods (append_message, etc.)
  * - I/O thread processes socket events and message queue
- * - Pipeline threads manage epoll interests via Connection methods
+ * - Pipeline threads register epoll write interest via append_message
  * - Connection tracks closed state to prevent EBADF errors
  *
  * Arena allocator usage:
@@ -258,7 +261,7 @@ struct Connection : MessageSender {
    *
    * Example usage:
    * ```cpp
-   * class HttpHandler : public ConnectionHandler {
+   * class HttpHandler : ConnectionHandler {
    *   void on_connection_established(Connection& conn) override {
    *     // Allocate HTTP state in connection's arena or heap
    *     auto* state = conn.get_arena().construct<HttpConnectionState>();
@@ -272,8 +275,8 @@ struct Connection : MessageSender {
    *   }
    *
    *   void on_data_arrived(std::string_view data,
-   *                        Ref<Connection>& conn_ptr) override {
+   *                        Connection& conn) override {
-   *     auto* state = static_cast<HttpConnectionState*>(conn_ptr->user_data);
+   *     auto* state = static_cast<HttpConnectionState*>(conn.user_data);
    *     // Use state for protocol processing...
    *   }
    * };
@@ -323,28 +326,25 @@ private:
   };
   uint32_t write_bytes();
 
-  // Direct access methods for Server (must hold mutex)
-  int getFd() const { return fd_; }
-  bool has_messages() const { return !message_queue_.empty(); }
-  size_t getEpollIndex() const { return epoll_index_; }
   void close();
 
   // Immutable connection properties
-  int fd_;
   const int64_t id_;
   const size_t epoll_index_; // Index of the epoll instance this connection uses
   struct sockaddr_storage addr_; // sockaddr_storage handles IPv4/IPv6
   ConnectionHandler *const handler_;
-  WeakRef<Server> server_;       // Weak reference to server for safe cleanup
+  WeakRef<Server> server_; // Weak reference to server for safe epoll_ctl calls
   WeakRef<Connection> self_ref_; // WeakRef to self for get_weak_ref()
 
-  // Thread-safe state (protected by mutex_)
+  // state shared with pipeline threads (protected by mutex_)
   mutable std::mutex mutex_; // Protects all mutable state
   std::deque<Message>
-      message_queue_; // Queue of messages to send. Protectec by
+      message_queue_; // Queue of messages to send. Protected by
                       // mutex_, but if non-empty mutex_ can be
                       // dropped while server accesses existing elements.
   int64_t outgoing_bytes_queued_{0}; // Counter of queued bytes
+  // Set to a negative number in `close`
+  int fd_;
 
 #if __has_feature(thread_sanitizer)
   void tsan_acquire() { tsan_sync.load(std::memory_order_acquire); }

src/connection_handler.hpp

@@ -51,6 +51,7 @@ public:
    * @param conn Connection that made write progress - server retains ownership
    * @note Called from this connection's io thread.
    * @note Called during writes, not necessarily when buffer becomes empty
+   * TODO Add bytes written argument?
    */
   virtual void on_write_progress(Connection &) {}
 

src/connection_registry.cpp

@@ -13,6 +13,7 @@ ConnectionRegistry::ConnectionRegistry() : connections_(nullptr), max_fds_(0) {
   }
   max_fds_ = rlim.rlim_cur;
 
+  // TODO re-enable "ondemand pages" behavior
   // // Calculate size rounded up to page boundary
   // size_t array_size = max_fds_ * sizeof(Connection *);
   // size_t page_size = getpagesize();

src/connection_registry.hpp

@@ -34,10 +34,10 @@ public:
 
   /**
    * Store a connection in the registry, indexed by its file descriptor.
-   * Takes ownership of the connection via unique_ptr.
+   * Takes a reference to the connection for storage.
    *
    * @param fd File descriptor (must be valid and < max_fds_)
-   * @param connection unique_ptr to the connection (ownership transferred)
+   * @param connection Ref<Connection> to store in the registry
    */
   void store(int fd, Ref<Connection> connection);
 

src/http_handler.cpp

@@ -69,14 +69,17 @@ void HttpHandler::on_connection_closed(Connection &conn) {
   conn.user_data = nullptr;
 }
 
+// TODO there might be an issue if we get pipelined requests here
+
 void HttpHandler::on_write_buffer_drained(Connection &conn) {
-  // Reset state after all messages have been written for the next request
+  // Reset state after entire reply messages have been written for the next
+  // request
   auto *state = static_cast<HttpConnectionState *>(conn.user_data);
   if (state) {
     TRACE_EVENT("http", "reply",
                 perfetto::Flow::Global(state->http_request_id));
   }
-  // TODO we don't need this anymore. Look at removing it.
+  // TODO consider replacing with HttpConnectionState->reset()
   on_connection_closed(conn);
   // Note: Connection reset happens at server level, not connection level
   on_connection_established(conn);
@@ -109,7 +112,8 @@ void HttpHandler::on_batch_complete(std::span<Connection *const> batch) {
     }
   }
 
-  // Send requests to 4-stage pipeline in batch
+  // Send requests to 4-stage pipeline in batch. Batching here reduces
+  // contention on the way into the pipeline.
   if (pipeline_count > 0) {
     auto guard = commitPipeline.push(pipeline_count, true);
     auto out_iter = guard.batch.begin();
@@ -396,7 +400,7 @@ void HttpHandler::handle_get_metrics(Connection &conn,
   conn.append_message(result, std::move(state.arena));
 }
 
-void HttpHandler::handle_get_ok(Connection &, HttpConnectionState &state) {
+void HttpHandler::handle_get_ok(Connection &, HttpConnectionState &) {
   ok_counter.inc();
   TRACE_EVENT("http", "GET /ok", perfetto::Flow::Global(state.http_request_id));
 

src/http_handler.hpp

@@ -133,7 +133,7 @@ struct HttpHandler : ConnectionHandler {
   void on_connection_closed(Connection &conn) override;
   void on_data_arrived(std::string_view data, Connection &conn) override;
   void on_batch_complete(std::span<Connection *const> batch) override;
-  void on_write_buffer_drained(Connection &conn_ptr) override;
+  void on_write_buffer_drained(Connection &conn) override;
 
   // llhttp callbacks (public for HttpConnectionState access)
   static int onUrl(llhttp_t *parser, const char *at, size_t length);

src/metric.hpp

@@ -45,7 +45,6 @@
 
 #include <functional>
 #include <initializer_list>
-#include <memory>
 #include <span>
 #include <type_traits>
 #include <vector>

src/process_collector.hpp

@@ -8,7 +8,7 @@
  * Gathers metrics like CPU usage, memory, and file descriptors by reading
  * files from the /proc filesystem.
  */
-struct ProcessCollector : public metric::Collector {
+struct ProcessCollector : metric::Collector {
   /**
    * @brief Constructs the collector and initializes the process metrics.
    */

src/server.cpp

@@ -324,7 +324,7 @@ void Server::start_io_threads(std::vector<std::thread> &threads) {
         // Process existing connections in batch
         if (batch_count > 0) {
           process_connection_batch(
-              epollfd, std::span(batch).subspan(0, batch_count),
+              std::span(batch).subspan(0, batch_count),
               std::span(batch_events).subspan(0, batch_count));
         }
 
@@ -388,7 +388,7 @@ void Server::start_io_threads(std::vector<std::thread> &threads) {
             // Process batch if full
             if (batch_count == config_.server.event_batch_size) {
               process_connection_batch(
-                  epollfd, {batch.data(), (size_t)batch_count},
+                  {batch.data(), (size_t)batch_count},
                   {batch_events.data(), (size_t)batch_count});
               batch_count = 0;
             }
@@ -398,7 +398,7 @@ void Server::start_io_threads(std::vector<std::thread> &threads) {
         // Process remaining accepted connections
         if (batch_count > 0) {
           process_connection_batch(
-              epollfd, std::span(batch).subspan(0, batch_count),
+              std::span(batch).subspan(0, batch_count),
               std::span(batch_events).subspan(0, batch_count));
           batch_count = 0;
         }
@@ -463,10 +463,9 @@ void Server::close_connection(Ref<Connection> &conn) {
   conn.reset();
 }
 
-static thread_local std::vector<Connection *> conn_ptrs;
+static thread_local std::vector<Connection *> batch_connections;
 
-void Server::process_connection_batch(int epollfd,
+void Server::process_connection_batch(std::span<Ref<Connection>> batch,
-                                      std::span<Ref<Connection>> batch,
                                       std::span<const int> events) {
 
   // First process writes for each connection
@@ -484,19 +483,19 @@ void Server::process_connection_batch(int epollfd,
   }
 
   // Call batch complete handler with connection pointers
-  conn_ptrs.clear();
+  batch_connections.clear();
-  for (auto &conn_ref : batch) {
+  for (auto &conn : batch) {
-    if (conn_ref) {
+    if (conn) {
-      conn_ptrs.push_back(conn_ref.get());
+      batch_connections.push_back(conn.get());
     }
   }
-  handler_.on_batch_complete(conn_ptrs);
+  handler_.on_batch_complete(batch_connections);
 
-  // Transfer all remaining connections back to registry
+  // Return all connections to registry
-  for (auto &conn_ptr : batch) {
+  for (auto &conn : batch) {
-    if (conn_ptr) {
+    if (conn) {
-      int fd = conn_ptr->getFd();
+      const int fd = conn->fd_;
-      connection_registry_.store(fd, std::move(conn_ptr));
+      connection_registry_.store(fd, std::move(conn));
     }
   }
 }

src/server.hpp

@@ -1,7 +1,6 @@
 #pragma once
 
 #include <atomic>
-#include <memory>
 #include <span>
 #include <thread>
 #include <vector>
@@ -101,20 +100,21 @@ private:
    * Private constructor - use create() factory method instead.
    *
    * @param config Server configuration (threads, ports, limits, etc.)
-   * @param handler Protocol handler for processing connection data
+   * @param handler Protocol handler for processing connection data. Must
+   * outlive the server.
    * @param listen_fds Vector of file descriptors to accept connections on.
-   *                   Server takes ownership and will close them on
+   * Server takes ownership and will close them on
    * destruction. Server will set these to non-blocking mode for safe epoll
    * usage.
    */
   explicit Server(const weaseldb::Config &config, ConnectionHandler &handler,
                   const std::vector<int> &listen_fds);
-  friend Ref<Server> make_ref<Server>(const weaseldb::Config &config,
+  template <typename T, typename... Args>
-                                      ConnectionHandler &handler,
+  friend Ref<T> make_ref(Args &&...args);
-                                      const std::vector<int> &listen_fds);
+
   WeakRef<Server> self_;
 
-  const weaseldb::Config &config_;
+  weaseldb::Config config_;
   ConnectionHandler &handler_;
 
   // Connection registry
@@ -145,25 +145,15 @@ private:
   int get_epoll_for_thread(int thread_id) const;
 
   // Helper for processing connection I/O
-  void process_connection_reads(Ref<Connection> &conn_ptr, int events);
+  void process_connection_reads(Ref<Connection> &conn, int events);
-  void process_connection_writes(Ref<Connection> &conn_ptr, int events);
+  void process_connection_writes(Ref<Connection> &conn, int events);
 
   void close_connection(Ref<Connection> &conn);
 
   // Helper for processing a batch of connections with their events
-  void process_connection_batch(int epollfd, std::span<Ref<Connection>> batch,
+  void process_connection_batch(std::span<Ref<Connection>> batch,
                                 std::span<const int> events);
 
-  /**
-   * Called internally to return ownership to the server.
-   *
-   * This method is thread-safe and can be called from any thread.
-   * The connection will be re-added to the epoll for continued processing.
-   *
-   * @param connection Unique pointer to the connection being released back
-   */
-  void receiveConnectionBack(Ref<Connection> connection);
-
   // Make non-copyable and non-movable
   Server(const Server &) = delete;
   Server &operator=(const Server &) = delete;

style.md

@@ -21,7 +21,6 @@ ______________________________________________________________________
 
 - **C++20** is the target standard
 - Use modern C++ features: RAII, move semantics, constexpr, concepts where appropriate
-- Prefer standard library containers and algorithms over custom implementations
 
 ### C Library Functions and Headers
 
@@ -65,16 +64,16 @@ signal(SIGTERM, handler);
   - Interfacing with APIs that require unsigned types
   - Where defined unsigned overflow behavior (wraparound) is intentional and desired
 - **Almost always auto** - let the compiler deduce types except when:
-  - The type is not obvious from context (prefer explicit for clarity)
+  - The type is not obvious from context and the exact type is important (prefer explicit for clarity)
   - Specific type requirements matter (numeric conversions, template parameters)
   - Interface contracts need explicit types (public APIs, function signatures)
 - **Prefer uninitialized memory to default initialization** when using before initializing would be an error
   - Valgrind will catch uninitialized memory usage bugs
-  - Avoid hiding logic errors with unnecessary zero-initialization
+  - Avoid hiding logic errors that Valgrind would have caught with unnecessary zero-initialization
   - Default initialization can mask bugs and hurt performance
 - **Floating point is for metrics only** - avoid `float`/`double` in core data structures and algorithms
   - Use for performance measurements, statistics, and monitoring data
-  - Never use for counts, sizes, or business logic
+  - Avoid branching on the values of floats
 
 ### Type Casting
 
@@ -106,7 +105,7 @@ auto addr = reinterpret_cast<uintptr_t>(ptr);         // Pointer to integer conv
 - **Strive for 0% CPU usage when idle** - avoid polling, busy waiting, or unnecessary background activity
 - Use **inline functions** for performance-critical code (e.g., `allocate_raw`)
 - **String views** with `std::string_view` to minimize unnecessary copying
-- **Arena allocation** for efficient memory management (~1ns vs ~20-270ns for malloc)
+- **Arena allocation** for efficient memory management, and to group related lifetimes together for simplicity
 
 ### String Formatting
 
@@ -131,6 +130,8 @@ std::string_view response = format(arena,
     static_cast<int>(body.size()), body.data());
 ```
 
+- Offer APIs that let you avoid concatenating strings if possible - e.g. if the bytes are going to get written to a file descriptor you can skip concatenating and use scatter/gather writev-type calls.
+
 ### Complexity Control
 
 - **Encapsulation is the main tool for controlling complexity**
@@ -141,7 +142,7 @@ std::string_view response = format(arena,
   - Thread safety guarantees
   - Performance characteristics
   - Ownership and lifetime semantics
-- **Do not rely on undocumented interface properties** - if it's not in the header, don't depend on it
+- **Do not rely on undocumented properties of an interface** - if it's not in the header, don't depend on it
 
 ______________________________________________________________________
 
@@ -165,17 +166,16 @@ int32_t initial_block_size_;
 - **Public members first, private after** - puts the interface users care about at the top, implementation details below
 - **Full encapsulation still applies** - use `private:` sections to hide implementation details and maintain deep, capable structs
 - The struct keyword doesn't mean shallow design - it means interface-first organization for human readers
+- Omit the `public` keyword when inheriting from a struct. It's public by default. E.g. `struct A : B {};` instead of `struct A : public B {};`
 
 ```cpp
-struct Arena {
+struct MyClass {
   // Public interface first
-  explicit Arena(int64_t initial_size = 1024);
+  void do_thing();
-  void* allocate_raw(int64_t size);
 
 private:
   // Private members after
-  int32_t initial_block_size_;
+  int thing_count_;
-  Block* current_block_;
 };
 ```
 
@@ -183,6 +183,7 @@ private:
 
 - **PascalCase** for enum class names
 - **PascalCase** for enum values (not SCREAMING_SNAKE_CASE)
+- C-style enums are acceptable where implicit int conversion is desirable, like for bitflags
 
 ```cpp
 enum class Type {
@@ -270,7 +271,7 @@ ______________________________________________________________________
 
 - **Move-only semantics** for resource-owning types
 - **Explicit constructors** to prevent implicit conversions
-- **Delete copy operations** when inappropriate
+- **Delete copy operations** when copying is inappropriate or should be discouraged
 
 ```cpp
 struct Arena {
@@ -313,7 +314,7 @@ Arena(Arena &&source) noexcept;
 
 ### Factory Patterns & Ownership
 
-- **Static factory methods** for complex construction requiring shared ownership
+- **Static factory methods** for complex construction requirements like enforcing shared ownership
 - **Friend-based factories** for access control when constructor should be private
 - **Ownership guidelines:**
   - **unique_ptr** for exclusive ownership (most common case)
@@ -329,7 +330,8 @@ auto connection = Connection::createForServer(addr, fd, connection_id, handler,
 
 // Friend-based factory for access control
 struct Connection {
-  void append_message(std::string_view message_data);
+  WeakRef<MessageSender> get_weak_ref() const;
+
 private:
   Connection(struct sockaddr_storage client_addr, int file_descriptor,
              int64_t connection_id, ConnectionHandler* request_handler,
@@ -382,7 +384,7 @@ ______________________________________________________________________
 
 ### Ownership & Allocation
 
-- **Arena allocators** for request-scoped memory with **STL allocator adapters** (see Performance Focus section for characteristics)
+- **Arena** for request-scoped memory with **STL allocator adapters**
 - **String views** pointing to arena-allocated memory to avoid unnecessary copying
 - **STL containers with arena allocators require default construction after arena reset** - `clear()` is not sufficient
 
@@ -425,7 +427,7 @@ ______________________________________________________________________
 - **Error codes are the API contract** - use enums for programmatic decisions
 - **Error messages are human-readable only** - never parse message strings
 - **Consistent error boundaries** - each component defines what it can/cannot recover from
-- **Interface precondition violations are undefined behavior** - acceptable to skip checks for performance in hot paths
+- **Interface precondition violations are undefined behavior** - it's acceptable to skip checks for performance in hot paths
 - **Error code types must be nodiscard** - mark error code enums with `[[nodiscard]]` to prevent silent failures
 
 ```cpp
@@ -439,7 +441,7 @@ if (!memory) {
 }
 // ... use memory, eventually std::free(memory)
 
-// Programming error - precondition violation (may be omitted for performance)
+// Programming error - precondition violation (gets compiled out in release builds)
 assert(ptr != nullptr && "Precondition violated: pointer must be non-null");
 ```
 
@@ -546,7 +548,7 @@ T *realloc(T *existing_ptr, int32_t current_size, int32_t requested_size);
 
 ### Code Comments
 
-- **Explain why, not what** - code should be self-documenting
+- **Explain why, not what** - *what* the code does should be clear without any comments
 - **Performance notes** for optimization decisions
 - **Thread safety** and ownership semantics
 
@@ -573,7 +575,7 @@ ______________________________________________________________________
 ### Test Structure
 
 - **Descriptive test names** explaining the scenario
-- **SUBCASE** for related test variations
+- **SUBCASE** for related test variations that share setup/teardown code
 - **Fresh instances** for each test to avoid state contamination
 
 ```cpp
@@ -600,25 +602,14 @@ TEST_CASE("Arena basic allocation") {
 - **Prefer fakes to mocks** - use real implementations for internal components, fake external dependencies
 - **Always enable assertions in tests** - use `-UNDEBUG` pattern to ensure assertions are checked (see Build Integration section)
 
+TODO make a new example here using APIs that exist
+
 ```cpp
-// Good: Testing through public API
-TEST_CASE("Server accepts connections") {
-  auto config = Config::defaultConfig();
-  auto handler = std::make_unique<TestHandler>();
-  auto server = Server::create(config, std::move(handler));
-
-  // Test observable behavior - server can accept connections
-  auto result = connectToServer(server->getPort());
-  CHECK(result.connected);
-}
-
-// Avoid: Testing internal implementation details
-// TEST_CASE("Server creates epoll instance") { /* implementation detail */ }
 ```
 
 ### What NOT to Test
 
-**Avoid testing language features and plumbing:**
+**Avoid testing language features:**
 
 - Don't test that virtual functions dispatch correctly
 - Don't test that standard library types work (unique_ptr, containers, etc.)
@@ -716,8 +707,9 @@ cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
 ```cmake
 # Test target with assertions always enabled
 add_executable(test_example tests/test_example.cpp src/example.cpp)
-target_link_libraries(test_example doctest::doctest)
+target_link_libraries(test_example doctest_impl)
 target_compile_options(test_example PRIVATE -UNDEBUG)  # Always enable assertions
+add_test(NAME test_example COMMAND test_example)
 
 # Production target follows build type
 add_executable(example src/example.cpp src/main.cpp)

tests/test_reference.cpp

@@ -28,14 +28,14 @@ struct Base {
   virtual int get_value() const { return base_value; }
 };
 
-struct Derived : public Base {
+struct Derived : Base {
   int derived_value;
   explicit Derived(int base_v, int derived_v)
       : Base(base_v), derived_value(derived_v) {}
   int get_value() const override { return base_value + derived_value; }
 };
 
-struct AnotherDerived : public Base {
+struct AnotherDerived : Base {
   int another_value;
   explicit AnotherDerived(int base_v, int another_v)
       : Base(base_v), another_value(another_v) {}
@@ -56,7 +56,7 @@ struct Interface2 {
 };
 
 // Multiple inheritance - this will cause pointer address changes
-struct MultipleInheritance : public Interface1, public Interface2 {
+struct MultipleInheritance : Interface1, Interface2 {
   int own_data;
   explicit MultipleInheritance(int data) : own_data(data) {}
   int get_own_data() const { return own_data; }