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

[PR #130] [CLOSED] malloc graph: do not poison the graph when a rogue candidate is freed early #131

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-aimdo/pull/130
Author: @lonelystarCX
Created: 10/22/2026
Status: ❌ Closed

Base: master ← Head: fix/rogue-freed-not-fatal

📝 Commits (2)

f9c7925 malloc graph: do not poison the graph when a rogue candidate is freed early
9185d8f malloc graph: drop the no-op candidate scan in the default build

📊 Changes

1 file changed (+19 additions, -0 deletions)

View changed files

📝 src/malloc-graph.c (+19 -0)

📄 Description

Follow-up to #129 (full mechanism, source walk-through and runtime evidence there).

What this changes

graph_failed() no longer marks the malloc graph failed when a rogue candidate was freed
before the root pop. Everything else is untouched: g->failed is still set by
RETURN_G_FAILED, by handoff_rogues() returning ROGUE_HANDOFF_ERROR, and by remap
failures.

Why the early free does not need to be fatal

While the 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).
Marking the graph failed instead poisons it permanently: every later
malloc_graph_alloc/free/pause/push/pop/set_stream returns false and callers only see
RuntimeError("aimdo memory compile error"), with no reset path.

-DAIMDO_ROGUE_FREE_IS_FATAL restores the old behaviour for A/B builds.

Verification

Same ComfyUI prompt, same machine, aimdo swapped in site-packages, shipped wheel backed up:

build	result
v0.5.6 unpatched, built locally (gcc 13 / nvcc 13.0, `scripts/build-linux-aimdo.sh`)	fails at `SelfLiftAvatarH3Sampler` with `aimdo memory compile error` (545 s)
v0.5.6 + this patch	completes, writes `1312x736 HEVC 24 fps + AAC 32k stereo, 10.125 s` (17 min 09 s)
master + this patch	builds clean (no warnings from `src/malloc-graph.c`)

The unpatched row matters: the local rebuild reproduces the defect, so the passing row is not
an artifact of building from source.

Caveats, for a fair review

The successful run reports graph breaks: 21, rogues: 235 (the fatal one had 9 / 154) — the
graph keeps living with more severed ranges. Whether those fenced ranges should eventually be
recycled (rather than held for the graph's lifetime) is your call.
Alternative directions from #129 that I did not take here, in case you prefer them:
run finalize_rogues() at every pop, or expose malloc_graph_reset() so a caller can
recreate the graph instead of failing the prompt.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-aimdo/pull/130 **Author:** [@lonelystarCX](https://github.com/lonelystarCX) **Created:** 10/22/2026 **Status:** ❌ Closed **Base:** `master` ← **Head:** `fix/rogue-freed-not-fatal` --- ### 📝 Commits (2) - [`f9c7925`](https://github.com/Comfy-Org/comfy-aimdo/commit/f9c7925ba983eaa753d2e4c177d6a42ce3d69e1d) malloc graph: do not poison the graph when a rogue candidate is freed early - [`9185d8f`](https://github.com/Comfy-Org/comfy-aimdo/commit/9185d8ff5ad89154e632cdf1dc383661746587c2) malloc graph: drop the no-op candidate scan in the default build ### 📊 Changes **1 file changed** (+19 additions, -0 deletions) <details> <summary>View changed files</summary> 📝 `src/malloc-graph.c` (+19 -0) </details> ### 📄 Description Follow-up to #129 (full mechanism, source walk-through and runtime evidence there). ## What this changes `graph_failed()` no longer marks the malloc graph failed when a rogue candidate was freed before the root pop. Everything else is untouched: `g->failed` is still set by `RETURN_G_FAILED`, by `handoff_rogues()` returning `ROGUE_HANDOFF_ERROR`, and by remap failures. ## Why the early free does not need to be fatal * While the 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). * Marking the graph failed instead poisons it permanently: every later `malloc_graph_alloc/free/pause/push/pop/set_stream` returns false and callers only see `RuntimeError("aimdo memory compile error")`, with no reset path. `-DAIMDO_ROGUE_FREE_IS_FATAL` restores the old behaviour for A/B builds. ## Verification Same ComfyUI prompt, same machine, `aimdo` swapped in site-packages, shipped wheel backed up: | build | result | |---|---| | v0.5.6 unpatched, built locally (gcc 13 / nvcc 13.0, `scripts/build-linux-aimdo.sh`) | **fails** at `SelfLiftAvatarH3Sampler` with `aimdo memory compile error` (545 s) | | v0.5.6 + this patch | **completes**, writes `1312x736 HEVC 24 fps + AAC 32k stereo, 10.125 s` (17 min 09 s) | | master + this patch | builds clean (no warnings from `src/malloc-graph.c`) | The unpatched row matters: the local rebuild reproduces the defect, so the passing row is not an artifact of building from source. ## Caveats, for a fair review * The successful run reports `graph breaks: 21, rogues: 235` (the fatal one had 9 / 154) — the graph keeps living with more severed ranges. Whether those fenced ranges should eventually be recycled (rather than held for the graph's lifetime) is your call. * Alternative directions from #129 that I did **not** take here, in case you prefer them: run `finalize_rogues()` at every pop, or expose `malloc_graph_reset()` so a caller can recreate the graph instead of failing the prompt. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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

closed this issue
added the
pull-request
label

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#131

No description provided.

Rows
Columns