fix(approval-gate): stop duplicate hook bindings deadlocking approvals

approval-gate/src/configuration.rs

@@ -118,7 +118,8 @@ pub async fn apply_config(cell: &ConfigCell, cfg: WorkerConfig) {
 }
 
 /// Bind the fixed `harness::hook::pre-trigger` hook at worker startup.
-pub fn bind_hook(iii: &IIIClient) {
+/// Returns `true` when the engine accepted the registration.
+pub fn bind_hook(iii: &IIIClient) -> bool {
     match iii.register_trigger(RegisterTriggerInput {
         trigger_type: "harness::hook::pre-trigger".to_string(),
         function_id: "approval::gate".to_string(),
@@ -129,25 +130,32 @@ pub fn bind_hook(iii: &IIIClient) {
         }),
         metadata: None,
     }) {
-        Ok(_) => tracing::info!(
-            trigger_type = "harness::hook::pre-trigger",
-            function_id = "approval::gate",
-            "trigger binding requested"
-        ),
-        Err(e) => tracing::warn!(
-            trigger_type = "harness::hook::pre-trigger",
-            function_id = "approval::gate",
-            error = %e,
-            "trigger binding failed (sibling absent?)"
-        ),
+        Ok(_) => {
+            tracing::info!(
+                trigger_type = "harness::hook::pre-trigger",
+                function_id = "approval::gate",
+                "trigger binding requested"
+            );
+            true
+        }
+        Err(e) => {
+            tracing::warn!(
+                trigger_type = "harness::hook::pre-trigger",
+                function_id = "approval::gate",
+                error = %e,
+                "trigger binding failed (sibling absent?)"
+            );
+            false
+        }
     }
 }
 
 /// Bind the fixed `harness::hook::post-trigger` hook for
 /// `approval::filesystem-access-watch` at worker startup — beside `bind_hook`, same
 /// best-effort discipline (a standalone deployment without the harness
 /// still boots; a missing binding surfaces as a log, never an `Err` here).
-pub fn bind_filesystem_access_watch_hook(iii: &IIIClient) {
+/// Returns `true` when the engine accepted the registration.
+pub fn bind_filesystem_access_watch_hook(iii: &IIIClient) -> bool {
     match iii.register_trigger(RegisterTriggerInput {
         trigger_type: "harness::hook::post-trigger".to_string(),
         function_id: "approval::filesystem-access-watch".to_string(),
@@ -158,41 +166,61 @@ pub fn bind_filesystem_access_watch_hook(iii: &IIIClient) {
         }),
         metadata: None,
     }) {
-        Ok(_) => tracing::info!(
-            trigger_type = "harness::hook::post-trigger",
-            function_id = "approval::filesystem-access-watch",
-            "trigger binding requested"
-        ),
-        Err(e) => tracing::warn!(
-            trigger_type = "harness::hook::post-trigger",
-            function_id = "approval::filesystem-access-watch",
-            error = %e,
-            "trigger binding failed (sibling absent?)"
-        ),
+        Ok(_) => {
+            tracing::info!(
+                trigger_type = "harness::hook::post-trigger",
+                function_id = "approval::filesystem-access-watch",
+                "trigger binding requested"
+            );
+            true
+        }
+        Err(e) => {
+            tracing::warn!(
+                trigger_type = "harness::hook::post-trigger",
+                function_id = "approval::filesystem-access-watch",
+                error = %e,
+                "trigger binding failed (sibling absent?)"
+            );
+            false
+        }
     }
 }
 
 /// Retry hook bindings until the harness has registered the hook trigger
 /// types. Approval-gate may start before harness; a one-shot registration in
 /// that order fails asynchronously and silently leaves the gate detached.
+///
+/// Each hook is registered AT MOST ONCE per startup (on the first successful
+/// attempt): the engine's instance count can lag a successful registration,
+/// and re-binding on a lagging count stacks duplicate hook instances — the
+/// harness would then run the gate N times per call, re-holding on release
+/// (approval deadlock). A failed attempt (harness not up yet) leaves the
+/// flag unset so the next iteration retries; a success is never repeated.
+///
+/// Readiness gates only the completion condition, never the attempt: the
+/// harness being active says nothing about whether THIS worker bound its
+/// hook, so a gate that starts after the harness must still register —
+/// otherwise the gate runs detached.
 pub fn retry_hook_bindings(iii: IIIClient) {
     tokio::spawn(async move {
+        let mut pre_bound = false;
+        let mut post_bound = false;
         loop {
+            if !pre_bound {
+                pre_bound = bind_hook(&iii);
+            }
+            if !post_bound {
+                post_bound = bind_filesystem_access_watch_hook(&iii);
+            }
+
             let pre_trigger_ready = trigger_instance_count(&iii, "harness::hook::pre-trigger")
                 .await
                 .is_some_and(|count| count > 0);
-            if !pre_trigger_ready {
-                bind_hook(&iii);
-            }
-
             let post_trigger_ready = trigger_instance_count(&iii, "harness::hook::post-trigger")
                 .await
                 .is_some_and(|count| count > 0);
-            if !post_trigger_ready {
-                bind_filesystem_access_watch_hook(&iii);
-            }
 
-            if pre_trigger_ready && post_trigger_ready {
+            if pre_bound && post_bound && pre_trigger_ready && post_trigger_ready {
                 tracing::info!("approval-gate hook bindings confirmed");
                 break;
             }

approval-gate/src/main.rs

@@ -166,8 +166,11 @@ async fn main() -> Result<()> {
 
     functions::register_all(&iii, &deps);
 
-    configuration::bind_hook(&iii);
-    configuration::bind_filesystem_access_watch_hook(&iii);
+    // Hook bindings go through `retry_hook_bindings` alone — it registers
+    // each hook at most once (the engine's instance count can lag a
+    // successful registration; a direct bind here plus the loop's first
+    // iteration would stack duplicate gate instances, which re-hold on
+    // release and deadlock approval).
     configuration::retry_hook_bindings(iii.as_ref().clone());
 
     // These two carry no config and are never re-bound — best-effort only.

approval-gate/tests/hook_binding_retry.rs

@@ -0,0 +1,170 @@
+//! `retry_hook_bindings` registration semantics on a live engine: each hook
+//! binds at most once per startup, attempts continue until success, and
+//! harness readiness gates only the completion condition — a gate that starts
+//! after the harness still binds (the readiness signal says the harness is
+//! active, not that THIS worker registered its hook).
+//!
+//! Self-skips when no `iii` engine binary is on PATH or `III_ENGINE_BIN`.
+
+use std::time::{Duration, Instant};
+
+use approval_gate::configuration::{
+    bind_filesystem_access_watch_hook, bind_hook, retry_hook_bindings,
+};
+use approval_gate::testkit::{engine_bin, spawn_engine};
+use iii_sdk::errors::Error;
+use iii_sdk::protocol::TriggerRequest;
+use iii_sdk::trigger::{TriggerConfig, TriggerHandler};
+use iii_sdk::{register_worker, IIIClient, InitOptions, RegisterTriggerType};
+use serde_json::json;
+
+/// No-op handler standing in for the harness's hook trigger types — a test
+/// registers these to simulate "the harness is active".
+struct NullHandler;
+
+#[async_trait::async_trait]
+impl TriggerHandler for NullHandler {
+    async fn register_trigger(&self, _config: TriggerConfig) -> Result<(), Error> {
+        Ok(())
+    }
+    async fn unregister_trigger(&self, _config: TriggerConfig) -> Result<(), Error> {
+        Ok(())
+    }
+}
+
+fn register_harness_hook_types(iii: &IIIClient) {
+    for hook_type in ["harness::hook::pre-trigger", "harness::hook::post-trigger"] {
+        let _ = iii.register_trigger_type(RegisterTriggerType::new(
+            hook_type,
+            "test double for the harness hook trigger types",
+            NullHandler,
+        ));
+    }
+}
+
+/// Engine-side instance counts for the two hook types (0 on any error).
+async fn hook_instance_counts(iii: &IIIClient) -> (u64, u64) {
+    let mut counts = (0, 0);
+    for (index, hook_type) in ["harness::hook::pre-trigger", "harness::hook::post-trigger"]
+        .iter()
+        .enumerate()
+    {
+        if let Ok(response) = iii
+            .trigger(TriggerRequest {
+                function_id: "engine::triggers::info".to_string(),
+                payload: json!({ "id": hook_type }),
+                action: None,
+                timeout_ms: None,
+            })
+            .await
+        {
+            if let Some(count) = response.get("instance_count").and_then(serde_json::Value::as_u64)
+            {
+                if index == 0 {
+                    counts.0 = count;
+                } else {
+                    counts.1 = count;
+                }
+            }
+        }
+    }
+    counts
+}
+
+/// Poll until both hook types report the expected instance count.
+async fn wait_for_counts(iii: &IIIClient, expected: (u64, u64)) -> bool {
+    let deadline = Instant::now() + Duration::from_secs(15);
+    loop {
+        if hook_instance_counts(iii).await == expected {
+            return true;
+        }
+        if Instant::now() > deadline {
+            return false;
+        }
+        tokio::time::sleep(Duration::from_millis(100)).await;
+    }
+}
+
+/// The gate starts AFTER a previous gate left instances behind: the hook
+/// types exist and `engine::triggers::info` already reports count > 0 on the
+/// first iteration. Readiness must not suppress this worker's own bind — the
+/// old loop broke immediately on the foreign count, leaving the gate
+/// detached. And each hook registers exactly once (no re-binding storm once
+/// bound).
+#[tokio::test(flavor = "multi_thread")]
+async fn leftover_instances_do_not_suppress_this_workers_own_registration() {
+    if engine_bin().is_none() {
+        eprintln!("skipping: no iii engine");
+        return;
+    }
+    let Some(engine) = spawn_engine().await else {
+        eprintln!("skipping: failed to spawn engine");
+        return;
+    };
+    let iii = register_worker(&engine.url, InitOptions::default());
+    register_harness_hook_types(&iii);
+
+    // Simulate a previous gate's instances that the engine has not yet
+    // garbage-collected: both types already report count 1.
+    assert!(bind_hook(&iii), "seed pre-trigger instance");
+    assert!(
+        bind_filesystem_access_watch_hook(&iii),
+        "seed post-trigger instance"
+    );
+    assert_eq!(hook_instance_counts(&iii).await, (1, 1), "seeded");
+
+    retry_hook_bindings(iii.clone());
+
+    // The loop must register its OWN instances on top of the leftovers
+    // (count 2), not conclude "ready" from the foreign count and skip.
+    assert!(
+        wait_for_counts(&iii, (2, 2)).await,
+        "this worker must bind its own hooks even though the harness reports ready"
+    );
+    // Once bound, the loop must not re-register: counts stay stable across
+    // several retry intervals instead of stacking duplicates.
+    tokio::time::sleep(Duration::from_millis(1_300)).await;
+    assert_eq!(
+        hook_instance_counts(&iii).await,
+        (2, 2),
+        "bindings must not be re-registered after success"
+    );
+}
+
+/// The gate starts BEFORE the harness: binds fail while the hook types are
+/// absent, and are retried once the harness comes up. The gate must not give
+/// up after the first failure.
+#[tokio::test(flavor = "multi_thread")]
+async fn failed_registration_is_retried_until_the_harness_is_ready() {
+    if engine_bin().is_none() {
+        eprintln!("skipping: no iii engine");
+        return;
+    }
+    let Some(engine) = spawn_engine().await else {
+        eprintln!("skipping: failed to spawn engine");
+        return;
+    };
+    let iii = register_worker(&engine.url, InitOptions::default());
+
+    // Hook types absent: every bind attempt fails.
+    retry_hook_bindings(iii.clone());
+    tokio::time::sleep(Duration::from_millis(1_300)).await;
+    assert_eq!(
+        hook_instance_counts(&iii).await,
+        (0, 0),
+        "no binding may register while the harness hook types are absent"
+    );
+
+    // The harness comes up; the retry loop must pick the binds up.
+    register_harness_hook_types(&iii);
+    assert!(
+        wait_for_counts(&iii, (1, 1)).await,
+        "failed registrations must be retried once the harness is ready"
+    );
+    tokio::time::sleep(Duration::from_millis(1_300)).await;
+    assert_eq!(
+        hook_instance_counts(&iii).await,
+        (1, 1),
+        "bindings must not be re-registered after success"
+    );
+}