-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix(openclaw-plugin): prevent duplicate plugin registration #955
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
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 |
|---|---|---|
| @@ -0,0 +1,154 @@ | ||
| import { describe, it, expect, vi, beforeEach } from "vitest"; | ||
|
|
||
| // Mock the modules before importing the plugin | ||
| vi.mock("./config.js", () => ({ | ||
| memoryOpenVikingConfigSchema: { | ||
| parse: vi.fn((config) => ({ | ||
| mode: "remote", | ||
| baseUrl: "http://localhost:8000", | ||
| apiKey: "test-key", | ||
| agentId: "test-agent", | ||
| targetUri: "viking://user/memories", | ||
| recallLimit: 5, | ||
| recallScoreThreshold: 0.7, | ||
| autoRecall: true, | ||
| autoCapture: true, | ||
| captureMode: "semantic", | ||
| captureMaxLength: 1000, | ||
| timeoutMs: 30000, | ||
| ...config, | ||
| })), | ||
| }, | ||
| })); | ||
|
|
||
| vi.mock("./client.js", () => ({ | ||
| OpenVikingClient: vi.fn().mockImplementation(() => ({ | ||
| healthCheck: vi.fn().mockResolvedValue(undefined), | ||
| find: vi.fn().mockResolvedValue({ memories: [] }), | ||
| read: vi.fn().mockResolvedValue(""), | ||
| addSessionMessage: vi.fn().mockResolvedValue(undefined), | ||
| commitSession: vi.fn().mockResolvedValue({ archived: true, memories_extracted: 0 }), | ||
| deleteSession: vi.fn().mockResolvedValue(undefined), | ||
| deleteUri: vi.fn().mockResolvedValue(undefined), | ||
| getSession: vi.fn().mockResolvedValue({ message_count: 0 }), | ||
| })), | ||
| localClientCache: new Map(), | ||
| localClientPendingPromises: new Map(), | ||
| isMemoryUri: vi.fn((uri) => uri?.startsWith("viking://")), | ||
| })); | ||
|
|
||
| vi.mock("./process-manager.js", () => ({ | ||
| IS_WIN: false, | ||
| waitForHealth: vi.fn().mockResolvedValue(undefined), | ||
| quickRecallPrecheck: vi.fn().mockResolvedValue({ ok: true }), | ||
| withTimeout: vi.fn((promise) => promise), | ||
| resolvePythonCommand: vi.fn().mockReturnValue("python3"), | ||
| prepareLocalPort: vi.fn().mockResolvedValue(8000), | ||
| })); | ||
|
|
||
| // Import the plugin after mocking | ||
| // We need to re-import to get a fresh instance with the guard reset | ||
| async function importPlugin() { | ||
| const module = await import("./index.js"); | ||
|
||
| return module.default; | ||
| } | ||
|
|
||
| describe("Plugin Registration Guard (Issue #948)", () => { | ||
| let mockApi: { | ||
| pluginConfig: Record<string, unknown>; | ||
| logger: { | ||
| info: ReturnType<typeof vi.fn>; | ||
| warn: ReturnType<typeof vi.fn>; | ||
| error: ReturnType<typeof vi.fn>; | ||
| debug?: ReturnType<typeof vi.fn>; | ||
| }; | ||
| registerTool: ReturnType<typeof vi.fn>; | ||
| registerService: ReturnType<typeof vi.fn>; | ||
| registerContextEngine?: ReturnType<typeof vi.fn>; | ||
| on: ReturnType<typeof vi.fn>; | ||
| }; | ||
|
|
||
| beforeEach(() => { | ||
| mockApi = { | ||
| pluginConfig: {}, | ||
| logger: { | ||
| info: vi.fn(), | ||
| warn: vi.fn(), | ||
| error: vi.fn(), | ||
| debug: vi.fn(), | ||
| }, | ||
| registerTool: vi.fn(), | ||
| registerService: vi.fn(), | ||
| registerContextEngine: vi.fn(), | ||
| on: vi.fn(), | ||
| }; | ||
| }); | ||
|
|
||
| it("should register the plugin on first call", async () => { | ||
| const plugin = await importPlugin(); | ||
| plugin.register(mockApi); | ||
|
|
||
| // Should have registered tools | ||
| expect(mockApi.registerTool).toHaveBeenCalled(); | ||
| // Should have registered service | ||
| expect(mockApi.registerService).toHaveBeenCalled(); | ||
| // Should NOT have logged the skip message | ||
| expect(mockApi.logger.info).not.toHaveBeenCalledWith( | ||
| expect.stringContaining("already registered") | ||
| ); | ||
| }); | ||
|
|
||
| it("should skip duplicate registration on subsequent calls", async () => { | ||
| const plugin = await importPlugin(); | ||
|
|
||
| // First registration | ||
| plugin.register(mockApi); | ||
| const firstCallCount = mockApi.registerTool.mock.calls.length; | ||
|
|
||
| // Reset mock to track second call | ||
| mockApi.registerTool.mockClear(); | ||
| mockApi.registerService.mockClear(); | ||
|
|
||
| // Second registration attempt (simulating the duplicate registration bug) | ||
| plugin.register(mockApi); | ||
|
|
||
| // Should have logged the skip message | ||
| expect(mockApi.logger.info).toHaveBeenCalledWith( | ||
| "openviking: plugin already registered, skipping duplicate registration" | ||
| ); | ||
|
|
||
| // Should NOT have registered tools or service again | ||
| expect(mockApi.registerTool).not.toHaveBeenCalled(); | ||
| expect(mockApi.registerService).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("should allow re-registration after stop is called", async () => { | ||
| const plugin = await importPlugin(); | ||
|
|
||
| // First registration | ||
| plugin.register(mockApi); | ||
|
|
||
| // Get the registered service | ||
| const serviceCall = mockApi.registerService.mock.calls[0]; | ||
| const service = serviceCall[0]; | ||
|
|
||
| // Reset mocks | ||
| mockApi.registerTool.mockClear(); | ||
| mockApi.registerService.mockClear(); | ||
|
|
||
| // Call stop (this should reset the guard) | ||
| service.stop(); | ||
|
|
||
| // Try to register again | ||
| plugin.register(mockApi); | ||
|
|
||
| // Should NOT have logged the skip message | ||
| expect(mockApi.logger.info).not.toHaveBeenCalledWith( | ||
| expect.stringContaining("already registered") | ||
| ); | ||
|
|
||
| // Should have registered tools and service again | ||
| expect(mockApi.registerTool).toHaveBeenCalled(); | ||
| expect(mockApi.registerService).toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,10 @@ import { | |
| import { createMemoryOpenVikingContextEngine } from "./context-engine.js"; | ||
| import type { ContextEngineWithSessionMapping } from "./context-engine.js"; | ||
|
|
||
| // Module-level guard to prevent duplicate plugin registrations within the same process. | ||
| // This addresses issue #948 where the plugin registers repeatedly every minute. | ||
| let hasPluginBeenRegistered = false; | ||
|
|
||
| type PluginLogger = { | ||
| debug?: (message: string) => void; | ||
| info: (message: string) => void; | ||
|
|
@@ -80,6 +84,13 @@ const contextEnginePlugin = { | |
| configSchema: memoryOpenVikingConfigSchema, | ||
|
|
||
| register(api: OpenClawPluginApi) { | ||
| // Guard against duplicate plugin registration (issue #948) | ||
| if (hasPluginBeenRegistered) { | ||
| api.logger.info("openviking: plugin already registered, skipping duplicate registration"); | ||
| return; | ||
| } | ||
| hasPluginBeenRegistered = true; | ||
|
||
|
|
||
| const cfg = memoryOpenVikingConfigSchema.parse(api.pluginConfig); | ||
| const localCacheKey = `${cfg.mode}:${cfg.baseUrl}:${cfg.configPath}:${cfg.apiKey}`; | ||
|
|
||
|
|
@@ -720,6 +731,8 @@ const contextEnginePlugin = { | |
| } else { | ||
| api.logger.info("openviking: stopped"); | ||
| } | ||
| // Reset the registration guard to allow clean re-registration after stop | ||
| hasPluginBeenRegistered = false; | ||
| }, | ||
| }); | ||
| }, | ||
|
|
||
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.
[Bug] (blocking) These mocks point at ./config.js, ./client.js, and ./process-manager.js, but this test file lives under tests, so ./... resolves inside examples/openclaw-plugin/tests rather than the plugin root. The same applies to import("./index.js") below. As written, this test is not targeting the real plugin module. The existing tests in this package use ../index.js style imports for this reason.