docs(h264): disconnect-side capture — on_close never fires (structural) - #7
Merged
Conversation
…ructural) Ran the disconnect-side instrumentation on real mstsc (2026-06-22). Result: EGFX `on_close` NEVER fires — and it's structural, not client-dependent. The vendored ironrdp-server teardown (client_loop → RunState::Disconnect → on_disconnected) does not call close() on the DVC channels, so the EGFX on_close() callback is never invoked on any disconnect; macrdp only learns the old connection died indirectly (its ship loop sees the output channel drop) after the next connection has already begun. Conclusion: there is NO disconnect-time hook — the "DeleteSurface before teardown" idea is dead (no callback to send from, socket already gone). The only remaining lever is a connect-time DeleteSurface(0) on reconnect, which needs re-vendoring egfx, can't reliably tell a reconnecting mstsc (holds surface 0) from a fresh one, and broke mstsc before — left unpursued. The disconnect-side avenue is conclusively closed; documented client limitation stands, with the reliable close+reopen recovery (a fresh mstsc process, verified to render). Docs-only; the instrumentation itself shipped in the prior PR.
clintcan
added a commit
that referenced
this pull request
Jun 23, 2026
…econnect blank) (#10) * experiment(arch): --fork-workers PoC — spawn a fresh worker process per connection EXPERIMENTAL, NOT for main. Tests whether xrdp-style process-freshness fixes the mstsc H.264 reconnect blank. With --fork-workers, a thin supervisor binds the port and fork+execs a fresh copy of this binary per RDP connection, handing it the already-accepted socket fd via MACRDP_WORKER_FD; the worker serves exactly one connection then exits. Mirror-primary only (headless modes need a persistent display owner, out of PoC scope). If mstsc re-composites on reconnect, process- freshness is the fix; if not, it's a wire difference and we drop this. * fix(fork-workers): force worker process::exit after the connection The 'first two reconnects render, then blank' pattern is server-side resource exhaustion, not mstsc's surface cache (it reproduces with a fresh mstsc after a Windows reboot or close+reopen — an empty client cache would render). Root cause: the worker did 'return res' after run_connection, letting the runtime unwind. But once an SCStream is live, ScreenCaptureKit framework threads keep the process from actually terminating on a normal return (the same hazard the SIGINT handler dodges with std::process::exit). So each worker LINGERED holding its capture stream; after ~2 concurrent capturers macOS starves frame delivery to newer workers and the 3rd+ reconnect is blank. Fix: std::process::exit(code) in the worker branch after the connection, so the SCStream/VT session tears down immediately (and the 'caffeinate -w <pid>' child exits with us). Each reconnect's fresh worker then captures cleanly. Recasts the residual fork-PoC blank from 'unfixable mstsc client bug' to a server-side worker-lifecycle bug. * fix(fork-workers): serialize workers — drain previous before next captures After forcing the worker to process::exit, the hard 'blank after 2' cliff became an INTERMITTENT blank (e.g. desktop,desktop,blank,desktop,desktop,...). That residual is a startup race: process::exit kills the old worker, but the OS + ScreenCaptureKit daemon don't release its capture slot instantly, and the supervisor forks the next worker on accept — so a fast reconnect overlaps the new worker's SCStream with the dying one's not-yet-released slot → blank. macrdp mirror-primary is single-session, so serialize in the supervisor: before spawning the next worker, wait (bounded by WORKER_WAIT_TIMEOUT=3s) for the previous worker to fully exit, then a short WORKER_SCK_SETTLE=250ms beat for SCK's daemon to free the slot. That same wait reaps the child, so the old concurrent reaper is dropped (no zombies). Timeout guards against a stuck old session wedging reconnects. Layered on the process::exit fix: together they target the full lifecycle race behind the fork-PoC reconnect blank. * fix(fork-workers): default SCK settle 250→750ms + env-tunable 2/11 reconnects still blanked, and ADJACENT (#6,#7) — a blank worker's failed SCStream releases the capture slot more slowly than a clean one, so 250ms after the previous worker exits isn't always enough. Bump the default inter-worker settle to 750ms and make it overridable via MACRDP_WORKER_SCK_SETTLE_MS so the floor can be tuned per-machine without a rebuild (hardware-in-the-loop). If a fixed settle proves insufficient at any value, the robust fix is a worker-side SCStream self-heal (first-frame watchdog + rebuild, like audio.rs). * fix(fork-workers): default SCK settle to 0 (settle never helped — residual is client-side) Diagnostic run (RUST_LOG capture=debug, settle=0): 1 blank of 7 reconnects, and the BLANK connection's worker logged 'capture: first frame delivered' + 'EGFX surface created + mapped' + 'first EGFX frame shipped' + 'ended cleanly' — byte-for-byte identical to the rendering connections. So the residual blank is NOT a capture/server problem (worker-side SCStream self-heal would do nothing); it's the mstsc client re-composite quirk, now rare because the fork model usually dodges it. settle=0 outperformed 750ms (which was worse), confirming the settle chased the wrong cause. Keep process::exit (the real fix) + the serialization wait (drains the previous worker); drop the settle to 0. Knob retained for experimentation. * spike(fork-workers): supervisor-owned virtual display, worker captures by id (Phase 0) Validates the make-or-break productization unknown: can a worker PROCESS capture a CGVirtualDisplay created and owned by the SUPERVISOR process? - Supervisor (--fork-workers --virtual-display --width W --height H): creates the vdisplay once, holds it for its lifetime (persists across worker reconnects), hands its display_id to each worker via MACRDP_VD_ID. - Worker: when MACRDP_VD_ID is set, skips creating its own vdisplay and captures the supervisor-owned one by id (geometry from its CGDisplayBounds; input picks up the origin the same way). New geometry branch in async_main. - Relaxed the supervisor's hard reject: --virtual-display is now allowed with --fork-workers; --capture-primary/--detach-primary still rejected (need supervisor-owned blanking, Phase 1 item 2). Spike only (mirror-primary + this). If cross-process capture works, Phase 1 is unblocked; if not, the supervisor-owned-vdisplay model must be rethought. * feat(fork-workers): Phase 1 — supervisor-owned headless blanking + worker cleanup Builds on the passed Phase-0 spike (supervisor-owned vdisplay, stable cross-process capture). Productizes the headless path: - Item 2 (supervisor-owned blanking, CONTINUOUS): --capture-primary / --detach-primary / --make-primary now work WITH --fork-workers. The supervisor engages the chosen blanking on the FIRST connection and HOLDS it for its lifetime (off the accept loop, since install can block seconds). Process-scoped → auto-restores on supervisor death (incl. SIGKILL). Workers are gated OUT of installing blanking (they own no vdisplay; the old per-worker path would panic). Removed the Phase-0 rejection of these flags; added the vdisplay-required + capture/detach-mutually-exclusive checks to the supervisor branch. - Item 3 (worker cleanup before process::exit): the worker now runs file_promise_lazy::shutdown_cleanup() + rdpdr::shutdown_cleanup() before exiting, so RDPDR NFS mounts + clipboard paste temp dirs don't leak per reconnect (process::exit skips Drop). Item 1 (supervisor-owned vdisplay) already landed with the spike. Decision A (blanking lifecycle) = continuous (engage-on-first-connect, restore-on-exit). Still on egfx-fork-workers-poc, NOT main. * feat(fork-workers): Phase 3 — deployable via config.env/LaunchAgent + supervisor caffeinate Wire --fork-workers into the deployed (launchd) path: - args_from_config: FORK_WORKERS=1 -> --fork-workers (+ test coverage in the config-parsing suite; empty-config asserts the default-off). - config.env.example: document FORK_WORKERS (experimental, opt-in). - caffeinate -> supervisor: an always-on headless server must stay awake across the brief gaps between worker reconnects, so the SUPERVISOR holds one 'caffeinate -w <pid>' for its lifetime; workers skip their own (gated on worker_fd) to avoid per-reconnect churn. Single-process path unchanged. LaunchAgent plist needs NO change: it launches 'macrdp --config <file>' (the supervisor); the supervisor's base_args is '--config <path>', and each worker re-expands it but takes the worker branch because MACRDP_WORKER_FD is set (precedence over --fork-workers -> no recursion). KeepAlive watches the supervisor; AbandonProcessGroup defaults false so 'launchctl bootout' kills the worker children too, and supervisor-owned blanking auto-restores on its death. Still on egfx-fork-workers-poc, NOT main. * feat(fork-workers): Phase 2 — supervisor-owned app-switcher HUD helper The macrdphud helper LISTENS on :40243; macrdp pushes SHOW/ADVANCE/HIDE to it (display id rides in each SHOW). Per-worker that means each worker spawns its own helper, churning the process and contending on :40243 across reconnects. Move ownership to the supervisor: it spawns ONE persistent helper (parent=supervisor, self-exits with it); workers only push to it (they keep set_display_id + set_app_switcher_hud, just skip spawning — gated on worker_fd). Single-process (non-fork) path unchanged. Matches the vdisplay/blanking/caffeinate ownership. Smart-card (:40242) is per-connection by nature (the card lives on the client, the bridge talks to the live worker's RdpdrHandle) so the supervisor can't own it — left per-worker, to be verified with real hardware. * docs(fork-workers): Phase 4 — document the feature + smart-card compat warning - known-quirks.md: the H.264 reconnect-blank note's 'not server-fixable / STOP' conclusion is OVERTURNED — append an UPDATE documenting that --fork-workers (opt-in) largely fixes it via xrdp's process model (fresh worker per connection dodges layer 2), the ~1/7 residual + reconnect-once-more recovery, the worker-process::exit + serialization fixes, and that teardown-delete was abandoned. Smart-card under fork = UNVERIFIED. - CLAUDE.md: add the --fork-workers flag to the CLI block. - architecture.md: add the supervisor/worker process-model note (ownership split, MACRDP_WORKER_FD precedence, launchd interaction). - main.rs: startup WARNING when --enable-smartcard-redirection is combined with --fork-workers (per-connection :40242 bridge, slotd reconnect across workers untested) — not gated off, just flagged. Phase 4 done. fork-workers is documented + functionally complete; open items are smart-card verification (needs hardware) and the merge-to-main/ship decision. * feat(gui): expose fork-workers in the controller; preset turns it on - 'Set Up Remote Desktop' preset now writes FORK_WORKERS=1 (reconnect to a still-running server renders instead of going blank on mstsc — a fresh worker process per connection dodges the EGFX surface-retention bug). - New menu toggle 'Per-connection workers (reconnect fix)' (FORK_WORKERS) + toggleForkWorkers handler, matching the other flag toggles. macrdptray builds clean. * docs(fork-workers): update READMEs (root, packaging, gui) Phase 4 missed the READMEs. Add fork-workers to: - README.md: --fork-workers in the CLI flag list, and the 'Known limitations' reconnect-blank note now leads with --fork-workers as the (experimental) fix (process-per-connection, ~1/7 residual → reconnect once more) alongside the close+reopen recovery. - packaging/README.md: FORK_WORKERS config knob. - gui/README.md: preset now lists per-connection workers; Options gains the 'Per-connection workers (reconnect fix)' + 'App-switcher HUD' checkmarks. --------- Co-authored-by: Clint Christopher Canada <clint.canada@clubitech.com>
clintcan
added a commit
that referenced
this pull request
Aug 6, 2026
#178) Bumps the IronRDP pin 879ffed8 -> a5d1c682 (133 commits); no user-facing behavior change. Retires the ironrdp-rdpeusb fork (URBDRC server ported to the split PDUs) and drops the ironrdp-async fork (a5d1c682 carries #1515's find_size DoS fix). Absorbs upstream #1359 (rdpsnd) / #1345 (egfx) / honor-size into src/. Harvests divergence #7 (QOI --qoi-force-rgb; converges to upstream always-Rgb). Version 0.9.3 -> 0.9.5. Green on CI (linux+macos build/test/clippy/fmt, CredSSP audit e2e, cargo-deny) + a 24h+ clean soak. Net: -2 vendored forks, -1 divergence.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Records the result of running the disconnect-side EGFX instrumentation (from #6) on real mstsc.
Result:
EGFX on_closenever fires — structural. The vendored ironrdp-server teardown doesn't callclose()on the DVC channels, so the EGFXon_close()is never invoked on any disconnect. There is no disconnect-time hook → the "DeleteSurface before teardown" idea is dead (no callback, socket already gone). The only remaining lever is a connect-timeDeleteSurface(0)on reconnect — needs re-vendoring egfx, can't reliably tell a reconnecting mstsc from a fresh one, broke mstsc before — left unpursued.The user's capture also confirmed close+reopen mstsc = a fresh process (empty surface cache) = renders cleanly (the documented recovery, not the blank).
Net: disconnect-side avenue conclusively closed; documented client limitation stands. Docs-only (the instrumentation shipped in #6).