Independent review — 2026-10-23

Reviewed initial commit 4b01fb47b1e8afcf2f985d0bb76d6c245d8637b4, the complete patch inventory in /tmp/mirror-tool-review.patch, repository requirements, implementation modules, integration tests, embedded UI, API/deployment documentation and benchmark. Line references below refer to that commit. No production targets were contacted; review reproductions used temporary configuration and loopback. This reviewer changed only this document.

Verdict

Changes required: five Important findings, no Critical finding. The async architecture and the explicit strict/isolate delivery distinction match the requested rewrite, but the initial commit does not yet satisfy bounded process shutdown, trustworthy health freshness, complete proxy-loop prevention and two preserved operator workflows. Resolve R1–R5 and review their regression evidence before approval.

The parent reports 23 passing tests on native macOS and Orb amd64, plus clean Clippy. Those are parent-provided results, not reruns by this reviewer. They do not cover the defects below. The reviewer independently reproduced R1 with the built executable, verified R4 with a deterministic JavaScript example, and checked the remaining findings directly against source. UI, benchmark and Linux verification continue under the parent.

Important findings

R1 — An unfinished HTTP body prevents SIGTERM shutdown indefinitely

Location: src/main.rs:67–79; src/web.rs:123–130.
Trigger: Connect to the web listener, send POST /api/login with Content-Type: application/json, Content-Length: 1000 and only { as its body; leave the socket open and send SIGTERM.
Evidence: With network.drain_timeout_ms=100, the process remained alive after two seconds. Closing the incomplete HTTP socket immediately allowed exit status 0. No password or authenticated session is required for this reproduction.
Cause/impact: The main task awaits Axum's unbounded graceful completion before calling engine.shutdown(). The body extractor is still waiting, so the engine's drain deadline never starts. Existing data listeners also continue accepting during this period. This contradicts the README's whole-process bounded shutdown guarantee and can force systemd to kill the process without completing the intended drain.
Fix: On the signal, promptly stop data acceptance and establish one overall shutdown deadline. Bound HTTP graceful shutdown, stop/reject further control mutations, and coordinate already executing mutations before final engine teardown. Merely setting a short normal TCP idle timeout would violate the data-path requirements.
Regression: A subprocess test holding an incomplete HTTP body and a live proxy session must prove listener closure and process termination within the documented shutdown bound, without closing the HTTP peer first.

R2 — Health results receive the end-of-batch time, making old probes falsely fresh

Location: src/control.rs:254–272, candidate freshness filter at src/control.rs:387–389.
Trigger: With bounded monitoring concurrency, place a responsive candidate early in a probe cycle and enough slow endpoints after it that the whole cycle lasts longer than the allowed health age. Let that early candidate go down while the remaining probes finish.
Evidence: Each future returns only (address, ok, latency); all results are collected before checked_at_ms = now_ms() is assigned. Thus the early success is timestamped at publication rather than observation. This follows deterministically from the source; a network-timing reproduction was not run here.
Impact: Automatic failover can promote a candidate whose last successful observation is already outside the freshness window. UI health also claims that the old result was just checked. For example, 640 unreachable endpoints at concurrency 32 and a one-second timeout can make early results roughly 20 seconds old, yet selection sees age zero against the default 12-second maximum.
Fix: Capture the observation timestamp when each probe finishes and retain that timestamp when publishing. Consider streaming completed observations or probing relevant route targets first, but never refresh timestamps to compensate for a slow cycle.
Regression: Exercise a fast candidate followed by delayed probes with a controlled clock/test seam; the candidate must be rejected once its actual observation ages out.

R3 — Wildcard listener loop prevention misses Linux loopback aliases

Location: src/config.rs:247–255, src/config.rs:314–319.
Trigger: Configure a team at 127.0.0.2, a service listener at 0.0.0.0:P, and that service's upstream port at P; select that team as primary. On Linux the full 127.0.0.0/8 is loopback, even when interface enumeration lists only 127.0.0.1.
Evidence: The wildcard test requires membership in the enumerated address set. No is_loopback() check covers 127.0.0.2. The parent has been asked to supply the Orb reproduction; this reviewer did not run Linux commands.
Impact: The proxy recursively connects back to its own listener, consuming the global connection budget and disrupting other services. Monitoring can initiate the recursion too. A finite connection limit bounds the damage but does not make this valid topology.
Fix: Account for the loopback range and IPv4-mapped IPv6 equivalence when checking routes against managed listeners, while respecting socket address-family behavior. Reject the loop before binding/starting.
Regression: In Orb, validate both ordinary 127.0.0.1 and non-enumerated 127.0.0.2 wildcard self-routes and cross-service loops. Use loopback fixtures only.

R4 — The operator journal hides the newest events after the thirtieth entry

Location: web/app.js:268; producer order is src/control.rs:99–110.
Trigger: Produce more than 30 events, then perform a manual change or automatic failover.
Evidence: The controller inserts newest events at the front, but the UI uses slice(-30).reverse(). For descending event IDs [100,99,...,1], the rendered selection contains IDs 1–30 and omits ID 100 (independently checked with Node).
Impact: The operator cannot see recent route changes or errors. Once the server retains 100 events, the displayed window trails by 70 entries.
Fix: Select the first 30 entries and choose the intended display order explicitly.
Regression: Supply a snapshot with at least 35 distinguishable events; the most recent event must be visible and older entries outside the selected window absent.

R5 — Quick primary promotion silently removes the old primary from the mirrored set

Location: web/app.js:179; compare automatic promotion in src/control.rs:327–333.
Trigger: Start with primary A and replica B, then click B's primary radio.
Evidence: The UI sends primary B and replicas []: it removes B from the existing replica list but never adds A. The legacy selectPrimary function in ../ad_scripts/mirror_tool/templates/index.html:259–270 explicitly adds the previous primary. The controller's automatic promotion also adds it. Legacy source was read only.
Impact: A routine manual promotion stops mirroring to A and leaves no automatic failover candidate in the common two-server setup. This is an unintended workflow regression from the requested UI transfer, rather than merely cosmetic behavior.
Fix: Preserve the previous primary as a replica in the quick promotion action, excluding the new primary and deduplicating. Keep the full edit form's explicitly selected replica list authoritative so an operator can intentionally remove a target.
Regression: The quick action must send primary B, replicas [A]; subsequent snapshots must confirm the saved selection. An explicit full-form edit must still be able to choose zero replicas.

Non-blocking observations

Transient accept errors: src/engine.rs:424 permanently ends a listener for every accept error, including resource-pressure/transient OS errors. Consider bounded cancellation-aware retry for recoverable errors. A low-fd loopback stress attempt here did not reproduce this branch: upstream connect failures occurred first and the listener recovered. This is source-backed resilience advice, not a claimed runtime failure.
Heartbeat deadline: src/scoreboard.rs:327–378 responds to Engine.IO pings and bounds each polling request, but does not track an absolute deadline since the last ping. A broken server returning non-ping frames continuously can keep the session connected beyond the negotiated heartbeat window. Freshness of scoreboard data remains separately enforced. An absolute heartbeat timer would improve protocol fidelity; no production failure was demonstrated.
API error envelope: docs/api.md says all failures are JSON {error:...}, but Axum's default method-not-allowed/path-extraction responses are not normalized by the JSON error helper. Clarify the contract or normalize these framework responses. Normal documented mutations already translate JSON/body rejection into a JSON error.

Specification and quality assessment

Requirement	Initial review assessment
Native async Rust executable with embedded UI	Implemented; clean module boundaries and no runtime CDN/build requirement
Internal upstream selection, no kernel routing	Implemented; no host firewall/route mutations found
Existing sessions retain target; no partial replay	Route is cloned at acceptance; no reconnect/replay path found
TCP byte order, half-close and long idle	Both directions complete independently; EOF shuts down the opposite write half; no inherited short idle timeout found
Strict bounded backpressure	Bounded channels and shared payload semaphore; strict replica failure cancels the session
Isolate without chunk-drop continuation	Queue/budget pressure cancels the entire replica and increments counters; no continuation after dropping an arbitrary chunk found
Replica response draining and task ownership	Concurrent response drain and nested worker joining are present
Bounded shutdown	Engine-level drain is bounded; process-level behavior fails R1
Fresh health and stable deterministic failover	Thresholds, cooldown, fresh checker gating and deterministic ordering are present; observation timestamps fail R2
Validated topology/loop prevention	Validation is extensive; Linux wildcard self-route gap remains R3
ForcAD init/state updates and reconnect	Metadata/state shapes are distinct, persistent polling/pong/reconnect implemented; out-of-order rounds rejected
Durable topology and rollback	Same-directory temporary write/fsync/rename; operation serialization and runtime rollback on pre-commit storage failure are present; short rollback window is accurately documented
Authenticated controls and operator safety	Random HttpOnly/SameSite sessions, CSRF, origin check, login limits, textContent rendering and restrictive CSP are present; no embedded production secrets found in reviewed material
Preserved UI workflow and operational evidence	Core service/team/route/statistics views present; latest-event display and quick primary promotion fail R4/R5
Performance claims	Benchmark checks bytes and compares loopback paths; production and zero-overhead guarantees are correctly avoided. Timing results must come from the parent's fresh runs

Overall, this is a coherent rewrite with substantive fixes for the legacy stream corruption, timeout and replica-response problems. Approval is withheld specifically for R1–R5, not for the optional observations. Re-review should inspect the final patch and fresh targeted regressions, then the required formatting, Clippy and all-target tests recorded by the parent.

12 KiB Raw Permalink Blame History Unescape Escape