fix(groups): a paused cadence discussion no longer wedges a standing team forever - #650
Merged
Conversation
…team forever Wave C of the Agent / Group Agent review -- a liveness defect in I13 Standing Teams. TeamCadenceService.reconcile releases a workspace's runningDiscussionId claim when its discussion reaches a terminal state. AWAITING_APPROVAL and AWAITING_HUMAN_INPUT are not terminal, so they fell into the "default -> false" (still running) arm -- correctly, for a discussion that will be approved. But the default group HITL timeout policy is WAIT_INDEFINITELY, so a pause nobody resolves never becomes terminal either, and the claim was held forever: every subsequent cadence fire for that group was skipped as "still running", and the backlog tasks that run had pulled stayed IN_PROGRESS. There was no claim TTL and no reaper anywhere -- out of character for a subsystem whose task-force half carries an explicit no-progress guard precisely to guarantee termination. - GroupWorkspace gains a nullable claimedAt stamp, written next to runningDiscussionId and cleared next to it in settle(). Nullable on purpose: documents predating the field have no stamp, and reclaiming those on a missing timestamp would be a guess. - reconcile's non-terminal arm routes through reclaimIfStale, which cancels the stranded discussion and runs the ORDINARY failure writeback (pulled tasks back to PENDING, claim cleared) rather than a bespoke path. Cancel before release, so a reclaimed run cannot keep spending against a budget nobody tracks. - TTL is eddi.groups.cadence.claim-ttl, default PT24H. Deliberately generous: an approval arriving the next business morning must land on the discussion it belongs to, not on a reclaimed corpse. Non-positive disables reclaiming. - New counter eddi_team_cadence_claims_reclaimed_total, plus a WARN naming the stranded discussion and how long the claim was held. - Adding a cadence now warns when the group combines requiresApproval phases with WAIT_INDEFINITELY -- that combination is what makes the backstop reachable. A warning, not a rejection: it is legitimate for a team whose approver really is always available. Also: cancelQuietly now passes CANCEL_GRACEFUL explicitly. null already resolved to graceful, but the method has a second caller now. 585 tests green; 9 new.
Contributor
📝 WalkthroughWalkthroughCadence claims now store acquisition timestamps. Configurable TTL handling reclaims stale paused discussions through graceful cancellation and normal task writeback. Cadence creation warns when approval phases can wait indefinitely for human input. ChangesCadence claim lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TeamCadenceService
participant GroupWorkspaceStore
participant GroupConversationService
TeamCadenceService->>GroupWorkspaceStore: Read claim timestamp and running discussion
TeamCadenceService->>GroupConversationService: Cancel expired discussion gracefully
TeamCadenceService->>GroupWorkspaceStore: Return tasks to PENDING and release claim
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
This was referenced Aug 9, 2026
docs(groups): attachments, protocol defaults, context scopes, and a "not yet supported" section
#652
Merged
aisabella-ai
self-requested a review
August 10, 2026 17:32
aisabella-ai
approved these changes
Aug 10, 2026
# Conflicts: # docs/changelog.md
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/ai/labs/eddi/configs/groups/rest/RestGroupWorkspace.java`:
- Around line 331-333: Update the warning construction in RestGroupWorkspace so
it does not unconditionally claim the claim TTL will reclaim the run. Make the
wording conditional on a positive, enabled eddi.groups.cadence.claim-ttl value,
and explicitly indicate that non-positive values can retain runningDiscussionId
indefinitely.
- Around line 193-197: Move the indefinite-pause warning block using
indefinitePauseWarning and LOG.warnf to after scheduleStore.createSchedule and
workspaceStore.update both complete successfully, so it only reports a persisted
cadence. Keep the existing warning message and sanitization unchanged.
In `@src/main/java/ai/labs/eddi/engine/runtime/internal/TeamCadenceService.java`:
- Around line 339-343: Update the cancellation handling in the flow around
cancelQuietly and writebackFailure so failure writeback occurs only after
cancellation is confirmed; when cancellation fails or is unconfirmed, retain the
claim or re-read the discussion and follow the normal terminal-state path. Move
cadenceClaimsReclaimed.increment() to execute only when
writebackFailure(workspace, gc) returns true, preventing failed CAS races from
being counted as reclaimed claims.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 872b92cd-488f-43fe-b797-4c846c635988
📒 Files selected for processing (6)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/groups/model/GroupWorkspace.javasrc/main/java/ai/labs/eddi/configs/groups/rest/RestGroupWorkspace.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/TeamCadenceService.javasrc/test/java/ai/labs/eddi/engine/runtime/internal/TeamCadenceClaimExpiryTest.javasrc/test/java/ai/labs/eddi/engine/runtime/internal/TeamCadenceServiceTest.java
Comment on lines
+193
to
+197
| String indefinitePause = indefinitePauseWarning(groupId); | ||
| if (indefinitePause != null) { | ||
| LOG.warnf("Cadence added to group %s which pauses for approval under WAIT_INDEFINITELY: %s", | ||
| sanitize(groupId), indefinitePause); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Log the warning only after cadence persistence succeeds.
This block runs before scheduleStore.createSchedule and workspaceStore.update. If either operation fails, the log states that a cadence was added even though no cadence was persisted. Move the warning after the successful workspace update, or use preflight wording.
Proposed fix
String indefinitePause = indefinitePauseWarning(groupId);
-if (indefinitePause != null) {
- LOG.warnf("Cadence added to group %s which pauses for approval under WAIT_INDEFINITELY: %s",
- sanitize(groupId), indefinitePause);
-}
...
workspaceStore.update(workspace);
+if (indefinitePause != null) {
+ LOG.warnf("Cadence added to group %s which pauses for approval under WAIT_INDEFINITELY: %s",
+ sanitize(groupId), indefinitePause);
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/ai/labs/eddi/configs/groups/rest/RestGroupWorkspace.java`
around lines 193 - 197, Move the indefinite-pause warning block using
indefinitePauseWarning and LOG.warnf to after scheduleStore.createSchedule and
workspaceStore.update both complete successfully, so it only reports a persisted
cadence. Keep the existing warning message and sanitization unchanged.
Comment on lines
+331
to
+333
| return "the group has requiresApproval phase(s) and hitlConfig.timeoutPolicy=WAIT_INDEFINITELY, so an " | ||
| + "unapproved run holds this team's cadence claim until the claim TTL reclaims it; set a finite " | ||
| + "timeoutPolicy to resolve such pauses properly"; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make the warning accurate when claim reclamation is disabled.
Non-positive eddi.groups.cadence.claim-ttl values disable reclamation. This message always says that the claim TTL reclaims the run. In the disabled configuration, an unapproved discussion can retain runningDiscussionId indefinitely.
Make the message conditional on an enabled positive TTL, or remove the unconditional reclamation promise.
Proposed wording
- + "unapproved run holds this team's cadence claim until the claim TTL reclaims it; set a finite "
- + "timeoutPolicy to resolve such pauses properly";
+ + "unapproved run can hold this team's cadence claim indefinitely; set a finite timeoutPolicy "
+ + "and configure a positive eddi.groups.cadence.claim-ttl if reclamation is desired";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return "the group has requiresApproval phase(s) and hitlConfig.timeoutPolicy=WAIT_INDEFINITELY, so an " | |
| + "unapproved run holds this team's cadence claim until the claim TTL reclaims it; set a finite " | |
| + "timeoutPolicy to resolve such pauses properly"; | |
| return "the group has requiresApproval phase(s) and hitlConfig.timeoutPolicy=WAIT_INDEFINITELY, so an " | |
| "unapproved run can hold this team's cadence claim indefinitely; set a finite timeoutPolicy " | |
| "and configure a positive eddi.groups.cadence.claim-ttl if reclamation is desired"; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/ai/labs/eddi/configs/groups/rest/RestGroupWorkspace.java`
around lines 331 - 333, Update the warning construction in RestGroupWorkspace so
it does not unconditionally claim the claim TTL will reclaim the run. Make the
wording conditional on a positive, enabled eddi.groups.cadence.claim-ttl value,
and explicitly indicate that non-positive values can retain runningDiscussionId
indefinitely.
Comment on lines
+339
to
+343
| cancelQuietly(gc.getId()); | ||
| cadenceClaimsReclaimed.increment(); | ||
| // The ordinary failure writeback: returns the pulled tasks to PENDING and | ||
| // clears the claim, exactly as a FAILED/CANCELLED discussion would. | ||
| return writebackFailure(workspace, gc); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not release a claim when cancellation is unconfirmed.
Line 339 calls cancelQuietly, but Lines 517-526 discard both exceptions and the boolean result from cancelDiscussion. Line 343 then releases the claim and returns tasks to PENDING even if cancellation failed. The original discussion can continue without a workspace claim while a later fire starts another discussion.
Only run failure writeback after cancellation succeeds. If cancellation fails, keep the claim or re-read the discussion and use the normal terminal-state path. Increment cadenceClaimsReclaimed only after writebackFailure returns true, or failed CAS races overcount reclaimed claims.
Proposed fix
- cancelQuietly(gc.getId());
- cadenceClaimsReclaimed.increment();
- // The ordinary failure writeback: returns the pulled tasks to PENDING and
- // clears the claim, exactly as a FAILED/CANCELLED discussion would.
- return writebackFailure(workspace, gc);
+ if (!cancelQuietly(gc.getId())) {
+ return false;
+ }
+ // The ordinary failure writeback: returns the pulled tasks to PENDING and
+ // clears the claim, exactly as a FAILED/CANCELLED discussion would.
+ boolean released = writebackFailure(workspace, gc);
+ if (released) {
+ cadenceClaimsReclaimed.increment();
+ }
+ return released;-private void cancelQuietly(String discussionId) {
+private boolean cancelQuietly(String discussionId) {
try {
- groupConversationService.cancelDiscussion(discussionId, ControlSignal.CANCEL_GRACEFUL);
+ return groupConversationService.cancelDiscussion(discussionId, ControlSignal.CANCEL_GRACEFUL);
} catch (Exception e) {
LOGGER.warnf("Could not cancel discussion %s after a lost or expired cadence claim: %s", discussionId, e.getMessage());
+ return false;
}
}Also applies to: 517-526
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/ai/labs/eddi/engine/runtime/internal/TeamCadenceService.java`
around lines 339 - 343, Update the cancellation handling in the flow around
cancelQuietly and writebackFailure so failure writeback occurs only after
cancellation is confirmed; when cancellation fails or is unconfirmed, retain the
claim or re-read the discussion and follow the normal terminal-state path. Move
cadenceClaimsReclaimed.increment() to execute only when
writebackFailure(workspace, gc) returns true, preventing failed CAS races from
being counted as reclaimed claims.
This was referenced Aug 10, 2026
Closed
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
2 participants
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.
Wave C of the Agent / Group Agent review (Wave A #648, Wave B #649) — a liveness defect in I13 Standing Teams, the newest part of the group subsystem.
The defect
TeamCadenceService.reconcilereleases a workspace'srunningDiscussionIdclaim when its discussion reaches a terminal state:AWAITING_APPROVALandAWAITING_HUMAN_INPUTfall intodefault— correctly, for a discussion that will be approved. But the default group HITL timeout policy isWAIT_INDEFINITELY(AgentGroupConfiguration:1354), so a pause nobody resolves never becomes terminal either.The claim was therefore held forever:
IN_PROGRESS,One unapproved run permanently wedges a standing team, with no unusual configuration required. That is out of character for this subsystem — its task-force half carries an explicit no-progress fingerprint guard (
taskPauseFingerprint) precisely to guarantee termination.The fix
GroupWorkspace.claimedAt— a nullable stamp written next torunningDiscussionIdand cleared next to it insettle(). The two are one fact, so they are set and cleared together. Nullable on purpose: documents written before the field existed have no stamp, and reclaiming those on a missing timestamp would be a guess — they get one on their next claim.reclaimIfStale—reconcile's non-terminal arm now routes through it. It cancels the stranded discussion, then runs the ordinary failure writeback (pulled tasks back toPENDING, claim cleared) rather than a bespoke path. Cancel before release, so a reclaimed run cannot keep spending against a budget nobody is tracking any more.eddi.groups.cadence.claim-ttl, defaultPT24H. Deliberately generous: an approval that arrives the next business morning must still land on the discussion it belongs to, not on a reclaimed corpse. This is a liveness backstop for a wedged team, not an SLA. Non-positive disables reclaiming entirely, for an operator who would rather wedge than risk abandoning a pause.eddi_team_cadence_claims_reclaimed_total, plus a WARN naming the stranded discussion, its state, and how long the claim was held.requiresApprovalphases withWAIT_INDEFINITELY— that combination is exactly what makes the backstop reachable, and the operator almost certainly wanted a finitetimeoutPolicy(which resolves such pauses properly instead of abandoning them). A warning, not a rejection: the combination is legitimate for a team whose approver genuinely is always available, and rejecting it would break existing configs.Also
cancelQuietlynow passesControlSignal.CANCEL_GRACEFULexplicitly.nullalready resolved to graceful — onlyCANCEL_IMMEDIATEtakes the other branch inGroupHitlCoordinator— but the method has a second caller now, and "which cancel is this?" should not require reading another class to answer.Testing
585 tests green across
TeamCadence*,GroupWorkspace*,RestGroupWorkspace*,GroupConversationService*; 9 new inTeamCadenceClaimExpiryTest, covering both pause states, a fresh pause inside the TTL (must keep its claim), a genuinely running discussion, a missing stamp, a disabled TTL, an idle workspace, and that the ordinary COMPLETED path is not shadowed.Summary by CodeRabbit
New Features
Bug Fixes
Documentation