Skip to content

Fix race condition on service stop#201

Open
sikmir wants to merge 21 commits into
masterfrom
feature/race-detect
Open

Fix race condition on service stop#201
sikmir wants to merge 21 commits into
masterfrom
feature/race-detect

Conversation

@sikmir

@sikmir sikmir commented May 13, 2026

Copy link
Copy Markdown
Contributor

Что происходит; кому и зачем нужно:
см https://github.com/wirenboard/wbgo-private/pull/90

==================
WARNING: DATA RACE
Write at 0x00c000178714 by goroutine 87:
  github.com/wirenboard/wb-rules/wbrules.(*RuleEngine).handleStop()
      github.com/wirenboard/wb-rules/wbrules/engine.go:1299 +0x47b
  github.com/wirenboard/wb-rules/wbrules.(*RuleEngine).mainLoop()
      github.com/wirenboard/wb-rules/wbrules/engine.go:888 +0x887
  github.com/wirenboard/wb-rules/wbrules.(*RuleEngine).Start.func2()
      github.com/wirenboard/wb-rules/wbrules/engine.go:1353 +0x33

Previous read at 0x00c000178714 by goroutine 6:
  github.com/wirenboard/wb-rules/wbrules.(*RuleEngine).MaybeCallSync()
      github.com/wirenboard/wb-rules/wbrules/engine.go:1002 +0x89
  github.com/wirenboard/wb-rules/wbrules.(*RuleEngine).MaybeCallSync-fm()
      <autogenerated>:1 +0x3d
  github.com/wirenboard/wb-rules/wbrules.(*ESContext).removeCallbackSync()
      github.com/wirenboard/wb-rules/wbrules/escontext.go:387 +0x182
  github.com/wirenboard/wb-rules/wbrules.callbackFinalizer()
      github.com/wirenboard/wb-rules/wbrules/escontext.go:364 +0xae

Goroutine 87 (running) created at:
  github.com/wirenboard/wb-rules/wbrules.(*RuleEngine).Start()
      github.com/wirenboard/wb-rules/wbrules/engine.go:1353 +0x4b3
  github.com/wirenboard/wb-rules/wbrules.(*RuleSuiteBase).SetupTest()
      github.com/wirenboard/wb-rules/wbrules/rule_test.go:277 +0x1264
  github.com/wirenboard/wb-rules/wbrules.(*RuleSuiteBase).SetupSkippingDefs()
      github.com/wirenboard/wb-rules/wbrules/rule_test.go:329 +0x6d
  github.com/wirenboard/wb-rules/wbrules.(*AlarmSuite).SetupTest()
      github.com/wirenboard/wb-rules/wbrules/rule_alarm_test.go:20 +0x50
  github.com/stretchr/testify/suite.Run.func1()
      github.com/stretchr/testify@v1.11.0/suite/suite.go:188 +0x252
  testing.tRunner()
      testing/testing.go:1595 +0x238
  testing.(*T).Run.func1()
      testing/testing.go:1648 +0x44

Goroutine 6 (running) created at:
  runtime.createfing()
      runtime/mfinal.go:163 +0x3c
  os.NewFile()
      os/file_unix.go:119 +0x87
  os.init()
      os/file.go:65 +0x332
==================

Что поменялось для пользователей:
ничего


Как проверял/а:
локально

@sikmir sikmir requested review from KraPete and Copilot May 13, 2026 19:17
@codacy-production

codacy-production Bot commented May 13, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric Results
Complexity 0
Duplication 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

Copilot AI 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.

Pull request overview

This PR addresses a reported data race during engine shutdown by synchronizing access to shutdown-related state in RuleEngine, and updates CI/build tooling to run tests with the Go race detector.

Changes:

  • Protect syncQueueActive access with statusMtx in MaybeCallSync() to eliminate the reported race.
  • Replace the previous shutdown “wait” mechanism with a mainLoopDone channel to coordinate Stop() with main loop completion.
  • Enable -race in make test, and adjust Jenkins build parameters to use a specific wbgo.so branch.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
wbrules/engine.go Adds mainLoopDone and locking around syncQueueActive; changes Stop() waiting logic.
Makefile Runs Go tests with -race enabled.
Jenkinsfile Pins defaultWbGoSoBranch for CI builds.
debian/changelog Bumps package version and notes the race-condition fix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread wbrules/engine.go Outdated
Comment thread Jenkinsfile

Copilot AI 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.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment thread wbrules/engine.go
Comment thread Makefile

Copilot AI 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.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Comment thread wbrules/engine.go
Comment thread wbrules/engine.go
Comment thread wbrules/engine.go
@sikmir sikmir marked this pull request as draft May 13, 2026 20:40

Copilot AI 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.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

wbrules/engine.go:1392

  • Stop() stops syncLoop before waiting for mainLoop/handleStop to finish. If mainLoop is still processing buffered events, it can call CallSync(); with syncLoop already stopped and syncStopCh not yet closed (it’s closed in handleStop), CallSync may block sending to syncQueue and prevent handleStop from running, making Stop() wait forever. Consider waiting for handleStop (e.g., via syncStopCh / a wait group) before stopping syncLoop, or closing syncStopCh before terminating syncLoop to ensure CallSync won’t block during shutdown.
	engine.eventBuffer.Close()

	// stop sync loop
	q := make(chan struct{})
	engine.syncQuitCh <- q
	<-q

	for atomic.LoadUint32(&engine.syncQueueActive) == ATOMIC_TRUE {
		runtime.Gosched()
	}

wbrules/engine.go:1392

  • The post-syncLoop wait uses a busy spin with runtime.Gosched(). Since syncStopCh is already closed in handleStop(), Stop() can block on that signal instead of spinning, which avoids unnecessary CPU usage and removes the need for the runtime import.

	for atomic.LoadUint32(&engine.syncQueueActive) == ATOMIC_TRUE {
		runtime.Gosched()
	}

Comment thread wbrules/engine.go Outdated
@sikmir sikmir marked this pull request as ready for review May 18, 2026 11:11

Copilot AI 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.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

Comment thread wbrules/engine.go
Comment thread wbrules/rule_test.go Outdated
Comment thread wbrules/rule_test.go
Comment thread wbrules/vcells_storage_test.go
Comment thread wbrules/persistent_storage_test.go
Comment thread wbrules/rule_reload_test.go
Comment thread wbrules/rule_reload_test.go
sikmir and others added 2 commits July 1, 2026 10:56
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI 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.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Comment thread wbrules/engine.go
Comment thread wbrules/engine.go
sikmir and others added 2 commits July 1, 2026 16:42
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI 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.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comment thread wbrules/engine.go
Comment on lines +991 to +995
engine.statusMtx.Lock()
defer engine.statusMtx.Unlock()
if atomic.LoadUint32(&engine.syncQueueActive) != ATOMIC_TRUE {
return false
}
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.

3 participants