refactor(server): stop branching on errno after accept/recv/send

CLAUDE.md

@@ -70,3 +70,15 @@ Dispatch enforces a **registration gate**: only CAP/PASS/NICK/USER/QUIT/PONG run
 ## Testing
 
 Tests use Google Test but also feed every result into **PostMan** (`vendor/PostMan.cpp`), a styled Unicode-table reporter — `tests/test_main.cpp` bridges the two via a custom `TestEventListener`. `tests/Makefile` builds all of `src/` *except* `main.cpp` (linking `tier_full.cpp` as the one `registerExtensions` definition). Protocol-level suites share `tests/TestHarness.hpp` (TCP `TestClient` + `IrcServerTest` fixture; subclass and override `portBase()` per suite, `onServerReady()` to inject probe extensions). Test files: `test_message`, `test_client`, `test_channel`, `test_bot`, `test_integration`, `test_robustness`, `test_security`, `test_filetransfer`, `test_extensions`, `test_libcpp98`. Suite is 138/138; PostMan's leak counter is atomic and `assertNoLeaks` takes `const char*` (a `std::string` argument would count itself as a leak — keep it that way).
+
+## Known traps
+
+- **RST/error teardown**: real socket errors (`ECONNRESET`, etc.) are torn down
+  via the `EPOLLERR|EPOLLHUP` branch in `run()`, **not** by inspecting `errno`
+  after `recv`/`send` — that was removed (the subject forbids errno-driven
+  control flow after non-blocking I/O syscalls; it also cured a latent EINTR
+  false-positive disconnect). Caveat for future event-loop work: which branch
+  reaps an RST is **timing-dependent** when `EPOLLIN` and `EPOLLHUP` arrive in
+  the same event — do NOT assume `EPOLLERR|HUP` alone covered RST before that
+  errno removal (`RobustnessTest.AbruptDisconnectViaRST` guards it now).
+

src/Server.cpp

@@ -230,11 +230,7 @@ void Server::acceptClient()
 						  reinterpret_cast<struct sockaddr *>(&clientAddr),
 						  &addrLen);
 	if (clientFd < 0)
-	{
-		if (errno != EAGAIN && errno != EWOULDBLOCK)
-			Log::error(std::string("accept() failed: ") + strerror(errno));
 		return;
-	}
 
 	// Connection cap: reject gracefully instead of exhausting fds
 	if (_clients.size() >= MAX_CLIENTS)
@@ -286,7 +282,7 @@ void Server::handleClientInput(int fd)
 	ssize_t bytesRead = recv(fd, buf, MAX_MSGLEN, 0);
 	if (bytesRead <= 0)
 	{
-		if (bytesRead == 0 || (errno != EAGAIN && errno != EWOULDBLOCK))
+		if (bytesRead == 0)
 			disconnectClient(fd, "Connection closed");
 		return;
 	}
@@ -322,11 +318,7 @@ void Server::handleClientOutput(int fd)
 	const std::string &buf = client->getSendBuffer();
 	ssize_t bytesSent = send(fd, buf.c_str(), buf.size(), 0);
 	if (bytesSent < 0)
-	{
-		if (errno != EAGAIN && errno != EWOULDBLOCK)
-			disconnectClient(fd, "Send error");
 		return;
-	}
 	client->clearSendBuffer(bytesSent);
 
 	// SendQ sweep: the peer is too slow / flooded; its stream already

tests/COVERAGE.md

@@ -0,0 +1,107 @@
+# ft_irc — Defense Coverage Map (42 correction sheet)
+
+Cross-references every item on the 42 evaluation sheet against what the repo
+actually proves today. Built from a scan of `tests/`, `scripts/audit.sh`, and
+`src/` on `main` (`4e78d69`).
+
+**Legend:** ✅ covered · 🟡 partial · 🔴 gap / risk · 🧭 manual-only (not automatable)
+
+Defend on the **`make mandatory`** binary — it excludes PlatformBus / AuditLog /
+FancyLogSink, shrinking the surface an evaluator can question.
+
+---
+
+## A. Basic checks (ELIMINATORY — any failure = grade 0)
+
+| Sheet item | Status | Proven by | Notes / gap |
+|---|---|---|---|
+| Makefile; compiles `-Wall -Wextra -Werror -std=c++98`; exec `ircserv` | ✅ | `audit.sh` §compile | Also `make mandatory/bonus/full` all build |
+| **Only one** poll/epoll/select | ✅ | `audit.sh` §single-poll; verified 1 `epoll_wait` (Server.cpp:158) | PlatformBus multiplexes into the same epoll — no 2nd wait |
+| poll called before each accept/recv/send | 🟡 | architectural (all I/O is epoll-event-driven) | **Exception:** best-effort `send()` in `disconnectClient` (Server.cpp:503) is not preceded by a write event → decide defend vs remove |
+| **errno not used to trigger action after recv/send/accept** | 🔴 | — (unaudited, untested) | **TOP RISK.** `errno==EAGAIN` checks after recv (289), send (326), accept (234). No retry-loop, but errno branches control flow. Refactor recommended (EPOLLERR/HUP already handles real errors) |
+| fcntl only `F_SETFL, O_NONBLOCK` | ✅ | `audit.sh` §fcntl; verified (Server.cpp:104,250) | PlatformBus fcntl also compliant, but it's an extra |
+
+---
+
+## B. Networking
+
+| Sheet item | Status | Proven by | Notes / gap |
+|---|---|---|---|
+| Starts, listens on all interfaces on CLI port | 🟡 | integration binds+connects | "All interfaces" (INADDR_ANY) is a code fact; not asserted, easy to defend |
+| `nc` connects, sends, gets answers | ✅ | `IntegrationTest.*` (raw TCP `TestClient` = nc-equivalent) | |
+| Reference client (HexChat) connects | 🧭 | — | **Manual before defense.** Not automatable |
+| Multiple simultaneous connections, non-blocking | ✅ | `Robustness.RapidConnectDisconnect`, `CommandFlood`; `EventLoopTest` (idle no-spin) | Strong |
+| Join channel; broadcast to all members | ✅ | `Integration.JoinChannel/ChannelMessage`; `Channel.BroadcastToAllMembers/ExcludesSender` | |
+
+---
+
+## C. Networking specials (robustness)
+
+| Sheet item | Status | Proven by | Notes / gap |
+|---|---|---|---|
+| Partial commands; others keep running | 🟡 | `Integration.PartialDataReassembly`; `ClientBuffer.PartialMessagePreserved`; `LineBuffer98.FragmentedCRLF` | Reassembly proven; "other clients fine *while* a partial is pending" not explicitly interleaved |
+| Kill client abruptly; server stays up | ✅ | `Robustness.AbruptDisconnect`, `RapidConnectDisconnect` | |
+| Kill nc mid-command | 🟡 | `Robustness.AbruptDisconnect` | Not specifically "disconnect while a partial is buffered" |
+| **^Z a reader + flood from another; no hang; drains on resume; no leaks** | 🔴 | partial: `SendQ.OverflowLatches`, `CommandFlood`, `NoLeakAfterClientChurn` | **GAP.** No integration test of a *frozen reader* being flooded (output backpressure). Directly tied to the EPOLLOUT/SENDQ design — high-value test |
+| No memory leaks during operations | 🟡 | in-process leak counter (`NoLeak*`, PostMan) + `scripts/memcheck.sh` exists | Eval uses **valgrind**; in-process counting ≠ valgrind. Verify `memcheck.sh` runs the sheet's scenarios (esp. ^Z+flood) under valgrind |
+
+---
+
+## D. Client commands (basic)
+
+| Sheet item | Status | Proven by | Notes / gap |
+|---|---|---|---|
+| Auth, NICK, USER, JOIN | ✅ | `Integration.SuccessfulRegistration/WrongPassword/NoPassword/JoinChannel` | |
+| PRIVMSG with different parameters | 🟡 | `Integration.PrivateMessage/ChannelMessage` | Add error cases: no such nick (401), no text (412), multi-target |
+
+---
+
+## E. Channel operator commands (scored 0–5, −1 per broken feature)
+
+| Sheet item | Status | Proven by | Notes / gap |
+|---|---|---|---|
+| Operator CAN: KICK / INVITE / TOPIC / MODE i,t,k,o,l | ✅ | `Integration.KickUser/InviteToChannel/TopicSetAndQuery/ChannelMode{Query,Key,Limit}`; `Channel.*Mode*`; `ModeBounds*` | Happy path well covered |
+| **Regular user is DENIED operator actions** | 🔴 | — | **GAP.** Sheet explicitly checks this. No negative test asserting `ERR_CHANOPRIVSNEEDED (482)` for non-op KICK/TOPIC/MODE/INVITE |
+| +i / +t enforced end-to-end (uninvited JOIN blocked; non-op TOPIC blocked) | 🟡 | mode *state* tested at unit level | Enforcement path not asserted end-to-end via TCP |
+
+---
+
+## F. Bonus (only if mandatory is perfect)
+
+| Sheet item | Status | Proven by | Notes / gap |
+|---|---|---|---|
+| File transfer with reference client | 🟡 | `FileTransfer.*` (10 tests, incl. DCC relay) | Unit coverage strong; "with reference client" is 🧭 manual |
+| A bot | ✅ | `Bot.*` (10 tests) | |
+
+---
+
+## G. Overarching (checked continuously during defense)
+
+| Sheet item | Status | Notes |
+|---|---|---|
+| No segfault / crash for the whole defense | 🟡 | Robustness suite helps; ultimately live + valgrind |
+| No leaks (heap freed before exit) | 🟡 | See C — needs a valgrind harness as a defense artifact |
+
+---
+
+## Prioritized action list (defense order)
+
+**P0 — eliminatory, do first (a gap here = 0, everything else is moot):**
+1. **errno-after-I/O** (A). Audit → refactor recv/send/accept to not branch on
+   `errno` (rely on `EPOLLERR|EPOLLHUP`), or prepare an airtight justification.
+   Guard: `Robustness.AbruptDisconnect` must stay green.
+2. **Un-polled `send()` in `disconnectClient`** (A/B). Decide: remove (accept
+   losing app-buffered QUIT/ERROR text) or defend as a teardown flush.
+
+**P1 — scored / high-value robustness:**
+3. **Non-operator denial tests** (E) — the operator score is 0–5 and the sheet
+   checks denial explicitly. Add `ERR_CHANOPRIVSNEEDED` negative tests.
+4. **Frozen-reader + flood** integration test (C) — the marquee robustness
+   scenario; showcases the EPOLLOUT/SENDQ backpressure design.
+5. **Valgrind harness** (C/G) — verify/extend `scripts/memcheck.sh` to run the
+   sheet's scenarios under valgrind; keep the output as a defense artifact.
+
+**P2 — completeness / polish:**
+6. Re-land `test_eventloop.cpp` (idle no-spin) — merged fix, missing its guard.
+7. +i/+t end-to-end enforcement tests (E); PRIVMSG error cases (D).
+8. **Manual reference-client (HexChat) checklist** (B/F) — the 🧭 items.

tests/TESTING.md

@@ -0,0 +1,161 @@
+# Testing & QA Discipline
+
+How this project turns "I fixed it" into "a test would catch it if it ever
+came back." This document is the QA companion to `WORKFLOW.md`: the workflow
+defines *how a change moves through audit → plan → review*; this defines *what
+evidence a change must carry* and *how tests are written, validated, and kept
+honest*.
+
+It is a **discipline, not a platform.** It rides on what already exists — the
+GoogleTest suite under `tests/` and the three-agent workflow — and deliberately
+adds no new framework, runner, or "QA subsystem." If you find yourself building
+infrastructure to support QA, stop: that is scope creep, and this project
+already carries enough of it.
+
+---
+
+## 1. Principle: a test is evidence, not decoration
+
+A bug fix is not done when the symptom disappears. It is done when a test
+exists that **fails on the broken code and passes on the fixed code**. That
+discrimination — red before, green after — is the only thing that proves the
+test actually exercises the bug rather than passing by accident.
+
+The corollary is uncomfortable and load-bearing: a green test proves nothing
+on its own. A test that was never seen to fail might be asserting the wrong
+thing, hitting the wrong code path, or be a tautology. Every regression test
+must have a recorded red state.
+
+---
+
+## 2. The Red-Green-Review loop
+
+This layers directly onto the three roles in `WORKFLOW.md`. The mapping is what
+makes it bias-resistant: no single agent both writes the test and decides it
+passes.
+
+| Stage | Role (from WORKFLOW.md) | Responsibility |
+|-------|-------------------------|----------------|
+| **Red** | Auditor | Reproduce the bug *as a failing test* in `tests/`, and show it failing. The test is the audit's reproduction made executable. |
+| **Green** | Implementer | Make the test pass — **without modifying the test** — while keeping the whole suite green. |
+| **Review** | Adversarial reviewer | Independently validate the *test itself* as much as the code. |
+
+The hard rule that prevents gaming: **the author of the fix may not edit,
+weaken, or delete the test.** In a single-agent loop, TDD quietly degrades into
+"write the test my code already passes." With separated roles, the implementer
+inherits a test they cannot touch and must satisfy it honestly.
+
+The Red test is diagnosis-as-code, so writing it does not break the auditor's
+"diagnose, do not fix" posture — a failing test is not a fix.
+
+---
+
+## 3. Not every change is Red-Green
+
+Forcing a failing-test-first ritual onto work that has no behavioral bug
+produces awkward, dishonest tests. Match the evidence to the kind of work:
+
+- **Bug / regression** → Red-Green. A test that reproduces the defect, shown
+  failing, then passing. This is the default and the strongest case.
+- **Refactor** (behavior must not change) → the existing suite stays green
+  before and after. If coverage of the touched area is thin, *add
+  characterization tests first* (capturing current behavior) so the refactor
+  has a safety net. No new "failing" test is expected.
+- **Performance / resource fix** (no wrong output, just wrong cost — e.g. the
+  idle-CPU spin) → a measured **before/after number** in the PR, plus a guard
+  test that asserts the resource property with a **generous margin** (see §5).
+- **New feature** → tests for the new behavior, including its failure and edge
+  cases. Red-Green applies per acceptance criterion where it fits.
+
+If you cannot write a reliable failing test for a bug, treat that as a signal
+that the bug is not yet understood — keep auditing, or document explicitly why
+a deterministic test is not achievable (e.g. a genuine heisenbug).
+
+---
+
+## 4. How tests are written here
+
+Match the existing style in `tests/` rather than inventing a parallel one:
+
+- GoogleTest, built and run via `tests/Makefile` (`make` in `tests/`). The
+  server is C++98; the test tier compiles at C++17 as GoogleTest requires.
+- Protocol-level tests use the shared harness in `tests/TestHarness.hpp`:
+  - `TestClient` — a real TCP client (connect, `sendCmd`, `recvAll`,
+    `registerClient`, `hasNumeric`).
+  - `IrcServerTest` — a fixture that runs a `Server` in a background thread.
+    Override `portBase()` with a unique base per suite (grep existing bases to
+    avoid bind clashes) and `onServerReady(Server&)` to inject test-only
+    extensions before `run()`.
+- Register every new test file in `TEST_SRCS` in `tests/Makefile`.
+- Put suite-local helpers (fake extensions, etc.) in an anonymous `namespace`.
+- **Assert observable behavior, not structure.** Drive the server through its
+  real interface (bytes on a socket, replies, numerics, loop activity via the
+  extension seam) and assert on what an actual client or operator would see.
+  "The object exists" / "the count is 1" proves existence, not behavior.
+
+Prefer reusing the architecture's own seams over adding production hooks for
+testing. (The idle-spin regression test counts event-loop iterations through
+the existing `IServerExtension::onTick` seam — it adds zero production code.)
+
+---
+
+## 5. Keeping the suite trustworthy
+
+Flaky and slow tests are how a suite dies: people start ignoring red. Defend
+against it:
+
+- **Prefer deterministic signals over timing or %CPU.** Count iterations,
+  inspect state, assert on returned bytes — not wall-clock thresholds.
+- **When a threshold is unavoidable, demand a huge margin and document it.**
+  (The idle-spin guard separates ~1–2 iterations/sec from >100000 with a
+  threshold of 1000 — six orders of magnitude of headroom.)
+- **Slow, stress, and load tests live in a separate target** (e.g. a `stress`
+  target), never in the default suite. Add them only when a concrete risk
+  justifies them — not speculatively.
+- The default suite must stay fast and green. A red default suite means stop
+  and fix, not "known flaky, ignore."
+
+---
+
+## 6. The reviewer's test-validity checklist
+
+In addition to reviewing the code (see the `adversarial-reviewer` skill), the
+reviewer must vet the **test** and record the result:
+
+- [ ] **Discrimination, reproduced independently.** The reviewer ran the test
+  against the unfixed code and saw it FAIL, and against the fixed code and saw
+  it PASS — not trusting the author's numbers.
+- [ ] **Right reason.** The red failure is caused by the actual bug, not by an
+  unrelated error (compile failure, wrong port, timeout).
+- [ ] **Behavioral, not structural.** The assertion reflects what a user or
+  operator observes.
+- [ ] **No tautology / no false positive.** The test can actually fail; it is
+  not asserting something always true.
+- [ ] **Margin and runtime.** Any threshold has ample headroom; the test does
+  not meaningfully slow the default suite, or it lives in a separate target.
+- [ ] **Untouched by the fix.** The implementer did not modify the test to make
+  it pass.
+
+A change with code that looks correct but a test that fails this checklist is
+**APPROVE WITH CHANGES** at best — an unverified test is not a regression guard.
+
+---
+
+## 7. QA as an ongoing loop
+
+Beyond single fixes, the same discipline scales into a repeatable habit for the
+project: **audit a component → reproduce a weakness as a failing test → fix →
+have the reviewer validate both → fold the lesson into `CLAUDE.md`.** Each pass
+leaves the suite stronger and the institutional memory (the "known traps" in
+`CLAUDE.md`) richer.
+
+This loop is intentionally low-tech. It is the existing suite plus the existing
+three-agent workflow plus the rules above — nothing to install, nothing to
+maintain. Resist the temptation to grow it into a QA product. Its value is the
+discipline, not the tooling.
+
+---
+
+*Referenced from `WORKFLOW.md`. The method is portable; adapt it. If the rules
+get in the way more than they help on a given change, you are applying them to
+the wrong kind of work — see the taxonomy in §3.*

tests/test_robustness.cpp

@@ -144,6 +144,61 @@ TEST_F(RobustnessTest, AbruptDisconnect)
 	close(fd2);
 }
 
+TEST_F(RobustnessTest, AbruptDisconnectViaRST)
+{
+	/* Connect, register, then force a real RST (not a FIN) via SO_LINGER */
+	int fd = quickConnect(serverPort);
+	ASSERT_GE(fd, 0);
+
+	const std::string nick = "abruptrst";
+	sendLine(fd, "PASS robpass");
+	sendLine(fd, "NICK " + nick);
+	sendLine(fd, "USER abruptrst 0 * :Test");
+	std::this_thread::sleep_for(std::chrono::milliseconds(200));
+	recvBuf(fd);
+
+	struct linger sl;
+	sl.l_onoff = 1;
+	sl.l_linger = 0;
+	setsockopt(fd, SOL_SOCKET, SO_LINGER, &sl, sizeof(sl));
+	close(fd);
+
+	/*
+	 * Black-box proof the reset client was actually torn down server-side
+	 * (not just that the listener still accepts): register a NEW client
+	 * reusing the SAME nick. Nick release depends on the server processing
+	 * EPOLLERR|HUP for the RST, which is not synchronous with our close()
+	 * — bounded retry instead of a fixed sleep: up to 15 attempts, 100ms
+	 * apart (1.5s total margin). Each attempt reconnects on a fresh socket
+	 * and sends a full PASS/NICK/USER handshake, so the test makes no
+	 * assumption about partial-registration or NICK-only retry semantics
+	 * — success is simply seeing RPL_WELCOME (001) for that attempt.
+	 */
+	const int MAX_ATTEMPTS = 15;
+	bool registered = false;
+	std::string lastReply;
+
+	for (int attempt = 0; attempt < MAX_ATTEMPTS && !registered; ++attempt)
+	{
+		int fd2 = quickConnect(serverPort);
+		ASSERT_GE(fd2, 0);
+
+		sendLine(fd2, "PASS robpass");
+		sendLine(fd2, "NICK " + nick);
+		sendLine(fd2, "USER abruptrst2 0 * :Test");
+		std::this_thread::sleep_for(std::chrono::milliseconds(100));
+		lastReply = recvBuf(fd2);
+
+		registered = lastReply.find(" 001 ") != std::string::npos;
+		close(fd2);
+	}
+
+	EXPECT_TRUE(registered)
+		<< "Nick '" << nick << "' should be released after RST within "
+		<< MAX_ATTEMPTS << " retries (100ms apart); last reply: '"
+		<< lastReply << "'";
+}
+
 /* ════════════════════════════════════════════════════════════════════════
  * Suite: Robustness — Rapid connect / disconnect
  * ════════════════════════════════════════════════════════════════════ */