mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-09-01 03:27:01 +00:00
🔐 fix: Forward per-file entity_id through code-env priming (#12958)
* 🔐 fix: Forward per-file `entity_id` through code-env priming Skill files and persisted code-env files now carry their `entity_id` on the in-memory file refs that seed `Graph.sessions`. Without this, an execute call that mixes a skill file (uploaded with `entity_id=skillId`) and a user attachment (uploaded with no `entity_id`) collapses onto a single request-level entity at the codeapi authorization step and one side 403s. With per-file `entity_id`, codeapi resolves sessionKey per file and both authorize. - `primeSkillFiles` / `primeInvokedSkills`: thread `entity_id` through fresh-upload, cache-hit, and per-skill-batch paths in `packages/api/src/agents/skillFiles.ts`. - `primeFiles` (Code/process.js): parse `entity_id` from the persisted `codeEnvIdentifier` query string once per iteration; forward through `pushFile`, including the reupload path which re-parses the fresh identifier returned by codeapi. - Tests: extend `skillFiles.spec.ts` with two cases — fresh-upload propagation and cached-hot-path parsing. Companion PRs in flight on `@librechat/agents` (forward `entity_id` through `_injected_files`) and codeapi (per-file authorization). All three are wire-back-compat: an absent `entity_id` falls back to the existing request-level resolution. * 🔧 chore: Update dependencies in package-lock.json and package.json - Bump `@librechat/agents` to version `3.1.78-dev.0` across multiple package files. - Upgrade `@langchain/langgraph-checkpoint` to version `1.0.2` and update its peer dependency for `@langchain/core` to `^1.1.44`. - Update `axios` to version `1.16.0` and `follow-redirects` to version `1.16.0`. - Add `@types/diff` as a new dependency at version `7.0.2` and include `diff` at version `9.0.0` in the `@librechat/agents` module. - Introduce optional peer dependency `@anthropic-ai/sandbox-runtime` for `@librechat/agents` with metadata indicating it is optional. * 🐛 fix: Make skill code-env cache persistence observable Two changes to surface the skill-bundle re-upload issue without behavioral changes to tenant scoping (root cause to be confirmed via the new warn log): 1. `primeSkillFiles` now awaits `updateSkillFileCodeEnvIds` instead of firing-and-forgetting it. The prior shape could race with the next prime (read-before-write) even when the bulkWrite itself succeeds, producing a silent cache miss. Latency cost: ~10–50ms on first prime; in exchange every subsequent prime can rely on the identifier being persisted by the time it reads. 2. `updateSkillFileCodeEnvIds` now returns `{matchedCount, modifiedCount}` from the underlying bulkWrite. `primeSkillFiles` warn-logs when `modifiedCount < updates.length`, making any silent drop visible — whether the cause is tenant filtering, a `relativePath` mismatch, schema-plugin scoping, or something else. Prior shape returned `Promise<void>` so any zero-modification result was invisible. Tests: - `skill.spec.ts`: real-MongoDB happy path (counts match), no-match case (modifiedCount=0), and empty-input contract. - `skillFiles.spec.ts`: deferred-promise harness proving the call site awaits the persist (prime stays pending until the persist resolves) and forwards partial-write counts. Deliberately narrower than the original draft of this commit, which also bypassed `tenantSafeBulkWrite` for the codeEnvIdentifier write on the speculative diagnosis that tenant filtering was the cause. That change was a behavior shift on tenant scoping without confirmation; reverted pending real-world signal from the new warn log. * 🐛 fix: Justify await for skill code-env persistence under concurrency The await on `updateSkillFileCodeEnvIds` isn't a defensive nicety — it's load-bearing for cache effectiveness under concurrent priming. Verified with an out-of-tree harness (`config/test-skill-cache.ts`, not committed) that wires `primeSkillFiles` against a real codeapi stack: - With fire-and-forget (prior shape after this branch's revert): back-to-back primes for the same skill miss the cache. Call N+1 reads SkillFile docs before Call N's write commits, sees no `codeEnvIdentifier`, re-uploads, fires its own forget that Call N+2 also races. Steady-state stays in cache miss for the full burst. - With await: the prime that does the upload commits its persist before resolving, so the next concurrent prime observes the cache pointer instead of racing the read. Latency cost ~10–50ms on the upload prime; subsequent concurrent primes save an entire batch upload. In production with primes seconds apart this race is rare; at scale with many users hitting the same skill in the same second it's the difference between M and N×M uploads. Updates the regression test to assert the await contract (deferred persist promise → prime stays pending until persist resolves). Comment in `skillFiles.ts` rewritten to document the concurrency rationale rather than the weaker "race-with-next-prime" framing the prior commit used.
This commit is contained in:
Danny Avila
GitHub
parent
187ab787da
commit
9efd61d57d
8 changed files with 350 additions and 40 deletions
|
|
@ -127,4 +127,155 @@ describe('primeInvokedSkills — execute_code capability gate', () => {
|
|||
expect(result).toEqual({});
|
||||
expect(deps.getSkillByName).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('forwards entity_id on every file in initialSessions after fresh upload', async () => {
|
||||
/* Regression: codeapi now resolves sessionKey per-file using `entity_id`.
|
||||
* Skill files must carry `entity_id=<skillId>` through priming so that
|
||||
* a subsequent execute mixing skill files and a user attachment can be
|
||||
* authorized — both files in the same call resolve to their own scope
|
||||
* instead of collapsing onto a single request-level entity. */
|
||||
const listSkillFiles = jest.fn().mockResolvedValue([
|
||||
{
|
||||
relativePath: 'references/style.md',
|
||||
filename: 'style.md',
|
||||
filepath: '/storage/brand-guidelines/references/style.md',
|
||||
source: 's3',
|
||||
bytes: 256,
|
||||
},
|
||||
]);
|
||||
const getStrategyFunctions = jest.fn().mockReturnValue({
|
||||
getDownloadStream: jest.fn().mockResolvedValue(Readable.from(Buffer.from(''))),
|
||||
});
|
||||
const batchUploadCodeEnvFiles = jest.fn().mockResolvedValue({
|
||||
session_id: 'session-42',
|
||||
files: [{ fileId: 'file-1', filename: 'brand-guidelines/references/style.md' }],
|
||||
});
|
||||
|
||||
const deps = makeDeps({
|
||||
codeEnvAvailable: true,
|
||||
listSkillFiles,
|
||||
getStrategyFunctions,
|
||||
batchUploadCodeEnvFiles,
|
||||
});
|
||||
|
||||
const result = await primeInvokedSkills(deps);
|
||||
|
||||
const codeSession = result.initialSessions?.get('execute_code');
|
||||
expect(codeSession?.files).toEqual([
|
||||
{
|
||||
id: 'file-1',
|
||||
name: 'brand-guidelines/references/style.md',
|
||||
session_id: 'session-42',
|
||||
entity_id: SKILL_ID.toString(),
|
||||
},
|
||||
]);
|
||||
});
|
||||
|
||||
it('awaits updateSkillFileCodeEnvIds before resolving to avoid concurrent-prime cache misses', async () => {
|
||||
/* Concurrency regression: when many users hit the same skill at
|
||||
* once, fire-and-forget keeps the cache in miss-steady-state for
|
||||
* the burst — User N's prime reads SkillFile docs before User N-1's
|
||||
* persist commits, sees no `codeEnvIdentifier`, re-uploads, and
|
||||
* fires its own forget that User N+1 also races. Awaiting the
|
||||
* persist before the prime resolves ensures the next concurrent
|
||||
* caller observes the cache pointer instead of racing a write. */
|
||||
const fileRecords = [
|
||||
{
|
||||
relativePath: 'references/style.md',
|
||||
filename: 'style.md',
|
||||
filepath: '/storage/brand-guidelines/references/style.md',
|
||||
source: 's3',
|
||||
bytes: 256,
|
||||
},
|
||||
];
|
||||
const listSkillFiles = jest.fn().mockResolvedValue(fileRecords);
|
||||
const getStrategyFunctions = jest.fn().mockReturnValue({
|
||||
getDownloadStream: jest.fn().mockResolvedValue(Readable.from(Buffer.from(''))),
|
||||
});
|
||||
const batchUploadCodeEnvFiles = jest.fn().mockResolvedValue({
|
||||
session_id: 'session-42',
|
||||
files: [{ fileId: 'file-1', filename: 'brand-guidelines/references/style.md' }],
|
||||
});
|
||||
|
||||
/* Defer resolution so we can assert the prime hasn't returned yet
|
||||
* — proves the call site is awaiting, not fire-and-forget. */
|
||||
let resolvePersist!: (v: { matchedCount: number; modifiedCount: number }) => void;
|
||||
const persistGate = new Promise<{ matchedCount: number; modifiedCount: number }>((r) => {
|
||||
resolvePersist = r;
|
||||
});
|
||||
const updateSkillFileCodeEnvIds = jest.fn().mockReturnValue(persistGate);
|
||||
|
||||
const deps = makeDeps({
|
||||
codeEnvAvailable: true,
|
||||
listSkillFiles,
|
||||
getStrategyFunctions,
|
||||
batchUploadCodeEnvFiles,
|
||||
updateSkillFileCodeEnvIds,
|
||||
});
|
||||
|
||||
let resolved = false;
|
||||
const primePromise = primeInvokedSkills(deps).then((r) => {
|
||||
resolved = true;
|
||||
return r;
|
||||
});
|
||||
|
||||
/* Drain the microtask queue. The prime should still be pending
|
||||
* because persistGate hasn't resolved. */
|
||||
await new Promise((r) => setImmediate(r));
|
||||
expect(resolved).toBe(false);
|
||||
|
||||
/* Resolve as a successful write to confirm the prime completes
|
||||
* after the persist returns. */
|
||||
resolvePersist({ matchedCount: 1, modifiedCount: 1 });
|
||||
await primePromise;
|
||||
|
||||
expect(updateSkillFileCodeEnvIds).toHaveBeenCalledTimes(1);
|
||||
const [updates] = updateSkillFileCodeEnvIds.mock.calls[0];
|
||||
expect(updates).toEqual([
|
||||
{
|
||||
skillId: SKILL_ID,
|
||||
relativePath: 'references/style.md',
|
||||
codeEnvIdentifier: `session-42/file-1?entity_id=${SKILL_ID.toString()}`,
|
||||
},
|
||||
]);
|
||||
});
|
||||
|
||||
it('parses entity_id off codeEnvIdentifier in the cached-skills hot path', async () => {
|
||||
/* When all skill files are still active in codeapi, primeInvokedSkills
|
||||
* skips the batch upload entirely and reconstructs file refs from each
|
||||
* skill file's persisted `codeEnvIdentifier`. The query string carries
|
||||
* `?entity_id=<skillId>`, which must survive into `_injected_files` so
|
||||
* downstream authorization still uses the skill's scope. */
|
||||
const listSkillFiles = jest.fn().mockResolvedValue([
|
||||
{
|
||||
relativePath: 'references/style.md',
|
||||
filename: 'style.md',
|
||||
filepath: '/storage/brand-guidelines/references/style.md',
|
||||
source: 's3',
|
||||
bytes: 256,
|
||||
codeEnvIdentifier: `session-cached/file-cached?entity_id=${SKILL_ID.toString()}`,
|
||||
},
|
||||
]);
|
||||
const batchUploadCodeEnvFiles = jest.fn();
|
||||
const deps = makeDeps({
|
||||
codeEnvAvailable: true,
|
||||
listSkillFiles,
|
||||
batchUploadCodeEnvFiles,
|
||||
getSessionInfo: jest.fn().mockResolvedValue('2026-05-05T00:00:00Z'),
|
||||
checkIfActive: jest.fn().mockReturnValue(true),
|
||||
});
|
||||
|
||||
const result = await primeInvokedSkills(deps);
|
||||
|
||||
expect(batchUploadCodeEnvFiles).not.toHaveBeenCalled();
|
||||
const codeSession = result.initialSessions?.get('execute_code');
|
||||
expect(codeSession?.files).toEqual([
|
||||
{
|
||||
id: 'file-cached',
|
||||
name: 'brand-guidelines/references/style.md',
|
||||
session_id: 'session-cached',
|
||||
entity_id: SKILL_ID.toString(),
|
||||
},
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -39,19 +39,31 @@ export interface PrimeSkillFilesParams {
|
|||
getSessionInfo?: (fileIdentifier: string) => Promise<string | null>;
|
||||
/** 23-hour freshness check */
|
||||
checkIfActive?: (dateString: string) => boolean;
|
||||
/** Persists codeEnvIdentifier on skill files after upload */
|
||||
/** Persists codeEnvIdentifier on skill files after upload. Implementations
|
||||
* warn-log on partial writes (matchedCount/modifiedCount mismatch)
|
||||
* internally — caller can fire-and-forget without losing visibility. */
|
||||
updateSkillFileCodeEnvIds?: (
|
||||
updates: Array<{
|
||||
skillId: Types.ObjectId | string;
|
||||
relativePath: string;
|
||||
codeEnvIdentifier: string;
|
||||
}>,
|
||||
) => Promise<void>;
|
||||
) => Promise<{ matchedCount: number; modifiedCount: number } | void>;
|
||||
}
|
||||
|
||||
export interface PrimeSkillFilesResult {
|
||||
session_id: string;
|
||||
files: Array<{ id: string; session_id: string; name: string }>;
|
||||
files: Array<{ id: string; session_id: string; name: string; entity_id?: string }>;
|
||||
}
|
||||
|
||||
/** Parses `entity_id` out of a persisted `codeEnvIdentifier`'s query string.
|
||||
* Returns `undefined` when the identifier has no query string or no
|
||||
* `entity_id` (legacy user-attachment uploads). */
|
||||
function parseEntityIdFromCodeEnvIdentifier(identifier: string): string | undefined {
|
||||
const queryString = identifier.split('?')[1];
|
||||
if (!queryString) return undefined;
|
||||
const value = new URLSearchParams(queryString).get('entity_id');
|
||||
return value && value.length > 0 ? value : undefined;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -109,8 +121,15 @@ export async function primeSkillFiles(
|
|||
if (allActive) {
|
||||
const files: PrimeSkillFilesResult['files'] = [];
|
||||
for (const sf of skillFiles) {
|
||||
const [sid, fid] = (sf.codeEnvIdentifier as string).split('?')[0].split('/');
|
||||
files.push({ id: fid, session_id: sid, name: `${skill.name}/${sf.relativePath}` });
|
||||
const identifier = sf.codeEnvIdentifier as string;
|
||||
const [sid, fid] = identifier.split('?')[0].split('/');
|
||||
const entity_id = parseEntityIdFromCodeEnvIdentifier(identifier);
|
||||
files.push({
|
||||
id: fid,
|
||||
session_id: sid,
|
||||
name: `${skill.name}/${sf.relativePath}`,
|
||||
...(entity_id != null ? { entity_id } : {}),
|
||||
});
|
||||
}
|
||||
|
||||
if (files.length > 0) {
|
||||
|
|
@ -183,6 +202,7 @@ export async function primeSkillFiles(
|
|||
id: f.fileId,
|
||||
session_id: result.session_id,
|
||||
name: f.filename,
|
||||
entity_id: entityId,
|
||||
}));
|
||||
|
||||
// Treat partial upload failures as a priming failure — missing bundled
|
||||
|
|
@ -199,7 +219,19 @@ export async function primeSkillFiles(
|
|||
return null;
|
||||
}
|
||||
|
||||
// Persist codeEnvIdentifiers on skill files (fire-and-forget)
|
||||
/**
|
||||
* Persist codeEnvIdentifiers on skill files. Awaited (not
|
||||
* fire-and-forget) so the next prime — which can start within
|
||||
* milliseconds when many users hit the same skill concurrently —
|
||||
* sees the cache pointer instead of racing the read against an
|
||||
* in-flight write. Without the await, a fire-and-forget under
|
||||
* concurrency stays in cache-miss steady-state for the duration
|
||||
* of the burst (each user's prime reads stale, re-uploads, then
|
||||
* fires its own forget that the next user also misses). Latency
|
||||
* cost is ~10–50ms on the prime that does the upload; subsequent
|
||||
* primes save an entire batch upload. Failures don't fail the
|
||||
* prime — the file refs returned to the caller are still valid.
|
||||
*/
|
||||
if (updateSkillFileCodeEnvIds) {
|
||||
const updates = result.files
|
||||
.filter((f) => !f.filename.endsWith('/SKILL.md'))
|
||||
|
|
@ -209,12 +241,14 @@ export async function primeSkillFiles(
|
|||
codeEnvIdentifier: `${result.session_id}/${f.fileId}?entity_id=${entityId}`,
|
||||
}));
|
||||
if (updates.length > 0) {
|
||||
updateSkillFileCodeEnvIds(updates).catch((err: unknown) => {
|
||||
try {
|
||||
await updateSkillFileCodeEnvIds(updates);
|
||||
} catch (err: unknown) {
|
||||
logger.warn(
|
||||
'[primeSkillFiles] Failed to persist codeEnvIdentifiers:',
|
||||
err instanceof Error ? err.message : err,
|
||||
);
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -349,8 +383,15 @@ export async function primeInvokedSkills(
|
|||
r.files
|
||||
.filter((f) => f.codeEnvIdentifier)
|
||||
.map((f) => {
|
||||
const [sid, fid] = (f.codeEnvIdentifier as string).split('?')[0].split('/');
|
||||
return { id: fid, name: `${r.skill.name}/${f.relativePath}`, session_id: sid };
|
||||
const identifier = f.codeEnvIdentifier as string;
|
||||
const [sid, fid] = identifier.split('?')[0].split('/');
|
||||
const entity_id = parseEntityIdFromCodeEnvIdentifier(identifier);
|
||||
return {
|
||||
id: fid,
|
||||
name: `${r.skill.name}/${f.relativePath}`,
|
||||
session_id: sid,
|
||||
...(entity_id != null ? { entity_id } : {}),
|
||||
};
|
||||
}),
|
||||
);
|
||||
if (cachedFiles.length > 0) {
|
||||
|
|
@ -374,7 +415,12 @@ export async function primeInvokedSkills(
|
|||
// Per-skill upload: each skill gets its own session with entity_id=skillId.
|
||||
// primeSkillFiles handles freshness caching per-skill, so only expired
|
||||
// skills re-upload. The code env handles mixed session_ids natively.
|
||||
const allPrimedFiles: Array<{ id: string; name: string; session_id: string }> = [];
|
||||
const allPrimedFiles: Array<{
|
||||
id: string;
|
||||
name: string;
|
||||
session_id: string;
|
||||
entity_id?: string;
|
||||
}> = [];
|
||||
const primeResults = await Promise.allSettled(
|
||||
fileListResults.map(async ({ skill, files }) => {
|
||||
const result = await primeSkillFiles({
|
||||
|
|
@ -393,7 +439,12 @@ export async function primeInvokedSkills(
|
|||
for (const r of primeResults) {
|
||||
if (r.status === 'fulfilled' && r.value.result) {
|
||||
for (const f of r.value.result.files) {
|
||||
allPrimedFiles.push({ id: f.id, name: f.name, session_id: f.session_id });
|
||||
allPrimedFiles.push({
|
||||
id: f.id,
|
||||
name: f.name,
|
||||
session_id: f.session_id,
|
||||
...(f.entity_id != null ? { entity_id: f.entity_id } : {}),
|
||||
});
|
||||
}
|
||||
} else if (r.status === 'rejected') {
|
||||
logger.warn('[primeInvokedSkills] Failed to prime skill files:', r.reason);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue