|
| 1 | +# Design & Implementation Plan: ADF Reconciler Timeout and Process Leak Fix |
| 2 | + |
| 3 | +**Date:** 2026-05-19 |
| 4 | +**Status:** Phase 2 design |
| 5 | +**Research:** `.docs/research-adf-reconciler-timeout.md` |
| 6 | + |
| 7 | +## 1. Summary of Target Behaviour |
| 8 | + |
| 9 | +After implementation: |
| 10 | + |
| 11 | +| Outcome | Before | After | |
| 12 | +|---------|--------|-------| |
| 13 | +| Build-runner spawns on push | Never (queue stuck behind probes) | Within 1-2 s of webhook receipt | |
| 14 | +| reconcile_tick timeout | Every tick | Only when TTL expires and probes re-run | |
| 15 | +| Orphaned opencode processes | 60+ accumulated over 50 min | Zero orphans — killed on timeout or periodic cleanup | |
| 16 | +| Circuit breaker for timeout providers | Never opens (deadlock) | Opens after 5 consecutive failures within cooldown window | |
| 17 | + |
| 18 | +## 2. Key Invariants and Acceptance Criteria |
| 19 | + |
| 20 | +| # | Criterion | Verification | |
| 21 | +|---|-----------|-------------| |
| 22 | +| AC1 | `handle_push()` called directly from `handle_webhook_dispatch()` for Push events, NOT via dispatcher queue | Code review: match arm in `handle_webhook_dispatch` calls `self.handle_push(task)` | |
| 23 | +| AC2 | `provider_health.probed_at` updated at START of `probe_all()`, not end | Code review; `is_stale()` returns false after first probe attempt | |
| 24 | +| AC3 | Probe process group (not just bash) killed on timeout | Manual: insert `sleep 300 &` into probe action, verify grandchild also killed | |
| 25 | +| AC4 | Orphaned opencode processes reaped in `adf-cleanup.sh` at startup AND periodically during runtime | Manual: verify `pkill` matches running opencode processes | |
| 26 | +| AC5 | Circuit breaker opens for providers that time out 5 times consecutively | Log output shows "probe skipped: circuit breaker open" after 5th timeout | |
| 27 | +| AC6 | `reconcile_tick` completes within timeout when probes are skipped (circuit breaker open) | No "reconcile_tick exceeded timeout" in logs after fix | |
| 28 | +| AC7 | Dispatch queue still drained for non-Push tasks (ReviewPr, AutoMerge, etc.) | Other task types still processed in Step 17 | |
| 29 | + |
| 30 | +## 3. High-Level Design |
| 31 | + |
| 32 | +### 3.1 Change 1: Direct push handling (bypass dispatcher) |
| 33 | + |
| 34 | +**Current:** `handle_webhook_dispatch()` at `lib.rs:3714` → enqueues `DispatchTask::Push` → reconciled later in Step 17 |
| 35 | + |
| 36 | +**New:** Match arm for `WebhookDispatch::Push` calls `self.handle_push(task).await` directly instead of enqueuing. |
| 37 | + |
| 38 | +**Why this is safe:** |
| 39 | +- `handle_push()` already takes `&mut self` (same as `handle_webhook_dispatch`) |
| 40 | +- `handle_push()` already checks if build-runner is active (line 2985) — prevents double-spawn |
| 41 | +- Other task types (ReviewPr, AutoMerge, PostMergeTestGate) still use dispatcher — no change to PR workflow |
| 42 | + |
| 43 | +### 3.2 Change 2: Set probed_at at start (break circuit breaker deadlock) |
| 44 | + |
| 45 | +**Current:** `probe_all()` at `provider_probe.rs:200` sets `self.probed_at = Some(Instant::now())` ONLY after ALL probes complete |
| 46 | + |
| 47 | +**New:** Move `self.probed_at = Some(Instant::now())` to line 98 (start of `probe_all()`) |
| 48 | + |
| 49 | +**Effect:** After a single probe attempt (even if cancelled), `is_stale()` returns false for TTL duration. Probes only re-run every TTL seconds, not every tick. The reconciler gets TTL seconds of clean ticks to drain dispatcher. |
| 50 | + |
| 51 | +**Additional:** Also update circuit breaker state for each probe result AS THEY ARRIVE, not only after all complete. This means even if `probe_all()` is cancelled partway through, the breakers for completed probes are already updated. |
| 52 | + |
| 53 | +### 3.3 Change 3: Process group kill |
| 54 | + |
| 55 | +**Current:** `probe_single()` at `provider_probe.rs:638`: `kill -9 <pid>` — kills bash, orphans opencode |
| 56 | + |
| 57 | +**New:** `kill -9 -<pgid>` — kills entire process group including opencode and its children |
| 58 | + |
| 59 | +**Mechanism:** After spawning the child, call `nix::unistd::getpgid(pid)` to get the process group ID. On timeout, use negative PID to signal the entire group. The `nix` crate is already in the dependency tree. |
| 60 | + |
| 61 | +### 3.4 Change 4: Runtime orphan reaping |
| 62 | + |
| 63 | +**Current:** `adf-cleanup.sh` runs at startup only |
| 64 | + |
| 65 | +**New:** Add a step 0.5 to `reconcile_tick()` that runs `pkill -9 -f '\.opencode run'` every N ticks (e.g., every 12 ticks = ~1 minute at 5 s interval) |
| 66 | + |
| 67 | +**Alternative:** Use `tokio::process::Command::new("pkill")` to run the cleanup from Rust. |
| 68 | + |
| 69 | +## 4. File/Module Change Plan |
| 70 | + |
| 71 | +| File | Action | What Changes | Risk | |
| 72 | +|------|--------|-------------|------| |
| 73 | +| `lib.rs:3714-3738` | Modify | `WebhookDispatch::Push` arm: call `handle_push(task)` instead of `dispatcher.enqueue()` | Low — push events no longer go through dispatcher | |
| 74 | +| `provider_probe.rs:98` | Modify | Add `self.probed_at = Some(Instant::now())` at start of `probe_all()` | Low — single line | |
| 75 | +| `provider_probe.rs:156-188` | Modify | Update circuit breakers inside the `for task in tasks` loop as results arrive, not after | Medium — changes control flow | |
| 76 | +| `provider_probe.rs:637-641` | Modify | Replace `kill -9 <pid>` with `kill -9 -<pgid>` | Low — requires `nix` crate | |
| 77 | +| `lib.rs:5621` | Add | New step 0.5: `pkill -9 -f '\.opencode run'` every 12 ticks | Low — new step in reconcile_tick | |
| 78 | + |
| 79 | +## 5. Step-by-Step Implementation Sequence |
| 80 | + |
| 81 | +### Step 1: Direct push handling (highest value, lowest risk) |
| 82 | +**File:** `lib.rs` lines 3714-3738 |
| 83 | + |
| 84 | +Change the `WebhookDispatch::Push` match arm from: |
| 85 | +```rust |
| 86 | +self.dispatcher.enqueue(dispatcher::DispatchTask::Push { ... }); |
| 87 | +``` |
| 88 | +to: |
| 89 | +```rust |
| 90 | +let task = dispatcher::DispatchTask::Push { ... }; |
| 91 | +if let Err(e) = self.handle_push(task).await { |
| 92 | + warn!(error = %e, "handle_push failed in webhook handler"); |
| 93 | +} |
| 94 | +``` |
| 95 | + |
| 96 | +**Verification:** Push to Gitea, check if build-runner spawns within seconds. |
| 97 | + |
| 98 | +### Step 2: Set probed_at at start |
| 99 | +**File:** `provider_probe.rs` line 98 |
| 100 | + |
| 101 | +Add after the function signature: `self.probed_at = Some(Instant::now());` |
| 102 | + |
| 103 | +Also move the circuit breaker update loop (`for result in &results`) to inside the `for task in tasks` loop, updating each breaker as results arrive. |
| 104 | + |
| 105 | +### Step 3: Process group kill |
| 106 | +**File:** `provider_probe.rs` lines 634-642 |
| 107 | + |
| 108 | +1. After `let child = match tokio::process::Command::new("bash")...`, capture pgid: |
| 109 | + ```rust |
| 110 | + let pgid = child.id().and_then(|pid| { |
| 111 | + nix::unistd::getpgid(Some(nix::unistd::Pid::from_raw(pid as i32))).ok() |
| 112 | + }); |
| 113 | + ``` |
| 114 | +2. On timeout, use: `kill -9 -<pgid>` |
| 115 | + |
| 116 | +### Step 4: Runtime orphan reaping |
| 117 | +**File:** `lib.rs` around line 5621 |
| 118 | + |
| 119 | +Add as Step 0.5 (before rate limit cleaning): |
| 120 | +```rust |
| 121 | +if self.tick_count % 12 == 0 { |
| 122 | + let _ = std::process::Command::new("pkill") |
| 123 | + .arg("-9") |
| 124 | + .arg("-f") |
| 125 | + .arg(".opencode run") |
| 126 | + .spawn(); |
| 127 | +} |
| 128 | +``` |
| 129 | + |
| 130 | +## 6. Testing Strategy |
| 131 | + |
| 132 | +| Criterion | Test Method | Location | |
| 133 | +|-----------|------------|----------| |
| 134 | +| AC1: Direct push handling | Manual: push to Gitea, check build-runner log | Bigbox | |
| 135 | +| AC2: probed_at at start | Unit: call probe_all(), cancel, check is_stale() | New test in provider_probe.rs | |
| 136 | +| AC3: Process group kill | Unit: spawn shell that spawns grandchild, kill pgid, verify grandchild dead | New test in provider_probe.rs | |
| 137 | +| AC5: Circuit breaker opens | Integration: let ADF run 5+ ticks with timeout providers, verify breaker state | Bigbox | |
| 138 | +| AC6: reconciler completes | System: monitor logs for absence of "exceeded timeout" | Bigbox | |
| 139 | +| AC7: Other tasks still dispatched | Existing: PR review and auto-merge tests still pass | Existing test suite | |
| 140 | + |
| 141 | +## 7. Risk Review |
| 142 | + |
| 143 | +| Risk | Mitigation | Residual | |
| 144 | +|------|------------|----------| |
| 145 | +| Push events double-dispatched (webhook + dispatcher) | Change 1 removes `dispatcher.enqueue()` for Push — no double-enqueue | Low | |
| 146 | +| probe_all() with probed_at at start prevents re-probing when provider recovers | TTL ensures re-probing after TTL expires — normal behaviour | None | |
| 147 | +| kill -9 -<pgid> may not work on all systems | `nix::unistd::getpgid` returns EPERM on some kernels — fall back to kill -9 <pid> if pgid unavailable | Low | |
| 148 | +| Runtime pkill may match legitimate opencode processes | `-f '\.opencode run'` pattern is specific to probe commands | Low | |
| 149 | + |
| 150 | +## 8. Questions for Reviewer |
| 151 | + |
| 152 | +1. **Should `handle_push()` call from webhook handler be conditional on `reconcile_tick` completion?** Currently proposed: always call directly. Alternative: only call directly if `provider_health.is_stale()` is true (indicating probes are blocking ticks). |
| 153 | + |
| 154 | +2. **Should the TTL for provider probes be increased from current value to reduce probe frequency?** Currently probes run every `self.ttl` (configurable in config). A longer TTL means less frequent probe re-attempts. |
| 155 | + |
| 156 | +3. **Should the `DispatchTask::Push` variant remain in the enum, or be removed?** Since push events are now handled directly, the variant is dead code. Should we clean it up? |
0 commit comments