mirror of
https://github.com/Studio-Saelix/sencho.git
synced 2026-08-21 15:46:43 +00:00
fix(stack-files): optimistic concurrency on file-tab writes via mtime ETag (#1206)
* fix(stack-files): optimistic concurrency on file-tab writes via mtime ETag PUT /api/stacks/:name/files/content previously did a blind write; two operators editing the same script lost one of the saves with no warning. The compose-file editor already had mtime optimistic concurrency (PR #1183); this brings the file-explorer write path to the same shape. GET /files/content now also returns mtimeMs and sets a weak ETag header derived from the stat. The matching PUT reads If-Match, asks FileSystemService.writeStackFileIfUnchanged to compare against the live mtime, and returns 412 PRECONDITION_FAILED with the current content and mtime when the stale-write check fails. Successful writes echo a fresh ETag so the client can pin the next save without re-GET. readStackFile and writeStackFileIfUnchanged each open the file once and stat+read through the same handle so the mtime returned to the client matches the bytes that were sent, even if the file is replaced between the two operations. PUT without If-Match still succeeds (backward compatibility with scripted clients that do not roundtrip the ETag). FileViewer now sends the loaded mtime on save, updates its local mtime from the success response, and on FileConflictError adopts the server snapshot as the new baseline so the user's follow-up edit-and-save does not loop on the same precondition. * fix(stack-files): treat deleted-target as conflict; preserve user buffer on conflict Two follow-ups from code review on the prior commit: - writeStackFileIfUnchanged now returns ok:false when expectedMtimeMs is set and the target has been deleted. The caller was editing a file that no longer exists; silently writing the buffer to the void is wrong. The client adopts the empty snapshot as 'file is gone, start over' and the user keeps control of what to save next. - The FileViewer conflict handler no longer overwrites the user's typed buffer with the server snapshot. It updates the baseline so the next save sends the fresh mtime, then leaves the editor content alone. The user sees their edits, the Save button stays enabled, and a follow-up click applies their changes on top of the new server version without silently destroying what they typed. * fix(api): preserve default headers when caller supplies a headers field apiFetch built defaultOptions.headers by merging Content-Type, x-node-id, and the caller's headers, but then spread the unmodified fetchOptions over defaultOptions at the outer level. The spread overwrote the merged headers with the caller's bare headers, silently dropping Content-Type on every request that supplied any custom header. This was latent until the file-explorer save path started sending an If-Match header. The Express body parser refused the PUT without Content-Type, the route returned 400, the editor showed an error toast instead of the success toast, and the Playwright save assertion timed out. Destructure headers out of fetchOptions before the outer spread so the already-merged defaultOptions.headers survives. Add api.test.ts with four regression cases pinning Content-Type, the If-Match merge, x-node-id presence when active, and localOnly skip.
This commit is contained in:
Anso
@@ -170,6 +170,17 @@ describe('GET /api/stacks/:stackName/files/content', () => {
|
||||
expect(res.body.binary).toBe(false);
|
||||
expect(res.body.oversized).toBe(false);
|
||||
expect(typeof res.body.content).toBe('string');
|
||||
expect(typeof res.body.mtimeMs).toBe('number');
|
||||
expect(res.body.mtimeMs).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
it('sets a quoted ETag header derived from mtimeMs', async () => {
|
||||
const res = await request(app)
|
||||
.get(`/api/stacks/${STACK}/files/content`)
|
||||
.query({ path: 'compose.yaml' })
|
||||
.set('Cookie', adminCookie);
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.headers['etag']).toMatch(/^W\/"\d+"$/);
|
||||
});
|
||||
|
||||
it('returns 404 for a non-existent file', async () => {
|
||||
@@ -432,6 +443,103 @@ describe('PUT /api/stacks/:stackName/files/content', () => {
|
||||
const content = await fs.readFile(path.join(stacksDir, STACK, 'written.txt'), 'utf-8');
|
||||
expect(content).toBe('written via PUT');
|
||||
});
|
||||
|
||||
it('echoes a fresh ETag header on successful write', async () => {
|
||||
const res = await request(app)
|
||||
.put(`/api/stacks/${STACK}/files/content`)
|
||||
.query({ path: 'etag-write.txt' })
|
||||
.set('Cookie', adminCookie)
|
||||
.send({ content: 'etag write' });
|
||||
expect(res.status).toBe(204);
|
||||
expect(res.headers['etag']).toMatch(/^W\/"\d+"$/);
|
||||
});
|
||||
|
||||
it('returns 412 when If-Match disagrees with the current mtimeMs and surfaces the live content', async () => {
|
||||
// Seed the target so a known mtime exists.
|
||||
const target = path.join(stacksDir, STACK, 'mtime-target.txt');
|
||||
await fs.writeFile(target, 'ORIGINAL');
|
||||
|
||||
const res = await request(app)
|
||||
.put(`/api/stacks/${STACK}/files/content`)
|
||||
.query({ path: 'mtime-target.txt' })
|
||||
.set('Cookie', adminCookie)
|
||||
.set('If-Match', '"1"') // deliberately stale
|
||||
.send({ content: 'overwrite attempt' });
|
||||
|
||||
expect(res.status).toBe(412);
|
||||
expect(res.body.code).toBe('PRECONDITION_FAILED');
|
||||
expect(res.body.currentContent).toBe('ORIGINAL');
|
||||
expect(typeof res.body.currentMtimeMs).toBe('number');
|
||||
|
||||
// Disk content is unchanged.
|
||||
const after = await fs.readFile(target, 'utf-8');
|
||||
expect(after).toBe('ORIGINAL');
|
||||
});
|
||||
|
||||
it('succeeds when If-Match matches the current mtimeMs', async () => {
|
||||
const target = path.join(stacksDir, STACK, 'mtime-match.txt');
|
||||
await fs.writeFile(target, 'first');
|
||||
const getRes = await request(app)
|
||||
.get(`/api/stacks/${STACK}/files/content`)
|
||||
.query({ path: 'mtime-match.txt' })
|
||||
.set('Cookie', adminCookie);
|
||||
expect(getRes.status).toBe(200);
|
||||
const etag = getRes.headers['etag'];
|
||||
expect(etag).toBeDefined();
|
||||
|
||||
const putRes = await request(app)
|
||||
.put(`/api/stacks/${STACK}/files/content`)
|
||||
.query({ path: 'mtime-match.txt' })
|
||||
.set('Cookie', adminCookie)
|
||||
.set('If-Match', etag)
|
||||
.send({ content: 'second' });
|
||||
expect(putRes.status).toBe(204);
|
||||
expect(putRes.headers['etag']).toBeDefined();
|
||||
expect(putRes.headers['etag']).not.toBe(etag);
|
||||
|
||||
const after = await fs.readFile(target, 'utf-8');
|
||||
expect(after).toBe('second');
|
||||
});
|
||||
|
||||
it('still writes when no If-Match header is sent (backward compat)', async () => {
|
||||
const target = path.join(stacksDir, STACK, 'no-ifmatch.txt');
|
||||
await fs.writeFile(target, 'before');
|
||||
const res = await request(app)
|
||||
.put(`/api/stacks/${STACK}/files/content`)
|
||||
.query({ path: 'no-ifmatch.txt' })
|
||||
.set('Cookie', adminCookie)
|
||||
.send({ content: 'after' });
|
||||
expect(res.status).toBe(204);
|
||||
const after = await fs.readFile(target, 'utf-8');
|
||||
expect(after).toBe('after');
|
||||
});
|
||||
|
||||
it('returns 412 with an empty snapshot when If-Match was set but the target has been deleted', async () => {
|
||||
// No file at this path; the caller's If-Match implies an existing file.
|
||||
const res = await request(app)
|
||||
.put(`/api/stacks/${STACK}/files/content`)
|
||||
.query({ path: 'vanished.txt' })
|
||||
.set('Cookie', adminCookie)
|
||||
.set('If-Match', '"42"')
|
||||
.send({ content: 'i was editing this' });
|
||||
expect(res.status).toBe(412);
|
||||
expect(res.body.code).toBe('PRECONDITION_FAILED');
|
||||
expect(res.body.currentContent).toBe('');
|
||||
expect(res.body.currentMtimeMs).toBe(0);
|
||||
// Nothing was written.
|
||||
await expect(fs.access(path.join(stacksDir, STACK, 'vanished.txt'))).rejects.toThrow();
|
||||
});
|
||||
|
||||
it('still writes a fresh file when no If-Match is sent (target does not exist)', async () => {
|
||||
const res = await request(app)
|
||||
.put(`/api/stacks/${STACK}/files/content`)
|
||||
.query({ path: 'brand-new.txt' })
|
||||
.set('Cookie', adminCookie)
|
||||
.send({ content: 'first content' });
|
||||
expect(res.status).toBe(204);
|
||||
const content = await fs.readFile(path.join(stacksDir, STACK, 'brand-new.txt'), 'utf-8');
|
||||
expect(content).toBe('first content');
|
||||
});
|
||||
});
|
||||
|
||||
// ── PATCH /:stackName/files/rename ───────────────────────────────────────────
|
||||
|
||||
@@ -1405,6 +1405,10 @@ stacksRouter.get('/:stackName/files/content', async (req: Request, res: Response
|
||||
logFileDiag('read start', { stackName, relPath, nodeId: req.nodeId });
|
||||
try {
|
||||
const result = await FileSystemService.getInstance(req.nodeId).readStackFile(stackName, relPath);
|
||||
// ETag is the integer mtimeMs the file was stat'd with, so the matching
|
||||
// PUT can compare millisecond-equal even though some filesystems return
|
||||
// float mtimeMs.
|
||||
res.setHeader('ETag', stackFileEtag(result.mtimeMs));
|
||||
logFileDiag('read complete', {
|
||||
stackName,
|
||||
relPath,
|
||||
@@ -1521,10 +1525,28 @@ stacksRouter.put('/:stackName/files/content', async (req: Request, res: Response
|
||||
if (typeof content !== 'string') {
|
||||
return res.status(400).json({ error: '"content" must be a string' });
|
||||
}
|
||||
const expectedMtimeMs = parseIfMatchMtime(req.header('if-match'));
|
||||
const startedAt = Date.now();
|
||||
logFileDiag('write start', { stackName, relPath, nodeId: req.nodeId, bytes: Buffer.byteLength(content, 'utf-8') });
|
||||
logFileDiag('write start', { stackName, relPath, nodeId: req.nodeId, bytes: Buffer.byteLength(content, 'utf-8'), hasIfMatch: expectedMtimeMs !== null });
|
||||
try {
|
||||
await FileSystemService.getInstance(req.nodeId).writeStackFile(stackName, relPath, content);
|
||||
const result = await FileSystemService.getInstance(req.nodeId).writeStackFileIfUnchanged(
|
||||
stackName,
|
||||
relPath,
|
||||
content,
|
||||
expectedMtimeMs,
|
||||
);
|
||||
if (!result.ok) {
|
||||
// Stale ETag: surface the current content + mtime so the client can
|
||||
// show a "file changed elsewhere" diff and let the user reconcile.
|
||||
res.setHeader('ETag', stackFileEtag(result.currentMtimeMs));
|
||||
return res.status(412).json({
|
||||
error: 'File has been modified since you last read it. Reload to see the current version.',
|
||||
code: 'PRECONDITION_FAILED',
|
||||
currentMtimeMs: result.currentMtimeMs,
|
||||
currentContent: result.currentContent,
|
||||
});
|
||||
}
|
||||
res.setHeader('ETag', stackFileEtag(result.mtimeMs));
|
||||
logFileOperation('info', 'write complete', { nodeId: req.nodeId });
|
||||
logFileDiag('write timing', { stackName, relPath, nodeId: req.nodeId, elapsedMs: Date.now() - startedAt });
|
||||
return res.status(204).send();
|
||||
|
||||
@@ -654,31 +654,39 @@ export class FileSystemService {
|
||||
stackName: string,
|
||||
relPath: string,
|
||||
maxBytes: number = 2 * 1024 * 1024
|
||||
): Promise<{ content?: string; binary: boolean; oversized: boolean; size: number; mime: string }> {
|
||||
): Promise<{ content?: string; binary: boolean; oversized: boolean; size: number; mime: string; mtimeMs: number }> {
|
||||
const safePath = await this.resolveSafeStackPath(stackName, relPath);
|
||||
const stat = await fsPromises.stat(safePath);
|
||||
const mime = this.guessMime(safePath);
|
||||
|
||||
if (stat.isDirectory()) {
|
||||
throw Object.assign(new Error('Target is a directory'), { code: 'IS_DIRECTORY' });
|
||||
// Open once and stat+read through the same handle so the mtime returned to
|
||||
// the client matches the bytes it received, even if the file is replaced
|
||||
// (atomic rename) between the two operations.
|
||||
const fh = await fsPromises.open(safePath, 'r');
|
||||
try {
|
||||
const stat = await fh.stat();
|
||||
const mtimeMs = stat.mtimeMs;
|
||||
|
||||
if (stat.isDirectory()) {
|
||||
throw Object.assign(new Error('Target is a directory'), { code: 'IS_DIRECTORY' });
|
||||
}
|
||||
|
||||
if (stat.size > maxBytes) {
|
||||
const probe = Buffer.allocUnsafe(8192);
|
||||
const { bytesRead } = await fh.read(probe, 0, 8192, 0);
|
||||
const binary = isBinaryBuffer(probe.subarray(0, bytesRead));
|
||||
return { binary, oversized: true, size: stat.size, mime, mtimeMs };
|
||||
}
|
||||
|
||||
const buf = await fh.readFile();
|
||||
|
||||
if (isBinaryBuffer(buf)) {
|
||||
return { binary: true, oversized: false, size: stat.size, mime, mtimeMs };
|
||||
}
|
||||
|
||||
return { binary: false, oversized: false, size: stat.size, mime, mtimeMs, content: buf.toString('utf-8') };
|
||||
} finally {
|
||||
await fh.close();
|
||||
}
|
||||
|
||||
if (stat.size > maxBytes) {
|
||||
const fd = await fsPromises.open(safePath, 'r');
|
||||
const probe = Buffer.allocUnsafe(8192);
|
||||
const { bytesRead } = await fd.read(probe, 0, 8192, 0);
|
||||
await fd.close();
|
||||
const binary = isBinaryBuffer(probe.subarray(0, bytesRead));
|
||||
return { binary, oversized: true, size: stat.size, mime };
|
||||
}
|
||||
|
||||
const buf = await fsPromises.readFile(safePath);
|
||||
|
||||
if (isBinaryBuffer(buf)) {
|
||||
return { binary: true, oversized: false, size: stat.size, mime };
|
||||
}
|
||||
|
||||
return { binary: false, oversized: false, size: stat.size, mime, content: buf.toString('utf-8') };
|
||||
}
|
||||
|
||||
async streamStackFile(
|
||||
@@ -795,6 +803,58 @@ export class FileSystemService {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Optimistic-concurrency write for arbitrary stack files (file-explorer
|
||||
* editor save path). If `expectedMtimeMs` is provided, opens the target,
|
||||
* stats it, and refuses the write (returning current content + mtime) when
|
||||
* the stat does not match the caller's expectation. Mirrors the
|
||||
* compose-file pattern in saveStackContentIfUnchanged.
|
||||
*
|
||||
* Mtime comparison uses Math.floor so sub-millisecond jitter between
|
||||
* different filesystems and Node versions does not produce false 412s.
|
||||
*
|
||||
* If the file does not exist yet, the write proceeds (no mtime to compare).
|
||||
* Returns the new mtimeMs so the route can emit a fresh ETag.
|
||||
*/
|
||||
async writeStackFileIfUnchanged(
|
||||
stackName: string,
|
||||
relPath: string,
|
||||
content: string,
|
||||
expectedMtimeMs: number | null,
|
||||
): Promise<
|
||||
| { ok: true; mtimeMs: number }
|
||||
| { ok: false; currentMtimeMs: number; currentContent: string }
|
||||
> {
|
||||
const safePath = await this.resolveSafeStackPath(stackName, relPath);
|
||||
await fsPromises.mkdir(path.dirname(safePath), { recursive: true });
|
||||
|
||||
if (expectedMtimeMs !== null) {
|
||||
let fh: import('fs/promises').FileHandle | null = null;
|
||||
try {
|
||||
fh = await fsPromises.open(safePath, 'r');
|
||||
const stat = await fh.stat();
|
||||
if (Math.floor(stat.mtimeMs) !== Math.floor(expectedMtimeMs)) {
|
||||
const currentContent = await fh.readFile('utf-8');
|
||||
return { ok: false, currentMtimeMs: stat.mtimeMs, currentContent };
|
||||
}
|
||||
} catch (err) {
|
||||
if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err;
|
||||
// The caller expected an existing file but it has been deleted under
|
||||
// the editor. That is itself a conflict (the file is gone, the user
|
||||
// is editing into a void) so surface it the same way as a stale-mtime
|
||||
// mismatch. An empty current snapshot tells the client "the live
|
||||
// version is gone, you are starting from scratch".
|
||||
return { ok: false, currentMtimeMs: 0, currentContent: '' };
|
||||
} finally {
|
||||
if (fh) await fh.close();
|
||||
}
|
||||
}
|
||||
|
||||
await fsPromises.writeFile(safePath, content, 'utf-8');
|
||||
const newStat = await fsPromises.stat(safePath);
|
||||
return { ok: true, mtimeMs: newStat.mtimeMs };
|
||||
}
|
||||
|
||||
async deleteStackPath(stackName: string, relPath: string, recursive: boolean = false): Promise<void> {
|
||||
if (isProtectedRelPath(relPath)) throw protectedFileError(relPath);
|
||||
const safePath = await this.resolveSafeStackPath(stackName, relPath);
|
||||
|
||||
Reference in New Issue
Block a user