mirror of https://github.com/garrytan/gstack.git
fix: reserve design variant destinations atomically
Destination labels were derived from the manifest's attempt count before generation, so two concurrent runs could pick the same variant-iteration-<label> path and overwrite each other's variant. Claim each destination atomically with an exclusive create (open "wx"), advancing to the next label on EEXIST, and guard the round-manifest read-modify-write with a lock file plus a temp-file + rename so concurrent updates don't drop an entry. Add concurrency and failure/restart tests asserting distinct labels and that no variant is overwritten.
This commit is contained in:
parent
0930beb099
commit
33f235dd62
|
|
@ -321,12 +321,15 @@ async function main(argv = process.argv): Promise<void> {
|
||||||
}
|
}
|
||||||
|
|
||||||
const ROUND_MANIFEST = ".gstack-design-rounds.json";
|
const ROUND_MANIFEST = ".gstack-design-rounds.json";
|
||||||
|
const ROUND_MANIFEST_LOCK = ".gstack-design-rounds.lock";
|
||||||
|
const ROUND_RESERVATION_LIMIT = 1000;
|
||||||
|
|
||||||
interface RoundAttempt {
|
interface RoundAttempt {
|
||||||
label: string;
|
label: string;
|
||||||
path: string;
|
path: string;
|
||||||
success: boolean;
|
success: boolean;
|
||||||
error?: string;
|
error?: string;
|
||||||
|
reserved?: boolean;
|
||||||
}
|
}
|
||||||
|
|
||||||
type RoundManifest = Record<string, RoundAttempt[]>;
|
type RoundManifest = Record<string, RoundAttempt[]>;
|
||||||
|
|
@ -368,7 +371,37 @@ function readRoundManifest(dir: string): RoundManifest {
|
||||||
|
|
||||||
function writeRoundManifest(dir: string, manifest: RoundManifest): void {
|
function writeRoundManifest(dir: string, manifest: RoundManifest): void {
|
||||||
fs.mkdirSync(dir, { recursive: true });
|
fs.mkdirSync(dir, { recursive: true });
|
||||||
fs.writeFileSync(path.join(dir, ROUND_MANIFEST), JSON.stringify(manifest, null, 2));
|
const manifestPath = path.join(dir, ROUND_MANIFEST);
|
||||||
|
const tempPath = path.join(dir, `${ROUND_MANIFEST}.${process.pid}.${Date.now()}.tmp`);
|
||||||
|
fs.writeFileSync(tempPath, JSON.stringify(manifest, null, 2));
|
||||||
|
fs.renameSync(tempPath, manifestPath);
|
||||||
|
}
|
||||||
|
|
||||||
|
function withRoundManifestLock<T>(dir: string, action: () => T): T {
|
||||||
|
fs.mkdirSync(dir, { recursive: true });
|
||||||
|
const lockPath = path.join(dir, ROUND_MANIFEST_LOCK);
|
||||||
|
let lockFd: number | undefined;
|
||||||
|
|
||||||
|
for (let attempt = 0; attempt < ROUND_RESERVATION_LIMIT; attempt++) {
|
||||||
|
try {
|
||||||
|
lockFd = fs.openSync(lockPath, "wx");
|
||||||
|
break;
|
||||||
|
} catch (err: any) {
|
||||||
|
if (err.code !== "EEXIST") throw err;
|
||||||
|
Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 1);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
if (lockFd === undefined) {
|
||||||
|
throw new Error(`Timed out reserving design round artifacts in ${dir}`);
|
||||||
|
}
|
||||||
|
|
||||||
|
try {
|
||||||
|
return action();
|
||||||
|
} finally {
|
||||||
|
fs.closeSync(lockFd);
|
||||||
|
fs.unlinkSync(lockPath);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
function planRoundArtifacts(outputPath: string): RoundArtifactPlan | null {
|
function planRoundArtifacts(outputPath: string): RoundArtifactPlan | null {
|
||||||
|
|
@ -376,32 +409,50 @@ function planRoundArtifacts(outputPath: string): RoundArtifactPlan | null {
|
||||||
if (!baseName) return null;
|
if (!baseName) return null;
|
||||||
|
|
||||||
const dir = path.dirname(outputPath);
|
const dir = path.dirname(outputPath);
|
||||||
const manifest = readRoundManifest(dir);
|
|
||||||
const key = roundKey(outputPath, baseName);
|
const key = roundKey(outputPath, baseName);
|
||||||
const attempts = manifest[key] || [];
|
return withRoundManifestLock(dir, () => {
|
||||||
const label = labelForIndex(attempts.length);
|
const manifest = readRoundManifest(dir);
|
||||||
|
const attempts = manifest[key] || [];
|
||||||
|
|
||||||
return {
|
for (let index = attempts.length; index < attempts.length + ROUND_RESERVATION_LIMIT; index++) {
|
||||||
aliasOutput: outputPath,
|
const label = labelForIndex(index);
|
||||||
primaryOutput: path.join(dir, `${baseName}-${label}.png`),
|
const primaryOutput = path.join(dir, `${baseName}-${label}.png`);
|
||||||
roundKey: key,
|
try {
|
||||||
label,
|
fs.closeSync(fs.openSync(primaryOutput, "wx"));
|
||||||
};
|
} catch (err: any) {
|
||||||
|
if (err.code === "EEXIST") continue;
|
||||||
|
throw err;
|
||||||
|
}
|
||||||
|
|
||||||
|
attempts.push({ label, path: primaryOutput, success: false, reserved: true });
|
||||||
|
manifest[key] = attempts;
|
||||||
|
writeRoundManifest(dir, manifest);
|
||||||
|
return { aliasOutput: outputPath, primaryOutput, roundKey: key, label };
|
||||||
|
}
|
||||||
|
|
||||||
|
throw new Error(`Unable to reserve a design round artifact after ${ROUND_RESERVATION_LIMIT} attempts`);
|
||||||
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
function recordRoundAttempt(plan: RoundArtifactPlan, success: boolean, error?: string): void {
|
function recordRoundAttempt(plan: RoundArtifactPlan, success: boolean, error?: string): void {
|
||||||
const dir = path.dirname(plan.aliasOutput);
|
const dir = path.dirname(plan.aliasOutput);
|
||||||
const manifest = readRoundManifest(dir);
|
withRoundManifestLock(dir, () => {
|
||||||
const attempts = manifest[plan.roundKey] || [];
|
const manifest = readRoundManifest(dir);
|
||||||
const existing = attempts.find(attempt => attempt.label === plan.label);
|
const attempts = manifest[plan.roundKey] || [];
|
||||||
const attempt = { label: plan.label, path: plan.primaryOutput, success, error };
|
const existing = attempts.find(attempt => attempt.label === plan.label);
|
||||||
if (existing) {
|
const attempt = { label: plan.label, path: plan.primaryOutput, success, error };
|
||||||
Object.assign(existing, attempt);
|
if (existing) {
|
||||||
} else {
|
Object.assign(existing, attempt);
|
||||||
attempts.push(attempt);
|
} else {
|
||||||
}
|
attempts.push(attempt);
|
||||||
manifest[plan.roundKey] = attempts;
|
}
|
||||||
writeRoundManifest(dir, manifest);
|
manifest[plan.roundKey] = attempts;
|
||||||
|
writeRoundManifest(dir, manifest);
|
||||||
|
|
||||||
|
if (!success && fs.existsSync(plan.primaryOutput) && fs.statSync(plan.primaryOutput).size === 0) {
|
||||||
|
fs.unlinkSync(plan.primaryOutput);
|
||||||
|
}
|
||||||
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
function copyRoundAlias(plan: RoundArtifactPlan): void {
|
function copyRoundAlias(plan: RoundArtifactPlan): void {
|
||||||
|
|
@ -630,5 +681,6 @@ export {
|
||||||
main,
|
main,
|
||||||
resolveImagePaths,
|
resolveImagePaths,
|
||||||
planRoundArtifacts,
|
planRoundArtifacts,
|
||||||
|
recordRoundAttempt,
|
||||||
resolveRoundImageAlias,
|
resolveRoundImageAlias,
|
||||||
};
|
};
|
||||||
|
|
|
||||||
|
|
@ -4,6 +4,7 @@ import os from "os";
|
||||||
import path from "path";
|
import path from "path";
|
||||||
import {
|
import {
|
||||||
planRoundArtifacts,
|
planRoundArtifacts,
|
||||||
|
recordRoundAttempt,
|
||||||
resolveImagePaths,
|
resolveImagePaths,
|
||||||
} from "../src/cli";
|
} from "../src/cli";
|
||||||
|
|
||||||
|
|
@ -51,6 +52,40 @@ describe("plan-design-review round variant preservation", () => {
|
||||||
expect(second?.primaryOutput).toBe(path.join(tmpDir, "variant-recommended-B.png"));
|
expect(second?.primaryOutput).toBe(path.join(tmpDir, "variant-recommended-B.png"));
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test("concurrent round reservations claim distinct paths and preserve every variant", async () => {
|
||||||
|
const alias = path.join(tmpDir, "variant-recommended.png");
|
||||||
|
const plans = await Promise.all(
|
||||||
|
Array.from({ length: 3 }, async () => {
|
||||||
|
const plan = planRoundArtifacts(alias)!;
|
||||||
|
await Promise.resolve();
|
||||||
|
writePng(plan.primaryOutput);
|
||||||
|
recordRoundAttempt(plan, true);
|
||||||
|
return plan;
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(plans.map(plan => plan.label).sort()).toEqual(["A", "B", "C"]);
|
||||||
|
expect(new Set(plans.map(plan => plan.primaryOutput)).size).toBe(3);
|
||||||
|
expect(plans.every(plan => fs.existsSync(plan.primaryOutput))).toBe(true);
|
||||||
|
await expect(resolveImagePaths(alias)).resolves.toEqual(
|
||||||
|
plans.map(plan => plan.primaryOutput),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
test("failed reservations are recorded without blocking a later variant", () => {
|
||||||
|
const alias = path.join(tmpDir, "variant-recommended.png");
|
||||||
|
const failed = planRoundArtifacts(alias)!;
|
||||||
|
recordRoundAttempt(failed, false, "API error");
|
||||||
|
|
||||||
|
const retry = planRoundArtifacts(alias)!;
|
||||||
|
writePng(retry.primaryOutput);
|
||||||
|
recordRoundAttempt(retry, true);
|
||||||
|
|
||||||
|
expect(retry.label).toBe("B");
|
||||||
|
expect(fs.existsSync(failed.primaryOutput)).toBe(false);
|
||||||
|
expect(fs.existsSync(retry.primaryOutput)).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
test("recommended round alias expands to every successful generated candidate", async () => {
|
test("recommended round alias expands to every successful generated candidate", async () => {
|
||||||
const alias = path.join(tmpDir, "variant-recommended.png");
|
const alias = path.join(tmpDir, "variant-recommended.png");
|
||||||
const variantA = path.join(tmpDir, "variant-recommended-A.png");
|
const variantA = path.join(tmpDir, "variant-recommended-A.png");
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue