Repository navigation
fix: persist remote workspace records on create - #173
Conversation
📝 WalkthroughWalkthroughSession creation now persists cloud workspace metadata by enriching origin fields in the HTTP server, defining a protocol contract for persistence storage, implementing persistence logic in SessionManager before owner lease acquisition, and validating the flow through updated test infrastructure. ChangesCloud Workspace Record Persistence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 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 (2)
coding-agent/src/coding_agent/ui/session_manager.py (1)
945-955: ⚡ Quick winStrengthen validation to check for non-empty strings.
The validation checks that fields are strings but doesn't verify they're non-empty. While the HTTP server populates origin with non-empty values, defensive validation would prevent potential issues if origin is constructed elsewhere or if the HTTP server logic changes.
🛡️ Proposed validation improvement
if ( not isinstance(provider, str) + or not provider.strip() or not isinstance(provider_instance_id, str) + or not provider_instance_id.strip() or not isinstance(workspace_root_ref, str) + or not workspace_root_ref.strip() or not isinstance(workspace_host_label, str) + or not workspace_host_label.strip() or not isinstance(owner_label, str) + or not owner_label.strip() or not isinstance(source_kind, str) + or not source_kind.strip() ): raise RuntimeError( "cloud workspace session is missing durable workspace metadata" )Additionally, consider making the error message more specific by indicating which field(s) are missing or invalid.
🤖 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 `@coding-agent/src/coding_agent/ui/session_manager.py` around lines 945 - 955, The current validation in the session metadata block only checks types; update the check in the session_manager validation (the if that inspects provider, provider_instance_id, workspace_root_ref, workspace_host_label, owner_label, source_kind) to also assert each value is a non-empty string (e.g., isinstance(..., str) and bool(... .strip()) ), and when raising the RuntimeError include a clear message naming which field(s) are missing or empty so callers can see exactly which metadata failed validation.coding-agent/tests/ui/test_session_manager_runtime.py (1)
1943-1943: 💤 Low valueConsider adding defensive assertion before record assignment.
The pattern
store.records[0] = _workspace_record(...)assumescreate_session()persisted exactly one record. While this provides implicit coverage of the persistence logic, an explicit assertion would make the assumption clear and provide better failure messages.🛡️ Optional: Add defensive check
For example, in
test_close_session_retains_workspace_when_policy_is_pinned:session_id = await manager.create_session( execution_binding=binding, origin={ "channel": "http", "binding_kind": "cloud", "workspace_source_kind": "git", "workspace_provider": "docker", "provider_instance_id": "docker-host-a", "workspace_root_ref": "/workspaces", "workspace_host_label": "docker-host-a", "owner_label": "owner:test", }, ) + assert len(store.records) == 1, "Expected create_session to persist one record" store.records[0] = _workspace_record( session_id=session_id, workspace_id=binding.workspace_id, retention_policy="pinned", )Apply similar assertions in the other three tests that use this pattern.
Also applies to: 1981-1981, 2022-2022, 2065-2065
🤖 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 `@coding-agent/tests/ui/test_session_manager_runtime.py` at line 1943, Before doing the assignment store.records[0] = _workspace_record(...), add a defensive assertion that the session creation actually persisted the expected record (e.g. assert len(store.records) == 1 or assert len(store.records) >= 1, with a helpful message) so the test fails clearly if persistence didn't occur; apply this change in test_close_session_retains_workspace_when_policy_is_pinned and the other three tests that use the same pattern (the lines using store.records[0], create_session(), and _workspace_record).
🤖 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 `@coding-agent/src/coding_agent/ui/session_manager.py`:
- Around line 1487-1499: When owner lease acquisition
(_acquire_owner_for_session) fails after the workspace record was persisted
(_persist_workspace_record_for_session), add rollback logic to mark the
persisted workspace record as stale so GC can collect it: after popping the
session from _session_cache and before/after releasing the owner lease
(_release_owner_lease_for_session), update the persisted workspace record for
that session (either by re-calling _persist_workspace_record_for_session with
status="stale"/retention_policy="delete_on_close" or by calling a new helper
e.g. _mark_workspace_record_stale(session_id)) so that
_workspace_record_is_gc_eligible will return True for the orphaned record;
ensure this update is done inside the same exception handling block and surround
it with its own try/except to log failures without masking the original
exception.
---
Nitpick comments:
In `@coding-agent/src/coding_agent/ui/session_manager.py`:
- Around line 945-955: The current validation in the session metadata block only
checks types; update the check in the session_manager validation (the if that
inspects provider, provider_instance_id, workspace_root_ref,
workspace_host_label, owner_label, source_kind) to also assert each value is a
non-empty string (e.g., isinstance(..., str) and bool(... .strip()) ), and when
raising the RuntimeError include a clear message naming which field(s) are
missing or empty so callers can see exactly which metadata failed validation.
In `@coding-agent/tests/ui/test_session_manager_runtime.py`:
- Line 1943: Before doing the assignment store.records[0] =
_workspace_record(...), add a defensive assertion that the session creation
actually persisted the expected record (e.g. assert len(store.records) == 1 or
assert len(store.records) >= 1, with a helpful message) so the test fails
clearly if persistence didn't occur; apply this change in
test_close_session_retains_workspace_when_policy_is_pinned and the other three
tests that use the same pattern (the lines using store.records[0],
create_session(), and _workspace_record).
🪄 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: 93fb0198-3e71-4c84-b452-77671cd406a5
⛔ Files ignored due to path filters (1)
coding-agent/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
coding-agent/pyproject.tomlcoding-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_runtime.py
| await self._persist_workspace_record_for_session(session) | ||
| await self._acquire_owner_for_session(session_id) | ||
| except BaseException: | ||
| self._session_cache.pop(session_id, None) | ||
| try: | ||
| await self._release_owner_lease_for_session(session_id) | ||
| except asyncio.CancelledError: | ||
| pass | ||
| except BaseException: | ||
| logger.exception( | ||
| "Failed to release partially created owner lease during rollback: %s", | ||
| session_id, | ||
| ) |
There was a problem hiding this comment.
Critical: Orphaned workspace records are not cleaned up when owner lease acquisition fails.
When workspace record persistence succeeds (line 1487) but owner lease acquisition fails (line 1488), the rollback removes the session but doesn't clean up the workspace record. The orphaned record will have status="active" and retention_policy="delete_on_close", but because the session doesn't exist, the workspace_id won't be in active_workspace_ids.
Checking the GC eligibility logic (_workspace_record_is_gc_eligible, lines 654-668), an orphaned record with status="active" returns False at line 668 because status != "stale". This means orphaned workspace records will accumulate and never be cleaned up by GC.
Potential solutions
Option 1: Update orphaned record status in rollback
except BaseException:
self._session_cache.pop(session_id, None)
try:
await self._release_owner_lease_for_session(session_id)
except asyncio.CancelledError:
pass
except BaseException:
logger.exception(
"Failed to release partially created owner lease during rollback: %s",
session_id,
)
+ # Mark workspace record as stale so GC can clean it up
+ if self._workspace_metadata_store is not None and isinstance(binding, CloudWorkspaceBinding):
+ try:
+ record = await self._workspace_metadata_store.load_for_session_workspace(
+ session_id=session_id,
+ workspace_id=binding.workspace_id,
+ )
+ if record is not None:
+ await self._workspace_metadata_store.update_status(
+ record.workspace_record_id,
+ status="cleanup_failed",
+ cleanup_error="Session creation failed after workspace provisioning",
+ )
+ except Exception:
+ logger.exception(
+ "Failed to mark orphaned workspace record as cleanup_failed: %s",
+ session_id,
+ )
try:
await self._run_store_io(self._store.delete, session_id)Option 2: Persist workspace record after owner lease acquisition
Move line 1487 to after line 1488, though this contradicts the stated design intent in the layer description.
Option 3: Enhance GC to detect orphaned records
Modify _workspace_record_is_gc_eligible to check if the session exists and clean up records with non-existent sessions.
🤖 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 `@coding-agent/src/coding_agent/ui/session_manager.py` around lines 1487 -
1499, When owner lease acquisition (_acquire_owner_for_session) fails after the
workspace record was persisted (_persist_workspace_record_for_session), add
rollback logic to mark the persisted workspace record as stale so GC can collect
it: after popping the session from _session_cache and before/after releasing the
owner lease (_release_owner_lease_for_session), update the persisted workspace
record for that session (either by re-calling
_persist_workspace_record_for_session with
status="stale"/retention_policy="delete_on_close" or by calling a new helper
e.g. _mark_workspace_record_stale(session_id)) so that
_workspace_record_is_gc_eligible will return True for the orphaned record;
ensure this update is done inside the same exception handling block and surround
it with its own try/except to log failures without masking the original
exception.
Summary
Validation
Summary by CodeRabbit
New Features
Dependencies