From 8632cc5fbb0b76695220ff033b413e7291b471fb Mon Sep 17 00:00:00 2001 From: Ken Simpson Date: Tue, 23 Jun 2026 09:54:04 -0700 Subject: [PATCH] fix: keep parallel sampler progress independent --- README.md | 1 + docs/LEARNINGS.md | 1 + tests/test_web_modal_toggle.py | 15 +++++++++++++-- web/modal_toggle.js | 34 ++++++++++++++++++++++++++++++---- 4 files changed, 45 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index a9267ab..5f516ad 100644 --- a/README.md +++ b/README.md @@ -340,6 +340,7 @@ Current mapped-execution rules: - streamed per-item progress lanes now follow the real executing node id instead of being forced onto the component representative node, so concurrent remote samplers no longer all paint their progress bars onto the first node in the remote island - mapped progress events now carry both the representative display id and the real executing node id, and the frontend resolves component membership from any node in that component so mapped fan-out runs do not leave sibling remote nodes stuck orange - mapped node highlighting now derives its purple/active state from live per-node lane or numeric progress, so overlapping `A -> B` work across different containers can keep both remote nodes active at once while ordinary non-mapped runs still use the older single-active-node behavior +- ordinary streamed node progress no longer lets one active sampler complete another sampler from a parallel remote component; previous-active cleanup is limited to nodes in the same component, so concurrent progress bars advance independently - worker lanes no longer emit a fake execution-progress bar on the component representative node before real node progress arrives, which avoids leaving stray lane bars behind on the first node in the remote island - mapped lane setup now emits its own `setup_only` lane events before a worker starts real node execution, and the frontend renders those as translucent pulsing lane bars. That makes cold-lane provisioning visible without pretending the worker is already doing sampler progress. - remote component dispatch now emits an immediate `starting` status before Modal streams real node progress, so long provisioning or worker-queue waits still show which remote island is pending instead of leaving the UI visually idle diff --git a/docs/LEARNINGS.md b/docs/LEARNINGS.md index 9d423f3..b5c7f94 100644 --- a/docs/LEARNINGS.md +++ b/docs/LEARNINGS.md @@ -266,3 +266,4 @@ - `CLIP` bridges have the same failure mode as linked `MODEL` bridges when a sampler component exports shared text-encoder state. Durable bridge rehydration should cover loader-style remote-only outputs such as `MODEL`, `CLIP`, and `VAE`, not only model patchers. - Durable subgraph rehydration plans can themselves depend on earlier bridge refs. They must execute inside the downstream target `remote_session`, otherwise nested bridge resolution has no session to restore into and fails before it can rebuild the requested loader-style output. - Dispatch-time feedback is separate from real remote execution. When Modal is provisioning or all warm workers are occupied, the local proxy can wait tens of seconds before the first streamed active-node event arrives, so the local dispatch path should emit a lightweight `starting` status immediately and let later streamed progress replace that temporary orange state. +- Prompt-wide active-node state is not a safe completion signal under parallel remote execution. If two components stream sampler progress at the same time, alternating events must not fade or complete the previous active node unless both nodes belong to the same remote component. diff --git a/tests/test_web_modal_toggle.py b/tests/test_web_modal_toggle.py index a94ec07..acce93d 100644 --- a/tests/test_web_modal_toggle.py +++ b/tests/test_web_modal_toggle.py @@ -246,16 +246,27 @@ def test_streamed_modal_node_progress_updates_active_overlay() -> None: def test_streamed_modal_node_progress_fades_previous_active_node() -> None: - """When remote progress moves to another node, the old active node should stop rendering purple.""" + """Only same-component progress should complete the previous active node.""" source = _modal_toggle_source() + assert "function nodesShareRemoteComponent(promptId, leftNodeId, rightNodeId)" in source assert "const previousActiveNodeId = promptState.activeNodeId;" in source - assert "previousActiveNodeId && previousActiveNodeId !== progressNodeId" in source + assert "previousActiveNodeId !== progressNodeId" in source + assert "nodesShareRemoteComponent(promptId, previousActiveNodeId, progressNodeId)" in source assert "fadeNodeProgress(previousActiveNodeId, promptId);" in source assert "setNodesPhase([previousActiveNodeId], STATE_COMPLETE, promptId);" in source assert "setPromptActiveNode(promptId, progressNodeId);" in source +def test_parallel_component_progress_does_not_complete_other_active_nodes() -> None: + """Parallel component progress should not treat the prompt-wide previous active node as complete.""" + source = _modal_toggle_source() + + assert "nodesShareRemoteComponent(promptId, previousActiveNodeId, nextActiveNodeId)" in source + assert "nodesShareRemoteComponent(promptId, previousActiveNodeId, progressNodeId)" in source + assert "promptState.activeNodeId && nodeIds.includes(promptState.activeNodeId)" in source + + def test_streamed_modal_node_progress_completes_reported_ancestors() -> None: """When a remote node becomes active, reported upstream nodes should be marked complete.""" source = _modal_toggle_source() diff --git a/web/modal_toggle.js b/web/modal_toggle.js index d11a1f9..483a9a1 100644 --- a/web/modal_toggle.js +++ b/web/modal_toggle.js @@ -1152,6 +1152,24 @@ function completeRemoteAncestorsBeforeActiveNode(promptId, ancestorNodeIds, acti setNodesPhase(completedNodeIds, STATE_COMPLETE, promptId); } +/** + * Return whether two nodes belong to the same known remote component. + * @param {string} promptId + * @param {string | null | undefined} leftNodeId + * @param {string | null | undefined} rightNodeId + * @returns {boolean} + */ +function nodesShareRemoteComponent(promptId, leftNodeId, rightNodeId) { + if (!leftNodeId || !rightNodeId) { + return false; + } + const leftComponentNodeIds = resolveComponentNodeIds(promptId, leftNodeId); + if (!leftComponentNodeIds?.length) { + return String(leftNodeId) === String(rightNodeId); + } + return leftComponentNodeIds.map((nodeIdValue) => String(nodeIdValue)).includes(String(rightNodeId)); +} + /** * Return the numeric progress payload for one node when it belongs to the prompt. * @param {string} nodeIdValue @@ -2527,7 +2545,11 @@ function handleModalStatus(event) { }); } setNodesPhase(nodeIds, STATE_READY, promptId); - if (previousActiveNodeId && previousActiveNodeId !== nextActiveNodeId) { + if ( + previousActiveNodeId && + previousActiveNodeId !== nextActiveNodeId && + nodesShareRemoteComponent(promptId, previousActiveNodeId, nextActiveNodeId) + ) { fadeNodeProgress(previousActiveNodeId, promptId); setNodesPhase([previousActiveNodeId], STATE_COMPLETE, promptId); } @@ -2547,11 +2569,11 @@ function handleModalStatus(event) { clearPromptQueued(promptId); promptState.hasStreamedProgress = true; promptState.hasRemoteExecutionStarted = true; - if (promptState.activeNodeId) { + if (promptState.activeNodeId && nodeIds.includes(promptState.activeNodeId)) { fadeNodeProgress(promptState.activeNodeId, promptId); setNodesPhase([promptState.activeNodeId], STATE_COMPLETE, promptId); + setPromptActiveNode(promptId, null); } - setPromptActiveNode(promptId, null); setNodesPhase(nodeIds, STATE_COMPLETE, promptId); return; } @@ -2659,7 +2681,11 @@ function handleModalProgress(event) { setNodesPhase(readyNodeIds, STATE_READY, promptId); } const previousActiveNodeId = promptState.activeNodeId; - if (previousActiveNodeId && previousActiveNodeId !== progressNodeId) { + if ( + previousActiveNodeId && + previousActiveNodeId !== progressNodeId && + nodesShareRemoteComponent(promptId, previousActiveNodeId, progressNodeId) + ) { fadeNodeProgress(previousActiveNodeId, promptId); setNodesPhase([previousActiveNodeId], STATE_COMPLETE, promptId); }