diff --git a/apps/memos-local-plugin/core/storage/repos/_helpers.ts b/apps/memos-local-plugin/core/storage/repos/_helpers.ts index 6286074cd..d4b3d8cc2 100644 --- a/apps/memos-local-plugin/core/storage/repos/_helpers.ts +++ b/apps/memos-local-plugin/core/storage/repos/_helpers.ts @@ -46,16 +46,34 @@ export function nullable(v: T | undefined): T | null { return v === undefined ? null : v; } +/** + * Hard upper bound for explicitly requested page sizes. Callers that need + * bulk windows (e.g. `metrics()` loads 10_000 traces for the viewer's + * "memory writes per day" chart, world-model maintenance scans 100_000 + * rows) must actually receive what they ask for. The previous + * `Math.min(n, 500)` silently rewrote every explicit request to the + * default page size, which truncated the viewer's analytics to the newest + * 500 traces and undercounted `sessions` / `embeddings` (#2401). + * + * This ceiling is for trusted in-process bulk reads. Untrusted input + * (HTTP query params, agent tool args) must be bounded at the API + * boundary — see `MAX_HTTP_PAGE_LIMIT` in `server/routes/params.ts`. + */ +export const MAX_QUERY_LIMIT = 100_000; + +/** Default page size when a caller omits `limit`. */ +export const DEFAULT_PAGE_SIZE = 500; + export function buildPageClauses(opts: PageOptions | undefined, tsColumn: string): string { const newestFirst = opts?.newestFirst !== false; - const limit = clampLimit(opts?.limit ?? 500); + const limit = clampLimit(opts?.limit ?? DEFAULT_PAGE_SIZE); const offset = Math.max(opts?.offset ?? 0, 0); return `ORDER BY ${tsColumn} ${newestFirst ? "DESC" : "ASC"} LIMIT ${limit} OFFSET ${offset}`; } export function clampLimit(n: number): number { - if (!Number.isFinite(n) || n <= 0) return 500; - return Math.min(Math.trunc(n), 500); + if (!Number.isFinite(n) || n <= 0) return DEFAULT_PAGE_SIZE; + return Math.min(Math.trunc(n), MAX_QUERY_LIMIT); } export function timeRangeWhere( diff --git a/apps/memos-local-plugin/server/routes/api-logs.ts b/apps/memos-local-plugin/server/routes/api-logs.ts index 6738cc138..fe5d57701 100644 --- a/apps/memos-local-plugin/server/routes/api-logs.ts +++ b/apps/memos-local-plugin/server/routes/api-logs.ts @@ -14,15 +14,14 @@ * { logs: ApiLogDTO[], total: number, limit, offset, nextOffset? } */ import type { ServerDeps } from "../types.js"; +import { parseLimitParam } from "./params.js"; import type { Routes } from "./registry.js"; export function registerApiLogsRoutes(routes: Routes, deps: ServerDeps): void { routes.set("GET /api/v1/api-logs", async (ctx) => { const params = ctx.url.searchParams; - const parsedLimit = Number(params.get("limit")); + const limit = parseLimitParam(params.get("limit")); const parsedOffset = Number(params.get("offset")); - const limit = - Number.isFinite(parsedLimit) && parsedLimit > 0 ? parsedLimit : 50; const offset = Number.isFinite(parsedOffset) && parsedOffset >= 0 ? parsedOffset : 0; const tool = params.get("tool") || undefined; diff --git a/apps/memos-local-plugin/server/routes/params.ts b/apps/memos-local-plugin/server/routes/params.ts new file mode 100644 index 000000000..0990e8ba5 --- /dev/null +++ b/apps/memos-local-plugin/server/routes/params.ts @@ -0,0 +1,24 @@ +/** + * Upper bound for a page size taken from UNTRUSTED input (HTTP query + * params, agent tool args). Storage-level `clampLimit` honors explicit + * limits up to `MAX_QUERY_LIMIT` for trusted in-process bulk reads + * (#2401), so every route that forwards a caller-supplied `limit` into a + * core/repo list call must cap it here first. 500 matches the effective + * ceiling these endpoints had before that change. + */ +export const MAX_HTTP_PAGE_LIMIT = 500; + +/** + * Parse a `limit` query parameter into a safe page size. + * + * - missing / non-numeric / non-positive → `fallback` (50 by default) + * - values above `MAX_HTTP_PAGE_LIMIT` are clamped to it + */ +export function parseLimitParam( + raw: string | null, + fallback = 50, +): number { + const parsed = Number(raw); + if (!Number.isFinite(parsed) || parsed <= 0) return fallback; + return Math.min(Math.trunc(parsed), MAX_HTTP_PAGE_LIMIT); +} diff --git a/apps/memos-local-plugin/server/routes/policies.ts b/apps/memos-local-plugin/server/routes/policies.ts index 14b236cf2..47e209bcb 100644 --- a/apps/memos-local-plugin/server/routes/policies.ts +++ b/apps/memos-local-plugin/server/routes/policies.ts @@ -21,6 +21,7 @@ */ import type { PolicyDTO } from "../../agent-contract/dto.js"; import type { ServerDeps } from "../types.js"; +import { parseLimitParam } from "./params.js"; import { parseJson, writeError, type Routes } from "./registry.js"; export function registerPoliciesRoutes(routes: Routes, deps: ServerDeps): void { @@ -28,10 +29,8 @@ export function registerPoliciesRoutes(routes: Routes, deps: ServerDeps): void { routes.set("GET /api/v1/policies", async (ctx) => { const params = ctx.url.searchParams; - const parsedLimit = Number(params.get("limit")); + const limit = parseLimitParam(params.get("limit")); const parsedOffset = Number(params.get("offset")); - const limit = - Number.isFinite(parsedLimit) && parsedLimit > 0 ? parsedLimit : 50; const offset = Number.isFinite(parsedOffset) && parsedOffset >= 0 ? parsedOffset : 0; const statusRaw = params.get("status"); @@ -267,10 +266,8 @@ export function registerPoliciesRoutes(routes: Routes, deps: ServerDeps): void { routes.set("GET /api/v1/world-models", async (ctx) => { const params = ctx.url.searchParams; - const parsedLimit = Number(params.get("limit")); + const limit = parseLimitParam(params.get("limit")); const parsedOffset = Number(params.get("offset")); - const limit = - Number.isFinite(parsedLimit) && parsedLimit > 0 ? parsedLimit : 50; const offset = Number.isFinite(parsedOffset) && parsedOffset >= 0 ? parsedOffset : 0; const q = params.get("q") || undefined; diff --git a/apps/memos-local-plugin/server/routes/session.ts b/apps/memos-local-plugin/server/routes/session.ts index 2bf3fc639..9ecee8793 100644 --- a/apps/memos-local-plugin/server/routes/session.ts +++ b/apps/memos-local-plugin/server/routes/session.ts @@ -17,6 +17,7 @@ import { parseTaskStatusFilter, } from "../../agent-contract/episode-status.js"; import type { ServerDeps } from "../types.js"; +import { parseLimitParam } from "./params.js"; import { parseJson, writeError, type Routes } from "./registry.js"; /** @@ -77,9 +78,8 @@ export function registerSessionRoutes(routes: Routes, deps: ServerDeps): void { const ownerProfileId = ctx.url.searchParams.get("ownerProfileId") || undefined; const q = (ctx.url.searchParams.get("q") || "").trim().toLowerCase(); const status = parseTaskStatusFilter(ctx.url.searchParams.get("status")); - const rawLimit = numberOrUndefined(ctx.url.searchParams.get("limit")); const rawOffset = numberOrUndefined(ctx.url.searchParams.get("offset")); - const limit = rawLimit && rawLimit > 0 ? rawLimit : 50; + const limit = parseLimitParam(ctx.url.searchParams.get("limit")); const offset = rawOffset && rawOffset >= 0 ? rawOffset : 0; // The legacy `?shape=ids` path is unaffected by `status` / diff --git a/apps/memos-local-plugin/server/routes/trace.ts b/apps/memos-local-plugin/server/routes/trace.ts index 7a8e742dd..96605554b 100644 --- a/apps/memos-local-plugin/server/routes/trace.ts +++ b/apps/memos-local-plugin/server/routes/trace.ts @@ -9,6 +9,7 @@ */ import type { EpisodeId, SessionId, TraceDTO } from "../../agent-contract/dto.js"; import type { ServerDeps } from "../types.js"; +import { parseLimitParam } from "./params.js"; import { parseJson, writeError, type Routes } from "./registry.js"; export function registerTraceRoutes(routes: Routes, deps: ServerDeps): void { @@ -29,9 +30,10 @@ export function registerTraceRoutes(routes: Routes, deps: ServerDeps): void { */ routes.set("GET /api/v1/traces", async (ctx) => { const params = ctx.url.searchParams; - const parsedLimit = Number(params.get("limit")); + // Caller-supplied `limit` is untrusted — cap it at the documented + // max (500) instead of relying on the storage layer's bulk ceiling. + const limit = parseLimitParam(params.get("limit")); const parsedOffset = Number(params.get("offset")); - const limit = Number.isFinite(parsedLimit) && parsedLimit > 0 ? parsedLimit : 50; const offset = Number.isFinite(parsedOffset) && parsedOffset >= 0 ? parsedOffset : 0; const sessionId = params.get("sessionId") || undefined; const namespace = parseNamespace(params.get("namespace")); diff --git a/apps/memos-local-plugin/tests/unit/pipeline/memory-core.test.ts b/apps/memos-local-plugin/tests/unit/pipeline/memory-core.test.ts index 2759ae5bd..50120d1e1 100644 --- a/apps/memos-local-plugin/tests/unit/pipeline/memory-core.test.ts +++ b/apps/memos-local-plugin/tests/unit/pipeline/memory-core.test.ts @@ -2856,3 +2856,89 @@ algorithm: expect(meta.reward?.traceIds).toEqual(["tr_real"]); }); }); + +// #2401 regression: metrics() reads `traces.list({ limit: 10_000 })` to +// bucket the viewer's "memory writes per day" chart, but the storage layer +// used to clamp every explicit limit to the 500-row default page. With 520 +// traces written "now" and 100 three days earlier, the clamped read only +// saw the newest 500 rows — all from today — so the older day bucketed to +// 0 (the chart showed nothing but the last ~2 days) and the `sessions` +// card lost every but the newest session. +describe("MemoryCore metrics sees beyond the 500-row page (#2401)", () => { + it("dailyWrites / sessions cover traces older than the newest 500", async () => { + pipeline = createPipeline(buildDeps(db!)); + core = createMemoryCore( + pipeline, + resolveHome("openclaw", "/tmp/memos-mc-test"), + "test", + ); + await core.init(); + + db!.repos.sessions.upsert({ + id: "s-today", + agent: "openclaw", + startedAt: 1, + lastSeenAt: 1, + meta: {}, + }); + db!.repos.sessions.upsert({ + id: "s-old", + agent: "openclaw", + startedAt: 1, + lastSeenAt: 1, + meta: {}, + }); + db!.repos.episodes.insert({ + id: "e-bulk", + sessionId: "s-today", + startedAt: 1, + endedAt: null, + traceIds: [], + rTask: null, + status: "open", + }); + + const DAY = 86_400_000; + const now = Date.now(); + const oldTs = now - 3 * DAY; + const seed = (i: number, ts: number, sessionId: string): void => { + db!.repos.traces.insert({ + id: `tr-bulk-${i}`, + episodeId: "e-bulk", + sessionId, + ts, + userText: `user ${i}`, + agentText: `agent ${i}`, + summary: `summary ${i}`, + toolCalls: [], + value: 0, + alpha: 0, + priority: 0, + tags: [], + errorSignatures: [], + turnId: i, + schemaVersion: 1, + }); + }; + for (let i = 0; i < 520; i++) seed(i, now + i, "s-today"); + for (let i = 520; i < 620; i++) seed(i, oldTs + i, "s-old"); + + const m = await core.metrics({ days: 7, includeAllNamespaces: true }); + + // metrics() derives day keys from the trace ts exactly like this + // (local midnight, serialized as a UTC ISO date). Mirroring the + // formula keeps this test independent of the host timezone. + const dayKey = (ms: number): string => { + const d = new Date(ms); + d.setHours(0, 0, 0, 0); + return d.toISOString().slice(0, 10); + }; + + expect(m.dailyWrites).toHaveLength(7); + const oldBucket = m.dailyWrites.find((b) => b.date === dayKey(oldTs)); + expect(oldBucket?.count).toBe(100); + // Both sessions must be visible — the 500-clamped read only ever saw + // today's session. + expect(m.sessions).toBe(2); + }); +}); diff --git a/apps/memos-local-plugin/tests/unit/server/params.test.ts b/apps/memos-local-plugin/tests/unit/server/params.test.ts new file mode 100644 index 000000000..3c70672ff --- /dev/null +++ b/apps/memos-local-plugin/tests/unit/server/params.test.ts @@ -0,0 +1,43 @@ +import { describe, expect, it } from "vitest"; + +import { + MAX_HTTP_PAGE_LIMIT, + parseLimitParam, +} from "../../../server/routes/params.js"; + +// Boundary cap for untrusted `limit` input: storage-level `clampLimit` +// honors explicit limits up to MAX_QUERY_LIMIT (#2401), so routes must +// keep caller-supplied page sizes at the 500-row ceiling these endpoints +// had before that change — otherwise a `?limit=100000` request could pull +// up to MAX_QUERY_LIMIT rows in one round-trip. +describe("server/routes/params — parseLimitParam", () => { + it("falls back to the default for missing or invalid input", () => { + expect(parseLimitParam(null)).toBe(50); + expect(parseLimitParam(undefined as unknown as null)).toBe(50); + expect(parseLimitParam("")).toBe(50); + expect(parseLimitParam("abc")).toBe(50); + expect(parseLimitParam("0")).toBe(50); + expect(parseLimitParam("-5")).toBe(50); + expect(parseLimitParam("Infinity")).toBe(50); + expect(parseLimitParam(null, 20)).toBe(20); + }); + + it("accepts positive integers", () => { + expect(parseLimitParam("1")).toBe(1); + expect(parseLimitParam("50")).toBe(50); + expect(parseLimitParam(String(MAX_HTTP_PAGE_LIMIT))).toBe( + MAX_HTTP_PAGE_LIMIT, + ); + }); + + it("clamps oversized values to MAX_HTTP_PAGE_LIMIT", () => { + expect(parseLimitParam("501")).toBe(MAX_HTTP_PAGE_LIMIT); + expect(parseLimitParam("10000")).toBe(MAX_HTTP_PAGE_LIMIT); + expect(parseLimitParam("1e9")).toBe(MAX_HTTP_PAGE_LIMIT); + }); + + it("truncates fractional values", () => { + expect(parseLimitParam("10.9")).toBe(10); + expect(parseLimitParam("500.5")).toBe(MAX_HTTP_PAGE_LIMIT); + }); +}); diff --git a/apps/memos-local-plugin/tests/unit/storage/explicit-limit.test.ts b/apps/memos-local-plugin/tests/unit/storage/explicit-limit.test.ts new file mode 100644 index 000000000..4aec26e22 --- /dev/null +++ b/apps/memos-local-plugin/tests/unit/storage/explicit-limit.test.ts @@ -0,0 +1,112 @@ +import { describe, it, expect, beforeEach } from "vitest"; +import Database from "better-sqlite3"; +import { makeTracesRepo } from "../../../core/storage/repos/traces.js"; +import { clampLimit, MAX_QUERY_LIMIT } from "../../../core/storage/repos/_helpers.js"; +import type { TraceRow } from "../../../agent-contract/dto.js"; + +// #2401 regression: an EXPLICIT `limit` must be honored (up to +// MAX_QUERY_LIMIT) instead of being silently rewritten to the 500-row +// default page. Before this fix `traces.list({ limit: 10_000 })` returned +// at most 500 rows, so the viewer's "memory writes per day" chart only +// ever saw the newest 500 traces (~2 days at typical volume) and the +// `sessions` / `embeddings` cards undercounted. Omitting `limit` keeps the +// 500-row default page — that contract is unchanged. +describe("explicit list limits are honored (#2401)", () => { + let db: Database.Database; + let repo: ReturnType; + + beforeEach(() => { + db = new Database(":memory:"); + db.exec(` + CREATE TABLE traces ( + id TEXT PRIMARY KEY, + episode_id TEXT, + session_id TEXT NOT NULL, + owner_agent_kind TEXT, + owner_profile_id TEXT, + owner_workspace_id TEXT, + ts INTEGER NOT NULL, + user_text TEXT, + agent_text TEXT, + summary TEXT, + tool_calls_json TEXT, + reflection TEXT, + agent_thinking TEXT, + value REAL NOT NULL DEFAULT 0, + alpha REAL NOT NULL DEFAULT 0, + r_human REAL, + priority REAL NOT NULL DEFAULT 0, + tags_json TEXT, + error_signatures_json TEXT, + vec_summary BLOB, + vec_action BLOB, + share_scope TEXT, + share_target TEXT, + shared_at INTEGER, + turn_id INTEGER NOT NULL DEFAULT 0, + schema_version INTEGER NOT NULL DEFAULT 1 + ); + CREATE INDEX idx_traces_ts ON traces(ts); + CREATE INDEX idx_traces_episode_turn ON traces(episode_id, turn_id, ts); + `); + repo = makeTracesRepo(db); + }); + + it("clampLimit honors explicit limits up to MAX_QUERY_LIMIT", () => { + expect(clampLimit(10_000)).toBe(10_000); + expect(clampLimit(100_000)).toBe(MAX_QUERY_LIMIT); + // Beyond the ceiling: bounded, not rejected. + expect(clampLimit(250_000)).toBe(MAX_QUERY_LIMIT); + // Small explicit pages still work. + expect(clampLimit(5)).toBe(5); + // Invalid / missing input keeps the 500-row default page. + expect(clampLimit(0)).toBe(500); + expect(clampLimit(-5)).toBe(500); + expect(clampLimit(Number.NaN)).toBe(500); + }); + + it("list({ limit }) returns every row a bulk caller asked for", () => { + const DAY = 86_400_000; + const now = 1_700_000_000_000; + for (let i = 0; i < 600; i++) { + // Three distinct days so a day-bucketing caller (metrics) would see + // non-zero counts on days that fall outside the newest-500 window. + const ts = now - (i % 3) * DAY + i; + const trace: TraceRow = { + id: `trace-${i}`, + episodeId: "episode-1", + sessionId: `session-${i % 3}`, + ts, + userText: `user ${i}`, + agentText: `agent ${i}`, + summary: `summary ${i}`, + toolCalls: [], + value: 0, + alpha: 0, + priority: 0, + tags: [], + errorSignatures: [], + turnId: i, + schemaVersion: 1, + }; + repo.insert(trace); + } + + // Bulk read used by metrics(): must see all 600 rows, not 500. + const bulk = repo.list({ limit: 10_000 }); + expect(bulk).toHaveLength(600); + + // …and the rows it previously lost were the OLDEST ones (newest-first + // ordering), which is exactly what emptied the left side of the + // viewer's daily-writes chart. + const oldestVisible = Math.min(...bulk.map((t) => t.ts)); + // Smallest seeded ts is `now - 2*DAY + 2` (i === 2, 5, 8, …). + expect(oldestVisible).toBe(now - 2 * DAY + 2); + const distinctDays = new Set(bulk.map((t) => Math.floor(t.ts / DAY))); + expect(distinctDays.size).toBe(3); + + // The implicit default page stays 500 rows — callers that omit + // `limit` keep their existing behavior. + expect(repo.list()).toHaveLength(500); + }); +}); diff --git a/apps/memos-local-plugin/tests/unit/storage/traces-count.test.ts b/apps/memos-local-plugin/tests/unit/storage/traces-count.test.ts index 888d691aa..841dd14ad 100644 --- a/apps/memos-local-plugin/tests/unit/storage/traces-count.test.ts +++ b/apps/memos-local-plugin/tests/unit/storage/traces-count.test.ts @@ -71,13 +71,15 @@ describe("traces count with > 500 items", () => { const count = repo.count(); expect(count).toBe(600); - // Verify list with no limit still caps at 500 + // Verify list with no limit still caps at 500 (default page) const listed = repo.list({}); expect(listed.length).toBe(500); - // Verify list with explicit high limit also caps at 500 + // Verify an explicit high limit is honored up to MAX_QUERY_LIMIT + // (#2401) — bulk callers (metrics, maintenance sweeps) must see every + // row they asked for instead of the 500-row default page. const listedWithLimit = repo.list({ limit: 10000 }); - expect(listedWithLimit.length).toBe(500); + expect(listedWithLimit.length).toBe(600); }); it("countTurns() should return accurate count > 500", () => {