-
Notifications
You must be signed in to change notification settings - Fork 1
feat: persist session cache across process restarts #27
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
2f7942d
d9ee5d8
9c03c4d
e76930a
9476fd1
342be88
d59429c
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 |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import { | |
| ARTIFACT_FILENAMES, | ||
| DEFAULT_ARTIFACT_DIR, | ||
| DEFAULT_SOURCE_PATH, | ||
| SESSION_CACHE_FILENAME, | ||
| } from './constants.js'; | ||
| import { compileFpfSource } from './compiler.js'; | ||
| import { createSynthesizerFromEnv } from './lm-studio-synthesizer.js'; | ||
|
|
@@ -34,6 +35,7 @@ export interface FpfRuntimeOptions { | |
| artifactDir?: string; | ||
| synthesizer?: LocalAnswerSynthesizer; | ||
| maxSessions?: number; | ||
| persistSessionCache?: boolean; | ||
| } | ||
|
|
||
| export class FpfRuntime { | ||
|
|
@@ -59,12 +61,20 @@ export class FpfRuntime { | |
| ]), | ||
| ) as Record<keyof typeof ARTIFACT_FILENAMES, string>; | ||
| this.synthesizer = options.synthesizer ?? createSynthesizerFromEnv(); | ||
| this.sessionCache = new SessionCache(options.maxSessions ?? 50); | ||
| const persistSession = | ||
| options.persistSessionCache ?? process.env.FPF_PERSIST_SESSION_CACHE === 'true'; | ||
| this.sessionCache = new SessionCache({ | ||
| maxSessions: options.maxSessions ?? 50, | ||
| persistPath: persistSession | ||
| ? resolve(this.artifactDir, SESSION_CACHE_FILENAME) | ||
| : undefined, | ||
| }); | ||
| } | ||
|
|
||
| async refresh(force = false): Promise<BuildAudit> { | ||
| await mkdir(this.artifactDir, { recursive: true }); | ||
| const currentSourceHash = await hashFile(this.sourcePath); | ||
| await this.sessionCache.load(currentSourceHash); | ||
| const existingSnapshot = await this.loadSnapshot(); | ||
|
Comment on lines
74
to
78
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. Already addressed in cd1eadc — |
||
| const compatibleSnapshot = existingSnapshot && !snapshotNeedsRebuild(existingSnapshot); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,3 +1,6 @@ | ||||||||||||||||||||||||||||||||||||||||||||||
| import { readFile, writeFile, mkdir, rename } from 'node:fs/promises'; | ||||||||||||||||||||||||||||||||||||||||||||||
| import { dirname, join } from 'node:path'; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| export interface RetrievalSessionState { | ||||||||||||||||||||||||||||||||||||||||||||||
| lastNormalizedQuestion: string; | ||||||||||||||||||||||||||||||||||||||||||||||
| lastSelectedNodeIds: string[]; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -6,10 +9,87 @@ export interface RetrievalSessionState { | |||||||||||||||||||||||||||||||||||||||||||||
| updatedAt: string; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| interface PersistedSessionCache { | ||||||||||||||||||||||||||||||||||||||||||||||
| sourceHash: string; | ||||||||||||||||||||||||||||||||||||||||||||||
| entries: Array<[string, RetrievalSessionState]>; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| export interface SessionCacheOptions { | ||||||||||||||||||||||||||||||||||||||||||||||
| maxSessions?: number; | ||||||||||||||||||||||||||||||||||||||||||||||
| persistPath?: string; | ||||||||||||||||||||||||||||||||||||||||||||||
| /** Debounce delay in ms before flushing to disk (default 500). */ | ||||||||||||||||||||||||||||||||||||||||||||||
| flushDelayMs?: number; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| export class SessionCache { | ||||||||||||||||||||||||||||||||||||||||||||||
| private readonly entries = new Map<string, RetrievalSessionState>(); | ||||||||||||||||||||||||||||||||||||||||||||||
| private readonly maxSessions: number; | ||||||||||||||||||||||||||||||||||||||||||||||
| private readonly persistPath?: string; | ||||||||||||||||||||||||||||||||||||||||||||||
| private readonly flushDelayMs: number; | ||||||||||||||||||||||||||||||||||||||||||||||
| private sourceHash?: string; | ||||||||||||||||||||||||||||||||||||||||||||||
| private flushPromise?: Promise<void>; | ||||||||||||||||||||||||||||||||||||||||||||||
| private loadPromise?: Promise<void>; | ||||||||||||||||||||||||||||||||||||||||||||||
| private flushTimer?: ReturnType<typeof setTimeout>; | ||||||||||||||||||||||||||||||||||||||||||||||
| private hasLoggedWriteError = false; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| constructor(options: SessionCacheOptions = {}) { | ||||||||||||||||||||||||||||||||||||||||||||||
| this.maxSessions = options.maxSessions ?? 50; | ||||||||||||||||||||||||||||||||||||||||||||||
| this.persistPath = options.persistPath; | ||||||||||||||||||||||||||||||||||||||||||||||
| this.flushDelayMs = options.flushDelayMs ?? 500; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| async load(sourceHash: string): Promise<void> { | ||||||||||||||||||||||||||||||||||||||||||||||
| if (this.sourceHash === sourceHash) { | ||||||||||||||||||||||||||||||||||||||||||||||
| await this.loadPromise; | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+42
to
+44
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.
With persistence enabled, concurrent requests on a cold start can lose newly written session state: Useful? React with 👍 / 👎.
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. Good catch — this was a real race condition. Fixed in e3afaf0:
|
||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| // Await any in-flight load before reassigning to avoid race conditions | ||||||||||||||||||||||||||||||||||||||||||||||
| if (this.loadPromise) { | ||||||||||||||||||||||||||||||||||||||||||||||
| await this.loadPromise; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if (this.sourceHash !== undefined) { | ||||||||||||||||||||||||||||||||||||||||||||||
| // Cancel any pending flush — entries are about to be cleared so writing them | ||||||||||||||||||||||||||||||||||||||||||||||
| // under the new sourceHash would persist stale session context. | ||||||||||||||||||||||||||||||||||||||||||||||
| if (this.flushTimer) { | ||||||||||||||||||||||||||||||||||||||||||||||
| clearTimeout(this.flushTimer); | ||||||||||||||||||||||||||||||||||||||||||||||
| this.flushTimer = undefined; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| this.entries.clear(); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| this.sourceHash = sourceHash; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if (!this.persistPath) { | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+60
to
+64
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. Already addressed in cd1eadc — when sourceHash changes, if (this.sourceHash !== undefined) {
this.entries.clear();
}
this.sourceHash = sourceHash;And if the persisted file's hash doesn't match, entries were already cleared by the guard above, so stale context is never reused. |
||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| this.loadPromise = this.readFromDisk(this.persistPath, sourceHash); | ||||||||||||||||||||||||||||||||||||||||||||||
| await this.loadPromise; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| constructor(private readonly maxSessions = 50) {} | ||||||||||||||||||||||||||||||||||||||||||||||
| /** Read persisted entries from disk. Path is passed explicitly to avoid non-null assertions. */ | ||||||||||||||||||||||||||||||||||||||||||||||
| private async readFromDisk(path: string, sourceHash: string): Promise<void> { | ||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||
| const raw = await readFile(path, 'utf8'); | ||||||||||||||||||||||||||||||||||||||||||||||
| const data: PersistedSessionCache = JSON.parse(raw); | ||||||||||||||||||||||||||||||||||||||||||||||
| if (data.sourceHash !== sourceHash || this.sourceHash !== sourceHash) { | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+64
to
+76
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. Good catch — fixed in d398c44. Entries are now persisted as |
||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| const tuples = data.entries; | ||||||||||||||||||||||||||||||||||||||||||||||
| if (!Array.isArray(tuples)) { | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| const start = Math.max(0, tuples.length - this.maxSessions); | ||||||||||||||||||||||||||||||||||||||||||||||
| for (let i = start; i < tuples.length; i++) { | ||||||||||||||||||||||||||||||||||||||||||||||
| const [key, value] = tuples[i]; | ||||||||||||||||||||||||||||||||||||||||||||||
| if (!this.entries.has(key)) { | ||||||||||||||||||||||||||||||||||||||||||||||
| this.entries.set(key, value); | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+84
to
+86
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.
The loader accepts any JSON Useful? React with 👍 / 👎.
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. Valid hardening suggestion, but not a blocking concern for this PR. The data is written by our own serialization code ( The narrow scenario (manually edited cache file with wrong field types) would cause a downstream crash, but the fix is simply deleting the cache file. For a production-grade defense, a Zod schema validation on load would be the right approach, but that's a follow-up improvement — not a merge blocker given the current risk profile. |
||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||||||||
| // File missing or corrupt — start fresh | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+41
to
+92
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. The Making this method idempotent and ensuring memory is cleared on hash changes fixes these issues. async load(sourceHash: string): Promise<void> {
if (this.sourceHash === sourceHash) {
return;
}
// Clear existing sessions if the compilation hash has changed
if (this.sourceHash !== undefined) {
this.entries.clear();
}
this.sourceHash = sourceHash;
if (!this.persistPath) {
return;
}
try {
const raw = await readFile(this.persistPath, 'utf8');
const data: PersistedSessionCache = JSON.parse(raw);
if (data.sourceHash !== sourceHash) {
return;
}
this.entries.clear();
const keys = Object.keys(data.entries);
const start = Math.max(0, keys.length - this.maxSessions);
for (let i = start; i < keys.length; i++) {
const key = keys[i];
this.entries.set(key, data.entries[key]);
}
} catch {
// File missing or corrupt — start fresh
}
}
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. Both issues addressed in cd1eadc — |
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| get(sessionId: string): RetrievalSessionState | undefined { | ||||||||||||||||||||||||||||||||||||||||||||||
| const value = this.entries.get(sessionId); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -33,13 +113,65 @@ export class SessionCache { | |||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| this.entries.delete(oldest); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| this.scheduleFlush(); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| summary(): { enabled: boolean; maxSessions: number; activeSessions: number } { | ||||||||||||||||||||||||||||||||||||||||||||||
| summary(): { enabled: boolean; maxSessions: number; activeSessions: number; persistent: boolean } { | ||||||||||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||||||||||
| enabled: true, | ||||||||||||||||||||||||||||||||||||||||||||||
| maxSessions: this.maxSessions, | ||||||||||||||||||||||||||||||||||||||||||||||
| activeSessions: this.entries.size, | ||||||||||||||||||||||||||||||||||||||||||||||
| persistent: this.persistPath != null, | ||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| /** Debounced flush — batches rapid set() calls into a single disk write. */ | ||||||||||||||||||||||||||||||||||||||||||||||
| private scheduleFlush(): void { | ||||||||||||||||||||||||||||||||||||||||||||||
| if (!this.persistPath || !this.sourceHash) { | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| // Clear any pending debounce timer so we only write once after the last set() | ||||||||||||||||||||||||||||||||||||||||||||||
| if (this.flushTimer) { | ||||||||||||||||||||||||||||||||||||||||||||||
| clearTimeout(this.flushTimer); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| this.flushTimer = setTimeout(() => { | ||||||||||||||||||||||||||||||||||||||||||||||
| this.flushTimer = undefined; | ||||||||||||||||||||||||||||||||||||||||||||||
| this.doFlush(); | ||||||||||||||||||||||||||||||||||||||||||||||
| }, this.flushDelayMs); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| private doFlush(): void { | ||||||||||||||||||||||||||||||||||||||||||||||
| if (!this.persistPath || !this.sourceHash) { | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| const path = this.persistPath; | ||||||||||||||||||||||||||||||||||||||||||||||
| // Capture the hash at flush time so we can verify it hasn't changed | ||||||||||||||||||||||||||||||||||||||||||||||
| // between scheduling and the async write completing. | ||||||||||||||||||||||||||||||||||||||||||||||
| const hashAtFlush = this.sourceHash; | ||||||||||||||||||||||||||||||||||||||||||||||
| const data: PersistedSessionCache = { | ||||||||||||||||||||||||||||||||||||||||||||||
| sourceHash: hashAtFlush, | ||||||||||||||||||||||||||||||||||||||||||||||
| entries: Array.from(this.entries.entries()), | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+151
to
+153
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.
With persistence enabled, concurrent requests can persist old-trace context under a new compilation hash: a query that started before a spec change can call Useful? React with 👍 / 👎.
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. Good catch — this is a real race condition. Fixed in
Together these prevent stale session context from being persisted under a newer compilation hash. |
||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||
| const json = JSON.stringify(data); | ||||||||||||||||||||||||||||||||||||||||||||||
| this.flushPromise = (this.flushPromise ?? Promise.resolve()) | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+129
to
+156
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. The
Suggested change
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. Applied — |
||||||||||||||||||||||||||||||||||||||||||||||
| .then(async () => { | ||||||||||||||||||||||||||||||||||||||||||||||
| // If the sourceHash changed while we were waiting, skip this write — | ||||||||||||||||||||||||||||||||||||||||||||||
| // the entries were captured under a potentially stale hash. | ||||||||||||||||||||||||||||||||||||||||||||||
| if (this.sourceHash !== hashAtFlush) { | ||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| await mkdir(dirname(path), { recursive: true }); | ||||||||||||||||||||||||||||||||||||||||||||||
| // Atomic write: write to temp file then rename to avoid corruption on crash | ||||||||||||||||||||||||||||||||||||||||||||||
| const tmpPath = join(dirname(path), `.session-cache.tmp.${Date.now()}`); | ||||||||||||||||||||||||||||||||||||||||||||||
| await writeFile(tmpPath, json, 'utf8'); | ||||||||||||||||||||||||||||||||||||||||||||||
| await rename(tmpPath, path); | ||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||
| .catch((err: unknown) => { | ||||||||||||||||||||||||||||||||||||||||||||||
| // Log the first write failure so silent disk issues are visible | ||||||||||||||||||||||||||||||||||||||||||||||
| if (!this.hasLoggedWriteError) { | ||||||||||||||||||||||||||||||||||||||||||||||
| this.hasLoggedWriteError = true; | ||||||||||||||||||||||||||||||||||||||||||||||
| console.error('[SessionCache] disk write failed:', err); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
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.
With persistence enabled, every
query/tracecall runsrefresh(), and this line reloads cache state from disk before request handling. In the same commit,SessionCache.set()persists asynchronously (scheduleFlushqueueswriteFileand does not await it), so a follow-up request can arrive before the prior flush completes;load()then replaces in-memory state with stale file contents and drops the just-written session context. This breaks multi-turn continuity precisely in the persistent mode this change introduces.Useful? React with 👍 / 👎.
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.
Both issues are already addressed in the latest commit (cd1eadc):
load()now returns immediately whensourceHashmatches the already-loaded hash, so repeated calls fromrefresh()don't re-read from disk or race with pending flushes.sourceHashchanges,this.entries.clear()is called before loading new data — stale sessions from the old compilation are discarded.