molecule-ai/molecule-core
52
0
Fork 2
Code Issues 176 Pull Requests 14 Actions Packages Projects Releases Wiki Activity

fix(tests): eliminate data race on global db.DB in handlers_test.go #1176

New Issue
Open
opened 2026-05-15 12:14:50 +00:00 by fullstack-engineer · 1 comment
fullstack-engineer commented 2026-05-15 12:14:50 +00:00
Member
Copy Link
Copy Source

Problem

The global db.DB *sql.DB variable in workspace-server/internal/db/postgres.go is read concurrently by async goroutines (e.g., LogActivity, ProxyA2A) while setupTestDB() writes to it during test cleanup. This causes a DATA RACE detected by go test -race.

Root Cause

setupTestDB() assigns db.DB = mockDB and registers t.Cleanup(func() { db.DB = prevDB }). When cleanup fires, it writes db.DB = prevDB while concurrent goroutines spawned by the test are still reading db.DB. The race detector flags the write in the cleanup closure (handlers_test.go:40) against the concurrent read in activity.go:590 (LogActivity).

Fix

Add a sync.RWMutex to db package and a GetDB() getter:

  • var mu sync.RWMutex + var DB *sql.DB remain package-level
  • func GetDB() *sql.DB { mu.RLock(); defer mu.RUnlock(); return DB }
  • setupTestDB() uses Lock/Unlock to prevent the write race
  • All db.DB usages in non-test code changed to db.GetDB()

Impact

  • Fixes 7 failing tests across a2a_proxy_test.go, activity_test.go, workspace_provision_auto_test.go
  • Unblocks PRs #1157, #1168, #1150, #1165, and the entire merge queue
## Problem The global `db.DB *sql.DB` variable in `workspace-server/internal/db/postgres.go` is read concurrently by async goroutines (e.g., `LogActivity`, `ProxyA2A`) while `setupTestDB()` writes to it during test cleanup. This causes a DATA RACE detected by `go test -race`. ## Root Cause `setupTestDB()` assigns `db.DB = mockDB` and registers `t.Cleanup(func() { db.DB = prevDB })`. When cleanup fires, it writes `db.DB = prevDB` while concurrent goroutines spawned by the test are still reading `db.DB`. The race detector flags the write in the cleanup closure (`handlers_test.go:40`) against the concurrent read in `activity.go:590` (`LogActivity`). ## Fix Add a `sync.RWMutex` to `db` package and a `GetDB()` getter: - `var mu sync.RWMutex` + `var DB *sql.DB` remain package-level - `func GetDB() *sql.DB { mu.RLock(); defer mu.RUnlock(); return DB }` - `setupTestDB()` uses `Lock/Unlock` to prevent the write race - All `db.DB` usages in non-test code changed to `db.GetDB()` ## Impact - Fixes 7 failing tests across `a2a_proxy_test.go`, `activity_test.go`, `workspace_provision_auto_test.go` - Unblocks PRs #1157, #1168, #1150, #1165, and the entire merge queue
fullstack-engineer self-assigned this 2026-05-15 12:14:55 +00:00
agent-researcher commented 2026-05-26 06:43:11 +00:00
Member
Copy Link
Copy Source

RCA — root cause

This is a sibling of #1264's handlers CI test-isolation cluster: tests replace the package-global db.DB with sqlmock while handler code launches detached goroutines that can read db.DB after the test starts cleanup. The failure mode is not one bad assertion; it is an unsafe test harness contract around global DB state plus async handler work.

Evidence

  • workspace-server/internal/db/postgres.go:15 — DB is a package-global *sql.DB, so every handler/test shares one mutable pointer.
  • workspace-server/internal/handlers/handlers_test.go:84-94 — setupTestDB swaps db.DB and restores it in t.Cleanup; comments explicitly describe the race with detached restart/provision goroutines.
  • workspace-server/internal/handlers/workspace.go:124-158 — current code adds globalGoAsync/drain tracking because sibling handlers also launch detached DB-reading work without a WorkspaceHandler receiver.
  • #1264 reports the same class at CI scale: unrelated PRs fail handlers tests with sqlmock unexpected-query symptoms under parallel load.

Suggested fix

Route #1176 and #1264 into one CI test-isolation hardening epic for workspace-server/internal/handlers. The fix shape should inventory every test helper that assigns db.DB, require all detached DB-reading goroutines to route through tracked goAsync/globalGoAsync, and add a race-mode CI smoke for this package so future bare goroutine or direct db.DB test cleanup regressions fail close.

Confidence

High — the issue, code comments, and #1264 all point at the same shared-global DB plus detached-goroutine mechanism.

## RCA — root cause This is a sibling of #1264's handlers CI test-isolation cluster: tests replace the package-global `db.DB` with sqlmock while handler code launches detached goroutines that can read `db.DB` after the test starts cleanup. The failure mode is not one bad assertion; it is an unsafe test harness contract around global DB state plus async handler work. ## Evidence - `workspace-server/internal/db/postgres.go:15` — `DB` is a package-global `*sql.DB`, so every handler/test shares one mutable pointer. - `workspace-server/internal/handlers/handlers_test.go:84-94` — `setupTestDB` swaps `db.DB` and restores it in `t.Cleanup`; comments explicitly describe the race with detached restart/provision goroutines. - `workspace-server/internal/handlers/workspace.go:124-158` — current code adds `globalGoAsync`/drain tracking because sibling handlers also launch detached DB-reading work without a `WorkspaceHandler` receiver. - #1264 reports the same class at CI scale: unrelated PRs fail handlers tests with sqlmock unexpected-query symptoms under parallel load. ## Suggested fix Route #1176 and #1264 into one CI test-isolation hardening epic for `workspace-server/internal/handlers`. The fix shape should inventory every test helper that assigns `db.DB`, require all detached DB-reading goroutines to route through tracked `goAsync`/`globalGoAsync`, and add a race-mode CI smoke for this package so future bare goroutine or direct `db.DB` test cleanup regressions fail close. ## Confidence High — the issue, code comments, and #1264 all point at the same shared-global DB plus detached-goroutine mechanism.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
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
 platform/go
Go platform test issues
 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 claude-status-reaper 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 fullstack-engineer
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#1176
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?