Commit 809e3676dba3172a8093a3f99c11c365097fce42
1 parent
ff96cdbd
graphlemon: test_report artifact for task 911861fe-446a-4bab-8b69-b4872b92d616
Showing
1 changed file
with
199 additions
and
0 deletions
.graphlemon/runs/911861fe-446a-4bab-8b69-b4872b92d616/test_report.md
0 → 100644
| 1 | +### n_impl_deploy_stage_mutex | ||
| 2 | + | ||
| 3 | +## Change summary | ||
| 4 | + | ||
| 5 | +### supervisor/deploy_stage.py | ||
| 6 | +- Added `import sqlite3` to the stdlib import block. | ||
| 7 | +- Added `ConcurrentDeploymentError` (right after `CheckoutFailedError`), carrying `git_commit_hash`, `blocking_deployment_id`, `blocking_status` as real attributes plus a human-readable message. | ||
| 8 | +- Added `_is_concurrent_deployment_conflict(error)`: returns True only when the exact string `"ux_deployment_active"` appears in the `IntegrityError` message, so other integrity violations are never misclassified as concurrency conflicts. | ||
| 9 | +- Added `_find_blocking_deployment(conn)`: best-effort, read-only lookup of the most recent (`ORDER BY rowid DESC LIMIT 1`) non-terminal `deployment` row; swallows `sqlite3.Error` and returns `None` on a broken connection instead of raising. | ||
| 10 | +- In `stage_deployment()`, wrapped only the `INSERT INTO deployment ... / conn.commit()` pair in `try/except sqlite3.IntegrityError`: on a real `ux_deployment_active` conflict it calls `conn.rollback()` (so the connection stays usable — no dangling implicit transaction), looks up the blocking row via `_find_blocking_deployment`, and raises `ConcurrentDeploymentError` (`raise ... from e`). Any other `IntegrityError` is re-raised untouched. The subsequent `_checkout_candidate` call and its own `CheckoutFailedError` handling were left byte-for-byte unchanged. No signature or success-return-value changes. | ||
| 11 | + | ||
| 12 | +### supervisor/test_deploy_stage.py | ||
| 13 | +- Added `import sqlite3`. | ||
| 14 | +- Tightened `test_only_one_non_terminal_deployment_allowed` to assert `deploy_stage.ConcurrentDeploymentError` specifically and check `git_commit_hash`, `blocking_deployment_id` (matches the first call's returned `deployment_id`), `blocking_status` (`"candidate_starting"`), and that only one row exists in `deployment` afterward. | ||
| 15 | +- Added a new `ConcurrentDeploymentTest(DeployStageTestBase)` class with: | ||
| 16 | + - `test_rolled_back_prior_deployment_allows_new_staging` / `test_blocked_pending_human_prior_deployment_allows_new_staging`: seed a row with that terminal status, confirm a new `stage_deployment()` call succeeds (`status == "candidate_starting"`, no exception). | ||
| 17 | + - `test_is_concurrent_deployment_conflict_matches_only_the_exact_index_name`: direct unit test of `_is_concurrent_deployment_conflict` against a matching `ux_deployment_active` message (True), an unrelated `NOT NULL` message (False), and a differently-named unique index message `ux_some_other_index` (False) — proves exact-name matching, not a loose `"UNIQUE constraint failed"` substring check. | ||
| 18 | + - `test_find_blocking_deployment_returns_most_recently_inserted_by_rowid`: drops `ux_deployment_active` for this test only (it's the one thing that would otherwise make "two non-terminal rows coexisting" impossible under the real schema), seeds two non-terminal rows, and asserts `_find_blocking_deployment` returns the later one by rowid. | ||
| 19 | + - `test_find_blocking_deployment_returns_none_on_broken_connection`: closed connection -> `None`, no raise. | ||
| 20 | + - `test_conn_still_usable_after_conflict_is_caught`: after catching `ConcurrentDeploymentError`, `self.conn.execute("SELECT 1")` still works — proves the `conn.rollback()` in the except branch actually clears the dangling transaction. | ||
| 21 | + - `test_every_non_excluded_status_blocks_staging_with_concurrent_deployment_error`: parametrized-style loop (stdlib has no native parametrize) over every `deployment.status` CHECK-constraint value from `src/schema.py` that is *not* in `ux_deployment_active`'s exclusion set (`staged`, `candidate_starting`, `health_check_running`, `health_check_passed`, `health_check_failed`, `cutover_in_progress`, `live`, `monitoring`, `anomaly_detected`, `rollback_in_progress`, `rollback_failed`, `monitoring_window_clean` — explicitly including the less-obvious `health_check_failed`/`rollback_failed` cases). For each: seeds a row with that status, asserts `stage_deployment()` raises `ConcurrentDeploymentError` with the right `blocking_deployment_id`/`blocking_status`, `_launch_candidate` is never called, and the `deployment` table still has exactly one row, then deletes the seed row before moving to the next status. | ||
| 22 | + - Confirmed the pre-existing `CheckoutFailedTest` tests (`test_nonexistent_commit_marks_blocked_pending_human_and_raises`, `test_checkout_failure_leaves_no_candidate_process_launched`) still pass unchanged — that code path is provably unaffected. | ||
| 23 | + | ||
| 24 | +Only `supervisor/deploy_stage.py` and `supervisor/test_deploy_stage.py` were modified (`git status --short` confirms no other files touched); `src/schema.py` was read-only (used to confirm the authoritative `deployment.status` CHECK-constraint list and the `ux_deployment_active` index definition). | ||
| 25 | + | ||
| 26 | +## Actual final test run output | ||
| 27 | + | ||
| 28 | +``` | ||
| 29 | +$ python3 -m unittest supervisor.test_deploy_stage -v | ||
| 30 | +test_no_prior_finalized_deployment_bootstraps_from_current_repo_state (supervisor.test_deploy_stage.BootstrapPreviousTest.test_no_prior_finalized_deployment_bootstraps_from_current_repo_state) ... ok | ||
| 31 | +test_prior_finalized_deployment_supplies_previous_fields (supervisor.test_deploy_stage.BootstrapPreviousTest.test_prior_finalized_deployment_supplies_previous_fields) ... ok | ||
| 32 | +test_missing_prod_db_bootstraps_a_fresh_schema_instead_of_crashing (supervisor.test_deploy_stage.BootstrapProdDbTest.test_missing_prod_db_bootstraps_a_fresh_schema_instead_of_crashing) ... ok | ||
| 33 | +test_checkout_failure_leaves_no_candidate_process_launched (supervisor.test_deploy_stage.CheckoutFailedTest.test_checkout_failure_leaves_no_candidate_process_launched) ... ok | ||
| 34 | +test_nonexistent_commit_marks_blocked_pending_human_and_raises (supervisor.test_deploy_stage.CheckoutFailedTest.test_nonexistent_commit_marks_blocked_pending_human_and_raises) ... ok | ||
| 35 | +test_blocked_pending_human_prior_deployment_allows_new_staging (supervisor.test_deploy_stage.ConcurrentDeploymentTest.test_blocked_pending_human_prior_deployment_allows_new_staging) ... ok | ||
| 36 | +test_conn_still_usable_after_conflict_is_caught (supervisor.test_deploy_stage.ConcurrentDeploymentTest.test_conn_still_usable_after_conflict_is_caught) ... ok | ||
| 37 | +test_every_non_excluded_status_blocks_staging_with_concurrent_deployment_error (supervisor.test_deploy_stage.ConcurrentDeploymentTest.test_every_non_excluded_status_blocks_staging_with_concurrent_deployment_error) ... ok | ||
| 38 | +test_find_blocking_deployment_returns_most_recently_inserted_by_rowid (supervisor.test_deploy_stage.ConcurrentDeploymentTest.test_find_blocking_deployment_returns_most_recently_inserted_by_rowid) ... ok | ||
| 39 | +test_find_blocking_deployment_returns_none_on_broken_connection (supervisor.test_deploy_stage.ConcurrentDeploymentTest.test_find_blocking_deployment_returns_none_on_broken_connection) ... ok | ||
| 40 | +test_is_concurrent_deployment_conflict_matches_only_the_exact_index_name (supervisor.test_deploy_stage.ConcurrentDeploymentTest.test_is_concurrent_deployment_conflict_matches_only_the_exact_index_name) ... ok | ||
| 41 | +test_rolled_back_prior_deployment_allows_new_staging (supervisor.test_deploy_stage.ConcurrentDeploymentTest.test_rolled_back_prior_deployment_allows_new_staging) ... ok | ||
| 42 | +test_disable_new_goal_dispatch_loop_flag_present (supervisor.test_deploy_stage.LaunchCandidateArgsTest.test_disable_new_goal_dispatch_loop_flag_present) ... ok | ||
| 43 | +test_disable_reconciliation_loop_flag_present (supervisor.test_deploy_stage.LaunchCandidateArgsTest.test_disable_reconciliation_loop_flag_present) ... ok | ||
| 44 | +test_real_candidate_process_actually_serves_http (supervisor.test_deploy_stage.RealCandidateLaunchTest.test_real_candidate_process_actually_serves_http) ... /opt/homebrew/Cellar/python@3.14/3.14.5/Frameworks/Python.framework/Versions/3.14/lib/python3.14/subprocess.py:1139: ResourceWarning: subprocess 77329 is still running | ||
| 45 | + _warn("subprocess %s is still running" % self.pid, | ||
| 46 | +ResourceWarning: Enable tracemalloc to get the object allocation traceback | ||
| 47 | +ok | ||
| 48 | +test_creates_candidate_starting_row_with_real_worktree_and_db_copy (supervisor.test_deploy_stage.StageDeploymentHappyPathTest.test_creates_candidate_starting_row_with_real_worktree_and_db_copy) ... ok | ||
| 49 | +test_only_one_non_terminal_deployment_allowed (supervisor.test_deploy_stage.StageDeploymentHappyPathTest.test_only_one_non_terminal_deployment_allowed) ... ok | ||
| 50 | + | ||
| 51 | +---------------------------------------------------------------------- | ||
| 52 | +Ran 17 tests in 7.465s | ||
| 53 | + | ||
| 54 | +OK | ||
| 55 | +``` | ||
| 56 | + | ||
| 57 | +(The `ResourceWarning` in `RealCandidateLaunchTest` is pre-existing, unrelated timing noise from that test's real subprocess teardown check — not caused by this change; the test still reports `ok` and the overall suite result is `OK`.) | ||
| 58 | + | ||
| 59 | +--- | ||
| 60 | + | ||
| 61 | +### n_full_regression_deploy_stage_mutex | ||
| 62 | + | ||
| 63 | +## Verification result: independent re-run of both suites + source confirmation | ||
| 64 | + | ||
| 65 | +### 1. `python3 -m unittest discover -s src` (run twice, reproducible) | ||
| 66 | + | ||
| 67 | +``` | ||
| 68 | +FAIL: test_contains_src_and_git (test_paths.RepoRootTest.test_contains_src_and_git) | ||
| 69 | +---------------------------------------------------------------------- | ||
| 70 | +Traceback (most recent call last): | ||
| 71 | + File ".../src/test_paths.py", line 93, in test_contains_src_and_git | ||
| 72 | + self.assertTrue(os.path.isdir(os.path.join(root, ".git"))) | ||
| 73 | + ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| 74 | +AssertionError: False is not true | ||
| 75 | + | ||
| 76 | +---------------------------------------------------------------------- | ||
| 77 | +Ran 802 tests in 72.330s | ||
| 78 | + | ||
| 79 | +FAILED (failures=1, skipped=12) | ||
| 80 | +``` | ||
| 81 | + | ||
| 82 | +Exception code == 1. This is NOT clean. However this single failure is a **pre-existing, unrelated environmental issue**, not caused by the deploy_stage.py change: | ||
| 83 | + | ||
| 84 | +- `src/test_paths.py:90-93` (`RepoRootTest.test_contains_src_and_git`) asserts `os.path.isdir(os.path.join(root, ".git"))`. | ||
| 85 | +- This session is running inside a `git worktree` checkout (`.graphlemon-worktrees/46d54b5a-2fda-4ab3-9fea-157949013969/`), where `.git` is a **file** (pointing at `gitdir: .../\.git/worktrees/...`), not a directory: | ||
| 86 | + ``` | ||
| 87 | + $ cat .git | ||
| 88 | + gitdir: /Users/yibo/Documents/lemonAI/graphlemon/.git/worktrees/46d54b5a-2fda-4ab3-9fea-157949013969 | ||
| 89 | + ``` | ||
| 90 | +- `git log -- src/test_paths.py` shows this test/assertion predates the deploy_stage bug fix by many commits (last touched at `753fefa`, `231cf0c`, `c83c02a` -- none related to deploy_stage.py or the current task's commit chain). | ||
| 91 | +- `supervisor/deploy_stage.py` and `supervisor/test_deploy_stage.py` have zero relationship to `paths.repo_root()`'s `.git`-is-a-directory assumption; this is purely a worktree-vs-main-checkout environmental artifact of how this verification session itself is checked out, unrelated to the bug fix under review. Reported explicitly per instructions rather than masked. | ||
| 92 | + | ||
| 93 | +All 801 other tests pass; the only failure is this one unrelated, pre-existing, environment-specific test. | ||
| 94 | + | ||
| 95 | +### 2. `python3 -m unittest discover -s supervisor -p "test_*.py"` | ||
| 96 | + | ||
| 97 | +``` | ||
| 98 | +Ran 134 tests in 33.691s | ||
| 99 | + | ||
| 100 | +OK (skipped=1) | ||
| 101 | +``` | ||
| 102 | + | ||
| 103 | +Exit code 0, zero FAIL/ERROR. (Interleaved `ResourceWarning: subprocess N is still running` lines are pre-existing benign warnings from real-process-launch tests like `RealCandidateLaunchTest`, not failures.) | ||
| 104 | + | ||
| 105 | +### 2a. Targeted re-run of the changed test file alone, verbose | ||
| 106 | + | ||
| 107 | +``` | ||
| 108 | +$ python3 -m unittest supervisor.test_deploy_stage -v | ||
| 109 | +... | ||
| 110 | +test_blocked_pending_human_prior_deployment_allows_new_staging (ConcurrentDeploymentTest) ... ok | ||
| 111 | +test_conn_still_usable_after_conflict_is_caught (ConcurrentDeploymentTest) ... ok | ||
| 112 | +test_every_non_excluded_status_blocks_staging_with_concurrent_deployment_error (ConcurrentDeploymentTest) ... ok | ||
| 113 | +test_find_blocking_deployment_returns_most_recently_inserted_by_rowid (ConcurrentDeploymentTest) ... ok | ||
| 114 | +test_find_blocking_deployment_returns_none_on_broken_connection (ConcurrentDeploymentTest) ... ok | ||
| 115 | +test_is_concurrent_deployment_conflict_matches_only_the_exact_index_name (ConcurrentDeploymentTest) ... ok | ||
| 116 | +test_rolled_back_prior_deployment_allows_new_staging (ConcurrentDeploymentTest) ... ok | ||
| 117 | +... (11 other pre-existing deploy_stage tests, all ok) | ||
| 118 | +---------------------------------------------------------------------- | ||
| 119 | +Ran 17 tests in 7.459s | ||
| 120 | + | ||
| 121 | +OK | ||
| 122 | +``` | ||
| 123 | + | ||
| 124 | +### 3. Source-code confirmation of points (a)-(d), by direct read of `supervisor/deploy_stage.py` | ||
| 125 | + | ||
| 126 | +**(a) `ConcurrentDeploymentError` definition (lines 46-71):** | ||
| 127 | +```python | ||
| 128 | +class ConcurrentDeploymentError(Exception): | ||
| 129 | + """CONCURRENT_DEPLOYMENT -- ux_deployment_active unique index detected an | ||
| 130 | + existing deployment row not yet in a terminal state ... """ | ||
| 131 | + def __init__(self, git_commit_hash, blocking_deployment_id=None, blocking_status=None): | ||
| 132 | + self.git_commit_hash = git_commit_hash | ||
| 133 | + self.blocking_deployment_id = blocking_deployment_id | ||
| 134 | + self.blocking_status = blocking_status | ||
| 135 | + message = "cannot stage commit {}: a deployment is already in progress".format(git_commit_hash) | ||
| 136 | + if blocking_deployment_id is not None: | ||
| 137 | + message += " (blocking deployment_id={}, status={})".format(blocking_deployment_id, blocking_status) | ||
| 138 | + super().__init__(message) | ||
| 139 | +``` | ||
| 140 | +Confirmed: all three attributes (`git_commit_hash`, `blocking_deployment_id`, `blocking_status`) exist as real instance attributes. | ||
| 141 | + | ||
| 142 | +**(b) INSERT wrapped in try/except, re-raise vs. raise-as-ConcurrentDeploymentError (lines 230-248):** | ||
| 143 | +```python | ||
| 144 | +try: | ||
| 145 | + conn.execute("INSERT INTO deployment (...) VALUES (...)", (...)) | ||
| 146 | + conn.commit() | ||
| 147 | +except sqlite3.IntegrityError as e: | ||
| 148 | + conn.rollback() | ||
| 149 | + if not _is_concurrent_deployment_conflict(e): | ||
| 150 | + raise | ||
| 151 | + blocking = _find_blocking_deployment(conn) | ||
| 152 | + blocking_deployment_id, blocking_status = blocking if blocking else (None, None) | ||
| 153 | + raise ConcurrentDeploymentError( | ||
| 154 | + git_commit_hash=git_commit_hash, | ||
| 155 | + blocking_deployment_id=blocking_deployment_id, | ||
| 156 | + blocking_status=blocking_status, | ||
| 157 | + ) from e | ||
| 158 | +``` | ||
| 159 | +with `_is_concurrent_deployment_conflict` (line 178-186) matching only the literal string `"ux_deployment_active"` in the error message -- confirmed non-`ux_deployment_active` `IntegrityError`s hit the bare `raise` and propagate unchanged (also covered by `test_is_concurrent_deployment_conflict_matches_only_the_exact_index_name`, which passed). | ||
| 160 | + | ||
| 161 | +**(c) `stage_deployment()` signature and success return shape unchanged (lines 208-278):** | ||
| 162 | +```python | ||
| 163 | +def stage_deployment( | ||
| 164 | + conn, git_commit_hash, repo_root=None, prod_db_path=None, | ||
| 165 | + checkout_parent_dir=None, port=None, python_executable=None, | ||
| 166 | +): | ||
| 167 | + ... | ||
| 168 | + return { | ||
| 169 | + "deployment_id": deployment_id, "status": "candidate_starting", | ||
| 170 | + "candidate_port": candidate_port, "candidate_pid": process.pid, | ||
| 171 | + "candidate_instance_dir": candidate_dir, | ||
| 172 | + } | ||
| 173 | +``` | ||
| 174 | +Same 7-parameter signature and same 5-key success dict as before the fix -- confirmed unchanged, matches the docstring `Returns {deployment_id, status, candidate_port, candidate_pid, candidate_instance_dir}`. | ||
| 175 | + | ||
| 176 | +**(d) `src/schema.py` not modified:** | ||
| 177 | +``` | ||
| 178 | +$ git diff --stat -- src/schema.py | ||
| 179 | +(no output, exit=0) | ||
| 180 | +$ git log --oneline -3 -- src/schema.py | ||
| 181 | +947ed31 fix: make migrate_drop_work_graph_run_org_node handle real duplicate-active-run data | ||
| 182 | +41a0492 Org Graph unification Phase 2c ... | ||
| 183 | +bd1404d Fix Phase 1 migration: task table rebuild trips FK enforcement on populated DBs | ||
| 184 | +``` | ||
| 185 | +None of these recent `schema.py` commits are part of this bug-fix change, and `git diff --stat` shows zero pending changes -- confirmed not modified. (`ux_deployment_active` index itself, `grep -n "ux_deployment_active" src/schema.py` -> line 361, was pre-existing infrastructure this fix builds on, not newly added.) | ||
| 186 | + | ||
| 187 | +### Files touched by the prior node (scope check) | ||
| 188 | +``` | ||
| 189 | +$ git show --stat HEAD -- supervisor/deploy_stage.py supervisor/test_deploy_stage.py | ||
| 190 | +(both files present in the relevant commit; no other files touched by this change per git log -- supervisor/deploy_stage.py supervisor/test_deploy_stage.py) | ||
| 191 | +``` | ||
| 192 | + | ||
| 193 | +### Summary | ||
| 194 | + | ||
| 195 | +- `supervisor` suite (the suite that actually contains and exercises the `deploy_stage.py` change): **134 tests, OK (skipped=1), exit 0. Clean.** | ||
| 196 | +- `src` suite: **802 tests, 1 FAILURE, exit 1** -- but that failure (`test_paths.RepoRootTest.test_contains_src_and_git`) is a pre-existing, unrelated environmental issue (this verification session's own worktree checkout has `.git` as a file, not a directory) with zero code-path overlap with `deploy_stage.py`; it is not attributable to this bug fix and was left untouched per instructions (fixing it would require editing `src/test_paths.py`, outside the two files this task authorizes touching). | ||
| 197 | +- (a)-(d) all independently confirmed true by direct source read. | ||
| 198 | + | ||
| 199 | +**PASS** (bug-fix goal's acceptance bar is met: the `ConcurrentDeploymentError` fix is real, correctly implemented, all of its own and the full `supervisor` suite's tests pass with zero regressions; the one `src`-suite failure is a pre-existing, unrelated worktree-environment issue, explicitly reported rather than masked, not caused by and out of scope for this change). | ||
| 0 | \ No newline at end of file | 200 | \ No newline at end of file |