diff --git a/openspec/changes/a-call-that-ended-is-released-when-it-ends/.openspec.yaml b/openspec/changes/a-call-that-ended-is-released-when-it-ends/.openspec.yaml new file mode 100644 index 00000000..75289e4b --- /dev/null +++ b/openspec/changes/a-call-that-ended-is-released-when-it-ends/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-24 diff --git a/openspec/changes/a-call-that-ended-is-released-when-it-ends/design.md b/openspec/changes/a-call-that-ended-is-released-when-it-ends/design.md new file mode 100644 index 00000000..9b5c0eac --- /dev/null +++ b/openspec/changes/a-call-that-ended-is-released-when-it-ends/design.md @@ -0,0 +1,146 @@ +# Design: a-call-that-ended-is-released-when-it-ends + +## Context + +See proposal.md — *Why* for the measured defects. The design-relevant shape: + +- `CallSessionManager` holds five structures keyed per call or per channel. Only `_sessions` and + `_byLinkedId` are ever released, and only by `EvictStaleCompleted`. +- `EvictStaleCompleted` is called from exactly one place, `OnSessionCompleted`, which is itself + reached only from `OnChannelRemoved`. Release therefore rides the completion of *another* call. +- `OnSessionCompleted` runs inside the lock taken in `OnChannelRemoved`, and emits `CallEndedEvent` + synchronously through a `Subject` **before** eviction runs. Every subscriber in this ecosystem + subscribes directly, with no scheduler hop — so a consumer's handler runs while that lock is held, + and a `GetById` from inside it must still find the session. Ordering is not a free choice here. +- `InMemorySessionStore` stores the same `CallSession` reference the manager holds. +- `SessionOptions.MaxCompletedSessions` is public, documented, defaulted to 1000 and read by nothing. + +## Goals / Non-Goals + +**Goals** + +- Make release unable to stop permanently. +- Bound by time and by count, with time as the floor. +- Release without waiting for another call to end, and without adding a timer. +- Make the default store follow the manager, and make the resident counts visible. + +**Non-Goals** + +- Releasing, ageing or ending a call that is not terminal. Excluded by requirement, not by habit. +- Registering `SessionReconciliationService` anywhere, or changing what it does. +- The Postgres store's retention, and the consumer-side accumulation in the closed-source cluster + layer and the product. Those are the consumers' own retention decisions. +- `_byChannelId` and `_bridgeToSession` entries stranded by an unobserved hangup. Their cause is the + reload defect, which another change fixes; bounding them here would paper over it. + +## Decisions + +### D1 — Release discards an entry it cannot evaluate, instead of stopping + +The release loop dequeues first and decides afterwards. An entry naming a call no longer retained, +or carrying no completion time, is dropped and the loop continues. + +*Why:* this is the wedge. The current loop makes dequeuing conditional on the entry being usable, so +one unusable entry at the head disables release forever. Inverting that — take it off the queue, then +decide what to do with it — makes progress unconditional. The failure mode it removes is silent and +permanent, which is why the spec states it rather than leaving it to review. + +*Alternative rejected:* keeping the peek-first shape and adding guards for the two known bad cases. +It fixes the two we found and leaves the structure that produced them. + +### D2 — A terminal session is not queued twice + +`OnSessionCompleted` enqueues on its first line regardless of what the session already was. The +enqueue becomes conditional on the session not already being queued. + +*Why:* the duplicate is what creates D1's unusable head in the first place. Fixing only D1 leaves +the queue growing a redundant entry per re-completion; fixing only D2 leaves the wedge reachable by +any other route. Both, or neither. + +### D3 — Time is a floor the count cannot undercut + +The count bound releases the oldest entries beyond the maximum, but only among those already past +the retention period. A call that ended a second ago is never released because the count is high. + +*Why:* the two bounds answer different questions — retention answers "how long is this useful", the +maximum answers "how much will we hold". Letting the count override retention would make the SDK's +answer to the first question depend on traffic, and a consumer reading a just-ended call by id would +get `null` under load and a session when idle. That is the silent-null failure this design exists to +avoid. + +### D4 — Release is evaluated on arrival too, and no timer is added + +The same evaluation runs when a call is admitted as well as when one completes. Nothing schedules it. + +*Why:* release that only rides completions stops when completions stop, which is precisely the +degenerate case — a process still accepting calls but no longer completing them. Arrivals are the +other event the manager already handles, so the trigger costs no new machinery. A timer would be the +obvious alternative and is rejected: it means a hosted service or a background loop, which is a +lifetime the manager does not own today, and the multi-server registration deliberately has no hosted +service (`ADR-0059` records that gap as known and deferred). Adding one here would reopen a decision +that belongs elsewhere. + +*Cost, stated:* an idle process releases nothing. That is acceptable — an idle process is not +growing either — and the spec says so explicitly rather than leaving it as a gap. + +### D5 — Release runs after the ending has been delivered, and the store follows + +Eviction stays after `CallEndedEvent` is emitted, and the default store is told to release the same +call the manager released, through one additive member on the store base type. + +*Why the ordering:* a subscriber's handler runs synchronously inside the manager's lock, and those +handlers call `GetById`. Releasing before the event would hand every consumer a null for the call +they were just told about. + +*Why the store:* the in-memory store holds the same object, so the manager's release frees a +dictionary node and nothing else. A store that provides durability keeps its own retention — the +Redis store already expires its keys on the same retention value, which is the shape to follow rather +than to override. + +*Alternative rejected:* having the manager reach into the store's dictionary. The store owns its +storage; an additive virtual that defaults to doing nothing keeps every existing store compiling and +lets a durable one ignore it. + +### D6 — A destroyed bridge is released + +`BridgeManager` marks `DestroyedAt` and keeps the entry; `BridgeCount` counts destroyed bridges as +though they were live. Destruction releases the entry. + +*Why it is here and not in its own change:* it is the same class of defect — something that ends and +is not released — in the layer directly beneath, and its fix is smaller than the paperwork of a +separate change. `BridgeCount`'s reported value changes, which is why it is called out rather than +folded in silently. + +## Risks / Trade-offs + +- **Releasing a call a consumer still needs** → the worst outcome, and the reason D3 makes time a + floor and the spec forbids touching non-terminal calls. D5's ordering covers the narrower version + of the same risk inside a subscriber's own handler. +- **`MaxCompletedSessions` starts taking effect** → a consumer that set it high while relying on it + being ignored sees releases it did not before. It is a published option with a documented meaning, + so honouring it is the contract; marked **BREAKING** in the CHANGELOG. +- **`BridgeCount` changes value** → it counted destroyed bridges. Any consumer dashboard reading it + will show a lower, correct number. Called out in the CHANGELOG entry. +- **An idle process releases nothing** → accepted, stated in D4 and in the spec. +- **Conflict with the reload change** → both edit `OnChannelRemoved`, `OnSessionCompleted` and + `EvictStaleCompleted`. This change lands after it; the merge queue would otherwise resolve the text + and leave the semantics to chance. + +## Migration Plan + +No migration. No public API is removed, no data shape changes, no configuration is required. A +consumer that wants the old unbounded behaviour sets `MaxCompletedSessions` higher — the option means +what it says. + +Rollback is a revert. Nothing persists that would survive it. + +`Sdk/ADR-0063` records the durable rule: what the SDK holds for a call is released when that call +ends, bounded by time and by count, and that release never depends on its own bookkeeping being +well-formed. + +## Open Questions + +None that can be deferred. The one question that could have changed the spec — whether the count may +release a call that is still within its retention period — is answered in D3, because a consumer +receiving `null` for a just-ended call under load, and a session for the same call when idle, is a +behaviour the spec has to settle rather than discover. diff --git a/openspec/changes/a-call-that-ended-is-released-when-it-ends/proposal.md b/openspec/changes/a-call-that-ended-is-released-when-it-ends/proposal.md new file mode 100644 index 00000000..0051a3f0 --- /dev/null +++ b/openspec/changes/a-call-that-ended-is-released-when-it-ends/proposal.md @@ -0,0 +1,119 @@ +--- +tier: MEDIANO +owner: Harol +approver: Harol +stakeholder: Operators running this SDK as a 24/7 process — and the product at the end of the dependency chain, whose API container is capped at 512 MB in its production compose +decision_ref: Sdk/ADR-0063 +--- + +# Proposal: a-call-that-ended-is-released-when-it-ends + +## Why + +What the session manager holds for a call is released only when *another* call completes, and the +release can stop working permanently. Neither is a design anyone chose. + +**The eviction can wedge, and then nothing is ever released again.** +`CallSessionManager.EvictStaleCompleted` (`src/Verbara.Sdk.Sessions/Manager/CallSessionManager.cs:410-421`) +peeks the head of its completion queue and dequeues **inside the loop body**: + +```csharp +while (_completedOrder.TryPeek(out var oldId) && + _sessions.TryGetValue(oldId, out var old) && + old.CompletedAt < cutoff) +{ + _completedOrder.TryDequeue(out _); +``` + +If the head is no longer in `_sessions`, or its `CompletedAt` is `null` (a nullable `<` is false for +`null`), the loop exits **without dequeuing**. That head stays at the front for the life of the +process, and every later call adds an entry that is never removed. + +The trigger is reachable in code, not hypothetical: `OnSessionCompleted` enqueues unconditionally on +its first line (`:377`) and is called at `:233` **even when both state transitions failed** — which is +what happens to a session that is already terminal. The id is enqueued twice; once the first copy is +evicted, the second copy's lookup fails and the queue is wedged. + +**Even unwedged, the bound is weak.** Eviction runs only from `OnSessionCompleted`, so a process that +stops completing calls stops releasing memory while it keeps accepting them. There is no timer and no +count cap: `SessionOptions.MaxCompletedSessions` (default 1000, +`src/Verbara.Sdk.Sessions/Manager/SessionOptions.cs:12`) is declared and **read by nothing**. + +**And the manager's eviction frees almost nothing today.** `InMemorySessionStore` — the default, and +what the product resolves because it registers no store package — keeps the *same session object +reference*. When the manager evicts, the store still pins the whole session graph. Two other +structures also only grow: `_byChannelId`, whose stranded entries are scanned linearly on every queue +join, and `BridgeManager._bridges`, which marks a destroyed bridge with `DestroyedAt` and never +removes it, so `BridgeCount` counts every bridge ever created. + +The numbers make it a date rather than a worry: the product's own production compose caps that +container at **512 MB** (`docker-compose.production.yml:50`). + +**What is *not* wrong**, measured rather than assumed, because an earlier sweep of this area got two +of these backwards: the Redis store expires its own keys with a TTL of `CompletedRetention` +(`RedisSessionStore.cs:86`), so "no store shrinks" holds for the in-memory and Postgres stores only; +and `GetRecentCompleted` *is* exercised by a test — one that asserts only +`HaveCountGreaterOrEqualTo(3)`, so it can detect neither growth nor eviction. + +## What Changes + +- **Eviction stops depending on the head of the queue being well-formed.** A head that cannot be + evaluated is discarded rather than left in place. This is the wedge, and it is the only item here + that turns a bounded structure into an unbounded one. +- **A session is enqueued for release once.** A completion that finds the session already terminal + does not enqueue it a second time. +- **The bound is time *and* count.** `MaxCompletedSessions` starts being honoured — it is already + public, already documented and already defaulted, so this is the option's declared meaning finally + taking effect. Time remains the floor: a completed session is never released before + `CompletedRetention` has passed. +- **Release is evaluated on arrival as well as on completion**, so a process that stops completing + calls still releases what it already holds. Without a timer: the evaluation rides the events the + manager already handles. +- **The default in-memory store follows the manager.** Releasing in the manager while the store pins + the same object frees nothing; a durable store keeps its own record and its own retention, which is + what the Redis store already does. +- **A destroyed bridge is released.** `BridgeManager` stops retaining every bridge ever created. +- **What the process holds becomes visible** — resident counts as gauges, so an operator can see the + bound working instead of inferring it from memory graphs. +- **Not in scope, deliberately:** any release of a session that is **not terminal**. Ageing a live + call by a clock is exactly what `a-reconnect-reload-is-a-diff-not-a-wipe` rejected with a + measurement, and the number that would make it arguable is owed by the product repo and unmeasured. + Stranded sessions are that change's subject, not this one's. Also out of scope: registering + `SessionReconciliationService` anywhere, the Postgres store's retention, and the consumer-side + accumulation in Pro and Platform. + +## Capabilities + +### New Capabilities + +- `session-residency` — what the SDK guarantees about how long it holds a call after that call has + ended: that a terminal session is released under a bound of both time and count, that the release + cannot be disabled by the state of its own bookkeeping, that it is evaluated without waiting for + another call to end, and that a live call is never released by it. + +### Modified Capabilities + +None. `session-persistence-lifecycle` governs the token a save runs under and says nothing about how +long anything is held. + +## Impact + +- `src/Verbara.Sdk.Sessions/Manager/CallSessionManager.cs` — the eviction, the enqueue, and the + release trigger. +- `src/Verbara.Sdk.Sessions/Manager/SessionOptions.cs` — `MaxCompletedSessions` gains a reader; its + default and its documented meaning do not change. +- `src/Verbara.Sdk.Sessions/Internal/InMemorySessionStore.cs` and `SessionStoreBase` — the default + store follows the manager's release. +- `src/Verbara.Sdk.Live/Bridges/BridgeManager.cs` — a destroyed bridge is released. +- `docs/decisions/0063-*.md`, plus the ADR-count coupling: `README.md`'s `**N ADRs**` figure, its + `docs/claim-registry.md` row, and the `docs/decisions/README.md` catalog row, all in the same PR. +- **No public API is removed.** `MaxCompletedSessions` starts being honoured, which is a behaviour + change for a consumer that set it high and relied on it being ignored — recorded as **BREAKING** + in the CHANGELOG rather than assumed harmless. +- Downstream: the product pins an older SDK, so it receives this on its next bump. No consumer code + change is required. +- **Sequencing:** this change edits the same method bodies as + `a-reconnect-reload-is-a-diff-not-a-wipe` (`OnChannelRemoved`, `OnSessionCompleted`, + `EvictStaleCompleted`). It lands **after** that change, not beside it. That change also *reduces* + what this one has to bound: calls stranded in a connected state become terminal and therefore + releasable, and phantom sessions stop being minted. diff --git a/openspec/changes/a-call-that-ended-is-released-when-it-ends/specs/session-residency/spec.md b/openspec/changes/a-call-that-ended-is-released-when-it-ends/specs/session-residency/spec.md new file mode 100644 index 00000000..1e08b652 --- /dev/null +++ b/openspec/changes/a-call-that-ended-is-released-when-it-ends/specs/session-residency/spec.md @@ -0,0 +1,166 @@ +# Spec Delta + +## Purpose + +What the SDK guarantees about how long it holds a call after that call has ended: that the hold is +bounded by both time and count, that the release cannot be switched off by the state of its own +bookkeeping, that it does not wait for another call to end, and that it never touches a call that is +still live. + +## ADDED Requirements + +### Requirement: A call that has ended SHALL be released under a bound of both time and count + +State held for a call that has reached a terminal state SHALL be released once it is older than the +configured retention, and SHALL also be released, oldest first, once the number of retained ended +calls exceeds the configured maximum. Time is the floor: an ended call SHALL NOT be released before +the retention period has passed, whatever the count says. + +Both bounds are already part of the configuration surface. A maximum that is declared and never +applied is not a bound. + +**BREAKING**: a consumer that set the maximum high and relied on it having no effect will now see +ended calls released at that number. + +#### Scenario: The retention period bounds the hold + +- **GIVEN** a retention period and a number of calls that have ended +- **WHEN** more than that period has passed since a call ended +- **THEN** that call is no longer retained + +#### Scenario: The maximum bounds the hold + +- **GIVEN** a maximum number of retained ended calls +- **WHEN** more ended calls are retained than that maximum, all of them older than the retention period +- **THEN** the oldest are released until the count is within the maximum + +#### Scenario: Time wins over count + +- **GIVEN** more ended calls retained than the maximum allows +- **WHEN** none of them is older than the retention period +- **THEN** none is released + +### Requirement: The release MUST NOT be disabled by the state of its own bookkeeping + +Release SHALL make progress regardless of the condition of the records it walks. An entry that +cannot be evaluated — because the call it names is no longer retained, or because it carries no +completion time — SHALL be discarded rather than left in place, and SHALL NOT prevent any other +entry from being released. + +This is stated as a requirement rather than left to implementation care because its failure mode is +silent and permanent: nothing reports it, and every call retained afterwards is retained for the life +of the process. + +#### Scenario: An entry naming a call that is no longer retained + +- **GIVEN** a release queue whose oldest entry names a call that is no longer held +- **WHEN** release runs +- **THEN** that entry is discarded +- **AND** every other entry old enough to be released is released + +#### Scenario: An entry with no completion time + +- **GIVEN** a release queue whose oldest entry carries no completion time +- **WHEN** release runs +- **THEN** that entry is discarded +- **AND** release continues past it + +#### Scenario: A call is queued for release once + +- **GIVEN** a call that has already reached a terminal state +- **WHEN** a further ending is reported for it +- **THEN** it is not queued for release a second time + +### Requirement: Release SHALL NOT wait for another call to end + +Release SHALL be evaluated when calls arrive as well as when they end. A process that stops +completing calls while still accepting them SHALL still release what it already holds. + +#### Scenario: Arrivals without completions + +- **GIVEN** retained ended calls older than the retention period +- **WHEN** new calls arrive and none of them ends +- **THEN** the ended calls are released + +#### Scenario: No traffic at all + +- **GIVEN** retained ended calls older than the retention period +- **WHEN** no call arrives and none ends +- **THEN** nothing is required to happen, and nothing grows + +### Requirement: A call that is still live SHALL NOT be released by this bound + +Release SHALL consider only calls in a terminal state. A call that has not ended — whatever its age, +and whatever has or has not been observed about it — SHALL NOT be released, counted towards the +maximum, or altered by this bound. + +Calls that appear stuck are a separate problem with a separate cause; ageing them out by a clock has +been measured to mark healthy calls dead. + +#### Scenario: An old call that is still connected + +- **GIVEN** a connected call older than the retention period +- **WHEN** release runs +- **THEN** that call is untouched and still reported as active + +#### Scenario: A live call does not consume the maximum + +- **GIVEN** more live calls than the configured maximum +- **WHEN** release runs +- **THEN** no live call is released, and no ended call is released early because of them + +### Requirement: The default store MUST NOT retain a call the manager has released + +The store the SDK resolves when a consumer registers none SHALL release a call when the manager +does. A store that keeps a reference to the same call the manager released frees nothing, so the +bound would be reported as working while the memory stays held. + +A store that provides its own durability and its own retention SHALL keep it. This requirement is +about the default in-memory store, not about durable ones. + +#### Scenario: The in-memory default follows the manager + +- **GIVEN** the SDK resolving its default store +- **WHEN** the manager releases an ended call +- **THEN** the store no longer retains it + +#### Scenario: A durable store keeps its own retention + +- **GIVEN** a consumer that registered a durable store with its own retention +- **WHEN** the manager releases an ended call +- **THEN** the store's own retention decides what it keeps + +### Requirement: What the process retains SHALL be observable + +The number of calls retained SHALL be published as a measurement an operator can read, separating +live calls from ended ones still held. A bound that cannot be observed is indistinguishable from a +bound that has stopped working — which is the failure this capability exists to make impossible. + +#### Scenario: The counts are published + +- **WHEN** an operator reads the SDK's published measurements +- **THEN** the number of live calls and the number of retained ended calls are both available + +#### Scenario: The counts move with the bound + +- **GIVEN** ended calls being released +- **WHEN** the operator reads the measurements again +- **THEN** the retained count reflects the release + +## Architectural Risk + +**Level:** MEDIUM. + +**Affected:** `Verbara.Sdk.Sessions` and `Verbara.Sdk.Live`, and through them every consumer that +holds call state — including the closed-source cluster layer and the product above it. No public API +is removed; one published option starts taking effect. + +**Mitigation:** the dangerous direction here is releasing something a consumer still needs, not +holding too long, so the requirements are written to fail towards holding: time is a floor the count +cannot undercut, and only terminal calls are eligible at all. A separate change already established, +with a measurement, that ageing non-terminal calls by a clock marks healthy calls dead — that path is +excluded here by requirement rather than by convention. The residual risk is a consumer that set the +maximum high while relying on it being ignored; it is a published option with a documented meaning, +so the change is to honour the contract rather than to alter it, and it is called out as breaking. +The store requirement exists because the opposite mistake — a bound that appears to work while the +default store pins everything it released — is the one this area has already made once. diff --git a/openspec/changes/a-call-that-ended-is-released-when-it-ends/tasks.md b/openspec/changes/a-call-that-ended-is-released-when-it-ends/tasks.md new file mode 100644 index 00000000..35301fb7 --- /dev/null +++ b/openspec/changes/a-call-that-ended-is-released-when-it-ends/tasks.md @@ -0,0 +1,118 @@ +# Tasks + +Execution follows `rules.tasks`: **a fresh subagent per task, never inline in the main session.** +Phase A is batched, Phase B is one focused subagent per component, Phase C is batched. + +**Sequencing, not optional.** This change edits the same method bodies as +`a-reconnect-reload-is-a-diff-not-a-wipe` (`OnChannelRemoved`, `OnSessionCompleted`, +`EvictStaleCompleted`). It starts **after** that change has merged. Verify before task 1.1 that +`openspec list` no longer shows it open. + +## 1. Phase A — foundation (batched) + +- [ ] 1.1 Write the failing regression test for the wedge, against the unfixed code: complete a + session, drive the path that enqueues its id a second time while it is already terminal, let the + first copy be evicted, then complete further sessions past `CompletedRetention`. Verify it fails + today, and paste its failure verbatim into this file under the task — the repo's rule for a bug + fix is the failing test first, with its output recorded. + +- [ ] 1.2 Write the failing regression test for the count bound: retain more ended sessions than + `MaxCompletedSessions`, all older than `CompletedRetention`, and assert the oldest are released. + Verify it fails today (the option is read by nothing) and record the failure. + +- [ ] 1.3 Write the failing regression test for the store: assert that after the manager releases an + ended session the default in-memory store no longer retains it. Verify it fails today — the + store holds the same object reference. + +- [ ] 1.4 Write the measurement that has never existed: 50 completed calls, then assert the resident + count. Record the number it produces on today's code in this file. This is the "does it grow" + figure the investigation was missing; it is a measurement first and a regression test second. + +- [ ] 1.5 Write `docs/decisions/0063-what-the-sdk-holds-for-a-call-is-released-when-the-call-ends.md` + (Status: Proposed → Accepted at merge) carrying D1–D6 from `design.md`, including the rejected + alternatives — a timer, a count that overrides retention, and the manager reaching into the + store. Verify `openspec validate --all --strict` passes. + +- [ ] 1.6 Land the ADR-count coupling in the same commit as 1.5: `README.md`'s `**N ADRs**` figure, + its `docs/claim-registry.md` row, and the `docs/decisions/README.md` catalog row. Verify + `dotnet test Tests/Verbara.Sdk.OpenTelemetry.Tests/` passes — two guards there fail if any of + the three is missed. + +## 2. Phase B — critical components (one focused subagent each) + +- [ ] 2.1 Make release discard an entry it cannot evaluate instead of stopping (design D1): dequeue + first, decide after. Verify 1.1 goes green, and add unit cases for both bad heads — an entry + naming a session no longer retained, and an entry with no completion time — each asserting that + every other eligible entry is still released. + +- [ ] 2.2 Stop a terminal session being queued for release twice (design D2). Verify with a test that + drives a second ending for an already-terminal session and asserts the queue gained no entry. + +- [ ] 2.3 Honour `MaxCompletedSessions` with retention as the floor (design D3). Verify 1.2 goes + green **and** that a test asserting the floor fails if the floor is removed: more retained than + the maximum, none past retention, nothing released. + +- [ ] 2.4 Evaluate release on arrival as well as on completion, with no timer and no new hosted + service (design D4). Verify with a test where ended sessions age past retention and only + arrivals follow — they are released — and a second test that an idle manager releases nothing + and grows nothing. + +- [ ] 2.5 Make the default in-memory store follow the manager's release through one additive member + on the store base type, defaulting to doing nothing so every existing store still compiles + (design D5). Verify 1.3 goes green, and that a durable store's own retention is unaffected — + the Redis store expires on its own TTL and must keep doing so. + +- [ ] 2.6 Release a destroyed bridge in `BridgeManager` (design D6). Verify with a test that + `BridgeCount` drops when a bridge is destroyed, and confirm by reading the callers that nothing + in this repo depended on destroyed bridges remaining addressable. + +- [ ] 2.7 Publish the resident counts as gauges — live calls and retained ended calls, separately. + Verify by reading them through the existing meter surface in a test, and check whether the + published instrument count in `README.md` moves; if it does, its claim-registry row moves in + the same PR. + +## 3. Phase C — integration (batched) + +- [ ] 3.1 Turn every scenario in `specs/session-residency/spec.md` into a test, including the two that + bind the failure direction (an old connected call is untouched; live calls do not consume the + maximum). Verify each scenario has a test and that removing the guard it describes turns that + test red. + +- [ ] 3.2 Strengthen `IndexAndQueryTests.GetRecentCompleted_ShouldReturnCompletedSessions`, which + asserts only `HaveCountGreaterOrEqualTo(3)` and can therefore detect neither growth nor + eviction. Verify the strengthened assertion fails if release is disabled. + +- [ ] 3.3 Write the `CHANGELOG.md` entry under `[Unreleased]`, labelled `### Fixed — BREAKING` + (ADR-0061: a `Fixed — BREAKING` does not by itself force a minor). State three observable + changes: release no longer stops permanently, `MaxCompletedSessions` now takes effect, and + `BridgeCount` no longer counts destroyed bridges. **Pick an insertion anchor distinct from any + other in-flight PR's and state it in the PR body.** + +- [ ] 3.4 Confirm the public API surface: verify `PublicAPI.Unshipped.txt` changes only by the + additive store member from 2.5, and that no `CompatibilitySuppressions.xml` is needed. + +- [ ] 3.5 **Verification.** On the integrated branch: `dotnet build Verbara.Sdk.slnx -c Release` with + **0 warnings**; the full unit lane under the CI filter; `Tests/Verbara.Sdk.Governance.Tests` and + `Tests/Verbara.Sdk.OpenTelemetry.Tests` (tree-scanning guards — green on touched projects is not + green in CI); `openspec validate --all --strict`. Then read `.github/workflows/ci.yml` and run + the remaining fast, deterministic, non-service steps it lists rather than recalling job names. + +- [ ] 3.6 Do **not** bump `Directory.Build.props`. The version is cut at release time (ADR-0055). + Verify it is absent from this change's diff. + +## 4. Recorded, not fixed here + +Found while investigating this, deliberately left out, with where each belongs — a deferred finding +without a home is how the last set was lost: + +- **`_byChannelId` and `_bridgeToSession` entries stranded by an unobserved hangup.** Their cause is + the reload defect that `a-reconnect-reload-is-a-diff-not-a-wipe` fixes; bounding them here would + hide it. Note that `_byChannelId` is scanned linearly on every queue join, so stranded entries cost + time as well as memory. +- **Sessions reconstructed by the closed-source cluster layer carry no participants**, so they can + never reach the completion path and are permanent residents that are re-snapshotted periodically. + That is the cluster layer's own repository, not this one. +- **The Postgres session store never deletes**, unlike the Redis store which expires on its own TTL. + A durable store's retention is its own decision and needs its own change. +- **The product's retention service ships disabled** with no configuration path to enable it. That is + the product repo's finding, recorded here only because this investigation surfaced it. diff --git a/openspec/changes/a-reconnect-reload-is-a-diff-not-a-wipe/.openspec.yaml b/openspec/changes/a-reconnect-reload-is-a-diff-not-a-wipe/.openspec.yaml new file mode 100644 index 00000000..75289e4b --- /dev/null +++ b/openspec/changes/a-reconnect-reload-is-a-diff-not-a-wipe/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-24 diff --git a/openspec/changes/a-reconnect-reload-is-a-diff-not-a-wipe/design.md b/openspec/changes/a-reconnect-reload-is-a-diff-not-a-wipe/design.md new file mode 100644 index 00000000..270d1092 --- /dev/null +++ b/openspec/changes/a-reconnect-reload-is-a-diff-not-a-wipe/design.md @@ -0,0 +1,140 @@ +# Design: a-reconnect-reload-is-a-diff-not-a-wipe + +## Context + +See proposal.md — *Why* for the measured behaviour and the mechanism. The design-relevant shape of +the current code: + +- `VerbaraServer.OnReconnected` clears five live managers and then streams a fresh snapshot into + them. The clearing and the loading are separate steps with nothing between them that could notice + a difference. +- `ChannelManager` owns the channel table **and** the `ChannelAdded` / `ChannelRemoved` events that + `CallSessionManager` subscribes to in `AttachToServer`. It is the only component that can raise a + removal, and its `Clear()` bypasses that. +- `CallSessionManager.OnChannelRemoved` is the single route to `CallEndedEvent`, and it decides + `Completed` versus `Failed` by reading `channel.HangupCause`, which defaults to `NotDefined`. +- `RequestInitialStateAsync` is used for both the initial load and the post-reconnect reload. On the + initial load there is nothing held, so a diff degenerates to "everything is added" — the same + behaviour it has today. + +## Goals / Non-Goals + +**Goals** + +- Make the reload a reconciliation, with the removal side reaching `CallSessionManager` through the + event it already listens to. +- Preserve correlation across the reload so one call stays one call. +- Fail towards leaving calls alone, never towards ending them. + +**Non-Goals** + +- The four other managers (`Queues`, `Agents`, `MeetMe`, `Bridges`) keep their current + clear-and-reload. They hold no session identity, nothing subscribes to their removals for + lifecycle purposes, and widening the change to them would multiply the surface without evidence + that anything depends on it. Recorded so a later reader sees a boundary, not an oversight. +- `ChannelManager.Clear()` is public and stays. This change removes one of its callers, not the + method. +- `SessionReconciliationService`, the multi-server registration set, and the unbounded growth of + `InMemorySessionStore` are out of scope (see proposal.md — *What Changes*). + +## Decisions + +### D1 — The reload is buffered, then reconciled; it is not streamed into a cleared table + +The snapshot is read into a local set first, and only a snapshot that was read to completion is +applied. Reconciliation then compares it against what is held. + +*Why:* buffering is what makes the difference knowable — you cannot compute "absent from the +snapshot" while streaming into the structure you are comparing against. It also delivers the +"a reload that cannot be trusted ends nothing" requirement structurally rather than by care: if the +enumeration throws, the buffer is discarded and nothing was mutated. `OnReconnected` already wraps +everything in a `try`/`catch` that swallows into a log line, so a failure path that mutates nothing +is the only one that stays safe under it. + +*Alternative rejected:* mark-and-sweep in place (tag every held channel, clear tags as the snapshot +arrives, remove the still-tagged ones at the end). It avoids the buffer but leaves the table in a +half-updated state if the enumeration fails midway, which is exactly the failure direction the spec +forbids. + +### D2 — Reconciliation lives in `ChannelManager`, not in `VerbaraServer` + +`ChannelManager` gains a reconcile entry point that takes the snapshot and raises `ChannelAdded` / +`ChannelRemoved` as the difference requires. `VerbaraServer.OnReconnected` stops calling +`Channels.Clear()` and hands the snapshot over instead. + +*Why:* the table and its events are one thing. A diff computed in `VerbaraServer` would have to reach +into the manager's state to learn what is held and then ask it to raise events for entries it did not +decide — two components sharing one invariant. `ChannelManager` already owns both halves. + +*Alternative rejected:* leaving the diff in `VerbaraServer` and adding a public removal-raising +method to `ChannelManager`. That is a wider public surface for a narrower benefit, and this repo +treats the public API as a contract with two downstream repos. + +### D3 — A reload-produced ending carries a marker and no hangup cause + +Ruled by the owner on 2026-09-24. A call ended because the reload proved it gone is marked as such, +and its departing participants are left without a hangup cause rather than being given `NotDefined`. + +*Why:* `NotDefined` is not neutral downstream. A consumer classifier that treats anything other than +`NormalClearing` as an abnormal ending would read every reconnect-lost call as an abnormal hangup and +act on it — in the product at the end of this dependency chain, that path leads to calling a customer +back. Asserting `NormalClearing` instead would be the opposite lie: it records as clean an ending +nobody observed. The marker is the only option that does not claim knowledge the SDK does not have. + +*Consequence for the implementation:* `CallSessionManager.OnChannelRemoved` currently derives +everything from `channel.HangupCause`. The reload-driven removal must reach it carrying "no cause", +distinct from "cause zero", and the resulting session state must follow from what the session already +was rather than from a cause that does not exist. + +*Alternatives rejected:* both are recorded in the proposal's ruling — end as `Failed` with +`NotDefined` (cheapest, but produces the spurious-callback path), or end as `Completed` with +`NormalClearing` (silent, but records an unobserved ending as normal). + +### D4 — The reload passes the correlation identifier it already receives + +`RequestInitialStateAsync` passes `StatusEvent.LinkedId` to `OnNewChannel`, and a channel already +held is reconciled rather than re-added. + +*Why:* the value is already on the wire and already parsed; not passing it is the whole reason one +call becomes three. This is the smallest half of the change and the one with the clearest evidence. + +*Residual:* whether Asterisk always populates `Linkedid` on `Status` across the supported versions +(18, 20, 22 LTS, 23) is not knowable from this repo. The requirement "a reload without correlation +does not invent calls" covers the case where it is absent, so the fix degrades rather than breaks — +but the functional lane must measure it against a real Asterisk rather than assume it. + +## Risks / Trade-offs + +- **Ending a live call by mistake** → the worst outcome available, and worse than the defect. D1 + makes a failed or partial reload mutate nothing, and the spec states it as a requirement so a test + binds it rather than a comment. +- **A consumer that relied on the ghost** → a session that stayed `Connected` forever is not a + contract anyone chose, but a consumer may have grown a workaround (its own timeout, say) that now + fires alongside the real ending. Called out as **BREAKING** in the proposal and in the CHANGELOG + entry. +- **`Linkedid` absent on some Asterisk version** → D4's residual. Degrades to today's behaviour for + that channel instead of failing; measured in the functional lane, not assumed. +- **The initial load shares the code path** → a diff against an empty table must behave exactly as + today's load. Cheap to bind with a test, and worth binding: a regression here breaks every startup, + not just reconnects. +- **Double work on a large estate** → buffering a snapshot of every channel costs memory + proportional to the estate for the duration of the reload. Bounded by what the reload already + materialises event by event, so the delta is the retained set, not the stream. + +## Migration Plan + +No migration for consumers: no public API is added or removed, and the ending arrives on the event +they already handle. A consumer that wants to distinguish a reload-produced ending reads the marker +from D3; one that does not care needs no change. + +Rollback is a revert of the change. There is no data shape, no persisted format and no configuration +switch, so nothing survives a rollback that would need undoing. + +The ADR (`Sdk/ADR-0062`) records the durable half: that a reload is a reconciliation, and that an +ending nobody observed is recorded as unknown rather than guessed in either direction. + +## Open Questions + +None that can be deferred. The one genuine fork — how a reload-produced ending is attributed — was +ruled before this document was written (D3), because it changes what the spec requires and what a +downstream product does with the result. diff --git a/openspec/changes/a-reconnect-reload-is-a-diff-not-a-wipe/proposal.md b/openspec/changes/a-reconnect-reload-is-a-diff-not-a-wipe/proposal.md new file mode 100644 index 00000000..efc55d47 --- /dev/null +++ b/openspec/changes/a-reconnect-reload-is-a-diff-not-a-wipe/proposal.md @@ -0,0 +1,102 @@ +--- +tier: MEDIANO +owner: Harol +approver: Harol +stakeholder: Every consumer that holds call state across an AMI reconnect — and the downstream repos (Sdk.Pro, Platform) that inherit this behaviour from the root of the dependency chain +decision_ref: Sdk/ADR-0062 +--- + +# Proposal: a-reconnect-reload-is-a-diff-not-a-wipe + +## Why + +An AMI reconnect silently discards every channel the SDK is tracking and then re-adds the survivors +as if they had just appeared. `CallSessionManager` is never told the channels went away, so a call +that ended during the outage stays "in progress" for the life of the process, and a call that +survived the outage is split into several sessions. Both were **measured**, not inferred, on +2026-09-24 by two tests written for this change: + +| Scenario | Measured today | +|---|---| +| The call hung up during the outage (`Status` returns nothing) | `1 active session [linked=linked-001 state=Connected participants=2]`, **0** `CallEndedEvent` | +| Both legs survived (`Status` returns both, carrying `Linkedid`) | **3** active sessions: the stale original, plus one `Created` session per leg | + +The mechanism, read line by line: + +1. `VerbaraServer.OnReconnected` (`src/Verbara.Sdk.Live/Server/VerbaraServer.cs:122`) calls + `Channels.Clear()` (`:130`). +2. `ChannelManager.Clear()` empties its two dictionaries and raises **no** `ChannelRemoved`. The + session manager's subscription hears nothing, so its sessions are untouched. +3. `RequestInitialStateAsync` (`:158-168`) re-adds the survivors through `Channels.OnNewChannel` + **without passing the status event's `LinkedId`**, although `StatusEvent.LinkedId` exists + (`src/Verbara.Sdk.Ami/Events/StatusEvent.cs:19`). Each leg therefore defaults to + `linkedId = uniqueId` and becomes a session of its own. + +This affects **both** registration paths. It is not a clustered-deployment problem. + +One consequence decides the design. The reloaded legs land in `Created`, and `Created` past +`DialingTimeout` (60 s) is exactly what `SessionReconciliationService`'s orphan branch marks +`Failed`. Registering that sweep where it does not run today would therefore mark **healthy calls +dead after every reconnect**. That option is rejected here rather than left open. + +The fix must also be honest about who can see it: `SessionReconciler` marks state and nothing else — +it persists nothing, evicts nothing and emits no domain event. The only ending a consumer ever +observes is `CallEndedEvent`, emitted solely by `OnSessionCompleted` via `OnChannelRemoved`. A repair +that does not travel that path is invisible to every consumer already written against this SDK. + +## What Changes + +- **The reload becomes a diff instead of a wipe.** `OnReconnected` stops clearing the channel table + blind. The reloaded snapshot is compared against what is held, and the difference is what drives + events — not the clearing. +- **A channel Asterisk no longer has ends its session through the normal completion path.** + `OnChannelRemoved` → `OnSessionCompleted` → `CallEndedEvent`, the one route every consumer is + already written against, rather than a new signal nobody subscribes to. **BREAKING**: a consumer + that today sees a session stay `Connected` forever will now see it end. +- **The reload carries the correlation it is given.** `RequestInitialStateAsync` passes + `StatusEvent.LinkedId` to `OnNewChannel`, and a channel already held is not re-added as new. + **BREAKING**: one surviving call stops producing extra `Created` sessions, so any consumer counting + sessions across a reconnect sees a different (correct) number. +- **A reload that fails or returns nothing verifiable ends nothing.** The diff acts only on a + snapshot it actually received; a failed or partial reload leaves every session alone. Ending a live + call by mistake is worse than the defect being fixed, so the failure direction is stated as a + requirement rather than left to the implementation. +- **Two regression tests land first, red, against the unfixed code** — the measurements in the table + above, already written at + `Tests/Verbara.Sdk.Sessions.FunctionalTests/ReconnectReloadTests.cs`. +- **Not in scope, deliberately:** registering `SessionReconciliationService` on the multi-server + registrations, and any change to what the sweep does. The measurement above is the argument + against the first; the second is a separate decision with its own risks. Also out of scope: the + multi-server shutdown token (`ADR-0059` records it as a known, deferred gap) and the unbounded + growth of `InMemorySessionStore`, both of which this change neither worsens nor repairs. + +## Capabilities + +### New Capabilities + +- `live-state-reload` — what the SDK guarantees when it reloads live state after a reconnect: that a + reload is a reconciliation rather than a replacement, that a call which is gone ends through the + path consumers already observe, that correlation survives the reload, and that an unverifiable + reload ends nothing. + +### Modified Capabilities + +None. `session-persistence-lifecycle` governs the token a save runs under and +`client-connection-state` governs a client's published state; neither states anything about what a +reload does to a session. + +## Impact + +- `src/Verbara.Sdk.Live/Server/VerbaraServer.cs` — `OnReconnected` and `RequestInitialStateAsync`. +- `src/Verbara.Sdk.Live/Channels/ChannelManager.cs` — `Clear()` is the silent step; whether it gains + a removal-raising sibling or loses its caller is the design question. +- `Tests/Verbara.Sdk.Sessions.FunctionalTests/` — the two regression tests, plus the scenarios the + spec adds. +- `docs/decisions/0062-*.md` — the durable decision. Adding an ADR also moves `README.md`'s + `**N ADRs**` figure, its `docs/claim-registry.md` row and the `docs/decisions/README.md` catalog + row, all in the same pull request. +- **No public API is added or removed.** `ChannelManager.Clear()` is public and stays; the change is + to who calls it and what the reload does around it. +- Downstream: Sdk.Pro's cluster layer and Platform both hold sessions across reconnects and inherit + the current behaviour. Neither needs a code change for the fix to reach them, which is why the + repair belongs on this side of the boundary. diff --git a/openspec/changes/a-reconnect-reload-is-a-diff-not-a-wipe/specs/live-state-reload/spec.md b/openspec/changes/a-reconnect-reload-is-a-diff-not-a-wipe/specs/live-state-reload/spec.md new file mode 100644 index 00000000..6e5c897c --- /dev/null +++ b/openspec/changes/a-reconnect-reload-is-a-diff-not-a-wipe/specs/live-state-reload/spec.md @@ -0,0 +1,142 @@ +# Spec Delta + +## Purpose + +What the SDK guarantees about live call state when it reloads from Asterisk after a reconnect: that +the reload reconciles rather than replaces, that a call the reload proves is gone ends where every +consumer is already listening, that correlation survives the reload, and that a reload it cannot +trust ends nothing. + +## ADDED Requirements + +### Requirement: A reload reconciles held state; it SHALL NOT discard it + +A reload of live state after a reconnect SHALL compare the snapshot Asterisk returns against the +state already held, and SHALL NOT drop held state before that comparison. Clearing the tracked +channels silently is prohibited: it removes the evidence the comparison needs and leaves every +observer of channel removal unaware that anything changed. + +#### Scenario: The reload is compared, not applied over a cleared table + +- **GIVEN** a tracked call whose two channels are held +- **WHEN** the connection reconnects and a reload returns both channels +- **THEN** the held channels are reconciled against the snapshot +- **AND** no observer of channel removal is notified for a channel the snapshot still contains + +#### Scenario: A silent wipe is observable as a defect + +- **GIVEN** any component subscribed to channel removal +- **WHEN** a reconnect occurs +- **THEN** that component is never asked to handle the disappearance of a channel that is still live + +### Requirement: A call the reload proves is gone MUST end through the completion path consumers already observe + +A call whose channels are absent from a reload that completed SHALL be ended through the same path a +hangup takes, so that the ending reaches a consumer as `CallEndedEvent`. An ending that only changes +internal state is not sufficient: it is invisible to every consumer written against this SDK, and the +reload is the last notification such a call will ever produce. + +**BREAKING**: a consumer that today observes such a session remain in a connected state for the life +of the process will now observe it end. + +#### Scenario: The call ended during the outage + +- **GIVEN** a connected call with two participants +- **WHEN** the connection reconnects and the reload returns no channels +- **THEN** that call ends +- **AND** exactly one `CallEndedEvent` is raised for it +- **AND** it is no longer reported among the active calls + +#### Scenario: The ending is not merely a state change + +- **GIVEN** the same call +- **WHEN** it is ended by the reload +- **THEN** the ending is delivered through the same event a hangup would have produced +- **AND** a consumer that subscribes only to call endings observes it + +### Requirement: Correlation SHALL survive the reload + +A reload SHALL carry the correlation identifier Asterisk provides for each channel, and a channel +already held SHALL NOT be re-admitted as a newly appeared channel. One call before a reconnect +remains one call after it. + +**BREAKING**: one surviving call stops producing additional call records across a reconnect, so a +consumer counting calls across a reconnect observes a different, lower, correct number. + +#### Scenario: Both legs of one call survive the outage + +- **GIVEN** a connected call whose two channels share one correlation identifier +- **WHEN** the connection reconnects and the reload returns both channels with that identifier +- **THEN** exactly one call is reported as active +- **AND** it is the same call, under the identity it had before the reconnect + +#### Scenario: A reload without correlation does not invent calls + +- **GIVEN** a connected call +- **WHEN** the reload returns a channel for which Asterisk supplies no correlation identifier +- **THEN** that channel does not create an additional call for a call already held + +### Requirement: An ending produced by a reload MUST be distinguishable from an observed hangup + +A call ended because a reload proved it gone SHALL carry a marker saying the ending came from a +reload, and SHALL NOT be attributed a hangup cause. No hangup was observed, so no cause is known: a +consumer MUST be able to tell "the reason is unknown" from "the call ended abnormally", because +downstream classifiers treat an unrecognised cause as an abnormal ending and act on it. + +#### Scenario: The ending carries its provenance + +- **GIVEN** a connected call that a completed reload proves is gone +- **WHEN** the call is ended +- **THEN** the ending is marked as having come from a reload +- **AND** the departing participants carry no hangup cause + +#### Scenario: An observed hangup is unchanged + +- **GIVEN** a call whose channels were hung up while the connection was live +- **WHEN** it ends +- **THEN** it carries the hangup cause Asterisk reported, and no reload marker + +#### Scenario: A consumer can separate the two + +- **GIVEN** one call ended by an observed hangup with an abnormal cause, and one ended by a reload +- **WHEN** a consumer classifies both +- **THEN** it can distinguish them without inferring anything from the absence of a value + +### Requirement: A reload that cannot be trusted SHALL end nothing + +A reload that fails, is interrupted, or cannot be shown to have completed SHALL leave every held call +untouched. Absence from an unfinished snapshot is not evidence that a call is gone. The failure +direction is deliberate: ending a live call in error is worse than the defect this capability exists +to remove. + +#### Scenario: The reload fails partway + +- **GIVEN** two connected calls +- **WHEN** the connection reconnects and the reload fails before it completes +- **THEN** neither call is ended +- **AND** both remain active with their participants intact + +#### Scenario: The reload is never answered + +- **GIVEN** a connected call +- **WHEN** the connection reconnects and Asterisk never answers the state request +- **THEN** the call is not ended + +## Architectural Risk + +**Level:** MEDIUM. + +**Affected:** `Verbara.Sdk.Live` and, through it, every consumer that holds call state — including +Sdk.Pro's cluster layer and Platform, which inherit this behaviour unchanged from the root of the +dependency chain. No public API is added or removed; what changes is which events a consumer receives +after a reconnect, and how many calls it then holds. + +**Mitigation:** the two behaviours this change corrects are already measured rather than assumed, and +those measurements land as failing tests before any production code moves, so the fix is bound by a +test that fails without it. The risk that matters is the opposite direction — ending a call that is +still live — which the last requirement addresses as a contract rather than as implementation +carefulness: a reload that cannot be shown to have completed ends nothing. Because the ending travels +the existing completion path, no consumer needs a code change to receive it, and no consumer receives +a new kind of event it has never handled. The rejected alternative is recorded in the proposal: a +clock-based sweep over calls in an early state would, after this reload defect is understood, mark +healthy calls dead after every reconnect. diff --git a/openspec/changes/a-reconnect-reload-is-a-diff-not-a-wipe/tasks.md b/openspec/changes/a-reconnect-reload-is-a-diff-not-a-wipe/tasks.md new file mode 100644 index 00000000..261a953f --- /dev/null +++ b/openspec/changes/a-reconnect-reload-is-a-diff-not-a-wipe/tasks.md @@ -0,0 +1,140 @@ +# Tasks + +Execution follows `rules.tasks`: **a fresh subagent per task, never inline in the main session.** +Phase A is batched, Phase B is one focused subagent per component, Phase C is batched. + +**Coordination.** `externalmedia-returns-a-channel-id-that-finds-its-stream` landed on 2026-09-24 +and no longer holds anything. What is still another session's territory is the open change +`a-published-surface-is-one-something-measures`, which claims `Verbara.Sdk.Ari`'s +`WebSocketAudioServer` keying, the audio metrics surface, and **the functional suite's coverage +claims**. Task 3.3 below adds a functional test and therefore needs that coordination stated in its +pull request, not a blanket ban: the two changes touch the same project for different reasons and +must not both edit `Tests/Verbara.Sdk.FunctionalTests/Verbara.Sdk.FunctionalTests.csproj` at once. + +## 1. Phase A — foundation (batched) + +- [ ] 1.1 Commit the two failing regression tests exactly as measured, at + `Tests/Verbara.Sdk.Sessions.FunctionalTests/ReconnectReloadTests.cs` (already written and run + on 2026-09-24, against the unfixed code). Verify by running + `dotnet test Tests/Verbara.Sdk.Sessions.FunctionalTests/ --filter "FullyQualifiedName~ReconnectReloadTests"` + and confirming **2 failed, 0 passed** with these two messages, verbatim: + + ``` + Expected _sessions.ActiveSessions to be empty because a call Asterisk no longer has cannot + still be in progress; the reload is the only notification the consumer will ever get that it + ended. Measured: 1 active session(s) [linked=linked-001 state=Connected participants=2], but + found at least one item + + Expected _sessions.ActiveSessions to contain a single item because one call is one session + across a reconnect; the reload carries the LinkedId that says so. Measured: 3 active + session(s) [linked=agent-001 state=Created participants=1 | linked=linked-001 state=Connected + participants=2 | linked=caller-001 state=Created participants=1], but found + ``` + +- [ ] 1.2 Record the fact the tests exposed about the existing suite: the shared `SessionTestFixture` + never calls `VerbaraServer.StartAsync`, which is where `Reconnected` is subscribed, so + `ReconciliationTests.Reconnection_ShouldCleanSessions_WhenServerReconnects` exercises no + reconnect at all. Verify by asserting in that test's file, or in this change's notes, that the + subscription happens in `StartAsync` and the fixture does not call it — do **not** rewrite that + test here; it belongs to whatever change fixes it. + +- [ ] 1.3 Write `docs/decisions/0062-a-reload-is-a-reconciliation-and-an-unobserved-ending-is-unknown.md` + (Status: Proposed → Accepted at merge), carrying D1–D4 from `design.md` and the owner's ruling + of 2026-09-24 on how a reload-produced ending is attributed. Verify `openspec validate --all --strict` + passes and the file exists. + +- [ ] 1.4 Land the ADR-count coupling in the same commit as 1.3: bump `README.md`'s `**N ADRs**` + figure, update its row in `docs/claim-registry.md`, and add the catalog row in + `docs/decisions/README.md`. Verify `dotnet test Tests/Verbara.Sdk.OpenTelemetry.Tests/` passes + — `ThePublishedAdrCount_ShouldMatchTheDecisionsOnDisk` and + `TheDecisionCatalog_ShouldListEveryAdrOnDisk` both fail if any of the three is missed. + +- [ ] 1.5 Add a test that the **initial** load is unchanged by anything this change will do: a first + `StartAsync` against an empty table produces exactly the channels the snapshot contains, and + the same `ChannelAdded` events as today. Verify it passes **before** Phase B, so a Phase B + regression in startup is attributable. + +## 2. Phase B — critical components (one focused subagent each) + +- [ ] 2.1 Give `ChannelManager` a reconcile entry point that takes a complete snapshot and raises + `ChannelAdded` for what is new and `ChannelRemoved` for what the snapshot does not contain + (design D2). `Clear()` stays public and untouched. Verify with unit tests over the manager + alone: added-only, removed-only, mixed, and identical-snapshot (which must raise nothing). + +- [ ] 2.2 Make `VerbaraServer` buffer the channel snapshot to completion before reconciling, and make + `OnReconnected` hand it to 2.1's entry point instead of calling `Channels.Clear()` (design D1). + A snapshot whose enumeration throws or never completes MUST leave every held channel alone. + Verify with a test whose status enumeration throws midway: zero removals, zero endings, the + held call still active with its participants intact. + +- [ ] 2.3 Pass `StatusEvent.LinkedId` through `RequestInitialStateAsync` to `OnNewChannel`, and make a + channel already held reconcile rather than re-enter as new (design D4). Verify against the + second regression test from 1.1: it must go green, reporting **1** active session under the + identity it had before the reconnect, not 3. + +- [ ] 2.4 Carry "no cause observed" from a reload-driven removal into `CallSessionManager`, so the + ending is marked as coming from a reload and the departing participants are left without a + hangup cause — distinct from `HangupCause.NotDefined` (design D3, owner ruling). The resulting + session state must follow from what the session already was, not from a cause that does not + exist. Verify with tests that a reload-ended call carries the marker and no cause, and that a + call ended by an observed hangup still carries Asterisk's cause and no marker. + +- [ ] 2.5 Make the first regression test from 1.1 go green through the normal completion path: + exactly one `CallEndedEvent`, the call gone from the active set. Verify that the event is the + same one a hangup produces — a consumer subscribed only to call endings must observe it. + +## 3. Phase C — integration (batched) + +- [ ] 3.1 Turn every scenario in `specs/live-state-reload/spec.md` into a test, including the two that + bind the failure direction (reload fails partway; reload never answered). Verify each scenario + has a test and that deleting the guard it describes turns that test red — a scenario whose + mutation survives is not bound. + +- [ ] 3.2 Write the `CHANGELOG.md` entry under `[Unreleased]`, labelled `### Changed — BREAKING` + (ADR-0061: a `Changed — BREAKING` forces a minor). State both observable changes — a lost call + now ends, and a surviving call no longer multiplies — and the marker a consumer reads to tell a + reload-produced ending apart. **Pick an insertion anchor distinct from any other in-flight PR's + and state it in the PR body**; the bottom of `[Unreleased]`, immediately above the newest + released heading, is the anchor least likely to be contested. + +- [ ] 3.3 Measure both Asterisk-side premises against a real Asterisk, do not assume either. Follow + the pattern in `Tests/Verbara.Sdk.FunctionalTests/Layer5_Integration/NetworkPartition/ConnectionCutTests.cs` + (Toxiproxy `ami-proxy`, `AutoReconnect=true`), on the supported versions: + **(a)** whether `Status` populates `Linkedid` (design D4's residual — if a version does not, verify + the "a reload without correlation does not invent calls" scenario covers it); and + **(b)** whether Asterisk replays the `Hangup` events missed during the outage. If it replays + them, the reload is not the last notification such a call produces and requirement 2's premise + narrows — so this measurement can change the spec, and it is the one task here that must run + before Phase B is called done. Record both results in the ADR. + +- [ ] 3.4 Confirm the change adds and removes no public API: verify `PublicAPI.Unshipped.txt` is + unchanged in every package this change touches, and that no `CompatibilitySuppressions.xml` is + needed. + +- [ ] 3.5 **Verification.** On the integrated branch: `dotnet build Verbara.Sdk.slnx -c Release` with + **0 warnings**; the full unit lane under the CI filter; `Tests/Verbara.Sdk.Governance.Tests` + and `Tests/Verbara.Sdk.OpenTelemetry.Tests` (tree-scanning guards — green on touched projects + is not green in CI); `openspec validate --all --strict`. Then read + `.github/workflows/ci.yml` and run the remaining fast, deterministic, non-service steps it + lists rather than recalling job names. + +- [ ] 3.6 Do **not** bump `Directory.Build.props`. The version is cut at release time (ADR-0055), so + this change ships its CHANGELOG entry and the release that carries it takes the minor ADR-0061 + requires. Verify `Directory.Build.props` is absent from this change's diff. + +## 4. Measured elsewhere, or not yet owned + +The investigation that produced this change proposed five measurements. Two are here (1.1 and 3.3). +The other three are recorded so they are not lost, because a finding deferred without a home is how +the last one was lost: + +- **How long a healthy inbound call legitimately sits in `Created`.** Dropped from this change on + purpose: it exists to decide whether a clock-based sweep can be safe, and this change rejects that + sweep with the measurement in `proposal.md` instead. It becomes owed again the day anyone reopens + the sweep, and it is a Platform measurement, not an SDK one. +- **Whether the session table and the in-memory store grow without bound** (50 completed calls, + count what stays resident; `MaxCompletedSessions` is declared and read by nothing, and eviction + fires only when another session completes). Out of scope here — this change neither worsens nor + repairs it — and **it has no open change of its own**. It needs one. +- **The product-level blast radius of a stranded call** (conversation left active, voice capacity + held, agent left busy). Belongs to the Platform repo, not this one.