fix(kick): drop the kicked participant from cached membership - #386
Open
schronck wants to merge 2 commits into
Open
fix(kick): drop the kicked participant from cached membership#386schronck wants to merge 2 commits into
schronck wants to merge 2 commits into
Conversation
`POST /add-party` and `POST /kick` validate against `dec_party_participants`, whose only production writer is the `/decentralized-parties` refresh. A completed kick left the coordinator still holding the removed participant, so an immediate re-add was rejected with "already a member" and the post-add threshold bound was computed one member too high. This is what fails the add_party_missing_dar phase in CI. That phase kicks P3, waits for /kick/status to report completed, then re-adds P3 — green only when an unrelated refresh happened to land in between, which is why the job alternated pass/fail on main.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a kick/add-party workflow bug where the coordinator’s cached decentralized-party membership (dec_party_participant) remained stale after a successful kick, causing immediate re-adds to fail with a 409 “already a member” and inflating post-add threshold bounds.
Changes:
- Adds a new kick step helper (
prune_cached_membership) to remove the kicked participant from the coordinator’s cached membership. - Invokes the cache-prune immediately after
submit_kickin the coordinator kick workflow. - Adds SQLx integration tests covering both “kicked member removed” and “absent member is a no-op” behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/decman/src/workflow/kick/steps/mod.rs | Exposes the new cache-pruning step module and re-exports its entrypoint. |
| crates/decman/src/workflow/kick/steps/cache.rs | Implements cache pruning for kicked participants and adds integration tests. |
| crates/decman/src/workflow/kick/mod.rs | Re-exports prune_cached_membership from the kick module API surface. |
| crates/decman/src/workflow/kick/coordinator.rs | Calls cache pruning after submit_kick in the coordinator workflow. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
11 tasks
Two review findings. Rebuilding the participant set re-upserted every surviving row, and `replace_dec_party_participants` overwrites `permission` unconditionally, so a permission change landed by a concurrent /decentralized-parties refresh between the read and the write was reverted. Delete the single row instead; nothing here needs the others. Pruning also ran with `?` inside SubmitKick, so a DB error after the topology transactions were already durable left the workflow on that step and a resume resubmitted them. Log and continue: a stale cache is recoverable by the next refresh, a rewound kick is not.
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.
Summary
The
add_party_missing_darintegration phase has been failing onmainroughly every other run, always at the same step:It is not the test.
POST /add-partyvalidates membership against the cacheddec_party_participantstable, and the only production writer of that table isstore_parties_to_db, reached solely from the/decentralized-partiesrefresh path. The kick workflow never touched it, so after a kick completed the coordinator still held the removed participant as a member. The phase was green only when an unrelated refresh happened to land between the kick and the re-add, which is exactly the alternating pass/fail we saw.The same staleness inflates
post_add_member_count, so thenew_thresholdupper bound was one member too high after any kick.An operator scripting kick then add-party hits the same 409, so this is a product bug rather than test flake.
Related issues
Surfaced while chasing the red Integration Tests job on #385.
Type of change
Checklist
cargo fmt -- --checkpassescargo clippy --all-targets --all-features -- -D warningsis cleancargo testpasses<type>(<scope>): <subject>conventionNotes for reviewers
The prune runs on the coordinator right after
submit_kickhas confirmed both topology mappings, so it only fires once the kick is actually durable.It is deliberately surgical rather than a full
/decentralized-partiesrefresh: the workflow already knows exactly which participant it removed, so no extra network call is needed and there is nothing to fail on a flaky admin API.Two gaps I did not close here, happy to follow up:
dec_party.thresholdis also stale after a kick. Nothing validates against it today (previous_thresholdis display-only and carried in the request), so it is latent rather than live.Worth merging before #385, whose Integration Tests job is red on this and nothing else.