Skip to content

a#491

Open
BugsGroundhog wants to merge 81 commits into
topfreegames:mainfrom
BugsGroundhog:master
Open

a#491
BugsGroundhog wants to merge 81 commits into
topfreegames:mainfrom
BugsGroundhog:master

Conversation

@BugsGroundhog

@BugsGroundhog BugsGroundhog commented Apr 18, 2026

Copy link
Copy Markdown

Note

Medium Risk
Medium risk due to broad dependency upgrades (Go 1.15, etcd/nats/protobuf/validator) and behavior changes in cluster service discovery/watchers and pipeline handler signatures that can affect runtime concurrency and request processing.

Overview
Switches CI from Travis to GitHub Actions (tests.yaml), updates README build badge, and bumps the project to Go 1.15 with large dependency upgrades (notably go.etcd.io/etcd, NATS v2, protobuf, validator v10).

Improves cluster/runtime behavior: etcd service discovery is refactored to explicitly (re)initialize the etcd client, better handle dead lease/watch channels, reduce initial sync round-trips, and avoid watcher/sync races; gRPC server now binds to a configurable address; RPC tracing span names are clarified.

Updates request/pipeline and messaging internals: before-pipeline/struct validator signatures now return (context, out, err) to allow context propagation, request IDs switch to nuid, message route dictionaries become thread-safe, NATS RPC server drains on shutdown, and several tests/e2e/demo code are adjusted accordingly (including regenerated demo protos and new protos-compile-demo target).

Reviewed by Cursor Bugbot for commit 07f2cd1. Bugbot is set up for automated code reviews on this repo. Configure here.

codingcn and others added 30 commits December 26, 2020 08:38
* Fix local tests in OSX
* Update dependencies
Breaks Metric codes
This reverts commit a215e15.
Fix metric tags, etcd reconnect, improve nats shutdown handling and logging
zengjiwen and others added 29 commits May 18, 2021 22:05
When running in high concurrency scenarios the routes/codes maps may
be updated by concurrent goroutines at the same time, resulting in a
race condition while calling `SetDictionary`.

This patch adds a mutex to control access to these maps.

Signed-off-by: Rodrigo Chacon <rochacon@kumbuca.com>
Since Go maps work as pointers to the internal data structures, when
calling GetDictionary we're actually exposing the internal routes map
for its callers.

This commit copies the routes map into a new map instance to prevent
its exposure.

Signed-off-by: Rodrigo Chacon <rochacon@kumbuca.com>
We're also improving some errors to be more descritive on the client.
We're also adding the missing descriptor handler which is used by the
pitaya-cli.
[bugfix] Protoclient was crashing if any handler had no outputs
[bug fix]return err when parse failed
* prometheus reporter startup failure is not forced to panic, but output log

* err of remote call  should be added with routing information

* when serverID is empty, execute remoteCall directly. Avoid executing sd.GetServer() once
…initial-size-get-servers-return-map

Change initial size of get servers return map
* Updating HdrHistogram dependency

* go mod tidy

Co-authored-by: Henrique Oelze <henrique.oelze@wildlifestudios.com>
…recation-warning-at-readme-file

Adding deprecation warning at README.md file

@cursor cursor Bot 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.

Cursor Bugbot has reviewed your changes and found 4 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 07f2cd1. Configure here.

sd.syncServersRunning <- true
defer func() {
sd.syncServersRunning <- false
}()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unbuffered channel causes potential deadlock during Init

High Severity

syncServersRunning is an unbuffered channel (line 93). In Init(), go sd.watchEtcdChanges() is launched, then sd.bootstrap() is immediately called, which calls SyncServers(true). SyncServers blocks on sd.syncServersRunning <- true until the watcher goroutine's inner select is ready to receive. Since watchEtcdChanges must first call sd.cli.Watch() and then launch its own inner goroutine before reaching select, there's a race where the main goroutine blocks before any receiver exists — a potential deadlock.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 07f2cd1. Configure here.

Comment thread cluster/etcd_service_discovery.go Outdated
return err
}

sd.SyncServers()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Server added locally despite etcd Put failure

Medium Severity

addServerIntoEtcd unconditionally calls sd.addServer(server) on line 221 after the cli.Put() call, regardless of whether the Put succeeded or failed. If the etcd write fails, the server is added to local maps but not persisted in etcd, leaving the service discovery in an inconsistent state. The caller (bootstrapServer) checks the returned error, but by then addServer has already modified local state.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 07f2cd1. Configure here.

chn = sd.cli.Watch(context.Background(), "servers/", clientv3.WithPrefix())
failedWatchAttempts = 0
}
continue

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Watcher reconnect counter reset delays recovery unnecessarily

Medium Severity

When InitETCDClient() fails at line 669, failedWatchAttempts is reset to 0 at line 670. Since the watch channel chn is still closed, every subsequent select iteration immediately hits the !ok branch, but now must accumulate 10 more failures before retrying InitETCDClient. This unnecessarily delays reconnection attempts by ~10 seconds on each failed reconnect, rather than retrying immediately on the next iteration.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 07f2cd1. Configure here.

func (m *MockServiceDiscovery) SyncServers() error {
ret := m.ctrl.Call(m, "SyncServers")
func (m *MockServiceDiscovery) SyncServers(firstSync bool) error {
ret := m.ctrl.Call(m, "SyncServers", firstSync)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Mock recorder missing argument for updated SyncServers signature

Medium Severity

The MockServiceDiscovery.SyncServers method was updated to accept a firstSync bool parameter, but the corresponding MockServiceDiscoveryMockRecorder.SyncServers() recorder method was not updated to accept a matching argument. This prevents tests from setting expectations with specific firstSync values and may cause gomock argument-count mismatches at runtime.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 07f2cd1. Configure here.

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.