fix(ironrdp-server): bound accept_finalize — a wedged client held the session slot

[antonmos]

What

Bounds accept_finalize with a new FINALIZE_TIMEOUT (30 s), so an authenticated client that wedges mid-handshake can no longer hold the live-session slot indefinitely. Vendored-server divergence (24).

Why

Windows App for macOS build 68614 intermittently hangs the user at "Configuring remote session". ironrdp_acceptor=trace on a live hang shows the server completing the entire handshake — X.224, TLS, CredSSP (including the HYBRID_EX EarlyUserAuthResult), MCS Connect, Erect Domain, Attach User and all seven channel joins — and then blocking on a read, because the client never sends its Client Info PDU.

That part is client-side and not fixable here:

• the server's ConnectResponse is byte-identical between hung and successful connections;
• tcpdump during a live hang captured nothing, with the socket ESTABLISHED and Recv-Q=0 and Send-Q=0;
• two consecutive fresh connections from the same client process both wedged, and only restarting the client cleared it.

Same class as the documented mstsc EGFX surface-retention quirk: stale client-process state.

The server bug is that it parked on that read forever. All three accept_finalize call sites (serve_negotiated, both arms of run_connection) were unbounded awaits, so a wedged client held the session slot — one such connection sat there 45 minutes. #180 bounded only the pre-auth candidate probe (CANDIDATE_NEGOTIATION_TIMEOUT) on the other half of the accept path; this is the same hazard on the serving path.

Reviewer notes

Placement is load-bearing. The bound goes on the inner ironrdp_acceptor::accept_finalize call, not on RdpServer::accept_finalize or its callers. That method is a loop whose body calls client_accepted, which runs the entire live session — a timeout hoisted to the loop or a call site would cap session length. Per-pass placement also gives a deactivation–reactivation its own budget and bounds one that wedges the same way.

Why 30 s. Measured across 197 successful connections in the field logs: median finalize 0.07 s, all but 13 samples under 1 s, slowest ever 4.97 s. So 30 s is ~6× the observed worst case. It is deliberately generous because the same budget covers reactivations (live resize, blank recovery), whose tail is not measurable from these logs — the fingerprint only fires on the first pass. A false timeout is cheap (drop + auto-reconnect); a false drop of a live session mid-reactivation is not. Open to tightening to ~15 s, or making it env-tunable (MACRDP_FINALIZE_TIMEOUT_SECS) in the style of MACRDP_GUARD_* / MACRDP_BLANK_RECOVERY_* — say the word.

Note this bounds the damage, it does not fix the hang. The client still wedges; it now gets dropped in 30 s instead of parking the slot. The cure remains restarting the client.

Theories refuted en route (recorded in the divergence log so they are not re-chased): path MTU (ping -D -s 1472 from the client succeeds); a missing HYBRID_EX EarlyUserAuthResult (the trace shows result=Ok(()) on the hung connections too); client-type correlation (the iOS client also hangs — 4/4 from one address, 3/3 fine from another); network-path correlation (the same LAN IP both hangs and succeeds); server startup state (the first connection after a restart hung).

Testing

New regression test a_client_that_wedges_during_finalize_is_dropped — a client completes X.224 + TLS then deliberately never sends its Connect Initial.

• Verified to fail without the fix: the test hangs (killed at the 150 s mark, exit 144).
• Passes with it in 0.04 s.
• Runs under #[tokio::test(start_paused = true)] so the 30 s bound costs the suite no wall-clock time. Caveat, stated in the divergence log: paused time makes it prove a bound exists, not the specific duration.
• tokio's test-util is added as a dev-dependency only — cargo build does not pull dev-deps, so it never reaches the shipped binary.

Full suite 206 passed, 0 failed; cargo clippy --all-targets -- -D warnings clean; cargo fmt applied.

🤖 Generated with Claude Code

[clintcan]

Thanks @antonmos — the investigation here is excellent. Proving the server
completed every handshake stage and then showing tcpdump capturing nothing
on an ESTABLISHED socket with both queues empty is about as clean a
client-side-wedge diagnosis as you can get, and recording the refuted theories
in the divergence log is exactly right.

I checked the load-bearing parts and they hold up:

• Placement is correct. The bound is on the inner
  ironrdp_acceptor::accept_finalize, with client_accepted on the next
  statement outside it, so session length isn't capped and a
  deactivation–reactivation gets a fresh budget each pass. Your reviewer note
  about not hoisting it is right, and this is the one spot that works.
• The tokio/test-util dev-dependency does not reach the shipped binary —
  confirmed with cargo tree --no-dev-dependencies (edition 2021 ⇒ resolver v2).
• A 30 s drop lands well outside MACRDP_GUARD_FAILFAST_SECS (~3 s), so a
  wedged client that gets dropped won't accrue toward an auth-guard lockout.

Two things I'd like changed before merge, one follow-up, and one heads-up.

1. Make it a no-progress deadline, not a total budget

This is the substantive one, and I think it makes your own offer to tighten the
number unnecessary — a no-progress deadline can be both tighter and safer.

The wedge you diagnosed is a client sending zero bytes. An idle deadline
targets that precisely. As a total budget, FINALIZE_TIMEOUT
(vendor/ironrdp-server/src/server.rs:122) instead has to cover the whole
finalize phase: MCS Connect, Erect Domain, Attach User, the seven channel
joins your own trace shows, Client Info, licensing, Demand/Confirm Active,
Synchronize, Control ×2, Font List/Map. Call it 14+ round trips.

The doc comment defends 30 s with "the slowest observed real handshake
(cellular, heavy retransmits) spent 12 s in CredSSP alone, well before this
point." But CredSSP is ~3 round trips. If a link takes 12 s for 3, finalize's
14+ plausibly runs past 30 s on that same link — so that datapoint argues the
budget is too tight, not that it's generous.

I might be wrong about this, and you have the logs: the PR body separately
reports median finalize 0.07 s / max 4.97 s across 197 connections. Is
that cellular connection one of the 197? If it is, its finalize was ≤ 4.97 s
and my extrapolation is dead. If those two figures come from different corpora,
the question is open. Either way I'd still prefer the idle deadline, because it
removes the need to guess.

The failure mode if the number is wrong is the bad kind: finalize dies at the
same point every attempt, the ARC cookie reconnects, the identical handshake
times out again — a slow-but-healthy client goes from "connects eventually" to
"cannot connect at all". Given how much of this project is aimed at high-RTT
ZeroTier/cellular links, that's the regression I'd most want to avoid.

The per-step version is cheap — everything it needs is already public

I went looking for what this would cost, expecting to have to argue for it, and
it turns out to be a smaller change than the one in this PR:

• ironrdp_async::single_sequence_step is already pub (framed.rs:227) — it's
  what accept_finalize loops over;
• Acceptor::get_result() is already pub (connection.rs:308);
• vendored ironrdp-server already depends on ironrdp-async
  (vendor/ironrdp-server/Cargo.toml:50).

So the loop can just live here, no acceptor change and no new acceptor
divergence — roughly:

let mut buf = WriteBuf::new();
let result = loop {
    if let Some(result) = acceptor.get_result() { break result; }
    tokio::time::timeout(FINALIZE_STEP_TIMEOUT, single_sequence_step(&mut framed, &mut acceptor, &mut buf))
        .await
        .map_err(|_| /* same warn + bail as today */)??;
};

That's about ten lines replacing the fifteen already in the diff, and the
divergence stays exactly where it is now. The bound also gets to be tighter —
10 s per step rather than 30 s total — because fourteen slow-but-progressing
steps each pass comfortably while a silent client fails on the very first one.

Worth noting this is plausibly upstreamable, in the same family as #1515 and
#1556: an unbounded await on an untrusted peer, fixed at the primitive. A
per-step bound in accept_finalize would harden every IronRDP server rather
than just macrdp, and acceptor divergence (2) is precedent that the path works.
I'd rather land it there than carry a total-budget divergence indefinitely.

If you'd rather ship the bound now, I'll take your own offer instead: raise
the default and add MACRDP_FINALIZE_TIMEOUT_SECS in the MACRDP_GUARD_* /
MACRDP_BLANK_RECOVERY_* style, and file the per-step version as a follow-up. A
45-minute slot park is worse than a generous total budget, so I don't want this
stuck behind the perfect fix.

2. Divergence number collision — (24) is taken

Heads-up rather than a code problem: I have an unmerged branch that also claims
vendored-server divergence (24), for MS-RDPEAI microphone redirection.
Neither exists on main yet, so both of us saw (24) as free. Whichever lands
second needs renumbering — if it's missed we end up with two (24)s in the log
that a future pin bump reads to decide what to keep, and mishandling a
divergence at bump time is exactly what produced #179. I'll sort the ordering;
just flagging so it isn't a surprise.

3. Follow-up: this bounds only the last leg, and the unbounded half is pre-auth

Not asking for it in this PR, but worth recording. On the serving path,
accept_begin (server.rs:1724), tls_acceptor.accept (1735) and
accept_credssp (1760) are all still unbounded awaits — I confirmed there's no
tokio::time::timeout anywhere in those legs. The candidate half gets this for
free because line 1015 wraps the whole of negotiate_candidate.

So the same connect-then-go-silent wedge one step earlier still parks the
serving connection, and that half doesn't require authenticating — same class
as #180. Impact is limited today (the accept loop keeps servicing
listener.accept(), and an authenticated candidate can preempt), but it means
"can no longer hold the live-session slot indefinitely" is currently true only
after auth. Worth its own issue.

4. Minor: the test hangs instead of failing

src/conn_test.rs:1222 — wedged.read(&mut buf).await has no bound of its own,
so if the fix is reverted or the timeout stops firing, the regression shows up
as a suite hang rather than a failure, which is much harder to attribute in
CI. The runtime is already start_paused, so wrapping it in a 120 s
tokio::time::timeout costs no wall-clock (auto-advance fires the server's 30 s
deadline first while the fix is present) and turns it into a fast, labelled
failure. The assertion also accepts any Err, so it would pass if a future
change rejected the connection at TLS for an unrelated reason.

[clintcan]

Filed the pre-auth follow-up from section 3 as #186 — run_connection's accept_begin / TLS / CredSSP legs, with the main line numbers and the contrast against negotiate_candidate_bounded. Not a blocker for this PR.

[clintcan]

Quick numbering note, @antonmos: #183 has since added its own vendored divergence (24) (the per-served-connection input-reset handle) and looks likely to merge first, so could this one take (25)? My mic branch will take (26). The per-step deadline question in my earlier comment still stands.

[antonmos]

Renumbered the finalize divergence (24) → (25) per your note (#183 keeps (24), your mic branch (26)) — pushed as b031da8, comment-only, test still green.

On section 4 (the test hanging instead of failing): I tried the 120 s tokio::time::timeout wrap and it doesn't hold for this test — under #[tokio::test(start_paused)] with the real loopback socket, once the server's 30 s FINALIZE_TIMEOUT fires and drops the stream, the runtime auto-advances to the 120 s timer before the OS delivers the socket close to the client read, so the outer timeout wins and the passing test goes red (verified: wrap → Elapsed failure, revert → green). The bare read passes precisely because, after the server's lone timer fires, there are no pending timers left, forcing a real-IO wait. A fast-fail bound would need the test moved onto an in-memory duplex (synchronous drop, no IO race) rather than a wrap — leaving that as a follow-up rather than landing a flaky change.

🤖 Addressed by Claude Code

[clintcan]

Thanks — renumber confirmed, and the only .rs changes in b031da8 are the three divergence markers in doc comments.

On section 4, you're right and I'm withdrawing it. I reproduced it on your head: with the 120 s tokio::time::timeout wrap the test fails deterministically with Elapsed in 0.04 s, and reverting it passes. That matches your explanation exactly — the second timer gives the paused runtime somewhere to jump to, so it advances to 120 s instead of parking on real IO and waiting for the socket close. My suggestion assumed the server's earlier deadline would win, and it doesn't once there are two timers.

The duplex follow-up is the right shape, and cheaper than it sounds: src/conn_test.rs is already built around tokio::io::duplex — see the module doc ("over an in-memory tokio::io::duplex pipe — no TCP"), the run_connection-over-duplex helper, and tokio::io::duplex(64 * 1024) in the shared connect path. This test is the outlier that uses a real loopback socket. Dropping the server half of a duplex closes it synchronously, with no OS delivery race, so a fast-fail bound would behave there. Fine as a follow-up.

Still open from the earlier comment: the per-step (no-progress) deadline versus the 30 s total budget — that's the substantive one.

Unrelated heads-up on your red CI: the cargo-deny failure isn't from your branch. RUSTSEC-2026-0285 (rustls 0.23.40, TLS 1.3 handshake messages accepted across encryption-level boundaries) and RUSTSEC-2026-0286 (cryptoki 0.12.0 via sspi) started failing main's daily scan on 2026-09-15. Fixed on main in ac3491f (cargo update -p rustls -p cryptoki); rebase and it should go green.

[antonmos]

On per-step vs the 30 s total budget — I'd rather this live upstream than in the vendored fork.

The bound already merged into IronRDP itself: Devolutions/IronRDP#1890 landed the total-budget FINALIZE_TIMEOUT in master (a8b4b2a7), an identical const to the one here. So my plan is to hold this PR and pick the fix up on the next IronRDP pin bump — dropping vendored divergence (25) outright rather than carrying it, which is also why I'd rather not reshape it into a per-step form here.

On the "is 30 s too tight?" worry: the datapoint settles it. The 12.55 s-CredSSP cellular link you flagged is one of the 197, and its finalize was 0.53 s — finalize doesn't inherit CredSSP's round-trip cost, so the max finalize across all 197 was 4.97 s and 30 s keeps ~6× margin even on that link. Total-budget is safe. Per-step is still the nicer design, and I agree its home is upstream ("land it there rather than carry a total-budget divergence indefinitely") — as a follow-up on IronRDP it hardens every server, and macrdp still just drops the divergence on the bump.

Given that, this doesn't need more automated iteration. Could you take a human review pass and call the disposition? I'm leaning hold-for-the-pin-bump, but I'm happy to merge the total-budget vendored form now if you'd rather #182 land on its own.

[clintcan]

I took a quick look: agreed on waiting for the pin bump, don't reshape, don't land the vendored form.

Reasons for the decision:

1. The bound is already upstream, and it's yours. #1890 merged with the same 30 s bound on the same inner accept_finalize call, so the bump harvests it rather than porting it — divergence (25) never has to exist.
2. The wedge is already self-healing here. macrdp preempts unconditionally, and eviction isn't gated on the incumbent having activated: the ERRINFO_DISCONNECTED_BY_OTHERCONNECTION notice is best-effort and after EVICTION_GRACE the connection is just dropped. So a client wedged in finalize is displaced by the next authenticated connection; the 45-minute park you saw persists only while nobody reconnects. That's a parked slot, not a lockout, which is a fine thing to carry until the bump.
3. Your datapoint settles my 30 s question and thanks, as you mentioned these are from your logs - 0.53 s finalize on the 12.55 s-CredSSP link is the answer to exactly what I was unsure about: finalize doesn't inherit CredSSP's round-trip cost, so the 4.97 s max holds and the margin is real. My arithmetic assumed it scaled with the link and that was wrong.

Numbering: with this held, (25) frees up. #183 keeps (24) and my mic branch takes (25); I'll update my markers.

Leaving this open rather than closing it — the regression test and the diagnosis write-up are worth keeping attached to a live PR, and it doubles as the reminder at bump time. Please don't delete the branch.

One related thread: #186 (the unbounded accept_begin/TLS/CredSSP awaits on the serving path) is the half that isn't upstream. Same play would work there — upstream it and harvest it — if you want to pick it up.

[clintcan]

Thanks for keeping this rebased. One thing after the rebase: #182 now uses divergence (24), which #183 already claims (it's been (24) there since 09-16). With the mic taking (25) in #191, this one should be (26).

Separately, just confirming the plan from 09-17 still holds: we wait for the pin bump to pick up the upstream bound (Devolutions/IronRDP#1890) rather than merge this form. If something has changed, like the hang recurring often enough that waiting isn't acceptable, let me know and we can reconsider.

[clintcan]

Heads-up: my docs commit 17530f6 on main (marking the mic divergence as shipped in v0.9.10) now conflicts with your entry in vendor/ironrdp-server/CLAUDE.md. It's a one-line resolution: keep main's "P3 (packaging, cleanup, docs) done. SHIPPED in v0.9.10 …" line and put your entry after it. The rebase for the (26) renumber will pick it up in the same push. Sorry for the churn.

[clintcan]

Thanks for the (26) renumber and the rebase. Two small things:

• One leftover: the Cargo.toml comment still says "the divergence-24 finalize-timeout regression test". It should be 26.
• fix(ironrdp-server): bound accept_finalize — a wedged client held the session slot #182 and fix(input): held modifiers on mouse events + Ctrl+click→Cmd+click remap #183 both edit the "vendor dir until (2)/(3)/…" list line in vendor/ironrdp-server/CLAUDE.md, so whichever merges second will need a one-line fix. The final line should include both (24) and (26); fix(input): held modifiers on mouse events + Ctrl+click→Cmd+click remap #183's version currently leaves out its own (24).

[antonmos]

Thanks — both fixed in 1d0228b:

• Cargo.toml comment now says divergence-26.
• The "vendor dir until …" header line now lists (24) alongside (25)/(26), so the list is complete.

Rebased onto current main; cargo check --tests passes.

🤖 Generated with Claude Code