Skip to content

Commit e04a61f

Browse files
committed
fix: address review follow-up edge cases
1 parent ef76833 commit e04a61f

10 files changed

Lines changed: 107 additions & 9 deletions

src/handlers/event.test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -785,7 +785,7 @@ describe("event handler", () => {
785785
);
786786
});
787787

788-
it("retries context-limit lookup after a transient provider failure", async () => {
788+
it("caches the default context-limit after a transient provider failure", async () => {
789789
const sessionManager = new MockSessionManager();
790790
const state = sessionManager.createDefaultState("group-1", "user-1");
791791
sessionManager.setState("session-1", state);
@@ -846,8 +846,8 @@ describe("event handler", () => {
846846
});
847847
await flushPromises();
848848

849-
assertEquals(providerCalls, 2);
850-
assertEquals(state.contextLimit, 123_456);
849+
assertEquals(providerCalls, 1);
850+
assertEquals(state.contextLimit, 200_000);
851851
});
852852

853853
it("records supported non-special events into the hot-tier log for main sessions", async () => {

src/services/connection-manager.test.ts

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,32 @@ describe("connection manager", () => {
419419
assertEquals(startError.state, "stopped");
420420
});
421421

422+
it("preserves the closing state when start is called during shutdown", async () => {
423+
const closeGate = deferred<void>();
424+
const manager = new GraphitiConnectionManager({
425+
endpoint: "http://test",
426+
connectionFactory: () => ({
427+
connect: () => Promise.resolve(),
428+
close: () => closeGate.promise,
429+
callTool: () => Promise.resolve({ ok: true }),
430+
}),
431+
});
432+
433+
manager.start();
434+
assertEquals(await manager.ready(10), true);
435+
436+
const stopPromise = manager.stop();
437+
const startError = assertThrows(
438+
() => manager.start(),
439+
GraphitiOfflineError,
440+
);
441+
442+
assertEquals(startError.state, "closing");
443+
444+
closeGate.resolve();
445+
await stopPromise;
446+
});
447+
422448
it("surfaces typed errors after failed retry", async () => {
423449
let connectionIndex = 0;
424450
const manager = new GraphitiConnectionManager({

src/services/connection-manager.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -250,8 +250,10 @@ export class GraphitiConnectionManager implements GraphitiToolCaller {
250250
start(): void {
251251
if (this.state === "closing" || this.state === "stopped") {
252252
throw new GraphitiOfflineError(
253-
"stopped",
254-
"Graphiti connection manager has been stopped and cannot be restarted",
253+
this.state,
254+
this.state === "closing"
255+
? "Graphiti connection manager is closing"
256+
: "Graphiti connection manager has been stopped and cannot be restarted",
255257
);
256258
}
257259
if (this.started) return;

src/services/context-limit.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ export async function resolveContextLimit(
3333
}
3434
} catch (err) {
3535
logger.warn("Failed to fetch provider context limit", err);
36+
cache.set(modelKey, DEFAULT_CONTEXT_LIMIT);
37+
return DEFAULT_CONTEXT_LIMIT;
3638
}
3739

3840
return DEFAULT_CONTEXT_LIMIT;

src/services/graphiti-mcp.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,4 +37,24 @@ describe("GraphitiMcpClient", () => {
3737

3838
assertEquals(error.state, "stopped");
3939
});
40+
41+
it("marks unexpected search node failures as degraded", async () => {
42+
const client = new GraphitiMcpClient({
43+
start() {},
44+
stop() {
45+
return Promise.resolve();
46+
},
47+
ready() {
48+
return Promise.resolve(true);
49+
},
50+
callTool() {
51+
return Promise.reject(new Error("boom"));
52+
},
53+
});
54+
55+
assertEquals(await client.searchNodesWithStatus({ query: "test" }), {
56+
nodes: [],
57+
degraded: true,
58+
});
59+
});
4060
});

src/services/graphiti-mcp.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,7 @@ export class GraphitiMcpClient {
221221
return { nodes: [], degraded: true };
222222
}
223223
logger.error("searchNodes error", err);
224-
return { nodes: [], degraded: false };
224+
return { nodes: [], degraded: true };
225225
}
226226
}
227227

src/services/logger.test.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,28 @@ describe("logger", () => {
170170
});
171171
});
172172

173+
it("falls back to console.warn when structured warn scheduling throws", async () => {
174+
setSuppressConsoleWarningsDuringTestsOverride(false);
175+
setWarningTaskScheduler(() => {
176+
throw new Error("schedule failed");
177+
});
178+
setOpenCodeClient({
179+
app: {
180+
log: () => Promise.resolve(),
181+
},
182+
});
183+
184+
const { logger } = await import("./logger.ts");
185+
logger.warn("warning", { code: 42 });
186+
187+
assertEquals(consoleWarnSpy.calls.length, 1);
188+
assertEquals(consoleWarnSpy.calls[0].args, [
189+
"[graphiti]",
190+
"warning",
191+
{ code: 42 },
192+
]);
193+
});
194+
173195
it("should forward multiple arguments to error", async () => {
174196
const { logger } = await import("./logger.ts");
175197
const error = new Error("test");

src/services/logger.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,11 @@ export const logger = {
8888
warn: (...args: unknown[]) => {
8989
if (silentOverride) return;
9090
const payload = toWarningPayload(args);
91-
if (logStructuredWarning(payload.message, payload.extra)) return;
91+
try {
92+
if (logStructuredWarning(payload.message, payload.extra)) return;
93+
} catch {
94+
// Fall back to console below when structured warning scheduling fails.
95+
}
9296
if (shouldSuppressConsoleWarningsDuringTests()) return;
9397
console.warn(PREFIX, ...args);
9498
},

src/services/redis-cache.test.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { assertEquals } from "jsr:@std/assert@^1.0.0";
22
import { describe, it } from "jsr:@std/testing@^1.0.0/bdd";
33
import { RedisCacheService } from "./redis-cache.ts";
44
import { RedisClient } from "./redis-client.ts";
5+
import { memoryCacheMetaKey } from "./redis-events.ts";
56

67
describe("redis cache", () => {
78
it("stores cache entries per group without leaking across groups", async () => {
@@ -144,4 +145,22 @@ describe("redis cache", () => {
144145
lastRefresh: (await cache.get("group-1"))?.refreshedAt,
145146
});
146147
});
148+
149+
it("preserves a string lastRefresh value of 0 in metadata", async () => {
150+
const redis = new RedisClient({ endpoint: "redis://unused" });
151+
const cache = new RedisCacheService(redis, {
152+
ttlSeconds: 300,
153+
driftThreshold: 0.5,
154+
});
155+
156+
await redis.setHashFields(memoryCacheMetaKey("group-1"), {
157+
lastQuery: "query",
158+
lastRefresh: "0",
159+
}, 300);
160+
161+
assertEquals(await cache.getMeta("group-1"), {
162+
lastQuery: "query",
163+
lastRefresh: 0,
164+
});
165+
});
147166
});

src/services/redis-cache.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,10 +98,13 @@ export class RedisCacheService {
9898
const raw = await this.redis.getHashAll(memoryCacheMetaKey(groupId));
9999
if (Object.keys(raw).length === 0) return null;
100100

101+
const hasLastRefresh = Object.hasOwn(raw, "lastRefresh");
102+
const parsedLastRefresh = hasLastRefresh ? Number(raw.lastRefresh) : NaN;
103+
101104
return {
102105
lastQuery: raw.lastQuery?.trim() || undefined,
103-
lastRefresh: raw.lastRefresh && Number.isFinite(Number(raw.lastRefresh))
104-
? Number(raw.lastRefresh)
106+
lastRefresh: Number.isFinite(parsedLastRefresh)
107+
? parsedLastRefresh
105108
: undefined,
106109
};
107110
}

0 commit comments

Comments
 (0)