Comfy-Org/comfy-aimdo

Fork 0

mirror of https://github.com/Comfy-Org/comfy-aimdo.git synced 2026-11-04 23:37:21 +00:00

[GH-ISSUE #129] malloc graph: a rogue candidate freed before the root pop permanently poisons the graph → RuntimeError("aimdo memory compile error") #49

New issue

Open

opened 2026-10-26 22:24:04 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:24:04 +00:00

Owner

Originally created by @lonelystarCX on GitHub (Oct 22, 2026).
Original GitHub issue: https://github.com/Comfy-Org/comfy-aimdo/issues/129

Summary

On a long single-graph lifetime (a 7-forward sequence inside one sampler node) the aimdo
memory compiler stops accepting work mid-sampling:

RuntimeError: aimdo memory compile error
  comfy/model_prefetch.py:39  in _PauseMallocGraph.__enter__
    graph.pause(sync=self.sync)
  comfy_aimdo/malloc_graph.py:35  in MallocGraph.pause
    self._call(control.lib.malloc_graph_pause, True, sync)
  comfy_aimdo/malloc_graph.py:16  in MallocGraph._call
    raise RuntimeError("aimdo memory compile error")

The failing call is malloc_graph_pause(), but the graph was already dead before it:
g->failed had been set because one rogue candidate was freed before finalize_rogues()
could adopt it. From that point on alloc/free/pause/push/pop/set_stream all return
false, i.e. the whole recording is unusable, and the only recovery is destroying the graph.

Everything below is measured on 7 reruns of the same API prompt (5 baseline, 2 controls,
all deterministic); the mechanism is read from master source and cross-checked against the
shipped 0.5.6 binary.

Environment


ComfyUI	v0.42.3, rev 6185 `edb3db00` (2026-10-20), Linux
comfy-aimdo	0.5.6 (commit `g08b37eec3`)
comfy-kitchen	0.2.39+spark.h3.sm89.cu130.1 — see artifact note
torch / CUDA	2.13.0+cu130, driver 610.57.04
Python	3.12.11
GPU	RTX 4090 (48 GB modified card), 49140 MiB, cc 8.9
Allocator	`cudaMallocAsync`, dynamic VRAM enabled, `--async-offload 2`
Launch	`python3 main.py --listen 127.0.0.1 --port 8188` (no compiler/VRAM flags)

Artifact note (please read before dismissing as third-party): comfy-kitchen is not the
stock wheel — the Spark-H3 v0.1.5 installer replaces it with
0.2.39+spark.h3.sm89.cu130.1. Graph is MiniMax-H3 + ComfyUI-Spark-H3 (block-sparse
attention patch) + selflift-Avatar (two-pass sampler with 4-tile high-res spatial tiling)

3 × core LoraLoaderBypassModelOnly. I have accounted for all of those in the controls
below; the aimdo-side behaviour is independent of them.

Mechanism (from master source)

malloc_graph_pop() severs every allocation that is still alive in the scope being
popped (src/malloc-graph.c, collect_rogue_candidates()), registering each in the
global candidate list and bumping g->rogue_count:

static bool collect_rogue_candidates(MallocGraph *g) {
    Event *allocation = g->state->allocations;
    while (allocation) {
        if (!register_rogue_candidate(candidate_ptr(g, allocation))) { g->failed = true; ... }
        if (!sever_event(g, allocation)) { ... }
        allocation->allocation_previous = g->rogue_candidates;
        g->rogue_candidates = allocation;
        g->rogue_count++;
        ...
    }
}

malloc_graph_pop() calls it unconditionally. So any pop that happens while tensors
from that scope are still referenced by the application mints rogue candidates. In this
workload that is ~50 per model forward.

Freeing one of those candidates before adoption marks it freed — free_rogue() in
src/malloc-rogue.c falls into the "still a candidate" branch and sets
candidate->freed = true.

graph_failed() turns that into a permanent, whole-graph flag, and it runs on the
entry path of every API call:

static bool graph_failed(MallocGraph *g) {
    for (Event *candidate = g->rogue_candidates; candidate;
         candidate = candidate->allocation_previous) {
        if (rogue_candidate_freed(candidate_ptr(g, candidate))) { g->failed = true; break; }
    }
    return g->failed;
}

SHARED_EXPORT bool malloc_graph_pause(void *handle, bool paused, bool sync) {
    MallocGraph *g = handle;
    if (!g || g != active_graph || graph_failed(g)) return false;
    ...
}

Candidates are only meant to be resolved at the root pop —
if (!g->state->next && !finalize_rogues(g)) — i.e. after the entire forward unwinds.
Any candidate freed between a nested pop and the root pop poisons the graph.
Consequence for callers: there is no reset. malloc_graph_abort() refuses once
handoff_attempted is set, malloc_graph_destroy() is the only way out, and ComfyUI only
destroys at cleanup — so the prompt dies with RuntimeError.

Note on allocations while paused: malloc_graph_alloc() returns false when
g->paused (and when stream != g->stream), so paused-window allocations are never tracked
and never become candidates. This matters for triage — see the controls below.

Runtime evidence

A sitecustomize probe (PYTHONPATH only, no ComfyUI file modified) wrapped
MallocGraph.pause/pop/push and read the native failed byte directly at
handle + 0x5E0D0 (the offset graph_failed() writes; verified against the 0.5.6
disassembly of graph_failed at 0x8790 and malloc_graph_pause at 0xa880).

[01:17:44] NEW_GRAPH #1 handle=0x7e52db8b30e0 failed=0
[01:19:06] POP pop#762 pause#348 broken=False breaks#10 failed=0 rogues=154
[01:19:06] PAUSE_FAIL pause#349 graph#1 failed_byte=1 paused_byte=0 scopes=['block']
           rogues=154 peak=5584973824 vbytes=16626221056 phys=5662310400
           in_MALLOC_GRAPHS=True _comfy_active=True

failed is 0 at the last pop and 1 on the very next pause() a few ms later:
the flip happens between two block-boundary calls, i.e. exactly at the free of a
candidate, and the pause only reports it.
in_MALLOC_GRAPHS=True / _comfy_active=True: this is not the
g != active_graph branch — it is graph_failed().
rogue_count ladder over one graph lifetime (malloc_graph_stat(3), cumulative):
48 → 54 → 104 → 154, jumping only at forward boundaries, flat afterwards. So severing
happens at pops, and the fatal early-free comes later, from a candidate severed earlier.

Controls (each one a full rerun of the same API prompt)

Variant	What changed	rogue ladder	Fail point
baseline	—	48 / 54 / 104 / 154	pop#762 → pause#349
A	Spark-H3's `pause_malloc_graph()` narrowed to wrap only the native `rms_rope` kernel (`attn.qkv_proj` and `index_select` moved out of the paused window)	identical	pop#762 → pause#1045 (3× pauses/block, same block)
B	`comfy_kitchen.set_allocation_context` replaced with a no-op, so kitchen never pauses around its ops	identical	pop#762 → pause#349

Both controls leave the rogue ladder byte-identical → the candidates are not produced by
any Python-side pause window, consistent with point 5 above. The failure is bound to the block
sequence (always the same pop#762), not to the number of pauses.

Minimal shape of the trigger

one sampler node keeps the same malloc graph alive across ~7 model forwards
(2 low-res steps + transition + 4 tiled high-res forwards);
each forward runs 50 DiT blocks, each block pops prefetch scopes while tensors are still
referenced → ~50 candidates/forward;
Python drops one of those tensors before the root pop → graph poisoned.

A stock MiniMax-H3 graph with fewer forwards never reaches the plateau; the tiled/looped
caller is what makes it deterministic (7/7 runs, 3 different configurations).

Proposed fixes (any of these unblocks callers)

Do not poison on early free. When rogue_candidate_freed() is true, drop that
candidate and fence its VA with the existing black_holes / small_unusable machinery
instead of setting g->failed for the whole graph. The graph can keep planning around a
hole; it cannot continue with a global fatal flag.
Resolve candidates at every pop, not only at the root pop. finalize_rogues() at the
end of malloc_graph_pop() would adopt/fence candidates while their owners are still
alive, shrinking the window between "severed" and "adopted" to zero.
Expose a supported reset (malloc_graph_reset()), so ComfyUI can recreate the graph
mid-prompt instead of failing the run. Related: ComfyUI PR #16883 already stops cleanup
from calling abort() on a failed graph — a reset would let the caller retry instead of
dying.
While not fixing anything: the generic message (_call maps push/pop/pause/abort/
set_stream/create failures onto one string) makes remote triage very expensive. A closed
enum of failure reasons from the C side would be enough.

I'm happy to test a patch on the reproducer (7/7 deterministic) and report pass/fail per line.

#16665 — pop() failing at 0 % sampling (Qwen Image 2.1 cache), fixed via PR #16667
#16144 — MiniMax H3 + BlockSparse attention node, 188 breaks, OOM; user patch moved an
allocation inside pause_malloc_graph()
#16342 — MiniMax H3 could not start recording (still open)
#16883 — stop cleanup from calling abort() on an already-failed graph

Workaround currently in use

--disable-comfy-compiler (no malloc graph → no graph_failed()), at the cost of the flat
VRAM profile. Keeping the compiler is what this report is about.

Local verification of the proposed direction (same machine, same prompt)

Built comfy-aimdo v0.5.6 (the installed version, commit 08b37eec3) from source with
gcc 13 / nvcc 13.0 (funchook 1.1.3 via the repo's own scripts/build-linux-aimdo.sh) and
swapped the resulting comfy_aimdo/aimdo.so into site-packages (shipped wheel backed up,
sha256 8764ede0…), then replayed the same API prompt:

aimdo build	result	wall clock
v0.5.6 unpatched (our build)	fails, `node 328 SelfLiftAvatarH3Sampler`, `aimdo memory compile error`	545 s
v0.5.6 + patch below	completes, `graph breaks: 21, rogues: 235`, writes `1312×736 HEVC 24 fps + AAC 32k stereo, 10.125 s`	17 min 09 s

So the local rebuild reproduces the defect and the change below removes it end to end.

Patch actually tested (49 lines, `src/malloc-graph.c`)

 static bool graph_failed(MallocGraph *g) {
+#ifdef AIMDO_ROGUE_FREE_IS_FATAL
     for (Event *candidate = g->rogue_candidates; candidate;
          candidate = candidate->allocation_previous) {
         if (rogue_candidate_freed(candidate_ptr(g, candidate))) {
             g->failed = true;
             break;
         }
     }
+#else
+    /* Keep the scan for observability only: never poison the graph here. */
+    for (Event *candidate = g->rogue_candidates; candidate;
+         candidate = candidate->allocation_previous) {
+        if (rogue_candidate_freed(candidate_ptr(g, candidate))) {
+            break;
+        }
+    }
+#endif
     return g->failed;
 }

Rationale for it being sufficient rather than merely silencing:

while a candidate is still linked in g->rogue_candidates, rogue_va() / rogue_phys()
keep its address range out of any newly planned allocation, so the compiler cannot hand the
range out again;
a freed candidate is already a case handoff_rogues() reports as ROGUE_HANDOFF_FREED and
finalize_rogues() handles at the root pop (it is not adopted, then scrubbed);
nothing else depends on the poison: g->failed is still set by the genuine failure paths
(RETURN_G_FAILED, handoff_rogues() returning ROGUE_HANDOFF_ERROR, remap failures).

Caveats I can see, in the interest of a fair review: the successful run reports more breaks
(21) and candidates (235) than the fatal one (9 / 154), i.e. the graph keeps living with an
increasing number of severed ranges; whether the fenced-VA bookkeeping should eventually
recycle them (instead of holding them for the graph's lifetime) is your call — from the
caller's side, finishing the prompt beats dying at 60 % either way.

Happy to send this as a PR (rebased onto master — graph_failed() there is byte-identical)
with AIMDO_ROGUE_FREE_IS_FATAL as the A/B switch, if the direction is acceptable.

Originally created by @lonelystarCX on GitHub (Oct 22, 2026). Original GitHub issue: https://github.com/Comfy-Org/comfy-aimdo/issues/129 ## Summary On a long single-graph lifetime (a 7-forward sequence inside one sampler node) the aimdo memory compiler stops accepting work mid-sampling: ``` RuntimeError: aimdo memory compile error comfy/model_prefetch.py:39 in _PauseMallocGraph.__enter__ graph.pause(sync=self.sync) comfy_aimdo/malloc_graph.py:35 in MallocGraph.pause self._call(control.lib.malloc_graph_pause, True, sync) comfy_aimdo/malloc_graph.py:16 in MallocGraph._call raise RuntimeError("aimdo memory compile error") ``` The failing call is `malloc_graph_pause()`, but the graph was already dead before it: `g->failed` had been set because **one rogue candidate was freed before `finalize_rogues()` could adopt it**. From that point on `alloc/free/pause/push/pop/set_stream` all return false, i.e. the whole recording is unusable, and the only recovery is destroying the graph. Everything below is measured on 7 reruns of the same API prompt (5 baseline, 2 controls, all deterministic); the mechanism is read from `master` source and cross-checked against the shipped 0.5.6 binary. ## Environment | | | |---|---| | ComfyUI | v0.42.3, rev 6185 `edb3db00` (2026-10-20), Linux | | comfy-aimdo | **0.5.6** (commit `g08b37eec3`) | | comfy-kitchen | **0.2.39+spark.h3.sm89.cu130.1** — see artifact note | | torch / CUDA | 2.13.0+cu130, driver 610.57.04 | | Python | 3.12.11 | | GPU | RTX 4090 (48 GB modified card), 49140 MiB, cc 8.9 | | Allocator | `cudaMallocAsync`, dynamic VRAM enabled, `--async-offload 2` | | Launch | `python3 main.py --listen 127.0.0.1 --port 8188` (no compiler/VRAM flags) | **Artifact note (please read before dismissing as third-party):** `comfy-kitchen` is not the stock wheel — the Spark-H3 v0.1.5 installer replaces it with `0.2.39+spark.h3.sm89.cu130.1`. Graph is `MiniMax-H3` + `ComfyUI-Spark-H3` (block-sparse attention patch) + `selflift-Avatar` (two-pass sampler with 4-tile high-res spatial tiling) + 3 × core `LoraLoaderBypassModelOnly`. I have accounted for all of those in the controls below; the aimdo-side behaviour is independent of them. ## Mechanism (from master source) 1. **`malloc_graph_pop()` severs every allocation that is still alive in the scope being popped** (`src/malloc-graph.c`, `collect_rogue_candidates()`), registering each in the global candidate list and bumping `g->rogue_count`: ```c static bool collect_rogue_candidates(MallocGraph *g) { Event *allocation = g->state->allocations; while (allocation) { if (!register_rogue_candidate(candidate_ptr(g, allocation))) { g->failed = true; ... } if (!sever_event(g, allocation)) { ... } allocation->allocation_previous = g->rogue_candidates; g->rogue_candidates = allocation; g->rogue_count++; ... } } ``` `malloc_graph_pop()` calls it unconditionally. So **any pop that happens while tensors from that scope are still referenced by the application mints rogue candidates.** In this workload that is ~50 per model forward. 2. **Freeing one of those candidates before adoption marks it freed** — `free_rogue()` in `src/malloc-rogue.c` falls into the "still a candidate" branch and sets `candidate->freed = true`. 3. **`graph_failed()` turns that into a permanent, whole-graph flag**, and it runs on the entry path of every API call: ```c static bool graph_failed(MallocGraph *g) { for (Event *candidate = g->rogue_candidates; candidate; candidate = candidate->allocation_previous) { if (rogue_candidate_freed(candidate_ptr(g, candidate))) { g->failed = true; break; } } return g->failed; } ``` ```c SHARED_EXPORT bool malloc_graph_pause(void *handle, bool paused, bool sync) { MallocGraph *g = handle; if (!g || g != active_graph || graph_failed(g)) return false; ... } ``` 4. **Candidates are only meant to be resolved at the root pop** — `if (!g->state->next && !finalize_rogues(g))` — i.e. after the entire forward unwinds. Any candidate freed between a nested pop and the root pop poisons the graph. 5. Consequence for callers: there is no reset. `malloc_graph_abort()` refuses once `handoff_attempted` is set, `malloc_graph_destroy()` is the only way out, and ComfyUI only destroys at cleanup — so the prompt dies with `RuntimeError`. **Note on allocations *while paused*:** `malloc_graph_alloc()` returns false when `g->paused` (and when `stream != g->stream`), so paused-window allocations are never tracked and never become candidates. This matters for triage — see the controls below. ## Runtime evidence A `sitecustomize` probe (PYTHONPATH only, no ComfyUI file modified) wrapped `MallocGraph.pause/pop/push` and read the native `failed` byte directly at `handle + 0x5E0D0` (the offset `graph_failed()` writes; verified against the 0.5.6 disassembly of `graph_failed` at `0x8790` and `malloc_graph_pause` at `0xa880`). ``` [01:17:44] NEW_GRAPH #1 handle=0x7e52db8b30e0 failed=0 [01:19:06] POP pop#762 pause#348 broken=False breaks#10 failed=0 rogues=154 [01:19:06] PAUSE_FAIL pause#349 graph#1 failed_byte=1 paused_byte=0 scopes=['block'] rogues=154 peak=5584973824 vbytes=16626221056 phys=5662310400 in_MALLOC_GRAPHS=True _comfy_active=True ``` - `failed` is **0** at the last pop and **1** on the very next `pause()` a few ms later: the flip happens *between* two block-boundary calls, i.e. exactly at the free of a candidate, and the pause only reports it. - `in_MALLOC_GRAPHS=True` / `_comfy_active=True`: this is **not** the `g != active_graph` branch — it is `graph_failed()`. - `rogue_count` ladder over one graph lifetime (`malloc_graph_stat(3)`, cumulative): `48 → 54 → 104 → 154`, jumping only at forward boundaries, flat afterwards. So severing happens at pops, and the fatal early-free comes later, from a candidate severed earlier. ## Controls (each one a full rerun of the same API prompt) | Variant | What changed | rogue ladder | Fail point | |---|---|---|---| | baseline | — | 48 / 54 / 104 / 154 | pop#762 → pause#349 | | A | Spark-H3's `pause_malloc_graph()` narrowed to wrap only the native `rms_rope` kernel (`attn.qkv_proj` and `index_select` moved out of the paused window) | **identical** | pop#762 → pause#1045 (3× pauses/block, same block) | | B | `comfy_kitchen.set_allocation_context` replaced with a no-op, so kitchen never pauses around its ops | **identical** | pop#762 → pause#349 | Both controls leave the rogue ladder byte-identical → the candidates are **not** produced by any Python-side pause window, consistent with point 5 above. The failure is bound to the block sequence (always the same `pop#762`), not to the number of pauses. ## Minimal shape of the trigger - one sampler node keeps the *same* malloc graph alive across ~7 model forwards (2 low-res steps + transition + 4 tiled high-res forwards); - each forward runs 50 DiT blocks, each block pops prefetch scopes while tensors are still referenced → ~50 candidates/forward; - Python drops one of those tensors before the root pop → graph poisoned. A stock `MiniMax-H3` graph with fewer forwards never reaches the plateau; the tiled/looped caller is what makes it deterministic (7/7 runs, 3 different configurations). ## Proposed fixes (any of these unblocks callers) 1. **Do not poison on early free.** When `rogue_candidate_freed()` is true, drop that candidate and fence its VA with the existing `black_holes` / `small_unusable` machinery instead of setting `g->failed` for the whole graph. The graph can keep planning around a hole; it cannot continue with a global fatal flag. 2. **Resolve candidates at every pop, not only at the root pop.** `finalize_rogues()` at the end of `malloc_graph_pop()` would adopt/fence candidates while their owners are still alive, shrinking the window between "severed" and "adopted" to zero. 3. **Expose a supported reset** (`malloc_graph_reset()`), so ComfyUI can recreate the graph mid-prompt instead of failing the run. Related: ComfyUI PR #16883 already stops cleanup from calling `abort()` on a failed graph — a reset would let the caller retry instead of dying. 4. While not fixing anything: the generic message (`_call` maps push/pop/pause/abort/ set_stream/create failures onto one string) makes remote triage very expensive. A closed enum of failure reasons from the C side would be enough. I'm happy to test a patch on the reproducer (7/7 deterministic) and report pass/fail per line. ## Related - #16665 — `pop()` failing at 0 % sampling (Qwen Image 2.1 cache), fixed via PR #16667 - #16144 — MiniMax H3 + BlockSparse attention node, 188 breaks, OOM; user patch moved an allocation inside `pause_malloc_graph()` - #16342 — MiniMax H3 `could not start recording` (still open) - #16883 — stop cleanup from calling `abort()` on an already-failed graph ## Workaround currently in use `--disable-comfy-compiler` (no malloc graph → no `graph_failed()`), at the cost of the flat VRAM profile. Keeping the compiler is what this report is about. ## Local verification of the proposed direction (same machine, same prompt) Built `comfy-aimdo` **v0.5.6** (the installed version, commit `08b37eec3`) from source with gcc 13 / nvcc 13.0 (funchook 1.1.3 via the repo's own `scripts/build-linux-aimdo.sh`) and swapped the resulting `comfy_aimdo/aimdo.so` into site-packages (shipped wheel backed up, sha256 `8764ede0…`), then replayed the same API prompt: | aimdo build | result | wall clock | |---|---|---| | v0.5.6 unpatched (our build) | **fails**, `node 328 SelfLiftAvatarH3Sampler`, `aimdo memory compile error` | 545 s | | v0.5.6 + patch below | **completes**, `graph breaks: 21, rogues: 235`, writes `1312×736 HEVC 24 fps + AAC 32k stereo, 10.125 s` | 17 min 09 s | So the local rebuild reproduces the defect and the change below removes it end to end. ### Patch actually tested (49 lines, `src/malloc-graph.c`) ```diff static bool graph_failed(MallocGraph *g) { +#ifdef AIMDO_ROGUE_FREE_IS_FATAL for (Event *candidate = g->rogue_candidates; candidate; candidate = candidate->allocation_previous) { if (rogue_candidate_freed(candidate_ptr(g, candidate))) { g->failed = true; break; } } +#else + /* Keep the scan for observability only: never poison the graph here. */ + for (Event *candidate = g->rogue_candidates; candidate; + candidate = candidate->allocation_previous) { + if (rogue_candidate_freed(candidate_ptr(g, candidate))) { + break; + } + } +#endif return g->failed; } ``` Rationale for it being sufficient rather than merely silencing: - while a candidate is still linked in `g->rogue_candidates`, `rogue_va()` / `rogue_phys()` keep its address range out of any newly planned allocation, so the compiler cannot hand the range out again; - a freed candidate is already a case `handoff_rogues()` reports as `ROGUE_HANDOFF_FREED` and `finalize_rogues()` handles at the root pop (it is not adopted, then scrubbed); - nothing else depends on the poison: `g->failed` is still set by the genuine failure paths (`RETURN_G_FAILED`, `handoff_rogues()` returning `ROGUE_HANDOFF_ERROR`, remap failures). Caveats I can see, in the interest of a fair review: the successful run reports more breaks (21) and candidates (235) than the fatal one (9 / 154), i.e. the graph keeps living with an increasing number of severed ranges; whether the fenced-VA bookkeeping should eventually recycle them (instead of holding them for the graph's lifetime) is your call — from the caller's side, finishing the prompt beats dying at 60 % either way. Happy to send this as a PR (rebased onto master — `graph_failed()` there is byte-identical) with `AIMDO_ROGUE_FREE_IS_FATAL` as the A/B switch, if the direction is acceptable.

No labels

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Comfy-Org/comfy-aimdo#49

No description provided.

Rows
Columns