-
Notifications
You must be signed in to change notification settings - Fork 89
fix(memory): guard Qdrant singleton against non-default port overwrite #27459
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
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 |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ | |
|
|
||
| import { getConfig } from "../../config/loader.js"; | ||
| import { initializeDb } from "../db-init.js"; | ||
| import { initQdrantClient } from "../qdrant-client.js"; | ||
| import { initQdrantClient, resolveQdrantUrl } from "../qdrant-client.js"; | ||
| import { | ||
| countNodes, | ||
| getEdgesForNode, | ||
|
|
@@ -29,7 +29,7 @@ initializeDb(); | |
| const config = getConfig(); | ||
| try { | ||
|
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. As part of OOS'ing
Contributor
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. Done — restored the try/catch here and in |
||
| initQdrantClient({ | ||
| url: config.memory.qdrant.url ?? "http://127.0.0.1:6333", | ||
| url: resolveQdrantUrl(config), | ||
| collection: config.memory.qdrant.collection, | ||
| vectorSize: config.memory.qdrant.vectorSize, | ||
| onDisk: config.memory.qdrant.onDisk ?? true, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| import { afterEach, beforeEach, describe, expect, test } from "bun:test"; | ||
|
|
||
| import { applyNestedDefaults } from "../config/loader.js"; | ||
| import type { AssistantConfig } from "../config/types.js"; | ||
| import { resolveQdrantUrl } from "./qdrant-client.js"; | ||
|
|
||
| const DEFAULT_CONFIG: AssistantConfig = applyNestedDefaults({}); | ||
|
|
||
| describe("resolveQdrantUrl", () => { | ||
| const savedPort = process.env.QDRANT_HTTP_PORT; | ||
| const savedUrl = process.env.QDRANT_URL; | ||
|
|
||
| beforeEach(() => { | ||
| delete process.env.QDRANT_HTTP_PORT; | ||
| delete process.env.QDRANT_URL; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| if (savedPort === undefined) delete process.env.QDRANT_HTTP_PORT; | ||
| else process.env.QDRANT_HTTP_PORT = savedPort; | ||
| if (savedUrl === undefined) delete process.env.QDRANT_URL; | ||
| else process.env.QDRANT_URL = savedUrl; | ||
| }); | ||
|
|
||
| test("falls back to config when no env vars are set", () => { | ||
| expect(resolveQdrantUrl(DEFAULT_CONFIG)).toBe("http://127.0.0.1:6333"); | ||
| }); | ||
|
|
||
| test("honours QDRANT_URL when set", () => { | ||
| process.env.QDRANT_URL = "http://qdrant.example.com:6333"; | ||
| expect(resolveQdrantUrl(DEFAULT_CONFIG)).toBe( | ||
| "http://qdrant.example.com:6333", | ||
| ); | ||
| }); | ||
|
|
||
| test("QDRANT_HTTP_PORT wins over QDRANT_URL", () => { | ||
| process.env.QDRANT_URL = "http://qdrant.example.com:6333"; | ||
| process.env.QDRANT_HTTP_PORT = "20200"; | ||
| expect(resolveQdrantUrl(DEFAULT_CONFIG)).toBe("http://127.0.0.1:20200"); | ||
| }); | ||
|
|
||
| test("QDRANT_HTTP_PORT wins over config default", () => { | ||
| process.env.QDRANT_HTTP_PORT = "20200"; | ||
| expect(resolveQdrantUrl(DEFAULT_CONFIG)).toBe("http://127.0.0.1:20200"); | ||
| }); | ||
|
|
||
| test("respects a non-default config URL when no env is set", () => { | ||
| const config: AssistantConfig = { | ||
| ...DEFAULT_CONFIG, | ||
| memory: { | ||
| ...DEFAULT_CONFIG.memory, | ||
| qdrant: { | ||
| ...DEFAULT_CONFIG.memory.qdrant, | ||
| url: "http://custom-host:9999", | ||
| }, | ||
| }, | ||
| }; | ||
| expect(resolveQdrantUrl(config)).toBe("http://custom-host:9999"); | ||
| }); | ||
| }); |
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.
I would've expected
qdrant.urlto be"http://127.0.0.1:20200"in the workspace config if"resources.qdrantPort": 20200in the lock fileThere 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.
Good catch. The CLI does not currently write
memory.qdrant.urlfrom the lock file —cli/src/lib/local.tssetsenv.QDRANT_HTTP_PORTwhen spawning the daemon but never materializes the port into the workspace config. That is whyresolveQdrantUrlhas to fall back to the env var.Treating that as a follow-up: the CLI should either (a) update
memory.qdrant.urlin the workspace config wheneverresources.qdrantPortchanges, or (b) removememory.qdrant.urlfrom the static config entirely and make the resolved URL derive solely from allocated resources + env overrides. Option (b) is probably cleaner — the static config default has caused divergence more than once.Happy to open a separate ticket/PR for that once this centralization lands.