diff --git a/REVIEW-conc.md b/REVIEW-conc.md index 64e652c..27c1c3f 100644 --- a/REVIEW-conc.md +++ b/REVIEW-conc.md @@ -126,3 +126,71 @@ Net: the entire P1–P4 diff has been pre-audited and is clean against my break- run, once claimed (after tests/concurrency + P5 land), reduces to: fresh checkout on cc-ci → `cc-ci-run -m pytest tests/unit -q` + `cc-ci-run -m pytest tests/concurrency -q` + lint, plus a focused review of only the tests/concurrency suite (vs the 19 planned cases) and the P5 doc delta. + +## M1: PASS @2026-06-10T04:38Z — implementation verified (branch restructure/concurrency @d3fe9e2) + +Verdict formed from the plan (SSOT), the code/git, the STATUS claim's verify recipe, and my own +COLD acceptance run — WITHOUT reading JOURNAL first (anti-anchoring honored; noting here that I had +NOT consulted JOURNAL-conc at verdict time). + +COLD ENVIRONMENT: fresh `git clone --branch restructure/concurrency` into /tmp/adv-m1 on cc-ci +(NOT the Builder's tree); `git rev-parse HEAD == d3fe9e26bb0fbaedb37383539ba3973bc1c80aff` (matches +claim), `git status` clean. Ran via the host `cc-ci-run` pyEnv (pytest 8.3.3 + playwright) and the +pinned `.#lint` devshell. + +ACCEPTANCE RESULTS (expected → observed): +- `cc-ci-run -m pytest tests/unit -q` → 138 passed in 4.72s ✓ (claim: 138 passed) +- `cc-ci-run -m pytest tests/concurrency -q` → 20 passed in 9.91s ✓ (claim: 20 passed) +- `nix develop .#lint --command bash scripts/lint.sh` → `lint: PASS` ✓ +- `pytest tests/unit --collect-only` concurrency items → 0 ✓ (suite NOT in default gate) +- dangling-ref grep (register_run_app, unregister_run_app, _run_owner_state, ACTIVE_RUN_DIR, + CCCI_JANITOR_MAX_AGE, acquire_recipe_lock, RECIPE_LOCK_DIR, _stack_age_seconds) over + *.py/*.nix/*.yml/*.sh → ZERO hits outside docs/ ✓ + +GATE-INTEGRITY (guardrails honored): +- `RUN_APP_RE` regex unchanged (lifecycle.py:26, identical pattern); warm/canonical apps still + never become probe candidates (test_11 asserts no lockfiles even created for warm names). +- `services_converged()` / paused-is-settled / `backup_app()` waits: NOT in the code diff — all + RUN_APP_RE/services_converged/paused diff hits are docs/concurrency.md prose (P5 rewrite). +- `teardown_app` ordering untouched; only its trailing unregister call removed (registry gone). +- Only `tests//` change is the mechanical `RECIPE_DIR=${ABRA_DIR:-$HOME/.abra}/...` line + in ghost+discourse install_steps.sh — NO assertion/gate touched (diff-confirmed). Guardrail + "never weaken recipe-test gates / touch tests// content" honored. +- P4: `concurrency.limit` block removed from .drone.yml; drone-runner.nix comment makes + DRONE_RUNNER_CAPACITY the single knob. + +ADVERSARIAL DIFF REVIEW (P1–P4 pre-audited in the two notes above; refuted: green-on-red exit-code +masking [empirically tested], unlink/reacquire inode race [fstat==stat identity recheck], +half-reaped coexistence [reap-under-probe-lock], signal-mid-teardown reentrancy [begin_teardown +first line of both finally blocks], guard/ABRA_DIR/fetch ordering [no abra call pre-export]). + +TEST-SUITE AUDIT vs the 19 plan cases: real kernel flocks, NEVER mocked (only teardown_app + +abra-discovery stubbed, both disclosed). Coverage complete: cases 1–4 test_locks, 5–12 +test_janitor, 13–16 test_lifetime, 17–19 test_abra_dir, +test_18b (manual-pid isolation) = 20. +Assertions are substantive, not tautological: exact funnel exit codes 142/143 (test_15/16), +reap-vs-new-run timestamp ordering + fresh-inode `lock_state=="held"` (test_7), two-janitor +arbitration via separate open()s (test_8 — valid: flock binds the open file description, so +threads-with-distinct-fds model processes), long-held mtime-backdate flag-not-steal (test_10), +PEP 446 fd non-inheritance with a surviving child (test_3), divergent per-run trees + canonical +untouched (test_18). + +INDEPENDENT PROBE (my own driver, NOT the Builder's helpers.py): drove the real +`lifecycle.acquire_app_lock` from a standalone script with a sandbox CCCI_APP_LOCK_DIR on cc-ci → +state `held` after acquire; a second acquirer BLOCKED while the first held (no ack2 after 1.5s); +after `SIGKILL` of the holder the second acquired within 10s (kernel auto-release). Core invariant +confirmed against the real code, not just the Builder's tests. + +NON-BLOCKING NOTES (carry to M2 live-verify; none gate M1): +- setsid-fork edge in the .drone.yml trap wrapper: if `setsid` ever forks (only when it's a pgrp + leader — not the case for a backgrounded job in a non-job-control drone shell), $PID would be the + intermediate and the harness could reparent (ppid==1) and self-abort. MUST be live-verified by + the actual drone-cancel path at M2(a) — the plan already flags this ("verify drone exec runner + signal delivery; the trap must fire on drone cancel"). Not unit-testable here. +- End-of-janitor stale-lockfile tidy sweep (appless leftover lockfile unlink) is not directly + covered by a named test (not one of the 19); low risk (tidiness only). Noted, not a defect. +- test_14 (ppid race) depends on the helper reparenting to pid 1; under a subreaper it marks + NEVER_REPARENTED and FAILS VISIBLY (never false-passes). Passed in this env. + +CONCLUSION: M1 — implementation verified — PASS. M2 (merge to main + live verification a–d) is +unblocked. Reminder for both loops: recipe-mirror PRs are !testme targets only — never merge/push +them. (After this verdict I may consult JOURNAL-conc to contextualize, per §6.1.)