Repository navigation
refactor(retention): move log stripping to gateway #3222
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 2 commits
c5ca3b9
d765dce
90a36a6
6281afc
f912fb5
40de976
749704c
43c6950
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,86 @@ | ||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
|
|
||
| import { insertLog } from "./logs.js"; | ||
|
|
||
| import type { LogInsertData } from "@llmgateway/db"; | ||
|
|
||
| const publishToQueue = vi.fn(); | ||
|
|
||
| vi.mock(import("@llmgateway/cache"), async (importOriginal) => { | ||
| const actual = await importOriginal(); | ||
| return { | ||
| ...actual, | ||
| publishToQueue: (...args: unknown[]) => publishToQueue(...args), | ||
| }; | ||
| }); | ||
|
|
||
| vi.mock("@llmgateway/instrumentation", () => ({ | ||
| recordChatCompletionMetrics: vi.fn(), | ||
| })); | ||
|
|
||
| function baseLogData(overrides: Partial<LogInsertData>): LogInsertData { | ||
| return { | ||
| requestId: "req-1", | ||
| organizationId: "org-1", | ||
| projectId: "project-1", | ||
| apiKeyId: "api-key-1", | ||
| duration: 100, | ||
| requestedModel: "some-model", | ||
| usedModel: "some-model", | ||
| usedProvider: "openai", | ||
| responseSize: 0, | ||
| mode: "credits", | ||
| usedMode: "credits", | ||
| finishReason: "stop", | ||
| hasError: false, | ||
| messages: [{ role: "user", content: "secret prompt" }], | ||
| content: "secret completion", | ||
| reasoningContent: "secret reasoning", | ||
| tools: [{ type: "function", function: { name: "f" } }], | ||
| toolChoice: "auto", | ||
| toolResults: [{ id: "1" }], | ||
| responsesApiData: { foo: "bar" }, | ||
| ...overrides, | ||
| } as LogInsertData; | ||
| } | ||
|
|
||
| describe("insertLog retention stripping", () => { | ||
| beforeEach(() => { | ||
| publishToQueue.mockClear(); | ||
| }); | ||
|
|
||
| it("strips payload fields before publishing when retention is none", async () => { | ||
| await insertLog(baseLogData({}), { retentionLevel: "none" }); | ||
|
|
||
| expect(publishToQueue).toHaveBeenCalledTimes(1); | ||
| const published = publishToQueue.mock.calls[0][1] as LogInsertData; | ||
| expect(published.messages).toBeNull(); | ||
| expect(published.content).toBeNull(); | ||
| expect(published.reasoningContent).toBeNull(); | ||
| expect(published.tools).toBeNull(); | ||
| expect(published.toolChoice).toBeNull(); | ||
| expect(published.toolResults).toBeNull(); | ||
| expect(published.responsesApiData).toBeNull(); | ||
| // Metadata is preserved. | ||
| expect(published.requestId).toBe("req-1"); | ||
| expect(published.organizationId).toBe("org-1"); | ||
| expect(JSON.stringify(published)).not.toContain("secret"); | ||
| }); | ||
|
|
||
| it("keeps payload fields when retention is retain", async () => { | ||
| await insertLog(baseLogData({}), { retentionLevel: "retain" }); | ||
|
|
||
| const published = publishToQueue.mock.calls[0][1] as LogInsertData; | ||
| expect(published.content).toBe("secret completion"); | ||
| expect(published.messages).toEqual([ | ||
| { role: "user", content: "secret prompt" }, | ||
| ]); | ||
| }); | ||
|
|
||
| it("keeps payload fields when retention is unspecified", async () => { | ||
| await insertLog(baseLogData({})); | ||
|
|
||
| const published = publishToQueue.mock.calls[0][1] as LogInsertData; | ||
| expect(published.content).toBe("secret completion"); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,6 @@ import { | |
| addApiKeyPeriodDuration, | ||
| and, | ||
| apiKey, | ||
| cdb, | ||
| closeDatabase, | ||
| db, | ||
| enqueueWebhookDeliveries, | ||
|
|
@@ -122,11 +121,6 @@ const LOG_QUEUE_CONCURRENCY = Math.max( | |
| 1, | ||
| Number(process.env.LOG_QUEUE_CONCURRENCY) || 4, | ||
| ); | ||
| // Cache organization retention levels to avoid a serial Postgres round-trip | ||
| // before every log batch insert. retentionLevel changes rarely; the short TTL | ||
| // bounds how long a stale value can keep retaining or stripping log payloads. | ||
| const ORG_RETENTION_CACHE_TTL_MS = | ||
| Number(process.env.ORG_RETENTION_CACHE_TTL_MS) || 60_000; | ||
| const CREDIT_BATCH_SIZE = Number(process.env.CREDIT_BATCH_SIZE) || 100; | ||
| const BATCH_PROCESSING_INTERVAL_SECONDS = | ||
| Number(process.env.CREDIT_BATCH_INTERVAL) || 5; | ||
|
|
@@ -1686,50 +1680,6 @@ function recordLogInsertSuccess(): void { | |
| logInsertCircuit.nextAttemptAt = 0; | ||
| } | ||
|
|
||
| const orgRetentionCache = new Map< | ||
| string, | ||
| { retentionLevel: "retain" | "none"; expiresAt: number } | ||
| >(); | ||
|
|
||
| // Resolve organization retention levels, serving from the in-memory cache when | ||
| // fresh and only querying Postgres for the ids that are missing or expired. | ||
| async function getOrganizationRetentionLevels( | ||
| organizationIds: string[], | ||
| ): Promise<Map<string, "retain" | "none">> { | ||
| const now = Date.now(); | ||
| const result = new Map<string, "retain" | "none">(); | ||
| const missing: string[] = []; | ||
|
|
||
| for (const id of organizationIds) { | ||
| const cached = orgRetentionCache.get(id); | ||
| if (cached && cached.expiresAt > now) { | ||
| result.set(id, cached.retentionLevel); | ||
| } else { | ||
| missing.push(id); | ||
| } | ||
| } | ||
|
|
||
| if (missing.length > 0) { | ||
| const organizations = await cdb | ||
| .select({ | ||
| id: organization.id, | ||
| retentionLevel: organization.retentionLevel, | ||
| }) | ||
| .from(organization) | ||
| .where(inArray(organization.id, missing)); | ||
|
|
||
| for (const org of organizations) { | ||
| result.set(org.id, org.retentionLevel); | ||
| orgRetentionCache.set(org.id, { | ||
| retentionLevel: org.retentionLevel, | ||
| expiresAt: now + ORG_RETENTION_CACHE_TTL_MS, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| // Returns the number of messages successfully inserted, so the drain loop can | ||
| // decide whether to sleep (partial batch) or immediately fetch the next batch | ||
| // (full batch, queue likely still backed up). | ||
|
|
@@ -1747,49 +1697,21 @@ export async function processLogQueue(): Promise<number> { | |
| const MAX_RETRIES = 5; | ||
|
|
||
| try { | ||
| // The gateway decides what to persist: it strips request/response payload | ||
| // fields before publishing for orgs that don't retain data, so the worker | ||
| // inserts the queued rows as-is with no per-batch org retention lookup. | ||
| const logData = message.map((i) => JSON.parse(i) as LogInsertData); | ||
| const organizationIds = Array.from( | ||
| new Set(logData.map((data) => data.organizationId)), | ||
| ); | ||
| const selectStart = Date.now(); | ||
| const retentionByOrg = | ||
| organizationIds.length > 0 | ||
| ? await getOrganizationRetentionLevels(organizationIds) | ||
| : new Map<string, "retain" | "none">(); | ||
| const selectMs = Date.now() - selectStart; | ||
|
|
||
| const processedLogData: ( | ||
| | LogInsertData | ||
| | Omit<LogInsertData, "messages" | "content"> | ||
| )[] = logData.map((data) => { | ||
| if (retentionByOrg.get(data.organizationId) === "none") { | ||
| const { | ||
| messages: _messages, | ||
| content: _content, | ||
| reasoningContent: _reasoningContent, | ||
| tools: _tools, | ||
| toolChoice: _toolChoice, | ||
| toolResults: _toolResults, | ||
| responsesApiData: _responsesApiData, | ||
| ...metadataOnly | ||
| } = data; | ||
| return metadataOnly; | ||
| } | ||
|
|
||
| return data; | ||
| }); | ||
|
|
||
| // Insert logs with retry logic | ||
| let lastError: Error | undefined; | ||
| for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) { | ||
| try { | ||
| // Type assertion is safe here as both LogInsertData and its subset are compatible with the log insert schema | ||
| const insertStart = Date.now(); | ||
| await db.insert(log).values(processedLogData as LogInsertData[]); | ||
| await db.insert(log).values(logData); | ||
|
Comment on lines
+1700
to
+1710
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. 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift Do not bypass retention for pre-existing queued messages. This assumes every Redis message was published by the new gateway. During a rolling deployment—or for a queue backlog created before this change—an org with 🤖 Prompt for AI Agents |
||
| const insertMs = Date.now() - insertStart; | ||
| recordLogInsertSuccess(); | ||
| logger.info( | ||
| `Processed log batch: ${message.length} rows (org lookup ${selectMs}ms, insert ${insertMs}ms)`, | ||
| `Processed log batch: ${message.length} rows (insert ${insertMs}ms)`, | ||
| ); | ||
| return message.length; // Success, exit function | ||
| } catch (insertError) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| import type { LogInsertData } from "./types.js"; | ||
|
|
||
| /** | ||
| * Log columns that hold request/response payload data (the actual prompt, | ||
| * completion, reasoning, tool definitions/results and Responses API payload). | ||
| * Organizations with `retentionLevel` "none" do not retain any of this, so it | ||
| * is cleared before the row is persisted. | ||
| */ | ||
| export const RETENTION_SENSITIVE_LOG_FIELDS = [ | ||
| "messages", | ||
| "content", | ||
| "reasoningContent", | ||
| "tools", | ||
| "toolChoice", | ||
| "toolResults", | ||
| "responsesApiData", | ||
| ] as const; | ||
|
Comment on lines
+9
to
+17
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. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win Strip the raw request/response payload columns too.
Proposed fix export const RETENTION_SENSITIVE_LOG_FIELDS = [
"messages",
"content",
"reasoningContent",
"tools",
"toolChoice",
"toolResults",
"responsesApiData",
+ "rawRequest",
+ "rawResponse",
+ "upstreamRequest",
+ "upstreamResponse",
] as const;
return {
...logData,
messages: null,
content: null,
reasoningContent: null,
tools: null,
toolChoice: null,
toolResults: null,
responsesApiData: null,
+ rawRequest: null,
+ rawResponse: null,
+ upstreamRequest: null,
+ upstreamResponse: null,
};Also applies to: 28-37 🤖 Prompt for AI Agents |
||
|
|
||
| /** | ||
| * Return a copy of the log data with every retention-sensitive payload field | ||
| * cleared. Used by the gateway so payloads for non-retaining orgs never travel | ||
| * through Redis (or reach the database) in the first place — the gateway is the | ||
| * sole decider of what gets persisted, so the worker inserts queued rows as-is. | ||
| */ | ||
| export function stripRetentionSensitiveLogFields<T extends LogInsertData>( | ||
| logData: T, | ||
| ): T { | ||
| return { | ||
| ...logData, | ||
| messages: null, | ||
| content: null, | ||
| reasoningContent: null, | ||
| tools: null, | ||
| toolChoice: null, | ||
| toolResults: null, | ||
| responsesApiData: null, | ||
| }; | ||
| } | ||
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.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Make unresolved retention fail closed. Omitted,
null, or unresolved organization retention currently preserves payloads, contradicting the required default of"none".apps/gateway/src/lib/logs.ts#L296-L298: strip unlessretentionLevel === "retain".apps/gateway/src/lib/logs-retention.spec.ts#L80-L85: change the unspecified-retention test to expect stripped fields.apps/worker/src/worker.ts#L1765-L1770: strip unless the lookup explicitly returns"retain".Proposed fix
📝 Committable suggestion
📍 Affects 3 files
apps/gateway/src/lib/logs.ts#L296-L298(this comment)apps/gateway/src/lib/logs-retention.spec.ts#L80-L85apps/worker/src/worker.ts#L1765-L1770🤖 Prompt for AI Agents