Skip to content

buck2_test: don't poison results channel via internal orchestrator drop - #1461

Open
vlad-ivanov-name wants to merge 1 commit into
facebook:mainfrom
vlad-ivanov-name:fix-internal-orchestrator-teardown
Open

buck2_test: don't poison results channel via internal orchestrator drop#1461
vlad-ivanov-name wants to merge 1 commit into
facebook:mainfrom
vlad-ivanov-name:fix-internal-orchestrator-teardown

Conversation

@vlad-ivanov-name

@vlad-ivanov-name vlad-ivanov-name commented Aug 13, 2026

Copy link
Copy Markdown

Any buck2 test run in which at least one target used InternalRunnerTestInfo fails at teardown with

Did not receive all results from executor: BuckTestOrchestrator
exited before end-of-tests was received

even though all per-test results were already reported correctly.

Root cause: drop order in test_targets.

Fix: drop the internal orchestrator after try_fold.

This fix also surfaces other issue so the next fix is the exit code for failing internal-runner tests: they showed Fail in the UI but buck2 test exited 0 (green CI on red tests).

Reproducer

.buckconfig

[cells]
root = .

defs.bzl

def _listing(content: str) -> list[dict]:
    return [{"name": n, "filter": n} for n in content.split()]

def _result(stdout: str, stderr: str, exit_code: int) -> list[dict]:
    # empty -> the runner synthesizes PASS/FAIL from the exit code
    return []

def _impl(ctx):
    return [DefaultInfo(), InternalRunnerTestInfo(
        type = "sh",
        listing_command = ["/bin/sh", "-c", "echo alpha beta"],
        command = ["/bin/sh", "-c", "exit 0"],
        parse_test_listing = _listing,
        parse_test_result = _result,
    )]

internal_test = rule(impl = _impl, attrs = {})

BUCK

load(":defs.bzl", "internal_test")

internal_test(name = "t")
Before
$ buck2 test //:t; echo $?
✓ Pass: alpha (0.0s)
✓ Pass: beta (0.0s)
Command failed:
Test executor exited unexpectedly with status 1.
Stdout:

Stderr:
Command failed: code: 'Unknown error', message: "Failed to report end-of-tests: end_of_tests was received twice (internal error)"
1
After
$ buck2 test //:t; echo $?
Tests finished: Pass 2. Fail 0. Timeout 0. Fatal 0. Skip 0. Omit 0. Infra Failure 0. Build failure 0
0

With a failing command (exit 1 for test beta):

$ buck2 test //:t; echo $?
Tests finished: Pass 1. Fail 1. Timeout 0. Fatal 0. Skip 0. Omit 0. Infra Failure 0. Build failure 0
1 TESTS FAILED
  ✗ beta (<unspecified>)
32

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 13, 2026
@meta-codesync

meta-codesync Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

This pull request has been imported. If you are a Meta employee, you can view this in D115868652. (Because this pull request was imported automatically, there will not be any future comments.)

@vlad-ivanov-name
vlad-ivanov-name marked this pull request as draft August 13, 2026 14:00
@vlad-ivanov-name

vlad-ivanov-name commented Aug 13, 2026

Copy link
Copy Markdown
Author

Turns out this requires some adjustments in how exit code is derived. I will fix that and mark it ready for review after.

Edit: fixed

Any `buck2 test` run in which at least one target used InternalRunnerTestInfo fails at teardown with

    Did not receive all results from executor: BuckTestOrchestrator
    exited before end-of-tests was received

even though all per-test results were already reported correctly.

Root cause: drop order in test_targets.

Fix: drop the internal orchestrator after try_fold.

Also fixed: failing internal-runner tests showed Fail in the UI but
`buck2 test` exited 0. The internal runner cannot call
end_of_test_results (the external executor owns that call), so its
results never fed the exit code. ExecutorReport.exit_code() now derives
the verdict: the executor's code when nonzero, else 32 (matching
RunVerdict::Fail) if any failing status was reported. SKIP/OMITTED do
not fail a run.
@vlad-ivanov-name
vlad-ivanov-name force-pushed the fix-internal-orchestrator-teardown branch from 8d94998 to 6997fb9 Compare August 13, 2026 15:22
@vlad-ivanov-name
vlad-ivanov-name marked this pull request as ready for review August 13, 2026 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant