fix(recovery): address PR #703 review findings F1-F6 (#702)

Fixes the six defects from the formal REQUEST_CHANGES review at 889931d553: - F1: run sanctioned stale-binding recovery in gitea_resolve_task_capability BEFORE the terminal launcher probe, so a worktree removed mid-session can no longer wedge mutation resolution on 'missing cwd' before recovery runs. The fail-closed probe block now also carries the binding classification evidence. - F2: _resolve_preflight_workspace_path resolves with the same verify_paths existence checks as the canonical mutation context, and _get_workspace_porcelain returns a synthetic tracked-dirty sentinel when the workspace is missing or git fails — an uninspectable workspace can never read as clean/empty porcelain. - F3: the orphaned_expired_superseded_head cleanup matrix now requires affirmative safe worktree evidence (worktree_clean=true or worktree_exists=false), aligned with the sibling orphaned_owner_missing gate and the diagnosis path; unknown evidence fails closed. - F4: reviewer-session-lease shadows and stale-binding audit records are recovery-critical kinds exempt from the 4h session-state TTL; they persist until a sanctioned clear terminally reconciles them. - F5: session-lease shadows are keyed by lease session id (collision-safe identity) with a list_states enumeration API; concurrent same-profile sessions can no longer overwrite or misattribute each other's crash evidence. Legacy single-slot records remain readable. - F6: the durable audit record is persisted (status=pending) BEFORE the environment binding is cleared; if audit persistence fails the clear does not happen and the failure is reported explicitly. 30 new regression tests cover deleted-worktree recovery ordering, missing/ deleted/symlinked/mid-evaluation path changes, unknown-evidence cleanup, TTL boundary (before/at/after), interleaved and reconnected concurrent sessions, and audit-write failure/retry/idempotence/ordering. Issue #704 dotenv load-path prevention is intentionally NOT included. Full suite: 2695 passed, 6 skipped, 161 subtests passed. Co-Authored-By: Claude Fable 5 <[email protected]>
2026-07-13 16:40:58 -04:00
parent 889931d553
commit c2fc2683b9
4 changed files with 947 additions and 44 deletions
@@ -46,6 +46,16 @@ KIND_REVIEWER_SESSION_LEASE = "reviewer_session_lease"
 # recovery clears an inherited GITEA_ACTIVE_WORKTREE binding (#702).
 KIND_STALE_BINDING_RECOVERY = "stale_binding_recovery"

+# Kinds that carry recovery-critical crash-orphan evidence (#702). They are
+# exempt from TTL expiry: a crashed daemon can never refresh its own record,
+# so a wall-clock TTL would silently destroy the only owner-exit proof the
+# guarded cleanup path accepts. These records live until a sanctioned clear
+# terminally reconciles them (durable lifecycle), never until a timer fires.
+RECOVERY_CRITICAL_KINDS = frozenset({
+    KIND_REVIEWER_SESSION_LEASE,
+    KIND_STALE_BINDING_RECOVERY,
+})
+


 _SAFE_SEGMENT_RE = re.compile(r"[^A-Za-z0-9._+-]+")
@@ -95,6 +105,7 @@ def state_key(
    org: str | None = None,
    repo: str | None = None,
    profile_identity: str | None = None,
+    instance_id: str | None = None,
 ) -> str:
    """Build durable filename key.

@@ -102,17 +113,20 @@ def state_key(
    so the primary key is kind + profile identity. Remote/org/repo are stored
    inside the payload and validated on load (#559), which lets a later daemon
    process recover state without already knowing the remote argument.
+
+    *instance_id* extends the key for kinds where one-per-profile collides —
+    concurrent same-profile sessions each own their reviewer-lease shadow
+    (#702 F5), keyed by their lease session id so a later heartbeat can never
+    overwrite or misattribute another session's crash evidence.
    """
    # Keep remote/org/repo parameters for API stability / future kinds; they are
    # intentionally not part of the filename for session-scoped proofs.
    _ = (remote, org, repo)
-    return "-".join(
-        _sanitize_segment(part)
-        for part in (
-            kind,
-            profile_identity or "unknown-profile",
-        )
-    )
+    parts = [kind, profile_identity or "unknown-profile"]
+    instance = (instance_id or "").strip()
+    if instance:
+        parts.append(instance)
+    return "-".join(_sanitize_segment(part) for part in parts)


 def state_file_path(
@@ -123,6 +137,7 @@ def state_file_path(
    repo: str | None = None,
    profile_identity: str | None = None,
    state_dir: str | None = None,
+    instance_id: str | None = None,
 ) -> str:
    root = (state_dir or default_state_dir()).strip()
    name = state_key(
@@ -131,6 +146,7 @@ def state_file_path(
        org=org,
        repo=repo,
        profile_identity=profile_identity,
+        instance_id=instance_id,
    )
    return os.path.join(root, f"{name}.json")

@@ -251,10 +267,12 @@ def identity_match_reasons(
        reasons.append("session state missing recorded_at timestamp (fail closed)")
    else:
        age = _now_utc() - recorded_at
+        record_kind = (str(record.get("kind") or "")).strip()
        if age > timedelta(hours=ttl_hours()):
-            reasons.append(
-                f"session state expired after {ttl_hours():g}h (fail closed)"
-            )
+            if record_kind not in RECOVERY_CRITICAL_KINDS:
+                reasons.append(
+                    f"session state expired after {ttl_hours():g}h (fail closed)"
+                )
        if age < timedelta(0):
            reasons.append("session state recorded_at is in the future (fail closed)")
    return reasons
@@ -268,6 +286,7 @@ def load_state(
    repo: str | None = None,
    profile_identity: str | None = None,
    state_dir: str | None = None,
+    instance_id: str | None = None,
 ) -> dict[str, Any] | None:
    """Load durable state payload when identity checks pass."""
    profile = current_profile_identity(profile_identity=profile_identity)
@@ -278,6 +297,7 @@ def load_state(
        repo=repo,
        profile_identity=profile,
        state_dir=state_dir,
+        instance_id=instance_id,
    )
    lock_path = f"{path}.lock"
    with _exclusive_file_lock(lock_path):
@@ -323,6 +343,7 @@ def save_state(
    repo: str | None = None,
    profile_identity: str | None = None,
    state_dir: str | None = None,
+    instance_id: str | None = None,
 ) -> dict[str, Any] | None:
    """Persist or clear durable state for the given session identity key."""
    profile = current_profile_identity(
@@ -344,6 +365,7 @@ def save_state(
        repo=key_repo,
        profile_identity=profile,
        state_dir=root,
+        instance_id=instance_id,
    )
    lock_path = f"{path}.lock"
    with _exclusive_file_lock(lock_path):
@@ -365,6 +387,8 @@ def save_state(
            body["session_profile_lock"] = profile
        body["recorded_at"] = body.get("recorded_at") or now
        body["updated_at"] = now
+        if (instance_id or "").strip():
+            body["instance_id"] = instance_id.strip()
        if key_remote is not None:
            body["remote"] = key_remote
        if key_org is not None:
@@ -396,6 +420,7 @@ def clear_state(
    repo: str | None = None,
    profile_identity: str | None = None,
    state_dir: str | None = None,
+    instance_id: str | None = None,
 ) -> None:
    save_state(
        kind=kind,
@@ -405,4 +430,56 @@ def clear_state(
        repo=repo,
        profile_identity=profile_identity,
        state_dir=state_dir,
+        instance_id=instance_id,
    )
+
+
+def list_states(
+    *,
+    kind: str,
+    profile_identity: str | None = None,
+    state_dir: str | None = None,
+) -> list[dict[str, Any]]:
+    """Enumerate durable records of *kind* across all instance keys.
+
+    Crash recovery cannot know which session id left a shadow behind, so it
+    must be able to enumerate every record of a kind (#702 F5). Identity
+    checks (profile / TTL policy) apply per record exactly as in
+    :func:`load_state`; records that fail closed are omitted.
+    """
+    root = (state_dir or default_state_dir()).strip()
+    if not root or not os.path.isdir(root):
+        return []
+    profile = current_profile_identity(profile_identity=profile_identity)
+    results: list[dict[str, Any]] = []
+    try:
+        names = sorted(os.listdir(root))
+    except OSError:
+        return []
+    for name in names:
+        if not name.endswith(".json") or name.startswith("."):
+            continue
+        envelope = _read_json(os.path.join(root, name))
+        if not envelope or envelope.get("kind") != kind:
+            continue
+        payload = envelope.get("payload")
+        if not isinstance(payload, dict):
+            continue
+        merged = dict(payload)
+        for key in (
+            "kind",
+            "remote",
+            "org",
+            "repo",
+            "profile_identity",
+            "session_profile_lock",
+            "recorded_at",
+            "updated_at",
+            "writer_pid",
+        ):
+            if key in envelope and key not in merged:
+                merged[key] = envelope[key]
+        if identity_match_reasons(merged, profile_identity=profile):
+            continue
+        results.append(merged)
+    return results