fix(opencode-plugin): persist per-session git-return state and worktree - #54
Merged
mislavivanda merged 12 commits intoSep 2, 2026
Merged
Conversation
… per project The storage file kept a single project-level worktree that every session access overwrote. Runtime paths already bind to the active worktree, but the delete path reads the STORED value, so with linked worktrees the final sync of session A could run against whichever checkout session B touched last - evaluating repo accessibility (the deletion-abort guard) against the wrong directory and hard-resetting the wrong checkout's working files when it had opencode/N checked out. Store the worktree per session and prefer it on lookup, falling back to the project-level value for storage files written by older versions. Part of #46 (follow-up item 2). Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
Failures on the automatic git-return paths were transient log lines and TUI toasts: a failed initial push still ended in a successful sandbox return, idle-sync failures were logged only, and a shutdown that exceeded the drain window exited cleanly. A host observing the session could not distinguish 'changes are back' from 'return is pending or broken'. Record every transition of the return path in the session's entry in the per-project state file (gitReturn: pending / synced / failed / setup-failed / disabled, with message and timestamp): - sandbox creation records pending, disabled, or setup-failed - idle syncs record synced or failed with the git error - the gitSync tool records outcomes, and its result now also surfaces a prior setup failure or failed automatic sync instead of implying a clean state - an aborted delete-time sync records failed - a shutdown drain timeout marks still-pending sessions failed README documents the field and the supervisor contract: verify a session's return by reading gitReturn or by checking the gitSync tool result, never by treating silence as success. Part of #46 (follow-up item 1). Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
mislavivanda
force-pushed
the
fix/opencode-plugin-session-scoped-git-return
branch
from
September 1, 2026 14:14
1990d64 to
3192140
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
…lure, not silence Live testing of the git-return ledger exposed two paths that still reported a broken return quietly: - autoCommitAndPull returned 'no changes' when the local repository was missing, so an idle sync after the worktree vanished recorded a healthy 'synced' state. Sessions with syncing legitimately disabled never get a branch number and cannot reach this path, so a missing repo here is always breakage - throw instead, letting callers record 'failed' with the real reason. - The delete-time 'repository inaccessible' abort threw but wrote nothing to the ledger; it now records 'failed' with the abort message before throwing. Verified with a live end-to-end run through the real event handlers and session manager against real sandboxes: linked-worktree sessions keep their own worktrees (the reported A/B overwrite scenario), shared refs land from either checkout, a vanished worktree records 'failed' on idle, deletion aborts and preserves the sandbox with the failure recorded, restore-and-retry deletes cleanly, and an unsynced change is pulled before final deletion. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
…nst concurrent instances Review follow-ups on the git-return ledger: - Guard project-file updates with a cross-process lock and write them atomically (temp + rename): two OpenCode instances sharing a project replace the whole JSON on save, so concurrent updates could revert the other instance's newer worktree or ledger fields, and a crash mid-write could leave a torn file. Within a process the storage methods are synchronous, so the lock only guards other processes; stale locks from crashed processes are stolen after 5s. - Record 'pending' BEFORE awaiting initializeAndSync, so a process dying mid-setup leaves a trace instead of an absent record. - Record delete-time failures around the whole final-sync attempt (refreshData and repository checks included), not only the sync call. - gitSync: persist 'disabled' on the no-branch early return, read the previous state inside the queue entry so the retry note reflects a failure persisted by a queued sync, and stop attributing every failure to 'automatic' sync in the note. Verified: cross-process contention test (two processes, 120 interleaved read-modify-writes, both sessions' final states intact, no stray lock or temp files) and the full live lifecycle suite re-run against real sandboxes (linked-worktree isolation, vanished-worktree failure, aborted delete with preserved sandbox, restore-and-retry, final-sync-before- delete) - all passing. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…missing-repo contract The JSDoc still described the pre-b5e7924 behavior (false when the local repo is missing). Document the actual contract - false only when both tips already match, throw when the repository is inaccessible or any git step fails - and the current call sites (idle, gitSync, pre-deletion). Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…ocks, separate disabled from broken Third review round on the storage/ledger work: - getSession's cross-project migration performed whole-file writes outside withFileLock, so its stale snapshot could overwrite another instance's concurrent session or ledger updates. The destination copy and source cleanup now each re-read and write under their own lock (never nested, destination first). - Stale locks are stolen only when the recorded owner PID is confirmed dead (signal-0 probe). A live-but-slow holder keeps its lock and contenders wait out the deadline instead of running concurrently with it. Lease/heartbeat schemes were considered and rejected: they require async storage plumbing for a scenario (a live process stalled >5s inside a millisecond-scale file write) with no realistic trigger. - gitSync no longer records 'disabled' when a branch number is missing but the local repository exists: that combination means allocation failed earlier, so it records 'failed' and errors instead of labeling a broken return path as intentionally off. Verified live: dead-owner stale lock stolen in under 2s with the write landing and the lock released; live-owner stale lock NOT stolen (full deadline wait, fail-open write, lock left in place); two-process contention rerun clean with no lost updates and no leftover lock or temp files; typecheck clean. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
…t predate their repository Fourth review round: - getSession copied a pre-lock snapshot, so a source-side session update landing between the snapshot and the source cleanup was deleted with the stale record. The snapshot is now taken under the source lock and cleanup is compare-and-delete: the source record is only removed when it still equals what was copied; otherwise the newer record survives and the stale destination copy is refreshed by the session's next updateSession. Migration never destroys data it did not copy. - gitSync treated every missing branch number with an existing repository as a failed allocation, which misclassified sessions that legitimately started before their worktree became a repository (git init after session start). A prior 'disabled' ledger state now yields a graceful disabled response explaining that syncing cannot be adopted mid-session (the initial push would clobber the agent's sandbox work); only non-disabled prior states report the broken-allocation failure. Verified: deterministic mid-migration source-update simulation (newer record survives cleanup, snapshot lands in destination, no lock stalls or leftovers) and a live sandbox run of the exact git-init-after-start flow (graceful disabled message, ledger unchanged) plus the failed- allocation counterpart (throws, ledger failed); typecheck clean. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
…source record The compare-and-delete abort protected the newer source record but left the stale copy in the destination - and getSession/findSession consult the destination first, so reads (including the delete path's worktree and branch lookups) could be served the stale record until the session's next updateSession. Migration now retries from the newer source record (up to three attempts); if the source keeps changing, the destination copy is withdrawn so the newest source record stays the single authoritative one, the caller receives that newest record, and a later access re-migrates. Cleanup errors (as opposed to races) keep best-effort semantics since the duplicate they leave is byte-identical. Verified deterministically: plain migration; one mid-migration source update (retry migrates the newer record and cleans the source); a continuously-updated source (destination withdrawn, newest record stays authoritative and is returned, no leftover locks); typecheck clean. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
…estination The withdrawal path deleted the destination record unconditionally, so a session updated in the destination file during the source-change retries (another instance recording gitReturn for that session under the new project id) lost that newer record - the same failure the source-side compare-and-delete already prevented, mirrored onto the destination. Track the last snapshot this migration copied and withdraw only while the destination record still equals it. A destination record updated after our copy is newer than anything we hold, so it stays in place and is returned to the caller. Both removals are now conditional: neither side's concurrent update can be destroyed by migration. Verified deterministically across four interleavings: plain migration; one source change (retry migrates the newer record, source cleaned); a continuously-changing source with an untouched destination (our copy withdrawn, newest source record authoritative); and a continuously- changing source WITH a destination update after the last copy (newer destination record kept and returned, source intact, no leftover locks). Typecheck clean. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
…ct and purge deletes everywhere An abandoned migration can leave the same session recorded in two project files (when the destination copy was updated concurrently, neither side is provably older, so neither is deleted). findSession then returned whichever file it scanned first, so ledger writes, ledger reads and deletion could resolve to different copies: a gitReturn update could land in one file while a host read the other, and deletion could target one copy and leave the other pointing at a destroyed sandbox. - findSession takes an optional preferProjectId and searches that file first; every caller passes the project it operates as (delete path, hasSandbox, recordGitReturn, getGitReturn), so all paths resolve to the same record. - removeSession now purges the session from EVERY project file: once a sandbox is confirmed gone, any record for it is stale, and a leftover copy would be reconnected to a destroyed sandbox on a later resume. Deliberately not reconciling the duplicate by timestamp: ordering two concurrently-written records would need an invented merge policy, and equal or missing timestamps would risk destroying the newer one - the failure mode this work exists to eliminate. Preference plus purge makes the duplicate harmless without guessing. Verified: 19 assertions - duplicate-record targeting (both directions, write isolation, purge-all), manager plumbing following project context, all four migration interleavings as regression, and a live sandbox create/delete confirming the record is purged. Typecheck clean. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode-plugin/.opencode/plugin/daytona/core/project-data-storage.ts">
<violation number="1" location="packages/opencode-plugin/.opencode/plugin/daytona/core/project-data-storage.ts:423">
P2: When migration or another OpenCode process updates storage during deletion, `removeSession` can leave the new record behind because the file list and existence check precede the per-file locks. Coordinate the purge with a cross-file lock or re-scan/re-read under lock before treating the session as removed.</violation>
</file>
<file name="packages/opencode-plugin/.opencode/plugin/daytona/core/session-manager.ts">
<violation number="1" location="packages/opencode-plugin/.opencode/plugin/daytona/core/session-manager.ts:445">
P2: When deletion runs before this manager has selected the target project, a delete-time sync failure can be persisted to the wrong project record. Pass the `projectId` received by `deleteSandboxInner` through `syncBeforeDelete` and `recordGitReturn` so the failed status updates the record being deleted.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
… delete purge - Deletion never selects a project context, so the delete-time failure status was persisted with whatever ambient project the manager happened to hold: with a session recorded in two project files it could land in the copy the host is not reading. recordGitReturn now takes an explicit projectId, and every caller that knows which project it acts for passes it (creation, idle sync, gitSync tool, delete-time sync). Only the shutdown drain, which spans sessions from any project, still falls back to the ambient context and then to a scan. - removeSession reads the file list and the existence check outside the per-file locks, so a record written into an already-visited file while the purge ran could survive it and later be reconnected to a destroyed sandbox. The purge now runs a second pass whenever the first removed anything. Not adding a global cross-file lock for the purge: it would serialize every unrelated project behind one lock to close a window that a bounded second pass already covers. Verified: 17 assertions - explicit target beats ambient context (and the ambient fallback still works), a record injected into an already-visited file is purged by the second pass, plus duplicate-record targeting and all four migration interleavings as regression; live sandbox create/delete confirms ledger and purge behavior. Typecheck clean. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The short-circuit skipped the second pass whenever the first removed nothing, which assumed that finding no record proves none exists. It does not: the file list and existence checks run outside the per-file locks, so a record written into a file the first pass had already walked past survives it - exactly the window the second pass exists to cover, and it is independent of whether anything was removed elsewhere. Both passes now run unconditionally. The extra pass is a directory scan and a few small reads on a path that runs once per session teardown. Verified: a record injected into an already-visited file during a pass that removes nothing is purged by the second pass, unrelated sessions in both files are untouched, no leftover locks, and the ordinary purge-from-every-file case still passes. Typecheck clean. Signed-off-by: Mislav Ivanda <mislavivanda454@gmail.com>
mislavivanda
deleted the
fix/opencode-plugin-session-scoped-git-return
branch
September 2, 2026 10:03
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Follow-up to #46 — addresses items 1 and 2 from the reporter's 0.192.0 test report (item 3, the published host-key fingerprint, is being handled at the platform level via
/api/config.sshGatewayPublicKey+ security policy).Item 2 — per-session worktree (commit 1)
The storage file kept a single project-level worktree overwritten by every session access. Runtime paths already bind to the active worktree (#48), but the delete path reads the stored value — so with linked worktrees, session A's final delete-time sync could run against whichever checkout session B touched last: the deletion-abort guard evaluated the wrong directory, and the wrong checkout's working files could be hard-reset.
Fix:
SessionInfo.worktreeis written on every session touch and preferred on lookup; the project-level field remains as fallback for storage files written by older versions.Item 1 — persistent git-return ledger (commit 2)
Failures on the automatic return paths were transient (log + TUI toast): a failed initial push still ended in a successful sandbox return; idle-sync failures were logged only; a shutdown exceeding the 60s drain window exited cleanly. A host could not distinguish "changes are back" from "return pending/broken" without parsing logs.
Fix: every transition of a session's return path is recorded in the per-project state file under
gitReturn—pending/synced/failed/setup-failed/disabled, with message + timestamp:pending,disabled, orsetup-failedsyncedorfailed(git error in message)gitSynctoolfailedfailedfor each still-pending sessionREADME documents the field and the supervisor contract: verify a session's return by reading
gitReturn(or thegitSynctool result) — never by treating silence as success.Testing
tsc --noEmitcleangitReturnround-trip, no-op after session removalDaytonaSessionManageragainst a real sandbox: creation recordspending+ per-session worktree + branch number; deletion (final sync no-op on running sandbox) removes the session record; no leaksSummary by cubic
Fixes the Daytona OpenCode plugin's session isolation and git handoff tracking. Sessions previously shared a project-level worktree and exposed sync failures only through logs; they now store their own worktree and persistent
gitReturnstatus, while failed final syncs preserve the sandbox.Storage
Git Return
pending,synced,failed,setup-failed, ordisabledwith a message and timestamp.gitSyncdistinguishes intentionally disabled sessions from broken branch allocation and reports prior setup or sync failures.Written for commit 6831bf5. Summary will update on new commits.