Skip to content

ChromaReadRetries.MinDelay, collections created when missing, options from the services, and the migration guide (2.18.0) - #145

Merged
Laurianti merged 11 commits into
mainfrom
read-retries-min-delay
Oct 21, 2026
Merged

Laurianti merged 11 commits into
mainfrom
read-retries-min-delay

Conversation

@Laurianti

@Laurianti Laurianti commented Oct 21, 2026 •

Copy link
Copy Markdown
Member

ChromaReadRetries.MinDelay, a test of the retries that no longer depends on chance, collections created when missing, options made from the services in the dependency injection, the migration guide, and version 2.18.0.

MinDelay

Before: the delay before a new attempt of a read was a random time from zero to the cap, the smaller of MaxDelay and BaseDelay times 2 to the attempts that went again before. A read could go again at once.

Now: it is a random time from MinDelay to the cap.

  • new ChromaReadRetries(maxAttempts, minDelay, baseDelay, maxDelay) takes it; 0 <= minDelay <= baseDelay <= maxDelay, and each wrong value throws an ArgumentOutOfRangeException that names its parameter.
  • The constructor without it takes 0.8 times baseDelay, the lower bound of the jitter of Azure.Core (DelayStrategy, a jitter factor of 0.2). It is never above the base, so every setting that was valid stays valid, and it is zero for a base of zero.
  • ChromaReadRetries.Default waits 40 to 50 ms, then 40 to 100 ms and 40 to 200 ms, instead of 0 to 50, 100 and 200 ms. ChromaReadRetries.None does not change.
  • docs/errors.md says it, with an example.

The test of a cancellation between the attempts

Before: ReadRetriesTests.ACancellationBetweenTheAttempts cancels 200 ms after the first attempt fails, and expects the delay before the second attempt, up to 10 minutes, to be longer. A random delay under 200 ms let the second attempt go first and the read succeed, which is right. In a loop of 6000 runs the test failed once, on main before #144 and after it; with a largest delay of 1 s it failed 16 to 18% of the runs, and with 300 ms 62 to 64%.

Now: ChromaReadRetries has an internal Jitter, the random number of each delay, which only the tests set. The test sets it to 1, so the cancellation always comes during the delay of 10 minutes, and the loop of 6000 runs passes every time. AShortDelayGoesAgain sets it to 0 with a MinDelay of 20 ms: the second attempt goes after 20 ms and the read succeeds.

Tests: TheDelay with the smallest delay of the base, TheDelayWithASmallestDelay, TheSmallestDelayOfTheBase, WrongSmallestDelays, SmallestDelaysThatGo, ANegativeBaseWithoutASmallestDelay, AShortDelayGoesAgain, and the defaults in TheDefault.

Collections created when missing

Before: a client that wanted the collection created when missing called GetOrCreateCollectionAsync itself, often before every operation.

Now:

  • client.GetCollectionClient(name, createIfMissing: true), and the overload with a ChromaCollectionDefinition, send no request. The first operation gets or creates the collection with one request, which the operations that start before it ends share, each with its own cancellation; an error of that request comes from the first operation, and the next one asks again.
  • await client.GetOrCreateCollectionClientAsync(name), and the overload with a definition, send the request now, so its error comes from the call.
  • Both keep the collection. When a request fails because the collection was deleted elsewhere, they get or create it again, empty, with the same definition, and run the operation again once, never a write that went in part. createIfMissing: false is GetCollectionClient(name).
  • A first request that failed is not waited for again, also when every operation that waited for it was cancelled before: the next operation sends a new one.
  • An operation cancelled before it starts sends no request, also not the creation; GetOrCreateCollectionClientAsync with such a token neither.
  • The collection is read with the way of reading metadata values of the collection client, also of one that WithMetadataValues returns, as when it is read by name.
  • These are new overloads: GetCollectionClient(name) does not change, also for compiled applications.
  • docs/collections.md says it.

Tests: CollectionClientByNameTests, with a fake server: the first operation creates, the first operations share one request, a cancelled one leaves the others, a failed creation goes again, a collection deleted elsewhere is created again, a partial write is not run again, the With methods keep the creation, a failure after every waiter was cancelled is not kept, an operation cancelled before it starts creates nothing, the metadata values are read the way of the client, the definition goes with the request, and GetOrCreateCollectionClientAsync creates at once and throws at once. The fake server records the requests under a lock, as the first operations send them at the same time. CreateIfMissingServerTests on the server, passed on 0.4.10 and 0.4.23 with the v1 API, 0.5.16, 0.6.3, 1.0.0 and 1.5.9. The trimming and NativeAOT test runs it too.

Options made from the services

Before: AddChromaClient takes a function of the default options, called at the registration, so the options cannot come from IConfiguration or IOptions of the application.

Now: AddChromaClientFromServices(serviceProvider => options, configureHttpClient) and AddKeyedChromaClientFromServices(key, ...) call the function once, when the client or its options are first resolved; a null it returns throws an InvalidOperationException. They have their own names: an overload of AddChromaClient with the service provider would make AddChromaClient(_ => options), or AddChromaClient(null, builder => ...), ambiguous, and both compile as before. docs/dependency-injection.md says it.

Tests: DependencyInjectionFromServicesTests: the options come from services registered after the call, once; the HttpClient goes through the builder; keyed clients each with their options and HttpClient; wrong arguments.

Migration guide

From a migration of an application from 1.x: just the address of the server works as the URI; the example takes the collection client from GetCollectionClient; handlers that rewrote the v1 routes, or read the tenant and the database from auth/identity, can go, with WithTenantAndDatabaseFromIdentityAsync for the credentials; dependency injection; and what behaves differently from 1.x: reads that go again, NaN distances, empty metadata. QueryAsync returns the embeddings only when asked, as in 1.x, so the table of breaking changes does not change.

Version 2.18.0 for both packages: new public API, nothing breaks.

The test cancels 200 ms after the first attempt fails and expects the delay before the second one to be longer. With the full
jitter the delay of up to 10 minutes can be under 200 ms, about once in 3000 runs: the second attempt then goes first and the
read succeeds, which is right, and the test failed. ChromaReadRetries gets an internal Jitter that the tests can set; the test
sets it to 1, so the delay is 10 minutes and the cancellation always comes during it. The client and its API do not change.
The delay before a new attempt was a random time from zero to the cap, the smaller of MaxDelay and BaseDelay times 2 to the
attempts that went again before, so a read could go again at once. It is now a random time from MinDelay to the cap. A new
constructor takes MinDelay, from zero to BaseDelay; the constructor without it takes 0.8 times BaseDelay, the lower bound of the
jitter of Azure.Core, which is never above the base and is zero for a base of zero. The default waits 40 to 50 ms, then 40 to 100
and 40 to 200 ms; None does not change.
@Laurianti
Laurianti requested a review from a team as a code owner October 21, 2026 10:06
Copilot AI balanced review requested due to automatic review settings October 21, 2026 10:06

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

Static review found no blocking issues in the additive API, retry integration, or accompanying tests; tests were not executed.

No open issues 🎉

What changed in this PR

Adds a configurable minimum delay to read retries and removes random jitter from cancellation-test behavior for version 2.18.0.

Changes:

  • Adds MinDelay, constructor validation, and a default minimum of 80% of the base delay.
  • Adds controlled jitter for tests and expands retry coverage.
  • Documents the behavior and updates both package versions.
File Description
docs/​errors.md Explains minimum delays and configuration.
ChromaDB.Client/​PublicAPI.Unshipped.txt Registers the new public API.
ChromaDB.Client/​ChromaReadRetries.cs Implements minimum delays and test-controlled jitter.
ChromaDB.Client/​ChromaDB.Client.csproj Updates client version to 2.18.0.
ChromaDB.Client/​ChromaCollectionClient.cs Uses the optional jitter override during retries.
ChromaDB.Client.Tests/​ReadRetriesTests.cs Covers delay bounds, validation, and controlled retries.
ChromaDB.Client.DependencyInjection/​ChromaDB.Client.DependencyInjection.csproj Updates dependency-injection package to 2.18.0.

Review effort: Balanced

Just the address of the server works as the URI; the example takes the collection client from GetCollectionClient; the handlers
that rewrote v1 routes or read the tenant and the database from auth/identity can go, with WithTenantAndDatabaseFromIdentityAsync
for the credentials; dependency injection; and the behaviors that differ from 1.x: reads that go again, NaN distances, empty
metadata. QueryAsync returns the embeddings only when asked, as in 1.x.
…issing) and GetOrCreateCollectionClientAsync

GetCollectionClient(name or definition, createIfMissing: true) sends no request: its first operation gets or creates the
collection, with one request that the operations starting before it ends share, each with its own cancellation; a failure of it
comes from the first operation, and the next one asks again. GetOrCreateCollectionClientAsync(name or definition) sends the request
now, so its errors come from the call. Both keep the collection, and when it is deleted elsewhere they get or create it again and
run the operation again once, never a write that went in part. createIfMissing: false is GetCollectionClient(name).
…ions made from the services

The options of AddChromaClient come from a function of the default options, called at the registration, so they cannot read
IConfiguration or IOptions of the application. The new methods take a function of the IServiceProvider, called once when the client
or its options are first resolved; a null it returns throws an InvalidOperationException. They have their own names: an overload of
AddChromaClient would make AddChromaClient(_ => options) or AddChromaClient(null, builder => ...) ambiguous, and both compile as
before.
@Laurianti Laurianti changed the title ChromaReadRetries.MinDelay, and a test of the retries that does not depend on chance (2.18.0) ChromaReadRetries.MinDelay, collections created when missing, options from the services, and the migration guide (2.18.0) Oct 21, 2026
@Laurianti
Laurianti requested a balanced review from Copilot October 21, 2026 10:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Shared resolution can retain stale failures after cancellation, and creation callbacks ignore cloned clients’ metadata settings.

2 open issues

Review effort: Balanced

Comment thread ChromaDB.Client/ChromaCollectionClient.cs Outdated
Comment thread ChromaDB.Client/ChromaCollectionClient.cs Outdated
…r it was cancelled

When every operation waiting for the shared first read was cancelled and the read then failed, nothing let it go: the next operation
threw that failure again without a request, and only the one after it asked again. A failed or cancelled first read is now let go
when the next operation starts, so that operation sends a new request.
…ing metadata values

The creation called GetOrCreateCollectionAsync of the ChromaClient the collection client came from, so a client of WithMetadataValues
read the metadata of the collection the way of that ChromaClient: with an Inferred one, a date in the metadata came back as a
DateTime under Exact. The creation now goes with the ChromaHttpClient of the collection client, as the read by name does.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

An already-cancelled first operation can still initiate collection creation.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Check cancellation before starting collection resolution

ChromaDB.Client/​ChromaCollectionClient.cs:246

An already-cancelled first operation starts Resolve(CancellationToken.None) before its token is checked. With createIfMissing: true, this can create a collection even though the caller cancelled before invoking the operation, including through GetOrCreateCollectionClientAsync. Check the caller's token before starting or joining resolution; keep the shared request independent of subsequent waiter cancellation. Add a regression test asserting that an already-cancelled first call sends no creation request.

2 resolved since last review

Review effort: Balanced

The first operation of a client that creates its collection started the shared request before it looked at its token, so an
operation cancelled before it was called, or GetOrCreateCollectionClientAsync with such a token, still created the collection. The
token is now checked first, and such an operation sends no request; the shared request stays independent of the waiters that are
cancelled later.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Concurrent writes to the test request recorder can make the new regression test flaky.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Protect concurrent Requests.Add with synchronization

ChromaDB.Client.Tests/​CollectionClientByNameTests.cs:195

Opening the gate lets the eight count operations resume concurrently, but FakeServer.SendAsync records each request through an unsynchronized Requests.Add (line 376). Concurrent writes to List<T> can lose entries or throw during resizing, making this regression test flaky. Wrap Requests.Add in lock (Requests); the assertions already read the list after Task.WhenAll completes.

Review effort: Balanced

In FirstOperationsShareTheRequest the eight operations send their counts at the same time once the creation answers, and the fake
server added each request to a List<T>, which takes one writer at a time. Not seen in 5000 runs of the test, but such a write can
lose an entry or throw: the requests and the bodies are now recorded under a lock.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Shared asynchronous initialization, independent cancellation, and collection recreation with operation replay warrant final human review.

No open issues 🎉

Review effort: Balanced

@Laurianti
Laurianti merged commit 487ba89 into main Oct 21, 2026
16 checks passed
@Laurianti
Laurianti deleted the read-retries-min-delay branch October 21, 2026 12:12
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.

3 participants