ctf-ad-mirror-tool-rust/docs/final-review.md

Final independent rewrite review — 2026-10-23

Verdict

Approved on source and targeted-regression review: no remaining Critical or Important findings. R1–R6 are addressed in the reviewed source. The restored UI uses the legacy foundation, and the forwarding design meets the requested internal upstream-selection model. R6 was independently reproduced before repair and independently verified after repair. The parent owns the final integrated formatting/Clippy/native/Orb/release gates and completion of the project evidence log.

This report reviews local base 4b01fb47b1e8afcf2f985d0bb76d6c245d8637b4 on rewrite/rust plus the dirty implementation described by /tmp/mirror-final-review.patch and /tmp/mirror-final-review-untracked.txt. The historical R6 finding below identifies source before repair; the resolution section reviews the current cancellation-safe implementation in /tmp/mirror-r6-review.patch. There is no remote deployment or production-target validation in this review.

Resolved Important finding

R6 — HTTP disconnect cancelled a partially committed control transaction

• Locations: src/web.rs:201 and src/web.rs:207 await controller changes directly in the HTTP handler; src/control.rs:267 persists deletion, src/control.rs:268 awaits engine retirement, and src/control.rs:269 publishes the new in-memory topology. Related mutation boundaries are src/control.rs:216, src/control.rs:217, src/control.rs:232, and src/control.rs:291.
• Reproduction: Start a loopback proxy with drain_timeout_ms=2000; hold an established client/upstream stream open; disable the service; send authenticated DELETE /api/services/echo; wait until the state file records zero services; close the DELETE HTTP socket before its response; wait 2.4 seconds; fetch /api/snapshot.
• Pre-fix result, reproduced twice against the then-current debug executable: After disconnected DELETE: disk services 0 live services 1 active after drain deadline 1. The second run explicitly checked the surviving active-session counter. The diagnostic script is /tmp/mirror-review-cancel.py; it uses a temporary directory, ephemeral loopback ports and a public fixture password, and cleans up the subprocess. No real credentials or game hosts are involved.
• Cause: Hyper drops the handler future when the peer disconnects. The transaction mutex guard is owned by that future, so it is dropped before the transaction completes. In the proven DELETE case the disk commit has happened, but engine retirement and topology publication are abandoned. The active session remains beyond the configured removal deadline because its retirement future no longer runs. PUT service/team changes have the same cancellation-sensitive structure around the detached blocking state writer; those specific timing windows were source-reviewed, not independently induced.
• Impact/blast radius: All three authenticated mutation endpoints can lose transactional ownership on client/network failure. A subsequent ordinary mutation can persist the stale topology again. This violates authoritative saved state and bounded retirement, despite correct rollback when persistence returns an ordinary error. It does not require an unauthenticated attacker: closing a browser tab or losing the operator connection is sufficient.
• History and coverage: The direct-handler transaction structure exists in initial commit 4b01fb4; the shutdown serialization changes do not remove it. Existing tests cover storage failure, concurrent shutdown and body-extractor stalls, but do not disconnect a client after a mutation starts.
• Repair acceptance criterion: Keep admitted transactions owned independently of the HTTP request until commit/rollback and cleanup finish. Preserve serialization and shutdown admission ordering: shutdown must reject new work and coordinate every admitted transaction before normal teardown, subject to the existing overall process deadline. Merely moving persistence to another blocking task does not protect the rest of the transaction.
• Regression acceptance criterion: A real HTTP disconnect after DELETE's disk commit must still result in matching disk/snapshot topology and zero active sessions within the drain allowance. Add cancellation coverage for PUT persistence/rollback as appropriate to the ownership boundary. Re-run the existing shutdown/quiesce/storage regressions after changing transaction ownership.

R6 resolution and independent acceptance

Resolved. Controller::transaction (src/control.rs:191) acquires the shared owned operation mutex and checks stopping before it spawns a transaction worker. There is no await/cancellation point between admission and transfer of the guard/controller Arc into that worker. Dropping the HTTP handler's JoinHandle detaches the transaction; apply, save, publication, rollback and stream retirement continue under one guard. Callers waiting on admission remain cancellable and do not allocate a detached worker. The controller retains only a Weak self-reference outside an active worker, avoiding a permanent strong cycle.

Manual service PUT/DELETE and team PUT use this wrapper; automatic promotion also owns its admitted selection/mutation phase. Shutdown first sets stopping and quiesces acceptance, then waits for the same transaction mutex before draining. Engine quiescence prevents an in-flight commit/rollback from reopening listeners. Cancellation before admission cannot mutate state. The existing overall process deadline still takes precedence over a stalled transaction; the repair does not promise completion through forced termination, panic or machine failure.

Fresh independent post-fix checks:

• Re-ran the original actual HTTP/socket-disconnect script against the rebuilt debug executable: After disconnected DELETE: disk services 0 live services 0 active after drain deadline 0.
• Ran cargo test --lib cancellation_tests --locked: 5 passed, 0 failed. These cover DELETE cleanup with a real held TCP stream, service PUT after save, service PUT rollback after a failing save, team PUT after save and automatic promotion after save. The PUT success fixture also verifies exact loopback payload delivery; DELETE verifies stream closure and removal from the engine registry.
• Inspected the shutdown/quiesce tests and wrapper integration. The parent's final suite supplies the fresh whole-suite shutdown/storage checks; this reviewer did not rerun the full suite.

The five regressions included in the repository test suite cancel the controller caller task at deterministic transaction boundaries. The actual HTTP-disconnect scenario was independently executed as a local script and is not included as an automated HTTP regression test. The combined source inspection, deterministic cancellation tests and actual HTTP red/green evidence are sufficient for this finding's acceptance; this distinction is retained explicitly. README and docs/api.md correctly tell operators that a lost response can follow a committed mutation and to refresh the snapshot before retrying.

Critical and Minor findings

No Critical findings were identified. No additional Minor finding is raised as a release condition. This is a bounded source and targeted-runtime review, not proof that every operating-system or malicious-input failure mode has been exhausted.

Resolution assessment of R1–R5

Finding	Assessment	Source and evidence
R1: incomplete HTTP body blocks process shutdown	Addressed	src/main.rs:83 starts shutdown independently of HTTP graceful completion; the shared async deadline is drain + 1000 ms and explicit runtime teardown adds a 100 ms wait. src/control.rs:102 marks stopping, quiesces before acquiring the controller transaction mutex, then drains. tests/shutdown.rs exercises the actual process with an incomplete body and live TCP peer. The distinct transaction-ownership defect R6 is resolved in the section above.
R2: publication-time health timestamps	Addressed	observe_probe captures now_ms() when each future finishes; publication copies observed_at unchanged. delayed_batch_does_not_refresh_early_candidate_observation traverses actual publication and candidate selection with a deliberately aged early result.
R3: wildcard Linux loopback/mapped IPv6 route loops	Addressed	normalized_ip, family-aware wildcard checks and is_loopback() reject aliases without relying solely on interface enumeration. Tests cover self/cross-service routes, mapped IPv6 and a native IPv6 target behind an IPv4-only listener. Linux has an actual 127.0.0.2-to-wildcard fixture.
R4: reversed event-journal window	Resolved by authorized UI restoration	The rejected journal is removed from the operator panel. The snapshot API continues to return the bounded newest-first backend events. There is no remaining slice(-30).reverse() rendering path.
R5: manual promotion loses former primary	Addressed	web/app.js:87–92 removes the new primary from replicas and adds the former primary once, clearing the direct-IP override. This matches the legacy selectPrimary behavior. Replica rows still allow explicit removal.

Whole-rewrite assessment

Binding requirement	Assessment
Native executable and module separation	Config, engine, control, store, auth/web and scoreboard are separate. Frontend assets are embedded; no runtime Python, Node or external CDN is required.
Internal upstream auto-selection only	No system route/firewall mutation is present. Auto mode selects only eligible configured replicas with cooldown, failure/recovery thresholds and fresh observations.
Existing streams and no replay	The supervisor captures an immutable route per accepted session. Retargeting updates future sessions; there is no reconnect/replay of partially sent application data.
Byte integrity, TCP half-close and idle	Primary directions are awaited together; EOF shuts down the matching write half without prematurely cancelling the opposite direction. Replica replies are drained concurrently. No short application idle timer was introduced.
Strict policy	Bounded channel/payload acquisition backpressures the client. Required-replica setup failure prevents payload forwarding; runtime replica errors cancel the entire primary session. Strict waiting on a slow live replica is explicitly documented.
Isolate policy	Queue/budget exhaustion cancels the complete affected replica instead of silently skipping a chunk and continuing its stream. Disconnect/error counters and bounded tail drain are present. Partial delivery to a disconnected replica is accurately documented.
Resource ownership	Global data-session permits, bounded replica queues, shared payload budget and owned nested worker joins are present. Working buffers and OS socket buffers are explicitly outside the payload budget. R6 now retains transaction/retirement ownership after caller cancellation.
Graceful shutdown/quiesce	Acceptance stops before waiting for a filesystem/control transaction; a quiesced upsert can restore bookkeeping but cannot reopen a listener. The process has a single outer shutdown deadline. The R6 ownership wrapper preserves these properties.
Persistence and ordinary rollback	Same-directory create-new temporary file, file sync, atomic rename, and attempted runtime rollback on precommit failure are implemented. Failed new direct-IP service creation removes its listener and does not publish its hidden team. The R6 wrapper now preserves ordinary transaction completion across HTTP cancellation.
ForcAD transport	Engine.IO 4 polling, namespace initialization, ping/pong, reconnect and distinct init/state updates are implemented. The absolute heartbeat deadline is renewed only by a ping; a continuously non-ping server cannot keep the connection alive indefinitely. Incoming bodies are bounded.
Transient accepts	Recoverable errors retry at 50 ms through a one-second cap, reset after success, and remain cancellation-aware. Counters/diagnostics distinguish accept failures. Injection tests use the production supervisor and real loopback transfer; actual OS descriptor exhaustion was not induced here.
Authentication and API	Cookie sessions, constant-time digest comparison, CSRF, browser origin checks, login throttling, restrictive response headers and body limits remain intact. Framework-error envelope differences are now documented.
Legacy UI foundation	The full original CSS block remains present after trailing-whitespace normalization; 24,037 original characters were checked independently. The original login/card/columns/controls/team selectors/custom dialog structure is retained with local dependency versions. Small accessibility, stale-state and layout additions are consistent with the user's scope.
Direct-IP workflow	Standard service PUT resolves/reuses the address atomically and creates a hidden target when needed. No manual team-creation step is required. Custom input/create, manual promotion and server validation are wired together. Parent's browser verification confirmed actual keyboard editing and persisted snapshots.
Claims and documentation	Documentation distinguishes request mirroring from database replication, local writes from application commitment, strict backpressure from isolate availability, and loopback benchmarks from production guarantees. Final journal/plan/verification completion remains the parent's responsibility.

Evidence and limits

The reviewer read the full production Rust modules, restored HTML/JS/CSS, changed-file patch inventory, relevant integration tests, API/deployment/README contracts, historical review, backend-fix report, UI report and task plan. Legacy templates/base.html, index.html and login.html were read only; the sibling configuration and credentials were not read or imported. The only repository file this reviewer writes is this report.

Fresh reviewer commands/evidence:

• Two pre-fix loopback subprocess reproductions of R6 and one post-fix reproduction confirming convergence, plus all five post-fix cancellation tests passing independently.
• Python normalized containment check: Legacy CSS normalized containment: PASS 24037 characters.
• node --check web/app.js: exit 0.
• cargo metadata --format-version 1 --locked: exit 0; no resolved package advertises an MSRV above the manifest's 1.85.

Parent-provided, separately executed pre-R6 integrated evidence: native format/Clippy, 35 native tests, 36 Orb amd64 tests including the Linux alias fixture, release builds on both platforms, real upstream-version Socket.IO/Engine.IO interoperability, three-mode byte-checked benchmarks and browser checks. The backend implementer reports post-R6 native formatting/Clippy/40 tests and Orb amd64/41 tests; the parent is running final integrated gates separately. These suites were deliberately not rerun wholesale by this reviewer. The parent reports the final browser direct-IP edit passed with actual keyboard input, 390px viewport had no document overflow, a seven-second SSE freeze disabled mutations, and resumed snapshots restored controls. These browser results are attributed to the parent, not represented as independently executed here.

No production scoreboard/game host, real low-fd exhaustion, indefinitely blocked kernel filesystem call, multi-process shared-state operation, or broad benchmark workload was tested by this reviewer. R6 targeted re-review is complete and approved; no new Critical/Important finding was found. Parent-integrated final build/test gates remain separately attributed.