diff --git a/api/pkg/org/application/lifecycle/lifecycle.go b/api/pkg/org/application/lifecycle/lifecycle.go index d100eb690c..6091f40133 100644 --- a/api/pkg/org/application/lifecycle/lifecycle.go +++ b/api/pkg/org/application/lifecycle/lifecycle.go @@ -51,6 +51,10 @@ type Service struct { // collapses an ex-manager's team Stream when its last report just // left. nil is a no-op (tests without topology wiring). Topology *topology.Reconciler + + // Mirror is the transcript mirror; Fire stops the fired Worker's + // subscription so it doesn't leak. nil is a no-op. + Mirror *helix.Mirror } // ErrOwnerProtected is returned by Fire when the caller targets the @@ -116,6 +120,10 @@ func (s *Service) Fire(ctx context.Context, orgID string, id orgchart.WorkerID) state, _ := helix.LoadState(ctx, s.Store, orgID, id) + if s.Mirror != nil { + s.Mirror.Stop(id) + } + if s.Helix != nil && state.ProjectID != "" { if err := s.Helix.DeleteProject(ctx, state.ProjectID); err != nil && !errors.Is(err, helix.ErrProjectNotFound) { s.logger().Warn("fire: delete helix project", "worker", id, "project", state.ProjectID, "err", err) diff --git a/api/pkg/org/infrastructure/runtime/helix/mirror.go b/api/pkg/org/infrastructure/runtime/helix/mirror.go new file mode 100644 index 0000000000..163a522a2a --- /dev/null +++ b/api/pkg/org/infrastructure/runtime/helix/mirror.go @@ -0,0 +1,241 @@ +package helix + +import ( + "context" + "log/slog" + "sync" + "time" + + "github.com/helixml/helix/api/pkg/org/application/agent" + "github.com/helixml/helix/api/pkg/org/application/streamhub" + "github.com/helixml/helix/api/pkg/org/domain/orgchart" + "github.com/helixml/helix/api/pkg/org/domain/store" + "github.com/helixml/helix/api/pkg/pubsub" + "github.com/helixml/helix/api/pkg/types" +) + +const defaultMirrorPoll = 5 * time.Second + +// MirrorConfig wires the session-layer transcript Mirror. +type MirrorConfig struct { + PubSub pubsub.PubSub + Snapshotter SessionPreamble + // Client resolves the session owner so we subscribe to the right + // GetSessionQueue(owner, session) topic. + Client SpawnerClient + // ExploratorySession returns a project's current exploratory session + // (the one the inline chat / live UI follow). The mirror polls it to + // track the worker as its session churns. "" means no session yet. + ExploratorySession func(ctx context.Context, projectID string) (string, error) + Store *store.Store + Hub *streamhub.Hub + NewID func() string + Now func() time.Time + Logger *slog.Logger + PollInterval time.Duration // <=0 uses defaultMirrorPoll; seam for tests +} + +// Mirror is the single writer of worker transcript segments: it keeps +// one subscription per tracked worker pointed at that worker's current +// session and republishes every settled entry (plus the user prompt) +// onto s-activations-. Every turn — spawner activation, inline +// chat, anything on /sessions/chat — flows through the session topic, so +// one subscriber captures them all. +// +// A worker's session is not stable (stale resume opens a fresh one; +// inline chat can land on a newer one than the spawner persisted), so we +// track the worker — whose project is stable — and poll its current +// exploratory session, re-pointing when it changes. +type Mirror struct { + base context.Context + cfg MirrorConfig + + mu sync.Mutex + tracked map[orgchart.WorkerID]context.CancelFunc +} + +// NewMirror constructs a Mirror. base bounds every tracker (typically +// the server lifetime). A nil PubSub disables the mirror. +func NewMirror(base context.Context, cfg MirrorConfig) *Mirror { + if base == nil { + base = context.Background() + } + return &Mirror{ + base: base, + cfg: cfg, + tracked: map[orgchart.WorkerID]context.CancelFunc{}, + } +} + +func (m *Mirror) pollInterval() time.Duration { + if m.cfg.PollInterval > 0 { + return m.cfg.PollInterval + } + return defaultMirrorPoll +} + +// Ensure starts tracking a worker (idempotent). The session is resolved +// by the tracker, not passed in — the caller's notion of "the session" +// is exactly what goes stale. +func (m *Mirror) Ensure(orgID string, workerID orgchart.WorkerID) { + if m == nil || m.cfg.PubSub == nil { + return + } + m.mu.Lock() + if _, ok := m.tracked[workerID]; ok { + m.mu.Unlock() + return + } + ctx, cancel := context.WithCancel(m.base) + m.tracked[workerID] = cancel + m.mu.Unlock() + go m.track(ctx, orgID, workerID) +} + +// EnsureAll tracks every worker in an org — called from bootstrap so +// pre-existing / inline-chat-only workers are mirrored without an +// activation first. +func (m *Mirror) EnsureAll(ctx context.Context, orgID string) { + if m == nil || m.cfg.PubSub == nil || m.cfg.Store == nil { + return + } + workers, err := m.cfg.Store.Workers.List(ctx, orgID) + if err != nil { + if m.cfg.Logger != nil { + m.cfg.Logger.Warn("helix mirror: list workers for sweep", "org", orgID, "err", err) + } + return + } + for _, w := range workers { + m.Ensure(orgID, w.ID()) + } +} + +// Stop stops tracking a worker (on fire). +func (m *Mirror) Stop(workerID orgchart.WorkerID) { + if m == nil { + return + } + m.mu.Lock() + defer m.mu.Unlock() + if cancel, ok := m.tracked[workerID]; ok { + cancel() + delete(m.tracked, workerID) + } +} + +// track resolves the worker's current session and (re)subscribes +// whenever it changes, polling on an interval, until ctx fires. Each +// session gets a fresh bridge. +func (m *Mirror) track(ctx context.Context, orgID string, workerID orgchart.WorkerID) { + var ( + curSession string + curCancel = func() {} + ) + defer func() { curCancel() }() + + repoint := func() { + desired := m.resolveSession(ctx, orgID, workerID) + if desired == "" || desired == curSession { + return + } + curCancel() // old pump flushes pending entries on ctx.Done + subCtx, cancel := context.WithCancel(ctx) + curCancel = cancel + curSession = desired + m.subscribe(subCtx, orgID, workerID, desired) + if m.cfg.Logger != nil { + m.cfg.Logger.Info("helix mirror: pointed at session", "worker", workerID, "session", desired) + } + } + + repoint() // immediately, don't wait for the first tick + ticker := time.NewTicker(m.pollInterval()) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + repoint() + } + } +} + +// resolveSession returns the worker's current exploratory session, +// falling back to the persisted pointer when that lookup is unavailable. +func (m *Mirror) resolveSession(ctx context.Context, orgID string, workerID orgchart.WorkerID) string { + state, err := LoadState(ctx, m.cfg.Store, orgID, workerID) + if err != nil { + return "" + } + if state.ProjectID != "" && m.cfg.ExploratorySession != nil { + if sid, err := m.cfg.ExploratorySession(ctx, state.ProjectID); err == nil && sid != "" { + return sid + } else if err != nil && m.cfg.Logger != nil { + m.cfg.Logger.Warn("helix mirror: resolve exploratory session", "worker", workerID, "project", state.ProjectID, "err", err) + } + } + return state.SessionID +} + +// subscribe attaches a bridge to one session's topic and pumps its +// frames onto the activation stream until ctx fires. The first subscribe +// is synchronous so a frame published right after isn't raced. +func (m *Mirror) subscribe(ctx context.Context, orgID string, workerID orgchart.WorkerID, sessionID string) { + ownerID := "" + if m.cfg.Client != nil { + if owner, err := m.cfg.Client.SessionOwner(ctx, sessionID); err != nil { + if m.cfg.Logger != nil { + m.cfg.Logger.Warn("helix mirror: resolve session owner", "worker", workerID, "session", sessionID, "err", err) + } + } else { + ownerID = owner + } + } + publish := func(body string) { + if body == "" { + return + } + _, _ = agent.PublishActivationEvent(ctx, m.cfg.Store, m.cfg.Hub, m.cfg.NewID, m.cfg.Now, m.cfg.Logger, orgID, workerID, body) + } + b := newBridge(publish) + ch, err := SubscribeSessionUpdates(ctx, m.cfg.PubSub, m.cfg.Snapshotter, ownerID, sessionID) + if err != nil { + if m.cfg.Logger != nil { + m.cfg.Logger.Warn("helix mirror: subscribe", "worker", workerID, "session", sessionID, "err", err) + } + ch = nil + } + go m.pump(ctx, b, ownerID, sessionID, ch) +} + +// pump drains the subscription channel into the bridge and reconnects +// with capped backoff until ctx fires. +func (m *Mirror) pump(ctx context.Context, b *bridge, ownerID, sessionID string, ch <-chan types.WebsocketEvent) { + delay := time.Second + for { + if ch != nil { + for u := range ch { + b.apply(u) + } + } + select { + case <-ctx.Done(): + b.stream.Flush() + return + case <-time.After(delay): + } + if delay < 30*time.Second { + delay *= 2 + } + var err error + ch, err = SubscribeSessionUpdates(ctx, m.cfg.PubSub, m.cfg.Snapshotter, ownerID, sessionID) + if err != nil { + if m.cfg.Logger != nil { + m.cfg.Logger.Warn("helix mirror: re-subscribe", "session", sessionID, "err", err) + } + ch = nil + } + } +} diff --git a/api/pkg/org/infrastructure/runtime/helix/mirror_test.go b/api/pkg/org/infrastructure/runtime/helix/mirror_test.go new file mode 100644 index 0000000000..f53c4fefb4 --- /dev/null +++ b/api/pkg/org/infrastructure/runtime/helix/mirror_test.go @@ -0,0 +1,235 @@ +package helix + +import ( + "context" + "encoding/json" + "fmt" + "io" + "log/slog" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/helixml/helix/api/pkg/org/domain/activation" + "github.com/helixml/helix/api/pkg/org/domain/orgchart" + "github.com/helixml/helix/api/pkg/org/domain/store" + "github.com/helixml/helix/api/pkg/pubsub" + "github.com/helixml/helix/api/pkg/types" +) + +// newTestMirror builds a Mirror with a fast poll interval. Drive session +// resolution via fc.setExploratory(id); the worker needs a persisted +// project (SaveProject) so the mirror resolves via ExploratorySession. +func newTestMirror(t *testing.T, s *store.Store, ps *fakePubSub, owner string) (*Mirror, *fakeHelixClient) { + t.Helper() + fc := &fakeHelixClient{sessionOwner: owner} + var idCounter int32 + m := NewMirror(context.Background(), MirrorConfig{ + PubSub: ps, + Snapshotter: NoopSessionPreamble{}, + Client: fc, + ExploratorySession: fc.ExploratorySession, + Store: s, + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + NewID: func() string { return fmt.Sprintf("e-%d", atomic.AddInt32(&idCounter, 1)) }, + Now: func() time.Time { return time.Now().UTC() }, + PollInterval: 15 * time.Millisecond, + }) + return m, fc +} + +func waitForSegment(t *testing.T, s *store.Store, wid orgchart.WorkerID, want string) bool { + t.Helper() + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + events, err := s.Events.ListForStream(context.Background(), "org-test", activation.StreamID(wid), 200) + if err != nil { + t.Fatalf("list events: %v", err) + } + for _, e := range events { + if msg, err := e.Message(); err == nil && strings.Contains(msg.Body, want) { + return true + } + } + time.Sleep(10 * time.Millisecond) + } + return false +} + +func waitForHandlers(ps *fakePubSub, topic string, want int) bool { + deadline := time.Now().Add(time.Second) + for time.Now().Before(deadline) { + if ps.handlerCount(topic) == want { + return true + } + time.Sleep(5 * time.Millisecond) + } + return ps.handlerCount(topic) == want +} + +// A frame on the worker's session topic, with no spawner, is mirrored +// onto s-activations- (the inline-chat regression). +func TestMirrorCapturesTurnWithoutSpawner(t *testing.T) { + t.Parallel() + s, wid := newHelixTestStore(t) + if err := SaveProject(context.Background(), s, "org-test", wid, "prj_x", "app_x", "repo_x"); err != nil { + t.Fatalf("save project: %v", err) + } + ps := newFakePubSub() + m, fc := newTestMirror(t, s, ps, "u-owner") + fc.setExploratory("ses_inline") + m.Ensure("org-test", wid) + + topic := pubsub.GetSessionQueue("u-owner", "ses_inline") + if !waitForHandlers(ps, topic, 1) { + t.Fatal("mirror never subscribed to the resolved session") + } + patch, _ := json.Marshal(types.WebsocketEvent{EntryPatches: []types.EntryPatch{ + {Index: 0, MessageID: "m1", Type: "text", Patch: "hello from inline chat"}, + }}) + ps.publish(t, topic, patch) + complete, _ := json.Marshal(types.WebsocketEvent{Interaction: &types.Interaction{State: "complete"}}) + ps.publish(t, topic, complete) + + if !waitForSegment(t, s, wid, "assistant: hello from inline chat") { + t.Fatal("inline-chat turn never reached the activation stream") + } +} + +// Core fix: when the worker's session changes, the mirror drops the old +// subscription and follows the new one instead of going silent. +func TestMirrorRepointsOnSessionChurn(t *testing.T) { + t.Parallel() + s, wid := newHelixTestStore(t) + if err := SaveProject(context.Background(), s, "org-test", wid, "prj_x", "app_x", "repo_x"); err != nil { + t.Fatalf("save project: %v", err) + } + ps := newFakePubSub() + m, fc := newTestMirror(t, s, ps, "u-owner") + + fc.setExploratory("ses_old") + m.Ensure("org-test", wid) + oldTopic := pubsub.GetSessionQueue("u-owner", "ses_old") + if !waitForHandlers(ps, oldTopic, 1) { + t.Fatal("mirror never subscribed to ses_old") + } + + // The worker's session churns to ses_new. + fc.setExploratory("ses_new") + newTopic := pubsub.GetSessionQueue("u-owner", "ses_new") + if !waitForHandlers(ps, newTopic, 1) { + t.Fatal("mirror did not re-point to ses_new after churn") + } + if !waitForHandlers(ps, oldTopic, 0) { + t.Fatal("mirror did not drop the old (ses_old) subscription on re-point") + } + + // A turn on the NEW session is captured. + patch, _ := json.Marshal(types.WebsocketEvent{EntryPatches: []types.EntryPatch{ + {Index: 0, MessageID: "m1", Type: "text", Patch: "on the new session"}, + }}) + ps.publish(t, newTopic, patch) + complete, _ := json.Marshal(types.WebsocketEvent{Interaction: &types.Interaction{State: "complete"}}) + ps.publish(t, newTopic, complete) + if !waitForSegment(t, s, wid, "assistant: on the new session") { + t.Fatal("turn on the churned-to session not captured") + } +} + +// The mirror records the prompt as a `user:` segment, once per +// interaction, alongside the agent's reply. +func TestMirrorCapturesUserPrompt(t *testing.T) { + t.Parallel() + s, wid := newHelixTestStore(t) + if err := SaveProject(context.Background(), s, "org-test", wid, "prj_x", "app_x", "repo_x"); err != nil { + t.Fatalf("save project: %v", err) + } + ps := newFakePubSub() + m, fc := newTestMirror(t, s, ps, "u-owner") + fc.setExploratory("ses_p") + m.Ensure("org-test", wid) + topic := pubsub.GetSessionQueue("u-owner", "ses_p") + if !waitForHandlers(ps, topic, 1) { + t.Fatal("mirror never subscribed") + } + + iu, _ := json.Marshal(types.WebsocketEvent{ + Type: types.WebsocketEventInteractionUpdate, + Interaction: &types.Interaction{ID: "int_1", PromptMessage: "what is 2+2?"}, + }) + ps.publish(t, topic, iu) + ps.publish(t, topic, iu) // duplicate — must not double-emit + patch, _ := json.Marshal(types.WebsocketEvent{EntryPatches: []types.EntryPatch{ + {Index: 0, MessageID: "m1", Type: "text", Patch: "4"}, + }}) + ps.publish(t, topic, patch) + complete, _ := json.Marshal(types.WebsocketEvent{ + Interaction: &types.Interaction{ID: "int_1", PromptMessage: "what is 2+2?", State: "complete"}, + }) + ps.publish(t, topic, complete) + + if !waitForSegment(t, s, wid, "user: what is 2+2?") { + t.Fatal("user prompt not captured") + } + if !waitForSegment(t, s, wid, "assistant: 4") { + t.Fatal("assistant reply not captured") + } + events, _ := s.Events.ListForStream(context.Background(), "org-test", activation.StreamID(wid), 200) + userLines := 0 + for _, e := range events { + if msg, err := e.Message(); err == nil && strings.HasPrefix(msg.Body, "user: ") { + userLines++ + } + } + if userLines != 1 { + t.Fatalf("user lines = %d, want exactly 1 (dedup per interaction)", userLines) + } +} + +// TestMirrorEnsureIsIdempotent: Ensure twice for the same worker must +// not stack duplicate trackers/subscriptions. +func TestMirrorEnsureIsIdempotent(t *testing.T) { + t.Parallel() + s, wid := newHelixTestStore(t) + if err := SaveProject(context.Background(), s, "org-test", wid, "prj_x", "app_x", "repo_x"); err != nil { + t.Fatalf("save project: %v", err) + } + ps := newFakePubSub() + m, fc := newTestMirror(t, s, ps, "u-owner") + fc.setExploratory("ses_x") + m.Ensure("org-test", wid) + m.Ensure("org-test", wid) + m.Ensure("org-test", wid) + + topic := pubsub.GetSessionQueue("u-owner", "ses_x") + if !waitForHandlers(ps, topic, 1) { + t.Fatalf("handlerCount = %d, want 1 (Ensure must not stack subscriptions)", ps.handlerCount(topic)) + } + // Give any erroneously-spawned extra trackers a chance to subscribe. + time.Sleep(60 * time.Millisecond) + if got := ps.handlerCount(topic); got != 1 { + t.Fatalf("handlerCount = %d, want 1", got) + } +} + +// TestMirrorStop tears down the tracker + subscription. +func TestMirrorStop(t *testing.T) { + t.Parallel() + s, wid := newHelixTestStore(t) + if err := SaveProject(context.Background(), s, "org-test", wid, "prj_x", "app_x", "repo_x"); err != nil { + t.Fatalf("save project: %v", err) + } + ps := newFakePubSub() + m, fc := newTestMirror(t, s, ps, "u-owner") + fc.setExploratory("ses_x") + m.Ensure("org-test", wid) + topic := pubsub.GetSessionQueue("u-owner", "ses_x") + if !waitForHandlers(ps, topic, 1) { + t.Fatal("mirror never subscribed") + } + m.Stop(wid) + if !waitForHandlers(ps, topic, 0) { + t.Fatal("Stop did not drop the subscription") + } +} diff --git a/api/pkg/org/infrastructure/runtime/helix/sessions.go b/api/pkg/org/infrastructure/runtime/helix/sessions.go index dbf02d93b7..1d44ab0199 100644 --- a/api/pkg/org/infrastructure/runtime/helix/sessions.go +++ b/api/pkg/org/infrastructure/runtime/helix/sessions.go @@ -3,7 +3,6 @@ package helix import ( "context" "encoding/json" - "errors" "fmt" "github.com/helixml/helix/api/pkg/types" @@ -11,20 +10,35 @@ import ( "github.com/helixml/helix/api/pkg/pubsub" ) -// SessionClient is the small slice of the chat-session API -// EnsureAndSend depends on. Production impl is the in-process adapter -// at api/pkg/server/helix_org_inproc.go::inProcHelixClient, which -// routes calls to HelixAPIServer handler methods directly. +// SessionClient is the slice of the session API EnsureAndSend depends +// on, backed by the same primitives the cron trigger / spec tasks use: +// - StartSession → StartExternalAgentSession (create session + start +// desktop + queue first message). +// - SendMessage → POST /sessions/{id}/messages (fire-and-forget +// continuation; Helix auto-resumes a downed desktop on the same +// session). // -// hadStreamErr semantics: the SSE-error path is a workaround for -// streaming-error chunks the chat handler may emit mid-stream. The -// retry at line 130 below treats it as transient; safe to keep — -// it's a one-shot, idempotent retry. +// Neither blocks on the turn, so neither hits the external-agent response +// timeout; the Spawner observes completion via pollUntilDone + the mirror. type SessionClient interface { - StartChatWithStatus(ctx context.Context, req StartChatRequest) (types.Session, bool, error) + StartSession(ctx context.Context, params StartSessionParams) (sessionID string, err error) + SendMessage(ctx context.Context, sessionID, prompt string) error ServerStatus(ctx context.Context) (ServerStatus, error) } +// StartSessionParams configures one-time creation of a worker's session. +// The adapter always sets session_role "exploratory" so it's resolvable +// by the mirror's GetProjectExploratorySession lookup. +type StartSessionParams struct { + ProjectID string + OrganizationID string + AppID string + AgentType string + Provider string + Model string + Prompt string +} + // SpawnerClient is the chat-session surface the helix Spawner uses // during an activation. Superset of SessionClient: // @@ -44,23 +58,6 @@ type SpawnerClient interface { SessionOwner(ctx context.Context, sessionID string) (string, error) } -// sendToSession pushes a message to an existing session via -// /sessions/chat with SessionID set. Returns an error if the session -// is no longer running (Helix reports streamHadErr=true). -func sendToSession(ctx context.Context, client SessionClient, req StartChatRequest) (types.Session, error) { - if req.SessionID == "" { - return types.Session{}, errors.New("sendToSession: SessionID required") - } - session, streamHadErr, err := client.StartChatWithStatus(ctx, req) - if err != nil { - return types.Session{}, err - } - if streamHadErr { - return types.Session{}, errors.New("session no longer running on the server") - } - return session, nil -} - // checkDesktopQuota pre-flights the desktop quota gate before // opening a new session. func checkDesktopQuota(ctx context.Context, client SessionClient) error { @@ -106,36 +103,12 @@ type SendPromptParams struct { OnSessionID func(sessionID string) } -// EnsureAndSend is the single primitive for "make this Helix session -// run this prompt." Both the owner-chat bridge and the worker -// activation Spawner call it — same request shape, same resume-or-fresh -// recovery, same session_role ("exploratory" — so every session is -// visible via Helix's per-project desktop view). Without one shared -// primitive these two paths drift and behave differently against -// stale state, broken WS connections, etc. -// -// Behaviour: -// -// 1. If params.SessionID is set: try to resume via SendToSession. -// Success → invoke OnSessionID(SessionID) and return. -// Any failure (HTTP error or SSE error chunk after the ID echo) → -// log nothing here (caller decides) and fall through to step 2. -// 2. Pre-flight the desktop quota: a fresh session always boots a -// Zed sandbox, and Helix fails late if the quota is exhausted. -// 3. Open a new session via StartChatWithStatus. OnSessionID is -// wired into the StartChatRequest so it fires the moment Helix -// emits the session ID — before the agent has produced anything, -// so callers can attach a WS subscriber early. -// -// Returns the active session ID and a bool indicating whether step 1 -// (resume) succeeded. fresh=true means a new session was opened and -// the caller should persist the returned ID. -// -// session_role is fixed at "exploratory" so the new session is -// discoverable from Helix's per-project UI (the project handlers -// query store.GetProjectExploratorySession, which filters on this -// role). Worker activations and owner chats both go through this -// path, so neither is special-cased in Helix. +// EnsureAndSend makes a worker's session run a prompt. A worker has one +// durable session, so there's no staleness to detect: +// - existing session → SendMessage (fire-and-forget; Helix recovers a +// downed desktop on the same session). fresh=false. +// - no session → quota pre-flight, then StartSession. fresh=true so the +// caller persists the new id. const exploratoryRole = "exploratory" func EnsureAndSend(ctx context.Context, client SessionClient, params SendPromptParams) (sessionID string, fresh bool, err error) { @@ -149,74 +122,35 @@ func EnsureAndSend(ctx context.Context, client SessionClient, params SendPromptP return "", false, fmt.Errorf("EnsureAndSend: AgentType is required") } - // Step 1 — try to resume. if params.SessionID != "" { - resumeReq := StartChatRequest{ - SessionID: params.SessionID, - ProjectID: params.ProjectID, - OrganizationID: params.OrganizationID, - AppID: params.AppID, - SessionRole: exploratoryRole, - AgentType: params.AgentType, - Type: "text", - ExternalAgentConfig: &types.ExternalAgentConfig{}, - Messages: []SessionChatMessage{NewTextMessage("user", params.Prompt)}, + if err := client.SendMessage(ctx, params.SessionID, params.Prompt); err != nil { + return "", false, fmt.Errorf("send message to session %s: %w", params.SessionID, err) } - if _, sendErr := sendToSession(ctx, client, resumeReq); sendErr == nil { - if params.OnSessionID != nil { - params.OnSessionID(params.SessionID) - } - return params.SessionID, false, nil + if params.OnSessionID != nil { + params.OnSessionID(params.SessionID) } - // Resume failed — caller will see fresh=true and can take - // the opportunity to log + persist the new ID. + return params.SessionID, false, nil } - // Step 2 — pre-flight quota. if err := checkDesktopQuota(ctx, client); err != nil { return "", false, err } - - // Step 3 — open fresh. - startReq := StartChatRequest{ - ProjectID: params.ProjectID, - OrganizationID: params.OrganizationID, - AppID: params.AppID, - SessionRole: exploratoryRole, - AgentType: params.AgentType, - Type: "text", - Provider: params.Provider, - Model: params.Model, - ExternalAgentConfig: &types.ExternalAgentConfig{}, - Messages: []SessionChatMessage{NewTextMessage("user", params.Prompt)}, - OnSessionID: params.OnSessionID, - } - session, hadStreamErr, err := client.StartChatWithStatus(ctx, startReq) + sid, err := client.StartSession(ctx, StartSessionParams{ + ProjectID: params.ProjectID, + OrganizationID: params.OrganizationID, + AppID: params.AppID, + AgentType: params.AgentType, + Provider: params.Provider, + Model: params.Model, + Prompt: params.Prompt, + }) if err != nil { - return "", false, fmt.Errorf("open fresh helix session: %w", err) + return "", false, fmt.Errorf("start helix session: %w", err) } - - // Step 4 — cold-start fallback. With Helix's per-session readiness - // check (waitForExternalAgentReady now polls the agent's own WS - // rather than the global connection list), the first dispatch - // should land cleanly. If hadStreamErr is still set we re-issue - // once on the same session via SessionID continuation. Belt-and- - // braces — the original race that made this critical has been - // fixed at the source, so this should be rare. - if hadStreamErr { - retryReq := StartChatRequest{ - SessionID: session.ID, - ProjectID: params.ProjectID, - AppID: params.AppID, - SessionRole: exploratoryRole, - AgentType: params.AgentType, - Type: "text", - ExternalAgentConfig: &types.ExternalAgentConfig{}, - Messages: []SessionChatMessage{NewTextMessage("user", params.Prompt)}, - } - _, _, _ = client.StartChatWithStatus(ctx, retryReq) + if params.OnSessionID != nil { + params.OnSessionID(sid) } - return session.ID, true, nil + return sid, true, nil } // SessionPreamble exposes the late-joiner catch-up snapshot the diff --git a/api/pkg/org/infrastructure/runtime/helix/spawner.go b/api/pkg/org/infrastructure/runtime/helix/spawner.go index 5c4ea83379..f225e00c57 100644 --- a/api/pkg/org/infrastructure/runtime/helix/spawner.go +++ b/api/pkg/org/infrastructure/runtime/helix/spawner.go @@ -15,6 +15,7 @@ import ( "github.com/helixml/helix/api/pkg/org/domain/streaming" "github.com/helixml/helix/api/pkg/org/infrastructure/runtime" "github.com/helixml/helix/api/pkg/pubsub" + "github.com/helixml/helix/api/pkg/types" ) // SpawnerConfig wires the helix-backed Spawner. The Client is @@ -33,6 +34,9 @@ type SpawnerConfig struct { // per-session WebsocketEvent frames to in-process subscribers. PubSub pubsub.PubSub Snapshotter SessionPreamble + // Mirror is the transcript writer; the spawner Ensure()s it per + // activation. nil disables mirroring (tests / app-only wirings). + Mirror *Mirror HelixOrgURL string // forwarded to project secrets so the in-sandbox agent can reach helix-org's MCP server // Runtime overrides the default `zed_agent` runtime. Empty falls // back to helix.Runtime. See WorkerProject.Runtime for the @@ -242,22 +246,20 @@ func Spawner(cfg SpawnerConfig) runtime.Spawner { } } + // Register the worker with the transcript mirror (idempotent; + // the tracker persists across activations and follows the + // session as it churns). The spawner no longer owns a bridge. + if cfg.Mirror != nil { + cfg.Mirror.Ensure(orgID, workerID) + } + sessionID, err := cfg.ensureSession(actCtx, orgID, workerID, prompt, publish) if err != nil { publish(activation.OutcomeFromError(err).Marker()) return err } - // Live transcript bridge. On disconnect the spawner reconnects - // for the lifetime of the activation; the dedup map prevents - // republishing on snapshot replay. - bridge := newBridge(publish) - bridgeCtx, bridgeCancel := context.WithCancel(actCtx) - defer bridgeCancel() - go bridge.run(bridgeCtx, cfg, sessionID) - err = cfg.pollUntilDone(actCtx, sessionID, publish) - bridgeCancel() publish(activation.OutcomeFromError(err).Marker()) return err } @@ -489,14 +491,30 @@ func (c SpawnerConfig) pollUntilDone(ctx context.Context, sessionID string, publ type bridge struct { publish func(body string) stream *EntryStream + seenPrompts map[string]bool // interaction IDs whose user prompt we've emitted (dedup) } func newBridge(publish func(body string)) *bridge { - b := &bridge{publish: publish} + b := &bridge{publish: publish, seenPrompts: map[string]bool{}} b.stream = NewEntryStream(b.onEvent) return b } +// apply renders one session frame: it emits the user's prompt (once per +// interaction) then feeds the agent's entry patches through EntryStream, +// so the transcript is two-sided. Prompts come from the current +// interaction only (not u.Session history), so a restart doesn't re-emit +// past prompts. +func (b *bridge) apply(u types.WebsocketEvent) { + if in := u.Interaction; in != nil && in.ID != "" && !b.seenPrompts[in.ID] { + if body := in.PromptMessage; body != "" { + b.seenPrompts[in.ID] = true + b.publish(activation.TranscriptSegment{Kind: activation.SegmentUser, Body: body}.Marker()) + } + } + b.stream.Apply(u) +} + // onEvent renders one settled EntryStream event into the canonical // activation-transcript line shape. The owner-chat bridge in // server/chat uses the same TranscriptBody helper to publish @@ -544,51 +562,6 @@ func transcriptSegmentFromEvent(e Event) (activation.TranscriptSegment, bool) { return activation.TranscriptSegment{}, false } -func (b *bridge) run(ctx context.Context, cfg SpawnerConfig, sessionID string) { - // Resolve the session owner once. Helix publishes every session - // update (assistant text, tool_use, tool_result) to - // GetSessionQueue(session.Owner, sessionID); subscribing with an - // empty owner lands us on the wrong topic and the bridge receives - // zero frames — leaving only the spawner's own lifecycle markers on - // the activation stream. Owner never changes, so resolve it before - // the reconnect loop rather than on every reconnect. - ownerID := "" - if cfg.Client != nil { - if owner, err := cfg.Client.SessionOwner(ctx, sessionID); err != nil { - if cfg.Logger != nil { - cfg.Logger.Warn("helix transcript bridge: resolve session owner", "session", sessionID, "err", err) - } - } else { - ownerID = owner - } - } - - delay := time.Second - for { - ch, err := SubscribeSessionUpdates(ctx, cfg.PubSub, cfg.Snapshotter, ownerID, sessionID) - if err != nil { - if cfg.Logger != nil { - cfg.Logger.Warn("helix subscribe", "session", sessionID, "err", err) - } - } else { - for u := range ch { - b.stream.Apply(u) - } - } - // Reconnect with capped exponential backoff while the - // activation context is still live. - select { - case <-ctx.Done(): - b.stream.Flush() - return - case <-time.After(delay): - } - if delay < 30*time.Second { - delay *= 2 - } - } -} - // publishActivationEvent is a thin wrapper around the shared // agent.PublishActivationEvent so the helix spawner's call sites // stay terse. The owner-chat bridge uses the same shared helper diff --git a/api/pkg/org/infrastructure/runtime/helix/spawner_test.go b/api/pkg/org/infrastructure/runtime/helix/spawner_test.go index 57d11c3297..33ff296983 100644 --- a/api/pkg/org/infrastructure/runtime/helix/spawner_test.go +++ b/api/pkg/org/infrastructure/runtime/helix/spawner_test.go @@ -29,23 +29,36 @@ type fakeHelixClient struct { sendCalls int32 outputCalls int32 subscribeCalls int32 - startSessionID string - sessionOwner string // returned by SessionOwner; the transcript bridge subscribes to this owner's pubsub topic - startErr error - sendErr error - outputs []types.SessionOutputResponse - updatesFactory func() <-chan types.WebsocketEvent - lastStartReq StartChatRequest - lastSendSID string - lastSendBody string -} - -func (f *fakeHelixClient) StartChatWithStatus(_ context.Context, req StartChatRequest) (types.Session, bool, error) { + startSessionID string + sessionOwner string // returned by SessionOwner; the transcript bridge subscribes to this owner's pubsub topic + exploratorySession string // returned by ExploratorySession; the mirror polls this to track the worker's current session + startErr error + sendErr error + outputs []types.SessionOutputResponse + updatesFactory func() <-chan types.WebsocketEvent + lastStartParams StartSessionParams + lastSendSID string + lastSendBody string +} + +func (f *fakeHelixClient) StartSession(_ context.Context, params StartSessionParams) (string, error) { atomic.AddInt32(&f.startCalls, 1) f.mu.Lock() - f.lastStartReq = req + f.lastStartParams = params f.mu.Unlock() - return types.Session{ID: f.startSessionID}, false, f.startErr + if f.startErr != nil { + return "", f.startErr + } + return f.startSessionID, nil +} + +func (f *fakeHelixClient) SendMessage(_ context.Context, sessionID, prompt string) error { + atomic.AddInt32(&f.sendCalls, 1) + f.mu.Lock() + f.lastSendSID = sessionID + f.lastSendBody = prompt + f.mu.Unlock() + return f.sendErr } func (f *fakeHelixClient) GetOutput(_ context.Context, _ string) (types.SessionOutputResponse, error) { @@ -62,6 +75,20 @@ func (f *fakeHelixClient) StopExternalAgent(_ context.Context, _ string) error { func (f *fakeHelixClient) SessionOwner(_ context.Context, _ string) (string, error) { return f.sessionOwner, nil } + +// exploratorySession + setExploratory back the Mirror's session +// resolver. The mirror polls this to track the worker as its session +// changes; tests flip it to simulate session churn. +func (f *fakeHelixClient) setExploratory(sid string) { + f.mu.Lock() + f.exploratorySession = sid + f.mu.Unlock() +} +func (f *fakeHelixClient) ExploratorySession(_ context.Context, _ string) (string, error) { + f.mu.Lock() + defer f.mu.Unlock() + return f.exploratorySession, nil +} func (f *fakeHelixClient) ServerStatus(_ context.Context) (ServerStatus, error) { return ServerStatus{MaxConcurrentDesktops: 0, ActiveConcurrentDesktops: 0}, nil } @@ -131,13 +158,13 @@ func TestSpawnerStartsFreshAndPersistsSession(t *testing.T) { if state.ProjectID != "prj_test" || state.AgentAppID != "app_test" { t.Errorf("project IDs not persisted: project=%q agent_app=%q", state.ProjectID, state.AgentAppID) } - // StartChat must point at the per-Worker project, not at any + // StartSession must point at the per-Worker project, not at any // global one. - if fc.lastStartReq.ProjectID != "prj_test" { - t.Errorf("StartChat ProjectID = %q (want prj_test)", fc.lastStartReq.ProjectID) + if fc.lastStartParams.ProjectID != "prj_test" { + t.Errorf("StartSession ProjectID = %q (want prj_test)", fc.lastStartParams.ProjectID) } - if fc.lastStartReq.AppID != "app_test" { - t.Errorf("StartChat AppID = %q (want app_test)", fc.lastStartReq.AppID) + if fc.lastStartParams.AppID != "app_test" { + t.Errorf("StartSession AppID = %q (want app_test)", fc.lastStartParams.AppID) } } @@ -240,16 +267,20 @@ func TestSpawnerFollowUpResumesPersistedSession(t *testing.T) { if err := sp(context.Background(), "org-test", wid, "/ignored", []activation.Trigger{{Kind: activation.TriggerEvent, EventID: "e-1"}}); err != nil { t.Fatalf("spawn: %v", err) } - // The first StartChatWithStatus call (resume) carries the existing - // SessionID. The session pointer in the store must remain unchanged. + // A follow-up with a persisted session sends via SendMessage to the + // existing session — no fresh StartSession, no churn. The session + // pointer must remain unchanged. fc.mu.Lock() defer fc.mu.Unlock() - if fc.lastStartReq.SessionID != "ses_existing" { - t.Errorf("StartChatRequest.SessionID = %q (want ses_existing) — resume must target persisted session", fc.lastStartReq.SessionID) + if fc.lastSendSID != "ses_existing" { + t.Errorf("SendMessage sessionID = %q (want ses_existing) — follow-up must target persisted session", fc.lastSendSID) + } + if got := atomic.LoadInt32(&fc.startCalls); got != 0 { + t.Errorf("StartSession called %d times; a follow-up must reuse the session, not create a fresh one", got) } state, _ := LoadState(context.Background(), s, "org-test", wid) if state.SessionID != "ses_existing" { - t.Errorf("session pointer changed to %q; resume must NOT open a fresh session", state.SessionID) + t.Errorf("session pointer changed to %q; follow-up must NOT open a fresh session", state.SessionID) } } @@ -290,56 +321,6 @@ func (f *quotaFullFakeClient) ServerStatus(_ context.Context) (ServerStatus, err return ServerStatus{MaxConcurrentDesktops: 2, ActiveConcurrentDesktops: 2}, nil } -// TestSpawnerColdStartReQueues verifies that when StartChatWithStatus -// reports hadStreamErr=true on the fresh open, EnsureAndSend re-issues -// the same prompt against the same session ID (belt-and-braces — the -// original race that made this critical was fixed in Helix; this -// retry is the fallback). -func TestSpawnerColdStartReQueues(t *testing.T) { - t.Parallel() - s, wid := newHelixTestStore(t) - fc := &coldStartFakeClient{ - fakeHelixClient: fakeHelixClient{ - startSessionID: "ses_new", - outputs: []types.SessionOutputResponse{{Status: "complete", Output: "ok"}}, - }, - hadWSError: true, - } - cfg := newHelixCfg(t, &fc.fakeHelixClient, s) - cfg.Client = fc - sp := Spawner(cfg) - if err := sp(context.Background(), "org-test", wid, "/ignored", []activation.Trigger{{Kind: activation.TriggerHire}}); err != nil { - t.Fatalf("spawn: %v", err) - } - // Two StartChatWithStatus calls: the fresh open and the retry on - // the same session. (Cold-start retry replaced the older - // SendSessionMessage path in EnsureAndSend.) - if got := atomic.LoadInt32(&fc.startCalls); got < 2 { - t.Errorf("StartChat calls: %d (want >=2 — fresh open + cold-start retry)", got) - } - // Retry targets the freshly-opened session. - fc.mu.Lock() - defer fc.mu.Unlock() - if fc.lastStartReq.SessionID != "ses_new" { - t.Errorf("retry SessionID = %q (want ses_new — retry on same session)", fc.lastStartReq.SessionID) - } -} - -// coldStartFakeClient overrides StartChatWithStatus to return -// hadWSError=true, simulating Helix's "no agent WS yet" race. -type coldStartFakeClient struct { - fakeHelixClient - hadWSError bool -} - -func (f *coldStartFakeClient) StartChatWithStatus(_ context.Context, req StartChatRequest) (types.Session, bool, error) { - atomic.AddInt32(&f.startCalls, 1) - f.mu.Lock() - f.lastStartReq = req - f.mu.Unlock() - return types.Session{ID: f.startSessionID}, f.hadWSError, f.startErr -} - func TestSpawnerTimeoutEmitsExitError(t *testing.T) { t.Parallel() s, wid := newHelixTestStore(t) @@ -401,7 +382,10 @@ type concurrencyClient struct { peak *int32 } -func (c *concurrencyClient) StartChatWithStatus(ctx context.Context, req StartChatRequest) (types.Session, bool, error) { +// track records peak concurrency then blocks on the gate, so the test +// can hold multiple activations in-flight at once and assert the +// spawner's semaphore caps them. +func (c *concurrencyClient) track() func() { cur := atomic.AddInt32(c.inflight, 1) for { p := atomic.LoadInt32(c.peak) @@ -409,9 +393,18 @@ func (c *concurrencyClient) StartChatWithStatus(ctx context.Context, req StartCh break } } - defer atomic.AddInt32(c.inflight, -1) <-c.gate - return c.inner.StartChatWithStatus(ctx, req) + return func() { atomic.AddInt32(c.inflight, -1) } +} + +func (c *concurrencyClient) StartSession(ctx context.Context, params StartSessionParams) (string, error) { + defer c.track()() + return c.inner.StartSession(ctx, params) +} + +func (c *concurrencyClient) SendMessage(ctx context.Context, sessionID, prompt string) error { + defer c.track()() + return c.inner.SendMessage(ctx, sessionID, prompt) } func (c *concurrencyClient) ServerStatus(ctx context.Context) (ServerStatus, error) { @@ -430,54 +423,50 @@ func (c *concurrencyClient) SessionOwner(ctx context.Context, sid string) (strin return c.inner.SessionOwner(ctx, sid) } -// TestSpawnerPublishesTranscriptViaEntryStream verifies the bridge -// subscribes via pubsub.GetSessionQueue, feeds frames through -// EntryStream, and republishes settled events as activation Stream -// events. -func TestSpawnerPublishesTranscriptViaEntryStream(t *testing.T) { +// An activation Ensure()s the mirror, so a turn arriving on the session +// AFTER the activation returns (e.g. inline chat) is still captured. +func TestSpawnerEnsuresSessionMirror(t *testing.T) { t.Parallel() s, wid := newHelixTestStore(t) + // Steady state: project + session persisted, so the activation + // resumes ses_y and the spawner Ensures the mirror up front. + if err := SaveProject(context.Background(), s, "org-test", wid, "prj_test", "app_test", "repo_test"); err != nil { + t.Fatalf("save project: %v", err) + } + if err := SaveSession(context.Background(), s, "org-test", wid, "ses_y"); err != nil { + t.Fatalf("save session: %v", err) + } ps := newFakePubSub() fc := &fakeHelixClient{ - startSessionID: "ses_y", - // The session is owned by a real user; helix publishes its - // updates to that owner's pubsub topic. The bridge must resolve - // the owner (via SessionOwner) and subscribe there — subscribing - // with an empty owner is the regression this test guards. - sessionOwner: "u-owner", - // Several waiting outputs so the bridge has time to consume - // the pubsub frames before pollUntilDone terminates. - outputs: []types.SessionOutputResponse{ - {Status: "waiting"}, {Status: "waiting"}, {Status: "complete", Output: "ok"}, - }, + sessionOwner: "u-owner", + exploratorySession: "ses_y", + outputs: []types.SessionOutputResponse{{Status: "complete", Output: "ok"}}, } cfg := newHelixCfg(t, fc, s) cfg.PubSub = ps - cfg.PollInitial = 80 * time.Millisecond - cfg.ActivationTimeout = 2 * time.Second - // Unique IDs so each publishActivationEvent insert succeeds. var idCounter int32 cfg.NewID = func() string { return fmt.Sprintf("e-%d", atomic.AddInt32(&idCounter, 1)) } + cfg.Mirror = NewMirror(context.Background(), MirrorConfig{ + PubSub: ps, Snapshotter: NoopSessionPreamble{}, Client: fc, + ExploratorySession: fc.ExploratorySession, + Store: s, Logger: cfg.Logger, NewID: cfg.NewID, Now: cfg.Now, + PollInterval: 15 * time.Millisecond, + }) sp := Spawner(cfg) - // Drive the activation in a goroutine so we can publish frames - // after the bridge has subscribed. - done := make(chan error, 1) - go func() { - done <- sp(context.Background(), "org-test", wid, "/ignored", []activation.Trigger{{Kind: activation.TriggerHire}}) - }() + if err := sp(context.Background(), "org-test", wid, "/ignored", []activation.Trigger{{Kind: activation.TriggerEvent, EventID: "e1"}}); err != nil { + t.Fatalf("spawn: %v", err) + } - // Wait for the bridge to subscribe (handlers map populated). - deadline := time.Now().Add(2 * time.Second) + // The activation has returned, but the spawner registered the worker + // with the mirror, which now tracks its session. A turn arriving on + // that session from another surface (the inline chat) must still be + // captured. topic := pubsub.GetSessionQueue("u-owner", "ses_y") - for time.Now().Before(deadline) { - if ps.handlerCount(topic) > 0 { - break - } - time.Sleep(10 * time.Millisecond) + if !waitForHandlers(ps, topic, 1) { + t.Fatal("spawner did not leave a live mirror subscription for the session") } - patch, _ := json.Marshal(types.WebsocketEvent{EntryPatches: []types.EntryPatch{ {Index: 0, MessageID: "m1", Type: "text", Patch: "hi there"}, }}) @@ -485,75 +474,40 @@ func TestSpawnerPublishesTranscriptViaEntryStream(t *testing.T) { complete, _ := json.Marshal(types.WebsocketEvent{Interaction: &types.Interaction{State: "complete"}}) ps.publish(t, topic, complete) - if err := <-done; err != nil { - t.Fatalf("spawn: %v", err) - } - - events, err := s.Events.ListForStream(context.Background(), "org-test", activation.StreamID(wid), 100) - if err != nil { - t.Fatalf("list events: %v", err) - } - var sawAssistant bool - for _, e := range events { - msg, err := e.Message() - if err != nil { - continue - } - if strings.Contains(msg.Body, "assistant: hi there") { - sawAssistant = true - } - } - if !sawAssistant { - t.Fatalf("activation stream missing transcript line; events: %+v", events) + if !waitForSegment(t, s, wid, "assistant: hi there") { + t.Fatal("post-activation session turn not mirrored to the activation stream") } } -// TestSpawnerOpensFreshOnStaleSession: when the persisted session ID -// resume fails (Helix reports hadStreamErr), the spawner opens a -// fresh session and persists the new ID. -func TestSpawnerOpensFreshOnStaleSession(t *testing.T) { +// A follow-up must never churn to a fresh session: SendMessage is +// fire-and-forget and Helix auto-resumes a downed desktop on the same +// session, so the persisted pointer stays intact. +func TestSpawnerFollowUpSurvivesDownDesktop(t *testing.T) { t.Parallel() s, wid := newHelixTestStore(t) if err := SaveProject(context.Background(), s, "org-test", wid, "prj_test", "app_test", "repo_test"); err != nil { t.Fatalf("save project: %v", err) } - if err := SaveSession(context.Background(), s, "org-test", wid, "ses_stale"); err != nil { + if err := SaveSession(context.Background(), s, "org-test", wid, "ses_existing"); err != nil { t.Fatalf("save session: %v", err) } - fc := &staleSessionFake{ - fakeHelixClient: fakeHelixClient{ - startSessionID: "ses_fresh", - outputs: []types.SessionOutputResponse{{Status: "complete", Output: "ok"}}, - }, + fc := &fakeHelixClient{ + startSessionID: "ses_should_not_be_used", + outputs: []types.SessionOutputResponse{{Status: "complete", Output: "ok"}}, } - cfg := newHelixCfg(t, &fc.fakeHelixClient, s) - cfg.Client = fc - sp := Spawner(cfg) + sp := Spawner(newHelixCfg(t, fc, s)) if err := sp(context.Background(), "org-test", wid, "/ignored", []activation.Trigger{{Kind: activation.TriggerEvent, EventID: "e1"}}); err != nil { t.Fatalf("spawn: %v", err) } + if got := atomic.LoadInt32(&fc.startCalls); got != 0 { + t.Errorf("StartSession called %d times; a follow-up must never create a fresh session (no churn)", got) + } state, _ := LoadState(context.Background(), s, "org-test", wid) - if state.SessionID != "ses_fresh" { - t.Errorf("session pointer = %q, want ses_fresh (stale resume must fall through to fresh)", state.SessionID) + if state.SessionID != "ses_existing" { + t.Errorf("session pointer = %q, want ses_existing (follow-up must keep the same session)", state.SessionID) } } -// staleSessionFake reports hadStreamErr=true when a resume call is -// made (SessionID != "") — simulating Helix's "session no longer -// running" signal. Fresh opens (SessionID empty) succeed normally. -type staleSessionFake struct { - fakeHelixClient -} - -func (f *staleSessionFake) StartChatWithStatus(_ context.Context, req StartChatRequest) (types.Session, bool, error) { - atomic.AddInt32(&f.startCalls, 1) - f.mu.Lock() - f.lastStartReq = req - f.mu.Unlock() - hadErr := req.SessionID != "" // resume path → "session no longer running" - return types.Session{ID: f.startSessionID}, hadErr, f.startErr -} - // TestSpawnerRecordsActivationRowOnSuccess pins B5.6 — the Spawner // MUST create an activation row at start and complete it with // StatusOK at end, so the audit/replay surface stays in sync with diff --git a/api/pkg/org/infrastructure/runtime/helix/types.go b/api/pkg/org/infrastructure/runtime/helix/types.go index c42aed0361..31964593aa 100644 --- a/api/pkg/org/infrastructure/runtime/helix/types.go +++ b/api/pkg/org/infrastructure/runtime/helix/types.go @@ -11,72 +11,6 @@ func IsTerminalOutput(o types.SessionOutputResponse) bool { return o.Status == "complete" || o.Status == "error" } -// StartChatRequest is the body of POST /sessions/chat. Mirrors -// types.SessionChatRequest on the wire but adds the OnSessionID -// callback — a Go-only hook for early WS attach that doesn't -// serialise. Kept local because the callback field has no place on -// the canonical types.SessionChatRequest. -type StartChatRequest struct { - ProjectID string `json:"project_id"` - OrganizationID string `json:"organization_id,omitempty"` - SessionID string `json:"session_id,omitempty"` - SessionRole string `json:"session_role,omitempty"` - AgentType string `json:"agent_type,omitempty"` - AppID string `json:"app_id,omitempty"` - AssistantID string `json:"assistant_id,omitempty"` - Type string `json:"type,omitempty"` - ExternalAgentConfig *types.ExternalAgentConfig `json:"external_agent_config,omitempty"` - SystemPrompt string `json:"system,omitempty"` - Messages []SessionChatMessage `json:"messages"` - Stream bool `json:"stream,omitempty"` - Provider string `json:"provider,omitempty"` - Model string `json:"model,omitempty"` - CallbackURL string `json:"callback_url,omitempty"` - - // OnSessionID, if set, is invoked the moment Helix emits the - // session ID — before the agent has produced a reply. Callers - // attach a WS subscriber early via this hook. Not serialised. - OnSessionID func(sessionID string) `json:"-"` -} - -// SessionChatMessage is one entry in StartChatRequest.Messages. We -// keep a local minimum-fields struct rather than aliasing -// types.Message because Helix's /sessions/chat endpoint expects this -// trimmed shape (role + content only); the full types.Message -// carries id / created_at / state fields that the request body -// would include with default-zero values, which Helix rejects. -type SessionChatMessage struct { - Role string `json:"role"` - Content MessageContent `json:"content"` -} - -// MessageContent is the multipart body. helix-org only ever sends a -// single text part. content_type is omitted to match Helix UI's wire -// shape ({"parts":[…]}); Helix infers text from the part type. -type MessageContent struct { - Parts []any `json:"parts"` -} - -// NewTextMessage builds a single user-text message. -func NewTextMessage(role, text string) SessionChatMessage { - return SessionChatMessage{ - Role: role, - Content: MessageContent{Parts: []any{text}}, - } -} - -// SendMessageOptions are the optional knobs on SendSessionMessage. -type SendMessageOptions struct { - Interrupt bool - NotifyUserID string -} - -// SendMessageResponse is the body of POST /sessions/{id}/messages. -type SendMessageResponse struct { - RequestID string `json:"request_id"` - InteractionID string `json:"interaction_id"` -} - // ServerStatus mirrors the slice of /api/v1/config helix-org reads. type ServerStatus struct { MaxConcurrentDesktops int `json:"max_concurrent_desktops"` diff --git a/api/pkg/server/helix_org.go b/api/pkg/server/helix_org.go index 7a7b5e390c..b903a02cb8 100644 --- a/api/pkg/server/helix_org.go +++ b/api/pkg/server/helix_org.go @@ -279,7 +279,27 @@ func initHelixOrgHandler(cfg helixOrgConfig, helixStore helixstore.Store) (*heli secretInjectors := []runtimehelix.SpawnSecretInjector{ githubtransport.NewSecretInjector(githubtransport.TokenResolver(gitHubTokenResolver)), } - spawnerFn := lazyHelixOrgSpawner(configReg, helixStore, inProcClient, inProcClient, st, bc, cfg.APIServer.pubsub, logger, projectApplier, secretInjectors, deps.NewID, deps.Now) + // Transcript mirror — process-wide singleton shared by the spawner + // (Ensure), bootstrap (EnsureAll), and lifecycle.Fire (Stop). + mirror := runtimehelix.NewMirror(context.Background(), runtimehelix.MirrorConfig{ + PubSub: cfg.APIServer.pubsub, + Snapshotter: runtimehelix.NoopSessionPreamble{}, + Client: inProcClient, + ExploratorySession: func(ctx context.Context, projectID string) (string, error) { + sess, err := helixStore.GetProjectExploratorySession(ctx, projectID) + if err != nil || sess == nil { + return "", err + } + return sess.ID, nil + }, + Store: st, + Hub: bc, + NewID: deps.NewID, + Now: deps.Now, + Logger: logger, + }) + + spawnerFn := lazyHelixOrgSpawner(configReg, helixStore, inProcClient, inProcClient, st, bc, cfg.APIServer.pubsub, logger, projectApplier, secretInjectors, deps.NewID, deps.Now, mirror) dispatcher := dispatch.New(st, spawnerFn, logger) deps.Dispatcher = dispatcher @@ -323,6 +343,7 @@ func initHelixOrgHandler(cfg helixOrgConfig, helixStore helixstore.Store) (*heli // the REST handlers — one owner of activation/team Stream // lifecycle across hire, reparent, and fire. Topology: deps.Topology, + Mirror: mirror, // Fire stops the fired worker's subscription } apiDeps := helixorgapi.Deps{ @@ -515,7 +536,7 @@ func initHelixOrgHandler(cfg helixOrgConfig, helixStore helixstore.Store) (*heli Str("envs", envsDir). Int("json_api_routes", len(extras)). Msg("helix-org mounted at /api/v1/orgs/{org}/helix-org/") - scope := newHelixOrgScope(configReg, st, envsDir, helixStore) + scope := newHelixOrgScope(configReg, st, envsDir, helixStore, mirror) // Public github webhook handler — mounted on the insecure router // because GitHub deliveries authenticate via HMAC, not the helix @@ -910,6 +931,7 @@ func lazyHelixOrgSpawner( secretInjectors []runtimehelix.SpawnSecretInjector, newID func() string, now func() time.Time, + mirror *runtimehelix.Mirror, ) runtime.Spawner { var ( mu sync.Mutex @@ -934,6 +956,7 @@ func lazyHelixOrgSpawner( if err != nil { return fmt.Errorf("helix-org spawner not configured: %w", err) } + cfgVal.Mirror = mirror // shared singleton; not per-org config built := runtimehelix.Spawner(cfgVal) mu.Lock() if spawner == nil { diff --git a/api/pkg/server/helix_org_inproc.go b/api/pkg/server/helix_org_inproc.go index 16b4ece320..70712da934 100644 --- a/api/pkg/server/helix_org_inproc.go +++ b/api/pkg/server/helix_org_inproc.go @@ -463,175 +463,54 @@ func (c *inProcHelixClient) StopExternalAgent(ctx context.Context, sessionID str return nil } -// StartChatWithStatus opens or continues a chat session and reports -// whether the SSE stream surfaced a transient error after the session -// ID came through. The underlying startChatSessionHandler is streaming -// (writes SSE chunks to the ResponseWriter and Flushes), so we capture -// via a custom sseCapture writer that scans the chunks — `data: ` -// prefix, JSON chunks with `id` and `error.message`, sets hadWSError -// when an error chunk arrives. -func (c *inProcHelixClient) StartChatWithStatus(ctx context.Context, req runtimehelix.StartChatRequest) (types.Session, bool, error) { - if req.Type == "" { - req.Type = "text" - } - if len(req.Messages) == 0 { - return types.Session{}, false, errors.New("StartChatWithStatus: req.Messages must contain at least one message") - } - if req.AgentType == "zed_external" && req.ExternalAgentConfig == nil { - req.ExternalAgentConfig = &types.ExternalAgentConfig{} +// StartSession creates the worker's session (+ desktop + queued first +// message) via the shared StartExternalAgentSession primitive the cron +// trigger uses. Non-blocking. session_role "exploratory" so the mirror's +// GetProjectExploratorySession lookup resolves it. +func (c *inProcHelixClient) StartSession(ctx context.Context, params runtimehelix.StartSessionParams) (string, error) { + if params.Prompt == "" { + return "", errors.New("StartSession: Prompt is required") } - - r, err := c.newRequest(ctx, http.MethodPost, "/api/v1/sessions/chat", req, nil) + user, err := c.resolveUser(ctx) if err != nil { - return types.Session{}, false, err - } - cap := newSSECapture(req.OnSessionID) - c.server.startChatSessionHandler(cap, r) - if cap.statusCode >= 400 { - return types.Session{}, false, fmt.Errorf("start chat: HTTP %d: %s", cap.statusCode, strings.TrimSpace(cap.errBody.String())) - } - // Try SSE parsing first — `data: ` prefix means we got a stream. - if id, hadErr := cap.parseSSE(); id != "" { - return types.Session{ID: id}, hadErr, nil - } - // Fall back: handler may have returned a JSON body (helix_basic / - // openai shape). - if cap.body.Len() > 0 { - s, perr := parseStartChatResponseInProc(cap.body.Bytes()) - if perr != nil { - return types.Session{}, false, perr - } - if req.OnSessionID != nil && s.ID != "" { - req.OnSessionID(s.ID) - } - return s, false, nil + return "", err } - return types.Session{}, false, errors.New("start chat: no session id and no body") -} - -// parseStartChatResponseInProc handles both the zed_external -// types.Session shape and the OpenAI chat-completion shape -// helix_basic returns. -func parseStartChatResponseInProc(raw []byte) (types.Session, error) { - var s types.Session - _ = json.Unmarshal(raw, &s) - if len(s.Interactions) > 0 { - if s.ID == "" { - return types.Session{}, errors.New("start chat: session has no id") - } - return s, nil - } - var oai struct { - ID string `json:"id"` - Choices []struct { - Index int `json:"index"` - Message struct { - Role string `json:"role"` - Content string `json:"content"` - } `json:"message"` - } `json:"choices"` - } - if err := json.Unmarshal(raw, &oai); err != nil { - return types.Session{}, fmt.Errorf("decode start-chat response: %w", err) - } - if oai.ID == "" { - return types.Session{}, errors.New("start chat: empty session id") - } - out := types.Session{ID: oai.ID} - if len(oai.Choices) > 0 && oai.Choices[0].Message.Content != "" { - out.Interactions = []*types.Interaction{{ - ID: oai.ID + ":synth", - State: "complete", - ResponseMessage: oai.Choices[0].Message.Content, - }} - } - return out, nil -} - -// sseCapture is an http.ResponseWriter + http.Flusher that buffers -// everything the streaming startChatSessionHandler writes, so the -// adapter can scan it for SSE chunks (session ID + error.message). -// -// Flush() is a no-op — there's no client to push to, but the handler's -// `if f, ok := rw.(http.Flusher); ok` check still needs to succeed for -// chunks to be emitted on the buffer mid-handler. -type sseCapture struct { - header http.Header - body bytes.Buffer - errBody bytes.Buffer - statusCode int - onSessionID func(string) -} - -func newSSECapture(onSessionID func(string)) *sseCapture { - return &sseCapture{ - header: http.Header{}, - statusCode: http.StatusOK, - onSessionID: onSessionID, + req := &types.SessionChatRequest{ + ProjectID: params.ProjectID, + OrganizationID: params.OrganizationID, + AppID: params.AppID, + AgentType: params.AgentType, + Provider: types.Provider(params.Provider), + Model: params.Model, + SessionRole: "exploratory", + Messages: []*types.Message{{ + Role: "user", + Content: types.MessageContent{Parts: []any{params.Prompt}}, + }}, + } + session, err := c.server.StartExternalAgentSession(ctx, req, user.ID) + if err != nil { + return "", fmt.Errorf("start external agent session: %w", err) } + return session.ID, nil } -// Header satisfies http.ResponseWriter. -func (s *sseCapture) Header() http.Header { return s.header } - -// Write satisfies http.ResponseWriter. We buffer error bodies separately -// from success bodies so the caller can surface a meaningful HTTP-style -// error when the handler returned >=400. -func (s *sseCapture) Write(b []byte) (int, error) { - if s.statusCode >= 400 { - return s.errBody.Write(b) - } - return s.body.Write(b) -} - -// WriteHeader satisfies http.ResponseWriter. -func (s *sseCapture) WriteHeader(code int) { s.statusCode = code } - -// Flush satisfies http.Flusher — no-op since we're an in-process buffer. -func (s *sseCapture) Flush() {} - -// parseSSE scans the buffered body looking for `data: …` chunks of the -// shape `{"id":"…","error":{"message":"…"}}` and returns the first -// session ID it sees along with a flag indicating whether any chunk -// carried an error.message. -func (s *sseCapture) parseSSE() (sessionID string, hadWSError bool) { - // Quick check: does the body look like SSE? Handler emits "data: " - // prefixed lines for streaming sessions. helix_basic returns plain JSON. - bodyStr := s.body.String() - if !strings.Contains(bodyStr, "data:") { - return "", false - } - for _, line := range strings.Split(bodyStr, "\n") { - payload := strings.TrimSpace(line) - if payload == "" { - continue - } - payload = strings.TrimPrefix(payload, "data:") - payload = strings.TrimSpace(payload) - if payload == "" || payload == "[DONE]" { - continue - } - var chunk struct { - ID string `json:"id"` - Error *struct { - Message string `json:"message"` - } `json:"error,omitempty"` - } - if err := json.Unmarshal([]byte(payload), &chunk); err != nil { - continue - } - if chunk.ID != "" && sessionID == "" { - sessionID = chunk.ID - if s.onSessionID != nil { - s.onSessionID(sessionID) - } - } - if chunk.Error != nil { - hadWSError = true - break - } +// SendMessage dispatches a follow-up turn via the same REST handler the +// frontend / spec tasks use (POST /sessions/{id}/messages). Fire-and- +// forget; Helix auto-starts a downed desktop and delivers on reconnect. +func (c *inProcHelixClient) SendMessage(ctx context.Context, sessionID, prompt string) error { + if sessionID == "" { + return errors.New("SendMessage: sessionID is required") } - return sessionID, hadWSError + body := SessionMessageRequest{Content: prompt} + r, err := c.newRequest(ctx, http.MethodPost, "/api/v1/sessions/"+sessionID+"/messages", body, map[string]string{"id": sessionID}) + if err != nil { + return err + } + if _, herr := c.server.sendSessionMessage(nil, r); herr != nil { + return fmt.Errorf("send session message to %s: %s", sessionID, herr.Error()) + } + return nil } // Compile-time interface assertions — both ports must be satisfied by diff --git a/api/pkg/server/helix_org_inproc_test.go b/api/pkg/server/helix_org_inproc_test.go index bf819ec11c..a9a1e9cb48 100644 --- a/api/pkg/server/helix_org_inproc_test.go +++ b/api/pkg/server/helix_org_inproc_test.go @@ -137,12 +137,11 @@ func TestInProcSpawnerClient_StopExternalAgent_NoSession_ReturnsError(t *testing require.Error(t, err) } -// TODO: test for StartChatWithStatus. The streaming handler -// `startChatSessionHandler` calls into the chat controller (LLM + -// provider validation), which is non-trivial to satisfy from -// memorystore in isolation — providers, model catalogue, controller -// scheduler, etc. The structural adapter logic (sseCapture + SSE -// parsing) is exercised end-to-end by the helix-org alpha sandbox -// flow in the inner Helix; a focused unit test belongs in the -// follow-up that stubs Controller.ChatCompletion / a fake -// startChatSessionHandler entrypoint. +// TODO: tests for StartSession / SendMessage. StartSession routes to +// StartExternalAgentSession (starts a real dev container) and +// SendMessage to sendSessionMessage (needs a connected external-agent +// WS), both non-trivial to satisfy from memorystore in isolation. These +// adapters are the same shared primitives the cron trigger and spec +// tasks use, and are exercised end-to-end by the helix-org alpha sandbox +// flow in the inner Helix; focused unit tests belong in a follow-up that +// stubs the executor + WS manager. diff --git a/api/pkg/server/helix_org_middleware.go b/api/pkg/server/helix_org_middleware.go index 0851879a58..4f0044ef06 100644 --- a/api/pkg/server/helix_org_middleware.go +++ b/api/pkg/server/helix_org_middleware.go @@ -17,6 +17,7 @@ import ( "github.com/helixml/helix/api/pkg/org/application/configregistry" "github.com/helixml/helix/api/pkg/org/application/topology" helixorgstore "github.com/helixml/helix/api/pkg/org/domain/store" + runtimehelix "github.com/helixml/helix/api/pkg/org/infrastructure/runtime/helix" helixorgserver "github.com/helixml/helix/api/pkg/org/interfaces/server" helixstore "github.com/helixml/helix/api/pkg/store" ) @@ -31,6 +32,10 @@ type helixOrgScope struct { envsRoot string helixStore helixstore.Store + // mirror's EnsureAll runs after bootstrap so pre-existing / + // inline-chat-only workers are mirrored without an activation first. + mirror *runtimehelix.Mirror + mu sync.Mutex bootstrapped map[string]bool // bootstrapFlight dedupes concurrent first-load races on the same @@ -50,12 +55,13 @@ type helixOrgScope struct { // orgStore are the same instances handed to the helix-org handler; // envsRoot is the parent directory under which `/w-owner/` // will land at bootstrap time. -func newHelixOrgScope(configs *configregistry.Registry, orgStore *helixorgstore.Store, envsRoot string, hs helixstore.Store) *helixOrgScope { +func newHelixOrgScope(configs *configregistry.Registry, orgStore *helixorgstore.Store, envsRoot string, hs helixstore.Store, mirror *runtimehelix.Mirror) *helixOrgScope { return &helixOrgScope{ configs: configs, orgStore: orgStore, envsRoot: envsRoot, helixStore: hs, + mirror: mirror, bootstrapped: map[string]bool{}, } } @@ -130,6 +136,9 @@ func (s *helixOrgScope) ensureBootstrap(ctx context.Context, orgID string) error log.Warn().Err(err).Str("org_id", orgID).Msg("helix-org topology reconcile-all failed") } + // Mirror pre-existing workers (once per org per process). + s.mirror.EnsureAll(ctx, orgID) + s.mu.Lock() s.bootstrapped[orgID] = true s.mu.Unlock() diff --git a/api/pkg/server/helix_org_middleware_test.go b/api/pkg/server/helix_org_middleware_test.go index 0ca5a2a804..85578f99dd 100644 --- a/api/pkg/server/helix_org_middleware_test.go +++ b/api/pkg/server/helix_org_middleware_test.go @@ -56,6 +56,7 @@ func TestEnsureBootstrapConcurrentCallsAllSucceed(t *testing.T) { orgStore, t.TempDir(), &noAdminHelixStore{}, + nil, // mirror — nil is a safe no-op for this bootstrap-race test ) const N = 8 diff --git a/design/2026-06-09-activation-stream-transcript-still-empty.md b/design/2026-06-09-activation-stream-transcript-still-empty.md new file mode 100644 index 0000000000..b4a147685f --- /dev/null +++ b/design/2026-06-09-activation-stream-transcript-still-empty.md @@ -0,0 +1,155 @@ +# Worker transcripts on the activation stream (#2557 follow-up) + +Date: 2026-06-09 +Status: **FIXED** via a session-layer transcript mirror. Verified end-to-end +(spawner activations AND inline chat now both land on +`s-activations-`, DB + Streams UI). + +## Symptoms + +1. After #2557, hire/event/manual **activations** still recorded only + lifecycle markers (`=== activation … ===`, `=== exit … ===`) on + `s-activations-` — no `assistant:` / `tool_use` / `tool_result`. +2. Typing into the **inline "Chat with this worker"** panel (owner or any + worker) produced a turn the chat panel displayed, but **nothing** on + the worker's activation stream. + +## Root cause: the transcript writer was bolted onto the spawner + +Both surfaces already run the LLM turn through the **same** code: the +inline chat (`streaming.NewInference` → `POST /api/v1/sessions/chat`, +`streaming.tsx`) and the spawner's in-proc client (`StartChatWithStatus` +→ same `POST /api/v1/sessions/chat`, `helix_org_inproc.go`) drive the +**same per-worker `exploratory` session**. + +The only thing the spawner added was a *transcript bridge* — a +subscriber to `session-updates..` that mirrors settled +entries onto `s-activations-`. Two problems: + +- It was attached **only by the spawner**, **only for the duration of + one activation** — so inline-chat turns (no spawner) were never + mirrored. +- Even for activations it subscribed **too late**: `ensureSession`/ + `EnsureAndSend`/`StartChatWithStatus` runs the whole turn + synchronously and publishes every frame *before returning*; the bridge + was started only *after* `ensureSession` returned, so it subscribed + after the turn was already over. + +The owner-chat bridge that used to unify this (`api/pkg/org/server/chat`, +`HelixBridge`) was deleted in the #2516 redesign (`5a739cb42`) when the +htmx UI went away, and never rebuilt for the React/NewInference UI — the +comments referencing it stayed. + +## Fix: a session-layer Mirror that *follows* the worker's session + +`api/pkg/org/infrastructure/runtime/helix/mirror.go` — a `Mirror` +republishes every settled entry (and the user's prompt) to +`s-activations-` via the existing +`newBridge`/`TranscriptBody`/`PublishActivationEvent` pipeline. Because +spawner activations and inline chat share the worker's session, one +subscriber captures **every** turn from **every** surface — the single +writer the data model always wanted. + +**Key:** a worker's *session is not stable.* A stale resume opens a +fresh session, and the inline chat can land on a newer session than the +spawner last persisted (observed live: persisted `ses_…38v` while the +live turn was on `ses_…3hd`/`…3n3`/`…3v5`). The **stable** identity is +the worker's *project* — every one of its sessions shares it. So the +mirror does NOT pin a session ID. It tracks the *worker* and polls its +current session, re-pointing the subscription when it changes: + +- `Ensure(org, worker)` — start a per-worker tracker (idempotent). The + tracker resolves the worker's current session, subscribes, and on each + poll re-points if it changed (drops the old subscription, attaches to + the new — old pump flushes on cancel). First subscribe is synchronous. +- The "current session" = the project's most-recent **exploratory** + session (`store.GetProjectExploratorySession`) — exactly what the + inline chat / live UI follow — wired via `MirrorConfig.ExploratorySession`. +- Poll interval: `defaultMirrorPoll` (5s); so the stream can lag a real + session change by up to one interval, then catches up. +- `EnsureAll(org)` — tracks every worker in the org. +- `Stop(worker)` — cancels the tracker + subscription. + +Lifecycle (no fixed-session pinning, no per-activation bridge): +- **Spawner** (`spawner.go`) calls `Ensure(org, worker)` — the tracker is + long-lived and persists across activations, so it's already subscribed + before turns happen. +- **`ensureBootstrap`** (`helix_org_middleware.go`) calls `EnsureAll` + after its existing per-org `ReconcileAll` — once per org per process, + so pre-existing / inline-chat-only workers are tracked after a restart. +- **`lifecycle.Fire`** calls `Stop`. + +The dead `bridge.run` (per-activation subscribe loop) was removed; the +`bridge`/`EntryStream`/`TranscriptBody` rendering pipeline is reused. + +### Why poll, not a broad `session-updates.>` subscription + +A single wildcard subscription (resolve `session→project→worker` per +frame) would be churn-proof with no lag, but it's a firehose (all +sessions, cross-org) needing a session→worker cache + invalidation. The +poll-and-re-point approach stays per-worker, is far less code, and is +robust to churn at the cost of ≤1 poll-interval of lag. Chosen as the +proportionate fix; the broad subscription remains a future option if the +lag or per-worker poll cost ever matters. (We DO use NATS — an embedded +in-process server — so the wildcard option is technically available.) + +### Note: the churn itself + +aaa's rapid session churn (`exit: error: … open fresh helix session: … +external agent … timeout`) is a **separate, pre-existing** issue — the +sandbox agent intermittently not responding — not caused by this work. +It's what exposed the mirror's fixed-session fragility. Worth a separate +look at why activations time out. + +## Tests + +- `mirror_test.go`: + - `TestMirrorCapturesTurnWithoutSpawner` — a frame on the session topic + with **no spawner** lands on the activation stream (inline-chat + regression). + - `TestMirrorRepointsOnSessionChurn` — when the worker's session + changes, the mirror drops the old subscription and follows the new + one; a turn on the new session is captured. (The core fix.) + - `TestMirrorCapturesUserPrompt` — `user:` segment, once per + interaction (dedup). + - `TestMirrorEnsureIsIdempotent`, `TestMirrorStop`. +- `spawner_test.go::TestSpawnerEnsuresSessionMirror` — an activation + registers the worker, so a *later* session turn (inline chat) is still + captured. + +## End-to-end verification + +- Inline chat to `w-owner`: `assistant: MIRROR-E2E-PROBE-9931` appeared on + `s-activations-w-owner` — captured by the mirror, no activation. +- Manual activation of `aaa` (earlier iteration): full + `tool_use`/`tool_result`/`assistant` transcript on `s-activations-aaa`. + +## User turns ARE recorded (every prompt, no filtering) + +The mirror also emits a `user:` segment for each prompt. The user's +prompt isn't an entry-patch, but the same frames carry +`Interaction.PromptMessage` (`publishInteractionUpdateToFrontend` sends +the full interaction). `bridge.apply` reads `u.Interaction.PromptMessage` +and emits one `user:` line per interaction (deduped by interaction ID; +prompts come only from the single current interaction, never the +full-session history, so a restart doesn't re-emit past prompts). + +Every prompt is recorded deliberately — human inline-chat turns AND the +synthetic activation prompts the spawner injects. No filtering: the +stream is a faithful, observable record of exactly what each worker was +told and what it replied. Verified live: inline chat to `w-owner` +produced `user: Reply with exactly: …` immediately followed by +`assistant: …` on `s-activations-w-owner`. + +## Known limitations / follow-ups + +- **Multi-part prompts** (images / non-text `PromptMessageContent`) + produce no `user:` line — only the flat `PromptMessage` text field is + read. Text prompts (the common case) are covered. +- **First fresh-session turn (hire).** A brand-new worker's very first + turn streams on a session whose ID isn't known until the turn runs, so + the mirror attaches just after and misses that one turn; every + subsequent turn is captured. A session snapshotter (currently + `NoopSessionPreamble`) could backfill it. +- **One goroutine/subscription per active worker.** Fine for the alpha; + revisit if worker counts grow large. diff --git a/design/2026-06-09-helix-org-session-churn-fix.md b/design/2026-06-09-helix-org-session-churn-fix.md new file mode 100644 index 0000000000..24e05bb0d3 --- /dev/null +++ b/design/2026-06-09-helix-org-session-churn-fix.md @@ -0,0 +1,95 @@ +# helix-org session churn — root cause + fix + +Date: 2026-06-09 +Status: **FIXED** — helix-org now drives worker sessions through the same +canonical primitives every other autonomous flow uses; no blocking turn +wait, no stale detection, no fresh-session churn. + +## Symptom + +A worker (`aaa`) churned through fresh sessions every few minutes +(`ses_…38v → …3hd → …3n3 → …3v5`), each preceded by +`exit: error: ensure session: open fresh helix session: … external +agent … timeout`. Long agentic turns never completed; the activation +stream went silent (which also exposed the mirror's fixed-session +fragility — see `2026-06-09-activation-stream-transcript-still-empty.md`). + +## Root cause: helix-org used the wrong send path + +There are two ways to drive an external (Zed) agent in Helix: + +1. **Blocking OpenAI-compat** — `POST /sessions/chat` → + `handleExternalAgentStreaming` → `RunExternalAgent` → + `waitForExternalAgentResponse`, which **blocks up to 180s** + (`defaultExternalAgentWaitTimeout`) for the *whole turn*. Built for + OpenAI API clients expecting a synchronous chat completion. +2. **Fire-and-forget** — `POST /sessions/{id}/messages` → + `sendChatMessageToExternalAgent`: persists a Waiting interaction, + sends the WS command, **returns immediately**. The reply arrives async + via WS sync. No turn timeout. This is what the **human desktop** (types + straight into Zed), **spec tasks**, and the **cron trigger** use. + +helix-org's spawner used path (1) via the in-proc client's bespoke +`StartChatWithStatus`. Real helix-org turns (git pull specs, read +agent.md/role.md/identity.md, do work, commit, push) routinely exceed +180s, so they were killed mid-turn → `EnsureAndSend` misread the timeout +as a **stale session** → opened a **fresh** one → which also timed out → +churn. The "stale detection" was both unnecessary and the churn's engine. + +## Fix: reuse the canonical interfaces, delete the bespoke path + +`SessionClient` now exposes two methods, backed by the shared Helix +primitives the in-proc adapter routes to: + +| Interface method | Backed by | Used elsewhere by | +|---|---|---| +| `StartSession` | `StartExternalAgentSession` | cron trigger (`ExternalAgentStarter`) | +| `SendMessage` | `POST /sessions/{id}/messages` (`sendSessionMessage`) | frontend, spec tasks | + +Both are non-blocking, so neither is subject to the 180s response +timeout. `EnsureAndSend` collapses to: + +- **No session yet** → quota pre-flight → `StartSession` (creates the + session, starts the desktop, queues the prompt). Persist the id. +- **Has a session** → `SendMessage` (fire-and-forget). + +### Why "stale" detection was deleted, not preserved + +A worker now keeps **one durable session**, created once. There is no +staleness to detect because Helix already recovers a downed session +transparently: when `sendCommandToExternalAgent` finds no WS, it fires +`autoStartDevContainerForSession` (any `zed_external` session, not just +spec tasks) and `pickupWaitingInteraction` delivers the queued message on +reconnect — on the **same** session, preserving the Zed thread / +conversation. That's strictly better than the old "open a fresh session" +recovery, which lost continuity. The *capability* to recover moved to +where it already lives in Helix; helix-org just stopped *deciding* it. + +## Deleted + +- In-proc `StartChatWithStatus` + the `sseCapture`/`parseSSE` SSE-scraping + machinery + `parseStartChatResponseInProc`. +- `runtimehelix.StartChatRequest` / `SessionChatMessage` / `MessageContent` + / `NewTextMessage` (the old `/sessions/chat` request shape). +- `EnsureAndSend`'s resume-vs-fresh branching, `hadStreamErr`, cold-start + retry, and `sendToSession`. + +## Tests + +- `controller_external_agent.go` is untouched (the 180s timeout stays as + the correct cap for genuine OpenAI-compat callers). +- `spawner_test.go`: `TestSpawnerStartsFreshAndPersistsSession` (no + session → `StartSession`), `TestSpawnerFollowUpResumesPersistedSession` + (has session → `SendMessage`, no fresh `StartSession`), + `TestSpawnerFollowUpSurvivesDownDesktop` (follow-up never churns), + quota gate, semaphore, mirror wiring. Removed: cold-start-requeue and + open-fresh-on-stale tests (behaviours that no longer exist). +- Full helix/controller/server suites green. + +## Follow-up + +The 180s `waitForExternalAgentResponse` timer is still a *total* cap (not +reset on activity) for the OpenAI-compat path. That's fine for bounded +chat-completion clients, but if any future caller drives long turns +through `/sessions/chat`, consider making it an idle timeout. Not needed +for helix-org now that it's off that path entirely.