Execution Log Related Changes#4346
Conversation
WalkthroughDisposes HttpLogAppender instances during CLI shutdown and enhances HttpLogAppender with Run Set context tracking, guarded central-DB reporting, synchronous queue draining during disposal, improved error-origin capture, and debug-only logging of disposal/reporting failures. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant App as App.xaml.cs
participant Log4Net as log4net repo
participant Appender as HttpLogAppender
participant Queue as LocalQueue
participant API as CentralDB / AccountReportAPI
CLI->>App: RunNewCLI finally
App->>Log4Net: GetRepository().GetAppenders()
Log4Net-->>App: appenders[]
App->>Appender: DisposeHttpLogAppenders() -> Dispose()
rect rgb(240,250,240)
Note over Appender,Queue: Disposal (guarded, sync drain)
Appender->>Queue: Drain remaining items synchronously
Queue-->>Appender: pending entries
Appender->>API: SendRunsetExecutionDataToCentralDBAsync (try/catch)
API-->>Appender: success / error
Appender-->>App: disposal complete (errors debug-logged)
end
App->>CLI: continue shutdown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
66-71: AccountReportApiHandler constructed with pre-gateway URLSetter builds handler before appending gateway, so handler/rest client may target the wrong base. Build final URL first, then construct the handler.
- if (string.IsNullOrWhiteSpace(_apiUrl) || (!string.IsNullOrWhiteSpace(value) && value != _apiUrl)) - { - _apiUrl = value; - _accountReportApiHandler = string.IsNullOrWhiteSpace(_apiUrl) ? null : new AccountReportApiHandler(_apiUrl); - _apiUrl = $"{value}{GingerPlayEndPointManager.GetAccountReportServiceGateWay()}"; //Final Url with gateway - } + if (string.IsNullOrWhiteSpace(_apiUrl) || (!string.IsNullOrWhiteSpace(value) && !string.Equals(value, _apiUrl, StringComparison.Ordinal))) + { + var finalUrl = string.IsNullOrWhiteSpace(value) + ? null + : $"{value}{GingerPlayEndPointManager.GetAccountReportServiceGateWay()}"; + _apiUrl = finalUrl; + _accountReportApiHandler = string.IsNullOrWhiteSpace(_apiUrl) ? null : new AccountReportApiHandler(_apiUrl); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Ginger/Ginger/App.xaml.cs(1 hunks)Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (4)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)Ginger/GingerCoreNET/WorkSpaceLib/WorkSpace.cs (1)
WorkSpace(61-1006)Ginger/GingerCoreNET/Run/RunsetExecutor.cs (1)
RunsetExecutor(53-1061)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (4)
AccountReportRunSet(94-119)AccountReportApiHandler(38-809)AccountReportApiHandler(66-89)AccountReportApiHandler(90-93)
Ginger/Ginger/App.xaml.cs (2)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (3)
HttpLogAppender(38-605)HttpLogAppender(180-183)Dispose(147-178)Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (3)
Dispose(129-132)Reporter(28-357)ToLog(41-87)
🔇 Additional comments (3)
Ginger/Ginger/App.xaml.cs (1)
516-516: LGTM! Proper placement for cleanup.The call to
DisposeHttpLogAppenders()in the finally block ensures HTTP log appenders are disposed before application shutdown, allowing them to flush remaining logs regardless of whether exceptions occurred during CLI execution.Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (2)
295-296: LGTM: drain on queue completionIncluding
_queue.IsCompletedin the flush condition ensures remaining buffered items are sent during shutdown.
480-481: LGTM: exclude exit-code summary line from ExecutionErrorRequestsThe filter avoids duplicating the process-exit error in error aggregation.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
405-431: Major: Incomplete run-state flag reset causes state leakage across runs.
isRunSetStartedis set totrueon line 408 but never reset tofalsewhen the run set ends (line 425-431). Similarly,isPreRunSetOperationandisPostRunSetOperationare not reset. This causes state to leak across multiple run set executions, leading to misclassification of log events.For example, line 435 checks
!isRunSetStarted, which will fail after the first run completes because the flag remainstrue.Apply this fix:
if (evt.RenderedMessage.IndexOf("Run Set Execution Started", StringComparison.OrdinalIgnoreCase) >= 0) { isExecutionStarted = true; isRunSetStarted = true; + isPreRunSetOperation = false; + isPostRunSetOperation = false; EntityId = Guid.Empty; solutionId = Guid.Empty; runsetName = string.Empty; } ... if (evt.RenderedMessage.IndexOf("Run Set Execution Ended", StringComparison.OrdinalIgnoreCase) >= 0) { isExecutionStarted = false; + isRunSetStarted = false; + isPreRunSetOperation = false; + isPostRunSetOperation = false; EntityId = Guid.Empty; solutionId = Guid.Empty; runsetName = string.Empty; }
♻️ Duplicate comments (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
381-398: Critical: Guid? null-check bug still present; empty catch swallows exceptions.The issue flagged in previous reviews remains unresolved. When
RunSetConfig.GuidorSolution.Guidis nullable (Guid?), comparingGuid? != Guid.Emptyreturnstruewhen the value isnull, then casting(Guid)nullthrowsInvalidOperationException, which is silently swallowed by the empty catch block.Apply this fix:
- try - { - if (EntityId == Guid.Empty && WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Guid != Guid.Empty) - { - EntityId = (Guid)WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Guid; - } - if (solutionId == Guid.Empty && WorkSpace.Instance?.Solution?.Guid != Guid.Empty) - { - solutionId = (Guid)WorkSpace.Instance?.Solution?.Guid; - } - if (string.IsNullOrEmpty(runsetName) && !string.IsNullOrEmpty(WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Name)) - { - runsetName = WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Name; - } - } - catch - { - } + var runsetGuid = WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Guid; + if (EntityId == Guid.Empty && runsetGuid.HasValue && runsetGuid.Value != Guid.Empty) + { + EntityId = runsetGuid.Value; + } + var solGuid = WorkSpace.Instance?.Solution?.Guid; + if (solutionId == Guid.Empty && solGuid.HasValue && solGuid.Value != Guid.Empty) + { + solutionId = solGuid.Value; + } + var rsName = WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Name; + if (string.IsNullOrEmpty(runsetName) && !string.IsNullOrWhiteSpace(rsName)) + { + runsetName = rsName; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Ginger/Ginger/App.xaml.cs(1 hunks)Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Ginger/Ginger/App.xaml.cs (2)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (3)
HttpLogAppender(38-610)HttpLogAppender(180-183)Dispose(147-178)Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (3)
Dispose(129-132)Reporter(28-357)ToLog(41-87)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (3)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)Ginger/GingerCoreNET/WorkSpaceLib/WorkSpace.cs (1)
WorkSpace(61-1006)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (4)
AccountReportApiHandler(38-809)AccountReportApiHandler(66-89)AccountReportApiHandler(90-93)AccountReportRunSet(94-119)
🔇 Additional comments (2)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
171-171: Improved error message for disposal logging.The updated log message provides clearer context for disposal errors.
Ginger/Ginger/App.xaml.cs (1)
522-557: LGTM! Robust disposal implementation with defensive checks.The implementation correctly includes all requested defensive measures:
- Null checks for repository and appenders collection
- Individual try-catch per appender to ensure all are disposed even if one fails
- Proper error logging at each level
- Outer try-catch for unexpected errors
This ensures graceful cleanup during CLI shutdown even in edge cases.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
382-399: CRITICAL: Nullable Guid comparison and cast bug.When
RunSetConfig.GuidorSolution.GuidisGuid?(nullable), comparingGuid? != Guid.Emptyreturnstruewhen the value isnull, then casting(Guid)nullthrowsInvalidOperationException. The empty catch silently swallows this exception, causing context fields to remain unpopulated without any indication of failure.Apply this fix to properly handle nullable Guids:
try { - if (EntityId == Guid.Empty && WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Guid != Guid.Empty) + var runsetGuid = WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Guid; + if (EntityId == Guid.Empty && runsetGuid.HasValue && runsetGuid.Value != Guid.Empty) { - EntityId = (Guid)WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Guid; + EntityId = runsetGuid.Value; } - if (solutionId == Guid.Empty && WorkSpace.Instance?.Solution?.Guid != Guid.Empty) + var solGuid = WorkSpace.Instance?.Solution?.Guid; + if (solutionId == Guid.Empty && solGuid.HasValue && solGuid.Value != Guid.Empty) { - solutionId = (Guid)WorkSpace.Instance?.Solution?.Guid; + solutionId = solGuid.Value; } - if (string.IsNullOrEmpty(runsetName) && !string.IsNullOrEmpty(WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Name)) + var rsName = WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Name; + if (string.IsNullOrEmpty(runsetName) && !string.IsNullOrWhiteSpace(rsName)) { - runsetName = WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Name; + runsetName = rsName; } } -catch //Any exceptions that occur in this block should be ignored — they should neither be logged nor cause a return, so that the subsequent code continues to execute +catch (Exception ex) { + Reporter.ToLog(eLogLevel.DEBUG, "[HttpLogAppender] Failed to populate context from WorkSpace", ex); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (5)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)Ginger/GingerCoreNET/log4netLib/GingerLog.cs (1)
ToLog(92-123)Ginger/GingerCoreNET/WorkSpaceLib/WorkSpace.cs (1)
WorkSpace(61-1006)Ginger/GingerCoreNET/Run/RunsetExecutor.cs (1)
RunsetExecutor(53-1061)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (4)
AccountReportApiHandler(38-809)AccountReportApiHandler(66-89)AccountReportApiHandler(90-93)AccountReportRunSet(94-119)
🔇 Additional comments (7)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (7)
368-370: LGTM: Instance-scoped context fields.The context fields are now properly instance-scoped, preventing state leakage across appender instances and runs as recommended in previous reviews.
409-412: LGTM: Context properly reset on Run Set start.Resetting
EntityId,solutionId, andrunsetNameat Run Set start correctly prevents state leakage across runs.
429-431: LGTM: Context properly reset on Run Set end.Resetting context fields at Run Set end completes the lifecycle management, ensuring no stale state persists.
486-489: LGTM: Correctly excludes exit code errors from ExecutionError.The condition correctly skips adding "Error(s) occurred process exit code" messages to ExecutionError list, as these are handled separately in the block above.
531-536: LGTM: Robust regex pattern for operation name extraction.The improved regex pattern with word boundary and newline handling correctly extracts operation names even when they contain special characters or appear near line breaks.
545-550: LGTM: Consistent regex pattern for Post RunSet operation.The same robust regex pattern is correctly applied for Post RunSet operation name extraction, maintaining consistency.
436-463: Incorrect isUpdate flag for early failures.When sending Run Set execution data for errors that occur before Run Set starts,
isUpdate: trueis used. However, since the Run Set never started, no initial record was sent to central DB, meaning_isRunSetDataSentwould befalse. According toSendRunsetExecutionDataToCentralDBAsynclogic (from relevant code snippets), whenisUpdate=trueand_isRunSetDataSent=false, the update is aborted and returnsfalseimmediately.This means early failure data will never be successfully sent.
Change to
isUpdate: falsefor initial send:-bool response = await AccountReportApiHandler.SendRunsetExecutionDataToCentralDBAsync(accountReportRunSet, isUpdate: true).ConfigureAwait(false); +bool response = await AccountReportApiHandler.SendRunsetExecutionDataToCentralDBAsync(accountReportRunSet, isUpdate: false).ConfigureAwait(false);⛔ Skipped due to learnings
Learnt from: Maheshkale447 PR: Ginger-Automation/Ginger#4293 File: Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs:56-60 Timestamp: 2025-09-05T10:50:57.706Z Learning: In AccountReportApiHandler.cs, the _isRunSetDataSent flag is correctly initialized to true by default. This flag acts as a circuit breaker where true allows operations to proceed and false blocks downstream operations after a RunSet send failure. The optimistic default (true) is necessary to allow the initial RunSet send attempt.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (1)
381-399: Critical: Nullable Guid comparison bug causes silent failures.The code at lines 383-394 has a critical bug when
RunSetConfig.GuidorSolution.Guidis nullable (Guid?):
- Faulty comparison: When
Guid?isnull, the expressionGuid? != Guid.Emptyreturnstrue(becausenull != any value).- Cast failure: The subsequent cast
(Guid)nullableGuidthrowsInvalidOperationExceptionwhen the value isnull.- Silent swallow: The empty catch block at lines 396-398 hides the exception, causing
EntityId,solutionId, andrunsetNameto silently fail to populate.This breaks Run Set context tracking and central-DB reporting. Based on the codebase snippets,
RunSetConfigandSolutionproperties are likely nullable.Apply this fix to use safe null-checking patterns:
try { - if (EntityId == Guid.Empty && WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Guid != Guid.Empty) + var runsetGuid = WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Guid; + if (EntityId == Guid.Empty && runsetGuid.HasValue && runsetGuid.Value != Guid.Empty) { - EntityId = (Guid)WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Guid; + EntityId = runsetGuid.Value; } - if (solutionId == Guid.Empty && WorkSpace.Instance?.Solution?.Guid != Guid.Empty) + var solGuid = WorkSpace.Instance?.Solution?.Guid; + if (solutionId == Guid.Empty && solGuid.HasValue && solGuid.Value != Guid.Empty) { - solutionId = (Guid)WorkSpace.Instance?.Solution?.Guid; + solutionId = solGuid.Value; } - if (string.IsNullOrEmpty(runsetName) && !string.IsNullOrEmpty(WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Name)) + var rsName = WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Name; + if (string.IsNullOrEmpty(runsetName) && !string.IsNullOrWhiteSpace(rsName)) { - runsetName = WorkSpace.Instance?.RunsetExecutor?.RunSetConfig?.Name; + runsetName = rsName; } } -catch //Any exceptions that occur in this block should be ignored — they should neither be logged nor cause a return, so that the subsequent code continues to execute +catch (Exception ex) { + Reporter.ToLog(eLogLevel.DEBUG, "[HttpLogAppender] Failed to resolve Run Set context from WorkSpace", ex); }Additionally, log exceptions instead of silently swallowing them so failures are visible during debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (4)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)Ginger/GingerCoreNET/WorkSpaceLib/WorkSpace.cs (1)
WorkSpace(61-1006)Ginger/GingerCoreNET/Run/RunsetExecutor.cs (1)
RunsetExecutor(53-1061)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (4)
AccountReportApiHandler(38-809)AccountReportApiHandler(66-89)AccountReportApiHandler(90-93)AccountReportRunSet(94-119)
🔇 Additional comments (8)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (8)
367-369: LGTM: Instance fields prevent cross-instance state leakage.The fields
EntityId,solutionId, andrunsetNameare now correctly scoped as instance fields rather than static, which prevents mutable context from bleeding across appender instances and runs. This addresses the concern from the previous review.Based on learnings
408-411: LGTM: Run Set context is reset on start.The code correctly resets
EntityId,solutionId, andrunsetNameto empty values when a new Run Set execution starts. This ensures that context from a previous run doesn't leak into the new execution, which is essential for accurate central-DB reporting.
425-431: LGTM: Run Set context is cleared on end.The code correctly clears
isExecutionStartedand resetsEntityId,solutionId, andrunsetNamewhen the Run Set execution ends. This prevents stale state from persisting after execution completes.
435-462: LGTM: Failure reporting has proper guards and error handling.The code correctly guards the central-DB update with checks for
ExecutionId.HasValue,AccountReportApiHandler != null, and!string.IsNullOrEmpty(ApiUrl)before attempting to send data. The use ofConfigureAwait(false)at line 451 prevents potential deadlocks on background threads, and exceptions are properly logged rather than silently swallowed.Note: This feature depends on
EntityId,solutionId, andrunsetNamebeing correctly resolved in lines 381-399. If that resolution fails due to the null-check bug I flagged, these fields may be empty/Guid.Empty in the failure report.
479-523: LGTM: Error tracking logic is comprehensive and correct.The enhancements to error tracking are well-designed:
- Line 481: Guards against setting
ErrorSourceto an empty value.- Line 485: Correctly excludes "Error(s) occurred process exit code" messages from
ExecutionErrorlist since they're handled separately at lines 435-462.- Lines 494-518: Thoroughly extracts error details including
ErrorOriginPath,ErrorSource, retry counts, andActionIdfrom structured log messages.- Line 519: Ensures errors are only reported after all retries are exhausted (
TotalRetries == CurrentRetry), preventing duplicate error entries during retry attempts.
528-538: LGTM: Regex pattern is robust for operation name extraction.The improved regex pattern
@"\band Name\s+([^,\r\n]+)"at line 530 properly handles edge cases:
\bensures "Name" is matched as a whole word\s+handles variable whitespace between "Name" and the value[^,\r\n]+captures the operation name until a comma or newline, preventing incorrect captures when names contain special charactersThis addresses the robustness concerns from the previous review.
Based on learnings
542-552: LGTM: Post-operation regex matches Pre-operation pattern.The regex pattern at line 544 is identical to the Pre-operation pattern, ensuring consistent operation name extraction across both Pre and Post Run Set operations. This consistency is important for reliable error tracking.
580-609: LGTM: Error extraction is tailored to operation type.The addition of the
isRunsetOperationparameter at line 580 enables appropriate error message extraction:
- Runset operations (lines 586-596): Extracts errors from the
"Errors= (.*?)\n"pattern, which matches Pre/Post Run Set operation log format.- Action/Setup errors (lines 599-608): Uses the existing
"Error Details=([\s\S]*?)\r?\nExtra Details="pattern for detailed action failures.This ensures error messages are correctly captured regardless of their source context.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (4)
495-512: Make newline handling OS-agnostic in regex captures.Use \r?\n to handle Windows/Linux line endings.
- var match = Regex.Match(evt.RenderedMessage, @"SourcePath= (.*?)\n"); + var match = Regex.Match(evt.RenderedMessage, @"SourcePath=\s*(.*?)\r?\n"); @@ - var ActionNamematch = Regex.Match(evt.RenderedMessage, @"Description= (.*?)\n"); + var ActionNamematch = Regex.Match(evt.RenderedMessage, @"Description=\s*(.*?)\r?\n"); @@ - var TotalRetriesmatch = Regex.Match(evt.RenderedMessage, @"Total Retries Configured= (.*?)\n"); + var TotalRetriesmatch = Regex.Match(evt.RenderedMessage, @"Total Retries Configured=\s*(.*?)\r?\n"); @@ - var CurrentRetrymatch = Regex.Match(evt.RenderedMessage, @"Current Retry Iteration= (.*?)\n"); + var CurrentRetrymatch = Regex.Match(evt.RenderedMessage, @"Current Retry Iteration=\s*(.*?)\r?\n");
573-576: Await with ConfigureAwait(false) on background worker.Prevents unnecessary context capture and matches surrounding awaits.
- bool isSuccess = await AccountReportApiHandler.SendExecutionLogToCentralDBAsync(ApiUrl, ExecutionLogRequest); + bool isSuccess = await AccountReportApiHandler + .SendExecutionLogToCentralDBAsync(ApiUrl, ExecutionLogRequest) + .ConfigureAwait(false);
261-266: Include exception when logging Append failures for diagnosis.Current log drops the stack trace.
- Reporter.ToLog(eLogLevel.DEBUG, $"[HttpLogAppender] Error in Append: {ex.Message}"); + Reporter.ToLog(eLogLevel.DEBUG, "[HttpLogAppender] Error in Append", ex);
52-73: HttpLogAppender: handler vs ApiUrl gateway mismatch — construct handler from base URL, compute gateway-appended ApiUrl separately.Confirmed: HttpLogAppender creates AccountReportApiHandler from the raw setter value then overwrites _apiUrl with value+gateway. AccountReportApiHandler.SendExecutionLogToCentralDBAsync builds FinalAPIUrl by appending SEND_EXECUTIONLOG to the passed apiUrl — this can produce duplicated segments like …/api/AccountReport/api/AccountReport/…. Fix: keep a base URL for the handler and build a separate ApiUrlWithGateway for direct calls.
File: Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (getter/setter around lines ~52–73; similar pattern at ~106–116)
Minimal suggested changes (still relevant):
- _apiUrl = value; - _accountReportApiHandler = string.IsNullOrWhiteSpace(_apiUrl) ? null : new AccountReportApiHandler(_apiUrl); - _apiUrl = $"{value}{GingerPlayEndPointManager.GetAccountReportServiceGateWay()}"; //Final Url with gateway + var baseUrl = value?.TrimEnd('/'); + var gateway = GingerPlayEndPointManager.GetAccountReportServiceGateWay()?.Trim('/'); + // Handler must use the base (no gateway) to avoid double-append with relative endpoints + _accountReportApiHandler = string.IsNullOrWhiteSpace(baseUrl) ? null : new AccountReportApiHandler(baseUrl); + // Final URL for direct calls that require the gateway prefix + _apiUrl = string.IsNullOrWhiteSpace(baseUrl) || string.IsNullOrWhiteSpace(gateway) + ? baseUrl + : $"{baseUrl}/{gateway}";Guard the getter so the handler is created from a canonical base URL (not from _apiUrl that may include gateway):
- if (_accountReportApiHandler == null) - { - if (!string.IsNullOrEmpty(_apiUrl)) - { - _accountReportApiHandler = new AccountReportApiHandler(_apiUrl); - } - } + if (_accountReportApiHandler == null && !string.IsNullOrWhiteSpace(GingerPlayEndPointManager.GetAccountReportServiceUrl())) + { + _accountReportApiHandler = new AccountReportApiHandler(GingerPlayEndPointManager.GetAccountReportServiceUrl()); + }Reason: SendExecutionLogToCentralDBAsync builds FinalAPIUrl via $"{apiUrl.TrimEnd('/')}/{SEND_EXECUTIONLOG}" (AccountReportApiHandler.cs) — passing a gateway-appended ApiUrl will duplicate the "/api/AccountReport" segment. The above keeps the handler bound to the base endpoint and ensures ApiUrl used for direct POSTs includes the gateway once.
♻️ Duplicate comments (2)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (2)
406-414: Reset run-state flags on start/end to prevent leakage across runs.isRunSetStarted is never cleared; pre/post flags aren’t reset at boundaries, leading to misclassification.
if (evt.RenderedMessage.IndexOf("Run Set Execution Started", StringComparison.OrdinalIgnoreCase) >= 0) { isExecutionStarted = true; - isRunSetStarted = true; - EntityId = Guid.Empty; - solutionId = Guid.Empty; - runsetName = string.Empty; + isRunSetStarted = true; + isPreRunSetOperation = false; + isPostRunSetOperation = false; + EntityId = Guid.Empty; + solutionId = Guid.Empty; + runsetName = string.Empty; } @@ if (evt.RenderedMessage.IndexOf("Run Set Execution Ended", StringComparison.OrdinalIgnoreCase) >= 0) { isExecutionStarted = false; + isRunSetStarted = false; + isPreRunSetOperation = false; + isPostRunSetOperation = false; EntityId = Guid.Empty; solutionId = Guid.Empty; runsetName = string.Empty; }Also applies to: 415-424, 426-432
168-173: Filter TaskCanceledException before logging to avoid noise during shutdown.Log only unexpected exceptions.
- // Handle TaskCanceledException explicitly - Reporter.ToLog(eLogLevel.DEBUG, "Dispose Error disposing HttpLogAppenders", ex); - ex.Handle(e => e is TaskCanceledException); + // Filter expected cancellations first; log only unexpected ones + var hasUnexpected = !ex.Handle(e => e is TaskCanceledException); + if (hasUnexpected) + { + Reporter.ToLog(eLogLevel.DEBUG, "Dispose: unexpected error disposing HttpLogAppenders", ex); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (3)
Ginger/GingerCoreCommon/ReporterLib/Reporter.cs (1)
Reporter(28-357)Ginger/GingerCoreNET/WorkSpaceLib/WorkSpace.cs (1)
WorkSpace(61-1006)Ginger/GingerCoreNET/Run/RunListenerLib/CenteralizedExecutionLogger/AccountReportApiHandler.cs (4)
AccountReportApiHandler(38-809)AccountReportApiHandler(66-89)AccountReportApiHandler(90-93)AccountReportRunSet(94-119)
🔇 Additional comments (2)
Ginger/GingerCoreNET/log4netLib/HttpLogAppender.cs (2)
581-610: Good: extract SetExecutionError and keep it stateless.Static helper with clean separation of runset vs execution parsing looks fine.
367-370: Refactoring complete and verified—static fields successfully eliminated.All references to EntityId, solutionId, and runsetName are consistently instance-scoped. The fields are:
- Declared as private instance members with proper initialization (Guid.Empty, string.Empty)
- Populated lazily from WorkSpace state without cross-run bleed
- Reset on execution boundaries (lines 410–412, 429–431) to isolate context per appender instance
- Used exclusively as instance members in request objects (lines 446–448)
No static qualifiers, class-level access, or lingering references remain. The refactoring eliminates the cross-run state-bleeding risk identified in the review.
2 participants
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
Bug Fixes
New Features