Wire guarded PostgreSQL application runtime - #23
Merged
Conversation
|
Warning Review limit reached
Next review available in: 46 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
Summary by CodeRabbit
WalkthroughThe change adds a guarded PostgreSQL multi-instance preview. It introduces validated configuration, generation-fenced event coordination, a PostgreSQL application runtime, distributed WebSocket state, backend-specific CLI behavior, integration tests, and deployment documentation. ChangesPostgreSQL preview configuration
Generation-fenced coordination
Runtime integration
Release documentation and validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant create_app
participant PostgresApplicationRuntime
participant PostgresRealtimeRelay
participant PostgreSQL
Client->>create_app: Open HTTP or WebSocket connection
create_app->>PostgresApplicationRuntime: Acquire lease and initialize state
PostgresApplicationRuntime->>PostgreSQL: Read and write distributed state
PostgresRealtimeRelay->>PostgreSQL: Poll generation-scoped events
PostgresRealtimeRelay->>Client: Broadcast relayed events excluding origin
Possibly related PRs
🚥 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 |
Deathcharge
marked this pull request as ready for review
August 11, 2026 05:13
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
samsarix_chat_engine/app.py (1)
1177-1216: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease the PostgreSQL connection lease when post-admission steps fail.
Line 1179 acquires a distributed lease. Lines 1204-1205 then call
store.get_roomandstore.get_member_moderation. If either raisesPostgresFoundationError, the handler exits without reaching thefinallyblock at line 1496, because thetrystarts at line 1335. The lease stays untilpostgres_lease_secondsexpires, which blocks a capacity slot and delays thepresence.leftevent to the reaper.Move the lease release into a
try/exceptthat covers every step between admission and the main receive loop.🛡️ Proposed structure
- room = await store.get_room(room_id) - moderation = await store.get_member_moderation(room_id, principal.subject) if principal.subject else None + try: + room = await store.get_room(room_id) + moderation = await store.get_member_moderation(room_id, principal.subject) if principal.subject else None + except Exception: + await manager.unregister(websocket) + if postgres_runtime is not None: + await postgres_runtime.release_connection(connection_id) + raise🤖 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 `@samsarix_chat_engine/app.py` around lines 1177 - 1216, Wrap all post-admission operations in the WebSocket handler—including manager registration, room and moderation lookups, rejection handling, and setup before the main receive loop—in a try/except or cleanup scope that releases the PostgreSQL lease via postgres_runtime.release_connection(connection_id) whenever any step fails. Ensure the existing main-loop cleanup remains correct without double-releasing, and preserve normal admission and rejection 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 `@CHANGELOG.md`:
- Around line 20-21: Update the /v1/stats API documentation to describe the
active WebSocket connection count as deployment-wide when PostgreSQL is enabled,
replacing the inaccurate “current process's” wording. Preserve the existing
PostgreSQL test coverage for the shared count across instances.
In `@docs/OPERATIONS.md`:
- Around line 83-84: Update the PostgreSQL preview runbook link in the SQLite
deployment guidance to use the target fragment `#migration-backup-and-rollback`
instead of `#backup-restore-and-rollback`, leaving the surrounding documentation
unchanged.
In `@ROADMAP.md`:
- Around line 93-95: Update the v0.13 checklist in ROADMAP.md to record
production gates for unique stable instance IDs, live-lag and NOTIFY validation,
and PostgreSQL backup/restore verification. Add these as explicit checklist
entries or link the authoritative release checklist while preserving the
existing PostgreSQL and application-integration items.
In `@samsarix_chat_engine/app.py`:
- Line 401: Replace the Any annotation on postgres_runtime with
PostgresApplicationRuntime | None, importing PostgresApplicationRuntime under
TYPE_CHECKING alongside the existing imports. Preserve the lazy runtime import
so psycopg remains optional, and apply the real type to the variable used by
acquire_connection, renew_connection, set_typing, and connection_counts.
- Around line 1264-1288: The WebSocket lease-loss path can concurrently send or
close while the receive loop is operating on the same connection. Add one
per-connection asyncio.Lock and use it to serialize every manager.send and
websocket.close for this WebSocket, including the operations in
maintain_connection_lease and the main receive loop; ensure lease-loss handling
acquires the same lock before sending the error and closing.
In `@samsarix_chat_engine/config.py`:
- Around line 269-271: Extend Settings.__post_init__ SQLite-mode validation
beyond postgres_url and postgres_instance_id to reject any explicitly configured
PostgreSQL operational settings loaded by the PostgreSQL configuration block
around the pool, lease, relay, maintenance, rate-bucket, and event-retention
options. Treat non-default tuning values as invalid in SQLite mode, preserve
valid SQLite configuration behavior, and add a test covering PostgreSQL settings
with SAMSARIX_CHAT_STORAGE set to sqlite.
In `@samsarix_chat_engine/postgres_runtime.py`:
- Line 47: Change the maintenance flow around prune_events so event pruning does
not run on every one-second maintenance pass; use a separate longer pruning
interval or ensure only one instance performs pruning per interval while
preserving regular maintenance execution and the
POSTGRES_EVENT_RETENTION_LOCK_ID protection.
- Around line 182-205: Update run_maintenance_once to execute each of its six
cleanup operations independently, isolating failures so one exception does not
prevent later steps from running. Add per-step warning logs that identify the
failed maintenance operation and preserve the existing bounded cleanup
arguments; keep _run_maintenance as the outer scheduling loop.
In `@samsarix_chat_engine/postgres.py`:
- Around line 378-384: Mark the public instance APIs represented by
claim_instance and the other two unfenced methods as deprecated, using the
project’s established deprecation mechanism and guidance toward fenced
alternatives. Update all test callers to use the fenced APIs, while retaining
the deprecated methods temporarily for compatibility and documenting their
planned removal.
- Around line 461-473: Read and store the UPDATE result’s rowcount inside the
self.transaction() block before the connection is returned to the pool, then
return the stored value from this method. Follow the existing pattern used by
heartbeat_claimed_instance and preserve the rowcount == 1 result.
In `@samsarix_chat_engine/websocket_manager.py`:
- Line 25: Replace the four-field metadata tuple used by WebSocketManager with a
NamedTuple defining descriptive field names, and update the _metadata annotation
and positional accesses in the relevant methods to use those named fields while
preserving tuple compatibility.
In `@tests/test_postgres_app.py`:
- Around line 33-41: Update
test_two_app_instances_share_http_websocket_presence_typing_and_messages to be a
synchronous test: remove the `@pytest.mark.asyncio` decorator and change the async
def declaration to def. Keep the existing synchronous TestClient calls and test
behavior unchanged.
---
Outside diff comments:
In `@samsarix_chat_engine/app.py`:
- Around line 1177-1216: Wrap all post-admission operations in the WebSocket
handler—including manager registration, room and moderation lookups, rejection
handling, and setup before the main receive loop—in a try/except or cleanup
scope that releases the PostgreSQL lease via
postgres_runtime.release_connection(connection_id) whenever any step fails.
Ensure the existing main-loop cleanup remains correct without double-releasing,
and preserve normal admission and rejection 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d35ad929-5200-45b5-9687-7dc164b86791
📒 Files selected for processing (24)
.env.example.github/workflows/ci.ymlCHANGELOG.mdREADME.mdROADMAP.mddocs/API_REFERENCE.mddocs/MULTI_INSTANCE_ARCHITECTURE.mddocs/OPERATIONS.mddocs/POSTGRES_PREVIEW.mdpyproject.tomlsamsarix_chat_engine/app.pysamsarix_chat_engine/cli.pysamsarix_chat_engine/config.pysamsarix_chat_engine/postgres.pysamsarix_chat_engine/postgres_connections.pysamsarix_chat_engine/postgres_realtime.pysamsarix_chat_engine/postgres_runtime.pysamsarix_chat_engine/postgres_store.pysamsarix_chat_engine/websocket_manager.pytests/test_config_cli.pytests/test_connection_manager.pytests/test_postgres_app.pytests/test_postgres_foundation.pytests/test_postgres_realtime.py
Comment on lines
+378
to
+384
| async def claim_instance( | ||
| self, | ||
| instance_id: str, | ||
| *, | ||
| lease_seconds: int, | ||
| generation: UUID | None = None, | ||
| ) -> InstanceRegistration: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find callers of the unfenced instance-lease APIs.
set -euo pipefail
rg -nP --type=py -C3 '\.(register_instance|heartbeat_instance|recover_instance_after_gap)\s*\('Repository: Deathcharge/samsarix-chat-engine
Length of output: 170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all tracked references ---'
rg -n -C2 'register_instance|heartbeat_instance|recover_instance_after_gap|claim_instance|heartbeat_claimed_instance|recover_claimed_instance_after_gap' \
"$(git ls-files)" || true
printf '%s\n' '--- postgres.py structure ---'
ast-grep outline samsarix_chat_engine/postgres.pyRepository: Deathcharge/samsarix-chat-engine
Length of output: 7397
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- tracked references ---'
git ls-files -z | xargs -0 rg -n -C2 \
'register_instance|heartbeat_instance|recover_instance_after_gap|claim_instance|heartbeat_claimed_instance|recover_claimed_instance_after_gap' || true
printf '%s\n' '--- method definitions and nearby implementation ---'
rg -n '^ async def (register_instance|claim_instance|heartbeat_instance|heartbeat_claimed_instance|recover_instance_after_gap|recover_claimed_instance_after_gap)\b' samsarix_chat_engine/postgres.py
sed -n '300,560p' samsarix_chat_engine/postgres.pyRepository: Deathcharge/samsarix-chat-engine
Length of output: 33408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- instance API documentation and deprecation markers ---'
git ls-files -z | xargs -0 rg -n -i -C3 \
'register_instance|heartbeat_instance|recover_instance_after_gap|claim_instance|generation|deprecated|legacy|unfenced' \
--glob '*.md' --glob '*.py' || true
printf '%s\n' '--- legacy test call context ---'
sed -n '45,180p' tests/test_postgres_foundation.py
sed -n '1,125p' tests/test_postgres_connections.py
sed -n '1,125p' tests/test_postgres_presence.py
printf '%s\n' '--- production instance API call sites excluding tests ---'
git ls-files -z -- '*.py' ':!:tests/*' ':!:examples/*' | xargs -0 rg -n -C2 \
'register_instance|heartbeat_instance|recover_instance_after_gap|claim_instance|heartbeat_claimed_instance|recover_claimed_instance_after_gap' || trueRepository: Deathcharge/samsarix-chat-engine
Length of output: 50391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import pathlib
import re
names = (
"register_instance",
"heartbeat_instance",
"recover_instance_after_gap",
"claim_instance",
"heartbeat_claimed_instance",
"recover_claimed_instance_after_gap",
)
pattern = re.compile(r"\b(" + "|".join(names) + r")\b")
counts = {name: {"tests": 0, "production": 0, "docs": 0} for name in names}
for path_text in __import__("subprocess").check_output(
["git", "ls-files", "-z"], text=False
).decode().split("\0"):
if not path_text:
continue
path = pathlib.Path(path_text)
if path.suffix not in {".py", ".md"}:
continue
text = path.read_text(encoding="utf-8")
bucket = "tests" if path.parts and path.parts[0] == "tests" else (
"docs" if path.suffix == ".md" else "production"
)
for match in pattern.finditer(text):
counts[match.group(1)][bucket] += 1
for name in names:
print(name, counts[name])
print("--- non-test Python call/reference lines ---")
for path_text in __import__("subprocess").check_output(
["git", "ls-files", "-z", "--", "*.py"], text=False
).decode().split("\0"):
if not path_text or path_text.startswith("tests/"):
continue
path = pathlib.Path(path_text)
for number, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1):
if pattern.search(line):
print(f"{path}:{number}:{line.strip()}")
PYRepository: Deathcharge/samsarix-chat-engine
Length of output: 1509
Deprecate the unfenced instance APIs
No production caller uses the unfenced APIs. The test suite still contains calls to all three methods. Mark these public methods deprecated, migrate the tests, and remove the methods after the compatibility period.
🤖 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 `@samsarix_chat_engine/postgres.py` around lines 378 - 384, Mark the public
instance APIs represented by claim_instance and the other two unfenced methods
as deprecated, using the project’s established deprecation mechanism and
guidance toward fenced alternatives. Update all test callers to use the fenced
APIs, while retaining the deprecated methods temporarily for compatibility and
documenting their planned removal.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This guarded v0.13 increment connects the proven PostgreSQL store and coordination primitives to the real FastAPI HTTP/WebSocket runtime while retaining SQLite as the default and supported v0.12 deployment.
Included
sslmode=verify-fullenforcementVerification
GitHub Actions run 31461687953 is green:
Local verification also passed Ruff, mypy, focused startup/WebSocket tests, and 144 non-PostgreSQL tests on Python 3.14.
Deployment boundary
PostgreSQL mode remains an unreleased v0.13 preview. Each Uvicorn process requires a unique stable instance ID, and the current Compose profile remains SQLite-only. Subprocess/network-interruption, measured load/soak, live-lag/NOTIFY, and exercised PostgreSQL backup/restore gates remain before a production multi-instance support claim.
Migration and rollback
Opening PostgreSQL initializes or upgrades the schema to version 8. Operators must take and verify a PostgreSQL-native backup before upgrade. Rollback is application rollback when the older release understands the schema; otherwise restore the matching pre-upgrade backup. The bundled SQLite backup/restore command intentionally does not operate on PostgreSQL.