fix: vfs.rename() issues a metadata-only update + rollback of fresh adds removes them
Two fixes reported/found by downstream consumers: (1) vfs.rename() spread the entire fetched entity into brain.update(), forwarding a vector field that failed the 384-dimension validation when the entity was fetched without vectors — every rename threw. A rename is a path/metadata change, never a content change: the update is now metadata-only (no vector touched, no re-embedding). Regression test covers the verbatim consumer repro plus cross-directory moves and EEXIST. (2) AddToHNSWOperation.itemExists() probed an optional getItem capability with `await (index as any).getItem?.(id)` — `await undefined` resolves without throwing, so every item was reported as pre-existing and rollback of fresh adds never removed them, leaving phantom index entries after a failed transaction. The probe now feature-detects the method and defaults to false when absent; update flows pair this op with RemoveFromHNSWOperation whose own rollback restores the prior vector, so reverse-order rollback reconstructs the original state either way. 1479/1479 unit suite passing.
This commit is contained in:
parent
9b52629a0a
commit
ac29b0e650
3 changed files with 97 additions and 18 deletions
|
|
@ -51,13 +51,26 @@ export class AddToHNSWOperation implements Operation {
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Check if item exists in index
|
* Check if item exists in index.
|
||||||
|
*
|
||||||
|
* `getItem` is an optional provider capability that the built-in HNSW index
|
||||||
|
* does not implement. When the capability is missing the answer must be
|
||||||
|
* `false`, not `true`: `await undefined` resolves without throwing, so the
|
||||||
|
* old probe reported every item as pre-existing and rollback of fresh adds
|
||||||
|
* never removed them — leaving phantom index entries after a failed
|
||||||
|
* transaction. The safe default is to remove what this operation added;
|
||||||
|
* update flows pair this op with a RemoveFromHNSWOperation whose own
|
||||||
|
* rollback restores the prior vector, so reverse-order rollback
|
||||||
|
* reconstructs the original state either way.
|
||||||
*/
|
*/
|
||||||
private async itemExists(id: string): Promise<boolean> {
|
private async itemExists(id: string): Promise<boolean> {
|
||||||
|
const index = this.index as HNSWIndex & {
|
||||||
|
getItem?: (id: string) => Promise<unknown>
|
||||||
|
}
|
||||||
|
if (typeof index.getItem !== 'function') return false
|
||||||
try {
|
try {
|
||||||
// Try to get item - if exists, no error
|
const item = await index.getItem(id)
|
||||||
await (this.index as any).getItem?.(id)
|
return item !== undefined && item !== null
|
||||||
return true
|
|
||||||
} catch {
|
} catch {
|
||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -1664,17 +1664,6 @@ export class VirtualFileSystem implements IVirtualFileSystem {
|
||||||
if (err.code !== VFSErrorCode.ENOENT) throw err
|
if (err.code !== VFSErrorCode.ENOENT) throw err
|
||||||
}
|
}
|
||||||
|
|
||||||
// Update entity metadata
|
|
||||||
const updatedEntity = {
|
|
||||||
...entity,
|
|
||||||
metadata: {
|
|
||||||
...entity.metadata,
|
|
||||||
path: newPath,
|
|
||||||
name: this.getBasename(newPath),
|
|
||||||
modified: Date.now()
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
// Update parent relationships if needed
|
// Update parent relationships if needed
|
||||||
const oldParentPath = this.getParentPath(oldPath)
|
const oldParentPath = this.getParentPath(oldPath)
|
||||||
const newParentPath = this.getParentPath(newPath)
|
const newParentPath = this.getParentPath(newPath)
|
||||||
|
|
@ -1700,10 +1689,19 @@ export class VirtualFileSystem implements IVirtualFileSystem {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Update the entity
|
// A rename is a path/metadata change, never a content change — issue a
|
||||||
|
// metadata-only update. Spreading the whole entity here used to forward
|
||||||
|
// its vector field into update(), which failed dimension validation when
|
||||||
|
// the entity was fetched without vectors (and would needlessly touch the
|
||||||
|
// vector index when it wasn't).
|
||||||
await this.brain.update({
|
await this.brain.update({
|
||||||
...updatedEntity,
|
id: entityId,
|
||||||
id: entityId
|
metadata: {
|
||||||
|
...entity.metadata,
|
||||||
|
path: newPath,
|
||||||
|
name: this.getBasename(newPath),
|
||||||
|
modified: Date.now()
|
||||||
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
// Update path cache
|
// Update path cache
|
||||||
|
|
|
||||||
68
tests/unit/vfs/vfs-rename-metadata-only.test.ts
Normal file
68
tests/unit/vfs/vfs-rename-metadata-only.test.ts
Normal file
|
|
@ -0,0 +1,68 @@
|
||||||
|
/**
|
||||||
|
* @module vfs/vfs-rename-metadata-only.test
|
||||||
|
* @description Regression test for the vfs.rename() vector-dimension crash
|
||||||
|
* (7.31.7). rename() used to spread the entire fetched entity into
|
||||||
|
* brain.update(), forwarding a vector field that failed the 384-dimension
|
||||||
|
* validation when the entity was fetched without vectors. A rename is a
|
||||||
|
* path/metadata change — the update must be metadata-only.
|
||||||
|
*
|
||||||
|
* Production repro (verbatim from the consumer report):
|
||||||
|
* await brain.vfs.writeFile('/a.txt', 'x')
|
||||||
|
* await brain.vfs.rename('/a.txt', '/b.txt') // threw pre-7.31.7
|
||||||
|
*/
|
||||||
|
|
||||||
|
import { describe, it, expect, beforeEach, afterEach } from 'vitest'
|
||||||
|
import { Brainy } from '../../../src/index.js'
|
||||||
|
|
||||||
|
describe('vfs.rename() issues a metadata-only update (7.31.7)', () => {
|
||||||
|
let brain: Brainy
|
||||||
|
|
||||||
|
beforeEach(async () => {
|
||||||
|
brain = new Brainy({ storage: { type: 'memory' } })
|
||||||
|
await brain.init()
|
||||||
|
})
|
||||||
|
|
||||||
|
afterEach(async () => {
|
||||||
|
await brain.close()
|
||||||
|
})
|
||||||
|
|
||||||
|
it('renames a file without touching vectors (the production repro)', async () => {
|
||||||
|
await brain.vfs.writeFile('/a.txt', 'x')
|
||||||
|
await brain.vfs.rename('/a.txt', '/b.txt')
|
||||||
|
|
||||||
|
expect(await brain.vfs.exists('/b.txt')).toBe(true)
|
||||||
|
expect(await brain.vfs.exists('/a.txt')).toBe(false)
|
||||||
|
const content = await brain.vfs.readFile('/b.txt')
|
||||||
|
expect(content.toString()).toBe('x')
|
||||||
|
})
|
||||||
|
|
||||||
|
it('preserves file content and custom metadata across rename', async () => {
|
||||||
|
await brain.vfs.writeFile('/notes/draft.md', 'hello world')
|
||||||
|
await brain.vfs.rename('/notes/draft.md', '/notes/final.md')
|
||||||
|
|
||||||
|
const content = await brain.vfs.readFile('/notes/final.md')
|
||||||
|
expect(content.toString()).toBe('hello world')
|
||||||
|
// The renamed entity's metadata carries the new basename
|
||||||
|
const stat = await brain.vfs.stat('/notes/final.md')
|
||||||
|
expect(stat.size).toBeGreaterThan(0)
|
||||||
|
expect(await brain.vfs.exists('/notes/draft.md')).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
it('moves a file across directories', async () => {
|
||||||
|
await brain.vfs.mkdir('/src')
|
||||||
|
await brain.vfs.mkdir('/dest')
|
||||||
|
await brain.vfs.writeFile('/src/file.txt', 'moving')
|
||||||
|
await brain.vfs.rename('/src/file.txt', '/dest/file.txt')
|
||||||
|
|
||||||
|
expect(await brain.vfs.exists('/dest/file.txt')).toBe(true)
|
||||||
|
expect(await brain.vfs.exists('/src/file.txt')).toBe(false)
|
||||||
|
const content = await brain.vfs.readFile('/dest/file.txt')
|
||||||
|
expect(content.toString()).toBe('moving')
|
||||||
|
})
|
||||||
|
|
||||||
|
it('throws EEXIST when the target already exists', async () => {
|
||||||
|
await brain.vfs.writeFile('/one.txt', '1')
|
||||||
|
await brain.vfs.writeFile('/two.txt', '2')
|
||||||
|
await expect(brain.vfs.rename('/one.txt', '/two.txt')).rejects.toThrow(/exists/i)
|
||||||
|
})
|
||||||
|
})
|
||||||
Loading…
Add table
Add a link
Reference in a new issue