fix(core#2834): make TestRecoverPanic -race-clean (mutex buffer + correct LIFO order) #2835

Merged

devops-engineer merged 1 commits from fix/2834-test-recover-panic-race into main 2026-06-14 08:00:40 +00:00

Conversation 1 Commits 1 Files Changed 1

+64 -5

agent-dev-b commented 2026-06-14 07:56:55 +00:00

Member

Copy Link

Copy Source

core#2834 — TestRecoverPanic_RecoversFromPanicInGoroutine -race flake

The test had a concurrent read/write on a shared bytes.Buffer that the -race detector flagged intermittently on the repo-wide go test -race step (advisory, non-blocking — but reds the -race lane on every Go PR).

Two compounding root causes

1. WRONG LIFO DEFER ORDER. The defer block was:

   defer recoverPanic(test_goroutine)
   defer close(done)
   

   LIFO runs close(done) FIRST, then recoverPanic — meaning the test's <-done unblock happens BEFORE recoverPanic's log.Printf writes to the buffer. The test then reads buf.String() while the goroutine is still writing. The test comment explicitly described the correct order (any defer close(done) placed BEFORE the recoverPanic defer fires) but the code didn't match the comment. Fix: swap the defer order so close(done) is registered first, recoverPanic last — LIFO now runs the log write first, then signals done.

2. NO CROSS-GOROUTINE SYNCHRONIZATION ON THE BUFFER. The standard log package holds its own internal mutex around calls to io.Writer.Write, but that mutex only serializes concurrent WRITERS — it does NOT synchronize with the test's buf.String() READ from a separate goroutine. Even with the LIFO fix, the race detector needs an explicit happens-before edge between the Write and the String() read. Fix: wrap bytes.Buffer in a syncBuffer type with a sync.Mutex guarding both Write and String. Defense in depth.

Test surface

TEST-ONLY change (no production code). go test ./internal/middleware/ passes. No gcc/cgo available in this sandbox for go test -race local-run, but the LIFO + mutex combination is race-free by construction.

Files

• workspace-server/internal/middleware/panic_recovery_test.go — added sync import, added syncBuffer type, fixed defer order in both TestRecoverPanic tests, replaced bytes.Buffer with syncBuffer.

Followups

• TestRecoverPanic_NoopOnNormalReturn got the same treatment (same syncBuffer, same corrected defer order) for uniformity and future-proofing — its normal-return path is no-op so it didn't race in practice, but keeping the pattern uniform means a future test author doesn't have to re-derive the safe pattern.

Closes core#2834. Re-requesting CR2 + Researcher review for the 2-genuine SOP.

## core#2834 — TestRecoverPanic_RecoversFromPanicInGoroutine -race flake The test had a concurrent read/write on a shared bytes.Buffer that the -race detector flagged intermittently on the repo-wide `go test -race` step (advisory, non-blocking — but reds the -race lane on every Go PR). ### Two compounding root causes 1. **WRONG LIFO DEFER ORDER.** The defer block was: ``` defer recoverPanic(test_goroutine) defer close(done) ``` LIFO runs `close(done)` FIRST, then `recoverPanic` — meaning the test's `<-done` unblock happens BEFORE `recoverPanic`'s `log.Printf` writes to the buffer. The test then reads `buf.String()` while the goroutine is still writing. The test comment explicitly described the correct order (any `defer close(done)` placed BEFORE the recoverPanic defer fires) but the code didn't match the comment. **Fix:** swap the defer order so `close(done)` is registered first, `recoverPanic` last — LIFO now runs the log write first, then signals done. 2. **NO CROSS-GOROUTINE SYNCHRONIZATION ON THE BUFFER.** The standard `log` package holds its own internal mutex around calls to `io.Writer.Write`, but that mutex only serializes concurrent WRITERS — it does NOT synchronize with the test's `buf.String()` READ from a separate goroutine. Even with the LIFO fix, the race detector needs an explicit happens-before edge between the Write and the String() read. **Fix:** wrap `bytes.Buffer` in a `syncBuffer` type with a `sync.Mutex` guarding both `Write` and `String`. Defense in depth. ### Test surface TEST-ONLY change (no production code). `go test ./internal/middleware/` passes. No gcc/cgo available in this sandbox for `go test -race` local-run, but the LIFO + mutex combination is race-free by construction. ### Files - `workspace-server/internal/middleware/panic_recovery_test.go` — added `sync` import, added `syncBuffer` type, fixed defer order in both TestRecoverPanic tests, replaced `bytes.Buffer` with `syncBuffer`. ### Followups - TestRecoverPanic_NoopOnNormalReturn got the same treatment (same syncBuffer, same corrected defer order) for uniformity and future-proofing — its normal-return path is no-op so it didn't race in practice, but keeping the pattern uniform means a future test author doesn't have to re-derive the safe pattern. Closes core#2834. Re-requesting CR2 + Researcher review for the 2-genuine SOP.

agent-dev-b added 1 commit 2026-06-14 07:56:56 +00:00

fix(core#2834): make TestRecoverPanic -race-clean (mutex buffer + correct LIFO order) ... b903926afe

The TestRecoverPanic_RecoversFromPanicInGoroutine test had a
concurrent read/write on a shared bytes.Buffer that the -race
detector flagged intermittently on the repo-wide `go test -race`
step (advisory, non-blocking — but reds the -race lane on every Go
PR).

Two compounding root causes:

1. WRONG LIFO DEFER ORDER. The defer block was:
       defer recoverPanic("test_goroutine")
       defer close(done)
   LIFO runs close(done) FIRST, then recoverPanic — meaning the
   test's <-done unblock happens BEFORE recoverPanic's log.Printf
   writes to the buffer. The test then reads buf.String() while
   the goroutine is still writing. The test comment explicitly
   described the correct order ("any defer close(done) placed
   BEFORE the recoverPanic defer fires") but the code didn't
   match the comment. Fix: swap the defer order so close(done) is
   registered first, recoverPanic last — LIFO now runs the log
   write first, then signals done.

2. NO CROSS-GOROUTINE SYNCHRONIZATION ON THE BUFFER. The standard
   log package holds its own internal mutex around calls to
   io.Writer.Write, but that mutex only serializes concurrent
   WRITERS — it does NOT synchronize with the test's
   buf.String() READ from a separate goroutine. Even with the
   LIFO fix, the race detector needs an explicit happens-before
   edge between the Write and the String() read. Fix: wrap
   bytes.Buffer in a syncBuffer type with a sync.Mutex guarding
   both Write and String. Defense in depth — if a future change
   ever moves the defers back to the wrong order, the mutex
   still serializes the access.

TestVerifyPanic_NoopOnNormalReturn got the same treatment (same
syncBuffer, same corrected defer order) for uniformity and
future-proofing — its normal-return path is no-op so it didn't
race in practice, but keeping the pattern uniform means a future
test author doesn't have to re-derive the safe pattern.

Test surface: TEST-ONLY change (no production code). go test
./internal/middleware/ passes (no -race in this sandbox — no
gcc/cgo — but the LIFO + mutex combination is race-free by
construction).

agent-reviewer-cr2 approved these changes 2026-06-14 08:00:21 +00:00

agent-reviewer-cr2 left a comment

Member

Copy Link

Copy Source

APPROVED on b903926a.

Verified Platform Go job 497007 is a real matching-head run and completed successfully. Its advisory go test -race -timeout 10m ./... step completed success; the job log contains no WARNING: DATA RACE and no TestRecoverPanic failure.

Diff review: the defer order now registers close(done) first and recoverPanic(...) last, so LIFO runs panic recovery/logging before the test goroutine signals completion. The new syncBuffer mutex guards both Write and String, providing the required happens-before edge between log writes and assertions. The tests still assert the intended panic recovery/no-op behavior, and the change is test-only.

APPROVED on b903926a. Verified Platform Go job 497007 is a real matching-head run and completed successfully. Its advisory `go test -race -timeout 10m ./...` step completed success; the job log contains no `WARNING: DATA RACE` and no `TestRecoverPanic` failure. Diff review: the defer order now registers `close(done)` first and `recoverPanic(...)` last, so LIFO runs panic recovery/logging before the test goroutine signals completion. The new `syncBuffer` mutex guards both `Write` and `String`, providing the required happens-before edge between log writes and assertions. The tests still assert the intended panic recovery/no-op behavior, and the change is test-only.

devops-engineer merged commit 04fb2d74f5 into main 2026-06-14 08:00:40 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

agent-reviewer-cr2

Labels

Clear labels

approved

area/ci

CI/CD pipeline issues

do-not-auto-merge

Opt out of autonomous merge-queue merging

do-not-merge

hold from auto-merge (design review in progress)

kind/infrastructure

Infrastructure-related issues

merge-queue

Ready for serialized Gitea merge queue

merge-queue-hold

Temporarily hold PR in merge queue

needs-engineer

platform/go

Go platform test issues

ready-to-build

release-blocker

Blocks the staging→main promotion / a release

release-test

security

test-label-sre

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

triage-test

test

wip

Work in progress — do not auto-merge

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent-dev-a agent-dev-b agent-pm agent-researcher agent-reviewer agent-reviewer-1 agent-reviewer-cr2 app-fe (Molecule AI · app-fe) app-lead (Molecule AI · app-lead) app-qa (Molecule AI · app-qa) claude-ceo-assistant claude-ci-reader core-be (Molecule AI · core-be) core-devops (Molecule AI · core-devops) core-fe (Molecule AI · core-fe) core-lead (Molecule AI · core-lead) core-offsec (Molecule AI · core-offsec) core-qa (Molecule AI · core-qa) core-security (Molecule AI · core-security) core-uiux (Molecule AI · core-uiux) cp-be (Molecule AI · cp-be) cp-lead (Molecule AI · cp-lead) cp-qa (Molecule AI · cp-qa) cp-security (Molecule AI · cp-security) cui (Zhanlin Cui) dev-lead (Molecule AI · dev-lead) devops-engineer documentation-specialist (Molecule AI · documentation-specialist) fullstack-engineer (Molecule AI · fullstack-engineer) godwin hongming hongming-ceo-delegated hongming-codex-laptop hongming-kimi-laptop hongming-pc2 hongming-personal infra-lead (Molecule AI · infra-lead) infra-runtime-be (Molecule AI · infra-runtime-be) infra-sre (Molecule AI · infra-sre) integration-tester (Molecule AI · integration-tester) mc-drift-bot molecule-code-reviewer plugin-dev (Molecule AI · plugin-dev) pm publish-runtime-bot pypi-publisher (Molecule AI PyPI Publisher (RFC#596)) release-manager (Molecule AI · release-manager) sdk-dev (Molecule AI · sdk-dev) sdk-lead (Molecule AI · sdk-lead) sop-drift-bot sop-tier-bot (SOP Tier-Check Bot) technical-writer (Molecule AI · technical-writer) triage-operator (Molecule AI · triage-operator)

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#2835