test(csharp): retry EnableMultipleCatalogSupport metadata query on transient server 500 - #615
test(csharp): retry EnableMultipleCatalogSupport metadata query on transient server 500#615eric-wang-1990 wants to merge 1 commit into
Conversation
…ansient server 500 EnableMultipleCatalogSupportAffectsMetadataQueries intermittently fails the merge-queue E2E run with a server-side HTTP 500 INTERNAL_ERROR "The result files are not available in the result metadata" on a metadata query (GetTables / GetColumns via ExecuteMetadataSqlAsync). This is a SEA backend result-staging race, not a driver or test-logic defect: it is always the API-level 500 (never an assertion mismatch), the identical query succeeds on retry, and it hit whichever PR happened to be live on the shared warehouse (observed on both #614 and #604 merge_group runs at the same time). Fix: wrap the metadata query in ExecuteMetadataQueryWithRetryAsync — up to 4 attempts with exponential backoff (0.5/1/2s), each on a fresh statement, retrying ONLY on that specific transient error string. Every other exception and all the schema/row assertions propagate unchanged, so genuine regressions still fail. Note: the more general fix is a driver-side retry of this transient 500 in StatementExecutionClient (PowerBI users hit the same server race); that is a separate, higher-risk driver change and is left as a follow-up. This PR de-flakes the test so the merge queue stabilizes. Local: EnableMultipleCatalogSupportAffectsMetadataQueries 2/2 passed. Co-authored-by: Isaac
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean, well-reasoned test-only de-flake. Retry logic is correct (backoff sequence, attempt < maxAttempts stop condition, and narrow transient-marker matching all check out); one low note about not disposing failed statements across retry attempts.
|
|
||
| for (int attempt = 1; ; attempt++) | ||
| { | ||
| var statement = connection.CreateStatement(); |
There was a problem hiding this comment.
🔵 Low — The retry loop creates a fresh statement on every attempt but never disposes the failed ones. On a transient 500 the current statement is abandoned (the catch filter delays, then the loop continues and allocates a new statement) so up to 3 statements — each holding a server-side handle / HTTP resources — leak per invocation until GC. Much of this test file uses using var statement = connection.CreateStatement(); (e.g. lines 78, 840, 1321) for exactly this reason. Consider disposing the statement per attempt, e.g. wrap the body in using var statement = connection.CreateStatement(); inside the loop so each failed attempt is released before the backoff/retry.
1 participant
Summary
De-flakes
StatementTests.EnableMultipleCatalogSupportAffectsMetadataQueries, which intermittently fails the merge-queue E2E run with a server-side error:The 500 is raised inside
ExecuteMetadataSqlAsync(fromGetTables/GetColumns) — a SEA backend result-staging race, not a driver or test-logic defect:merge_groupruns at the same time — confirming it's an environmental/backend condition, not PR-specific.Fix
Wrap the metadata query in
ExecuteMetadataQueryWithRetryAsync: up to 4 attempts with exponential backoff (0.5s / 1s / 2s), each on a fresh statement, retrying only when the exception text contains"result files are not available in the result metadata". Any other exception — and all the schema-comparison and row-count assertions inTestMetadataQuery— propagate unchanged, so genuine regressions still fail.Verification
EnableMultipleCatalogSupportAffectsMetadataQueries2/2 passed (retry is a no-op on the happy path).Follow-up (not in this PR)
The more general fix is a driver-side retry of this transient 500 in
StatementExecutionClient.EnsureSuccessStatusCodeAsync— real PowerBI clients hit the same server race, not just this test. That is a higher-risk driver change (needs careful scoping of which 500s are safe to retry) and is intentionally left as a separate follow-up; this PR only stabilizes the test / merge queue.This pull request and its description were written by Isaac.