request improvements and documentation

[vicent-dev]

Fix four silent proxy defects, add /health, and document the gateway

Four bugs in the proxy path were making the gateway quietly wrong, and none of
them was visible from the client side: an upstream 404/500 arrived as
200, request headers and query strings never reached the service behind the
gateway, and a client disconnect left the upstream call running. This fixes all
four, adds an unauthenticated liveness endpoint, and replaces the README with
documentation that matches what the code actually does.

11 files changed, +1327 / -48. One commit, 65cea94.

---

The four proxy fixes

1. Every proxied response reached the client as 200 OK

defaultRouteHandler copied the upstream headers and body but never called
w.WriteHeader, so the first body write implied 200. An upstream 404 or
500 was flattened into a success.

• app/route.go writes w.WriteHeader(call.Response.StatusCode) explicitly, on
  both the live and the cached path. A cache hit restores the status it stored,
  so the two paths are identical.
• Call.SetValue now rejects a decoded status outside 100–999
  (pkg/request/call.go). The handler hands that value straight to
  ResponseWriter.WriteHeader, which panics there — a payload that decodes but
  carries an impossible code is corrupt in exactly the same way an undecodable
  one is, so it fails the read instead of becoming a panic per request until the
  entry expires.

2. Request headers never reached the upstream

Client.Request built the upstream request from the method, URL and body only.
Authorization, Content-Type, cookies and custom headers were all dropped.

pkg/request/client.go now clones the caller's header map, strips the hop-by-hop
set, strips the gateway's own bearer, and forwards the rest. The map is cloned
rather than shared because NewCall rewrites the inbound request in place — an
aliased map would let the outbound request keep rewriting the inbound one.

Authorization is stripped deliberately. The bearer authenticates the caller
against the gateway; handing the same token to every upstream would spread a
credential that is valid here, and a compromised upstream could replay it.
Consequence to be aware of: nothing replaces it, so a backend still cannot tell
which caller it is serving. That stays a known limitation rather than becoming a
guess at a header contract.

app.copyUpstreamHeaders applies the same rule on the way back, minus
Content-Length, since the body is buffered here and its length is this
handler's to declare.

3. Query strings were dropped

Call.requestUrl was built from r.URL.Path, which never carries the query. The
worse half: the fingerprint hashed the whole *url.URL, so ?page=2 got its own
cache entry for a byte-identical upstream request.

The query is now appended after the rewrite, and only when non-empty, so no
request grows a bare ?. It is appended after matching, never before, so a
query cannot change which service answers.

4. A client disconnect did not cancel the upstream call

The request was built with http.NewRequest, so nothing tied the upstream call
to the client that asked for it.

http.NewRequestWithContext is used now. The asynchronous cache write is the
deliberate exception: it runs with context.WithoutCancel(ctx), so a caller who
hangs up after the response was fetched whole does not throw it away. That
exception is the reason this fix needed two tests rather than one.

---

New: GET /health

app/health.go answers 200 {"status":"ok"} with no token, registered on the
root router ahead of the authenticated catch-all — which is what makes it
reachable by a probe that holds no credentials. It never reaches pkg/request,
so it answers even with no services configured at all.

It is liveness only: it checks no dependency, so a gateway with every
upstream down still reports healthy. There is no readiness endpoint, and this is
a deliberate scope decision rather than an oversight — the one dependency the
gateway cannot start without is already required to boot, and the cache is built
to fail open, so a probe that checked them could only repeat what start-up
proved.

Also added: a compose healthcheck for the app service, speaking HTTP over
bash's /dev/tcp so the image needs no extra package.

---

Bug found while writing the tests: the rate limiter does nothing

Not part of the brief, so not fixed here — but it invalidates a claim the
docs used to make, and it would mislead the next reader.

rateLimiterMiddleware builds rate.NewLimiter(5, 10) inside the middleware
constructor, and gorilla/mux calls that constructor again on every matched
request (Router.Match rebuilds the chain per request). Every request therefore
starts from a fresh, full bucket, Allow() always succeeds, and no request has
ever been refused with 429.

TestRateLimiterMiddleware_Throttle passes only because it calls the built
handler directly instead of going through the router, so it was green while the
server throttled nothing. That test now carries a comment saying so.

Fixing it means hoisting the limiter to server state, and then making it
per-client — one global 5 rps bucket would throttle almost all real traffic.
Tracked in AGENTS.md.

---

Documentation

• README.md — rewritten end to end (49 → ~650 lines): what the gateway
  does, architecture and request-flow diagrams, auth, routing, response cache,
  configuration, getting started, the error model, testing, limitations, layout.
  Limitations are derived from the code rather than from memory — a section that
  documented the four bugs fixed here would have shipped a lie.
• AGENTS.md — the four defects moved from "Known issues" to "Already
  fixed", with the reasoning kept; the rate-limiter entry corrected; /health
  documented as liveness-only and as shadowing a service on the same path.

---

Tests

158 → 177, all hermetic (miniredis + in-memory SQLite, no running services).
Coverage: app 71.4% → 73.4%, pkg/request 97.4% → 97.6%.

New coverage, one test per claim rather than per function:

• Status: TestProxyForwardsTheUpstreamStatus (201/404/500), and
  TestProxyReplaysTheCachedStatus for the cache path — 418 twice, with the
  upstream hit counter proving the second came from Redis.
• Codec: TestCall_SetValueRejectsAnImpossibleStatus for the 100–999 guard.
• Headers: TestClient_ForwardsTheCallersHeaders,
  TestClient_StripsTheGatewayBearerAndHopByHopHeaders,
  TestClient_DoesNotAliasTheCallersHeaders, TestIsHopByHop,
  TestCopyUpstreamHeaders, and TestProxyDropsTheUpstreamConnectionHeaders.
• Query: TestClient_ForwardsTheQueryString,
  TestProxyForwardsTheQueryString,
  TestNewCall_{PathRewriteKeepsTheQuery,PathRewriteWithoutAQuery,QueryDoesNotChangeTheMatch}.
• Cancellation: TestClient_CancelledCallerStopsTheUpstream and
  TestClient_CachesAResponseTheCallerAlreadyHungUpOn — the pair that keeps the
  WithoutCancel exception honest.
• Health: TestHealthIsAnsweredWithoutAToken,
  TestHealthIsNotShadowedByAService, TestHealthIsLogged.

The two subtlest tests were mutation-checked rather than trusted: reverting
context.WithoutCancel fails
TestClient_CachesAResponseTheCallerAlreadyHungUpOn, and reverting the query
append fails TestClient_ForwardsTheQueryString.

gofmt, go vet ./..., make build, go test ./... -race and
docker compose config are all clean.

---

Behaviour changes a reviewer should check

1. Upstream 4xx/5xx now reach clients as themselves. Anything downstream that
   was relying on the old always-200 behaviour will see different statuses.
2. Query strings and request headers now reach upstreams. Services that were
   written against a gateway which dropped them may behave differently.
3. A client disconnect now cancels the in-flight upstream call. Upstreams may
   observe fewer requests for abandoned traffic, which is the intent.
4. /health is now taken. A service configured on the health path becomes
   unreachable; registration order decides, and health is first.
5. The cache key is unchanged, so existing entries keep working. The status
   guard only rejects payloads that would have panicked.

Known limitations left in place

Recorded rather than fixed, each because fixing it means changing behaviour or
adding a feature: responses are cached regardless of method; the upstream's
Content-Length is dropped; the caller's Authorization is not forwarded and
nothing replaces it; NewCall still deletes Date from the caller's live header
map (contained now, but the handler still observes the missing header);
server.host is parsed and ignored; internal/external is not a
public/private split; ServicesConfig has no Validate(); Redis credentials are
hardcoded.