fix: address codex review round 3
Record the freshness token observed AFTER the read (re-stat post-read, falling back to the routing stat if the file vanished) so the version returned/recorded matches the bytes returned — a writer racing between the routing stat and the read can no longer make a follow-up edit spuriously stale. Stream reads when the backend reports no size, so a size-less backend never buffers a large file whole. Update the cordis-catalog link map to the current filesystem API symbols (FileContextExec/FileReadRequest/FileReadOutcome/FsInfo/FsWriteExpectation).
This commit is contained in:
@@ -118,27 +118,36 @@ export class FileContext extends Service {
|
||||
/**
|
||||
* Read a bounded line window from a target. Stats first (rejecting an absent
|
||||
* target with `FS_NOT_FOUND` and a non-regular one with `FS_NOT_REGULAR_FILE`),
|
||||
* chooses `readText` vs `streamText` by size, builds the window, and — when an
|
||||
* owner is derivable — records the version so a later write/edit is authorized.
|
||||
* chooses `readText` vs `streamText` by size — streaming when the size is
|
||||
* large OR unknown so a size-less backend never buffers an arbitrarily large
|
||||
* file — builds the window, then records the version observed AFTER the read
|
||||
* so the recorded freshness token corresponds to the bytes actually returned
|
||||
* (a writer racing between the routing stat and the read can't make a
|
||||
* follow-up edit spuriously stale against a pre-read version).
|
||||
*/
|
||||
async read(target: FsTarget, request: FileReadRequest, exec?: FileContextExec, signal?: AbortSignal): Promise<FileReadOutcome> {
|
||||
const info = await this.ctx.fs.stat(target, signal)
|
||||
if (!info) throw new FsError(`cannot read "${target.displayPath}": not found`, 'FS_NOT_FOUND')
|
||||
if (info.type !== 'file') throw new FsError(`cannot read "${target.displayPath}": not a regular file`, 'FS_NOT_REGULAR_FILE')
|
||||
|
||||
const chunks = info.size !== undefined && info.size >= STREAM_MIN_SIZE
|
||||
const chunks = info.size === undefined || info.size >= STREAM_MIN_SIZE
|
||||
? await this.ctx.fs.streamText(target, signal)
|
||||
: [await this.ctx.fs.readText(target, signal)]
|
||||
const window = await buildWindow(chunks, request, target.displayPath)
|
||||
|
||||
// The version that matches the bytes just read: a stat taken after the read
|
||||
// (falling back to the routing stat if the file vanished in the interim).
|
||||
const after = await this.ctx.fs.stat(target, signal)
|
||||
const version = after?.version ?? info.version
|
||||
|
||||
const owner = this.owner(exec)
|
||||
if (owner) this.record(owner, target.targetKey, info.version)
|
||||
if (owner) this.record(owner, target.targetKey, version)
|
||||
return {
|
||||
offset: request.offset,
|
||||
limit: request.limit,
|
||||
lines: window.lines,
|
||||
totalLines: window.totalLines,
|
||||
version: info.version,
|
||||
version,
|
||||
...window.truncatedByBytes ? { truncatedByBytes: true } : {},
|
||||
}
|
||||
}
|
||||
|
||||
@@ -27,6 +27,8 @@ class FakeFs extends FileSystem {
|
||||
versions = new Map<string, number>()
|
||||
/** Size to report from stat (lets a test push read onto the streaming path). */
|
||||
reportSize?: number
|
||||
/** When true, stat omits `size` entirely (a size-less backend). */
|
||||
omitSize = false
|
||||
/** Whether streamText was used for the last read (vs readText). */
|
||||
lastReadStreamed = false
|
||||
writeExpectations: FsWriteExpectation[] = []
|
||||
@@ -47,7 +49,7 @@ class FakeFs extends FileSystem {
|
||||
override async stat(target: FsTarget): Promise<FsInfo | undefined> {
|
||||
const content = this.files.get(target.targetKey)
|
||||
if (content === undefined) return undefined
|
||||
return { version: this.ver(target.targetKey), type: 'file', size: this.reportSize ?? content.length }
|
||||
return { version: this.ver(target.targetKey), type: 'file', ...this.omitSize ? {} : { size: this.reportSize ?? content.length } }
|
||||
}
|
||||
override async readText(target: FsTarget): Promise<string> {
|
||||
this.lastReadStreamed = false
|
||||
@@ -154,6 +156,48 @@ describe('read', () => {
|
||||
expect(fs.lastReadStreamed).toBe(true)
|
||||
})
|
||||
|
||||
it('streams when the backend reports no size (never buffers a size-less file)', async () => {
|
||||
const { fs, fileContext } = await setup()
|
||||
fs.files.set('a.txt', 'one\ntwo')
|
||||
fs.omitSize = true
|
||||
await fileContext.read(await fs.resolve('a.txt'), READ_ALL)
|
||||
expect(fs.lastReadStreamed).toBe(true)
|
||||
})
|
||||
|
||||
it('records the version observed after the read, not the routing stat', async () => {
|
||||
const { fs, fileContext } = await setup()
|
||||
const exec = ownerExec({})
|
||||
fs.files.set('a.txt', 'hello')
|
||||
fs.versions.set('a.txt', 1)
|
||||
const target = await fs.resolve('a.txt')
|
||||
// A writer bumps the version after the routing stat but before the post-read stat.
|
||||
const realReadText = fs.readText.bind(fs)
|
||||
fs.readText = async (t) => {
|
||||
const text = await realReadText(t)
|
||||
fs.versions.set('a.txt', 5) // file changed during the read
|
||||
return text
|
||||
}
|
||||
const outcome = await fileContext.read(target, READ_ALL, exec)
|
||||
expect(outcome.version).toBe('v5')
|
||||
// The recorded (post-read) version authorizes an edit without going stale.
|
||||
await fileContext.edit(target, { oldString: 'hello', newString: 'bye', replaceAll: false }, exec)
|
||||
expect(fs.editExpectedVersions).toEqual(['v5'])
|
||||
})
|
||||
|
||||
it('falls back to the routing-stat version if the file vanishes after the read', async () => {
|
||||
const { fs, fileContext } = await setup()
|
||||
fs.files.set('a.txt', 'hello')
|
||||
const target = await fs.resolve('a.txt')
|
||||
const realReadText = fs.readText.bind(fs)
|
||||
fs.readText = async (t) => {
|
||||
const text = await realReadText(t)
|
||||
fs.files.delete('a.txt') // vanishes → post-read stat returns undefined
|
||||
return text
|
||||
}
|
||||
const outcome = await fileContext.read(target, READ_ALL)
|
||||
expect(outcome.version).toBe('v0') // the routing-stat version
|
||||
})
|
||||
|
||||
it('surfaces truncatedByBytes when the window hits the byte cap', async () => {
|
||||
const { fs, fileContext } = await setup()
|
||||
fs.files.set('big.txt', Array.from({ length: 2000 }, () => 'y'.repeat(100)).join('\n'))
|
||||
|
||||
Reference in New Issue
Block a user