Repository navigation
Conversation
Fixes plastic-labs#940. Preserve joined_at for active session peers during idempotent re-adds. Only a peer that has left and rejoins starts a new membership window. This keeps peer_perspective search visibility across client restarts. The change includes regression coverage for membership timestamps, configuration behavior, and peer_perspective search visibility.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe session-peer update preserves ChangesSession peer re-add behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/crud/session.py (1)
1074-1076: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftCalculate the observer limit from the effective configuration.
The observer-limit query at Lines 1023-1046 excludes every peer in
peer_namesand counts the incoming configuration. This upsert now retains the stored configuration for active peers.At a limit of one, re-add an active peer with stored
observe_others=Trueand incomingobserve_others=False, then add a new observer. The check permits one observer, but this upsert preserves the first observer and inserts the second observer.Keep active peers in the persisted observer count. Count incoming configuration only for new or departed peers. Add this case to the observer-limit tests.
🤖 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/crud/session.py` around lines 1074 - 1076, Update the observer-limit query and the SessionPeer upsert around the configuration case to calculate counts from each peer’s effective configuration. Preserve stored configuration for active peers when counting, while using incoming configuration only for new or departed peers; ensure the limit check matches the persisted result. Add a regression test covering limit one, re-adding an active observer with observe_others disabled, then adding another observer.
🤖 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.
Outside diff comments:
In `@src/crud/session.py`:
- Around line 1074-1076: Update the observer-limit query and the SessionPeer
upsert around the configuration case to calculate counts from each peer’s
effective configuration. Preserve stored configuration for active peers when
counting, while using incoming configuration only for new or departed peers;
ensure the limit check matches the persisted result. Add a regression test
covering limit one, re-adding an active observer with observe_others disabled,
then adding another observer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ad643b0-865e-4bf4-870c-b684e1775a51
📒 Files selected for processing (3)
src/crud/session.pytests/crud/test_session.pytests/test_search.py
|
Static Analysis passes on #986. The FastAPI test workflow still fails at the same unrelated test as #981:
The run completed with 1501 passed, 25 skipped, 1 error. The PostgreSQL service log also shows the allowlist test racing with database initialization ( @eisene Could you please help rerun or assess this workflow failure? The implementation-specific tests pass locally (39 passed without an API key), and Static Analysis is green. |
|
Hi @akattelu — I noticed you reviewed and merged #983 after the CI issue was resolved there. Could you please take a look at #986 as well? Static Analysis passes. The FastAPI workflow reaches 1501 passed / 25 skipped, then errors in the unrelated The changed tests and local keyless validation are clean (39 passed). Could you advise whether this CI failure should simply be rerun or whether the allowlist test/database fixture needs a separate fix? Thanks. |
|
Follow-up to #987: I verified the additional I addressed this in
Validation: 40 passed across session/search/document tests; ruff clean. This keeps the fix in one PR and covers both session initialization and PUT peers entry points. |
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 `@tests/crud/test_session.py`:
- Around line 125-128: In the test assertion around session_peer_stmt, remove
the unused first_joined_at binding by renaming it to _first_joined_at or
omitting it while preserving the existing first_left_at and first_config
assertions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d326d45f-7e93-449a-9be1-ad5a6c954072
📒 Files selected for processing (2)
src/crud/session.pytests/crud/test_session.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/crud/session.py
|
Superseded by #1059, which landed both the joined_at CASE guard and the set_peers soft-delete exclusion. Thanks for the fix. |
2 participants
Fixes #940.
Problem
When
get_or_create_sessionre-adds a peer whose membership is still active (left_at IS NULL), the session-peer upsert currently resetsjoined_attonow().peer_perspectivesearch filters messages usingmessage.created_at >= joined_at, so clients that re-add peers on startup can lose visibility of messages created before the latest restart, without an error.Change
Preserve
joined_atfor active peers. Only reset it when a peer has actually left and rejoins (left_at IS NOT NULL). Existing observer-count logic, configuration handling, andleft_atclearing are unchanged.Tests
joined_atand the stored configuration.joined_atand incoming configuration.peer_perspectivesearch retains messages after an active re-add and starts a new window after a genuine rejoin.Local validation against current
main(including #955):pytest tests/test_search.py tests/crud/test_session.py tests/crud/test_document.py: 39 passedruff check: cleanThis PR supersedes #981, which encountered an unrelated full-suite CI cancellation in
tests/test_session_allowlist.pyafter 1501 tests had passed. The new PR starts from the currentmainand uses a single clean commit.Summary by CodeRabbit
Bug Fixes
Tests