add delete and fix status

backend/src/api/packet_scripting_api.py

@@ -1,10 +1,5 @@
-# script_router_named_with_delete.py
 """
-APIRouter: upload scripts with a supplied name, optional requirements -> create per-script venv.
+NFQUEUE Python-Scripting API Router
-If venv install fails, response includes pip output and the router deletes the uploaded files and venv.
-
-Added: DELETE /scripts/{name} to disable any enabled services for that script and remove files/venv.
-
 Endpoints:
  - POST /scripts                -> upload script (multipart): script file, optional requirements file, required 'name' form field
  - GET  /scripts                -> list scripts
@@ -32,7 +27,7 @@ from pydantic import BaseModel
 # ---------- Configuration ----------
 SCRIPT_DIR = "/srv/fw-scripts"
 VENV_BASE = "/srv/fw-scripts/venvs"
-UNIT_DIR = "/etc/systemd/system"
+UNIT_DIR = "/etc/systemd/system"   # retained for writing new units, but discovery uses systemctl
 UNIT_PREFIX = "fw-script"
 
 # ensure dirs exist
@@ -41,7 +36,7 @@ os.makedirs(VENV_BASE, exist_ok=True)
 
 # ---------- Logging ----------
 logging.basicConfig(level=logging.INFO, format="%(asctime)s %(levelname)s [%(name)s] %(message)s")
-logger = logging.getLogger("script-router-named-delete")
+logger = logging.getLogger("script-router-systemd")
 
 # ---------- Router ----------
 router = APIRouter(prefix="/scripts", tags=["scripts"])
@@ -57,7 +52,7 @@ def validate_name(name: str) -> None:
     if name in (".", ".."):
         raise ValueError("invalid name")
 
-# ---------- Utility paths ----------
+# ---------- Paths ----------
 def script_path_for(name: str) -> str:
     return os.path.join(SCRIPT_DIR, f"{name}.py")
 
@@ -74,12 +69,188 @@ def venv_python_for(name: str) -> str:
     return "/usr/bin/python3"
 
 def make_service_name(name: str, qnum: int) -> str:
+    # safe, deterministic service name
     return f"{UNIT_PREFIX}-{name}-q{qnum}"
 
-def unit_path_for(service_name: str) -> str:
+def unit_path_for_name(service_name: str) -> str:
+    # default location for units we write
     return os.path.join(UNIT_DIR, service_name + ".service")
 
-# ---------- Venv helpers ----------
+# ---------- systemd interaction (systemctl-based, no fallback) ----------
+def _systemctl_unit_name(unit: str) -> str:
+    """Return unit with .service suffix if missing."""
+    return unit if unit.endswith(".service") else unit + ".service"
+
+def list_fw_units() -> List[str]:
+    """
+    Return list of systemd units (without .service suffix) whose name starts with UNIT_PREFIX-.
+    Uses `systemctl list-units` to be canonical.
+    """
+    units: List[str] = []
+    try:
+        p = subprocess.run(["systemctl", "list-units", "--type=service", "--all", "--no-legend"],
+                           capture_output=True, text=True, check=False, timeout=3)
+        out = p.stdout or ""
+        # each line starts with unit name
+        for line in out.splitlines():
+            line = line.strip()
+            if not line:
+                continue
+            cols = line.split()
+            unit = cols[0]
+            if unit.startswith(UNIT_PREFIX + "-") and unit.endswith(".service"):
+                units.append(unit[:-8])  # strip ".service"
+    except Exception:
+        logger.exception("systemctl list-units failed")
+    return units
+
+def get_unit_fragment_path(service_name: str) -> Optional[str]:
+    """
+    Use `systemctl show -p FragmentPath --value` to obtain the unit file path (if any).
+    Returns None if unknown / empty.
+    """
+    unit = _systemctl_unit_name(service_name)
+    try:
+        p = subprocess.run(["systemctl", "show", "-p", "FragmentPath", "--value", unit],
+                           capture_output=True, text=True, check=False, timeout=2)
+        frag = (p.stdout or "").strip()
+        if not frag:
+            return None
+        return frag
+    except Exception:
+        logger.exception("systemctl show FragmentPath failed for %s", service_name)
+        return None
+
+def parse_unit_execstart(service_name: str) -> Optional[Dict]:
+    """
+    Query systemd for ExecStart (most reliable) and parse the script name and qnum.
+    Returns dict with keys: service, exec_start, name, script_path, qnum, extra
+    or None on irrecoverable error.
+    """
+    unit = _systemctl_unit_name(service_name)
+    try:
+        p = subprocess.run(["systemctl", "show", "-p", "ExecStart", "--value", unit],
+                           capture_output=True, text=True, check=False, timeout=2)
+        exec_start_raw = (p.stdout or "").strip()
+    except Exception:
+        logger.exception("systemctl show ExecStart failed for %s", service_name)
+        return {"service": service_name, "exec_start": None, "name": None, "script_path": None, "qnum": None, "extra": None}
+
+    if not exec_start_raw:
+        # no ExecStart known
+        return {"service": service_name, "exec_start": None, "name": None, "script_path": None, "qnum": None, "extra": None}
+
+    exec_start = exec_start_raw.strip()
+
+    # Find script path under /srv/fw-scripts
+    m = re.search(r'(/[^ \t\n\r"]*/srv/fw-scripts/(?P<name>[A-Za-z0-9_.-]+)\.py)\b', exec_start)
+    if not m:
+        # Return raw ExecStart but no parsed metadata
+        return {"service": service_name, "exec_start": exec_start, "name": None, "script_path": None, "qnum": None, "extra": None}
+
+    script_path = m.group(1)
+    name = m.group("name")
+
+    # Find integer queue number after the script path (first integer token after the script)
+    after = exec_start[m.end():].strip()
+    qnum = None
+    extra = None
+    if after:
+        toks = after.split()
+        for i, t in enumerate(toks):
+            try:
+                val = int(t)
+                if 0 <= val <= 65535:
+                    qnum = val
+                    extra = " ".join(toks[i+1:]) if len(toks) > i+1 else ""
+                    break
+            except Exception:
+                continue
+
+    return {"service": service_name, "exec_start": exec_start, "name": name, "script_path": script_path, "qnum": qnum, "extra": extra}
+
+def is_unit_active(service_name: str) -> bool:
+    unit = _systemctl_unit_name(service_name)
+    p = subprocess.run(["systemctl", "is-active", "--quiet", unit])
+    return p.returncode == 0
+
+def write_unit(service_name: str, exec_start: str, description: str = "", enable_at_boot: bool = False) -> str:
+    """
+    Write unit file to default UNIT_DIR and daemon-reload. This writes to disk as systemd expects.
+    """
+    unit_path = unit_path_for_name(service_name)
+    unit_text = f"""[Unit]
+Description={description}
+After=network.target
+
+[Service]
+Type=simple
+ExecStart={exec_start}
+Restart=always
+RestartSec=2
+StandardOutput=syslog
+StandardError=syslog
+
+[Install]
+WantedBy=multi-user.target
+"""
+    with open(unit_path, "w") as fh:
+        fh.write(unit_text)
+    subprocess.run(["systemctl", "daemon-reload"], check=True)
+    logger.info("Wrote unit %s", unit_path)
+    if enable_at_boot:
+        try:
+            subprocess.run(["systemctl", "enable", _systemctl_unit_name(service_name)], check=True)
+            logger.info("Enabled %s at boot", service_name)
+        except subprocess.CalledProcessError:
+            logger.warning("Failed to enable %s at boot", service_name)
+    return unit_path
+
+def start_unit(service_name: str) -> None:
+    subprocess.run(["systemctl", "start", _systemctl_unit_name(service_name)], check=True)
+    logger.info("Started service %s", service_name)
+
+def stop_unit(service_name: str) -> None:
+    subprocess.run(["systemctl", "stop", _systemctl_unit_name(service_name)], check=True)
+    logger.info("Stopped service %s", service_name)
+
+def disable_unit(service_name: str) -> None:
+    subprocess.run(["systemctl", "disable", _systemctl_unit_name(service_name)], check=False)
+    logger.info("Disabled service %s", service_name)
+
+def remove_unit(service_name: str) -> None:
+    """
+    Stop + disable the unit, remove the unit file using FragmentPath (if present),
+    and daemon-reload. All via systemctl queries (no fallback to scanning /etc).
+    """
+    unit = _systemctl_unit_name(service_name)
+    try:
+        subprocess.run(["systemctl", "stop", unit], check=False)
+    except Exception:
+        logger.exception("Failed stopping %s", unit)
+    try:
+        subprocess.run(["systemctl", "disable", unit], check=False)
+    except Exception:
+        pass
+
+    frag = get_unit_fragment_path(service_name)
+    if frag:
+        try:
+            if os.path.exists(frag):
+                os.remove(frag)
+                logger.info("Removed unit file at %s for %s", frag, service_name)
+        except Exception:
+            logger.exception("Failed to remove unit file %s", frag)
+    else:
+        logger.debug("No FragmentPath for %s; nothing to delete on-disk", service_name)
+
+    # ensure systemd reloads units
+    try:
+        subprocess.run(["systemctl", "daemon-reload"], check=True)
+    except Exception:
+        logger.exception("daemon-reload failed after removing unit %s", service_name)
+
+# ---------- venv + pip helpers ----------
 def create_venv(name: str, timeout: int = 60) -> str:
     venv_dir = venv_path_for(name)
     if os.path.exists(venv_dir):
@@ -125,91 +296,6 @@ def jsonify_cmd_output(out: Dict[str, str]) -> str:
         s += "STDERR:\n" + out["stderr"] + "\n"
     return s.strip()
 
-# ---------- systemd helpers ----------
-def write_unit(service_name: str, exec_start: str, description: str = "", enable_at_boot: bool = False) -> str:
-    unit_path = unit_path_for(service_name)
-    unit_text = f"""[Unit]
-Description={description}
-After=network.target
-
-[Service]
-Type=simple
-ExecStart={exec_start}
-Restart=always
-RestartSec=2
-StandardOutput=syslog
-StandardError=syslog
-
-[Install]
-WantedBy=multi-user.target
-"""
-    with open(unit_path, "w") as fh:
-        fh.write(unit_text)
-    subprocess.run(["systemctl", "daemon-reload"], check=True)
-    logger.info("Wrote unit %s", unit_path)
-    if enable_at_boot:
-        try:
-            subprocess.run(["systemctl", "enable", service_name], check=True)
-            logger.info("Enabled %s at boot", service_name)
-        except subprocess.CalledProcessError:
-            logger.warning("Failed to enable %s at boot", service_name)
-    return unit_path
-
-def remove_unit(service_name: str) -> None:
-    unit_path = unit_path_for(service_name)
-    try:
-        subprocess.run(["systemctl", "stop", service_name], check=False)
-    except Exception:
-        logger.exception("systemctl stop failed for %s", service_name)
-    if os.path.exists(unit_path):
-        try:
-            subprocess.run(["systemctl", "disable", service_name], check=False)
-        except Exception:
-            pass
-        os.remove(unit_path)
-        subprocess.run(["systemctl", "daemon-reload"], check=True)
-        logger.info("Removed unit %s", unit_path)
-
-def start_unit(service_name: str) -> None:
-    subprocess.run(["systemctl", "start", service_name], check=True)
-    logger.info("Started service %s", service_name)
-
-def stop_unit(service_name: str) -> None:
-    subprocess.run(["systemctl", "stop", service_name], check=True)
-    logger.info("Stopped service %s", service_name)
-
-def is_unit_active(service_name: str) -> bool:
-    p = subprocess.run(["systemctl", "is-active", "--quiet", service_name])
-    return p.returncode == 0
-
-def list_fw_units() -> List[str]:
-    units = []
-    try:
-        for fn in os.listdir(UNIT_DIR):
-            if fn.startswith(UNIT_PREFIX + "-") and fn.endswith(".service"):
-                units.append(fn[:-8])
-    except FileNotFoundError:
-        logger.warning("Unit dir %s not found", UNIT_DIR)
-    return units
-
-_RE_EXECSTART = re.compile(r'(?P<py>/\S*python\S*)\s+(?P<script>/\S*?/srv/fw-scripts/(?P<name>[A-Za-z0-9_.-]+)\.py)\s+(?P<qnum>\d+)(?:\s+(?P<extra>.*))?')
-
-def parse_unit_execstart(service_name: str) -> Optional[Dict]:
-    unit_path = unit_path_for(service_name)
-    if not os.path.exists(unit_path):
-        return None
-    with open(unit_path, "r") as fh:
-        content = fh.read()
-    m = re.search(r'^ExecStart=(.+)$', content, flags=re.MULTILINE)
-    if not m:
-        return None
-    exec_start = m.group(1).strip()
-    match = _RE_EXECSTART.search(exec_start)
-    if match:
-        sd = match.groupdict()
-        return {"service": service_name, "exec_start": exec_start, "name": sd["name"], "script_path": sd["script"], "qnum": int(sd["qnum"]), "extra": sd.get("extra") or ""}
-    return {"service": service_name, "exec_start": exec_start, "name": None, "script_path": None, "qnum": None, "extra": None}
-
 # ---------- Models ----------
 class ScriptInfo(BaseModel):
     name: str
@@ -222,6 +308,44 @@ class EnableRequest(BaseModel):
     enable_at_boot: Optional[bool] = False
 
 # ---------- Endpoints ----------
+# Note: Place status endpoints before the dynamic GET /{name} route to avoid routing conflicts.
+
+@router.get("/status")
+def status_all():
+    """
+    Discover all fw-script units via systemctl and report parsed ExecStart + active state.
+    """
+    units = list_fw_units()
+    results: Dict[str, Dict] = {}
+    for svc in units:
+        parsed = parse_unit_execstart(svc)
+        try:
+            active = is_unit_active(svc)
+        except Exception:
+            active = False
+        results[svc] = {"parsed": parsed, "active": active}
+    logger.debug("Status queried: found %d units", len(results))
+    return results
+
+@router.get("/{name}/status")
+def status_for_name(name: str):
+    try:
+        validate_name(name)
+    except ValueError as e:
+        raise HTTPException(status_code=400, detail=str(e))
+    units = list_fw_units()
+    matches = []
+    prefix = f"{UNIT_PREFIX}-{name}-q"
+    for svc in units:
+        if svc.startswith(prefix):
+            parsed = parse_unit_execstart(svc)
+            try:
+                active = is_unit_active(svc)
+            except Exception:
+                active = False
+            matches.append({"service": svc, "parsed": parsed, "active": active})
+    logger.debug("Status for %s -> %d matches", name, len(matches))
+    return {"name": name, "mappings": matches}
 
 @router.post("", response_model=ScriptInfo)
 async def upload_script(
@@ -229,6 +353,11 @@ async def upload_script(
     name: str = Form(...),
     requirements: Optional[UploadFile] = File(None),
 ):
+    """
+    Upload a script with supplied 'name' and optional requirements file.
+    On pip/venv install failure, cleanup uploaded files and venv and return 500 with details.
+    """
+    # validate name
     try:
         validate_name(name)
     except ValueError as e:
@@ -274,7 +403,7 @@ async def upload_script(
             except Exception as pip_exc:
                 err_msg = str(pip_exc)
                 logger.error("pip install error for %s: %s", name, err_msg[:2000])
-                # cleanup
+                # cleanup: remove script, req file, partial venv
                 try:
                     if os.path.exists(spath):
                         os.remove(spath)
@@ -324,6 +453,7 @@ def list_scripts():
 
 @router.get("/{name}")
 def download_script(name: str):
+    # dynamic route - placed after /status and /{name}/status
     try:
         validate_name(name)
     except ValueError as e:
@@ -382,22 +512,20 @@ def disable_script(name: str, qnum: int):
     except ValueError as e:
         raise HTTPException(status_code=400, detail=str(e))
     service_name = make_service_name(name, qnum)
-    unit_p = unit_path_for(service_name)
+    # attempt stop + remove via systemctl-based remove_unit
-    if not os.path.exists(unit_p):
-        try:
-            subprocess.run(["systemctl", "stop", service_name], check=False)
-        except Exception:
-            pass
-        raise HTTPException(status_code=404, detail="service/unit not found")
     try:
-        stop_unit(service_name)
+        # If unit exists according to systemctl, remove it; otherwise still try stopping
-    except Exception:
+        units = list_fw_units()
-        logger.exception("Failed stopping service %s", service_name)
+        if service_name in units:
-    try:
+            remove_unit(service_name)
-        remove_unit(service_name)
+            logger.info("Disabled and removed unit %s", service_name)
-    except Exception:
+        else:
-        logger.exception("Failed removing unit %s", service_name)
+            # attempt stop anyway
-    logger.info("Disabled script %s on qnum=%s (removed service %s)", name, qnum, service_name)
+            subprocess.run(["systemctl", "stop", _systemctl_unit_name(service_name)], check=False)
+            logger.info("Tried stopping unit %s (unit file not present)", service_name)
+    except Exception as e:
+        logger.exception("Failed to disable unit %s: %s", service_name, e)
+        raise HTTPException(status_code=500, detail=str(e))
     return {"status": "ok", "name": name, "qnum": qnum}
 
 @router.delete("/{name}")
@@ -418,12 +546,11 @@ def delete_script(name: str, qnum: Optional[int] = Query(None, description="If g
     failed_units: List[str] = []
     errors: List[str] = []
 
-    # determine which units to remove
+    # determine units to handle
     if qnum is not None:
         svc = make_service_name(name, qnum)
         units_to_handle = [svc]
     else:
-        # scan unit directory for matching units
         all_units = list_fw_units()
         prefix = f"{UNIT_PREFIX}-{name}-q"
         units_to_handle = [u for u in all_units if u.startswith(prefix)]
@@ -431,69 +558,50 @@ def delete_script(name: str, qnum: Optional[int] = Query(None, description="If g
     # stop and remove units
     for svc in units_to_handle:
         try:
-            # attempt to stop via systemctl even if unit file missing
             try:
-                subprocess.run(["systemctl", "stop", svc], check=False)
+                subprocess.run(["systemctl", "stop", _systemctl_unit_name(svc)], check=False)
             except Exception:
                 pass
-            unit_file = unit_path_for(svc)
+            # remove unit (systemd-based)
-            if os.path.exists(unit_file):
+            remove_unit(svc)
-                try:
+            removed_units.append(svc)
-                    remove_unit(svc)
-                    removed_units.append(svc)
-                except Exception as e:
-                    logger.exception("Failed to remove unit %s: %s", svc, e)
-                    failed_units.append(svc)
-                    errors.append(f"remove_unit {svc}: {e}")
-            else:
-                # unit file doesn't exist - treat as stopped/absent but attempt systemd stop above
-                removed_units.append(svc)
         except Exception as e:
-            logger.exception("Error handling unit %s: %s", svc, e)
+            logger.exception("Error removing unit %s: %s", svc, e)
             failed_units.append(svc)
-            errors.append(f"error handling {svc}: {e}")
+            errors.append(f"remove_unit {svc}: {e}")
 
-    # remove files: script, requirements, venv
+    # remove files
     spath = script_path_for(name)
     rpath = requirements_path_for(name)
     vpath = venv_path_for(name)
     file_removed = []
     file_failed = []
 
-    # remove script file
     try:
         if os.path.exists(spath):
             os.remove(spath)
             file_removed.append(spath)
             logger.info("Removed script file %s", spath)
-        else:
-            logger.debug("Script file %s not present", spath)
     except Exception as e:
         logger.exception("Failed removing script file %s: %s", spath, e)
         file_failed.append(spath)
         errors.append(f"remove_script {spath}: {e}")
 
-    # remove requirements file
     try:
         if os.path.exists(rpath):
             os.remove(rpath)
             file_removed.append(rpath)
             logger.info("Removed requirements file %s", rpath)
-        else:
-            logger.debug("Requirements file %s not present", rpath)
     except Exception as e:
         logger.exception("Failed removing requirements file %s: %s", rpath, e)
         file_failed.append(rpath)
         errors.append(f"remove_requirements {rpath}: {e}")
 
-    # remove venv dir
     try:
         if os.path.isdir(vpath):
             shutil.rmtree(vpath, ignore_errors=False)
             file_removed.append(vpath)
             logger.info("Removed venv directory %s", vpath)
-        else:
-            logger.debug("Venv dir %s not present", vpath)
     except Exception as e:
         logger.exception("Failed removing venv %s: %s", vpath, e)
         file_failed.append(vpath)
@@ -507,44 +615,9 @@ def delete_script(name: str, qnum: Optional[int] = Query(None, description="If g
         "files_failed": file_failed,
         "errors": errors,
     }
-
     logger.info("Delete script %s completed: removed_units=%d files_removed=%d errors=%d", name, len(removed_units), len(file_removed), len(errors))
     return result
 
-@router.get("/status")
-def status_all():
-    units = list_fw_units()
-    results = {}
-    for svc in units:
-        parsed = parse_unit_execstart(svc)
-        try:
-            active = is_unit_active(svc)
-        except Exception:
-            active = False
-        results[svc] = {"parsed": parsed, "active": active}
-    logger.debug("Status queried: found %d units", len(results))
-    return results
-
-@router.get("/{name}/status")
-def status_for_name(name: str):
-    try:
-        validate_name(name)
-    except ValueError as e:
-        raise HTTPException(status_code=400, detail=str(e))
-    units = list_fw_units()
-    matches = []
-    prefix = f"{UNIT_PREFIX}-{name}-q"
-    for svc in units:
-        if svc.startswith(prefix):
-            parsed = parse_unit_execstart(svc)
-            try:
-                active = is_unit_active(svc)
-            except Exception:
-                active = False
-            matches.append({"service": svc, "parsed": parsed, "active": active})
-    logger.debug("Status for %s -> %d matches", name, len(matches))
-    return {"name": name, "mappings": matches}
-
 # ---------- Lifecycle helper ----------
 def register_lifecycle(app):
     @app.on_event("shutdown")
@@ -554,7 +627,7 @@ def register_lifecycle(app):
         for svc in units:
             if svc.startswith(UNIT_PREFIX + "-"):
                 try:
-                    subprocess.run(["systemctl", "stop", svc], check=False)
+                    subprocess.run(["systemctl", "stop", _systemctl_unit_name(svc)], check=False)
                 except Exception:
                     pass
                 try: