Skip to content

fix: keep workspaces tied to repository identity - #993

Open
wesm wants to merge 4 commits into
mainfrom
workspace-error-renamed
Open

fix: keep workspaces tied to repository identity#993
wesm wants to merge 4 commits into
mainfrom
workspace-error-renamed

Conversation

@wesm

@wesm wesm commented Aug 29, 2026 •
edited by mariusvniekerk
Loading

Copy link
Copy Markdown
Member

Workspace creation now keys provider-backed workspaces by the stable catalog repository ID. Previously, a route that had been occupied by another repository was rejected even when the catalog knew the current occupant was different.

  • Renames retain existing workspace associations, while a later occupant of the old route gets an independent workspace namespace.
  • Migration 54 backfills only unambiguous legacy rows; ambiguous rows keep the historical-route safety fence.
  • If pre-rename and post-rename routes contain duplicate workspaces for the same item, migration 54 keeps the newest workspace and deletes the older one as a unit.
  • Setup rechecks stable identity before repository access, so route replacement races still fail closed.
  • Base-branch sync verifies repository ownership without reacquiring the reconciliation read lock.
  • Workspace listing joins repository metadata in one query instead of issuing nested reads while result rows remain open.
  • Background pull request association and pushed-head refreshes use the workspace repository ID and stop when that repository is inactive.

@wesm @codex
fix: keep workspaces tied to repository identity
8e0dd25 
A repository route can be reassigned after a repository is removed or renamed.
Route-keyed workspaces then reject the new repository even though the catalog
already knows that it has a different stable identity.

Persist the catalog repository ID on provider workspaces. Renames now retain
their workspaces, and a later occupant of the old route gets an independent
workspace namespace. Legacy rows stay route-keyed only when their ownership is
unambiguous; otherwise the existing safety fence remains in place.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

This comment has been minimized.

Sign in to view
@mariusvniekerk mariusvniekerk self-assigned this Aug 30, 2026
@mariusvniekerk @codex
fix: keep workspace identity reads moving
b80537b 
Workspace listing resolved each stable repository with a nested read while the outer result rows still held a pool connection. Concurrent callers could consume every read connection and stop all list requests. Resolve the current repository route in the list query instead.

Base-branch sync also acquired the reconciliation read lock again during repository verification. A queued writer could then block the nested read while the outer read prevented that writer from proceeding. Reuse the existing lock for verification.

The existing rename and route-reuse tests now assert the stable workspace ownership introduced by this pull request.

Review decision: the user marked route-keyed local Git storage as not an issue, so this commit makes no change for that finding. Source: roborev-ci comment #993 (comment).

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

This comment has been minimized.

Sign in to view
@mariusvniekerk @codex
fix: keep background workspace work on stable repositories
39acae5 
Existing databases can contain the same repository item under both sides of a rename. Assigning both rows the same repository ID made the new unique index reject the migration and prevented startup. Keep the newest workspace and delete the older workspace as one unit instead of extending legacy route support.

Background pull request association and pushed-head refreshes also resolved mutable routes after loading a workspace. A replacement repository could therefore receive work for the old workspace. Carry the stable repository ID through refresh operations and stop when that repository is inactive.

Review source: #993 (comment)

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

This comment has been minimized.

Sign in to view
@mariusvniekerk @codex
fix: fence delayed workspace refreshes by repository
290959e 
Pushed-head work can wait in a background queue while a repository route is reassigned. A route-only refresh could then fetch data from the replacement repository and store it under the original workspace owner. Keep full and CI refreshes pinned to the stable repository ID, and discard results when the route fence changes.

Migration timestamps have second-level precision. Use insertion order to choose the newer duplicate when timestamps tie, so migration cleanup does not depend on random workspace IDs.

The base-branch lock regression now exercises the real caller in an isolated process. A future nested read-lock regression fails without hanging the test suite.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Aug 31, 2026

Copy link
Copy Markdown

roborev: Combined Review (290959e)

Verdict: 4 issues remain, including 1 High-severity and 3 Medium-severity findings.

High

  • internal/server/workspaceapi/workspace_pushed_head.go:138-147, internal/github/sync.go:11040-11043: A discarded route-fenced CI refresh returns nil, causing the completion handler to report success, mark the observer refresh complete, and broadcast a refresh despite no persisted CI state. Return an explicit skipped/stale result and only mark success or broadcast when the guarded update applies.

Medium

  • internal/workspace/manager.go:1499-1557: RefreshWorkspaceHeadRepoSnapshot resolves repositories by mutable owner/name, allowing an old workspace after route reuse to read and persist metadata from the replacement repository. Resolve via ws.RepoID and require the update to match the stable repository ID.

  • internal/workspace/manager.go:1125-1140: If a loaded workspace with RepoID == 0 is deleted, reload returns nil and verification falls through to route-based checks, potentially continuing setup with stale state. Return ErrWorkspaceNotFound whenever an ID-bearing workspace cannot be reloaded.

  • internal/github/sync.go:12716-12727, internal/github/sync.go:12865-12875: SyncMRForRepository validates expectedRepoID only after reconciliation may have mutated repository identity, allowing stale queued requests to alter route ownership before rejection. Validate or capture the stable route fence before reconciliation and make reconciliation honor it.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 12m18s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@mariusvniekerk mariusvniekerk

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wesm @mariusvniekerk