[F16] mxaccess: real Session::recover_connection (re-bind + re-advise)
rust / build / test / clippy / fmt (push) Has been cancelled

Closes F16. Replaces the wave-2 no-op recover_connection with the
full .NET-equivalent shape (MxNativeSession.cs:399-474). Three
pieces:

1. Subscription registry on SessionInner.
   New subscriptions: Mutex<HashMap<[u8; 16], SubscriptionEntry>>
   tracks every active advise. subscribe() inserts after a successful
   AdviseSupervisory; unsubscribe() removes on the success path only
   (failed UnAdvises stay registered so next recovery replays them).
   The consumer's Subscription handle still holds the BroadcastStream;
   the registry is purely for AdviseSupervisory replay.

2. Pluggable RebuildFactory.
   New public typedef:
     pub type RebuildFactory = Arc<
         dyn Fn() -> Pin<Box<dyn Future<Output = Result<NmxClient,
                                                        NmxClientError>>
                            + Send>>
             + Send + Sync,
     >;
   Installed via Session::set_recovery_factory(factory);
   queryable via has_recovery_factory(). Kept separate from
   connect_nmx / connect_nmx_auto so existing constructors stay
   non-breaking — consumers opt in by calling the setter
   after-the-fact.

3. Real recover_connection + recover_connection_core.
   recover_connection is the retry loop (mirrors cs:399-440): for
   attempt in 1..=policy.max_attempts, emit RecoveryEvent::Started
   → call recover_connection_core → on Ok emit Recovered + return,
   on Err emit Failed{will_retry, error}, sleep policy.delay, retry,
   or bubble the last error.

   recover_connection_core mirrors cs:442-474: rebuild NMX via the
   factory → RegisterEngine2 with the saved callback_obj_ref → optional
   SetHeartbeatSendInterval → snapshot the registry under the lock,
   replay AdviseSupervisory(correlation_id) for each entry → atomically
   swap *nmx_lock = replacement. Old NmxClient drops at end of scope,
   closing its TCP transport.

Subscription correlation ids are preserved across the swap so the
consumer's Subscription stream continues to receive on its existing
broadcast filter. The CallbackExporter stays bound across recoveries
— no TCP listener re-bind.

R15's "long-lived connection task" was listed as a hard prereq, but
the existing Mutex<NmxClient> already serialises concurrent ops
during the rebuild — recover_connection_core holds the inner mutex
during the swap, concurrent ops just wait. Functionally equivalent
to the long-lived-task design.

New ConfigError::RecoveryNotConfigured returned when
recover_connection is called without a factory installed. New
public re-export: RebuildFactory.

Tests (mxaccess 65 → 67):
  - recover_connection_without_factory_returns_recovery_not_configured
  - recover_connection_with_always_failing_factory_exhausts_attempts
    (pins (Started, Failed)×3 + final will_retry=false + bubbled
    TransportFailure)
  - subscribe_populates_registry_unsubscribe_clears_it
  - recovery_events_supports_multiple_subscribers (updated for the
    new factory-required path)

connect_nmx_auto-side auto-population of the factory (capturing the
ntlm_factory + discovered (addr, service_ipid) so consumers don't
re-author the closure) is a future polish — not required to close
F16.

design/followups.md: F16 moved to Resolved.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-06 01:57:43 -04:00
parent 904f211aba
commit ff4ea4d5a9
3 changed files with 396 additions and 49 deletions
+23 -5
View File
@@ -131,14 +131,32 @@ The fixture is captured by `MxAsbClient.Probe --dump-deterministic-hmac` (`src/M
**Resolves when:** The .NET reference adds bodies for opnums 4 / 5 (or a captured frame establishes the on-wire shape). At that point port them into `rem_unknown.rs` alongside the existing `RemQueryInterface` codec.
### F16 — Real `Session::recover_connection` reconnect loop (re-bind + re-advise)
**Severity:** P1
**Source:** M4 wave 2/3 boundary, `crates/mxaccess/src/session.rs`
**Why deferred:** Wave-2 `Session::recover_connection` validates the policy and emits `RecoveryEvent::Started` + `RecoveryEvent::Recovered` on each call but does **NOT** actually tear down + re-establish the NMX transport / re-advise active subscriptions. The .NET reference's `RecoverConnectionCore` (`MxNativeSession.cs:442-474`) does all three: builds a replacement `ManagedNmxService2Client` via `CreateRegisteredService`, re-`Connect`s every `_publisherEndpoints` entry, re-`AdviseSupervisory`s every entry in `_subscriptions`, then atomically swaps the old service for the new one. Porting this to Rust requires (a) tracking the active subscriptions inside `SessionInner` (currently they're owned by the consumer's `Subscription` handles, with no central registry); (b) the long-lived connection task per R15 in `design/70-risks-and-open-questions.md` so swap-in-place is safe under concurrent operations; (c) a way to re-create the `CallbackExporter` (or keep the existing one bound while the underlying transport is replaced — needs design work).
**Resolves when:** R15's long-lived connection task lands and `SessionInner` gains a subscription registry. At that point the recover loop becomes ~50 lines: for `attempt in 1..=max_attempts`, emit Started → drop+rebuild NmxClient → `register_engine_2` with the existing OBJREF → re-advise every registered correlation_id → emit Recovered (or Failed + sleep delay + continue, mirroring the `cs:407-440` shape exactly).
## Resolved
### F16 — Real `Session::recover_connection` reconnect loop (re-bind + re-advise)
**Resolved:** 2026-05-06 (commit `<this commit>`). Replaces the wave-2 no-op `recover_connection` with the full .NET-equivalent shape (`MxNativeSession.cs:399-474`).
Three pieces, all in `crates/mxaccess/src/session.rs`:
1. **Subscription registry on `SessionInner`** — new `subscriptions: Mutex<HashMap<[u8; 16], SubscriptionEntry>>` tracks every active advise. `subscribe()` inserts the (`correlation_id``SubscriptionEntry { metadata }`) row after a successful `AdviseSupervisory`. `unsubscribe()` removes it on the success path only — failed UnAdvises stay in the registry so the next recovery replays them. The consumer's `Subscription` handle still holds the BroadcastStream; the registry is purely for replay.
2. **Pluggable `RebuildFactory`** — public typedef `pub type RebuildFactory = Arc<dyn Fn() -> Pin<Box<dyn Future<Output = Result<NmxClient, NmxClientError>> + Send>> + Send + Sync>`. Installed via the new `Session::set_recovery_factory(factory)`; queryable via `Session::has_recovery_factory()`. Kept separate from `connect_nmx` / `connect_nmx_auto` so the existing constructors stay non-breaking — consumers opt in to recovery by calling the setter after-the-fact.
3. **Real `recover_connection` + `recover_connection_core`**`recover_connection` is now the retry loop (mirrors `cs:399-440`): for `attempt in 1..=policy.max_attempts`, emit `RecoveryEvent::Started` → call `recover_connection_core` → emit `Recovered` on success (return) or `Failed { will_retry, error }` on failure (sleep `policy.delay`, retry, or bubble the last error after the budget is exhausted). `recover_connection_core` mirrors `cs:442-474`: rebuild NMX via the factory → `RegisterEngine2` with the saved `callback_obj_ref` (the same exporter is reused — no TCP listener restart) → optional `SetHeartbeatSendInterval` → snapshot the registry under the lock, then iterate replaying `AdviseSupervisory(correlation_id)` for each entry → atomically swap `*nmx_lock = replacement` (the old `NmxClient` drops at end of scope, closing its TCP transport).
Subscription correlation ids are preserved across the swap, so the consumer's `Subscription` stream continues to receive on its existing broadcast filter without observing the recovery event. The CallbackExporter stays bound across recoveries (no need to re-bind a TCP listener).
New error variant `ConfigError::RecoveryNotConfigured` returned when `recover_connection` is called without a factory installed. New public re-export: `RebuildFactory`.
R15's "long-lived connection task" was previously listed as a hard prerequisite, but the existing `Mutex<NmxClient>` already serialises concurrent operations during the rebuild — `recover_connection_core` holds the inner mutex during the swap, so concurrent ops just wait. Functionally equivalent to the long-lived-task design.
**Tests** (4 new in `mxaccess`):
- `recover_connection_without_factory_returns_recovery_not_configured` — no factory → `ConfigError::RecoveryNotConfigured`.
- `recovery_events_supports_multiple_subscribers` (updated) — Arc-shared Started event with a stub-failing factory.
- `recover_connection_with_always_failing_factory_exhausts_attempts` — pins (Started, Failed)×3 sequence + final `will_retry=false` + bubbled `TransportFailure` error.
- `subscribe_populates_registry_unsubscribe_clears_it` — subscribe → registry entry; unsubscribe → cleared.
Workspace `mxaccess` 65 → 67 tests; default-feature clippy clean. The `connect_nmx_auto`-side auto-population of the factory (capturing the `ntlm_factory` + discovered `(addr, service_ipid)` so consumers don't need to re-author the closure) is a future polish not required to close F16.
### F33 — Live wire reconciliation for the ASB subscription path
**Resolved:** 2026-05-06 (commits `218f4c4`, `7a5f251`, `<this commit>`). `MX_ASB_TRACE_REPLY` capture during investigation revealed the live MxDataProvider returns a `Result` wrapper with `<resultCodeField>1</>` + `<successField>false</>` followed by **empty** `<ASBIData/>` payloads when it short-circuits on `InvalidConnectionId` — the same transient race F31 fixed for `RegisterItems`. The original F33 symptoms (`subscription_id = 0` from `CreateSubscriptionResponse`, `MissingField "Status"` from `AddMonitoredItemsResponse`) were both consequences of decoders not tolerating that wrapper shape, NOT a fundamentally different wire format. Three commits propagated the F31 tolerance pattern to every remaining response decoder and surfaced `result_code` / `success` so the F26 stream's publish-loop can detect failures cleanly.