fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

CB-601: deterministic head start for AmqpReplyInboxRecoveryRaceTest #97

Closed
agent wants to merge 0 commits from worker/cb601-b42837-18 into main
pull from: worker/cb601-b42837-18
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-08-16 18:03:17 +02:00
Member
Copy Link
Copy Source

Fixes the flaky race test diagnosed in CB-601: the 5ms Thread.sleep head start before starting the "fresh" publish thread did not guarantee the recovery sweep was holding publishChannelLock, so under machine load the sweep sometimes hadn't started yet, fresh won the lock first, and the test failed on correct code.

Replaces the sleep with a CountDownLatch counted down by the first stale publish thread to observe its own IllegalStateException. That exception can only be thrown by failPendingPublishesOnRecovery() completing a Pending exceptionally, so seeing it is direct proof the sweep is already inside its (lock-holding, once fixed) loop -- not a timing guess. This also makes Case 2 (fresh winning the lock before the sweep starts) impossible by construction, since freshThread is only started after the sweep has already failed at least one entry.

Also reduced STALE_PUBLISHES from 100,000 to 2,000: the deterministic signal no longer needs a huge backlog to create a timing window, just a large-enough tail after the first observed failure to guarantee the sweep hasn't finished. Fixed a stale comment referencing an old 20,000 value.

No production code changes. Verified the test still fails (5/5) when publishChannelLock's synchronized guard is removed from failPendingPublishesOnRecovery, then restored the guard (confirmed clean diff on AmqpReplyInbox.java). Ran the race test 20 times consecutively while up to 3 parallel mvn clean install builds ran in separate worktree copies -- all 20 passed. Full mvn -f bridged/pom.xml clean install is green: 822 tests, 0 failures, 0 errors.

Fixes the flaky race test diagnosed in CB-601: the 5ms Thread.sleep head start before starting the "fresh" publish thread did not guarantee the recovery sweep was holding publishChannelLock, so under machine load the sweep sometimes hadn't started yet, fresh won the lock first, and the test failed on correct code. Replaces the sleep with a CountDownLatch counted down by the first stale publish thread to observe its own IllegalStateException. That exception can only be thrown by failPendingPublishesOnRecovery() completing a Pending exceptionally, so seeing it is direct proof the sweep is already inside its (lock-holding, once fixed) loop -- not a timing guess. This also makes Case 2 (fresh winning the lock before the sweep starts) impossible by construction, since freshThread is only started after the sweep has already failed at least one entry. Also reduced STALE_PUBLISHES from 100,000 to 2,000: the deterministic signal no longer needs a huge backlog to create a timing window, just a large-enough tail after the first observed failure to guarantee the sweep hasn't finished. Fixed a stale comment referencing an old 20,000 value. No production code changes. Verified the test still fails (5/5) when publishChannelLock's synchronized guard is removed from failPendingPublishesOnRecovery, then restored the guard (confirmed clean diff on AmqpReplyInbox.java). Ran the race test 20 times consecutively while up to 3 parallel `mvn clean install` builds ran in separate worktree copies -- all 20 passed. Full `mvn -f bridged/pom.xml clean install` is green: 822 tests, 0 failures, 0 errors.
agent added 1 commit 2026-08-16 18:03:17 +02:00
CB-601: make the recovery-race test's head start deterministic, not a sleep
CI / contract (pull_request) Successful in 1m8s
Details
CI / build (pull_request) Successful in 1m9s
Details
16de9df000
ltms commented 2026-08-16 18:08:51 +02:00
Owner
Copy Link
Copy Source

Merged locally as a36b7cc and pushed to main. Gitea cannot mark a locally merged PR as merged, so I am closing it by hand — this is merged, not rejected.

Verification is written up on #95. Good work: the latch signal is the right shape, dropping STALE_PUBLISHES to 2,000 was the correct call, and proving the guard removal still fails the test is what made this reviewable.

Merged locally as a36b7cc and pushed to `main`. Gitea cannot mark a locally merged PR as merged, so I am closing it by hand — this is **merged, not rejected**. Verification is written up on #95. Good work: the latch signal is the right shape, dropping `STALE_PUBLISHES` to 2,000 was the correct call, and proving the guard removal still fails the test is what made this reviewable.
ltms closed this pull request 2026-08-16 18:08:55 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m8s
Details
CI / build (pull_request) Successful in 1m9s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#97
Delete Branch "worker/cb601-b42837-18"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?