Local and Remote execution defect fix#4344
Conversation
WalkthroughAssignGraphQLObjectEndPoint() in RunSetsExecutionsHistoryPage.xaml.cs now validates GraphQL availability before initializing clients: it checks PublishLogToCentralDB, retrieves the endpoint, performs a 3-second HTTP POST introspection health check (using System.Text.Json/System.Text.Encoding), logs and disables GraphQL on failures, and only initializes clients on success. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant Assign as AssignGraphQLObjectEndPoint()
participant Config
participant HTTP as HTTP Client
participant GraphQL as GraphQL Client
Caller->>Assign: invoke
Assign->>Config: read PublishLogToCentralDB
alt Publish disabled or config null
Assign->>Config: set GraphQL disabled
Assign-->>Caller: return false
else Publish enabled
Assign->>Config: get endpoint URL
alt endpoint empty
Assign->>Config: set GraphQL disabled
Assign-->>Caller: return false
else endpoint exists
rect rgb(230,245,255)
note over Assign,HTTP: Health-check phase (POST introspection, 3s timeout)
Assign->>HTTP: POST introspection query
HTTP-->>Assign: response / timeout / error
end
alt health check fails
Assign->>Config: log warning, disable GraphQL
Assign-->>Caller: return false
else health check succeeds
Assign->>GraphQL: initialize GraphQlClient & ExecutionReportGraphQLClient
Assign->>Config: set GraphQL enabled
Assign-->>Caller: return true
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Ginger/Ginger/Run/RunSetsExecutionsHistoryPage.xaml.cs (2)
303-311: Resource leak: _runsetFromReportLoader.Dispose() gated by _httpClient != nullThe _runsetFromReportLoader implements IDisposable and must be disposed unconditionally. Currently, if _httpClient is null, the loader is never disposed. Apply this diff:
private void OnUnloaded(object sender, RoutedEventArgs e) { + _runsetFromReportLoader?.Dispose(); if (_httpClient != null) { _httpClient.Dispose(); _httpClient = null; - _runsetFromReportLoader.Dispose(); } }Optionally remove the unused _httpClient field. GraphQlClient and ExecutionReportGraphQLClient do not implement IDisposable, so no additional disposals are needed.
167-217: Verification complete. All core concerns confirmed; fix addresses critical UI-blocking bug and auth gap.Verification findings:
- UI-blocking bug (CRITICAL): Confirmed.
HttpClient.PostAsync(...).Resultat line 201 blocks synchronously on UI thread when called fromRemoteRadioButton_Selected(line 261, async void event) andReloadExecutionHistoryData(line 325, public void). This freezes the WPF UI during the 3-second health check.- Missing auth headers (VALID GAP): Confirmed. GraphQLHttpClient v6.1.0 constructor accepts only
(endpoint, serializer)with no auth parameter. GraphQL queries use no Bearer tokens or auth headers. Health check currently blind to authentication that real queries require. Codebase patterns for Bearer tokens exist (e.g.,General.csline 1352,SealightsReportApiHandler.csline 164).- Variable shadowing (CONFIRMED): Local
responsevariable shadows field at line 201.- Proposed diff is appropriate: Async conversion with CancellationToken, renamed variable, TODO for auth wiring.
Convert to async and wire auth headers into health-check request
Synchronous
.ResultonHttpClient.PostAsyncat line 201 blocks the WPF UI thread during 3-second health checks. Called from UI event handlers (RemoteRadioButton_Selected, line 261;ReloadExecutionHistoryData, line 325), causing UI freeze.Also: health-check omits auth headers the GraphQL client uses → false negatives on secured endpoints. Local variable
responseshadows a field (readability).Apply the provided diff to:
- Convert
AssignGraphQLObjectEndPoint()toAssignGraphQLObjectEndPointAsync()- Use
awaitwithCancellationTokenSourceinstead of.Result- Rename response to
httpResponse- Update call sites at lines 261 and 325 to
await- Add TODO to propagate auth headers (Bearer token, etc.) to match real GraphQL client auth
For the auth implementation: confirm with the server provider whether GraphQL endpoints require Bearer tokens or other auth schemes, then retrieve and attach those headers in the health-check request via
request.Headers.Authorizationor similar.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Ginger/Ginger/Run/RunSetsExecutionsHistoryPage.xaml.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Ginger/Ginger/Run/RunSetsExecutionsHistoryPage.xaml.cs (3)
Ginger/GingerCoreCommon/ReportsLib/ExecutionLoggerConfiguration.cs (1)
ExecutionLoggerConfiguration(26-412)Ginger/GingerCoreNET/Run/GingerRemoteExecutionUtils.cs (2)
GingerRemoteExecutionUtils(35-178)GetReportDataServiceUrl(74-83)Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)
🔇 Additional comments (1)
Ginger/Ginger/Run/RunSetsExecutionsHistoryPage.xaml.cs (1)
49-49: Import OKSystem.Text is required for Encoding.UTF8 usage in the health‑check payload.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Ginger/Ginger/Run/RunSetsExecutionsHistoryPage.xaml.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Ginger/Ginger/Run/RunSetsExecutionsHistoryPage.xaml.cs (3)
Ginger/GingerCoreCommon/ReportsLib/ExecutionLoggerConfiguration.cs (1)
ExecutionLoggerConfiguration(26-412)Ginger/GingerCoreNET/Run/GingerRemoteExecutionUtils.cs (2)
GingerRemoteExecutionUtils(35-178)GetReportDataServiceUrl(74-83)Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)
🔇 Additional comments (6)
Ginger/Ginger/Run/RunSetsExecutionsHistoryPage.xaml.cs (6)
49-49: LGTM! Import added for health check encoding.The
System.Textimport is necessary to supportEncoding.UTF8used in the GraphQL health check content serialization.
171-175: LGTM! Null safety properly implemented.The null check for
execLoggerConfigand the check forPublishLogToCentralDB.Nocorrectly prevent initialization when remote execution logging is disabled or configuration is unavailable.
176-181: LGTM! Endpoint retrieval with proper validation.The endpoint retrieval and empty check correctly handle the case where the report service URL is not configured.
183-184: LGTM! Defensive URL composition implemented.The use of
TrimEnd('/')before appending/api/graphqlprevents double-slash issues and is consistent with defensive coding practices.
212-216: LGTM! Client initialization after successful health check.The GraphQL clients are properly initialized only after the health check confirms the endpoint is available, preventing failed operations downstream.
218-223: LGTM! Improved error handling with descriptive message.The error message clearly indicates the health check or connection failure, and the flag is properly reset to prevent use of an unconfigured client.
| // GraphQL POST health check | ||
| using (var httpClient = new HttpClient()) | ||
| { | ||
| httpClient.Timeout = TimeSpan.FromSeconds(3); | ||
|
|
||
| var healthCheckQuery = new | ||
| { | ||
| query = "{ __typename }" | ||
| }; | ||
|
|
||
| var content = new StringContent( | ||
| System.Text.Json.JsonSerializer.Serialize(healthCheckQuery), | ||
| Encoding.UTF8, | ||
| "application/json" | ||
| ); | ||
|
|
||
| HttpResponseMessage graphQLresponse = httpClient.PostAsync(graphQlUrl, content).Result; | ||
|
|
||
| if (!graphQLresponse.IsSuccessStatusCode) | ||
| { | ||
| Reporter.ToLog(eLogLevel.WARN, | ||
| $"GraphQL endpoint responded with {graphQLresponse.StatusCode}. Marking as unavailable."); | ||
| isGraphQlClinetConfigure = false; | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Risk of deadlock: avoid blocking async with .Result in UI contexts.
Line 201 uses .Result to block synchronously on PostAsync() in a WPF Page context. This pattern can cause deadlocks when the continuation tries to marshal back to the UI thread.
Refactor AssignGraphQLObjectEndPoint() to be async:
- private bool AssignGraphQLObjectEndPoint()
+ private async Task<bool> AssignGraphQLObjectEndPoint()
{
try
{
if (execLoggerConfig == null || execLoggerConfig.PublishLogToCentralDB == ExecutionLoggerConfiguration.ePublishToCentralDB.No)
{
isGraphQlClinetConfigure = false;
return false;
}
string endPoint = GingerRemoteExecutionUtils.GetReportDataServiceUrl();
if (string.IsNullOrEmpty(endPoint))
{
isGraphQlClinetConfigure = false;
return false;
}
string graphQlUrl = $"{endPoint.TrimEnd('/')}/api/graphql";
// GraphQL POST health check
using (var httpClient = new HttpClient())
{
httpClient.Timeout = TimeSpan.FromSeconds(3);
var healthCheckQuery = new
{
query = "{ __typename }"
};
var content = new StringContent(
System.Text.Json.JsonSerializer.Serialize(healthCheckQuery),
Encoding.UTF8,
"application/json"
);
- HttpResponseMessage graphQLresponse = httpClient.PostAsync(graphQlUrl, content).Result;
+ HttpResponseMessage graphQLresponse = await httpClient.PostAsync(graphQlUrl, content);
if (!graphQLresponse.IsSuccessStatusCode)
{
Reporter.ToLog(eLogLevel.WARN,
$"GraphQL endpoint responded with {graphQLresponse.StatusCode}. Marking as unavailable.");
isGraphQlClinetConfigure = false;
return false;
}
}
// healthy, initialize the client
graphQlClient = new GraphQlClient(graphQlUrl);
executionReportGraphQLClient = new ExecutionReportGraphQLClient(graphQlClient);
isGraphQlClinetConfigure = true;
return true;
}
catch (Exception ex)
{
Reporter.ToLog(eLogLevel.ERROR, "Error while verifying or connecting GraphQL endpoint.", ex);
isGraphQlClinetConfigure = false;
return false;
}
}Then update all call sites (lines 261, 325) to await the method:
- if (SetExectionHistoryVisibility(execLoggerConfig) && AssignGraphQLObjectEndPoint())
+ if (SetExectionHistoryVisibility(execLoggerConfig) && await AssignGraphQLObjectEndPoint())- if (AssignGraphQLObjectEndPoint() && SetExectionHistoryVisibility(execLoggerConfig))
+ if (await AssignGraphQLObjectEndPoint() && SetExectionHistoryVisibility(execLoggerConfig))Committable suggestion skipped: line range outside the PR's diff.
0fbbc6d
into
Releases/Official-Release
2 participants
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit