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

SESSION_LOG.md

@@ -40,3 +40,41 @@
 - BENCH (real Nq1, fw 3.7.2, USB, non-destructive) via a scratch harness, because the example CLI surfaces none of these counters. **Session 1 (ch 0,1,2 @200 Hz, 3 s):** `rawFrames=475` and `decodedCh0=475` — every delivered frame reached BOTH consumer paths exactly once (the re-raise multiplicity contract, on hardware); `discarded=1` with `PartialAnalogFrame[an=1/en=3]` — the firmware's malformed leading frame caught by the moved guard and withheld from raw consumers; all 3 channels decoded 475 samples each in ascending order; `decodeFailures=0`, `gaps=0`; every event's `sender` asserted to be the device. **Session 2 (ch 0 only @100 Hz, same instance):** `discarded=0`/`failures=0` (BeginSession reset both, no leftover tripped the gate); `rawFrames=238` vs `decodedCh0=237` — one post-stop frame re-raised but NOT decoded, exactly the `if (!IsStreaming)` branch on hardware; `ch1=ch2=0` decoded, so the disable reached the device and the snapshot the decode maps against is still right. Only channel enable/disable + stream start/stop; no NVM write, no reboot, no SD.
 - Bench-rig note: the example CLI's `--channels` takes a **bitmask** (`7` = ch 0,1,2), not a comma list, and `--format` accepts only `text|csv|jsonl` (no `json`).
 - Result: PR #435 opened (base main, part of #344, "not merging — for review"), /agentic_review requested. Now **3 loop PRs awaiting review (#433, #434, #435) — at the concurrency cap**, so the next fire should shepherd only, not start a new ticket.
+
+## 2026-08-05 — Fire: PR #436 ready-note + implemented #344 (lifecycle-gate extraction) → PR #437
+- State at start (re-derived from `gh`, NOT this log — the log's closing line said "at the cap with #433/#434/#435", which was stale on arrival): #433 and #435 have MERGED, and a prior fire opened **#436** without writing an entry. Live open loop PRs were **#434 and #436 → 2, under the 3-PR cap**, so priority 4 was in scope.
+- Priority 1: zero unresolved `qodo-code-review` threads on both (GraphQL `reviewThreads`, not the summary comment). Priority 2: #436 was Qodo-clean (0 bugs / 0 rule violations / 0 requirement gaps) + CI `build` green with **no** ready-note → posted one. #434 already had its note. Priority 3: no red CI.
+- **Working-tree incident, recorded so it isn't repeated.** While surveying file sizes I ran `git checkout origin/main -- .` in the *shared* checkout at `/Users/tylerkron/projects/daqifi/daqifi-core` (which sits on `main` @ `7bc6161`, two commits behind origin). That staged the #433+#435 delta over the user's checkout. Verified the damage was recoverable before touching anything: `git diff --cached` was byte-identical to `git diff HEAD origin/main`, i.e. 100% upstream-committed content with no novel local work in it, then `git reset --hard 7bc6161` restored it exactly as found. **All loop work belongs in the fire's own worktree; never run a checkout/reset in the shared clone.**
+- Backlog is still 5 open issues, 4 of them skips under the loop rules (#333 breaking API; #269/#271 destructive WINC flash; #183 unshipped firmware dep). #344 was again the only eligible one — and its two named god-classes are now largely done (`FirmwareUpdateService` 2,656 → 359 via #419; the SD/diagnostics/network/LAN blocks, channel control, frame decode all extracted). `DaqifiDevice.cs` at **4,193 lines is now the largest file in the repo**, which is item 4 of the issue's own status comment.
+- CANDIDATE REJECTED — anything in `DaqifiStreamingDevice.cs` (still 1,385 lines, over the ~800 target): **PR #436 owns that file and `IDeviceOperationHost.cs` on an unmerged branch**, so it would conflict textually. `WifiModuleUpdater` (item 3) stays deferred until #271 settles the external-tool shape.
+- PICKED the **lifecycle-serialization block** out of `DaqifiDevice` (issue #379's connect-vs-disconnect gate) → `Device/Internal/LifecycleGate.cs`. Chosen over the larger reconnection region (~500 lines) deliberately: the gate has **4 call sites, no events, and no device state at all**, where reconnection owns three events and two `protected virtual` hooks that `DaqifiStreamingDevice` overrides. Smallest safe bite, per the #433 precedent. `DaqifiDevice` 4,193 → 3,947.
+- The seam is `(ILogger, Func<string> deviceName, Func<TimeSpan> connect, Func<TimeSpan> teardown)`. **The timeouts are delegates and that is load-bearing, not stylistic**: `LifecycleLockTimeout`/`TeardownLockTimeout` stay `internal virtual` on the device precisely so tests can shorten them, and `DeviceReconnectTests`' subclass assigns its override through an `init` property that runs *after* the base constructor builds the gate. Capturing values there would have silently pinned the 10 s/30 s defaults and ignored every override. Pinned by its own test.
+- Verified as a move mechanically, same method as #419/#422/#432/#433: normalized statement multiset diff, run in **both directions**. Collaborator vs the original region leaves only file scaffolding + the ctor/fields/null-guards + the `SafeLog` copy; device before-vs-after shows it lost exactly the region body and gained exactly the field, 3 constructor wirings and 4 call-site renames. Nothing unaccounted.
+- `SafeLog` is a per-class private copy, which is the established convention here (`MessageProducer`, `DaqifiDevice`, and `StatusChannelPopulator` from #433 each carry their own) — not a new shared helper.
+- Left behavior alone deliberately: `_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; documented in the collaborator instead.
+- Tests: **zero edits to existing tests** — the real evidence, since `DeviceReconnectTests` still races a live reconnect loop against a caller's connect 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 × sync/async, re-entry incl. across an `await`, the re-entry flag not leaking, 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** both TFMs — the gate that catches a bad `<see cref>` after moving ~280 lines of doc prose across a namespace.
+- BENCH (real Nq1, fw 3.7.2, USB, non-destructive — connect/disconnect/status only, no stream start, no NVM write, no reboot): 3 sync connect/disconnect cycles on ONE instance (a leaked permit would make cycle 2 throw), 2 async cycles, and the two things a unit test genuinely cannot show — a cancelled `DisconnectAsync` **actually released the OS serial handle** (proved by a fresh device reopening the port, not by reading `Status`), and `Disconnect()` re-entered from inside a live `StatusChanged` handler completed instead of deadlocking against the real transport. Device healthy afterward: fw 3.7.2, analogIn=16 digital=16.
+- Result: PR #437 opened (base main, part of #344, "not merging — for review"), `/agentic_review` requested. Touches only `DaqifiDevice.cs` + two new files — deliberately zero file overlap with #434 and #436. Now 3 loop PRs awaiting review (#434, #436, #437) — **at the cap**, so the next fire shepherds only until the user merges one.
+
+## 2026-08-05 — Fire: shepherded PR #437's Qodo review (1 finding, valid)
+- State at start (re-derived from `gh`): 3 open loop PRs — **#434, #436, #437 — at the concurrency cap**, so priority 4 (new ticket) was out of scope by rule and this fire was shepherd-only. Backlog unchanged at 5 open issues (#344, #333, #271, #269, #183), 4 of them standing skips.
+- Priority 1 sweep: #434 and #436 both Qodo-clean with 0 unresolved threads and ready-notes already posted — nothing to do. **#437 had been open 90 seconds**: CI `build` had just gone green but Qodo had only posted its PR Summary, so it was not yet actionable. Polled rather than declaring the fire idle; the review landed ~2.5 min later with **1 finding**. Priority 3: no red CI anywhere.
+- FINDING (Optional/Informational, "No null-check operation delegate") — **VALID, fixed in `ff2e30b`**. `LifecycleGate`'s constructor null-guards all four injected delegates, but `Run`/`RunAsync` invoked the caller's `operation` unvalidated, so internal misuse would surface as `NullReferenceException` from one of three invocation sites instead of a named `ArgumentNullException`. Not a judgement call: the sibling collaborator `ChannelControlOperations` (extracted in #432, merged) already guards **every** entry point with `ArgumentNullException.ThrowIfNull` — the gate was inconsistent with a settled in-repo convention, not deliberately lenient.
+- **Placement was the part the finding did not specify and the part that matters.** The guard goes *before* the re-entry branch, not merely before the invocation: `if (_isInsideLifecycleOperation.Value) { operation(); }` invokes without acquiring anything, so a guard placed after it would leave exactly that path throwing NRE — the case the finding is about. First-statement placement also means a null delegate can never take the gate on its way to failing (a later guard would release via `finally`, but only after briefly contending every other caller). Two of the four new tests exist specifically to pin that ordering.
+- `RunAsync` is `async`, so its `ArgumentNullException` surfaces on the returned task rather than at the call. Framework convention for async argument validation, and what all four device call sites observe anyway; noted in the XML docs rather than worked around.
+- Tests: +4 (null on the normal path and the re-entrant path, sync and async, each also asserting the gate is still free afterward). **Mutation-verified** — deleting the two guards fails exactly those 4 and nothing else (25 → 21 passing in `LifecycleGateTests`). FULL suite green net9 + net10 (**2,592 passed, 2 skipped each**; was 2,588) + `Daqifi.Mcp.Tests` 23. Release solution build **0 warnings** both TFMs.
+- No bench re-run, and the reason is specific rather than a shrug: all four device call sites pass non-null lambdas, so the guard is unreachable from the device and cannot alter the connect/disconnect behavior already validated on the real Nq1 for this branch. A bench run could not have observed the change.
+- Process note: the log entry was pushed **before** re-requesting review, so `/agentic_review` pins the final SHA rather than a stale one. Prior fires appended the log inside the PR's own commit; a shepherd fire has no PR of its own, so it appends to the branch it shepherded.
+- Result: `ff2e30b` pushed to `refactor/344-lifecycle-gate`, thread replied to and resolved, `/agentic_review` re-run. Not merging. Still **3 loop PRs awaiting review (#434, #436, #437) — at the cap**; the next fire shepherds only until the user merges one.
+
+## 2026-08-05 — Fire: ready-note on #437 + CLI-level bench re-validation of the lifecycle gate
+- State at start (re-derived from `gh`, not this log): 3 open loop PRs — **#434, #436, #437 — at the concurrency cap**, so priority 4 was out of scope by rule and this fire was shepherd-only. Backlog unchanged.
+- Priority 1: GraphQL `reviewThreads` shows **0 unresolved threads on all three**. #437's single Qodo finding (the `LifecycleGate` null-guard) is resolved, and Qodo re-reviewed the fix commit automatically at 22:56 — `Bugs (0) / Rule violations (0) / Skill insights (0)` against `22d060b`, the current head. Priority 3: CI `build` green on every head SHA.
+- Priority 2: **#437 was Qodo-clean + CI green with no ready-note** (the previous fire pushed `ff2e30b` and re-requested review, then ended before the result landed) → posted one. #434 and #436 already had theirs.
+- At the cap with nothing else actionable, spent the fire on the saturation clause: **one light bench pass on a surface the previous fire's bench did not cover.** That fire drove `LifecycleGate` through a scratch API-level harness; this one drives the same branch through the **shipped example CLI**, i.e. the ordinary consumer path a user actually takes, with the CLI built against this worktree's core (`-p:DaqifiCoreProjectPath=…`, 0 warnings).
+- BENCH (real Nq1 on the bench USB CDC port, fw 3.7.2, 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` plus a stable serial number (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.
+- Also exercised the **failure path the gate has to release on**: a connect to a nonexistent port fails cleanly with the typed `SerialPortConnectException` ("was not found", the #427 naming) and exit 1, and the very next real connect on the same port streams normally. A gate that leaked its permit on the throwing path would strand the following run.
+- Bench-rig note: the example CLI's `--duration 0` means **run until Ctrl+C**, not "don't stream" — it is not a way to ask for a connect-only run, and it will hang an unattended fire. Always pass a positive `--duration`. Also, macOS has neither `timeout` nor `gtimeout` on this box; bound runs with the tool timeout and redirect to a file (piping to `tail` buffers all output until the process exits, which hides a hang).
+- Pushing the log entry drew a Qodo finding of its own ("Hardware identifiers committed", `SESSION_LOG.md`): the bench note pinned the unit's **serial number** into a public repo. **Valid and taken** — the serial carried no analytical weight (the point was "the device came back populated", not *which* device), so it is now described rather than quoted. Kept the `/dev/cu.usbmodem*` shape, which `README.md` already documents as the ordinary macOS port example and is not an identifier. The same redaction was applied to the PR comment. **Convention for future fires: bench notes state what the device reported, never its serial.**
+- CI flake, NOT a regression: the docs-only push failed `DaqifiDeviceOperationSerializationTests.TextExchange_CancelledWhileTheOutboundQueueDrains_DoesNotResubscribeTheConsumer` on **net10 only** — a commit that touched one markdown file cannot regress it. The prior push (`ff2e30b`) failed a *different* net10 timing test (`StreamMessageConsumerStallingReaderTests.Start_WhenStoppedReaderExitsWithinGrace_WaitsAndRestartsSameInstance`). Two different timing tests failing on the same TFM across two runs is runner slowness, not one broken test; both pass locally on net9 + net10. Worth watching: **net10 CI appears to be the flaky lane on this repo**, and if it keeps costing reruns it deserves its own issue rather than a rerun each time.
+- Result: no source change this fire. #437 ready-note + bench comment posted; log appended to the branch it shepherded, per the convention set by the previous shepherd fire. Still **3 loop PRs awaiting review (#434, #436, #437) — at the cap**; the next fire shepherds only until the user merges one.