fix: make session store fcntl import cross-platform compatible#1544
Conversation
…1540) - Add conditional fcntl import like file_memory.py already has - Use _HAS_FCNTL flag to check fcntl availability before usage - Windows uses msvcrt for locking, Unix uses fcntl when available - Fixes Windows test collection failure due to missing fcntl module Co-authored-by: praisonai-triage-agent[bot] <praisonai-triage-agent[bot]@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change adds platform-aware file locking to the session store by conditionally importing and using Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
Greptile SummaryThis PR fixes a Windows import-time crash by moving Confidence Score: 5/5Safe to merge — the fix is correct, narrowly scoped, and all remaining findings are P2 or lower. The root cause (unconditional No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Module Import] --> B{sys.platform win32?}
B -- Yes --> C[_HAS_FCNTL = False\nfcntl not imported]
B -- No --> D[import fcntl\n_HAS_FCNTL = True]
C --> E[FileLock.acquire]
D --> E
E --> F{sys.platform win32?}
F -- Yes --> G[msvcrt.locking LK_NBLCK]
F -- No --> H{_HAS_FCNTL?}
H -- True --> I[fcntl.flock LOCK_EX+NB]
H -- False --> J[Log warning once\nReturn True without lock]
G --> K{Success?}
I --> K
J --> K
K -- Yes --> L[return True]
K -- No --> M{Timeout?}
M -- No --> N[sleep 50ms and Retry]
M -- Yes --> O[Log warning\nreturn False]
N --> E
Reviews (2): Last reviewed commit: "fix: add one-time warning for degraded f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 the current code and only fix it if needed.
Inline comments:
In `@src/praisonai-agents/praisonaiagents/session/store.py`:
- Around line 157-161: The acquire()/release() path silently no-ops when
_HAS_FCNTL is False, so add a one-time warning when entering acquire() or
release() and _HAS_FCNTL is False (and os.name != "nt") to alert operators that
inter-process locking is degraded; implement this by adding a module-level
sentinel (e.g., _WARNED_NO_FCNTL) and, when first detected in methods acquire()
and release() (which reference self._lock_file and call fcntl.flock with
LOCK_EX/LOCK_UN), emit a warning via the existing logger (or the logging module)
that clearly states file locking is unavailable and may risk concurrent writes,
then set the sentinel so the warning only logs once.
🪄 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
Run ID: cef47fd0-7c66-4eba-bfd7-1c17f390ece0
📒 Files selected for processing (1)
src/praisonai-agents/praisonaiagents/session/store.py
| else: | ||
| # Unix locking | ||
| fcntl.flock(self._lock_file.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) | ||
| if _HAS_FCNTL: | ||
| fcntl.flock(self._lock_file.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) | ||
| return True |
There was a problem hiding this comment.
Silent no-op locking when _HAS_FCNTL is False on a non-Windows platform.
If _HAS_FCNTL ends up False on a non-Windows platform (possible today only in the hardened variant above, but still worth guarding), acquire() skips the flock call and immediately returns True, giving callers the illusion of a successful inter-process lock while no locking actually occurs. This can silently corrupt session JSON under concurrent writers. At minimum, log a one-time warning so operators are aware locking is degraded; the same applies symmetrically to release() at Lines 181–182.
🪵 Suggested one-time warning
else:
# Unix locking
if _HAS_FCNTL:
fcntl.flock(self._lock_file.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
+ elif not getattr(FileLock, "_warned_no_lock", False):
+ logger.warning(
+ "File locking unavailable on this platform (no fcntl/msvcrt); "
+ "concurrent writers may corrupt session files."
+ )
+ FileLock._warned_no_lock = True📝 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.
| else: | |
| # Unix locking | |
| fcntl.flock(self._lock_file.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) | |
| if _HAS_FCNTL: | |
| fcntl.flock(self._lock_file.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) | |
| return True | |
| else: | |
| # Unix locking | |
| if _HAS_FCNTL: | |
| fcntl.flock(self._lock_file.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) | |
| elif not getattr(FileLock, "_warned_no_lock", False): | |
| logger.warning( | |
| "File locking unavailable on this platform (no fcntl/msvcrt); " | |
| "concurrent writers may corrupt session files." | |
| ) | |
| FileLock._warned_no_lock = True | |
| return True |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai-agents/praisonaiagents/session/store.py` around lines 157 -
161, The acquire()/release() path silently no-ops when _HAS_FCNTL is False, so
add a one-time warning when entering acquire() or release() and _HAS_FCNTL is
False (and os.name != "nt") to alert operators that inter-process locking is
degraded; implement this by adding a module-level sentinel (e.g.,
_WARNED_NO_FCNTL) and, when first detected in methods acquire() and release()
(which reference self._lock_file and call fcntl.flock with LOCK_EX/LOCK_UN),
emit a warning via the existing logger (or the logging module) that clearly
states file locking is unavailable and may risk concurrent writes, then set the
sentinel so the warning only logs once.
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture ReviewPhase 1: Architecture Review per AGENTS.md
Phase 2: Address Valid Reviewer Issues
Phase 3: Final Verdict
Architecture Analysis✅ Protocol-driven design: The fix maintains proper layering - file locking is a low-level implementation detail in the session store, not core SDK concerns. ✅ Backward compatibility: No public API changes. Existing code will continue to work exactly as before. ✅ Performance: No hot-path regressions. The conditional import happens once at module load time and adds negligible overhead. The fix correctly mirrors the pattern already established in Issues Addressed✅ Silent no-op locking warning: Added module-level Files Modified
Testing✅ Syntax validation: Python compilation successful Final Verdict✅ APPROVED - This PR successfully fixes the Windows compatibility issue with minimal, focused changes that align with the framework's architecture principles. The additional warning enhancement improves error visibility and code robustness. Ready to merge. |
Addresses CodeRabbit feedback about silent no-op locking when fcntl is unavailable on non-Windows platforms. Adds module-level sentinel to emit warning only once, following AGENTS.md error handling principles. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
1 participant
Fixes #1540
Summary
Changes
Testing
Generated with Claude Code
Summary by CodeRabbit