Expose a non-owning wrapper factory for an existing cOAPI (#1)

[tadoEng]

Closes #1.

Exact reviewed head: 2fdc5d70d37cddc1905f1825327719482568efcd, based on baa07045b145cdbcc7fb25314211e7455718428e.

What this adds

public static ETABSApplication WrapExisting(
    ETABSv1.cOAPI api,
    int majorVersion,
    double apiVersion,
    string fullVersion,
    ILogger<ETABSApplication>? logger = null)

WrapExisting is deliberately a pure wrapper over an already-created, already-started caller-supplied cOAPI. The only member it reads is SapModel. It does not create/start/attach through PID or ROT, enumerate/select ETABS processes, hide/unhide, or exit ETABS. ETABSApplication's constructor remains internal.

It throws on invalid caller input rather than returning null. apiVersion == 0 is accepted because EtabSharp's existing version discovery uses 0 when the OAPI version cannot be read.

Lifecycle / Dispose contract

ETABSApplication.Dispose() is non-exiting reference cleanup; it never calls ApplicationExit.

• Attached/external sessions: Dispose() alone is correct wrapper cleanup and the user's ETABS remains running.
• Caller-owned sessions being shut down: the caller performs and resolves authoritative ApplicationExit(false) / process exit first, then disposes the wrapper.

The stale XML documentation claiming Dispose exits ETABS was corrected.

A pre-existing COM-release defect found during review was also fixed: managed EtabSharp wrappers (ETABSModel, ETABSApplicationManager) are no longer passed to Marshal.ReleaseComObject. Only the actual raw _sapModel and _api references are attempted, each independently so failure releasing one cannot suppress the other.

Offline tests

test/EtabSharp.Test/wrap_existing_test.cs contains 14 [Trait("Category", "Offline")] tests using DispatchProxy recorders. They cover:

• exact caller-supplied model identity and metadata;
• no lifecycle/discovery side effects from WrapExisting;
• validation before touching the supplied object;
• missing SapModel fails rather than starting ETABS;
• distinct wraps remain distinct;
• Dispose never issues ApplicationExit;
• materializing both app.Model and app.Application before Dispose still results in two independent raw COM-release attempts;
• double Dispose is a no-op after the first cleanup.

Mutation check: restoring the former single-try Dispose body makes the materialized-wrapper cleanup test fail (Expected: 2, Actual: 0), proving the test detects the defect.

Evidence at exact head

• net8.0 library build: 0 errors, 293 warnings;
• net10.0 library build: 0 errors, 293 warnings;
• warning count matches base baa07045 exactly; no suppressions added;
• offline tests: 14/14 on net8.0 and 14/14 on net10.0;
• dotnet pack succeeds with lib/net8.0 and lib/net10.0;
• ETABSv1.dll is not bundled in the package;
• zero ETABS processes throughout the offline proof;
• Mode A/B live suites were intentionally not run;
• no version bump and no package publication performed.

The test project now resolves ETABSv1 consistently with the library (lib\\ override, then installed ETABS 24 → 23 → 22) and targets both net8.0 and net10.0.

No live ETABS operation is part of this PR.

[tadoEng]

Exact-head review at 95e2d026dfd158ac9886a8b461947dfb95ffc338.

The WrapExisting factory itself is accepted: pure wrap, exact cOAPI path, constructor remains internal, throwing semantics are appropriate, apiVersion == 0 is compatible with existing EtabSharp behavior, and the test-project ETABS 24→23→22 resolution + net8/net10 multi-targeting are acceptable. The DispatchProxy suite gives good negative evidence that the factory itself does not start/attach/exit or touch lifecycle members.

There is one blocking cleanup finding before merge, with two parts caused by the same Dispose contract:

1. The new XML wording overstates the ordering rule. Existing Mode A tests explicitly use app.Dispose() to release an attached wrapper without calling ApplicationExit, leaving the user's ETABS running. Therefore Dispose() cannot universally say it “must not be used before” ApplicationExit, nor that every caller must perform an exit. Correct distinction:

   • Dispose is always non-exiting COM/reference cleanup.
   • For a non-owning/external attach, Dispose alone is the correct final wrapper cleanup and ETABS remains running.
   • For a caller-owned session that the caller intends to terminate (the CLI Defect C case), ApplicationExit(false) / authoritative lifecycle resolution happens first, then Dispose releases wrapper-held references.

2. More importantly, current Dispose() does not reliably fulfill the newly documented “releases COM references” contract after the high-level wrappers have been materialized. _model.Value is an EtabSharp-managed ETABSModel, and _application.Value is the managed ETABSApplicationManager; neither is a COM RCW. Yet Dispose() calls Marshal.ReleaseComObject on those managed wrappers first inside one outer try. Marshal.ReleaseComObject is defined for COM RCWs and rejects non-COM objects, so the first such failure can jump to the catch and skip release of the actual raw _sapModel and _api. This matters directly to Defect C because the raw CSI lifecycle requires dropping the model/API references after the authoritative exit.

Required narrow correction in this PR:

• never call Marshal.ReleaseComObject on ETABSModel or ETABSApplicationManager;
• release only actual raw COM references (_sapModel, _api), with cleanup structured so one release failure cannot suppress the other;
• preserve the invariant that Dispose never calls ApplicationExit;
• rewrite the XML docs with the owning-vs-attached distinction above;
• add/adjust offline tests that materialize both app.Model and app.Application, then dispose, proving no lifecycle call is issued and double-dispose remains safe. The real COM-release mechanics remain a live/platform concern; do not start ETABS for this fix.

No need to revert the test-project multi-target/reference change. No version bump, package publish, merge, or CLI Defect C yet. Post the refreshed exact head + net8/net10 offline/build/pack evidence and stop again.

[tadoEng]

Additional note: I independently checked the .NET contract for Marshal.ReleaseComObject; it decrements an RCW and is only valid for a COM object. The blocking point above is therefore not stylistic — ETABSModel and ETABSApplicationManager are ordinary managed wrapper classes, so they must not be passed to ReleaseComObject.

[tadoEng]

Blocker closed — refreshed exact head 2fdc5d70d37cddc1905f1825327719482568efcd

Follow-up commit 2fdc5d7 on top of 95e2d02. Scope is the Dispose() blocker only; the factory itself is unchanged from the reviewed head.

The fix

_model.Value and _application.Value are no longer released. They are ordinary managed wrappers — ETABSModel holds the cSapModel, ETABSApplicationManager holds the cOAPI — and Marshal.ReleaseComObject rejects a non-COM object, so with one shared try the very first release threw and skipped _sapModel and _api, the only two that are real COM references.

Now only those two are released, each through a small helper with its own try, so a failure on one cannot prevent the other. Dispose() remains completely non-exiting, and the platform guard moved to OperatingSystem.IsWindows() with the helper annotated [SupportedOSPlatform("windows")] so the guard is recognised across the call.

Wording corrected

You were right that "must not be used before ApplicationExit" contradicted Mode A. The doc is now per-mode:

• attached / external session (Connect, ConnectToProcess) — disposing on its own is the correct and complete cleanup; the user's ETABS stays running, which is the point;
• caller-owned session being shut down (CreateNew, or a handle from WrapExisting) — request the authoritative ApplicationExit(false) and resolve the process exit first, then dispose.

WrapExisting's own ownership paragraph carried the same over-strong claim and is corrected the same way.

Test extended — and mutation-checked

WrapExisting_DisposeWithMaterializedWrappersStillAttemptsBothComReleases materializes both app.Model and app.Application, disposes, and asserts two independent release attempts (one naming SapModel, one naming cOAPI) through a recording ILogger, plus no lifecycle call on either recorder. WrapExisting_DoubleDisposeIsSafeAndDoesNothingTheSecondTime covers the second dispose.

The assertion was mutation-checked rather than trusted: restoring the old single-try body makes exactly that test fail (Expected: 2, Actual: 0) while the other 13 keep passing. Restored source is byte-identical to the fixed version.

Evidence at this head

• Library builds clean on net8.0 and net10.0: 0 errors, 293 warnings each — identical to the baa07045 baseline, zero added, no suppressions. The two warnings in the touched files (ETABSWrapper.cs:358, :415) are pre-existing, in ConnectToETABS/GetVersionFromProcess.
• Offline suite: 14/14 on net8.0 and 14/14 on net10.0 (12 + 2 new).
• dotnet pack succeeds: EtabSharp.0.3.5-beta.nupkg, lib/net8.0 + lib/net10.0, ETABSv1.dll not bundled.
• Zero ETABS processes before and after every run; Mode A/B live suites not run.
• No version bump, no publish.

One transparency note: an intermediate run reported 1 net10.0 failure while the mutation build was still staged in that framework's output. A clean rebuild at the restored source is green on both frameworks, and the numbers above are from that clean run.

Ready for re-review at 2fdc5d70d37cddc1905f1825327719482568efcd.

[tadoEng]

Lead exact-head acceptance — 2fdc5d70d37cddc1905f1825327719482568efcd

Re-review complete against base baa07045b145cdbcc7fb25314211e7455718428e. No remaining code blocker.

Accepted at this exact head:

• WrapExisting(cOAPI, …) remains a pure wrapper over the caller-supplied handle; no create/start/attach/ROT/process-discovery/hide/exit behavior was introduced;
• the previous Dispose blocker is closed: managed EtabSharp wrappers are no longer passed to Marshal.ReleaseComObject; only _sapModel and _api are attempted, each independently;
• Dispose() remains non-exiting and idempotent;
• documentation now correctly distinguishes attached/external sessions (Dispose alone, ETABS remains alive) from caller-owned sessions being shut down (authoritative exit/process resolution first, then Dispose);
• offline coverage now materializes both app.Model and app.Application before disposal and detects the former single-try defect under mutation;
• test-project ETABSv1 resolution and net8/net10 multi-targeting remain accepted.

Reported exact-head evidence accepted: 14/14 Offline on net8.0 and 14/14 on net10.0; 293 warnings on each TFM matching base; pack succeeds with both lib folders; ETABSv1.dll not bundled; zero ETABS processes; no live Mode A/B run; no version bump/publish.

I also refreshed the PR body so its SHA, 14-test count, Dispose semantics, and proof match the reviewed head.

GitHub will not let this account submit an APPROVE review because the PR was opened under the same tadoEng identity (Review Can not approve your own pull request). Treat this comment as the authoritative lead acceptance.

Merge is authorized only at this exact head. Package publication is a separate post-merge step. Do not start CLI Defect C against an unpublished/ad-hoc package.