Mark nodes removed; only remove orphaned nodes (#195)

* Mark nodes removed instead of deleting them

Problem:

- When POET loads a page, references to resources on the page are stored in
the  page node (see _resourceLinks)

- When a resource was removed from the bundle, perhaps via a filesystem
event, the references in the page remained, even though the resources no
longer existed in the bundle.

- When a resource with the same absPath was created, it was added to the
bundle again. This new resource instance, however, was distinct from the
one that existed within the page even though they pointed to the same content
on the filesystem.

- Until the page was updated, which causes the links to be reconstructed,
resource links stored in the page did not correlate with the ones stored in
the bundle. Consequently, the `exists` property of the resource node was not
being updated

Solution:

- Keep all nodes in the bundle

- When a file referenced by a node is deleted from the filesystem, update the
node's contents to `undefined` so that the node's `exists` property will be
set to false

Possible problems & justifications:

- This technically introduces a memory leak where nodes continue to accumulate
in the bundle forever; however, there are many ways this memory leak could be
addressed if it becomes a real problem.

- Out of all the approaches I explored, this one made the best use of Quarx,
required the least number of changes, and exhibited the best performance.

* Remove deleted nodes that are orphaned

This fixes the aforementioned memory leak for everything except bundles

When a file is deleted and its associated node is orphaned, as defined in model manager, then that node will be removed
If the node is not orphaned, it will stay in memory even though the file no longer exists

* Fix some comments; add some polish

* Send diagnostics before removing nodes

* Use readAndUpdate instead of readAndLoad

A file creation event could map to an existing node that is loaded

* Nodes only need to be loaded to be orphaned

Fixes a problem with cascading deletions

server/src/model-manager.spec.ts

@@ -317,18 +317,46 @@ describe('processFilesystemChange()', () => {
     // Load the Bundle, Book, and Page
     loadSuccess(first(loadSuccess(first(loadSuccess(manager.bundle).books)).pages))
 
+    const sizeBefore = manager.bundle.allNodes.size
+    expect(manager.bundle.allPages.all.size).toBe(1)
+    expect(manager.bundle.allResources.all.size).toBe(0)
     // Delete non-existent file
     expect((await fireChange(FileChangeType.Deleted, 'media/newpic.png')).toArray()).toEqual([])
     // Delete a file
     const deletedModules = await fireChange(FileChangeType.Deleted, 'modules/m1234/index.cnxml')
     expect(deletedModules.size).toBe(1)
     expect(first(deletedModules)).toBeInstanceOf(PageNode)
+    // Page is still referenced by book: should still be in bundle
+    expect(manager.bundle.allPages.all.size).toBe(1)
     // Delete a directory
     expect((await fireChange(FileChangeType.Deleted, 'collections')).size).toBe(1)
     // expect(sendDiagnosticsStub.callCount).toBe(0)
+    // Book deleted -> page orphaned -> page deleted
+    expect(manager.bundle.allPages.all.size).toBe(0)
 
     // Delete everything (including the bundle)
-    expect((await fireChange(FileChangeType.Deleted, '')).size).toBe(1)
+    // All nodes should still be in bundle, but they should no longer exist
+    expect((await fireChange(FileChangeType.Deleted, '')).size).toBe(sizeBefore - 1)
+    expect(manager.bundle.exists).toBe(false)
+    expect(manager.bundle.allNodes.every((n) => !n.exists))
+  })
+  it('cascades deletes correctly', async () => {
+    // Load the Bundle, Book, and Page
+    loadSuccess(first(loadSuccess(first(loadSuccess(manager.bundle).books)).pages))
+
+    const sizeBefore = manager.bundle.allNodes.size
+    expect(manager.bundle.allBooks.all.size).toBe(1)
+    expect(manager.bundle.allPages.all.size).toBe(1)
+    expect(manager.bundle.allResources.all.size).toBe(0)
+    expect(manager.bundle.allNodes.size).toBe(3) // bundle + book + page
+
+    // Delete everything (including the bundle)
+    expect((await fireChange(FileChangeType.Deleted, '')).size).toBe(sizeBefore)
+    expect(manager.bundle.allBooks.all.size).toBe(0)
+    expect(manager.bundle.allPages.all.size).toBe(0)
+    // Only the bundle should remain (bundles are immortal at the moment)
+    expect(manager.bundle.allNodes.size).toBe(1)
+    expect(first(manager.bundle.allNodes)).toBe(manager.bundle)
     expect(manager.bundle.exists).toBe(false)
   })
   it('deletes Image/Page/Book', async () => {

server/src/model-manager.ts

@@ -175,21 +175,36 @@ export class ModelManager {
     return this.bundle.allPages.all
   }
 
+  public get orphanedBooks() {
+    const loadedBooks = this.bundle.allBooks.all.filter((n) => n.isLoaded)
+    const referencedBooks = this.bundle.books
+    return !this.bundle.exists ? loadedBooks : loadedBooks.subtract(referencedBooks)
+  }
+
   public get orphanedPages() {
     const books = this.bundle.books.filter(loadedAndExists)
-    return this.bundle.allPages.all.filter(loadedAndExists).subtract(books.flatMap(b => b.pages))
+    return this.bundle.allPages.all.filter((n) => n.isLoaded).subtract(books.flatMap(b => b.pages))
   }
 
   public get orphanedResources() {
     const books = this.bundle.books.filter(loadedAndExists)
-    const pages = books.flatMap(b => b.pages)
-    return this.bundle.allResources.all.filter(loadedAndExists).subtract(pages.flatMap(p => p.resources))
+    const pages = books.flatMap(b => b.pages).filter(loadedAndExists)
+    return this.bundle.allResources.all.filter((n) => n.isLoaded).subtract(pages.flatMap(p => p.resources))
   }
 
   public get orphanedH5P() {
     const books = this.bundle.books.filter(loadedAndExists)
-    const pages = books.flatMap(b => b.pages)
-    return this.bundle.allH5P.all.filter(loadedAndExists).subtract(pages.flatMap(p => p.h5p))
+    const pages = books.flatMap(b => b.pages).filter(loadedAndExists)
+    return this.bundle.allH5P.all.filter((n) => n.isLoaded).subtract(pages.flatMap(p => p.h5p))
+  }
+
+  public get orphanedNodes() {
+    return I.Set<Fileish>().withMutations((s) => {
+      s.union(this.orphanedBooks)
+      s.union(this.orphanedPages)
+      s.union(this.orphanedH5P)
+      s.union(this.orphanedResources)
+    })
   }
 
   public async loadEnoughForToc() {
@@ -246,7 +261,7 @@ export class ModelManager {
       const node = findOrCreateNode(bundle, uri)
       if (node !== undefined) {
         ModelManager.debug('[FILESYSTEM_EVENT] Adding item')
-        await this.readAndLoad(node)
+        await this.readAndUpdate(node)
         this.sendAllDiagnostics()
         return I.Set([node])
       } else {
@@ -270,28 +285,42 @@ export class ModelManager {
       ModelManager.debug('[FILESYSTEM_EVENT] Removing everything with this URI (including subdirectories if they exist)', uri)
 
       const removedNodes = I.Set<Fileish>().withMutations(s => {
-        // Unload if the user deleted the bundle directory
-        if (bundle.absPath.startsWith(uri)) s.add(bundle)
-        // Remove if it was a file
-        const removedNode = bundle.allBooks.remove(uri) ??
-                  bundle.allPages.remove(uri) ??
-                  bundle.allResources.remove(uri) ??
-                  bundle.allH5P.remove(uri)
-        if (removedNode !== undefined) s.add(removedNode)
-        // Remove if it was a directory
+        const markRemoved = <T extends Fileish>(n: T) => {
+          ModelManager.debug(`[MODEL_MANAGER] Marking as removed: ${n.absPath}`)
+          this.errorHashesByPath.delete(n.absPath)
+          n.load(undefined)
+          s.add(n)
+        }
         const filePathDir = `${uri}${PATH_SEP}`
-        s.union(bundle.allBooks.removeByKeyPrefix(filePathDir))
-        s.union(bundle.allPages.removeByKeyPrefix(filePathDir))
-        s.union(bundle.allResources.removeByKeyPrefix(filePathDir))
-        s.union(bundle.allH5P.removeByKeyPrefix(filePathDir))
-      })
-      // Unload all removed nodes so users do not think the files still exist
-      removedNodes.forEach(n => {
-        this.errorHashesByPath.delete(n.absPath)
-        n.load(undefined)
-        this.sendFileDiagnostics(n)
+        // NOTE: order is important. Ideally try to delete things that could
+        // hold references first (books/pages)
+        const allFactories = [
+          bundle.allBooks, bundle.allPages, bundle.allH5P, bundle.allResources
+        ]
+
+        // First mark all the matching nodes as mark not existing (i.e. load undefined)
+        if (bundle.absPath.startsWith(uri)) markRemoved(bundle)
+        allFactories.forEach((factory) => {
+          const maybeNode = factory.get(uri)
+          if (maybeNode !== undefined) markRemoved(maybeNode)
+          factory.findByKeyPrefix(filePathDir).forEach(markRemoved)
+        })
+
+        // Send diagnostics before removing nodes
+        this.sendAllDiagnostics()
+
+        // Then remove nodes if they are orphaned, loaded, and not existing
+        this.orphanedNodes
+          .filter(({ isLoaded, exists }) => isLoaded && !exists)
+          .forEach(({ absPath }) => {
+            ModelManager.debug(`[MODEL_MANAGER] Dropping: ${absPath}`)
+            // Expect at least one factory to have this node
+            expectValue(
+              allFactories.find((factory) => factory.remove(absPath) !== undefined),
+              `[MODEL_MANAGER] ERROR: Failed to drop: ${absPath} (possible memory leak)`
+            )
+          })
       })
-      this.sendAllDiagnostics()
       return removedNodes
     }
   }
@@ -448,14 +477,17 @@ export class ModelManager {
           .length > 0
       },
       getRange: this.rangeFinderFactory('src="', '"'),
-      getCompletionItems: (page, range) => this.orphanedResources.toArray().map(i => {
-        const insertText = path.relative(path.dirname(page.absPath), i.absPath)
-        const item = CompletionItem.create(insertText)
-        item.textEdit = TextEdit.replace(range, insertText)
-        item.kind = CompletionItemKind.File
-        item.detail = 'Orphaned Resource'
-        return item
-      })
+      getCompletionItems: (page, range) => this.orphanedResources
+        .filter((r) => r.exists)
+        .toArray()
+        .map(i => {
+          const insertText = path.relative(path.dirname(page.absPath), i.absPath)
+          const item = CompletionItem.create(insertText)
+          item.textEdit = TextEdit.replace(range, insertText)
+          item.kind = CompletionItemKind.File
+          item.detail = 'Orphaned Resource'
+          return item
+        })
     }
     return this.autocomplete(page, cursor, resourceAutocompleter)
   }
@@ -470,7 +502,9 @@ export class ModelManager {
       },
       getRange: this.rangeFinderFactory('url="', '"'),
       getCompletionItems: (_page, range) => {
-        return this.orphanedH5P.toArray().filter((h) => h.exists)
+        return this.orphanedH5P
+          .filter((h) => h.exists)
+          .toArray()
           .map((h) => path.dirname(h.absPath))
           .map((p) => path.basename(p))
           .map((name) => {

server/src/model/bundle.ts

@@ -14,7 +14,7 @@ export class Bundle extends Fileish implements Bundleish {
   public readonly allH5P: Factory<H5PExercise> = new Factory<H5PExercise>((absPath: string) => new H5PExercise(this, this.pathHelper, absPath), (x) => this.pathHelper.canonicalize(x))
   public readonly allBooks = new Factory((absPath: string) => new BookNode(this, this.pathHelper, absPath), (x) => this.pathHelper.canonicalize(x))
   private readonly _books = Quarx.observable.box<Opt<I.Set<WithRange<BookNode>>>>(undefined)
-  private readonly _duplicateResourcePaths = Quarx.observable.box<I.Set<string>>(I.Set<string>())
+  private readonly _duplicateFilePaths = Quarx.observable.box<I.Set<string>>(I.Set<string>())
   private readonly _duplicateUUIDs = Quarx.observable.box<I.Set<string>>(I.Set<string>())
   // TODO: parse these from META-INF/books.xml
   public readonly paths = {
@@ -29,20 +29,21 @@ export class Bundle extends Fileish implements Bundleish {
     super(undefined, pathHelper, pathHelper.join(workspaceRootUri, 'META-INF/books.xml'))
     super.setBundle(this)
     Quarx.autorun(() => {
-      this._duplicateResourcePaths.set(
+      this._duplicateFilePaths.set(
         I.Set(
-          findDuplicates(I.List(this.allResources.all)
-            .map(n => n.absPath.toLowerCase())
+          findDuplicates(
+            I.List(this.allNodes)
+              .filter(n => n.exists)
+              .map(n => n.absPath.toLowerCase())
           )
         )
       )
     })
     Quarx.autorun(() => {
       this._duplicateUUIDs.set(
         I.Set(
-          findDuplicates(I.List(this.allPages.all)
-            .filter(n => n.exists)
-            .map(n => n.uuid())
+          findDuplicates(
+            I.List(this.allPages.all).filter(n => n.exists).map(n => n.uuid())
           )
         )
       )
@@ -63,15 +64,20 @@ export class Bundle extends Fileish implements Bundleish {
   }
 
   public get allNodes() {
-    return I.Set([this]).union(this.allBooks.all).union(this.allPages.all).union(this.allResources.all).union(this.allH5P.all)
+    // TODO: Will all nodes continue to be fileish in future?
+    return I.Set([this])
+      .union(this.allBooks.all)
+      .union(this.allPages.all)
+      .union(this.allH5P.all)
+      .union(this.allResources.all)
   }
 
   public get books() {
     return this.__books().map(b => b.v)
   }
 
-  public isDuplicateResourcePath(path: string): boolean {
-    return this._duplicateResourcePaths.get().has(path.toLowerCase())
+  public isDuplicateFilePath(path: string): boolean {
+    return this._duplicateFilePaths.get().has(path.toLowerCase())
   }
 
   private __books() {

server/src/model/factory.spec.ts

@@ -14,16 +14,16 @@ describe('Factory', () => {
     expect(f.getOrAdd('key2').thing).toEqual(1)
     expect(f.get('key2')).not.toBeUndefined()
   })
-  it('removesByKeyPrefix works', () => {
+  it('findByKeyPrefix works', () => {
     const f = new Factory((x) => ({ foo: x, bar: 'dummy-object' }), (x) => x)
     f.getOrAdd('keyPrefix1')
     f.getOrAdd('keyPrefix2')
     f.getOrAdd('anotherprefix')
     f.getOrAdd('not_a_keyPrefix')
 
     expect(f.size).toEqual(4)
-    const removed = f.removeByKeyPrefix('keyPrefix')
-    expect(removed.size).toEqual(2)
-    expect(f.size).toEqual(2)
+    const found = f.findByKeyPrefix('keyPrefix')
+    expect(found.size).toEqual(2)
+    expect(f.size).toEqual(4)
   })
 })

server/src/model/factory.ts

@@ -30,12 +30,11 @@ export class Factory<T> {
     return item
   }
 
-  public removeByKeyPrefix(pathPrefix: string) {
+  public findByKeyPrefix(pathPrefix: string) {
     pathPrefix = this.canonicalizer(pathPrefix)
     const m = this._map.get()
-    const removedItems = m.filter((_, key) => key.startsWith(pathPrefix))
-    this._map.set(m.filter((_, key) => !key.startsWith(pathPrefix)))
-    return I.Set(removedItems.values())
+    const matchingItems = m.filter((_, key) => key.startsWith(pathPrefix))
+    return I.Set(matchingItems.values())
   }
 
   public get size() { return this._map.get().size }

server/src/model/resource.ts

@@ -8,7 +8,7 @@ export class ResourceNode extends Fileish {
     return [{
       message: ResourceValidationKind.DUPLICATE_RESOURCES,
       nodesToLoad: I.Set<Fileish>(),
-      fn: () => this.bundle.isDuplicateResourcePath(this.absPath)
+      fn: () => this.bundle.isDuplicateFilePath(this.absPath)
         ? I.Set([NOWHERE])
         : I.Set<Range>()
     }]

server/src/model/utils.ts

@@ -81,7 +81,7 @@ export interface Bundleish {
   allH5P: Factory<H5PExercise>
   workspaceRootUri: string
   isDuplicateUuid: (uuid: string) => boolean
-  isDuplicateResourcePath: (path: string) => boolean
+  isDuplicateFilePath: (path: string) => boolean
   paths: Paths
 }
 

server/src/server-handler.ts

@@ -50,7 +50,7 @@ export async function autocompleteHandler(documentPosition: CompletionParams, ma
   const cursor = documentPosition.position
   const page = manager.bundle.allPages.get(documentPosition.textDocument.uri)
 
-  if (page !== undefined) {
+  if (page !== undefined && page.exists) {
     return [
       ...manager.autocompleteResources(page, cursor),
       ...manager.autocompleteUrls(page, cursor)

server/src/server.ts

@@ -166,7 +166,7 @@ connection.onDocumentLinks(async ({ textDocument }) => {
   const { uri } = textDocument
   const manager = getBundleForUri(uri)
   const page = manager.bundle.allPages.get(uri)
-  if (page !== undefined) {
+  if (page !== undefined && page.exists) {
     return await manager.getDocumentLinks(page)
   } else {
     return []