refactor(proxy/tui): deepen http_wire, slim handleConnection, guard held edits - #7
Open
vandot wants to merge 8 commits into
Open
refactor(proxy/tui): deepen http_wire, slim handleConnection, guard held edits#7vandot wants to merge 8 commits into
vandot wants to merge 8 commits into
Conversation
bufferChunkedBody and forwardChunkedBody were near-identical drives of the chunkedStep state machine, differing only in whether each read was also written to the client. Collapse both into a single deep function in http_wire, keeping the module pure: the transport enters via an anytype src (read) and sink (write), so http_wire still has no sockets or TLS. The sink receives the raw chunked framing verbatim while body[] gets the decoded payload; proxy passes SslSink to forward, NullSink to buffer-only. chunkedStep now reports a dropped payload byte via a truncated out-param, which pumpChunked surfaces in PumpResult. The chunked response paths use it to set resp_body_truncated precisely, replacing the imprecise captured >= max_body_len heuristic and closing a capture-parity gap with the content-length path. Tested via slice source + capturing sink: raw forwarding, prefix+stream split, truncation flag, and parse_error propagation.
…ders Three header-forwarding loops (request->upstream, response->client, and the buffered intercepted response) each re-derived their own skip-list with startsWithIgnoreCase and, for two of them, a duplicated Set-Cookie Domain rewrite. Drift between the three was invisible. Collapse them into http_wire.forwardHeaders(headers, sink, policy, domain): each call site passes a ForwardPolicy naming the header prefixes to drop and whether to rewrite Set-Cookie Domain. The fragile cookie-domain rewrite moves to http_wire.writeCookieWithDomain, now unit-tested (leading-dot stripping, no-Domain passthrough, and the domain= in-value case the first-semicolon scan is meant to guard against). The module stays pure: transport enters through an anytype sink, so the whole forwarding policy is exercised with a capturing sink and no socket. proxy gains an UpstreamSink alongside SslSink; both swallow write errors, making the request direction consistent with the response direction.
handleConnection inlined ~100 lines of upstream setup — DNS resolve / IP parse, socket, connect, and the external TLS handshake — with seven near-identical error-exit blocks, each repeating "if (was_intercepted) finishEntry(502); sslSendError(502); return". Move all of it into connectUpstream() !UpstreamConn: it logs the specific cause, owns the resolved AddressList for its own lifetime, sets the socket timeouts, and returns error.UpstreamUnavailable on any failure. The caller maps that to a single 502 handler. handleConnection drops from 603 to 510 lines. Behavior-preserving: every log line is kept and the 502 path is unchanged; the only difference is error-path duration is now the elapsed time uniformly rather than 0 in a few of the old blocks.
…Body The non-chunked response body was captured by four near-identical loops: content-length and read-until-close, each duplicated between the response-intercept buffering path and the normal streaming path. They differed only in whether each read was also relayed to the client. Extract http_wire.streamBody(initial, src, sink, content_length, body): it relays each read to the sink verbatim (SslSink to stream, NullSink to buffer-only) while capturing up to body.len into the entry, reporting captured + truncated. Both response paths now read as a clean pair — pumpChunked for chunked, streamBody otherwise. Truncation is drop-based (consistent with pumpChunked) rather than the old declared-length heuristic; content-length still does not trim an over-read, matching prior behavior. handleConnection drops to 439 lines. Tested with a slice source + capturing sink covering content-length, read-until-close, initial-slice, and truncation.
The request-emit block — request line, forwarded headers, external Host rewrite, the proxy's X-* headers, recomputed Content-Length, and body — was ~60 inline lines in handleConnection, each write a bare `upstream.writeAll(...) catch return`. Extract forwardRequest(sink, entry, ctx) over an anytype sink, the same seam pumpChunked/streamBody/forwardHeaders use. proxy passes UpstreamSink; tests pass a capturing sink and assert the exact bytes for local and external routes, including that Content-Length is recomputed from the captured body rather than echoed from the original headers. The req_body_truncated 413 check moves ahead of the call, so a truncated request now aborts before any bytes reach upstream instead of after a partial emit. Request-emit write failures are swallowed by UpstreamSink (consistent with the header/response directions); the response read that follows still detects a dead upstream. handleConnection drops to 395 lines.
The upstream response head — read-until-CRLFCRLF, status-line parse, and slicing out the header block and leftover body — was inline in handleConnection. Move it to http_wire.readResponseHead(src, buf) ?ResponseHead over an anytype reader, the last socket-bound piece of the response path. handleConnection drops to 370 lines (from 603). This makes the whole upstream-response path testable end to end with a fake reader: tests drive readResponseHead -> streamBody and readResponseHead -> pumpChunked, asserting client output and entry capture with no socket — the in-memory upstream adapter the per-phase seams were built for. Fix: a response whose status line is immediately followed by the terminating CRLFCRLF (no headers, e.g. a bare 204) made first_line_end == hdr_end, so the header slice was buf[n+2..n] — an invalid range that panicked the proxy thread. The original inline code had the same latent bug; readResponseHead now yields an empty header block instead, with a regression test.
The TUI editor carried a raw ring backing index and drove the held-entry edit protocol as four loose calls — logicalToBackingIndex, phaseOf, snapshotByBackingIndex, then applyRequestEdit/applyResponseEdit — with nothing checking the entry was still held when the write landed. applyRequestEdit wrote entries_backing[idx] unconditionally, and the response editor took a second lock via statusOf to read its fallback. Introduce requests.editHeld(logical) ?EditableHold: it resolves the index and classifies the phase under one lock, returning null for a missing entry or a settled response. The handle bundles the index with the opened phase and exposes snapshot / currentStatus / commitRequest / commitResponse / accept. Both commits re-check, under the lock, that the entry is still .intercepted in the expected phase, and no-op (returning false) if the hold was resolved meanwhile — so a stale editor can no longer scribble onto whatever now occupies the slot. The TUI accepts the intercept only when the write actually applied. applyRequestEdit, applyResponseEdit, and statusOf were used only by the editor; their bodies move into the guarded handle methods and the standalone functions are gone. EditState now holds an ?EditableHold instead of a usize, so a raw ring index no longer leaves requests.zig.
The detail view formatted both the request and response bodies into one module-level json_pretty_buf within a single frame, then rendered the accumulated DetailLines together. When both bodies were JSON, formatting the response overwrote the buffer the request body's lines still pointed into — so the REQUEST BODY pane rendered the response's bytes. Extract the JSON reserialization (detect, parse, re-emit into a caller-owned buffer) into a std-only jsonfmt module: pretty for display, compact for forwarding an edited body. tui's three copies of that core (EditState.prettyPrintJson, EditState.compactJson, and the inline pretty print in splitBodyLines) collapse onto it. splitBodyLines now takes a caller-owned scratch buffer; the detail view passes a separate one for each pane, so the two bodies no longer share storage. Because jsonfmt is pure, the formatting — including the regression that caused the clobber — is now unit-tested without the TUI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Architecture deepening pass: turn shallow/duplicated code into deep modules behind small interfaces, with the new interfaces as the test surface. Each commit is behavior-preserving (two exceptions noted below, both bug fixes). No
zig build teststep — suites run per file.Commits
pumpChunked—bufferChunkedBody/forwardChunkedBodywere near-identical drives of thechunkedStepstate machine. Collapsed into one deephttp_wire.pumpChunkedover ananytypesrc/sink, keeping the module socket-free. Adds drop-based truncation reporting.forwardHeaders— three hand-rolled header skip-lists (+ a duplicated, fragileSet-CookieDomain rewrite) collapse into onehttp_wire.forwardHeaders(headers, sink, policy, domain). Cookie rewrite moves towriteCookieWithDomain, now tested.handleConnectiondecomposition (603 → 370 lines) — extractedconnectUpstream(folds 7 duplicated connect-error exits into one),streamBody(collapses 4 non-chunked capture loops),forwardRequest(request emit, byte-for-byte tested), andreadResponseHead.EditableHold— the TUI editor drove the held-entry edit protocol with a raw ring index and four loose calls, writing unconditionally. Nowrequests.editHeld(logical) ?EditableHoldbundles index + phase; commits re-check under the lock that the entry is still held and no-op otherwise. No raw ring index leavesrequests.zig.jsonfmt— three copies of the detect→parse→re-emit JSON core collapse into a pure, std-only module formatting into a caller-owned buffer.Bug fixes surfaced by the new test surfaces
readResponseHead): a response whose status line is immediately followed byCRLFCRLF(e.g. a bare204) produced an invalid header slicebuf[n+2..n]that panicked the proxy thread. The original inline code had the same latent bug.jsonfmt): the detail view formatted both request and response bodies into one sharedjson_pretty_bufper frame, so a JSON request + JSON response made the REQUEST BODY pane render the response's bytes. Each pane now formats into its own buffer.Behavior notes (non-bug, deliberate)
0in a few branches.Tests
All per-file suites green: jsonfmt 7, http_wire 46, proxy 113, requests 54, intercept 20, dns 16, har 67.
zig buildclean. New pure functions are tested through their interfaces with fake readers + capturing sinks — no sockets, no TLS, no vaxis.Note:
src/jsonfmt.zigis a new test target not yet listed inCLAUDE.md's test commands.