mailbox: fix AwaitRPC lost-wakeup flake - #121
Conversation
Summary of ChangesHello @bhandras, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical concurrency bug within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
db828de to
fb6c25b
Compare
There was a problem hiding this comment.
Code Review
The changes effectively address the lost-wakeup flake in AwaitRPC by making the response check and waiter registration atomic. The introduction of popPendingOrAddWaiter correctly synchronizes access to pending responses and waiter channels, resolving the race condition. The code is clear and the fix is well-explained in the PR description.
TestClient_ConcurrentInFlightDoesNotDrop was flaking with "context deadline exceeded" due to a lost-wakeup race in AwaitRPC. AwaitRPC checked the pending-response map and registered a waiter in two separate steps under different lock acquisitions. If handleEnvelope cached a response between the pending check and waiter registration, it notified only the existing waiters — the new waiter was never signaled and blocked until its context timed out. Fix this by introducing popPendingOrAddWaiter, which atomically returns a pending response or registers a waiter, closing the missed-notification window.
fb6c25b to
32ac83b
Compare
|
Folded into the latest series. |
Summary
This fixes a flaky timeout in
TestClient_ConcurrentInFlightDoesNotDropbyeliminating a lost-wakeup race in
AwaitRPC.Root Cause
AwaitRPCpreviously did these steps separately:pendingfor the response.If a response arrived between steps 1 and 2,
handleEnvelopewould cachethe response and notify the current waiter set — which did not yet
include the new waiter. The newly registered waiter would then block until
context timeout.
Fix
Make the response check and waiter registration atomic under one lock via
popPendingOrAddWaiter:This removes the race window where notifications can be missed.
CI Failure
Observed CI failure (PR #79, unit-cover):
Failure signature:
TestClient_ConcurrentInFlightDoesNotDropcontext deadline exceededValidation
go test ./mailbox/client -run TestClient_ConcurrentInFlightDoesNotDrop -count=20000 -timeout=60mgo test -covermode=atomic -coverpkg=github.com/lightninglabs/darepo-client/... -tags='dev nolog' ./mailbox/client -run TestClient_ConcurrentInFlightDoesNotDrop -count=1000 -timeout=20m