Enable Go race detector in test scripts and update dev docs by clarabennettdev · Pull Request #335 · kubernetes-sigs/agent-sandbox

clarabennettdev · 2026-02-20T16:05:26Z

Description

Fixes #331

This PR enables Go's -race flag in the test tooling to detect data races in concurrent controller code.

Changes

dev/tools/test-unit: Added -race flag to go test invocation
dev/tools/test-e2e: Added -race flag to Go e2e test invocation
docs/development.md: Added Testing section documenting race detection usage and runtime overhead caveats

Fixes kubernetes-sigs#331 - Added -race flag to dev/tools/test-unit - Added -race flag to dev/tools/test-e2e (Go e2e tests) - Updated docs/development.md with Testing section documenting race detection usage and runtime overhead caveats Signed-off-by: Clara Bennett <clarabennett2626@gmail.com>

netlify · 2026-02-20T16:05:33Z

✅ Deploy Preview for agent-sandbox canceled.

Name	Link
🔨 Latest commit	`83fc8ab`
🔍 Latest deploy log	https://app.netlify.com/projects/agent-sandbox/deploys/69a33fed6d8d120008db0cf4

linux-foundation-easycla · 2026-02-20T16:05:35Z

The committers listed above are authorized under a signed CLA.

✅ login: clarabennettdev / name: Clara Bennett (83fc8ab, 8d9c6c6, 9c86361)

k8s-ci-robot · 2026-02-20T16:05:35Z

Welcome @clarabennett2626!

It looks like this is your first PR to kubernetes-sigs/agent-sandbox 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/agent-sandbox has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2026-02-20T16:05:36Z

Hi @clarabennett2626. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

janetkuo · 2026-02-20T23:24:46Z

/ok-to-test

janetkuo · 2026-02-20T23:26:25Z

docs/development.md

+
+Both `test-unit` and `test-e2e` run with Go's `-race` flag enabled by default to detect data races in concurrent code. This is especially important because the controllers (`SandboxReconciler`, `SandboxClaimReconciler`, `SandboxWarmPoolReconciler`) run concurrently via controller-runtime.
+
+Note that enabling the race detector increases memory usage (5-10×) and execution time (2-20×). If you need to disable it for local development (e.g., resource-constrained environments), you can run `go test` directly without the `-race` flag.


@justinsb will this be a concern?

How about making -race available for local tests (e.g. add a flag to dev/tools/test-e2e and then have a new Make target test-e2e-race; same for unit tests) for now?

It might be fine to enable -race for unit tests in CI. We can run a separate periodic job for e2e with -race to avoid slowing down PRs.

janetkuo · 2026-02-21T00:03:10Z

e2e test failure shows data race (TestRunChromeSandbox, TestRunPythonRuntimeSandbox, TestRunPythonRuntimeSandboxClaim, TestRunPythonRuntimeSandboxWarmpool), which proves that the race detector is working.

janetkuo · 2026-02-21T00:26:09Z

docs/development.md

+
+```sh
+./dev/tools/test-e2e
+```


make is more user friendly. We also have https://github.com/kubernetes-sigs/agent-sandbox/blob/main/docs/testing.md

janetkuo · 2026-02-21T01:19:11Z

docs/development.md

+
+Both `test-unit` and `test-e2e` run with Go's `-race` flag enabled by default to detect data races in concurrent code. This is especially important because the controllers (`SandboxReconciler`, `SandboxClaimReconciler`, `SandboxWarmPoolReconciler`) run concurrently via controller-runtime.
+
+Note that enabling the race detector increases memory usage (5-10×) and execution time (2-20×). If you need to disable it for local development (e.g., resource-constrained environments), you can run `go test` directly without the `-race` flag.


How about making -race available for local tests (e.g. add a flag to dev/tools/test-e2e and then have a new Make target test-e2e-race; same for unit tests) for now?

It might be fine to enable -race for unit tests in CI. We can run a separate periodic job for e2e with -race to avoid slowing down PRs.

yashasvimisra2798 · 2026-02-21T20:03:39Z

@clarabennettdev Thanks for putting this up, the race detector surfacing these failures is really helpful. I’d be happy to coordinate and take a pass at stabilizing one of the failing e2e tests and open a small follow-up PR with a fix.
Let me know if you’d prefer I start with a specific test

clarabennettdev · 2026-02-21T20:53:31Z

thanks @yashasvimisra2798 - sounds good, i'm down to coordinate. race detectors doing what it's supposed to, which is kinda the point anyway. ping me if you want a hand with the follow-up PR

Two race conditions were found by the race detector enabled in PR kubernetes-sigs#335: 1. PortForward (framework/client.go): bytes.Buffer was shared between the goroutine copying cmd stdout/stderr and the main goroutine polling the buffer via String(). Fixed by introducing a syncBuffer type that wraps bytes.Buffer with a mutex. 2. ChromeSandboxMetrics (chromesandbox_test.go): t.Logf with %+v used reflection to read AtomicTimeDuration fields directly, bypassing atomic accessors. Fixed by adding a String() method to ChromeSandboxMetrics that reads all fields through their atomic accessors, and using %s format instead of %+v.

k8s-ci-robot · 2026-02-21T20:56:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: clarabennettdev
Once this PR has been reviewed and has the lgtm label, please ask for approval from janetkuo. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

clarabennettdev · 2026-02-21T20:57:01Z

thanks @yashasvimisra2798 for jumping in. i just pushed a fix for the race conditions

the race detector flagged two actual issues:

PortForward in framework/client.go: we had a bytes.Buffer getting shared between the goroutine copying cmd stdout/stderr and the main goroutine polling it via String(). so reads/writes were racing. fixed it by adding a syncBuffer wrapper around bytes.Buffer with a mutex
ChromeSandboxMetrics in chromesandbox_test.go: t.Logf with %+v ends up using reflection, which was reading AtomicTimeDuration fields directly and kinda sidestepping the atomic getters. fixed by giving ChromeSandboxMetrics a String() that reads everything via the atomic accessors

so yeah, race detector is doing its job — it caught real bugs!

janetkuo

@clarabennettdev Thanks for the quick turnaround on fixing those data races in the 4 failed e2e tests!

To move this forward, could you please address a few remaining items from my earlier review:

It might be fine to enable -race for unit tests in CI by default, but for e2e, we should likely move that to a separate periodic job later to avoid slowing down PRs. Let's default it to off in the script for test-e2e and allow it to be enabled via a flag in Make.
Please add a new Make target (e.g., test-e2e-race) in Makefile to enable the optional -race flag when running e2e tests (it could be used to run e2e tests locally or in a periodic job).
Please add doc changes in the existing docs/testing.md instead of docs/development.md, and mention the new make target you added in the previous step.

Per reviewer feedback: - Remove hardcoded -race from dev/tools/test-e2e; it is now opt-in via --race flag to avoid slowing down PR presubmits - Add --race passthrough in dev/ci/presubmits/test-e2e - Add Makefile target test-e2e-race for local use or periodic jobs - Unit tests keep -race enabled by default (dev/tools/test-unit unchanged) - Move race-detector documentation from docs/development.md to docs/testing.md and mention the new make target Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

k8s-ci-robot · 2026-02-28T19:20:18Z

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot · 2026-02-28T19:20:29Z

@clarabennettdev: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
presubmit-agent-sandbox-unit-test	`83fc8ab`	link	true	`/test presubmit-agent-sandbox-unit-test`
presubmit-agent-sandbox-lint-go	`83fc8ab`	link	true	`/test presubmit-agent-sandbox-lint-go`
presubmit-test-autogen-up-to-date	`83fc8ab`	link	true	`/test presubmit-test-autogen-up-to-date`
presubmit-agent-sandbox-e2e-test	`83fc8ab`	link	true	`/test presubmit-agent-sandbox-e2e-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

aditya-shantanu · 2026-03-06T00:25:15Z

This is awesome.
I would very much like to run the e2e tests in race mode as a blocking step.
@janetkuo - I don't want to make this as optional. We can make it as a separate target but IMO this should be a presubmit test.

We can only enforce this after we have fixed the race issues but fixing them is a P0 imo. The fixing part can be outsourced to more ppl to help expedite.

k8s-ci-robot requested review from janetkuo and justinsb February 20, 2026 16:05

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 20, 2026

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 20, 2026

janetkuo reviewed Feb 20, 2026

View reviewed changes

janetkuo self-assigned this Feb 20, 2026

janetkuo reviewed Feb 21, 2026

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 21, 2026

janetkuo reviewed Feb 26, 2026

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2026

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 28, 2026


		Both `test-unit` and `test-e2e` run with Go's `-race` flag enabled by default to detect data races in concurrent code. This is especially important because the controllers (`SandboxReconciler`, `SandboxClaimReconciler`, `SandboxWarmPoolReconciler`) run concurrently via controller-runtime.

		Note that enabling the race detector increases memory usage (5-10×) and execution time (2-20×). If you need to disable it for local development (e.g., resource-constrained environments), you can run `go test` directly without the `-race` flag.

Conversation

clarabennettdev commented Feb 20, 2026

Description

Changes

Uh oh!

netlify bot commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for agent-sandbox canceled.

Uh oh!

linux-foundation-easycla bot commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 20, 2026

Uh oh!

k8s-ci-robot commented Feb 20, 2026

Uh oh!

janetkuo commented Feb 20, 2026

Uh oh!

janetkuo Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

janetkuo Feb 21, 2026

Choose a reason for hiding this comment

Uh oh!

janetkuo commented Feb 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janetkuo Feb 21, 2026

Choose a reason for hiding this comment

Uh oh!

janetkuo Feb 21, 2026

Choose a reason for hiding this comment

Uh oh!

yashasvimisra2798 commented Feb 21, 2026

Uh oh!

clarabennettdev commented Feb 21, 2026

Uh oh!

k8s-ci-robot commented Feb 21, 2026

Uh oh!

clarabennettdev commented Feb 21, 2026

Uh oh!

janetkuo left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Feb 28, 2026

Uh oh!

k8s-ci-robot commented Feb 28, 2026

Uh oh!

aditya-shantanu commented Mar 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

netlify bot commented Feb 20, 2026 •

edited

Loading

linux-foundation-easycla bot commented Feb 20, 2026 •

edited

Loading

janetkuo commented Feb 21, 2026 •

edited

Loading