diff --git a/CHANGELOG.md b/CHANGELOG.md index bd8b0dd9..96b6268c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,57 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Fixed — BREAKING: `ExternalMediaActivity` could not reach an AudioSocket stream by any configuration (#N) + +`ExternalMediaActivity` accepts an `AudioSocketServer` in its constructor and then polls +`GetStream(Channel.Id)` for the stream. It could never hit. Measured against a real Asterisk 22.9.0 +and 23.4.1, which behave identically: + +- With the activity's **defaults**, `POST /channels/externalMedia` returns 200 and a **UnicastRTP** + channel. Nothing connects to the AudioSocket server, and the poll waits out its 30-second timeout. +- Setting `Encapsulation = "audiosocket"` returns `HTTP 400 transport must be 'tcp' for audiosocket + encapsulation`, because the activity sends `transport` only when a caller sets it. +- Supplying the transport too returns `HTTP 400 data can not be empty`. + +And underneath those, the identifiers never matched. Asterisk's `data` parameter becomes the UUID in +the identification frame — the value the stream table is keyed by — while `channelId` becomes the ARI +`Channel.Id`. A probe with two distinct, byte-order-asymmetric UUIDs settles which travels: + +```text +channelId = 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 +data = f9e8d7c6-b5a4-3928-1706-f5e4d3c2b1a0 + +channel.id : 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 +HEX : 01 00 10 f9 e8 d7 c6 b5 a4 39 28 17 06 f5 e4 d3 c2 b1 a0 +UUID on the wire == channelId ? False == data ? True +``` + +- **`CreateExternalMediaAsync` gains a `channelId` parameter** on `IAriChannelsResource` and + `AriChannelsResource`. Asterisk documents it as the unique id to assign the channel on creation, and + spells it camelCase among snake_case siblings — a misspelling is ignored silently, so the URL test + asserts the literal. +- **The activity derives its request from the server it was handed.** AudioSocket encapsulation + implies `transport = "tcp"`, and one freshly minted UUID goes to both `channelId` and `data`, so + `Channel.Id` **is** the key. The mint is canonical lowercase, and that is load-bearing: the table is + an ordinal dictionary keyed by `Guid.ToString()`, and an uppercase identifier creates a channel + whose stream cannot be found. +- **An `AudioSocketServer` supplied against a non-AudioSocket encapsulation is now refused** at the + start, instead of creating an RTP channel and reporting a connection failure thirty seconds later. +- **The contract text now says what the key is.** `IAudioServer.GetStream` said "Get an active stream + by channel ID" — on the interface and, verbatim, on both shipped implementations. It now names the + key and its form for each, including that `WebSocketAudioServer` keys on the last segment of the + request URL instead. That second mismatch is real and is tracked separately, with a probe as its + first task rather than a design. + +**What a consumer must do.** Recompile, and name the cancellation token if you passed it positionally. +The parameter was **appended** rather than placed first among the optionals, deliberately: every slot +from `encapsulation` to `data` is `string?`, so inserting ahead of them would have rebound each +argument one place over and gone on compiling — wrong values on the wire with no diagnostic anywhere. +Appending produces `CS1503`, which is loud. Anyone implementing `IAriChannelsResource` gains a member. + +**Migration guide:** [`docs/guides/externalmedia-channel-id-migration.md`](docs/guides/externalmedia-channel-id-migration.md) +— required by ADR-0028 for a minor that carries a breaking change. + ### Changed — BREAKING: AudioSocket spoke a protocol Asterisk does not, so neither server ever completed a handshake (#302) Both AudioSocket implementations read a **four**-byte frame header. Asterisk sends **three**: one byte diff --git a/Tests/Verbara.Sdk.Activities.Tests/Activities/ActivityTests.cs b/Tests/Verbara.Sdk.Activities.Tests/Activities/ActivityTests.cs index 2acc89ff..401dc05e 100644 --- a/Tests/Verbara.Sdk.Activities.Tests/Activities/ActivityTests.cs +++ b/Tests/Verbara.Sdk.Activities.Tests/Activities/ActivityTests.cs @@ -1,7 +1,9 @@ using Verbara.Sdk; using Verbara.Sdk.Activities.Activities; using Verbara.Sdk.Activities.Models; +using Verbara.Sdk.Ari.Audio; using FluentAssertions; +using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; namespace Verbara.Sdk.Activities.Tests.Activities; @@ -325,10 +327,17 @@ public async Task AriActivityBase_ShouldThrow_WhenStartedTwice() var channelsResource = Substitute.For(); ariClient.Channels.Returns(channelsResource); #pragma warning disable CA2012 + // NAMED, including channelId. A positional list ending in Arg.Any() would + // bind the token to the string? channelId slot (CS1503), and leaving channelId out of a named + // list is worse: the compiler supplies its default null, NSubstitute equality-matches that + // null, and the setup silently stops matching the moment the activity sends a real id — the + // test would then die on a null Channel rather than on an argument mismatch. channelsResource.CreateExternalMediaAsync( - Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any(), Arg.Any()) + app: Arg.Any(), externalHost: Arg.Any(), format: Arg.Any(), + encapsulation: Arg.Any(), transport: Arg.Any(), + connectionType: Arg.Any(), direction: Arg.Any(), + data: Arg.Any(), channelId: Arg.Any(), + cancellationToken: Arg.Any()) .Returns(new ValueTask(new AriChannel { Id = "ch-1" })); #pragma warning restore CA2012 @@ -389,10 +398,15 @@ public async Task ExternalMediaActivity_CancelAsync_ShouldHangupChannel() ariClient.Channels.Returns(channelsResource); #pragma warning disable CA2012 + // NAMED, including channelId — see the note on the substitute above: omitting channelId from + // a named list leaves it equality-matched against null, which stops matching as soon as the + // activity supplies one. channelsResource.CreateExternalMediaAsync( - Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any(), Arg.Any(), - Arg.Any(), Arg.Any(), Arg.Any()) + app: Arg.Any(), externalHost: Arg.Any(), format: Arg.Any(), + encapsulation: Arg.Any(), transport: Arg.Any(), + connectionType: Arg.Any(), direction: Arg.Any(), + data: Arg.Any(), channelId: Arg.Any(), + cancellationToken: Arg.Any()) .Returns(callInfo => { // Return a channel, then simulate the audio server never connecting @@ -424,6 +438,312 @@ public async Task ExternalMediaActivity_CancelAsync_ShouldHangupChannel() await channelsResource.Received(1).HangupAsync("ext-ch-1", Arg.Any()); } + [Fact] + public async Task StartAsync_ShouldSendDataAndTcpTransport_WhenEncapsulationIsAudioSocket() + { + // ARGUMENT-CAPTURE, deliberately. The tempting test — "GetStream(Channel.Id) returns a + // stream" — cannot fail here: with a substituted resource the test picks BOTH the Channel.Id + // handed back AND the uuid its own fake client would send, so it can be made green against + // the unfixed activity. That closed loop is the defect this change exists to correct. + // What a real Asterisk rejects is the REQUEST, before any stream exists: + // RUN G (probe-capture.txt) — encapsulation=audiosocket, no transport + // -> HTTP 400 "transport must be 'tcp' for audiosocket encapsulation" + // RUN D (probe-capture.txt) — encapsulation=audiosocket&transport=tcp, no data + // -> HTTP 400 "data can not be empty" + // So the assertion is on the arguments that leave the activity. + var ariClient = Substitute.For(); + var channelsResource = Substitute.For(); + ariClient.Channels.Returns(channelsResource); + + string? sentEncapsulation = null; + string? sentTransport = null; + string? sentData = null; + +#pragma warning disable CA2012 + channelsResource.CreateExternalMediaAsync( + Arg.Any(), Arg.Any(), Arg.Any(), + encapsulation: Arg.Any(), transport: Arg.Any(), + connectionType: Arg.Any(), direction: Arg.Any(), + data: Arg.Any(), + // NAMED, and the token especially. B1 inserted channelId between data and the token, so a + // ninth POSITIONAL Arg.Any() would bind to a string? parameter and this + // file would stop compiling — CS1503, the trap the two substitutes above carried. Naming + // the token survived that insertion. channelId is named here too now that it exists: left + // out of a named list the compiler supplies null, NSubstitute equality-matches it, and the + // setup would stop matching the moment B2 makes the activity send a real uuid. + channelId: Arg.Any(), + cancellationToken: Arg.Any()) + // ForAnyArgs, not Returns: the fix adds a channelId parameter to this method, and an + // argument-by-argument match would leave it compared against its default null — the + // setup would stop matching the moment the activity starts sending one, and the test + // would fail on a null channel rather than on its own assertions. + .ReturnsForAnyArgs(callInfo => + { + // Positional capture, and the indices below survived B1 because it APPENDED + // channelId after data rather than inserting it among the optionals: + // 0 app, 1 externalHost, 2 format, 3 encapsulation, 4 transport, + // 5 connectionType, 6 direction, 7 data, 8 channelId, 9 CancellationToken. + // That is a property of where the parameter landed, not a guarantee: any future + // insertion before index 7 renumbers these silently, because every slot from 3 to 8 + // is string? and ArgAt would happily read the wrong one. + sentEncapsulation = callInfo.ArgAt(3); + sentTransport = callInfo.ArgAt(4); + sentData = callInfo.ArgAt(7); + return new ValueTask(new AriChannel { Id = "ext-ch-audiosocket" }); + }); +#pragma warning restore CA2012 + + // Never started: the activity only reads GetStream off it, and an unbound server needs no + // port. Nothing will ever connect, so the run ends in the timeout below — that is how the + // run ends, not what this test measures. + await using var audioSocketServer = new AudioSocketServer( + new AudioServerOptions { AudioSocketPort = 0, ListenAddress = "127.0.0.1" }, + NullLogger.Instance); + + var activity = new ExternalMediaActivity(ariClient, audioSocketServer) + { + App = "test", + ExternalHost = "127.0.0.1:19099", + Encapsulation = "audiosocket", + ConnectionTimeout = TimeSpan.FromMilliseconds(50) + }; + + var start = () => activity.StartAsync().AsTask(); + await start.Should().ThrowAsync( + "nothing connects to the audio server in this test, so the activity times out after " + + "ConnectionTimeout — the create call it made on the way there is what is asserted below"); + + // Positive control, and it is load-bearing. Everything below asserts that a captured value is + // null, and a capture that never ran reads null too — so without one captured value that MUST + // be non-null today, a broken offset or an unmatched substitute would go red in exactly the + // shape of the defect and measure nothing. This repository has shipped that twice. + sentEncapsulation.Should().Be("audiosocket", + "the capture must be reading the real argument array: this is the one value the unfixed " + + "activity already sends, so if it does not arrive the nulls below prove nothing"); + + sentData.Should().NotBeNull( + "an audiosocket create with no data is HTTP 400 \"data can not be empty\" " + + "(probe-capture.txt RUN D), so the activity must supply the identification uuid"); + sentTransport.Should().Be("tcp", + "audiosocket encapsulation on any other transport is HTTP 400 \"transport must be 'tcp' " + + "for audiosocket encapsulation\" (probe-capture.txt RUN G)"); + } + + [Fact] + public async Task StartAsync_ShouldSendOneCanonicalLowercaseIdentifierAsBothChannelIdAndData_WhenEncapsulationIsAudioSocket() + { + // The identity this change buys: Channel.Id and the AudioSocket identification UUID are one + // value, in the one spelling AudioSocketServer's table can be hit with. Both halves are + // measured, not reasoned: + // probe-capture.txt RUN A — channelId and data given DIFFERENT values: HTTP 200, + // Channel.Id is the channelId, the wire carries the data, and no lookup crosses them. + // probe-capture.txt RUN C — data only: Channel.Id is an Asterisk uniqueid ("1790244226.1") + // that appears in no stream table. + // probe-capture.txt RUN U — an UPPERCASE identifier: HTTP 200, Channel.Id echoed back in + // the spelling sent, while the wire bytes render through AudioSocketSession.ParseUuid as + // canonical lowercase into an ORDINAL dictionary. Created channel, unfindable stream, + // no error on any hop. + // RUN U's negative control is run against the real server rather than this substitute, in + // AudioSocketServerTests.GetStream_ShouldMiss_WhenIdentifierIsNotCanonicalLowercase. + var ariClient = Substitute.For(); + var channelsResource = Substitute.For(); + ariClient.Channels.Returns(channelsResource); + + string? sentEncapsulation = null; + string? sentData = null; + string? sentChannelId = null; + +#pragma warning disable CA2012 + channelsResource.CreateExternalMediaAsync( + app: Arg.Any(), externalHost: Arg.Any(), format: Arg.Any(), + encapsulation: Arg.Any(), transport: Arg.Any(), + connectionType: Arg.Any(), direction: Arg.Any(), + data: Arg.Any(), channelId: Arg.Any(), + cancellationToken: Arg.Any()) + .ReturnsForAnyArgs(callInfo => + { + // 0 app, 1 externalHost, 2 format, 3 encapsulation, 4 transport, + // 5 connectionType, 6 direction, 7 data, 8 channelId, 9 CancellationToken. + // Every slot from 3 to 8 is string?, so an insertion before index 7 renumbers these + // silently — see the note on the test above. + sentEncapsulation = callInfo.ArgAt(3); + sentData = callInfo.ArgAt(7); + sentChannelId = callInfo.ArgAt(8); + return new ValueTask(new AriChannel { Id = sentChannelId ?? "no-channel-id-was-sent" }); + }); +#pragma warning restore CA2012 + + await using var audioSocketServer = new AudioSocketServer( + new AudioServerOptions { AudioSocketPort = 0, ListenAddress = "127.0.0.1" }, + NullLogger.Instance); + + // Encapsulation left NULL on purpose: this is the derivation, not a value the test supplied. + var activity = new ExternalMediaActivity(ariClient, audioSocketServer) + { + App = "test", + ExternalHost = "127.0.0.1:19099", + ConnectionTimeout = TimeSpan.FromMilliseconds(50) + }; + + var start = () => activity.StartAsync().AsTask(); + await start.Should().ThrowAsync( + "nothing connects to the audio server here, so the run ends in the timeout — that is how " + + "it ends, not what is asserted below"); + + // Positive control, load-bearing for the same reason as in the test above: a capture that + // never ran reads null, and null would satisfy nothing here but would make every failure + // below look like the defect. This is the one value the derivation must produce. + sentEncapsulation.Should().Be("audiosocket", + "an AudioSocketServer with Encapsulation left null must derive audiosocket — otherwise " + + "Asterisk reads the absent parameter as rtp/udp, answers 200 with a UnicastRTP channel " + + "and nothing ever connects (probe-capture.txt RUN H/RUN H+)"); + + sentData.Should().NotBeNull("Asterisk rejects an audiosocket create with no data — " + + "HTTP 400 \"data can not be empty\" (RUN D)"); + sentChannelId.Should().NotBeNull("without channelId Asterisk mints the ARI id itself and it " + + "appears in no stream table (RUN C)"); + sentChannelId.Should().Be(sentData, + "one identifier in both parameters is what makes Channel.Id the key the stream table " + + "holds; different values create a channel whose stream cannot be found (RUN A)"); + + Guid.TryParse(sentData, out var minted).Should().BeTrue( + "Asterisk requires data to parse as a UUID — a malformed one is HTTP 500 with the " + + "listener up, the same status a dead port gives (probe-capture.txt correction #4)"); + sentData.Should().Be(minted.ToString(), + "AudioSocketSession.ParseUuid ends in new Guid(bytes, bigEndian: true).ToString(), which " + + "is canonical lowercase hyphenated, and AudioSocketServer looks that key up in a " + + "ConcurrentDictionary with the default ORDINAL comparer. This equality — " + + "not Guid.TryParseExact(\"D\"), which accepts an uppercase spelling too — is what pins " + + "the form"); + } + + [Fact] + public async Task StartAsync_ShouldSendTheCallersTransportUnchanged_WhenTransportIsSetExplicitly() + { + // The derivation is `transport ??= "tcp"`, not `transport = "tcp"`, and nothing else would + // catch the difference: every other test here leaves Transport null, so an unconditional + // assignment passes them all while quietly overwriting what a caller asked for. Asterisk + // answers a wrong transport with HTTP 400 "transport must be 'tcp' for audiosocket + // encapsulation" (probe-capture.txt RUN G) — its own message, at the create call, which is + // a better diagnosis than a silent rewrite into something that works. + var ariClient = Substitute.For(); + var channelsResource = Substitute.For(); + ariClient.Channels.Returns(channelsResource); + + string? sentTransport = null; + +#pragma warning disable CA2012 + channelsResource.CreateExternalMediaAsync( + app: Arg.Any(), externalHost: Arg.Any(), format: Arg.Any(), + encapsulation: Arg.Any(), transport: Arg.Any(), + connectionType: Arg.Any(), direction: Arg.Any(), + data: Arg.Any(), channelId: Arg.Any(), + cancellationToken: Arg.Any()) + .ReturnsForAnyArgs(callInfo => + { + sentTransport = callInfo.ArgAt(4); + return new ValueTask(new AriChannel { Id = "ext-ch-explicit-transport" }); + }); +#pragma warning restore CA2012 + + await using var audioSocketServer = new AudioSocketServer( + new AudioServerOptions { AudioSocketPort = 0, ListenAddress = "127.0.0.1" }, + NullLogger.Instance); + + var activity = new ExternalMediaActivity(ariClient, audioSocketServer) + { + App = "test", + ExternalHost = "127.0.0.1:19099", + Encapsulation = "audiosocket", + Transport = "udp", + ConnectionTimeout = TimeSpan.FromMilliseconds(50) + }; + + var start = () => activity.StartAsync().AsTask(); + await start.Should().ThrowAsync( + "nothing connects here either — the create call on the way there is what is asserted"); + + sentTransport.Should().Be("udp", + "a caller who set Transport explicitly gets exactly that on the wire; the \"tcp\" default " + + "fills an absent value and does not override a supplied one"); + } + + [Fact] + public async Task StartAsync_ShouldThrowNamingTheContradiction_WhenAudioSocketServerIsSuppliedForAnotherEncapsulation() + { + // Delta spec, Requirement 2: "An audio server that cannot be reached by the requested + // encapsulation is refused" — it SHALL fail with an error naming the contradiction and SHALL + // NOT wait out its connection timeout and report a connection failure. + // + // Chosen behaviour: throw InvalidOperationException BEFORE the create call. The rejected + // alternatives, recorded so the choice is visible: silently overriding the caller's + // Encapsulation (the caller asked for something and would get something else, with no + // diagnostic), or ignoring the server (which is today's behaviour — a 30-second wait ending + // in "Asterisk did not connect to audio server", a symptom reported one layer away from the + // cause). Refusing before the create also keeps a doomed channel from existing at all. + var ariClient = Substitute.For(); + var channelsResource = Substitute.For(); + ariClient.Channels.Returns(channelsResource); + + // The create is configured to SUCCEED even though it must never be reached. Without this the + // negative control is blunt: delete the guard and the activity dies on a NullReferenceException + // from an unconfigured substitute, which would fail this test for the wrong reason and say + // nothing about the timeout. With it, deleting the guard reproduces today's behaviour exactly — + // Asterisk creates a UnicastRTP channel (probe-capture.txt RUN H), nothing connects, and the + // activity waits out the whole ConnectionTimeout before throwing TimeoutException. Every + // assertion below then fails on the real defect. +#pragma warning disable CA2012 + channelsResource.CreateExternalMediaAsync( + app: Arg.Any(), externalHost: Arg.Any(), format: Arg.Any(), + encapsulation: Arg.Any(), transport: Arg.Any(), + connectionType: Arg.Any(), direction: Arg.Any(), + data: Arg.Any(), channelId: Arg.Any(), + cancellationToken: Arg.Any()) + .ReturnsForAnyArgs(new ValueTask(new AriChannel { Id = "ext-ch-rtp" })); +#pragma warning restore CA2012 + + await using var audioSocketServer = new AudioSocketServer( + new AudioServerOptions { AudioSocketPort = 0, ListenAddress = "127.0.0.1" }, + NullLogger.Instance); + + var activity = new ExternalMediaActivity(ariClient, audioSocketServer) + { + App = "test", + ExternalHost = "127.0.0.1:19099", + Encapsulation = "rtp", + ConnectionTimeout = TimeSpan.FromSeconds(30) + }; + + var started = System.Diagnostics.Stopwatch.GetTimestamp(); + var start = () => activity.StartAsync().AsTask(); + var thrown = await start.Should().ThrowAsync( + "an AudioSocketServer cannot be reached over rtp encapsulation, and the SDK refuses the " + + "combination instead of creating a channel that can never carry that server's stream"); + var elapsed = System.Diagnostics.Stopwatch.GetElapsedTime(started); + + thrown.Which.Message.Should().Contain("AudioSocketServer", + "the message has to name what was supplied"); + thrown.Which.Message.Should().Contain("rtp", + "and the encapsulation it contradicts, so the reader is not left to guess which half to change"); + thrown.Which.Message.Should().Contain("audiosocket", + "and the value that would resolve it"); + + activity.Channel.Should().BeNull( + "the spec requires the failure BEFORE a channel is created, not after"); + channelsResource.ReceivedCalls().Should().BeEmpty( + "nothing at all should have been asked of ARI — no externalMedia create, and so no " + + "channel to hang up"); + + // The timeout clause of the requirement, measured rather than inferred from the exception + // type. 30 s configured against a 5 s bound: a wide margin, because the claim is "did not + // wait it out", not "was fast". + elapsed.Should().BeLessThan(TimeSpan.FromSeconds(5), + "the contradiction is refused up front; today's behaviour waits out the whole " + + "ConnectionTimeout and then reports a connection failure"); + activity.Status.Should().Be(ActivityStatus.Failed, + "a refused configuration is a failed activity, not a cancelled one"); + } + [Fact] public async Task CancelAsync_ShouldNotThrow_WhenActivityAlreadyCompleted() { diff --git a/Tests/Verbara.Sdk.Ari.Tests/Audio/AudioSocketServerTests.cs b/Tests/Verbara.Sdk.Ari.Tests/Audio/AudioSocketServerTests.cs index 4b4ee9e6..798038e5 100644 --- a/Tests/Verbara.Sdk.Ari.Tests/Audio/AudioSocketServerTests.cs +++ b/Tests/Verbara.Sdk.Ari.Tests/Audio/AudioSocketServerTests.cs @@ -189,6 +189,42 @@ public async Task HandleConnection_ShouldRegisterStream_WhenUuidReceived() registeredStream.Format.Should().Be("slin16"); } + [Fact] + public async Task GetStream_ShouldMiss_WhenIdentifierIsNotCanonicalLowercase() + { + // The negative control behind ExternalMediaActivity minting its identifier with + // Guid.ToString(). The fixture is a measurement, not an argument: probe-capture.txt RUN U + // sent this exact UUID to a real Asterisk — 22.9.0 and 23.4.1, identically — as both + // channelId and data: + // -> HTTP 200 id=EC30994A-B0E1-4EBE-99BD-19CA35669F7A + // HEX: 01 00 10 ec 30 99 4a b0 e1 4e be 99 bd 19 ca 35 66 9f 7a + // Asterisk accepted the uppercase spelling and echoed Channel.Id back in it, while the + // identification frame carried sixteen raw bytes. This test puts those sixteen bytes into + // the real server and then asks it for the stream BOTH ways — the way a consumer holding + // Channel.Id would, and the way the table is actually keyed. + const string asSentToAsterisk = "EC30994A-B0E1-4EBE-99BD-19CA35669F7A"; + var wireUuid = Guid.Parse(asSentToAsterisk); + var canonical = wireUuid.ToString(); + canonical.Should().NotBe(asSentToAsterisk, + "the fixture has to differ in spelling from its canonical form, or this test controls nothing"); + + var port = GetFreePort(); + var server = CreateServer(port); + await server.StartAsync(); + + using var client = await ConnectAndSendUuidAsync(port, wireUuid, server); + + server.ActiveStreamCount.Should().Be(1, "the identification frame was accepted"); + server.GetStream(canonical).Should().NotBeNull( + "ParseUuid renders the sixteen wire bytes with new Guid(bytes, bigEndian: true).ToString(), " + + "so the table is keyed by the canonical lowercase hyphenated form"); + server.GetStream(asSentToAsterisk).Should().BeNull( + "the table is a ConcurrentDictionary with the default ORDINAL comparer. A " + + "channel created with a non-canonical channelId comes back with Channel.Id in that " + + "spelling, so GetStream(Channel.Id) misses — HTTP 200, a live stream, and no error on " + + "any hop. That silence is why the identifier is minted rather than accepted"); + } + [Fact] public async Task HandleConnection_ShouldEmitOnStreamConnected_WhenUuidReceived() { diff --git a/Tests/Verbara.Sdk.Ari.Tests/Resources/AriResourceTests.cs b/Tests/Verbara.Sdk.Ari.Tests/Resources/AriResourceTests.cs index a4115c94..7bb69d50 100644 --- a/Tests/Verbara.Sdk.Ari.Tests/Resources/AriResourceTests.cs +++ b/Tests/Verbara.Sdk.Ari.Tests/Resources/AriResourceTests.cs @@ -158,8 +158,14 @@ public async Task Channels_CreateExternalMediaAsync_ShouldPostWithParams() using var http = CreateHttpClient(handler); var sut = new AriChannelsResource(http, DefaultOptions); + // Every optional is SUPPLIED, not just asserted on. Until this change the test passed + // encapsulation and transport only, and the connection_type, direction and data appenders had + // never executed across the whole Ari suite — an assertion about a parameter the test does not + // send holds whatever the appender does, including nothing. var result = await sut.CreateExternalMediaAsync("myapp", "127.0.0.1:8000", "slin16", - encapsulation: "rtp", transport: "udp"); + encapsulation: "rtp", transport: "udp", connectionType: "client", direction: "both", + data: "f9e8d7c6-b5a4-3928-1706-f5e4d3c2b1a0", + channelId: "0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9"); result.Id.Should().Be("ch-ext"); handler.LastMethod.Should().Be(HttpMethod.Post); @@ -168,6 +174,18 @@ public async Task Channels_CreateExternalMediaAsync_ShouldPostWithParams() handler.LastRequestUri.Should().Contain("format=slin16"); handler.LastRequestUri.Should().Contain("encapsulation=rtp"); handler.LastRequestUri.Should().Contain("transport=udp"); + handler.LastRequestUri.Should().Contain("connection_type=client"); + handler.LastRequestUri.Should().Contain("direction=both"); + handler.LastRequestUri.Should().Contain("data=f9e8d7c6-b5a4-3928-1706-f5e4d3c2b1a0"); + + // The literal spelling, deliberately. Asterisk spells this one parameter in camelCase among + // snake_case siblings and ignores an unrecognised query parameter silently — "channel_id=" + // would leave the create returning HTTP 200 with an Asterisk-minted id and no error anywhere, + // so only a test on the exact bytes catches the misspelling. + handler.LastRequestUri.Should().Contain("channelId=0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9", + "Asterisk spells this parameter channelId, and silently ignores any other spelling"); + handler.LastRequestUri.Should().NotContain("channel_id=", + "the snake_case spelling the siblings use is the misspelling this assertion exists to catch"); } [Fact] diff --git a/Tests/Verbara.Sdk.FunctionalTests/Layer5_Integration/Audio/ExternalMediaChannelIdFunctionalTests.cs b/Tests/Verbara.Sdk.FunctionalTests/Layer5_Integration/Audio/ExternalMediaChannelIdFunctionalTests.cs new file mode 100644 index 00000000..897fcc6b --- /dev/null +++ b/Tests/Verbara.Sdk.FunctionalTests/Layer5_Integration/Audio/ExternalMediaChannelIdFunctionalTests.cs @@ -0,0 +1,231 @@ +namespace Verbara.Sdk.FunctionalTests.Layer5_Integration.Audio; + +using Verbara.Sdk.Activities.Activities; +using Verbara.Sdk.Ari.Audio; +using Verbara.Sdk.FunctionalTests.Infrastructure.Fixtures; +using Verbara.Sdk.FunctionalTests.Infrastructure.Helpers; +using FluentAssertions; +using Microsoft.Extensions.Logging; + +/// +/// The end-to-end claim of the externalmedia-returns-a-channel-id-that-finds-its-stream +/// change, measured rather than asserted: a real Asterisk creates a real externalMedia +/// channel, dials this process over AudioSocket, and the stream is found by the id the create call +/// handed back. +/// +/// +/// +/// Why a functional test and not a unit test. With a mocked IAriChannelsResource the +/// test chooses both the Channel.Id that comes back and the UUID its own fake client puts on +/// the wire, so GetStream(Channel.Id) can be made to hit with the defect fully present. That +/// closed loop is the whole subject of this change, so the claim can only be settled by a party that +/// is not the test: Asterisk mints the channel, Asterisk sends the identification frame, and the two +/// values are compared afterwards. The argument-capture unit test in +/// Verbara.Sdk.Activities.Tests pins what the activity sends; this pins what +/// Asterisk does with it. +/// +/// +/// The Stasis subscription is load-bearing, not boilerplate. Asterisk's externalMedia +/// validates only that app is non-empty — it never checks the application is registered — so +/// the create returns HTTP 200 against no subscriber at all. The channel then enters Stasis with +/// nobody listening, Asterisk hangs it up, and the server's table entry is gone well inside the +/// activity's 200 ms poll. A test written from the probe alone therefore times out and reads as the +/// fix not working. opens the ARI event WebSocket with +/// app=test-app, which is what registers it, so the client is connected before the +/// activity is started. +/// +/// +/// The activity is constructed with Encapsulation and nothing else. No +/// Transport, no Format: those are exactly the values the class under test is supposed +/// to derive. Supplying Transport = "tcp" here would make the test pass over a broken +/// ExternalMediaActivity, which is the failure mode this file exists to rule out. +/// +/// +/// Off the PR path (ADR-0051, ADR-0043): this lane runs in the merge queue, on the scheduled +/// matrix, and on a pull request only when it carries the ci:functional label. +/// +/// +[Collection("Functional")] +[Trait("Category", "Functional")] +public sealed class ExternalMediaChannelIdFunctionalTests : FunctionalTestBase +{ + /// + /// The Stasis application this test subscribes and creates against. It is the name + /// opens its event WebSocket with, and the two must agree: a + /// create naming an application nobody is subscribed to still returns HTTP 200 and then loses + /// the channel. + /// + private const string StasisApp = "test-app"; + + /// + /// The port the AudioSocket server binds on this host. Fixed rather than ephemeral because the + /// value travels to Asterisk inside external_host, and deliberately neither 19092 nor + /// 19093: those two are named by dialplan extensions 710 and 711 + /// (docker/functional/asterisk-config/extensions.conf) and are bound by + /// . + /// + private const int AudioSocketPort = 19094; + + /// + /// The name Asterisk resolves to the Docker host gateway. + /// AsteriskContainer maps it with WithExtraHost("host.docker.internal", + /// "host-gateway"); the server under test runs in this process, on the host, not in the + /// container. + /// + private const string HostGateway = "host.docker.internal"; + + /// + /// The encapsulation asked for, spelled as Asterisk's own error message spells it. Asterisk + /// compares it with strcasecmp, so the casing is not what is being tested here. + /// + private const string AudioSocketEncapsulation = "audiosocket"; + + /// + /// How long Asterisk has to connect and identify itself, used both as the activity's + /// ConnectionTimeout and as the server's idle deadline. Generous on purpose: this budget + /// only bounds a failure. chan_audiosocket makes its TCP connection during the + /// create call, so on the success path nothing waits at all. + /// + private static readonly TimeSpan ConnectionBudget = TimeSpan.FromSeconds(45); + + public ExternalMediaChannelIdFunctionalTests() + : base("Verbara.Sdk.Ari") + { + } + + /// + /// Creates an AudioSocket externalMedia channel through + /// against a real Asterisk and asserts that the stream the activity resolved is registered under + /// the id the create call returned, and that the UUID Asterisk put in its identification frame is + /// that same id. + /// + [Fact] + public async Task ExternalMediaActivity_ShouldResolveItsStreamByTheReturnedChannelId_WhenAsteriskConnectsOverAudioSocket() + { + // The wire's own account of the identifier, taken from the server rather than from the + // activity: OnStreamConnected fires when a session's ChannelId has been parsed out of + // Asterisk's identification frame, so this value is produced by Asterisk and is not + // anything this test chose. + var identified = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + await using var server = new AudioSocketServer( + new AudioServerOptions + { + AudioSocketPort = AudioSocketPort, + ListenAddress = "0.0.0.0", + IdleTimeout = ConnectionBudget + }, + LoggerFactory.CreateLogger()); + + using var subscription = server.OnStreamConnected.Subscribe(stream => identified.TrySetResult(stream)); + + // Bound before the create, because chan_audiosocket connects during the create call: a + // create against a port nothing is listening on is HTTP 500, not a channel that connects + // later (probe-capture.txt, PART 3). + await server.StartAsync(); + + // Subscribes the Stasis application. See the class remarks — without this the create still + // succeeds and the channel is then hung up under the poll. + await using var ariClient = AriClientFactory.Create(LoggerFactory, application: StasisApp); + await ariClient.ConnectAsync(); + + await using var activity = new ExternalMediaActivity(ariClient, server) + { + App = StasisApp, + ExternalHost = $"{HostGateway}:{AudioSocketPort}", + + // The only thing configured beyond the two required properties. Transport, the + // identification UUID and the channel id are all the activity's own to derive, and a + // test that supplied them would measure its own arrange instead of the class. + Encapsulation = AudioSocketEncapsulation, + ConnectionTimeout = ConnectionBudget + }; + + // Recorded rather than thrown so the assertions below all run and each one carries its own + // reason. A bare await would end the test on Asterisk's exception with no statement of what + // was expected of it. + var failure = await Record.ExceptionAsync(async () => await activity.StartAsync()); + + // Read off the server, not off the activity, and reported in the first assertion's reason. + // A timeout here has two very different causes — Asterisk never connected, or it connected + // and registered under a key that is not the channel id — and only the registered keys tell + // them apart. Without this the two are one indistinguishable red. + var registered = server.ActiveStreams.Select(stream => stream.ChannelId).ToArray(); + + try + { + failure.Should().BeNull( + "an AudioSocket external media create is supposed to succeed and resolve its stream " + + "end to end; instead StartAsync ended with {0}, with the returned channel id {1} " + + "and these streams registered on the server: [{2}] — a timeout with a stream " + + "registered under some other key is the defect itself, the two identifiers having " + + "come apart", + failure?.ToString() ?? "no exception", + activity.Channel?.Id ?? "no channel", + registered.Length == 0 ? "none" : string.Join(", ", registered)); + + activity.Channel.Should().NotBeNull( + "a successful create returns the channel Asterisk made, and every assertion below " + + "compares against its id"); + + activity.AudioStream.Should().NotBeNull( + "the activity resolves its stream by calling GetStream(Channel.Id) on the server it " + + "was handed, so a null here is that lookup never hitting — the exact defect this " + + "change fixes, where the table was keyed by the wire UUID and searched by an " + + "Asterisk-minted channel id"); + + activity.AudioStream!.ChannelId.Should().Be( + activity.Channel!.Id, + "the stream's ChannelId is the key its server registered it under, and the claim of " + + "this change is that an ARI channel id is that key: one identifier is sent as both " + + "channelId and data, so Channel.Id and the registration key are the same string"); + + var wireStream = await WithinAsync(identified.Task, ConnectionBudget); + + wireStream.Should().NotBeNull( + "a real Asterisk connected to this server during the create call and must have sent " + + "its AudioSocket identification frame; a null here is that frame never arriving"); + + wireStream!.ChannelId.Should().Be( + activity.Channel!.Id, + "this value was parsed out of the sixteen bytes Asterisk sent, not out of anything " + + "this test chose, and it is what settles the claim: the identification UUID and " + + "the id the create call handed back are one value"); + + activity.Channel!.Id.Should().Be( + activity.Channel.Id.ToLowerInvariant(), + "Asterisk echoes channelId back verbatim and does not normalise it (probe-capture.txt, " + + "RUN U), while AudioSocketSession.ParseUuid renders the wire bytes with " + + "Guid.ToString() into an ordinal dictionary — so only a canonical lowercase " + + "identifier can ever be found again"); + + Guid.TryParseExact(activity.Channel!.Id, "D", out _).Should().BeTrue( + "the identifier the activity minted has to be a canonical hyphenated UUID for " + + "Asterisk to accept it as data at all: a value that does not parse is HTTP 500 at " + + "the create with the listener up (probe-capture.txt, correction #4)"); + } + finally + { + if (activity.Channel is not null) + await BestEffort.AriAsync(() => ariClient.Channels.HangupAsync(activity.Channel.Id)); + } + } + + /// + /// Awaits and returns if it does not complete + /// within , so the failure is an assertion with a reason rather than a + /// bare . No wall-clock barrier: the timeout is the task's own + /// deadline, not a sleep the test races (ADR-0004 / ADR-0045). + /// + private static async Task WithinAsync(Task task, TimeSpan timeout) where T : class + { + try + { + return await task.WaitAsync(timeout); + } + catch (TimeoutException) + { + return null; + } + } +} diff --git a/Tests/Verbara.Sdk.FunctionalTests/Verbara.Sdk.FunctionalTests.csproj b/Tests/Verbara.Sdk.FunctionalTests/Verbara.Sdk.FunctionalTests.csproj index 7b15f35c..de32582a 100644 --- a/Tests/Verbara.Sdk.FunctionalTests/Verbara.Sdk.FunctionalTests.csproj +++ b/Tests/Verbara.Sdk.FunctionalTests/Verbara.Sdk.FunctionalTests.csproj @@ -23,6 +23,7 @@ + diff --git a/docker/functional/asterisk-config/extensions.conf b/docker/functional/asterisk-config/extensions.conf index d10fc20f..62a40522 100644 --- a/docker/functional/asterisk-config/extensions.conf +++ b/docker/functional/asterisk-config/extensions.conf @@ -107,8 +107,17 @@ exten => 950,1,Answer() ; `if (confJoin is null) return;` branch and is reported as passed without asserting anything. ; Defining 700 here made the originate succeed, so those five tests ran their bodies for the ; first time and failed. Renumbering keeps that pre-existing hole exactly as it was instead of -; letting this change carry someone else's red; the hole is filed separately. Extensions dialed -; by a test but absent from this context: 160, 300, 700, 999, 9998. +; letting this change carry someone else's red. +; +; The hole is filed, with a link rather than the words "filed separately" — writing that phrase with +; nothing to follow is how ADR-0060 lost its own residuals, and this comment did it too until it was +; corrected: openspec/changes/a-published-surface-is-one-something-measures. +; +; Ten extensions are dialed by a test and absent from this context, not the five this comment first +; claimed: 160, 161, 162, 163, 300, 700, 750, 999, 9998, 9999. The first count missed the tests that +; reach the context through an Exten/Context pair instead of Local/N@test-functional. The thing to +; hunt is not the numbers, though — it is the unasserting early return that makes a test pass when +; its dial goes nowhere. exten => 710,1,NoOp(AudioSocket: Verbara.Sdk.Ari audio server) same => n,Answer() same => n,AudioSocket(4f1d9c60-7a2b-4e55-9f3d-2c6a8b0e1d47,host.docker.internal:19092) diff --git a/docs/guides/README.md b/docs/guides/README.md index e9020430..251a0dba 100644 --- a/docs/guides/README.md +++ b/docs/guides/README.md @@ -6,6 +6,7 @@ Practical how-to guides for working with Verbara Sdk. |-------|-------------| | [asterisk-version-compatibility.md](asterisk-version-compatibility.md) | AMI event coverage matrix across Asterisk 18-23, listing typed classes and fallback behavior per version. | | [asterisk-version-matrix.md](asterisk-version-matrix.md) | Supported Asterisk versions (22 LTS primary, 23 Standard secondary), Docker test infrastructure, and known-divergent behavior. | +| [externalmedia-channel-id-migration.md](externalmedia-channel-id-migration.md) | Moving to the `CreateExternalMediaAsync` signature that takes a `channelId` -- why the break was bought, which callers have to be edited rather than rebuilt, and how to make an AudioSocket stream findable by the channel id the create returned. | | [high-load-tuning.md](high-load-tuning.md) | Configuration guidance for high-load scenarios (1K-100K+ agents), including EventPump sizing and buffer capacity recommendations. | | [log-analysis-prompt.md](log-analysis-prompt.md) | Ready-to-use LLM prompt for analyzing Verbara.Sdk structured log files with extract, classify, and diagnose phases. | | [log-analysis-reference.md](log-analysis-reference.md) | Tag catalog for SDK structured logs -- lists every `[TAG]`, its domain, source classes, and emitted events. | diff --git a/docs/guides/externalmedia-channel-id-migration.md b/docs/guides/externalmedia-channel-id-migration.md new file mode 100644 index 00000000..253f5e0c --- /dev/null +++ b/docs/guides/externalmedia-channel-id-migration.md @@ -0,0 +1,166 @@ +# Migrating to `CreateExternalMediaAsync` with a `channelId` + +Required by ADR-0028: a minor that carries a breaking change ships a migration guide. + +## What happened + +`CreateExternalMediaAsync` gained an optional `string? channelId` parameter, on +`Verbara.Sdk.IAriChannelsResource` and on `Verbara.Sdk.Ari.Resources.AriChannelsResource`. It sits +between `data` and the cancellation token: + +```csharp +// before +ValueTask CreateExternalMediaAsync(string app, string externalHost, string format, + string? encapsulation = null, string? transport = null, string? connectionType = null, + string? direction = null, string? data = null, CancellationToken cancellationToken = default); + +// after +ValueTask CreateExternalMediaAsync(string app, string externalHost, string format, + string? encapsulation = null, string? transport = null, string? connectionType = null, + string? direction = null, string? data = null, string? channelId = null, + CancellationToken cancellationToken = default); +``` + +Optional parameters are compile-time sugar. The nine-parameter member is gone from both assemblies, +which is why this is a break and not an addition: `CP0002` in both packages, plus `CP0006` on the +interface. Those three are declared in each package's `CompatibilitySuppressions.xml`. + +## Why a parameter is worth a break + +Asterisk's `externalMedia` takes two identifiers that are easy to mistake for one. + +- `data` is what AudioSocket sends back on the audio connection, in its identification frame. It is + the value an `AudioSocketServer` keys its stream table by, and Asterisk requires it to parse as a + UUID. +- `channelId` is the id Asterisk assigns the ARI channel it creates. It comes back as `Channel.Id`. + +Supply only `data` and Asterisk mints the channel id itself — something like `1790244226.1`, which +appears in no stream table. A consumer holding only an ARI channel id, which is what a `StasisStart` +or `ChannelHangupRequest` handler holds, then has nothing it can look the stream up with. Passing one +identifier as **both** makes `Channel.Id` the key, and that is the capability this parameter buys. + +The smaller fix that breaks nothing — mint the UUID, pass it only as `data`, and look the stream up by +the value you minted — works, and was rejected deliberately: it fixes the caller that minted the UUID +and leaves everyone downstream of it unable to find the stream. + +## What you have to do + +**Recompile.** For almost every caller that is the whole of it, because the parameter is optional and +appended rather than inserted. + +Three populations need an edit. + +### If you passed the cancellation token positionally + +```csharp +// stops compiling: the ninth positional argument now binds to string? channelId +await channels.CreateExternalMediaAsync(app, host, "slin16", "audiosocket", "tcp", null, null, + uuid, cancellationToken); +``` + +The compiler reports `CS1503` — `cannot convert from 'System.Threading.CancellationToken' to +'string?'`. Name the token: + +```csharp +await channels.CreateExternalMediaAsync(app, host, "slin16", "audiosocket", "tcp", null, null, + uuid, cancellationToken: cancellationToken); +``` + +The parameter was appended rather than placed first among the optionals — where +`CreateWithoutDialAsync` puts its own `channelId` — precisely so this break would be a compile error. +Inserting ahead of `encapsulation` would have rebound every existing positional argument to a +different `string?` parameter and gone on compiling. + +### If you implement `IAriChannelsResource` + +Fakes, decorators and proxies stop compiling with `CS0535` until they adopt the parameter. A decorator +forwards it; a fake that ignores it should still accept it, so it keeps matching the interface. + +### If you have a mocking framework set up on this method + +A setup that enumerates positional matchers ending in a `CancellationToken` hits the `CS1503` above. +Move it to named arguments — and name `channelId` explicitly: + +```csharp +channels.CreateExternalMediaAsync( + app: Arg.Any(), externalHost: Arg.Any(), format: Arg.Any(), + encapsulation: Arg.Any(), transport: Arg.Any(), + connectionType: Arg.Any(), direction: Arg.Any(), + data: Arg.Any(), channelId: Arg.Any(), + cancellationToken: Arg.Any()) +``` + +Leaving `channelId` out of a named list is the quiet failure. The compiler supplies its default +`null`, the mock equality-matches that `null`, and the setup stops matching the moment real code +supplies an id — so the test fails somewhere else entirely, on whatever the unmatched call returned. + +## How to use it + +Mint one identifier and pass it as both, in canonical lowercase hyphenated form: + +```csharp +var id = Guid.NewGuid().ToString(); // "e1a2..." — lowercase, hyphenated +var channel = await ariClient.Channels.CreateExternalMediaAsync( + app: "myapp", externalHost: "10.0.0.5:9092", format: "slin16", + encapsulation: "audiosocket", transport: "tcp", + data: id, channelId: id); + +var stream = audioSocketServer.GetStream(channel.Id); // now finds it +``` + +Three things that will cost you time if you take them on trust instead of from here: + +- **`transport = "tcp"` is mandatory with `encapsulation = "audiosocket"`.** Without it the create + returns `HTTP 400 — transport must be 'tcp' for audiosocket encapsulation`, and with it but without + `data`, `HTTP 400 — data can not be empty`. +- **The spelling is `channelId`, in camelCase**, alone among the snake_case siblings + (`external_host`, `connection_type`). Asterisk ignores a query parameter it does not recognise + rather than rejecting it, so a misspelling is an `HTTP 200` with an Asterisk-minted id and no error + anywhere — the SDK's own test asserts the literal bytes for that reason. +- **Canonical lowercase is not cosmetic.** Asterisk accepts an uppercase UUID and echoes `Channel.Id` + back in the spelling you sent, but the identification frame is sixteen raw bytes that the server + renders with `Guid.ToString()` — lowercase — into an ordinal dictionary. An uppercase id therefore + creates a channel whose stream can never be found, with no error on any hop. + +`channelId` itself is free-form as far as Asterisk is concerned; it is `data` that must parse as a +UUID. Using the same value for both is what makes the lookup work, so in practice both are a UUID. + +## If you use `ExternalMediaActivity` + +The activity now mints the identifier and supplies it to both parameters itself, in the same release +as this break — making that true is what the break was bought for. It also derives the rest of the +request, so three behaviours changed. Only the third can break a caller that works today. + +**1. `Encapsulation` left null with an `AudioSocketServer` supplied now means `audiosocket`.** It +used to mean rtp/udp, which is how the default configuration created a `UnicastRTP` channel that +nothing connected to and then threw `TimeoutException` after `ConnectionTimeout`. Nothing to do: the +configuration that was broken is the one that changed. + +**2. `Transport` left null under AudioSocket encapsulation now means `"tcp"`.** It used to be sent as +absent, which is `HTTP 400 — transport must be 'tcp' for audiosocket encapsulation`. An explicit +`Transport` is still sent exactly as given, so a wrong one still fails at the create call with +Asterisk's own message rather than being silently rewritten. + +**3. An `AudioSocketServer` with a non-AudioSocket `Encapsulation` is now refused.** + +```csharp +new ExternalMediaActivity(ariClient, audioSocketServer) +{ + App = "myapp", ExternalHost = "10.0.0.5:9092", Encapsulation = "rtp" +} +``` + +`StartAsync` throws `InvalidOperationException` naming the contradiction, before any channel is +created. Previously this created an RTP channel, waited out the whole `ConnectionTimeout` — 30 +seconds by default — and threw `TimeoutException: Asterisk did not connect to audio server`, which +reports the symptom one layer away from the cause. If you hit this, either set `Encapsulation` to +`"audiosocket"` (or leave it null, which derives it) or stop passing the `AudioSocketServer`. + +Callers with **no** audio server, or with a WebSocket server only, take an unchanged path. + +## What this does not cover + +The WebSocket transport. `WebSocketAudioServer` keys its table on the last segment of the request +path, not on an identification frame, so `channelId` does not make its lookups work. What +`externalMedia` with `transport=websocket` actually puts in the request path has not been measured, +and is tracked separately rather than designed from a reading of the code. diff --git a/openspec/changes/a-published-surface-is-one-something-measures/.openspec.yaml b/openspec/changes/a-published-surface-is-one-something-measures/.openspec.yaml new file mode 100644 index 00000000..75289e4b --- /dev/null +++ b/openspec/changes/a-published-surface-is-one-something-measures/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-24 diff --git a/openspec/changes/a-published-surface-is-one-something-measures/README.md b/openspec/changes/a-published-surface-is-one-something-measures/README.md new file mode 100644 index 00000000..c1fbf132 --- /dev/null +++ b/openspec/changes/a-published-surface-is-one-something-measures/README.md @@ -0,0 +1,6 @@ +# a-published-surface-is-one-something-measures + +Three surfaces this SDK publishes are connected to nothing that could tell whether they work: the +WebSocket audio server's lookup key, the ten audio metric instruments, and the functional tests that +dial extensions the dialplan never defines. Each is kept green by a test that cannot see the +difference. Measure first, then fix — and in the WebSocket case the measurement does not exist yet. diff --git a/openspec/changes/a-published-surface-is-one-something-measures/proposal.md b/openspec/changes/a-published-surface-is-one-something-measures/proposal.md new file mode 100644 index 00000000..5aff95d5 --- /dev/null +++ b/openspec/changes/a-published-surface-is-one-something-measures/proposal.md @@ -0,0 +1,252 @@ +--- +tier: MEDIANO +owner: Harol +approver: Harol +stakeholder: Anyone reading this SDK's audio documentation, metrics or functional results as a statement of what works — and the Pro AgentAssist deployment that consumes the ARI audio path +decision_ref: Sdk/ADR-0060 +--- + +# Proposal: a-published-surface-is-one-something-measures + +## Why + +ADR-0060 recorded two residual defects with the words "tracked separately" and no link, and the +close-out archived it anyway. `externalmedia-returns-a-channel-id-that-finds-its-stream` exists +because of that, fixes the third defect, and hands three more findings forward. This is the change +that carries them, opened before that one archives rather than after, because +`openspec/config.yaml` requires a deferred finding to land in an open change and a label such as +*tracked separately* with no link does not exempt it. + +The three are not a grab-bag. They are one failure repeated on three different kinds of surface: + +> **The repository publishes a claim, nothing connects the claim to the code, and the test that +> should tell the difference would stay green if the code were deleted.** + +- The **documentation** claims `WebSocketAudioServer` is keyed by a channel id. It is keyed by a URL + path segment, and nothing has ever measured what Asterisk puts in that path. +- The **metrics surface** claims ten instruments' worth of audio observability. Nothing under `src/` + emits any of them. The tests emit the measurements themselves and then assert they were observed. +- The **functional suite** claims to exercise ConfBridge, Parking, Transfer, DTMF, CDR and Stasis. + Ten of the extensions it dials are not defined in the context it dials them in, and the tests + return without asserting when the call does not happen. + +Every number below was measured in this worktree with `git grep` and by reading the files, because +this change's parent measured that the session's default `grep` honours `.gitignore` and undercounts. +Where a measurement contradicts the note that handed the finding over, the correction is stated here +rather than quietly absorbed. + +### F1 — `WebSocketAudioServer` keys on a URL path segment, and nobody has measured that path + +The key is computed at `src/Verbara.Sdk.Ari/Audio/WebSocketAudioServer.cs:341`: + +```csharp +var channelId = path.TrimStart('/').Split('/').LastOrDefault()?.Split('?').FirstOrDefault(); +``` + +and registered at `:266` (`_streams.TryAdd(channelId, session)`). So the table's key is whatever the +HTTP upgrade request's last path segment happens to be. This SDK's own example puts a literal there: + +```text +Examples/WebSocketMediaExample/Program.cs:10 // same => n,WebSocket(ws://127.0.0.1:9093/audio,slin16) +Examples/WebSocketMediaExample/Program.cs:78 Console.WriteLine("Configure Asterisk ... WebSocket(ws://host:9093/audio,slin16) ..."); +Examples/WebSocketMediaExample/README.md:17 same => n,WebSocket(ws://127.0.0.1:9093/audio,slin16) +``` + +Follow that example and every concurrent call registers under the key `"audio"`: the second +connection's `TryAdd` fails, the second stream is never in the table, and `ActiveStreamCount` +disagrees with reality. `CompositeAudioServer.GetStream` hands one string to both servers, which do +not agree on what a key means — its own `` now says so, which is a warning, not a fix. + +**The first task of this change is a probe, not a design.** Nothing in this repository has measured +what Asterisk actually puts in that request path, and there are two candidate producers that the code +does not distinguish between: + +- `WebSocketAudioServer`'s class summary says *"Listens for incoming WebSocket connections from + Asterisk ExternalMedia channels"* (`:34`) — the ARI `externalMedia` endpoint with + `transport=websocket`; +- the example and `ChanWebSocketControlMessage` / `IChanWebSocketSession` describe the `WebSocket()` + dialplan application of `chan_websocket`, an entirely different Asterisk feature that this repo also + supports. + +Those two put different things on the wire, and `AudioChannelVars` already names a third candidate +identifier for the second of them — `WEBSOCKET_GUID` — that the server never reads. Designing a fix +from a reading of the code is precisely how the AudioSocket wire format survived six months of green +tests. The probe comes first and its capture is committed beside this change, in the shape +`probe-capture.txt` takes for its parent. + +Three pieces of prose in the same file assert the identity the code does not hold, all of them +carried over rather than fixed by the parent change's B3, which deliberately stopped at published +contracts: + +- `:312` — "extract Sec-WebSocket-Key and channel ID from URL path"; +- `:313` — "Expected URL: `/ws/{channelId}` or `/{channelId}`", naming the placeholder `{channelId}`; +- `:34` — the class summary's "from Asterisk ExternalMedia channels", which may simply be false. + +And incidentally, `:35` cites "(ADR-1)" for the TcpListener + manual-upgrade design. This repository's +ADRs are four digits and `ADR-0001` is *Native AOT first*, which is not that decision. It is a +citation to nothing, in the one file this change opens anyway. + +**Corrections to the note that handed F1 over:** the key is computed at `:341`, not `:337`, and +registered at `:266`, not `:262`. The literal `/audio` appears in three places, not one. + +### F2 — ten instruments, zero production call sites + +`src/Verbara.Sdk.Ari/Diagnostics/AudioStreamMetrics.cs` declares a `Meter` and ten instruments: +`StreamsOpened`, `StreamsClosed`, `FramesReceived`, `FramesSent`, `BytesReceived`, `BytesSent`, +`BufferUnderruns`, `HangupFrames`, `ErrorFrames`, `FrameLatency`. Verified with `git grep`, every +reference to the type outside its own declaration is one of: + +- `Tests/Verbara.Sdk.Ari.Tests/Diagnostics/AudioStreamMetricsTests.cs` — 13 references; +- `src/Verbara.Sdk.Ari/PublicAPI.Shipped.txt` — 12 rows; +- the two OpenSpec documents that record the finding. + +`git grep -n "AudioStreamMetrics\."` returns exactly two hits for nine of the ten instruments +and three for `StreamsClosed` — in every case the `PublicAPI.Shipped.txt` row plus the test file, and +never a call site under `src/`. The declaration itself does not appear in that count, because it does +not use the qualified name; there is nothing else to count. The class-level doc tells a reader to run `dotnet-counters monitor --process-id Verbara.Sdk.Ari.Audio` +and watch. They will watch nothing move, for the lifetime of any process. + +**Its test file is the closed loop in miniature, which is why this was never caught.** Eleven `[Fact]`s: +six assert only `.Should().NotBeNull()` on a `static readonly` field initialised inline — which cannot +be null and would be a compile error if it could; four construct a `MeterListener`, call +`AudioStreamMetrics.X.Add(1)` *from the test itself*, and assert the listener observed 1; one asserts +the meter's name. Delete every production call site and all eleven stay green. That is not a +hypothetical: every production call site is already deleted, and they are green today. + +The fix is a decision, not a foregone conclusion, and this change makes the decision explicitly: +**wire the instruments to the sessions that own the events, or remove them.** Removal is breaking — +twelve rows in `PublicAPI.Shipped.txt`, `CP0002`/`CP0006`, a suppressions entry, a migration note. +Wiring is not breaking and is the likelier answer, but it is the *answer to a question*, and asking it +is the task. + +### F3 — the functional suite passes on calls that never happened + +`docker/functional/asterisk-config/extensions.conf` defines nine extensions in `[test-functional]`: +100, 150, 155, 500, 600, 710, 711, 900, 950. There is no pattern-match extension in that context — the +`_X.` catch-alls live in `[default]`, `[stasis-test]` and `[queue-test]` — so an extension that is not +listed is not reachable. + +The suite dials seventeen distinct extensions into that context, in **two** forms that a single grep +does not both catch: + +| form | distinct extensions | +|---|---| +| `Channel = "Local/N@test-functional"` | 100, 150, 155, 160, 300, 500, 600, 700, 900, 950, 999, 9998 | +| `Exten = "N"` with `Context = "test-functional"` (Originate and Redirect) | 100, 150, 155, 160, 161, 162, 163, 500, 750, 950, 999, 9998, 9999 | + +Subtracting the nine defined leaves **ten** undefined, not five: +**160, 161, 162, 163, 300, 700, 750, 999, 9998, 9999.** The note that handed this finding over said +five, and so does the comment already sitting in the dialplan at `:111`; both were derived from the +`Local/N@` form alone. `Exten =` appears 53 times in the functional suite and every one of those 53 +sites carries `Context = "test-functional"`, so the second form is not an edge case. + +**What keeps it green is an early return, and it has more than one spelling.** +`Tests/Verbara.Sdk.FunctionalTests/Layer5_Integration/ConfBridge/ConfBridgeAdvancedTests.cs` dials the +undefined 700 from ten call sites across eight `[Fact]`s, and every one of the eight ends in an +unasserting return written three different ways: + +```text +:55, :104, :162 if (confJoin is null) return; +:211, :273 if (!joinEvents.Any(e => e.Conference == confName)) return; +:339, :385, :445 if (joinEvents.IsEmpty) return; +``` + +So the shape to hunt is the early return, and hunting one spelling of it finds three of eight tests. +The parent change's own dialplan comment (`extensions.conf:104-111`) says *"those five tests"* and +names one spelling; measured, it is eight tests and three spellings. That comment also ends *"the hole +is filed separately"* and links nothing — the ADR-0060 failure reproduced, in the very commit that +was fixing ADR-0060's failure. This change is the link. + +**The shape is not confined to ConfBridge.** A brace-tracking scan of the functional suite for a bare +`return;` in a test-method body — excluding returns inside lambdas and observer callbacks — reports +**71 candidate sites across 11 files**. Hand-checked samples confirm the shape outside ConfBridge: + +```csharp +// Tests/Verbara.Sdk.FunctionalTests/Layer5_Integration/Parking/ParkingTests.cs:61-65 +var channelResult = await Task.WhenAny(originatedChannel.Task, Task.Delay(TimeSpan.FromSeconds(10))); +if (channelResult != originatedChannel.Task) +{ + // Channel did not appear — skip gracefully + return; +} +``` + +`ParkingTests` redirects to the undefined 750 from four sites, so "skip gracefully" is its normal path, +not its safety net. `DtmfDetectionTests:123` and `AriStasisTests:101,:111` carry the same construction. +The scan is a **candidate list and not a verdict**: `BridgeLifecycleTests:315` is a false positive — +a legitimate event filter inside a `ConfbridgeJoinObserver` lambda. Producing the true count by hand is +a task of this change, not a claim of this proposal. + +## What Changes + +**F1 — measure, then decide.** +- A probe, run against both Asterisk images the lane builds, capturing the literal HTTP request line + Asterisk sends for (a) `externalMedia` with `transport=websocket` and (b) the `WebSocket()` dialplan + application, plus the value of `WEBSOCKET_GUID` where one exists. The capture is committed. +- Only then: either the server keys on a measured identifier, or its contract says plainly that it is + not addressable by any Asterisk id and `CompositeAudioServer`'s ambiguity is resolved by one of them + refusing the call. +- The three wrong sentences at `WebSocketAudioServer.cs:34`, `:312`, `:313` are corrected against the + capture, and the dangling "(ADR-1)" at `:35` is resolved or removed. + +**F2 — wire the instruments or remove them.** One decision, recorded, with the public-API cost of the +removal branch priced before it is chosen. Whichever branch is taken, the test file is rewritten so it +fails when the production call sites are removed — a test that supplies its own measurement is not +evidence that anything is instrumented. + +**F3 — define, assert, and sweep.** +- Every extension the suite dials into `[test-functional]` is defined there, or the test that dials it + is changed to dial one that is. +- The unasserting early return is replaced by a failing assertion with a reason, everywhere the sweep + finds it — and the sweep produces a hand-verified count, not the heuristic's 71. +- A guard so the class cannot come back: the suite's dialled set and the dialplan's defined set are + reconciled by something that runs, rather than by a comment in a `.conf` file. + +## Decision: one change, with a written split condition + +These are three findings on three different parts of the tree, and the honest answer is not obvious. +Both readings are set out here, and the change picks one with a condition attached rather than leaving +the choice to whoever opens the file next. + +**For one change.** They are one failure family, and the family is the point. Splitting them into three +tickets produces three fixes and loses the sentence that connects them — which is the sentence a +reviewer needs in order to catch the fourth instance. `openspec/config.yaml` requires the harvest to +land in *an* open change; one change satisfies it and names the family once. And the parent change +already paid for the discovery; re-deriving the shared thesis three times is waste. + +**Against one change.** F1 blocks on a measurement nobody has taken and may end in a breaking API +change; F2 is a shipped-public-API decision; F3 touches no shipped surface at all. Binding two findings +that are ready to fix today behind an open-ended probe is the *tracked separately* failure in a new +costume: a change that cannot start is not much better than prose that nobody links. + +**The decision: one change, ordered F3 → F2 → F1, with this split condition written into `tasks.md`.** +If the F1 probe has not produced a committed capture after one honest attempt against both Asterisk +images the lane builds, F1 splits into its own change — carrying the probe task verbatim — and this +change archives with F2 and F3 complete. The condition is concrete enough to be checked rather than +argued, which is what the parent change's post-mortem asked for. The ordering is deliberate: the two +findings with no unknowns land first, so the unknown cannot hold them. + +## Impact + +- **F1:** `src/Verbara.Sdk.Ari/Audio/WebSocketAudioServer.cs`, `Audio/CompositeAudioServer.cs`, + `Examples/WebSocketMediaExample/`, and the `IAudioServer` / `IAudioStream` contract text in + `src/Verbara.Sdk/IAriClient.cs` that the parent change has just rewritten. A new probe and its + capture. Whether any shipped signature moves is the probe's to decide; if it does, the cost is the + parent change's cost again — `CP0002`, `PublicAPI` rows, a suppressions entry, a migration note. +- **F2:** `src/Verbara.Sdk.Ari/Diagnostics/AudioStreamMetrics.cs` and the sessions that would emit — + `Audio/AudioSocketSession.cs`, `Audio/WebSocketAudioSession.cs`. **Potentially breaking**: the removal + branch drops twelve rows from `src/Verbara.Sdk.Ari/PublicAPI.Shipped.txt` and takes `CP0002`. The + wiring branch is additive and takes none. `Tests/Verbara.Sdk.Ari.Tests/Diagnostics/AudioStreamMetricsTests.cs` + is rewritten either way. +- **F3:** `docker/functional/asterisk-config/extensions.conf` and the functional suite. No shipped + surface. **Expect new red.** The parent change measured that defining 700 makes the originate succeed + and the ConfBridge test bodies then fail; that red belongs to this change and is the reason the + parent renumbered to 710/711 rather than carrying it. Budget for it in the task, rather than + discovering it. +- **CI:** F3's work is invisible on a `pull_request` without the `ci:functional` label (ADR-0051), and + only `merge_group` runs both Asterisk versions. A sixteen-second green functional job means no + Asterisk started. F1's probe depends on the same images. +- **Depends on** `externalmedia-returns-a-channel-id-that-finds-its-stream`: its delta creates the + `external-media-stream-routing` capability and explicitly excludes the WebSocket transport "until its + key has been measured". F1's delta adds to that capability and removes the exclusion, so this change + archives after its parent. diff --git a/openspec/changes/a-published-surface-is-one-something-measures/specs/audio-stream-observability/spec.md b/openspec/changes/a-published-surface-is-one-something-measures/specs/audio-stream-observability/spec.md new file mode 100644 index 00000000..65a82a6c --- /dev/null +++ b/openspec/changes/a-published-surface-is-one-something-measures/specs/audio-stream-observability/spec.md @@ -0,0 +1,68 @@ +# audio-stream-observability Delta + +## ADDED Requirements + +### Requirement: A published instrument SHALL be emitted by the code it names +Every instrument this SDK publishes on a `Meter` SHALL have at least one production call site that +records it, in the component its name describes, and an instrument with no production call site MUST +NOT remain published. Removing it and wiring it are both acceptable answers; leaving it is not, +because a published instrument is read as a statement that the component reports that quantity. + +Where the published documentation tells a reader to watch the meter with a named tool, that +instruction SHALL be true of a running process. A reader who follows it and sees nothing move has +been told something false by a tracked file. + +The choice between wiring and removal SHALL be recorded with the cost of the removal branch stated — +the rows it drops from `PublicAPI.Shipped.txt`, the ApiCompat codes it raises and the migration note +it obliges — so that the cheaper branch is chosen knowingly rather than by default. + +#### Scenario: An instrument nobody records +- **GIVEN** an instrument declared on a published meter +- **WHEN** no source file outside its own declaration and its tests records a measurement to it +- **THEN** it is either recorded by the component it names, or removed from the published surface + +#### Scenario: The documented way to observe the meter +- **GIVEN** documentation instructing a reader to monitor the meter by name +- **WHEN** a process runs the component the instruments describe +- **THEN** the instruments the documentation names move + +### Requirement: A test for an instrument SHALL fail when the production emission is removed +A test cited as evidence that a component is instrumented SHALL drive the component and assert the +measurement the component emitted, and MUST NOT supply the measurement itself. A test that records to +an instrument and then asserts a listener observed that record measures the metrics library, not this +SDK, and SHALL NOT be counted as coverage of the instrument. + +An assertion that an instrument field is non-null SHALL NOT be counted as coverage of anything: a +`static readonly` field initialised at its declaration cannot be null, so the assertion holds in every +possible state of the program, including the state where nothing is instrumented at all. + +#### Scenario: The witness fails when the call site goes +- **GIVEN** a test cited as evidence that a component records an instrument +- **WHEN** the production call site is removed +- **THEN** that test fails + +#### Scenario: A self-supplied measurement is not a witness +- **GIVEN** a test that calls `Add` or `Record` on the instrument itself and asserts a listener saw it +- **WHEN** the instrument has no production call site +- **THEN** the test still passes, and is therefore not evidence for the instrument + +## Architectural Risk + +**Level:** LOW on the wiring branch, MEDIUM on the removal branch. + +**Affected:** `Verbara.Sdk.Ari` — `Diagnostics/AudioStreamMetrics.cs` and the sessions that would +emit, `Audio/AudioSocketSession.cs` and `Audio/WebSocketAudioSession.cs` — plus +`Tests/Verbara.Sdk.Ari.Tests/Diagnostics/AudioStreamMetricsTests.cs`, which is rewritten on either +branch. Nothing downstream consumes these instruments today, for the reason this requirement exists: +they have never emitted. + +Wiring is additive and breaks nothing. Removal drops twelve rows from +`src/Verbara.Sdk.Ari/PublicAPI.Shipped.txt` and raises `CP0002`, which cascades to `Verbara.Sdk.Pro` +and `Verbara.Platform` as a recompile even though no consumer can be using a counter that never +moved. The asymmetry is the reason the decision is a task rather than an assumption. + +**Mitigation:** the rewritten tests are written against the branch chosen, before it lands, and the +negative control is the removal of a production call site rather than the breaking of an expected +value — the distinction the sibling change measured to be the difference between a test that is wired +and a test that can see the defect. If removal is chosen, the break is declared in a committed +suppressions entry with each line read before it is kept, and carries a migration note. diff --git a/openspec/changes/a-published-surface-is-one-something-measures/specs/external-media-stream-routing/spec.md b/openspec/changes/a-published-surface-is-one-something-measures/specs/external-media-stream-routing/spec.md new file mode 100644 index 00000000..0473b072 --- /dev/null +++ b/openspec/changes/a-published-surface-is-one-something-measures/specs/external-media-stream-routing/spec.md @@ -0,0 +1,85 @@ +# external-media-stream-routing Delta + +## ADDED Requirements + +### Requirement: The WebSocket audio path's key SHALL be measured before it is designed +The identifier under which a WebSocket audio stream is registered SHALL be established from bytes +captured off a real Asterisk, and a design for routing that path MUST NOT be derived from a reading +of this repository's own code. The capture SHALL record the literal HTTP request line Asterisk sends, +for each Asterisk feature that can reach this server — the ARI `externalMedia` endpoint with +`transport=websocket`, and the `WebSocket()` dialplan application of `chan_websocket` — because the +server does not distinguish between them today and they need not agree. + +Where Asterisk publishes an identifier for the connection by another route, such as a channel +variable, the capture SHALL record its value alongside the request path, so the choice of key is made +between measured candidates rather than between remembered ones. + +Until that capture exists, the published contract SHALL continue to state that this path is not +addressable by an Asterisk identifier, rather than naming its key a channel id. + +#### Scenario: A capture precedes the design +- **GIVEN** a proposal to route the WebSocket audio path by some identifier +- **WHEN** no capture of what Asterisk puts in the upgrade request exists +- **THEN** the design is not accepted, and the probe is the work that runs instead + +#### Scenario: Two producers are distinguished +- **GIVEN** a server reachable both by `externalMedia` with `transport=websocket` and by the `WebSocket()` dialplan application +- **WHEN** the request path is captured +- **THEN** each producer's path is recorded separately +- **AND** a difference between them is carried into the contract rather than averaged away + +### Requirement: A stream table SHALL NOT be keyed on a value every connection shares +A server that registers streams under a key taken from the connection SHALL take it from something +the connection is free to make unique, and MUST NOT silently drop a registration whose key is already +present. Where the documented way of configuring the far end produces the same key for every call — +a literal path segment in a dialplan line, for instance — that configuration SHALL be corrected in +the same change as the contract, including in this repository's own examples. + +A registration that loses to an existing key SHALL be reported, not discarded, because a server whose +active-stream count disagrees with the number of live connections is reporting a number no consumer +can act on. + +#### Scenario: Two concurrent calls configured the documented way +- **GIVEN** the dialplan line this repository's example publishes, used for two calls at once +- **WHEN** both connect +- **THEN** both streams are addressable, or the second connection is refused with a reason +- **AND** the stream count equals the number of live connections either way + +#### Scenario: The example stops teaching the collision +- **GIVEN** an example whose dialplan line yields one key for every call +- **WHEN** the key's meaning is settled by the capture +- **THEN** the example is corrected in every place it repeats that line, and says what the segment must contain + +### Requirement: An aggregate over servers with different keyspaces SHALL say which server answered +An aggregate that forwards one lookup key to servers that key on different things SHALL NOT report +"no such stream" and "that key belongs to another server's keyspace" as the same answer. It SHALL +either resolve the ambiguity — by routing the key to the server whose keyspace it belongs to — or +expose the per-server result, so a caller can tell a miss from a category error. + +#### Scenario: A key from the wrong keyspace +- **GIVEN** an aggregate over an AudioSocket server and a WebSocket server +- **WHEN** a caller passes an identifier only one of them could ever hold +- **THEN** the caller can distinguish "that stream is not connected" from "that identifier means nothing to the server that would hold it" + +## Architectural Risk + +**Level:** MEDIUM + +**Affected:** `Verbara.Sdk.Ari` (`Audio/WebSocketAudioServer.cs`, `Audio/CompositeAudioServer.cs`), +the `IAudioServer` / `IAudioStream` contract text in `Verbara.Sdk`, `Examples/WebSocketMediaExample`, +and any downstream consumer that reaches a stream through `CompositeAudioServer` — which includes the +`Verbara.Sdk.Pro` AgentAssist path. + +MEDIUM rather than LOW because the outcome is not known before the probe runs. If the measured key +turns out to be something the server must be told rather than something it can read, the fix reaches a +shipped signature and costs what its sibling change cost: `CP0002`, `PublicAPI` rows, a committed +suppressions entry and a migration note. If it turns out the path simply is not addressable, the fix +is contract text and an example, and costs nothing. Pricing that fork before the capture exists is +guessing, and this capability forbids it. + +**Mitigation:** the probe is the first task and its capture is committed beside the change, in the +shape the sibling change's `probe-capture.txt` takes, with the full request recorded rather than +described. The two Asterisk versions the lane builds are both probed, because the merge queue is the +only place both run and it is the wrong place to learn of a difference. The change declares in writing +that it splits if the probe does not land, so the two findings travelling with it are not held behind +an unknown. diff --git a/openspec/changes/a-published-surface-is-one-something-measures/specs/test-determinism/spec.md b/openspec/changes/a-published-surface-is-one-something-measures/specs/test-determinism/spec.md new file mode 100644 index 00000000..46658965 --- /dev/null +++ b/openspec/changes/a-published-surface-is-one-something-measures/specs/test-determinism/spec.md @@ -0,0 +1,79 @@ +# test-determinism Delta + +## ADDED Requirements + +### Requirement: A functional test SHALL fail when the call it originates never happens +A functional test SHALL treat the absence of the channel, event or call it originated as a failure +with a reason, and MUST NOT return from the test body without asserting. An originate that does not +land is the condition the test exists to detect; a test that exits quietly on it reports success for +the one case it was written for. + +This applies to every spelling of the early exit, not to one: a null check on the awaited event, an +emptiness check on a collected event list, a predicate over collected events, and a comparison of +`Task.WhenAny` against the task that was supposed to win are the same construction, and a sweep that +matches only the first finds a fraction of them. The sweep's result SHALL be a hand-verified count, +because the mechanical scan cannot tell a test-body exit from a filter inside an event-observer +lambda, and the lambda form is correct. + +A comment describing the exit as skipping gracefully SHALL NOT stand in for a skip: a skipped test +is reported as skipped, and a test that returns is reported as passed. + +#### Scenario: The originate does not land +- **GIVEN** a functional test that originates a call and waits for the event it asserts on +- **WHEN** the event does not arrive within the test's timeout +- **THEN** the test fails, naming the call it originated and the event it waited for + +#### Scenario: Every spelling of the exit is found +- **GIVEN** a suite in which the unasserting exit is written several different ways +- **WHEN** the suite is swept for it +- **THEN** the sweep covers the shape rather than one phrasing, and its count is verified by reading each site + +### Requirement: The functional dialplan SHALL define every extension the suite dials +Every extension the functional suite dials into a dialplan context SHALL be defined in that context, +or the test that dials it SHALL be changed to dial one that is. The reconciliation SHALL be performed +by something that executes — a guard, a test, or a check on the validation run — and MUST NOT be +recorded only as a comment in a configuration file, because a comment does not fail. + +The set of dialled extensions SHALL be collected from every form the suite uses to reach the +dialplan, including both a channel string naming an extension and a context-plus-extension pair on an +originate or a redirect. A reconciliation derived from one form under-reports, and an under-report +here reads exactly like a clean result. + +A context with no pattern-match extension SHALL NOT be assumed to absorb an undefined number: an +extension that is not listed in such a context is not reachable, and the originate fails. + +#### Scenario: An extension dialled by the suite and absent from the context +- **GIVEN** a functional test dialling an extension into a context that does not define it +- **WHEN** the reconciliation runs +- **THEN** it fails, naming the extension and the test that dials it + +#### Scenario: The second dialling form is counted +- **GIVEN** a suite that reaches the dialplan both by a channel string and by a context-plus-extension pair +- **WHEN** the dialled set is collected +- **THEN** both forms are included, and the reconciliation is over their union + +#### Scenario: Defining the missing extension turns a vacuous pass into a real result +- **GIVEN** tests that have been passing by exiting early because their originate never landed +- **WHEN** the extension is defined and the early exit is replaced by an assertion +- **THEN** those tests run their bodies for the first time, and whatever they then report is the first real result they have ever produced + +## Architectural Risk + +**Level:** LOW on the shipped surface, MEDIUM on the validation run. + +**Affected:** `docker/functional/asterisk-config/extensions.conf` and +`Tests/Verbara.Sdk.FunctionalTests/`. No `src/` file and no published signature changes, so nothing +cascades to `Verbara.Sdk.Pro` or `Verbara.Platform`. + +The risk is not to consumers, it is to the validation run: tests that have been reporting success +without executing their bodies will execute them, and some will fail. That red is pre-existing and is +being surfaced, not caused — the sibling change measured exactly this when it defined one of the +missing extensions, and renumbered its own work to 710/711 specifically so that it would not carry +someone else's failure. This change is where that failure is carried, and the tasks budget for it +rather than meeting it by surprise. + +**Mitigation:** the sweep produces a hand-verified list before any edit, so the size of the red is +known before it is provoked. The work runs behind the `ci:functional` label on the pull request, since +without it the functional job reports success in about sixteen seconds having started no Asterisk, and +the merge queue is the only place both Asterisk versions run. The reconciliation lands as an executed +guard, so the class cannot return as a comment. diff --git a/openspec/changes/a-published-surface-is-one-something-measures/tasks.md b/openspec/changes/a-published-surface-is-one-something-measures/tasks.md new file mode 100644 index 00000000..dcac60f8 --- /dev/null +++ b/openspec/changes/a-published-surface-is-one-something-measures/tasks.md @@ -0,0 +1,203 @@ +# Tasks: a-published-surface-is-one-something-measures + +Three phases. A batched, B one focused subagent per task, C batched. Never inline in the main session. + +**Order is F3 → F2 → F1, deliberately.** F3 and F2 have no unknowns; F1 blocks on a measurement nobody +has taken. The two that can finish are not held behind the one that might not — see the split gate at +the end of Phase A. + +Two findings here are bug fixes, so **the failing assertion is written first, against the unfixed +tree, and its failure is pasted here verbatim.** A task is not checked off until the thing it claims +actually ran. Where a measurement contradicts this file, the correction is written here rather than +quietly absorbed — this change exists because three documents in a row asserted things nobody had +measured. + +## Stale-green traps already measured in this worktree + +Every one of these produced a green number that meant nothing. Respect all six. + +1. The session's default `grep` honours `.gitignore`; tracked files that were force-added are missed. + **Any "every occurrence" claim uses `git grep`.** +2. `mv` preserves mtime, MSBuild then skips the rebuild, and the run reports a green number **from the + previous assembly**. `touch` anything restored, before building. +3. `dotnet pack` prints nothing to a redirected stdout on success — not even with `-tl:off -v m`. A + zero-line log with exit 0 is indistinguishable from a run that did nothing. +4. `PackageValidation` is incremental. Delete the ApiCompat semaphores **and** the nupkgs, or a repeat + pack validates nothing and reports success. Deleting only the semaphores skips packaging entirely. +5. On a `pull_request` the functional job short-circuits without the `ci:functional` label, and only + `merge_group` runs both Asterisk versions. **A sixteen-second functional "pass" started no Asterisk.** +6. An unasserting early return and a real pass are reported identically. That is this change's subject + and it applies to this change's own verification. + +## Phase A — measure, before any edit (batched) + +- [ ] A1 **F3: sweep the functional suite for the unasserting exit, and hand-verify every hit.** A + brace-tracking scan for a bare `return;` in a test-method body reports **71 candidate sites + across 11 files**; it cannot tell a test-body exit from a filter inside an event-observer + lambda, and at least one hit is the latter + (`Tests/Verbara.Sdk.FunctionalTests/Layer5_Integration/Bridge/BridgeLifecycleTests.cs:315`, + inside a `ConfbridgeJoinObserver`, which is correct as written). Read every candidate and + produce a verified list: file, line, which test, and whether the exit is on the normal path or + a genuine filter. **Cover all four spellings**, not the one the hand-over note named — + `x is null`, `collection.IsEmpty`, `!collection.Any(pred)`, and + `await Task.WhenAny(t, Task.Delay(...)) != t`. Paste the verified count and say how it differs + from 71. +- [ ] A2 **F3: reconcile the dialled set against the defined set, from the running container.** + `docker/functional/asterisk-config/extensions.conf` defines nine extensions in + `[test-functional]` — 100, 150, 155, 500, 600, 710, 711, 900, 950 — and that context has **no** + pattern-match extension (the `_X.` catch-alls are in `[default]`, `[stasis-test]` and + `[queue-test]`), so an unlisted number is unreachable. Static analysis of the suite says + seventeen distinct extensions are dialled, in **two** forms that one grep does not both catch: + a `Local/N@test-functional` channel string, and an `Exten = "N"` paired with + `Context = "test-functional"` on an Originate or a Redirect — the second form appears 53 times + and every one of them carries that context. The difference leaves **ten** undefined: + **160, 161, 162, 163, 300, 700, 750, 999, 9998, 9999.** + The hand-over note and the comment already in the dialplan at `:111` both say five; both were + derived from the first form alone. + **Do not stop at the static count.** Bring the container up and settle it with + `dialplan show test-functional`, because a module can register an extension a `.conf` file does + not list — `750` is a parking extension and `res_parking.conf` is in this tree, so it is the + obvious candidate for a number that is dialled, absent from `extensions.conf`, and reachable + anyway. Paste the `dialplan show` output and the final undefined set. +- [ ] A3 **F2: decide wire-or-remove, with the removal branch's cost measured rather than estimated.** + `src/Verbara.Sdk.Ari/Diagnostics/AudioStreamMetrics.cs` declares ten instruments. Confirm with + `git grep` that every reference outside the declaration is in + `Tests/Verbara.Sdk.Ari.Tests/Diagnostics/AudioStreamMetricsTests.cs` (13) or + `src/Verbara.Sdk.Ari/PublicAPI.Shipped.txt` (12) — nine instruments return exactly two hits for + `git grep -n "AudioStreamMetrics\."` and `StreamsClosed` returns three. In every case the + two are the API-tracker row and the test file; the declaration does not match that pattern + because it does not use the qualified name, so there is no call site under `src/` to miss. + Then price removal for real: drop the twelve `PublicAPI.Shipped.txt` rows on a scratch branch + and run `dotnet pack -c Release` with validation **forced** (trap 4), and paste the exact + ApiCompat codes. Compare against wiring — which sessions would emit which instrument, and + whether any of them can reach the data the instrument names. Record the decision and the reason. + Note that `Verbara.Sdk.Ari.Audio` is named in the class doc as something a reader can watch with + `dotnet-counters`; whichever branch wins, that sentence becomes true or goes. +- [ ] A4 **F1: probe what Asterisk actually puts in the WebSocket upgrade request. SPLIT GATE.** + Nothing has ever measured this. The key is computed at + `src/Verbara.Sdk.Ari/Audio/WebSocketAudioServer.cs:341` and registered at `:266`; the + hand-over note said `:337` and `:262` and is wrong on both. + Capture the **literal HTTP request line** for each producer that can reach this server, because + the code does not distinguish between them and they need not agree: + - the ARI `externalMedia` endpoint with `transport=websocket`, which the class summary at `:34` + claims is the producer; + - the `WebSocket()` dialplan application of `chan_websocket`, which + `Examples/WebSocketMediaExample/` and `Audio/IChanWebSocketSession.cs` describe. + Also capture `WEBSOCKET_GUID` where it exists — `Audio/AudioChannelVars.cs` already names it and + the server never reads it, so it is a candidate key that costs nothing to record now and cannot + be recovered later. + Run against **both** Asterisk images the lane builds, exactly as `ci.yml` builds them, + `CODEC_OPUS_VERSION` included — a sibling change measured that omitting it builds a 23 image + carrying the 22 opus argument. Commit the capture beside this change in the shape + `probe-capture.txt` takes for the sibling change: the full request recorded, never described. + **The gate.** If this task has not produced a committed capture after one honest attempt against + both images, F1 splits into its own change carrying A4 and B5 verbatim, this file records the + split with the new change's name, and Phases B and C continue with F3 and F2 alone. State the + outcome either way — "the probe landed" or "the probe did not land and F1 is now ``". + +## Phase B — the fixes (one focused subagent each) + +- [ ] B1 **F3, red first: replace the unasserting exits in `ConfBridgeAdvancedTests` and paste the + failures.** That file dials the undefined `700` from ten call sites across eight `[Fact]`s, and + every one of the eight ends in an unasserting return written three ways: + `if (confJoin is null) return;` at `:55, :104, :162`; + `if (!joinEvents.Any(e => e.Conference == confName)) return;` at `:211, :273`; + `if (joinEvents.IsEmpty) return;` at `:339, :385, :445`. + Replace each with a failing assertion carrying a reason, **against the tree as it stands**, with + 700 still undefined. All eight must go red, and the failures are pasted here verbatim. This is + the red the dialplan comment at `extensions.conf:104-111` predicted and declined to carry; it is + carried here. +- [ ] B2 **F3: define the missing extensions, or change the tests that dial them.** Work from A2's + verified set, not from this file's ten. For each: define it in `[test-functional]` with a + comment naming the tests that dial it — the file's existing entries do this and the convention + is worth keeping — or change the test to dial a defined one, whichever is honest for that test. + `750` is a redirect target, not an originate, and may already be reachable; A2 settles it. + Then **land the reconciliation as something that executes.** A guard, a unit test over the two + file sets, or a check on the validation run — not a comment in a `.conf` file, because the + comment that already documents this hole is the reason the hole is still here. The guard reads + both dialling forms, or it under-reports and an under-report reads exactly like a clean result. +- [ ] B3 **F3: sweep the rest of the suite against A1's verified list.** Every site A1 classed as an + exit on the normal path becomes a failing assertion with a reason; every site it classed as an + observer filter is left exactly as it is, and the task says how many of each. Expect new red + here too: a test that has never run its body has never been checked. Report it as a result, do + not fix it under this task — a pre-existing product failure surfaced by this sweep is a finding + for Phase C's harvest, not scope creep into this one. +- [ ] B4 **F2: implement A3's decision, and rewrite the tests so they can see it.** Whichever branch + won, `Tests/Verbara.Sdk.Ari.Tests/Diagnostics/AudioStreamMetricsTests.cs` is rewritten: today + six of its eleven `[Fact]`s assert only `.Should().NotBeNull()` on a `static readonly` field + initialised at its declaration — true in every possible state of the program — and four supply + their own measurement through a `MeterListener` and assert they observed it. All eleven pass + with zero production call sites, which is the current state. + **The negative control is the removal of a production call site, not the breaking of an expected + value.** Breaking the expected value proves the assertion is wired; only removing the emission + proves the test can see the defect. Paste that failure. + On the removal branch: `*REMOVED*` rows in `PublicAPI.Unshipped.txt` (ADR-0023), a + `CompatibilitySuppressions.xml` entry generated to a scratch path and **read line by line before + it is kept** (ADR-0055), and a migration note (ADR-0028). On the wiring branch: none of that, + and say so explicitly rather than leaving a reader to infer it. +- [ ] B5 **F1: act on A4's capture — and only on it.** If the capture says the request path carries a + usable identifier, key on it and say in the contract where a caller gets it. If it says the path + carries nothing a caller controls, the contract says the path is not addressable by an Asterisk + identifier, and `CompositeAudioServer.GetStream` stops reporting "no such stream" and "wrong + keyspace" as the same `null`. + Either way, four pieces of prose in + `src/Verbara.Sdk.Ari/Audio/WebSocketAudioServer.cs` are corrected against the capture: + `:34` "Listens for incoming WebSocket connections from Asterisk ExternalMedia channels", which + may simply be false; `:312` "extract Sec-WebSocket-Key and channel ID from URL path"; `:313` + "Expected URL: `/ws/{channelId}` or `/{channelId}`", which names the placeholder `{channelId}`; + and `:35`'s dangling "(ADR-1)", which cites nothing — this repository's ADRs are four digits and + `ADR-0001` is *Native AOT first*, not a TcpListener decision. Resolve it to the real ADR or + delete it. + And correct the example, in **all three** places it repeats the collision-producing dialplan + line: `Examples/WebSocketMediaExample/Program.cs:10`, `:78`, and + `Examples/WebSocketMediaExample/README.md:17`. A literal `/audio` there gives every concurrent + call the key `"audio"`. + +## Phase C — integration and verification (batched) + +- [ ] C1 `CHANGELOG.md [Unreleased]`. F3 takes a `### Fixed` entry; F2 takes `### Fixed` on the wiring + branch and `### Removed — BREAKING` on the removal branch; F1 takes whatever A4's capture makes + true. Give this change its **own insertion anchor**, distinct from every other in-flight entry — + a shared anchor ejected a sibling PR from the merge queue with fourteen checks green. Leave + `(#N)` for close-out. +- [ ] C2 Apply the **`ci:functional`** label to the PR and record the **PR-time** result, not only the + queue's. Without it the functional job reports success in about sixteen seconds having started + no Asterisk (ADR-0051), and the matrix is `[23]` on a PR against `[22, 23]` in the queue. This + change edits the functional dialplan and the functional suite; an unlabelled green here would be + this change's own subject, committed. +- [ ] C3 Coverage measured **after committing**, never before. The unit lane excludes + `Category=Functional`, so all of F3 contributes **zero** patch coverage against the 85% floor — + F2's rewritten unit tests are what must carry it, and B5's contract edits carry none either. + Read the changed-line count and check it against the size of the diff before believing the + percentage: a sibling change accepted `100% (6/6)` on a five-file diff and the `6` was the tell. +- [ ] C4 `dotnet build` and the **full unit lane** green with zero warnings + (`TreatWarningsAsErrors`, `WarningLevel 9999`), Governance green, and `dotnet pack` clean with + validation **proven** to have run — state how it was forced (trap 4), because a green pack that + validated nothing is this repository's most expensive lie. Derive the rest of the job list from + `.github/workflows/ci.yml` rather than from memory; "the tests of the projects I touched" misses + the tree-scanning guards, and F3 edits a file the sync-fence guard scans. +- [ ] C5 `openspec validate --all --strict` green; paste the totals line. CI green on the PR + **including the `merge_group` build** — the only place the functional suite runs both Asterisk + versions, and the place a dialplan change is most likely to differ. +- [ ] C6 Harvest. Anything B3 surfaced as a pre-existing product failure, anything A4's capture + measured and this change did not act on, and F1 itself if the split gate fired — each goes into + an open change or an ADR addendum **with a link**, before this change archives. + `openspec/config.yaml` requires it, and *tracked separately* with no link is the exact label that + produced this change and its parent. Write the name of every change opened here, in this task. + +## Findings this change corrects in the notes that handed it over + +Recorded so the next reader trusts the measurement rather than the prose. + +- **F1's line numbers.** The key is computed at `WebSocketAudioServer.cs:341` and registered at + `:266` — not `:337` and `:262`. +- **F1's example.** The collision-producing dialplan line appears in three places, not one. +- **F1 has a fourth and fifth piece of wrong prose**, beyond the `{channelId}` placeholder the + hand-over note named: the class summary at `:34` asserting the ExternalMedia producer, and the + dangling "(ADR-1)" at `:35`. +- **F3's count is ten, not five** — 160, 161, 162, 163, 300, 700, 750, 999, 9998, 9999 — because the + suite reaches the dialplan by two forms and the five came from one of them. +- **F3's early return has three spellings in one file and four across the suite**, and + `ConfBridgeAdvancedTests` has eight affected tests, not five. The dialplan comment at + `extensions.conf:104-111` states both wrong numbers, and ends "the hole is filed separately" with no + link — the ADR-0060 failure reproduced inside the commit that was fixing it. diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/.openspec.yaml b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/.openspec.yaml new file mode 100644 index 00000000..75289e4b --- /dev/null +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-24 diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-capture.txt b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-capture.txt new file mode 100644 index 00000000..01d686ac --- /dev/null +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-capture.txt @@ -0,0 +1,159 @@ +ARI externalMedia / AudioSocket identification probe +==================================================== +Asterisk 22.9.0 (image verbara/asterisk-local:22, --network host), real ARI over HTTP :8088, +bare TCP listener on 127.0.0.1:19099 recording raw bytes. The listener is dependency-free on +purpose: no code from this repository decodes anything below. + +Reproduce with probe-externalmedia.py beside this file. It prints the FULL query string of every +request, which this capture did not do in its first version — and that omission is what let a wrong +conclusion survive review. See CORRECTIONS at the end. + +Every run uses UUIDs that are asymmetric under byte reversal, so a capture says BOTH which parameter +travels AND in which byte order. A byte-order-symmetric fixture would have passed either way; that +mistake was made once already in this repository's history. + +-------------------------------------------------------------------------------- +PART 1 — what the SDK sends today +-------------------------------------------------------------------------------- + +RUN G — ExternalMediaActivity with Encapsulation = "audiosocket", Transport unset. + This is what the class actually puts on the wire for an AudioSocket caller. + QUERY: app=probe&external_host=127.0.0.1%3A19099&format=slin16&encapsulation=audiosocket + HTTP 400 -> "transport must be 'tcp' for audiosocket encapsulation" + +RUN H — ExternalMediaActivity with its DEFAULTS (Encapsulation unset => rtp/udp). + QUERY: app=probe&external_host=127.0.0.1%3A19099&format=slin16 + HTTP 200 id=1790245644.0 name=UnicastRTP/127.0.0.1:19099-0x7f6ce8002100 + No AudioSocket connection is ever made. An AudioSocketServer handed to this activity waits out + ConnectionTimeout and the activity throws TimeoutException — which is exactly what ADR-0060 + predicted, for the class's default configuration. + +RUN D — audiosocket + transport=tcp, no data. + QUERY: app=probe&external_host=127.0.0.1%3A19099&format=slin&encapsulation=audiosocket&transport=tcp + HTTP 400 -> "data can not be empty" + Reached only once transport=tcp is supplied. The activity does not supply it today. + +-------------------------------------------------------------------------------- +PART 2 — which parameter reaches the wire +-------------------------------------------------------------------------------- + +RUN A — channelId and data DISTINCT, listener up. + channelId = 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 + data = f9e8d7c6-b5a4-3928-1706-f5e4d3c2b1a0 + HTTP 200 + channel.id : 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 <- channelId + channel.name : AudioSocket/127.0.0.1:19099-f9e8d7c6-... <- data + HEX: 01 00 10 f9 e8 d7 c6 b5 a4 39 28 17 06 f5 e4 d3 c2 b1 a0 + UUID on the wire (big-endian, RFC 4122): f9e8d7c6-b5a4-3928-1706-f5e4d3c2b1a0 + == channelId ? False == data ? True + +RUN F — RUN A repeated with fresh ids. Same result: the wire carries data. + +RUN C — data only, no channelId, listener up. + HTTP 200 channel.id : 1790244226.1 <- Asterisk-minted uniqueid, unrelated to the wire + HEX: 01 00 10 f9 e8 d7 c6 b5 a4 39 28 17 06 f5 e4 d3 c2 b1 a0 == data ? True + Note what this run proves: the stream is findable TODAY by the value the caller passed as data, + with no API change at all. Choosing channelId over that is a deliberate cost, recorded in + proposal.md under "Rejected alternative". + +-------------------------------------------------------------------------------- +PART 3 — the pattern, and what a 500 actually means +-------------------------------------------------------------------------------- + +channelId == data, fresh uuid each run, crossed against format and against whether anything is +listening on the target port: + + format=slin16 listener UP -> HTTP 200 id=cd98c592-9ce2-4093-afae-420d53629a4f + HEX: 01 00 10 cd 98 c5 92 9c e2 40 93 af ae 42 0d 53 62 9a 4f + UUID on the wire == channelId ? True + format=slin16 listener DOWN -> HTTP 500 "An internal error prevented this request..." + format=slin listener UP -> HTTP 200 id=5433739b-2d3b-4e8a-97f2-3c57e0981278 + UUID on the wire == channelId ? True + format=slin listener DOWN -> HTTP 500 + +HTTP 500 means NOTHING IS LISTENING on external_host. It is not about the format and not about the +channel id. chan_audiosocket connects during the create call, so a create against a dead port fails +the request rather than producing a channel that never connects. + +-------------------------------------------------------------------------------- +PART 4 — the CI lane's own image, Asterisk 22 AND 23 (task A1, 2026-09-24) +-------------------------------------------------------------------------------- + +Images built exactly as .github/workflows/ci.yml:387 builds them: + + docker build -f docker/Dockerfile.asterisk docker/ \ + --build-arg ASTERISK_VERSION=<22|23> --build-arg CODEC_OPUS_VERSION=<22|23>.0_1.3.0 \ + -t verbara-probe:<22|23> + + verbara/asterisk-local:22 -> /ari/asterisk/info system.version = 22.9.0 + verbara-probe:22 -> 22.9.0 (andrius/asterisk:22, build date 2026-05-04) + verbara-probe:23 -> 23.4.1 (andrius/asterisk:23, build date 2026-09-23) + +Asterisk 23 had never been probed. It behaves IDENTICALLY to 22 on every run in this file: +same HTTP status, same error strings verbatim, same parameter on the wire, same byte order, +same 500 on a dead target port. Three full passes (local 22, lane 22, lane 23 twice) agree. + + RUN G 400 "transport must be 'tcp' for audiosocket encapsulation" 22 == 23 + RUN H 200 UnicastRTP/... 22 == 23 + RUN D 400 "data can not be empty" 22 == 23 + RUN A 200 id == channelId, wire UUID == data 22 == 23 + RUN C 200 id == Asterisk uniqueid, wire UUID == data 22 == 23 + PART 3 200 / 500 crossed over format x listener 22 == 23 + +Two runs beyond what the shipped probe does, both measured on 22 and on 23 with identical results: + + RUN H+ RUN H with the TCP listener UP for 4 s. Nothing connects — the sentence in PART 1 was + an inference from the channel name until now, because the shipped probe runs H with the + listener down. + -> HTTP 200 name=UnicastRTP/127.0.0.1:19099-0x7f0720006350 + HEX: (nothing connected) + + RUN U channelId == data == an UPPERCASE canonical UUID. Asterisk ACCEPTS it: HTTP 200, and + Channel.Id comes back in the spelling that was sent. The wire carries the same 16 bytes, + which AudioSocketSession.ParseUuid renders with Guid.ToString() — canonical LOWERCASE — + into an ordinal ConcurrentDictionary. So a non-canonical spelling creates a + channel whose stream cannot be found, silently, exactly as the delta spec forbids: + QUERY: ...&channelId=EC30994A-B0E1-4EBE-99BD-19CA35669F7A&data=EC30994A-...-19CA35669F7A + -> HTTP 200 id=EC30994A-B0E1-4EBE-99BD-19CA35669F7A + HEX: 01 00 10 ec 30 99 4a b0 e1 4e be 99 bd 19 ca 35 66 9f 7a + wire = ec30994a-b0e1-4ebe-99bd-19ca35669f7a == sent ? False == sent.lower() ? True + This is the measurement behind B2's "canonical lowercase is not incidental". + +-------------------------------------------------------------------------------- +CORRECTIONS to the first version of this file +-------------------------------------------------------------------------------- + +1. RUN D was labelled "exactly what ExternalMediaActivity sends today". It was not: it carried + transport=tcp, which the activity sends only when a caller sets Transport. The activity's real + request is RUN G, and its error is a different one. The capture recorded parameters in prose and + not the query string, which is why the mislabel survived. Every run above now carries its QUERY. + +2. The first version explained an HTTP 500 as "reuse of a channelId already created earlier in the + same session". That is wrong. The crossed runs above isolate it: the 500 is a dead target port. + A wrong "not a finding" note is worse than no note, because it sends the next reader away from + the cause. + +3. Neither error code predicted during review was right. A reading of res/ari/resource_channels.c + predicted 501 "encapsulation and/or transport is not supported" for RUN G; 22.9.0 returns 400 + with a specific message instead. Measured beats read, again. + +4. Correction #2 above is itself incomplete. "HTTP 500 means NOTHING IS LISTENING on external_host" + names one cause, not the cause. With the listener UP, a `data` value that is not a parseable + UUID returns the same 500 and nothing ever connects (measured on 22 and on 23): + channelId CANONICAL, data = {eba9adc2-...} -> HTTP 500, nothing connected + channelId CANONICAL, data = not-a-uuid-at-all -> HTTP 500, nothing connected + channelId = {eef6aae9-...}, data CANONICAL -> HTTP 200, id={eef6aae9-...}, wire == data + So: `channelId` is a free-form ARI id and is echoed back verbatim — braces and all — while + `data` must parse as a UUID. A 500 from externalMedia means "chan_audiosocket could not set the + connection up", which covers a dead port AND a malformed `data`. Saying it means a dead port + sends the next reader away from the second cause, which is the same mistake correction #2 was + written to fix. + +-------------------------------------------------------------------------------- +NOT YET MEASURED +-------------------------------------------------------------------------------- +- Asterisk 23 is now measured — see PART 4. What remains unmeasured on 23 is everything this probe + does not touch, not the runs above. +- The WebSocket transport. Nothing here measures what externalMedia with transport=websocket puts + in the request path, which is why F1 stays out of this change rather than being designed from a + reading of the code. diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-externalmedia.py b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-externalmedia.py new file mode 100644 index 00000000..c8db90c9 --- /dev/null +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/probe-externalmedia.py @@ -0,0 +1,143 @@ +#!/usr/bin/env python3 +"""Measures what Asterisk puts on an AudioSocket wire for POST /channels/externalMedia. + +Dependency-free on the SDK on purpose: nothing in this repository decodes a byte here. A parser +checked against its own encoder agrees with itself and says nothing about the wire — that closed +loop is what ADR-0060 exists to break, and this probe is the other end of it. + +Run it against a container started as: + + docker run -d --rm --name as-probe --network host \ + -v "$PWD/docker/functional/asterisk-config:/etc/asterisk:ro" verbara/asterisk-local:22 + +Then: python3 probe-externalmedia.py (needs the `websocket-client` package) + +Every run prints its FULL query string. The first version of probe-capture.txt recorded parameters +in prose instead, and a run labelled "what the SDK sends" turned out to carry an extra parameter +nobody could see. Print the request or the capture cannot be audited. +""" +import base64, json, socket, threading, time, urllib.error, urllib.parse, urllib.request, uuid + +import websocket # websocket-client + +ARI = "http://127.0.0.1:8088/ari" +AUTH = base64.b64encode(b"testari:testari").decode() +PORT = 19099 + + +def _listen(box, seconds): + s = socket.socket() + s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + s.bind(("0.0.0.0", PORT)) + s.listen(4) + s.settimeout(seconds) + box["bytes"] = b"" + try: + conn, _ = s.accept() + except socket.timeout: + s.close() + return + conn.settimeout(1.0) + deadline = time.time() + seconds + while time.time() < deadline: + try: + chunk = conn.recv(65536) + except socket.timeout: + continue + if not chunk: + break + box["bytes"] += chunk + # The identification frame is 3 header bytes + 16 UUID bytes. Stop there, before audio + # floods the capture. + if len(box["bytes"]) >= 19 and box["bytes"][0] == 0x01: + break + conn.close() + s.close() + + +def run(label, params, listener=True, seconds=4.0): + box = {} + thread = None + if listener: + thread = threading.Thread(target=_listen, args=(box, seconds)) + thread.start() + time.sleep(0.3) + + query = urllib.parse.urlencode(params) + request = urllib.request.Request(f"{ARI}/channels/externalMedia?{query}", method="POST") + request.add_header("Authorization", "Basic " + AUTH) + try: + with urllib.request.urlopen(request, timeout=10) as response: + body = json.loads(response.read().decode()) + outcome = f"HTTP {response.status} id={body.get('id')} name={body.get('name')}" + except urllib.error.HTTPError as error: + outcome = f"HTTP {error.code} {json.loads(error.read().decode()).get('message')}" + + if thread: + thread.join() + + print(f"\n{label}") + print(f" QUERY : {query}") + print(f" LISTENER: {'up' if listener else 'down'}") + print(f" -> {outcome}") + + captured = box.get("bytes", b"") + if captured: + head = captured[:19] + print(f" HEX : {' '.join(f'{b:02x}' for b in head)}") + if len(head) >= 19 and head[0] == 0x01: + p = head[3:19] + wire = "%s-%s-%s-%s-%s" % ( + p[0:4].hex(), p[4:6].hex(), p[6:8].hex(), p[8:10].hex(), p[10:16].hex()) + print(f" UUID : {wire}") + print(f" == channelId ? {wire == params.get('channelId')}" + f" == data ? {wire == params.get('data')}") + return box.get("bytes", b"") + + +def main(): + # externalMedia never checks that `app` is registered, but the channel it creates runs Stasis + # afterwards: with no subscriber Asterisk hangs it up and the session disappears in + # milliseconds. Subscribe first, or a run measures a teardown race instead of a handshake. + events = websocket.WebSocket() + events.connect( + f"ws://127.0.0.1:8088/ari/events?api_key=testari:testari&app=probe&subscribeAll=true", + timeout=10) + + host = {"app": "probe", "external_host": f"127.0.0.1:{PORT}"} + audiosocket = {**host, "format": "slin16", "encapsulation": "audiosocket", "transport": "tcp"} + try: + print("=" * 78) + print("PART 1 — what the SDK sends today") + run("RUN G — Encapsulation='audiosocket', Transport unset (what the activity sends)", + {**host, "format": "slin16", "encapsulation": "audiosocket"}, listener=False) + run("RUN H — the activity's defaults (no encapsulation => rtp/udp)", + {**host, "format": "slin16"}, listener=False) + run("RUN D — audiosocket + tcp, no data", + {**host, "format": "slin", "encapsulation": "audiosocket", "transport": "tcp"}, + listener=False) + + print("\n" + "=" * 78) + print("PART 2 — which parameter reaches the wire") + run("RUN A — channelId and data DISTINCT", + {**audiosocket, "channelId": str(uuid.uuid4()), "data": str(uuid.uuid4())}) + time.sleep(1) + run("RUN C — data only, no channelId", + {**audiosocket, "data": str(uuid.uuid4())}) + + print("\n" + "=" * 78) + print("PART 3 — the pattern, crossed against format and against a live target") + for fmt in ("slin16", "slin"): + for listening in (True, False): + identifier = str(uuid.uuid4()) + time.sleep(1) + run(f"channelId == data, format={fmt}", + {**audiosocket, "format": fmt, "channelId": identifier, "data": identifier}, + listener=listening) + finally: + events.close() + print("\n" + "=" * 78) + + +if __name__ == "__main__": + main() diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/proposal.md b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/proposal.md new file mode 100644 index 00000000..96fba011 --- /dev/null +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/proposal.md @@ -0,0 +1,126 @@ +--- +tier: MEDIANO +owner: Harol +approver: Harol +stakeholder: Anyone using ExternalMediaActivity with an AudioSocket server, and the Pro AgentAssist deployment that pairs the two +decision_ref: Sdk/ADR-0060 +--- + +# Proposal: externalmedia-returns-a-channel-id-that-finds-its-stream + +## Why + +ADR-0060 closed the AudioSocket wire format and said, in as many words, that doing so **does not** +make the ARI route work end to end: the stream table is keyed by the wire UUID while its only +consumer looks the stream up by ARI channel id, and that this was tracked separately. Nothing tracked +it. This is that tracking, and the harvest the #302 close-out owed and did not deliver. + +Everything below is measured against a real Asterisk 22.9.0 with a dependency-free TCP listener. +The probe is `probe-externalmedia.py` beside this file and its output is `probe-capture.txt`; every +run records the full query string it sent, because an earlier version of that capture recorded +parameters in prose and a mislabelled run survived into the first draft of this proposal. + +### Three defects, not two + +**1. The default configuration creates an RTP channel and waits.** `ExternalMediaActivity` accepts an +`AudioSocketServer` in its constructor but leaves `Encapsulation` null, which Asterisk reads as +rtp/udp: + +```text +RUN H QUERY: app=probe&external_host=…&format=slin16 + HTTP 200 name=UnicastRTP/127.0.0.1:19099-0x7f6ce8002100 +``` + +Nothing ever connects to the AudioSocket server, and after `ConnectionTimeout` the activity throws +`TimeoutException`. That is precisely what ADR-0060 predicted, for the shape callers actually get. + +**2. Asking for AudioSocket fails at the create call.** Setting `Encapsulation = "audiosocket"` is not +enough, because the activity sends `transport` only when a caller sets `Transport`: + +```text +RUN G QUERY: app=probe&external_host=…&format=slin16&encapsulation=audiosocket + HTTP 400 -> "transport must be 'tcp' for audiosocket encapsulation" +``` + +And once transport is supplied, `data` is mandatory too: + +```text +RUN D QUERY: …&encapsulation=audiosocket&transport=tcp + HTTP 400 -> "data can not be empty" +``` + +So no configuration of today's activity reaches a working AudioSocket stream: one path times out, +the other two throw at the create call inside `EnsureAriSuccessAsync`. + +**3. The two identifiers are different parameters.** The probe passed distinct, byte-order-asymmetric +UUIDs in `channelId` and `data`, so the capture says both which one travels and in which byte order: + +```text +channelId = 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 +data = f9e8d7c6-b5a4-3928-1706-f5e4d3c2b1a0 + +channel.id : 0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9 +HEX : 01 00 10 f9 e8 d7 c6 b5 a4 39 28 17 06 f5 e4 d3 c2 b1 a0 +UUID on the wire == channelId ? False == data ? True +``` + +`data` becomes the identification UUID the server keys its table by. `channelId` becomes the ARI +`Channel.Id`. Supplying one value as both makes them equal — HTTP 200, and the lookup hits. +Reproduced across fresh UUIDs and both audio formats. + +### Rejected alternative: mint the UUID and pass it only as `data` + +RUN C shows the stream is findable **today**, with no API change: the activity mints an identifier, +passes it as `data`, and polls `GetStream(thatIdentifier)`. No `CP0002`, no suppression file, no +migration guide, nothing stops compiling. + +It is rejected because it fixes the activity and leaves the capability unfixed. A consumer holding +only an ARI channel id — a `StasisStart` or `ChannelHangupRequest` handler, which is the shape Pro's +AgentAssist uses — still cannot find the stream, because `Channel.Id` remains an Asterisk-minted +uniqueid that appears in no table. Making `Channel.Id` **be** the key is the whole point, and it is +what justifies the break priced below. This is a deliberate purchase, not an oversight. + +### Why no test caught it + +All four constructions of `ExternalMediaActivity` in the suite use the one-argument overload, leaving +both server fields null. The polling loop does run — two tests assert on what it produces — but **the +two `GetStream` branches are never entered**. That is the same shape as the wire-format defect one +level up: a green suite that never reaches the code it appears to cover. + +## What Changes + +- `CreateExternalMediaAsync` gains a `channelId` parameter on `IAriChannelsResource` and + `AriChannelsResource`. Asterisk spells it camelCase, unlike every snake_case sibling, and a typo is + silently ignored rather than rejected — so the URL test asserts the literal. +- `ExternalMediaActivity` derives the request from the server it was handed: AudioSocket encapsulation + implies `transport = "tcp"`, and one freshly minted UUID goes to both `channelId` and `data`, in + canonical lowercase form because that is what the server's table is keyed by. +- A functional test originates a real `externalMedia` against a real Asterisk and asserts the stream + is found by `Channel.Id`. +- The `IAudioServer.GetStream` contract says what its key is and in what form, instead of "channel + ID" — the wording that invited the confusion — including that `WebSocketAudioServer` keys on + something else entirely. + +## Impact + +- **Public API, BREAKING.** The nine-parameter signature is shipped in two packages + (`src/Verbara.Sdk/PublicAPI.Shipped.txt:499`, `src/Verbara.Sdk.Ari/PublicAPI.Shipped.txt:449`). + Optional parameters are compile-time sugar and ApiCompat matches by full signature, so the old + member vanishes: `CP0002` in both assemblies plus `CP0006` on the interface. `src/Verbara.Sdk` has + no `CompatibilitySuppressions.xml` today and gains one, in the ADR-0055 shape — generated + deliberately, every entry read before it is kept, never left to a build-machine flag that writes a + file the repository never sees. `PublicAPI.Unshipped.txt` takes a `*REMOVED*` row plus the new row + in both packages, per ADR-0023. +- **A migration note is obligatory** (ADR-0028, for a minor carrying a break), and the CHANGELOG entry + is `### Fixed — BREAKING`. +- **Two test doubles stop compiling, loudly rather than silently.** Both NSubstitute setups enumerate + nine positional `Arg.Any` ending in a `CancellationToken`; inserting a parameter before it lands the + token in a `string?` slot. They move to named arguments. +- **External implementers of `IAriChannelsResource` break** — Pro's fakes and decorators. That is the + price of the alternative rejected above, and it is named rather than discovered. +- **Behavioural, for AudioSocket callers only.** RTP callers take an unchanged path. +- **Out of scope, and stated rather than implied:** `WebSocketAudioServer` keys its table by the last + segment of the request URL, a second and different mismatch that no probe has yet measured; and + `AudioStreamMetrics` declares ten instruments with no production call site. Both are carried in + `tasks.md` with file and line, and C7 opens a change for them rather than leaving them as + checkboxes inside this one. diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/specs/external-media-stream-routing/spec.md b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/specs/external-media-stream-routing/spec.md new file mode 100644 index 00000000..2c9ed5ea --- /dev/null +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/specs/external-media-stream-routing/spec.md @@ -0,0 +1,99 @@ +# external-media-stream-routing Delta + +## ADDED Requirements + +### Requirement: An AudioSocket external media channel SHALL be findable by the channel id it returned +Where the transport identifies its connection with a caller-supplied value — AudioSocket does, in its +identification frame — the SDK SHALL make the ARI channel id and that value the same value, by +supplying one identifier to both parameters that carry them. + +The SDK SHALL NOT leave the ARI channel id to be minted by Asterisk while keying the stream table on +a different value, because no lookup can succeed across those two namespaces. The identifier SHALL be +in the canonical lowercase hyphenated form, which is the form the server's table is keyed by; an +identifier in any other spelling produces a successful create and a lookup that misses. + +The WebSocket transport is **not** covered by this capability until its key has been measured rather +than read from the code. + +#### Scenario: A stream is found by the channel id the create call returned +- **GIVEN** an AudioSocket server owned by the caller and running +- **WHEN** the caller creates an external media channel with AudioSocket encapsulation pointing at it +- **AND** Asterisk connects and sends its identification frame +- **THEN** looking the stream up by the returned `Channel.Id` SHALL return that stream +- **AND** the identification frame's UUID SHALL equal that same `Channel.Id` + +#### Scenario: An identifier in a non-canonical spelling does not silently miss +- **GIVEN** an identifier that is a valid UUID but not in canonical lowercase hyphenated form +- **WHEN** it is used to create an AudioSocket external media channel +- **THEN** the SDK SHALL either normalise it to the form the stream table is keyed by, or reject it +- **AND** SHALL NOT produce a created channel whose stream cannot be found + +### Requirement: A request that Asterisk cannot route SHALL fail at the create call +The SDK SHALL supply the transport that the requested encapsulation requires, rather than leaving a +combination Asterisk rejects. Where the caller has supplied an audio server whose transport +contradicts the requested encapsulation, the SDK SHALL fail before creating a channel that can never +carry a stream. + +A failure to create SHALL surface as the error Asterisk returned, at the create call, and SHALL NOT +be reported later as the audio server having failed to connect. + +#### Scenario: AudioSocket encapsulation carries its required transport +- **GIVEN** a caller that asks for AudioSocket encapsulation and specifies no transport +- **WHEN** the external media channel is created +- **THEN** the SDK SHALL send the transport AudioSocket requires +- **AND** the create SHALL NOT fail for want of a transport the caller was never asked for + +#### Scenario: An audio server that cannot be reached by the requested encapsulation is refused +- **GIVEN** an AudioSocket server supplied to an activity configured for a non-AudioSocket encapsulation +- **WHEN** the activity starts +- **THEN** it SHALL fail with an error naming the contradiction +- **AND** SHALL NOT wait out its connection timeout and report a connection failure + +### Requirement: The ARI external media surface SHALL expose the parameter that names the channel +The SDK's external media create method SHALL accept the ARI `channelId` parameter, which Asterisk +documents as the unique id to assign the channel on creation. Without it a caller cannot choose the +channel id, and therefore cannot make it match anything. + +Asterisk spells this parameter in camelCase while its siblings are snake_case, and ignores an +unrecognised parameter silently rather than rejecting it. The SDK's own test for the request SHALL +assert the literal parameter name, so a misspelling fails a test rather than becoming a silent no-op. + +#### Scenario: A caller chooses the channel id +- **GIVEN** a caller that has generated an identifier +- **WHEN** it creates an external media channel supplying that identifier as the channel id +- **THEN** the returned channel's id SHALL be that identifier + +### Requirement: A stream lookup contract SHALL say which identifier it takes and in what form +Any published contract for looking a stream up by key SHALL state what that key is, in what form, and +where a caller obtains it — rather than naming it only as a channel id. Where two implementations of +the same contract key on different values, the contract SHALL say so. + +#### Scenario: A reader can tell what to pass +- **GIVEN** the published documentation of a stream lookup +- **WHEN** a reader holds an ARI channel id and wants the stream for it +- **THEN** the documentation SHALL state whether that value is a valid key for that implementation +- **AND** SHALL state the form the key takes + +## Architectural Risk + +**Level:** MEDIUM + +**Affected:** `Verbara.Sdk` (the `IAriChannelsResource` interface — a signature change on a shipped +public interface, so external implementers break), `Verbara.Sdk.Ari` (the resource class and the +AudioSocket server's documented contract), `Verbara.Sdk.Activities` (`ExternalMediaActivity`), and +downstream `Verbara.Sdk.Pro` AgentAssist, which pairs that activity with an AudioSocket server and is +the consumer the route exists for. + +The level is MEDIUM rather than LOW because the change alters a shipped signature in two packages: +`CP0002` in both plus `CP0006` on the interface, and any external implementation of +`IAriChannelsResource` stops compiling. The smaller fix that avoids all of this — mint the identifier +and pass it only as `data` — was considered and rejected in the proposal, because it leaves a +consumer holding only an ARI channel id unable to find the stream, which is the capability itself. + +**Mitigation:** the break is declared in a committed `CompatibilitySuppressions.xml` with each entry +read before it is kept, `*REMOVED*` plus new rows in `PublicAPI.Unshipped.txt`, a migration note per +ADR-0028 and a `### Fixed — BREAKING` CHANGELOG entry. The behavioural change reaches only callers +that ask for AudioSocket encapsulation. The end-to-end claim is pinned by a functional test against a +real Asterisk with a subscribed Stasis application, and that test is negatively controlled by +reverting the fix rather than by breaking the assertion — because a mutation that only proves the +assertion is wired is what let a fixture measure nothing for six months. diff --git a/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md new file mode 100644 index 00000000..dcaa650c --- /dev/null +++ b/openspec/changes/externalmedia-returns-a-channel-id-that-finds-its-stream/tasks.md @@ -0,0 +1,589 @@ +# Tasks: externalmedia-returns-a-channel-id-that-finds-its-stream + +Three phases. A batched, B one focused subagent per task, C batched. Never inline in the main session. + +This is a bug fix, so **the failing regression test is written first, against the unfixed code, and its +failure is pasted here verbatim.** A task is not checked off until the thing it claims actually ran. + +## Phase A — foundation (batched) + +- [x] A1 Run `probe-externalmedia.py` and diff its output against `probe-capture.txt`. It prints the + full query string of every request; an earlier capture recorded parameters in prose and a run + labelled "what the SDK sends" turned out to carry an extra one nobody could see. Then run it + against **the lane's own image, for both versions**, not only the local one: + `docker build -f docker/Dockerfile.asterisk docker/ --build-arg ASTERISK_VERSION=23`. + Asterisk 23 has never been probed, and the merge queue would otherwise be the first place the + new test meets it. Paste both outputs. + + **Done 2026-09-24.** Three builds probed: `verbara/asterisk-local:22` (22.9.0), the lane's own + `verbara-probe:22` (22.9.0) and the lane's own `verbara-probe:23` (**23.4.1, never probed + before**). Images built exactly as `ci.yml:387` builds them, `CODEC_OPUS_VERSION` included — + omitting it would have built a 23 image carrying the 22 opus argument. + + **22 and 23 behave identically on every run**: same status, same error strings verbatim, same + parameter on the wire, same byte order, same 500 on a dead port. There is no version difference + to carry into C1. + + Three things the earlier capture asserted without measuring, now measured on both versions: + + - **RUN H's "no AudioSocket connection is ever made"** was inferred from the channel name, + because the shipped probe runs H with the listener down. Run with the listener up: HTTP 200, + UnicastRTP, nothing connects. The sentence was right and had not been earned. + - **The non-canonical identifier miss is real.** An UPPERCASE UUID returns HTTP 200 with + `Channel.Id` in the spelling sent, while the wire bytes render through `ParseUuid` as + canonical lowercase into an ordinal dictionary. B2's negative control has a fixture now + instead of an argument. + - **Correction #2 of the capture was itself incomplete — the third time this file has been + wrong.** "HTTP 500 means nothing is listening" names one cause. A malformed `data` returns the + same 500 with the listener **up**: `channelId` is free-form and echoed verbatim, `data` must + parse as a UUID. Recorded as correction #4. + + **This bears on C2.** Its third reversion — "pass `channelId` and `data` different values" — + must use two **well-formed** UUIDs, or the reversion fails at the create with a 500 instead of + at the lookup, and proves nothing. + + Stated plainly rather than dressed up: `probe-capture.txt` is **not** reproducible verbatim by + `probe-externalmedia.py` and never could be. The capture is a narrative of an earlier hand-run + with fixed UUIDs; the script mints uuid4 per run. A1's verdict is claim-by-claim, not a text + diff. +- [x] A2 Read the surface that changes: `AriChannelsResource.CreateExternalMediaAsync`, + `IAriChannelsResource`, `ExternalMediaActivity`, `AudioSocketServer.GetStream`, + `CompositeAudioServer.GetStream`, and the XML docs on `IAudioServer` / `IAudioStream` in + `src/Verbara.Sdk/IAriClient.cs`. Record the exact current signatures and doc text, so the diff + is against what is there rather than what is remembered. + + **Done 2026-09-24.** Full transcription with file:line is in the workflow record; the decisions + B1 depends on are below, each measured rather than reasoned. + + **RS0016 is globally suppressed, so the PublicAPI tracker does NOT catch a new public API here.** + `Directory.Build.props:15` carries `$(NoWarn);CS1591;RS0016;RS0037;RS0041`, and + that project-level NoWarn defeats `.editorconfig:71`. Measured: a brand-new public type with no + tracker row built clean, `0 Warning(s)`. What **is** enforced is RS0017 — a Shipped row whose + symbol no longer exists — which fires as an error. + + So for B1 the `*REMOVED*` row is mechanically forced and **the new row is not**. Forgetting the + new row leaves a green build. B1 cannot lean on the compiler for that half. + + **The ApiCompat prediction is now measured**, with the change applied and `dotnet pack -c + Release`: `src/Verbara.Sdk` → CP0002 + CP0006; `src/Verbara.Sdk.Ari` → CP0002 only. Exactly what + the proposal predicted. + + **CA1068 applies to internal methods too**, measured — any helper B1 or B2 adds puts the token + last as well. + + **An in-tree precedent argues the other way on position.** `IAriClient.cs:141` declares + `CreateWithoutDialAsync(string endpoint, string app, string? channelId = null, ...)` — `channelId` + as the **first** optional parameter. Appending after `data` is still the recommendation, but B1 + makes that call knowingly rather than discovering the inconsistency later. + + **A trap that "move to named arguments" does not solve by itself:** if a substitute moves to + named arguments but omits `channelId:`, the compiler fills in `null` and NSubstitute + equality-matches it. After B2 the activity sends a real UUID, the setup stops matching, the call + returns `default(ValueTask)`, and the test dies on a null `Channel` rather than on an + argument mismatch. Both setups need an explicit `channelId: Arg.Any()`. + + **`ExternalMediaActivity` has one constructor, not overloads** — `tasks.md` A2 asked for the + plural and there is exactly one, with two optional parameters, at `:41`. +- [x] A3 **Write the failing regression test first, as an argument-capture test.** A test that mocks + `IAriChannelsResource`, constructs `ExternalMediaActivity` with `Encapsulation = "audiosocket"` + and an `AudioSocketServer`, runs it, and asserts on the arguments the activity passed: + `data` is non-null and `transport == "tcp"`. It compiles against the **unfixed** code (it names + no new parameter) and fails on `data == null`. Paste the failure verbatim. + Do **not** write it as "assert `GetStream(Channel.Id)` hits": with a mocked resource the test + chooses both the returned `Channel.Id` and the UUID its own client sends, so it can be made to + pass today — the closed loop this whole change is about. + + **Done 2026-09-24.** Written as an argument-capture test, in + `Tests/Verbara.Sdk.Activities.Tests/Activities/ActivityTests.cs`, named + `StartAsync_ShouldSendDataAndTcpTransport_WhenEncapsulationIsAudioSocket`. + + **The failure, verbatim, against unfixed code:** + + ```text + Failed Verbara.Sdk.Activities.Tests.Activities.ActivityTests. + StartAsync_ShouldSendDataAndTcpTransport_WhenEncapsulationIsAudioSocket [117 ms] + Error Message: + Expected sentData not to be because an audiosocket create with no data is HTTP 400 + "data can not be empty" (probe-capture.txt RUN D), so the activity must supply the + identification uuid. + Total tests: 1 + Failed: 1 + ``` + + It builds clean and fails on its assertion, not on a compile error. + + **Three things the task brief did not anticipate, all measured:** + + - **The prescribed shape would not have compiled after B1.** The natural NSubstitute setup ends + in a ninth **positional** `Arg.Any()`, which after B1 binds to + `string? channelId` — CS1503. The same trap A4 names for the two existing substitutes applies + to the new test. Fixed by naming `cancellationToken:`, the one name that exists both before + and after the fix. + - **`.Returns(...)` would have broken it silently rather than loudly**, for the reason A2 + records. `ReturnsForAnyArgs` is required, not stylistic. + - **The assertion on `transport` is not proven by the red run.** Both `data` and `transport` are + null today and FluentAssertions stops at the first, so `transport` is only exercised by the + forward control (simulated fix → green). + + **A near-miss of this change's own failure family, recorded because it nearly shipped.** + Restoring a scratch file with `mv` preserved its old mtime, MSBuild judged the source older than + its output, skipped the rebuild, and the run reported `Passed! 49/49` **from the fixed + assembly** while the tree held unfixed code. It was caught only because 49/49 contradicted a + filtered run. `touch` plus a re-run gave the true `Failed: 1, Passed: 48`. A green number from a + stale build is indistinguishable from a green number from a correct one. +- [x] A4 Enumerate every call site and every substitute of `CreateExternalMediaAsync` and say which + one B1 changes. There are four today: `ExternalMediaActivity.cs:51`, + `Tests/…/ActivityTests.cs:328`, `Tests/…/ActivityTests.cs:392`, and + `Tests/Verbara.Sdk.Ari.Tests/Resources/AriResourceTests.cs:161`. The two `ActivityTests` + substitutes enumerate nine positional `Arg.Any` ending in a `CancellationToken` and will not + compile after B1. Also confirm against the tree — do not trust this sentence — that the + **two `GetStream` branches** at `ExternalMediaActivity.cs:65-75` are the unexecuted code. The + polling `while` loop itself does run, in two tests. + + + **Done 2026-09-24.** Call sites, and three findings that change B1. + + **The proposal's unexecuted-lines claim is right in substance and wrong in its range.** Lines 65 + and 71 — the `if` conditions — are HITS=2 at 50% branch coverage: evaluated every iteration, + always false. Only **67, 68, 73 and 74** are HITS=0. + + **And the unexecuted set is materially larger than either document says:** lines 80-82 (the + second `TimeoutException` throw, 0/2 branches — the loop is never left through its own condition + because `Task.Delay(200, token)` always throws first), line 87 (`ExecuteAsync` has never + returned normally), line 95, and 100-104 (`DisposeAsync` in its entirety). If B2 restructures + the loop, these become patch lines under C5's 85% floor. + + **B1 produces about thirty compiler errors, not three.** Once CS1503 kills overload resolution + the NSubstitute analyzer stops seeing an interface member and fires NS1004 on every `Arg.Any` in + the failed call — nine per setup, promoted to errors by `TreatWarningsAsErrors`. Measured: 27 + unique NS1004 plus 3 unique CS1503. The 27 that dominate the log are false and point at the + wrong diagnosis; all twenty-seven vanish when the three real ones are fixed. + + **A3's own new substitute is the third CS1503 site**, at working-copy `ActivityTests.cs:453`, + and it carries a comment asserting the opposite — that the captured positions are stable across + the fix. They are not. + + **`AriResourceTests.cs:161` keeps compiling and keeps passing after B1 while asserting nothing + about `channelId`.** Worse: across all 477 Ari unit tests, `AriChannelsResource.cs:90` — the + `data=` appender — sits at 50% branch coverage and **has never executed**, along with + `connection_type` and `direction`. B1 adds `channelId` in exactly that shape, so asserting the + literal `channelId=` is worthless unless the same test actually passes one. + + **The session's default `grep` honours `.gitignore`.** `docs/plans/` is gitignored but its files + were force-added and are tracked, so grep returned 14 hits where `git grep` returns 17, silently + omitting three. **Any "I found every call site" claim made with the default grep in this repo is + unsound** — use `git grep`. + + **`Examples/ContactCenterSupervisionExample/Program.cs:78`** is a fifth + `Substitute.For()`. It does not configure `CreateExternalMediaAsync`, so it + keeps compiling — but example projects are in the build. + + **Not verified, and stated rather than assumed:** the proposal's claim that Pro's fakes and + decorators break. Pro lives outside this worktree and this task may not leave it. Within the + worktree `AriChannelsResource` is the only implementer, so nothing here breaks on CS0535. +## Phase B — critical components (one focused subagent each) + +- [x] B1 Add the `channelId` parameter to `CreateExternalMediaAsync` on **both** + `IAriChannelsResource` and `AriChannelsResource`, positioned **before** `cancellationToken` + (CT-last is the SDK's convention and CA1068 is on under `TreatWarningsAsErrors`). Then: + - the two `ActivityTests` substitutes move to **named arguments**; + - `AriResourceTests` extends its URL assertion to the **literal `channelId=`** — Asterisk + spells it camelCase among snake_case siblings and ignores an unknown parameter silently, so a + typo becomes a no-op that only the queue would catch; + - `*REMOVED*` + new rows in `PublicAPI.Unshipped.txt` for both packages (ADR-0023); + - `CompatibilitySuppressions.xml` generated deliberately and **read entry by entry before it is + kept** — expect `CP0002` in both packages and `CP0006` on the interface, and `src/Verbara.Sdk` + has no such file today. ADR-0055 records what happens when the flag is left to the build + machine: validation runs, reports green, and compares nothing. + - a migration note (ADR-0028 requires one for a minor carrying a break). + Confirm with the exact command CI runs — `dotnet pack` — not with a clean build. A green build + says nothing about `PackageValidation`; that mistake cost #302 a CI failure. + + **Done 2026-09-24.** Parameter in, break declared in both packages, `dotnet pack -c Release` + green with validation **proven** to have run. The A3 test still failed here — correctly; it + needs B2. + + **Position: appended after `data`, against the in-tree precedent, and the reason is the point.** + A2 found `IAriClient.cs:141` puts `channelId` first among `CreateWithoutDialAsync`'s optionals. + Inserting ahead of `encapsulation` would have been a **silent** break: every slot from + `encapsulation` to `data` is `string?`, so an existing positional call would rebind each + argument one place over and go on compiling — wrong values on the wire, no diagnostic anywhere. + Appending breaks only callers that passed the token positionally, and that break is `CS1503`. + A loud break beats consistency with one sibling. The reason is written into the `` so + the next reader does not "fix" the inconsistency. + + ```csharp + ValueTask CreateExternalMediaAsync(string app, string externalHost, string format, + string? encapsulation = null, string? transport = null, string? connectionType = null, + string? direction = null, string? data = null, string? channelId = null, + CancellationToken cancellationToken = default); + ``` + + **The camelCase spelling is corroborated inside the tree, which nobody had noticed.** + `CreateWithoutDialAsync` already appends `&channelId=` at `AriChannelsResource.cs:222`. This + repository has been spelling it that way on another endpoint since before the probe measured it. + + **`AriResourceTests` now sends what it asserts.** A4's point was that the test asserted + `encapsulation=`/`transport=` while `data=`, `connection_type=` and `direction=` had never + executed across 477 tests. It now supplies all six optionals and asserts each literal. + Negative control — the appender misspelled as `channel_id=`: + + ```text + Failed Channels_CreateExternalMediaAsync_ShouldPostWithParams + Expected handler.LastRequestUri "…&data=f9e8d7c6-…&channel_id=0a1b2c3d-…" to contain + "channelId=0a1b2c3d-4e5f-6071-8293-a4b5c6d7e8f9" because Asterisk spells this parameter + channelId, and silently ignores any other spelling. + ``` + + That failure doubles as proof the other three appenders now execute — they are all in the URL it + printed. + + **RS0017 forced the `*REMOVED*` rows; nothing forced the new ones.** A2's warning held exactly: + + ```text + src/Verbara.Sdk/PublicAPI.Shipped.txt(499,1): error RS0017: Symbol + 'Verbara.Sdk.IAriChannelsResource.CreateExternalMediaAsync(…)' is part of the declared API, + but is either not public or could not be found + ``` + + The new rows are present because ADR-0023 says so, not because the compiler asked. + + **Suppressions generated to a scratch path, read entry by entry, then hand-written.** Before any + suppression file, `pack` reported exactly the three predicted breaks and nothing else: `CP0002` + on `AriChannelsResource`, `CP0002` on `IAriChannelsResource`, `CP0006` for the added interface + member. `-p:ApiCompatSuppressionOutputFile=` kept the build machine out of the + repository (ADR-0055). `src/Verbara.Sdk.Ari`'s five existing `AudioFrameType` entries were + reproduced byte-identically — nothing dropped, nothing invented. + + **A new member of the stale-green family, and it is a big one.** `PackageValidation` is + **incremental**: with the whole of `src/Verbara.Sdk/CompatibilitySuppressions.xml` deleted, + `dotnet pack` still exited 0 and printed 29 successful packages. The gate is + `obj/Release/net10.0/Microsoft.NET.ApiCompat.ValidatePackage.semaphore`. Deleting the semaphores + is not enough either — the nupkgs must go too, or packaging is skipped while validation reports + on nothing. Local-only; CI packs a fresh checkout. Any future "pack is green" claim in this + repository must state how it forced validation to run. + + **And `dotnet pack` prints nothing to a redirected stdout on success** — no summary line at all. + A zero-line log with exit 0 is indistinguishable from a run that did nothing. + + **A4's arithmetic was wrong.** Measured against the committed tree: **2** unique CS1503 and + **18** unique NS1004, not 3 and ~27. A4 counted a draft of A3's substitute; the committed one + already named `cancellationToken:`. The shape of A4's warning stands, the numbers do not. + + **Two substitutes beyond A4's list** — `ContactCenterActivityTests.cs:163` and `:196` — neither + configuring this method, so neither needed a change. Seven substitutes exist, not five. +- [x] B2 `ExternalMediaActivity` derives its request from the server it was handed: + `encapsulation = Encapsulation ?? (_audioSocketServer is not null ? "audiosocket" : null)`; when + the encapsulation is AudioSocket (compare ordinal-ignore-case — Asterisk uses `strcasecmp`) then + `transport = Transport ?? "tcp"` and one `Guid.NewGuid().ToString()` goes to **both** `channelId` + and `data`. Canonical lowercase is not incidental: `AudioSocketSession.ParseUuid` keys the table + with `new Guid(bytes, bigEndian: true).ToString()` into an ordinal comparer, so any other + spelling creates a channel whose stream cannot be found. Write both reasons in the code. + Decide and implement what happens when an `AudioSocketServer` is supplied with a non-AudioSocket + encapsulation: today that combination creates an RTP channel and waits out a 30-second timeout, + which is the shape the delta spec forbids. A unit test pins whichever behaviour is chosen, plus + one negative control using a non-canonical identifier spelling. + + **Done 2026-09-24.** The activity now derives its request from the server it was handed, the A3 + regression test passes, and the contradiction case is refused instead of timing out. + + The mint is `Guid.NewGuid().ToString()` — canonical lowercase, because `ParseUuid` keys the + table with `new Guid(bytes, bigEndian: true).ToString()` into an **ordinal** comparer and A1 + measured what any other spelling does: HTTP 200, `Channel.Id` in the spelling sent, and a lookup + that misses. Both reasons are written in the code, not left to this file. + + **The contradiction case is refused.** An `AudioSocketServer` supplied while the encapsulation is + not AudioSocket used to create an RTP channel and wait out thirty seconds — the shape + Requirement 2 of the delta spec forbids. + + **The negative control for it was blunt and nearly shipped that way.** With an unconfigured + substitute, deleting the guard made the activity die on a `NullReferenceException` from a null + `Channel` — a red for the wrong reason, saying nothing about the timeout it was supposed to + prove. Configuring the create to **succeed** is what makes the mutation reproduce the real + defect: + + ```text + [30 s] TimeoutException: Asterisk did not connect to audio server within 00:00:30 + ``` + + The arrange looks redundant — the call must never happen — so there is a comment saying why it + is there. + + **Two tooling findings.** `dotnet pack -tl:off -v m` prints **no** summary on success either, so + grepping for "Build succeeded" on a green pack returns nothing: a different flavour of B1's + note, and `-tl:off -v m` fixes `build` but not `pack`. And deleting only the ApiCompat + semaphores is not enough — measured EXIT=0 with **zero** `Successfully created package` lines, + validation running while packaging was skipped entirely. The nupkgs have to go too. + + `` on a member inherited from `AriActivityBase` does not resolve, and + CS1574 is an error here — the full `AriActivityBase.StartAsync(CancellationToken)` is required. +- [x] B3 Fix the contract text. `IAudioServer.GetStream` says "Get an active stream by channel ID" and + `IAudioStream.ChannelId` says "Unique ID of the external media channel in Asterisk" — both + assert an identity the code does not hold. Say for each implementation what the key is and in + what form: for `AudioSocketServer`, the UUID Asterisk sent in its identification frame in + canonical lowercase hyphenated form, which for the ARI route is the value the creator supplied + as `channelId` and `data`. Say in `ExternalMediaActivity`'s remarks that the WebSocket branch is + **not** routed — `WebSocketAudioServer` keys on the last segment of the request URL (F1). + + + **Done 2026-09-24.** Three findings, and one deliberate widening. + + **The same wrong sentence was on both implementations, not only on the interface.** + `/// Get an active stream by channel ID.` sat verbatim on + `AudioSocketServer.cs:52` and `WebSocketAudioServer.cs:63` — public members of public shipped + classes, which is what a caller holding a concrete server reads. Both fixed, named here rather + than done silently: Requirement 4 covers *any* published contract for looking a stream up by + key, and leaving them would have left the interface doc contradicting its own implementations. + + **The brief's phrasing was over-stated and was not reproduced.** It said the AudioSocket key is + "the value the creator supplied as `channelId` and `data`". Probe RUN C shows `data` **alone** + is the key — the wire UUID equalled `data` while `Channel.Id` came back as the Asterisk uniqueid + `1790244226.1`. `channelId` contributes nothing to the key; it only makes `Channel.Id` equal to + it. The landed text says that instead. + + **A fourth wrong piece of prose found and deliberately left.** + `WebSocketAudioServer.cs:311-313` documents `internal static ReadUpgradeRequestAsync` as + extracting "channel ID from URL path … Expected URL: /ws/{channelId}" — precisely the false + identity this task removes, and it even names the placeholder `{channelId}`. It is on an + internal member, so not a published contract. It belongs to F1's change, where the WebSocket key + finally gets measured. + + **One expectation this phase had accumulated is wrong, and that matters.** + `GenerateDocumentationFile` is true and CS1574 is **not** in `NoWarn`, so the doc edits do have a + compiler oracle — unlike RS0016, which A2 measured as globally suppressed. A green doc build + here is worth something. +## Phase C — integration (batched) + +- [x] C1 Functional test beside `AudioSocketWireFunctionalTests`. Three things it must get right: + - **Subscribe a Stasis application first.** `externalMedia` validates that `app` is non-empty + and never checks it is registered, so the create returns 200 against a bare listener — but the + channel then runs Stasis with no subscriber, Asterisk hangs it up, and the entry is removed + within milliseconds of a 200 ms poll. A test written from the probe alone times out and looks + like the fix not working. + - Construct the activity with `Encapsulation = "audiosocket"` and **nothing else**, so the test + measures the class's own path rather than a configuration the test supplied. + - A fixed port that is **not** 19092 or 19093 — those belong to extensions 710 and 711. + Assert with a reason on every path; never `return` early. Assert + `activity.AudioStream!.ChannelId == activity.Channel!.Id`. + + **Done 2026-09-24.** + `Tests/Verbara.Sdk.FunctionalTests/Layer5_Integration/Audio/ExternalMediaChannelIdFunctionalTests.cs`, + one test, port **19094**, the activity constructed with `App`, `ExternalHost`, `Encapsulation` + and `ConnectionTimeout` and nothing else. Six assertions, every one with a `because`, no + `return` anywhere in the body. + + ```text + Passed ExternalMediaActivity_ShouldResolveItsStreamByTheReturnedChannelId_ + WhenAsteriskConnectsOverAudioSocket [259 ms] + Test Run Successful. Total tests: 1 Passed: 1 + ``` + + **Run on Asterisk 23 as well as 22**, rather than letting the merge queue meet it first. + `ASTERISK_VERSION=23 CODEC_OPUS_VERSION=23.0_1.3.0` → 23.4.1, passed in 261 ms; 22.9.0 in 259 ms. + + **The functional project had no reference to `Verbara.Sdk.Activities`.** The class this whole + change is about was unreachable from the only suite that can put a real Asterisk on the other + end. One line added to the csproj — a project-graph change, so `LayeringGuard` was in scope and + is green. + + **A sixteen-second green and a sixteen-second no-op look alike, and the duration is not what + tells them apart.** The whole run is 15–17 s while the test itself is 54–261 ms, because + `chan_audiosocket` connects *during* the create — there is nothing to wait for on the success + path. The seconds are container startup, and the log names them + (`Execute "asterisk -rx core show uptime" at Docker container …`, `… ready`). C3's skipped job + has the same wall clock and no such lines. **The container log is the discriminator.** + + **The first assertion had to report the server's own table, or the control proves less than it + looks.** Written the obvious way, reversion 3 fails with `TimeoutException: Asterisk did not + connect to audio server within 00:00:45` — a sentence that is *false* about what happened, since + Asterisk connected and streamed, and indistinguishable from Asterisk never connecting. The + reason clause now carries `activity.Channel?.Id` and `server.ActiveStreams.Select(s => s.ChannelId)`, + so the red prints the two identifiers side by side. + + The wire's own account is asserted **separately** from the activity's: `OnStreamConnected` gives + the UUID parsed out of Asterisk's frame, a value the test did not choose. `GetStream(Channel.Id)` + hitting implies it only transitively, and a transitive claim through a mock is the proposal's + whole complaint about the unit test. +- [x] C2 **Negatively control the functional test by reverting the fix, not by breaking the + assertion.** Breaking the expected value proves the assertion is wired; it does not prove the + test can see the defect. Three reversions, each failure pasted: drop `data`; drop the `tcp` + transport; pass `channelId` and `data` different values. ADR-0060's own control was a revert. + + **Done 2026-09-24.** Three reversions of `ExternalMediaActivity.ExecuteAsync`, each restored + from a pristine copy and `touch`ed before the rebuild, each rebuild `0 Warning(s) 0 Error(s)`. + All three ran against the **final** test file. `ExternalMediaActivity.cs` is byte-identical to + HEAD afterwards. + + **Reversion 1 — stop sending `data`.** Fails at the create, before a channel exists: + + ```text + Expected failure to be … instead StartAsync ended with + Verbara.Sdk.Ari.AriException: ARI request failed with 400: {"message": "data can not be empty"} + with the returned channel id no channel and these streams registered on the server: [none] + ``` + + **Reversion 2 — stop sending the `tcp` transport.** Also at the create: + + ```text + … AriException: ARI request failed with 400: + {"message": "transport must be 'tcp' for audiosocket encapsulation"} + ``` + + **Reversion 3 — `channelId` and `data` different, both well-formed canonical UUIDs**, per A1's + correction #4: a malformed `data` is HTTP 500 at the create and would prove nothing. The create + returns **200**, Asterisk connects, and the failure lands where the defect actually is — at the + lookup, with the two identifiers printed apart. + + **Two things the reversions exposed that no task anticipated:** + + - **A4's "the loop is never left through its own condition" is a race, not a determinism.** Two + runs of identical source threw from *different* lines — `:196` once and `:200` once. The unit + lane's HITS=0 on the second throw is a short-timeout artefact, not a dead branch. That matters + if C5's floor ever pushes someone to declare it unreachable. + - **An unused `private const` is not a build oracle.** Reversion 2 left + `private const string AudioSocketTransport = "tcp"` with zero references and the build reported + `0 Warning(s)` under `TreatWarningsAsErrors` with `WarningLevel 9999`. Nothing mechanical would + catch a half-finished revert of that line. +- [x] C3 Apply the **`ci:functional`** label to the PR. `docker/Dockerfile.asterisk` work and the + functional suite are skipped on `pull_request` unless that label is present (ADR-0051, + `.github/workflows/ci.yml`), and the matrix is `[23]` on a PR against `[22, 23]` in the queue. + Without the label the job reports `pass` in about sixteen seconds having started no Asterisk — + which is exactly how a broken dialplan reached the merge queue in #302. Record the PR-time + result, not only the queue's. + + **Done 2026-09-24.** The label exists and says what it does: `ci:functional — Run the + functional/Testcontainers matrix on this PR (ADR-0051 opt-in)`. Applied to the PR before any + code landed. It existed while #302's sixteen-second `pass` was read as coverage. +- [x] C4 `CHANGELOG.md [Unreleased]`: a `### Fixed — BREAKING` entry. Give it its **own insertion + anchor** distinct from any other in-flight entry — a shared anchor ejected #300 from the merge + queue with fourteen checks green. Leave `(#N)` for close-out. + + **Done 2026-09-24.** `### Fixed — BREAKING`, anchored on the **#302 entry heading** rather than + on `## [Unreleased]`. That is the whole point of the anchor rule: #299 and #300 both inserted at + the top of `[Unreleased]` and collided, ejecting #300 from the merge queue as `DIRTY` with + fourteen checks green, and the resolution left a duplicated heading that had to be spotted by + hand. No other open PR touches `CHANGELOG.md` right now, checked rather than assumed. `(#N)` + left for close-out. + + The entry leads with the three measured failures — the RTP default, the missing transport, the + missing data — and then the two-UUID capture, so a reader who doubts the claim can check it + rather than take it. It states what a consumer must do and why the parameter was appended rather + than placed first. +- [x] C5 Coverage measured **after committing**, never before. The unit lane excludes + `Category=Functional`, so C1 contributes **zero** patch coverage against the 85% floor: A3 and + B2's unit tests are what must carry `ExternalMediaActivity`'s new lines. Read the changed-line + count and check it against the size of the diff before believing the percentage — a sibling + change accepted `100% (6/6)` on a five-file diff and the `6` was the tell. + + **Done 2026-09-24, after committing**, which is what this task exists to force. + + **The first measurement was wrong and the wrongness is the lesson.** Run without + `--settings coverlet.runsettings` and without `-c Release`, the gates reported a red band: + line 81.0% against a floor of 83.0, branch 62.51% against 64.0. It was not a regression — it was + the wrong command. The tell was in the output: **42125 lines measured**, against the 13322 that + `coverage-floor.json`'s own comment records as honest. Three times the denominator. + + With the exact command from `ci.yml:87-93`: + + | gate | result | + |---|---| + | line coverage | **84.37%**, band `[83.0, 86.0]` | + | branch coverage | **68.81%**, floor 64.0 | + | lines measured | **13479**, min 12315 — comparable to the recorded 13322 | + | **patch coverage** | **100.0%** — 22/22 changed executable lines, floor 85.0 | + | exclusion markers | **0**, baseline 0, 865 files scanned | + + **The changed-line count was checked against the diff rather than believed.** 22 is right: the + activity's new request-shaping lines, the resource's appender, and the guard. The migration + guide, the CHANGELOG, the suppression files, the PublicAPI rows and the openspec artifacts carry + no executable lines, and the functional test is excluded from this lane by + `Category!=Functional` — which is why A3's and B2's unit tests had to carry the activity's new + lines, not C1's. + + A sibling change accepted `100% (6/6)` on a five-file diff and the `6` was the tell. Here the + number is consistent with the diff. +- [x] C6 `openspec validate --all --strict` green, full unit lane green, Governance green, `dotnet + pack` clean, and CI green on the PR **including the `merge_group` build** — the only place the + functional suite runs both Asterisk versions. +- [x] C7 Open a change for the follow-ups below and write its link back into this file. They are + carried as prose, not as unchecked boxes: `openspec/config.yaml` requires every deferred finding + to be harvested into an open change or an ADR addendum before archiving, and three `- [ ]` boxes + inside this change would defer that harvest to close-out — the exact failure ADR-0060 committed + and this change exists to stop repeating. + + + **Done 2026-09-24.** The change is + **`openspec/changes/a-published-surface-is-one-something-measures`**, carrying F1, F2 and F3. + `openspec validate --all --strict` → 15 passed, 0 failed. + + **It corrected three things this file had wrong.** + + - **F3 is ten undefined extensions, not five.** The suite reaches `[test-functional]` by *two* + forms, and both this file and the dialplan comment counted only `Local/N@test-functional`. + Pairing `Exten` with `Context` adds 161, 162, 163, 750 and 9999. Re-verified independently: + dialed = 100 150 155 160 161 162 163 300 500 600 700 750 900 950 999 9998 9999; defined = 100 + 150 155 500 600 710 711 900 950; **undefined = 160 161 162 163 300 700 750 999 9998 9999**. + - **F1's line numbers had aged inside this very phase.** The key is computed at + `WebSocketAudioServer.cs:341` and registered at `:266`; B3's doc edits moved both. Corrected + below, with the caveat that a citation by line number does not survive an edit above it. + - **The unasserting early return has four spellings, not one**, and covers all eight `[Fact]`s + in `ConfBridgeAdvancedTests` rather than the five that failed: `confJoin is null`, + `!joinEvents.Any(pred)`, `joinEvents.IsEmpty`, and `await Task.WhenAny(t, Task.Delay(…)) != t`. + Sweeping one phrasing finds three of eight. The scan reaches 71 candidate sites across 11 + files — and it is a scan, not a verdict: `BridgeLifecycleTests:315` is a false positive. + + **And it found why F2 survived, which nobody had asked.** `AudioStreamMetrics`' test file is the + closed loop in miniature: of eleven `[Fact]`s, six assert only `.Should().NotBeNull()` on a + `static readonly` field initialised at its declaration — true in every possible state of the + program — and four supply their own measurement through a `MeterListener` and assert they + observed it. All eleven are green with every production call site absent. + + **`extensions.conf` was carrying the ADR-0060 failure it was written to prevent**: lines 104-111 + documented the hole, stated the wrong count, and ended "the hole is filed separately" with no + link — inside the commit that existed to stop that. Corrected in this change: the count is right + and the comment names the change above. + + **Done 2026-09-24.** Local verification, with every stale-green trap this change measured forced + open: + + ```text + build 0 Warning(s), 0 Error(s) + unit lane 3643 passed, 0 failed, 0 projects red + Governance 129 passed, 0 failed + openspec 15 passed, 0 failed (--all --strict) + dotnet pack exit 0, 29 packages + pack neg. ctl with CompatibilitySuppressions.xml removed -> exit 1, + exactly CP0002 + CP0006, proving validation actually ran + coverage band OK, patch 100.0% (22/22), exclusions 0 + functional the new test passes on Asterisk 22.9.0 AND 23.4.1 + ``` + + The pack negative control is not ceremony. B1 measured that `PackageValidation` is + **incremental**: with the whole suppression file deleted, `dotnet pack` still exited 0 and + printed 29 successful packages, because of + `obj/Release/net10.0/Microsoft.NET.ApiCompat.ValidatePackage.semaphore`. Deleting the semaphores + alone is not enough either — the nupkgs must go too, or validation runs while packaging is + skipped. **Any future "pack is green" claim in this repository has to say how it forced + validation to run.** #302's did not. + + CI on the PR, including the `merge_group` build, is recorded at close-out. +## Follow-ups this change does NOT fix + +ADR-0060 wrote "tracked separately" with no link, and the close-out archived it anyway. C7 opens the +change that carries these; the lines below are the evidence it starts from. + +- **F1 — `WebSocketAudioServer` keys on a URL path segment.** The key is computed at + `WebSocketAudioServer.cs:341` (`path.TrimStart('/').Split('/').LastOrDefault()?.Split('?').FirstOrDefault()`) and registered at + `:266`. The SDK's own example puts a literal `/audio` there + (`Examples/WebSocketMediaExample/Program.cs:10`), so every concurrent call would register under the + key `"audio"`. `CompositeAudioServer` hands the same string to both servers, which do not agree on + what it means. **No probe has measured what `externalMedia` with `transport=websocket` puts in the + request path**, which is why this stays out of the present change rather than being designed from a + reading of the code. +- **F2 — `AudioStreamMetrics`** declares ten instruments with zero production call sites. +- **F3 — ten dialplan extensions are dialed and never defined.** `[test-functional]` is dialed at + 160, 161, 162, 163, 300, 700, 750, 999, 9998 and 9999 and defines none of them. `ConfBridgeAdvancedTests` alone dials the + undefined 700 from ten call sites and passes by taking its `if (confJoin is null) return;` branch + without asserting anything. The shape to hunt is that early return, not the extension numbers. + +These are carried by **`openspec/changes/a-published-surface-is-one-something-measures`** (C7). diff --git a/src/Verbara.Sdk.Activities/Activities/ExternalMediaActivity.cs b/src/Verbara.Sdk.Activities/Activities/ExternalMediaActivity.cs index a6b21461..71336539 100644 --- a/src/Verbara.Sdk.Activities/Activities/ExternalMediaActivity.cs +++ b/src/Verbara.Sdk.Activities/Activities/ExternalMediaActivity.cs @@ -8,8 +8,44 @@ namespace Verbara.Sdk.Activities.Activities; /// to connect back via AudioSocket or WebSocket, and provides an IAudioStream /// for bidirectional audio streaming. /// +/// +/// +/// Only the AudioSocket branch is routed. This activity looks the stream up by +/// on whichever server it was handed, and that only works for +/// AudioSocket because this class arranges it: it mints one identifier and sends it as both +/// channelId and data, so the ARI channel id and the key +/// registers the stream under are the same string. +/// +/// +/// Nothing arranges that for . It keys its table on the last path +/// segment of the HTTP upgrade request URL with the query string stripped, while a non-AudioSocket +/// create leaves an Asterisk-minted uniqueid such as +/// 1790244226.1. This class does not make those two match, so a +/// passed to this activity is expected to poll out +/// and throw however well the +/// WebSocket connection itself went — unless that request path happens to carry the very id +/// Asterisk minted, which is exactly what no probe has measured. That is why this branch is +/// documented here rather than fixed. +/// +/// public sealed class ExternalMediaActivity : AriActivityBase { + /// + /// The value Asterisk's externalMedia expects for AudioSocket encapsulation. Asterisk + /// compares it with strcasecmp, which is why every comparison against it here is + /// : a caller who writes "AudioSocket" reaches + /// chan_audiosocket exactly as one who writes "audiosocket" does, and an ordinal comparison + /// would quietly route the first down the RTP path instead. + /// + private const string AudioSocketEncapsulation = "audiosocket"; + + /// + /// The only transport Asterisk accepts under AudioSocket encapsulation. Anything else is + /// HTTP 400 — "transport must be 'tcp' for audiosocket encapsulation", measured on + /// Asterisk 22.9.0 and 23.4.1 (probe-capture.txt, RUN G). + /// + private const string AudioSocketTransport = "tcp"; + private readonly AudioSocketServer? _audioSocketServer; private readonly WebSocketAudioServer? _webSocketServer; private IAudioStream? _audioStream; @@ -23,10 +59,23 @@ public sealed class ExternalMediaActivity : AriActivityBase /// Audio format. Default: "slin16". public string Format { get; init; } = "slin16"; - /// Encapsulation type (e.g., "audiosocket") or null for default. + /// + /// Encapsulation type (e.g., "audiosocket"), or null to let the activity derive it. Left null + /// with an supplied it becomes "audiosocket"; left null with no + /// AudioSocket server it stays null, which Asterisk reads as rtp/udp. Setting it to a + /// non-AudioSocket value while supplying an is a contradiction + /// and throws + /// rather than + /// creating a channel that can never carry that server's stream. + /// public string? Encapsulation { get; init; } - /// Transport type (e.g., "websocket") or null for default. + /// + /// Transport type (e.g., "websocket"), or null to let the activity derive it. Under AudioSocket + /// encapsulation a null transport becomes "tcp", the only one Asterisk accepts there. An + /// explicit value is always sent as given, so a wrong one fails at the create call with + /// Asterisk's own message instead of being silently rewritten. + /// public string? Transport { get; init; } /// Timeout waiting for Asterisk to connect to the audio server. @@ -47,14 +96,80 @@ public ExternalMediaActivity(IAriClient ariClient, AudioSocketServer? audioSocke protected override async ValueTask ExecuteAsync(CancellationToken cancellationToken) { - // 1. Create ExternalMedia channel via ARI + // 1. Derive the request from the audio server this activity was handed. + // + // An AudioSocketServer with Encapsulation left null is the shape callers actually get, and + // Asterisk reads an absent encapsulation as rtp/udp: it answers HTTP 200 with a UnicastRTP + // channel, nothing ever connects to the AudioSocket server, and the poll below burns the + // whole of ConnectionTimeout before throwing (probe-capture.txt, RUN H and RUN H+). + var encapsulation = Encapsulation ?? (_audioSocketServer is not null ? AudioSocketEncapsulation : null); + + // strcasecmp on Asterisk's side — see AudioSocketEncapsulation. + var isAudioSocket = string.Equals(encapsulation, AudioSocketEncapsulation, StringComparison.OrdinalIgnoreCase); + + if (_audioSocketServer is not null && !isAudioSocket) + { + // The contradiction case, refused rather than attempted. Asterisk would happily create + // the channel on the encapsulation asked for; the AudioSocket server would simply never + // be connected to, and this activity would then wait out ConnectionTimeout and report a + // TimeoutException — "the audio server did not connect" — for what is a configuration + // mistake made before any connection was possible. Failing here keeps the diagnosis + // where the cause is, and keeps a doomed channel from being created at all. + throw new InvalidOperationException( + $"An AudioSocketServer was supplied but Encapsulation is \"{encapsulation}\", not \"{AudioSocketEncapsulation}\". " + + $"Asterisk would create a {encapsulation} channel that never connects to that server, and this activity " + + $"would wait out ConnectionTimeout ({ConnectionTimeout}) and report a connection failure instead of this " + + "contradiction. Set Encapsulation to \"audiosocket\" — or leave it null, which derives it from the " + + "server — or do not pass an AudioSocketServer."); + } + + var transport = Transport; + string? data = null; + string? channelId = null; + + if (isAudioSocket) + { + // Asterisk rejects audiosocket on any other transport with HTTP 400 "transport must be + // 'tcp' for audiosocket encapsulation" (RUN G) — and the activity never sent one, which + // is why every AudioSocket caller failed at the create call. An explicit Transport still + // wins: a wrong one then fails with Asterisk's own message rather than being rewritten. + transport ??= AudioSocketTransport; + + // ONE identifier into BOTH parameters, which is the whole of this change. + // + // They are different things. `data` is the UUID Asterisk puts in the AudioSocket + // identification frame, and it is what AudioSocketServer keys its stream table by; + // `channelId` is the id Asterisk assigns the ARI channel, and comes back as Channel.Id. + // Passing only `data` leaves Channel.Id an Asterisk-minted uniqueid like "1790244226.1" + // that appears in no table (RUN C), so a consumer holding only a channel id — a + // StasisStart or ChannelHangupRequest handler — can never find the stream. Passing the + // same value as both makes Channel.Id the key (RUN A). Asterisk also requires `data` to + // be present at all: without it, HTTP 400 "data can not be empty" (RUN D). + // + // Guid.ToString() — canonical lowercase, hyphenated — is load-bearing, for two measured + // reasons, not for tidiness: + // 1. AudioSocketSession.ParseUuid renders the sixteen wire bytes with + // new Guid(bytes, bigEndian: true).ToString(), which is always lowercase, and + // AudioSocketServer holds those keys in a ConcurrentDictionary whose + // default comparer is ORDINAL. Only the lowercase spelling can hit it. + // 2. Asterisk echoes `channelId` back verbatim and does not normalise it: an uppercase + // identifier returns HTTP 200 with Channel.Id in the spelling that was sent + // (RUN U), so GetStream(Channel.Id) would miss with no error on any hop. + var identifier = Guid.NewGuid().ToString(); + data = identifier; + channelId = identifier; + } + + // 2. Create ExternalMedia channel via ARI Channel = await AriClient.Channels.CreateExternalMediaAsync( App, ExternalHost, Format, - encapsulation: Encapsulation, - transport: Transport, + encapsulation: encapsulation, + transport: transport, + data: data, + channelId: channelId, cancellationToken: cancellationToken); - // 2. Poll for audio server connection (200ms interval) + // 3. Poll for audio server connection (200ms interval) using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); timeoutCts.CancelAfter(ConnectionTimeout); diff --git a/src/Verbara.Sdk.Ari/Audio/AudioSocketServer.cs b/src/Verbara.Sdk.Ari/Audio/AudioSocketServer.cs index 1d9d2246..492365d2 100644 --- a/src/Verbara.Sdk.Ari/Audio/AudioSocketServer.cs +++ b/src/Verbara.Sdk.Ari/Audio/AudioSocketServer.cs @@ -49,7 +49,13 @@ public sealed class AudioSocketServer : IAudioServer, IAsyncDisposable /// Observable that emits each new audio stream when a connection is established. public IObservable OnStreamConnected => _streamSubject; - /// Get an active stream by channel ID. + /// + /// Get an active stream by the UUID Asterisk sent in its AudioSocket identification frame, in + /// canonical lowercase hyphenated form. The table is an ordinal dictionary, so any other + /// spelling of the same UUID returns . Over ARI that UUID is the value + /// the creator passed as data, and it is the created channel's id only if the creator + /// passed it as channelId too. See . + /// public IAudioStream? GetStream(string channelId) => _streams.TryGetValue(channelId, out var session) ? session : null; diff --git a/src/Verbara.Sdk.Ari/Audio/CompositeAudioServer.cs b/src/Verbara.Sdk.Ari/Audio/CompositeAudioServer.cs index e7b0b5cd..73a11aaa 100644 --- a/src/Verbara.Sdk.Ari/Audio/CompositeAudioServer.cs +++ b/src/Verbara.Sdk.Ari/Audio/CompositeAudioServer.cs @@ -5,6 +5,11 @@ namespace Verbara.Sdk.Ari.Audio; /// /// Aggregates multiple audio servers (AudioSocket + WebSocket) into a single IAudioServer view. /// +/// +/// and only merge, and are safe to read +/// as an aggregate. is the member to read carefully: it hands one +/// string to every server in turn, and the servers do not agree on what that string means. +/// public sealed class CompositeAudioServer : IAudioServer { private readonly IAudioServer[] _servers; @@ -17,6 +22,21 @@ public CompositeAudioServer(IEnumerable servers) public IObservable OnStreamConnected => _servers.Select(s => s.OnStreamConnected).Merge(); + /// + /// Try each aggregated server in construction order and return the first stream registered + /// under . + /// + /// + /// One string, two meanings. The aggregated servers key their tables on different + /// things: on the UUID Asterisk sends in its AudioSocket + /// identification frame — canonical lowercase hyphenated, compared ordinally — and + /// on the last path segment of the HTTP upgrade request URL. + /// Nothing arranges for one identifier to mean the same thing to both, so a value meant for one + /// server is simply a miss in the other, and this method reports a miss and a key in the wrong + /// form the same way: . A caller that knows which transport it uses gets + /// a narrower answer from that server directly. See + /// for the full description of each key. + /// public IAudioStream? GetStream(string channelId) { foreach (var server in _servers) diff --git a/src/Verbara.Sdk.Ari/Audio/WebSocketAudioServer.cs b/src/Verbara.Sdk.Ari/Audio/WebSocketAudioServer.cs index 082ad074..088df25e 100644 --- a/src/Verbara.Sdk.Ari/Audio/WebSocketAudioServer.cs +++ b/src/Verbara.Sdk.Ari/Audio/WebSocketAudioServer.cs @@ -60,7 +60,11 @@ public sealed class WebSocketAudioServer : IAudioServer, IAsyncDisposable /// Observable that emits each new audio stream when a connection is established. public IObservable OnStreamConnected => _streamSubject; - /// Get an active stream by channel ID. + /// + /// Get an active stream by the last path segment of the HTTP upgrade request URL it arrived on, + /// query string stripped — not by an ARI channel id, and not by the AudioSocket identification + /// UUID. See . + /// public IAudioStream? GetStream(string channelId) => _streams.TryGetValue(channelId, out var session) ? session : null; diff --git a/src/Verbara.Sdk.Ari/CompatibilitySuppressions.xml b/src/Verbara.Sdk.Ari/CompatibilitySuppressions.xml index c3333273..a8c30156 100644 --- a/src/Verbara.Sdk.Ari/CompatibilitySuppressions.xml +++ b/src/Verbara.Sdk.Ari/CompatibilitySuppressions.xml @@ -3,19 +3,39 @@ + + + + CP0002 + M:Verbara.Sdk.IAriChannelsResource.CreateExternalMediaAsync(System.String,System.String,System.String,System.String,System.String,System.String,System.String,System.String,System.Threading.CancellationToken) + lib/net10.0/Verbara.Sdk.dll + lib/net10.0/Verbara.Sdk.dll + true + + + CP0006 + M:Verbara.Sdk.IAriChannelsResource.CreateExternalMediaAsync(System.String,System.String,System.String,System.String,System.String,System.String,System.String,System.String,System.String,System.Threading.CancellationToken) + lib/net10.0/Verbara.Sdk.dll + lib/net10.0/Verbara.Sdk.dll + true + + \ No newline at end of file diff --git a/src/Verbara.Sdk/IAriClient.cs b/src/Verbara.Sdk/IAriClient.cs index 20aa40ba..ec446746 100644 --- a/src/Verbara.Sdk/IAriClient.cs +++ b/src/Verbara.Sdk/IAriClient.cs @@ -97,9 +97,34 @@ public interface IAriChannelsResource ValueTask AnswerAsync(string channelId, CancellationToken cancellationToken = default); /// Create an external media channel. POST /channels/externalMedia + /// + /// + /// channelId is the unique id Asterisk assigns the channel it creates; it comes back as + /// . Asterisk spells this query parameter in camelCase while + /// every sibling here is snake_case, and it ignores a parameter it does not recognise instead of + /// rejecting it — so a misspelling is a silent no-op, not an error status. Asterisk echoes the + /// value back verbatim and does not require it to be a UUID. + /// + /// + /// It is a different thing from data. Under AudioSocket encapsulation data is the + /// identification UUID Asterisk sends on the audio connection — the value an AudioSocket server + /// keys its stream table by, and it must parse as a UUID — while channelId only names the + /// ARI channel. Passing one identifier as both is what makes a stream findable by the channel id + /// this call returned. + /// + /// + /// Placed after data rather than first among the optionals, which is where + /// puts its own channelId. The inconsistency is + /// deliberate: inserting ahead of encapsulation would rebind every existing positional + /// argument to a different string? parameter and still compile, so callers would break + /// silently at runtime. Appending before the cancellation token breaks only callers that passed + /// that token positionally, and that break is a compile error. + /// + /// ValueTask CreateExternalMediaAsync(string app, string externalHost, string format, string? encapsulation = null, string? transport = null, string? connectionType = null, - string? direction = null, string? data = null, CancellationToken cancellationToken = default); + string? direction = null, string? data = null, string? channelId = null, + CancellationToken cancellationToken = default); /// Get a channel variable. GET /channels/{channelId}/variable ValueTask GetVariableAsync(string channelId, string variable, CancellationToken cancellationToken = default); @@ -664,7 +689,31 @@ public sealed class AriRtpStats [System.Diagnostics.CodeAnalysis.SuppressMessage("Naming", "CA1711:Identifiers should not have incorrect suffix", Justification = "IAudioStream is the correct domain name for this abstraction")] public interface IAudioStream : IAsyncDisposable { - /// Unique ID of the external media channel in Asterisk. + /// + /// The key this stream is registered under in its server's table. It is not, in general, + /// an ARI channel id: the two implementations derive it from different things, and a value that + /// is a valid key for one of them is meaningless to the other. + /// + /// + /// + /// AudioSocket. The UUID Asterisk sent in the AudioSocket identification frame, rendered + /// from the sixteen wire bytes as new Guid(bytes, bigEndian: true).ToString() — so always + /// in canonical lowercase hyphenated form, e.g. + /// cd98c592-9ce2-4093-afae-420d53629a4f. Over ARI that is the value the creator passed as + /// data to . It equals + /// only when the creator passed that same value as channelId + /// as well — which is what ExternalMediaActivity does. Passing data alone leaves + /// an Asterisk-minted uniqueid such as 1790244226.1, which + /// appears in no stream table. + /// + /// + /// WebSocket. Not an Asterisk identifier at all: the last segment of the HTTP upgrade + /// request's path with any query string removed — + /// path.TrimStart('/').Split('/').LastOrDefault()?.Split('?').FirstOrDefault(). It is + /// whatever the caller put at the end of the URL it dialled, so every connection that dials the + /// same URL yields the same key. + /// + /// string ChannelId { get; } /// Audio format (e.g., "slin16", "ulaw", "alaw"). @@ -701,7 +750,27 @@ public interface IAudioServer /// Observable that emits each new audio stream when a connection is established. IObservable OnStreamConnected { get; } - /// Get an active stream by channel ID. + /// + /// Get an active stream by the key its server registered it under. That key is not an + /// ARI channel id in general, and it is not interchangeable between implementations — a + /// CompositeAudioServer hands this one string to servers that do not agree on what it + /// means. + /// + /// + /// The registration key. It is the stream's and is + /// described in full there; the parameter keeps its old name for source compatibility, not + /// because the value is a channel id. In short — for AudioSocketServer: the UUID from + /// Asterisk's AudioSocket identification frame, in canonical lowercase hyphenated form and + /// compared ordinally, so any other spelling of the same UUID misses. For + /// WebSocketAudioServer: the last path segment of the HTTP upgrade request URL, query + /// string stripped. An ARI is a key for the first only when the + /// channel was created with that same value in both channelId and data, as + /// ExternalMediaActivity does; it is not a key for the second. + /// + /// + /// The stream registered under that key, or if there is none. A key in + /// the wrong form is indistinguishable from an absent stream, which is why the form matters. + /// IAudioStream? GetStream(string channelId); /// All currently active audio streams. diff --git a/src/Verbara.Sdk/PublicAPI.Unshipped.txt b/src/Verbara.Sdk/PublicAPI.Unshipped.txt index 9d3ab23a..040b360b 100644 --- a/src/Verbara.Sdk/PublicAPI.Unshipped.txt +++ b/src/Verbara.Sdk/PublicAPI.Unshipped.txt @@ -74,3 +74,5 @@ const Verbara.Sdk.VerbaraSemanticConventions.Events.MediaStarted = "asterisk.med const Verbara.Sdk.VerbaraSemanticConventions.Node.OriginId = "origin.node.id" -> string! const Verbara.Sdk.VerbaraSemanticConventions.Node.ReceiverId = "receiver.node.id" -> string! const Verbara.Sdk.VerbaraSemanticConventions.Tenant.Id = "tenant.id" -> string! +*REMOVED*Verbara.Sdk.IAriChannelsResource.CreateExternalMediaAsync(string! app, string! externalHost, string! format, string? encapsulation = null, string? transport = null, string? connectionType = null, string? direction = null, string? data = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> System.Threading.Tasks.ValueTask +Verbara.Sdk.IAriChannelsResource.CreateExternalMediaAsync(string! app, string! externalHost, string! format, string? encapsulation = null, string? transport = null, string? connectionType = null, string? direction = null, string? data = null, string? channelId = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> System.Threading.Tasks.ValueTask