Fix: prevent duplicate AutoRelog retries (#3186)

[milutinke]

Summary

Fixes the duplicate AutoRelog decisions and unreliable reconnect ownership reported in #3186.

• Binds each disconnect decision and restart request to an immutable connection attempt.
• Coalesces automatic retries for the same attempt while allowing /reco and /connect to replace the pending settings snapshot until commit.
• Restores held bots before login so every rejection reaches the configured AutoRelog instance exactly once.
• Keeps an attempt-owned offline console route active through reconnect delays and hands it to the next client attempt.
• Adds deterministic regression coverage for failure claims, duplicate scheduling, retry rollback, settings replacement, stale cleanup, and console-route ownership.

Root cause

A login rejection was reported twice. Protocol18.Login() first delivered the real LoginRejected reason and server message, then returned false; the McClient constructor independently synthesized a second ConnectionLost decision. The restart coordinator could accept that duplicate after the first request entered its callback, causing two retry reservations and two reconnect waits for one rejection.

The reconnect path also removed the client input route without installing an offline route. Commands entered during a delay were silently discarded. In addition, bots held across restart were restored only after a successful login, so they were absent on alternating failed attempts.

Fix

Area	Change
Failure ownership	A per-client atomic lifecycle claim makes one callback the owner of disconnect cleanup and suppresses the constructor fallback after a protocol rejection.
Restart ownership	Restart requests carry their source connection attempt. Automatic same-attempt duplicates are rejected; explicit commands may update the existing request only before its atomic commit point.
Stale work	The worker validates the source attempt before disconnect and after delay, then commits the latest immutable settings snapshot exactly once.
Bot lifecycle	Held bots are restored before Login() so original rejection reasons and messages reach them on every attempt.
Console routing	The offline route is owned by an attempt, remains active during delay, transfers to the next attempt, and can only be cleared by its current owner.
Operator guidance	The AutoRelog documentation recommends a randomized 3 to 10 second delay for multi-process deployments while preserving fixed-delay compatibility.

Automated validation

• source tools/mcc-env.sh && mcc-build: passed with 0 errors. Existing NuGet vulnerability warnings remain.
• Focused regression suite: passed, 31/31 tests after the independent-review fixes.
• Complete MinecraftClient.Tests suite: passed, 31/31 tests after the independent-review fixes.
• git diff --check: passed.
• PR fix: preserve queued disconnect reason #3187 final packet drain remains present: while (!packetQueue.IsCompleted).

The docs build reached the repository's existing missing-translation-file problem before rendering. No translation files or user-visible C# strings were changed.

Real-server proof

All Minecraft clients used random offline-mode bot names and isolated temporary configs under /tmp/mcc-debug. No personal account or repository-root config was used.

Version / scenario	Result
Vanilla 1.20.4, ignore kick text	15 real whitelist rejections, 15 AutoRelog waits, and 14 completed restarts at capture. Every completed cycle had a 1:1 failure-to-wait ratio; the prior baseline was 2:1.
Vanilla 1.20.4, nonmatching filter	One rejection, zero automatic waits, and a usable offline prompt. /help produced command output and /quit exited.
Vanilla 1.20.4, exact filter	One wait per rejection with the original whitelist text preserved. /help, /reco, and /connect were accepted during the delay. A replacement /connect localhost:25566 committed and failed at that port; replacing it with localhost:25565 let the same process join after whitelist removal.
Vanilla 1.21.11, ignore kick text	12 real whitelist rejections, 12 waits, and 11 completed restarts at capture. Every completed cycle remained 1:1.
Vanilla 1.21.11, nonmatching filter	One rejection, zero waits, a usable offline prompt, and /quit handled through that route.
Vanilla 1.21.11, exact filter	7 real whitelist rejections and 7 waits. After whitelist removal, the same process successfully joined.
Vanilla 1.21.11, post-review minimum-delay stress	At a fixed 0.1-second delay, the random bot m3186r35cbdf completed 203 real whitelist rejections, 203 waits, and 203 restarts. After whitelist removal, that same process joined successfully and handled quit cleanly.
Vanilla 1.21.11, seven-instance exact-match test	Seven random bots used Ignore_Kick_Message=false, the exact whitelist rejection text, and a fixed 3-second delay against one server. Every bot completed exactly 10 rejections, 10 matching waits, and 10 restarts with zero fatal errors. After whitelist removal, all seven processes joined without being restarted, and all seven handled quit cleanly.
Ten-process continuity soak	Ran for 20 minutes with randomized 3 to 10 second delays. All ten workers remained active with 182 to 193 failures each, exactly 182 to 193 waits, 181 to 192 completed restarts, and zero fatal exits. The one-request difference was each worker's in-flight delay at capture.

The server logs independently recorded m3186e6d086c joined the game on 1.20.4 at 15:56:02 and m3186e6d086f joined the game on 1.21.11 at 16:00:32. Both were the same MCC processes that had been cycling through whitelist rejections.

Preserved proof logs:

• /tmp/mcc-debug/issue3186-fixed-1204-ignore/mcc-debug.log
• /tmp/mcc-debug/issue3186-fixed-12111-ignore/mcc-debug.log
• /tmp/mcc-debug/issue3186-fixed-12111-match/mcc-debug.log
• /tmp/mcc-debug/issue3186-review-races/mcc-debug.log
• /tmp/mcc-debug/issue3186-review-seven/
• /tmp/mcc-debug/issue3186-fixed-stress/

The shared 1.21.11 server was left running and its whitelist was restored to off. Server-side handleDisconnection() called twice warnings are vanilla cleanup messages and did not correspond to duplicate MCC retry decisions.

The ten-process run was a local generic transport-failure continuity soak because the sandbox denied its child processes permission to open sockets. It can prove that all workers keep cycling with one retry decision per failed attempt, but not server admission. Ten-client server admission and the fault-proxy recovery matrix remain unexecuted because the environment stopped granting unsandboxed process launches. The single-client server recovery matrix passed on both requested protocol generations, so this PR does not claim to identify or bound every possible synchronous network stall.

Process notes

• The independent review found two race conditions. Route preparation now completes before coordinator publication, and automatic retries now await the source client's disconnect completion before starting the next attempt.
• The existing branch name was retained even though it does not include the issue number.
• Unrelated user-owned changes were preserved and excluded from commits.

Fixes #3186

[milutinke]

• Implemented immutable connection-attempt ownership, duplicate retry coalescing, held-bot restoration, and offline-route handoff for [BUG] The bot doesn't try to reconnect while the server is under maintenance. #3186.
• Build passes with 0 errors; the complete regression suite passes 31/31 tests on the reviewed branch.
• Real whitelist-rejection and recovery scenarios passed on vanilla 1.20.4 and 1.21.11 using random offline bot names.
• Environment limitations for ten-client server admission and the fault-proxy matrix are documented in the PR body.
• Independent review findings, fixes, severity, confidence, and proof are posted; next action is artifact archival and final verification.

[milutinke]

🔍 Automated Code Review

Summary

The independent diff review found two medium-severity race conditions and one low-severity coverage gap. Both races were fixed in 8fde46db, the coverage was expanded, the complete suite passes 31/31, and the fixes were exercised against a real server at both the 0.1-second minimum delay and the requested seven-client 3-second scenario.

Findings

✅ Strengths

• Failure ownership is bound to an immutable connection attempt, so one login rejection produces one preserved reason, one retry reservation, and one restart.
• Automatic duplicates are coalesced, explicit settings replacement remains available until commit, and stale generations are rejected before side effects.
• Held bots and the offline console route survive failed login attempts without reintroducing PR fix: preserve queued disconnect reason #3187's final-packet regression.
• No public API expansion, authentication change, dependency change, or hardcoded user-facing text was introduced.

✅ Resolved findings

1. Offline route could be installed after a zero-delay restart had already installed the client route

   • Original severity: Medium
   • Confidence: High
   • Resolution: RestartCoordinator.TrySchedule() now executes a preparation gate before the request is written to the worker channel (MinecraftClient/RestartCoordinator.cs:71-114). Program.TryRestart() prepares the attempt-owned route through that gate, and stale attempts cannot activate an empty route (MinecraftClient/Program.cs:1101-1123, MinecraftClient/ConnectionAttemptLifecycle.cs:50-59).
   • Proof: deterministic preparation-before-execution and rejected-preparation tests in MinecraftClient.Tests/RestartCoordinatorTests.cs:6-61, plus stale-route coverage in MinecraftClient.Tests/McClientConnectionFailureTests.cs:83-93.

2. The next failed-login attempt could begin before source-client cleanup completed

   • Original severity: Medium
   • Confidence: Medium
   • Resolution: instance AutoRelog requests now carry the originating client's disconnect-completion task (MinecraftClient/ChatBots/AutoRelog.cs:171-177, MinecraftClient/McClient.cs:241). The coordinator awaits that task before invoking restart execution (MinecraftClient/RestartCoordinator.cs:176-179), and explicit replacements preserve the original cleanup gate.
   • Proof: faulted-cleanup execution suppression in MinecraftClient.Tests/RestartCoordinatorTests.cs:63-88; real-server minimum-delay stress completed 203 rejections, 203 waits, and 203 restarts before the same process joined.

3. Production integration seams had only helper-level automated coverage

   • Original severity: Low
   • Confidence: High
   • Resolution: coordinator publication and cleanup gates now have deterministic tests, and stale route activation has direct coverage. Static Program orchestration remains expensive to isolate, so it is additionally covered by executed real-server tests.
   • Proof: complete suite 31/31; seven exact-message clients each completed 10 rejections, 10 waits, and 10 restarts at a fixed 3-second delay, then all seven joined without MCC process restarts.

⚠️ Remaining non-blocking limitations

• The ten-client 20-minute continuity soak ran under sandbox-injected transport failure rather than real server admission. Real server admission was proven separately with seven concurrent clients and with single-client recovery on 1.20.4 and 1.21.11.
• The fault-proxy graceful/reset matrix remains unexecuted because the environment would not grant the additional unsandboxed proxy launch. This does not affect the proven duplicate-decision and ownership fixes.

🔒 Security

No security concerns were identified in the diff. The change adds no new external input surface, secret handling, authentication behavior, or dependency. Existing package vulnerability warnings are unchanged and outside this PR's dependency diff.

Checklist

• Fix addresses the root cause from the investigation
• Independent medium-severity findings resolved
• Code follows codebase patterns
• Complete test suite passes, 31/31
• Real-server matching and multi-client recovery verified
• No obvious security regression introduced

---

Self-reviewed by Codex • Ready for human review