Clarify render ownership and synchronize presentation teardown

CHANGES.md

@@ -1,5 +1,17 @@
 # Changes
 
+## Unreleased
+
+- Mark recursive fragment and diagnostic render trees acyclic; rely on Nim
+  inference for other render data. Document the downward ownership contract.
+- Add cross-thread ARC/ORC render-data coverage and fragment-cycle rejection
+  regressions.
+- Borrow the host window in the Siwin renderer and its OpenGL fallback state,
+  breaking the window/callback/renderer cycle. Callers keep the window alive
+  through rendering and renderer cleanup.
+- Add `finishPendingFrames` to wait for submitted Metal, Vulkan, or OpenGL work
+  before releasing a presentation target. Renderer types remain cycle-capable.
+
 ## 0.42.0
 
 - Add immutable shared glyph arrangements and glyph-range views for line render

README.md

@@ -17,6 +17,9 @@ Features:
 - Fast Gaussian 2-pass node operation for fast background blurs.
 - Clipping and layering support.
 
+See [render ownership](docs/render_ownership.md) for the acyclic render-data
+contract and ARC/ORC thread-transfer requirements.
+
 ## Quick Start
 
 This part works best with a recent Atlas (>= 0.9.6) version:

docs/render_ownership.md

@@ -0,0 +1,85 @@
+# Render-data ownership
+
+FigDraw render data has an acyclic ownership contract under ARC and ORC.
+Only the recursive `RenderFragment` and diagnostic `RenderTree` types need
+explicit `{.acyclic.}` annotations. Nim already infers acyclicity for `Fig`,
+`RenderList`, `Renders`, `RenderFragments`, and the glyph/text payload types.
+The annotation records an ownership contract; it does not break cycles or make
+mutable objects safe to share concurrently.
+
+| Type | Owning references and invariant |
+| --- | --- |
+| `Fig`, `RenderList`, `Renders` | Own drawing values and text payloads. `Fig.parent` is an integer index; nodes never retain their owner. |
+| `RenderFragment` | Owns descendant fragments only. It has no parent or tree reference. |
+| `RenderFragments` | Owns base lists and downward fragment edges. It never stores handles or cursors. |
+| `RenderFragmentHandle`, `RenderCursor` | Retain the tree and fragment for safe invalidation checks. Nothing in that tree retains them. |
+| `Utf8Runes`, `GlyphArrangement` | UTF-8 storage contains only bytes/checkpoints. Glyph views retain an immutable complete arrangement; that arrangement cannot itself be a view. |
+| `GlyphPosition` | Contains only glyph geometry values. |
+| `RenderTree` | Diagnostic output with downward child references. Callers must not insert the tree itself or an ancestor into its public `children` sequence. |
+
+`moveFragment` rejects both self-attachment and attachment beneath a descendant
+before mutating the graph. Its attachment records contain a parent reference only
+temporarily while moving an edge; fragments do not retain those records.
+
+Transfer exclusive ownership through an isolated channel payload and release
+producer-side aliases before handoff. Acyclic annotations do not make reference
+counts atomic. `FontRef` and `ImageRef` remain thread-affine managed leases;
+transfer their IDs and establish ownership on the receiving thread instead.
+
+## Renderer ownership is separate
+
+This contract does not cover `FigRenderer`, backend contexts, or native windows.
+An owning window reference formed a real cycle when a callback stored on that
+window captured the renderer:
+
+```text
+renderer -> backendState.window -> onRender closure -> renderer
+```
+
+The Siwin backend and its OpenGL fallback state now borrow their window using
+`{.cursor.}`, removing the renderer-to-window owning edge. The caller must keep
+the window alive through rendering and renderer cleanup. This is a borrow, not
+a weak reference that automatically becomes nil when the window is destroyed.
+Other window shims and custom backend states retain their existing contracts.
+
+`finishPendingFrames` waits for submitted GPU work on the renderer's owning
+thread: Metal waits for its last submitted command buffer, Vulkan waits for the
+device, and OpenGL finishes the current context. Call it after the final frame
+and before releasing the renderer or its native presentation target. Custom
+asynchronous backend contexts must override the operation.
+
+The renderer's `contextActivation` procedure uses `nimcall` and has no captured
+closure environment. It does not introduce that cycle.
+
+Siwin's `useDedicatedRenderThread` already clears the window reference before a
+handoff. However, clearing that reference and moving the object does not itself
+remove an earlier registration in the creating thread's ORC cycle-candidate
+buffer. After releasing producer-side aliases and assembling an exclusively
+owned, isolated renderer payload, the sender must retire those registrations
+with `GC_runOrc()` before publishing the payload to another thread. Sigils
+`RChan` can do this with `-d:nimSafeOrcSend`; Merenda instead performs the
+collection only for renderer commands, avoiding collections for each acyclic
+frame snapshot. Render-data annotations alone do not fix renderer handoff.
+
+## Verification
+
+The ownership regression exercises sixteen exclusive transfers of previously
+aliased fragment trees, render lists, diagnostic trees, and shared glyph views.
+The fragment suite verifies that cycle-forming moves fail without changing
+handles or topology. The Siwin callback regression verifies immediate context
+destruction when the owning window leaves scope.
+
+```sh
+atlas-run tests renderownership renderfragments siwin_dedicated_render -- --mm:orc
+atlas-run tests renderownership renderfragments siwin_dedicated_render -- --mm:arc
+```
+
+For sanitizer coverage, use Clang with malloc-backed Nim allocation and pass the
+sanitizers to both compilation and linking:
+
+```sh
+UBSAN_OPTIONS=halt_on_error=1 atlas-run tests renderownership -- \
+  --mm:orc --cc:clang -d:useMalloc -d:noSignalHandler --debugger:native \
+  --passC:'-fsanitize=address,undefined -fno-omit-frame-pointer' \
+  --passL:'-fsanitize=address,undefined'
+```

figdraw.nimble

@@ -1,4 +1,4 @@
-version = "0.42.0"
+version = "0.43.0"
 author = "Jaremy Creechley"
 description = "UI Engine for Nim"
 license = "MIT"

src/figdraw/common/fonttypes.nim

@@ -121,6 +121,7 @@ type
 
 type
   GlyphArrangement* = object
+    ## Views retain immutable complete layouts; owners never retain their views.
     shared*: ConstPtr[GlyphArrangement] ## Immutable owner for a glyph-range view.
     viewGlyphStart*: uint32 ## Inclusive glyph offset in `shared`.
     viewGlyphEnd*: uint32 ## Exclusive glyph offset in `shared`.

src/figdraw/common/transfer.nim

@@ -1,7 +1,9 @@
 import std/[math, sequtils]
 import ../fignodes
 
-type RenderTree* = ref object
+type RenderTree* {.acyclic.} = ref object
+  ## Diagnostic tree with downward-only child ownership. Callers must not insert
+  ## the tree itself or an ancestor into `children`.
   id*: int
   children*: seq[RenderTree]
 

src/figdraw/figbackend.nim

@@ -633,6 +633,12 @@ method beginFrame*(
 method endFrame*(impl: BackendContext) {.base.} =
   raise newException(ValueError, "Backend endFrame unavailable")
 
+method finishPendingFrames*(impl: BackendContext) {.base.} =
+  ## Waits for submitted GPU work before a presentation target is released.
+  ## Call on the owning render thread, after ending the last frame. Backends
+  ## that submit asynchronous work must override this operation.
+  discard
+
 method translate*(impl: BackendContext, v: Vec2) {.base.} =
   raise newException(ValueError, "Backend translate unavailable")
 

src/figdraw/fignodes.nim

@@ -43,11 +43,13 @@ type
   RenderShadows* = array[ShadowCount, RenderShadow]
 
   Renders* = ref object
+    ## Owns value-only render lists. Nodes must never retain their render owner.
     layers*: OrderedTable[ZLevel, RenderList]
 
   FigIdx* = distinct int16
 
   Fig* = object
+    ## Render payloads are acyclic. Parent links are indexes, not owning refs.
     zlevel*: ZLevel
     parent*: FigIdx = (-1).FigIdx
     flags*: set[FigFlags]

src/figdraw/figrender.nim

@@ -75,6 +75,12 @@ proc activateContext*[BackendState](renderer: FigRenderer[BackendState]) =
   if not renderer.contextActivation.isNil and renderer.backendKind() == rbOpenGL:
     renderer.contextActivation(renderer)
 
+proc finishPendingFrames*[BackendState](renderer: FigRenderer[BackendState]) =
+  ## Waits for submitted frames on the renderer's owning thread. The host window
+  ## must remain alive until this returns and renderer cleanup has finished.
+  renderer.activateContext()
+  renderer.ctx.finishPendingFrames()
+
 proc atlasUsage*[BackendState](renderer: FigRenderer[BackendState]): AtlasUsage =
   ## Returns current backend atlas usage.
   ##

src/figdraw/metal/metal_context.nim

@@ -1901,6 +1901,12 @@ method endFrame*(ctx: MetalContext) =
   ctx.commandBuffer.clear()
   ctx.frameAutoreleasePool.stop()
 
+method finishPendingFrames*(ctx: MetalContext) =
+  # All submissions use one command queue, so its last buffer covers earlier
+  # frames too. Waiting must precede destruction of the presentation layer.
+  if not ctx.lastCommitted.isNil:
+    waitUntilCompleted(ctx.lastCommitted.borrow)
+
 method translate*(ctx: MetalContext, v: Vec2) =
   ctx.mat = ctx.mat * translate(vec3(v))
 

src/figdraw/opengl/glcontext.nim

@@ -2055,6 +2055,9 @@ proc beginFrameDefaultProj(ctx: OpenGlContext, frameSize: Vec2) =
     ctx, frameSize, ortho[float32](0.0, frameSize.x, frameSize.y, 0, -1000.0, 1000.0)
   )
 
+method finishPendingFrames*(ctx: OpenGlContext) =
+  glFinish()
+
 method endFrame*(ctx: OpenGlContext) =
   ## Ends a frame.
   assert ctx.frameBegun == true, "ctx.beginFrame was not called first."

src/figdraw/renderfragments.nim

@@ -107,7 +107,9 @@ type
     parent: FigIdx
     position: int
 
-  RenderFragment = ref object
+  RenderFragment {.acyclic.} = ref object
+    # Owns descendant fragments only. Parent attachments are temporary values;
+    # moveFragment rejects attachment beneath this fragment or a descendant.
     id: uint64
     list: RenderList
     entries: RenderEntries
@@ -121,6 +123,8 @@ type
     ##
     ## Fragment edges are stored separately from physical `RenderList` nodes, so
     ## replacement does not change indexes in the surrounding lists.
+    ## Owning edges point downwards. Handles/cursors retain this tree, but the
+    ## tree never retains handles/cursors or parent links.
     base: Renders
     layerEntries: OrderedTable[ZLevel, RenderEntries]
     layerGenerations: Table[ZLevel, uint64]
@@ -1284,7 +1288,7 @@ proc appendMaterializedEntry(
   owner: RenderFragment,
   entry: RenderChild,
   parent: FigIdx,
-)
+) {.gcsafe.}
 
 proc appendMaterializedNode(
     fragments: RenderFragments,

src/figdraw/vulkan/vulkan_context.nim

@@ -3757,6 +3757,10 @@ method beginFrame*(
     clearMainColor = clearMainColor,
   )
 
+method finishPendingFrames*(ctx: VulkanContext) =
+  if ctx.device != vkNullDevice:
+    checkVkResult ctx.vk.vkDeviceWaitIdle(ctx.device)
+
 method endFrame*(ctx: VulkanContext) =
   assert ctx.frameBegun == true, "ctx.beginFrame was not called first."
   assert ctx.maskDepth == 0, "Not all masks have been popped."

src/figdraw/windowing/siwinshim.nim

@@ -463,7 +463,7 @@ when UseVulkanBackend and UseOpenGlFallback and (defined(linux) or defined(bsd))
 
     SiwinOpenGlFallbackState = ref SiwinOpenGlFallbackStateObj
     SiwinOpenGlFallbackStateObj = object
-      window: Window
+      window {.cursor.}: Window
       initialized: bool
       case kind: SiwinOpenGlFallbackKind
       of sogfX11:
@@ -490,15 +490,13 @@ when UseVulkanBackend and UseOpenGlFallback and (defined(linux) or defined(bsd))
           state.waylandContext.destroy()
     except Exception:
       discard
-    try:
-      `=destroy`(state.window)
-    except Exception:
-      discard
 
 type
   SiwinRenderBackend* = object
     ## Opaque per-window backend state used by siwin + FigDraw integration.
-    window*: Window
+    window* {.cursor.}: Window
+      ## Borrowed from the caller, which must keep the window alive while using
+      ## the renderer. Window callbacks may therefore own the renderer safely.
     dedicatedRender*: bool
     presentationReady: bool
     resizeClearColor: Color
@@ -787,6 +785,8 @@ proc activateRendererContext(renderer: FigRenderer[SiwinRenderBackend]) =
 
 proc setupBackend*(renderer: FigRenderer, window: Window) =
   ## One-time backend hookup between a siwin window and FigDraw renderer.
+  ## The caller owns `window` and keeps it alive until rendering and renderer
+  ## cleanup finish. Dedicated rendering clears this borrow before transfer.
   renderer.backendState.window = window
   renderer.contextActivation = activateRendererContext
   renderer.backendState.dedicatedRender = false

tests/trenderfragments.nim

@@ -362,6 +362,26 @@ suite "RenderFragments APIs":
     check reorderedRoot == first
     check fragments.rootCursors(0.ZLevel).mapIt(fragments[it].nodeId()) == @[10, 20]
 
+  test "self and descendant attachment cannot create an ownership cycle":
+    let fragments = newRenderFragments()
+    let root = fragments.addRoot(0.ZLevel, testFig(10))
+    var parentContents = RenderList()
+    discard parentContents.addRoot(testFig(20))
+    let parent = fragments.attachChildFragment(0.ZLevel, root, 0, move parentContents)
+    let parentRoot = fragments.fragmentRoots(parent)[0]
+    var childContents = RenderList()
+    discard childContents.addRoot(testFig(30))
+    let child = fragments.attachChildFragment(parentRoot, 0, move childContents)
+    let childRoot = fragments.fragmentRoots(child)[0]
+    expect RenderFragmentError:
+      discard fragments.moveFragment(parent, parentRoot, 0)
+    expect RenderFragmentError:
+      discard fragments.moveFragment(parent, childRoot, 0)
+    check fragments.isValid(parent)
+    check fragments.isValid(child)
+    check fragments.childIds(parentRoot) == @[30]
+    check fragments.materialize()[0.ZLevel].nodes.len == 3
+
   test "moves an empty fragment slot before restoring its contents":
     let fragments = newRenderFragments()
     let root = fragments.addRoot(0.ZLevel, testFig(10))

tests/trenderownership.nim

@@ -0,0 +1,71 @@
+import std/[isolation, unittest]
+import threading/smartptrs
+import sigils/rchannels
+
+import figdraw/common/[fonttypes, transfer]
+import figdraw/fignodes
+import figdraw/renderfragments
+
+type RenderTransfer = object
+  fragments: RenderFragments
+  renders: Renders
+  tree: RenderTree
+
+proc retainAlias[T](value: T): T {.noinline.} =
+  value
+
+proc makeTransfer(): RenderTransfer =
+  let source = initArrangementRunes("a🙂b")
+  let owner = shareGlyphArrangement(
+    GlyphArrangement(
+      sourceRunes: source,
+      runes: source,
+      arrangedGlyphs: @[ArrangedGlyph(), ArrangedGlyph(), ArrangedGlyph()],
+    )
+  )
+  result.fragments = newRenderFragments()
+  let root = result.fragments.addRoot(0.ZLevel, Fig(kind: nkRectangle))
+  var contents = RenderList()
+  discard contents.addRoot(Fig(kind: nkRectangle))
+  let fragment = result.fragments.attachChildFragment(0.ZLevel, root, 0, move contents)
+  let cursor = result.fragments.fragmentRoots(fragment)[0]
+  var nested = RenderList()
+  discard
+    nested.addRoot(Fig(kind: nkText, textLayout: owner.glyphArrangementView(1 .. 2)))
+  discard result.fragments.attachChildFragment(cursor, 0, move nested)
+  result.renders = result.fragments.materialize()
+  result.tree = result.renders[0.ZLevel].toTree()
+
+  # Releasing aliases before handoff must not leave render data registered in
+  # the producer's ORC candidate buffer. All handles and cursors also expire
+  # before this procedure returns its exclusively owned payload.
+  var fragmentsAlias = retainAlias(result.fragments)
+  var rendersAlias = retainAlias(result.renders)
+  var treeAlias = retainAlias(result.tree)
+  doAssert not fragmentsAlias.isNil
+  doAssert not rendersAlias.isNil
+  doAssert not treeAlias.isNil
+  fragmentsAlias = nil
+  rendersAlias = nil
+  treeAlias = nil
+
+proc releaseOnWorker(channel: RChan[RenderTransfer]) {.thread.} =
+  let payload = channel.recv()
+  doAssert payload.fragments.materialize()[0.ZLevel].nodes.len == 3
+  doAssert payload.renders[0.ZLevel].nodes.len == 3
+  let layout = payload.renders[0.ZLevel].nodes[2].textLayout
+  doAssert layout.isGlyphView()
+  doAssert layout.glyphCount() == 2
+  doAssert layout.shared[].sourceRunes.bytes == "a🙂b"
+  doAssert payload.tree.children.len == 1
+  doAssert payload.tree.children[0].children[0].children.len == 1
+
+suite "FigDraw render ownership":
+  test "aliased fragments, glyph views and render trees can move between threads":
+    for iteration in 0 ..< 16:
+      let channel = newRChan[RenderTransfer](1)
+      # Preload the channel so the finite worker never waits for readiness.
+      channel.send(unsafeIsolate(makeTransfer()))
+      var worker: Thread[RChan[RenderTransfer]]
+      createThread(worker, releaseOnWorker, channel)
+      joinThread(worker)