persist provider threads per conversation - #222
Conversation
|
Warning Review limit reached
Next review available in: 38 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)
📝 WalkthroughWalkthroughCodex completion now supports persistent, keyed sessions with 30-day expiration, session validation, incremental input, atomic state writes, and reset handling. The router derives and propagates scoped session keys across planning, tool, retry, fallback, and plain completion paths. ChangesCodex resumable sessions
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🔵 Low · up to The change makes provider threads durable across conversation turns and restarts. A fallback path can omit tool results in some context-mismatch cases, and concurrent processes sharing state can occasionally reset persisted sessions; these are bounded follow-up risks, so the PR is mergeable with explicit owner awareness. Sequence Diagram(s)sequenceDiagram
participant Router
participant CodexProvider
participant CodexSessionStore
participant CodexAppServer
Router->>CodexProvider: Send completion with codex_session_key
CodexProvider->>CodexSessionStore: Load session metadata
CodexProvider->>CodexAppServer: Resume thread with incremental input
CodexAppServer-->>CodexProvider: Return turn result
CodexProvider->>CodexSessionStore: Persist updated metadata
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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
🧹 Nitpick comments (3)
src/octopal/infrastructure/providers/codex_provider.py (2)
396-406: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueThe turn lock is instance-scoped.
self._session_lockslives on the provider instance. TwoCodexProviderinstances in one process that share astate_dircan run concurrent turns against the same persistedthread_id. If the runtime creates one provider per process, no action is needed. If several instances can exist, move the lock registry to module scope keyed by(state_dir, session_ref).🤖 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/octopal/infrastructure/providers/codex_provider.py` around lines 396 - 406, Make session-turn locking shared across CodexProvider instances by moving the _session_locks registry to module scope and keying entries by both state_dir and session_ref. Update the turn path around _run_session_turn and provider initialization/access accordingly, while preserving the existing per-session asyncio.Lock behavior.
77-97: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider cross-process safety for the registry read-modify-write.
putanddeleteread the full JSON file, mutate it, then rewrite it. Inside one event loop this cycle is atomic because noawaitoccurs. Across processes it is not. If two Octopal processes share the samestate_dir, one write can drop entries written by the other. The visible effect is an unnecessary session reset, so this is not urgent. If you expect several processes on one state directory, store one file persession_refor add an advisory file lock around the read-modify-write.🤖 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/octopal/infrastructure/providers/codex_provider.py` around lines 77 - 97, Make the session registry update cross-process safe by changing the read-modify-write flow in _CodexSessionRegistry.put and delete: either persist each session_ref in its own file or protect the existing JSON mutations with an advisory file lock spanning _read, mutation, and _write. Preserve current session data and deletion behavior while preventing concurrent processes from overwriting each other’s entries.tests/test_codex_provider_sessions.py (1)
242-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the TTL and corrupt-state reset paths.
_CodexSessionStore.getreturns"expired"afterCODEX_SESSION_TTL_DAYSand"invalid_state"for a malformed record. Neither path is covered. Both delete persisted state and force a freshthread/start. Add two cases: write a registry entry with an oldupdated_at, and write a registry entry that missesthread_id. Assert that the next call performsthread/startand notthread/resume.🤖 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 `@tests/test_codex_provider_sessions.py` around lines 242 - 270, Add separate tests alongside test_resume_failure_falls_back_to_full_context_and_replaces_mapping for expired and malformed session records. Seed codex_sessions.json with an entry whose updated_at exceeds CODEX_SESSION_TTL_DAYS, and another entry missing thread_id; invoke complete_with_tools with the matching codex_session_key and assert the client calls thread/start without thread/resume, preserving the expected fresh-session behavior.
🤖 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/octopal/infrastructure/providers/codex_provider.py`:
- Around line 736-747: Update the mismatch fallback in the fingerprint suffix
selection to preserve the trailing tool-result messages along with the latest
user message. When scanning reversed messages, collect the contiguous terminal
tool messages and include them with the most recent user message, while
retaining the existing behavior for ordinary user-ended requests.
---
Nitpick comments:
In `@src/octopal/infrastructure/providers/codex_provider.py`:
- Around line 396-406: Make session-turn locking shared across CodexProvider
instances by moving the _session_locks registry to module scope and keying
entries by both state_dir and session_ref. Update the turn path around
_run_session_turn and provider initialization/access accordingly, while
preserving the existing per-session asyncio.Lock behavior.
- Around line 77-97: Make the session registry update cross-process safe by
changing the read-modify-write flow in _CodexSessionRegistry.put and delete:
either persist each session_ref in its own file or protect the existing JSON
mutations with an advisory file lock spanning _read, mutation, and _write.
Preserve current session data and deletion behavior while preventing concurrent
processes from overwriting each other’s entries.
In `@tests/test_codex_provider_sessions.py`:
- Around line 242-270: Add separate tests alongside
test_resume_failure_falls_back_to_full_context_and_replaces_mapping for expired
and malformed session records. Seed codex_sessions.json with an entry whose
updated_at exceeds CODEX_SESSION_TTL_DAYS, and another entry missing thread_id;
invoke complete_with_tools with the matching codex_session_key and assert the
client calls thread/start without thread/resume, preserving the expected
fresh-session behavior.
🪄 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: 9d13d282-76c5-43be-a9a1-adad0f35cb0d
📒 Files selected for processing (4)
src/octopal/infrastructure/providers/codex_provider.pysrc/octopal/runtime/octo/route_completion.pysrc/octopal/runtime/octo/router.pytests/test_codex_provider_sessions.py
1 participant
Summary
Root cause and fix
The main conversation router computed the scoped session key, but its planner call used the raw provider. When the planner selected
mode=reply, the early return bypassed the keyed tool-loop path, so ordinary chat replies remained ephemeral.The planner now receives the same scoped conversation key through a narrow provider delegate. System and internal routes keep distinct scoped identities, while worker and helper calls without a conversation key remain isolated and ephemeral.
Validation
pytestsuite passed with one expected Windows-only skipSummary by CodeRabbit
New Features
Bug Fixes