fix: promote a retried node's latest accepted attempt, not its first

Real incident (2026-08-15, task b243dfe8): a node that went through loop_back/retry_self gets a fresh "<base>-attemptN" work_node_execution row each retry (work_graph_orchestrate._next_exec_node_id, N counted from real loop_back_triggered events) -- but graph_manifest only ever names the BASE node_id, never any "-attemptN" variant. _write_completed_node_artifacts's lookup (topological_order yields base node_ids) always resolved to done_outputs[base] -- attempt 1's REJECTED content, the only one filed under the bare node_id -- permanently stale the instant a node was ever rejected-then-retried-then-accepted. First surfaced today because this task is the first to actually reach "retried, then eventually accepted" for a node with real downstream artifact promotion. Fixed by resolving each base node_id to its highest-numbered "-attemptN" sibling's content before the promotion loop runs -- the highest attempt number is the most recently executed one, which is only still 'done' in the DB if the graph actually moved past it (a still-rejected final attempt would have either retried again or raised, both already handled by the existing _failed_node_ids() exclusion, which operates on base node_ids and is unaffected by this remap since only the dict VALUE changes, not the key). New tests: one drives the real 3-attempt scenario and confirms the promoted artifact holds attempt 3's content, not attempt 1's (verified failing on the pre-fix code with the exact real content mismatch); another confirms the exhausted-retry exclusion still works correctly after the remap. 847 tests total in the suite (24 new ones landed via a concurrent, unrelated self-hosting run -- see conversation); the 8 files this fix and today's other fixes actually touch are 476/476 green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

fix: promote a retried node's latest accepted attempt, not its first
Real incident (2026-08-15, task b243dfe8): a node that went through loop_back/retry_self gets a fresh "<base>-attemptN" work_node_execution row each retry (work_graph_orchestrate._next_exec_node_id, N counted from real loop_back_triggered events) -- but graph_manifest only ever names the BASE node_id, never any "-attemptN" variant. _write_completed_node_artifacts's lookup (topological_order yields base node_ids) always resolved to done_outputs[base] -- attempt 1's REJECTED content, the only one filed under the bare node_id -- permanently stale the instant a node was ever rejected-then-retried-then-accepted. First surfaced today because this task is the first to actually reach "retried, then eventually accepted" for a node with real downstream artifact promotion. Fixed by resolving each base node_id to its highest-numbered "-attemptN" sibling's content before the promotion loop runs -- the highest attempt number is the most recently executed one, which is only still 'done' in the DB if the graph actually moved past it (a still-rejected final attempt would have either retried again or raised, both already handled by the existing _failed_node_ids() exclusion, which operates on base node_ids and is unaffected by this remap since only the dict VALUE changes, not the key). New tests: one drives the real 3-attempt scenario and confirms the promoted artifact holds attempt 3's content, not attempt 1's (verified failing on the pre-fix code with the exact real content mismatch); another confirms the exhausted-retry exclusion still works correctly after the remap. 847 tests total in the suite (24 new ones landed via a concurrent, unrelated self-hosting run -- see conversation); the 8 files this fix and today's other fixes actually touch are 476/476 green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
bboonnee
1 parent 378fccd8
Showing 2 changed files with 76 additions and 0 deletions
src/e2e_pipeline.py
src/test_e2e_pipeline.py
@@ -625,6 +625,30 @@ def _write_completed_node_artifacts(
         reflect_id = node_id + "-reflect"
         if reflect_id in done_outputs:
             done_outputs[node_id] = done_outputs[reflect_id]
+    # 2026-08-15 (real incident, task b243dfe8): a node that went through
+    # loop_back/retry_self gets a fresh "<base>-attemptN" work_node_execution
+    # row each time (work_graph_orchestrate._next_exec_node_id, N counted
+    # from real loop_back_triggered events) -- graph_manifest itself only
+    # ever names the BASE node_id, never any "-attemptN" variant. Without
+    # this remap, the lookup two lines below (topological_order yields base
+    # node_ids) always resolved to done_outputs[base] -- attempt 1's
+    # content, the FIRST one, permanently stale the instant a node is ever
+    # rejected-then-retried-then-accepted. This node judged 'ok' via its own
+    # verify_fn is real content sitting in the DB; it was simply never
+    # findable under the id this function looks it up by. Highest attempt
+    # number = most recently executed = the one the graph actually moved
+    # past (if the LATEST attempt were still rejected, run_graph() would
+    # either have retried again or raised -- caught by the existing
+    # _failed_node_ids() exclusion below either way, which already operates
+    # on base node_ids and is unaffected by this remap).
+    attempts_by_base = {}
+    for node_id in done_outputs:
+        base, sep, suffix = node_id.rpartition("-attempt")
+        if sep and suffix.isdigit() and base in done_outputs:
+            attempts_by_base.setdefault(base, []).append((int(suffix), node_id))
+    for base, entries in attempts_by_base.items():
+        _, latest_id = max(entries)
+        done_outputs[base] = done_outputs[latest_id]
     for excluded in _failed_node_ids(run_graph_error):
         done_outputs.pop(excluded, None)
@@ -336,6 +336,58 @@ class WriteCompletedNodeArtifactsTest(PipelineTestBase):
         )
         self.assertEqual(set(artifact_ids), {"sdd", "benchmark"})
+    def test_retried_node_promotes_its_latest_attempts_content_not_the_first(self):
+        # Real incident (2026-08-15, task b243dfe8): a node that went
+        # through loop_back/retry_self gets a fresh "<base>-attemptN"
+        # work_node_execution row each retry -- graph_manifest only ever
+        # names the base node_id. Without resolving to the highest attempt
+        # number, this function always found attempt 1's REJECTED content
+        # (the only one filed under the bare node_id), never the later
+        # attempt that actually got accepted -- permanently stale the
+        # instant a node was ever rejected-then-retried-then-accepted.
+        manifest = {
+            "nodes": [{"node_id": "benchmark", "artifact_type": "benchmark", "depends_on": []}],
+            "edges": [],
+        }
+        self._mark_done("benchmark", "# REJECTED first attempt")
+        self._mark_done("benchmark-attempt2", "# also rejected")
+        self._mark_done("benchmark-attempt3", "# finally accepted -- this is the real content")
+        node_artifact_types = {"benchmark": "benchmark"}
+
+        artifact_ids = e2e_pipeline._write_completed_node_artifacts(
+            self.conn, self.tmp.name, "task-1", "run-1", manifest, node_artifact_types, lambda t: "md",
+        )
+        self.assertEqual(set(artifact_ids), {"benchmark"})
+        content = self.conn.execute(
+            "SELECT file_path FROM artifact WHERE artifact_id = ?", (artifact_ids["benchmark"],)
+        ).fetchone()[0]
+        with open(os.path.join(self.tmp.name, content)) as f:
+            self.assertEqual(f.read(), "# finally accepted -- this is the real content")
+
+    def test_exhausted_retry_still_excluded_even_with_multiple_attempts(self):
+        # The _failed_node_ids(run_graph_error) exclusion (existing
+        # behavior) must still work correctly after the attempt-number
+        # remap above -- it operates on base node_ids, which the remap
+        # preserves as dict keys (only the VALUE/content changes).
+        manifest = {
+            "nodes": [{"node_id": "benchmark", "artifact_type": "benchmark", "depends_on": []}],
+            "edges": [],
+        }
+        self._mark_done("benchmark", "# rejected")
+        self._mark_done("benchmark-attempt2", "# rejected again, exhausted budget")
+        run_graph_error = work_graph_orchestrate.ConsistencyCheckFailedError(
+            "loop_back from benchmark to benchmark exceeded max_attempts", node_id="benchmark",
+        )
+        node_artifact_types = {"benchmark": "benchmark"}
+
+        artifact_ids = e2e_pipeline._write_completed_node_artifacts(
+            self.conn, self.tmp.name, "task-1", "run-1", manifest, node_artifact_types, lambda t: "md",
+            run_graph_error=run_graph_error,
+        )
+        self.assertEqual(artifact_ids, {})
+        count = self.conn.execute("SELECT COUNT(*) FROM artifact WHERE task_id = 'task-1'").fetchone()[0]
+        self.assertEqual(count, 0)
+
 class PlanDocumentsTest(PipelineTestBase):
     def test_produces_five_artifacts_with_correct_lineage(self):