fix(backend): delegated config/all must not silently drop tenants on read errors

Codex Medium on 43a87ff7: the delegated subset was built from TryGetConfigurationAsync, which maps ANY storage failure to exists=false — transient table throttling made managed tenants vanish from a 200 response, whereas the replaced scan path rethrew and returned 500. - New TenantConfigurationService.GetConfigurationIfExistsAsync: strict point-read (cached, non-creating, null on 404) that PROPAGATES storage errors; TryGetConfigurationAsync now delegates to it and keeps its fail-closed mapping for agent security gates. - Delegated config/all reads via the strict method → a read failure fails the request (500) instead of shrinking the subset. - Adjacent regressions from the same hunk: AllowedTenantIds deduped case-insensitively (old HashSet-filter semantics), and ids missed by the exact-case point-read get one scan+filter rescue pass — config PartitionKey casing is not guaranteed (agent ingest auto-creates rows verbatim; same caveat the delegated sessions fan-out handles case-insensitively). Functions suite 2548/2548 (9 new tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-10-25 05:00:07 +00:00 · 2026-07-03 12:00:16 +02:00
parent bae97c2486
commit ac72ebb420
4 changed files with 221 additions and 26 deletions
@@ -24,10 +24,10 @@ public class GetAllTenantConfigurationsFunctionTests
    [Fact]
    public void ExistingManagedConfigs_DropsTenantsWithoutConfigRow()
    {
-        var reads = new (TenantConfiguration config, bool exists)[]
+        var reads = new TenantConfiguration?[]
        {
-            (new TenantConfiguration { TenantId = TenantB }, true),
-            (new TenantConfiguration { TenantId = TenantC }, false), // no config row → must be dropped
+            new TenantConfiguration { TenantId = TenantB },
+            null, // no config row (404) → must be dropped
        };

        var result = GetAllTenantConfigurationsFunction.ExistingManagedConfigs(reads);
@@ -39,10 +39,10 @@ public class GetAllTenantConfigurationsFunctionTests
    [Fact]
    public void ExistingManagedConfigs_AllExist_KeepsAll()
    {
-        var reads = new (TenantConfiguration config, bool exists)[]
+        var reads = new TenantConfiguration?[]
        {
-            (new TenantConfiguration { TenantId = TenantB }, true),
-            (new TenantConfiguration { TenantId = TenantC }, true),
+            new TenantConfiguration { TenantId = TenantB },
+            new TenantConfiguration { TenantId = TenantC },
        };

        var result = GetAllTenantConfigurationsFunction.ExistingManagedConfigs(reads);
@@ -52,13 +52,54 @@ public class GetAllTenantConfigurationsFunctionTests

    [Fact]
    public void ExistingManagedConfigs_NoneExist_Empty()
+        => Assert.Empty(GetAllTenantConfigurationsFunction.ExistingManagedConfigs(new TenantConfiguration?[] { null }));
+
+    [Fact]
+    public void MergeRescuedConfigs_AddsCaseVariantRow_MissedByExactCasePointRead()
    {
-        var reads = new (TenantConfiguration config, bool exists)[]
+        // AllowedTenantIds is lowercased; the stored config row has an upper-case PartitionKey,
+        // so the exact-case point-read missed it. The scan rescue must restore it.
+        var managedIds = new[] { TenantB, TenantC };
+        var hits = new List<TenantConfiguration> { new() { TenantId = TenantB } };
+        var scanned = new[]
        {
-            (new TenantConfiguration { TenantId = TenantB }, false),
+            new TenantConfiguration { TenantId = TenantB },
+            new TenantConfiguration { TenantId = TenantC.ToUpperInvariant() }, // case-variant row
        };

-        Assert.Empty(GetAllTenantConfigurationsFunction.ExistingManagedConfigs(reads));
+        var merged = GetAllTenantConfigurationsFunction.MergeRescuedConfigs(managedIds, hits, scanned);
+
+        Assert.Equal(2, merged.Count);
+        Assert.Contains(merged, c => c.TenantId == TenantC.ToUpperInvariant());
+    }
+
+    [Fact]
+    public void MergeRescuedConfigs_NeverAddsUnmanagedTenant()
+    {
+        var managedIds = new[] { TenantB, TenantC };
+        var hits = new List<TenantConfiguration> { new() { TenantId = TenantB } };
+        var scanned = new[]
+        {
+            new TenantConfiguration { TenantId = TenantB },
+            new TenantConfiguration { TenantId = "44444444-4444-4444-4444-444444444444" }, // NOT managed
+        };
+
+        var merged = GetAllTenantConfigurationsFunction.MergeRescuedConfigs(managedIds, hits, scanned);
+
+        var only = Assert.Single(merged);
+        Assert.Equal(TenantB, only.TenantId);
+    }
+
+    [Fact]
+    public void MergeRescuedConfigs_DoesNotDuplicatePointReadHits()
+    {
+        var managedIds = new[] { TenantB };
+        var hits = new List<TenantConfiguration> { new() { TenantId = TenantB } };
+        var scanned = new[] { new TenantConfiguration { TenantId = TenantB.ToUpperInvariant() } };
+
+        var merged = GetAllTenantConfigurationsFunction.MergeRescuedConfigs(managedIds, hits, scanned);
+
+        Assert.Single(merged); // scan row is the same tenant (case-insensitive) → not added twice
    }

    [Fact]
@@ -0,0 +1,103 @@
+using System;
+using System.Threading.Tasks;
+using AutopilotMonitor.Functions.Services;
+using AutopilotMonitor.Shared.DataAccess;
+using AutopilotMonitor.Shared.Models;
+using Microsoft.Extensions.Caching.Memory;
+using Microsoft.Extensions.Logging;
+using Moq;
+using Xunit;
+
+namespace AutopilotMonitor.Functions.Tests;
+
+/// <summary>
+/// Pins the two point-read semantics of <see cref="TenantConfigurationService"/>:
+/// <see cref="TenantConfigurationService.GetConfigurationIfExistsAsync"/> PROPAGATES storage
+/// failures (a caller like the delegated config/all subset must fail the request rather than
+/// silently drop a tenant), while <see cref="TenantConfigurationService.TryGetConfigurationAsync"/>
+/// maps them to exists=false (agent security gates fail closed). Both are non-creating.
+/// </summary>
+public sealed class TenantConfigurationServiceReadSemanticsTests
+{
+    private const string TenantId = "11111111-1111-1111-1111-111111111111";
+
+    private static (TenantConfigurationService service, Mock<IConfigRepository> repo) Build()
+    {
+        var repo = new Mock<IConfigRepository>(MockBehavior.Strict);
+        var service = new TenantConfigurationService(
+            repo.Object,
+            Mock.Of<ILogger<TenantConfigurationService>>(),
+            new MemoryCache(new MemoryCacheOptions()));
+        return (service, repo);
+    }
+
+    [Fact]
+    public async Task GetConfigurationIfExistsAsync_StorageError_Propagates()
+    {
+        var (service, repo) = Build();
+        repo.Setup(r => r.GetTenantConfigurationAsync(TenantId))
+            .ThrowsAsync(new InvalidOperationException("throttled"));
+
+        await Assert.ThrowsAsync<InvalidOperationException>(
+            () => service.GetConfigurationIfExistsAsync(TenantId));
+    }
+
+    [Fact]
+    public async Task GetConfigurationIfExistsAsync_NoRow_ReturnsNull_NeverCreates()
+    {
+        var (service, repo) = Build();
+        repo.Setup(r => r.GetTenantConfigurationAsync(TenantId))
+            .ReturnsAsync((TenantConfiguration?)null);
+
+        Assert.Null(await service.GetConfigurationIfExistsAsync(TenantId));
+        // MockBehavior.Strict: any SaveTenantConfigurationAsync call would have thrown.
+    }
+
+    [Fact]
+    public async Task GetConfigurationIfExistsAsync_RowFound_ReturnsAndCaches()
+    {
+        var (service, repo) = Build();
+        repo.Setup(r => r.GetTenantConfigurationAsync(TenantId))
+            .ReturnsAsync(new TenantConfiguration { TenantId = TenantId });
+
+        var first = await service.GetConfigurationIfExistsAsync(TenantId);
+        var second = await service.GetConfigurationIfExistsAsync(TenantId);
+
+        Assert.Equal(TenantId, first!.TenantId);
+        Assert.Same(first, second);
+        repo.Verify(r => r.GetTenantConfigurationAsync(TenantId), Times.Once);
+    }
+
+    [Fact]
+    public async Task GetConfigurationIfExistsAsync_EmptyTenantId_ReturnsNull()
+    {
+        var (service, _) = Build();
+        Assert.Null(await service.GetConfigurationIfExistsAsync(""));
+    }
+
+    [Fact]
+    public async Task TryGetConfigurationAsync_StorageError_FailsSafe_ExistsFalse()
+    {
+        var (service, repo) = Build();
+        repo.Setup(r => r.GetTenantConfigurationAsync(TenantId))
+            .ThrowsAsync(new InvalidOperationException("throttled"));
+
+        var (config, exists) = await service.TryGetConfigurationAsync(TenantId);
+
+        Assert.False(exists);
+        Assert.Equal(TenantId, config.TenantId); // fail-safe default, not a stored row
+    }
+
+    [Fact]
+    public async Task TryGetConfigurationAsync_RowFound_ExistsTrue()
+    {
+        var (service, repo) = Build();
+        repo.Setup(r => r.GetTenantConfigurationAsync(TenantId))
+            .ReturnsAsync(new TenantConfiguration { TenantId = TenantId });
+
+        var (config, exists) = await service.TryGetConfigurationAsync(TenantId);
+
+        Assert.True(exists);
+        Assert.Equal(TenantId, config.TenantId);
+    }
+}
@@ -56,11 +56,26 @@ namespace AutopilotMonitor.Functions.Functions.Config
                {
                    // READ-bounded, not just response-bounded: point-read ONLY the caller's managed tenants
                    // instead of scanning every tenant config and filtering in memory. A delegated MSP scoped
-                    // to k tenants triggers k point reads (cached, non-creating — TryGetConfigurationAsync
-                    // never writes a default row). exists==false drops any id with no config row.
-                    var reads = await Task.WhenAll(requestCtx.AllowedTenantIds.Select(async tid =>
-                        await _configService.TryGetConfigurationAsync(tid)));
+                    // to k tenants triggers k point reads (cached, non-creating — GetConfigurationIfExistsAsync
+                    // never writes a default row). Only a true "no config row" (404) drops an id; a storage
+                    // READ FAILURE propagates to the outer catch → 500, so a transient error can never
+                    // silently drop a managed tenant from a success response. Ids are deduped case-
+                    // insensitively (the old scan+filter semantics) so a duplicated grant list cannot
+                    // produce duplicate config entries.
+                    var managedIds = requestCtx.AllowedTenantIds
+                        .Distinct(StringComparer.OrdinalIgnoreCase)
+                        .ToList();
+                    var reads = await Task.WhenAll(managedIds.Select(tid =>
+                        _configService.GetConfigurationIfExistsAsync(tid)));
                    var subset = ExistingManagedConfigs(reads);
+
+                    // AllowedTenantIds is lowercased while the config PartitionKey casing is not guaranteed
+                    // (agent paths auto-create rows with the caller-supplied id verbatim) — the same caveat
+                    // the delegated sessions fan-out handles case-insensitively. A point-read is exact-case,
+                    // so any id it missed gets ONE scan+filter rescue pass (the pre-point-read behavior).
+                    // Only stale grants or case-variant rows pay for it; the common path stays read-bounded.
+                    if (subset.Count < managedIds.Count)
+                        subset = MergeRescuedConfigs(managedIds, subset, await _configService.GetAllConfigurationsAsync());
                    _logger.LogInformation("GetAllTenantConfigurations (delegated subset, {Count} tenants) by {User}",
                        subset.Count, userIdentifier);

@@ -153,13 +168,32 @@ namespace AutopilotMonitor.Functions.Functions.Config

        /// <summary>
        /// From the per-tenant point-read results, keep only the managed tenants that actually HAVE a config
-        /// row (exists). A delegated caller's AllowedTenantIds is the authoritative bound; an id with no row
+        /// row (non-null). A delegated caller's AllowedTenantIds is the authoritative bound; an id with no row
        /// (offboarded / never onboarded) is silently dropped rather than surfaced as an empty default.
+        /// Storage errors never reach this seam — GetConfigurationIfExistsAsync throws, failing the request.
        /// Pure + testable seam (handler HTTP entry is not unit-tested — see GetAllBlockedDevicesFunctionTests).
        /// </summary>
        internal static List<TenantConfiguration> ExistingManagedConfigs(
-            IEnumerable<(TenantConfiguration config, bool exists)> reads)
-            => reads.Where(r => r.exists).Select(r => r.config).ToList();
+            IEnumerable<TenantConfiguration?> reads)
+            => reads.Where(c => c != null).Select(c => c!).ToList();
+
+        /// <summary>
+        /// Case-variant rescue for managed ids the exact-case point-reads missed: from the full config scan,
+        /// add every config whose TenantId matches a managed id case-insensitively and is not already in the
+        /// point-read hits. The managed-id bound stays authoritative — an unmanaged config is never added, so
+        /// this can only ever RESTORE tenants the pre-point-read scan+filter would have returned. Pure + testable.
+        /// </summary>
+        internal static List<TenantConfiguration> MergeRescuedConfigs(
+            IReadOnlyCollection<string> managedIds,
+            IReadOnlyCollection<TenantConfiguration> pointReadHits,
+            IEnumerable<TenantConfiguration> allConfigs)
+        {
+            var managed = new HashSet<string>(managedIds, StringComparer.OrdinalIgnoreCase);
+            var found = new HashSet<string>(pointReadHits.Select(c => c.TenantId), StringComparer.OrdinalIgnoreCase);
+            var merged = new List<TenantConfiguration>(pointReadHits);
+            merged.AddRange(allConfigs.Where(c => managed.Contains(c.TenantId) && !found.Contains(c.TenantId)));
+            return merged;
+        }

        /// <summary>
        /// The delegated bare-array view of config/all: every managed-tenant config with its secrets
@@ -121,28 +121,45 @@ namespace AutopilotMonitor.Functions.Services
            _cache.Remove($"tenant-config:{tenantId}");
        }

+        /// <summary>
+        /// Strict point-read: returns the config, or null when no row exists (404) — does NOT
+        /// auto-create. Any other storage failure PROPAGATES to the caller, so "read failed" can
+        /// never be conflated with "tenant not configured". Use where a dropped tenant is worse
+        /// than a failed request (e.g. the delegated config/all subset).
+        /// </summary>
+        public virtual async Task<TenantConfiguration?> GetConfigurationIfExistsAsync(string tenantId)
+        {
+            if (string.IsNullOrEmpty(tenantId))
+                return null;
+
+            var cacheKey = $"tenant-config:{tenantId}";
+
+            if (_cache.TryGetValue(cacheKey, out TenantConfiguration? cachedConfig) && cachedConfig != null)
+                return cachedConfig;
+
+            var config = await _configRepo.GetTenantConfigurationAsync(tenantId);
+            if (config != null)
+                _cache.Set(cacheKey, config, CacheDuration);
+
+            return config;
+        }
+
        /// <summary>
        /// Returns (config, exists). exists=false when no row was found — does NOT auto-create.
-        /// Use for agent security gates where unknown tenants must be rejected.
+        /// Use for agent security gates where unknown tenants must be rejected: storage errors are
+        /// mapped to exists=false (fail closed). Callers that must instead surface read failures
+        /// use <see cref="GetConfigurationIfExistsAsync"/>.
        /// </summary>
        public virtual async Task<(TenantConfiguration config, bool exists)> TryGetConfigurationAsync(string tenantId)
        {
            if (string.IsNullOrEmpty(tenantId))
                return (TenantConfiguration.CreateDefault("unknown"), false);

-            var cacheKey = $"tenant-config:{tenantId}";
-
-            if (_cache.TryGetValue(cacheKey, out TenantConfiguration? cachedConfig) && cachedConfig != null)
-                return (cachedConfig, true);
-
            try
            {
-                var config = await _configRepo.GetTenantConfigurationAsync(tenantId);
+                var config = await GetConfigurationIfExistsAsync(tenantId);
                if (config != null)
-                {
-                    _cache.Set(cacheKey, config, CacheDuration);
                    return (config, true);
-                }

                return (TenantConfiguration.CreateDefault(tenantId), false);
            }