Skip to content

fix(opencode): recover remote workspace sync after restarts - #1

Merged
internetisalie merged 9 commits into
devfrom
workspace-sync-recovery
Sep 25, 2026
Merged

fix(opencode): recover remote workspace sync after restarts#1
internetisalie merged 9 commits into
devfrom
workspace-sync-recovery

Conversation

@internetisalie

@internetisalie internetisalie commented Sep 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Issue for this PR

Closes #2

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

A remote workspace can remain in error after its proxy restarts. Adapter target failures can surface as defects, so the old error handler did not retry them. This change retries target resolution and transient connection, history, and event-stream failures while preserving interruption. After reconnecting, OpenCode replays history before reporting the workspace as connected.

Repeated /sync/start calls leave an active retrying listener alone, preserving its backoff. Removing a workspace interrupts that listener and prevents further target calls.

Concurrent starts also recheck for an existing listener after asynchronous target resolution, so a late start does not replace a listener that connected in the meantime.
If that late start's target resolution fails, it leaves the healthy listener's connected status intact. Interrupted startup propagates without a misleading recovery warning.

After the OpenCode process itself restarts, persisted workspace records have no active listeners. Opening the project now resumes its workspace sync after the adapter plugin loads. The per-project scan runs once per process, and an explicit /sync/start remains available.

This PR targets the dev branch of this fork only. It does not install or modify any running OpenCode or OpenChamber server.
The fork's CI workflows select standard GitHub Ubuntu runners because its Blacksmith jobs remained queued without runners. Since this fork is being used as a server, its test matrix runs on Linux only. Other repositories retain the original Linux and Windows Blacksmith matrix. A subprocess regression test now checks for a real error exit rather than a wall-clock limit that flaked under CI load.

How did you verify your code works?

  • bun test test/control-plane/workspace.test.ts from packages/opencode: 40 passed, including a concurrent-start test where the late target fails.
  • bun test test/server/httpapi-workspace-routing.test.ts from packages/opencode: 9 passed.
  • bun test test/server/httpapi-instance-context.test.ts from packages/opencode: 10 passed, including persisted workspace recovery on project load.
  • bun typecheck from packages/opencode: passed.
  • A disposable OpenCode → proxy → OpenChamber integration test passed with no /sync/start after either a proxy process restart or a stable OpenCode container restart. The test verified both workspace connections, retained history, and live prompts through OpenChamber. All fixture containers were purged afterward.
  • Focused unknown-model subprocess regression test: 1 passed locally.
  • Fork CI on GitHub runners: Nix evaluation, typecheck, Linux unit, Linux end-to-end, and PR standards passed on the previous commit. A new run is verifying the Linux-only matrix.

Screenshots / recordings

No UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

@github-actions

github-actions Bot commented Sep 24, 2026

Copy link
Copy Markdown

This PR doesn't fully meet our contributing guidelines and PR template.

What needs to be fixed:

  • PR description is missing required template sections. Please use the PR template.

Please edit this PR description to address the above within 2 hours, or it will be automatically closed.

If you believe this was flagged incorrectly, please let a maintainer know.

@github-actions

github-actions Bot commented Sep 24, 2026

Copy link
Copy Markdown

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

  1. Open an issue describing the bug/feature (if one doesn't exist)
  2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

internetisalie
internetisalie commented Sep 24, 2026
View reviewed changes

@internetisalie internetisalie left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 363d929 by reading the diff and the surrounding code. Tests were not run by the reviewer.

Root cause confirmed. WorkspaceAdapterRuntime.target wraps the adapter in EffectBridge.fromPromise, and nothing gives it an error type (workspace-routing.ts types it Effect<Target>). So when an adapter rejects during a proxy outage, the error is a defect. The old Effect.catch never caught it, and the listener died with the workspace still showing error. Switching to catchCause and re-raising interrupts is the right fix, and it is applied consistently in all four places.

Tests: both new tests would fail without the fix.

  • Stream error: before the fix, the error escaped the loop and the fiber ended, so connections stays at 1.
  • Target that throws: the error escaped startSync, so the loop never started.
  • The 4 s timeouts leave about 3 s of slack over the 1 s backoff.

Findings

  1. Medium: error now means "retrying", but two checks still treat it as final. startSync restarts the fiber on exists && status !== "error", and isSyncing makes the same check.
    • During an outage the loop sits in error through each backoff sleep. Any /sync/start call in that window stops the live loop and starts a new one at attempt = 0. A client that retries /sync/start therefore cancels out the 2-minute backoff and hits the proxy about once a second.
    • Nothing distinguishes a loop that is still retrying from one that has given up.
    • The obvious fix, keeping a retrying loop in connecting, would break create(): it waits for error or connected, so it would block for the whole outage. One option is to skip the restart in startSync while the loop is alive and still retrying.
    • The proxy's 503 during an outage is correct as it stands.
  2. Low: the claim "preserve interruption during shutdown" has no test. workspace.remove runs at the end of each test but nothing is asserted. A useful test would remove the workspace during the backoff sleep, then check that isSyncing is false and the adapter's target is not called again.
  3. Nit: the new comment in startSync sits above the exists check but describes the loop, so move it or drop it. Also, startSync resolves the target and the loop then resolves it again, so each start calls the adapter twice. That predates this PR, but both paths now handle local targets, so the first call could go.

Process

  • The compliance bot will auto-close this PR within two hours unless the description uses the PR template sections and links an issue (Fixes #N).

internetisalie
internetisalie commented Sep 24, 2026
View reviewed changes

@internetisalie internetisalie left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up review of 1d8ef99, done by reading the code. Tests were not run by the reviewer. Nothing blocking: the earlier findings are resolved.

Earlier finding Status
Medium: /sync/start restarts a listener that is still retrying and resets its backoff Fixed. startSync returns early whenever the workspace's sync fiber already exists.
Low: no test that interruption stops the retries Fixed. The new test removes the workspace during the first backoff sleep and asserts no further target calls.
Nit: comment in the wrong place Fixed.
Nit: each start resolves the target twice Still there. It predates this PR and doesn't need to block it.
Process: compliance bot auto-close Fixed. The description follows the template, Closes #2 points at an open issue, and the compliance and standards checks pass.

Why the early return is safe:

  • A FiberMap entry disappears when its fiber ends.
  • The loop now ends in only two cases: it resolves a local target, or it hits a failure it doesn't catch, which the startSync wrapper marks as error.
  • In both cases the fiber is gone, so a later /sync/start still restarts it.
  • A loop stuck in connecting or connected was never restarted before this PR either, so nothing that used to recover now doesn't.

Does the new test catch the bug? Yes.

  • Repeated start: under 363d929, the second startWorkspaceSyncing resolves the target a third time and restarts the loop, so expect(attempts).toBe(2) fails.
  • Removal: the loop's second attempt is due at about 1 s. The removal happens at about 0.1 s and the test waits 1.2 s after it, so a loop that kept running would be caught.
  • It can't detect a handler that swallows interrupts, because Effect re-interrupts a fiber that has been interrupted. That limits what the test proves; it isn't a problem with the code.

Remaining, not blocking

  1. Concurrent startSync race: already there, but this PR widened it.
    • Before: the FiberMap.has check ran after target resolution. Between that check and FiberMap.run, the only other step was a synchronous setStatus, so the window was essentially zero.
    • Now: the check runs before WorkspaceAdapterRuntime.target, so the window lasts as long as the adapter's target() promise, which can be an HTTP request.
    • When it bites: it takes two starts for the same workspace close together, such as create() and a /sync/start. The second FiberMap.run stops the first loop and replaces it, costing one extra connect. Nothing leaks.
    • Fix: check the fiber again just before FiberMap.run.
  2. error also means "retrying": already there.
    • The base loop already set error on a failed connection, then slept and retried.
    • This PR extends that meaning to target-resolution failures and stream failures.
    • Inside the code, whether the fiber exists now tells the two cases apart. Anything that reads only the status still can't tell "retrying" from "gave up".

@internetisalie internetisalie changed the title fix(opencode): recover remote workspace sync after proxy restart fix(opencode): recover remote workspace sync after restarts Sep 24, 2026
@internetisalie

internetisalie commented Sep 24, 2026

Copy link
Copy Markdown
Owner Author

Follow-up review of 53f8ffe (new since the last review: e00c029, 53f8ffe). Done by reading the code; tests were not run by the reviewer. CI's unit, e2e, typecheck and nix-eval jobs were still queued at review time.

Earlier item Status
Concurrent startSync race widened by the early FiberMap.has Fixed. startSync checks FiberMap.has again after target() resolves. On the remote path, the only step between that check and FiberMap.run is a synchronous setStatus, so the window is back to what it was before this PR.
error also means "retrying" Unchanged. It predates this PR and doesn't block it.
Each start resolves the target twice Unchanged. It predates this PR.

Does the concurrent-start test catch the race? Yes. Without the re-check, the second start's FiberMap.run replaces the first listener, which opens a second event stream, so expect(connections).toBe(1) fails.

Does the resume test catch the missing resume? Yes. Without resumeWorkspaceSyncing in the middleware, nothing starts sync for the persisted row, so the connected status event never fires and the wait fails. It uses a local adapter, so it covers the resume wiring, not the remote loop. The remote loop is covered by the earlier tests.

Findings

  1. Medium: a start that loses the race can leave a connected workspace in error, and requests to it then return 503.
    • The catch around WorkspaceAdapterRuntime.target calls setStatus(space.id, "error") before the new re-check. If the losing start's target() rejects after the winner has connected, it sets error and then returns at the re-check.
    • Once connected, the loop doesn't set status again until the stream ends. So the workspace stays error while its listener is healthy.
    • isSyncing returns exists && status !== "error", so workspace-routing.ts answers every proxied request with 503 broken sync connection until the stream next drops.
    • When it bites: two starts for the same workspace close together, such as create(), /sync/start, or the new resume on a project's first request. The winner's target() must succeed and the loser's must fail, for example during a transient adapter or proxy failure.
    • Fix: set error after the re-check. Have the catch return a marker instead of calling setStatus, then call setStatus(space.id, "error") only once FiberMap.has has come back false. On the normal path the result is unchanged, because the start that failed is also the one that goes on to run the loop.
    • Test: extend the concurrent-start test so the second target() rejects instead of resolving. Assert the status is still connected and a routed request is not a 503.
  2. Low: the recovery warning also fires on interruption. resumeWorkspaceSyncing catches the whole cause, so a client disconnecting during the row query logs "workspace sync recovery failed" and clears the project from resumedProjects. Clearing it is harmless, because the next request retries, but the warning misreports what happened. Re-raise interrupts the same way startSync does (Cause.hasInterrupts).
  3. Note: resumedProjects is never cleared after a successful resume. That fits how things work now: the fibers live in the Workspace service and outlast an instance reload, and a loop that has ended still needs /sync/start, as before this PR. It is worth a comment so nobody later reads the set as "currently syncing".

Nothing else is blocking. Once finding 1 is fixed and CI is green, I have no objection to merging.

@internetisalie

internetisalie commented Sep 25, 2026

Copy link
Copy Markdown
Owner Author

Follow-up review of 82cad0a (new since the last review: ab06172, plus the CI commits 359fe46, f605541, 82cad0a). Done by reading the code; tests were not run by the reviewer. All three findings from the previous review are resolved and nothing is blocking.

Previous finding Status
Medium: a start that loses the race leaves a connected workspace in error Fixed. The target() catch no longer writes status. error is set only after the re-check finds no listener, so a start that loses the race exits without touching status. On the path without a race, the result is the same as before: error is set, create() still sees it straight away, and the loop sets connecting as soon as it starts.
Low: recovery warning fires on interruption Fixed. Interrupts are re-raised. The project is still removed from resumedProjects first, so the next request retries, which is the right order.
Note: resumedProjects meaning Fixed. Comment added.

Does the test catch it? Yes. The concurrent-start test now has the second target() reject after the first listener has connected, and it asserts the status is connected and isSyncing is true. Under 53f8ffe, the losing start sets error before exiting, so both assertions fail. isSyncing is exactly what workspace-routing.ts checks before returning 503, so this also covers the routed-request case without needing a separate HTTP test.

The CI commits

  • On this fork, the runners switch from Blacksmith to GitHub-hosted. Upstream still gets Blacksmith, because the switch is conditioned on github.repository.
  • run-process.test.ts drops the wall-clock assertion and now asserts exitCode > 0. That still catches a hang. On a timeout, AppProcess.run fails with an AppProcessError that has no exitCode, and cli-process.ts turns that into -1, so a hung process fails the new assertion. The change applies to every platform, not just this fork, which is fine: the old duration assertion was the flaky part.
  • Note: the Windows install retry, the ripgrep warm-up and the Playwright worker limit only apply to Windows on this fork, and unit (windows) still fails. If Windows isn't a target here, dropping it from the fork's matrix would be simpler than keeping these workarounds.
  • Note: these three commits are CI infrastructure for the fork, not part of the workspace-sync fix. If this change is meant to go upstream, keep them out of that PR.

CI at 82cad0a: typecheck, nix-eval, unit (linux), e2e (linux), e2e (windows) and both PR-standards checks pass. unit (windows) fails.

@internetisalie
internetisalie merged commit be51a6b into dev Sep 25, 2026
7 of 8 checks passed
@internetisalie
internetisalie deleted the workspace-sync-recovery branch October 3, 2026 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remote workspace sync should recover after proxy restart

1 participant

@internetisalie