Add IServerContext and move server startup tasks onto it (#4193)

## Stacked on #4188

Base is `marcschier/server-seam-analysis`, not `master`.
`IServerInternal.cs` and
`ServerInternalData.cs` both diverge from master on that branch (chunk
4c removed the two
`object` locks), so branching off master would conflict on both files
*and* design
`IServerContext` against a stale interface. GitHub retargets the base to
`master`
automatically when #4188 merges. **Review #4188 first.**

Closes part of #4184.

## What this does

`IServerInternal` is a service locator: 56 members, 30-odd of which hand
out one of 20+
subsystems. A component that only needs to observe the server and act on
it has to take the
whole thing, and no signature tells a reader which parts a component
actually touches.

This adds `IServerContext` — the ambient view of a running server — and
moves
`IServerStartupTask` onto it.

```csharp
public interface IServerContext
{
    ServerState CurrentState { get; }
    ServerObjectState ServerObject { get; }
    IServiceMessageContext MessageContext { get; }
    ServerSystemContext DefaultSystemContext { get; }

    ServerSystemContext CreateSystemContext(ISession session);
    T? FindPredefinedNode<T>(NodeId nodeId) where T : NodeState;
    IEnumerable<T> FindNodeManagers<T>() where T : class;

    ValueTask ReportEventAsync(IFilterTarget e, CancellationToken ct = default);
    ValueTask CloseSessionAsync(OperationContext context, NodeId sessionId,
        bool deleteSubscriptions, CancellationToken ct = default);
    ValueTask DeleteSubscriptionAsync(uint subscriptionId, CancellationToken ct = default);

    void UpdateServerDiagnostics(Action<ServerDiagnosticsSummaryDataType> update);
}
```

Nothing on it returns a subsystem. A component that needs one takes it
as a constructor
dependency, which states the requirement instead of hiding it behind an
ambient handle.

`IServerInternal : IServerContext`, so every existing handle already
satisfies the new seam
and consumers migrate one at a time.

## The member set was measured, not guessed

Every member of `IServerInternal` was counted by read site across `src`,
`samples` and
`tests`, split by area. `Ext` is non-test production consumers, which is
what decides
whether something belongs on an ambient interface.

| Member | InServer | OtherSrc | Sample | Test | **Ext** | On
`ISystemContext`? |
|---|---:|---:|---:|---:|---:|---|
| `NamespaceUris` | 63 | 50 | 34 | 131 | **84** | yes |
| `Telemetry` | 58 | 23 | 35 | 5 | **58** | yes |
| `MessageContext` | 2 | 8 | 7 | 11 | **15** | no |
| `TypeTree` | 42 | 3 | 11 | 10 | **14** | yes, as `TypeTable` |
| `NodeManager` | 66 | 7 | 6 | 74 | **13** | no |
| `DefaultSystemContext` | 21 | 4 | 1 | 7 | **5** | no |
| `IdentityRegistry` | 1 | 4 | 1 | 1 | **5** | no |
| `SubscriptionManager` | 23 | 0 | 0 | 17 | **0** | no |
| `SessionManager` | 21 | 0 | 0 | 15 | **0** | no |
| `RequestManager` | 10 | 0 | 0 | 23 | **0** | no |
| `AggregateManager` | 12 | 0 | 0 | 2 | **0** | no |
| `ModellingRulesManager`, `ConformanceUnitsManager`,
`DiagnosticsEnabled` | 0 | 0 | 0 | 0 | **0** | no |

Two things fall out. The highest-traffic members are already carried by
`ISystemContext`,
which `ServerSystemContext` copies off the server at construction — so
most of those 278
`NamespaceUris` reads are callers reaching through the server for
something they already
hold. And the subsystems with zero external demand are exactly the ones
that should be
injected rather than published.

The check that matters: the eight `IServerStartupTask` implementations
in
`Opc.Ua.Redundancy.Server` are the heaviest real consumer of the ambient
handle, and
between them they touch **eight distinct members**. That is the shape
the interface is cut
to.

## Commits

### `e4a912eb` Remove seven unused members from `IServerInternal`

The deletion test, applied before anything is designed against the
interface.

| Removed | Evidence |
|---|---|
| `CloseSession(OperationContext, NodeId, bool)` | zero call sites;
sync-over-async; already obsolete |
| `ReadServerDiagnostics<TResult>` | zero production sites — **added one
commit earlier in #4188 for symmetry and never called** |
| `ModellingRulesManager` | property never read; `ServerInternalData`
owns the lifetime |
| `ConformanceUnitsManager` | same |
| `DiagnosticsEnabled` | façade over
`IDiagnosticsNodeManager.DiagnosticsEnabled`, which is what callers use
|
| `Status` | one site, itself inside an obsolete method |
| `ServerDiagnostics` | only reachable through the diagnostics methods
that replaced it |

`EndpointAddresses` was kept — `Opc.Ua.Di.Tests` uses it to resolve an
endpoint URL. Thin,
but real.

The only build fallout was three lines in one test assembly.
`Opc.Ua.Redundancy.Server`,
`Opc.Ua.Redundancy.Kubernetes`, `Opc.Ua.Gds.Server.Common`,
`Opc.Ua.WotCon.Server`,
`Opc.Ua.PubSub.Server`, `Opc.Ua.Di.Server`, `ConsoleReferenceServer` and
`Quickstarts.Servers` all built untouched. That is the evidence the
members were dead.

### `ffcc80b9` Add `IServerContext`

Additive. `ServerInternalData` implements it; no caller changes.

### `e8c4e15f` Hand startup tasks `IServerContext`

Nine implementations, `OpcUaServerHostedService`, and the test call
sites.

### `1b4de97a` Drop the `IServerStartupTask` migration-guide section

Review feedback. `IServerStartupTask` does not exist in `master378`, so
it is a 2.0-only
concept and nobody migrating from 1.5.378 can own an implementation of
it. The **Removed
members on `IServerInternal`** section stays - those members *are* in
master378 - and is now
self-contained.

### `2ab7d689` Merge `marcschier/server-seam-analysis`

Picks up the base branch including its merge of master. Textually clean,
no conflicts.

### `6a650f12` Migrate `RobotIntentHostStartupTask` to `IServerContext`

The merge was textually clean but **semantically broken**: master's
Robot Intent feature
(#4165) added a tenth `IServerStartupTask` implementation written
against the old
`IServerInternal` signature, so it no longer satisfied the interface.
Its body walked
`server.NodeManager.AsyncNodeManagers` and type-tested each entry for
`RobotIntentNodeManager` - exactly the pattern `FindNodeManagers<T>()`
replaces - so it
migrated the same way as the other nine. `FindNodeManagers` also
searches the synchronous
registry and de-dupes by reference, so a manager registered on both
faces still starts its
intent controller hosts exactly once.

## Three findings from migrating real consumers

**`MessageContext` is not derivable.** The design had callers use
`DefaultSystemContext.AsMessageContext()`, on the reasoning that a
message context can be
built from a system context. It can't: that conversion carries the
namespace tables and the
factory but **not** `MaxStringLength`, `MaxArrayLength` or
`MaxByteStringLength`. Four
replication paths in `Opc.Ua.Redundancy.Server` would have silently
fallen back to default
decoding limits — a component quietly accepting more than the server is
configured to. It
is a real member, and the interface documents why.

**`new`-shadowed members split the mock surface.** `IServerInternal`
first redeclared
`DefaultSystemContext` with a narrower type and `CurrentState` with a
setter. That
compiles, but a shadowed member is a *second interface slot*: production
code calling
through `IServerContext` read a slot that every existing
`Mock<IServerInternal>` leaves
unstubbed. Seventeen tests went from passing to `NullReferenceException`
**while still
compiling**. The narrower typing bought no encapsulation anyway — the
object really is a
`ServerSystemContext` and any caller can cast — so both shadows are
gone. `CurrentState` is
read-only on the ambient interface and the one site that assigns it uses
the concrete type,
as `GetStatus` already does.

**`IServiceProvider` is deliberately *not* on `OnServerStartedAsync`.**
The plan sketched
it. All nine implementations already receive their dependencies through
their constructors
from the builder extensions, so a container on the callback would have
replaced one service
locator with another.

## `FindNodeManagers<T>`

Two tasks walked `server.NodeManager.NodeManagers` and type-tested each
entry for
`ILocalAddressSpaceSource`; two more reached for a specific manager to
cast it to a
capability. Asking for the capability says what the caller wants; asking
for the registry
says how to find it.

```csharp
// before
foreach (INodeManager nodeManager in server.NodeManager.NodeManagers)
{
    if (nodeManager is not ILocalAddressSpaceSource source) { continue; }
    ...
}

// after
foreach (ILocalAddressSpaceSource source in server.FindNodeManagers<ILocalAddressSpaceSource>())
{
    ...
}
```

It searches both the asynchronous and synchronous registries:
`ToSyncNodeManager()` returns
`this` when a manager already implements `INodeManager`, so the two
lists overlap and a
capability may sit on either face.

## Tests

Mock harnesses are **wired, not stubbed** — they return the node
managers and contexts the
tests already build, so the tests keep asserting against real behaviour
rather than against
a mock that returns whatever is convenient. Four new tests cover
`CreateSystemContext`
(identity/locale propagation, null guard), the explicit
`DefaultSystemContext` identity,
and `FindPredefinedNode` before the address space is bound.

| Suite | net10.0 | net48 |
|---|---|---|
| `Opc.Ua.Redundancy.Server.Tests` | 573 passed | 573 passed |
| `Opc.Ua.Redundancy.Kubernetes.Tests` | 97 passed | n/a (net8/net10
only) |
| `Opc.Ua.Server.Tests` | 4028 passed, 1 failed | 225 passed, 1 failed
(filtered) |

The single failure is
`ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync`,
a local certificate-store problem. Verified it reproduces unchanged at
HEAD with these
changes stashed — it is not caused by this PR.

All twelve dependent assemblies and samples build clean on both TFMs,
zero warnings.

### Re-verified after the base merge (`6a650f12`)

`Opc.Ua.Server`, `Redundancy.Server`, `Redundancy.Kubernetes`,
`Robotics.Server`,
`WotCon.Server`, `Di.Server`, `PubSub.Server`, `Gds.Server.Common` and
`XRegistry.Server` all build clean on net10.0. `Opc.Ua.Robotics.Tests`
491 passed,
`Opc.Ua.Redundancy.Server.Tests` 573 passed,
`Opc.Ua.Redundancy.Kubernetes.Tests` 97 passed.

### CI status: 119 passed, 3 failed - none caused by this PR

| Failing job | Why it is not this PR |
|---|---|
| `SourceGeneration_Core_Tests_windows` (net48) |
`GenerateAndCompileDesignFileAsync(..., CSharp11, ...)` fails with
`CS9058: Feature 'Lock object' is not available in C# 11.0` because
generated `Opc.Ua.NodeStates.g.cs` emits `System.Threading.Lock`.
**Identical failure on base #4188 (build 16737)**, which contains none
of these commits. This PR touches no source-generation file. |
| `SourceGeneration_Core_Tests_windows` (net10.0) | Same root cause;
also failing on base #4188. |
| `PubSub_Eth_Tests_windows` (net48) | Agent DNS outage:
`NuGetToolInstaller` died with `ERR:getaddrinfo ENOTFOUND
dist.nuget.org`. The job aborted before restore/build, so **the test
step never executed**. Infra flake; needs a re-run, not a code change. |

Both `SourceGeneration.Core` failures belong to the base branch and
should be fixed there
rather than papered over here.

## Correction to the recorded plan

Chunk 7 (the frozen bind phase) was recorded as the largest mechanical
footprint remaining
and the one most likely to stall. It isn't: **each of the 12 `Set*`
mutators has exactly one
call site**, in the `StandardServer` wiring, plus one sample. It is a
~13 call-site change.
The 268-file figure that produced that assessment counted files
*referencing*
`IServerInternal`, which chunk 7 never has to touch, because the
interface survives and only
the mutators move off it.

## Docs

`docs/MigrationGuide.md` gains one section: the removed
`IServerInternal` members with their
replacements, plus the note that `MessageContext`,
`DefaultSystemContext`, `CurrentState`,
`ServerObject`, `ReportEventAsync`, `CloseSessionAsync`,
`DeleteSubscriptionAsync` and
`UpdateServerDiagnostics` moved *down* to `IServerContext` and stay
reachable through
`IServerInternal` unchanged, so no call site has to move.

An earlier revision also documented the `IServerStartupTask` signature
change there. That
was dropped after review (1b4de97a4): `IServerStartupTask` does not
exist in `master378`, so
it is a 2.0-only concept and nobody migrating from 1.5.378 can own an
implementation of it.
The rationale for the ambient view - including the `AsMessageContext()`
decoding-limit trap -
lives on the `IServerContext` and `IServerStartupTask` XML docs instead,
which is where a
2.0-only concept belongs.

## Reviewer questions

- `IServerContext` is 11 members, not the 9 the design targeted.
`MessageContext` and
`FindNodeManagers<T>` were added because migrating real consumers showed
the alternatives
were wrong (silent decoding-limit loss) or a pass-through
(`NodeManager.NodeManagers` plus
  a type test at every call site). Reasonable?
- `IServerInternal : IServerContext` keeps the fat interface alive on
purpose — it is what
makes this a signature change rather than a 268-file rewrite. It only
shrinks once the DI
migration lands. The 6a deletions are unconditional, so even a full
stall leaves it seven
  members smaller.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e9c13dd0-a6e3-4ad2-8ad6-1c3fe6323041
Copilot-Session: b3c0e146-5b9e-4d08-a0f6-db3f940484b1
Copilot-Session: d18f87e9-2484-44ce-98ce-463ba6cf5f63
This commit is contained in:
Marc Schier
2026-08-10 21:14:09 +02:00
committed by GitHub
parent 4cbcec6e23
commit 4e0b82af5f
32 changed files with 415 additions and 244 deletions
+21
View File
@@ -196,6 +196,27 @@ for the before/after and
[Custom node types and assignment control](NodeManagers.md#custom-node-types-and-assignment-control)
for the runtime rules.
## Removed members on IServerInternal
The following members had no consumer anywhere in the stack, its samples or
its tests, and have been removed. Each has a direct replacement:
| Removed | Use instead |
| --- | --- |
| `CloseSession(OperationContext, NodeId, bool)` | `CloseSessionAsync(…)` |
| `Status` | `CurrentState` to read, `UpdateServerStatus` to write |
| `ServerDiagnostics` | `UpdateServerDiagnostics(Action<…>)` |
| `DiagnosticsEnabled` | `IDiagnosticsNodeManager.DiagnosticsEnabled` |
| `ModellingRulesManager`, `ConformanceUnitsManager` | the concrete `ServerInternalData`, which owns them |
`MessageContext`, `DefaultSystemContext`, `CurrentState`, `ServerObject`,
`ReportEventAsync`, `CloseSessionAsync`, `DeleteSubscriptionAsync` and
`UpdateServerDiagnostics` all moved *down* to `IServerContext`, the ambient
view of a running server that `IServerInternal` now derives from. They remain
reachable through `IServerInternal` unchanged, so no call site has to move.
`CurrentState` is read-only on the ambient interface; the server itself still
sets it.
## Migrating servers that relied on unserved history advertisement
Server startup now reconciles variables that advertise history with the