test(objectio): synchronize dedupLoad waiter admission - #27782
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
956a890 to
5dbe117
Compare
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Blocking test-lifecycle issue on exact head 955853109432c603eb75f51e94e8059cf747591e.
The admission observation is sound, but all three tests now wait on <-waiterCtx.admitted without a watchdog or an early-exit branch. admitted is closed only from Done(). If the waiter unexpectedly becomes an owner, returns before entering the waiter select, or a future dedupLoad regression bypasses that Done() evaluation, its result is buffered in waiterDone while the test goroutine blocks forever. The old test would report unexpected waiter load; the replacement turns that failure into a package-wide hang until the outer go test timeout. This contradicts the PR claim that the barrier will fail directly when the seam changes.
Please make each admission wait bounded and failure-diagnostic: select on admitted, waiter early completion, and a test timeout; on timeout/early exit also release/drain the owner so no goroutine is leaked. Keep the timeout only as a watchdog, not as the phase trigger. A small reusable helper can avoid duplicating this across the three tests.
Evidence: -race -count=100 -timeout=120s for all three changed tests passes on the current implementation, confirming the normal synchronization path. Q1-Q3: no production ownership or wait graph changed; the only remaining issue is the test failure path itself being unbounded.
9558531 to
32cf49b
Compare
|
Fixed in commit |
matrix-meow
added
size/M
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-reviewed exact head 32cf49b, including the complete delta from previously reviewed head 9558531.
The previous hang blocker is fixed. The tests still use an exact waiter-admission phase barrier through context Done, but admission is now bounded to one second. A waiter that exits before admission or an admission timeout cancels the waiter, releases the blocked owner, and drains both goroutines with bounded waits before failing. Success paths observe owner/waiter completion before reading shared results, and cancellation/release operations are single-close or idempotent in their respective cases. No production code changes are introduced.
Validation on this exact head:
- focused TestDedupLoad tests, count=100: PASS
- focused TestDedupLoad tests under race, count=50: PASS
- complete pkg/objectio package: PASS
- diff check: PASS
No remaining correctness, race, leak, hang, performance, or unhappy-path blocker found.
32cf49b to
f9c8bc6
Compare
Merge Queue Status
This pull request spent 4 minutes 33 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonThe pull request #27782 has been manually updated Failing checks:
Requeued — the merge queue status continues in this comment ↓. |
Merge Queue Status
This pull request spent 19 seconds in the queue, including 1 second running CI. Required conditions to merge
|
3 participants
What type of PR is this?
Which issue(s) this PR fixes:
Related to #27760
What this PR does / why we need it:
Root cause
The three
dedupLoadwaiter tests used a 10 ms scheduler sleep as the phase trigger. The existingstartedchannel only proves that the owner entered its loader; it does not prove that the second goroutine acquiredmetaLoadMuand found the existingmetaLoadCalls[key]entry. Under coverage or other load-sensitive scheduling, the owner could be released first, complete, and delete the call. The delayed second goroutine would then become a new owner and run its sentinel loader, producing the reportedunexpected waiter loaderror. This is a test synchronization defect; the productiondedupLoadownership protocol is unchanged.The first barrier revision also left the admission wait unbounded: if the waiter returned early or the internal seam changed, the test could hang instead of reporting the failed phase transition.
Changes
Donemethod observes the existing waiter branch. IndedupLoad, that method is evaluated only after an existing owner call has been found.Doneobservation.TestDedupLoadWaiterGetsSuccessfulOwnerValue,TestDedupLoadCleansUpAfterPanic, andTestDedupLoadCleansUpAfterLoadCancelwith the admission helper.Issue-to-test proof
TestDedupLoadWaiterGetsSuccessfulOwnerValuewaits until the waiter has observed the in-flight owner, then releases it and verifies the waiter gets[]byte("ok")without invoking its sentinel loader.TestDedupLoadCleansUpAfterPanicproves an admitted waiter receives the incomplete-load error, the call entry is removed, and a later owner can load successfully.TestDedupLoadCleansUpAfterLoadCancelproves an admitted waiter receivescontext.Canceledand its loader is never called._test.gosynchronization protocol and does not change SQL, frontend, wire, or production behavior.Tests run
make -j8 NATIVE_BUILD_JOBS=8 cgo.agents/skills/mo-dev/scripts/mo-cgo-test -list '^TestDedupLoad(CleansUpAfterPanic|WaiterGetsSuccessfulOwnerValue|CleansUpAfterLoadCancel)$' ./pkg/objectio-v -count=1.-race -count=100.pkg/objectiounit tests with-count=1.go vet -mod=readonly ./pkg/objectiowith the repository CGo include and library paths.git diff --checkResidual risks
The change is test-only and adds no production synchronization, allocation, I/O, or runtime overhead. The barrier intentionally observes the existing waiter-admission call through a local context wrapper. If that internal test seam changes, the helper now reports either early waiter completion or a bounded admission timeout and attempts bounded cleanup of both test goroutines instead of silently relying on scheduler timing.