Merge pull request #3489 from charmbracelet/server-404s

fix(server): keep concurrent sessions from killing each other
This commit is contained in:
Christian Rocha authored and GitHub committed 2026-08-03 19:37:29 -04:00
commit fcfad839bb
22 files changed
+2257 -85

No files matched your search

+233 -20
View File
@@ -37,6 +37,9 @@ var (
ErrInvalidClientID = errors.New("invalid client_id")
ErrClientNotAttached = errors.New("client not attached")
ErrWorkspaceClosing = errors.New("workspace closing")
ErrServerShuttingDown = errors.New("server is shutting down")
ErrServerNotIdle = errors.New("server is hosting live workspaces")
ErrClientRetired = errors.New("client has been retired")
ErrChannelOptInMismatch = errors.New("requested channels differ from the existing workspace; channels are an explicit opt-in and are not shared across duplicate creates")
)
@@ -57,6 +60,16 @@ var DefaultCreateGrace = 30 * time.Second
// old shut-down-immediately behavior).
var DefaultIdleShutdownDelay = 60 * time.Second
// DefaultDetachGrace is how long a client's claim on a workspace survives
// after its last SSE stream drops without an explicit release. The stream
// is the client's refcount claim, so tearing the workspace down the instant
// it closes turns any momentary drop (a hiccup, a suspended laptop, a proxy
// timeout) into a permanently lost workspace: the client's reconnect comes
// back milliseconds later to an ID the server no longer knows. A client
// that released its claim first (a clean exit) skips the grace. Overridable
// via CRUSH_SERVER_DETACH_GRACE (seconds; 0 restores immediate teardown).
var DefaultDetachGrace = 10 * time.Second
// ShutdownFunc is called when the backend needs to trigger a server
// shutdown (e.g. when the last workspace is removed).
type ShutdownFunc func()
@@ -94,26 +107,53 @@ type Backend struct {
// waiting out lingerDelay before it shuts the server down. It is
// guarded by mu and cancelled the moment a new create arrives.
shutdownTimer *time.Timer
mu sync.Mutex
// closing latches the decision to exit. Every site that commits to
// a shutdown sets it while still holding mu, because shutdownFn has
// to run unlocked; CreateWorkspace refuses once it is set. That
// makes the shutdown-vs-create decision atomic: a create can never
// be handed a workspace on a process that is already leaving.
closing bool
// retired holds the IDs of clients that announced their exit via
// RetireClient. Creates from a retired client are refused, which is what
// lets a client release a workspace whose ID it never learned.
//
// Entries are never pruned, and deliberately so: an entry is exactly
// what refuses a create that was already on the wire when the client
// said goodbye, and HTTP gives no ordering between two requests, so
// there is no moment at which the server can prove none is still coming.
// Dropping an entry to save memory would reintroduce the orphaned
// workspace this mechanism exists to prevent. The cost is one UUID per
// client process, and the server only outlives its clients for as long
// as sessions keep arriving inside the idle-shutdown window.
retired map[string]struct{}
mu sync.Mutex
cfg *config.ConfigStore
ctx context.Context
shutdownFn ShutdownFunc
createGrace time.Duration
lingerDelay time.Duration
detachGrace time.Duration
}
// clientState tracks one client's claim on a workspace.
//
// - streams counts the number of live SSE event streams the client
// currently has open against the workspace.
// - holdTimer is non-nil iff the client created the workspace but has
// not yet attached an SSE stream; it fires after createGrace and
// releases the hold.
// - holdTimer is non-nil in the two timer-held states: the client
// created the workspace but has not yet attached an SSE stream
// (fires after createGrace), or the client's last SSE stream dropped
// without an explicit release (fires after detachGrace, giving the
// client's reconnect loop a window to re-attach). Either way the
// timer releases the claim when it expires.
// - currentSessionID records which session this client is currently
// viewing. Empty string means the client has no session selected
// (e.g. the landing screen). Cleared automatically when the
// clientState entry is removed.
// - released marks that the client gave up its claim explicitly while
// streams were still open, which a clean exit does. The final stream
// detach then tears down immediately instead of waiting out the
// detach grace for a client that is not coming back.
//
// streams and holdTimer are mutually exclusive in practice (the hold
// timer is stopped the moment an SSE stream attaches), but both being
@@ -122,6 +162,7 @@ type clientState struct {
streams int
holdTimer *time.Timer
currentSessionID string
released bool
}
// Workspace represents a running [app.App] workspace with its
@@ -220,23 +261,33 @@ func New(ctx context.Context, cfg *config.ConfigStore, shutdownFn ShutdownFunc)
return &Backend{
workspaces: csync.NewMap[string, *Workspace](),
pathIndex: make(map[string]string),
retired: make(map[string]struct{}),
cfg: cfg,
ctx: ctx,
shutdownFn: shutdownFn,
createGrace: DefaultCreateGrace,
lingerDelay: idleShutdownDelayFromEnv(),
detachGrace: durationFromEnv("CRUSH_SERVER_DETACH_GRACE", DefaultDetachGrace),
}
}
// idleShutdownDelayFromEnv returns the idle-shutdown delay, honoring a
// CRUSH_SERVER_IDLE_TIMEOUT override (in seconds; 0 disables lingering).
func idleShutdownDelayFromEnv() time.Duration {
if v := os.Getenv("CRUSH_SERVER_IDLE_TIMEOUT"); v != "" {
return durationFromEnv("CRUSH_SERVER_IDLE_TIMEOUT", DefaultIdleShutdownDelay)
}
// durationFromEnv reads a whole number of seconds from the named
// environment variable, falling back to def when it is unset or
// unparseable. Zero is a meaningful value for both lifecycle windows it
// configures, so it is accepted.
func durationFromEnv(name string, def time.Duration) time.Duration {
if v := os.Getenv(name); v != "" {
if secs, err := strconv.Atoi(v); err == nil && secs >= 0 {
return time.Duration(secs) * time.Second
}
}
return DefaultIdleShutdownDelay
return def
}
// SetCreateGrace overrides the create-grace window. Intended for tests
@@ -247,6 +298,15 @@ func (b *Backend) SetCreateGrace(d time.Duration) {
b.createGrace = d
}
// SetDetachGrace overrides how long a client's claim survives after its
// last SSE stream drops. A value <= 0 restores the tear-down-immediately
// behavior. Intended for tests.
func (b *Backend) SetDetachGrace(d time.Duration) {
b.mu.Lock()
defer b.mu.Unlock()
b.detachGrace = d
}
// SetIdleShutdownDelay overrides how long the server lingers after its
// last workspace is released before shutting down. A value <= 0 restores
// the shut-down-immediately behavior. Intended for tests.
@@ -297,6 +357,10 @@ func (b *Backend) CreateWorkspace(args proto.Workspace) (*Workspace, proto.Works
}
b.mu.Lock()
if err := b.admitLocked(clientID); err != nil {
b.mu.Unlock()
return nil, proto.Workspace{}, err
}
// A client is arriving: cancel any pending idle shutdown so we never
// hand back a workspace on a server that is about to tear itself down.
b.cancelShutdownLocked()
@@ -396,6 +460,17 @@ func (b *Backend) CreateWorkspace(args proto.Workspace) (*Workspace, proto.Works
}
b.mu.Lock()
// Re-check admission: the client may have retired while the slow
// init above ran with b.mu released, and registering a claim for a
// client that has already announced its exit would strand the
// workspace. (b.closing cannot have flipped: every shutdown decision
// requires pending == 0, and this create has held pending since
// before it released b.mu.)
if err := b.admitLocked(clientID); err != nil {
b.mu.Unlock()
ws.invokeShutdown()
return nil, proto.Workspace{}, err
}
// Re-check the index under the lock: a concurrent caller may have
// won the race between the initial unlock and here.
if existingID, ok := b.pathIndex[key]; ok {
@@ -522,9 +597,12 @@ func (b *Backend) AttachClient(workspaceID, clientID string) error {
return nil
}
// DetachClient releases one SSE stream's hold on the workspace. If the
// client has no other streams and no pending creation hold, its claim
// is removed and the workspace is torn down once refcount hits zero.
// DetachClient releases one SSE stream's hold on the workspace. When the
// client has no streams left and no pending creation hold, its claim
// either enters the detach grace — giving a reconnecting client time to
// re-attach — or, if the grace is disabled or the client already released
// its claim, is removed, tearing the workspace down once the refcount
// hits zero.
func (b *Backend) DetachClient(workspaceID, clientID string) {
ws, ok := b.workspaces.Get(workspaceID)
if !ok {
@@ -533,6 +611,61 @@ func (b *Backend) DetachClient(workspaceID, clientID string) {
b.detachStream(ws, clientID)
}
// admitLocked reports whether clientID may still take a claim on this
// server. It must be called with b.mu held, which is what makes the
// answer atomic with respect to the shutdown latch and to RetireClient.
func (b *Backend) admitLocked(clientID string) error {
if b.closing {
return ErrServerShuttingDown
}
if _, ok := b.retired[clientID]; ok {
return ErrClientRetired
}
return nil
}
// RetireClient records that a client has exited and releases every claim it
// holds, across every workspace. It is the authoritative "this client is
// gone" signal, and the reason a client never has to guess whether a create
// whose response it lost left a workspace behind: either the create landed
// first and this call releases its claim, or it arrives later and is
// refused, registering nothing.
//
// Idempotent. Lock order is the canonical b.mu -> ws.clientsMu; teardowns
// for workspaces the client was last on run after b.mu is released and
// re-check under both locks.
func (b *Backend) RetireClient(clientID string) error {
if _, err := validateClientID(clientID); err != nil {
return err
}
b.mu.Lock()
if b.retired == nil {
b.retired = make(map[string]struct{})
}
b.retired[clientID] = struct{}{}
var orphaned []*Workspace
for _, ws := range b.workspaces.Seq2() {
ws.clientsMu.Lock()
if cs, ok := ws.clients[clientID]; ok {
if cs.holdTimer != nil {
cs.holdTimer.Stop()
}
delete(ws.clients, clientID)
if len(ws.clients) == 0 {
orphaned = append(orphaned, ws)
}
}
ws.clientsMu.Unlock()
}
b.mu.Unlock()
for _, ws := range orphaned {
b.teardown(ws)
}
return nil
}
// releaseHold releases the creation hold for a client, if any. Active
// stream claims are unaffected. Idempotent: returns nil if the
// workspace or the client's hold no longer exist.
@@ -550,19 +683,37 @@ func (b *Backend) releaseHold(workspaceID, clientID string) error {
// registerClient installs (idempotently) the given client's claim on
// the workspace and starts a grace timer if the entry is fresh.
//
// A duplicate create from a client whose claim is timer-held (waiting to
// attach, or waiting out the detach grace) re-arms it for a full create
// grace and supersedes any earlier explicit release: the client is coming
// back. The re-arm installs a fresh clientState rather than resetting the
// old timer, so an already-fired timer racing this call fails expireHold's
// identity check instead of killing the new claim.
func (b *Backend) registerClient(ws *Workspace, clientID string) {
ws.clientsMu.Lock()
defer ws.clientsMu.Unlock()
if _, ok := ws.clients[clientID]; ok {
// Idempotent: a duplicate CreateWorkspace from the same
// client does not add a second claim.
if old, ok := ws.clients[clientID]; ok {
old.released = false
if old.holdTimer == nil {
// Live streams hold the claim; nothing to re-arm.
return
}
old.holdTimer.Stop()
ws.clients[clientID] = b.newHeldClient(ws, clientID, old.currentSessionID, b.createGrace)
return
}
cs := &clientState{}
cs.holdTimer = time.AfterFunc(b.createGrace, func() {
ws.clients[clientID] = b.newHeldClient(ws, clientID, "", b.createGrace)
}
// newHeldClient builds a clientState whose claim is held only by a timer
// that releases it after grace. Callers must hold ws.clientsMu.
func (b *Backend) newHeldClient(ws *Workspace, clientID, sessionID string, grace time.Duration) *clientState {
cs := &clientState{currentSessionID: sessionID}
cs.holdTimer = time.AfterFunc(grace, func() {
b.expireHold(ws, clientID, cs)
})
ws.clients[clientID] = cs
return cs
}
// expireHold is the body of the grace timer. It runs in its own
@@ -599,6 +750,11 @@ func (b *Backend) releaseHoldLocked(ws *Workspace, clientID string) {
if cs.streams == 0 {
delete(ws.clients, clientID)
teardown = len(ws.clients) == 0
} else {
// The client gave up its claim while streams are still open, which
// is what a clean exit looks like. Remember it so the final detach
// skips the reconnect grace.
cs.released = true
}
ws.clientsMu.Unlock()
if teardown {
@@ -607,6 +763,10 @@ func (b *Backend) releaseHoldLocked(ws *Workspace, clientID string) {
}
func (b *Backend) detachStream(ws *Workspace, clientID string) {
b.mu.Lock()
grace := b.detachGrace
b.mu.Unlock()
ws.clientsMu.Lock()
cs, ok := ws.clients[clientID]
if !ok {
@@ -618,8 +778,18 @@ func (b *Backend) detachStream(ws *Workspace, clientID string) {
}
teardown := false
if cs.streams == 0 && cs.holdTimer == nil {
delete(ws.clients, clientID)
teardown = len(ws.clients) == 0
if grace > 0 && !cs.released {
// The stream dropped without the client releasing its claim, so
// treat it as an interruption rather than an exit: hold the
// workspace under a timer long enough for the client's
// reconnect to re-attach (AttachClient stops the timer).
cs.holdTimer = time.AfterFunc(grace, func() {
b.expireHold(ws, clientID, cs)
})
} else {
delete(ws.clients, clientID)
teardown = len(ws.clients) == 0
}
}
ws.clientsMu.Unlock()
if teardown {
@@ -681,6 +851,9 @@ func (b *Backend) teardown(ws *Workspace) {
// lingerDelay and shuts down then, and returns false. When the server is
// not idle (a workspace is live or a create is in flight) it does
// nothing and returns false.
//
// Returning true also latches [Backend.closing], since the caller has to
// release b.mu before it can run shutdownFn.
func (b *Backend) scheduleShutdownIfIdleLocked() (shutdownNow bool) {
if b.shutdownFn == nil {
return false
@@ -689,6 +862,7 @@ func (b *Backend) scheduleShutdownIfIdleLocked() (shutdownNow bool) {
return false
}
if b.lingerDelay <= 0 {
b.closing = true
return true
}
if b.shutdownTimer == nil {
@@ -710,11 +884,16 @@ func (b *Backend) cancelShutdownLocked() {
// down only if it is still idle when the linger window elapses; any
// create that arrived in the meantime cancelled the timer (or bumped
// pending / the workspace count), so this re-check makes the linger
// race-free.
// race-free. Deciding to exit latches [Backend.closing] under the same
// lock, so a create arriving in the gap before shutdownFn runs is
// refused instead of being initialized on a departing process.
func (b *Backend) maybeShutdown() {
b.mu.Lock()
b.shutdownTimer = nil
idle := b.workspaces.Len() == 0 && b.pending == 0
if idle {
b.closing = true
}
fn := b.shutdownFn
b.mu.Unlock()
if idle && fn != nil {
@@ -821,11 +1000,45 @@ func (b *Backend) Config() *config.ConfigStore {
// Shutdown initiates a graceful server shutdown.
func (b *Backend) Shutdown() {
if b.shutdownFn != nil {
b.shutdownFn()
b.mu.Lock()
b.closing = true
fn := b.shutdownFn
b.mu.Unlock()
if fn != nil {
fn()
}
}
// ShutdownIfIdle shuts the server down only when it is hosting no
// workspaces and has no creates in flight, reporting false when it declined
// because work is live.
//
// This is the only shutdown a client may request. A client asks in order to
// replace a version-mismatched server, and it cannot check idleness itself
// without a second round trip a new session can slip into. Deciding here,
// under the lock creates and teardowns take, closes that window: the answer
// is atomic, and granting it latches the decision so creates arriving
// afterwards are refused.
func (b *Backend) ShutdownIfIdle() bool {
b.mu.Lock()
live, pending := b.workspaces.Len(), b.pending
idle := live == 0 && pending == 0
if idle {
b.closing = true
}
fn := b.shutdownFn
b.mu.Unlock()
if !idle {
slog.Warn("Refusing shutdown request: server is not idle",
"workspaces", live, "pending_creates", pending)
return false
}
if fn != nil {
fn()
}
return true
}
// resolveWorkspaceKey returns a stable canonical form of path suitable
// for use as a dedup key. It applies filepath.Abs, then attempts
// filepath.EvalSymlinks; because EvalSymlinks errors on non-existent
+298
View File
@@ -1484,3 +1484,301 @@ func TestServer_ShutsDownAfterLingerWhenIdle(t *testing.T) {
2*time.Second, 10*time.Millisecond,
"idle server must shut down after the linger window")
}
// -- Detach grace --
//
// The SSE stream is a client's refcount claim, so a stream that drops
// without an explicit release must not destroy the workspace before the
// client's reconnect loop can come back. These cover both outcomes of
// that window plus the fast path a clean exit still gets.
// TestDetachStream_GraceSurvivesReattach is the core regression: a
// momentary stream drop followed by a reconnect must leave the workspace
// exactly where it was. Before the grace, the reconnect came back to a
// workspace ID the server no longer knew, and every later request 404'd
// forever because a sibling session kept the server itself alive.
func TestDetachStream_GraceSurvivesReattach(t *testing.T) {
t.Parallel()
b, srvShutdowns := newTestBackend(t)
b.SetDetachGrace(time.Hour)
ws, wsShutdowns := insertTestWorkspace(t, b, "/tmp/blip")
cid := newClientID(t)
b.registerClient(ws, cid)
require.NoError(t, b.AttachClient(ws.ID, cid))
b.DetachClient(ws.ID, cid)
require.Equal(t, int32(0), wsShutdowns.Load(),
"a dropped stream must not tear the workspace down within the grace")
live, err := b.GetWorkspace(ws.ID)
require.NoError(t, err, "the workspace must still be addressable by its original ID")
require.Same(t, ws, live)
// The reconnect lands and converts the grace back into a stream claim.
require.NoError(t, b.AttachClient(ws.ID, cid))
ws.clientsMu.Lock()
cs := ws.clients[cid]
require.Equal(t, 1, cs.streams)
require.Nil(t, cs.holdTimer, "re-attaching must cancel the grace timer")
ws.clientsMu.Unlock()
require.Equal(t, int32(0), srvShutdowns.Load())
}
// TestDetachStream_GraceExpiryTearsDown checks the grace is bounded: a
// client that never comes back must not pin the workspace (and with it
// the server) indefinitely.
func TestDetachStream_GraceExpiryTearsDown(t *testing.T) {
t.Parallel()
b, srvShutdowns := newTestBackend(t)
b.SetDetachGrace(50 * time.Millisecond)
ws, wsShutdowns := insertTestWorkspace(t, b, "/tmp/gone-for-good")
cid := newClientID(t)
b.registerClient(ws, cid)
require.NoError(t, b.AttachClient(ws.ID, cid))
b.DetachClient(ws.ID, cid)
require.Eventually(t, func() bool { return srvShutdowns.Load() == 1 },
2*time.Second, 10*time.Millisecond,
"an unclaimed grace must expire and tear the workspace down")
require.Equal(t, int32(1), wsShutdowns.Load())
_, err := b.GetWorkspace(ws.ID)
require.ErrorIs(t, err, ErrWorkspaceNotFound)
}
// TestDetachStream_ExplicitReleaseSkipsGrace keeps clean exits fast: a
// client that says goodbye before its stream closes is not coming back,
// so the grace must not delay the teardown.
func TestDetachStream_ExplicitReleaseSkipsGrace(t *testing.T) {
t.Parallel()
b, _ := newTestBackend(t)
b.SetDetachGrace(time.Hour)
ws, wsShutdowns := insertTestWorkspace(t, b, "/tmp/clean-exit")
cid := newClientID(t)
b.registerClient(ws, cid)
require.NoError(t, b.AttachClient(ws.ID, cid))
// The order a quitting client produces: release, then the stream ends.
require.NoError(t, b.releaseHold(ws.ID, cid))
require.Equal(t, int32(0), wsShutdowns.Load(), "the live stream still holds it")
b.DetachClient(ws.ID, cid)
require.Equal(t, int32(1), wsShutdowns.Load(),
"an explicitly released client must tear down without waiting out the grace")
}
// TestRegisterClient_RearmsGraceAndCancelsRelease covers a client that
// recovers by re-creating the workspace rather than re-attaching: the
// duplicate create must buy it a fresh attach window and undo an earlier
// release, and the superseded timer must not be able to kill the new claim.
func TestRegisterClient_RearmsGraceAndCancelsRelease(t *testing.T) {
t.Parallel()
b, _ := newTestBackend(t)
b.SetDetachGrace(50 * time.Millisecond)
b.createGrace = time.Hour
ws, wsShutdowns := insertTestWorkspace(t, b, "/tmp/rearm")
cid := newClientID(t)
b.registerClient(ws, cid)
require.NoError(t, b.AttachClient(ws.ID, cid))
require.NoError(t, b.releaseHold(ws.ID, cid))
b.DetachClient(ws.ID, cid)
require.Equal(t, int32(1), wsShutdowns.Load())
// A second workspace, this time reclaimed by a duplicate create while
// its short detach grace is running.
ws2, ws2Shutdowns := insertTestWorkspace(t, b, "/tmp/rearm-2")
b.registerClient(ws2, cid)
require.NoError(t, b.AttachClient(ws2.ID, cid))
b.DetachClient(ws2.ID, cid)
b.registerClient(ws2, cid)
ws2.clientsMu.Lock()
require.False(t, ws2.clients[cid].released)
ws2.clientsMu.Unlock()
//nolint:forbidigo // The superseded 50ms timer has to be given its chance to fire.
time.Sleep(200 * time.Millisecond)
require.Equal(t, int32(0), ws2Shutdowns.Load(),
"the re-armed claim must outlive the timer it replaced")
}
// -- Client retirement --
// TestRetireClient_ReleasesEveryClaim checks the primitive a quitting
// client relies on: one call drops its claims everywhere, whether they
// are held by a stream, a create hold, or a detach grace.
func TestRetireClient_ReleasesEveryClaim(t *testing.T) {
t.Parallel()
b, _ := newTestBackend(t)
b.SetDetachGrace(time.Hour)
b.createGrace = time.Hour
streamed, streamedShutdowns := insertTestWorkspace(t, b, "/tmp/retire-stream")
held, heldShutdowns := insertTestWorkspace(t, b, "/tmp/retire-hold")
shared, sharedShutdowns := insertTestWorkspace(t, b, "/tmp/retire-shared")
cid, other := newClientID(t), newClientID(t)
b.registerClient(streamed, cid)
require.NoError(t, b.AttachClient(streamed.ID, cid))
b.registerClient(held, cid)
b.registerClient(shared, cid)
b.registerClient(shared, other)
require.NoError(t, b.RetireClient(cid))
require.Equal(t, int32(1), streamedShutdowns.Load())
require.Equal(t, int32(1), heldShutdowns.Load())
require.Equal(t, int32(0), sharedShutdowns.Load(),
"a workspace another client is still using must be left alone")
shared.clientsMu.Lock()
require.NotContains(t, shared.clients, cid)
require.Contains(t, shared.clients, other)
shared.clientsMu.Unlock()
// Idempotent: retiring twice is not an error and changes nothing.
require.NoError(t, b.RetireClient(cid))
require.Equal(t, int32(1), streamedShutdowns.Load())
}
// TestRetireClient_RefusesLaterCreates is what makes teardown exact
// without timing guesses. A recovery create whose response the client
// never saw can still land on the server afterwards; refusing it is the
// only way to guarantee it cannot leave a workspace nobody can name.
func TestRetireClient_RefusesLaterCreates(t *testing.T) {
t.Setenv("HOME", t.TempDir())
t.Setenv("XDG_CACHE_HOME", t.TempDir())
t.Setenv("XDG_CONFIG_HOME", t.TempDir())
t.Setenv("XDG_DATA_HOME", t.TempDir())
b := New(context.Background(), nil, func() {})
t.Cleanup(func() { drainBackend(t, b) })
cid := newClientID(t)
require.NoError(t, b.RetireClient(cid))
_, _, err := b.CreateWorkspace(protoWS(t.TempDir(), t.TempDir(), cid))
require.ErrorIs(t, err, ErrClientRetired)
require.Zero(t, b.workspaces.Len(), "a refused create must register nothing")
}
// TestRetireClient_DuringPendingCreate drives the actual ordering the
// design has to survive: retirement lands while a create is already
// inside its slow initialization, holding b.mu released. The create must
// notice at its commit point and discard the workspace it built rather
// than registering a claim for a client that has already gone.
func TestRetireClient_DuringPendingCreate(t *testing.T) {
t.Setenv("HOME", t.TempDir())
t.Setenv("XDG_CACHE_HOME", t.TempDir())
t.Setenv("XDG_CONFIG_HOME", t.TempDir())
t.Setenv("XDG_DATA_HOME", t.TempDir())
b := New(context.Background(), nil, func() {})
t.Cleanup(func() { drainBackend(t, b) })
cid := newClientID(t)
cwd, dataDir := t.TempDir(), t.TempDir()
createErr := make(chan error, 1)
go func() {
_, _, err := b.CreateWorkspace(protoWS(cwd, dataDir, cid))
createErr <- err
}()
// pending is bumped under b.mu before the create releases the lock for
// its slow init (config, db, app), and dropped only after the workspace
// is registered. Observing pending == 1 therefore means the create is
// genuinely mid-flight with b.mu free — the exact window retirement has
// to cover. Nothing here fakes the race; the create really is running.
require.Eventually(t, func() bool {
b.mu.Lock()
defer b.mu.Unlock()
return b.pending == 1
}, 30*time.Second, time.Millisecond, "create must reach its slow path")
require.NoError(t, b.RetireClient(cid))
require.ErrorIs(t, <-createErr, ErrClientRetired,
"a create that commits after retirement must be refused")
require.Zero(t, b.workspaces.Len(), "no workspace may be left behind")
}
// -- Shutdown as an atomic decision --
// TestShutdownIfIdle_RefusesWhileWorkspaceLive is the guard that keeps a
// second session alive: an upgrading client asks a mismatched server to
// stand down, and only the server can answer without racing a session
// that arrives between the question and the answer.
func TestShutdownIfIdle_RefusesWhileWorkspaceLive(t *testing.T) {
t.Parallel()
b, shutdowns := newTestBackend(t)
ws, _ := insertTestWorkspace(t, b, "/tmp/in-use")
cid := newClientID(t)
b.registerClient(ws, cid)
require.False(t, b.ShutdownIfIdle(), "a server hosting a workspace must refuse")
require.Equal(t, int32(0), shutdowns.Load())
// Refusing must not latch anything: the server keeps serving.
b.mu.Lock()
require.False(t, b.closing)
b.mu.Unlock()
}
// TestShutdownIfIdle_RefusesWhileCreatePending closes the subtler half of
// the same window: a workspace still working through its slow startup is
// not yet in the workspace map, so a count-based check would call the
// server idle and kill a session that is being born.
func TestShutdownIfIdle_RefusesWhileCreatePending(t *testing.T) {
t.Parallel()
b, shutdowns := newTestBackend(t)
b.mu.Lock()
b.pending = 1
b.mu.Unlock()
require.False(t, b.ShutdownIfIdle())
require.Equal(t, int32(0), shutdowns.Load())
}
// TestShutdownIfIdle_GrantedAndFinal preserves upgrades: an idle server
// does stand down, and the decision is final so a create arriving
// afterwards is refused instead of being initialized on a dying process.
func TestShutdownIfIdle_GrantedAndFinal(t *testing.T) {
t.Parallel()
b, shutdowns := newTestBackend(t)
require.True(t, b.ShutdownIfIdle())
require.Equal(t, int32(1), shutdowns.Load())
_, _, err := b.CreateWorkspace(protoWS(t.TempDir(), t.TempDir(), newClientID(t)))
require.ErrorIs(t, err, ErrServerShuttingDown)
}
// TestIdleShutdown_RefusesLaterCreates covers the same finality for the
// unprompted path: a server that decided to exit because it went idle
// must not accept the create of a client that arrives a moment later.
func TestIdleShutdown_RefusesLaterCreates(t *testing.T) {
t.Parallel()
b, shutdowns := newTestBackend(t)
b.SetIdleShutdownDelay(10 * time.Millisecond)
ws, _ := insertTestWorkspace(t, b, "/tmp/going-idle")
cid := newClientID(t)
require.NoError(t, b.AttachClient(ws.ID, cid))
b.DetachClient(ws.ID, cid)
require.Eventually(t, func() bool { return shutdowns.Load() == 1 },
2*time.Second, 10*time.Millisecond)
_, _, err := b.CreateWorkspace(protoWS(t.TempDir(), t.TempDir(), cid))
require.ErrorIs(t, err, ErrServerShuttingDown,
"the client must be told to retry against a replacement server")
}
+46 -5
View File
@@ -3,6 +3,7 @@ package client
import (
"context"
"encoding/json"
"errors"
"fmt"
"io"
"net"
@@ -115,17 +116,57 @@ func (c *Client) VersionInfo(ctx context.Context) (*proto.VersionInfo, error) {
return &vi, nil
}
// ShutdownServer sends a shutdown request to the server.
func (c *Client) ShutdownServer(ctx context.Context) error {
// ShutdownServerIfIdle asks the server to shut down, which it grants only
// if it is hosting nothing. There is deliberately no unconditional
// variant: a client only ever wants a server replaced, never other
// sessions killed.
//
// A server that declines because it is in use returns an error wrapping
// [ErrServerBusy]. A server too old to know the command returns
// [ErrUnsupported]; it must be left running, since the shutdown request
// it does understand is unconditional and would take its sessions down.
func (c *Client) ShutdownServerIfIdle(ctx context.Context) error {
rsp, err := c.post(ctx, "/control", nil, jsonBody(proto.ServerControl{
Command: "shutdown",
Command: proto.ServerControlShutdownIfIdle,
}), nil)
if err != nil {
return err
}
defer rsp.Body.Close()
if rsp.StatusCode != http.StatusOK {
return fmt.Errorf("server shutdown failed: %s", rsp.Status)
if rsp.StatusCode == http.StatusOK {
return nil
}
failure := fmt.Errorf("server shutdown failed: %s", rsp.Status)
switch rsp.StatusCode {
case http.StatusConflict:
return fmt.Errorf("%w: %w", ErrServerBusy, failure)
case http.StatusBadRequest:
// The only way a well-formed control request is rejected as bad
// is an unknown command, i.e. a server predating this one.
return fmt.Errorf("%w: %w", ErrUnsupported, failure)
}
return failure
}
// RetireClient tells the server this client has exited, releasing every
// claim it holds on every workspace. It is the client's authoritative
// goodbye: after it returns, the server refuses further workspace
// creates from this client ID, so a create whose response was lost cannot
// leave a workspace nobody can name.
//
// Servers predating the endpoint answer 404, reported as
// [ErrUnsupported] so callers can fall back to releasing by workspace ID.
func (c *Client) RetireClient(ctx context.Context) error {
rsp, err := c.delete(ctx, "/clients/"+c.clientID, nil, nil)
if err != nil {
return err
}
defer rsp.Body.Close()
if err := checkStatus(rsp); err != nil {
if errors.Is(err, ErrNotFound) {
return fmt.Errorf("%w: %w", ErrUnsupported, err)
}
return fmt.Errorf("failed to retire client: %w", err)
}
return nil
}
+64
View File
@@ -0,0 +1,64 @@
package client
import (
"errors"
"fmt"
"net/http"
"slices"
)
// Typed outcomes callers need to distinguish from ordinary transport
// failures. Match them with errors.Is.
var (
// ErrNotFound reports that the server answered 404. For a
// workspace-scoped call this means the server no longer knows the
// workspace — it was torn down, or the server was replaced under the
// client — so the right response is to re-register rather than retry
// the same ID, which can never start succeeding again.
ErrNotFound = errors.New("not found")
// ErrServerBusy reports that the server declined to shut down
// because it is still hosting workspaces or is midway through
// creating one. A client asking a version-mismatched server to stand
// down must keep using it instead of assuming it is going away.
ErrServerBusy = errors.New("server busy")
// ErrServerShuttingDown reports that the server refused the request
// because it has already committed to exiting. The work is not lost:
// a replacement server can be started and the request retried
// against it.
ErrServerShuttingDown = errors.New("server is shutting down")
// ErrUnsupported reports that the running server does not understand
// the request because it predates the feature. Callers must decide
// what is safe to do with an older server rather than treating the
// failure as transient.
ErrUnsupported = errors.New("unsupported by the running server")
)
// checkStatus returns nil when rsp's status code is one of ok
// (http.StatusOK when none are given). Otherwise it returns an error
// carrying the status code and, when the body decodes as a proto.Error,
// the server-provided message. Statuses that callers act on are wrapped
// in the matching sentinel. checkStatus may consume the response body.
func checkStatus(rsp *http.Response, ok ...int) error {
if len(ok) == 0 {
ok = []int{http.StatusOK}
}
if slices.Contains(ok, rsp.StatusCode) {
return nil
}
var err error
if msg := decodeErrorMessage(rsp.Body); msg != "" {
err = fmt.Errorf("status code %d: %s", rsp.StatusCode, msg)
} else {
err = fmt.Errorf("status code %d", rsp.StatusCode)
}
switch rsp.StatusCode {
case http.StatusNotFound:
return fmt.Errorf("%w: %w", ErrNotFound, err)
case http.StatusServiceUnavailable:
return fmt.Errorf("%w: %w", ErrServerShuttingDown, err)
}
return err
}
+92
View File
@@ -0,0 +1,92 @@
package client
import (
"io"
"net/http"
"strings"
"testing"
"github.com/stretchr/testify/require"
)
// TestCheckStatus_TypesActionableStatuses is the regression for the bug
// that made a lost workspace unrecoverable: every response used to
// collapse into an opaque "status code %d" string, so no caller could
// tell "the server no longer knows my workspace" from any other failure,
// and the client just retried the dead ID forever.
func TestCheckStatus_TypesActionableStatuses(t *testing.T) {
t.Parallel()
tests := []struct {
name string
status int
body string
accept []int
wantErr error
// wantUntyped asserts the error carries no lifecycle meaning, so
// callers keep treating it as an ordinary failure.
wantUntyped bool
wantMsg string
}{
{name: "ok by default", status: http.StatusOK},
{
name: "accepted when listed",
status: http.StatusAccepted,
accept: []int{http.StatusOK, http.StatusAccepted},
},
{
name: "not found triggers workspace recovery",
status: http.StatusNotFound,
wantErr: ErrNotFound,
},
{
name: "service unavailable means retry against a replacement",
status: http.StatusServiceUnavailable,
wantErr: ErrServerShuttingDown,
},
{
name: "server message is surfaced",
status: http.StatusNotFound,
body: `{"message":"workspace not found"}`,
wantErr: ErrNotFound,
wantMsg: "workspace not found",
},
{
name: "conflict carries no lifecycle meaning",
status: http.StatusConflict,
wantUntyped: true,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
body := tc.body
if body == "" {
body = "{}"
}
err := checkStatus(&http.Response{
StatusCode: tc.status,
Body: io.NopCloser(strings.NewReader(body)),
}, tc.accept...)
if tc.wantErr == nil && !tc.wantUntyped {
require.NoError(t, err)
return
}
require.Error(t, err)
require.Contains(t, err.Error(), "status code")
if tc.wantErr != nil {
require.ErrorIs(t, err, tc.wantErr)
}
if tc.wantUntyped {
require.NotErrorIs(t, err, ErrNotFound)
require.NotErrorIs(t, err, ErrServerShuttingDown)
}
if tc.wantMsg != "" {
require.Contains(t, err.Error(), tc.wantMsg)
}
})
}
}
+10 -10
View File
@@ -45,8 +45,8 @@ func (c *Client) CreateWorkspace(ctx context.Context, ws proto.Workspace) (*prot
return nil, fmt.Errorf("failed to create workspace: %w", err)
}
defer rsp.Body.Close()
if rsp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("failed to create workspace: status code %d", rsp.StatusCode)
if err := checkStatus(rsp); err != nil {
return nil, fmt.Errorf("failed to create workspace: %w", err)
}
var created proto.Workspace
if err := json.NewDecoder(rsp.Body).Decode(&created); err != nil {
@@ -62,8 +62,8 @@ func (c *Client) GetWorkspace(ctx context.Context, id string) (*proto.Workspace,
return nil, fmt.Errorf("failed to get workspace: %w", err)
}
defer rsp.Body.Close()
if rsp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("failed to get workspace: status code %d", rsp.StatusCode)
if err := checkStatus(rsp); err != nil {
return nil, fmt.Errorf("failed to get workspace: %w", err)
}
var ws proto.Workspace
if err := json.NewDecoder(rsp.Body).Decode(&ws); err != nil {
@@ -104,8 +104,8 @@ func (c *Client) SetCurrentSession(ctx context.Context, workspaceID, sessionID s
return fmt.Errorf("failed to set current session: %w", err)
}
defer rsp.Body.Close()
if rsp.StatusCode != http.StatusOK {
return fmt.Errorf("failed to set current session: status code %d", rsp.StatusCode)
if err := checkStatus(rsp); err != nil {
return fmt.Errorf("failed to set current session: %w", err)
}
return nil
}
@@ -124,9 +124,9 @@ func (c *Client) SubscribeEvents(ctx context.Context, id string) (<-chan any, er
return nil, fmt.Errorf("failed to subscribe to events: %w", err)
}
if rsp.StatusCode != http.StatusOK {
if err := checkStatus(rsp); err != nil {
rsp.Body.Close()
return nil, fmt.Errorf("failed to subscribe to events: status code %d", rsp.StatusCode)
return nil, fmt.Errorf("failed to subscribe to events: %w", err)
}
go func() {
@@ -400,8 +400,8 @@ func (c *Client) GetAgentInfo(ctx context.Context, id string) (*proto.AgentInfo,
return nil, fmt.Errorf("failed to get agent status: %w", err)
}
defer rsp.Body.Close()
if rsp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("failed to get agent status: status code %d", rsp.StatusCode)
if err := checkStatus(rsp); err != nil {
return nil, fmt.Errorf("failed to get agent status: %w", err)
}
var info proto.AgentInfo
if err := json.NewDecoder(rsp.Body).Decode(&info); err != nil {
+230
View File
@@ -0,0 +1,230 @@
package cmd
import (
"encoding/json"
"net/http"
"net/http/httptest"
"net/url"
"sync"
"testing"
"github.com/charmbracelet/crush/internal/client"
"github.com/charmbracelet/crush/internal/proto"
"github.com/charmbracelet/crush/internal/version"
"github.com/spf13/cobra"
"github.com/stretchr/testify/require"
)
// controlServerOpts describes the server restartIfStale will find.
type controlServerOpts struct {
// vi is what /version reports.
vi proto.VersionInfo
// legacy makes the server reject the conditional shutdown command as
// unknown, the way every server released before the idleness guard
// does. Such a server obeys a plain shutdown unconditionally, so a
// client must never send it one.
legacy bool
// busy makes the server decline the shutdown because it is in use.
busy bool
}
// controlLog records the control commands a server received.
type controlLog struct {
mu sync.Mutex
commands []string
}
func (l *controlLog) all() []string {
l.mu.Lock()
defer l.mu.Unlock()
return append([]string(nil), l.commands...)
}
func newControlServer(t *testing.T, opts controlServerOpts) (*url.URL, *controlLog) {
t.Helper()
log := &controlLog{}
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/v1/version":
require.NoError(t, json.NewEncoder(w).Encode(opts.vi))
case "/v1/control":
var req proto.ServerControl
require.NoError(t, json.NewDecoder(r.Body).Decode(&req))
log.mu.Lock()
log.commands = append(log.commands, req.Command)
log.mu.Unlock()
switch {
case opts.legacy && req.Command != proto.ServerControlShutdown:
http.Error(w, "unknown command", http.StatusBadRequest)
case opts.busy:
http.Error(w, "server is hosting live workspaces", http.StatusConflict)
default:
w.WriteHeader(http.StatusOK)
}
default:
w.WriteHeader(http.StatusOK)
}
}))
t.Cleanup(srv.Close)
u, err := url.Parse(srv.URL)
require.NoError(t, err)
return &url.URL{Scheme: "tcp", Host: u.Host}, log
}
// staleVersion never matches this build, so restartIfStale always
// considers the server a candidate for replacement.
func staleVersion() proto.VersionInfo {
return proto.VersionInfo{
Version: "not-" + version.Version,
BuildID: "stale-build",
}
}
func versionCheckCmd(t *testing.T) *cobra.Command {
t.Helper()
cmd := &cobra.Command{}
cmd.SetContext(t.Context())
return cmd
}
// TestRestartIfStale_HonorsRefusedShutdown is the headline regression.
// BuildID derives from the executable's mtime, so any rebuild — including
// every `go run` — makes a second session look like an upgrade. Launching
// it used to shut down the first session's server, taking its workspaces
// with it and stranding it on a dead workspace ID. Now the server has the
// final say, and a refusal means "keep using me".
func TestRestartIfStale_HonorsRefusedShutdown(t *testing.T) {
t.Parallel()
hostURL, log := newControlServer(t, controlServerOpts{
vi: staleVersion(),
busy: true,
})
restarted, err := restartIfStale(versionCheckCmd(t), hostURL)
require.NoError(t, err)
require.False(t, restarted, "a refused shutdown must not be reported as a restart")
require.Equal(t, []string{proto.ServerControlShutdownIfIdle}, log.all(),
"the request must be the conditional one the server can veto")
}
// TestRestartIfStale_LeavesLegacyServerRunning is the upgrade-transition
// case. A server predating the idleness guard rejects the conditional
// command, and the only command it does understand is unconditional —
// sending that would kill every session it hosts. It has to be left
// alone; it shuts itself down when it goes idle, and the client after that
// finds no socket and spawns a current server.
func TestRestartIfStale_LeavesLegacyServerRunning(t *testing.T) {
t.Parallel()
hostURL, log := newControlServer(t, controlServerOpts{
vi: staleVersion(),
legacy: true,
})
restarted, err := restartIfStale(versionCheckCmd(t), hostURL)
require.NoError(t, err)
require.False(t, restarted, "a legacy server must be reused, not replaced")
require.Equal(t, []string{proto.ServerControlShutdownIfIdle}, log.all(),
"the unconditional shutdown must never be sent")
}
// TestRestartIfStale_RestartsWhenServerAgrees preserves the upgrade path:
// a mismatched server that grants the shutdown is replaced promptly.
func TestRestartIfStale_RestartsWhenServerAgrees(t *testing.T) {
t.Parallel()
hostURL, log := newControlServer(t, controlServerOpts{vi: staleVersion()})
restarted, err := restartIfStale(versionCheckCmd(t), hostURL)
require.NoError(t, err)
require.True(t, restarted, "an idle stale server must be restarted")
require.Equal(t, []string{proto.ServerControlShutdownIfIdle}, log.all())
}
// TestRestartIfStale_MatchingServerUntouched is the control: same version
// and build ID means no control request at all.
func TestRestartIfStale_MatchingServerUntouched(t *testing.T) {
t.Parallel()
hostURL, log := newControlServer(t, controlServerOpts{
vi: proto.VersionInfo{Version: version.Version, BuildID: version.BuildID},
})
restarted, err := restartIfStale(versionCheckCmd(t), hostURL)
require.NoError(t, err)
require.False(t, restarted)
require.Empty(t, log.all())
}
// TestCreateWorkspaceOnLiveServer_RetriesPastExitingServer covers the
// startup race left over from making the shutdown decision final: a client
// can reach a server in the instant between its committing to an idle
// shutdown and its socket disappearing. Failing the CLI there would be a
// regression, so the client has to bring up a replacement and ask again.
func TestCreateWorkspaceOnLiveServer_RetriesPastExitingServer(t *testing.T) {
t.Parallel()
var mu sync.Mutex
replaced := 0
creates := 0
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
require.Equal(t, "/v1/workspaces", r.URL.Path)
mu.Lock()
creates++
// Only once the client has stood up a replacement does the create
// succeed, so passing this test requires the retry to be real.
serve := replaced > 0
mu.Unlock()
if !serve {
http.Error(w, "server is shutting down", http.StatusServiceUnavailable)
return
}
require.NoError(t, json.NewEncoder(w).Encode(proto.Workspace{ID: "ws-new"}))
}))
t.Cleanup(srv.Close)
u, err := url.Parse(srv.URL)
require.NoError(t, err)
c, err := client.NewClient(t.TempDir(), "tcp", u.Host)
require.NoError(t, err)
ws, err := createWorkspaceOnLiveServer(t.Context(), c, proto.Workspace{Path: t.TempDir()},
func() error {
mu.Lock()
replaced++
mu.Unlock()
return nil
})
require.NoError(t, err)
require.Equal(t, "ws-new", ws.ID)
mu.Lock()
defer mu.Unlock()
require.Equal(t, 1, replaced, "exactly one replacement must be spawned")
require.Equal(t, 2, creates)
}
// TestCreateWorkspaceOnLiveServer_GivesUpOnOtherFailures makes sure the
// retry is scoped to the shutdown race and does not paper over real
// errors by respawning servers.
func TestCreateWorkspaceOnLiveServer_GivesUpOnOtherFailures(t *testing.T) {
t.Parallel()
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
http.Error(w, "nope", http.StatusInternalServerError)
}))
t.Cleanup(srv.Close)
u, err := url.Parse(srv.URL)
require.NoError(t, err)
c, err := client.NewClient(t.TempDir(), "tcp", u.Host)
require.NoError(t, err)
_, err = createWorkspaceOnLiveServer(t.Context(), c, proto.Workspace{Path: t.TempDir()},
func() error {
t.Fatal("a non-lifecycle failure must not spawn a replacement server")
return nil
})
require.Error(t, err)
}
+104 -21
View File
@@ -346,7 +346,7 @@ func localSkillsDiscoveryConfig(store *config.ConfigStore) skills.DiscoveryConfi
// setupClientServerWorkspace connects to a server process and wraps the
// result in a ClientWorkspace.
func setupClientServerWorkspace(cmd *cobra.Command) (workspace.Workspace, func(), error) {
c, protoWs, cleanupServer, err := connectToServer(cmd)
c, protoWs, _, err := connectToServer(cmd)
if err != nil {
return nil, nil, err
}
@@ -359,7 +359,10 @@ func setupClientServerWorkspace(cmd *cobra.Command) (workspace.Workspace, func()
}
}
return clientWs, cleanupServer, nil
// Clean up via Shutdown rather than connectToServer's closure: it stops
// the subscription's reconnect/recovery loop first, so our own exit
// cannot be mistaken for a lost workspace and re-created mid-quit.
return clientWs, clientWs.Shutdown, nil
}
// connectToServer ensures the server is running, creates a client and
@@ -378,7 +381,6 @@ func connectToServer(cmd *cobra.Command) (*client.Client, *proto.Workspace, func
yolo, _ := cmd.Flags().GetBool("yolo")
channels, _ := cmd.Flags().GetStringSlice("channels")
dataDir, _ := cmd.Flags().GetString("data-dir")
ctx := cmd.Context()
cwd, err := ResolveCwd(cmd)
if err != nil {
@@ -400,9 +402,11 @@ func connectToServer(cmd *cobra.Command) (*client.Client, *proto.Workspace, func
Env: os.Environ(),
}
ws, err := c.CreateWorkspace(ctx, wsReq)
ws, err := createWorkspaceOnLiveServer(cmd.Context(), c, wsReq, func() error {
return replaceExitingServer(cmd, hostURL)
})
if err != nil {
return nil, nil, nil, fmt.Errorf("failed to create workspace: %v", err)
return nil, nil, nil, err
}
if shouldEnableMetrics(ws.Config) {
@@ -414,10 +418,65 @@ func connectToServer(cmd *cobra.Command) (*client.Client, *proto.Workspace, func
crushlog.Setup(logFile, debug)
}
cleanup := func() { _ = c.DeleteWorkspace(context.Background(), ws.ID) }
// Retiring the client releases every claim it holds, so it covers
// workspaces this process created but never learned the ID of.
cleanup := func() {
if err := c.RetireClient(context.Background()); err != nil {
_ = c.DeleteWorkspace(context.Background(), ws.ID)
}
}
return c, ws, cleanup, nil
}
// maxStaleServerRetries bounds how many times workspace creation may be
// retried against a replacement server. Only one client can lose the race
// against a given server's shutdown, so a single retry is normally enough;
// the bound just keeps a pathological loop finite.
const maxStaleServerRetries = 3
// createWorkspaceOnLiveServer creates the workspace, retrying against a
// replacement when the server it reached has already committed to shutting
// itself down for being idle.
//
// That race is unavoidable: the server decides to exit while no client is
// talking to it, and a client can arrive between that decision and the
// socket going away. The decision is final on the server's side, so the
// only correct response is to bring up a fresh server and ask again
// instead of failing the command.
func createWorkspaceOnLiveServer(
ctx context.Context, c *client.Client, req proto.Workspace, replace func() error,
) (*proto.Workspace, error) {
for attempt := range maxStaleServerRetries {
ws, err := c.CreateWorkspace(ctx, req)
if err == nil {
return ws, nil
}
if !errors.Is(err, client.ErrServerShuttingDown) || attempt == maxStaleServerRetries-1 {
return nil, fmt.Errorf("failed to create workspace: %v", err)
}
slog.Warn("Server is shutting down; retrying against a replacement",
"attempt", attempt+1, "error", err)
if err := replace(); err != nil {
return nil, err
}
}
return nil, fmt.Errorf("failed to create workspace: server kept shutting down")
}
// replaceExitingServer waits out the socket of a server that has committed
// to exiting, then brings up a fresh one.
func replaceExitingServer(cmd *cobra.Command, hostURL *url.URL) error {
if hostURL.Scheme == "unix" {
if err := awaitSocketGone(cmd.Context(), hostURL); err != nil {
return err
}
}
if err := spawnAndWaitReady(cmd, hostURL); err != nil {
return fmt.Errorf("failed to initialize crush server: %v", err)
}
return nil
}
// ensureServer auto-starts a detached server if the socket file does not
// exist. When the socket exists, it verifies that the running server
// version matches the client; on mismatch it shuts down the old server
@@ -673,12 +732,21 @@ func probeHealth(ctx context.Context, h *http.Client, reqURL string, hostURL *ur
}
// restartIfStale checks whether the running server matches the current
// client version. When they differ, it sends a shutdown command and
// removes the stale socket so the caller can start a fresh server.
// client version. When they differ it asks the server to stand down and,
// if it agrees, removes the stale socket so the caller can start a fresh
// server.
//
// It returns restarted=true when it has shut down a stale server and the
// caller must spawn a new one. When the server matches the client version
// (or the check itself fails), restarted is false.
// The request is conditional and the server has the last word: it refuses
// while it is hosting anything, because BuildID derives from the
// executable's mtime, so any rebuild (including every `go run`) makes a
// second session look like an upgrade and would otherwise kill the first
// session's workspaces. Servers too old to understand the conditional
// command are left running for the same reason — the request they do
// understand is unconditional. They shut themselves down when they go
// idle, and the next client then finds no socket and spawns a current one.
//
// It returns restarted=true only when the server accepted the shutdown and
// the caller must spawn a replacement.
func restartIfStale(cmd *cobra.Command, hostURL *url.URL) (restarted bool, err error) {
c, err := client.NewClient("", hostURL.Scheme, hostURL.Host)
if err != nil {
@@ -691,28 +759,43 @@ func restartIfStale(cmd *cobra.Command, hostURL *url.URL) (restarted bool, err e
if vi.Version == version.Version && vi.BuildID == version.BuildID {
return false, nil
}
slog.Info(
"Server version mismatch, restarting",
versionFields := []any{
"server_version", vi.Version,
"client_version", version.Version,
"server_build_id", vi.BuildID,
"client_build_id", version.BuildID,
)
_ = c.ShutdownServer(cmd.Context())
// Give the old process a moment to release the socket.
}
// Every refusal — in use, too old to be asked, or unreachable — leads to
// the same safe outcome: keep using the running server. The wrapped
// error says which it was.
if err := c.ShutdownServerIfIdle(cmd.Context()); err != nil {
slog.Warn("Server version differs but it will not stand down; reusing it",
append(versionFields, "error", err)...)
return false, nil
}
slog.Info("Stale server accepted shutdown, restarting", versionFields...)
if err := awaitSocketGone(cmd.Context(), hostURL); err != nil {
return true, err
}
return true, nil
}
// awaitSocketGone gives a server that has committed to exiting a moment to
// release its socket, then force-removes whatever is left: the old process
// has latched its decision and will not serve requests again.
func awaitSocketGone(ctx context.Context, hostURL *url.URL) error {
for range 20 {
if _, err := os.Stat(hostURL.Host); errors.Is(err, fs.ErrNotExist) {
break
return nil
}
select {
case <-cmd.Context().Done():
return true, cmd.Context().Err()
case <-ctx.Done():
return ctx.Err()
case <-time.After(100 * time.Millisecond):
}
}
// Force-remove if the socket is still lingering.
_ = os.Remove(hostURL.Host)
return true, nil
return nil
}
var safeNameRegexp = regexp.MustCompile(`[^a-zA-Z0-9._-]`)
+15
View File
@@ -1,5 +1,20 @@
package proto
// Server control commands accepted by the /control endpoint.
const (
// ServerControlShutdown is the original, unconditional spelling.
// Servers still accept it, but now apply the same idleness check as
// ServerControlShutdownIfIdle so that clients predating the check
// cannot take live sessions down.
ServerControlShutdown = "shutdown"
// ServerControlShutdownIfIdle asks the server to shut down only if it
// is idle. It exists so a client can tell whether the server checks
// that condition at all: servers predating the check reject the
// unknown command instead of shutting down and taking other sessions
// with them.
ServerControlShutdownIfIdle = "shutdown_if_idle"
)
// ServerControl represents a server control request.
type ServerControl struct {
Command string `json:"command"`
+17 -7
View File
@@ -342,10 +342,15 @@ func drainUntil[T any](ctx context.Context, evc <-chan any, match func(T) bool)
// config.Init does not read the host machine's real config.
func TestE2E_TwoClientsReceiveSameMessage(t *testing.T) {
h := newRealCreateHarness(t)
// Shorten the create-grace window so the workspace's pending
// creation holds release quickly during test cleanup once both
// SSE streams have been detached.
// Shorten the lifecycle windows so the workspace's client holds
// release quickly during test cleanup once both SSE streams have
// been detached: the create grace covers the window before the
// streams attach, the detach grace the window after they drop.
// The pooled DB connection is only released once the last hold
// expires, and Windows cannot remove the temp data directory
// while that connection is still open.
h.backend.SetCreateGrace(200 * time.Millisecond)
h.backend.SetDetachGrace(200 * time.Millisecond)
ctx, cancel := context.WithCancel(t.Context())
t.Cleanup(cancel)
@@ -463,7 +468,9 @@ func TestE2E_PermissionFlowCrossClient(t *testing.T) {
// Wait for the PermissionRequest to arrive on client A's SSE
// stream. We need its ID to drive the grant.
pickCtx, pickCancel := context.WithTimeout(ctx, 3*time.Second)
// 10s instead of the 3s used elsewhere because this context
// bounds three sequential waits, not one.
pickCtx, pickCancel := context.WithTimeout(ctx, 10*time.Second)
defer pickCancel()
reqEv, ok := drainUntil(pickCtx, evcA, func(e pubsub.Event[proto.PermissionRequest]) bool {
return e.Payload.ToolCallID == toolCallID
@@ -572,8 +579,10 @@ func TestE2E_KillingClientASSEDoesNotBreakClientB(t *testing.T) {
func TestE2E_ShutdownCallbackFiresWhenLastClientLeaves(t *testing.T) {
t.Parallel()
h := newE2EHarness(t)
// Shorten the idle linger so the test doesn't wait out the
// production window; the callback still fires, just after the delay.
// Shorten both lifecycle windows so the test doesn't wait out the
// production values. Both stay non-zero, so teardown still travels the
// real path: detach grace, then idle linger.
h.backend.SetDetachGrace(100 * time.Millisecond)
h.backend.SetIdleShutdownDelay(200 * time.Millisecond)
ctxA, cancelA := context.WithCancel(t.Context())
@@ -603,7 +612,8 @@ func TestE2E_ShutdownCallbackFiresWhenLastClientLeaves(t *testing.T) {
killB()
require.Eventually(t, h.shutdownHit.Load,
3*time.Second, 10*time.Millisecond,
"shutdown callback must fire once the last client disconnects (after the idle linger)")
"shutdown callback must fire once the last client disconnects "+
"(after the detach grace and the idle linger)")
// Workspace must be gone from the index.
_, err := h.backend.GetWorkspace(h.workspace.ID)
+5 -2
View File
@@ -196,7 +196,10 @@ func TestPostCurrentSession_UnknownClient(t *testing.T) {
ws := installSyntheticWorkspace(t, c)
rec := postCurrentSession(t, c, ws.ID, uuid.New().String(), "S1")
require.Equal(t, http.StatusNotFound, rec.Code)
// 409, not 404: the workspace is fine, this client just has no stream
// on it. A 404 would look like "workspace gone" and send a recovering
// client off to re-register for no reason.
require.Equal(t, http.StatusConflict, rec.Code)
}
func TestPostCurrentSession_HoldOnly(t *testing.T) {
@@ -209,7 +212,7 @@ func TestPostCurrentSession_HoldOnly(t *testing.T) {
t.Cleanup(func() { _ = c.backend.DeleteWorkspace(ws.ID, cid) })
rec := postCurrentSession(t, c, ws.ID, cid, "S1")
require.Equal(t, http.StatusNotFound, rec.Code, "hold-only client must be rejected")
require.Equal(t, http.StatusConflict, rec.Code, "hold-only client must be rejected")
}
func TestPostCurrentSession_AttachedClientSucceeds(t *testing.T) {
+39 -5
View File
@@ -43,9 +43,10 @@ func (c *controllerV1) handleGetVersion(w http.ResponseWriter, _ *http.Request)
// @Summary Send server control command
// @Tags system
// @Accept json
// @Param request body proto.ServerControl true "Control command (e.g. shutdown)"
// @Param request body proto.ServerControl true "Control command (e.g. shutdown, shutdown_if_idle)"
// @Success 200
// @Failure 400 {object} proto.Error
// @Failure 409 {object} proto.Error
// @Router /control [post]
func (c *controllerV1) handlePostControl(w http.ResponseWriter, r *http.Request) {
var req proto.ServerControl
@@ -56,8 +57,16 @@ func (c *controllerV1) handlePostControl(w http.ResponseWriter, r *http.Request)
}
switch req.Command {
case "shutdown":
c.backend.Shutdown()
case proto.ServerControlShutdown, proto.ServerControlShutdownIfIdle:
// Both spellings are conditional. Only the backend can rule on
// idleness without racing a session that arrives between a
// client's own check and its request, and guarding the plain
// command too means clients predating the check cannot take live
// sessions down either.
if !c.backend.ShutdownIfIdle() {
c.handleError(w, r, backend.ErrServerNotIdle)
return
}
default:
c.server.logError(r, "Unknown command", "command", req.Command)
jsonError(w, http.StatusBadRequest, "unknown command")
@@ -184,6 +193,21 @@ func (c *controllerV1) handlePostWorkspaceCurrentSession(w http.ResponseWriter,
}
}
// handleDeleteClient retires a client, releasing every claim it holds.
//
// @Summary Retire a client
// @Tags system
// @Param client_id path string true "Client ID (UUID)"
// @Success 200
// @Failure 400 {object} proto.Error
// @Router /clients/{client_id} [delete]
func (c *controllerV1) handleDeleteClient(w http.ResponseWriter, r *http.Request) {
if err := c.backend.RetireClient(r.PathValue("client_id")); err != nil {
c.handleError(w, r, err)
return
}
}
// handleDeleteWorkspaces deletes a workspace.
//
// @Summary Delete workspace
@@ -1161,9 +1185,19 @@ func (c *controllerV1) handleError(w http.ResponseWriter, r *http.Request, err e
case errors.Is(err, backend.ErrInvalidClientID):
status = http.StatusBadRequest
case errors.Is(err, backend.ErrClientNotAttached):
status = http.StatusNotFound
case errors.Is(err, backend.ErrWorkspaceClosing):
// 409, not 404: the workspace exists, the caller just has no live
// stream yet. A 404 here is indistinguishable from "workspace
// gone" and would trip clients that treat 404 as the trigger for
// workspace recovery.
status = http.StatusConflict
case errors.Is(err, backend.ErrWorkspaceClosing),
errors.Is(err, backend.ErrServerNotIdle),
errors.Is(err, backend.ErrClientRetired):
status = http.StatusConflict
case errors.Is(err, backend.ErrServerShuttingDown):
// 503, not 409: the request is not wrong, this process is just
// leaving. Clients retry against its replacement.
status = http.StatusServiceUnavailable
}
c.server.logError(r, err.Error())
jsonError(w, status, err.Error())
+8
View File
@@ -105,6 +105,13 @@ func (s *Server) SetLogger(logger *slog.Logger) {
s.logger = logger
}
// Backend returns the server's backend. Intended for integration tests
// that drive lifecycle transitions (detach, grace tuning) against a live
// HTTP surface.
func (s *Server) Backend() *backend.Backend {
return s.backend
}
// DefaultServer returns a new [Server] with the default address.
func DefaultServer(cfg *config.ConfigStore) *Server {
hostURL, err := ParseHostURL(DefaultHost())
@@ -152,6 +159,7 @@ func (s *Server) installHandler() {
mux.HandleFunc("GET /v1/version", c.handleGetVersion)
mux.HandleFunc("GET /v1/config", c.handleGetConfig)
mux.HandleFunc("POST /v1/control", c.handlePostControl)
mux.HandleFunc("DELETE /v1/clients/{client_id}", c.handleDeleteClient)
mux.HandleFunc("GET /v1/workspaces", c.handleGetWorkspaces)
mux.HandleFunc("POST /v1/workspaces", c.handlePostWorkspaces)
mux.HandleFunc("DELETE /v1/workspaces/{id}", c.handleDeleteWorkspaces)
+35 -1
View File
@@ -22,6 +22,34 @@ const docTemplate = `{
"host": "{{.Host}}",
"basePath": "{{.BasePath}}",
"paths": {
"/clients/{client_id}": {
"delete": {
"tags": [
"system"
],
"summary": "Retire a client",
"parameters": [
{
"type": "string",
"description": "Client ID (UUID)",
"name": "client_id",
"in": "path",
"required": true
}
],
"responses": {
"200": {
"description": "OK"
},
"400": {
"description": "Bad Request",
"schema": {
"$ref": "#/definitions/proto.Error"
}
}
}
}
},
"/config": {
"get": {
"produces": [
@@ -52,7 +80,7 @@ const docTemplate = `{
"summary": "Send server control command",
"parameters": [
{
"description": "Control command (e.g. shutdown)",
"description": "Control command (e.g. shutdown, shutdown_if_idle)",
"name": "request",
"in": "body",
"required": true,
@@ -70,6 +98,12 @@ const docTemplate = `{
"schema": {
"$ref": "#/definitions/proto.Error"
}
},
"409": {
"description": "Conflict",
"schema": {
"$ref": "#/definitions/proto.Error"
}
}
}
}
+35 -1
View File
@@ -15,6 +15,34 @@
},
"basePath": "/v1",
"paths": {
"/clients/{client_id}": {
"delete": {
"tags": [
"system"
],
"summary": "Retire a client",
"parameters": [
{
"type": "string",
"description": "Client ID (UUID)",
"name": "client_id",
"in": "path",
"required": true
}
],
"responses": {
"200": {
"description": "OK"
},
"400": {
"description": "Bad Request",
"schema": {
"$ref": "#/definitions/proto.Error"
}
}
}
}
},
"/config": {
"get": {
"produces": [
@@ -45,7 +73,7 @@
"summary": "Send server control command",
"parameters": [
{
"description": "Control command (e.g. shutdown)",
"description": "Control command (e.g. shutdown, shutdown_if_idle)",
"name": "request",
"in": "body",
"required": true,
@@ -63,6 +91,12 @@
"schema": {
"$ref": "#/definitions/proto.Error"
}
},
"409": {
"description": "Conflict",
"schema": {
"$ref": "#/definitions/proto.Error"
}
}
}
}
+23 -1
View File
@@ -1046,6 +1046,24 @@ info:
title: Crush API
version: "1.0"
paths:
/clients/{client_id}:
delete:
parameters:
- description: Client ID (UUID)
in: path
name: client_id
required: true
type: string
responses:
"200":
description: OK
"400":
description: Bad Request
schema:
$ref: '#/definitions/proto.Error'
summary: Retire a client
tags:
- system
/config:
get:
produces:
@@ -1063,7 +1081,7 @@ paths:
consumes:
- application/json
parameters:
- description: Control command (e.g. shutdown)
- description: Control command (e.g. shutdown, shutdown_if_idle)
in: body
name: request
required: true
@@ -1076,6 +1094,10 @@ paths:
description: Bad Request
schema:
$ref: '#/definitions/proto.Error'
"409":
description: Conflict
schema:
$ref: '#/definitions/proto.Error'
summary: Send server control command
tags:
- system
+35
View File
@@ -1333,6 +1333,8 @@ func (m *UI) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
TTL: ttl,
})
cmds = append(cmds, clearInfoMsgCmd(ttl))
case workspace.ConnectionEvent:
cmds = append(cmds, m.handleConnectionEvent(msg)...)
case util.ClearStatusMsg:
m.status.ClearInfoMsg()
case completions.CompletionItemsLoadedMsg:
@@ -1450,6 +1452,39 @@ func (m *UI) setSessionMessages(msgs []message.Message) tea.Cmd {
return tea.Sequence(cmds...)
}
// handleConnectionEvent reports the health of the client-server link and,
// once it recovers, reloads the open session. A reload is always needed
// after a degraded episode: events published while the stream was down are
// gone, and if the workspace itself was re-created any run died with it.
func (m *UI) handleConnectionEvent(msg workspace.ConnectionEvent) []tea.Cmd {
info := util.InfoMsg{
Type: util.InfoTypeWarn,
Msg: "Lost connection to the Crush server — reconnecting…",
TTL: 30 * time.Second,
}
switch msg.State {
case workspace.ConnectionDegraded:
slog.Warn("Server connection degraded", "error", msg.Err, "stuck", msg.Stuck)
if msg.Stuck {
info.Type = util.InfoTypeError
info.Msg = "Can't restore the connection to the Crush server. Restart Crush to recover."
info.TTL = time.Minute
}
case workspace.ConnectionRecovered:
info = util.InfoMsg{
Type: util.InfoTypeSuccess,
Msg: "Reconnected to the Crush server.",
TTL: DefaultStatusTTL,
}
}
m.status.SetInfoMsg(info)
cmds := []tea.Cmd{clearInfoMsgCmd(info.TTL)}
if msg.State == workspace.ConnectionRecovered && m.session != nil {
cmds = append(cmds, m.loadSession(m.session.ID))
}
return cmds
}
// loadNestedToolCalls recursively loads nested tool calls for agent/agentic_fetch tools.
func (m *UI) loadNestedToolCalls(items []chat.MessageItem) {
for _, item := range items {
+208 -11
View File
@@ -7,6 +7,7 @@ import (
"log/slog"
"strings"
"sync"
"sync/atomic"
"time"
tea "charm.land/bubbletea/v2"
@@ -28,6 +29,7 @@ import (
"github.com/charmbracelet/crush/internal/question"
"github.com/charmbracelet/crush/internal/session"
"github.com/charmbracelet/crush/internal/skills"
"github.com/charmbracelet/crush/internal/version"
"github.com/charmbracelet/x/powernap/pkg/lsp/protocol"
)
@@ -41,12 +43,24 @@ type ClientWorkspace struct {
mu sync.RWMutex
ws proto.Workspace
skills *skills.Manager
// lastSession is the most recent session ID reported via
// SetCurrentSession. The subscription loop re-asserts it after a
// reconnect, because the server's per-client presence entry (or the
// whole workspace) may have been re-created in the meantime.
lastSession string
// subCtx bounds the lifetime of the event subscription (and its
// reconnect loop). Shutdown cancels it so Subscribe stops
// reconnecting instead of racing the teardown.
subCtx context.Context
subCancel context.CancelFunc
// subStarted reports whether the subscription loop ever ran, and
// subDone is closed when it returns. Shutdown uses them to let an
// in-flight workspace recovery finish before it says goodbye to the
// server, so the workspace it releases is the one recovery just
// minted.
subStarted atomic.Bool
subDone chan struct{}
// herdrClient reports agent state to herdr when running inside
// a herdr-managed pane. Nil when not in a herdr environment.
@@ -81,6 +95,7 @@ func NewClientWorkspace(c *client.Client, ws proto.Workspace) *ClientWorkspace {
skills: mgr,
subCtx: subCtx,
subCancel: subCancel,
subDone: make(chan struct{}),
herdrClient: herdr.Init(),
}
}
@@ -173,6 +188,9 @@ func (w *ClientWorkspace) ParseAgentToolSessionID(sessionID string) (string, str
// the presence record is a hint, not correctness-critical state.
func (w *ClientWorkspace) SetCurrentSession(ctx context.Context, sessionID string) error {
w.herdrClient.SetSessionID(sessionID)
w.mu.Lock()
w.lastSession = sessionID
w.mu.Unlock()
return w.client.SetCurrentSession(ctx, w.workspaceID(), sessionID)
}
@@ -254,6 +272,12 @@ func (w *ClientWorkspace) AgentIsReady() bool {
func (w *ClientWorkspace) AgentReadyErr() error {
info, err := w.client.GetAgentInfo(context.Background(), w.workspaceID())
if err != nil {
if errors.Is(err, client.ErrNotFound) {
// The server answered, it just does not know this workspace
// any more. The subscription loop is already re-registering;
// saying "lost connection" here would be plainly wrong.
return ErrWorkspaceGone
}
// The workspace/server could not be reached. This is distinct
// from an initialized-but-not-ready agent: the server may have
// torn the workspace down or restarted underneath us.
@@ -727,13 +751,47 @@ func (w *ClientWorkspace) Subscribe(program *tea.Program) {
w.runSubscription(program.Send)
}
// maxRecoveryEscalate is the number of consecutive failed workspace
// recovery attempts after which the loop tells the UI the connection
// looks unrecoverable. It keeps retrying regardless: a hard stop would
// strand a user whose server comes back a minute later, and Shutdown can
// always cancel it.
const maxRecoveryEscalate = 20
// recoveryCreateTimeout bounds a single re-registration attempt. It is
// generous because workspace startup is slow (config, database, LSP, MCP);
// it exists only so an unresponsive server cannot pin the subscription
// goroutine indefinitely, and the loop simply retries when it trips.
// A var, not a const, so tests can shrink it.
var recoveryCreateTimeout = 30 * time.Second
// runSubscription subscribes to the workspace event stream and forwards
// translated events to send, reconnecting with capped exponential
// backoff whenever the stream drops. It returns only when the
// subscription context is cancelled (via Shutdown). Split out from
// Subscribe so it can be tested without a real *tea.Program.
//
// Two failures need more than a retry. A 404 means the server no longer
// knows this workspace, so resubscribing with the same ID can never
// succeed and the loop re-registers instead. And any stream that closes
// loses whatever was published while the client was away, so every
// re-established stream — even one that reconnects on the first try —
// re-asserts the client's session and asks the UI to resync.
func (w *ClientWorkspace) runSubscription(send func(tea.Msg)) {
w.subStarted.Store(true)
defer close(w.subDone)
backoff := sseReconnectInitialBackoff
degraded := false
recoveryFailures := 0
markDegraded := func(err error, stuck bool) {
if degraded && !stuck {
return
}
degraded = true
send(ConnectionEvent{State: ConnectionDegraded, Err: err, Stuck: stuck})
}
for {
if w.subCtx.Err() != nil {
return
@@ -744,8 +802,21 @@ func (w *ClientWorkspace) runSubscription(send func(tea.Msg)) {
if w.subCtx.Err() != nil {
return
}
slog.Error("Failed to subscribe to workspace events; retrying",
"error", err, "retry_in", backoff)
markDegraded(err, false)
if !errors.Is(err, client.ErrNotFound) {
slog.Error("Failed to subscribe to workspace events; retrying",
"error", err, "retry_in", backoff)
} else if w.recoverWorkspace() == nil {
// Re-registered: resubscribe immediately under the fresh
// workspace ID.
backoff = sseReconnectInitialBackoff
continue
} else if w.subCtx.Err() == nil {
recoveryFailures++
if recoveryFailures == maxRecoveryEscalate {
markDegraded(ErrWorkspaceGone, true)
}
}
if !w.sleepOrDone(backoff) {
return
}
@@ -753,19 +824,23 @@ func (w *ClientWorkspace) runSubscription(send func(tea.Msg)) {
continue
}
// Connected: reset the backoff and pump events until the
// stream drops.
if degraded {
degraded = false
recoveryFailures = 0
w.afterReconnect(send)
}
backoff = sseReconnectInitialBackoff
w.consumeEvents(evc, send)
// The event channel closed: the server restarted, the stream
// was interrupted, or the workspace briefly went away.
// Reconnect after a short delay instead of leaving the TUI
// permanently orphaned, which is what surfaced as a stuck
// "coder agent is offline".
// The event channel closed: the server restarted, the stream was
// interrupted, or the workspace briefly went away. Reconnect
// after a short delay instead of leaving the TUI permanently
// orphaned, which is what surfaced as a stuck "coder agent is
// offline".
if w.subCtx.Err() != nil {
return
}
markDegraded(ErrStreamClosed, false)
slog.Warn("Workspace event stream closed; reconnecting", "retry_in", backoff)
if !w.sleepOrDone(backoff) {
return
@@ -774,6 +849,86 @@ func (w *ClientWorkspace) runSubscription(send func(tea.Msg)) {
}
}
// recoverWorkspace re-registers the workspace after the server reported it
// gone: it re-creates it from the cached snapshot (the server's own view of
// path, data dir, flags and env), adopts the new ID, and re-initializes the
// coder agent when the config is ready, mirroring the startup handshake. The
// server's path dedupe means this either rejoins a live sibling workspace or
// mints a fresh one. It must only be called from the subscription goroutine,
// the only writer of the cached ID.
//
// The create deliberately runs detached from the subscription context: the
// server does not abandon a create when the requesting connection goes away,
// so cancelling would hide the outcome while the workspace got registered
// anyway. Riding it out means the ID is known by the time Shutdown looks,
// and retirement covers a lost response regardless. Its own timeout keeps a
// wedged server from pinning the subscription goroutine forever; the client
// SDK sets no request timeout of its own.
func (w *ClientWorkspace) recoverWorkspace() error {
ctx, cancel := context.WithTimeout(
context.WithoutCancel(w.subCtx), recoveryCreateTimeout,
)
defer cancel()
created, err := w.client.CreateWorkspace(ctx, w.recreateArgs())
if err != nil {
slog.Error("Failed to re-register workspace; retrying", "error", err)
return err
}
if created.Config != nil {
created.Config.SetupAgents()
}
w.mu.Lock()
oldID := w.ws.ID
w.ws = *created
w.mu.Unlock()
slog.Info("Re-registered workspace after server-side loss",
"old_id", oldID, "new_id", created.ID)
if created.Config != nil && created.Config.IsConfigured() {
if err := w.InitCoderAgent(w.subCtx); err != nil {
// Matches the startup handshake: agent init failure is
// logged, not fatal, since the user can still pick a model.
slog.Error("Failed to initialize coder agent after workspace recovery", "error", err)
}
}
return nil
}
// recreateArgs derives the CreateWorkspace request used for recovery from
// the cached snapshot. The ID is dropped so the server can dedupe by
// path or mint a fresh workspace, and Version carries this client's
// version, matching the startup handshake.
func (w *ClientWorkspace) recreateArgs() proto.Workspace {
ws := w.cached()
return proto.Workspace{
Path: ws.Path,
DataDir: ws.DataDir,
Debug: ws.Debug,
YOLO: ws.YOLO,
Channels: ws.Channels,
Env: ws.Env,
Version: version.Version,
}
}
// afterReconnect runs once a degraded subscription is re-established. It
// re-asserts the client's current-session selection, since the server's
// presence entry (or the whole workspace) may have been re-created while we
// were away, and tells the UI to resync state published while detached. The
// SSE handler attaches the client before writing its 200, so the presence
// call cannot be rejected as not-attached here.
func (w *ClientWorkspace) afterReconnect(send func(tea.Msg)) {
w.mu.RLock()
sid := w.lastSession
w.mu.RUnlock()
if sid != "" {
if err := w.SetCurrentSession(w.subCtx, sid); err != nil {
slog.Warn("Failed to re-assert current session after reconnect", "error", err)
}
}
send(ConnectionEvent{State: ConnectionRecovered})
}
// sleepOrDone waits for d or until the subscription context is
// cancelled. It reports false when the context was cancelled, signalling
// the caller to stop reconnecting.
@@ -810,16 +965,58 @@ func (w *ClientWorkspace) consumeEvents(evc <-chan any, send func(tea.Msg)) {
}
}
// shutdownDrainTimeout bounds how long Shutdown waits for the subscription
// loop to stop. Exceeding it is not a correctness problem — retiring the
// client releases whatever a late recovery registers — it only makes the
// goodbye less tidy.
const shutdownDrainTimeout = 5 * time.Second
func (w *ClientWorkspace) Shutdown() {
// Stop the event subscription's reconnect loop before releasing the
// workspace so it doesn't race to re-subscribe during teardown.
// Stop the reconnect/recovery loop first, then wait for it: cancelling
// alone does not unwind a workspace recovery that is already in
// flight, and we want to release the workspace that recovery ended up
// with rather than one it is about to replace.
if w.subCancel != nil {
w.subCancel()
}
w.awaitSubscription()
w.herdrClient.Close()
// Retiring the client releases every claim it holds, on every workspace,
// and blocks any further create from this client ID. That is what makes
// teardown exact even when a recovery create's response was lost: the
// create either landed before this call, and its claim is released here,
// or it arrives afterwards and registers nothing.
err := w.client.RetireClient(context.Background())
if err == nil {
return
}
if !errors.Is(err, client.ErrUnsupported) {
slog.Warn("Failed to retire client on the server", "error", err)
return
}
// The server predates client retirement, so fall back to releasing
// the workspace we know about. Nothing better is possible against an
// older server.
_ = w.client.DeleteWorkspace(context.Background(), w.workspaceID())
}
// awaitSubscription waits for the subscription loop to return. It returns
// immediately when the loop never started, which is the case for
// workspaces shut down before Subscribe runs.
func (w *ClientWorkspace) awaitSubscription() {
if !w.subStarted.Load() || w.subDone == nil {
return
}
t := time.NewTimer(shutdownDrainTimeout)
defer t.Stop()
select {
case <-w.subDone:
case <-t.C:
slog.Warn("Timed out waiting for the workspace subscription to stop")
}
}
// translateEvent converts proto-typed SSE events into the domain types
// that the TUI's Update() method expects. Skills events also update the
// process-local skills.Manager so callers reading
+502
View File
@@ -7,6 +7,7 @@ import (
"net/http/httptest"
"net/url"
"strings"
"sync"
"sync/atomic"
"testing"
"time"
@@ -443,3 +444,504 @@ func agentInfoWorkspace(t *testing.T, info proto.AgentInfo) *ClientWorkspace {
require.NoError(t, err)
return NewClientWorkspace(c, proto.Workspace{ID: "ws-1"})
}
// -- Workspace recovery --
//
// The bug these cover: the reconnect loop retried SubscribeEvents with
// the workspace ID captured at startup, forever. Once the server no
// longer knew that ID — because the workspace was torn down under the
// client, or the server was replaced — every retry and every other
// request answered 404 permanently, and a sibling session kept the
// server (and its 404s) alive.
// recoveryServer is a scripted server for the recovery tests. It 404s
// event subscriptions for stale workspace IDs, mints a new ID on create,
// and records what the client did.
type recoveryServer struct {
mu sync.Mutex
// liveID is the only workspace ID whose event stream is served.
liveID string
// nextID names the workspace the next create hands back.
nextID string
// createErr, when set, fails creates instead of serving them.
createErr func() int
// hangUpOnCreate drops the connection mid-response so the client
// never learns the ID of a workspace the server did register.
hangUpOnCreate bool
creates int
streams int
sessionPosts []string
retired []string
deleted []string
}
func (s *recoveryServer) start(t *testing.T) *client.Client {
t.Helper()
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
s.mu.Lock()
defer s.mu.Unlock()
switch {
case r.Method == http.MethodPost && r.URL.Path == "/v1/workspaces":
s.creates++
if s.createErr != nil {
http.Error(w, "no", s.createErr())
return
}
if s.hangUpOnCreate {
// Register the workspace, then hang up before the client
// can read the ID. This is the case no amount of client
// bookkeeping can name.
s.liveID = s.nextID
if hj, ok := w.(http.Hijacker); ok {
conn, _, err := hj.Hijack()
require.NoError(t, err)
_ = conn.Close()
}
return
}
s.liveID = s.nextID
require.NoError(t, json.NewEncoder(w).Encode(proto.Workspace{
ID: s.liveID, Path: "/tmp/recover",
}))
case r.Method == http.MethodDelete && strings.HasPrefix(r.URL.Path, "/v1/clients/"):
s.retired = append(s.retired, strings.TrimPrefix(r.URL.Path, "/v1/clients/"))
s.liveID = ""
case r.Method == http.MethodDelete && strings.HasPrefix(r.URL.Path, "/v1/workspaces/"):
s.deleted = append(s.deleted, strings.TrimPrefix(r.URL.Path, "/v1/workspaces/"))
case strings.HasSuffix(r.URL.Path, "/current-session"):
id := strings.TrimSuffix(strings.TrimPrefix(r.URL.Path, "/v1/workspaces/"), "/current-session")
if id != s.liveID {
http.Error(w, "workspace not found", http.StatusNotFound)
return
}
var req proto.CurrentSession
require.NoError(t, json.NewDecoder(r.Body).Decode(&req))
s.sessionPosts = append(s.sessionPosts, req.SessionID)
case strings.HasSuffix(r.URL.Path, "/events"):
id := strings.TrimSuffix(strings.TrimPrefix(r.URL.Path, "/v1/workspaces/"), "/events")
if id != s.liveID {
http.Error(w, "workspace not found", http.StatusNotFound)
return
}
s.streams++
w.Header().Set("Content-Type", "text/event-stream")
w.WriteHeader(http.StatusOK)
if f, ok := w.(http.Flusher); ok {
f.Flush()
}
// Drop the stream so the loop keeps cycling.
default:
w.WriteHeader(http.StatusOK)
}
}))
t.Cleanup(srv.Close)
u, err := url.Parse(srv.URL)
require.NoError(t, err)
c, err := client.NewClient(t.TempDir(), "tcp", u.Host)
require.NoError(t, err)
return c
}
func (s *recoveryServer) snapshot(f func(*recoveryServer)) {
s.mu.Lock()
defer s.mu.Unlock()
f(s)
}
// connectionRecorder collects the ConnectionEvents the subscription loop
// reports to the UI.
type connectionRecorder struct {
mu sync.Mutex
events []ConnectionEvent
}
func (r *connectionRecorder) send(msg tea.Msg) {
ev, ok := msg.(ConnectionEvent)
if !ok {
return
}
r.mu.Lock()
defer r.mu.Unlock()
r.events = append(r.events, ev)
}
func (r *connectionRecorder) states() []ConnectionState {
r.mu.Lock()
defer r.mu.Unlock()
out := make([]ConnectionState, len(r.events))
for i, ev := range r.events {
out[i] = ev.State
}
return out
}
func (r *connectionRecorder) sawStuck() bool {
r.mu.Lock()
defer r.mu.Unlock()
for _, ev := range r.events {
if ev.Stuck {
return true
}
}
return false
}
// TestClientWorkspace_RecoversFromWorkspaceGone is the headline client
// regression: a 404 for the cached workspace ID must make the client
// re-register, adopt the new ID, re-assert the session it was viewing,
// and tell the UI to resync — not retry a dead ID forever.
func TestClientWorkspace_RecoversFromWorkspaceGone(t *testing.T) {
t.Cleanup(SetSSEBackoffForTest(time.Millisecond, 5*time.Millisecond))
srv := &recoveryServer{liveID: "", nextID: "ws-2"}
c := srv.start(t)
ws := NewClientWorkspace(c, proto.Workspace{ID: "ws-1", Path: "/tmp/recover"})
// The client was viewing a session, so recovery has to restore that
// selection: the server's presence entry died with the workspace.
ws.mu.Lock()
ws.lastSession = "sess-1"
ws.mu.Unlock()
rec := &connectionRecorder{}
done := make(chan struct{})
go func() {
ws.runSubscription(rec.send)
close(done)
}()
require.Eventually(t, func() bool { return ws.workspaceID() == "ws-2" },
3*time.Second, 5*time.Millisecond,
"a 404 must make the client re-register instead of retrying a dead ID")
require.Eventually(t, func() bool {
var posts []string
srv.snapshot(func(s *recoveryServer) { posts = append(posts, s.sessionPosts...) })
return len(posts) > 0 && posts[0] == "sess-1"
}, 3*time.Second, 5*time.Millisecond,
"the recovered workspace must be told which session the client is on")
require.Eventually(t, func() bool {
states := rec.states()
var degraded, recovered bool
for _, s := range states {
degraded = degraded || s == ConnectionDegraded
recovered = recovered || s == ConnectionRecovered
}
return degraded && recovered
}, 3*time.Second, 5*time.Millisecond,
"the UI must be told to resync after the workspace was re-created")
ws.Shutdown()
select {
case <-done:
case <-time.After(3 * time.Second):
t.Fatal("runSubscription did not return after Shutdown")
}
}
// TestClientWorkspace_ResyncsAfterPlainStreamDrop covers the quieter
// failure: the stream closes and the very next subscribe succeeds. That
// used to be treated as a non-event, so the TUI silently showed state
// frozen at the moment of the drop — everything published while the
// client was away is gone, and the server had dropped its presence entry.
func TestClientWorkspace_ResyncsAfterPlainStreamDrop(t *testing.T) {
t.Cleanup(SetSSEBackoffForTest(time.Millisecond, 5*time.Millisecond))
srv := &recoveryServer{liveID: "ws-1"}
c := srv.start(t)
ws := NewClientWorkspace(c, proto.Workspace{ID: "ws-1", Path: "/tmp/blip"})
ws.mu.Lock()
ws.lastSession = "sess-9"
ws.mu.Unlock()
rec := &connectionRecorder{}
done := make(chan struct{})
go func() {
ws.runSubscription(rec.send)
close(done)
}()
require.Eventually(t, func() bool {
states := rec.states()
var degraded, recovered bool
for _, s := range states {
degraded = degraded || s == ConnectionDegraded
recovered = recovered || s == ConnectionRecovered
}
return degraded && recovered
}, 3*time.Second, 5*time.Millisecond,
"a reconnect that succeeds first try must still resync")
var posts int
var creates int
srv.snapshot(func(s *recoveryServer) { posts, creates = len(s.sessionPosts), s.creates })
require.Positive(t, posts, "the session selection must be re-asserted after the drop")
require.Zero(t, creates, "a live workspace must not be re-created")
require.Equal(t, "ws-1", ws.workspaceID())
ws.Shutdown()
select {
case <-done:
case <-time.After(3 * time.Second):
t.Fatal("runSubscription did not return after Shutdown")
}
}
// TestClientWorkspace_EscalatesUnrecoverableConnection checks the loop
// keeps trying when re-registration itself keeps failing, and escalates
// the UI notice rather than stopping: a hard stop would strand a user
// whose server comes back a minute later.
func TestClientWorkspace_EscalatesUnrecoverableConnection(t *testing.T) {
t.Cleanup(SetSSEBackoffForTest(time.Millisecond, 2*time.Millisecond))
srv := &recoveryServer{createErr: func() int { return http.StatusInternalServerError }}
c := srv.start(t)
ws := NewClientWorkspace(c, proto.Workspace{ID: "ws-1", Path: "/tmp/hopeless"})
rec := &connectionRecorder{}
done := make(chan struct{})
go func() {
ws.runSubscription(rec.send)
close(done)
}()
require.Eventually(t, rec.sawStuck, 5*time.Second, 5*time.Millisecond,
"repeated recovery failures must escalate to a persistent notice")
var creates int
srv.snapshot(func(s *recoveryServer) { creates = s.creates })
require.GreaterOrEqual(t, creates, maxRecoveryEscalate,
"the loop must keep retrying rather than giving up")
ws.Shutdown()
select {
case <-done:
case <-time.After(3 * time.Second):
t.Fatal("runSubscription did not return after Shutdown")
}
}
// TestClientWorkspace_ShutdownRetiresAfterLostCreateResponse is the
// quit-during-recovery case, in its nastiest form: the client asks for a
// replacement workspace and the connection dies before it can read the
// ID. The server registered the workspace anyway, so the client is
// holding a claim it cannot name. Retiring the client is what makes
// teardown exact here — there is nothing to guess and nothing to scan.
func TestClientWorkspace_ShutdownRetiresAfterLostCreateResponse(t *testing.T) {
t.Cleanup(SetSSEBackoffForTest(time.Millisecond, 5*time.Millisecond))
srv := &recoveryServer{nextID: "ws-orphan", hangUpOnCreate: true}
c := srv.start(t)
ws := NewClientWorkspace(c, proto.Workspace{ID: "ws-1", Path: "/tmp/recover"})
done := make(chan struct{})
go func() {
ws.runSubscription(func(tea.Msg) {})
close(done)
}()
// Wait until the server has registered a workspace the client never
// learned about: that is the state Shutdown has to clean up.
require.Eventually(t, func() bool {
var live string
srv.snapshot(func(s *recoveryServer) { live = s.liveID })
return live == "ws-orphan"
}, 3*time.Second, 5*time.Millisecond)
require.NotEqual(t, "ws-orphan", ws.workspaceID(),
"the client must genuinely not know the workspace's ID")
ws.Shutdown()
select {
case <-done:
case <-time.After(3 * time.Second):
t.Fatal("runSubscription did not return after Shutdown")
}
srv.snapshot(func(s *recoveryServer) {
require.Equal(t, []string{c.ClientID()}, s.retired,
"Shutdown must retire the client, which releases claims it cannot name")
require.Empty(t, s.liveID, "no workspace may be left holding the server open")
})
}
// TestClientWorkspace_ShutdownFallsBackForLegacyServer keeps quits working
// against a server that predates client retirement: a 404 for the retire
// endpoint must fall back to releasing the workspace by ID, not be
// reported as a failed teardown.
func TestClientWorkspace_ShutdownFallsBackForLegacyServer(t *testing.T) {
t.Parallel()
var mu sync.Mutex
var deleted []string
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
mu.Lock()
defer mu.Unlock()
switch {
case strings.HasPrefix(r.URL.Path, "/v1/clients/"):
http.NotFound(w, r)
case r.Method == http.MethodDelete:
deleted = append(deleted, strings.TrimPrefix(r.URL.Path, "/v1/workspaces/"))
default:
w.WriteHeader(http.StatusOK)
}
}))
t.Cleanup(srv.Close)
u, err := url.Parse(srv.URL)
require.NoError(t, err)
c, err := client.NewClient(t.TempDir(), "tcp", u.Host)
require.NoError(t, err)
NewClientWorkspace(c, proto.Workspace{ID: "ws-1"}).Shutdown()
mu.Lock()
defer mu.Unlock()
require.Equal(t, []string{"ws-1"}, deleted)
}
// TestClientWorkspace_AgentReadyErr_WorkspaceGone checks the status the
// UI is given while recovery runs. A 404 from a live server used to print
// "lost connection to the crush server: ... status code 404", which is
// both wrong and unactionable.
func TestClientWorkspace_AgentReadyErr_WorkspaceGone(t *testing.T) {
t.Parallel()
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
http.NotFound(w, r)
}))
t.Cleanup(srv.Close)
u, err := url.Parse(srv.URL)
require.NoError(t, err)
c, err := client.NewClient(t.TempDir(), "tcp", u.Host)
require.NoError(t, err)
ws := NewClientWorkspace(c, proto.Workspace{ID: "ws-1"})
readyErr := ws.AgentReadyErr()
require.ErrorIs(t, readyErr, ErrWorkspaceGone)
require.NotErrorIs(t, readyErr, ErrServerUnreachable)
require.NotErrorIs(t, readyErr, ErrAgentNotInitialized)
}
// TestClientWorkspace_ShutdownWaitsForInFlightRecovery pins the ordering
// requirement: quitting must stop recovery before saying goodbye to the
// server. Cancelling alone does not unwind a create that is already in
// flight, so a goodbye sent first would be followed by the workspace the
// create went on to register — an orphan keeping the server alive and
// blocking the next upgrade.
func TestClientWorkspace_ShutdownWaitsForInFlightRecovery(t *testing.T) {
t.Cleanup(SetSSEBackoffForTest(time.Millisecond, 5*time.Millisecond))
var mu sync.Mutex
var ops []string
creating := make(chan struct{})
var once sync.Once
live := ""
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch {
case r.Method == http.MethodPost && r.URL.Path == "/v1/workspaces":
once.Do(func() { close(creating) })
// Slow enough that Shutdown lands mid-create.
//nolint:forbidigo // The overlap is the point of the test.
time.Sleep(300 * time.Millisecond)
mu.Lock()
ops = append(ops, "create")
live = "ws-2"
mu.Unlock()
require.NoError(t, json.NewEncoder(w).Encode(proto.Workspace{ID: "ws-2"}))
case strings.HasPrefix(r.URL.Path, "/v1/clients/"):
mu.Lock()
ops = append(ops, "retire")
live = ""
mu.Unlock()
case strings.HasSuffix(r.URL.Path, "/events"):
mu.Lock()
known := live == strings.TrimSuffix(strings.TrimPrefix(r.URL.Path, "/v1/workspaces/"), "/events")
mu.Unlock()
if !known {
http.Error(w, "workspace not found", http.StatusNotFound)
return
}
w.Header().Set("Content-Type", "text/event-stream")
w.WriteHeader(http.StatusOK)
default:
w.WriteHeader(http.StatusOK)
}
}))
t.Cleanup(srv.Close)
u, err := url.Parse(srv.URL)
require.NoError(t, err)
c, err := client.NewClient(t.TempDir(), "tcp", u.Host)
require.NoError(t, err)
ws := NewClientWorkspace(c, proto.Workspace{ID: "ws-1", Path: "/tmp/quit-mid-recovery"})
done := make(chan struct{})
go func() {
ws.runSubscription(func(tea.Msg) {})
close(done)
}()
<-creating
ws.Shutdown()
select {
case <-done:
case <-time.After(3 * time.Second):
t.Fatal("runSubscription did not return after Shutdown")
}
mu.Lock()
defer mu.Unlock()
require.Equal(t, []string{"create", "retire"}, ops,
"the goodbye must follow the create it was racing, not precede it")
require.Empty(t, live, "the recovered workspace must not outlive the client")
require.Equal(t, "ws-2", ws.workspaceID(),
"the client must have adopted the workspace it created before releasing it")
}
// TestClientWorkspace_RecoveryCreateIsBounded checks a wedged server cannot
// pin the recovery attempt forever. The client SDK sets no request timeout,
// so without a bound here the subscription goroutine would block for good
// on a server that accepts the create and never answers.
func TestClientWorkspace_RecoveryCreateIsBounded(t *testing.T) {
t.Parallel()
release := make(chan struct{})
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.Method == http.MethodPost && r.URL.Path == "/v1/workspaces" {
<-release // Accept the create and never answer it.
return
}
http.NotFound(w, r)
}))
// Unwedge the handler before closing the server, which waits on it.
t.Cleanup(func() {
close(release)
srv.Close()
})
u, err := url.Parse(srv.URL)
require.NoError(t, err)
c, err := client.NewClient(t.TempDir(), "tcp", u.Host)
require.NoError(t, err)
ws := NewClientWorkspace(c, proto.Workspace{ID: "ws-1", Path: "/tmp/wedged"})
// Shrink the bound so the test does not wait out the production window.
// The recovery must give up on its own, without anything cancelling it:
// the create is deliberately detached from the subscription context.
orig := recoveryCreateTimeoutForTest(50 * time.Millisecond)
t.Cleanup(orig)
done := make(chan error, 1)
go func() { done <- ws.recoverWorkspace() }()
select {
case err := <-done:
require.Error(t, err, "a create that never answers must not report success")
case <-time.After(5 * time.Second):
t.Fatal("recoverWorkspace blocked on an unresponsive server")
}
}
+33
View File
@@ -1,6 +1,8 @@
package workspace
import (
"time"
tea "charm.land/bubbletea/v2"
)
@@ -12,3 +14,34 @@ import (
func (w *ClientWorkspace) ConsumeEventsForTest(evc <-chan any, send func(tea.Msg)) {
w.consumeEvents(evc, send)
}
// RunSubscriptionForTest runs the full subscribe/reconnect/recovery loop.
// Exposed for cross-package integration tests. It returns once the
// workspace is shut down.
func (w *ClientWorkspace) RunSubscriptionForTest(send func(tea.Msg)) {
w.runSubscription(send)
}
// WorkspaceIDForTest returns the currently cached workspace ID, which
// recovery may have re-minted.
func (w *ClientWorkspace) WorkspaceIDForTest() string {
return w.workspaceID()
}
// SetSSEBackoffForTest shrinks the subscription reconnect backoff and
// returns a restore function for t.Cleanup.
func SetSSEBackoffForTest(initial, maxBackoff time.Duration) (restore func()) {
origInitial, origMax := sseReconnectInitialBackoff, sseReconnectMaxBackoff
sseReconnectInitialBackoff, sseReconnectMaxBackoff = initial, maxBackoff
return func() {
sseReconnectInitialBackoff, sseReconnectMaxBackoff = origInitial, origMax
}
}
// recoveryCreateTimeoutForTest shrinks the bound on a single workspace
// re-registration attempt and returns a restore function for t.Cleanup.
func recoveryCreateTimeoutForTest(d time.Duration) (restore func()) {
orig := recoveryCreateTimeout
recoveryCreateTimeout = d
return func() { recoveryCreateTimeout = orig }
}
@@ -30,6 +30,7 @@ func xdgIsolate(t *testing.T) {
// runtimeServer wires the production server handler around an
// httptest.NewServer for integration testing.
type runtimeServer struct {
srv *server.Server
httpSrv *httptest.Server
host string
}
@@ -42,13 +43,20 @@ func newRuntimeServer(t *testing.T) *runtimeServer {
u, err := url.Parse(hs.URL)
require.NoError(t, err)
return &runtimeServer{httpSrv: hs, host: u.Host}
return &runtimeServer{srv: s, httpSrv: hs, host: u.Host}
}
func (r *runtimeServer) newClient(t *testing.T, path string) *client.Client {
t.Helper()
c, err := client.NewClient(path, "tcp", r.host)
require.NoError(t, err)
// Retire the client during cleanup so the server releases every
// claim it holds and tears the workspace down at once, closing the
// pooled DB connection. Without this a test that leaves clients
// attached (the SSE cache tests never shut theirs down) keeps the
// workspace, and its open crush.db, alive past t.TempDir cleanup,
// which Windows cannot remove while the file is locked.
t.Cleanup(func() { _ = c.RetireClient(context.Background()) })
return c
}
@@ -174,3 +182,181 @@ loop:
}
require.True(t, gotConfigChanged, "expected ConfigChanged event over SSE")
}
// -- Concurrent session lifecycle --
//
// These drive the real HTTP surface, because the bug they cover only
// appears when two sessions share a server: the first client's workspace
// died, and the sibling kept the server (and its 404s) alive so the first
// client never got a fresh start.
// TestServer_ConcurrentSessionsSameDirShareOneWorkspace checks the
// baseline for two sessions in one directory: they share a workspace, and
// one of them leaving must not disturb the other.
func TestServer_ConcurrentSessionsSameDirShareOneWorkspace(t *testing.T) {
xdgIsolate(t)
rt := newRuntimeServer(t)
rt.srv.Backend().SetDetachGrace(0)
cwd, dataDir := t.TempDir(), t.TempDir()
cA, cB := rt.newClient(t, cwd), rt.newClient(t, cwd)
ctx := t.Context()
wsA, err := cA.CreateWorkspace(ctx, proto.Workspace{Path: cwd, DataDir: dataDir})
require.NoError(t, err)
wsB, err := cB.CreateWorkspace(ctx, proto.Workspace{Path: cwd, DataDir: dataDir})
require.NoError(t, err)
require.Equal(t, wsA.ID, wsB.ID, "same directory must share one workspace")
// A leaves. B's claim must keep the workspace addressable.
require.NoError(t, cA.RetireClient(ctx))
got, err := cB.GetWorkspace(ctx, wsB.ID)
require.NoError(t, err, "one client leaving must not destroy the shared workspace")
require.Equal(t, wsB.ID, got.ID)
}
// TestServer_ConcurrentSessionsDifferentDirsAreIndependent is the other
// half: two directories get two workspaces on one server, and retiring
// one client must leave the other's workspace completely alone.
func TestServer_ConcurrentSessionsDifferentDirsAreIndependent(t *testing.T) {
xdgIsolate(t)
rt := newRuntimeServer(t)
rt.srv.Backend().SetDetachGrace(0)
cwdA, cwdB := t.TempDir(), t.TempDir()
cA, cB := rt.newClient(t, cwdA), rt.newClient(t, cwdB)
ctx := t.Context()
wsA, err := cA.CreateWorkspace(ctx, proto.Workspace{Path: cwdA, DataDir: t.TempDir()})
require.NoError(t, err)
wsB, err := cB.CreateWorkspace(ctx, proto.Workspace{Path: cwdB, DataDir: t.TempDir()})
require.NoError(t, err)
require.NotEqual(t, wsA.ID, wsB.ID)
require.NoError(t, cA.RetireClient(ctx))
_, err = cB.GetWorkspace(ctx, wsB.ID)
require.NoError(t, err, "a sibling session's workspace must survive")
_, err = cB.GetWorkspace(ctx, wsA.ID)
require.ErrorIs(t, err, client.ErrNotFound, "the retired client's workspace must be gone")
}
// TestServer_RefusesShutdownWhileWorkspaceLive is the invariant that keeps
// an upgrading client from killing a running session. The refusal has to
// come from the server: a client checking idleness itself needs a second
// round trip that a new session can slip into.
func TestServer_RefusesShutdownWhileWorkspaceLive(t *testing.T) {
xdgIsolate(t)
rt := newRuntimeServer(t)
cwd := t.TempDir()
cLive := rt.newClient(t, cwd)
ctx := t.Context()
ws, err := cLive.CreateWorkspace(ctx, proto.Workspace{Path: cwd, DataDir: t.TempDir()})
require.NoError(t, err)
// A second client, standing in for one that found a version mismatch.
err = rt.newClient(t, t.TempDir()).ShutdownServerIfIdle(ctx)
require.ErrorIs(t, err, client.ErrServerBusy,
"a server hosting a workspace must refuse to stand down")
_, err = cLive.GetWorkspace(ctx, ws.ID)
require.NoError(t, err, "the live session's workspace must be untouched")
}
// TestServer_DetachGraceSurvivesStreamBlip is the server-side half of the
// SSE regression. Cutting the stream used to destroy the workspace
// instantly, so the client's reconnect — 250ms later — came back to an ID
// the server no longer knew, and 404'd from then on.
func TestServer_DetachGraceSurvivesStreamBlip(t *testing.T) {
xdgIsolate(t)
rt := newRuntimeServer(t)
rt.srv.Backend().SetDetachGrace(30 * time.Second)
cwd := t.TempDir()
c := rt.newClient(t, cwd)
ws, err := c.CreateWorkspace(t.Context(), proto.Workspace{Path: cwd, DataDir: t.TempDir()})
require.NoError(t, err)
streamCtx, killStream := context.WithCancel(t.Context())
evc, err := c.SubscribeEvents(streamCtx, ws.ID)
require.NoError(t, err)
// Cut the stream the way a network blip does: no release first.
killStream()
for range evc { //nolint:revive // Drain until the server closes it.
}
require.Eventually(t, func() bool {
_, err := c.GetWorkspace(t.Context(), ws.ID)
return err == nil
}, 3*time.Second, 25*time.Millisecond,
"the workspace must survive the drop so the reconnect can re-attach")
// The reconnect lands and the workspace is still the same one.
evc2, err := c.SubscribeEvents(t.Context(), ws.ID)
require.NoError(t, err, "the client must be able to re-attach to its workspace")
require.NotNil(t, evc2)
}
// TestClientWorkspace_RecoversAfterServerSideTeardown is the whole bug,
// end to end: the server drops the client's workspace while the server
// itself stays up (a sibling session keeps it alive). The client must
// notice the 404, re-register, and end up on a workspace it can use again
// rather than 404ing forever.
func TestClientWorkspace_RecoversAfterServerSideTeardown(t *testing.T) {
xdgIsolate(t)
t.Cleanup(workspace.SetSSEBackoffForTest(5*time.Millisecond, 25*time.Millisecond))
rt := newRuntimeServer(t)
rt.srv.Backend().SetDetachGrace(0)
cwd, dataDir := t.TempDir(), t.TempDir()
// A sibling session in another directory keeps the server alive, which
// is what made the 404s permanent instead of self-healing.
sibling := rt.newClient(t, t.TempDir())
siblingCwd := t.TempDir()
_, err := sibling.CreateWorkspace(t.Context(), proto.Workspace{
Path: siblingCwd, DataDir: t.TempDir(),
})
require.NoError(t, err)
c := rt.newClient(t, cwd)
wsProto, err := c.CreateWorkspace(t.Context(), proto.Workspace{Path: cwd, DataDir: dataDir})
require.NoError(t, err)
originalID := wsProto.ID
// The server drops the workspace while the client still holds the
// snapshot naming it. That is what an upgrade, or a teardown racing a
// stream drop, leaves behind: a live server that answers 404 for the
// only workspace ID this client knows.
require.NoError(t, c.DeleteWorkspace(t.Context(), originalID))
_, err = c.GetWorkspace(t.Context(), originalID)
require.ErrorIs(t, err, client.ErrNotFound, "the workspace must really be gone")
ws := workspace.NewClientWorkspace(c, *wsProto)
done := make(chan struct{})
go func() {
ws.RunSubscriptionForTest(func(tea.Msg) {})
close(done)
}()
require.Eventually(t, func() bool {
id := ws.WorkspaceIDForTest()
return id != "" && id != originalID
}, 10*time.Second, 25*time.Millisecond,
"the client must re-register instead of retrying a workspace the server forgot")
// The recovered workspace is usable, which is what 404s prevented.
_, err = ws.ListSessions(t.Context())
require.NoError(t, err, "the recovered workspace must serve requests again")
ws.Shutdown()
select {
case <-done:
case <-time.After(5 * time.Second):
t.Fatal("the subscription loop did not stop after Shutdown")
}
}
+38
View File
@@ -36,8 +36,46 @@ var (
// to determine the agent's status (server down, or the workspace was
// torn down out from under the client).
ErrServerUnreachable = errors.New("lost connection to the crush server")
// ErrWorkspaceGone means the server is reachable but no longer knows
// this client's workspace: it was torn down, or the server was
// replaced underneath the client. The subscription loop re-registers
// the workspace in the background when it sees this.
ErrWorkspaceGone = errors.New("the server reset this workspace; reconnecting")
// ErrStreamClosed means an established event stream ended.
// Resubscribing usually succeeds immediately, but events published in
// the meantime are lost for good, so the client treats it as a
// degraded link that requires a resync.
ErrStreamClosed = errors.New("the event stream closed; reconnecting")
)
// ConnectionState describes the health of the client-server link as
// reported by the [ClientWorkspace] subscription loop.
type ConnectionState int
const (
// ConnectionDegraded means the event stream is down (or the workspace
// was lost server-side) and the client is retrying or re-registering
// in the background.
ConnectionDegraded ConnectionState = iota
// ConnectionRecovered means the event stream was re-established,
// possibly against a re-created workspace.
ConnectionRecovered
)
// ConnectionEvent is delivered to the TUI as a tea.Msg on degraded and
// recovered transitions of the client-server link. Local (in-process)
// workspaces never emit it.
type ConnectionEvent struct {
State ConnectionState
// Err is the most recent failure, set when State is
// ConnectionDegraded.
Err error
// Stuck marks a degraded connection that has resisted repeated
// recovery attempts. The loop keeps retrying regardless; the UI
// should escalate from a transient notice to a persistent error.
Stuck bool
}
// LSPClientInfo holds information about an LSP client's state. This is
// the frontend-facing type; implementations translate from the
// underlying app or proto representation.