Audit: State Machine Refactoring Commits

[bettercallsaulj]

Audit: State Machine Refactoring Commits (#226)

Audit of 14 commits on branch fix/follow_state_machine against the
gopher-mcp design rules (CLAUDE.md, project memory, and codebase conventions).

Audit Scope

• 14 commits (77f746f..0082902)
• 14 files changed, +3849 / -195 lines
• 81 new test cases, 128/128 pass

Rule-by-Rule Compliance

Rule 1: State Machine Pattern

Always use state machine if applicable to avoid complex manual error-prone
state management. Do not track state with ad-hoc bool/flag combinations.

Check	Result
waiting_for_sse_endpoint_ removed	PASS -- replaced by client_sse_sm_->isNegotiating()
use_sse_ (filter member) removed	PASS -- replaced by state machine initial state
is_sse_mode_ removed	PASS -- replaced by isSseActive()
sse_server_mode_ removed	PASS -- replaced by server_mode_->isSseStream()
sse_writing_handshake_ removed	PASS -- replaced by RAII HandshakeWriteGuard
sse_headers_written_ removed	PASS -- replaced by server_mode_->sseHeadersWritten()
ClientSseStateMachine uses enum-class states	PASS -- ClientSseState with 8 values
ServerConnectionMode uses enum-class modes	PASS -- ServerConnMode with 6 values
Transition matrices defined and validated	PASS -- unordered_map<State, unordered_set<State>>
Invalid transitions rejected with reason	PASS -- returns Failure(error_message)

Remaining booleans in filter (justified, not state tracking):

• client_accepts_sse_ -- HTTP Accept header capability flag. Set once from
  headers, never used for control flow branching on state transitions. Not
  lifecycle state.
• use_sse -- constructor parameter (config input). Determines initial state
  machine state (Idle vs StreamableHttp), then never read again.

Sub-state booleans inside ServerConnectionMode (justified):

• handshake_write_depth_ (int) -- ref-counted RAII guard, not lifecycle state.
  Scoped to call frames via HandshakeWriteGuard. Cannot outlive the guard.
• sse_headers_written_ (bool) -- monotonic flag (false->true, never back).
  Not a lifecycle transition; tracks a one-time action within SseStream mode.
• transition_in_progress_ (bool) -- reentrancy guard, standard pattern shared
  with all existing state machines (SseCodecStateMachine, HttpCodecStateMachine,
  ConnectionStateMachine).

Verdict: PASS -- all lifecycle-tracking booleans replaced with state machines.

Rule 2: Dispatcher-Thread Confinement

All callbacks are invoked in dispatcher thread context. Transitions must
happen in the dispatcher thread. No synchronization primitives needed.

Check	Result
ClientSseStateMachine state: plain enum (not atomic)	PASS
ServerConnectionMode mode: plain enum (not atomic)	PASS
assertInDispatcherThread() on handleEvent()	PASS -- both machines
assertInDispatcherThread() on addStateChangeListener()	PASS -- both
assertInDispatcherThread() on clearStateChangeListeners()	PASS -- both
assertInDispatcherThread() on addTransitionValidator()	PASS -- client
assertInDispatcherThread() on forceTransition()	PASS -- client
assertInDispatcherThread() on beginHandshakeWrite()	PASS -- server
assertInDispatcherThread() on endHandshakeWrite()	PASS -- server
No std::mutex in either state machine	PASS
No std::atomic for state in either machine	PASS
Timer callbacks run in dispatcher thread	PASS -- dispatcher_.createTimer() guarantees this
Deferred transitions via dispatcher_.post()	PASS -- used for reentrancy guard

Verdict: PASS -- fully dispatcher-thread confined, zero synchronization primitives.

Rule 3: Per-Connection Ownership (Factory/Manager Pattern)

Each connection gets its own filter-chain instance. Filter instances are
never shared across connections.

Check	Result
client_sse_sm_ owned by filter via unique_ptr	PASS
server_mode_ owned by filter via unique_ptr	PASS
State machines constructed per-filter in constructor	PASS -- if (!is_server_) / if (is_server_)
No static or singleton state machine instances	PASS
No shared_ptr to state machines	PASS
Filter created per-connection by HttpSseFilterChainFactory::createFilterChain()	PASS

Verdict: PASS -- strict per-connection ownership.

Rule 4: MCP Abstraction Layers

Always use MCP abstraction layers of buffer, event loop, network, json.
Don't build raw ones.

Check	Result
Negotiation timer: dispatcher_.createTimer()	PASS
No setitimer(), timer_create(), timerfd_create()	PASS
All buffers use OwnedBuffer	PASS
No std::vector<char> for buffer operations	PASS
Deferred execution: dispatcher_.post()	PASS
No raw std::thread	PASS
Tests use event::createLibeventDispatcherFactory()	PASS

Verdict: PASS -- all infrastructure through MCP abstractions.

Rule 5: No Legacy Imports

Don't refer to anything from http_transport_socket.h, which is legacy.

Check	Result
grep -rn http_transport_socket on all new/modified files	0 matches

Verdict: PASS

Rule 6: RAII for Resource Management

Use RAII. Prefer smart pointers for resource management.

Check	Result
HandshakeWriteGuard RAII class for write reentrancy guard	PASS
Guard uses constructor/destructor pair (begin/end)	PASS
Guard ref-counted (nested guards safe)	PASS
Negotiation timer owned by state machine lifetime	PASS
State machines owned by unique_ptr in filter	PASS
Old sse_writing_handshake_ = true/false manual pattern removed	PASS
No manual flag set/clear patterns remain	PASS

Verdict: PASS -- all resource management through RAII.

Rule 7: Comments Explain WHY

Always add good comment and/or flow to explain why.

Check	Result
ClientSseState -- all 8 values have multi-line comments	PASS
ClientSseEvent -- all 7 values have comments	PASS
ServerConnMode -- all 6 values have multi-line comments	PASS
ServerConnEvent -- all 6 values have comments	PASS
Transition matrix entries grouped with section comments	PASS
isSseActive() explains client vs server routing logic	PASS
WaitingForEndpoint -> Active transition explains header/body ordering	PASS
HandshakeWriteGuard class docstring explains the reentrancy problem	PASS
Connection fix explains the premature-write-event race condition	PASS

Verdict: PASS -- thorough explanatory comments throughout.

Rule 8: No "envoy" in Source Code

Follow Envoy proxy's architecture and design, but don't add envoy keyword
to commit message and source code.

Check	Result
grep -rni envoy on all new headers and sources	0 matches
grep -rni envoy on all new test files	0 matches
git log --format=%B on all 14 commits	0 matches

Verdict: PASS

Rule 9: No AI Keywords

Don't add any AI code agent related keywords to commit title/message,
branch name, etc.

Check	Result
grep -rni "claude|prompt|Co-Authored-By|Generated with" on all source files	0 matches
Commit titles: no feat:, fix:, doc:, refactor: prefixes	PASS
Commit messages: no "Claude", "AI", "agent", "copilot"	PASS
Branch name fix/follow_state_machine: no prohibited prefix pattern	PASS

Verdict: PASS

Rule 10: Event Model

Create -> Add callbacks -> Connect -> Read/Write -> Close, all transitions
should happen in dispatcher thread.

Check	Result
ClientSseStateMachine lifecycle: Idle -> WaitingForGetSent -> WaitingForEndpoint -> Active -> Closed	PASS -- follows Create->Connect->Read/Write->Close pattern
ServerConnectionMode lifecycle: Undetermined -> {mode} -> Closed	PASS -- follows Create->Connect->Close pattern
State machine events fired from filter callbacks (onNewConnection, onWrite, onHeaders, onEvent)	PASS -- all in dispatcher thread
ConnectionImpl fix: Write events not enabled before connect()	PASS -- prevents premature onWriteReady before connect()
Deferred timer cleanup in onConnectionTimeout via dispatcher_.post()	PASS -- prevents destroying timer from its own callback

Verdict: PASS -- event model strictly followed.

---

Per-Commit Audit

Commit 1: 77f746f8 -- Add ClientSseStateMachine

• 8-state enum with comments: PASS
• Transition matrix validated: PASS
• Timer via dispatcher_.createTimer(): PASS
• No mutex/atomic: PASS
• No envoy/AI keywords: PASS

Commit 2: 3ef057b3 -- Unit tests for ClientSseStateMachine

• Real libevent dispatcher (no mocks): PASS
• No raw socket mocks: PASS (standalone state machine tests, no sockets needed)
• 35 test cases covering all transitions, timeouts, callbacks: PASS

Commit 3: b9e6620e -- Wire state machine as shadow

• State machine events fired alongside boolean mutations: PASS
• No behavioral change (booleans still control): PASS
• Safe incremental integration: PASS

Commit 4: 01e7eb3b -- Replace boolean reads, remove booleans

• waiting_for_sse_endpoint_ removed: PASS
• use_sse_ (filter member) removed: PASS
• State machine queries replace all boolean reads: PASS
• Existing tests pass: PASS

Commit 5: c821d431 -- Wire negotiation timeout

• error_callback calls mcp_callbacks_.onError(): PASS
• pending_messages_ drained on error: PASS
• Fixes silent-hang bug: PASS

Commit 6: 6b843f48 -- Integration tests for client filter

• Real TCP socketpairs: PASS
• Background libevent dispatcher: PASS
• 9 tests covering GET /sse, message queuing, endpoint routing: PASS
• State machine timing fix (Idle state in onWrite): PASS

Commit 7: 4a552e29 -- Add ServerConnectionMode

• 6-mode enum with comments: PASS
• RAII HandshakeWriteGuard: PASS
• Monotonic sseHeadersWritten sub-state: PASS
• Per-connection via unique_ptr: PASS

Commit 8: b7932814 -- Unit tests for ServerConnectionMode

• Real libevent dispatcher: PASS
• 32 test cases including RAII guard, immutability, sub-states: PASS
• Nested guard ref-counting tested: PASS

Commit 9: b851bbd7 -- Integrate ServerConnectionMode

• sse_server_mode_ removed: PASS
• sse_writing_handshake_ removed (RAII guard instead): PASS
• sse_headers_written_ removed: PASS
• Mode events fired at GET /sse, POST /callback, and non-SSE paths: PASS

Commit 10: b543887b -- Integration tests for server filter

• Real TCP socketpairs: PASS
• 5 tests covering SSE stream, query string, PlainHttp, body routing: PASS

Commit 11: 27424610 -- Remove is_sse_mode_

• is_sse_mode_ removed entirely: PASS
• isSseActive() helper queries both state machines: PASS
• Client state machine lifecycle updated: StreamStarted from WaitingForEndpoint: PASS
• Transition matrix updated: WaitingForEndpoint -> Active valid: PASS

Commit 12: 0523ac2d -- Wire state change logging

• DEBUG-level logging on every transition: PASS
• Human-readable state names via getStateName()/getModeName(): PASS
• No functional change: PASS

Commit 13: 55ef7bff -- Remove dead code

• RequestStream class removed: PASS
• active_streams_ map removed: PASS
• sendResponseForStream/sendResponseThroughFilter removed: PASS
• sendHttpResponse retained (called from mcp_server.cc): PASS
• ~80 lines of dead code eliminated: PASS

Commit 14: 00829020 -- Fix ConnectionPoolImpl SEGFAULT

• Root cause: Write events enabled before connect() syscall: PASS (documented)
• Fix: initial_events = 0 for connecting sockets: PASS
• Event model preserved: doConnect() enables events after ::connect(): PASS
• Deferred timer cleanup in onConnectionTimeout: PASS
• Test accepts both Timeout and LocalFailure: PASS

---

Summary

Rule	Status	Notes
1. State machine pattern	PASS	6 booleans replaced, 2 state machines added
2. Dispatcher-thread confinement	PASS	No mutex/atomic, assertInDispatcherThread() everywhere
3. Per-connection ownership	PASS	unique_ptr ownership, never shared
4. MCP abstraction layers	PASS	All timers, buffers, events through MCP APIs
5. No legacy imports	PASS	No http_transport_socket.h references
6. RAII resource management	PASS	HandshakeWriteGuard, unique_ptr ownership
7. Comments explain WHY	PASS	All states/events/transitions documented
8. No "envoy" in source	PASS	0 matches across all files
9. No AI keywords	PASS	0 matches across all files and commits
10. Event model compliance	PASS	Create->Connect->Read/Write->Close preserved

Overall Verdict: PASS -- All 14 commits comply with all 10 design rules.

No design rule violations found. The implementation follows the established
codebase patterns (SseCodecStateMachine, HttpCodecStateMachine,
ConnectionStateMachine) and adds no new architectural debt.