fix(environments): finish module on real IBackendApiClientProvider - #1042
Conversation
A missing environments API must not stop other studio features from initializing or leave the picker unregistered. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
| return Task.FromResult(new HttpResponseMessage(StatusCode) | ||
| { | ||
| RequestMessage = request, | ||
| Content = new StringContent(ResponseContent, Encoding.UTF8, new MediaTypeHeaderValue("application/json")) | ||
| }); |
|
sfmskywalker
left a comment
There was a problem hiding this comment.
Code Review — Round 1/4 @ 56c8ed2 → main
Verdict: REQUEST_CHANGES + HIGH (formal event COMMENT — own-PR APPROVE blocked for sfmskywalker)
What looks good (Option B for #1039)
- Delete
EnvironmentBackendApiClientProvider(private ServiceProvider / auth-stripped path). EnvironmentRemoteBackendAccessor+ keepDefaultBackendApiClientProvider.- Register
LoadEnvironmentsStartupTask/ circuitFeature.InitializeAsyncload; missing/environmentsno longer blocks other features. - Hosts stay opt-in (commented
AddEnvironmentsModule). - Tests cover Default provider kept, load/fill, missing API, env switch updates accessor URL, feature catalog refetch on URL change.
Blocking — RemoteFeatureProvider catalog URL TOCTOU (Greptile P1)
backendUrl is captured before _catalogLock. Inside the lock, GetApiAsync reads the live environment URL again. If the env switches between those reads, catalog B can be stored under _catalogUrl = A (or the reverse), so later checks for B hit a stale/wrong cache.
Fix: re-read remoteBackendApiClientProvider.Url inside the lock (after the double-check) and assign _catalogUrl from that same value after the fetch (ideally re-read again after GetApiAsync and key by the URL that was actually used). Add/adjust a unit test for switch-during-load if easy.
Soft steers (non-blocking)
- Empty
catch (ApiException)inFeature.InitializeAsync— add the same clarifying comment as theHttpRequestExceptionarm (CodeQL). - Greptile P2: optional logging on swallowed load failures for ops visibility.
- Test
HttpResponseMessagedispose nit (CodeQL).
Merge gates
- Code Review: REQUEST_CHANGES @
56c8ed2— re-request after P1 fix - Greptile: tip score unknown/credits low — when unavailable, merge would be CR APPROVE+HIGH ∧ CI green (CI currently green)
- CI: green on tip
Read IBackendApiClientProvider.Url inside the catalog lock after creating the client so an environment switch mid-load cannot store backend B's catalog under backend A's cache key. Also log expected environments load failures (404/401/403) instead of swallowing every ApiException. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
sfmskywalker
left a comment
There was a problem hiding this comment.
Code Review — Round 2/4 @ 9dcc42c → main
Verdict: APPROVE + HIGH (formal event COMMENT — own-PR APPROVE blocked for sfmskywalker)
Prior Round 1 @ 56c8ed2 is STALE. Soft steers parked.
Delta vs Round 1
- TOCTOU fixed: Under
_catalogLock,GetApiAsyncthen readIBackendApiClientProvider.Urlas_catalogUrl(no pre-lock capture). Double-check uses that post-fetch URL. - Test:
FeatureChecks_DoNotCacheUnderStaleUrlWhenEnvironmentChangesMidLoad. - Soft/P2: Feature + startup task only swallow
HttpRequestExceptionandApiException404/401/403, with warning logs.
Blocking Round 1 note addressed. Clear and maintainable.
Soft steers (non-blocking)
GetApiAsyncnow runs before the in-lock double-check (extra client mint when cache already warm for the same URL) — acceptable; reorder only if profiling cares.- Unexpected
ApiException(e.g. 500) onFeature.InitializeAsyncnow propagates beforeAddElement— intentional narrowing; watch host feature-init behavior.
Merge gates
- Code Review: APPROVE + HIGH @
9dcc42c - Greptile: tip 5/5 while scoring; if Greptile stalls/credits out → APPROVE+HIGH ∧ CI green is enough
- CI: green on tip
Purpose
Finish
Elsa.Studio.Environmentson the supportedIBackendApiClientProviderpath (Option B — do not remove). Closes #1039. Related: #326.Scope
Select one primary concern:
Description
Problem
EnvironmentBackendApiClientProviderbuilt a privateServiceCollection/ServiceProvideron everyGetApiAsynccall, left auth commented out, and replacedDefaultBackendApiClientProvider.LoadEnvironmentsStartupTaskwas commented out, soIEnvironmentServicestayed empty and EnvironmentPicker never filled (#326).Solution
Option B — reuse the real client stack:
IRemoteBackendAccessorwithEnvironmentRemoteBackendAccessor(selected environment URL, elseBackendOptions).DefaultBackendApiClientProvidersoApiClientFactory+ Blazor scope proxy + host auth still apply.EnvironmentBackendApiClientProvider.LoadEnvironmentsStartupTaskand run it fromFeature.InitializeAsyncso circuit-scoped pickers actually load.RemoteFeatureProvidercatalog by backend URL afterGetApiAsyncinside the catalog lock, so an env switch mid-load cannot store backend B's catalog under backend A's key.Sample hosts stay opt-in (commented
AddEnvironmentsModule) because/environmentsis not part of the default backend.Verification
Steps:
dotnet test src/modules/Elsa.Studio.Environments.Tests/Elsa.Studio.Environments.Tests.csproj— 6 passeddotnet test src/modules/Elsa.Studio.ExternalAuthentication.Tests --filter RemoteFeatureProviderTests— 9 passed (includes mid-load URL switch)AddEnvironmentsModule(backendApiConfig)on a host whose backend exposes/environmentsand confirm EnvironmentPicker fills after login.Expected outcome:
IBackendApiClientProviderremainsDefaultBackendApiClientProvider.IRemoteBackendAccessor.RemoteBackend.Urland subsequent API calls.IEnvironmentServiceafter the startup task / feature init.GetApiAsyncunder the lock.Checklist