Stopping a generation is a normal thing to do -- a bad seed, a wrong prompt,
a rental minute -- so it is not an error path that can be left approximate.
A leak here is invisible: the run AFTER a cancel just behaves slightly wrong.
The sampler says a run was cancelled and re-raises. It is not ours to
absorb; ComfyUI ends the queue item. But a run that simply stops with
nothing said is indistinguishable from one that broke.
Verified against the real engine: a 40-step generation interrupted mid-way
reports execution_interrupted, and the next one completes as run 2. Tests
cover what must be left behind, which is nothing -- no wrappers on the shared
model after five cancelled runs, the sharp anchor put back on a denoiser that
outlives the run, and no modifier marked as dropped, because a cancelled
modifier did not fail and disabling it next time would punish the user for
stopping.
Temp previews needed no change: core's PreviewImage already writes to the
temp directory under a unique prefix, so where the app SHOWS them is a UI
decision and is recorded as one rather than built here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The run id froze after the first generation. Runs were marked by the
modifiers node, and ComfyUI CACHES a node whose inputs have not changed --
only the seed changes between most generations, so that node executed once
and every later run reported under the first one's id. Per-run log dedup
degraded to per-process with it, which is precisely the v4 fault log.py's
own comment describes, reintroduced through a caching interaction the fix
did not account for.
The sampler marks the run now. It is what generates, and it re-executes
whenever anything about the generation differs. Verified through the real
PromptExecutor with caching on: three seeds, three run ids. The
dropped-modifier record is also emptied there, because it travels on the
model and ComfyUI may hand the same model back -- a modifier that failed
last time deserves this time.
And a bad settings payload was only caught during execution, after the
checkpoint had already loaded. On a rented GPU that is the cost paid before
the guard fires, one layer up from the sampling case this project already
guards. ComfyUI calls validate_inputs inside validate_prompt, so the payload
is now understood before the queue accepts it: a typo'd module id is refused
with nothing loaded. Both doors check the same things, and a test pins that
they agree.
There was no test file for runs at all, which is why the first fault could
be introduced by a fix and survive a green suite.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
v4's Chain Sampler became a hub: 8183 lines, roughly seventy optional
inputs, a _sample_chunk that grew six ALG arguments, and a hardcoded list of
knobs silently inert on one model family. Every technique that needed the
loop was added to it, so it collected all of them.
This one picks an algorithm and a schedule from ComfyUI's own registries --
so a sampler it gains appears here without an edit -- and runs them. Three
things stop it becoming a hub again. It never learns what a model-side
modifier is. Sampler-side ones reach it through ONE call site,
chain.process(ctx, latent), which cannot grow because the step travels in a
context object rather than in the signature. And it declares the hook points
it offers, so core hands it only what fits and it never names a modifier.
Some modifiers genuinely cannot ride on a model: they need the step index,
the schedule, or a second pass. Those attach here instead, and one asking
for a hook this sampler does not offer is absent and said so, not
half-running.
Verified against ComfyUI's own guider path with real weights: bit-identical
output. That took correcting the method, not the code -- see below.
MEASUREMENT, corrected: sampling on this Mac's MPS backend is
NONDETERMINISTIC. The same node with the same seed and the same inputs
differs run to run: 2.42 by max latent difference, 0.0008 by mean pixel gap.
Comparing by MAX said this sampler diverged from core by 14.2 and it was
about to be "fixed"; by MEAN against a measured noise floor it is 0.00000
while core against itself is 0.00183. Every comparison here now runs a
control twice first. Re-checked the Detail modifier the same way: 145x the
floor, so that claim stands.
Also matched what core's sampler actually does and this did not:
fix_empty_latent_channels using the ratios our own empty-latent node reports,
the preview and progress callback, the move back to the intermediate device,
and dropping the ratio keys so nothing rescales twice.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cancel reaches a hook as an exception like any other, and a guard that
caught it would leave someone pressing stop on a run that will not stop.
It works today only because ComfyUI made InterruptProcessingException a
BaseException rather than an Exception -- a detail this code depends on and
does not control. Widening the guard to BaseException would break Cancel
with nothing else failing, so the property is now a test rather than an
accident.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Catching a modifier that fails to INSTALL was never the hard part. The one
that costs a rental is the hook that works at step 0 and raises at step 3,
after the GPU time is spent -- which the Detail modifier does on any video
latent, because core's sharpen kernel cannot take a 5-D tensor.
A module is now handed a patcher that guards every hook it installs, so it
does not opt in to being safe: a module that could forget to is a module
that can break the foundation. A hook that raises is dropped for the rest of
that run, reported once with its traceback, and sampling continues. Thirty
steps on the latent that used to end the run now complete untouched.
Recorded per RUN, not per process, so a long session reports every
generation that went inert instead of only the first after a restart.
Full control turns all of it off. A one-step schedule, a width the model
cannot use, a modifier that throws -- someone exploring the edges wants the
real behaviour and the real traceback, not a careful refusal. One switch
rather than one per guard, because a half-disabled safety net is harder to
reason about than either extreme. It is asked for by capability, so nothing
names the module that answers, and a build without it installed has nobody
saying yes -- which reads as guarded.
Honest about its own limit: a hook shape with no known neutral result is
installed unguarded and named in the status, rather than counted as safe.
Two test faults fixed while writing this. The assertions compared conds_out
against the list passed in -- the same object the hook mutates -- so they
were true whatever the hook did. And the registry fakes were hand-rolled and
missing methods the real one has, which passed until the code called one.
They use the real Registry now.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Decode now takes the MODEL, so a provider answers "is this mine" by identity
instead of by shape. H3 was claiming any latent that happened to have two
parts -- right today because it is the only thing producing one, wrong the
moment a second AV model exists, and wrong in the way that reads as an
unrelated fault rather than an error. It declines when the model is not
wired, and the node says that is what is missing.
That completes the shape a model module should have: one folder saying how
this model behaves at each stage -- what it is, how its latent is built, how
its output is decoded -- instead of that knowledge spreading through the
conditioning and sampling code as special cases.
Added a modifier that runs on SD1.5, because a feature nobody can watch is a
feature nobody can check. It declares no traits, so it applies everywhere,
which is the other half of the rule ALG demonstrates by narrowing. Measured
on this machine at a fixed seed: off and on differ, 0.4 gives a visibly
richer picture that still reads correctly, and 1.2 blows the highlights --
so the range is -1..1, set from what was measured rather than from a round
number. A slider whose top half destroys the image lies about its range.
Two things found while building it. ComfyUI has a real circular import
between nodes_latent and nodes_post_processing that resolves only because
init_extra_nodes reaches one first; a custom node has to break it the same
way. And the accumulation bug returned through a third door: stripping
handled keyed wrappers but not the anonymous function lists on
model_options, so chaining doubled the effect. Tagging is now generic -- it
looks for our mark rather than for a list of hook names that would need
keeping current.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The NaN guard was on one of two doors. A FUNPACK_SETTINGS socket is typed by
a string tag, so a dict reaches the load node without necessarily having
passed through the settings node -- a raw API prompt does exactly that.
Through that door, until_sigma = NaN made ALG a permanent no-op: sigma > nan
is False at every step, so the anchor was never loosened, and the node
reported "1 modifier applied: alg". Nothing about the run looked wrong,
which is the only reason this is worth a commit of its own.
Values are now checked against the declaration at the point they are
consumed. The producer still checks too; that is not redundant, it is where
a person editing JSON gets told immediately.
Also recorded ALG's standing assumption rather than leaving it implicit: the
anchor is latent frame 0. True of everything it can currently be wired into,
untrue in general, and the denoise_mask already carries what would be needed
to do it properly. Left undone because nothing yet pins elsewhere, and a
derivation with no consumer cannot be checked against reality.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The claim in the last commit was too strong. Cloning stops this node
touching the model everyone else holds; it does nothing about the node's own
output coming back in. add_wrapper_with_key APPENDS under the key and a
clone carries the list, so chaining two of these -- two passes, a flattened
subgraph -- installed the same wrapper twice and ran it twice per step at
double strength, while both nodes reported "1 modifier(s) applied".
That is the accumulation this design is supposed to prevent, arriving
through a different door. Every install is now preceded by stripping our own
namespaced keys and saying how many were replaced. Foreign wrappers are
never in scope, which is what the namespace was for.
NaN passed validation: it compares False against both bounds, so a range
check alone never fires, and json.loads accepts NaN and the infinities by
default. A NaN strength reached a modifier and would have poisoned sampling
with nothing reported. Non-finite numbers are now refused by name.
A settings payload of the wrong shape raised AttributeError rather than the
error reporting everything else here uses; the socket type is only a string
match, so anything can arrive on it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two nodes. One carries the values a person picked, as a single validated
payload rather than a socket per setting -- a socket per setting means this
node's shape changes whenever any module gains an option, and every saved
workflow refers to sockets that moved.
The other reads what announced itself, filters it by what the model actually
is, orders it by declared relations, and installs each one through ComfyUI's
own patcher_extension wrappers. The consequence is checked against comfy's
real prepare_model_patcher and WrapperExecutor rather than against our own
dict: a model that has been through this node runs every per-step modifier
inside ComfyUI's own SamplerCustomAdvanced. The sampler is not consulted, so
it cannot own them.
Installing happens on a clone, and clone() copies the wrapper lists per key,
so the model everyone else holds is untouched. Fifty runs leave zero
wrappers behind. The accumulation that once degraded output across a hundred
generations and survived every reset short of a restart is structurally
impossible here rather than something to remember to undo.
The payload is validated against the same declaration that renders the
panel: an undeclared key, a wrong type, a value outside its own bounds and a
bool passed as a number are all refused, and all problems are reported at
once. v4 validated none of it, which is why it needed a hand-mirrored table
of every default and a list of keys that no longer meant anything.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
