Build the remote session store independent of the memory.persistence gate (#372) - #373
Merged
Merged
Conversation
…ry.persistence gate The remote session store (issue #243) was only built inside the memPersistence branch, so an agent whose forge.yaml left memory.persistence off — common for CI/BYO images, where the platform injects FORGE_SESSION_STORE=remote but the developer's forge.yaml never mentions memory — attached no store at all. persistSession then no-ops silently, so EVERY invocation (streaming and non-streaming) dropped its session: zero executions surfaced in the console despite the runs completing (traced live: a CI agent with FORGE_SESSION_STORE=remote set, no 'memory persistence enabled' at startup, 0 AgentSessions rows). A remote store is explicit platform-wired persistence and must not be gated behind memory.persistence. selectSessionStore now builds it unconditionally when configured; the memPersistence gate governs only the local file fallback. Extracted from runner.go so the decoupling is unit-tested (remote-ignores-gate, file-still-gated).
initializ-mk
commented
Jul 28, 2026
initializ-mk
left a comment
Contributor
Author
There was a problem hiding this comment.
Review: decouple remote session store from memory.persistence
Correct, well-diagnosed, well-tested. The decoupling logic is right, the failure modes now all log (fixing the silent drop), and there's no security regression. No findings. Merge-ready. CI fully green.
The fix is right
selectSessionStoreprecedence: remote (if configured) wins ungated → else file gated onmemPersistence→ else nil. Exactly the intended semantic —FORGE_SESSION_STORE=remoteis explicit platform-wired persistence that shouldn't be gated behind the developer'smemory.persistence, while that toggle still governs the local file fallback.- Non-remote behavior unchanged: the outer block went from
if memPersistenceto unconditional, butselectSessionStorereturns nil when!memPersistence && !remote, and the downstream compactor/attach stays behindif sessionStore != nil. A default or persistence-off agent with no remote config behaves exactly as before — only remote-configured agents change, which is the point. - Failure modes now all log, closing the silent drop: correct remote config →
Info: engaged remote backend(the startup log that was missing on the broken pod);mode=remotebut missing URL/token →Warn: … using file backend; not configured → silent nil → file. The Info log carries url/agent_id/org_id/workspace_id but not the platform token — no secret leak. - Root-cause diagnosis is exemplary — traced live (missing log + 0
AgentSessionsrows on the actual pod), honest about superseding the streaming-path theory in the issue title.
Security / observability
- No new data exposure: building the remote store unconditionally is the platform's explicit intent (it injected
FORGE_SESSION_STORE=remote+ URL + token so the console gets session rows). The bug was the configured store not attaching; making it attach is correct, not a privacy regression. - One behavior-change to be explicit about (already in the PR body, non-blocking): a BYO agent with
memory.persistence: false+ platform-injected remote env now persists to the remote store where it previously didn't. Intended (platform persistence ≠ local-file toggle) and documented — just the thing a BYO developer should know.
Tests
TestSelectSessionStore_RemoteIgnoresPersistenceGate (remote engages with memPersistence=false) and TestSelectSessionStore_FileGatedOnPersistence (file respects the gate, both directions) pin exactly the decoupling. Extracting the previously-buried selection logic into a unit-testable function is the right call — and every path now emits a startup log, so this class of silently-no-op'd persistence can't recur invisibly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the actual root cause of "no executions" for the fundraise (and other CI/BYO) agents — which turned out not to be the streaming-path theory in the issue title. Corrected diagnosis below.
Real root cause (traced live: initializ-test / ws_83dc9f48, CI agent
fundraise)ExecuteStreamjust wrapsExecute, so both paths persist viapersistSession→store.Save. The problem is the store was never attached:if memPersistence { … }inrunner.go.memPersistencecomes frommemory.persistencein forge.yaml (default true). For CI/BYO agents the forge.yaml is the developer's and commonly leaves persistence off — while the platform injectsFORGE_SESSION_STORE=remote+ URL + token at deploy expecting persistence.execCfg.Storestays nil, andpersistSessionsilently no-ops (if e.store == nil { return }). Every invocation — streaming and non-streaming — drops its session.Confirmed on the live pod:
FORGE_SESSION_STORE=remoteset, no "memory persistence enabled" / "engaged remote backend" log at startup, and 0AgentSessionsrows for the agent despite runs completing. Agents whose sessions DO show (console-chat, scheduled) had persistence on.Fix
A remote session store is explicit platform-wired persistence and must not be gated behind
memory.persistence. NewselectSessionStorebuilds the remote store unconditionally when configured (env/config); thememPersistencegate now governs only the local file fallback. Extracted fromrunner.goso it's unit-tested:TestSelectSessionStore_RemoteIgnoresPersistenceGate— remote engages withmemPersistence=false.TestSelectSessionStore_FileGatedOnPersistence— file store still respects the gate.runtime suite green, gofmt clean.
Rollout
This is baked into the agent image, so affected agents need a rebuild on a forge version with this fix. Immediate unblock without waiting for a forge release: set
memory.persistence: truein the agent's forge.yaml (or agent-builder can injectFORGE_MEMORY_PERSISTENCE-equivalent) — but this fix removes the footgun soFORGE_SESSION_STORE=remotealone is sufficient.Supersedes the streaming-path framing in the issue title.
Closes #372.