Task 8 — Add HttpConfigSource with ETag caching and robust fetch semantics#11
Task 8 — Add HttpConfigSource with ETag caching and robust fetch semantics#11KostasBan wants to merge 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63355d7341
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (fetchResult.Bytes == null) | ||
| { | ||
| return new RefreshResult(false, null); |
There was a problem hiding this comment.
The new Bytes == null branch returns Changed=false with no error for all successful fetches, but that is only safe when a last-known-good payload already exists. On a fresh repository (or after load failure), this path silently reports success while leaving the snapshot empty, so callers never get a usable config and have no error to act on. Gate this no-change path on existing snapshot bytes and return a failure when there is nothing cached locally.
Useful? React with 👍 / 👎.
| for (var attempt = 0; attempt <= _retryCount; attempt++) | ||
| { | ||
| ct.ThrowIfCancellationRequested(); | ||
| var result = await FetchOnceAsync(ct).ConfigureAwait(false); |
There was a problem hiding this comment.
FetchAsync uses ConfigureAwait(false) inside the retry loop, so after the first failed attempt subsequent retries can run without the original synchronization context. FetchOnceAsync then polls operation.isDone via repeated await Task.Yield(), which becomes a tight reschedule loop off-context and can burn CPU during slow/failing requests (the exact scenario where retries happen). Preserve context for retry iterations, or use a non-spin completion mechanism.
Useful? React with 👍 / 👎.
1 participant
Motivation
Description
Packages/com.kbanakakis.beacon/Runtime/Repositories/HttpConfigSource.csimplementingIConfigSourceusingUnityWebRequestwith configurable timeout, retry count, exponential backoff, optional static headers, optional dynamic header provider, and in-memory ETag caching;FetchAsyncsendsIf-None-Match, treats304asSuccess=true, Bytes=null, and returnsETag(orLast-Modified) asSourceVersionon200.DefaultConfigRepository.RefreshAsyncto treatSuccess=truewithBytes==nullas no-change (do not persist or fireSnapshotChanged) and addedHasNoMaterialChangeto avoid persisting/emitting when version/schema/bytes are identical.Packages/com.kbanakakis.beacon/Tests/RepositorySmokeTests.cswith NUnit tests covering: successful 200 update + persistence, 304/no-bytes -> treated as no-change, fetch failure preserves stored snapshot, and invalid payload preserves last-known-good; also addedSequenceConfigSourceand enhancedFakeConfigStoretest helpers.HttpConfigSource(HttpConfigSource.cs.meta) and a small fix to use_url.AbsoluteUriwhen creating the request.Testing
dotnet test, but the automated attempt failed becausedotnetis not installed in this environment (command not found), so no tests were executed here.Codex Task