-
Notifications
You must be signed in to change notification settings - Fork 2
fix(plugin): lifecycle correctness — dispose hook, claim release, startup races #62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b83a1e6
8c51548
e1b22d8
582aa83
d42275e
dcfe834
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ import type { UserProfileData } from "./services/user-profile/types.js"; | |
| import type { SearchResult } from "./services/sqlite/types.js"; | ||
| import { getLanguageName } from "./services/language-detector.js"; | ||
| import type { MemoryScope } from "./services/client.js"; | ||
| import { setProviderStateInit } from "./services/ai/opencode-provider.js"; | ||
|
|
||
| async function showToast( | ||
| ctx: PluginInput, | ||
|
|
@@ -128,25 +129,35 @@ export const OpenCodeMemPlugin: Plugin = async (ctx: PluginInput) => { | |
| const GLOBAL_PLUGIN_WARMUP_KEY = Symbol.for("opencode-mem0.plugin.warmedup"); | ||
|
|
||
| if (!(globalThis as any)[GLOBAL_PLUGIN_WARMUP_KEY] && isConfigured()) { | ||
| try { | ||
| const timeoutMs = CONFIG.warmupTimeoutMs ?? 30000; | ||
| await Promise.race([ | ||
| memoryClient.warmup(), | ||
| new Promise<void>((_, reject) => | ||
| setTimeout(() => reject(new Error(`Warmup timed out after ${timeoutMs}ms`)), timeoutMs) | ||
| ), | ||
| ]); | ||
| (globalThis as any)[GLOBAL_PLUGIN_WARMUP_KEY] = true; | ||
| } catch (error) { | ||
| log("Plugin warmup failed", { error: String(error) }); | ||
| if (error instanceof Error && error.message.includes("timed out")) { | ||
| embeddingService.embeddingAvailable = false; | ||
| embeddingService.isWarmedUp = true; | ||
| log( | ||
| "Embedding model warmup timed out — marking embeddings unavailable. Searches will use text-only fallback." | ||
| ); | ||
| void (async () => { | ||
| try { | ||
| const timeoutMs = CONFIG.warmupTimeoutMs ?? 30000; | ||
| let timeoutId: ReturnType<typeof setTimeout> | undefined; | ||
| try { | ||
| await Promise.race([ | ||
| memoryClient.warmup(), | ||
| new Promise<void>((_, reject) => { | ||
| timeoutId = setTimeout( | ||
| () => reject(new Error(`Warmup timed out after ${timeoutMs}ms`)), | ||
| timeoutMs | ||
| ); | ||
| }), | ||
| ]); | ||
| } finally { | ||
| clearTimeout(timeoutId); | ||
| } | ||
| (globalThis as any)[GLOBAL_PLUGIN_WARMUP_KEY] = true; | ||
| } catch (error) { | ||
| log("Plugin warmup failed", { error: String(error) }); | ||
| if (error instanceof Error && error.message.includes("timed out")) { | ||
| embeddingService.embeddingAvailable = false; | ||
| embeddingService.isWarmedUp = true; | ||
| log( | ||
| "Embedding model warmup timed out — marking embeddings unavailable. Searches will use text-only fallback." | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| })(); | ||
| } | ||
|
|
||
| // Notify when a newer release exists (OpenCode pins plugin versions in its | ||
|
|
@@ -178,9 +189,9 @@ export const OpenCodeMemPlugin: Plugin = async (ctx: PluginInput) => { | |
| } | ||
| } | ||
|
|
||
| // Wire opencode state path and provider list — fire-and-forget to avoid blocking init | ||
| // These calls can hang if opencode isn't fully bootstrapped yet | ||
| (async () => { | ||
| // Wire opencode state path and provider list — fire-and-forget to avoid blocking init. | ||
| // Callers await ensureProviderState() before getStatePath(). | ||
| const providerStateReady = (async () => { | ||
| try { | ||
| const { setStatePath, setConnectedProviders } = | ||
| await import("./services/ai/opencode-provider.js"); | ||
|
|
@@ -196,6 +207,7 @@ export const OpenCodeMemPlugin: Plugin = async (ctx: PluginInput) => { | |
| log("Failed to initialize opencode provider state", { error: String(error) }); | ||
| } | ||
| })(); | ||
| setProviderStateInit(providerStateReady); | ||
|
|
||
| if (isConfigured() && CONFIG.webServerEnabled) { | ||
| startWebServer({ | ||
|
|
@@ -247,12 +259,13 @@ export const OpenCodeMemPlugin: Plugin = async (ctx: PluginInput) => { | |
| // Start background memory scoring recalculation | ||
| if (isConfigured() && CONFIG.memoryScoring.enabled) { | ||
| startScoringRecalculation(); | ||
| // Run one-time recalculation on startup to ensure existing memories are scored | ||
| try { | ||
| recalculateAllScores(true); | ||
| } catch (error) { | ||
| log("Initial scoring recalculation failed", { error: String(error) }); | ||
| } | ||
| void Promise.resolve().then(() => { | ||
| try { | ||
| recalculateAllScores(true); | ||
|
Comment on lines
+262
to
+264
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Score recalculation still blocks startup
Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified and fixed in #66 (
Comment on lines
+262
to
+264
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in #66 ( |
||
| } catch (error) { | ||
| log("Initial scoring recalculation failed", { error: String(error) }); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| // Start memory lifecycle job (STM/LTM decay, promotion, archiving) | ||
|
|
@@ -271,6 +284,10 @@ export const OpenCodeMemPlugin: Plugin = async (ctx: PluginInput) => { | |
| const shutdownHandler = async () => { | ||
| delete (globalThis as any)[Symbol.for("opencode-mem0.shutdown")]; | ||
| try { | ||
| for (const timer of sessionIdleTimers.values()) { | ||
| clearTimeout(timer); | ||
| } | ||
| sessionIdleTimers.clear(); | ||
|
Comment on lines
+287
to
+290
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Running idle work survives disposal Once an idle callback starts, Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Partially addressed in #66 ( |
||
| stopScoringRecalculation(); | ||
| stopLifecycleJob(); | ||
| clearInterval(sessionCleanupTimer); | ||
|
|
@@ -596,7 +613,10 @@ export const OpenCodeMemPlugin: Plugin = async (ctx: PluginInput) => { | |
| if (!args.memoryId) | ||
| return JSON.stringify({ success: false, error: "memoryId required" }); | ||
| const delRes = await memoryClient.deleteMemory(args.memoryId); | ||
| return JSON.stringify({ success: delRes.success, message: "Memory removed" }); | ||
| return JSON.stringify({ | ||
| success: delRes.success, | ||
| message: delRes.success ? "Memory removed" : delRes.error || "Memory removal failed", | ||
| }); | ||
| } | ||
|
|
||
| try { | ||
|
|
@@ -634,6 +654,7 @@ export const OpenCodeMemPlugin: Plugin = async (ctx: PluginInput) => { | |
| await handleSessionCompacted(event, ctx, directory); | ||
| } | ||
| }, | ||
| dispose: shutdownHandler, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Late web server survives disposal When disposal precedes Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified and fixed in #66 ( |
||
| }; | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,9 +131,12 @@ export async function performAutoCapture( | |
| isCapturing = true; | ||
| let claimedPromptId: string | null = null; | ||
| try { | ||
| const { ensureProviderState } = await import("./ai/opencode-provider.js"); | ||
| await ensureProviderState(); | ||
|
Comment on lines
+134
to
+135
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified and fixed in #66 (
Comment on lines
+134
to
+135
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in #66 ( |
||
| const prompt = userPromptManager.getLastUncapturedPrompt(sessionID); | ||
| if (!prompt) return; | ||
| if (!userPromptManager.claimPrompt(prompt.id)) return; | ||
| claimedPromptId = prompt.id; | ||
| const maxRetries = CONFIG.autoCaptureMaxRetries ?? 3; | ||
| const existingAttempts = userPromptManager.getCaptureAttempts(prompt.id); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,6 +101,8 @@ export async function performUserProfileLearning( | |
|
|
||
| isLearningRunning = true; | ||
| try { | ||
| const { ensureProviderState } = await import("./ai/opencode-provider.js"); | ||
| await ensureProviderState(); | ||
|
Comment on lines
+104
to
+105
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified and fixed in #66 ( |
||
| const threshold = CONFIG.userProfileAnalysisInterval; | ||
| const maxBatches = CONFIG.userProfileMaxBatchesPerIdle; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a cold local-embedding startup, the factory can now expose
chat.messagewhile this warmup is still pending. That hook callssearchMemories(), whoseembedWithTimeout()awaits the in-progressembeddingService.warmup()before reaching signal-aware work, so its AbortController cannot interrupt model initialization. The first user prompt can therefore stall until the model load eventually finishes—potentially beyondwarmupTimeoutMs—rather than using the prior bounded startup degradation path.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified and fixed in #66 (
c78740b): the warmup wait insideembed()is now raced againstwarmupTimeoutMsand the caller's AbortSignal; it rejects AbortError-shaped so an already-running model load isn't re-triggered and the service isn't permanently disabled.searchMemoriesdegrades to text-only for that prompt instead of stalling it. Tests:tests/warmup-bound.test.ts.