fix(plugin): lifecycle correctness — dispose hook, claim release, startup races - #62
Conversation
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 3 potential issues.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| await handleSessionCompacted(event, ctx, directory); | ||
| } | ||
| }, | ||
| dispose: shutdownHandler, |
There was a problem hiding this comment.
🔴 Late web server survives disposal
When disposal precedes startWebServer completion, shutdownHandler sees no server to stop. The completion later installs a live server owned by the disposed plugin.
Prompt for agents
Coordinate asynchronous web-server startup with plugin disposal in src/index.ts. Track a disposed state or the startup promise. If disposal happens before startWebServer resolves, stop the returned server immediately and suppress its callbacks and toasts. Ensure shutdown also waits for or safely settles pending startup without allowing a disposed instance to become owner.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Verified and fixed in #66 (f20a844): the startWebServer().then handler now checks the disposed flag — a server resolving after disposal is stopped immediately and never assigned. Regression-tested in tests/disposal-lifecycle.test.ts.
| for (const timer of sessionIdleTimers.values()) { | ||
| clearTimeout(timer); | ||
| } | ||
| sessionIdleTimers.clear(); |
There was a problem hiding this comment.
🔴 Running idle work survives disposal
Once an idle callback starts, clearTimeout cannot cancel it. It continues captures and maintenance after memoryClient.close(), allowing disposed instances to write or reopen databases.
Prompt for agents
Add lifecycle cancellation for in-flight idle processing in src/index.ts, not only pending timeout handles. Track a disposed flag or AbortController per plugin instance. Check cancellation between awaited idle stages and before every post-disposal database operation. Make shutdown wait for active idle work to settle before closing memoryClient, or abort that work and then close connections.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Partially addressed in #66 (f20a844): the event handler gains an entry checkpoint and the capture/learning pipelines check again after their first await, so no NEW work starts post-dispose. Fully cancelling in-flight work mid-await (AbortController plumbing through capture) was left out — entry checkpoints plus the bounded provider-state wait cover the wedge scenarios; a full cancellation system is the follow-up if disposal-during-capture proves hot.
| void Promise.resolve().then(() => { | ||
| try { | ||
| recalculateAllScores(true); |
There was a problem hiding this comment.
🔴 Score recalculation still blocks startup
Promise.resolve().then runs synchronous recalculateAllScores before callers resume from plugin initialization. Large stores still block the host during startup.
Prompt for agents
Move initial score recalculation off the plugin-initialization microtask path. Schedule it on a later event-loop turn or use an asynchronous or worker-based implementation so the host can receive and use the plugin first. Integrate the scheduled job with disposal so it cannot start or continue after shutdown.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Verified and fixed in #66 (f20a844): the scan now runs on a setTimeout(0) macrotask (cleared on dispose), so the host's await of the factory resolves before the shard scan executes — the DX comment was right that the microtask ran first.
There was a problem hiding this comment.
🟡 Changes recommended
ensureProviderState() is awaited unconditionally in paths that can use the external provider, which can deadlock idle processing/auto-capture if provider-state init hangs and leave global capture state stuck.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves plugin lifecycle correctness and startup behavior by adding a proper dispose hook, fixing auto-capture claim handling, preventing warmup race rejections, and coordinating opencode provider-state initialization to avoid startup races.
Changes:
- Add provider-state initialization gating (
setProviderStateInit/ensureProviderState) and update lifecycle paths to wait for provider state before usinggetStatePath(). - Fix auto-capture prompt-claim release on early returns and add tests to prevent regressions.
- Make warmup and initial score recalculation non-blocking, ensure warmup timeout timer is cleared, and implement
disposeto clean up timers/jobs/server/sqlite.
File summaries
| File | Description |
|---|---|
| tests/user-memory-learning.test.ts | Updates provider mock to include ensureProviderState. |
| tests/transcript-idle-wiring.test.ts | Adds coverage for dispose clearing idle timers and shutdown behavior. |
| tests/tool-scope.test.ts | Adds regression test ensuring memory.forget reports real deletion failures. |
| tests/plugin-error-handling.test.ts | Adds tests for warmup timeout race cleanup and non-blocking factory return. |
| tests/memory-conflicts.test.ts | Updates provider mock to include ensureProviderState. |
| tests/chat-message-mode.test.ts | Updates provider mock to include provider-state init hooks. |
| tests/auto-capture.test.ts | Adds regression tests for claim release and provider-state gating. |
| src/services/user-memory-learning.ts | Awaits provider-state initialization before profile learning run. |
| src/services/memory-conflicts.ts | Awaits provider-state initialization before opencode contradiction checks. |
| src/services/auto-capture.ts | Awaits provider-state initialization and fixes prompt-claim handling. |
| src/services/ai/opencode-provider.ts | Introduces provider-state init promise + ensureProviderState API. |
| src/index.ts | Implements dispose, makes warmup/scoring non-blocking, wires provider-state init, improves forget failure messaging. |
| docs/CHANGELOG.md | Documents the lifecycle/startup fixes in Unreleased. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const { ensureProviderState } = await import("./ai/opencode-provider.js"); | ||
| await ensureProviderState(); |
There was a problem hiding this comment.
Verified and fixed in #66 (f20a844): ensureProviderState() is now bounded (10s warn-and-proceed), so a hung host bootstrap can no longer wedge the capture mutex; a disposed-plugin checkpoint also backs out before claiming a prompt.
| const { ensureProviderState } = await import("./ai/opencode-provider.js"); | ||
| await ensureProviderState(); |
There was a problem hiding this comment.
Verified and fixed in #66 (f20a844) — the bounded ensureProviderState() covers this path (10s warn-and-proceed), plus a post-await disposed check before profile learning proceeds.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcfe834b54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let timeoutId: ReturnType<typeof setTimeout> | undefined; | ||
| try { | ||
| await Promise.race([ | ||
| memoryClient.warmup(), |
There was a problem hiding this comment.
On a cold local-embedding startup, the factory can now expose chat.message while this warmup is still pending. That hook calls searchMemories(), whose embedWithTimeout() awaits the in-progress embeddingService.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 beyond warmupTimeoutMs—rather than using the prior bounded startup degradation path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified and fixed in #66 (c78740b): the warmup wait inside embed() is now raced against warmupTimeoutMs and 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. searchMemories degrades to text-only for that prompt instead of stalling it. Tests: tests/warmup-bound.test.ts.
| const { ensureProviderState } = await import("./ai/opencode-provider.js"); | ||
| await ensureProviderState(); |
There was a problem hiding this comment.
If ctx.client.path.get() or provider.list() stalls during host bootstrap, providerStateReady never settles because its initialization has no timeout. A configuration using only memoryModel/memoryApiUrl does not need OpenCode state, but this unconditional await leaves isCapturing true forever; every later idle cycle then returns at the mutex check and automatic capture stops for the rest of the process. Limit this wait to the OpenCode-provider path (and apply the same distinction to profile learning) or bound the state initialization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #66 (f20a844) — bound applied to the OpenCode-provider path only where needed (all paths, via the shared 10s race), plus the disposed guard before claiming.
| void Promise.resolve().then(() => { | ||
| try { | ||
| recalculateAllScores(true); |
There was a problem hiding this comment.
recalculateAllScores() is synchronous and can scan every memory shard. Queueing it in a promise microtask does not make startup non-blocking: that microtask is already queued before the async plugin factory resolves to its awaiting caller, so it runs before the host can continue after await OpenCodeMemPlugin(...). With scoring enabled, large stores therefore still block plugin startup despite the new background-startup behavior and changelog entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #66 (f20a844): setTimeout(0) macrotask instead of the microtask — the host resumes first, changelog claim now true.
…score scan Code-review findings on #62 (Devin, Copilot, Codex): - ensureProviderState() had no timeout: a hung host bootstrap wedged auto-capture (isCapturing stuck true) and profile learning forever. Now bounded at 10s with a warn-and-proceed fallback. - startWebServer().then assigned webServer with no disposal guard: a server finishing after dispose ran orphaned with live toasts. Now stopped immediately on late resolution. - Idle events arriving after dispose scheduled new capture work; the event handler and capture/learning pipelines now check disposed state. - Initial recalculateAllScores ran on a Promise.resolve() microtask, which executes before the host resumes from awaiting the plugin factory — the startup-blocking claim was wrong. Now a setTimeout(0) macrotask, cleared on dispose.
Codex P1 on #62: embed() awaited warmup() with no bound — the AbortController in embedWithTimeout only guarded the API-fetch path, so a slow model download stalled the first chat.message hook past any configured timeout (and an abort during warmup left the service permanently disabled via embeddingAvailable=false). - warmup wait is raced against warmupTimeoutMs and the caller's AbortSignal; on either it rejects AbortError-shaped, which embed()'s catch rethrows without disabling the service. - searchMemories treats a warmup-pending AbortError as a degraded text-only search instead of propagating the failure to the prompt.
Summary
Full codebase audit batch 1/3 (plugin lifecycle). Every fix ships with a regression test that fails without it. Gates green:
format:check+typecheck+ 827 tests passed (baseline 821) +build.fix(auto-capture)captured=2forever when capture skipped after claiming (thefinallyreset was dead code:claimedPromptIdwas only assigned from a function that returnsnullon both skip and success)fix(plugin)disposehook. Cleanup previously lived only in aglobalThisstash the host never calls; timers, scoring, lifecycle jobs, the web server and sqlite connections survived reloadfix(plugin)Promise.racetimer cleared infinally— a late-losing timer rejected an orphan promisefix(plugin)await ensureProviderState()instead of racing the fire-and-forget provider-state init at startupfix(tools)memory.forgettool reports actual delete failures instead of always"Memory removed"perf(plugin)warmup()and initial score recalculation are fire-and-forget; the plugin object returns immediately (config init stays synchronous)Verification
bun run format:check && bun run typecheck && bun run test && bun run build— all green (827 passed / 1 skipped)Notes
shutdownHandlerglobalThis stash retained for CLI consumers —disposewraps the same handler.