refactor(device): extract connect/disconnect serialization into a collaborator (part of #344)

[tylerkron]

Problem

With the SD-card, diagnostics, network, LAN-info, channel-control and frame-decode blocks now extracted, DaqifiDevice.cs at 4,193 lines is the largest file in the repo — larger than either class #344 originally named. That is item 4 of the issue's own status comment, raised there as an open scope question. This answers it by taking the smallest safe bite rather than re-scoping the issue.

What moved

The issue #379 lifecycle gate: the semaphore that serializes connect against disconnect so the device never drives its transport from two threads at once. It was the best candidate on the file by a wide margin — 4 call sites, no events, no device state, where the next largest region (automatic reconnection, ~500 lines) owns three events and two protected virtual hooks that DaqifiStreamingDevice overrides.

Device/Internal/LifecycleGate.cs now holds the semaphore, the AsyncLocal re-entry flag, the LifecycleContention policy enum and the two Run/RunAsync entry points. DaqifiDevice 4,193 → 3,947 lines.

LifecycleLockTimeout and TeardownLockTimeout deliberately stay on the device — they are the internal virtual seam DeviceReconnectTests overrides — and reach the gate as Func<TimeSpan>.

The one design point worth reading

The timeouts are delegates rather than values, and that is load-bearing rather than stylistic. The test subclass assigns its override through an init property, which runs after the base constructor that builds the gate. A gate that read them at construction would have captured the 10 s / 30 s defaults and silently ignored every override — tests would still pass, just slowly and for the wrong reason. Pinned by ContentionWait_ReadsTheTimeoutWhenContentionHappens_NotAtConstruction.

Behavior deliberately left alone

_lifecycleLock is never disposed today — ReleaseResources disposes only _textExchangeLock — so the ObjectDisposedException handlers are defensive rather than reachable. Adding disposal would be a behavior change smuggled into a pure refactor, so it is documented in the collaborator instead of "fixed" here.

SafeLog is a per-class private copy, matching the established convention (MessageProducer, DaqifiDevice, and StatusChannelPopulator from #433 each carry their own) rather than introducing a new shared helper.

Verification

Pure move, verified mechanically in both directions — normalized statement multiset diff (comments/usings stripped, mechanical renames canonicalized), the same method used by #419/#422/#432/#433:

• Collaborator vs the original region: residue is only file scaffolding, the constructor + its four fields and null guards, and the SafeLog copy.
• Device before vs after: it lost exactly the region body, and gained exactly the field, three constructor wirings and four call-site renames. Nothing unaccounted for.

Tests: zero edits to existing tests. That is the evidence that matters — DeviceReconnectTests still races a real reconnect loop against a caller's connect over a scripted transport, through the new seam. +21 new cases against the collaborator directly, which the extraction newly makes possible (contention is now constructed rather than raced into): both policies across sync and async, re-entry including across an await, the re-entry flag not leaking to a later caller, release-on-throw, the lazy-timeout seam, and the #341 cancelled-teardown contract.

• FULL suite green net9 + net10 — 2,588 passed, 2 skipped each; Daqifi.Mcp.Tests 23.
• Release solution build 0 warnings on both TFMs, which is the real gate after moving ~280 lines of doc prose across a namespace (TreatWarningsAsErrors + GenerateDocumentationFile turns a stale <see cref> into an error).

Bench (real Nq1, fw 3.7.2, USB, non-destructive)

Connect / disconnect / status only — no stream start, no NVM write, no reboot.

Check	Result
3 sync connect/disconnect cycles on one instance	PASS (720 / 509 / 511 ms) — a leaked permit makes cycle 2 throw
2 async ConnectAsync/DisconnectAsync cycles	PASS
Cancelled DisconnectAsync still tears down (#341)	PASS — and the OS serial handle was genuinely released, proved by a fresh device reopening the port, not by reading Status
Disconnect() re-entered from inside a live StatusChanged handler	PASS — completed instead of deadlocking against the real transport
Device healthy afterward	fw 3.7.2, analogIn=16, digital=16

The last two are the ones a unit test cannot establish: a mocked transport cannot show an OS-level handle release, and a simulated handler cannot show that re-entry survives a real blocking SerialPort teardown.

Part of #344.

Not merging — opened for your review.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Extract lifecycle connect/disconnect gate into internal collaborator

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Extract connect-vs-disconnect serialization into internal LifecycleGate collaborator.
• Wire DaqifiDevice constructors and connect/disconnect paths to use the gate.
• Add focused unit tests for contention, re-entry, cancellation, and timeout semantics.

Diagram

graph TD
  A["Connect/Disconnect callers"] --> B["DaqifiDevice"] --> C["LifecycleGate"] --> D(("SemaphoreSlim"))
  C --> E["ILogger"]
  F["LifecycleGateTests"] --> C
  subgraph Legend
    direction LR
    _m["Module/Class"] ~~~ _l(("Lock"))
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep lifecycle gate inside DaqifiDevice

• ➕ No extra collaborator to construct and thread through
• ➕ All lifecycle concerns remain co-located
• ➖ Continues growth of an already very large file
• ➖ Harder to unit-test contention/re-entry behavior without racing real operations

2. Unify with the broader per-device operation serialization (issue #342)

• ➕ Single serialization mechanism for all operations
• ➕ Potentially fewer concurrency primitives and invariants to reason about
• ➖ Higher scope/risk: changes ordering semantics across the public API
• ➖ More entangled with text exchange and teardown behavior; less “pure refactor”

3. Adopt a standard async lock helper (e.g., AsyncLock library)

• ➕ Less bespoke semaphore/re-entry bookkeeping
• ➕ Potentially clearer semantics with well-known primitives
• ➖ Adds dependency or introduces shared infrastructure for a narrow internal invariant
• ➖ Still must handle re-entry and special cancellation semantics for teardown

Recommendation: Current approach is the best fit for the stated goal (small, safe reduction of DaqifiDevice while preserving behavior). Extracting the narrow connect/disconnect gate keeps scope contained, improves testability, and avoids changing broader operation-ordering semantics that a #342 unification would likely impact.

Files changed (4) +860 / -261

Refactor (2) +372 / -261

DaqifiDevice.csDelegate connect/disconnect serialization to LifecycleGate +29/-261

Delegate connect/disconnect serialization to LifecycleGate

• Replaces the in-class semaphore/AsyncLocal lifecycle serialization region with a 'LifecycleGate' field, constructed in all constructors. Updates Connect/Disconnect core paths to call '_lifecycleGate.Run/RunAsync' while keeping 'LifecycleLockTimeout' and 'TeardownLockTimeout' as the existing virtual seam for tests.

src/Daqifi.Core/Device/DaqifiDevice.cs

LifecycleGate.csIntroduce LifecycleGate collaborator extracted from DaqifiDevice +343/-0

Introduce LifecycleGate collaborator extracted from DaqifiDevice

• Adds an internal sealed 'LifecycleGate' and 'LifecycleContention' enum implementing the former connect-vs-disconnect critical section using 'SemaphoreSlim' plus an 'AsyncLocal' re-entry flag. Preserves special-case semantics (teardown token not governing gate wait; defensive ObjectDisposedException handling; SafeLog swallowing logger failures) and reads timeouts via delegates at contention time.

src/Daqifi.Core/Device/Internal/LifecycleGate.cs

Tests (1) +473 / -0

LifecycleGateTests.csAdd direct unit tests for LifecycleGate contention and re-entry +473/-0

Add direct unit tests for LifecycleGate contention and re-entry

• Introduces comprehensive unit coverage for uncontended execution, contention policies (Fail vs Abandon), timeout-delegate laziness, re-entry across sync and await boundaries, cancellation rules (#341 teardown contract), release-on-throw, and logger isolation. Enables deterministic contention testing by holding the gate explicitly rather than racing real device operations.

src/Daqifi.Core.Tests/Device/Internal/LifecycleGateTests.cs

Documentation (1) +15 / -0

SESSION_LOG.mdRecord lifecycle-gate extraction worklog and verification notes +15/-0

Record lifecycle-gate extraction worklog and verification notes

• Adds a dated entry describing the rationale for extracting the lifecycle serialization block, verification approach, and test/bench results. This is operational documentation of the refactor decision-making rather than code behavior changes.

SESSION_LOG.md

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Hardware identifiers committed ✓ Resolved 🐞 Bug ⛨ Security ⭐ New

Description

SESSION_LOG.md now commits a real device serial number and local /dev/... port path, which leaks a
persistent hardware identifier and developer environment details to anyone with repo access. This is
primarily a privacy/repo-hygiene information disclosure risk (not a credential leak).

Code

SESSION_LOG.md[75]

+- BENCH (real Nq1 `/dev/cu.usbmodem1101`, fw 3.7.2, USB, non-destructive — connect/stream/disconnect only; no NVM write, no reboot, no SD): 5 full process-level connect → populate → stream → stop → disconnect cycles, **all exit 0**, each reporting `analogIn=16 digital=16 fw=3.7.2 sn=9090539562006014104` (a populated device, not a bare connect). 3 s @ 10 Hz ch 0+1 gave 22 frames, then 4× 1 s @ 20 Hz gave 14 each — consistent run to run, no drift, no wedge across repeated open/close of the same port.

Relevance

●●● Strong

Likely to redact persistent identifiers; team has accepted security/privacy hardening changes in
prior reviews.

PR-#99
PR-#150

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The newly added bench entry explicitly includes both a local device node and a concrete serial
number, which are persistent identifiers committed to the repo.

SESSION_LOG.md[70-77]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`SESSION_LOG.md` includes a real device serial number and local `/dev/...` port path in a committed bench note. This leaks a persistent hardware identifier and local environment details in version control history.

## Issue Context
This is a session/bench narrative log. The technical value of the entry is preserved if the identifiers are anonymized.

## Fix Focus Areas
- SESSION_LOG.md[75-75]

## Suggested fix
Replace the concrete port path and serial number with placeholders (e.g., ``/dev/<redacted>`` and ``sn=<redacted>``). If this repository is widely shared and the identifier is considered sensitive, consider rewriting history to remove the identifier from prior commits as well.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. No null-check operation delegate ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

LifecycleGate.Run and RunAsync invoke the provided operation delegate without validating it, so an
internal misuse would throw a NullReferenceException rather than an ArgumentNullException. This is
low-impact today (current call sites pass non-null), but adding the guard makes the new
collaborator’s API contract consistent with its constructor null-checks.

Code

src/Daqifi.Core/Device/Internal/LifecycleGate.cs[R198-205]

+        internal bool Run(Action operation, LifecycleContention onContention)
+        {
+            // Re-entry from inside the critical section (a StatusChanged handler calling back in)
+            // proceeds without acquiring, exactly as a reentrant monitor would.
+            if (_isInsideLifecycleOperation.Value)
+            {
+                operation();
+                return true;

Relevance

●●● Strong

Team has accepted adding null-guards to throw ArgumentNullException instead of NRE for clearer
contracts.

PR-#99
PR-#319

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The constructor already enforces non-null dependencies, but both Run and RunAsync call operation()
directly (including in the re-entry path), so a null delegate would throw a NullReferenceException
instead of a clear argument error.

src/Daqifi.Core/Device/Internal/LifecycleGate.cs[115-125]
src/Daqifi.Core/Device/Internal/LifecycleGate.cs[198-206]
src/Daqifi.Core/Device/Internal/LifecycleGate.cs[247-256]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`LifecycleGate` constructor defensively null-checks its injected collaborators, but the public entry points (`Run`/`RunAsync`) don’t null-check the operation delegate. A null operation would currently fail as a `NullReferenceException` at the invocation site, which is less diagnosable than an `ArgumentNullException`.

### Issue Context
This is an internal collaborator extracted from `DaqifiDevice`, so it may gain additional call sites over time. Keeping argument validation consistent reduces future debugging time.

### Fix Focus Areas
- src/Daqifi.Core/Device/Internal/LifecycleGate.cs[198-206]
- src/Daqifi.Core/Device/Internal/LifecycleGate.cs[246-256]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

Previous review results

Review updated until commit a0f60eb

Results up to commit 2c3d3aa ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. No null-check operation delegate ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

LifecycleGate.Run and RunAsync invoke the provided operation delegate without validating it, so an
internal misuse would throw a NullReferenceException rather than an ArgumentNullException. This is
low-impact today (current call sites pass non-null), but adding the guard makes the new
collaborator’s API contract consistent with its constructor null-checks.

Code

src/Daqifi.Core/Device/Internal/LifecycleGate.cs[R198-205]

+        internal bool Run(Action operation, LifecycleContention onContention)
+        {
+            // Re-entry from inside the critical section (a StatusChanged handler calling back in)
+            // proceeds without acquiring, exactly as a reentrant monitor would.
+            if (_isInsideLifecycleOperation.Value)
+            {
+                operation();
+                return true;

Relevance

●●● Strong

Team has accepted adding null-guards to throw ArgumentNullException instead of NRE for clearer
contracts.

PR-#99
PR-#319

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The constructor already enforces non-null dependencies, but both Run and RunAsync call operation()
directly (including in the re-entry path), so a null delegate would throw a NullReferenceException
instead of a clear argument error.

src/Daqifi.Core/Device/Internal/LifecycleGate.cs[115-125]
src/Daqifi.Core/Device/Internal/LifecycleGate.cs[198-206]
src/Daqifi.Core/Device/Internal/LifecycleGate.cs[247-256]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`LifecycleGate` constructor defensively null-checks its injected collaborators, but the public entry points (`Run`/`RunAsync`) don’t null-check the operation delegate. A null operation would currently fail as a `NullReferenceException` at the invocation site, which is less diagnosable than an `ArgumentNullException`.

### Issue Context
This is an internal collaborator extracted from `DaqifiDevice`, so it may gain additional call sites over time. Keeping argument validation consistent reduces future debugging time.

### Fix Focus Areas
- src/Daqifi.Core/Device/Internal/LifecycleGate.cs[198-206]
- src/Daqifi.Core/Device/Internal/LifecycleGate.cs[246-256]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[tylerkron]

/agentic_review

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 22d060b

[tylerkron]

Ready for review: Qodo review came back clean on the latest commit (22d060b) — 0 bugs, 0 rule violations, 0 skill insights, no unresolved threads — and CI build is green on net9 + net10. Not merging — awaiting your review.

[tylerkron]

Bench re-validation (real Nq1, fw 3.7.2, USB, non-destructive) — this time through the shipped example CLI built against this branch (-p:DaqifiCoreProjectPath=…, 0 warnings), rather than the API-level scratch harness the earlier run used. Same gate, ordinary consumer path.

• 5 full process-level connect → populate → stream → stop → disconnect cycles, all exit 0, each reporting a populated device (analogIn=16 digital=16 fw=3.7.2 plus a stable serial number). 3s @ 10 Hz on ch 0+1 → 22 frames; 4× 1s @ 20 Hz → 14 frames each. Consistent run to run, no wedge across repeated open/close of the same port.
• Failure path the gate must release on: a connect to a nonexistent port fails cleanly with the typed SerialPortConnectException ("was not found") and exit 1, and the very next real connect on the same port streams normally. A leaked permit on the throwing path would have stranded that follow-up run.

Pushed eda3157 + a0f60eb, both docs-only (SESSION_LOG entry for this fire, then the serial-number redaction below) — no source or test change since the Qodo-clean 22d060b.

(Edited to drop the device serial number, per the Qodo finding on the same content.)

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit eda3157

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit a0f60eb

[tylerkron]

Ready for review (supersedes the earlier note, which pinned 22d060b). Qodo is clean on the current head a0f60eb — Bugs (0) / Rule violations (0) / Skill insights (0), 0 unresolved threads — and CI build is green on net9 + net10.

Both commits since 22d060b are docs-only; the source and tests are unchanged from the Qodo-clean state.

One note on CI history for this branch: the two intermediate red runs were net10-only timing flakes on docs-only commits — ff2e30b failed StreamMessageConsumerStallingReaderTests.Start_WhenStoppedReaderExitsWithinGrace_WaitsAndRestartsSameInstance, eda3157 failed DaqifiDeviceOperationSerializationTests.TextExchange_CancelledWhileTheOutboundQueueDrains_DoesNotResubscribeTheConsumer. Two different timing tests on the same TFM, on commits that touched one markdown file, is runner slowness rather than a regression. Full suite passes locally on both TFMs (2,592 passed / 2 skipped each, plus Daqifi.Mcp.Tests 23), and the head commit is green in CI.

Not merging — awaiting your review.