[aaydin-tr]

Adds server.http2.max_concurrent_streams, health_checker_timeout and backends[].health_check_status; covers /metrics and /stats in the integration suite.

[github-actions]

Claude finished @aaydin-tr's task in 2m 8s —— View job

---

Code review: PR #47 (Tier 2: Go 1.27 + deps, HTTP/2 max_concurrent_streams, Probe tuning)

Fixed point: origin/main (718e329), chosen by default because the run is non-interactive. The diff is one commit (dba5528), 36 files. There is no spec lookup. The Spec axis is replaced by a Correctness axis. Items already listed in TODO.md were excluded.

Standards

No hard violations of a documented rule except comment length. The findings below are in CLAUDE.md "Code style" terms (comments only for constraints the code can't show, one or two lines).

Hard (comment style)

• integration-test/monitoring_test.go, TestMonitoringReportsBackendLivenessAcrossRejoin: the 3-line comment starting // /metrics backend_alive and /stats is_host_alive read 0/false... narrates the test body.
• integration-test/monitoring_test.go, TestMonitoringReportsPerBackendRequestCounts: the 3-line // /stats total_req_count and /metrics... comment is the same problem.
• Doc comments on requireStatsRowsInConfigOrder, readStatsRowsInConfigOrder and backendAddress mostly restate their names. This one is borderline. Keep the "readiness request was proxied too" baseline note, since it describes a real constraint.

Judgement calls (smell baseline)

1. Data Clumps / Duplicated Code / Shotgun Surgery.
   • ProbeURL and ExpectedProbeStatus always travel together.
   • core/pool/pool.go repeats p.probe(backend.ProbeURL, backend.ExpectedProbeStatus) at two sites.
   • Changing the types.IsHostAlive signature forced edits to about 8 test doubles in core/balancer_test.go, core/pool/pool_test.go, core/serve_bench_test.go and mocks/mocks.go.
   • A Backend.Probe(...) method or a small ProbeTarget{URL, ExpectedStatus} value would remove both the repetition and the ripple.
2. Mysterious Name / naming drift.
   • The same concept is HealthCheckStatus in config.Backend and ExpectedProbeStatus in pool.Backend and types.
   • AnySuccessOrRedirect doesn't match the CONTEXT.md "2xx or 3xx" wording, and a "success" that includes a redirect reads oddly.
3. Mysterious Name (magic number).
   • isFinalHTTPStatus in pkg/config/config.go hardcodes 599.
   • It also duplicates the 2xx/3xx range knowledge in ExpectedProbeStatus.IsSatisfiedBy.
   • A named constant, or a validity method on ExpectedProbeStatus, would keep the status rules in one place.
4. Divergent Change (placement). The HealthCheckerTimeout default and validation is inline in PrepareConfig. The sibling knobs go through applyServerDefaults. This is cosmetic, because health_checker_timeout is top-level like health_checker_time.
5. Speculative Generality (mild). HTTP2Config wraps one field, and newProbeClient exists only so a test can inject a dialer. Both are defensible.
6. Scope notes (not violations).
   • The config.*Type constants and backendAddressLabel come from goconst fallout after the toolchain bump.
   • The //nolint:gosec shuffles are tooling noise.
   • The probes_test.go refactor to the new getMonitoring helper removes duplication.

Checked and clean: names are written out in full, CONTEXT.md's Probe entry was updated, production comments are one or two lines, and no ADR is contradicted.

Correctness

Bugs: none that could be tied to a concrete failing input.

• Every caller and test double uses the new IsHostAlive(url, ExpectedProbeStatus) signature, and no stale func(string) bool is left.
• ExpectedProbeStatus is set once at construction and only read by the Probe goroutine, so it adds no race with Pick.
• Config validation is sound:
  • health_check_status rejects negatives, 1xx and values above 599, and 0 means unset.
  • A negative health_checker_timeout fails, and 0 becomes 5s.
  • max_concurrent_streams is capped at math.MaxInt32, and <= 0 becomes 250.
• The Probe client moved from ReadTimeout/WriteTimeout to DoTimeout plus DialTimeout. Every phase stays bounded, as ADR 0003 requires. I could not read the vendored fasthttp source, so the deferred-release safety after a DoTimeout is unverified.
• Probes run sequentially. A large health_checker_timeout times several hanging backends can stretch a round past health_checker_time, which just delays the next tick. A value above types.HealthCheckerStopTimeout (5s) only means Shutdown stops waiting after 5s, which is the existing cap working as designed.

Security: nothing found. There is no new inbound surface. Probe timeouts stay bounded, and dropping the //nolint:gosec on tls.Config{MinVersion: ...} does not weaken TLS, since the version still comes from the validated tls_min_version.

Conflicts: none.

• CONTEXT.md, README, both example configs and the TODO.md entries agree with the code.
• The new keys decode under strict KnownFields.
• max_concurrent_streams above 2147483647 also fails startup on http1.1, though the key is documented as inert there. The README and example text both say so, so it is intended.
• review.md:309 still says "no knob (additive later)". It is a historical note, not a behaviour claim.
• I did not run the new monitoring_test.go because it needs Docker. Its assertions look consistent with CONTEXT.md.

Summary

• Standards: 3 hard comment-style findings (all in integration-test/monitoring_test.go) and 5 judgement calls. The most worthwhile one is the repeated ProbeURL/ExpectedProbeStatus pair.
• Correctness: 0 findings.
• I left no inline comments, because none of the findings points at a single verified line.