diff --git a/cmd/shellwatch/main.go b/cmd/shellwatch/main.go index 92dbea8..f1aa363 100644 --- a/cmd/shellwatch/main.go +++ b/cmd/shellwatch/main.go @@ -102,11 +102,14 @@ func run() error { flusher := store.NewLastUsedFlusher(db, clk) go flusher.Run(ctx, time.Minute) - // First-run seeding (admin account + passkeys + endpoints) + inactive-account - // cleanup. - if res, err := seed.FromConfig(ctx, db, cfg, newUUID, clk.Now()); err != nil { - slog.Warn("first-run seeding failed", "err", err) - } else if res.SeededAdminAccount || res.SeededAdminPasskey { + // First-run seeding (admin account + passkeys + endpoints). Fatal on + // failure like the Node boot (index.ts:43) — continuing without an admin + // account leaves a fresh deployment silently un-loginable. + res, err := seed.FromConfig(ctx, db, cfg, newUUID, clk.Now()) + if err != nil { + return fmt.Errorf("first-run seeding failed: %w", err) + } + if res.SeededAdminAccount || res.SeededAdminPasskey { slog.Info("seeded from config", "adminAccount", res.SeededAdminAccount, "adminPasskey", res.SeededAdminPasskey) } webauthnDeps := &webauthn.Deps{ @@ -143,6 +146,16 @@ func run() error { RpID: cfg.Security.RpID, Origin: firstOrigin(cfg.Security.TrustedWebauthnOrigins), NewConnectionID: newUUID, + // Dead terminal connection -> cancel its stranded sign prompts and + // clear the toasts (#91). signBroker resolves lazily like BrokerFunc. + OnConnectionEnded: func(connID, reason string) { + if signBroker == nil { + return + } + if n := signBroker.CancelForConnection(connID, reason); n > 0 { + slog.Info("cancelled pending sign prompts for dead connection", "connection", connID, "count", n) + } + }, }) manager := terminal.NewManager(factory, clk, 0) // Idle janitor: auto-close sessions idle >30 min (Node parity, H6). @@ -175,11 +188,14 @@ func run() error { auditWriter.AttachManager(manager, manager.GetSession) auditWriter.AttachStore(actionStore) + buildInfo := buildinfo.Load(mustGetwd()) mcpDeps := &mcp.Deps{ AgentDeps: agent.Deps{Manager: manager, Endpoints: endpointStore, Demo: demoSvc}, Keys: store.NewSSHKeys(db), NewID: newUUID, + Version: buildInfo.Display, SessionTimeout: time.Duration(*cfg.Mcp.SessionTimeoutMinutes) * time.Minute, + MaxOwned: store.NewAccounts(db).MaxSessions, } // Account-deleted teardown (app.ts accountLifecycle "deleted", #217): close @@ -190,6 +206,11 @@ func run() error { if n := manager.CloseAllForAccount(accountID, terminal.CloseAccountDeleted); n > 0 { slog.Info("closed sessions for deleted account", "account", accountID, "count", n) } + // Purge retained post-mortem sessions too — a deleted account's + // terminal output must not stay readable in memory. + if n := manager.RemoveForAccount(accountID); n > 0 { + slog.Info("purged retained sessions for deleted account", "account", accountID, "count", n) + } if n := mcpDeps.DropAccount(accountID); n > 0 { slog.Info("tore down MCP transports for deleted account", "account", accountID, "count", n) } @@ -216,7 +237,7 @@ func run() error { Resolve: resolve, TouchLastUsed: flusher.Touch, StaticFS: staticFS, - BuildInfo: buildinfo.Load(mustGetwd()), + BuildInfo: buildInfo, WebAuthn: webauthnDeps, HydraAdmin: admin, HasPasskeys: func() bool { diff --git a/internal/agent/session.go b/internal/agent/session.go index f9fa4ec..89a7927 100644 --- a/internal/agent/session.go +++ b/internal/agent/session.go @@ -39,9 +39,11 @@ type Session struct { } // New builds an AgentSession for an account. maxOwned caps concurrent owned -// sessions (default 5). +// sessions: negative means "unresolved, use the default 5"; an explicit 0 +// blocks all creation (Node honors a stored max_sessions of 0, and so does +// the REST cap check — MCP must not silently upgrade 0 to 5). func New(deps Deps, accountID, sourceIP string, maxOwned int) *Session { - if maxOwned <= 0 { + if maxOwned < 0 { maxOwned = 5 } return &Session{deps: deps, accountID: accountID, sourceIP: sourceIP, maxOwned: maxOwned, owned: map[string]bool{}} @@ -156,16 +158,24 @@ func (s *Session) CreateSession(ctx context.Context, endpointID, reason string) return nil, fmt.Errorf("unknown endpoint: %s", endpointID) } s.mu.Lock() - // Prune ids the manager no longer knows (idle-timeout janitor, server - // hangup, account cleanup) before enforcing the cap. Deliberate divergence - // from Node, which counts the raw set: there, N externally-closed sessions - // permanently starve the cap until the agent reconnects. + // The cap counts only LIVE sessions — deliberate divergence from Node, + // which counts its raw owned set and so lets N externally-closed sessions + // (idle janitor, server hangup) starve the cap until the agent reconnects. + // Dead-but-retained ids (post-mortem closed/error sessions, M6) stay in + // owned so read_output/close_session keep working on them; only ids the + // manager has forgotten entirely are dropped. + live := 0 for id := range s.owned { - if s.deps.Manager.GetSession(id) == nil { + sess := s.deps.Manager.GetSession(id) + if sess == nil { delete(s.owned, id) + continue + } + if sess.Status != terminal.StatusClosed && sess.Status != terminal.StatusError { + live++ } } - if len(s.owned) >= s.maxOwned { + if live >= s.maxOwned { s.mu.Unlock() return nil, fmt.Errorf("maximum concurrent sessions (%d) reached", s.maxOwned) } diff --git a/internal/agent/session_cap_test.go b/internal/agent/session_cap_test.go index 2ecf177..bcd964d 100644 --- a/internal/agent/session_cap_test.go +++ b/internal/agent/session_cap_test.go @@ -1,11 +1,14 @@ // SPDX-License-Identifier: LicenseRef-FSL-1.1-Apache-2.0 -// The maxOwned cap must not count sessions the manager already closed (idle +// The maxOwned cap must not count sessions that died under the agent (idle // janitor, server hangup) — deliberate divergence from Node, whose raw-set -// count starves the cap after N external closes. +// count starves the cap after N external closes. Post-mortem retained +// sessions (M6) stay OWNED (read_output keeps working); they just don't +// count toward the cap. package agent import ( "context" + "sync" "testing" "time" @@ -26,9 +29,17 @@ func TestCapIgnoresExternallyClosedSessions(t *testing.T) { ctx := context.Background() db.ExecContext(ctx, `INSERT INTO accounts (id,name,created_at,updated_at) VALUES ('acc','A','2026-01-01T00:00:00Z','2026-01-01T00:00:00Z')`) + // Keep every mock so the test can kill transports out from under the + // manager (server hangup), not via explicit close. + var mu sync.Mutex + var mocks []*terminal.MockTransport mgr := terminal.NewManager( func(context.Context, terminal.FactoryParams) (terminal.Transport, error) { - return terminal.NewMockTransport(), nil + m := terminal.NewMockTransport() + mu.Lock() + mocks = append(mocks, m) + mu.Unlock() + return m, nil }, clock.Real{}, 0) eps := store.NewEndpoints(db, clock.Real{}) sess := New(Deps{Manager: mgr, Endpoints: eps}, "acc", "", 2) @@ -36,8 +47,7 @@ func TestCapIgnoresExternallyClosedSessions(t *testing.T) { t.Fatal(err) } - // Fill the cap (2), then have the MANAGER close both — simulating the - // idle-timeout janitor / a server hangup, not an agent-initiated close. + // Fill the cap (2), write some output, then hang up both transports. var ids []string for i := 0; i < 2; i++ { s, err := sess.CreateSession(ctx, "ep1", "cap test") @@ -46,25 +56,80 @@ func TestCapIgnoresExternallyClosedSessions(t *testing.T) { } ids = append(ids, s.SessionID) } + if err := sess.SendKeys(ids[0], []string{"text:hello"}); err != nil { + t.Fatal(err) + } if _, err := sess.CreateSession(ctx, "ep1", "over cap"); err == nil { t.Fatal("cap should be enforced while sessions are live") } - for _, id := range ids { - mgr.Close(id, terminal.CloseIdleTimeout) + mu.Lock() + for _, m := range mocks { + _ = m.Close() + } + mu.Unlock() + waitForStatus(t, mgr, ids[0], terminal.StatusClosed) + waitForStatus(t, mgr, ids[1], terminal.StatusClosed) + + // The agent regains full capacity: dead sessions don't count... + for i := 0; i < 2; i++ { + if _, err := sess.CreateSession(ctx, "ep1", "after hangup"); err != nil { + t.Fatalf("create after external close %d: %v", i, err) + } + } + // ...but the dead ids stay OWNED: post-mortem output remains readable + // through the agent even after new creates pruned the live count. + r, err := sess.ReadOutput(ids[0], 0, 100) + if err != nil { + t.Fatalf("post-mortem read on retained session: %v", err) } - // Close is async through the transport pump; wait for the registry to drop them. + if string(r.Data) != "hello" { + t.Fatalf("post-mortem read: %q", r.Data) + } + if err := sess.CloseSession(ids[0]); err != nil { + t.Fatalf("close_session on retained session: %v", err) + } +} + +func waitForStatus(t *testing.T, mgr *terminal.Manager, id string, want terminal.Status) { + t.Helper() deadline := time.Now().Add(3 * time.Second) for time.Now().Before(deadline) { - if len(mgr.ListForAccount("acc")) == 0 { - break + if s := mgr.GetSession(id); s != nil && s.Status == want { + return } time.Sleep(10 * time.Millisecond) } + t.Fatalf("session %s never reached %s", id, want) +} - // The agent must regain full capacity: stale ids may not starve the cap. - for i := 0; i < 2; i++ { - if _, err := sess.CreateSession(ctx, "ep1", "after janitor"); err != nil { - t.Fatalf("create after external close %d: %v", i, err) - } +// max_sessions = 0 blocks every create (Node: 0 is honored, not defaulted; +// the Go REST path also 429s at 0 — MCP must match). +func TestCapZeroBlocksCreation(t *testing.T) { + db, err := store.Open("sqlite::memory:") + if err != nil { + t.Fatal(err) + } + defer db.Close() + if err := store.Migrate(db); err != nil { + t.Fatal(err) + } + ctx := context.Background() + db.ExecContext(ctx, `INSERT INTO accounts (id,name,created_at,updated_at) VALUES ('acc','A','2026-01-01T00:00:00Z','2026-01-01T00:00:00Z')`) + mgr := terminal.NewManager( + func(context.Context, terminal.FactoryParams) (terminal.Transport, error) { + return terminal.NewMockTransport(), nil + }, clock.Real{}, 0) + eps := store.NewEndpoints(db, clock.Real{}) + sess := New(Deps{Manager: mgr, Endpoints: eps}, "acc", "", 0) + if err := sess.CreateEndpoint(ctx, store.Endpoint{ID: "ep1", Label: "Box", Host: "h", Port: 22, Username: "u", UserVerification: "required"}); err != nil { + t.Fatal(err) + } + if _, err := sess.CreateSession(ctx, "ep1", "blocked"); err == nil { + t.Fatal("max_sessions=0 must block creation") + } + // Negative = "no cap resolved" -> default 5 still applies. + sess2 := New(Deps{Manager: mgr, Endpoints: eps}, "acc", "", -1) + if _, err := sess2.CreateSession(ctx, "ep1", "default cap"); err != nil { + t.Fatalf("default-cap create: %v", err) } } diff --git a/internal/agent/session_endpoint_test.go b/internal/agent/session_endpoint_test.go index c2b86f1..9a11750 100644 --- a/internal/agent/session_endpoint_test.go +++ b/internal/agent/session_endpoint_test.go @@ -57,7 +57,12 @@ func TestSessionEndpointMutations(t *testing.T) { if err != nil || !ok { t.Fatalf("delete: ok=%v err=%v", ok, err) } - if ep, _ := sess.GetEndpoint(ctx, "ep1"); ep != nil { - t.Error("endpoint still present after delete") + // Soft delete (Node parity): hidden from the list, but get-by-id still + // resolves — endpoint-repo.ts filters enabled only in findAllForAccount. + if eps, _ := sess.ListEndpoints(ctx); len(eps) != 0 { + t.Errorf("deleted endpoint still listed: %+v", eps) + } + if ep, _ := sess.GetEndpoint(ctx, "ep1"); ep == nil { + t.Error("get-by-id must still resolve a soft-deleted endpoint (Node parity)") } } diff --git a/internal/agentproxy/route.go b/internal/agentproxy/route.go index 6bb3cc7..e9ef149 100644 --- a/internal/agentproxy/route.go +++ b/internal/agentproxy/route.go @@ -73,8 +73,8 @@ func (d *Deps) Handler() http.HandlerFunc { ClientVersion: util.SanitizeClientReported(r.Header.Get("X-ShellWatch-Version")), }) - // Cancel stranded approvals when the connection ends. - defer d.Broker.Store().CancelForConnection(connID, "agent-proxy connection closed") + // Cancel stranded approvals when the connection ends (+ clear toasts). + defer d.Broker.CancelForConnection(connID, "agent-proxy connection closed") rw := newWSReadWriter(ctx, wsc) _ = agent.ServeAgent(ba, rw) // returns on I/O error (client disconnect) diff --git a/internal/approval/broker.go b/internal/approval/broker.go index 19ccac6..6b6a798 100644 --- a/internal/approval/broker.go +++ b/internal/approval/broker.go @@ -19,6 +19,8 @@ var ( ErrDenied = errors.New("signing request denied") // ErrExpired is returned when the action TTL elapses. ErrExpired = errors.New("signing request expired") + // ErrCancelled is returned when the owning connection died. + ErrCancelled = errors.New("signing request cancelled") ) // Channel delivers an action to a notification surface (WS toast, push). @@ -112,6 +114,18 @@ func (b *Broker) RequestKeyApproval(ctx context.Context, accountID, keyLabel, ke } } +// CancelForConnection cancels a dead connection's pending actions AND clears +// their toasts on every channel (index.ts:112-114, M9). An awaiter unblocked +// by the cancel's reject may also notify — a duplicate sign:resolved for the +// same action id is an idempotent toast removal client-side. +func (b *Broker) CancelForConnection(connectionID, reason string) int { + cancelled := b.store.CancelForConnection(connectionID, reason) + for _, a := range cancelled { + b.notifyResolved(a) + } + return len(cancelled) +} + func (b *Broker) notifyResolved(a *Action) { for _, ch := range b.channels { ch.Resolved(a) diff --git a/internal/approval/store.go b/internal/approval/store.go index 1830283..f91936a 100644 --- a/internal/approval/store.go +++ b/internal/approval/store.go @@ -261,9 +261,15 @@ func (s *Store) Deny(id string) bool { } // CancelForConnection denies every pending action for a dead SSH connection -// (fix for #91: stranded prompts don't outlive the session). The reject -// closure is NOT called — the awaiter is already gone. -func (s *Store) CancelForConnection(connectionID, reason string) int { +// (fix for #91: stranded prompts don't outlive the session). Unlike Node — +// where an unresolved promise is simply garbage-collected — the reject +// closure MUST be called here: a terminal-path forwarding awaiter blocks on +// the broker with context.Background() (passkey_factory.go), so a cancel +// that never rejects would strand that goroutine forever. The error channels +// are buffered, so rejecting an awaiter that already left (agent-proxy's +// request context cancelled first) is harmless. Returns the cancelled +// actions so the caller can clear their toasts (Broker.CancelForConnection). +func (s *Store) CancelForConnection(connectionID, reason string) []*Action { s.mu.Lock() var cancelled []*Action for _, a := range s.actions { @@ -274,21 +280,33 @@ func (s *Store) CancelForConnection(connectionID, reason string) int { } s.mu.Unlock() for _, a := range cancelled { + if a.reject != nil { + a.reject(ErrCancelled) + } s.emitResolved(a, OutcomeCancelled, reason) } - return len(cancelled) + return cancelled } -// Sweep expires overdue pending actions (janitor). +// cleanupGrace keeps terminal-state actions fetchable for status polling +// before Sweep deletes them (store.ts:143-145). +const cleanupGrace = 120 * time.Second + +// Sweep expires overdue pending actions and deletes terminal-state actions +// older than the grace window (janitor) — without the delete the store grows +// forever and resolved actions stay fetchable indefinitely. func (s *Store) Sweep() { now := s.clk.Now() s.mu.Lock() var expired []*Action - for _, a := range s.actions { + for id, a := range s.actions { if a.Status == StatusPending && !a.ExpiresAt.After(now) { a.Status = StatusExpired expired = append(expired, a) } + if a.Status != StatusPending && now.Sub(a.ExpiresAt) > cleanupGrace { + delete(s.actions, id) + } } s.mu.Unlock() for _, a := range expired { diff --git a/internal/approval/store_test.go b/internal/approval/store_test.go index 4ec945c..12cd833 100644 --- a/internal/approval/store_test.go +++ b/internal/approval/store_test.go @@ -2,6 +2,7 @@ package approval import ( + "context" "testing" "time" @@ -60,24 +61,55 @@ func TestStoreExpireRejectsAndEmits(t *testing.T) { } } -func TestStoreCancelForConnectionDoesNotReject(t *testing.T) { +// Deliberate divergence from Node: cancel MUST reject. Node can drop the +// resolver (the pending promise is garbage-collected); in Go a forwarding +// awaiter blocks on the broker with context.Background(), so a cancel that +// never rejects strands that goroutine forever. +func TestStoreCancelForConnectionRejectsWithCancelled(t *testing.T) { s, _ := newStore(t) var outcome Outcome var cancelReason string s.OnResolved(func(e ResolvedEvent) { outcome = e.Outcome; cancelReason = e.CancelReason }) - rejected := false + var rejectedWith error s.Create(CreateParams{AccountID: "acc", Type: TypeWebAuthnSign, ConnectionID: "c1", - Reject: func(error) { rejected = true }}) + Reject: func(err error) { rejectedWith = err }}) - if n := s.CancelForConnection("c1", "connection closed"); n != 1 { - t.Fatalf("cancelled %d", n) + if cancelled := s.CancelForConnection("c1", "connection closed"); len(cancelled) != 1 { + t.Fatalf("cancelled %d", len(cancelled)) } - // The reject closure is NOT called on cancel (awaiter already gone), but - // the audit outcome is "cancelled". - if rejected { - t.Error("reject should not be called on cancel") + if rejectedWith != ErrCancelled { + t.Errorf("reject: got %v, want ErrCancelled", rejectedWith) } if outcome != OutcomeCancelled || cancelReason != "connection closed" { t.Fatalf("outcome %s reason %q", outcome, cancelReason) } } + +// The leak scenario end-to-end: an awaiter blocked with a non-cancellable +// context (terminal-path agent forwarding) must be released by a +// connection-cancel, not stranded until process exit. +func TestBrokerCancelUnblocksBackgroundAwaiter(t *testing.T) { + s, _ := newStore(t) + b := NewBroker(s, func() string { return "https://sw.example" }) + + done := make(chan error, 1) + go func() { + done <- b.RequestKeyApproval(context.Background(), "acc", "key", "SHA256:fp", "c1", Context{Source: "agent-forwarding"}) + }() + // Wait for the action to exist, then cancel the connection. + deadline := time.Now().Add(3 * time.Second) + for time.Now().Before(deadline) { + if len(s.CancelForConnection("c1", "SSH connection closed")) == 1 { + break + } + time.Sleep(5 * time.Millisecond) + } + select { + case err := <-done: + if err != ErrCancelled { + t.Fatalf("awaiter returned %v, want ErrCancelled", err) + } + case <-time.After(3 * time.Second): + t.Fatal("awaiter still blocked after connection cancel — goroutine leak") + } +} diff --git a/internal/audit/sessions.go b/internal/audit/sessions.go index c5df50f..973ac60 100644 --- a/internal/audit/sessions.go +++ b/internal/audit/sessions.go @@ -16,7 +16,7 @@ import ( const ( pageLimitDefault = 50 - pageLimitMax = 200 + pageLimitMax = 500 // PAGE_LIMIT_MAX, session-lifecycle-repo.ts:61 ) // SessionRow is one session-lifecycle audit record (audit_session_lifecycle). @@ -77,9 +77,9 @@ func (s *Sessions) List(ctx context.Context, accountID string, f SessionFilters, conds = append(conds, "created_at <= ?") args = append(args, f.To) } - if c := decodeCursor(cursorStr); c != nil { + if c := decodeSessionCursor(cursorStr); c != nil { conds = append(conds, "(created_at < ? OR (created_at = ? AND session_id < ?))") - args = append(args, c.CreatedAt, c.CreatedAt, c.ID) + args = append(args, c.CreatedAt, c.CreatedAt, c.SessionID) } query := `SELECT session_id, account_id, endpoint_id, source, status, created_at, @@ -95,7 +95,7 @@ func (s *Sessions) List(ctx context.Context, accountID string, f SessionFilters, } defer rows.Close() - var out []SessionRow + out := []SessionRow{} // non-nil: empty pages serialize as "rows": [] for rows.Next() { var r SessionRow var closedAt, sourceIP, mcpReason, mcpName, mcpVer, cHost, cOS, cVer, closeReason sql.NullString @@ -121,12 +121,21 @@ func (s *Sessions) List(ctx context.Context, accountID string, f SessionFilters, return Page[SessionRow]{}, err } - next := paginate(&out, limit, func(r SessionRow) cursor { return cursor{CreatedAt: r.CreatedAt, ID: r.SessionID} }) + next := paginate(&out, limit, func(r SessionRow) any { + return sessionCursor{CreatedAt: r.CreatedAt, SessionID: r.SessionID} + }) return Page[SessionRow]{Rows: out, NextCursor: next}, nil } // --- cursor + helpers --- +// sessionCursor matches Node's sessions cursor payload {createdAt, sessionId} +// (session-lifecycle-repo.ts:166-168); signings use {createdAt, id}. +type sessionCursor struct { + CreatedAt string `json:"createdAt"` + SessionID string `json:"sessionId"` +} + type cursor struct { CreatedAt string `json:"createdAt"` ID string `json:"id"` @@ -142,7 +151,7 @@ func clampLimit(n int) int { return n } -func encodeCursor(c cursor) string { +func encodeCursor(c any) string { raw, _ := json.Marshal(c) return base64.RawURLEncoding.EncodeToString(raw) } @@ -162,8 +171,23 @@ func decodeCursor(raw string) *cursor { return &c } +func decodeSessionCursor(raw string) *sessionCursor { + if raw == "" { + return nil + } + data, err := base64.RawURLEncoding.DecodeString(raw) + if err != nil { + return nil + } + var c sessionCursor + if json.Unmarshal(data, &c) != nil || c.CreatedAt == "" || c.SessionID == "" { + return nil + } + return &c +} + // paginate trims an over-fetched slice to limit and returns the next cursor. -func paginate[T any](rows *[]T, limit int, key func(T) cursor) *string { +func paginate[T any](rows *[]T, limit int, key func(T) any) *string { if len(*rows) <= limit { return nil } diff --git a/internal/audit/signings.go b/internal/audit/signings.go index 0a1a820..ad89140 100644 --- a/internal/audit/signings.go +++ b/internal/audit/signings.go @@ -111,7 +111,7 @@ func (s *Signings) List(ctx context.Context, accountID string, f SigningFilters, return Page[SigningRow]{}, err } defer rows.Close() - var out []SigningRow + out := []SigningRow{} // non-nil: empty pages serialize as "rows": [] for rows.Next() { r, err := scanSigning(rows) if err != nil { @@ -122,7 +122,7 @@ func (s *Signings) List(ctx context.Context, accountID string, f SigningFilters, if err := rows.Err(); err != nil { return Page[SigningRow]{}, err } - next := paginate(&out, limit, func(r SigningRow) cursor { return cursor{CreatedAt: r.CreatedAt, ID: r.ID} }) + next := paginate(&out, limit, func(r SigningRow) any { return cursor{CreatedAt: r.CreatedAt, ID: r.ID} }) return Page[SigningRow]{Rows: out, NextCursor: next}, nil } diff --git a/internal/config/config.go b/internal/config/config.go index fdb3d42..937a727 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -181,8 +181,11 @@ func Load(configPath string) (*Config, error) { return nil, fmt.Errorf("invalid config at %s:\n - %s", resolved, strings.Join(errs, "\n - ")) } - // Post-load derivations (loader.ts): - cfg.KeyDirectory = filepath.Join(filepath.Dir(resolved), cfg.KeyDirectory) + // Post-load derivations (loader.ts). path.resolve semantics: an absolute + // keyDirectory wins; a relative one resolves against the config file's dir. + if !filepath.IsAbs(cfg.KeyDirectory) { + cfg.KeyDirectory = filepath.Join(filepath.Dir(resolved), cfg.KeyDirectory) + } if abs, err := filepath.Abs(cfg.KeyDirectory); err == nil { cfg.KeyDirectory = abs } diff --git a/internal/httpserver/mcp_lifecycle_test.go b/internal/httpserver/mcp_lifecycle_test.go index 2e1ed80..adcae94 100644 --- a/internal/httpserver/mcp_lifecycle_test.go +++ b/internal/httpserver/mcp_lifecycle_test.go @@ -253,3 +253,24 @@ func TestMCPIdleSessionExpiryClosesEverything(t *testing.T) { t.Fatalf("expired session id: got %d %q, want uniform JSON-RPC 404", res.StatusCode, body) } } + +// M3: initialize returns live-endpoint instructions. +func TestMCPServerInstructions(t *testing.T) { + ts, _ := mcpLifecycleServer(t, 0) + sess := mcpConnectAs(t, ts, "tok-a") + defer sess.Close() + res := sess.InitializeResult() + if res == nil { + t.Fatal("no initialize result") + } + for _, want := range []string{ + "ShellWatch is an SSH session broker", + "- ep-a: Box A (u@127.0.0.1:22)", + "shellwatch_create_session", + "sudo:", + } { + if !strings.Contains(res.Instructions, want) { + t.Errorf("instructions missing %q\n%s", want, res.Instructions) + } + } +} diff --git a/internal/hydra/routes.go b/internal/hydra/routes.go index 021587d..f976bb1 100644 --- a/internal/hydra/routes.go +++ b/internal/hydra/routes.go @@ -94,10 +94,13 @@ func mountProviderPages(r chi.Router, p ProviderParams) { writeHTML(w, 400, renderErrorPage("invalid_consent_challenge", "")) return } - // First-party SPA (and remembered sessions) auto-accept — no second passkey. + // First-party SPA (and remembered sessions) auto-accept — no second + // passkey. remember/remember_for keep the SPA client in "Authorized + // clients" and renew remembered skips (routes.ts:360-370, M13). if cr.Client.ClientID == p.SPAClientID || cr.Skip { redir, err := p.Admin.AcceptConsentRequest(r.Context(), challenge, AcceptConsent{ GrantScope: cr.RequestedScope, GrantAccessTokenAudience: cr.RequestedAccessTokenAudience, + Remember: true, RememberFor: rememberFor, }) if err != nil { writeHTML(w, 400, renderErrorPage("consent_flow_expired", "")) diff --git a/internal/mcp/instructions.go b/internal/mcp/instructions.go new file mode 100644 index 0000000..21aa32e --- /dev/null +++ b/internal/mcp/instructions.go @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: LicenseRef-FSL-1.1-Apache-2.0 +// Server instructions (port of the instructions block in src/mcp/server.ts): +// a live endpoint list + workflow / session-lifecycle / sudo guidance the +// client model reads at initialize time. One deliberate divergence: Node's +// "Notifications:" section is omitted — the go-sdk cannot send custom +// notification methods (M1 is blocked on that), and promising notifications +// that never arrive would make agents wait instead of polling read_output. +package mcp + +import ( + "fmt" + "strings" + + "github.com/rado0x54/shellwatch/internal/agent" +) + +func buildInstructions(endpoints []agent.EndpointInfo) string { + lines := make([]string, 0, len(endpoints)) + for _, e := range endpoints { + head := fmt.Sprintf("- %s: %s (%s@%s:%d)", e.ID, e.Label, e.Username, e.Host, e.Port) + if e.Description != nil && *e.Description != "" { + head += "\n description: " + *e.Description + } + lines = append(lines, head) + } + endpointList := strings.Join(lines, "\n") + + return strings.Join([]string{ + "ShellWatch is an SSH session broker. You can create terminal sessions to remote servers, send commands, and read output.", + "", + "Available endpoints:", + endpointList, + "", + "Workflow:", + "1. Create a session with shellwatch_create_session (pick an endpoint ID from above)", + `2. Send commands with shellwatch_send_keys (e.g., keys: ["text:ls -la", "enter"])`, + "3. Read the result with shellwatch_read_output (use afterOffset for incremental reads)", + "4. Keep the session open for follow-up commands — do NOT close it after each command", + "5. Only close with shellwatch_close_session when you are certain no more interactions are needed", + "", + "Session lifecycle:", + "- Sessions are automatically closed when your MCP connection ends — you do not need to close them manually", + "- Keep sessions open between commands so the human observer can see your work and send follow-ups", + "- Creating a new session for every command is wasteful — reuse your existing session", + "", + "sudo:", + "- Do NOT pass -n (non-interactive). The human operator can attach to your session and type the password directly, so a [sudo] password: prompt is not a failure mode.", + "- If you see a [sudo] password: prompt, ask the operator (in your reply) to enter the password in the session, then continue once read_output shows the prompt has cleared.", + "- Do NOT chain sudo commands with && or || (e.g., `sudo cmd1 && sudo cmd2`). When a prompt appears the operator can't tell which command it belongs to. Send each sudo command separately so every prompt is unambiguous.", + "- Sudo auth may go through a PAM module the operator satisfies out-of-band (push notification, hardware token, etc.) and can take tens of seconds — possibly falling back to a password prompt if the out-of-band step is declined or times out. A stalled prompt is not failure: keep polling read_output until the prompt clears or you see an explicit denial.", + }, "\n") +} diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 0fc4d11..04c02f8 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -27,10 +27,15 @@ import ( type Deps struct { AgentDeps agent.Deps Keys *store.SSHKeys - MaxOwned int + // MaxOwned resolves the account's concurrent-session cap (accounts. + // max_sessions, http-transport.ts:113-124). nil/miss -> agent default (5). + MaxOwned func(ctx context.Context, accountID string) (int, bool) // NewID mints Mcp-Session-Ids (Node uses randomUUID); nil falls back to a // local v4 generator. NewID func() string + // Version is the serverInfo version (Node uses buildInfo.display; "" falls + // back to "1.0.0"). + Version string // SessionTimeout closes MCP sessions with no in-flight HTTP activity for // this long (0 = never, the Node behavior). Wired from // mcp.sessionTimeoutMinutes; the sdk timer is suspended while any request @@ -87,8 +92,19 @@ func (d *Deps) Handler() http.Handler { if !ok { return nil } - as := agent.New(d.AgentDeps, principal.AccountID, realip.FromRequest(r), d.MaxOwned) - return d.buildServer(as, principal.AccountID) + maxOwned := -1 // unresolved -> agent.New defaults to 5; explicit 0 blocks + if d.MaxOwned != nil { + if m, ok := d.MaxOwned(r.Context(), principal.AccountID); ok { + maxOwned = m + } + } + as := agent.New(d.AgentDeps, principal.AccountID, realip.FromRequest(r), maxOwned) + // Instructions carry the account's live endpoint list (server.ts:32-73). + instructions := "" + if eps, err := as.ListEndpoints(r.Context()); err == nil { + instructions = buildInstructions(eps) + } + return d.buildServer(as, principal.AccountID, instructions) }, &mcpsdk.StreamableHTTPOptions{ // go-sdk's DNS-rebinding protection 403s a loopback local address with // a non-loopback Host — which is exactly a reverse-proxy deployment @@ -120,8 +136,13 @@ func sendSessionNotFound(w http.ResponseWriter) { _, _ = w.Write([]byte(`{"jsonrpc":"2.0","error":{"code":-32001,"message":"Session not found"},"id":null}`)) } -func (d *Deps) buildServer(as *agent.Session, accountID string) *mcpsdk.Server { - srv := mcpsdk.NewServer(&mcpsdk.Implementation{Name: "shellwatch", Version: "1.0.0"}, &mcpsdk.ServerOptions{ +func (d *Deps) buildServer(as *agent.Session, accountID, instructions string) *mcpsdk.Server { + version := d.Version + if version == "" { + version = "1.0.0" + } + srv := mcpsdk.NewServer(&mcpsdk.Implementation{Name: "shellwatch", Version: version}, &mcpsdk.ServerOptions{ + Instructions: instructions, GetSessionID: d.newSessionID, // On initialized: capture the client's advertised name/version for the // approval UI (agent-session clientInfo, M4). diff --git a/internal/rest/actions.go b/internal/rest/actions.go index 75cef13..bf80085 100644 --- a/internal/rest/actions.go +++ b/internal/rest/actions.go @@ -107,7 +107,9 @@ func (a *Actions) deny(w http.ResponseWriter, r *http.Request) { func actionView(a *approval.Action) map[string]any { v := map[string]any{ "id": a.ID, "accountId": a.AccountID, "type": string(a.Type), "status": string(a.Status), - "createdAt": a.CreatedAt.UTC().Format(isoMillis), "expiresAt": a.ExpiresAt.UTC().Format(isoMillis), + // Epoch-ms integers (contract + store.ts:46-47) — countdown clients do + // arithmetic on these. + "createdAt": a.CreatedAt.UnixMilli(), "expiresAt": a.ExpiresAt.UnixMilli(), "context": a.Context, } if a.RedirectTo != "" { diff --git a/internal/sshx/passkey_factory.go b/internal/sshx/passkey_factory.go index 31c60ab..6f353ae 100644 --- a/internal/sshx/passkey_factory.go +++ b/internal/sshx/passkey_factory.go @@ -57,6 +57,10 @@ type PasskeyFactoryParams struct { // NewConnectionID mints per-connection ids so a dead connection's stranded // approvals can be cancelled (broker.CancelForConnection). NewConnectionID func() string + // OnConnectionEnded fires once when a connection dies — on connect failure + // or when the established transport ends — so stranded sign prompts don't + // linger the full TTL (#91; create-factory.ts:147-156 -> index.ts:102-115). + OnConnectionEnded func(connectionID, reason string) } // labeledFileKey is one admin file key selected for a connection. @@ -109,13 +113,47 @@ func NewPasskeyFactory(p PasskeyFactoryParams) terminal.TransportFactory { if fp.Endpoint.AgentForward { fwdAgent = p.buildForwardingAgent(fp, connID, fileKeys) } - return Connect(ctx, ConnectParams{ + transport, err := Connect(ctx, ConnectParams{ Host: fp.Endpoint.Host, Port: fp.Endpoint.Port, Username: fp.Endpoint.Username, Signers: signers, AgentForward: fp.Endpoint.AgentForward, ForwardingAgent: fwdAgent, }) + if p.OnConnectionEnded == nil { + return transport, err + } + if err != nil { + // Failed auth can leave prompts pending (e.g. an ignored key-approve + // while the passkey ceremony timed out) — cancel them now. + p.OnConnectionEnded(connID, "SSH connection closed") + return nil, err + } + return watchTransportEnd(transport, func() { + p.OnConnectionEnded(connID, "SSH connection closed") + }), nil } } +// watchTransportEnd forwards a transport's events unchanged and invokes onEnd +// exactly once when the underlying event stream ends (any close/error path — +// the transport closes its channel in all of them). +func watchTransportEnd(t terminal.Transport, onEnd func()) terminal.Transport { + w := &endWatchTransport{Transport: t, events: make(chan terminal.Event, 16)} + go func() { + for ev := range t.Events() { + w.events <- ev + } + close(w.events) + onEnd() + }() + return w +} + +type endWatchTransport struct { + terminal.Transport + events chan terminal.Event +} + +func (w *endWatchTransport) Events() <-chan terminal.Event { return w.events } + func (p PasskeyFactoryParams) buildSigners(ctx context.Context, fp terminal.FactoryParams, connID string, fileKeys []labeledFileKey) ([]ssh.Signer, error) { var signers []ssh.Signer diff --git a/internal/sshx/transport.go b/internal/sshx/transport.go index e07d98e..a257b48 100644 --- a/internal/sshx/transport.go +++ b/internal/sshx/transport.go @@ -8,6 +8,7 @@ package sshx import ( "context" + "errors" "fmt" "io" "log/slog" @@ -193,14 +194,24 @@ func Connect(ctx context.Context, p ConnectParams) (terminal.Transport, error) { go t.pipe(stdout, &wg) go t.pipe(stderr, &wg) go func() { - wg.Wait() // both streams EOF - _ = session.Wait() // reap the remote command + wg.Wait() // both streams EOF + werr := session.Wait() // reap the remote command t.mu.Lock() already := t.closed t.closed = true t.mu.Unlock() if !already { - t.events <- terminal.Event{Closed: true} + // Distinguish a broken transport from a normal end (ssh2 parity: + // client "error" vs "close", ssh-transport.ts:42-44). A nonzero + // remote exit or a close without exit-status are still normal + // closes (shell exit, server hangup); anything else — reset, + // protocol error — surfaces as an error event so the manager + // records status=error / transport-error. + if werr != nil && !isNormalSessionEnd(werr) { + t.events <- terminal.Event{Err: werr} + } else { + t.events <- terminal.Event{Closed: true} + } } close(t.events) _ = client.Close() @@ -208,3 +219,12 @@ func Connect(ctx context.Context, p ConnectParams) (terminal.Transport, error) { return t, nil } + +func isNormalSessionEnd(err error) bool { + if err == nil || errors.Is(err, io.EOF) { + return true + } + var exitErr *ssh.ExitError + var missingErr *ssh.ExitMissingError + return errors.As(err, &exitErr) || errors.As(err, &missingErr) +} diff --git a/internal/sshx/transport_test.go b/internal/sshx/transport_test.go index f0f138a..0e2a5ef 100644 --- a/internal/sshx/transport_test.go +++ b/internal/sshx/transport_test.go @@ -156,7 +156,10 @@ func TestServerHangupClosesSession(t *testing.T) { t.Fatalf("create: %v", err) } deadline := time.Now().Add(2 * time.Second) - for mgr.GetSession(sess.SessionID) != nil { + for { + if s := mgr.GetSession(sess.SessionID); s != nil && s.Status == terminal.StatusClosed { + break + } if time.Now().After(deadline) { t.Fatal("session not closed on server hangup") } @@ -165,6 +168,11 @@ func TestServerHangupClosesSession(t *testing.T) { if lastStatus != terminal.StatusClosed { t.Errorf("final status: %v", lastStatus) } + // Node parity (M6): a transport-driven close RETAINS the session for + // post-mortem reads but hides it from lists; explicit Close removes it. + if len(mgr.ListSessions()) != 0 { + t.Errorf("hung-up session still listed: %+v", mgr.ListSessions()) + } } func waitForOutput(t *testing.T, mgr *terminal.Manager, sessionID, want string) { diff --git a/internal/store/endpoints.go b/internal/store/endpoints.go index c7806df..fe658fd 100644 --- a/internal/store/endpoints.go +++ b/internal/store/endpoints.go @@ -99,9 +99,12 @@ func (e *Endpoints) Update(ctx context.Context, ep Endpoint) (bool, error) { return n > 0, err } -// Delete removes an endpoint; returns false when nothing matched. +// Delete soft-deletes an endpoint (enabled=0, Node parity); returns false +// when nothing matched. func (e *Endpoints) Delete(ctx context.Context, id, accountID string) (bool, error) { - n, err := gen.New(e.db).DeleteEndpointForAccount(ctx, gen.DeleteEndpointForAccountParams{ID: id, AccountID: accountID}) + n, err := gen.New(e.db).DeleteEndpointForAccount(ctx, gen.DeleteEndpointForAccountParams{ + UpdatedAt: e.clk.Now().UTC().Format(isoMillis), ID: id, AccountID: accountID, + }) return n > 0, err } diff --git a/internal/store/gen/endpoints.sql.go b/internal/store/gen/endpoints.sql.go index 1791a93..392f9f2 100644 --- a/internal/store/gen/endpoints.sql.go +++ b/internal/store/gen/endpoints.sql.go @@ -11,16 +11,19 @@ import ( ) const deleteEndpointForAccount = `-- name: DeleteEndpointForAccount :execrows -DELETE FROM endpoints WHERE id = ? AND account_id = ? +UPDATE endpoints SET enabled = 0, updated_at = ? WHERE id = ? AND account_id = ? ` type DeleteEndpointForAccountParams struct { + UpdatedAt string ID string AccountID string } +// Soft delete (endpoint-repo.ts:146-152): history keeps its endpoint rows and +// a shared-data-dir cutover can't resurrect Node-era deletions. func (q *Queries) DeleteEndpointForAccount(ctx context.Context, arg DeleteEndpointForAccountParams) (int64, error) { - result, err := q.db.ExecContext(ctx, deleteEndpointForAccount, arg.ID, arg.AccountID) + result, err := q.db.ExecContext(ctx, deleteEndpointForAccount, arg.UpdatedAt, arg.ID, arg.AccountID) if err != nil { return 0, err } @@ -118,7 +121,7 @@ func (q *Queries) InsertEndpoint(ctx context.Context, arg InsertEndpointParams) const listEndpointsForAccount = `-- name: ListEndpointsForAccount :many SELECT id, account_id, label, host, port, username, user_verification, description, agent_forward -FROM endpoints WHERE account_id = ? ORDER BY created_at, id +FROM endpoints WHERE account_id = ? AND enabled = 1 ORDER BY created_at, id ` type ListEndpointsForAccountRow struct { @@ -136,6 +139,9 @@ type ListEndpointsForAccountRow struct { // SPDX-License-Identifier: LicenseRef-FSL-1.1-Apache-2.0 // Endpoint queries (Phase 3). Every account-owned query takes account_id in // SQL (W13). Keep pure ASCII (sqlc offset bug on multi-byte chars). +// enabled filter matches Node (findAllForAccount, endpoint-repo.ts:87): a +// soft-deleted endpoint is hidden from lists. Get-by-id deliberately does NOT +// filter (Node parity: post-delete session create / PUT by id still work). func (q *Queries) ListEndpointsForAccount(ctx context.Context, accountID string) ([]ListEndpointsForAccountRow, error) { rows, err := q.db.QueryContext(ctx, listEndpointsForAccount, accountID) if err != nil { diff --git a/internal/store/queries/endpoints.sql b/internal/store/queries/endpoints.sql index e63418b..2d4e90b 100644 --- a/internal/store/queries/endpoints.sql +++ b/internal/store/queries/endpoints.sql @@ -3,8 +3,11 @@ -- SQL (W13). Keep pure ASCII (sqlc offset bug on multi-byte chars). -- name: ListEndpointsForAccount :many +-- enabled filter matches Node (findAllForAccount, endpoint-repo.ts:87): a +-- soft-deleted endpoint is hidden from lists. Get-by-id deliberately does NOT +-- filter (Node parity: post-delete session create / PUT by id still work). SELECT id, account_id, label, host, port, username, user_verification, description, agent_forward -FROM endpoints WHERE account_id = ? ORDER BY created_at, id; +FROM endpoints WHERE account_id = ? AND enabled = 1 ORDER BY created_at, id; -- name: GetEndpointForAccount :one SELECT id, account_id, label, host, port, username, user_verification, description, agent_forward @@ -17,7 +20,9 @@ INSERT INTO endpoints ( ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, 1, ?, ?); -- name: DeleteEndpointForAccount :execrows -DELETE FROM endpoints WHERE id = ? AND account_id = ?; +-- Soft delete (endpoint-repo.ts:146-152): history keeps its endpoint rows and +-- a shared-data-dir cutover can't resurrect Node-era deletions. +UPDATE endpoints SET enabled = 0, updated_at = ? WHERE id = ? AND account_id = ?; -- name: GetShowDemoEndpoints :one SELECT show_demo_endpoints FROM accounts WHERE id = ?; diff --git a/internal/terminal/manager.go b/internal/terminal/manager.go index df960f3..a0250c6 100644 --- a/internal/terminal/manager.go +++ b/internal/terminal/manager.go @@ -183,10 +183,10 @@ func (m *Manager) pump(mg *managed) { for ev := range mg.transport.Events() { switch { case ev.Err != nil: - m.setStatus(mg, StatusError, reasonOr(mg, CloseTransportError)) + m.setStatus(mg, StatusError, m.reasonOr(mg, CloseTransportError)) return case ev.Closed: - m.setStatus(mg, StatusClosed, reasonOr(mg, CloseServerHangup)) + m.setStatus(mg, StatusClosed, m.reasonOr(mg, CloseServerHangup)) return default: mg.output.Append(ev.Data) @@ -203,10 +203,15 @@ func (m *Manager) pump(mg *managed) { } } } - m.setStatus(mg, StatusClosed, reasonOr(mg, CloseServerHangup)) + m.setStatus(mg, StatusClosed, m.reasonOr(mg, CloseServerHangup)) } -func reasonOr(mg *managed, fallback CloseReason) CloseReason { +// reasonOr reads the stamped close reason under the lock (setStatus / +// beginClosing write it under the same lock — the pump calls this +// concurrently with an explicit Close). +func (m *Manager) reasonOr(mg *managed, fallback CloseReason) CloseReason { + m.mu.Lock() + defer m.mu.Unlock() if mg.session.CloseReason != "" { return mg.session.CloseReason } @@ -276,38 +281,43 @@ func (m *Manager) Resize(sessionID string, cols, rows int) error { return mg.transport.Resize(cols, rows) } -// ListSessions returns a snapshot of all sessions. +// ListSessions returns a snapshot of all non-closed sessions. Closed sessions +// are retained in the registry (post-mortem tail/re-attach) but hidden from +// lists; errored ones stay visible (listSessions filter, terminal-manager.ts). func (m *Manager) ListSessions() []Session { m.mu.Lock() defer m.mu.Unlock() out := make([]Session, 0, len(m.terminals)) for _, mg := range m.terminals { + if mg.session.Status == StatusClosed { + continue + } out = append(out, *mg.session) } return out } -// ListForAccount returns an account's sessions. +// ListForAccount returns an account's non-closed sessions. func (m *Manager) ListForAccount(accountID string) []Session { m.mu.Lock() defer m.mu.Unlock() out := make([]Session, 0) for _, mg := range m.terminals { - if mg.session.AccountID == accountID { + if mg.session.AccountID == accountID && mg.session.Status != StatusClosed { out = append(out, *mg.session) } } return out } -// EndpointIDsForAccount lists endpoint ids an account has open sessions on -// (satisfies rest.SessionLister for the endpoint-delete guard). +// EndpointIDsForAccount lists endpoint ids an account has non-closed sessions +// on (satisfies rest.SessionLister for the endpoint-delete guard). func (m *Manager) EndpointIDsForAccount(accountID string) []string { m.mu.Lock() defer m.mu.Unlock() var ids []string for _, mg := range m.terminals { - if mg.session.AccountID == accountID { + if mg.session.AccountID == accountID && mg.session.Status != StatusClosed { ids = append(ids, mg.session.EndpointID) } } @@ -326,22 +336,61 @@ func (m *Manager) GetSession(sessionID string) *Session { return &s } -// Close closes a session with a reason. +// Close explicitly closes a session: transport down, buffer cleared, removed +// from the registry (terminal-manager.ts close()). No-op when already +// closed/closing — a retained post-mortem session stays readable until the +// process ends, mirroring Node. func (m *Manager) Close(sessionID string, reason CloseReason) { mg, err := m.get(sessionID) if err != nil { return } - m.setStatus(mg, StatusClosing, reason) + // The guard and the closing transition must be one atomic step: a + // transport-driven Closed from the pump racing this call must either win + // (we bail) or lose (it becomes the normal closing->closed step) — never + // interleave into a closed->closing regression. + if !m.beginClosing(mg, reason) { + return + } _ = mg.transport.Close() + mg.output.Clear() + m.setStatus(mg, StatusClosed, reason) + m.mu.Lock() + delete(m.terminals, sessionID) + m.mu.Unlock() } -// CloseAllForAccount closes an account's sessions (returns count). +// beginClosing atomically checks the status and transitions to closing, +// firing status hooks; false when the session is already closing/closed. +func (m *Manager) beginClosing(mg *managed, reason CloseReason) bool { + m.mu.Lock() + prev := mg.session.Status + if prev == StatusClosed || prev == StatusClosing { + m.mu.Unlock() + return false + } + mg.session.Status = StatusClosing + if reason != "" && mg.session.CloseReason == "" { + mg.session.CloseReason = reason + } + subs := make([]func(StatusEvent), 0, len(m.statusSubs)) + for _, fn := range m.statusSubs { + subs = append(subs, fn) + } + ev := StatusEvent{SessionID: mg.session.SessionID, Status: StatusClosing, Previous: prev, Reason: mg.session.CloseReason, CreatedAt: mg.session.CreatedAt} + m.mu.Unlock() + for _, fn := range subs { + fn(ev) + } + return true +} + +// CloseAllForAccount closes an account's non-closed sessions (returns count). func (m *Manager) CloseAllForAccount(accountID string, reason CloseReason) int { m.mu.Lock() ids := make([]string, 0) for id, mg := range m.terminals { - if mg.session.AccountID == accountID { + if mg.session.AccountID == accountID && mg.session.Status != StatusClosed { ids = append(ids, id) } } @@ -352,6 +401,28 @@ func (m *Manager) CloseAllForAccount(accountID string, reason CloseReason) int { return len(ids) } +// RemoveForAccount drops an account's retained post-mortem (closed/errored) +// sessions from the registry and releases their buffers. Account deletion +// must not leave a deleted account's terminal output readable in memory — +// CloseAllForAccount skips already-closed sessions, so the H1 teardown calls +// this afterwards. Returns the number removed. +func (m *Manager) RemoveForAccount(accountID string) int { + m.mu.Lock() + defer m.mu.Unlock() + n := 0 + for id, mg := range m.terminals { + if mg.session.AccountID != accountID { + continue + } + if mg.session.Status == StatusClosed || mg.session.Status == StatusError { + mg.output.Clear() + delete(m.terminals, id) + n++ + } + } + return n +} + // Destroy closes all sessions (shutdown). func (m *Manager) Destroy() { m.mu.Lock() @@ -376,8 +447,9 @@ func (m *Manager) get(sessionID string) (*managed, error) { } // setStatus transitions a session and fires guaranteed status hooks. Terminal -// states remove the session from the registry (after the hook, so subscribers -// see the final transition). +// transitions do NOT remove the session — transport-driven closes/errors keep +// it registered (with its buffer) so the final output stays readable; only an +// explicit Close() removes it (setStatus in terminal-manager.ts, M6). func (m *Manager) setStatus(mg *managed, status Status, reason CloseReason) { m.mu.Lock() prev := mg.session.Status @@ -389,11 +461,6 @@ func (m *Manager) setStatus(mg *managed, status Status, reason CloseReason) { if reason != "" && mg.session.CloseReason == "" { mg.session.CloseReason = reason } - terminal := status == StatusClosed || status == StatusError - if terminal { - delete(m.terminals, mg.session.SessionID) - mg.output.Clear() - } subs := make([]func(StatusEvent), 0, len(m.statusSubs)) for _, fn := range m.statusSubs { subs = append(subs, fn) diff --git a/internal/terminal/manager_test.go b/internal/terminal/manager_test.go index 1a0d76f..e91ad4b 100644 --- a/internal/terminal/manager_test.go +++ b/internal/terminal/manager_test.go @@ -91,3 +91,99 @@ func TestManagerSessionLimitHelper(t *testing.T) { t.Error("account scoping leaked") } } + +// M6 (Node parity): a transport-driven close retains the session — buffer +// readable, hidden from lists — while an explicit Close removes it; errored +// sessions stay visible in lists. +func TestTerminalStateRetention(t *testing.T) { + mgr, mock := mockManager(t) + ep := EndpointRef{ID: "e1", AccountID: "acc", Host: "h", Port: 22, Username: "u"} + sess, err := mgr.Create(context.Background(), ep, "acc", Trigger{Kind: SourceUI}) + if err != nil { + t.Fatal(err) + } + if err := mgr.SendInput(sess.SessionID, "hello"); err != nil { + t.Fatal(err) + } + waitFor(t, func() bool { r, _ := mgr.ReadOutput(sess.SessionID, 0, 100); return len(r.Data) > 0 }) + + // Server hangup: transport dies underneath the manager. + _ = mock.Close() + waitFor(t, func() bool { + s := mgr.GetSession(sess.SessionID) + return s != nil && s.Status == StatusClosed + }) + + // Retained: post-mortem output stays readable, but lists hide it. + if r, err := mgr.ReadOutput(sess.SessionID, 0, 100); err != nil || string(r.Data) != "hello" { + t.Fatalf("post-mortem read: %q err=%v", r.Data, err) + } + if got := mgr.ListSessions(); len(got) != 0 { + t.Fatalf("closed session listed: %+v", got) + } + if got := mgr.ListForAccount("acc"); len(got) != 0 { + t.Fatalf("closed session in account list: %+v", got) + } + // Explicit Close on an already-closed session is a no-op (Node close() + // early-return): the post-mortem record survives. + mgr.Close(sess.SessionID, CloseClientUI) + if mgr.GetSession(sess.SessionID) == nil { + t.Fatal("post-mortem session dropped by no-op Close") + } + + // Errored sessions stay VISIBLE in lists (status filter is closed-only). + mock2 := NewMockTransport() + mgr2 := NewManager(func(context.Context, FactoryParams) (Transport, error) { return mock2, nil }, clock.Real{}, 0) + sess2, err := mgr2.Create(context.Background(), ep, "acc", Trigger{Kind: SourceUI}) + if err != nil { + t.Fatal(err) + } + mock2.events <- Event{Err: context.DeadlineExceeded} + waitFor(t, func() bool { + s := mgr2.GetSession(sess2.SessionID) + return s != nil && s.Status == StatusError + }) + got := mgr2.ListSessions() + if len(got) != 1 || got[0].Status != StatusError { + t.Fatalf("errored session not listed: %+v", got) + } +} + +// An explicit Close removes the session entirely (registry + buffer). +func TestExplicitCloseRemovesSession(t *testing.T) { + mgr, _ := mockManager(t) + ep := EndpointRef{ID: "e1", AccountID: "acc", Host: "h", Port: 22, Username: "u"} + sess, err := mgr.Create(context.Background(), ep, "acc", Trigger{Kind: SourceUI}) + if err != nil { + t.Fatal(err) + } + mgr.Close(sess.SessionID, CloseClientUI) + if mgr.GetSession(sess.SessionID) != nil { + t.Fatal("explicitly closed session still in registry") + } +} + +// Account deletion must purge retained post-mortem sessions (their buffers +// hold the deleted account's terminal output). +func TestRemoveForAccountPurgesRetainedSessions(t *testing.T) { + mgr, mock := mockManager(t) + ep := EndpointRef{ID: "e1", AccountID: "acc", Host: "h", Port: 22, Username: "u"} + sess, err := mgr.Create(context.Background(), ep, "acc", Trigger{Kind: SourceUI}) + if err != nil { + t.Fatal(err) + } + _ = mock.Close() // server hangup -> retained post-mortem + waitFor(t, func() bool { + s := mgr.GetSession(sess.SessionID) + return s != nil && s.Status == StatusClosed + }) + if n := mgr.CloseAllForAccount("acc", CloseAccountDeleted); n != 0 { + t.Fatalf("CloseAllForAccount touched retained session: %d", n) + } + if n := mgr.RemoveForAccount("acc"); n != 1 { + t.Fatalf("RemoveForAccount: got %d, want 1", n) + } + if mgr.GetSession(sess.SessionID) != nil { + t.Fatal("retained session survived account purge") + } +}