refactor(core): collapse speculative flow-store scaffolding - #47
Conversation
The conversation flow engine carried a durable/multi-instance Store seam that nothing built against. Cut it down to what the single-instance, in-memory v1 actually uses: - Remove the ConversationStore and expirer interfaces (one impl each, never injected) — fold memConversationStore's map straight into conversationManager. - Drop ConversationState.Version — write-only, only ever self-incremented, read nowhere; it existed for a future compare-and-swap path. - Replace the 256-way striped locks + inlined FNV-1a hasher with a single mutex. Sweep now holds it across the scan, so the expiredKeys snapshot + per-key re-check goes away. - Remove sweepRecovered — with no custom store, sweep has no panic path. Behavior is unchanged; the flow/advance/sweeper tests still pass under -race. Net ~190 fewer lines, no dependency change.
|
✅ No issues found.
|
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (1)
✨ Finishing Touches📝 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 |
![botbooter-test[bot]](https://avatars.githubusercontent.com/u/197033?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: approve · Grade: 9/10
To reach a higher grade: Refresh the stale 'shard'/'striped shards' wording in flow.go's Validate doc to reflect the single-mutex model.
📝 Summary of changes
Clean refactor that removes speculative Store/expirer/Version/striped-lock scaffolding from the conversation engine, collapsing it to a single mutex + map. The concurrency reasoning is sound: sweep now holds the manager mutex across the whole scan (safe to delete during range in Go), which also eliminates the old snapshot-then-recheck dance; withLock still releases on panic via defer; transitionLocked continues to run user validators/callbacks under the lock with the same reaping asymmetry. startSweeper's panic-recovery removal is justified — sweep now contains only map/time operations with no user code, so there is no panic path to guard. Tests are updated consistently and the -race coverage is preserved.
One out-of-diff follow-up worth noting (not a blocking finding since the file isn't touched here): flow.go's Validate doc still warns that a slow validator "blocks the shard for its whole duration, stalling ... every other one whose key hashes to the same one of the striped shards." With the new single global mutex that warning is both stale (no shards) and understated — a slow validator now stalls every conversation, not just same-shard ones. Refreshing that comment would keep the docs honest with the new locking model.
✅ No issues found.
Reviewed by prbooter. Comment /recheck to re-run, /security-review for a security audit, /auto-fix to have me attempt the fixes, or /clean to remove my comments.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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 `@internal/core/flow_test.go`:
- Around line 105-111: Direct conversationManager get/set calls in flow tests
bypass the required locking contract. Add getConvState and setConvState helpers
that invoke get/set inside withLock, then replace every unguarded
bot.conversations.get/set call throughout this test file with the corresponding
helper while preserving existing assertions and state updates.
🪄 Autofix (Beta)
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: c43b3c41-4efd-4ddd-a708-01e1ff8a7557
📒 Files selected for processing (4)
internal/core/conversation.gointernal/core/conversation_test.gointernal/core/core.gointernal/core/flow_test.go
get/set/del are documented as requiring the manager mutex; flow_test was calling them unguarded. Add getConvState/setConvState that wrap the access in withLock so every site honors the contract.
1 participant
Removes the durable/multi-instance-Store scaffolding from the conversation flow engine — abstraction built for a future that isn't here and that no caller touches. Behavior is unchanged; only the in-memory single-instance path remains.
Cuts (from the repo-wide over-engineering audit)
ConversationStore+expirerinterfaces — one implementation each, never injected (the ctor hardcodedmemConversationStore, no setter). Folded the map directly intoconversationManager.ConversationState.Version— write-only field; only ever self-incremented inSet, read nowhere in prod or tests. Existed for a future compare-and-swap path.sweepnow holds it across the scan, so theexpiredKeyssnapshot + per-key re-check collapses into one pass.sweepRecovered— with no custom store,sweephas no panic path, so the recover guard guarded nothing.Verification
make allgreen (fmt + vet + lint +test-race, incl._examples).-race. Pure-abstraction tests (Version bump, striped-lock bounds, expirer snapshot, store-panic recovery) removed with the code they covered.Net: ~190 fewer lines, no dependency change.
Summary by CodeRabbit