Skip to content

add credit leases, reservations, and preflight checks - #195

Open
bpapillon wants to merge 17 commits into
mainfrom
bpapillon/sch-7508-schematic-csharp-credit-leases-reservations-and-preflight
Open

bpapillon wants to merge 17 commits into
mainfrom
bpapillon/sch-7508-schematic-csharp-credit-leases-reservations-and-preflight

Conversation

@bpapillon

@bpapillon bpapillon commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator

Brings the C# SDK to parity with Node, Go, and Python on credit leases and reservations.

Check with a usage now reserves credits: in client mode against a lease held locally (in memory, or in Redis so a fleet shares one lease), in server mode with one check-and-reserve call. TrackWithReservation settles the reservation and release refunds an unused one. The plain check takes usage, event usage, and credit cost preflight options, threaded to both the API body and the local engine. Identify can prewarm a lease. Extend and check-and-reserve each send a fresh idempotency key per call and keep the default retry policy. Acquire sends none, as in the other SDKs: the API returns the slot's active lease on a repeat.

The Redis key layout and Lua scripts are Node's, byte for byte, so a mixed-language fleet shares leases. conformance/ is copied verbatim from schematic-node and the runner passes every vector on both backends. .NET has no in-process Redis, so the Redis backend runs against a fake that interprets the scripts, which is what the Node reference does too.

C#-specific choices: the check flow reads DataStream through a narrow ICheckDataStream, as Go does, rather than reaching into the client. The generated PreflightRequestBody doubles as the engine's options envelope since the shapes already match. EventBodyTrack.Quantity is a long, so a fractional settle rounds up as in Go. Prewarm takes company keys directly. Server mode maps the API entitlement field by field because the existing converter drops the consumption rate and event subtype.

One behaviour change outside the lease paths: a DataStream that throws on start is now logged and cleared instead of failing the constructor, so the client falls back to REST checks and auto mode resolves to server. This is what schematic-node does, and it is what keeps a bad base URL from taking down a client that REST can still answer for.

@bpapillon
bpapillon requested a review from a team as a code owner September 17, 2026 15:21
@bpapillon
bpapillon force-pushed the bpapillon/sch-7508-schematic-csharp-credit-leases-reservations-and-preflight branch from 7f14385 to 2c626ba Compare September 17, 2026 15:23
Prewarm without a datastream resolves nothing instead of throwing,
the reservation pins to the debited lease with no fallback, an empty
pin matches nothing in the in-memory store, and the Redis scripts are
pinned by hash because the fake backend cannot see drift in them.
The API's flag check takes a preflight now, so a check that falls back
to it answers the same hypothetical the local engine would. A
preflighted check skips the flag cache, which is keyed without it.
The conformance runner lists the vectors directory and fails on an
expect key it does not assert on, so a vector synced from node cannot
pass unrun. The SDK closes a Redis connection it opened itself and
never one the caller supplied. A zero-usage check stays cacheable.
…onfig

The datastream client folds an engine exception into a normal result,
so the lease check never cancelled the reservation on one. A
reservation is now sized from the rounded-up usage so it matches what
the settle bills. Also validates config, re-checks stop under the
lock, and writes a reservation hash and its expiry in one transaction.
A lease that lands after stop is left to the drain or server expiry,
as in node; releasing it refunded a lease sibling pods share. Hold
sizing and the settle debit go back to the spec's usage times rate.
Also builds the lease Redis options the way the cache does, keeps
reconnecting when Redis is down at startup, guards a null balance
payload, bounds quantities, and pins LF on the Lua-bearing files.
The interface said the in-memory store gets per-slot atomicity from a
per-slot lock; it takes one monitor over the whole table, as its own
class doc says.
Validate ran at the end of the constructor, after the event buffer's
flush loop and the datastream socket were already up, so a rejected
config leaked a thread and a websocket per client. It now sits with the
replicator-mode checks at the top, above everything the constructor
starts.
Extend, once a caller's join budget was spent, still handed back whoever
else's flight was registered, awaited with neither the caller's deadline
nor a check that it asked for enough. Both are the invariants the budget
exists to hold. Node's startExtend registers over the top instead, and
Go passes joinsLeft > 0 as its join flag, so the exhausted caller issues
its own extend; do the same.

MaybeExtendInBackgroundAsync read the stopped flag outside the gate, so a
Stop and its drain could snapshot an empty pending set between the check
and the registration, landing an extend after the close released the
lease. Acquire already re-checks under the gate, and Go's spawn does the
check and the registration under one mutex.

The datastream path dropped the caller's DefaultValue. The engine does
not throw when it declines; it answers with the client-wide flag default
under one of two reasons, so the fallback never ran and server mode
honoured an option client mode ignored. Node resolves it the same way
(wrapper.ts, "the engine declining to answer is the case defaultValue
exists for").

Also wraps the lease teardown so it cannot take the event buffer flush
and the socket close with it, and rejects a negative PrewarmResolveTimeout
rather than letting it collapse into the cache-only branch.
Covers the fix in ca77961 from both sides: the predicate that tells a
refusal to evaluate from a verdict, and an end-to-end check proving a
verdict the engine did reach still wins over the caller's default.

The refusal itself cannot be driven from a test here. The WASM engine is
built inside the datastream client with no seam to disable it, so it
initialises and answers; the lease path's equivalent predicate is what
LeaseCheckEngineFailureTests exercises.
The expiry index is what the sweeper reaches a surviving byCredit field
through, so consume dropped the index first and orphaned the field if it
then failed: the tenant's reserved-credits sum read high forever. The
per-tenant field goes first now, matching schematic-node 923dee5 and the
Go and Java stores.

The two lease stores also read an empty pin differently. The Lua takes it
as no pin and credits whichever lease holds the slot; the per-process
store takes it as a pin nothing matches and drops the refund. Node reaches
neither case because its absent pin is undefined, but C# has no undefined:
a missing leaseId field reads back as an empty string, and so does the
default on the public ReservationRecord. Both reservation stores now make
the call themselves and decline, which is the safe half and what the
per-process store already did.
CheckResult tells callers to bind a credits-remaining counter to
CreditSettled, but the REST check and server mode each build the rules
engine entitlement by hand, and neither copied the credit split, the
consumption rate, the event subtype or the warning tiers. Every check
without a usage goes through one of those two, so the counter the docs
point at read as null.

The test walks the target's properties by reflection rather than listing
them, so a field added upstream fails here instead of being dropped.
@bpapillon bpapillon self-assigned this Sep 21, 2026
{
Company = company,
User = user,
Quantity = options.Usage.Value,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sends a fractional Usage to the server as-is (Quantity is a double?), but TrackWithReservation settles (long)Math.Ceiling(actual). So Check(usage: 2.5) holds 2.5×rate and later bills 3×rate, which is more than it held.

@cbrady

cbrady commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

here are some claude suggestions:

return DateTime.UtcNow + timeout.Value;
   - This is computed at join time. LeaseCheck passes only requestOptions, not a deadline.
   - Node computes checkDeadline once when the check starts and passes it to maybeExtendInBackground.
   - A check with a 200ms timeout can therefore take acquire time + reserve time + 200ms. That breaks the guarantee the code's own comment describes (a short check shouldn't sit behind a slow extend).
   - Fix: compute the deadline when the check starts and thread it through.
2. A reservation with no lease id is refunded differently from Node. Both reservation stores have:
if (refund > 0 && Field(raw, "leaseId").Length > 0)
   - Node refunds unconditionally. In Node's Redis store an empty lease id disables the lease check, so the refund goes to whichever lease holds the slot now.
   - In a fleet mixing Node and C# servers, the same reservation settles differently depending on which server sweeps or consumes it.
   - The C# behaviour is the safer one, and the comment explains the reasoning. Either port it to Node or call it out in the PR description, since the PR claims byte-for-byte parity.
3. Async work runs while _gate is held. In CreditLeaseManager.cs:
started = AcquireAsync(companyId, creditTypeId, options);
_inflightAcquire[key] = started;
   Also return Track(ExtendIfNeededAsync(...));, both inside lock (_gate).
   - An async method runs synchronously up to its first real await, and that part runs under the lock here.
   - With InMemoryLeaseStore (which returns Task.FromResult), that covers the store reads, the recheck and the HTTP request setup. Every slot's acquires and extends queue behind that.
   - It doesn't deadlock today because Monitor locks are reentrant and the store never calls back into the manager. A user-supplied ILeaseStore or ILeaseWireClient that blocks could stall the whole client.
   - Node has no equivalent problem because it's single-threaded.
   - Fix: register a placeholder under the lock, then start the work after releasing it, e.g. Task.Run(() => AcquireAsync(...)) or a TaskCompletionSource created with RunContinuationsAsynchronously.
4. The new DataStream fallback can leak the replicator health poller. The catch in Schematic.cs calls _datastreamClient?.Close().
   - If the DatastreamClientAdapter constructor throws (e.g. new Uri(baseUrl) on a bad URL), the field was never assigned, so there's nothing to close.
   - The constructor has already started _replicatorHealthService (its own HttpClient plus .Start()), so in replicator mode that poller keeps running with nothing left to dispose it.
   - Separately, the fallback turns a bad base URL from a constructor exception into one logged error. REST then uses the same bad URL, and CheckFlag quietly returns defaults.
   - Node behaves the same, but this is a visible behaviour change for existing C# users and deserves a changelog line.
5. A joiner can wait on an extend that decides not to extend. RecheckAndExtendAsync rechecks against the requirement of whichever caller started the extend, not the joiner's.
   - If a sibling extend lands first, that recheck can decide no extend is needed. The joiner then accepts the entry with too little left, and its retry reserve fails.
   - Node has the same logic, so this is an upstream issue rather than a port bug. It's cheap to fix here: have the joiner re-run NeedsExtend against its own requirement.
6. Minor robustness points:
   - InMemoryLeaseStore compares DateTimes without normalizing Kind, so a non-Z expiry from the API would shift expiry by the host's UTC offset. The Redis path is unaffected.
   - DrainAsync can briefly busy-loop without yielding when a task has completed but its cleanup continuation hasn't run yet.
   - ReleaseAllLocalLeasesAsync calls lister.ListAsync() with no local try/catch, although the outer catch in Shutdown covers it.
7. Out of scope: WithHttpClient doesn't copy AdditionalHeaders. That bug already exists on main; it's worth a separate fix.```

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants