GitLab Runner crashes

changed milestone to %v1.10

It's strange. I'm looking at the code and can't understand why this panics:

func (b *buildsHelper) statesAndStages() map[common.BuildRuntimeState]map[common.BuildStage]int {
	b.lock.Lock()
	defer b.lock.Unlock()

	data := make(map[common.BuildRuntimeState]map[common.BuildStage]int)
	for _, build := range b.builds {
		if data[build.CurrentState] == nil {
			data[build.CurrentState] = make(map[common.BuildStage]int)
		}
		data[build.CurrentState][build.CurrentStage]++
	}
	return data
}

The panic from above stack trace is generated at line data[build.CurrentState][build.CurrentStage]++. I have no idea how (or in what case/environment) this could be a nil map.

make(map[common.BuildRuntimeState]map[common.BuildStage]int) and make(map[common.BuildStage]int) should ensure that data[build.CurrentState][build.CurrentStage] is at least an existing map with 0 as a value. Whole method is locked so we should not fall in the error of concurrent map access.

@ayufan @nick.thomas any ideas?

@tmaczukin hmm. The only obvious thing is that the builds helper is storing pointers to builds in b.builds

So if they're being added, but then modified elsewhere, that could explain the panic.

I've not looked for a site where that's happening, but I can't see another possibility offhand.

@tmaczukin ok, this is it. Check out commands/multi.go, func processRunner():

	mr.buildsHelper.addBuild(build)
	defer mr.buildsHelper.removeBuild(build)

// ...
	return build.Run(mr.config, trace)

build.Run alters CurrentState many times with no locking. Since the buildsHelper has a pointer to the build, this can race and CurrentState will change between the make() call and the postincrement.

Let me know if you need any more help with it.

@nick.thomas Thanks for pointing this. While looking at this now it seems to be the case I was looking for. I'll investigate this more :)

Going forward with this: simulating concurrency in tests is hard but with this I was able to generate failures in some executions: bcab5edc79d1d7f2b63807261123bcd6f716d474

At this moment I was generating from 4 to 10 errors for 20 subsequent executions of the test. So with multiple executions we can say it's quite reliable :)

Tests showed that we have two possible errors here. First is already known:

panic: assignment to entry in nil map
goroutine 28 [running]:
panic(0x9f60c0, 0xc42035c000)
	/usr/local/go/src/runtime/panic.go:500 +0x1a1
gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands.(*buildsHelper).statesAndStages(0xc420316ff0, 0x0)
	/home/tmaczukin/Projekty/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands/builds_helper.go:156 +0x1ee
gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands.concurrentlyCallStatesAndStages(0xc420316ff0)
	/home/tmaczukin/Projekty/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands/builds_helper_test.go:138 +0x2b
created by gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands.TestBuildHelperCollectWhenRemovingBuild
	/home/tmaczukin/Projekty/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands/builds_helper_test.go:179 +0xa0

And second found while running tests:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x45e67b]
goroutine 21 [running]:
panic(0x9f5fa0, 0xc4200160c0)
	/usr/local/go/src/runtime/panic.go:500 +0x1a1
gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands.(*buildsHelper).statesAndStages(0xc42032a840, 0x0)
	/home/tmaczukin/Projekty/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands/builds_helper.go:153 +0x115
gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands.concurrentlyCallStatesAndStages(0xc42032a840)
	/home/tmaczukin/Projekty/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands/builds_helper_test.go:138 +0x2b
created by gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands.TestBuildHelperCollectWhenRemovingBuild
	/home/tmaczukin/Projekty/go/src/gitlab.com/gitlab-org/gitlab-ci-multi-runner/commands/builds_helper_test.go:179 +0xa0

The reason seems to be the same, but in the second case it's raised three lines earlier.

Now, how to resolve this. First idea that is coming to my mind is to add Mutex (or even RWMutex) to common.Build. Then we could lock this for read in statesAndStages() and lock for write in removeBuild(). This should ensure that build is not removed from b.builds slice while statesAndStages() is working on it. What do you think about this @nick.thomas?

Hmmm, seems I found a much easier solution: abc8aa03e999dd90a3029f4a3dbda0e95fe1f88b 😉

mentioned in merge request !529 (merged)

closed via merge request !529 (merged)

GitLab Runner crashes

Designs

Child items 0

Activity

Admin message

Admin message

GitLab Runner crashes

Activity