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

#310: prevent idle reap from stopping delivered workers #311

Closed
agent wants to merge 0 commits from worker/fix-310-7a3974-9 into main
pull from: worker/fix-310-7a3974-9
merge into: fleet:main
Conversation 0 Commits - Files Changed -
agent commented 2026-09-04 08:18:54 +02:00
Member
Copy Link
Copy Source

Fixes #310.

Design

reapIdle now uses ConcurrentHashMap.remove(paneId, snapshot) as its linearization point. It tears down the worker only when the map still holds the immutable MemberSession it checked. onDelivered replaces that record with a BUSY record, so a delivery before the conditional remove makes the reap skip. No shared lock is needed. The ordinary production path has no callback and takes no lock.

The change keeps the existing per-session try/catch. A skipped compare-and-release does not count as a reap. The public release path stays unconditional.

Test and mutation proof

Added a package-private test seam at the exact point after the eligibility check and before compare-and-release. The test performs a real onDelivered transition there. It does not change the registry outside normal lifecycle methods.

With the conditional remove changed to unconditional remove(paneId), this command failed as expected:

cd fleetd && mvn -Dtest=SessionManagerTest#reapIdleDoesNotReleaseSessionDeliveredAfterItsEligibilityCheck test

Real failure: expected: <0> but was: <1> in reapIdleDoesNotReleaseSessionDeliveredAfterItsEligibilityCheck. I restored the conditional remove before the full build.

Checks run

  • cd fleetd && mvn -Dtest=SessionManagerTest#reapIdleDoesNotReleaseSessionDeliveredAfterItsEligibilityCheck test
    • Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
    • BUILD SUCCESS
  • cd fleetd && mvn clean install
    • Tests run: 1317, Failures: 0, Errors: 0, Skipped: 0
    • BUILD SUCCESS

Issue correction and severity

The race is reachable by the normal onDelivered state replacement. I did not observe it in a live run. The issue is right that the session can be released after delivery, but MemberRegistry is not the map used for these operations. SessionManager owns a ConcurrentHashMap; its existing replace method calls ConcurrentHashMap.replace, and this fix uses that map’s compare-based remove(key, value).

The release listener still tells a blocked send that the worker was released. This is not a silent loss. The problem is a just-dispatched worker being stopped, followed by a release reason that does not explain the reaper/delivery race.

Fixes #310. ## Design `reapIdle` now uses `ConcurrentHashMap.remove(paneId, snapshot)` as its linearization point. It tears down the worker only when the map still holds the immutable `MemberSession` it checked. `onDelivered` replaces that record with a BUSY record, so a delivery before the conditional remove makes the reap skip. No shared lock is needed. The ordinary production path has no callback and takes no lock. The change keeps the existing per-session `try/catch`. A skipped compare-and-release does not count as a reap. The public `release` path stays unconditional. ## Test and mutation proof Added a package-private test seam at the exact point after the eligibility check and before compare-and-release. The test performs a real `onDelivered` transition there. It does not change the registry outside normal lifecycle methods. With the conditional remove changed to unconditional `remove(paneId)`, this command failed as expected: `cd fleetd && mvn -Dtest=SessionManagerTest#reapIdleDoesNotReleaseSessionDeliveredAfterItsEligibilityCheck test` Real failure: `expected: <0> but was: <1>` in `reapIdleDoesNotReleaseSessionDeliveredAfterItsEligibilityCheck`. I restored the conditional remove before the full build. ## Checks run - `cd fleetd && mvn -Dtest=SessionManagerTest#reapIdleDoesNotReleaseSessionDeliveredAfterItsEligibilityCheck test` - `Tests run: 1, Failures: 0, Errors: 0, Skipped: 0` - `BUILD SUCCESS` - `cd fleetd && mvn clean install` - `Tests run: 1317, Failures: 0, Errors: 0, Skipped: 0` - `BUILD SUCCESS` ## Issue correction and severity The race is reachable by the normal `onDelivered` state replacement. I did not observe it in a live run. The issue is right that the session can be released after delivery, but `MemberRegistry` is not the map used for these operations. `SessionManager` owns a `ConcurrentHashMap`; its existing `replace` method calls `ConcurrentHashMap.replace`, and this fix uses that map’s compare-based `remove(key, value)`. The release listener still tells a blocked send that the worker was released. This is not a silent loss. The problem is a just-dispatched worker being stopped, followed by a release reason that does not explain the reaper/delivery race.
agent added 1 commit 2026-09-04 08:18:54 +02:00
#310: prevent idle reap from stopping delivered workers
CI / contract (pull_request) Successful in 1m19s
Details
CI / build (pull_request) Successful in 2m1s
Details
a49671ceb9
ltms closed this pull request 2026-09-04 08:23:30 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m19s
Details
CI / build (pull_request) Successful in 2m1s
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
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#311
Delete Branch "worker/fix-310-7a3974-9"

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?