reverse-proxy/tasks/review/post-fix-review.md at cfba7491ae65af064e1bc14089aa92fb5773b349

Files

glm-5.1 f9d7b8112b Decompose implementation review fixes into 14 atomic tasks with post-fix review

Break down findings from review #002 into dependency-ordered fix tasks:

Critical/High:
- fix/acme-contact-and-challenge (C1+C2): Add acme_contact field, wire to
  ACME, remove unused challenge_config, add validation rule 19
- fix/remove-health-and-hardcode-https (W5+W14+ADR-022): Remove /health
  from main listener, hardcode X-Forwarded-Proto to https
- fix/config-reload-static-drift (C4): Use ArcSwap<StaticConfig> so reload
  diffs against last config, not startup config
- fix/access-logging (W13): Wire up log_request! macro for every proxied
  request with client_ip, host, method, path, status, upstream, duration_ms

Medium:
- fix/graceful-shutdown (W1+W7): Join HTTPS tasks with timeout instead of
  abort, add shutdown signal to admin socket and eviction task
- fix/connect-timeout (W4): Wire upstream_connect_timeout_secs to enforce
  separate connect timeout

Low/Independent:
- fix/token-bucket-nanosecond (W6): Use as_nanos() instead of as_millis()
- fix/normalize-host-ipv6 (S3): Handle IPv6 bracket notation in normalize_host
- fix/http-port-validation (S1): Validate http_port in range 0 or 1-65535
- fix/integration-test-toml (S10): Fix double-nested listeners.listeners.sites
- fix/logging-test-global-subscriber (W9): Use try_init() to avoid test conflicts
- fix/fragile-error-detection (W3): Add typed error matching or documented string match
- fix/add-code-comments (C3,W8,W10,W11,S9): Document correct-but-non-obvious behaviors
- fix/request-timeout-scope (S8): Document full-request timeout scope
- fix/clean-dead-code (S4+S2): Remove dead_code annotations, add #[non_exhaustive]

Review gate:
- review/post-fix-review: Verify all fixes against architecture spec

2026-06-12 04:08:45 +00:00

3.4 KiB

Raw Blame History

id, name, status, depends_on, scope, risk, impact, level

name

status

depends_on

scope

risk

impact

level

review/post-fix-review

Review all post-fix implementations for correctness and spec conformance

pending

fix/acme-contact-and-challenge

fix/remove-health-and-hardcode-https

fix/config-reload-static-drift

fix/access-logging

fix/graceful-shutdown

fix/connect-timeout

fix/token-bucket-nanosecond

fix/normalize-host-ipv6

fix/http-port-validation

fix/integration-test-toml

fix/logging-test-global-subscriber

fix/fragile-error-detection

fix/add-code-comments

fix/clean-dead-code

fix/request-timeout-scope

broad

low

project

review

Description

Post-implementation review of all fix tasks from review #002. Verify that each fix correctly addresses the original finding, matches the architecture spec, and doesn't introduce regressions.

Review Checklist

ACME contact and challenge (C1, C2):
- acme_contact field exists on TlsConfig and deserializes from TOML
- Validation rule 19 enforces mailto: format for acme_contact in ACME mode
- AcmeTlsConfig.contact receives configured contact (not empty vec)
- build_acme_challenge_config removed, TlsMode::Acme no longer has challenge_config
- TLS-ALPN-01 challenge still works via main listener ALPN protocol
Health route removal / HTTPS hardcode (W5, W14, ADR-022):
- No /health route on main listener
- is_https removed from ProxyState
- X-Forwarded-Proto always "https" with explanatory comment
- Port 9900 health check still works independently
Config reload drift (C4):
- ConfigReloadHandle.static_config uses ArcSwap<StaticConfig>
- Second reload only reports new changes, not repeated old changes
Access logging (W13):
- log_request! called for every proxied request
- log_upstream_error! called for upstream failures
- Log format matches spec: REQUEST client_ip=... host=... method=... path=... status=... upstream=... duration_ms=...
- duration_ms measures wall time accurately
Graceful shutdown (W1, W7):
- HTTPS tasks joined with timeout, not immediately aborted
- Admin socket and eviction task respond to shutdown signal
- Socket file cleaned up on exit
Connect timeout (W4):
- upstream_connect_timeout_secs wired up in proxy handler
- Separate connect timeout enforced
All other fixes:
- Token bucket nanosecond precision
- IPv6 normalize_host
- http_port validation
- Integration test TOML
- Logging test global subscriber
- Fragile error detection
- Code comments added
- Dead code cleaned, #[non_exhaustive] added
Quality gates:
- cargo clippy passes with no warnings
- cargo fmt --check passes
- cargo test passes (all unit and integration tests)
- No regressions in existing functionality

Acceptance Criteria

All fix tasks completed and verified
cargo clippy passes with no warnings
cargo fmt --check passes
cargo test passes
Architecture spec conformance verified for all changes
No regressions in existing tests

References

docs/reviews/002-implementation-review.md — all findings
docs/architecture/ — updated architecture docs
docs/architecture/decisions/022-health-check-scope.md — ADR-022

Notes

This review should run after all fix tasks are complete. Focus on correctness, spec conformance, and regression testing.

Summary

To be filled on completion

3.4 KiB Raw Blame History