fix: propagate SPRT discovery bind/broadcast failures instead of swallowing them
SprtDiscoveryService caught SocketException on the UDP 9100 bind and the broadcast send and returned an empty list either way. That defeated the CompositeDiscoveryService fix that rethrows only when every child discovery service fails: on a real network fault the composite would see one child throw (ENPC) and one child silently succeed empty (SPRT), conclude discovery worked and found nothing, and let PrinterDiscoveryBackgroundTask expire and deregister live Epson printers after the retention window - discarding their queued jobs. Move the policy decision to the composite, which already has it right: the child now reports bind/broadcast failure honestly by rethrowing, and CompositeDiscoveryService absorbs a single child's failure while still rethrowing when every child fails. The per-datagram receive-loop catch is unchanged - a single bad receive still shouldn't abort the pass. Rewrote SprtDiscoveryServiceTests' bind-failure test to assert the throw, and added an end-to-end CompositeDiscoveryServiceTests case with a real SprtDiscoveryService failing to bind alongside a stub that succeeds, proving the composite still degrades gracefully rather than losing the pass. Updated the POS895 design doc to match the new contract. Claude-Session: https://claude.ai/code/session_01E6N3P4nSZLtp4BC2LXVref
This commit is contained in:
@@ -190,4 +190,28 @@ public class CompositeDiscoveryServiceTests
|
||||
|
||||
Assert.Single(seen);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task A_real_sprt_bind_failure_degrades_instead_of_losing_the_other_child()
|
||||
{
|
||||
// End-to-end proof of the fix: SprtDiscoveryService now throws when it
|
||||
// cannot bind UDP 9100 (instead of swallowing the fault and returning
|
||||
// empty), but the composite still absorbs that single child's failure and
|
||||
// returns whatever the other child found - it does not propagate. Total
|
||||
// failure only happens when every child fails, which is asserted
|
||||
// separately by Every_child_throwing_propagates_instead_of_reporting_no_printers.
|
||||
using var blocker = new System.Net.Sockets.UdpClient();
|
||||
blocker.Client.Bind(new System.Net.IPEndPoint(System.Net.IPAddress.Any, 9100));
|
||||
|
||||
var composite = new CompositeDiscoveryService(new IDiscoveryService[]
|
||||
{
|
||||
new SprtDiscoveryService(),
|
||||
new StubDiscovery("10.0.0.9")
|
||||
});
|
||||
|
||||
var result = await composite.DiscoverPrintersAsync(TimeSpan.FromMilliseconds(200));
|
||||
|
||||
var only = Assert.Single(result);
|
||||
Assert.Equal("10.0.0.9", only.IPAddress);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -82,16 +82,20 @@ public class SprtDiscoveryServiceTests
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task DiscoverPrintersAsync_returns_empty_when_port_9100_is_taken()
|
||||
public async Task DiscoverPrintersAsync_throws_when_it_cannot_bind_port_9100()
|
||||
{
|
||||
// Hold UDP 9100 so the service cannot bind it. It must degrade, not throw:
|
||||
// ENPC discovery has to keep working when this one cannot start.
|
||||
// Hold UDP 9100 so the service cannot bind it. It must throw rather than
|
||||
// swallow the failure and return empty: CompositeDiscoveryService needs to
|
||||
// tell "this pass is broken" apart from "the network genuinely has no SPRT
|
||||
// printers on it". Swallowing it here would let a network fault look like
|
||||
// a clean empty result, which is exactly what let live Epson printers get
|
||||
// silently deregistered when discovery quietly "succeeded" with nothing.
|
||||
using var blocker = new System.Net.Sockets.UdpClient();
|
||||
blocker.Client.Bind(new System.Net.IPEndPoint(System.Net.IPAddress.Any, 9100));
|
||||
|
||||
var service = new SprtDiscoveryService();
|
||||
var result = await service.DiscoverPrintersAsync(TimeSpan.FromMilliseconds(200));
|
||||
|
||||
Assert.Empty(result);
|
||||
await Assert.ThrowsAsync<System.Net.Sockets.SocketException>(
|
||||
() => service.DiscoverPrintersAsync(TimeSpan.FromMilliseconds(200)));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -18,6 +18,15 @@ namespace Inspectron.Epson;
|
||||
/// reply from <printer>:3289 back to <querier>:9100, 39 ASCII bytes
|
||||
/// "28-0e-8b-3d-ca-f1 2608120478"
|
||||
/// |<-- MAC, 17 -->||<- 12 sp ->||<- serial ->|
|
||||
///
|
||||
/// Error contract: a failure to bind UDP 9100 or to send the broadcast probe
|
||||
/// propagates as a <see cref="SocketException"/> instead of being swallowed
|
||||
/// here. That lets <see cref="CompositeDiscoveryService"/>, which absorbs a
|
||||
/// single child's failure but rethrows when every child fails, tell a broken
|
||||
/// discovery pass (this service throwing) apart from a healthy pass on a
|
||||
/// network with no SPRT printers on it (this service returning an empty
|
||||
/// list). A single bad datagram in the receive loop is not treated as a pass
|
||||
/// failure and does not throw.
|
||||
/// </summary>
|
||||
public class SprtDiscoveryService : IDiscoveryService
|
||||
{
|
||||
@@ -104,7 +113,7 @@ public class SprtDiscoveryService : IDiscoveryService
|
||||
{
|
||||
_logger?.LogWarning(ex,
|
||||
"SPRT discovery could not bind UDP {Port}; skipping this pass", ReplyPort);
|
||||
return discovered;
|
||||
throw;
|
||||
}
|
||||
|
||||
var probe = BuildProbe();
|
||||
@@ -117,7 +126,7 @@ public class SprtDiscoveryService : IDiscoveryService
|
||||
catch (SocketException ex)
|
||||
{
|
||||
_logger?.LogWarning(ex, "SPRT discovery broadcast failed");
|
||||
return discovered;
|
||||
throw;
|
||||
}
|
||||
|
||||
DateTime deadline = DateTime.UtcNow.Add(timeout.Value);
|
||||
|
||||
@@ -58,13 +58,21 @@ listens for the timeout, and maps each 39-byte reply to a `DiscoveredPrinter`:
|
||||
Replies that do not parse as a 17-character dashed MAC are logged and skipped,
|
||||
never thrown.
|
||||
|
||||
**Binding UDP 9100 can fail** when another process holds it. On `SocketException`
|
||||
the service logs a warning and returns an empty list, so ENPC discovery still
|
||||
runs. Discovery degrades; it does not crash.
|
||||
**Binding UDP 9100 can fail** when another process holds it, and the broadcast
|
||||
send can fail too. On `SocketException` the service logs a warning and
|
||||
**rethrows** - it does not swallow the fault into an empty list. A silent empty
|
||||
list is indistinguishable from "no SPRT printers on this network", and
|
||||
`CompositeDiscoveryService` is the layer that needs to tell those apart, not
|
||||
this service.
|
||||
|
||||
**`CompositeDiscoveryService`** - takes `IEnumerable<IDiscoveryService>`, runs
|
||||
them concurrently on the shared timeout, merges results by `IPAddress` with
|
||||
first-wins, and forwards each child's `onPrinterDiscovered` callback as it fires.
|
||||
It absorbs a single child's failure (logs it, keeps the other children's
|
||||
results) but rethrows when every child fails, since that means discovery
|
||||
itself is broken rather than that the network simply has no printers on it.
|
||||
Discovery still degrades gracefully overall - it just does so at the composite
|
||||
layer, which is the only layer that can see all children and make that call.
|
||||
|
||||
**Wiring** - `PrintServerBootstrapper.cs:77` changes from
|
||||
`Bind<IDiscoveryService>().To<DiscoveryService>()` to the composite over both
|
||||
|
||||
Reference in New Issue
Block a user