diff --git a/forge-cli/runtime/runner.go b/forge-cli/runtime/runner.go index 0443ab8..b449ac4 100644 --- a/forge-cli/runtime/runner.go +++ b/forge-cli/runtime/runner.go @@ -1304,44 +1304,30 @@ func (r *Runner) Run(ctx context.Context) error { if os.Getenv("FORGE_MEMORY_PERSISTENCE") == "false" { memPersistence = false } - if memPersistence { + { // Select the session-memory backend (issue #243). // Remote (opt-in) pushes snapshots to the platform // session service so stateless pods resume any task on // any replica; file (default) keeps today's local - // .forge/sessions. buildSessionStore returns the remote - // store when configured, else nil to signal "use file". - var sessionStore coreruntime.SessionStore - var storeDesc map[string]any - - if remote := buildRemoteSessionStore( + // .forge/sessions. buildRemoteSessionStore returns the + // remote store when configured, else nil to signal "use + // file". + // A remote session store is EXPLICIT platform-wired + // persistence — selectSessionStore builds it + // independent of the memPersistence gate (#372); the + // gate now governs only the local file fallback. + sessDir := r.cfg.Config.Memory.SessionsDir + if sessDir == "" { + sessDir = filepath.Join(r.cfg.WorkDir, ".forge", "sessions") + } + sessionStore, storeDesc := selectSessionStore( + memPersistence, r.cfg.Config.AgentID, r.cfg.Config.Memory.SessionStore, r.cfg.Config.Memory.SessionStoreURL, + sessDir, r.logger, - ); remote != nil { - sessionStore = remote - storeDesc = map[string]any{"backend": "remote"} - } else { - sessDir := r.cfg.Config.Memory.SessionsDir - if sessDir == "" { - sessDir = filepath.Join(r.cfg.WorkDir, ".forge", "sessions") - } - memStore, storeErr := coreruntime.NewMemoryStore(sessDir) - if storeErr != nil { - r.logger.Warn("failed to create memory store, persistence disabled", map[string]any{ - "error": storeErr.Error(), - }) - } else { - // Clean up old sessions on startup (7-day TTL). - deleted, _ := memStore.Cleanup(7 * 24 * time.Hour) - if deleted > 0 { - r.logger.Info("cleaned up old sessions", map[string]any{"deleted": deleted}) - } - sessionStore = memStore - storeDesc = map[string]any{"backend": "file", "sessions_dir": sessDir} - } - } + ) if sessionStore != nil { compactor := coreruntime.NewCompactor(coreruntime.CompactorConfig{ diff --git a/forge-cli/runtime/session_store_loader.go b/forge-cli/runtime/session_store_loader.go index 0860781..96130ad 100644 --- a/forge-cli/runtime/session_store_loader.go +++ b/forge-cli/runtime/session_store_loader.go @@ -2,6 +2,7 @@ package runtime import ( "os" + "time" coreruntime "github.com/initializ/forge/forge-core/runtime" ) @@ -35,6 +36,34 @@ const ( // misconfiguration: rather than silently drop session persistence, we // warn (single-line fix in the manifest) and fall back to the file // backend by returning nil. +// selectSessionStore picks the session-memory backend (#243, #372). A +// configured REMOTE store is used unconditionally — it is explicit +// platform-wired persistence, so it must not be gated behind +// memory.persistence (an agent whose forge.yaml left persistence off, common +// for CI/BYO images where the platform injects FORGE_SESSION_STORE=remote, +// otherwise silently dropped ALL session history). The local FILE store is +// used only when memPersistence is on. Returns (nil, nil) when neither +// applies. storeDesc is the log/observability descriptor. +func selectSessionStore(memPersistence bool, agentID, cfgMode, cfgURL, sessionsDir string, logger coreruntime.Logger) (coreruntime.SessionStore, map[string]any) { + if remote := buildRemoteSessionStore(agentID, cfgMode, cfgURL, logger); remote != nil { + return remote, map[string]any{"backend": "remote"} + } + if !memPersistence { + return nil, nil + } + memStore, err := coreruntime.NewMemoryStore(sessionsDir) + if err != nil { + if logger != nil { + logger.Warn("failed to create memory store, persistence disabled", map[string]any{"error": err.Error()}) + } + return nil, nil + } + if deleted, _ := memStore.Cleanup(7 * 24 * time.Hour); deleted > 0 && logger != nil { + logger.Info("cleaned up old sessions", map[string]any{"deleted": deleted}) + } + return memStore, map[string]any{"backend": "file", "sessions_dir": sessionsDir} +} + func buildRemoteSessionStore(agentID, cfgMode, cfgURL string, logger coreruntime.Logger) coreruntime.SessionStore { mode := cfgMode if v := os.Getenv(EnvSessionStore); v != "" { diff --git a/forge-cli/runtime/session_store_loader_test.go b/forge-cli/runtime/session_store_loader_test.go index 34f76b1..7f34545 100644 --- a/forge-cli/runtime/session_store_loader_test.go +++ b/forge-cli/runtime/session_store_loader_test.go @@ -41,3 +41,38 @@ func TestBuildRemoteSessionStore(t *testing.T) { }) } } + +// #372: a configured remote session store must be selected even when +// memPersistence is off — an agent whose forge.yaml left persistence off +// (common for CI/BYO images, where the platform injects +// FORGE_SESSION_STORE=remote) otherwise dropped all session history. +func TestSelectSessionStore_RemoteIgnoresPersistenceGate(t *testing.T) { + t.Setenv(EnvSessionStore, sessionStoreRemote) + t.Setenv(EnvSessionStoreURL, "http://agent-builder.svc:8080/api/v1/agent-sessions") + t.Setenv(EnvPlatformToken, "tok-platform") + + // memPersistence=false must NOT stop the remote store from engaging. + store, desc := selectSessionStore(false, "fundraise", "", "", t.TempDir(), nil) + if store == nil { + t.Fatal("remote store must engage independent of memPersistence (#372)") + } + if desc["backend"] != "remote" { + t.Fatalf("backend = %v, want remote", desc["backend"]) + } +} + +// Without a remote store configured, the local file store still respects the +// memPersistence gate. +func TestSelectSessionStore_FileGatedOnPersistence(t *testing.T) { + t.Setenv(EnvSessionStore, "") + t.Setenv(EnvSessionStoreURL, "") + t.Setenv(EnvPlatformToken, "") + + if store, _ := selectSessionStore(false, "a", "", "", t.TempDir(), nil); store != nil { + t.Fatal("file store must NOT build when memPersistence is off") + } + store, desc := selectSessionStore(true, "a", "", "", t.TempDir(), nil) + if store == nil || desc["backend"] != "file" { + t.Fatalf("file store must build when memPersistence is on: store=%v desc=%v", store, desc) + } +}