fix(groups): cadence claim had no expiry; a NATS publish failure could drop a turn - #657
fix(groups): cadence claim had no expiry; a NATS publish failure could drop a turn#657ginccc wants to merge 3 commits into
Conversation
…ure dropping a turn The claim protocol had no expiry. Its Javadoc promised crash-proofness via 'the next fire finds the terminal state', but nothing moves a discussion to a terminal state when its pod dies, so reconcile answered 'still running' forever and the cadence was wedged until a human cancelled by hand. A non-terminal, non-paused discussion whose progress heartbeat has not advanced within the lease is now cancelled, its tasks returned, and the claim released. The lease is measured on gc.getLastModified(), not claim age: a claim-age lease cannot tell a dead pod from a healthy long-running discussion, and reclaiming a live one would orphan its outcomes AND double-schedule its tasks. AWAITING_* never expires — those resolve cross-pod. CREATED does, being the crash window between the claim CAS and the executor starting. reconcile also caught every read exception as 'discussion gone', so a transient error double-scheduled the backlog; narrowed to ResourceNotFoundException. NatsConversationCoordinator.publishAndExecute caught only the two checked types, so an unchecked publish failure escaped and skipped the execution below it — dropping the turn with no callback and no dead-letter, while the catch block promised 'executing locally'. +14 tests, including two that pin invariants previously resting on prose: NATS callables execute in-process, and neither storage backend inherits the throwing storeIfFieldEquals default.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Warning Review limit reached
Next review available in: 36 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds abandoned-discussion recovery to team cadence processing, preserves claims on transient reads, and keeps local execution after NATS publish failures. It also adds storage CAS contract tests and runtime invariants. ChangesRuntime reliability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TeamCadenceService
participant ConversationStore
participant WorkspaceStore
TeamCadenceService->>ConversationStore: Read claimed discussion
ConversationStore-->>TeamCadenceService: Return discussion or read error
TeamCadenceService->>TeamCadenceService: Check state and lease
TeamCadenceService->>ConversationStore: Cancel stale discussion and write failure
TeamCadenceService->>WorkspaceStore: 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 |
… log statements Matches the file's existing convention (groupId was already sanitized) and pre-empts the log-injection class CodeQL flagged on the sibling branches.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/engine/runtime/internal/TeamCadenceService.java`:
- Around line 367-370: Update the last-progress check in TeamCadenceService so a
null gc.getLastModified() is treated as stale and eligible for reclamation
rather than active; only a non-null timestamp newer than the abandonedRunLease
threshold should return false. Add coverage for recovery of a group conversation
with null lastModified.
🪄 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: e6bd3642-dda4-4433-a1e4-07a8504881b8
📒 Files selected for processing (7)
docs/changelog.mdsrc/main/java/ai/labs/eddi/engine/runtime/internal/NatsConversationCoordinator.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/TeamCadenceService.javasrc/test/java/ai/labs/eddi/datastore/StoreIfFieldEqualsContractTest.javasrc/test/java/ai/labs/eddi/engine/runtime/internal/NatsCoordinatorInProcessInvariantTest.javasrc/test/java/ai/labs/eddi/engine/runtime/internal/TeamCadenceClaimLeaseTest.javasrc/test/java/ai/labs/eddi/engine/runtime/internal/TeamCadenceServiceTest.java
…ussion CodeRabbit on #657 (Major): reclaimIfAbandoned treated a null lastModified as 'still advancing', which reinstates exactly the deadlock the lease exists to break for any record whose lastModified was never written. Falls back to the creation stamp — which the discussion-creation path always sets, so a discussion created moments ago is not mistaken for an abandoned one — and reclaims when neither timestamp exists, since that is the definition of no progress ever recorded. +2 tests: null-everything reclaims, null-lastModified-but-recently-created does not.
|
Superseded — closing without merging. While this branch was in review, I re-checked every finding in this PR against current
No work is lost; the review threads here remain readable for the reasoning. |
2 participants
1. A crashed pod wedged the cadence permanently
TeamCadenceService's Javadoc promised the run protocol was "crash-proof by construction: a pod crash mid-discussion loses nothing, because the next fire finds the terminal state and reconciles it."That only holds if something moves the discussion to a terminal state. On a pod crash nothing does — no startup sweep touches an
IN_PROGRESSGroupConversation, andHitlCrashRecoveryObserverhandles only theAWAITING_*states. SoreconcilereadIN_PROGRESS, answered "still running", and did so on every subsequent fire forever.casRunningDiscussionwas never released and the cadence stayed wedged until a human cancelled the discussion by hand.A non-terminal, non-paused discussion whose progress heartbeat has not advanced within
eddi.groups.cadence.abandoned-run-lease(defaultPT6H) is now cancelled, its tasks returned to the backlog, and the claim released.The lease is measured on
gc.getLastModified(), not on claim age. A claim-age lease cannot tell a dead pod from a healthy long-running discussion, and reclaiming a live one would orphan its outcomes and double-schedule its tasks — recreating, by design, the bug fixed in §2. The discussion loop persists the conversation at every phase boundary, solastModifiedis a free liveness heartbeat.AWAITING_*never expires, at any age. A discussion may legitimately wait on a human for days, and every surface that resolves one — resume, the cross-pod cancel CAS, the timeout policies re-armed at startup — works cross-pod, so a paused discussion on a dead pod still progresses. Expiring one would destroy a live pending approval.CREATEDis reclaimable — it is precisely the crash window between the claim CAS and the executor starting.The abandoned discussion is cancelled before the claim is released, so a zombie loop that somehow survives cannot keep spending the cadence's budget on work nobody will collect.
2. A transient read error released the claim
reconcilecaught every exception fromconversationStore.readand treated it as "discussion gone", returning the pulled tasks toPENDING. A read failure while the discussion was genuinely running therefore let the next fire start a second discussion on the same backlog.Narrowed to
ResourceNotFoundException— provable absence — which is the distinctionAgentDeploymentManagement.isAgentConfigMissingalready makes one package over. Anything else skips the fire and keeps the claim.3. A NATS publish failure could silently drop a conversation turn
publishAndExecutecaught onlyIOException | JetStreamApiException, so any unchecked failure — the NATS client throwsIllegalStateExceptionon a closed or draining connection — escaped the method and skipped thesubmitCallablebelow it entirely. The turn was dropped with no execution, no callback and no dead-letter, while that very catch block logged "executing locally".The publish is an ordering marker, never the work itself, so no publish failure may cost a conversation its turn.
Two invariants pinned that previously rested on prose
NatsCoordinatorInProcessInvariantTestasserts the NATS coordinator hands the callable to the localIRuntimerather than serializing it onto the stream.LiveDiscussionRegistryis per-node and its Javadoc names this dependency explicitly; if a member turn could ever run off-node the group task / artifact / recruit tools would not fail — they would silently not be assembled, and the model would lose them mid-discussion with nothing in the logs. The invariant was documented as verified in the changelog; it is now enforced.StoreIfFieldEqualsContractTestpins that neither backend inherits the throwing default and that both declare both CAS outcomes.compareAndSetState,updateIfStateandcasRunningDiscussionare all implemented once overIResourceStorage.storeIfFieldEquals, so every CAS-based safety property inexecuteDiscussionand the claim protocol is exactly as good as that method is on the active backend.Reviewed in full as part of this work: the Mongo and Postgres implementations do agree — conditional update,
rows == 0→ existence probe → 404-vs-409 — and thelongoverload correctly degrades to text on Postgres (wheredata->>renders a JSON number canonically) while Mongo needs typed BSON equality. Nothing tested any of it.Disproved during review, not changed
Cadence.maxBacklogTasksPerRunwas flagged as an unvalidated.limit()argument. It is not: the record's compact constructor clamps any non-positive value to the default, so.limit()can never receive a negative.Testing
151 tests across TeamCadence / NATS / GroupWorkspace stay green. +14 new.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests