fix(session-tracker): prevent bypass memory leak in OpenCode provider#502
Conversation
The OpenCode provider creates sessions for pipeline work (extraction, hints, summaries). The OpenCode plugin intercepts those prompts and calls hook endpoints back to the daemon, triggering full memory recall for the daemon's own internal prompts. This circular loop adds 30-90s overhead per pipeline call, causing systematic timeouts. Register each daemon-created OpenCode session in the bypass set via bypassSession(). When hooks arrive for that session, checkBypass() returns empty no-op responses instantly, breaking the loop. Clean up bypass state on all session invalidation paths (404, 422, malformed 200).
bypassedSessions changed from Set<string> to Map<string, number> so bypass-only entries (pipeline sessions with allowUnknown) expire and get evicted by cleanupStaleSessions. Removes all unbypassSession calls from the OpenCode provider — old sessions stay bypassed until TTL cleanup handles them, closing the circular hook loop root cause.
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
26 similar comments
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
6 similar comments
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
|
The reviewer bot is currently experiencing problems. Marking as draft temporarily so the bot does not token burn. |
|
Resuming the review. Claude Code updated and broke our PR-reviewer lol |
|
Hi @Ostico - I'm an automated code reviewer powered by pr-reviewer. I'm taking a look at the fixes in |
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: claude-sonnet-4-6 | commit:
b2184867
[Automated Review — pr-reviewer bot] PR #502 replaces a permanent Set<string> bypass store with a TTL-backed Map<string, number> and removes the four unbypassSession() call sites from provider.ts. The core mechanic is sound. Three issues found: one warning (bypass TTL not refreshed on session renewal) and two nitpicks (misleading log field, stale .size in consumer).
Confidence: Medium [sufficient_diff_evidence, missing_cross_module_context] - The bypass-TTL-renewal gap is directly provable from session-tracker.ts lines 167–173 (bypassSession writes a fixed expiresAt) and lines 249–260 (renewSession updates claim.expiresAt but never touches bypassedSessions). Whether non-pipeline callers exercise this path is not visible in the diff — pipeline-routes.ts and any other bypassSession callers outside provider.ts are not included, so medium confidence.
Unmapped findings (not on changed lines):
- packages/daemon/src/session-tracker.ts:253 - Warning: bypass TTL is not refreshed on session renewal.
renewSession() updates claim.expiresAt but never touches bypassedSessions. A session claimed and bypassed (non-allowUnknown path) that is renewed repeatedly will eventually have its bypass expire before the session claim does — at which point hooks resume and memory recall can fire again for what was supposed to be a permanently-bypassed session.
For OpenCode pipeline sessions this likely does not bite (sessions are short-lived and bypass is set via allowUnknown, not tied to a claim), but any future caller that bypasses a claimed session and also renews it will silently lose the bypass after 4 hours.
Suggested fix in renewSession():
// After updating claim.expiresAt:
const existingBypass = bypassedSessions.get(key);
if (existingBypass !== undefined) {
bypassedSessions.set(key, claim.expiresAt);
}| bypassedSessions.delete(key); | ||
| cleaned++; | ||
| } | ||
| } |
There was a problem hiding this comment.
Nitpick: misleading log field name.
bypassOnly: bypassedSessions.size logs the total size of the bypass map (which includes both session-claimed entries and bypass-only/allowUnknown entries). The label bypassOnly implies it counts entries without a matching session claim. Rename to remainingBypasses or bypassEntries to avoid confusion when reading daemon logs.
| @@ -195,7 +208,7 @@ export function getActiveSessions(): readonly SessionInfo[] { | |||
| runtimePath: claim.runtimePath, | |||
| claimedAt: claim.claimedAt, | |||
There was a problem hiding this comment.
Nitpick: getBypassedSessionKeys() may over-report .size until the next sweep.
The returned Map is the live bypassedSessions reference, which can contain expired entries that have not yet been lazily evicted (lazy eviction only happens on isSessionBypassed calls, and periodic cleanup runs every 15 min). The one known consumer (pipeline-routes.ts) reads .size, so the count can be inflated for up to 15 minutes after entries expire. Consider documenting this in the JSDoc, or performing an active filter before returning.
| } | ||
| bypassedSessions.add(key); | ||
| logger.info("session-tracker", "Session bypassed", { sessionKey: key }); | ||
| const ttl = opts?.ttlMs ?? STALE_SESSION_MS; |
There was a problem hiding this comment.
nice fix :) one small thing i think is worth guarding here: if ttlMs ever comes through as NaN or some other non-finite value, Date.now() + ttl becomes NaN, and then this bypass entry won’t expire because the later expiry checks never pass.
might be worth falling back to STALE_SESSION_MS unless ttlMs is a finite positive number, just to keep this fully leak-proof.
Guard bypassSession() against NaN/non-finite/negative ttlMs values falling back to STALE_SESSION_MS. Refresh bypass TTL in renewSession() so bypassed entries do not leak after session lifetime extension.
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: gpt-5.3-codex | commit:
891ecd3f
[Automated Review by pr-reviewer] The PR moves bypass tracking to TTL-backed storage and removes immediate unbypass on OpenCode session rotation, but one logic gap means the circular-recall risk can still return for long-lived OpenCode sessions.
Confidence: Medium [sufficient_diff_evidence, targeted_context_included, missing_runtime_repro] - In provider.ts, newly created OpenCode sessions are bypassed via bypassSession(id, { allowUnknown: true }) with no explicit TTL, and in session-tracker.ts the default TTL is STALE_SESSION_MS (4h). allowUnknown sessions are not claimed in sessions, so renewSession() never extends their bypass TTL. After 4h, bypass expires and hooks can run again for a still-alive OpenCode session, which conflicts with the PR goal of preventing the circular recall loop.
| } | ||
| logger.debug("pipeline", "OpenCode session created", { id }); | ||
| // Bypass hooks for our own pipeline sessions so the OpenCode plugin | ||
| // does not trigger memory recall back to the daemon (circular loop). |
There was a problem hiding this comment.
OpenCode-created sessions are bypassed with default TTL only (bypassSession(id, { allowUnknown: true })). Because these allowUnknown sessions are not in the claimed-session map, their bypass is never refreshed by renewSession(). If OpenCode keeps a rotated session alive beyond 4 hours, bypass will expire and the original circular memory-recall loop can reappear. Consider either (a) a much longer/explicit TTL for provider-owned sessions, or (b) claiming provider-owned sessions so bypass lifetime can track active session lifetime.
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: gpt-5.3-codex | commit:
2117aaf1
[Automated Review by pr-reviewer] The core fix is directionally correct, but one remaining logic gap can reintroduce the OpenCode hook loop after long daemon uptime.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - In createSession(), OpenCode sessions are bypassed with bypassSession(id, { allowUnknown: true }) (no explicit TTL). In session-tracker.ts, bypass entries default to STALE_SESSION_MS (4h), and only claimed sessions get TTL refresh via renewSession(). These allowUnknown OpenCode sessions are bypass-only entries, so bypass expires after 4h even if OpenCode keeps the session alive, allowing hooks/recall to resume.
| } | ||
| logger.debug("pipeline", "OpenCode session created", { id }); | ||
| // Bypass hooks for our own pipeline sessions so the OpenCode plugin | ||
| // does not trigger memory recall back to the daemon (circular loop). |
There was a problem hiding this comment.
OpenCode daemon-owned sessions are bypassed without an explicit long-lived TTL (bypassSession(id, { allowUnknown: true })). Because these sessions are not claimed in sessions, renewSession() never refreshes their bypass expiry. After the default 4h TTL elapses, bypass drops while the upstream session may still be active, which can re-enable the circular memory-recall loop the PR is trying to prevent. Consider either passing a provider-specific longer TTL for allowUnknown OpenCode sessions, or adding a refresh path tied to OpenCode session activity.
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: gpt-5.3-codex | commit:
b821dd69
[Automated Review by pr-reviewer] The core refactor is solid, but one remaining logic gap means the stated OpenCode loop-prevention goal is not fully met.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - In createOpenCodeProvider.createSession(), OpenCode sessions are bypassed with bypassSession(id, { allowUnknown: true }) and no explicit TTL. In session-tracker.ts, bypass entries default to STALE_SESSION_MS (4h) and bypass-only sessions are swept after expiry. These allowUnknown sessions are not in sessions, so renewSession() never extends their bypass. If OpenCode keeps a session alive beyond 4h, bypass expires and hooks can run again, which diverges from the PR claim of preventing the circular recall loop.
| } | ||
| logger.debug("pipeline", "OpenCode session created", { id }); | ||
| // Bypass hooks for our own pipeline sessions so the OpenCode plugin | ||
| // does not trigger memory recall back to the daemon (circular loop). |
There was a problem hiding this comment.
OpenCode-created daemon sessions are bypassed with default TTL only (bypassSession(id, { allowUnknown: true })). Because these are bypass-only entries (not claimed sessions), their bypass expires after 4h and is not refreshed by renewSession(). A long-lived OpenCode session can then resume normal hooks and re-trigger the same circular recall behavior this PR is intended to prevent. Consider setting an explicit long-lived TTL for daemon-owned OpenCode sessions, or re-applying bypass on each use.
Re-call bypassSession() in sendMessage() so bypass-only entries do not expire after 4h while the provider still sends messages.
|
Nice @Ostico! Btw, I responded to you on the discussion you opened :) |
PR-Reviewer-Ant
left a comment
There was a problem hiding this comment.
Automated review by pr-reviewer | model: gpt-5.3-codex | commit:
dd0ef9d3
[Automated Review by pr-reviewer] I found a blocking scope/behavior regression unrelated to the stated fix, plus PR-description divergence that needs correction before human merge review.
Confidence: High [sufficient_diff_evidence, targeted_context_included] - The incremental diff clearly changes runtime defaults in multiple user-facing paths (dashboard + CLI setup), including resolveSynthesisProvider() fallback switching from ollama to llama-cpp. That is a concrete behavior change not reflected in the PR description/readiness claims and can break environments without llama.cpp.
Note: This PR touches UI files but no screenshots were referenced in the description. Consider adding visual previews for reviewers.
Unmapped findings (not on changed lines):
- packages/cli/dashboard/src/lib/components/tabs/settings/pipeline-settings.ts:67 - Blocking: this changes the synthesis fallback default from
ollamatollama-cpp(return "llama-cpp";). That is a user-facing behavioral change and potential breaking regression for installs that have Ollama but not llama.cpp. It is outside the stated PR scope (session-tracker bypass leak fix) and should either be reverted from this PR or explicitly scoped/documented/tested as a separate change. - packages/cli/dashboard/src/lib/components/tabs/settings/EmbeddingsSection.svelte:160 - PR description divergence: this PR now includes UI/settings surface changes (new
llama-cppprovider paths and base URL behavior), but the description saysN/A — no UI changesand claims internal session-tracker plumbing only. Please update the PR description/readiness checklist to match actual diff scope, or split unrelated UI/config-default changes into a separate PR.
|
god dammit |
|
ha! theyre false flags. merging this |
|
Yeah thank you |
Summary
The OpenCode pipeline provider creates daemon-owned sessions that are bypassed to prevent circular hook loops. When sessions rotate (structured output rejection, 404/410, malformed payload),
unbypassSession()removed the bypass on the old session — but OpenCode kept that session alive and continued sending hooks to it, re-triggering the full memory recall loop. Additionally, bypass-only entries (sessions withallowUnknown: truethat never enter thesessionsMap) were stored in aSet<string>with no expiry, so they accumulated monotonically across daemon uptime.This PR removes all
unbypassSession()calls from the provider, changes the bypass store fromSet<string>toMap<string, number>(key → expiresAt), and lets the existingcleanupStaleSessionsinterval evict expired bypass entries.Changes
packages/daemon/src/session-tracker.ts—bypassedSessionsfromSet<string>toMap<string, number>with TTL;bypassSession()accepts optionalttlMs;isSessionBypassed()checks expiry and auto-evicts;cleanupStaleSessions()sweeps expired bypass-only entries; newrunStaleCleanup()test export; bypass/unbypass log level lowered todebugpackages/daemon/src/pipeline/provider.ts— removed all 4unbypassSession()calls and unused import; fixed misleading "bounded by workers" comment to reference TTL cleanuppackages/daemon/src/session-tracker.test.ts— 6 new tests coveringallowUnknownbypass, TTL expiry/cleanup, and session rotation persistencepackages/daemon/src/memory-config.ts— fixed stray character causing parser error (separate commit)Type
fix— bug fixPackages affected
@signet/daemonScreenshots
N/A — no UI changes.
PR Readiness (MANDATORY)
INDEX.md+dependencies.yaml) — no spec-governed behavior changed; internal session-tracker plumbing onlyttlMsdefaults toSTALE_SESSION_MS(4h); expiry timestamps validated inisSessionBypassedsession-tracker.test.tsTesting
bun testpasses (15/15 across session-tracker, session-api, hooks-recall)bun run typecheckpasses — pre-existingmemory-config.tsparser error on main (the stray character fix in this PR resolves it)bun run lintpasses (Biome clean on all changed files)AI disclosure
Assisted-bytags in commits)Notes
memory-config.tstypo fix (straypafter comma on line 833) is a pre-existing issue onmainthat breaksbun run build. It's in a separate commit (chore(daemon): fix stray character in memory-config parser).getBypassedSessionKeys()return type changed fromReadonlySet<string>toReadonlyMap<string, number>. The only consumer (pipeline-routes.ts) uses.sizewhich works on both — no breaking change.worker.test.tsare unrelated to this PR (they fail onmainas well).