OverviewFilesCommitsIssuesDocsReleasesPeers

hyper-note commits / d4466b5

note-manager: make renameNotebook crash-safe (copy → verify → commit)
renameNotebook had deleteNotebook(oldSlug) INSIDE the try whose catch deletes the NEW drive. So a failure during the old-delete would roll back by destroying the freshly-copied new drive while the old drive was already half-deleted — losing the notebook entirely. Also newMeta was declared with const inside the try but referenced after it (latent scope bug on the success path). Restructure into two phases: - Phase 1 (copy): metadata + notes into the new drive, then verify metadata landed. Failure here rolls back by deleting the new drive — safe, the old notebook is untouched. - Phase 2 (commit): delete the old notebook only after the verified copy. If this fails, do NOT delete the new drive (it is now canonical) — surface an error telling the operator both exist. newMeta hoisted above the try so it is in scope for the return. Test (real Corestore/DriveManager): happy path preserves note count + removes old; a forced Phase-2 delete failure leaves the OLD notebook + all notes intact (the data-loss case the fix prevents). 2/2 pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
d4466b58b0ee6e68ad7367262bbc8b47a4b71474
by plankatron <fleeky@gmail.com> · 2026-06-23T20:21:25.000Z

Changed files (2) +101 -13

lib/note-manager.js +26 -13
……@@ -521,32 +521,34 @@ export class NoteManager {
521521 // Create new drive
522522 const newDrive = await this.driveManager.createDrive(newSlug)
523523
524 // Declared here (not inside the try) so it stays in scope for the return.
525 const now = Date.now()
526 const newMeta = { ...oldMeta, slug: newSlug, updatedAt: now }
527 delete newMeta.isLocal
528 delete newMeta.driveKey
529
530 // Phase 1 — copy everything into the new drive. Any failure here is safe to
531 // roll back by deleting the new drive, because the OLD notebook has not been
532 // touched yet and remains fully intact.
524533 try {
525 // Copy metadata with new slug
526 const now = Date.now()
527 const newMeta = { ...oldMeta, slug: newSlug, updatedAt: now }
528 delete newMeta.isLocal
529 delete newMeta.driveKey
530534 await newDrive.put('/metadata.json', Buffer.from(JSON.stringify(newMeta, null, 2)))
531535
532 // Copy all notes
533536 const oldDrive = await this._getDrive(oldSlug)
534537 try {
535538 const noteFiles = await oldDrive.readdir('/notes')
536539 for await (const file of noteFiles) {
537540 const buf = await oldDrive.get(`/notes/${file}`)
538 if (buf) {
539 await newDrive.put(`/notes/${file}`, buf)
540 }
541 if (buf) await newDrive.put(`/notes/${file}`, buf)
541542 }
542543 } catch {
543 // no notes dir
544 // no notes dir — nothing to copy
544545 }
545546
546 // Delete old notebook
547 await this.deleteNotebook(oldSlug)
547 // Verify the copy landed before committing the destructive delete.
548 const verifyMeta = await newDrive.get('/metadata.json')
549 if (!verifyMeta) throw new Error('rename verify failed: new metadata missing')
548550 } catch (err) {
549 // Rollback: clean up the partially-created new drive
551 // Safe rollback — the old notebook was never modified.
550552 try {
551553 await this.driveManager.deleteDrive(newSlug)
552554 } catch (cleanupErr) {
……@@ -555,6 +557,17 @@ export class NoteManager {
555557 throw err
556558 }
557559
560 // Phase 2 — COMMIT. The new drive is a verified full copy, so now (and only
561 // now) remove the old notebook. If THIS fails we must NOT delete the new
562 // drive — it is the canonical copy. Leave both intact and surface the error,
563 // rather than rolling back into a state where neither is whole.
564 try {
565 await this.deleteNotebook(oldSlug)
566 } catch (err) {
567 console.error('[NoteManager] rename: "' + newSlug + '" is complete but removing old "' + oldSlug + '" failed; both notebooks now exist:', err.message)
568 throw new Error(`Notebook renamed to '${newSlug}' but removing old '${oldSlug}' failed: ${err.message}. Both notebooks now exist; remove '${oldSlug}' manually.`)
569 }
570
558571 // Return with transient fields
559572 newMeta.isLocal = true
560573 const info = await this.driveManager.getDriveInfo(newSlug)
test/unit/notebook-rename.test.js +75 -0
……@@ -0,0 +1,75 @@
1import test from 'brittle'
2import fs from 'bare-fs'
3import path from 'bare-path'
4import os from 'bare-os'
5import Corestore from 'corestore'
6import Hyperbee from 'hyperbee'
7import { DriveManager } from '../../lib/drive-manager.js'
8import { NoteManager } from '../../lib/note-manager.js'
9
10function tmpSetup () {
11 const dir = path.join(os.tmpdir(), 'hn-rename-' + Date.now() + '-' + Math.random().toString(36).slice(2))
12 fs.mkdirSync(dir, { recursive: true })
13 const store = new Corestore(dir)
14 const registry = new Hyperbee(store.get({ name: 'registry' }), { keyEncoding: 'utf-8', valueEncoding: 'json' })
15 const dm = new DriveManager(store, registry)
16 return { store, registry, dm, dir }
17}
18
19function rmrf (dir) {
20 try {
21 for (const f of fs.readdirSync(dir)) {
22 const fp = path.join(dir, f)
23 if (fs.statSync(fp).isDirectory()) rmrf(fp)
24 else fs.unlinkSync(fp)
25 }
26 fs.rmdirSync(dir)
27 } catch {}
28}
29
30test('renameNotebook — happy path moves metadata + all notes, removes old', async (t) => {
31 const { registry, dm, dir } = tmpSetup()
32 await registry.ready()
33 const nm = new NoteManager(dm)
34
35 await nm.createNotebook('work', { name: 'Work Notes' })
36 await nm.createNote('work', { title: 'One', content: '# One' })
37 await nm.createNote('work', { title: 'Two', content: '# Two' })
38 const before = (await nm.listNotes('work')).length
39 t.ok(before >= 2, 'at least the two created notes are present')
40
41 const renamed = await nm.renameNotebook('work', 'tasks')
42 t.is(renamed.slug, 'tasks', 'returns new slug (newMeta in scope)')
43
44 t.ok(await nm.getNotebook('tasks'), 'new notebook exists')
45 t.absent(await nm.getNotebook('work'), 'old notebook removed')
46 const notes = await nm.listNotes('tasks')
47 t.is(notes.length, before, 'all notes copied to new notebook (count preserved)')
48
49 clearInterval(nm._keyCacheCleanupInterval)
50 rmrf(dir)
51})
52
53test('renameNotebook — Phase-2 delete failure leaves the OLD notebook intact (no data loss)', async (t) => {
54 const { registry, dm, dir } = tmpSetup()
55 await registry.ready()
56 const nm = new NoteManager(dm)
57
58 await nm.createNotebook('work', { name: 'Work Notes' })
59 await nm.createNote('work', { title: 'One', content: '# One' })
60 await nm.createNote('work', { title: 'Two', content: '# Two' })
61 const before = (await nm.listNotes('work')).length
62
63 // Force the commit-phase delete to fail AFTER the copy succeeded.
64 nm.deleteNotebook = async () => { throw new Error('simulated delete failure') }
65
66 await t.exception(() => nm.renameNotebook('work', 'tasks'), /removing old/, 'rename surfaces the failure')
67
68 // The critical property: the original notebook + its notes are NOT corrupted.
69 t.ok(await nm.getNotebook('work'), 'old notebook still exists after failed commit')
70 const oldNotes = await nm.listNotes('work')
71 t.is(oldNotes.length, before, 'old notes intact — no data loss')
72
73 clearInterval(nm._keyCacheCleanupInterval)
74 rmrf(dir)
75})