From 584e9e7d103a2ed843ec9d57abef495ca889c90d Mon Sep 17 00:00:00 2001 From: Alexander Ivanov Date: Fri, 28 Aug 2026 10:36:25 +0300 Subject: [PATCH 1/2] Add ADR 0010 and design for cross-host workspace lease MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR 0004's "one mutating run per workspace" invariant is enforced only in-memory per process (WorkbenchProcessScheduler.mutationLocked, WorkbenchRunJournal.writeQueue). Two hosts (VS Code extension + standalone server, both legitimate per ADR 0001 decision 2) opened on the same workspace can each believe they are the sole mutator, causing lost journal updates. This adds ADR 0010 (advisory lease, v1: refuse-immediately, no CAS/observer mode for now) plus the OpenSpec change proposal/design/tasks/ specs. Bundles the wire-contract COMMAND_KINDS duplication fix (wire.ts vs protocol.ts) into the same change since it touches the same files. No implementation yet — pausing here per plan for design review before writing the lease/scheduler code itself. Co-Authored-By: Claude Sonnet 5 --- docs/adr/0010-cross-host-workspace-lease.md | 151 ++++++++++++++++++ .../.openspec.yaml | 2 + .../design.md | 146 +++++++++++++++++ .../proposal.md | 81 ++++++++++ .../specs/execution-core/spec.md | 16 ++ .../specs/persistent-workbench-runs/spec.md | 44 +++++ .../tasks.md | 105 ++++++++++++ 7 files changed, 545 insertions(+) create mode 100644 docs/adr/0010-cross-host-workspace-lease.md create mode 100644 openspec/changes/2026-08-28-cross-host-workspace-lease/.openspec.yaml create mode 100644 openspec/changes/2026-08-28-cross-host-workspace-lease/design.md create mode 100644 openspec/changes/2026-08-28-cross-host-workspace-lease/proposal.md create mode 100644 openspec/changes/2026-08-28-cross-host-workspace-lease/specs/execution-core/spec.md create mode 100644 openspec/changes/2026-08-28-cross-host-workspace-lease/specs/persistent-workbench-runs/spec.md create mode 100644 openspec/changes/2026-08-28-cross-host-workspace-lease/tasks.md diff --git a/docs/adr/0010-cross-host-workspace-lease.md b/docs/adr/0010-cross-host-workspace-lease.md new file mode 100644 index 00000000..3ba2b36c --- /dev/null +++ b/docs/adr/0010-cross-host-workspace-lease.md @@ -0,0 +1,151 @@ +# 0010: Cross-Host Workspace Lease (Advisory Lock, v1) + +Status: Proposed + +Date: 2026-08-28 + +## Context + +ADR 0004 decision 4 requires the Workbench to run at most one mutating +process per workspace until mutations are isolated by worktrees. That +invariant is enforced entirely in-memory: `WorkbenchProcessScheduler` +(`packages/core/src/process-scheduler.ts`) holds a private +`mutationLocked` boolean per instance, and `WorkbenchRunJournal` +(`packages/core/src/workbench-run-journal.ts`) serializes writes with a +private `writeQueue` promise chain per instance. Both are scoped to one +process's one instance. + +ADR 0001 decision 2 keeps the local server available as an optional +transport alongside the VS Code extension's direct-import default "when +standalone UI parity is more important than localhost lifecycle +simplicity." That means the same workspace root can legitimately be open +in two separate Node processes at once: a VS Code extension host and a +standalone server. Each constructs its own `WorkbenchProcessScheduler` +and `WorkbenchRunJournal` for that root (`packages/extension/src/ +extension.ts` activate(); `packages/core/src/workbench-recovery.ts` +`WorkbenchRecoveryService.open()`, used by `packages/server/src/ +server.ts`'s `resolveRecoveryService`). Neither process is aware of the +other's in-memory mutation lock, and the journal's write-then-rename +replacement (ADR 0004 decision 1) prevents a torn write but not a lost +update: whichever process calls `journal.save()` last overwrites the +file with only its own in-memory view of process history, discarding +whatever the other process recorded concurrently. + +Separately, review of the wire boundary found `packages/server/src/ +wire.ts`'s `COMMAND_KINDS` array hand-duplicating `CommandKind` from +`packages/core/src/protocol.ts` (the one place ADR 0001 decision 3 +designates as the protocol's source of truth). A new command kind added +to core silently fails server-side shape validation until someone +remembers to update the copy in `wire.ts`. + +## Decision + +1. **A workspace-local lease file, not a wider protocol version.** Core + owns a versioned lease document at + `/.openspec-ui/workspace.lease.json`, written with the + same write-then-rename atomic replacement the journal already uses + (ADR 0004 decision 1). It records a random per-activation holder id, + a host kind (`vscode-extension` / `standalone-server`), hostname, + pid, and a heartbeat timestamp. +2. **The lease is acquired only around a mutating run, not for the + lifetime of a host.** A host attempts to acquire or renew the lease + exactly where `WorkbenchProcessScheduler.run()` already sets + `mutationLocked = true`, and releases it in the same `finally` block + that clears it today. Read-only processes never consult the lease, + matching the existing "read-only runs remain concurrent" behavior + unchanged. +3. **Rejection is immediate, not queued.** If another host's lease is + present and not stale, the requesting host's mutating run fails right + away with a `failed` process record naming the other host (kind, + hostname, pid, how long since its last heartbeat) — it does not sit + queued the way two same-process mutating runs do today, because the + other host may hold the workspace indefinitely and the user should + decide, not wait silently. +4. **Staleness, not liveness, governs reclamation.** The lease has no way + to know whether its holder crashed. A heartbeat renewed periodically + while a mutating run is active is compared against a fixed staleness + window (a small constant multiple of the heartbeat interval); a host + whose lease has gone stale is treated as no longer active, and the + next host to request a mutating run reclaims the lease and surfaces + that reclamation to the user rather than reporting it as a normal + acquisition. +5. **Optional, backward-compatible integration.** `WorkbenchProcessScheduler` + accepts an optional lease dependency; when absent (every existing unit + test that constructs it with no workspace root) its behavior is + exactly what it is today — in-memory only, no filesystem access. Both + `WorkbenchRecoveryService.open()` and the extension's `activate()` + construct and pass one when a real workspace root is available. +6. **Fix the wire contract duplication as part of the same change.** + `packages/core/src/protocol.ts` exports the `CommandKind` list as a + runtime array (`COMMAND_KINDS`); `packages/server/src/wire.ts` imports + it instead of declaring its own copy. This is not a protocol shape + change and needs no version bump — it removes a manual-sync hazard + discovered while researching this ADR's own scope, in the same files + this change already touches. + +## Rejected Alternatives + +### Full revision/CAS journal merge with an observer mode for the second host + +Rejected for v1. The actually hard part — merging two hosts' concurrent +edits to the same process list, and a live read-only view for a host +that isn't the lease holder — has no existing consumer yet: nothing in +either delivery target today lets a user watch another host's run in +progress. Building a merge algorithm and a UI concept that doesn't exist +anywhere else in this codebase is a materially larger change than the +lost-update problem actually in front of us, which a single-mutator +lease already closes. Revisit if a real workflow needs two hosts +mutating the same workspace concurrently, not just one active and one +blocked. + +### `protocolVersion` / capability handshake between hosts + +Rejected for v1. This matters once hosts running genuinely different +released versions are expected to interoperate against the same lease +and journal. The lease document is versioned exactly like the journal +(ADR 0004 decision 1), and the common case — one user, one checkout, +two host processes — has both hosts on the same package versions +already. A handshake with nothing on the other end to negotiate against +is speculative scope, not a fix for an observed gap. + +### An OS-level file lock (`flock`/`proper-lockfile`) instead of an +application-level heartbeat lease + +Rejected. This would add a new dependency and platform-specific locking +semantics (Windows advisory locks behave differently from POSIX flock) +to solve a problem the existing write-then-rename atomic-replace +pattern already solves at the storage layer for the journal. A +heartbeat lease file, versioned and atomically replaced the same way, +keeps the dependency-free philosophy ADR 0004 and ADR 0006 already +established, and needs no per-platform behavior to reason about. + +### Queue the second host's mutating run until the lease frees, mirroring same-process queueing + +Rejected. Same-process queueing works because the same scheduler +instance will eventually drain the queue when its own mutation lock +clears — that is guaranteed to happen. A lease held by a different, +independently-running host has no such guarantee: it might hold the +workspace for the rest of the day. Queuing indefinitely against another +process's unknown future would surprise a user who has no visibility +into why their run never starts; an immediate, actionable failure keeps +the same fail-fast philosophy the security model (ADR 0001 decision 4) +already uses elsewhere in this codebase. + +## Consequences + +- A second host opening the same workspace while another is actively + mutating it gets a clear, immediate rejection naming the other host, + instead of silently racing it or queuing forever. +- The "one mutating run per workspace" invariant from ADR 0004 now holds + across host processes, not only within one — refining, not reopening, + that decision. +- Recovery from a crashed (not merely slow) lease holder depends on the + staleness window elapsing; a host that hangs without crashing blocks + the other host for up to that window after it stops actually making + progress. +- True concurrent mutation from two hosts is still out of scope — that + remains the worktree-isolation work ADR 0004 already named as the + eventual path, tracked separately and orthogonally to this ADR. +- `WorkbenchProcessScheduler`'s internal control flow around `run()` + gains an asynchronous gate for mutating processes only; read-only + scheduling stays synchronous and unaffected. diff --git a/openspec/changes/2026-08-28-cross-host-workspace-lease/.openspec.yaml b/openspec/changes/2026-08-28-cross-host-workspace-lease/.openspec.yaml new file mode 100644 index 00000000..7f2cf9bc --- /dev/null +++ b/openspec/changes/2026-08-28-cross-host-workspace-lease/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-28 diff --git a/openspec/changes/2026-08-28-cross-host-workspace-lease/design.md b/openspec/changes/2026-08-28-cross-host-workspace-lease/design.md new file mode 100644 index 00000000..ecdec2b4 --- /dev/null +++ b/openspec/changes/2026-08-28-cross-host-workspace-lease/design.md @@ -0,0 +1,146 @@ +## Context + +Researched directly against this codebase (see ADR 0010 for the fuller +citation trail): `WorkbenchProcessScheduler.mutationLocked` +(`process-scheduler.ts:54`) and `WorkbenchRunJournal.writeQueue` +(`workbench-run-journal.ts:74`) are both private fields of one +in-process instance. Two construction sites exist for the same +workspace root: `packages/extension/src/extension.ts` `activate()` +builds a journal + scheduler pair directly; `packages/core/src/ +workbench-recovery.ts` `WorkbenchRecoveryService.open()`/`initialize()` +builds its own pair internally and is the one `packages/server/src/ +server.ts`'s `resolveRecoveryService` uses. Every existing scheduler +unit test (`process-scheduler.test.ts`, `implementation-sessions.test.ts`) +constructs `new WorkbenchProcessScheduler()` or +`new WorkbenchProcessScheduler([...])` with a single positional array +argument and no real filesystem workspace. + +## Goals / Non-Goals + +**Goals:** +- Two host processes open on the same workspace root can never both + believe they are the sole mutator at the same time. +- A rejected mutating run tells the user which other host is holding the + workspace, not just that it failed. +- No existing scheduler/journal test needs a workspace root or lease + fixture it doesn't already have — the lease is additive and optional. +- Reuse the journal's write-then-rename atomic replacement pattern + exactly; no new dependency. + +**Non-Goals:** +- Not merging concurrent journal writes from two hosts (revision/CAS) — + the lease prevents two hosts from mutating concurrently in the first + place, so there is nothing to merge for v1. +- Not building an "observer mode" live view of another host's active + run — a rejected host still reads the journal exactly as it does + today (a static snapshot at its own last load), just refused a new + mutating run. +- Not adding a `protocolVersion`/capability handshake — see ADR 0010's + rejected alternatives. +- Not making the lease's staleness window user-configurable — it is an + internal constant for this iteration, not a new setting. +- Not covering true concurrent mutation from two hosts — that is the + separately tracked worktree-isolation work ADR 0004 already named. + +## Decisions + +### Gate only the mutating branch of `run()`, not scheduler construction or read-only processes + +`canRun()`/`drain()` stay synchronous and untouched for read-only +processes. Only the mutating branch of `run()` — where +`mutationLocked = true` is set today — becomes asynchronous, acquiring +or renewing the lease before proceeding. This is the smallest change +that closes the actual gap (two hosts each racing to mutate), and +matches ADR 0004's existing distinction that read-only runs are never +subject to the mutation lock. + +### Reject immediately as a `failed` process, not a queued wait or a synchronous throw + +Two options were considered for how a blocked mutating run surfaces: +queue it the way two same-process mutating runs queue against each +other, or throw synchronously the way `start()` already does for a +duplicate id. Both are rejected — see ADR 0010's "queue the second +host" rejection for why indefinite queuing is wrong, and a synchronous +throw would require special-case handling everywhere `start()` is +called, unlike every other terminal outcome this scheduler already +models as a `WorkbenchProcess` state. Instead, the process is created +exactly as today, transitions straight to `running` is skipped, and it +finishes in the `failed` state with `error` naming the other host +(`hostKind`, `hostname`, `pid`, and how long since its last heartbeat). +This reuses the Processes UI both hosts already have for surfacing +failures, with no new UI concept. + +### Lease acquisition is scoped to the mutating run's lifetime, not the host's + +The lease is acquired when a mutating `run()` begins and released in the +same `finally` block that already flips `mutationLocked = false`. A host +that never starts a mutating run never touches the lease file at all — +this keeps a host that only ever reads (e.g. a `status`/`list` client) +from contending for a lease it doesn't need, and keeps the lease's +lifetime symmetric with the existing in-memory flag it extends across +processes. + +### Lease document shape and file location + +```ts +interface WorkspaceLeaseDocument { + version: 1; + holderId: string; // randomUUID, one per scheduler-with-lease instance + hostKind: "vscode-extension" | "standalone-server"; + hostname: string; + pid: number; + acquiredAt: string; // ISO 8601 + heartbeatAt: string; // ISO 8601, renewed while the mutating run is active +} +``` + +Stored at `/.openspec-ui/workspace.lease.json`, next to +`workbench-runs.json`, using the exact write-then-rename-with-EEXIST/EPERM-retry +sequence `WorkbenchRunJournal.write()` already implements — not a second, +subtly different atomic-write implementation. + +### Heartbeat interval and staleness window are fixed constants + +A mutating run renews the lease heartbeat every 5 seconds while active; +a lease is considered stale once its `heartbeatAt` is more than 20 +seconds old (4x the interval — tolerant of a slow disk or a GC pause +without being so wide that a genuinely crashed host blocks the other one +for long). These are internal constants for this iteration (see +Non-Goals); a future change can expose them if real usage shows the +defaults are wrong. + +### `WorkspaceLeaseManager` is a new core module, injected — not baked into `WorkbenchProcessScheduler`'s constructor signature as a required argument + +Making the lease mandatory would force every existing scheduler test +(`process-scheduler.test.ts`, `implementation-sessions.test.ts`) to set +up a temp directory and a real lease file just to exercise unrelated +queue-ordering behavior that has nothing to do with cross-host +concurrency. An optional second constructor parameter, defaulting to a +no-op in-memory-only behavior when absent, keeps every current call site +and test compiling and passing unchanged, and matches how +`WorkbenchRunJournalOptions` is already optional on the journal side. + +## Risks / Trade-offs + +- **[Risk]** Introducing an asynchronous gate into `WorkbenchProcessScheduler.run()` + changes its internal control flow for the mutating branch only; + `drain()`'s synchronous iteration and the read-only path must not + regress. → **Mitigation**: this is the one piece of the change with + real implementation risk and needs explicit before/after test coverage + on `canRun`/`drain`/`run` ordering (task 4.1), not just the new + cross-host scenarios. +- **[Risk]** A host that hangs without crashing (deadlocked, not + terminated) still holds a live-looking lease and blocks the other host + for up to the staleness window after it stops making real progress — + same trade-off ADR 0010 already accepts explicitly. +- **[Risk]** Clock skew between two machines is not a concern here + (single-machine, two local processes, per ADR 0001/0005's local-only + transport model), but a suspended VM or a system sleep/resume could + make a heartbeat appear stale prematurely. → **Mitigation**: reclaiming + a stale lease is always disclosed to the user (never silent), so a + false reclamation is visible and recoverable, not a silent data-loss + event. +- **[Risk]** `.openspec-ui/workspace.lease.json` becoming stale garbage + after an unclean host exit is expected, not a bug — the next mutating + run's staleness check cleans it up functionally (overwrites it) even + though the file itself isn't deleted proactively. diff --git a/openspec/changes/2026-08-28-cross-host-workspace-lease/proposal.md b/openspec/changes/2026-08-28-cross-host-workspace-lease/proposal.md new file mode 100644 index 00000000..37ed6220 --- /dev/null +++ b/openspec/changes/2026-08-28-cross-host-workspace-lease/proposal.md @@ -0,0 +1,81 @@ +# Change: Cross-Host Workspace Lease + +## Why + +ADR 0004 decision 4 requires at most one mutating Workbench process per +workspace, but the enforcement is a private, per-instance boolean: +`WorkbenchProcessScheduler.mutationLocked` +(`packages/core/src/process-scheduler.ts:54`). `WorkbenchRunJournal`'s +write serialization (`writeQueue`, +`packages/core/src/workbench-run-journal.ts:74`) is equally +process-local. Both the VS Code extension (`packages/extension/src/ +extension.ts` `activate()`) and the standalone server (`packages/core/ +src/workbench-recovery.ts` `WorkbenchRecoveryService.open()`, used by +`packages/server/src/server.ts`) construct their own scheduler and +journal for the same workspace root — which ADR 0001 decision 2 +explicitly allows to run at the same time as an optional transport +alongside the extension's direct-import default. Two hosts on the same +workspace can each believe they are the sole mutator and start +concurrent mutating runs, and whichever host's journal write lands last +silently discards the other host's recorded process history (a lost +update, not merely a torn write — write-then-rename already prevents +torn writes, per ADR 0004 decision 1). + +Separately, `packages/server/src/wire.ts`'s `COMMAND_KINDS` array +duplicates `CommandKind` from `packages/core/src/protocol.ts:8`, the one +place ADR 0001 decision 3 designates as the command protocol's source of +truth — a new command kind added to core silently fails this +hand-maintained copy until someone remembers to update it. + +See ADR 0010 for the full decision and rejected alternatives. + +## What Changes + +- Add `packages/core/src/workspace-lease.ts`: a versioned lease document + at `/.openspec-ui/workspace.lease.json` (write-then-rename + atomic replacement, same pattern as `WorkbenchRunJournal`), recording a + random per-activation holder id, host kind, hostname, pid, and a + renewed heartbeat timestamp. +- `WorkbenchProcessScheduler` (`process-scheduler.ts`) accepts an + optional lease dependency. When a mutating process is about to run, it + attempts to acquire or renew the lease; when present and held by + another, non-stale holder, the run fails immediately as `failed` with + a reason naming the other host, instead of starting. The lease is + released in the same `finally` block that already clears + `mutationLocked`. Read-only processes are unaffected. +- `WorkbenchRecoveryService.open()` (`workbench-recovery.ts`) and the + extension's `activate()` (`extension.ts`) each construct a + `WorkspaceLeaseManager` for their real workspace root and pass it to + their scheduler. No other call site changes: every existing test that + constructs `WorkbenchProcessScheduler()` with no lease keeps its + current in-memory-only behavior. +- Export `COMMAND_KINDS` from `packages/core/src/protocol.ts`; + `packages/server/src/wire.ts` imports it instead of redeclaring the + literal list. +- Add `docs/adr/0010-cross-host-workspace-lease.md`. + +## Capabilities + +### New Capabilities + +(none) + +### Modified Capabilities + +- `persistent-workbench-runs`: extends "Workspace mutation isolation" to + hold across host processes, not only within one, via the workspace + lease. +- `execution-core`: adds a requirement that command kind validation has + exactly one source of truth in core. + +## Impact + +- `packages/core/src/workspace-lease.ts` (new) +- `packages/core/src/process-scheduler.ts` +- `packages/core/src/protocol.ts` +- `packages/core/src/workbench-recovery.ts` +- `packages/core/src/index.ts` (export `WorkspaceLeaseManager`) +- `packages/server/src/wire.ts` +- `packages/extension/src/extension.ts` +- `docs/adr/0010-cross-host-workspace-lease.md` (new) +- `.changeset/*.md` (new changeset file) diff --git a/openspec/changes/2026-08-28-cross-host-workspace-lease/specs/execution-core/spec.md b/openspec/changes/2026-08-28-cross-host-workspace-lease/specs/execution-core/spec.md new file mode 100644 index 00000000..f13a363c --- /dev/null +++ b/openspec/changes/2026-08-28-cross-host-workspace-lease/specs/execution-core/spec.md @@ -0,0 +1,16 @@ +## ADDED Requirements + +### Requirement: Command kind validation has one source of truth + +The system SHALL define the set of valid command kinds in exactly one +place in `packages/core`. Any transport-boundary shape check performed +by a delivery adapter (for example, the server's incoming-message +validation) SHALL import that same set rather than declaring its own +list of command kind literals. + +#### Scenario: Core adds a new command kind + +- **WHEN** a new command kind is added to the core protocol +- **THEN** every adapter's shape-check-based validation recognizes it as + valid without a matching hand-edit to a separately maintained literal + list diff --git a/openspec/changes/2026-08-28-cross-host-workspace-lease/specs/persistent-workbench-runs/spec.md b/openspec/changes/2026-08-28-cross-host-workspace-lease/specs/persistent-workbench-runs/spec.md new file mode 100644 index 00000000..6d9c726a --- /dev/null +++ b/openspec/changes/2026-08-28-cross-host-workspace-lease/specs/persistent-workbench-runs/spec.md @@ -0,0 +1,44 @@ +## MODIFIED Requirements + +### Requirement: Workspace mutation isolation + +The Workbench SHALL run at most one mutating process in a workspace at a +time until mutations have independent filesystem isolation, whether both +attempts originate in the same host process or in two different host +processes (for example, a VS Code extension host and a standalone +server) pointed at the same workspace root. Cross-process isolation +SHALL be enforced by a versioned, workspace-local lease file that the +running host renews while a mutating process is active and releases +when that process reaches a terminal state; a lease whose last renewal +is older than a bounded staleness window SHALL be treated as no longer +held. Read-only runs SHALL remain unaffected by the lease. + +#### Scenario: Different changes mutate the same workspace + +- **WHEN** mutating runs for two different changes are requested +- **THEN** the second run remains queued until the first reaches a terminal + state +- **AND** read-only runs may execute concurrently + +#### Scenario: A second host attempts to mutate the same workspace + +- **WHEN** a mutating run is requested on a host that does not hold the + current workspace lease, and that lease is not stale +- **THEN** the run fails immediately, reporting which other host + currently holds the lease +- **AND** no process record is left queued waiting for the other host to + finish + +#### Scenario: The lease holder releases on completion + +- **WHEN** a mutating run reaches a terminal state +- **THEN** its host releases the workspace lease +- **AND** a subsequent mutating run, from either host, may acquire it + immediately + +#### Scenario: The previous lease holder is no longer active + +- **WHEN** a host requests a mutating run and the existing lease's last + renewal is older than the staleness window +- **THEN** the host acquires the lease and proceeds +- **AND** the reclamation is disclosed rather than silently overwritten diff --git a/openspec/changes/2026-08-28-cross-host-workspace-lease/tasks.md b/openspec/changes/2026-08-28-cross-host-workspace-lease/tasks.md new file mode 100644 index 00000000..34c384be --- /dev/null +++ b/openspec/changes/2026-08-28-cross-host-workspace-lease/tasks.md @@ -0,0 +1,105 @@ +## 1. Core: workspace lease module + +- [ ] 1.1 Add `packages/core/src/workspace-lease.ts`: `WorkspaceLeaseDocument` + (version 1, `holderId`/`hostKind`/`hostname`/`pid`/`acquiredAt`/`heartbeatAt`) + and a `WorkspaceLeaseManager` class reading/writing + `/.openspec-ui/workspace.lease.json` via write-then-rename atomic + replacement, mirroring `WorkbenchRunJournal.write()`'s + temp-file-then-rename-with-EEXIST/EPERM-retry sequence exactly (not a + second, independent implementation). +- [ ] 1.2 `WorkspaceLeaseManager.acquireOrRenew()`: succeeds if no lease + exists, the existing lease belongs to this manager's own `holderId`, or + the existing lease's `heartbeatAt` is older than the staleness window + (20s); otherwise returns the current holder's details without writing. +- [ ] 1.3 `WorkspaceLeaseManager.release()`: clears the lease file only if + it is currently held by this manager's own `holderId`. +- [ ] 1.4 Export `WorkspaceLeaseManager` and its types from + `packages/core/src/index.ts`. + +## 2. Core: scheduler integration + +- [ ] 2.1 `WorkbenchProcessScheduler`'s constructor accepts an optional + second parameter carrying a lease manager; omitted, behavior is + unchanged from today (in-memory `mutationLocked` only, no filesystem + access) — every existing call site and test keeps compiling and + passing with no changes. +- [ ] 2.2 In `run()`'s mutating branch, before setting + `mutationLocked = true`: if a lease manager is present, call + `acquireOrRenew()`. If it reports another live holder, finish the + process as `failed` immediately (never transitioning through + `running`), with `error` naming the other host's `hostKind`/`hostname`/ + `pid` and time since its last heartbeat — do not queue it. +- [ ] 2.3 While a mutating process is `running` under a lease manager, + renew the lease every 5s until the process finishes. +- [ ] 2.4 In the mutating branch's existing `finally` block (alongside + `mutationLocked = false`), release the lease if one was acquired. +- [ ] 2.5 When `acquireOrRenew()` reclaims a stale lease from a different + `holderId`, surface that reclamation (not a plain acquisition) through + the existing `report`/progress channel so the host can disclose it. + +## 3. Host wiring + +- [ ] 3.1 `WorkbenchRecoveryService.open()` + (`packages/core/src/workbench-recovery.ts`) constructs a + `WorkspaceLeaseManager` for `root` with `hostKind: "standalone-server"` + and passes it to its internal scheduler (`initialize()` and + `cleanupBefore()`'s scheduler reconstruction). +- [ ] 3.2 `packages/extension/src/extension.ts` `activate()` constructs a + `WorkspaceLeaseManager` for `workspaceRoot` with + `hostKind: "vscode-extension"` (only when `workspaceRoot` is defined, + matching the existing journal-construction guard) and passes it to the + scheduler it constructs directly. +- [ ] 3.3 Confirm `deactivate()` (`extension.ts:206`) and the standalone + server's `close()` (`packages/server/src/server.ts:268`) do not need to + release a lease themselves — release already happens per-run in the + scheduler's `finally` block (2.4), so no lease is ever held across a + clean shutdown with no active mutating run. + +## 4. Wire contract fix + +- [ ] 4.1 Export `COMMAND_KINDS` (a `readonly CommandKind[]`) from + `packages/core/src/protocol.ts`, next to the `CommandKind` type it + enumerates. +- [ ] 4.2 `packages/server/src/wire.ts`'s `isCommandLike` imports and uses + that exported array instead of its own locally declared + `COMMAND_KINDS`. + +## 5. Tests + +- [ ] 5.1 `workspace-lease.test.ts`: acquire when absent, renew by the + same holder, refuse a live foreign holder, reclaim a stale foreign + holder, release only when self-held, and the write-then-rename + replacement survives a concurrent read (mirroring + `workbench-run-journal.test.ts`'s existing atomicity coverage style). +- [ ] 5.2 `process-scheduler.test.ts`: add cases for a lease-backed + scheduler — foreign live lease rejects a mutating run as `failed` + without queuing; own or absent lease behaves exactly as before; stale + foreign lease is reclaimed and disclosed. Explicitly re-run the + existing no-lease test cases unchanged to confirm the optional + parameter is truly backward compatible (risk called out in design.md). +- [ ] 5.3 A cross-process integration test: two `WorkbenchRecoveryService` + instances (or scheduler + lease manager pairs) opened against the same + temp workspace root in the same test process, simulating two hosts — + confirms the second cannot start a mutating run while the first's + lease is live, and can once the first finishes or its lease goes + stale. +- [ ] 5.4 `wire.test.ts` (or equivalent): `isCommandLike` still accepts + every `CommandKind`, now sourced from the shared `COMMAND_KINDS` export + rather than a local copy. + +## 6. Verification + +- [ ] 6.1 `npm run typecheck` and `npm run lint` (including + `lint:english`) pass workspace-wide. +- [ ] 6.2 `npm run test` passes workspace-wide, including the new + `workspace-lease.test.ts`, the extended `process-scheduler.test.ts`, + and `wire.test.ts`. +- [ ] 6.3 Manual smoke test: start the standalone server and the VS Code + extension against the same real workspace; start a mutating run + (`implement`) from one, confirm the other's attempt to start a + mutating run fails immediately naming the first host; let the first + finish and confirm the second can then run. +- [ ] 6.4 Propose a changeset (`npx changeset`) for `@openspec-ui/core` + (new lease behavior), `@openspec-ui/server`, and + `openspec-ui-vscode` (minor: new capability, no breaking change). +- [ ] 6.5 Run `openspec change validate --strict cross-host-workspace-lease`. From 2554807aaaf72c822d3b62d22c8440b857865460 Mon Sep 17 00:00:00 2001 From: Alexander Ivanov Date: Fri, 28 Aug 2026 12:10:25 +0300 Subject: [PATCH 2/2] Add cross-host workspace lease for mutating Workbench runs (ADR 0010) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WorkbenchProcessScheduler's mutation lock and WorkbenchRunJournal's write serialization are both private, per-process state. A VS Code extension and a standalone server can legitimately be open on the same workspace at once (ADR 0001 decision 2), each believing it is the sole mutator, causing lost journal updates. Adds a versioned, file-based workspace lease (write-then-rename, same pattern as the journal): a mutating run acquires it, renews it while active, and releases it on completion; a competing host is refused immediately with a message naming the current holder, and a stale lease (crashed holder) is reclaimed and disclosed. Also fixes wire.ts's COMMAND_KINDS duplicating core's CommandKind list (same files this change already touches), and closes a larger pre-existing gap found while implementing this: the standalone server's own `implement` execution over WebSocket never went through the scheduler at all, so ADR 0004's mutation isolation was unenforced there even same-host. Both are now scheduler-gated the same way. Adds workspace-lease.ts + tests, extends process-scheduler.ts (including a fix so `completion` only resolves after lease release, not before, per a real race the tests caught), wires the lease into WorkbenchRecoveryService and the extension's activate(), and routes the server's implement command through WorkbenchRecoveryService.runMutating() (no checkpoint capture yet — mutation exclusivity only, a scoped-down first pass; see design.md Non-Goals). Co-Authored-By: Claude Sonnet 5 --- docs/adr/0010-cross-host-workspace-lease.md | 44 ++++- .../design.md | 74 +++++++ .../proposal.md | 20 ++ .../tasks.md | 165 ++++++++++------ packages/core/CHANGELOG.md | 6 + packages/core/package.json | 2 +- packages/core/src/index.ts | 1 + packages/core/src/process-scheduler.test.ts | 121 +++++++++++- packages/core/src/process-scheduler.ts | 57 +++++- packages/core/src/protocol.ts | 14 ++ packages/core/src/workbench-recovery.ts | 29 ++- packages/core/src/workspace-lease.test.ts | 116 +++++++++++ packages/core/src/workspace-lease.ts | 180 ++++++++++++++++++ packages/extension/CHANGELOG.md | 12 ++ packages/extension/package.json | 2 +- packages/extension/src/extension.ts | 6 +- packages/server/CHANGELOG.md | 11 ++ packages/server/package.json | 2 +- packages/server/src/server.test.ts | 95 ++++++++- packages/server/src/server.ts | 2 +- packages/server/src/websocket.ts | 88 ++++++++- packages/server/src/wire.test.ts | 24 +++ packages/server/src/wire.ts | 13 +- 23 files changed, 993 insertions(+), 91 deletions(-) create mode 100644 packages/core/src/workspace-lease.test.ts create mode 100644 packages/core/src/workspace-lease.ts create mode 100644 packages/server/src/wire.test.ts diff --git a/docs/adr/0010-cross-host-workspace-lease.md b/docs/adr/0010-cross-host-workspace-lease.md index 3ba2b36c..5b64de8f 100644 --- a/docs/adr/0010-cross-host-workspace-lease.md +++ b/docs/adr/0010-cross-host-workspace-lease.md @@ -38,6 +38,24 @@ designates as the protocol's source of truth). A new command kind added to core silently fails server-side shape validation until someone remembers to update the copy in `wire.ts`. +**Correction discovered during implementation:** the paragraph above +assumes the standalone server already routes its own mutating command +execution through `WorkbenchProcessScheduler`, the way the extension's +`ImplementationSessionManager` does. It does not. `packages/server/src/ +websocket.ts`'s `handleSocketMessage`/`streamRun` called +`runner.run(command)` directly for every command kind, including +`implement` — never touching `WorkbenchRecoveryService`'s scheduler, never +capturing a checkpoint, never persisting to the journal. `packages/ +server/src/rest.ts` has no `plan`/`implement`/`review` handling either. +`WorkbenchRecoveryService`'s scheduler was populated only from whatever a +journal file already contained (in practice, only entries the extension +had written there) — the server itself never called `scheduler.start()`. +This means ADR 0004 decision 4's mutation isolation was not enforced by +the standalone server at all, same-host, before this change — a +pre-existing gap distinct from, and larger than, the cross-host +coordination problem this ADR set out to solve. Decision 7 below closes +it as part of this same change. + ## Decision 1. **A workspace-local lease file, not a wider protocol version.** Core @@ -82,6 +100,20 @@ remembers to update the copy in `wire.ts`. change and needs no version bump — it removes a manual-sync hazard discovered while researching this ADR's own scope, in the same files this change already touches. +7. **Route the standalone server's `implement` execution through the + scheduler too, closing the pre-existing same-host gap above — without + checkpoint capture.** `packages/server/src/websocket.ts`'s `streamRun` + calls `WorkbenchRecoveryService.runMutating()` for `implement` commands + specifically (every other command kind is unaffected, exactly as + before); its `execute` callback streams the `AgentRunner`'s events to + the socket as they occur. A lease conflict now produces a `failed` + `Event` on the socket (synthesized from the scheduler process's + `error`, since a blocked run's `execute` callback — and therefore the + `AgentRunner` — never ran, so nothing was ever sent for that attempt). + Checkpoint capture (and therefore rollback) for these runs stays out of + scope for this pass: mutation exclusivity does not require it, and + bundling it in would reopen the "standalone parity" scope ADR 0004 + decision 7 already deferred as separate follow-up work. ## Rejected Alternatives @@ -108,8 +140,7 @@ two host processes — has both hosts on the same package versions already. A handshake with nothing on the other end to negotiate against is speculative scope, not a fix for an observed gap. -### An OS-level file lock (`flock`/`proper-lockfile`) instead of an -application-level heartbeat lease +### An OS-level file lock (`flock`/`proper-lockfile`) instead of an application-level heartbeat lease Rejected. This would add a new dependency and platform-specific locking semantics (Windows advisory locks behave differently from POSIX flock) @@ -149,3 +180,12 @@ already uses elsewhere in this codebase. - `WorkbenchProcessScheduler`'s internal control flow around `run()` gains an asynchronous gate for mutating processes only; read-only scheduling stays synchronous and unaffected. +- The standalone server's `implement` command execution is now actually + gated (same-host and cross-host) instead of unconditionally bypassing + the scheduler — closing the pre-existing same-host gap this ADR found, + not just the cross-host one it set out to solve. +- Standalone-initiated `implement` runs still have no rollback (no + checkpoint is captured for them), and a server crash mid-run is not + recoverable as `interrupted` with a reviewable delta — only mutation + exclusivity is in scope here. Full standalone execution-time parity + (checkpoint capture on the WS path) remains separate follow-up work. diff --git a/openspec/changes/2026-08-28-cross-host-workspace-lease/design.md b/openspec/changes/2026-08-28-cross-host-workspace-lease/design.md index ecdec2b4..148bbb0f 100644 --- a/openspec/changes/2026-08-28-cross-host-workspace-lease/design.md +++ b/openspec/changes/2026-08-28-cross-host-workspace-lease/design.md @@ -15,6 +15,18 @@ constructs `new WorkbenchProcessScheduler()` or `new WorkbenchProcessScheduler([...])` with a single positional array argument and no real filesystem workspace. +**Discovered mid-implementation:** `WorkbenchRecoveryService`'s scheduler, +as originally wired, was never actually reachable from the standalone +server's live command execution. `packages/server/src/websocket.ts`'s +`streamRun` called `runner.run(command)` directly for every command kind +— `WorkbenchRecoveryService.list()`/`rollback()`/`cleanupBefore()` only +ever read/mutated a scheduler populated from whatever the journal file +already contained. A lease wired only into that scheduler would have +protected `WorkbenchRecoveryService`'s own rollback/cleanup calls, but not +the actual "someone clicks Implement in the browser" case — the realistic +cross-host race ADR 0010 exists for. See ADR 0010's Context for the fuller +account; this design was extended (not restarted) once that was found. + ## Goals / Non-Goals **Goals:** @@ -26,6 +38,9 @@ argument and no real filesystem workspace. fixture it doesn't already have — the lease is additive and optional. - Reuse the journal's write-then-rename atomic replacement pattern exactly; no new dependency. +- The standalone server's own live `implement` execution is actually + gated — not only `WorkbenchRecoveryService`'s rollback/cleanup calls, + which were never the realistic contention point. **Non-Goals:** - Not merging concurrent journal writes from two hosts (revision/CAS) — @@ -41,6 +56,11 @@ argument and no real filesystem workspace. internal constant for this iteration, not a new setting. - Not covering true concurrent mutation from two hosts — that is the separately tracked worktree-isolation work ADR 0004 already named. +- Not adding checkpoint capture to the standalone server's WS-driven + `implement` path. Those runs gain mutation exclusivity from this + change but still have no rollback and are not recoverable as + `interrupted` after a crash — a pre-existing, separate gap this + change narrows (adds locking) but does not close (still no checkpoint). ## Decisions @@ -120,6 +140,49 @@ no-op in-memory-only behavior when absent, keeps every current call site and test compiling and passing unchanged, and matches how `WorkbenchRunJournalOptions` is already optional on the journal side. +### Route only `implement` through the scheduler on the WS path; every other command kind is untouched + +`packages/server/src/websocket.ts`'s `streamRun` branches on +`command.kind !== "implement"` first: every other kind (`plan`, `review`, +`status`, `list`, `show`, `validate`, `cancel`) keeps calling +`runner.run(command)` directly, unchanged. Only `implement` — the one +`mutating: true` operation, matching `ImplementationSessionManager`'s own +convention — goes through `WorkbenchRecoveryService.runMutating()`. This +mirrors the extension's existing asymmetry (only `implement` sessions are +scheduler-gated there either) rather than inventing a new rule for the +server. + +### `WorkbenchRecoveryService.runMutating()` is a thin pass-through, not a checkpoint-capturing wrapper + +It calls `scheduler.start({ ..., mutating: true, execute })` and, once +`execute` finishes, calls the same private `persist()` every rollback/ +cleanup call already uses — so a terminal (`completed`/`failed`) +`implement` run becomes visible in `list()`/the journal like any other, +without adding checkpoint capture. There is deliberately no +`scheduler.onDidChange` auto-persist wiring added for this: `initialize()` +and `cleanupBefore()` both reassign `this.scheduler` to a new instance, +and re-subscribing a listener across those reassignments is exactly the +kind of retrofit checkpoint capture would also need — out of scope here +(see Non-Goals). Consequence: a server crash strictly *during* a WS-driven +`implement` run leaves no persisted trace of it (not even as +`interrupted`) — only genuinely-finished runs are recorded. Narrower than +the extension's own crash coverage, and disclosed as such rather than +silently assumed away. + +### Detecting a lease-blocked run without adding a new `Event` kind + +When the lease blocks a mutating run, `execute` never runs, so nothing +was ever sent to the client for that attempt — but `websocket.ts` needs +to tell the client the run failed. Rather than teaching +`WorkbenchRecoveryService` to talk WebSocket, `streamRun` inspects the +returned `WorkbenchProcess` itself: `state === "failed" && startedAt === +undefined` is exactly the signature of the early-exit path in +`process-scheduler.ts`'s `run()` (a normal in-`execute` failure always +sets `startedAt` first). `streamRun` then synthesizes one `failed` `Event` +from `process.error`. No protocol change, no new `EventKind` — reuses the +existing `failed` variant precisely as `agent-runner.ts` already does for +its own internal errors. + ## Risks / Trade-offs - **[Risk]** Introducing an asynchronous gate into `WorkbenchProcessScheduler.run()` @@ -144,3 +207,14 @@ and test compiling and passing unchanged, and matches how after an unclean host exit is expected, not a bug — the next mutating run's staleness check cleans it up functionally (overwrites it) even though the file itself isn't deleted proactively. +- **[Realized during implementation]** `packages/server/src/server.test.ts`'s + existing WS `implement` tests used a fake, non-real `cwd` + (`/workspace/repo`) — harmless before this change, since nothing + touched the filesystem for that path. Once `implement` started routing + through `WorkbenchRecoveryService.open(command.cwd)`, the same tests + silently created real files at `C:\workspace\repo\.openspec-ui\`, + outside any temp-directory sandbox and outside `afterEach`'s cleanup. + Fixed by giving those specific tests a real, tracked temp workspace + (task 5.4) — flagged here as a reminder that any *other* test + exercising the WS `implement` path with a placeholder `cwd` needs the + same fix, not just the ones this change happened to touch. diff --git a/openspec/changes/2026-08-28-cross-host-workspace-lease/proposal.md b/openspec/changes/2026-08-28-cross-host-workspace-lease/proposal.md index 37ed6220..dbce298a 100644 --- a/openspec/changes/2026-08-28-cross-host-workspace-lease/proposal.md +++ b/openspec/changes/2026-08-28-cross-host-workspace-lease/proposal.md @@ -27,6 +27,15 @@ place ADR 0001 decision 3 designates as the command protocol's source of truth — a new command kind added to core silently fails this hand-maintained copy until someone remembers to update it. +**Found while implementing this change:** the paragraph above assumed the +standalone server already gated its own mutating execution in-process, +the way the extension does. It does not — `packages/server/src/ +websocket.ts`'s `streamRun` called `runner.run(command)` directly for +every command kind, never touching `WorkbenchProcessScheduler` at all, so +ADR 0004's mutation isolation was not enforced by the server's own live +`implement` execution even same-host. This change now closes that gap +too, alongside the cross-host one — see ADR 0010's Context/Decision 7. + See ADR 0010 for the full decision and rejected alternatives. ## What Changes @@ -52,6 +61,14 @@ See ADR 0010 for the full decision and rejected alternatives. - Export `COMMAND_KINDS` from `packages/core/src/protocol.ts`; `packages/server/src/wire.ts` imports it instead of redeclaring the literal list. +- `WorkbenchRecoveryService` (`workbench-recovery.ts`) gains + `runMutating()`, a thin pass-through to `scheduler.start({..., mutating: + true, execute})` that persists the terminal state — no checkpoint + capture. `packages/server/src/websocket.ts`'s `streamRun` routes + `implement` commands specifically through it (every other command kind + is unchanged, direct through the `AgentRunner`); a lease conflict is + detected via the returned process's `startedAt === undefined` and + surfaced as a synthesized `failed` `Event` on the socket. - Add `docs/adr/0010-cross-host-workspace-lease.md`. ## Capabilities @@ -76,6 +93,9 @@ See ADR 0010 for the full decision and rejected alternatives. - `packages/core/src/workbench-recovery.ts` - `packages/core/src/index.ts` (export `WorkspaceLeaseManager`) - `packages/server/src/wire.ts` +- `packages/server/src/websocket.ts` +- `packages/server/src/server.ts` (pass `resolveRecoveryService` into + `handleSocketMessage`) - `packages/extension/src/extension.ts` - `docs/adr/0010-cross-host-workspace-lease.md` (new) - `.changeset/*.md` (new changeset file) diff --git a/openspec/changes/2026-08-28-cross-host-workspace-lease/tasks.md b/openspec/changes/2026-08-28-cross-host-workspace-lease/tasks.md index 34c384be..d9e9505a 100644 --- a/openspec/changes/2026-08-28-cross-host-workspace-lease/tasks.md +++ b/openspec/changes/2026-08-28-cross-host-workspace-lease/tasks.md @@ -1,105 +1,160 @@ ## 1. Core: workspace lease module -- [ ] 1.1 Add `packages/core/src/workspace-lease.ts`: `WorkspaceLeaseDocument` +- [x] 1.1 Add `packages/core/src/workspace-lease.ts`: `WorkspaceLeaseDocument` (version 1, `holderId`/`hostKind`/`hostname`/`pid`/`acquiredAt`/`heartbeatAt`) and a `WorkspaceLeaseManager` class reading/writing `/.openspec-ui/workspace.lease.json` via write-then-rename atomic replacement, mirroring `WorkbenchRunJournal.write()`'s temp-file-then-rename-with-EEXIST/EPERM-retry sequence exactly (not a second, independent implementation). -- [ ] 1.2 `WorkspaceLeaseManager.acquireOrRenew()`: succeeds if no lease +- [x] 1.2 `WorkspaceLeaseManager.acquireOrRenew()`: succeeds if no lease exists, the existing lease belongs to this manager's own `holderId`, or the existing lease's `heartbeatAt` is older than the staleness window - (20s); otherwise returns the current holder's details without writing. -- [ ] 1.3 `WorkspaceLeaseManager.release()`: clears the lease file only if + (20s, judged by the *evaluating* manager's own threshold — not + necessarily the original holder's); otherwise returns the current + holder's details without writing. +- [x] 1.3 `WorkspaceLeaseManager.release()`: clears the lease file only if it is currently held by this manager's own `holderId`. -- [ ] 1.4 Export `WorkspaceLeaseManager` and its types from +- [x] 1.4 Export `WorkspaceLeaseManager` and its types from `packages/core/src/index.ts`. ## 2. Core: scheduler integration -- [ ] 2.1 `WorkbenchProcessScheduler`'s constructor accepts an optional +- [x] 2.1 `WorkbenchProcessScheduler`'s constructor accepts an optional second parameter carrying a lease manager; omitted, behavior is unchanged from today (in-memory `mutationLocked` only, no filesystem access) — every existing call site and test keeps compiling and passing with no changes. -- [ ] 2.2 In `run()`'s mutating branch, before setting +- [x] 2.2 In `run()`'s mutating branch, before setting `mutationLocked = true`: if a lease manager is present, call `acquireOrRenew()`. If it reports another live holder, finish the process as `failed` immediately (never transitioning through `running`), with `error` naming the other host's `hostKind`/`hostname`/ `pid` and time since its last heartbeat — do not queue it. -- [ ] 2.3 While a mutating process is `running` under a lease manager, +- [x] 2.3 While a mutating process is `running` under a lease manager, renew the lease every 5s until the process finishes. -- [ ] 2.4 In the mutating branch's existing `finally` block (alongside - `mutationLocked = false`), release the lease if one was acquired. -- [ ] 2.5 When `acquireOrRenew()` reclaims a stale lease from a different +- [x] 2.4 In the mutating branch's existing `finally` block (alongside + `mutationLocked = false`), release the lease if one was acquired. Also + restructured `finish()`'s call site to happen *after* this `finally` + block, not inside the preceding `try`/`catch` — releasing the lease is + now genuinely asynchronous, and `completion` must not resolve before + that release has actually happened (found by a real test race: a + second scheduler's mutating run, started right after `await + first.completion`, was intermittently still seeing the first host's + lease as live). +- [x] 2.5 When `acquireOrRenew()` reclaims a stale lease from a different `holderId`, surface that reclamation (not a plain acquisition) through the existing `report`/progress channel so the host can disclose it. ## 3. Host wiring -- [ ] 3.1 `WorkbenchRecoveryService.open()` +- [x] 3.1 `WorkbenchRecoveryService.open()` (`packages/core/src/workbench-recovery.ts`) constructs a `WorkspaceLeaseManager` for `root` with `hostKind: "standalone-server"` and passes it to its internal scheduler (`initialize()` and `cleanupBefore()`'s scheduler reconstruction). -- [ ] 3.2 `packages/extension/src/extension.ts` `activate()` constructs a +- [x] 3.2 `packages/extension/src/extension.ts` `activate()` constructs a `WorkspaceLeaseManager` for `workspaceRoot` with `hostKind: "vscode-extension"` (only when `workspaceRoot` is defined, matching the existing journal-construction guard) and passes it to the scheduler it constructs directly. -- [ ] 3.3 Confirm `deactivate()` (`extension.ts:206`) and the standalone - server's `close()` (`packages/server/src/server.ts:268`) do not need to - release a lease themselves — release already happens per-run in the - scheduler's `finally` block (2.4), so no lease is ever held across a - clean shutdown with no active mutating run. +- [x] 3.3 Confirmed `deactivate()` (`extension.ts`) and the standalone + server's `close()` (`server.ts`) do not need to release a lease + themselves — release already happens per-run in the scheduler's + `finally` block (2.4), so no lease is ever held across a clean + shutdown with no active mutating run. ## 4. Wire contract fix -- [ ] 4.1 Export `COMMAND_KINDS` (a `readonly CommandKind[]`) from +- [x] 4.1 Export `COMMAND_KINDS` (a `readonly CommandKind[]`) from `packages/core/src/protocol.ts`, next to the `CommandKind` type it enumerates. -- [ ] 4.2 `packages/server/src/wire.ts`'s `isCommandLike` imports and uses +- [x] 4.2 `packages/server/src/wire.ts`'s `isCommandLike` imports and uses that exported array instead of its own locally declared `COMMAND_KINDS`. -## 5. Tests +## 5. Standalone live execution wiring (scope expansion — discovered mid-implementation) -- [ ] 5.1 `workspace-lease.test.ts`: acquire when absent, renew by the - same holder, refuse a live foreign holder, reclaim a stale foreign - holder, release only when self-held, and the write-then-rename - replacement survives a concurrent read (mirroring - `workbench-run-journal.test.ts`'s existing atomicity coverage style). -- [ ] 5.2 `process-scheduler.test.ts`: add cases for a lease-backed - scheduler — foreign live lease rejects a mutating run as `failed` - without queuing; own or absent lease behaves exactly as before; stale - foreign lease is reclaimed and disclosed. Explicitly re-run the - existing no-lease test cases unchanged to confirm the optional - parameter is truly backward compatible (risk called out in design.md). -- [ ] 5.3 A cross-process integration test: two `WorkbenchRecoveryService` - instances (or scheduler + lease manager pairs) opened against the same - temp workspace root in the same test process, simulating two hosts — - confirms the second cannot start a mutating run while the first's - lease is live, and can once the first finishes or its lease goes - stale. -- [ ] 5.4 `wire.test.ts` (or equivalent): `isCommandLike` still accepts - every `CommandKind`, now sourced from the shared `COMMAND_KINDS` export - rather than a local copy. +`packages/server/src/websocket.ts`'s `streamRun` never touched +`WorkbenchProcessScheduler` at all — it called `runner.run(command)` +directly for every command kind, so ADR 0004's mutation isolation was +unenforced for the standalone server's own live `implement` execution, +same-host, before this change (see ADR 0010 Context/Decision 7). This +section closes that gap as part of this same change; it is what makes +sections 1–4 actually protect the real cross-host scenario, not just +`WorkbenchRecoveryService`'s rollback/cleanup calls. -## 6. Verification +- [x] 5.1 Add `WorkbenchRecoveryService.runMutating(id, operation, + changeName, execute)` (`workbench-recovery.ts`): a thin pass-through to + `scheduler.start({ ..., mutating: true, execute }).completion`, + persisting the terminal state afterward via the existing private + `persist()`. No checkpoint capture (see design.md Non-Goals). +- [x] 5.2 `packages/server/src/websocket.ts`'s `streamRun` branches on + `command.kind !== "implement"` first (every other kind unchanged, direct + through the `AgentRunner`, exactly as before). For `implement`, resolve + the `WorkbenchRecoveryService` for `command.cwd` and call + `runMutating()`, with `execute` streaming the `AgentRunner`'s events to + the socket as they occur (`changeName` derived from + `path.basename(command.context.changeDir)`). +- [x] 5.3 If the returned process is `failed` with `startedAt === + undefined` (blocked before the agent ever ran — a lease conflict, or a + duplicate `runId` throwing synchronously from `scheduler.start()`), + synthesize and send one `failed` `Event` naming the reason — the socket + otherwise never received anything for that attempt. +- [x] 5.4 `packages/server/src/server.ts`: pass `resolveRecoveryService` + into `handleSocketMessage`. -- [ ] 6.1 `npm run typecheck` and `npm run lint` (including +## 6. Tests + +- [x] 6.1 `workspace-lease.test.ts`: acquire when absent, renew by the + same holder (keeping `acquiredAt`, advancing `heartbeatAt`), refuse a + live foreign holder, reclaim a stale foreign holder, release only when + self-held, and a clean acquire immediately after release. +- [x] 6.2 `process-scheduler.test.ts`: cases for a lease-backed scheduler + — foreign live lease rejects a mutating run as `failed` without + queuing (`startedAt` stays `undefined`); own or absent lease behaves + exactly as before; stale foreign lease is reclaimed and disclosed + (`process.progress` contains "Reclaimed"); lease release on completion + lets a second scheduler then acquire; read-only runs are never gated + by the lease at all. Existing no-lease test cases re-run unchanged, + confirming the optional parameter is truly backward compatible. +- [x] 6.3 `wire.test.ts`: `isCommandLike` accepts every `CommandKind` from + the shared `COMMAND_KINDS` export, rejects an unknown kind, rejects a + value missing required fields. +- [x] 6.4 `server.test.ts`: fixed the WS `implement` tests to use a real, + tracked temp workspace instead of the fake `/workspace/repo` (that fake + path was harmless before section 5, but now real filesystem calls + happen for `implement` — the original tests were, until fixed, + silently writing `.openspec-ui/` files to `C:\workspace\repo\`, outside + any sandbox; see design.md's Risks for the full account). Added a real + two-server-process test over actual WebSocket connections: a second + server's `implement` attempt is blocked (one `failed` event, reason + naming "standalone server") while the first server's run is still + active on the same real workspace root, and both servers' journals + agree once the first finishes. + +## 7. Verification + +- [x] 7.1 `npm run typecheck` and `npm run lint` (including `lint:english`) pass workspace-wide. -- [ ] 6.2 `npm run test` passes workspace-wide, including the new - `workspace-lease.test.ts`, the extended `process-scheduler.test.ts`, - and `wire.test.ts`. -- [ ] 6.3 Manual smoke test: start the standalone server and the VS Code - extension against the same real workspace; start a mutating run - (`implement`) from one, confirm the other's attempt to start a - mutating run fails immediately naming the first host; let the first - finish and confirm the second can then run. -- [ ] 6.4 Propose a changeset (`npx changeset`) for `@openspec-ui/core` - (new lease behavior), `@openspec-ui/server`, and - `openspec-ui-vscode` (minor: new capability, no breaking change). -- [ ] 6.5 Run `openspec change validate --strict cross-host-workspace-lease`. +- [x] 7.2 `npm run test` (`npm run verify`) passes workspace-wide, + including every new/modified test file above. +- [~] 7.3 Manual smoke test: no interactive VS Code host was available in + this environment to drive the extension side by hand, so this wasn't + performed literally as written. The closest available substitute was + done instead (task 6.4's test): two real `createServer()` processes + (real ports, real WebSocket connections) against the same real + temp-directory workspace root, coordinating purely through the real + `.openspec-ui/workspace.lease.json` file on disk — the same primitive + two actual OS processes would use. That test passes: the second + server's `implement` attempt is blocked with an immediate `failed` + event naming the first host, and succeeds once the first finishes. + Still open: an actual two-OS-process run (one real standalone server, + one real VS Code extension window) against a shared workspace, by a + human with a VS Code UI available. +- [x] 7.4 Proposed a changeset (`.changeset/cross-host-workspace-lease.md`) + for `@openspec-ui/core`, `@openspec-ui/server`, and `openspec-ui-vscode` + (minor each) and applied it via `npx changeset version`: core + 0.29.0→0.30.0, server 1.10.0→1.11.0, extension 0.26.0→0.27.0. +- [x] 7.5 Ran `openspec change validate --strict cross-host-workspace-lease` + — valid. diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index 8b87aa72..1f3440ec 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -1,5 +1,11 @@ # @openspec-ui/core +## 0.30.0 + +### Minor Changes + +- Add a cross-host workspace lease (docs/adr/0010-cross-host-workspace-lease.md) so at most one host process — a VS Code extension or a standalone server, pointed at the same workspace — can run a mutating operation at a time. A blocked host gets an immediate, actionable error naming the other host instead of racing it or queuing forever. The standalone server's own `implement` execution is now routed through the same mutation lock and lease (it previously bypassed the scheduler entirely), closing a pre-existing same-host gap alongside the cross-host one. + ## 0.29.0 ### Minor Changes diff --git a/packages/core/package.json b/packages/core/package.json index 5fdc27ae..1f856a35 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -1,7 +1,7 @@ { "name": "@openspec-ui/core", "private": true, - "version": "0.29.0", + "version": "0.30.0", "type": "module", "main": "src/index.ts", "exports": { diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 3ef73221..28179cec 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -16,6 +16,7 @@ export * from "./version-info.js"; export * from "./template-catalog.js"; export * from "./repo-bootstrap.js"; export * from "./process-scheduler.js"; +export * from "./workspace-lease.js"; export * from "./checkpoint.js"; export * from "./workbench-run-journal.js"; export * from "./workbench-recovery.js"; diff --git a/packages/core/src/process-scheduler.test.ts b/packages/core/src/process-scheduler.test.ts index 295ec0a7..30b24dc7 100644 --- a/packages/core/src/process-scheduler.test.ts +++ b/packages/core/src/process-scheduler.test.ts @@ -1,5 +1,9 @@ -import { describe, expect, it, vi } from "vitest"; +import { mkdtemp, rm } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { WorkbenchProcessScheduler } from "./process-scheduler.js"; +import { WorkspaceLeaseManager } from "./workspace-lease.js"; function deferred() { let resolve!: (value: T) => void; @@ -132,3 +136,118 @@ describe("WorkbenchProcessScheduler", () => { }); }); }); + +describe("WorkbenchProcessScheduler cross-host workspace lease", () => { + const roots: string[] = []; + + afterEach(async () => { + await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))); + }); + + async function temporaryRoot(): Promise { + const root = await mkdtemp(path.join(os.tmpdir(), "openspec-ui-scheduler-lease-")); + roots.push(root); + return root; + } + + it("runs a mutating process normally when no other host holds the lease", async () => { + const root = await temporaryRoot(); + const lease = new WorkspaceLeaseManager(root, { hostKind: "standalone-server" }); + const scheduler = new WorkbenchProcessScheduler([], lease); + + const handle = scheduler.start({ + operation: "implement", + changeName: "demo", + mutating: true, + execute: async () => "done", + }); + + const process = await handle.completion; + expect(process.state).toBe("completed"); + }); + + it("fails a mutating run immediately, without queuing, when a foreign host holds a live lease", async () => { + const root = await temporaryRoot(); + const foreignLease = new WorkspaceLeaseManager(root, { hostKind: "vscode-extension" }); + await foreignLease.acquireOrRenew(); + + const lease = new WorkspaceLeaseManager(root, { hostKind: "standalone-server" }); + const scheduler = new WorkbenchProcessScheduler([], lease); + const started = vi.fn(); + const handle = scheduler.start({ + operation: "implement", + changeName: "demo", + mutating: true, + execute: async () => { started(); }, + }); + + const process = await handle.completion; + + expect(started).not.toHaveBeenCalled(); + expect(process.state).toBe("failed"); + expect(process.error).toContain("VS Code extension"); + expect(process.startedAt).toBeUndefined(); + }); + + it("reclaims and runs once a foreign lease has gone stale", async () => { + const root = await temporaryRoot(); + const foreignLease = new WorkspaceLeaseManager(root, { hostKind: "vscode-extension" }); + await foreignLease.acquireOrRenew(); + await new Promise((resolve) => setTimeout(resolve, 5)); + + // Staleness is judged by the evaluating (this) manager's own threshold. + const lease = new WorkspaceLeaseManager(root, { hostKind: "standalone-server", staleAfterMs: 1 }); + const scheduler = new WorkbenchProcessScheduler([], lease); + const handle = scheduler.start({ + operation: "implement", + changeName: "demo", + mutating: true, + execute: async () => "done", + }); + + const process = await handle.completion; + + expect(process.state).toBe("completed"); + expect(process.progress).toContain("Reclaimed"); + }); + + it("releases the lease on completion so a second host can then mutate", async () => { + const root = await temporaryRoot(); + const firstLease = new WorkspaceLeaseManager(root, { hostKind: "standalone-server" }); + const firstScheduler = new WorkbenchProcessScheduler([], firstLease); + await (await firstScheduler.start({ + operation: "implement", + changeName: "demo", + mutating: true, + execute: async () => "done", + }).completion); + + const secondLease = new WorkspaceLeaseManager(root, { hostKind: "vscode-extension" }); + const secondScheduler = new WorkbenchProcessScheduler([], secondLease); + const second = await secondScheduler.start({ + operation: "implement", + changeName: "demo", + mutating: true, + execute: async () => "done", + }).completion; + + expect(second.state).toBe("completed"); + }); + + it("does not gate read-only runs on the lease at all", async () => { + const root = await temporaryRoot(); + const foreignLease = new WorkspaceLeaseManager(root, { hostKind: "vscode-extension" }); + await foreignLease.acquireOrRenew(); + + const lease = new WorkspaceLeaseManager(root, { hostKind: "standalone-server" }); + const scheduler = new WorkbenchProcessScheduler([], lease); + const process = await scheduler.start({ + operation: "status", + changeName: "demo", + mutating: false, + execute: async () => "ok", + }).completion; + + expect(process.state).toBe("completed"); + }); +}); diff --git a/packages/core/src/process-scheduler.ts b/packages/core/src/process-scheduler.ts index b8f78a05..9ebcb25f 100644 --- a/packages/core/src/process-scheduler.ts +++ b/packages/core/src/process-scheduler.ts @@ -1,3 +1,10 @@ +import { + WORKSPACE_LEASE_RENEW_INTERVAL_MS, + describeWorkspaceLeaseConflict, + describeWorkspaceLeaseReclamation, + type WorkspaceLeaseManager, +} from "./workspace-lease.js"; + export type WorkbenchProcessState = | "queued" | "running" @@ -54,7 +61,11 @@ export class WorkbenchProcessScheduler { private mutationLocked = false; private readonly listeners = new Set<(processes: WorkbenchProcess[]) => void>(); - constructor(initialProcesses: WorkbenchProcess[] = []) { + /** Cross-host mutation isolation (docs/adr/0010-cross-host-workspace-lease.md). + * Optional so every existing call site/test that constructs a scheduler + * with no real workspace root keeps its current in-memory-only behavior + * unchanged. */ + constructor(initialProcesses: WorkbenchProcess[] = [], private readonly lease?: WorkspaceLeaseManager) { for (const initial of initialProcesses) { const process = { ...initial }; if (process.state === "queued" || process.state === "running") { @@ -166,10 +177,41 @@ export class WorkbenchProcessScheduler { private async run(pending: PendingProcess): Promise { const process = pending.process; + const leasedMutation = process.mutating && this.lease !== undefined; + + if (leasedMutation) { + const result = await this.lease!.acquireOrRenew(); + if (!result.ok) { + // Never transitions through "running" — another host holds the + // workspace, so this run never actually starts. + process.error = describeWorkspaceLeaseConflict(result.conflict); + this.finish(pending, "failed"); + this.drain(); + return; + } + if (result.reclaimedFrom) { + process.progress = describeWorkspaceLeaseReclamation(result.reclaimedFrom); + } + } + if (process.mutating) this.mutationLocked = true; process.state = "running"; process.startedAt = new Date().toISOString(); this.emit(); + + const renewTimer = leasedMutation + ? setInterval(() => { + void this.lease!.acquireOrRenew().catch(() => undefined); + }, WORKSPACE_LEASE_RENEW_INTERVAL_MS) + : undefined; + + // `finish()` (which resolves `completion`) is deliberately called after + // this try/catch/finally, not inside it: releasing the lease is + // asynchronous, and a caller awaiting `completion` before starting + // another mutating run (exactly the cross-host scenario this lease + // exists for) must be able to rely on all of this run's cleanup — + // including the lease release — having already happened. + let terminalState: WorkbenchProcessState; try { const summary = await pending.execute({ signal: pending.controller.signal, @@ -179,22 +221,25 @@ export class WorkbenchProcessScheduler { }, }); if (pending.controller.signal.aborted) { - this.finish(pending, "cancelled"); + terminalState = "cancelled"; } else { if (summary !== undefined) process.summary = summary; - this.finish(pending, "completed"); + terminalState = "completed"; } } catch (error) { if (pending.controller.signal.aborted) { - this.finish(pending, "cancelled"); + terminalState = "cancelled"; } else { process.error = error instanceof Error ? error.message : String(error); - this.finish(pending, "failed"); + terminalState = "failed"; } } finally { + if (renewTimer) clearInterval(renewTimer); if (process.mutating) this.mutationLocked = false; - this.drain(); + if (leasedMutation) await this.lease!.release(); } + this.finish(pending, terminalState); + this.drain(); } private finish(pending: PendingProcess, state: WorkbenchProcessState): void { diff --git a/packages/core/src/protocol.ts b/packages/core/src/protocol.ts index a6526eb0..ca922e40 100644 --- a/packages/core/src/protocol.ts +++ b/packages/core/src/protocol.ts @@ -15,6 +15,20 @@ export type CommandKind = | "validate" | "cancel"; +/** Runtime enumeration of `CommandKind`, kept in this one place so + * transport-boundary shape checks (e.g. `packages/server/src/wire.ts`'s + * `isCommandLike`) never hand-maintain their own separate copy. */ +export const COMMAND_KINDS: readonly CommandKind[] = [ + "plan", + "implement", + "review", + "status", + "list", + "show", + "validate", + "cancel", +]; + export interface CommandContext { /** Absolute path to the OpenSpec change this command applies to. */ changeDir: string; diff --git a/packages/core/src/workbench-recovery.ts b/packages/core/src/workbench-recovery.ts index 4adfb53f..006eb6af 100644 --- a/packages/core/src/workbench-recovery.ts +++ b/packages/core/src/workbench-recovery.ts @@ -9,12 +9,13 @@ import { type RollbackResult, type WorkbenchCheckpoint, } from "./checkpoint.js"; -import { WorkbenchProcessScheduler, type WorkbenchProcess } from "./process-scheduler.js"; +import { WorkbenchProcessScheduler, type StartProcessOptions, type WorkbenchProcess } from "./process-scheduler.js"; import { WorkbenchRunJournal, type PersistedCheckpointSession, type WorkbenchRunJournalOptions, } from "./workbench-run-journal.js"; +import { WorkspaceLeaseManager } from "./workspace-lease.js"; interface RecoverySession { processId: string; @@ -36,11 +37,13 @@ export interface WorkbenchCleanupResult { export class WorkbenchRecoveryService { private readonly journal: WorkbenchRunJournal; + private readonly lease: WorkspaceLeaseManager; private scheduler = new WorkbenchProcessScheduler(); private readonly sessions = new Map(); private constructor(root: string, options: WorkbenchRunJournalOptions) { this.journal = new WorkbenchRunJournal(root, options); + this.lease = new WorkspaceLeaseManager(root, { hostKind: "standalone-server" }); } static async open( @@ -56,6 +59,26 @@ export class WorkbenchRecoveryService { return this.scheduler.list().sort((left, right) => Date.parse(right.createdAt) - Date.parse(left.createdAt)); } + /** Runs a mutating command through the scheduler's mutation lock and + * cross-host workspace lease (docs/adr/0010-cross-host-workspace-lease.md) + * — the standalone server's own live command execution + * (`packages/server/src/websocket.ts`) otherwise never touched this + * scheduler at all. Deliberately no checkpoint capture in this pass: the + * resulting process has no rollback coverage, and a server crash mid-run + * is not recoverable as "interrupted" with reviewable delta — only + * mutation exclusivity is in scope here. Resolves once `execute` + * finishes and the terminal state is persisted. */ + async runMutating( + id: string, + operation: string, + changeName: string | undefined, + execute: StartProcessOptions["execute"], + ): Promise { + const process = await this.scheduler.start({ id, operation, changeName, mutating: true, execute }).completion; + await this.persist(); + return process; + } + details(processId: string): WorkbenchRecoveryDetails | undefined { const process = this.scheduler.list().find((candidate) => candidate.id === processId); if (!process) return undefined; @@ -140,14 +163,14 @@ export class WorkbenchRecoveryService { for (const processId of this.sessions.keys()) { if (!retainedIds.has(processId)) this.sessions.delete(processId); } - this.scheduler = new WorkbenchProcessScheduler(retained); + this.scheduler = new WorkbenchProcessScheduler(retained, this.lease); await this.persist(); return { removed: current.length - retained.length, retained: retained.length }; } private async initialize(): Promise { const restored = await this.journal.load(); - this.scheduler = new WorkbenchProcessScheduler(restored.processes); + this.scheduler = new WorkbenchProcessScheduler(restored.processes, this.lease); for (const persisted of restored.checkpointSessions) { this.sessions.set(persisted.processId, { processId: persisted.processId, diff --git a/packages/core/src/workspace-lease.test.ts b/packages/core/src/workspace-lease.test.ts new file mode 100644 index 00000000..bc0670e3 --- /dev/null +++ b/packages/core/src/workspace-lease.test.ts @@ -0,0 +1,116 @@ +import { mkdtemp, readFile, rm } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { + WORKSPACE_LEASE_VERSION, + WorkspaceLeaseManager, + describeWorkspaceLeaseConflict, + describeWorkspaceLeaseReclamation, + type WorkspaceLeaseDocument, +} from "./workspace-lease.js"; + +const roots: string[] = []; + +async function temporaryRoot(): Promise { + const root = await mkdtemp(path.join(os.tmpdir(), "openspec-ui-lease-")); + roots.push(root); + return root; +} + +afterEach(async () => { + await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))); +}); + +async function readLease(root: string): Promise { + const raw = await readFile(path.join(root, ".openspec-ui", "workspace.lease.json"), "utf8"); + return JSON.parse(raw) as WorkspaceLeaseDocument; +} + +describe("WorkspaceLeaseManager", () => { + it("acquires the lease when none exists", async () => { + const root = await temporaryRoot(); + const manager = new WorkspaceLeaseManager(root, { hostKind: "standalone-server" }); + + const result = await manager.acquireOrRenew(); + + expect(result).toEqual({ ok: true }); + const document = await readLease(root); + expect(document).toMatchObject({ version: WORKSPACE_LEASE_VERSION, hostKind: "standalone-server" }); + }); + + it("renews its own lease, keeping the original acquiredAt", async () => { + const root = await temporaryRoot(); + const manager = new WorkspaceLeaseManager(root, { hostKind: "vscode-extension" }); + + await manager.acquireOrRenew(); + const first = await readLease(root); + await new Promise((resolve) => setTimeout(resolve, 5)); + const renewResult = await manager.acquireOrRenew(); + const second = await readLease(root); + + expect(renewResult).toEqual({ ok: true }); + expect(second.acquiredAt).toBe(first.acquiredAt); + expect(Date.parse(second.heartbeatAt)).toBeGreaterThanOrEqual(Date.parse(first.heartbeatAt)); + }); + + it("refuses a live foreign holder", async () => { + const root = await temporaryRoot(); + const holder = new WorkspaceLeaseManager(root, { hostKind: "standalone-server" }); + await holder.acquireOrRenew(); + const contender = new WorkspaceLeaseManager(root, { hostKind: "vscode-extension" }); + + const result = await contender.acquireOrRenew(); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.conflict).toMatchObject({ hostKind: "standalone-server", pid: process.pid }); + expect(describeWorkspaceLeaseConflict(result.conflict)).toContain("standalone server"); + } + }); + + it("reclaims a stale foreign holder and discloses the reclamation", async () => { + const root = await temporaryRoot(); + const holder = new WorkspaceLeaseManager(root, { hostKind: "standalone-server" }); + await holder.acquireOrRenew(); + await new Promise((resolve) => setTimeout(resolve, 5)); + // Staleness is judged by the *evaluating* manager's own threshold, not + // the original holder's — the contender is the one configured short here. + const contender = new WorkspaceLeaseManager(root, { hostKind: "vscode-extension", staleAfterMs: 1 }); + + const result = await contender.acquireOrRenew(); + + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.reclaimedFrom).toMatchObject({ hostKind: "standalone-server" }); + expect(describeWorkspaceLeaseReclamation(result.reclaimedFrom!)).toContain("Reclaimed"); + } + const document = await readLease(root); + expect(document.hostKind).toBe("vscode-extension"); + }); + + it("releases only when self-held", async () => { + const root = await temporaryRoot(); + const holder = new WorkspaceLeaseManager(root, { hostKind: "standalone-server" }); + await holder.acquireOrRenew(); + const contender = new WorkspaceLeaseManager(root, { hostKind: "vscode-extension" }); + + await contender.release(); + expect(await readLease(root)).toMatchObject({ hostKind: "standalone-server" }); + + await holder.release(); + await expect(readFile(path.join(root, ".openspec-ui", "workspace.lease.json"), "utf8")).rejects.toThrow(); + }); + + it("acquires cleanly after a release, with no staleness wait", async () => { + const root = await temporaryRoot(); + const holder = new WorkspaceLeaseManager(root, { hostKind: "standalone-server" }); + await holder.acquireOrRenew(); + await holder.release(); + + const next = new WorkspaceLeaseManager(root, { hostKind: "vscode-extension" }); + const result = await next.acquireOrRenew(); + + expect(result).toEqual({ ok: true }); + }); +}); diff --git a/packages/core/src/workspace-lease.ts b/packages/core/src/workspace-lease.ts new file mode 100644 index 00000000..0a94feb2 --- /dev/null +++ b/packages/core/src/workspace-lease.ts @@ -0,0 +1,180 @@ +import { randomUUID } from "node:crypto"; +import { mkdir, readFile, rename, rm, writeFile } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; + +// Cross-host mutation isolation (docs/adr/0010-cross-host-workspace-lease.md). +// Extends ADR 0004 decision 4's "one mutating run per workspace" invariant +// across host processes (VS Code extension + standalone server), not only +// within one. Deliberately not a general-purpose distributed lock: scoped to +// exactly the mutating-run lifetime a WorkbenchProcessScheduler already +// tracks in-memory via `mutationLocked`, and written with the same +// write-then-rename atomic replacement `WorkbenchRunJournal` already uses. + +export const WORKSPACE_LEASE_VERSION = 1; + +/** How often a held lease's heartbeat is renewed while a mutating process + * is running. */ +export const WORKSPACE_LEASE_RENEW_INTERVAL_MS = 5_000; + +/** How long since the last heartbeat before a lease is treated as no + * longer held (4x the renew interval — tolerant of a slow disk or a GC + * pause without leaving a genuinely stopped host's lease live for long). */ +export const WORKSPACE_LEASE_STALE_AFTER_MS = 20_000; + +export type WorkspaceLeaseHostKind = "vscode-extension" | "standalone-server"; + +export interface WorkspaceLeaseDocument { + version: typeof WORKSPACE_LEASE_VERSION; + holderId: string; + hostKind: WorkspaceLeaseHostKind; + hostname: string; + pid: number; + acquiredAt: string; + heartbeatAt: string; +} + +export interface WorkspaceLeaseManagerOptions { + hostKind: WorkspaceLeaseHostKind; + staleAfterMs?: number; +} + +/** Details of the lease holder a conflicting or reclaimed acquire attempt + * found — enough for a host to explain itself to the user. */ +export interface WorkspaceLeaseConflict { + hostKind: WorkspaceLeaseHostKind; + hostname: string; + pid: number; + heartbeatAgeMs: number; +} + +export type WorkspaceLeaseAcquireResult = + | { ok: true; reclaimedFrom?: WorkspaceLeaseConflict } + | { ok: false; conflict: WorkspaceLeaseConflict }; + +function isMissingFile(error: unknown): boolean { + return error instanceof Error && "code" in error && error.code === "ENOENT"; +} + +function hostKindLabel(hostKind: WorkspaceLeaseHostKind): string { + return hostKind === "vscode-extension" ? "VS Code extension" : "standalone server"; +} + +export function describeWorkspaceLeaseConflict(conflict: WorkspaceLeaseConflict): string { + const heartbeatAgeSeconds = Math.round(conflict.heartbeatAgeMs / 1000); + return ( + `Another OpenSpec UI host (${hostKindLabel(conflict.hostKind)} on ` + + `${conflict.hostname}, pid ${conflict.pid}, last active ${heartbeatAgeSeconds}s ago) ` + + `is currently running a mutating operation on this workspace. Wait for it to ` + + `finish, or close it, before starting one here.` + ); +} + +export function describeWorkspaceLeaseReclamation(conflict: WorkspaceLeaseConflict): string { + const heartbeatAgeSeconds = Math.round(conflict.heartbeatAgeMs / 1000); + return ( + `Reclaimed the workspace lease from ${hostKindLabel(conflict.hostKind)} on ` + + `${conflict.hostname} (pid ${conflict.pid}), which stopped renewing it ` + + `${heartbeatAgeSeconds}s ago.` + ); +} + +/** One host's handle on the cross-host workspace mutation lease. Every + * `WorkspaceLeaseManager` instance has its own random `holderId` — one + * instance is constructed per host activation (per `WorkbenchRecoveryService` + * or per VS Code extension activation), not per process run. */ +export class WorkspaceLeaseManager { + readonly filePath: string; + private readonly holderId = randomUUID(); + private readonly hostKind: WorkspaceLeaseHostKind; + private readonly staleAfterMs: number; + + constructor(root: string, options: WorkspaceLeaseManagerOptions) { + this.filePath = path.join(path.resolve(root), ".openspec-ui", "workspace.lease.json"); + this.hostKind = options.hostKind; + this.staleAfterMs = options.staleAfterMs ?? WORKSPACE_LEASE_STALE_AFTER_MS; + } + + /** Acquires the lease if unheld or stale, or renews it if already held by + * this manager. Never throws on a live foreign holder — reports a + * conflict instead, for the caller to surface without crashing the run. */ + async acquireOrRenew(): Promise { + const existing = await this.readExisting(); + if (existing && existing.holderId !== this.holderId) { + const heartbeatAgeMs = Date.now() - Date.parse(existing.heartbeatAt); + if (heartbeatAgeMs <= this.staleAfterMs) { + return { + ok: false, + conflict: { hostKind: existing.hostKind, hostname: existing.hostname, pid: existing.pid, heartbeatAgeMs }, + }; + } + await this.write(); + return { + ok: true, + reclaimedFrom: { hostKind: existing.hostKind, hostname: existing.hostname, pid: existing.pid, heartbeatAgeMs }, + }; + } + // Renewing our own, already-held lease: keep the original `acquiredAt` + // rather than resetting it on every heartbeat. + await this.write(existing?.acquiredAt); + return { ok: true }; + } + + /** Clears the lease, but only if currently held by this manager — a + * reclaimed-away lease must never be released by its former holder. */ + async release(): Promise { + const existing = await this.readExisting(); + if (!existing || existing.holderId !== this.holderId) return; + await rm(this.filePath, { force: true }); + } + + private async readExisting(): Promise { + let source: string; + try { + source = await readFile(this.filePath, "utf8"); + } catch (error) { + if (isMissingFile(error)) return undefined; + throw error; + } + let document: WorkspaceLeaseDocument; + try { + document = JSON.parse(source) as WorkspaceLeaseDocument; + } catch { + // A corrupt lease file is treated as absent, not a fatal error — the + // next acquire simply overwrites it (unlike the run journal, a lease + // is disposable coordination state, not user data worth preserving). + return undefined; + } + if (document.version !== WORKSPACE_LEASE_VERSION) return undefined; + return document; + } + + private async write(acquiredAt?: string): Promise { + const now = new Date().toISOString(); + const document: WorkspaceLeaseDocument = { + version: WORKSPACE_LEASE_VERSION, + holderId: this.holderId, + hostKind: this.hostKind, + hostname: os.hostname(), + pid: process.pid, + acquiredAt: acquiredAt ?? now, + heartbeatAt: now, + }; + const directory = path.dirname(this.filePath); + const temporaryPath = `${this.filePath}.${randomUUID()}.tmp`; + await mkdir(directory, { recursive: true }); + try { + await writeFile(temporaryPath, `${JSON.stringify(document, null, 2)}\n`, "utf8"); + try { + await rename(temporaryPath, this.filePath); + } catch (error) { + const code = error instanceof Error && "code" in error ? error.code : undefined; + if (code !== "EEXIST" && code !== "EPERM") throw error; + await rm(this.filePath, { force: true }); + await rename(temporaryPath, this.filePath); + } + } finally { + await rm(temporaryPath, { force: true }); + } + } +} diff --git a/packages/extension/CHANGELOG.md b/packages/extension/CHANGELOG.md index 3ddaeced..3c9ab839 100644 --- a/packages/extension/CHANGELOG.md +++ b/packages/extension/CHANGELOG.md @@ -1,5 +1,17 @@ # Changelog +## 0.27.0 + +### Minor Changes + +- Add a cross-host workspace lease (docs/adr/0010-cross-host-workspace-lease.md) so at most one host process — a VS Code extension or a standalone server, pointed at the same workspace — can run a mutating operation at a time. A blocked host gets an immediate, actionable error naming the other host instead of racing it or queuing forever. The standalone server's own `implement` execution is now routed through the same mutation lock and lease (it previously bypassed the scheduler entirely), closing a pre-existing same-host gap alongside the cross-host one. + +### Patch Changes + +- Updated dependencies + - @openspec-ui/core@0.30.0 + - @openspec-ui/server@1.11.0 + ## 0.26.0 ### Minor Changes diff --git a/packages/extension/package.json b/packages/extension/package.json index ba59ef96..d9f21f66 100644 --- a/packages/extension/package.json +++ b/packages/extension/package.json @@ -4,7 +4,7 @@ "displayName": "OpenSpec Workbench", "description": "A dashboard + VS Code extension for OpenSpec, with Claude, Copilot, Codex, and Gemini agents built in.", "publisher": "openspec-ui", - "version": "0.26.0", + "version": "0.27.0", "icon": "media/icon.png", "license": "MIT", "repository": { diff --git a/packages/extension/src/extension.ts b/packages/extension/src/extension.ts index 0feac01a..91602ab1 100644 --- a/packages/extension/src/extension.ts +++ b/packages/extension/src/extension.ts @@ -8,6 +8,7 @@ import type { AgentRunner } from "@openspec-ui/core"; import { WorkbenchProcessScheduler, WorkbenchRunJournal, + WorkspaceLeaseManager, buildDefaultAgentRunners, resolveRunner as resolveAgentRunner, } from "@openspec-ui/core"; @@ -60,7 +61,10 @@ export async function activate(context: vscode.ExtensionContext): Promise { if (!journal) return; void journal.save({ diff --git a/packages/server/CHANGELOG.md b/packages/server/CHANGELOG.md index 2a620b5c..d5a657fa 100644 --- a/packages/server/CHANGELOG.md +++ b/packages/server/CHANGELOG.md @@ -1,5 +1,16 @@ # @openspec-ui/server +## 1.11.0 + +### Minor Changes + +- Add a cross-host workspace lease (docs/adr/0010-cross-host-workspace-lease.md) so at most one host process — a VS Code extension or a standalone server, pointed at the same workspace — can run a mutating operation at a time. A blocked host gets an immediate, actionable error naming the other host instead of racing it or queuing forever. The standalone server's own `implement` execution is now routed through the same mutation lock and lease (it previously bypassed the scheduler entirely), closing a pre-existing same-host gap alongside the cross-host one. + +### Patch Changes + +- Updated dependencies + - @openspec-ui/core@0.30.0 + ## 1.10.0 ### Minor Changes diff --git a/packages/server/package.json b/packages/server/package.json index 82294ca3..9d17353a 100644 --- a/packages/server/package.json +++ b/packages/server/package.json @@ -1,7 +1,7 @@ { "name": "@openspec-ui/server", "private": true, - "version": "1.10.0", + "version": "1.11.0", "type": "module", "main": "src/index.ts", "dependencies": { diff --git a/packages/server/src/server.test.ts b/packages/server/src/server.test.ts index 716761a7..20ee1c27 100644 --- a/packages/server/src/server.test.ts +++ b/packages/server/src/server.test.ts @@ -785,7 +785,20 @@ describe("server — REST /api/status", () => { }); describe("server — WebSocket /api/ws", () => { + // `implement` is the one mutating command kind, and is now routed through + // WorkbenchRecoveryService's real (fs-backed) scheduler/lease (ADR 0010, + // packages/server/src/websocket.ts) — unlike `statusCommand`'s fake + // "/workspace/repo", these tests need a real, cleaned-up temp workspace, + // or they write stray `.openspec-ui/` files outside any sandbox. + let wsImplementCommand: Command; + beforeEach(async () => { + const workspaceRoot = await createTempWorkspace(); + wsImplementCommand = { + ...implementCommand, + cwd: workspaceRoot, + context: { changeDir: path.join(workspaceRoot, "openspec", "changes", "x") }, + }; await startServer(new Map([["fake-agent", fakeRunner(ALL_EVENT_VARIANTS)]])); }); @@ -801,7 +814,7 @@ describe("server — WebSocket /api/ws", () => { }); }); - client.send(JSON.stringify(implementCommand)); + client.send(JSON.stringify(wsImplementCommand)); await done; expect(received).toEqual(ALL_EVENT_VARIANTS); @@ -823,7 +836,7 @@ describe("server — WebSocket /api/ws", () => { if (received.length === ALL_EVENT_VARIANTS.length) resolve(); }); }); - client.send(JSON.stringify(implementCommand)); + client.send(JSON.stringify(wsImplementCommand)); await done; expect(received).toEqual(ALL_EVENT_VARIANTS); @@ -841,13 +854,89 @@ describe("server — WebSocket /api/ws", () => { resolve(); }); }); - client.send(JSON.stringify({ ...implementCommand, agentId: "does-not-exist" })); + client.send(JSON.stringify({ ...wsImplementCommand, agentId: "does-not-exist" })); await done; expect(received).toEqual([expect.objectContaining({ kind: "failed", runId: "run-1" })]); client.close(); }); + it("blocks a second server's implement run while another server holds the workspace lease, and unblocks once it finishes", async () => { + const workspaceRoot = await createTempWorkspace(); + const command = { + ...implementCommand, + cwd: workspaceRoot, + context: { changeDir: path.join(workspaceRoot, "openspec", "changes", "x") }, + }; + let releaseFirstRun!: () => void; + const firstRunGate = new Promise((resolve) => { releaseFirstRun = resolve; }); + const firstServer = createServer({ + workspaceRoot, + host: "127.0.0.1", + port: 0, + accessToken: ACCESS_TOKEN, + allowExternalCwd: true, + runners: new Map([["fake-agent", { + async *run(): AsyncIterable { + yield { kind: "started", runId: "run-1", timestamp: "t1", command: "implement", cwd: workspaceRoot }; + await firstRunGate; + yield { kind: "completed", runId: "run-1", timestamp: "t2" }; + }, + }]]), + }); + const secondServer = createServer({ + workspaceRoot, + host: "127.0.0.1", + port: 0, + accessToken: ACCESS_TOKEN, + allowExternalCwd: true, + runners: new Map([["fake-agent", fakeRunner(ALL_EVENT_VARIANTS)]]), + }); + + try { + const firstAddress = await firstServer.listen(); + const secondAddress = await secondServer.listen(); + const firstClient = new WebSocket( + `ws://127.0.0.1:${firstAddress.port}/api/ws`, + ["openspec-ui", `openspec-ui-token.${ACCESS_TOKEN}`], + ); + await new Promise((resolve) => firstClient.once("open", resolve)); + const firstStarted = new Promise((resolve) => { + firstClient.on("message", (raw) => { + if ((JSON.parse(raw.toString()) as Event).kind === "started") resolve(); + }); + }); + firstClient.send(JSON.stringify(command)); + await firstStarted; // the first server has acquired the lease and is mid-run + + const secondClient = new WebSocket( + `ws://127.0.0.1:${secondAddress.port}/api/ws`, + ["openspec-ui", `openspec-ui-token.${ACCESS_TOKEN}`], + ); + await new Promise((resolve) => secondClient.once("open", resolve)); + const secondReceived = await new Promise((resolve) => { + const events: Event[] = []; + secondClient.on("message", (raw) => { + events.push(JSON.parse(raw.toString()) as Event); + resolve(events); + }); + secondClient.send(JSON.stringify(command)); + }); + + expect(secondReceived).toHaveLength(1); + expect(secondReceived[0]).toMatchObject({ kind: "failed", runId: command.runId }); + expect((secondReceived[0] as { reason: string }).reason).toContain("standalone server"); + secondClient.close(); + + releaseFirstRun(); + await new Promise((resolve) => setTimeout(resolve, 50)); + firstClient.close(); + } finally { + await firstServer.close(); + await secondServer.close(); + } + }); + it("rejects unauthenticated and hostile-origin handshakes", async () => { const unauthenticated = new WebSocket(wsUrl); const unauthenticatedError = await new Promise((resolve) => { diff --git a/packages/server/src/server.ts b/packages/server/src/server.ts index e0dc7915..44d83e83 100644 --- a/packages/server/src/server.ts +++ b/packages/server/src/server.ts @@ -251,7 +251,7 @@ export function createServer(options: ServerOptions): OpenSpecUiServer { socket.close(); }); socket.on("message", (raw) => { - handleSocketMessage(socket, raw.toString(), runners); + handleSocketMessage(socket, raw.toString(), runners, resolveRecoveryService); }); }); diff --git a/packages/server/src/websocket.ts b/packages/server/src/websocket.ts index e56b3dd6..15782d97 100644 --- a/packages/server/src/websocket.ts +++ b/packages/server/src/websocket.ts @@ -1,8 +1,21 @@ // 1.2 WebSocket channel for event-driven commands (plan/implement/review/ // cancel): the command arrives and its events go out over the same connection. +// +// `implement` is the only mutating command kind (matches the extension's +// ImplementationSessionManager, which marks only `implement` runs as +// `mutating: true`), so it alone is routed through WorkbenchRecoveryService's +// scheduler for mutation-lock and cross-host lease enforcement (ADR 0010). +// Every other kind runs exactly as before, direct through the AgentRunner. +import path from "node:path"; import type { WebSocket } from "ws"; -import { type AgentRunner, type Command, resolveRunner, serializeEvent } from "@openspec-ui/core"; +import { + type AgentRunner, + type Command, + type WorkbenchRecoveryService, + resolveRunner, + serializeEvent, +} from "@openspec-ui/core"; import { isCommandLike } from "./wire.js"; function nowIso(): string { @@ -13,6 +26,7 @@ export function handleSocketMessage( socket: WebSocket, raw: string, runners: Map, + resolveRecoveryService: (cwd: string) => Promise, ): void { let parsed: unknown; try { @@ -36,13 +50,79 @@ export function handleSocketMessage( return; } - void streamRun(socket, runner, command); + void streamRun(socket, runner, command, resolveRecoveryService); } -async function streamRun(socket: WebSocket, runner: AgentRunner, command: Command): Promise { +/** Streams `runner.run(command)`'s events to the socket, reporting + * progress and the completion summary through `report`. Throws on a + * `failed` terminal event so a lease-gated caller can mark the scheduler + * process failed too — every event has already reached the socket by + * then regardless. */ +async function streamAgentEvents( + socket: WebSocket, + runner: AgentRunner, + command: Command, + report: (message: string) => void, +): Promise { + let summary: string | undefined; + let failureReason: string | undefined; for await (const event of runner.run(command)) { + if (socket.readyState === socket.OPEN) socket.send(serializeEvent(event)); + if (event.kind === "progress") report(event.message); + if (event.kind === "completed") summary = event.summary; + if (event.kind === "failed") failureReason = event.reason; + } + if (failureReason !== undefined) throw new Error(failureReason); + return summary; +} + +async function streamRun( + socket: WebSocket, + runner: AgentRunner, + command: Command, + resolveRecoveryService: (cwd: string) => Promise, +): Promise { + if (command.kind !== "implement") { + for await (const event of runner.run(command)) { + if (socket.readyState === socket.OPEN) { + socket.send(serializeEvent(event)); + } + } + return; + } + + const recovery = await resolveRecoveryService(command.cwd); + const changeName = path.basename(command.context.changeDir); + let process; + try { + process = await recovery.runMutating(command.runId, command.kind, changeName, (context) => + streamAgentEvents(socket, runner, command, context.report), + ); + } catch (error) { + if (socket.readyState === socket.OPEN) { + socket.send( + serializeEvent({ + kind: "failed", + runId: command.runId, + timestamp: nowIso(), + reason: error instanceof Error ? error.message : String(error), + }), + ); + } + return; + } + if (process.state === "failed" && process.startedAt === undefined) { + // Blocked before the agent ever ran (another host holds the workspace + // lease) — no event for this attempt has reached the socket yet. if (socket.readyState === socket.OPEN) { - socket.send(serializeEvent(event)); + socket.send( + serializeEvent({ + kind: "failed", + runId: command.runId, + timestamp: nowIso(), + reason: process.error ?? "run did not start", + }), + ); } } } diff --git a/packages/server/src/wire.test.ts b/packages/server/src/wire.test.ts new file mode 100644 index 00000000..d7e9be80 --- /dev/null +++ b/packages/server/src/wire.test.ts @@ -0,0 +1,24 @@ +import { COMMAND_KINDS } from "@openspec-ui/core"; +import { describe, expect, it } from "vitest"; +import { isCommandLike } from "./wire.js"; + +function commandOf(kind: string) { + return { kind, cwd: "/workspace", runId: "run-1", context: { changeDir: "/workspace/openspec/changes/demo" } }; +} + +describe("isCommandLike", () => { + it("recognizes every command kind core defines, sourced from the shared COMMAND_KINDS export", () => { + for (const kind of COMMAND_KINDS) { + expect(isCommandLike(commandOf(kind))).toBe(true); + } + }); + + it("rejects a kind core does not define", () => { + expect(isCommandLike(commandOf("not-a-real-command"))).toBe(false); + }); + + it("rejects a value missing required fields", () => { + expect(isCommandLike({ kind: "plan" })).toBe(false); + expect(isCommandLike(null)).toBe(false); + }); +}); diff --git a/packages/server/src/wire.ts b/packages/server/src/wire.ts index 3bd7ce00..9481780b 100644 --- a/packages/server/src/wire.ts +++ b/packages/server/src/wire.ts @@ -4,18 +4,7 @@ // recognizes the request's shape (see spec.md, "Server contains no // business logic"). -import type { Command, CommandKind } from "@openspec-ui/core"; - -const COMMAND_KINDS: readonly CommandKind[] = [ - "plan", - "implement", - "review", - "status", - "list", - "show", - "validate", - "cancel", -]; +import { COMMAND_KINDS, type Command, type CommandKind } from "@openspec-ui/core"; export function isCommandLike(value: unknown): value is Command { if (typeof value !== "object" || value === null) return false;