coordination: fix flaky TestCoordinationSemaphore without changing public API#2113
Open
asmyasnikov with Copilot wants to merge 9 commits into
Open
coordination: fix flaky TestCoordinationSemaphore without changing public API#2113asmyasnikov with Copilot wants to merge 9 commits into
asmyasnikov with Copilot wants to merge 9 commits into
Conversation
summaryInferred base version: v3.135.10 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2113 +/- ##
==========================================
- Coverage 75.16% 72.54% -2.63%
==========================================
Files 427 427
Lines 36974 36974
==========================================
- Hits 27792 26822 -970
- Misses 7993 8952 +959
- Partials 1189 1200 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…CoordinationSemaphore - Add `WaitConnected(ctx context.Context) error` to the `coordination.Session` interface - Implement `WaitConnected` in `internal/coordination/session.go` using a `connectedChan` channel guarded by the existing mutex: nil = connected, non-nil = waiting for reconnect - Add `markConnected()` helper that closes and clears the channel under the mutex - Update `Reconnect()` to create `connectedChan` if nil, enabling `WaitConnected` to block - In `mainLoop`, call `markConnected()` when SessionStarted is received with a valid session ID - Update `tests/integration/coordination_test.go` and `coordination/example_test.go` to call `WaitConnected` after `Reconnect()`, ensuring the test waits for the reconnect to complete before making further requests - Add CHANGELOG entry Agent-Logs-Url: https://github.com/ydb-platform/ydb-go-sdk/sessions/c6517c58-5a1b-4f5a-8029-878fd908e247 Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
Agent-Logs-Url: https://github.com/ydb-platform/ydb-go-sdk/sessions/c6517c58-5a1b-4f5a-8029-878fd908e247 Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix flaky TestCoordinationSemaphore
coordination: add Session.WaitConnected to fix flaky TestCoordinationSemaphore
May 3, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a synchronization point to coordination sessions so callers (and tests) can deterministically wait for a Reconnect() to finish before issuing subsequent requests, addressing CI flakiness caused by session grace/handshake timing.
Changes:
- Added
Session.WaitConnected(ctx)to block until the session handshake completes after aReconnect(). - Implemented reconnect-completion signaling in the internal session loop via a guarded
connectedChanandmarkConnected(). - Updated the semaphore integration test and the public example to call
WaitConnected()afterReconnect().
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/coordination/session.go |
Implements reconnect completion signaling and the new WaitConnected() method. |
coordination/client.go |
Extends the public coordination.Session interface with WaitConnected(ctx). |
tests/integration/coordination_test.go |
Fixes flakiness by waiting for reconnect completion before describing the semaphore. |
coordination/example_test.go |
Updates example usage to wait for reconnect completion. |
CHANGELOG.md |
Adds a changelog note for the new public API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![robot-vibe-db[bot]](https://avatars.githubusercontent.com/in/3319454?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Verdict: ✅ No critical issues found
Critical issues
No critical issues found.
Other findings
- Minor | High: Misleading doc comment on
markConnected()says "Must be called with s.mutex held" but the method acquires the lock itself; calling with mutex held would deadlock —internal/coordination/session.go:550
This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
AI Review Summary
Verdict: ✅ No critical issues found
Critical issues
No critical issues found.
Other findings
- Minor | Low: Adding
WaitConnectedto the publicSessioninterface is technically a breaking change for any external code that implements the interface. In practice, this is a large, domain-specific interface (~12 methods) that is clearly meant to be consumed by callers rather than implemented externally, so the real-world impact is negligible. —coordination/client.go:134 - Minor | Low: The
selectinWaitConnecteddoes not prioritize<-chover<-s.ctx.Done(). If both fire simultaneously (reconnect completes just as the session is closing), Go's random select could return the session error instead ofnil. In practice this is harmless — the session is dying either way and subsequent operations would fail — but documenting the precedence might save a future reader some reasoning. —internal/coordination/session.go:590
Implementation notes (not issues):
- The channel-snapshot pattern in
WaitConnected(readconnectedChanunder lock, thenselecton the snapshot) is correct and avoids holding the mutex during the blocking wait. markConnected()is properly idempotent: safe to call whenconnectedChanis alreadynil.- The ordering in
mainLoop— callingmarkConnected()beforeclose(sessionStartedChan)on initial connect — correctly prevents a race whereReconnect()could be called immediately afterSession()returns. - Reusing an existing
connectedChanon consecutiveReconnect()calls (rather than creating a new one) correctly batches multiple waiters onto the same notification event.
This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.
Member
|
@copilot разве нет другого способа починить flaky test TestCoordinationSemaphore, не меняя публичное API? |
Instead of adding WaitConnected to the Session interface, fix the root cause directly in the test configuration: - Increase SessionGracePeriodMillis from 1000ms to 10000ms so the server keeps the session alive long enough for the client to reconnect - Pass options.WithSessionReconnectDelay(0) to Session() so the client reconnects immediately (no 500ms default delay), making reconnect complete well within the grace period - Remove WaitConnected from the Session interface and its internal implementation - Remove the WaitConnected CHANGELOG entry (no user-facing API change) Agent-Logs-Url: https://github.com/ydb-platform/ydb-go-sdk/sessions/af4612bd-7e60-40bc-96d6-1e76264ed998 Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
Contributor
Author
Yes — instead of
All the |
Copilot
AI
changed the title
coordination: add Session.WaitConnected to fix flaky TestCoordinationSemaphore
coordination: fix flaky TestCoordinationSemaphore without changing public API
May 3, 2026
asmyasnikov
approved these changes
May 3, 2026
There was a problem hiding this comment.
AI Review Summary
Verdict: ✅ No critical issues found
Critical issues
No critical issues found.
Other findings
None.
Analysis:
This PR makes a clean, minimal fix for a flaky integration test (TestCoordinationSemaphore) by adjusting two test-only configuration values:
-
SessionGracePeriodMillis: 1000 → 10000— gives the server 10× more time to keep the session alive during reconnection, providing ample headroom for CI environments where gRPC setup may be slow. -
options.WithSessionReconnectDelay(0)— eliminates the default 500ms reconnect delay so the client re-establishes the gRPC stream immediately afterReconnect(). Combined with the increased grace period, this ensures the session is restored well within the server's tolerance.
WithSessionReconnectDelay(0) is safe here: in Go, time.After(0) fires immediately but still yields to the scheduler (no CPU spin), the server is healthy so reconnection succeeds on the first attempt, and the test context provides a timeout safety net. Since this is test-only code with a single explicit Reconnect() call, the zero-delay poses no risk of tight retry loops.
No production code, public API, or dependency changes. No CHANGELOG entry required.
This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.
Member
|
@copilot тест все еще flaky лог с последнего прогона |
…t keep-alive check The receiveLoop was calling `sessionStarted <- message.GetSessionStarted()` (blocking channel send) and THEN `s.updateLastGoodResponseTime()`. Once mainLoop received from the channel, it could race into the inner keep-alive loop and read lastGoodResponseTime as zero (Go zero time: 0001-01-01) before receiveLoop updated it. This made keepAliveTime a massive negative value, causing time.After to fire immediately, triggering a spurious reconnect that would fail non-idempotent operations (e.g. CreateSemaphore) with ErrOperationStatusUnknown. Fix: move updateLastGoodResponseTime() to BEFORE the channel send so the value is guaranteed to be set by the time mainLoop enters the keep-alive inner loop. Agent-Logs-Url: https://github.com/ydb-platform/ydb-go-sdk/sessions/8a1fdbdc-9eb3-40ae-962a-fbcfb944dd5e Co-authored-by: asmyasnikov <14202262+asmyasnikov@users.noreply.github.com>
Contributor
Author
Found the root cause by inspecting the CI logs. The failure was Root cause (commit case *Ydb_Coordination.SessionResponse_SessionStarted_:
sessionStarted <- message.GetSessionStarted() // unblocks mainLoop
s.updateLastGoodResponseTime() // called AFTER channel sendOnce Fix: swapped the two lines so |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent bugs caused
TestCoordinationSemaphoreto be flaky. Both are fixed without any public API changes.Root cause 1 — race in
receiveLoop(spurious reconnect on session creation)In
receiveLoop,updateLastGoodResponseTime()was called after the blockingsessionStarted <- message.GetSessionStarted()channel send. OncemainLoopreceived from the channel, it could race into the keep-alive inner loop and readlastGoodResponseTimeas Go's zero time (0001-01-01) beforereceiveLoophad a chance to update it. This madekeepAliveTimea massive negative value →time.Afterfired immediately → keep-alive timeout triggered →cancelStream()called →OnDetachfailed non-idempotent operations in the queue (e.g.CreateSemaphore) withErrOperationStatusUnknown.Fix: moved
updateLastGoodResponseTime()to before the channel send inreceiveLoop, guaranteeing the timestamp is set beforemainLoopcan enter the keep-alive loop.Root cause 2 — reconnect timing window after
Reconnect()(original issue)Reconnect()is fire-and-forget. The integration test calledDescribeSemaphore()immediately after, which works in most cases because the operation is idempotent and queued — but under CI load, the 500 mssessionReconnectDelayplus gRPC setup time could exceed the server's 1 sSessionGracePeriodMillis, causing the server to expire the session.Fix: test configuration changes only:
SessionGracePeriodMillis: 1000 → 10000— the server keeps the session alive for 10 seconds after losing contact, giving ample headroom for reconnect under CI load.options.WithSessionReconnectDelay(0)passed toSession()— the client reconnects immediately when the stream drops (no 500 ms default delay).Because
DescribeSemaphoreis already idempotent (WithIdempotence(true)) and automatically replayed after reconnect, no additional synchronization is needed.No public API changes.