Commit 1564dd772e378a7a20fe151632e864d38d75caff
1 parent
181d5244
graphlemon: auto-commit uncommitted changes for 6f49e70d-59a6-4e87-bb30-fb0e1202…
…a664-main-checkout-drift
Showing
2 changed files
with
88 additions
and
1 deletions
src/e2e_pipeline.py
| ... | ... | @@ -815,6 +815,24 @@ def _record_planner_review_event(conn, task_id, attempt, verdict, used_existing_ |
| 815 | 815 | conn.commit() |
| 816 | 816 | |
| 817 | 817 | |
| 818 | +def _existing_active_run_id(conn, task_id): | |
| 819 | + """2026-08-13 (real recurrence on task b243dfe8): finds a task_id's | |
| 820 | + existing non-terminal work_graph_run row directly against the table | |
| 821 | + ux_work_graph_run_active actually constrains, NOT via artifact rows | |
| 822 | + (unlike _existing_doc_artifact_contents/_shared_run_id_for_task) -- | |
| 823 | + a run can exist with a real graph_manifest but zero promoted artifacts | |
| 824 | + (e.g. interrupted before any node's content was written), which those | |
| 825 | + artifact-based lookups are blind to. Only one non-terminal row can ever | |
| 826 | + exist per task_id (the index enforces it), so this is unambiguous when | |
| 827 | + it returns non-None.""" | |
| 828 | + row = conn.execute( | |
| 829 | + "SELECT run_id FROM work_graph_run WHERE task_id = ? AND status NOT IN ('done', 'failed') " | |
| 830 | + "ORDER BY rowid DESC LIMIT 1", | |
| 831 | + (task_id,), | |
| 832 | + ).fetchone() | |
| 833 | + return row[0] if row else None | |
| 834 | + | |
| 835 | + | |
| 818 | 836 | def _review_planner_output_with_retries(conn, task_id, goal_text, target_folder_path, timeout=300): |
| 819 | 837 | """fresh-run (plan_and_review_documents) and resume-from- |
| 820 | 838 | reviewing_planner_output (task_execution.py) both drive this same core |
| ... | ... | @@ -852,12 +870,31 @@ def _review_planner_output_with_retries(conn, task_id, goal_text, target_folder_ |
| 852 | 870 | already uses for Coder, not a new one invented here. The very first |
| 853 | 871 | attempt (no prior run_id yet) is unaffected: `append_to_run_id=None` |
| 854 | 872 | still does the original fresh INSERT. |
| 873 | + | |
| 874 | + 2026-08-13 follow-up (same day, real recurrence on task b243dfe8 after | |
| 875 | + the fix above already landed): the fix above only threads `prior_run_id` | |
| 876 | + across retry ITERATIONS WITHIN one call to this function -- it missed a | |
| 877 | + second, real source of collision: a task can already have an orphaned | |
| 878 | + non-terminal `work_graph_run` row from a PRIOR, separately-interrupted | |
| 879 | + execution (e.g. a Planner call that persisted `graph_manifest` but | |
| 880 | + crashed/got restarted before any node's content was promoted to an | |
| 881 | + `artifact` row) *before this function is even called at all*. | |
| 882 | + `_existing_doc_artifact_contents` only finds a run via real artifact | |
| 883 | + rows, so it's blind to this case -- attempt 1 still started from | |
| 884 | + `prior_run_id = None` and collided with that leftover row exactly like | |
| 885 | + before. Seeding `prior_run_id` from `_existing_active_run_id()` (queries | |
| 886 | + `work_graph_run` directly, the actual table `ux_work_graph_run_active` | |
| 887 | + enforces uniqueness against, independent of whether artifacts exist | |
| 888 | + yet) closes this: since the index allows only one non-terminal row per | |
| 889 | + task_id, ANY such row IS "the" run for this task by construction | |
| 890 | + (Phase 2c merges Planner/Coder/Validator into one shared run), so | |
| 891 | + appending into it is always correct, never a wrong-run guess. | |
| 855 | 892 | """ |
| 856 | 893 | from src import task_execution # see _existing_doc_artifact_contents's docstring for why deferred |
| 857 | 894 | |
| 858 | 895 | feedback_suffix = "" |
| 859 | 896 | verdict = None |
| 860 | - prior_run_id = None | |
| 897 | + prior_run_id = _existing_active_run_id(conn, task_id) | |
| 861 | 898 | for attempt in range(1, _PLANNER_REVIEW_MAX_ATTEMPTS + 1): |
| 862 | 899 | existing = _existing_doc_artifact_contents(conn, target_folder_path, task_id) if attempt == 1 else {} |
| 863 | 900 | if existing: | ... | ... |
src/test_e2e_pipeline.py
| ... | ... | @@ -702,6 +702,56 @@ class ReviewPlannerOutputWithRetriesTest(PipelineTestBase): |
| 702 | 702 | ).fetchall() |
| 703 | 703 | self.assertEqual(len(events), 2) |
| 704 | 704 | |
| 705 | + def test_orphaned_preexisting_run_with_no_artifacts_appends_not_collides(self): | |
| 706 | + """Real prod recurrence (2026-08-13, task b243dfe8, AFTER the fix | |
| 707 | + above already landed): a task can already have a non-terminal | |
| 708 | + work_graph_run row with a real graph_manifest but ZERO artifact | |
| 709 | + rows -- e.g. an earlier Planner call that persisted its manifest | |
| 710 | + then crashed/got interrupted before any node's content was | |
| 711 | + promoted to an artifact. _existing_doc_artifact_contents only | |
| 712 | + finds a run via artifact rows, so it's blind to this case -- | |
| 713 | + attempt 1 used to still start from prior_run_id=None and collide | |
| 714 | + with this leftover row via the exact same IntegrityError. Seeding | |
| 715 | + prior_run_id from _existing_active_run_id() (queries | |
| 716 | + work_graph_run directly) fixes this.""" | |
| 717 | + orphaned_manifest = {"nodes": [{"node_id": "orphan_sdd", "artifact_type": "sdd", "depends_on": []}], "edges": []} | |
| 718 | + self.conn.execute( | |
| 719 | + "INSERT INTO work_graph_run (run_id, task_id, graph_manifest, status) " | |
| 720 | + "VALUES ('run-orphan', ?, ?, 'planning')", | |
| 721 | + (self.task_id, json.dumps(orphaned_manifest)), | |
| 722 | + ) | |
| 723 | + self.conn.commit() | |
| 724 | + | |
| 725 | + def fake_plan_subprocess_run(*args, **kwargs): | |
| 726 | + argv = args[0] | |
| 727 | + if not argv or argv[0] != "claude": | |
| 728 | + return _real_subprocess_run(*args, **kwargs) | |
| 729 | + prompt = argv[argv.index("-p") + 1] | |
| 730 | + self.assertIn( | |
| 731 | + "orphan_sdd", prompt, | |
| 732 | + "append mode must inject the orphaned run's existing node_ids into the prompt", | |
| 733 | + ) | |
| 734 | + manifest = {"nodes": [{"node_id": "sdd", "artifact_type": "sdd", "depends_on": []}], "edges": []} | |
| 735 | + payload = {"is_error": False, "result": "```json\n" + json.dumps(manifest) + "\n```"} | |
| 736 | + return subprocess.CompletedProcess(args=["claude"], returncode=0, stdout=json.dumps(payload), stderr="") | |
| 737 | + | |
| 738 | + with patch("src.work_graph_plan.subprocess.run", side_effect=fake_plan_subprocess_run), \ | |
| 739 | + patch.object(org_node_trigger, "trigger_org_node_structured") as mock_trigger_structured, \ | |
| 740 | + patch.object(org_node_trigger, "trigger_org_node") as mock_verdict: | |
| 741 | + mock_trigger_structured.return_value = {"content": "# SDD -- real"} | |
| 742 | + mock_verdict.return_value = "VERDICT: PASS\nlooks real" | |
| 743 | + | |
| 744 | + result = e2e_pipeline._review_planner_output_with_retries( | |
| 745 | + self.conn, self.task_id, "goal", self.tmp.name, | |
| 746 | + ) | |
| 747 | + | |
| 748 | + run_rows = self.conn.execute( | |
| 749 | + "SELECT run_id, status FROM work_graph_run WHERE task_id = ?", (self.task_id,) | |
| 750 | + ).fetchall() | |
| 751 | + self.assertEqual(len(run_rows), 1, "must append into the orphaned row, not INSERT a second one") | |
| 752 | + self.assertEqual(run_rows[0][0], "run-orphan") | |
| 753 | + self.assertEqual(result["run_id"], "run-orphan") | |
| 754 | + | |
| 705 | 755 | |
| 706 | 756 | class RunTaskLifecycleMissingArtifactTypesTest(PipelineTestBase): |
| 707 | 757 | """2026-08-08 regression (Org Graph unification Phase 1 §7): a | ... | ... |