Skip to content

Build the remote session store independent of the memory.persistence gate (#372) - #373

Merged
initializ-mk merged 1 commit into
mainfrom
fix/remote-session-store-persistence
Jul 28, 2026
Merged

Build the remote session store independent of the memory.persistence gate (#372)#373
initializ-mk merged 1 commit into
mainfrom
fix/remote-session-store-persistence

Conversation

@initializ-mk

Copy link
Copy Markdown
Contributor

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)

ExecuteStream just wraps Execute, so both paths persist via persistSessionstore.Save. The problem is the store was never attached:

  • The remote session store (Remote session store — stateless multi-pod session memory (conditional-GET + CAS), no PVC #243) is built only inside if memPersistence { … } in runner.go.
  • memPersistence comes from memory.persistence in forge.yaml (default true). For CI/BYO agents the forge.yaml is the developer's and commonly leaves persistence off — while the platform injects FORGE_SESSION_STORE=remote + URL + token at deploy expecting persistence.
  • Result: the whole block is skipped, execCfg.Store stays nil, and persistSession silently no-ops (if e.store == nil { return }). Every invocation — streaming and non-streaming — drops its session.

Confirmed on the live pod: FORGE_SESSION_STORE=remote set, no "memory persistence enabled" / "engaged remote backend" log at startup, and 0 AgentSessions rows 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. New selectSessionStore builds the remote store unconditionally when configured (env/config); the memPersistence gate now governs only the local file fallback. Extracted from runner.go so it's unit-tested:

  • TestSelectSessionStore_RemoteIgnoresPersistenceGate — remote engages with memPersistence=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: true in the agent's forge.yaml (or agent-builder can inject FORGE_MEMORY_PERSISTENCE-equivalent) — but this fix removes the footgun so FORGE_SESSION_STORE=remote alone is sufficient.

Supersedes the streaming-path framing in the issue title.

Closes #372.

…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 initializ-mk left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • selectSessionStore precedence: remote (if configured) wins ungated → else file gated on memPersistence → else nil. Exactly the intended semantic — FORGE_SESSION_STORE=remote is explicit platform-wired persistence that shouldn't be gated behind the developer's memory.persistence, while that toggle still governs the local file fallback.
  • Non-remote behavior unchanged: the outer block went from if memPersistence to unconditional, but selectSessionStore returns nil when !memPersistence && !remote, and the downstream compactor/attach stays behind if 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=remote but 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 AgentSessions rows 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.

@initializ-mk
initializ-mk merged commit 2018586 into main Jul 28, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Streaming (tasks/sendSubscribe) invocations don't persist sessions — channel/widget agents show no executions

1 participant