Skip to content
1 change: 1 addition & 0 deletions src/mcp/tool-contracts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ export const runtimeStatusSchema = z
enabled: z.boolean(),
maxSessions: z.number(),
activeSessions: z.number(),
persistent: z.boolean(),
})
.strict(),
})
Expand Down
2 changes: 2 additions & 0 deletions src/runtime/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ export const ARTIFACT_FILENAMES = {
anchorMap: 'anchor-map.json',
} as const;

export const SESSION_CACHE_FILENAME = 'session-cache.json';

export const PREFACE_MARKER = '# **Preface** (non-normative)';
export const PREFACE_ROUTE_CITATION = 'Preface/Where to start';
export const ROUTE_INDEX_CITATION = 'J.4';
Expand Down
12 changes: 11 additions & 1 deletion src/runtime/runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -34,6 +35,7 @@ export interface FpfRuntimeOptions {
artifactDir?: string;
synthesizer?: LocalAnswerSynthesizer;
maxSessions?: number;
persistSessionCache?: boolean;
}

export class FpfRuntime {
Expand All @@ -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);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid reloading session cache on every refresh

With persistence enabled, every query/trace call runs refresh(), and this line reloads cache state from disk before request handling. In the same commit, SessionCache.set() persists asynchronously (scheduleFlush queues writeFile and 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 👍 / 👎.

Copy link
Copy Markdown
Contributor

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):

  1. Idempotent load: load() now returns immediately when sourceHash matches the already-loaded hash, so repeated calls from refresh() don't re-read from disk or race with pending flushes.
  2. Hash change clears entries: When sourceHash changes, this.entries.clear() is called before loading new data — stale sessions from the old compilation are discarded.
async load(sourceHash: string): Promise<void> {
  if (this.sourceHash === sourceHash) {
    return; // idempotent — skip re-read
  }
  if (this.sourceHash !== undefined) {
    this.entries.clear(); // discard stale sessions on hash change
  }
  this.sourceHash = sourceHash;
  // ... disk read only on first load or hash change
}

const existingSnapshot = await this.loadSnapshot();
Comment on lines 74 to 78
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already addressed in cd1eadcload() is now idempotent: it early-returns when this.sourceHash === sourceHash, so repeated calls from refresh() don't re-read from disk or race with pending flushes. Only the first call (or a sourceHash change) triggers a disk read.

const compatibleSnapshot = existingSnapshot && !snapshotNeedsRebuild(existingSnapshot);

Expand Down
136 changes: 134 additions & 2 deletions src/runtime/session-cache.ts
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[];
Expand All @@ -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
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Wait for in-flight cache load before treating hash as loaded

With persistence enabled, concurrent requests on a cold start can lose newly written session state: load() marks sourceHash before the async readFile completes, so a second refresh() call returns early from load() and can call set(); when the first load() resumes, it clears entries and restores the older on-disk snapshot, wiping the second request's in-memory update. This is still possible even with the idempotent hash check, because the check does not distinguish "loaded" from "currently loading".

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — this was a real race condition. Fixed in e3afaf0:

  1. Added loadPromise field that stores the in-flight disk read promise
  2. When a concurrent caller sees sourceHash already matches, it awaits loadPromise before returning — so it won't proceed to set() while the disk read is still in progress
  3. Extracted readFromDisk() which now merges disk entries without entries.clear() — it skips keys already present in the map, so entries added by concurrent set() calls between the await boundary are preserved

}

// 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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already addressed in cd1eadc — when sourceHash changes, this.entries.clear() is called before setting the new hash:

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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in d398c44. Entries are now persisted as Array<[string, RetrievalSessionState]> (ordered tuples) instead of Record<string, ...>, preserving LRU insertion order across JSON round-trips regardless of key format.

}
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
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate persisted session payloads before restoring

The loader accepts any JSON entries tuple and writes its value directly into entries without checking it matches RetrievalSessionState. With persistence enabled, a parseable-but-invalid cache file (for example from manual edits, format drift, or external corruption) can inject non-array lastSelectedNodeIds/recentUnresolvedNodeIds; downstream query code assumes those fields are arrays and can throw when applying session context, causing requests to fail until the cache file is removed. Treat structurally invalid records as corrupt (skip or clear) instead of restoring them.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 (PersistedSessionCacheJSON.stringify), so structural corruption requires external tampering or cross-version format drift. The sourceHash check already gates against stale/incompatible data — if the spec changes, the entire cache is discarded.

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
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The load method is called on every query via FpfRuntime.refresh. In its current implementation, it clears the in-memory cache and re-reads from disk every time, which is inefficient and introduces a race condition. If a background flush from a previous query is still writing to disk, load might read stale data and overwrite the newer in-memory state. Additionally, if the sourceHash changes (indicating a new compilation), the existing in-memory sessions should be discarded to avoid using stale context that might refer to outdated node IDs.

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
    }
  }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both issues addressed in cd1eadcload() is now idempotent (early-returns when sourceHash matches) and clears entries on hash change. The compact JSON suggestion is also applied in the same commit.


get(sessionId: string): RetrievalSessionState | undefined {
const value = this.entries.get(sessionId);
Expand All @@ -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
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid labeling stale sessions with current source hash

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 set() after another request has run load(newHash), and doFlush() will stamp all in-memory entries with the current this.sourceHash. That writes stale node IDs as if they belong to newHash, so a restart will reload and reuse invalid context even though load() is supposed to isolate sessions by hash. Consider storing/verifying the originating hash per write (or passing expected hash into set) before flushing.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — this is a real race condition. Fixed in d59429c with two guards:

  1. load() cancels pending flush timers — when the sourceHash changes, any scheduled debounce timer is cleared before entries.clear(), so stale entries never get written under the new hash.
  2. doFlush() verifies hash at write time — captures hashAtFlush when the flush starts and checks this.sourceHash !== hashAtFlush before the async write completes, skipping the write if the hash changed in between.

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
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The scheduleFlush method performs synchronous serialization (Object.fromEntries and JSON.stringify) on every set operation. Using pretty-printing (null, 2) for a machine-read cache file adds unnecessary CPU and disk I/O overhead. Furthermore, since this is called on every update, it can lead to a large queue of redundant file writes if sessions are updated frequently.

Suggested change
private scheduleFlush(): void {
if (!this.persistPath || !this.sourceHash) {
return;
}
const path = this.persistPath;
const data: PersistedSessionCache = {
sourceHash: this.sourceHash,
entries: Object.fromEntries(this.entries),
};
const json = JSON.stringify(data, null, 2);
this.flushPromise = (this.flushPromise ?? Promise.resolve())
private scheduleFlush(): void {
if (!this.persistPath || !this.sourceHash) {
return;
}
const path = this.persistPath;
const data: PersistedSessionCache = {
sourceHash: this.sourceHash,
entries: Object.fromEntries(this.entries),
};
const json = JSON.stringify(data);
this.flushPromise = (this.flushPromise ?? Promise.resolve())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied — JSON.stringify(data) without pretty-print in cd1eadc.

.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);
}
});
}
}
1 change: 1 addition & 0 deletions src/runtime/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,7 @@ export interface RuntimeStatus {
enabled: boolean;
maxSessions: number;
activeSessions: number;
persistent: boolean;
};
}

Expand Down
Loading