System test runner: re-attempt tests that fail during setup phase

[kcreddy]

System test runner: re-attempt tests that fail during the setup phase

When a system test fails during setup (policy creation, agent enrollment
and assignment, service container startup), the cause is usually a
transient environment issue such as a dropped connection to Kibana, a
container killed by the CI runner, or a slow Fleet response, not a
problem in the package under test. Today these failures are terminal:
the build fails, a flaky-test issue is auto-filed, and a human triages
what turns out to be infrastructure noise.

Add an opt-in setup re-attempt to the system test runner. When
prepareScenario fails with an environment error, the attempt is torn
down and the single failing test config is re-attempted from scratch,
with a fresh service run ID and config, up to the configured number of
times.

Scope, deliberately strict:

- Setup-phase environment errors only. Failures reported as
  ErrTestCaseFailed from prepareScenario (no documents within the
  wait-for-data timeout, assert.hit_count / min_count / fields_present
  not satisfied, no data streams discovered) are real test signal and
  are never re-attempted. Data validation in validateTestScenario never
  surfaces as a Go error, so it cannot trigger a re-attempt either.
- The one exception is the test service exiting with a non-zero code
  while waiting for documents. It is marked with errServiceExited so it
  is re-attempted as an environment failure, while still unwrapping to
  ErrTestCaseFailed so it keeps being reported as a test case failure
  (xUnit <failure>) with the same message as before. The
  docker_failing_test_service false-positive test guards this.
- Each attempt is fully torn down before the next one starts. No
  re-attempt happens if the context was cancelled or if the teardown of
  the failed attempt returned an error.
- Context cancellation during setup is captured in the result as on
  main, so the <error> entry is preserved and post-processing continues
  without re-attempting.
- The agent logs check after the run uses the start time of the last
  attempt, so logs from torn-down attempts are not scanned.
- The --setup, --no-provision and --tear-down workflows are unchanged.

Configuration:

- --setup-reattempts N on `elastic-package test system`, default 0
  (disabled), maximum 5.
- ELASTIC_PACKAGE_TEST_SETUP_REATTEMPTS env var as an alternative, so CI
  can enable it without changing scripts. The flag takes precedence.

Reporting:

- Tests passing only after a re-attempt are reported as a plain PASS in
  the human and JSON formats, keeping the result values standard for
  Buildkite and the flaky-test automation.
- The failures of the previous attempts are kept for aggregation in a
  new FlakyMsg field on TestResult, emitted as a <flakyFailure> element
  in xUnit and a flaky_details field in JSON.
- Tests exhausting all attempts are reported exactly as a single failed
  attempt is today.

Also adds unit tests for the re-attempt policy, the error
classification and the reporters, and a section in the system testing
HOWTO.

Closes #3888

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Closes #3888

[Copilot]

🟡 Changes recommended

Context cancellation can be swallowed as a setup failure, causing the runner to continue into post-test steps even after the run has been cancelled.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR adds setup-phase re-attempts to the system test runner to reduce CI noise from transient infrastructure failures (e.g., Kibana timeouts, container termination) while preserving visibility of instability by marking tests as flaky when they only pass after a re-attempt.

Changes:

• Add a per-test setup re-attempt loop around scenario preparation, with teardown between attempts and a configurable --setup-reattempts flag (default: 1; 0 disables).
• Introduce FlakyMsg on testrunner.TestResult and surface it in human output (PASS (flaky: ...)) and xUnit output via <flakyFailure ...>.
• Add unit tests for the re-attempt logic and xUnit flaky reporting, plus system testing HOWTO documentation.

File summaries

File	Description
internal/testrunner/testrunner.go	Adds FlakyMsg to the shared TestResult model so reporters can surface flaky passes.
internal/testrunner/runners/system/tester.go	Implements setup-failure sentinel + re-attempt loop and annotates eventual passes as flaky.
internal/testrunner/runners/system/runner.go	Plumbs SetupReattempts runner option down into the system tester.
internal/testrunner/runners/system/reattempt_test.go	Adds unit tests for setup-phase re-attempt semantics and flaky annotation behavior.
internal/testrunner/reporters/formats/xunit.go	Emits <flakyFailure message="..."> for flaky passes in xUnit output.
internal/testrunner/reporters/formats/xunit_test.go	Tests xUnit flaky output and ensures flaky passes don’t count as failures/errors.
internal/testrunner/reporters/formats/human.go	Displays flaky passes as PASS (flaky: ...) in the human reporter.
internal/cobraext/flags.go	Defines the new --setup-reattempts CLI flag and description.
docs/howto/system_testing.md	Documents setup-phase re-attempt behavior and how flaky reporting is represented.
cmd/testrunner.go	Wires the new CLI flag into the system test runner options.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[mrodm]

Were you able to run a setup re-attempt locally? Maybe some way of forcing errors in the setup?
I was trying to do so, but not able to find a scenario where it happens. I'd like to try it locally if possible.

EDIT: It would also be interesting to add a comment test integrations here in this Pull Request to force testing all the packages in the integrations repository with these elastic-package changes.

[mrodm]

This new field is not added or taken into account in the json format.

Should it be added there for consistency ?

[kcreddy]

Added in a6c50f7

[kcreddy]

Revisited after Teresa's request to keep result as a plain PASS. JSON now carries the previous-attempt failures in a separate flaky_details field (omitted when empty).

[mrodm]

As a note, this new field will not be used by the Buildkite pipeline (IIRC it is not an standard field from xUnit) nor by the automation in the integrations repository to create new GitHub issues with label flaky-tests (no information about this flakiness will be added there).

[mrodm]

If the parameter --defer-cleanup is set in the CLI, it will block every re-attempt.

skipDeferCleanup is derived from Skipped status rather than from whether this is a re-attempt. Each cleanup between retry attempts waits the full --defer-cleanup duration (e.g. 5 minutes), making retries painfully slow in practice.

Would it be ok to wait in each re-attempt if the user sets that flag in the CLI?

elastic-package test system -v --defer-cleanup 5m

WDYT @teresaromero ? Keeping it as it is, it will let the developer to debug each re-attempt.

[mrodm]

Related to Copilot's message https://github.com/elastic/elastic-package/pull/3919/changes#r3928052171

I wonder if we should manage here if there is any context error to not wrap it with errSetupFailed

Suggested change

	var tcf testrunner.ErrTestCaseFailed
	if errors.As(err, &tcf) {
	return results, nil
	}
	return results, errSetupFailed{err: err}
	var tcf testrunner.ErrTestCaseFailed
	if errors.As(err, &tcf) {
	return results, nil
	}
	if ctx.Err() != nil {
	return results, ctx.Err()
	}
	return results, errSetupFailed{err: err}

Or would it be better to do it where Copilot's message points out ?

[kcreddy]

Took your suggestion a6c50f7

[kcreddy]

Reverted this because propagating ctx.Err() as hard error made runner discard the results

[kcreddy]

test integrations

[elastic-vault-github-plugin-prod]

Created or updated PR in integrations repository to test this version. Check elastic/integrations#21166

[Copilot]

🟡 Changes recommended

Unresolved retry classification, log-scan, and cleanup issues can bypass retries, fail flaky passes, or retry a dirty environment.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Critical result loss and moderate dirty-environment retry handling remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

internal/testrunner/runners/system/tester.go:2201

• prepareScenario is not setup-only for data semantics: finalizeScenario calls verifyDataStream, whose waitForDocs evaluates assert.hit_count, assert.min_count, and assert.fields_present before returning ErrTestCaseFailed. Wrapping every preparation error here makes missing-document/field/count validation failures retryable and can turn a real validation defect into a flaky pass, contrary to the strict no-retry contract. Distinguish setup/service errors from these assertion failures (or move the assertions out of prepareScenario) before applying errSetupFailed.

		return results, errSetupFailed{err: err}

internal/testrunner/runners/system/tester.go:802

• Checking only tdErr is not sufficient to enforce the no-dirty-environment rule. When setupService or setupAgent fails inside deployer SetUp, their teardown handlers have not yet been registered here, and the deployers' setup-error cleanup logs and suppresses teardown failures. In that case tdErr is nil even if partial resources could not be removed, so this branch retries on a dirty environment instead of stopping as required. Propagate those setup-cleanup errors or expose them to this retry gate.

			if attemptNum < maxAttempts && ctx.Err() == nil && tdErr == nil {

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[teresaromero]

I went through the code and left some comments. I am concern about having this activated by default; suggested to make it opt-in so we can be explicit about it.

Related to the formats/outputs; based on the comment of Mario https://github.com/elastic/elastic-package/pull/3919/changes#r3959485113 ; is it necesary to show it as a different "success" ? if the goal is to retry on setup (because is flaly) i think the output should be FAIL/PASS after all , regardless the setup retries it got. this way we keep output standard for buildkite. wdyt?

[teresaromero]

Suggested change

	cmd.Flags().Int(cobraext.SetupReattemptsFlagName, 1, cobraext.SetupReattemptsFlagDescription)
	cmd.Flags().Int(cobraext.SetupReattemptsFlagName, 0, cobraext.SetupReattemptsFlagDescription)

Can we change the default to 0 and be explicit on this setting where ever we are using the command?

[mrodm]

If it is set to zero the default value to not change the current behavior, it will be good to support setting an environment variable too for this. That would help to update the CI in the integrations and elastic-package repository without having to update the CI scripts (adding new parameters). However this parameter is not global for the test command and it will be needed to split the commands in scripts.

So, I'd prefer if we do so, reading also an environment variable to allow setting the same value as the new parameter.

[kcreddy]

Updated default as 0 and added env variable TEST_SETUP_REATTEMPTS that can be set inside integration pipeline to make re-attempts.

[teresaromero]

is there a max limit for reattempts?

[kcreddy]

Done.

[mrodm]

Added comments, my main concern is that these new re-attempts should not change the current behaviour for reporting failures and errors. And following that, it needs to be ensured that the cleaning (tearDownTest) runs between attempts and at the end of each test. Every re-attempt needs to be run after all the tearDown process is performed.

EDIT: If new changes are pushed, it would be interesting to run the tests with all the packages integrations via posting the test integrations comment. Not for all commits, but we would need to validate from time to time that this is not breaking anything and that it is working as expected.

[mrodm]

I'm not sure if this latest change would report the errors like service exited non-zero or waitForDocs with no hits as test cases failed.

They should appear in the xUnit file as failures or errors. I'm not sure how they are reported, but they are.

Could you check that this is still working in this way ?

[mrodm]

Moreover commit dba1cb7 removed the ErrTestCaseFailed guard, so waitForDocs failures, including assert.hit_count, assert.min_count and assert.fields_present, are re-attempted. A test whose input genuinely produces no data now waits the 10 minute default timeout twice, plus a second setup.

The PR description, the docs section, and the test comment at reattempt_test.go:86 all still claim these are never re-attempted. Could it be restored the guard for the waitForDocs error? In any case, it would be needed to update the description and docs independently of the solution chosen.

As it would happen in main, those waitForDocs errors should also be reported in the xUnit file.
It could be tested by changing some min_count or hit_count value in the test config of one package to force the failure.

[kcreddy]

Restored the ErrTestCaseFailed guard, so waitForDocs failures (no hits, hit_count, min_count, fields_present, and undiscovered data streams) are not re-attempted and still surface as via FailureMsg, same as main.

[kcreddy]

For the service-exit case, the first version turned it into a plain error, which made it retryable but also changed its xUnit report from <failure> to <error>. The docker_failing_test_service false-positive test caught that.

In 2d1eb0c, it is now wrapped in errServiceExited, which unwraps to ErrTestCaseFailed, so it is re-attempted but still reported as <failure> with the exact same message. TestServiceExitedReportedAsFailure guards this now.

Ran both locally against a 9.4.4 stack with --report-format xUnit --report-output file:

• docker_failing_test_service with defaults: one <failure>, both CI assertions pass.
• Same package with --setup-reattempts 1: attempt 1 fails, teardown runs, attempt 2 starts in a fresh namespace, fails again, single reported, no leftover containers.

[mrodm]

Related to this comment #3919, the startingTime is obtained before this function.

That time is used to check the agent logs looking for any error or warning. If that startingTime is obtained before the retries, it could be checking other containres.

And this is also related to the other comment posted by Copilot #3919, it would be needed to assess if this is true. If there is a re-attempt, elastic-package needs to ensure that the scenario (kibana, elasticsearch) is not dirty. Previous Elastic Agents should be unenrolled and removed, policies should be deleted, data streams should be deleted too, and so on.

[kcreddy]

Fixed in 45e8024

[mrodm]

If this error is overwritten with nil, and all the attempts finish with error.

The condition in line 809, will not be true. I'm thinking for instance in those cases that the service cannot start due to some error. We want to report that as before in the xUnit so it gets shown in Buildkite and in Github issues.

Related also to this other comment https://github.com/elastic/elastic-package/pull/3919/changes#r3980680995

[mrodm]

It could be tested forcing an error in some Dockerfile in any of the test packages, or any other error that could lead to a failure here.

Then it should be reported as part of the xUnit (as it is done currently).

IIRC this is the command to create the xUnit files in system tests:

elastic-package test system -v --report-output file --report-format xUnit

[kcreddy]

Thanks for the thorough look. This downstream result-loss risk are pre-existing behaviour in upstream/main and not introduced by this PR.

On main, runTest already returns (results, nil) for all setup errors (line return results, nil in the original prepareScenario error handler). As a result, run() already continues into the stack dump.

Our re-attempt loop reproduces that same behaviour: when all attempts are exhausted, errSetupFailed is cleared and we return (partial, nil), so run() sees the same nil-error path it would have seen on main for a single failed attempt. Nothing is worse.

Agreed that fixing the result-loss in run() would be a good improvement, but it's a separate pre-existing issue. Happy to track it as a follow-up if that's useful.

[mrodm]

IIRC on main a context cancellation during setup produced an <error> entry. Now runTest returns the context error as a hard error, and testrunner.run discards that tester's results

From main:

		// report all other errors as error entries in the xUnit file
		results, _ := result.WithError(err)
		return results, nil

It would be needed to validate if those errors are still reported in the xUnit file as <error> too.

[kcreddy]

Confirmed and fixed. runTest now handles the cancellation same way as main does. Also updated thread https://github.com/elastic/elastic-package/pull/3919/changes#r3989048115

[Copilot]

🟡 Changes recommended

Retry defaults, teardown safety, human flaky reporting, and prior-attempt log handling remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

cmd/testrunner.go:21

• This added import is out of gofmt order; internal/common should precede internal/environment. Running the repository formatter will rewrite this block and can make the format check fail.

	"github.com/elastic/elastic-package/internal/environment"
	"github.com/elastic/elastic-package/internal/common"

internal/testrunner/runners/system/tester.go:802

• Checking tdErr here is not sufficient to enforce the no-dirty-retry rule. When serviceDeployer.SetUp or agentDeployer.SetUp fails, their deferred cleanup calls TearDown but logs and suppresses cleanup errors, and the outer cleanup handlers are registered only after successful setup; tdErr therefore remains nil and this branch retries despite a failed teardown. Propagate that cleanup failure or carry an explicit dirty-environment signal before retrying.

			if attemptNum < maxAttempts && ctx.Err() == nil && tdErr == nil {

internal/testrunner/runners/system/tester.go:819

• FlakyMsg is only consumed by the xUnit formatter. The human formatter still renders every result without ErrorMsg, FailureMsg, or Skipped as plain PASS, so a recovered setup failure is not shown as PASS (flaky: …) as promised. Add the annotation to the human formatter and cover it in its tests.

			partial[0].FlakyMsg = strings.Join(setupFailures, "; ")

internal/testrunner/runners/system/tester.go:780

• The retry loop now runs after startTesting is captured in run, but checkAgentLogs later scans logs from that original timestamp. Any error-pattern entry emitted by a failed setup attempt is therefore reported again as an additional FailureMsg after a later setup retry passes, so the command can still fail instead of producing the advertised flaky pass. Scope the log check to the final attempt or otherwise exclude prior-attempt logs when the failure is recorded in FlakyMsg.

	return runWithSetupReattempts(ctx, r.setupReattempts, attempt)

• Files reviewed: 9/9 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Issues remain with the default retry behavior, setup cleanup handling, failure classification, and human-readable flaky reporting.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (7)

Previously missed (1) — in code that hasn't changed since the last review.

cmd/testrunner.go:21

• This import block is not gofmt-sorted: internal/environment is placed before internal/common, so the format/lint step will rewrite it. Reorder these imports or run gofmt before merging.

cmd/testrunner.go:488

• The CLI default here is 0, so when neither the flag nor environment variable is set, system tests get no setup re-attempts. The PR contract specifies one re-attempt by default; use 1 here while preserving explicit --setup-reattempts=0 as the opt-out.

	cmd.Flags().Int(cobraext.SetupReattemptsFlagName, 0, cobraext.SetupReattemptsFlagDescription)

docs/howto/system_testing.md:1003

• This documents the opposite default from the requested behavior: one setup re-attempt should be enabled by default, with zero disabling it. Please update the sentence so users are not told that retries are off unless they explicitly enable them.

re-attempted from scratch. Setup re-attempts are disabled by default; enable
them with `--setup-reattempts=N` (maximum 5). For example:

internal/testrunner/runners/system/tester.go:2208

• This comment overstates the validation contract: validateTestScenario can return non-nil errors for expected-dataset resolution, repository/validator setup, and other runtime failures; only its ErrTestCaseFailed results are captured in FailureMsg. Narrow the comment so future changes do not assume every validation error follows the same result path.

		// validateTestScenario failures are never returned as Go errors — they
		// are captured in result.FailureMsg via result.WithError — so there is
		// no risk of retrying real validation failures here.

internal/testrunner/runners/system/tester.go:811

• tdErr only reports errors from tearDownTest. If setupService or setupAgent fails inside its deployer's SetUp, the deployer runs cleanup before returning but logs and suppresses cleanup errors, and the tester never registers a teardown handler. This condition can therefore be true even after setup cleanup failed, causing a retry against a potentially dirty environment despite the no-retry-on-teardown-failure guarantee. Propagate setup-time cleanup failures into this decision or otherwise make that failure non-retryable.

			if attemptNum < maxAttempts && ctx.Err() == nil && tdErr == nil {

internal/testrunner/runners/system/tester.go:829

• When a later attempt passes, this only populates FlakyMsg. The human reporter still renders every successful result as the literal PASS and never reads this field, so the required PASS (flaky: …) signal is absent from the default human output. Update the human formatter to include the flaky message for passed results.

		if len(setupFailures) > 0 && len(partial) > 0 && testPassed(partial[0]) {
			partial[0].FlakyMsg = strings.Join(setupFailures, "; ")
		}

internal/testrunner/testrunner.go:123

• FlakyMsg is not rendered by the human reporter, which still emits plain PASS for results without FailureMsg. This hides the earlier setup failures in the default output and misses the requested PASS (flaky: …) presentation; add a FlakyMsg branch to the human formatter.

	// If the test passed only after being re-attempted, description of the
	// failures observed in previous attempts. The test is reported as
	// passed, but flagged as flaky in reports that support it.
	FlakyMsg string

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Retry defaults, teardown handling, error classification, and human flaky-pass reporting still have unresolved issues.

Review details

Suppressed comments (5)

cmd/testrunner.go:488

• The requested default is one re-attempt, but this flag is initialized to 0, so normal CLI runs never enter the retry loop unless the user opts in. This conflicts with the PR description; use 1 as the flag default while retaining an explicit 0 to disable retries.

	cmd.Flags().Int(cobraext.SetupReattemptsFlagName, 0, cobraext.SetupReattemptsFlagDescription)

internal/testrunner/runners/system/tester.go:811

• This clean-retry check only observes tearDownTest's handlers. When a deployer SetUp fails, those handlers are registered only after SetUp succeeds, while the deployers' deferred cleanup logs and suppresses TearDown errors (internal/servicedeployer/compose.go:138-157 and internal/agentdeployer/agent.go:180-200). A failed cleanup can therefore leave tdErr nil and this code retries in a dirty environment, contrary to the stop-on-teardown-failure requirement; propagate those cleanup errors or mark the attempt non-retryable.

			if attemptNum < maxAttempts && ctx.Err() == nil && tdErr == nil {

internal/testrunner/runners/system/tester.go:828

• This annotation is not consumed by the default human reporter: reportHumanFormat only checks ErrorMsg, FailureMsg, and Skipped, then renders plain PASS when those are empty. A test that passes after a setup re-attempt therefore never shows the promised PASS (flaky: …); update the human formatter (and its test) to render FlakyMsg.

			partial[0].FlakyMsg = strings.Join(setupFailures, "; ")

internal/testrunner/runners/system/tester.go:2213

• This catch-all classifies every prepareScenario error other than ErrTestCaseFailed as retryable setup failure. For example, waitForDocs returns plain errors for invalid assert.hit_count/assert.min_count values above the query limit, so a deterministic malformed test configuration is retried up to N times instead of being reported once; add a non-retryable classification for such config/data errors before wrapping environmental failures.

		return results, errSetupFailed{err: err}

internal/testrunner/testrunner.go:123

• FlakyMsg is never consumed by the human formatter (internal/testrunner/reporters/formats/human.go:53-63), which still emits plain PASS. A pass after setup retries therefore misses the promised PASS (flaky: …) signal; add the human-rendering branch and test it.

	FlakyMsg string

• Files reviewed: 11/11 changed files
• Comments generated: 0 new
• Review effort level: Lite

[infra-vault-gh-plugin-prod]

⏳ Build in-progress, with failures

• Buildkite Build
• Commit: 8bb884f

Failed CI Steps

• :go: Integration test (false positive): docker_failing_test_service

History

• 💔 Build #8747 failed 45e8024
• 💔 Build #8746 failed 26e425a
• 💚 Build #8735 succeeded a6c50f7

cc @kcreddy

[github-actions]

TL;DR

Buildkite failed in :go: Integration test (false positive): docker_failing_test_service because the expected false-positive pattern no longer matches the xUnit output after this PR changed setup-phase service-exit handling from a test failure (<failure>test case failed: ...</failure>) to a test error (<error>the test service ...</error>).

Remediation

• Update test/packages/false_positives/docker_failing_test_service.expected_errors:1 to match the new xUnit shape emitted by this PR (replace <failure>test case failed: ...</failure> with <error>the test service ...</error>, keeping regex-safe escaping).
• Re-run ./.buildkite/scripts/integration_tests.sh -t test-check-packages-false-positives -p docker_failing_test_service (or the Buildkite step) after updating the fixture.

Investigation details

Root Cause

The PR changed the system test setup-path error classification:

• internal/testrunner/runners/system/tester.go:1259 now returns a plain error for service exit during setup:
  • return fmt.Errorf("the test service %s unexpectedly exited with code %d", config.Service, code)
• The false-positive fixture still expects the old ErrTestCaseFailed failure text:
  • test/packages/false_positives/docker_failing_test_service.expected_errors:1
  • expects <failure>test case failed: the test service failing unexpectedly exited with code 1</failure>
• scripts/test-check-false-positives.sh:74 validates by grepping each expected regex line against generated XML; once text/element shape diverges, the script exits non-zero.

So this is a test fixture drift caused by the PR’s intended behavior change, not infra flakiness.

Evidence

• Build: https://buildkite.com/elastic/elastic-package/builds/8748
• Job/step: :go: Integration test (false positive): docker_failing_test_service
• Key log excerpt:
  • make: *** [Makefile:119: test-check-packages-false-positives] Error 1
  • make SERVERLESS=false PACKAGE_UNDER_TEST=docker_failing_test_service test-check-packages-false-positives failed with 2

Verification

• Not run locally (this step requires Docker-based integration stack in CI).

Follow-up

• I checked for an existing flaky-test issue for docker_failing_test_service and found none to reference.

---

What is this? | From workflow: PR Buildkite Detective

Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.

[Copilot]

🟡 Changes recommended

Restrict retries to environment errors and report exhausted service exits as xUnit errors.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

internal/testrunner/runners/system/tester.go:798

• Only ErrTestCaseFailed is excluded here, so every other prepareScenario error becomes retryable. However, prepareScenario also emits deterministic package/config errors such as missing or ambiguous policy templates (tester.go:1351-1360) and invalid built package policy (1421-1423). With this flag enabled, those errors are repeated up to five times instead of restricting retries to environment failures, adding teardown/setup churn and delaying the real failure. Add an explicit retryable marker around only environment operations and leave package/config errors non-retryable.

	return errSetupFailed{err: err}

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite