Skip to content

test(build): fix data race in TestBuildLayer under -race - #592

Merged
bergwolf merged 2 commits into
mainfrom
fix-buildlayer-test-race
Oct 10, 2026
Merged

bergwolf merged 2 commits into
mainfrom
fix-buildlayer-test-race

Conversation

@aftersnow

Copy link
Copy Markdown
Contributor

Summary

Fix a data race in TestBuilderSuite/TestBuildLayer reported by go test -race ./pkg/backend/build/.... Test-only change; production code is untouched.

Root cause

BuildLayer hands OutputLayer a live *io.PipeReader whose backing tar-writer goroutine (archiver.Tar) is mid-write. The subtest uses the generated testify mock for OutputStrategy, and testify's mock.Arguments.Diff formats every actual argument with fmt (fmt.Sprintf("(%[1]T=%[1]v)", actual)) to match the call. Formatting the *io.PipeReader reflects into the pipe's internal sync.Mutex state concurrently with the writer goroutine's Lock() — the reported race.

Write by goroutine (tar): io.(*pipe).write -> sync.(*Mutex).Lock
Read  by goroutine (test): fmt.Sprintf -> reflect.Value.Interface  <- testify Arguments.Diff

No mock matcher avoids it: Diff formats the actual argument before any matcher logic runs, so mock.Anything / AnythingOfType / MatchedBy make no difference.

Fix

Replace the testify mock in the "successful build layer" subtest with a small hand-written OutputStrategy that reads the reader (exactly as the real localOutput/remoteOutput strategies do). This avoids reflecting over the live pipe and unblocks the writer goroutine, while still asserting the call arguments and returned descriptor.

Notes

  • Production is unchanged. remoteOutput.OutputLayer relies on the reader's concrete *io.PipeReader type (to drain it when a blob already exists), so wrapping the reader was deliberately avoided.
  • Pre-existing: this race is present on main today and is not currently caught by CI (the test targets don't pass -race).

Test plan

  • go test -race -count=20 -run TestBuilderSuite/TestBuildLayer ./pkg/backend/build/... — clean (was failing).
  • go test -race ./... — all packages clean.
  • go vet ./... / gofmt clean.

BuildLayer hands OutputLayer a live *io.PipeReader backed by a background
tar writer goroutine. The generated testify mock formats every argument
with fmt (mock.Arguments.Diff) to match the call, which reflects into the
pipe's internal mutex concurrently with the writer -- a data race under
`go test -race`. No matcher avoids it: Diff formats the actual argument
before any matcher logic runs.

Replace the testify mock in the "successful build layer" subtest with a
small hand-written OutputStrategy that reads the reader (as the real
local/remote strategies do). This avoids reflecting over the live pipe
and unblocks the writer goroutine. Production is unchanged.

Claude-Session: https://claude.ai/code/session_01HHaMSeWpe3apQEx7n4UvRg
Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces a testify mock with a hand-written recordingStrategy in builder_test.go to resolve a data race caused by concurrent formatting of a live *io.PipeReader. The feedback recommends restoring the original s.builder.strategy at the end of the subtest to prevent test pollution, and initializing the embedded OutputStrategy interface to avoid potential nil pointer dereference panics.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread pkg/backend/build/builder_test.go Outdated
Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
@bergwolf
bergwolf enabled auto-merge (squash) October 10, 2026 14:11
@bergwolf
bergwolf merged commit 10a428a into main Oct 10, 2026
5 checks passed
@bergwolf
bergwolf deleted the fix-buildlayer-test-race branch October 10, 2026 14:15
aftersnow added a commit that referenced this pull request Oct 10, 2026
The merge of main in 9909b70 brought in the recordingStrategy helper from
#592, which uses io.Reader and io.Copy, but the io import was not added.
golangci-lint and go vet fail with "undefined: io".

Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com>
bergwolf pushed a commit that referenced this pull request Oct 10, 2026
The second commit of #592 moved recordingStrategy into builder_test.go.
The helper uses io.Reader and io.Copy, but the file does not import io.
As a result, go vet and golangci-lint fail on main with "undefined: io",
and the Lint and CI workflows for commit 10a428a are red.

Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com>
bergwolf added a commit that referenced this pull request Oct 10, 2026
…e-learning trees (#530)

* feat(modelfile): support TF SavedModel, ONNX external_data, and online-learning trees

Generator now correctly classifies extension-less TF SavedModel literals
(feature_map, checkpoint) as MODEL via new ModelFilePatterns entries, and
deterministically reclassifies all ONNX external_data tensor files as MODEL by
parsing the .onnx protobuf with google.golang.org/protobuf/encoding/protowire.

This removes the size-heuristic dependency that previously misclassified small
external tensor files as CODE. Online-learning directories (a parent dir
containing base/<ts> and delta/<ts> subtrees) are handled by existing recursive
walk; new test cases mirror the real workflow_10062365 layout.

No changes to build/push/pull/attach/upload/codec/storage paths --- those remain
byte-level and model-type-agnostic.

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

* fix(modelfile): address codex review on ONNX external_data handling

Three follow-up fixes from independent review of PR #530:

P1 (correctness/security): reclassifyONNXExternalData now only moves paths
already collected by the workspace walker, naturally inheriting all of the
walker's filtering: ExcludePatterns, isSkippable directories, file count /
size limits, and the workspace boundary. Adversarial or malformed ONNX
referencing `../escape` is silently dropped instead of being added to
mf.model.

P2 (coverage): the ONNX parser now recurses into NodeProto.attribute so
external_data references attached via Constant ops (attribute.t / tensors /
sparse_tensor / sparse_tensors) and inside If / Loop / Scan subgraphs
(attribute.g / graphs) are discovered. Subgraph recursion is bounded at 32
levels to defend against pathological inputs.

P3 (visibility): ONNX parse failures now print a clearly prefixed WARNING
line that names the offending file and explains the fallback ("external
tensor files will keep walker-assigned classification"). Generate does NOT
abort -- a single bad .onnx degrades to the pre-fix walker classification
without killing the whole pass.

New tests: NodeAttributeTensor, Subgraph (P2 unit); PathTraversalIgnored,
RespectsExclude, ParseFailureFallsBack (P1/P3 integration). go vet, go test
-race ./pkg/modelfile/..., and go test ./... all clean.

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

* fix(modelfile): cover ModelProto.training_info graphs in ONNX parser

Codex review on PR #530 noted a remaining coverage gap: ExtractONNXExternal-
DataPaths only walked ModelProto.graph (field 7), missing external_data
references in training-enabled ONNX (IR v7+) where weights/states can also
appear in ModelProto.training_info[*].initialization (field 1) and
.algorithm (field 2) GraphProtos.

Refactor ExtractONNXExternalDataPaths to iterate ModelProto fields directly
via forEachField rather than the now-removed readSubMessage helper. Both
graph and training_info subtrees route into the same walkGraph entry, so
all the existing recursion (initializer / sparse_initializer / NodeProto
attribute / subgraph) applies uniformly to inference and training graphs.

Tests: TrainingInfoGraphs covers a training-only model;
InferenceAndTrainingCombined verifies both surfaces are merged.

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

* fix(modelfile): reject absolute ONNX external_data paths, assert WARNING text, fix doc

Three follow-up findings from a second codex review pass on PR #530:

1. Reject absolute external_data.location values: filepath.Join(onnxDir, /decoy)
   silently strips the leading separator and produces a relative decoy path,
   which would reclassify an unrelated workspace file. Add filepath.IsAbs(ext)
   guard before Join.

2. Lock the WARNING contract: ParseFailureFallsBack now captures os.Stderr and
   asserts the warning prefix, file path, and fallback note are printed.

3. Update ExtractONNXExternalDataPaths godoc to enumerate the real error
   sources: I/O, size cap, and malformed protobuf wire data.

go vet, go test -race ./pkg/modelfile/..., and go test ./... all clean.

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

* feat(modelfile): infer Format from MODEL set evidence

Best-effort fill of mf.format when --format is not passed. Inference
keys off uniquely diagnostic signals only:

  saved_model.pb / saved_model.pbtxt -> tensorflow
  *.onnx                              -> onnx
  *.gguf                              -> gguf
  *.safetensors                       -> safetensors

Generic .bin/.pt/.pth are intentionally NOT signals: they appear in
many formats and would produce false positives on existing repos.

Failure (no signal, panic on a malformed hashset value, etc.) leaves
mf.format empty and degrades silently with a stderr WARNING. CLI
--format always wins over inference. Format remains best-effort
metadata; downstream build/push/pull paths already handle it being
blank, so this never blocks generation.

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

* fix(modelfile): scan all walker buckets for inferFormat signals

Codex review (P2) caught that saved_model.pbtxt is not in
ModelFilePatterns -- the walker lands it in CODE/DOC, so the prior
inferFormat (which only scanned mf.model) silently missed the textual
SavedModel variant. A workspace whose only TF signal was a .pbtxt
would yield format="" instead of "tensorflow".

Fix: extract the per-file scan into a closure and apply it to all four
walker buckets (model + config + code + doc). Set-based: duplicates
across buckets are harmless. Walker classification is unchanged.

Adds TestNewModelfileByWorkspace_InferFormatSavedModelPbtxt as
regression coverage and updates the doc comment to spell out the
multi-bucket scan and the rationale.

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

* fix(deps): update grpc to patched release

Co-authored-by: bergwolf <88972+bergwolf@users.noreply.github.com>

* test(build): add missing io import in builder_test.go

The merge of main in 9909b70 brought in the recordingStrategy helper from
#592, which uses io.Reader and io.Copy, but the io import was not added.
golangci-lint and go vet fail with "undefined: io".

Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com>

---------

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>
Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: bergwolf <88972+bergwolf@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants