Repository navigation
refactor: delegate workspace export - #522
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughExtracts workspace archive export orchestration from inline SessionManager implementation into a new RuntimeWorkspaceExportService that enforces turn-based locking, owner assertion, cloud-session validation, and pre/post-export callback bookkeeping. SessionManager delegates to this service via wiring during initialization. ChangesRuntime Workspace Export Service Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
Owner
Author
|
Local fallback review because CodeRabbit has remained pending with only the auto "review in progress" comment and no completed review. Manual P1/P2 review result: no blocking findings. Checked:
Verification run:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
coding-agent/.opencode/prompts/tasks/runtime-workspace-export-service-task-packet.md (1)
1-5: ⚡ Quick winAlign preserved behavior terminology with ADR completion item.
The goal lists preserved behaviors as "active-turn rejection, cloud-binding validation, concurrent idle exports, export-in-progress guards, and post-export owner revalidation." However, the corresponding ADR 0058 completion item (lines 362-365 in the other file) uses slightly different terminology: "cloud-session validation, export-in-progress bookkeeping, thread offload, and post-export owner revalidation."
Consider harmonizing these descriptions across both documents:
- "cloud-binding validation" vs "cloud-session validation"
- "active-turn rejection" is not mentioned in the ADR item
- "concurrent idle exports" is not mentioned in the ADR item
- "export-in-progress guards" vs "export-in-progress bookkeeping"
- "thread offload" in ADR is not explicitly in the task packet goal
📝 Suggested alignment
Either update this goal to match the ADR wording, or ensure both documents use identical terminology for the same concepts. For example:
Goal: Move workspace archive export orchestration out of `SessionManager` into a -runtime/workspace service boundary while preserving active-turn rejection, -cloud-binding validation, concurrent idle exports, export-in-progress guards, -and post-export owner revalidation. +runtime/workspace service boundary while preserving cloud-session validation, +export-in-progress bookkeeping, thread offload, and post-export owner +revalidation.Or update the ADR item to explicitly mention all preserved behaviors from this task packet.
🤖 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/.opencode/prompts/tasks/runtime-workspace-export-service-task-packet.md` around lines 1 - 5, The goal description in runtime-workspace-export-service-task-packet.md uses terminology that is inconsistent with ADR 0058 (e.g., "cloud-binding validation" vs "cloud-session validation", "active-turn rejection" and "concurrent idle exports" missing from ADR, and "export-in-progress guards" vs "export-in-progress bookkeeping" while ADR mentions "thread offload"); update the task packet so the preserved-behavior list either matches ADR 0058 verbatim or expand ADR 0058 to include the missing concepts—specifically reconcile "cloud-binding validation" to "cloud-session validation", decide whether to include "active-turn rejection" and "concurrent idle exports" in ADR or remove from the task packet, and standardize "export-in-progress guards" to "export-in-progress bookkeeping" (and add explicit mention of "thread offload" where intended) so both documents use identical terminology.
🤖 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/runs/workspace_export.py`:
- Around line 61-75: The turn state checks (turn_lock_for, turn_lock.locked(),
assert_owner, load_session, session.turn_in_progress) race with begin_export
because you never hold the session turn lock while validating and starting
export; fix by acquiring the per-session turn lock (use
turn_lock_for(session_id) as an async context or await turn_lock.acquire())
before performing the checks and calling begin_export, perform all checks
(including session.task and execution_binding) while the lock is held, then call
begin_export and only release the lock after export has been safely initiated
(ensure finally block releases the lock if you manually acquire it); update
references in this block (turn_lock_for, assert_owner, load_session,
begin_export, CloudWorkspaceBinding) accordingly.
---
Nitpick comments:
In
`@coding-agent/.opencode/prompts/tasks/runtime-workspace-export-service-task-packet.md`:
- Around line 1-5: The goal description in
runtime-workspace-export-service-task-packet.md uses terminology that is
inconsistent with ADR 0058 (e.g., "cloud-binding validation" vs "cloud-session
validation", "active-turn rejection" and "concurrent idle exports" missing from
ADR, and "export-in-progress guards" vs "export-in-progress bookkeeping" while
ADR mentions "thread offload"); update the task packet so the preserved-behavior
list either matches ADR 0058 verbatim or expand ADR 0058 to include the missing
concepts—specifically reconcile "cloud-binding validation" to "cloud-session
validation", decide whether to include "active-turn rejection" and "concurrent
idle exports" in ADR or remove from the task packet, and standardize
"export-in-progress guards" to "export-in-progress bookkeeping" (and add
explicit mention of "thread offload" where intended) so both documents use
identical terminology.
🪄 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: c38c7641-8a8b-4b9a-8194-85911ea75129
📒 Files selected for processing (6)
coding-agent/.opencode/prompts/tasks/runtime-workspace-export-service-task-packet.mdcoding-agent/docs/adr/0058-local-daemon-control-plane-executor-architecture.mdcoding-agent/src/coding_agent/runs/__init__.pycoding-agent/src/coding_agent/runs/workspace_export.pycoding-agent/src/coding_agent/server/session_manager.pycoding-agent/tests/coding_agent/test_runtime_workspace_export_service.py
Comment on lines
+61
to
+75
| turn_lock = self.turn_lock_for(session_id) | ||
| if turn_lock.locked(): | ||
| raise RuntimeError("turn already in progress") | ||
|
|
||
| await self.assert_owner(session_id) | ||
| session = await self.load_session(session_id) | ||
| if session.turn_in_progress or ( | ||
| session.task is not None and not session.task.done() | ||
| ): | ||
| raise RuntimeError("turn already in progress") | ||
| if not isinstance(session.execution_binding, CloudWorkspaceBinding): | ||
| raise ValueError("Workspace export requires cloud session") | ||
|
|
||
| self.begin_export(session_id) | ||
| try: |
Contributor
There was a problem hiding this comment.
Close the admission race between turn checks and begin_export.
Lines 61-74 validate turn state without actually holding the session turn lock. A turn can start in that window, so export can proceed concurrently with an admitted turn.
Proposed fix
async def export_archive(
self,
session_id: str,
export_archive: RuntimeWorkspaceArchiveExporter[T],
) -> T:
turn_lock = self.turn_lock_for(session_id)
if turn_lock.locked():
raise RuntimeError("turn already in progress")
- await self.assert_owner(session_id)
- session = await self.load_session(session_id)
- if session.turn_in_progress or (
- session.task is not None and not session.task.done()
- ):
- raise RuntimeError("turn already in progress")
- if not isinstance(session.execution_binding, CloudWorkspaceBinding):
- raise ValueError("Workspace export requires cloud session")
-
- self.begin_export(session_id)
+ async with turn_lock:
+ await self.assert_owner(session_id)
+ session = await self.load_session(session_id)
+ if session.turn_in_progress or (
+ session.task is not None and not session.task.done()
+ ):
+ raise RuntimeError("turn already in progress")
+ if not isinstance(session.execution_binding, CloudWorkspaceBinding):
+ raise ValueError("Workspace export requires cloud session")
+ self.begin_export(session_id)
try:
result = await asyncio.to_thread(
export_archive,
session.execution_binding,
)🤖 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/runs/workspace_export.py` around lines 61 - 75,
The turn state checks (turn_lock_for, turn_lock.locked(), assert_owner,
load_session, session.turn_in_progress) race with begin_export because you never
hold the session turn lock while validating and starting export; fix by
acquiring the per-session turn lock (use turn_lock_for(session_id) as an async
context or await turn_lock.acquire()) before performing the checks and calling
begin_export, perform all checks (including session.task and execution_binding)
while the lock is held, then call begin_export and only release the lock after
export has been safely initiated (ensure finally block releases the lock if you
manually acquire it); update references in this block (turn_lock_for,
assert_owner, load_session, begin_export, CloudWorkspaceBinding) accordingly.
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
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
Tests
Notes
Summary by CodeRabbit
Refactor
Tests