Repository navigation
fix(ui): wire phase2 owner leases - #84
Conversation
PG-backed HTTP sessions must not exist without an owner lease, or failover can leave orphaned sessions that no instance may safely mutate. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
PG-backed HTTP deployments need one owner wiring path for startup, renewal, and shutdown so leases remain fenced for the server lifetime. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConditionally enables and manages session-owner leases: SessionManager gains owner-lease configuration, acquisition, renewal, and release; the FastAPI HTTP server wires a PG-backed SessionOwnerStore, starts a renew background task, and releases owned sessions on shutdown; tests added/updated for owner-lease behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as FastAPI HTTP Server
participant SM as SessionManager
participant PG as PG SessionOwnerStore / DB
rect rgba(200,220,255,0.5)
Client->>Server: HTTP request -> create session
Server->>SM: request new session
SM->>PG: persist session record
SM->>PG: acquire owner lease (owner_id, fencing_token, lease_seconds)
PG-->>SM: lease acquired
SM-->>Server: session created
Server-->>Client: response with session
end
rect rgba(220,255,200,0.5)
Server->>SM: start background renew task
loop periodic
SM->>PG: renew owner leases for owned sessions
PG-->>SM: renew results
end
end
rect rgba(255,220,200,0.5)
Server->>SM: shutdown
SM->>SM: cancel/await renew task
SM->>PG: release owned sessions
PG-->>SM: release confirmed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 (2)
coding-agent/src/coding_agent/ui/session_manager.py (2)
591-601: Consider whether failed releases should be logged.The method iterates all sessions from
list_sessions_async()and callsrelease()for each. If a session is owned by a different owner,release()returnsFalse(per theSessionOwnerStoreimplementation), which is silently ignored. This is correct behavior for graceful shutdown, but consider logging failures for observability during shutdown diagnostics.🔧 Optional: Log failed releases
async def release_owned_sessions(self) -> None: if self._owner_store is None: return if self._owner_id is None or self._fencing_token is None: raise SessionOwnershipConflictError("stale owner or fencing token rejected") for session_id in await self.list_sessions_async(): - await self._owner_store.release( + released = await self._owner_store.release( session_id, self._owner_id, self._fencing_token, ) + if not released: + logger.debug("Skipped release for session %s (not owned)", session_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/src/coding_agent/ui/session_manager.py` around lines 591 - 601, The loop in release_owned_sessions silently ignores failed releases (SessionOwnerStore.release returning False) which hurts shutdown observability; update release_owned_sessions to check the boolean return per session_id from self._owner_store.release and log a warning/error including session_id, self._owner_id and self._fencing_token when release returns False, and also catch and log any exceptions raised by release; use the existing logger on the class (e.g., self._logger) or add one if missing so failures during list_sessions_async()/release() are recorded for diagnostics.
410-412: Propertypg_poolmay raiseRuntimeErrorif called without proper configuration.Accessing
pg_poolon aSessionManagerthat wasn't configured with a PG backend will raise aRuntimeErrorfrom_get_pg_pool()(line 445: "PG storage requires storage_config['dsn']"). This is used byhttp_server.pyonly when_storage_uses_pg_http_sessions()returnsTrue, so it's safe in practice. Consider documenting this behavior or making the property returnPGPool | None.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/src/coding_agent/ui/session_manager.py` around lines 410 - 412, The pg_pool property on SessionManager currently calls _get_pg_pool() which raises RuntimeError if the manager wasn't configured with PG; update the API to be explicit: change the pg_pool property signature to return PGPool | None and modify its implementation to call a safe accessor (e.g., a new _maybe_get_pg_pool or adjust _get_pg_pool) that returns None instead of raising when storage_config lacks a DSN; update the SessionManager docstring to mention that pg_pool may be None and add a brief guard note referencing _storage_uses_pg_http_sessions and http_server.py callers so callers check for None before use.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coding-agent/src/coding_agent/ui/http_server.py`:
- Around line 136-139: The background loop in _renew_owner_leases currently
calls session_manager.renew_owner_leases() without protection, so an exception
will kill the task; wrap the call in a try/except Exception block inside
_renew_owner_leases, log the error (using the module logger) when
session_manager.renew_owner_leases() raises, and ensure the loop continues
(optionally perform a short backoff via asyncio.sleep on error) so lease
renewals keep running; keep the existing await
asyncio.sleep(max(session_manager.owner_lease_seconds / 2.0, 1.0)) rhythm and
reference _renew_owner_leases and session_manager.renew_owner_leases when making
the change.
---
Nitpick comments:
In `@coding-agent/src/coding_agent/ui/session_manager.py`:
- Around line 591-601: The loop in release_owned_sessions silently ignores
failed releases (SessionOwnerStore.release returning False) which hurts shutdown
observability; update release_owned_sessions to check the boolean return per
session_id from self._owner_store.release and log a warning/error including
session_id, self._owner_id and self._fencing_token when release returns False,
and also catch and log any exceptions raised by release; use the existing logger
on the class (e.g., self._logger) or add one if missing so failures during
list_sessions_async()/release() are recorded for diagnostics.
- Around line 410-412: The pg_pool property on SessionManager currently calls
_get_pg_pool() which raises RuntimeError if the manager wasn't configured with
PG; update the API to be explicit: change the pg_pool property signature to
return PGPool | None and modify its implementation to call a safe accessor
(e.g., a new _maybe_get_pg_pool or adjust _get_pg_pool) that returns None
instead of raising when storage_config lacks a DSN; update the SessionManager
docstring to mention that pg_pool may be None and add a brief guard note
referencing _storage_uses_pg_http_sessions and http_server.py callers so callers
check for None before use.
🪄 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: acb54b22-f520-4338-bea4-4f367c628022
📒 Files selected for processing (4)
coding-agent/src/coding_agent/ui/http_server.pycoding-agent/src/coding_agent/ui/session_manager.pycoding-agent/tests/ui/test_http_server.pycoding-agent/tests/ui/test_session_manager_owner_checks.py
There was a problem hiding this comment.
Pull request overview
Wires Phase 2 “owner lease” semantics into the UI session lifecycle so PG-backed HTTP sessions acquire an owner record on creation, keep leases renewed while the server is running, and release leases on shutdown.
Changes:
- Add
owner_lease_secondsconfiguration and owner lease acquire/renew/release helpers toSessionManager, including rollback behavior on owner-acquire failure. - Update HTTP server startup/shutdown to configure a
SessionOwnerStore, run a background renew loop, and release owned sessions before closing. - Extend UI tests with regressions covering owner acquisition, rollback, release, HTTP wiring, and renew loop behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
coding-agent/src/coding_agent/ui/session_manager.py |
Adds owner lease configuration + acquire/renew/release logic and create-session rollback behavior. |
coding-agent/src/coding_agent/ui/http_server.py |
Configures owner store for PG sessions, runs a lease renew task during lifespan, and releases leases on shutdown. |
coding-agent/tests/ui/test_http_server.py |
Adds tests for _build_session_manager() owner-store wiring and _renew_owner_leases() behavior. |
coding-agent/tests/ui/test_session_manager_owner_checks.py |
Adds/adjusts tests for owner acquisition on create, rollback on acquire failure, and releasing owned sessions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await asyncio.sleep(max(session_manager.owner_lease_seconds / 2.0, 1.0)) | ||
| await session_manager.renew_owner_leases() |
There was a problem hiding this comment.
The renew loop sleeps before the first renewal. On startup this delays the first lease refresh by up to owner_lease_seconds/2, which can be risky if the process restarts and inherits sessions with near-expiry leases. Consider renewing immediately once at loop start, then sleeping between renewals.
| await asyncio.sleep(max(session_manager.owner_lease_seconds / 2.0, 1.0)) | |
| await session_manager.renew_owner_leases() | |
| await session_manager.renew_owner_leases() | |
| await asyncio.sleep(max(session_manager.owner_lease_seconds / 2.0, 1.0)) |
| await self._acquire_owner_for_session(session_id) | ||
| except BaseException: | ||
| self._session_cache.pop(session_id, None) | ||
| await self._run_store_io(self._store.delete, session_id) |
There was a problem hiding this comment.
create_session() rollback runs store.delete() inside the exception handler, which can mask the original failure if the delete also errors (e.g., storage outage). Consider preserving the original exception and making cleanup best-effort (log cleanup failures), so callers see the real root cause.
| await self._run_store_io(self._store.delete, session_id) | |
| try: | |
| await self._run_store_io(self._store.delete, session_id) | |
| except BaseException: | |
| logger.exception( | |
| "Failed to delete partially created session during rollback: %s", | |
| session_id, | |
| ) |
| async def _renew_owner_leases() -> None: | ||
| while True: | ||
| await asyncio.sleep(max(session_manager.owner_lease_seconds / 2.0, 1.0)) | ||
| await session_manager.renew_owner_leases() |
There was a problem hiding this comment.
_renew_owner_leases() has no exception handling. Any unexpected exception from session_manager.renew_owner_leases() (e.g., transient PG failure) will terminate the background task permanently, so leases will stop renewing until process restart. Consider catching Exception inside the loop, logging, and continuing (optionally with a short backoff).
| await session_manager.renew_owner_leases() | |
| try: | |
| await session_manager.renew_owner_leases() | |
| except asyncio.CancelledError: | |
| raise | |
| except Exception: | |
| logger.warning( | |
| "Failed to renew owner leases; will retry", | |
| exc_info=True, | |
| ) | |
| await asyncio.sleep(1.0) |
| try: | ||
| await owner_renew_task | ||
| except asyncio.CancelledError: | ||
| pass |
There was a problem hiding this comment.
In lifespan(), await owner_renew_task only suppresses CancelledError. If the renew task ever exits with another exception, shutdown will raise and potentially prevent session cleanup/release/close. Consider also catching/logging generic Exception when awaiting the task (or using contextlib.suppress(Exception) with explicit logging).
| pass | |
| pass | |
| except Exception: | |
| logger.warning( | |
| "Owner lease renewal task failed during shutdown", | |
| exc_info=True, | |
| ) |
Transient owner-store failures should not terminate the HTTP server renewal task and let active leases expire silently. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
coding-agent/tests/ui/test_http_server.py (1)
264-266: Prefer public pool accessor in assertion to reduce brittleness.The test currently couples to
SessionManagerinternals viamanager._pg_pool. Usingmanager.pg_poolkeeps coverage intent while being less fragile to internal refactors.Proposed minimal assertion tweak
- assert manager._owner_store.pg_pool is manager._pg_pool + assert manager._owner_store.pg_pool is manager.pg_pool🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/tests/ui/test_http_server.py` around lines 264 - 266, The test currently asserts against the private attribute manager._pg_pool which couples tests to SessionManager internals; change the assertion to use the public accessor manager.pg_pool instead (keep the other assertions for manager._owner_store being a FakeOwnerStore and manager._owner_id == "pod-a") so the test reads the pool via manager.pg_pool while still verifying manager._owner_store and manager._owner_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@coding-agent/tests/ui/test_http_server.py`:
- Around line 264-266: The test currently asserts against the private attribute
manager._pg_pool which couples tests to SessionManager internals; change the
assertion to use the public accessor manager.pg_pool instead (keep the other
assertions for manager._owner_store being a FakeOwnerStore and manager._owner_id
== "pod-a") so the test reads the pool via manager.pg_pool while still verifying
manager._owner_store and manager._owner_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fe4adfc6-3043-41c7-bae7-d84e4897d1c4
📒 Files selected for processing (2)
coding-agent/src/coding_agent/ui/http_server.pycoding-agent/tests/ui/test_http_server.py
✅ Files skipped from review due to trivial changes (1)
- coding-agent/src/coding_agent/ui/http_server.py
Rollback cleanup is best-effort so a secondary store delete failure cannot hide the owner-acquire failure that caused session creation to abort. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
HTTP startup should refresh inherited leases before the first sleep and still log failed renewal tasks during shutdown cleanup. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
coding-agent/src/coding_agent/ui/session_manager.py (1)
591-601: Consider logging whenrelease()returnsFalse.The method silently ignores failed releases. While this is best-effort behavior, logging failed releases (similar to
renew_owner_leases) would improve observability for debugging ownership issues.💡 Optional: add logging for failed releases
async def release_owned_sessions(self) -> None: if self._owner_store is None: return if self._owner_id is None or self._fencing_token is None: raise SessionOwnershipConflictError("stale owner or fencing token rejected") for session_id in await self.list_sessions_async(): - await self._owner_store.release( + released = await self._owner_store.release( session_id, self._owner_id, self._fencing_token, ) + if not released: + logger.debug("Skipped releasing session %s (not owned by this instance)", session_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/src/coding_agent/ui/session_manager.py` around lines 591 - 601, The loop in release_owned_sessions currently ignores a False return from self._owner_store.release; update release_owned_sessions to check the boolean result for each session_id returned by await self.list_sessions_async() and, when release(...) returns False, emit a warning log (use the same logger used by renew_owner_leases, e.g. self._logger) including the session_id and self._owner_id to aid debugging of ownership release failures; do not change behavior on False beyond logging (keep it best-effort).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coding-agent/tests/ui/test_session_manager_owner_checks.py`:
- Around line 389-395: The test currently asserts lease_expires_at against
itself which is meaningless; modify the assertion in the block that calls
owner_store.get_owner and compares to SessionOwnerRecord so that
lease_expires_at is checked against an expected time window: capture the current
time (now) immediately before creating/claiming the owner, compute
expected_expiry = now + owner_lease_seconds (or the literal 45s if that constant
isn’t accessible in the test), and assert the returned owner.lease_expires_at is
within a small tolerance (e.g., ±2–5 seconds) of expected_expiry; keep the other
fields (owner_id, fencing_token) asserted exactly as before. Ensure you
reference the owner_store.get_owner call and the SessionOwnerRecord comparison
when updating the test.
---
Nitpick comments:
In `@coding-agent/src/coding_agent/ui/session_manager.py`:
- Around line 591-601: The loop in release_owned_sessions currently ignores a
False return from self._owner_store.release; update release_owned_sessions to
check the boolean result for each session_id returned by await
self.list_sessions_async() and, when release(...) returns False, emit a warning
log (use the same logger used by renew_owner_leases, e.g. self._logger)
including the session_id and self._owner_id to aid debugging of ownership
release failures; do not change behavior on False beyond logging (keep it
best-effort).
🪄 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: 4596df0d-3215-453d-aeff-7d901f0d63a8
📒 Files selected for processing (4)
coding-agent/src/coding_agent/ui/http_server.pycoding-agent/src/coding_agent/ui/session_manager.pycoding-agent/tests/ui/test_http_server.pycoding-agent/tests/ui/test_session_manager_owner_checks.py
✅ Files skipped from review due to trivial changes (1)
- coding-agent/src/coding_agent/ui/http_server.py
🚧 Files skipped from review as they are similar to previous changes (1)
- coding-agent/tests/ui/test_http_server.py
Shutdown lease release remains best-effort, but failed owner-store releases now leave diagnostic evidence with the session and owner context. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coding-agent/src/coding_agent/ui/session_manager.py`:
- Around line 597-607: The loop currently aborts on exceptions from
_owner_store.release and _owner_store.renew; change both call sites (the release
block using session_id, self._owner_id, self._fencing_token and the renew block
referenced at lines 615-623) to wrap the await calls in try/except so exceptions
are caught, logged with context (session_id and owner id/fencing token), and the
loop continues; do not re-raise—treat failures as best-effort by logging a
warning/error and moving on to the next session.
- Around line 596-607: The loops currently call
_owner_store.release/_owner_store.renew for every session from
list_sessions_async(), causing spurious warnings and extra traffic; instead
first check the session's current owner/lease and only act when this replica
actually holds it: for each session_id from list_sessions_async(), call a read
method on the store (e.g., _owner_store.get_owner(session_id) or
_owner_store.get_lease(session_id)) and compare the returned owner id and
fencing token to self._owner_id and self._fencing_token, and only then call
_owner_store.release(...) or _owner_store.renew(...); apply the same change to
the other loop (renew loop) so you only touch leases this owner actually holds.
- Line 20: Remove the top-level import of PGPool and make the PG-only import
lazy: delete "from agentkit.storage.pg import PGPool" at module top and instead
import PGPool inside SessionManager._load_pg_storage_types (or any code path
that constructs PG-backed storage) so that PGPool is only imported when a
PostgreSQL backend is requested; update references in SessionManager that expect
PGPool to use the locally imported symbol inside that method (or pass it along
from that method) to avoid making agentkit.storage.pg a hard dependency at
module import time.
🪄 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: 30285a47-8dc3-4dc3-ad0d-992702117f87
📒 Files selected for processing (2)
coding-agent/src/coding_agent/ui/session_manager.pycoding-agent/tests/ui/test_session_manager_owner_checks.py
🚧 Files skipped from review as they are similar to previous changes (1)
- coding-agent/tests/ui/test_session_manager_owner_checks.py
| from typing import Any, Literal, TypeVar, cast | ||
|
|
||
| from agentkit.storage.checkpoint_fs import FSCheckpointStore | ||
| from agentkit.storage.pg import PGPool |
There was a problem hiding this comment.
Keep agentkit.storage.pg lazily imported.
SessionManager already has _load_pg_storage_types() to defer PG-only imports until a PG backend is actually requested. Pulling PGPool in at module import time makes that optional path a hard dependency for every backend and can break non-PG startup before the friendly runtime error is reachable.
Suggested fix
+from typing import TYPE_CHECKING
+
-from agentkit.storage.pg import PGPool
+if TYPE_CHECKING:
+ from agentkit.storage.pg import PGPool `@property`
def pg_pool(self) -> PGPool:
return self._get_pg_pool()
def _get_pg_pool(self) -> PGPool:
if self._pg_pool is not None:
- return cast(PGPool, self._pg_pool)
+ return cast("PGPool", self._pg_pool)
...
- return cast(PGPool, self._pg_pool)
+ return cast("PGPool", self._pg_pool)Use this to confirm whether agentkit.storage.pg still depends on optional PG imports at module load:
#!/bin/bash
set -euo pipefail
echo "== Top of coding-agent/src/agentkit/storage/pg.py =="
sed -n '1,120p' coding-agent/src/agentkit/storage/pg.py
pyproject="$(fd -a '^pyproject\.toml$' | head -n1 || true)"
if [ -n "${pyproject:-}" ]; then
echo
echo "== Dependency declarations mentioning PostgreSQL extras =="
rg -n -C2 'optional-dependencies|extras_require|asyncpg|postgres' "$pyproject"
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coding-agent/src/coding_agent/ui/session_manager.py` at line 20, Remove the
top-level import of PGPool and make the PG-only import lazy: delete "from
agentkit.storage.pg import PGPool" at module top and instead import PGPool
inside SessionManager._load_pg_storage_types (or any code path that constructs
PG-backed storage) so that PGPool is only imported when a PostgreSQL backend is
requested; update references in SessionManager that expect PGPool to use the
locally imported symbol inside that method (or pass it along from that method)
to avoid making agentkit.storage.pg a hard dependency at module import time.
Renew and release loops now skip sessions owned by other replicas and treat per-session store failures as best-effort so one bad lease does not poison the whole pass. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
The HTTP renewal regression now supplies owned session records so it exercises the scoped lease-renew path instead of an impossible ownerless session set. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
coding-agent/src/coding_agent/ui/session_manager.py (1)
20-20:⚠️ Potential issue | 🟠 MajorKeep
agentkit.storage.pglazily imported.
_load_pg_storage_types()already defers PG-only imports until a PG backend is requested, but this new top-levelPGPoolimport makes every non-PG startup importagentkit.storage.pganyway. That reintroduces the optional-dependency failure path before the friendly runtime error can fire.Suggested fix
+from typing import TYPE_CHECKING + from agentkit.storage.checkpoint_fs import FSCheckpointStore -from agentkit.storage.pg import PGPool from agentkit.checkpoint.models import CheckpointMeta ... +if TYPE_CHECKING: + from agentkit.storage.pg import PGPool + ... - def pg_pool(self) -> PGPool: + def pg_pool(self) -> "PGPool": return self._get_pg_pool() - def _get_pg_pool(self) -> PGPool: + def _get_pg_pool(self) -> "PGPool": if self._pg_pool is not None: - return cast(PGPool, self._pg_pool) + return cast("PGPool", self._pg_pool) ... - return cast(PGPool, self._pg_pool) + return cast("PGPool", self._pg_pool)#!/bin/bash set -euo pipefail pg_file="$(fd -a '^pg\.py$' coding-agent/src/agentkit/storage | head -n1)" sm_file="$(fd -a '^session_manager\.py$' coding-agent/src/coding_agent/ui | head -n1)" echo "== Top-level imports in ${pg_file} ==" sed -n '1,120p' "$pg_file" echo echo "== PGPool references in ${sm_file} ==" rg -n -C2 '\bPGPool\b|_load_pg_storage_types' "$sm_file" pyproject="$(fd -a '^pyproject\.toml$' | head -n1 || true)" if [ -n "${pyproject:-}" ]; then echo echo "== PostgreSQL dependency declarations ==" rg -n -C2 'optional-dependencies|asyncpg|postgres' "$pyproject" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/src/coding_agent/ui/session_manager.py` at line 20, The top-level import of PGPool in session_manager.py breaks lazy-loading; remove "from agentkit.storage.pg import PGPool" and instead import PGPool only where needed (e.g., inside _load_pg_storage_types() or the function that selects/configures a PG backend) so agentkit.storage.pg is only imported when a PG backend is requested; update any references to PGPool to use the locally imported name and keep _load_pg_storage_types() as the single place that brings in PG-only symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coding-agent/src/coding_agent/ui/session_manager.py`:
- Around line 604-609: The release_owned_sessions loop only iterates existing
session IDs, so sessions removed by
close_session()/_remove_session_async_no_lock() leave behind session_owners
rows; fix by ensuring owner entries are released when a session is deleted:
update close_session() and _remove_session_async_no_lock() to call the
owner-store release logic (or call a new helper that removes/releases the owner
row using _owner_store/_owner_id/_fencing_token) before deleting session
metadata, and/or add a cleanup path that scans session_owners for entries whose
session metadata no longer exists and releases them; reference
release_owned_sessions, close_session, _remove_session_async_no_lock, and the
owner-store interaction when implementing the change.
---
Duplicate comments:
In `@coding-agent/src/coding_agent/ui/session_manager.py`:
- Line 20: The top-level import of PGPool in session_manager.py breaks
lazy-loading; remove "from agentkit.storage.pg import PGPool" and instead import
PGPool only where needed (e.g., inside _load_pg_storage_types() or the function
that selects/configures a PG backend) so agentkit.storage.pg is only imported
when a PG backend is requested; update any references to PGPool to use the
locally imported name and keep _load_pg_storage_types() as the single place that
brings in PG-only symbols.
🪄 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: 8fb1692a-e284-4f74-9af7-248a5652ba0d
📒 Files selected for processing (3)
coding-agent/src/coding_agent/ui/session_manager.pycoding-agent/tests/ui/test_http_server.pycoding-agent/tests/ui/test_session_manager_owner_checks.py
Closed sessions should not leave session_owners rows behind; deletion now releases the held owner lease before metadata removal and maps the close race to a 404 SSE response. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Keep the owner row until session metadata deletion succeeds so failed closes do not strand durable sessions without an owner. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except SessionOwnershipConflictError as exc: | ||
| if str(exc) == "session has no owner": | ||
| raise HTTPException( | ||
| status_code=404, | ||
| detail=f"Session not found: {session_id}", | ||
| ) from exc |
There was a problem hiding this comment.
This branch relies on str(exc) == "session has no owner" to decide between 404 vs 409. Comparing exception messages is brittle (easy to break with refactors, localization, or formatting changes) and duplicates the string in multiple places. Prefer a typed exception (e.g., a dedicated subclass for missing owner) or an explicit attribute/error-code on SessionOwnershipConflictError that callers can branch on.
| except SessionOwnershipConflictError as exc: | ||
| if str(exc) == "session has no owner": | ||
| raise HTTPException( | ||
| status_code=404, | ||
| detail=f"Session not found: {session_id}", | ||
| ) from exc |
There was a problem hiding this comment.
Same issue as above: branching on str(exc) == "session has no owner" is message-string coupling and repeats logic. Consider refactoring to a helper that maps SessionOwnershipConflictError to HTTP status based on a typed reason/code rather than string comparison.
Attach structured reasons to owner conflicts so HTTP callers can map missing owners without depending on display text. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Use the structured owner-conflict reason in SSE authorization paths so deletion races stay 404 without coupling to exception text. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| owner = await self._owner_store.get_owner(session_id) | ||
| if owner is None: | ||
| raise SessionOwnershipConflictError("session has no owner") | ||
| raise SessionOwnershipConflictError( | ||
| "session has no owner", | ||
| reason=SessionOwnershipConflictReason.MISSING_OWNER, | ||
| ) | ||
| if owner.lease_expires_at <= datetime.now(UTC): | ||
| raise SessionOwnershipConflictError("session owner lease expired") | ||
| raise SessionOwnershipConflictError( | ||
| "session owner lease expired", | ||
| reason=SessionOwnershipConflictReason.EXPIRED_LEASE, | ||
| ) |
There was a problem hiding this comment.
_assert_owner() tries to distinguish expired leases via owner.lease_expires_at <= now, but the PG implementation of SessionOwnerStore.get_owner() is backed by PGSessionOwnerStore.get_owner() which currently filters out expired leases at the SQL level (... AND lease_expires_at > NOW()). As a result, expired leases will be surfaced as owner is None and reported with reason=MISSING_OWNER, making EXPIRED_LEASE effectively unreachable for PG and potentially misclassifying expiry as “no owner” for API responses.
| try: | ||
| await self._persist_session_async(session) | ||
| await self._acquire_owner_for_session(session_id) | ||
| except BaseException: | ||
| self._session_cache.pop(session_id, None) | ||
| try: | ||
| await self._run_store_io(self._store.delete, session_id) | ||
| except BaseException: | ||
| logger.exception( | ||
| "Failed to delete partially created session during rollback: %s", | ||
| session_id, | ||
| ) | ||
| self._approval_stores.pop(session_id, None) | ||
| raise |
There was a problem hiding this comment.
The create-session rollback path catches BaseException and then attempts async cleanup; if the coroutine is being cancelled (e.g., request disconnect/shutdown), the inner await _run_store_io(...) can raise asyncio.CancelledError and get logged via logger.exception("Failed to delete..."), producing noisy/incorrect error logs for normal cancellation. Consider handling asyncio.CancelledError separately (or shielding best-effort rollback) and avoid logging cancellations as rollback failures.
| configured_backend = http_backend if http_backend is not None else session_backend | ||
| if isinstance(configured_backend, str) and configured_backend.strip().lower() == "pg": | ||
| return True |
There was a problem hiding this comment.
_storage_uses_pg_http_sessions() returns True whenever tape_backend is pg, even if http_session_backend is explicitly configured to a non-PG value. This can cause the HTTP server to enable owner leases (and require a PG DSN) even though the session metadata backend is not PG. Recommend mirroring SessionManager._create_http_session_store() logic: only fall back to tape_backend == 'pg' when http_session_backend/session_backend are unset, not when they are explicitly non-PG.
| configured_backend = http_backend if http_backend is not None else session_backend | |
| if isinstance(configured_backend, str) and configured_backend.strip().lower() == "pg": | |
| return True | |
| if http_backend is not None: | |
| return isinstance(http_backend, str) and http_backend.strip().lower() == "pg" | |
| if session_backend is not None: | |
| return isinstance(session_backend, str) and session_backend.strip().lower() == "pg" |
| status_code=404, | ||
| detail=f"Session not found: {session_id}", |
There was a problem hiding this comment.
_owner_conflict_http_exception() maps reason=MISSING_OWNER to a 404 “Session not found”. With the current PG owner-store implementation, expired leases are also returned as “no owner” (the PG query filters lease_expires_at > NOW()), so an expired lease can incorrectly become a 404 instead of a conflict/ownership error. Either ensure the owner store can distinguish missing vs expired, or adjust this mapping to avoid turning all missing-owner conflicts into 404s.
| status_code=404, | |
| detail=f"Session not found: {session_id}", | |
| status_code=409, | |
| detail=f"Session is not currently owned or its owner lease expired: {session_id}", |
| # Startup | ||
| cleanup_task = asyncio.create_task(_cleanup_idle_sessions()) | ||
| owner_renew_task = asyncio.create_task(_renew_owner_leases()) | ||
| logger.info("HTTP server starting up") |
There was a problem hiding this comment.
The lifespan currently starts the owner-lease renewal loop unconditionally (asyncio.create_task(_renew_owner_leases())). When owner leases are not configured (default memory/redis session backend), _renew_owner_leases() will still run forever and wake up periodically, adding unnecessary background work. Consider only starting this task when session_manager has an owner store configured, or have _renew_owner_leases() exit immediately when _owner_store is None.
Return expired owner rows from PG so the session manager can classify lease expiry separately from missing owners. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Avoid enabling owner leases for non-PG HTTP sessions, skip renewal work when leases are disabled, and avoid noisy rollback logs for cancellation. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
@codex[agent] please review this PR |
|
To use Codex here, create a Codex account and connect to github. |
Co-authored-by: delete-cloud <76783546+delete-cloud@users.noreply.github.com>
Reviewed and pushed a small follow-up fix for a pytest collection issue (commit df8f994). Focused UI/owner-lease tests are passing locally. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| assert isinstance(manager._owner_store, FakeOwnerStore) | ||
| assert manager._owner_store.pg_pool is manager._pg_pool | ||
| assert manager._owner_id == "pod-a" | ||
| assert manager._fencing_token == 9 | ||
| assert manager.owner_lease_seconds == 40.0 | ||
|
|
There was a problem hiding this comment.
This test constructs a PG-backed SessionManager via _build_session_manager(), which starts a PGSessionMetadataStore background thread, but the manager is never closed. Over the full test suite this can accumulate threads and other resources. Add cleanup (e.g., asyncio.run(manager.close()) in a finally, or make the test async and await manager.close()).
| assert isinstance(manager._owner_store, FakeOwnerStore) | |
| assert manager._owner_store.pg_pool is manager._pg_pool | |
| assert manager._owner_id == "pod-a" | |
| assert manager._fencing_token == 9 | |
| assert manager.owner_lease_seconds == 40.0 | |
| try: | |
| assert isinstance(manager._owner_store, FakeOwnerStore) | |
| assert manager._owner_store.pg_pool is manager._pg_pool | |
| assert manager._owner_id == "pod-a" | |
| assert manager._fencing_token == 9 | |
| assert manager.owner_lease_seconds == 40.0 | |
| finally: | |
| asyncio.run(manager.close()) |
| async def _renew_owner_leases() -> None: | ||
| if not session_manager.has_owner_leases_configured: | ||
| return | ||
| while True: | ||
| try: | ||
| await session_manager.renew_owner_leases() | ||
| except Exception: | ||
| logger.exception("Error renewing owner leases") | ||
| await asyncio.sleep(max(session_manager.owner_lease_seconds / 2.0, 1.0)) |
There was a problem hiding this comment.
_renew_owner_leases() sleeps max(owner_lease_seconds / 2, 1.0). Because _configured_owner_lease_seconds() / configure_owner_leases() accept any positive float, configs with owner_lease_seconds < 1.0 will always renew too late (sleeping 1s while the lease expires sooner), guaranteeing lease expiry. Consider either enforcing a minimum lease duration (>= 1s, or higher) or changing the sleep floor so it can’t exceed owner_lease_seconds / 2 (while still avoiding a busy loop).
|
@codex[agent] Please review this PR |
Co-authored-by: delete-cloud <76783546+delete-cloud@users.noreply.github.com>
Reviewed and pushed follow-up fixes for two review nits: close the PG SessionManager in the wiring test and enforce a minimum |
|
To use Codex here, create a Codex account and connect to github. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df8f994c4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| manager.configure_owner_leases( | ||
| owner_store=owner_store, | ||
| owner_id=_configured_owner_id(storage_config), | ||
| fencing_token=_configured_fencing_token(storage_config), | ||
| owner_lease_seconds=_configured_owner_lease_seconds(storage_config), |
There was a problem hiding this comment.
Configuring owner leases for every PG-backed HTTP session here immediately turns on strict ownership checks for all existing persisted sessions, but pre-existing rows in the session store do not necessarily have entries in session_owners. After upgrade, those sessions hit _assert_owner() and fail with MISSING_OWNER, and the renew loop skips owner is None instead of claiming the lease, so they remain unusable (run/close/events) until manual intervention. Please acquire/backfill owner leases for existing session IDs (startup or lazy claim path) before enabling strict checks.
Useful? React with 👍 / 👎.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
@codex[agent] please review these fix 36ab6e2 and b7a0c52 |
Reviewed commits |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7a0c52680
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| owner = await self._owner_store.get_owner(session_id) | ||
| if owner is not None: | ||
| continue |
There was a problem hiding this comment.
When a replica dies without graceful shutdown, its session_owners row remains and eventually expires; on the next startup, backfill_owner_leases() skips that session because get_owner() returned a record, so no new owner is acquired. Combined with renew_owner_leases() skipping expired records and _assert_owner() rejecting them, those sessions stay permanently inaccessible (HTTP 409 on owner checks) until someone manually deletes owner rows. This turns expected failover recovery into a hard outage for affected sessions.
Useful? React with 👍 / 👎.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Summary
SessionManager.create_session()persists PG-backed sessions, with rollback on owner-acquire failure.SessionOwnerStore, renew leases during lifespan, and release owned sessions on shutdown.Verification
uv run pytest tests/ui/test_http_server.py tests/ui/test_http_server_failover.py tests/ui/test_session_manager_owner_checks.py -k "events or owner or failover or create_session or readyz or healthz" -v— 44 passed, 62 deselecteduv run pytest tests/ui/test_session_manager_public_api.py tests/ui/test_session_manager_runtime.py -k "binding or failover or event or owner or close_session" -v— 11 passed, 72 deselecteduv run pytest tests/agentkit/storage/test_pg.py -k "owner or lease or fencing" -v— 11 passed, 34 deselectedsrc/coding_agent/ui/http_server.py,src/coding_agent/ui/session_manager.py,tests/ui/test_http_server.py, andtests/ui/test_session_manager_owner_checks.pyNotes
Summary by CodeRabbit
New Features
Improvements
Tests