AddAsync/GetAsync/UpdateExpiryAsync throw on a DateTimeOffset expiry at or near now

[imperugo]

Found while reviewing #664. Pre-existing on master, not introduced by that PR.

Symptom

AddAsyncDateTimeOffset_WithTags_CorrectTaggedKey_Async passes as part of the full suite but fails when run in isolation:

dotnet test -f net10.0                      # 1754/1754 pass
dotnet test -f net10.0 --filter "FullyQualifiedName~AddAsyncDateTimeOffset_WithTags_CorrectTaggedKey_Async"
# 6 of 7 serializer variants fail, reproducible across runs

Only the first serializer variant passes; the rest fail consistently, which points at state left behind by whichever test ran before rather than at timing.

Why it matters

Test parallelization is disabled and all tests share one Redis instance, so this kind of coupling stays invisible until someone runs a filtered subset — exactly what you do when debugging a single failure. It also means a green full-suite run is not proof that the test actually asserts what it claims.

Likely cause

The tag Set key is probably not fully cleaned between variants, so the assertion passes on residue from a previous test rather than on what the test itself wrote. Worth checking the FlushDbAsync placement in CacheClientTestBase against the tag-related keys.

Acceptance criteria

• The test passes both in the full suite and when run alone with --filter
• Any sibling tag tests with the same coupling are fixed alongside it

[imperugo]

Correcting myself: the diagnosis in the description above is wrong, and the real cause is a library bug rather than a test-isolation problem. I chased it down after the flakiness started showing up in full-suite runs on master, not just under --filter.

Dispose already flushes between tests (db.FlushDatabase() in CacheClientTestBase), and xUnit builds a fresh instance per test, so there is no leftover tag key. That part of my earlier write-up was simply wrong.

What actually happens

Captured from a failing run:

System.ArgumentOutOfRangeException : Specified argument was out of the range of valid values. (Parameter 'value')
   at StackExchange.Redis.Expiration.<Init>g__Throw|11_0() in /_/src/StackExchange.Redis/Expiration.cs:line 101

The test asks for an expiry of now:

// CacheClientTestBase.Tags.cs:133
await Sut.GetDefaultDatabase().AddAsync(testKey, testClass, DateTimeOffset.UtcNow, tags: [testTag]);

and AddAsync serializes before it converts that into a relative TTL:

var entryBytes = value.OfValueSize(Serializer, maxValueLength, key);   // takes time
var expiration = expiresAt.UtcDateTime.Subtract(DateTime.UtcNow);      // ...so this goes negative

Whatever serialization costs lands between the caller's clock reading and ours, so expiration is always slightly negative. When the delta rounds to zero the call succeeds; when it exceeds a millisecond, the current StackExchange.Redis validates the expiry client-side and throws before anything reaches the server. That is the whole flakiness: warm JIT passes, cold JIT or a GC pause fails. It also explains the earlier observation that --filter failed 6 of 7 serializer variants while the full suite mostly passed — in isolation every serializer runs cold.

Worth noting the throw is client-side. Redis itself tolerates this: inside MULTI/EXEC a SET ... PX -5 errors while the sibling SADD still commits. So this behaviour arrived with the SE.Redis version bump rather than from Redis.

The bug is wider than these two tests

Five call sites derive a TTL from a DateTimeOffset, and only one checks the result:

Location	Method	Guarded?
RedisDatabase.cs:156	AddAsync<T>(key, value, DateTimeOffset, ...)	no
RedisDatabase.cs:117	GetAsync<T>(key, DateTimeOffset, ...)	no
PubSub.cs:90	UpdateExpiryAsync(key, DateTimeOffset, ...)	no
PubSub.cs:103	UpdateExpiryAllAsync(keys, DateTimeOffset, ...)	no
RedisDatabase.cs:246	AddAllAsync<T>(items, DateTimeOffset, ...)	yes — if (expiration <= TimeSpan.Zero) return false;

So AddAllAsync returns false for an already-past expiry while AddAsync throws for the same input. Beyond the inconsistency, this is reachable in ordinary use: computing an expiry a few milliseconds ahead and then hitting a slow serializer, a GC pause, or a loaded machine is enough to flip it negative.

What to decide

The behaviour for a non-positive computed TTL needs to be one thing across all five, and I don't think throwing is it — the caller passed a legitimate DateTimeOffset and lost the race to their own serialization cost.

1. Mirror AddAllAsync — return false / no-op when expiration <= TimeSpan.Zero. Consistent, non-breaking, smallest change.
2. Treat it as "already expired" — delete the key (or skip the write) and report success. Arguably the truest reading of "expires at a time in the past", but more moving parts.
3. Compute the TTL before serializing — narrows the window without closing it. Worth doing regardless of which of the above wins, since the caller's intent is anchored to their clock reading, not to ours after an arbitrary amount of work.

My preference is 1 plus 3. Whatever we pick, the two tag tests should stop passing DateTimeOffset.UtcNow and use an explicit future offset, so they stop encoding a race as an assertion.

Keeping this on v13.5: options 1 and 3 are behaviour fixes on a path that currently throws, so nothing that works today would break.