Skip to content

Conversation

@grahamplata
Copy link
Member

@grahamplata grahamplata commented Mar 19, 2025

Investigate CI issues for Go resolves: https://github.com/rilldata/rill-private-issues/issues/1389

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

@grahamplata grahamplata marked this pull request as ready for review March 20, 2025 21:35
@grahamplata grahamplata changed the title chore: Update Go actions to use go.mod for versioning and adjust depth Update Go actions to use go.mod for versioning and adjust depth Mar 20, 2025
@grahamplata grahamplata self-assigned this Mar 21, 2025
Comment on lines 199 to 200
r.instanceMu.RLock()
defer r.instanceMu.RUnlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't safe – there are actions in this scope that may run a long time, so it can lead to hanging that doesn't respond to a cancelled ctx. What is the exact race condition you are trying to mitigate here?

Copy link
Member Author

Choose a reason for hiding this comment

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

WARNING: DATA RACE
Write at 0x00c0004b4398 by goroutine 279:
  github.com/rilldata/rill/runtime.(*Runtime).UpdateInstanceConnector()
      /Users/grahamplata/dev/rilldata/rill/runtime/runtime.go:201 +0x5b0
  github.com/rilldata/rill/runtime/reconcilers.(*ConnectorReconciler).Reconcile()
      /Users/grahamplata/dev/rilldata/rill/runtime/reconcilers/connector.go:90 +0x39c
  github.com/rilldata/rill/runtime.(*Controller).invoke.func1()
      /Users/grahamplata/dev/rilldata/rill/runtime/controller.go:1330 +0x8ec

Previous read at 0x00c0004b4398 by goroutine 278:
  github.com/rilldata/rill/runtime.(*Runtime).ConnectorConfig()
      /Users/grahamplata/dev/rilldata/rill/runtime/connections.go:220 +0x388
  github.com/rilldata/rill/runtime.(*Runtime).AcquireHandle()
      /Users/grahamplata/dev/rilldata/rill/runtime/connections.go:40 +0x60
  github.com/rilldata/rill/runtime.(*Runtime).Repo()
      /Users/grahamplata/dev/rilldata/rill/runtime/connections.go:65 +0x98
  github.com/rilldata/rill/runtime/reconcilers.(*ModelReconciler).newModelEnv()
      /Users/grahamplata/dev/rilldata/rill/runtime/reconcilers/model.go:1383 +0x1f4
  github.com/rilldata/rill/runtime/reconcilers.(*ModelReconciler).Reconcile()
      /Users/grahamplata/dev/rilldata/rill/runtime/reconcilers/model.go:127 +0x3e8
  github.com/rilldata/rill/runtime.(*Controller).invoke.func1()
      /Users/grahamplata/dev/rilldata/rill/runtime/controller.go:1330 +0x8ec

Goroutine 279 (running) created at:
  github.com/rilldata/rill/runtime.(*Controller).invoke()
      /Users/grahamplata/dev/rilldata/rill/runtime/controller.go:1294 +0x97c
  github.com/rilldata/rill/runtime.(*Controller).trySchedule()
      /Users/grahamplata/dev/rilldata/rill/runtime/controller.go:1253 +0x458
  github.com/rilldata/rill/runtime.(*Controller).processQueue()
      /Users/grahamplata/dev/rilldata/rill/runtime/controller.go:1065 +0x1e0
  github.com/rilldata/rill/runtime.(*Controller).Run()
      /Users/grahamplata/dev/rilldata/rill/runtime/controller.go:208 +0x61c
  github.com/rilldata/rill/runtime.(*registryCache).restartController.func1()
      /Users/grahamplata/dev/rilldata/rill/runtime/registry.go:446 +0x8a0

Goroutine 278 (running) created at:
  github.com/rilldata/rill/runtime.(*Controller).invoke()
      /Users/grahamplata/dev/rilldata/rill/runtime/controller.go:1294 +0x97c
  github.com/rilldata/rill/runtime.(*Controller).trySchedule()
      /Users/grahamplata/dev/rilldata/rill/runtime/controller.go:1253 +0x458
  github.com/rilldata/rill/runtime.(*Controller).processQueue()
      /Users/grahamplata/dev/rilldata/rill/runtime/controller.go:1065 +0x1e0
  github.com/rilldata/rill/runtime.(*Controller).Run()
      /Users/grahamplata/dev/rilldata/rill/runtime/controller.go:208 +0x61c
  github.com/rilldata/rill/runtime.(*registryCache).restartController.func1()
      /Users/grahamplata/dev/rilldata/rill/runtime/registry.go:446 +0x8a0

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I think this should fix it: #7014

grahamplata and others added 2 commits March 31, 2025 09:35
Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>
@grahamplata grahamplata merged commit cd474dc into main Mar 31, 2025
8 of 9 checks passed
@grahamplata grahamplata deleted the gplata/go-ci-debugging branch March 31, 2025 14:00
grahamplata added a commit that referenced this pull request Apr 14, 2025
* chore: update Go actions to use go.mod for versioning and adjust checkout fetch depth

* fix: update go-version-file path to include leading dot in workflow files

* fix: update Go setup to specify version directly in workflow files

* nit: enable caching in Go setup comparison

* fix: reorder

* fix: simplify workflow files and reorder

* fix: handle error in TestRBAC for CreateProjectWhitelistedDomain

* fix: remove unnecessary error check in newTestServer function

* fix: improve graceful shutdown handling for gRPC and HTTP servers

* fix: use errors.Is

* fix: adjust parallel execution in metrics view comparison test

* fix: simplify graceful shutdown handling

* nit

* fix: graceful shutdown handling for HTTP server with timeout

* fix: improve graceful shutdown handling in HTTP server

* nit

* fix: add mutex for thread-safe access to instance configuration

* revert: changes to runtime

* Update runtime/runtime.go

Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>

* sync

---------

Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>
grahamplata added a commit that referenced this pull request Apr 14, 2025
* chore: update Go actions to use go.mod for versioning and adjust checkout fetch depth

* fix: update go-version-file path to include leading dot in workflow files

* fix: update Go setup to specify version directly in workflow files

* nit: enable caching in Go setup comparison

* fix: reorder

* fix: simplify workflow files and reorder

* fix: handle error in TestRBAC for CreateProjectWhitelistedDomain

* fix: remove unnecessary error check in newTestServer function

* fix: improve graceful shutdown handling for gRPC and HTTP servers

* fix: use errors.Is

* fix: adjust parallel execution in metrics view comparison test

* fix: simplify graceful shutdown handling

* nit

* fix: graceful shutdown handling for HTTP server with timeout

* fix: improve graceful shutdown handling in HTTP server

* nit

* fix: add mutex for thread-safe access to instance configuration

* revert: changes to runtime

* Update runtime/runtime.go

Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com>

* sync

---------

Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.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.

3 participants