Skip to content

Commit 94c8cab

Browse files
cemililikclaude
andcommitted
fix(kernel): keep ADR-0037's promise and prove the reset case
Round 2 of Step 11's review. No blockers; two long-standing promises the packet was the right place to settle. ADR-0037 said of the idempotency port's raw Guid tenant: "The strongly-typed TenantId lands with the tenancy schema in Packet 6, and both move together." They did not. Packet 6 typed ITenantContext.TenantId and left IIdempotencyStore on a raw Guid, with IdempotentAttribute carrying a comment naming the seam it crossed at a single call site — and nothing recorded the divergence, which is how a promise like that gets discovered three packets later by someone who trusted it. The three methods, the internal key and the census now take TenantId, the unwrapping site is gone, and Amendment 4 records it. The guard changed shape with the type, not just its signature. It refused Guid.Empty; a typed id has two ways to be unassigned, and reading .Value on the first throws from inside the id type. It now tests IsInitialized() before both sentinels, matching AuditInput.EnsureValid and the unit of work's setter, and its test drives the unassigned value from an array element because default(TenantId) does not compile. The Phase Exit Decision names a case that reads with app.tenant_id RESET rather than merely unset, and nothing asserted it. The distinction is the whole reason the policies use NULLIF: a never-set GUC reads NULL, a reset one reads the empty string, and ''::uuid raises 22P02 — so without NULLIF a reused connection would error rather than filter. Measured: removing NULLIF from the nineteen policy predicates turns the new case red with exactly that 22P02. Module: Tenancy ADR: 0037, 0003 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent f2d4371 commit 94c8cab

7 files changed

Lines changed: 155 additions & 32 deletions

File tree

‎backend/src/LearnStack.Api/Idempotency/IdempotentAttribute.cs‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System.Text;
55
using LearnStack.Api.Common;
66
using LearnStack.SharedKernel.Idempotency;
7+
using LearnStack.SharedKernel.Identifiers;
78
using LearnStack.SharedKernel.Localization;
89
using LearnStack.SharedKernel.Results;
910
using LearnStack.SharedKernel.Tenancy;
@@ -197,11 +198,11 @@ [new LocalizedMessage("lockey_idempotency_key_invalid")],
197198
return;
198199
}
199200

200-
// Value once, here: IIdempotencyStore's key space is (Guid, string) and
201-
// that port is not part of this conversion, so the seam is crossed at a
202-
// single site rather than at each of its five call sites. Safe — the
203-
// IsResolved gate above has already returned.
204-
var tenantId = tenantContext.TenantId.Value;
201+
// No unwrapping: the port's key space is (TenantId, string) as of Packet 7, which
202+
// is what ADR-0037 § What we punted on promised when it said the raw Guid and
203+
// ITenantContext.TenantId "both move together". The IsResolved gate above has
204+
// already returned, so this id is a real one.
205+
var tenantId = tenantContext.TenantId;
205206
var cancellationToken = context.HttpContext.RequestAborted;
206207
var fingerprint = await ComputeFingerprintAsync(context.HttpContext, cancellationToken)
207208
.ConfigureAwait(false);
@@ -266,7 +267,7 @@ await RunAndRecordAsync(context, next, tenantId, key, claim.Token, cancellationT
266267
private async Task RunAndRecordAsync(
267268
ResourceExecutingContext context,
268269
ResourceExecutionDelegate next,
269-
Guid tenantId,
270+
TenantId tenantId,
270271
string key,
271272
Guid token,
272273
CancellationToken cancellationToken)
@@ -387,7 +388,7 @@ private Outcome Classify(ResourceExecutedContext executed, HttpResponse response
387388
return Outcome.Record;
388389
}
389390

390-
private Task<bool> AbandonAsync(Guid tenantId, string key, Guid token) =>
391+
private Task<bool> AbandonAsync(TenantId tenantId, string key, Guid token) =>
391392
store.AbandonAsync(tenantId, key, token, CancellationToken.None);
392393

393394
/// <summary>

‎backend/src/LearnStack.Infrastructure/Idempotency/InMemoryIdempotencyStore.cs‎

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using LearnStack.SharedKernel.Identifiers;
12
using System.Collections.Concurrent;
23
using LearnStack.SharedKernel.Idempotency;
34
using LearnStack.SharedKernel.Time;
@@ -79,13 +80,13 @@ public sealed class InMemoryIdempotencyStore(IClock clock) : IIdempotencyStore
7980
/// </summary>
8081
public const int MaxEntriesPerTenant = 1_000;
8182

82-
private readonly ConcurrentDictionary<(Guid Tenant, string Key), Entry> _entries = new();
83+
private readonly ConcurrentDictionary<(TenantId Tenant, string Key), Entry> _entries = new();
8384

8485
private long _lastSweepTicks;
8586
private Census _census = Census.Empty;
8687

8788
public Task<IdempotencyClaimResult> TryClaimAsync(
88-
Guid tenantId, string key, string fingerprint, CancellationToken cancellationToken)
89+
TenantId tenantId, string key, string fingerprint, CancellationToken cancellationToken)
8990
{
9091
Guard(tenantId, key);
9192
ArgumentNullException.ThrowIfNull(fingerprint);
@@ -141,7 +142,7 @@ public Task<IdempotencyClaimResult> TryClaimAsync(
141142
}
142143

143144
public Task<bool> CompleteAsync(
144-
Guid tenantId,
145+
TenantId tenantId,
145146
string key,
146147
Guid token,
147148
IdempotentResponse? response,
@@ -176,7 +177,7 @@ current with
176177
}
177178

178179
public Task<bool> AbandonAsync(
179-
Guid tenantId, string key, Guid token, CancellationToken cancellationToken)
180+
TenantId tenantId, string key, Guid token, CancellationToken cancellationToken)
180181
{
181182
Guard(tenantId, key);
182183

@@ -186,23 +187,29 @@ public Task<bool> AbandonAsync(
186187
if (_entries.TryGetValue((tenantId, key), out var current) && current.Token == token)
187188
{
188189
return Task.FromResult(
189-
_entries.TryRemove(new KeyValuePair<(Guid, string), Entry>((tenantId, key), current)));
190+
_entries.TryRemove(new KeyValuePair<(TenantId, string), Entry>((tenantId, key), current)));
190191
}
191192

192193
return Task.FromResult(false);
193194
}
194195

195-
private static void Guard(Guid tenantId, string key)
196+
private static void Guard(TenantId tenantId, string key)
196197
{
197198
ArgumentException.ThrowIfNullOrWhiteSpace(key);
198199

199-
// The tenant is the key space. An empty one is not a tenant — it is a
200-
// call site that forgot to scope, and accepting it would build exactly
201-
// the flat space this store's contract exists to prevent.
202-
if (tenantId == Guid.Empty)
200+
// The tenant is the key space. An unassigned one is not a tenant — it is a call
201+
// site that forgot to scope, and accepting it would build exactly the flat space
202+
// this store's contract exists to prevent.
203+
//
204+
// IsInitialized() first: reading Value on an unset Vogen id throws from inside the
205+
// id type, which is neither this guard's contract nor a message a caller can act
206+
// on. Both sentinels are refused, because a struct nobody assigned and one
207+
// assigned the all-zero Guid are the same mistake.
208+
if (!tenantId.IsInitialized() || tenantId.Value == Guid.Empty)
203209
{
204210
throw new ArgumentException(
205-
"An idempotency key is scoped to a tenant; Guid.Empty is not one.", nameof(tenantId));
211+
"An idempotency key is scoped to a tenant; an unassigned id is not one.",
212+
nameof(tenantId));
206213
}
207214
}
208215

@@ -238,7 +245,7 @@ private void Sweep(DateTimeOffset now)
238245
return;
239246
}
240247

241-
var counts = new Dictionary<Guid, int>();
248+
var counts = new Dictionary<TenantId, int>();
242249
var total = 0;
243250

244251
foreach (var pair in _entries)
@@ -274,11 +281,11 @@ private void Sweep(DateTimeOffset now)
274281
/// claims — a soft ceiling, which is what a ceiling that must never evict a
275282
/// live record has to be.
276283
/// </remarks>
277-
private sealed record Census(IReadOnlyDictionary<Guid, int> PerTenant, int Total)
284+
private sealed record Census(IReadOnlyDictionary<TenantId, int> PerTenant, int Total)
278285
{
279-
public static readonly Census Empty = new(new Dictionary<Guid, int>(), 0);
286+
public static readonly Census Empty = new(new Dictionary<TenantId, int>(), 0);
280287

281-
public bool IsFull(Guid tenantId) =>
288+
public bool IsFull(TenantId tenantId) =>
282289
Total >= MaxEntries || PerTenant.GetValueOrDefault(tenantId) >= MaxEntriesPerTenant;
283290
}
284291

‎backend/src/LearnStack.SharedKernel/Idempotency/IIdempotencyStore.cs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using LearnStack.SharedKernel.Identifiers;
12
namespace LearnStack.SharedKernel.Idempotency;
23

34
/// <summary>
@@ -121,7 +122,7 @@ public interface IIdempotencyStore
121122
/// </param>
122123
/// <param name="cancellationToken">Cancellation.</param>
123124
Task<IdempotencyClaimResult> TryClaimAsync(
124-
Guid tenantId,
125+
TenantId tenantId,
125126
string key,
126127
string fingerprint,
127128
CancellationToken cancellationToken);
@@ -144,7 +145,7 @@ Task<IdempotencyClaimResult> TryClaimAsync(
144145
/// silence.
145146
/// </returns>
146147
Task<bool> CompleteAsync(
147-
Guid tenantId,
148+
TenantId tenantId,
148149
string key,
149150
Guid token,
150151
IdempotentResponse? response,
@@ -162,5 +163,5 @@ Task<bool> CompleteAsync(
162163
/// </remarks>
163164
/// <returns><c>true</c> when the key was released by this caller.</returns>
164165
Task<bool> AbandonAsync(
165-
Guid tenantId, string key, Guid token, CancellationToken cancellationToken);
166+
TenantId tenantId, string key, Guid token, CancellationToken cancellationToken);
166167
}

‎backend/tests/LearnStack.Tests.Integration/Database/TenantIsolationHttpTests.cs‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,11 @@ INSERT INTO tenant_settings
343343
}
344344

345345
/// <summary>The registry-assigned actor; every seeded row is attributed to it.</summary>
346-
private static readonly Guid Actor = Guid.Parse("00000000-0000-7000-8000-000000000001");
346+
/// <remarks>
347+
/// Read from <see cref="UserId.SystemActor"/> rather than repeating the literal. The
348+
/// value already has a home, and a second copy is a second thing to keep true.
349+
/// </remarks>
350+
private static readonly Guid Actor = UserId.SystemActor.Value;
347351

348352
/// <summary>A client whose requests arrive on <paramref name="host"/>.</summary>
349353
/// <remarks>
@@ -437,6 +441,12 @@ public sealed record ForeignWriteRequest(Guid TenantId);
437441
}
438442

439443
/// <summary>What the probe reads.</summary>
444+
/// <remarks>
445+
/// The unresolved-context read is a separate request type rather than a third member here,
446+
/// and the asymmetry is deliberate: what distinguishes it is not which rows it wants but
447+
/// which marker it carries, and a marker is a property of the type. Folding it in would
448+
/// mean one request type wearing two ceilings.
449+
/// </remarks>
440450
public enum ProbeSubject
441451
{
442452
Organizations,
@@ -558,8 +568,7 @@ INSERT INTO tenant_settings
558568
VALUES (uuidv7(), @tenant, NULL, 'smuggled', '"x"', now(), @actor, 0)
559569
""";
560570
command.Parameters.AddWithValue("tenant", request.TenantId);
561-
command.Parameters.AddWithValue(
562-
"actor", Guid.Parse("00000000-0000-7000-8000-000000000001"));
571+
command.Parameters.AddWithValue("actor", UserId.SystemActor.Value);
563572

564573
try
565574
{

‎backend/tests/LearnStack.Tests.Integration/Database/UnitOfWorkTests.cs‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using System.Data.Common;
12
using FluentAssertions;
23
using LearnStack.Api.Composition;
34
using LearnStack.Application.Pipeline;
@@ -946,6 +947,63 @@ await unitOfWork.SetTenantContextAsync(
946947
}
947948
}
948949

950+
[Fact]
951+
public async Task A_reset_tenant_context_reads_nothing_rather_than_everything()
952+
{
953+
// The case the Phase Exit Decision names: a read with `app.tenant_id` RESET
954+
// rather than merely unset. The distinction is the whole reason the policies are
955+
// written with NULLIF.
956+
//
957+
// Never-set and reset are different values. `current_setting(name, true)` returns
958+
// NULL for a variable that was never set, and the EMPTY STRING for one that was
959+
// set and then reset — which is the state a pooled connection is in after
960+
// DISCARD ALL, and the state every request leaves behind. `''::uuid` raises
961+
// 22P02, so a policy that cast without NULLIF would not filter: it would error
962+
// on the first read of every reused connection. With NULLIF the empty string
963+
// becomes NULL, the predicate is NULL, and NULL is false for both USING and
964+
// WITH CHECK — the table returns nothing.
965+
//
966+
// Driven on one physical connection, because that is the only way the two states
967+
// are distinguishable: set a real tenant, observe rows, reset, observe none.
968+
var builder = new NpgsqlDataSourceBuilder(_schema.Postgres.AppConnectionString);
969+
builder.ConnectionStringBuilder.MaxPoolSize = 1;
970+
builder.ConnectionStringBuilder.NoResetOnClose = true;
971+
await using var dataSource = builder.Build();
972+
973+
await using var connection = await dataSource.OpenConnectionAsync();
974+
975+
await ExecuteOnAsync(connection,
976+
$"SELECT set_config('app.tenant_id', '{SchemaFixture.TenantA}', false)");
977+
978+
(await ScalarOnAsync(connection, "SELECT count(*) FROM organizations"))
979+
.Should().NotBe("0", "the tenant is set, so its rows are visible");
980+
981+
// RESET, not "never set" — the variable exists and now holds ''.
982+
await ExecuteOnAsync(connection, "RESET app.tenant_id");
983+
984+
(await ScalarOnAsync(connection, "SELECT current_setting('app.tenant_id', true)"))
985+
.Should().BeEmpty(
986+
"a reset GUC reads back as the empty string, which is exactly the value "
987+
+ "NULLIF exists to turn into NULL");
988+
989+
(await ScalarOnAsync(connection, "SELECT count(*) FROM organizations"))
990+
.Should().Be("0",
991+
"and the read fails closed — it returns nothing rather than everything, "
992+
+ "and does not raise 22P02 on the way");
993+
}
994+
995+
private static async Task ExecuteOnAsync(DbConnection connection, string sql)
996+
{
997+
await using var command = new NpgsqlCommand(sql, (NpgsqlConnection)connection);
998+
await command.ExecuteNonQueryAsync();
999+
}
1000+
1001+
private static async Task<string?> ScalarOnAsync(DbConnection connection, string sql)
1002+
{
1003+
await using var command = new NpgsqlCommand(sql, (NpgsqlConnection)connection);
1004+
return (await command.ExecuteScalarAsync())?.ToString();
1005+
}
1006+
9491007
// ── Helpers ──────────────────────────────────────────────────────────────
9501008

9511009
private ServiceProvider BuildProvider(NpgsqlDataSource? dataSource = null)

‎backend/tests/LearnStack.Tests.Unit/Infrastructure/Idempotency/InMemoryIdempotencyStoreTests.cs‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
using FluentAssertions;
22
using LearnStack.Infrastructure.Idempotency;
33
using LearnStack.SharedKernel.Idempotency;
4+
using LearnStack.SharedKernel.Identifiers;
45
using LearnStack.SharedKernel.Time;
56
using Xunit;
67

@@ -22,8 +23,14 @@ public sealed class InMemoryIdempotencyStoreTests
2223
private static readonly DateTimeOffset Origin =
2324
new(2026, 8, 20, 9, 0, 0, TimeSpan.Zero);
2425

25-
private static readonly Guid Tenant = Guid.Parse("018f4d40-0000-7000-8000-00000000000a");
26-
private static readonly Guid OtherTenant = Guid.Parse("018f4d40-0000-7000-8000-00000000000b");
26+
// Typed, as of Packet 7: the port's key space is (TenantId, string), which is what
27+
// ADR-0037 promised when it said the raw Guid and ITenantContext.TenantId "both move
28+
// together". A raw Guid no longer compiles here, which is the point of the change.
29+
private static readonly TenantId Tenant =
30+
TenantId.From(Guid.Parse("018f4d40-0000-7000-8000-00000000000a"));
31+
32+
private static readonly TenantId OtherTenant =
33+
TenantId.From(Guid.Parse("018f4d40-0000-7000-8000-00000000000b"));
2734

2835
private const string Key = "01HXIDEMPOTENT0001";
2936
private const string Fingerprint = "fingerprint-a";
@@ -94,10 +101,19 @@ public async Task An_Unscoped_Tenant_Is_Refused()
94101
{
95102
var (store, _) = NewStore();
96103

97-
var claim = async () => await store.TryClaimAsync(Guid.Empty, Key, Fingerprint, default);
104+
// default(TenantId) does not compile — Vogen's VOG009 analyzer prohibits it — so
105+
// the unassigned value comes from an array element, which the analyzer cannot see
106+
// and the runtime leaves zeroed. That is also how one reaches production: a struct
107+
// field nobody assigned, a default(T) in a generic, a deserializer that skipped a
108+
// member. Typing the port did not remove this guard's job; it changed the shape of
109+
// the sentinel it has to refuse.
110+
var slot = new TenantId[1];
111+
var unassigned = slot[0];
112+
113+
var claim = async () => await store.TryClaimAsync(unassigned, Key, Fingerprint, default);
98114

99115
await claim.Should().ThrowAsync<ArgumentException>(
100-
"Guid.Empty is not a tenant — it is a call site that forgot to scope");
116+
"an unassigned id is not a tenant — it is a call site that forgot to scope");
101117
}
102118

103119
// ---- fingerprint -------------------------------------------------------

‎docs/decisions/0037-idempotency-key-contract.md‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -568,6 +568,37 @@ Neither changes what this ADR decides. `PostgresIdempotencyStore` does not exist
568568
yet — it is demand-gated on the trigger Amendment 1 names — so both land while the
569569
table has no rows and the constraint validates instantly.
570570

571+
### Amendment 4 — the port takes `TenantId`, one packet later than promised (2026-09-03)
572+
573+
§ What we punted on said of the raw `Guid` tenant: "The strongly-typed `TenantId`
574+
lands with the tenancy schema in Packet 6, **and both move together**." They did
575+
not. Packet 6 typed `ITenantContext.TenantId`; `IIdempotencyStore` kept the raw
576+
`Guid` through Packet 6 and most of Packet 7, and `IdempotentAttribute` carried a
577+
comment naming the seam it crossed at a single call site. Nothing recorded the
578+
divergence, which is how a promise like this is usually discovered — three packets
579+
later, by someone who trusted it.
580+
581+
**What ships.** `TryClaimAsync`, `CompleteAsync` and `AbandonAsync` take
582+
`TenantId`; the store's internal key becomes `(TenantId, string)` and its census
583+
`Dictionary<TenantId, int>`. The single unwrapping site in `IdempotentAttribute`
584+
is gone, so the port is now what [Standards 02](../standards/02-backend-coding.md)
585+
requires of a public surface rather than the exception the original text
586+
acknowledged.
587+
588+
**What changed in the guard, and why it is not cosmetic.** The refusal was
589+
`tenantId == Guid.Empty`. A typed id has two ways to be unassigned — a Vogen value
590+
nobody constructed, and one constructed from the all-zero `Guid` — and reading
591+
`.Value` on the first throws from inside the id type, which is neither this
592+
guard's contract nor a message a caller can act on. The guard now tests
593+
`IsInitialized()` first and both sentinels after, matching `AuditInput.EnsureValid`
594+
and `NpgsqlUnitOfWork`'s setter. Its test drives the unassigned value from an array
595+
element, because `default(TenantId)` does not compile — Vogen's VOG009 analyzer
596+
prohibits it — and an array slot is also how the value reaches production.
597+
598+
**What did not change.** The key space, the contract's states, the fingerprint
599+
rule, and Amendment 1's gating of the durable store. This is the type of one
600+
parameter, not a change to what the port does.
601+
571602
## References
572603

573604
- [ADR-0035: Demand-Gated Infrastructure](0035-demand-gated-infrastructure.md)

0 commit comments

Comments
 (0)