fix: Prevent overlapping skill sync runs

This commit is contained in:
Danny Avila 2026-05-24 15:57:12 -04:00
parent cc72b2a7c7
commit 4abf8ba50a
No known key found for this signature in database
GPG key ID: BF31EEB2C5CA0956
7 changed files with 129 additions and 16 deletions

View file

@ -48,6 +48,7 @@ function createRunner() {
listStatuses: db.listSkillSyncStatuses,
upsertStatus: db.upsertSkillSyncStatus,
tryAcquireLock: db.tryAcquireSkillSyncLock,
refreshLock: db.refreshSkillSyncLock,
releaseLock: db.releaseSkillSyncLock,
createSkill: db.createSkill,
updateSkill: db.updateSkill,

View file

@ -128,6 +128,13 @@ describe('parseFrontmatter', () => {
expect(result.invalidBooleans).toEqual(['always-apply']);
});
it('flags legacy YAML boolean aliases as invalid', () => {
const raw = `---\nname: n\ndescription: d\nalways-apply: on\n---\n\nbody`;
const result = parseFrontmatter(raw);
expect(result.alwaysApply).toBeUndefined();
expect(result.invalidBooleans).toEqual(['always-apply']);
});
it('does not flag always-apply when the key is absent', () => {
const raw = `---\nname: n\ndescription: d\n---\n\nbody`;
expect(parseFrontmatter(raw).invalidBooleans).toEqual([]);

View file

@ -30,6 +30,29 @@ function getCaseInsensitive(frontmatter: Record<string, unknown>, key: string):
return entry?.[1];
}
function getRawFrontmatterValue(block: string, key: string): string | undefined {
const escapedKey = key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
const pattern = new RegExp(`^\\s*${escapedKey}\\s*:\\s*(.*)$`, 'i');
const line = block.split('\n').find((candidate) => pattern.test(candidate));
const match = line?.match(pattern);
return match?.[1];
}
function stripInlineComment(value: string): string {
let quote: '"' | "'" | null = null;
for (let i = 0; i < value.length; i++) {
const char = value[i];
if ((char === '"' || char === "'") && (!quote || quote === char)) {
quote = quote ? null : char;
continue;
}
if (char === '#' && !quote) {
return value.slice(0, i).trim();
}
}
return value.trim();
}
function normalizeFrontmatterKeys(frontmatter: Record<string, unknown>): Record<string, unknown> {
return Object.entries(frontmatter).reduce<Record<string, unknown>>((acc, [key, value]) => {
acc[key.toLowerCase()] = value;
@ -37,9 +60,10 @@ function normalizeFrontmatterKeys(frontmatter: Record<string, unknown>): Record<
}, {});
}
function parseBoolean(value: unknown): boolean | undefined {
function parseBoolean(value: unknown, rawValue?: string): boolean | undefined {
const raw = rawValue === undefined ? undefined : stripInlineComment(rawValue).toLowerCase();
if (typeof value === 'boolean') {
return value;
return raw === 'true' || raw === 'false' ? value : undefined;
}
if (typeof value !== 'string') {
return undefined;
@ -75,6 +99,7 @@ export function parseSkillMarkdown(raw: string): ParsedSkillMarkdown {
const descriptionValue = getCaseInsensitive(frontmatter, 'description');
const whenToUseValue = getCaseInsensitive(frontmatter, 'when-to-use');
const alwaysApplyValue = getCaseInsensitive(frontmatter, 'always-apply');
const rawAlwaysApplyValue = getRawFrontmatterValue(block, 'always-apply');
const name = typeof nameValue === 'string' ? nameValue : '';
let description = '';
if (typeof descriptionValue === 'string') {
@ -85,7 +110,7 @@ export function parseSkillMarkdown(raw: string): ParsedSkillMarkdown {
let alwaysApply: boolean | undefined;
const invalidBooleans: string[] = [];
if (alwaysApplyValue !== undefined) {
alwaysApply = parseBoolean(alwaysApplyValue);
alwaysApply = parseBoolean(alwaysApplyValue, rawAlwaysApplyValue);
if (alwaysApply === undefined && alwaysApplyValue !== null && alwaysApplyValue !== '') {
invalidBooleans.push('always-apply');
}

View file

@ -145,6 +145,7 @@ function createDeps(
return status;
}),
tryAcquireLock: jest.fn(async () => true),
refreshLock: jest.fn(async () => true),
releaseLock: jest.fn(async () => undefined),
createSkill: jest.fn(async (input: CreateSkillInput): Promise<CreateSkillResult> => {
return { skill: makeSkill(input), warnings: [] };

View file

@ -99,6 +99,11 @@ export type GitHubSkillSyncDeps = {
lockOwner: string;
leaseMs: number;
}) => Promise<boolean>;
refreshLock: (params: {
provider: SkillSyncProvider;
lockOwner: string;
leaseMs: number;
}) => Promise<boolean>;
releaseLock: (params: { provider: SkillSyncProvider; lockOwner: string }) => Promise<void>;
createSkill: (data: CreateSkillInput) => Promise<CreateSkillResult>;
updateSkill: (params: {
@ -879,6 +884,22 @@ export function createGitHubSkillSyncRunner(deps: GitHubSkillSyncDeps) {
sources: statuses,
};
}
const refreshTimer = setInterval(
() => {
deps
.refreshLock({ provider: PROVIDER, lockOwner, leaseMs: LOCK_LEASE_MS })
.then((refreshed) => {
if (!refreshed) {
logger.warn('[GitHubSkillSync] Failed to refresh active sync lock');
}
})
.catch((error) => {
logger.error('[GitHubSkillSync] Failed to refresh active sync lock:', error);
});
},
Math.max(60_000, Math.floor(LOCK_LEASE_MS / 3)),
);
refreshTimer.unref?.();
try {
const sources: ISkillSyncStatus[] = [];
for (const source of github.sources) {
@ -890,6 +911,7 @@ export function createGitHubSkillSyncRunner(deps: GitHubSkillSyncDeps) {
sources,
};
} finally {
clearInterval(refreshTimer);
await deps.releaseLock({ provider: PROVIDER, lockOwner });
}
}

View file

@ -79,6 +79,13 @@ describe('createSkillSyncMethods', () => {
leaseMs: 60_000,
}),
).resolves.toBe(true);
await expect(
methods.tryAcquireSkillSyncLock({
provider: 'github',
lockOwner: 'worker-a',
leaseMs: 60_000,
}),
).resolves.toBe(false);
await expect(
methods.tryAcquireSkillSyncLock({
provider: 'github',
@ -86,6 +93,13 @@ describe('createSkillSyncMethods', () => {
leaseMs: 60_000,
}),
).resolves.toBe(false);
await expect(
methods.refreshSkillSyncLock({
provider: 'github',
lockOwner: 'worker-a',
leaseMs: 60_000,
}),
).resolves.toBe(true);
await methods.releaseSkillSyncLock({ provider: 'github', lockOwner: 'worker-a' });
await expect(
methods.tryAcquireSkillSyncLock({
@ -95,4 +109,26 @@ describe('createSkillSyncMethods', () => {
}),
).resolves.toBe(true);
});
it('clears stale source errors after a successful sync status update', async () => {
await methods.upsertSkillSyncStatus({
provider: 'github',
sourceId: 'librechat-skills',
status: 'failed',
errorCode: 'SKILL_PARSE_FAILED',
errorMessage: 'bad frontmatter',
});
const success = await methods.upsertSkillSyncStatus({
provider: 'github',
sourceId: 'librechat-skills',
status: 'succeeded',
syncedSkillCount: 1,
syncedFileCount: 2,
});
expect(success.status).toBe('succeeded');
expect(success.errorCode).toBeUndefined();
expect(success.errorMessage).toBeUndefined();
});
});

View file

@ -175,8 +175,6 @@ export function createSkillSyncMethods(mongoose: typeof import('mongoose')) {
paths: input.paths,
startedAt: input.startedAt,
finishedAt: input.finishedAt,
errorCode: input.errorCode,
errorMessage: input.errorMessage,
syncedSkillCount: input.syncedSkillCount ?? 0,
syncedFileCount: input.syncedFileCount ?? 0,
deletedSkillCount: input.deletedSkillCount ?? 0,
@ -184,10 +182,16 @@ export function createSkillSyncMethods(mongoose: typeof import('mongoose')) {
...(success ? { lastSuccessAt: input.finishedAt ?? now } : {}),
...(failure ? { lastFailureAt: input.finishedAt ?? now } : {}),
};
if (failure) {
setPayload.errorCode = input.errorCode;
setPayload.errorMessage = input.errorMessage;
}
const unsetPayload = failure ? {} : { errorCode: '', errorMessage: '' };
const row = await Status.findOneAndUpdate(
{ provider: input.provider, sourceId: input.sourceId },
{
$set: setPayload,
...(failure ? {} : { $unset: unsetPayload }),
$setOnInsert: {
provider: input.provider,
sourceId: input.sourceId,
@ -210,12 +214,7 @@ export function createSkillSyncMethods(mongoose: typeof import('mongoose')) {
provider: params.provider,
sourceId: LOCK_SOURCE_ID,
}).lean<ISkillSyncStatus | null>();
if (
existing?.lockOwner &&
existing.lockOwner !== params.lockOwner &&
existing.lockExpiresAt &&
existing.lockExpiresAt > now
) {
if (existing?.lockOwner && existing.lockExpiresAt && existing.lockExpiresAt > now) {
return false;
}
if (!existing) {
@ -241,11 +240,7 @@ export function createSkillSyncMethods(mongoose: typeof import('mongoose')) {
{
provider: params.provider,
sourceId: LOCK_SOURCE_ID,
$or: [
{ lockExpiresAt: { $exists: false } },
{ lockExpiresAt: { $lte: now } },
{ lockOwner: params.lockOwner },
],
$or: [{ lockExpiresAt: { $exists: false } }, { lockExpiresAt: { $lte: now } }],
},
{
$set: {
@ -270,6 +265,31 @@ export function createSkillSyncMethods(mongoose: typeof import('mongoose')) {
}
}
async function refreshSkillSyncLock(params: {
provider: SkillSyncProvider;
lockOwner: string;
leaseMs: number;
}): Promise<boolean> {
const Status = mongoose.models.SkillSyncStatus as Model<ISkillSyncStatusDocument>;
const now = new Date();
const row = await Status.findOneAndUpdate(
{
provider: params.provider,
sourceId: LOCK_SOURCE_ID,
lockOwner: params.lockOwner,
lockExpiresAt: { $gt: now },
},
{
$set: {
status: 'running',
lockExpiresAt: new Date(now.getTime() + params.leaseMs),
},
},
{ new: true },
).lean<ISkillSyncStatus | null>();
return Boolean(row);
}
async function releaseSkillSyncLock(params: {
provider: SkillSyncProvider;
lockOwner: string;
@ -300,6 +320,7 @@ export function createSkillSyncMethods(mongoose: typeof import('mongoose')) {
getSkillSyncStatus,
upsertSkillSyncStatus,
tryAcquireSkillSyncLock,
refreshSkillSyncLock,
releaseSkillSyncLock,
};
}