Fix policy routing and PPP failure handling

internal/api/backup_handlers.go

@@ -2,6 +2,7 @@ package api
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -63,9 +64,9 @@ func (s *Server) handleBackupInspect(w http.ResponseWriter, r *http.Request) {
 		s.backupMgr = backup.NewManager("", "", "", nil)
 	}
 
-	buf, err := readUploadedBackup(r)
+	buf, err := readUploadedBackup(w, r)
 	if err != nil {
-		writeError(w, http.StatusBadRequest, err.Error())
+		writeBackupUploadError(w, err)
 		return
 	}
 
@@ -90,9 +91,9 @@ func (s *Server) handleBackupRestore(w http.ResponseWriter, r *http.Request) {
 		s.backupMgr = backup.NewManager("", "", "", nil)
 	}
 
-	buf, err := readUploadedBackup(r)
+	buf, err := readUploadedBackup(w, r)
 	if err != nil {
-		writeError(w, http.StatusBadRequest, err.Error())
+		writeBackupUploadError(w, err)
 		return
 	}
 
@@ -186,35 +187,47 @@ func (s *Server) handleBackupRestoreLocal(w http.ResponseWriter, r *http.Request
 	writeJSON(w, http.StatusOK, res)
 }
 
-func readUploadedBackup(r *http.Request) ([]byte, error) {
-	contentType := r.Header.Get("Content-Type")
+const maxBackupUploadBytes int64 = backup.MaxArchiveBytes + (1 << 20)
+
+func writeBackupUploadError(w http.ResponseWriter, err error) {
+	var maxErr *http.MaxBytesError
+	if errors.As(err, &maxErr) {
+		writeError(w, http.StatusRequestEntityTooLarge, "Backup upload exceeds the size limit")
+		return
+	}
+	writeError(w, http.StatusBadRequest, err.Error())
+}
 
-	// Multipart form upload
+func readUploadedBackup(w http.ResponseWriter, r *http.Request) ([]byte, error) {
+	return readUploadedBackupLimit(w, r, maxBackupUploadBytes)
+}
+
+func readUploadedBackupLimit(w http.ResponseWriter, r *http.Request, limit int64) ([]byte, error) {
+	if r.Body == nil {
+		return nil, fmt.Errorf("empty request body")
+	}
+	if r.ContentLength > limit {
+		return nil, &http.MaxBytesError{Limit: limit}
+	}
+	r.Body = http.MaxBytesReader(w, r.Body, limit)
+	defer r.Body.Close()
+	contentType := r.Header.Get("Content-Type")
 	if strings.Contains(contentType, "multipart/form-data") {
 		if err := r.ParseMultipartForm(32 << 20); err != nil {
 			return nil, fmt.Errorf("failed parsing upload: %w", err)
 		}
-		var file io.Reader
+		if r.MultipartForm != nil {
+			defer r.MultipartForm.RemoveAll()
+		}
 		f, _, err := r.FormFile("backup")
-		if err == nil {
-			defer f.Close()
-			file = f
-		} else {
-			f, _, err2 := r.FormFile("file")
-			if err2 == nil {
-				defer f.Close()
-				file = f
-			} else {
-				return nil, fmt.Errorf("no 'backup' or 'file' uploaded")
-			}
+		if err != nil {
+			f, _, err = r.FormFile("file")
 		}
-		return io.ReadAll(file)
-	}
-
-	// Raw binary zip stream
-	if r.Body == nil {
-		return nil, fmt.Errorf("empty request body")
+		if err != nil {
+			return nil, fmt.Errorf("no 'backup' or 'file' uploaded")
+		}
+		defer f.Close()
+		return io.ReadAll(f)
 	}
-	defer r.Body.Close()
 	return io.ReadAll(r.Body)
 }

internal/api/backup_handlers_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"fmt"
 	"io"
 	"mime/multipart"
 	"net/http"
@@ -154,3 +155,36 @@ func TestBackupAPIHandlers(t *testing.T) {
 		t.Errorf("Expected 403 Forbidden for read-only user, got %d", roRec.Code)
 	}
 }
+
+func TestBackupUploadSizeLimit(t *testing.T) {
+	for _, multipartMode := range []bool{false, true} {
+		t.Run(fmt.Sprintf("multipart=%v", multipartMode), func(t *testing.T) {
+			var body bytes.Buffer
+			contentType := "application/zip"
+			if multipartMode {
+				w := multipart.NewWriter(&body)
+				part, err := w.CreateFormFile("backup", "x.zip")
+				if err != nil {
+					t.Fatal(err)
+				}
+				part.Write(bytes.Repeat([]byte("x"), 512))
+				w.Close()
+				contentType = w.FormDataContentType()
+			} else {
+				body.Write(bytes.Repeat([]byte("x"), 512))
+			}
+			req := httptest.NewRequest(http.MethodPost, "/", &body)
+			req.Header.Set("Content-Type", contentType)
+			req.ContentLength = -1
+			rec := httptest.NewRecorder()
+			_, err := readUploadedBackupLimit(rec, req, 256)
+			if err == nil {
+				t.Fatal("oversized body accepted")
+			}
+			writeBackupUploadError(rec, err)
+			if rec.Code != http.StatusRequestEntityTooLarge {
+				t.Fatalf("HTTP %d", rec.Code)
+			}
+		})
+	}
+}

internal/routing/manager.go

@@ -10,6 +10,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"regexp"
 	"sort"
 	"strconv"
 	"strings"
@@ -44,7 +45,9 @@ func NewRoutingManager() *RoutingManager {
 	}
 	m.initRTTableFile()
 	m.load()
-	m.syncCustomTablesToSystem()
+	if err := m.syncCustomTablesToSystem(); err != nil {
+		log.Printf("routing table synchronization failed: %v", err)
+	}
 	m.applyAll()
 	m.startHealthCheckWorker()
 	m.startRouteSyncWorker()
@@ -151,21 +154,49 @@ func (m *RoutingManager) save() error {
 	return nil
 }
 
-func (m *RoutingManager) syncCustomTablesToSystem() {
-	content, _ := os.ReadFile(RTTableFilePath)
-	str := string(content)
-
+func (m *RoutingManager) syncCustomTablesToSystem() error {
+	content, err := os.ReadFile(RTTableFilePath)
+	if err != nil && !os.IsNotExist(err) {
+		return fmt.Errorf("reading routing table registry: %w", err)
+	}
 	var sb strings.Builder
-	sb.WriteString(str)
-
-	for _, t := range m.customTables {
-		check := fmt.Sprintf("\t%s", t.Name)
-		if !strings.Contains(str, check) {
-			sb.WriteString(fmt.Sprintf("%d\t%s\n", t.TableID, t.Name))
+	sb.Write(content)
+	if len(content) > 0 && content[len(content)-1] != '\n' {
+		sb.WriteByte('\n')
+	}
+	existing := map[string]bool{}
+	for _, line := range strings.Split(string(content), "\n") {
+		fields := strings.Fields(line)
+		if len(fields) >= 2 && !strings.HasPrefix(fields[0], "#") {
+			existing[fields[1]] = true
 		}
 	}
-
-	_ = os.WriteFile(RTTableFilePath, []byte(sb.String()), 0644)
+	for _, table := range m.customTables {
+		if !existing[table.Name] {
+			fmt.Fprintf(&sb, "%d\t%s\n", table.TableID, table.Name)
+			existing[table.Name] = true
+		}
+	}
+	dir := filepath.Dir(RTTableFilePath)
+	f, err := os.CreateTemp(dir, ".rt-tables-*.tmp")
+	if err != nil {
+		return err
+	}
+	name := f.Name()
+	defer os.Remove(name)
+	if err = f.Chmod(0644); err == nil {
+		_, err = f.WriteString(sb.String())
+	}
+	if err == nil {
+		err = f.Sync()
+	}
+	if closeErr := f.Close(); err == nil {
+		err = closeErr
+	}
+	if err != nil {
+		return err
+	}
+	return os.Rename(name, RTTableFilePath)
 }
 
 func (m *RoutingManager) cleanRogueDefaultRoutes() {
@@ -1379,10 +1410,15 @@ func (m *RoutingManager) ListPolicyRules() []PolicyRuleItem {
 	tableNames := m.getTableMap()
 	var res []PolicyRuleItem
 
-	userRulesByPrio := make(map[int]PolicyRuleItem)
+	actions, actionErr := m.listKernelPolicyActions()
+	if actionErr != nil {
+		log.Printf("policy rule action lookup failed: %v", actionErr)
+	}
+	userRulesByPrio := make(map[int][]PolicyRuleItem)
 	for _, pr := range m.policyRules {
-		userRulesByPrio[pr.Priority] = pr
+		userRulesByPrio[pr.Priority] = append(userRulesByPrio[pr.Priority], pr)
 	}
+	matchedManaged := make(map[string]bool)
 
 	for _, r := range rules {
 		tableName := "main"
@@ -1406,6 +1442,13 @@ func (m *RoutingManager) ListPolicyRules() []PolicyRuleItem {
 			markStr = fmt.Sprintf("0x%x (%d)", r.Mark, r.Mark)
 		}
 
+		action := actions[policyKernelKey(r.Priority, src, dst, r.Mark, r.Table)]
+		if action == "" {
+			action = "unknown"
+		}
+		if action != "lookup" {
+			tableName = ""
+		}
 		id := fmt.Sprintf("rule-%d-%d", r.Priority, r.Table)
 		comment := ""
 		isDefault := false
@@ -1414,16 +1457,16 @@ func (m *RoutingManager) ListPolicyRules() []PolicyRuleItem {
 		if r.Priority == 0 || r.Priority == 32766 || r.Priority == 32767 {
 			isDefault = true
 			isSystem = true
-		} else if userRule, ok := userRulesByPrio[r.Priority]; ok {
-			if userRule.ID != "" {
-				id = userRule.ID
-			}
-			comment = userRule.Comment
-			isDefault = false
-			isSystem = false
 		} else {
-			// System/daemon rule not added by user in debtik (e.g. WireGuard, IPsec)
+			// A priority alone does not identify a rule: kernel priorities may be shared.
 			isSystem = true
+			for _, userRule := range userRulesByPrio[r.Priority] {
+				if !matchedManaged[userRule.ID] && m.matchesPolicyRule(userRule, r, action) {
+					id, comment, isSystem = userRule.ID, userRule.Comment, false
+					matchedManaged[userRule.ID] = true
+					break
+				}
+			}
 		}
 
 		res = append(res, PolicyRuleItem{
@@ -1434,7 +1477,7 @@ func (m *RoutingManager) ListPolicyRules() []PolicyRuleItem {
 			RoutingMark: markStr,
 			Table:       tableName,
 			TableID:     r.Table,
-			Action:      "lookup",
+			Action:      action,
 			Comment:     comment,
 			IsDefault:   isDefault,
 			IsSystem:    isSystem,
@@ -1444,12 +1487,161 @@ func (m *RoutingManager) ListPolicyRules() []PolicyRuleItem {
 	return res
 }
 
+type policyJSONRule struct {
+	Priority int             `json:"priority"`
+	Src      string          `json:"src"`
+	SrcLen   *int            `json:"srclen"`
+	Dst      string          `json:"dst"`
+	DstLen   *int            `json:"dstlen"`
+	Mark     string          `json:"fwmark"`
+	Table    json.RawMessage `json:"table"`
+	Action   string          `json:"action"`
+}
+
+func policyKernelKey(priority int, src, dst string, mark uint32, table int) string {
+	return fmt.Sprintf("%d|%s|%s|%d|%d", priority, normalizedRulePrefix(src), normalizedRulePrefix(dst), mark, table)
+}
+func jsonRulePrefix(addr string, bits *int) string {
+	if addr == "" || addr == "all" {
+		return ""
+	}
+	if bits == nil {
+		return addr
+	}
+	return fmt.Sprintf("%s/%d", addr, *bits)
+}
+
+// netlink v1.3.1 does not populate Rule.Type, so obtain actions from iproute2.
+// Ambiguous snapshots are intentionally not attributed to managed rules.
+func (m *RoutingManager) listKernelPolicyActions() (map[string]string, error) {
+	out, err := exec.Command("ip", "-j", "-4", "rule", "show").CombinedOutput()
+	if err != nil {
+		return nil, fmt.Errorf("listing policy actions: %s: %w", strings.TrimSpace(string(out)), err)
+	}
+	var rules []policyJSONRule
+	if err = json.Unmarshal(out, &rules); err != nil {
+		return nil, fmt.Errorf("parsing policy rules: %w", err)
+	}
+	result := make(map[string]string)
+	for _, r := range rules {
+		var table int
+		if len(r.Table) > 0 && string(r.Table) != "null" {
+			var name string
+			if json.Unmarshal(r.Table, &name) == nil {
+				table = m.getTableIDByName(name)
+			} else if err = json.Unmarshal(r.Table, &table); err != nil {
+				return nil, fmt.Errorf("invalid policy table: %w", err)
+			}
+		}
+		action := strings.ToLower(r.Action)
+		if action == "" && table != 0 {
+			action = "lookup"
+		}
+		if action == "" {
+			action = "unknown"
+		}
+		var mark uint64
+		if r.Mark != "" {
+			mark, err = strconv.ParseUint(strings.Split(r.Mark, "/")[0], 0, 32)
+			if err != nil {
+				return nil, fmt.Errorf("invalid policy mark: %w", err)
+			}
+		}
+		key := policyKernelKey(r.Priority, jsonRulePrefix(r.Src, r.SrcLen), jsonRulePrefix(r.Dst, r.DstLen), uint32(mark), table)
+		if _, exists := result[key]; exists {
+			result[key] = ""
+		} else {
+			result[key] = action
+		}
+	}
+	return result, nil
+}
+func normalizedRulePrefix(value string) string {
+	value = strings.TrimSpace(value)
+	if value == "" {
+		return ""
+	}
+	if ip := net.ParseIP(value); ip != nil {
+		if ip.To4() != nil {
+			return ip.String() + "/32"
+		}
+		return ip.String() + "/128"
+	}
+	_, network, err := net.ParseCIDR(value)
+	if err != nil {
+		return value
+	}
+	ones, _ := network.Mask.Size()
+	if ones == 0 {
+		return ""
+	}
+	return network.String()
+}
+func (m *RoutingManager) matchesPolicyRule(saved PolicyRuleItem, live netlink.Rule, liveAction string) bool {
+	if saved.Priority != live.Priority || live.IifName != "" || live.OifName != "" || live.Invert || live.Tos != 0 {
+		return false
+	}
+	action := saved.Action
+	if action == "" {
+		action = "lookup"
+	}
+	if action != liveAction {
+		return false
+	}
+	if action == "lookup" && m.getTableIDByName(saved.Table) != live.Table {
+		return false
+	}
+	src, dst := "", ""
+	if live.Src != nil {
+		src = live.Src.String()
+	}
+	if live.Dst != nil {
+		dst = live.Dst.String()
+	}
+	if normalizedRulePrefix(saved.SrcAddress) != normalizedRulePrefix(src) || normalizedRulePrefix(saved.DstAddress) != normalizedRulePrefix(dst) {
+		return false
+	}
+	mark := strings.TrimSpace(strings.Split(saved.RoutingMark, "(")[0])
+	var expected uint64
+	if mark != "" {
+		var err error
+		expected, err = strconv.ParseUint(mark, 0, 32)
+		if err != nil {
+			return false
+		}
+	}
+	if uint32(expected) != live.Mark {
+		return false
+	}
+	if mark == "" && live.Mask != nil {
+		return false
+	}
+	if mark != "" && expected == 0 && live.Mask == nil {
+		return false
+	}
+	if live.Mask != nil && *live.Mask != ^uint32(0) {
+		return false
+	}
+	return true
+}
+
 func (m *RoutingManager) SavePolicyRule(pr PolicyRuleItem) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
-
-	if pr.Table == "" {
-		pr.Table = "main"
+	pr.Action = strings.ToLower(strings.TrimSpace(pr.Action))
+	if pr.Action == "" {
+		pr.Action = "lookup"
+	}
+	switch pr.Action {
+	case "lookup":
+		if pr.Table == "" {
+			pr.Table = "main"
+		}
+	case "blackhole", "unreachable", "prohibit":
+		pr.Table = ""
+		pr.TableID = 0
+	default:
+		return fmt.Errorf("invalid policy rule action: %s", pr.Action)
 	}
 	if pr.ID == "" {
 		pr.ID = fmt.Sprintf("prule-%d", time.Now().UnixNano())
@@ -1457,45 +1649,99 @@ func (m *RoutingManager) SavePolicyRule(pr PolicyRuleItem) error {
 	if pr.Priority == 0 {
 		pr.Priority = 1000 + len(m.policyRules)*10
 	}
-
+	if pr.Priority <= 0 || pr.Priority == 32766 || pr.Priority == 32767 {
+		return fmt.Errorf("invalid or reserved policy rule priority")
+	}
+	for _, existing := range m.policyRules {
+		if existing.ID == pr.ID || existing.Priority == pr.Priority {
+			return fmt.Errorf("policy rule ID or priority already exists")
+		}
+	}
+	liveRules, err := netlink.RuleList(netlink.FAMILY_V4)
+	if err != nil {
+		return fmt.Errorf("listing existing policy rules: %w", err)
+	}
+	actions, err := m.listKernelPolicyActions()
+	if err != nil {
+		return err
+	}
+	for _, live := range liveRules {
+		if live.Priority != pr.Priority {
+			continue
+		}
+		src, dst := "", ""
+		if live.Src != nil {
+			src = live.Src.String()
+		}
+		if live.Dst != nil {
+			dst = live.Dst.String()
+		}
+		action := actions[policyKernelKey(live.Priority, src, dst, live.Mark, live.Table)]
+		if action == "" {
+			return fmt.Errorf("ambiguous existing policy rule state; retry")
+		}
+		if m.matchesPolicyRule(pr, live, action) {
+			return fmt.Errorf("matching policy rule already exists in kernel")
+		}
+	}
 	if err := m.applyRuleToKernel(pr); err != nil {
 		return err
 	}
-
+	before := append([]PolicyRuleItem(nil), m.policyRules...)
 	m.policyRules = append(m.policyRules, pr)
-	return m.save()
+	if err := m.save(); err != nil {
+		m.policyRules = before
+		persistErr := m.save()
+		if persistErr != nil {
+			persistErr = fmt.Errorf("restoring saved policy rules: %w", persistErr)
+		}
+		rollbackErr := m.removeRuleFromKernel(pr)
+		if rollbackErr != nil {
+			rollbackErr = fmt.Errorf("removing new kernel rule: %w", rollbackErr)
+		}
+		return errors.Join(fmt.Errorf("saving policy rule: %w", err), persistErr, rollbackErr)
+	}
+	return nil
 }
-
 func (m *RoutingManager) DeletePolicyRule(id string) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
-
-	var updated []PolicyRuleItem
-	var target *PolicyRuleItem
-	for _, pr := range m.policyRules {
+	index := -1
+	for i, pr := range m.policyRules {
 		if pr.ID == id || fmt.Sprintf("rule-%d-%d", pr.Priority, pr.TableID) == id {
-			target = &pr
-		} else {
-			updated = append(updated, pr)
+			index = i
+			break
 		}
 	}
-
-	if target != nil {
-		_ = m.removeRuleFromKernel(*target)
-	} else if strings.HasPrefix(id, "rule-") {
-		parts := strings.Split(id, "-")
-		if len(parts) >= 2 {
-			if prio, err := strconv.Atoi(parts[1]); err == nil && prio > 0 {
-				_ = exec.Command("ip", "rule", "del", "pref", strconv.Itoa(prio)).Run()
-			}
+	if index < 0 {
+		return fmt.Errorf("managed policy rule not found: %s", id)
+	}
+	before := append([]PolicyRuleItem(nil), m.policyRules...)
+	target := m.policyRules[index]
+	if err := m.removeRuleFromKernel(target); err != nil {
+		return err
+	}
+	m.policyRules = append(m.policyRules[:index:index], m.policyRules[index+1:]...)
+	if err := m.save(); err != nil {
+		m.policyRules = before
+		persistErr := m.save()
+		if persistErr != nil {
+			persistErr = fmt.Errorf("restoring saved policy rules: %w", persistErr)
 		}
+		rollbackErr := m.applyRuleToKernel(target)
+		if rollbackErr != nil {
+			rollbackErr = fmt.Errorf("restoring kernel rule: %w", rollbackErr)
+		}
+		return errors.Join(fmt.Errorf("saving policy rule deletion: %w", err), persistErr, rollbackErr)
 	}
-	m.policyRules = updated
-	return m.save()
+	return nil
 }
-
-func (m *RoutingManager) applyRuleToKernel(pr PolicyRuleItem) error {
-	args := []string{"rule", "add"}
+func policyRuleArgs(verb string, pr PolicyRuleItem) ([]string, error) {
+	action := strings.ToLower(strings.TrimSpace(pr.Action))
+	if action == "" {
+		action = "lookup"
+	}
+	args := []string{"rule", verb}
 	if pr.Priority > 0 {
 		args = append(args, "pref", strconv.Itoa(pr.Priority))
 	}
@@ -1506,43 +1752,46 @@ func (m *RoutingManager) applyRuleToKernel(pr PolicyRuleItem) error {
 		args = append(args, "to", pr.DstAddress)
 	}
 	if pr.RoutingMark != "" {
-		// Clean mark string if it has (hex)
-		markClean := pr.RoutingMark
-		if strings.Contains(markClean, "(") {
-			markClean = strings.TrimSpace(markClean[:strings.Index(markClean, "(")])
+		mark := pr.RoutingMark
+		if strings.Contains(mark, "(") {
+			mark = strings.TrimSpace(mark[:strings.Index(mark, "(")])
 		}
-		args = append(args, "fwmark", markClean)
+		args = append(args, "fwmark", mark)
 	}
-	if pr.Table != "" {
-		args = append(args, "lookup", pr.Table)
+	switch action {
+	case "lookup":
+		table := pr.Table
+		if table == "" {
+			table = "main"
+		}
+		args = append(args, "lookup", table)
+	case "blackhole", "unreachable", "prohibit":
+		args = append(args, action)
+	default:
+		return nil, fmt.Errorf("invalid policy rule action: %s", action)
 	}
-
-	cmd := exec.Command("ip", args...)
-	out, err := cmd.CombinedOutput()
+	return args, nil
+}
+func (m *RoutingManager) applyRuleToKernel(pr PolicyRuleItem) error {
+	args, err := policyRuleArgs("add", pr)
+	if err != nil {
+		return err
+	}
+	out, err := exec.Command("ip", args...).CombinedOutput()
 	if err != nil {
-		return fmt.Errorf("failed to add rule (%s): %s: %w", strings.Join(args, " "), string(out), err)
+		return fmt.Errorf("failed to add rule (%s): %s: %w", strings.Join(args, " "), strings.TrimSpace(string(out)), err)
 	}
 	return nil
 }
-
 func (m *RoutingManager) removeRuleFromKernel(pr PolicyRuleItem) error {
-	if pr.Priority > 0 {
-		_ = exec.Command("ip", "rule", "del", "pref", strconv.Itoa(pr.Priority)).Run()
-		return nil
-	}
-	args := []string{"rule", "del"}
-	if pr.RoutingMark != "" {
-		markClean := pr.RoutingMark
-		if strings.Contains(markClean, "(") {
-			markClean = strings.TrimSpace(markClean[:strings.Index(markClean, "(")])
-		}
-		args = append(args, "fwmark", markClean)
+	args, err := policyRuleArgs("del", pr)
+	if err != nil {
+		return err
 	}
-	if pr.Table != "" {
-		args = append(args, "lookup", pr.Table)
+	out, err := exec.Command("ip", args...).CombinedOutput()
+	if err != nil {
+		return fmt.Errorf("failed to delete rule (%s): %s: %w", strings.Join(args, " "), strings.TrimSpace(string(out)), err)
 	}
-
-	_ = exec.Command("ip", args...).Run()
 	return nil
 }
 
@@ -1566,23 +1815,38 @@ func (m *RoutingManager) SaveTable(t RoutingTableItem) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
-	if t.Name == "" {
-		return fmt.Errorf("table name is required")
+	t.Name = strings.TrimSpace(t.Name)
+	if t.Name == "" || !regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_.-]*$`).MatchString(t.Name) {
+		return fmt.Errorf("invalid routing table name")
+	}
+	if t.TableID < 0 || uint64(t.TableID) > uint64(^uint32(0)) {
+		return fmt.Errorf("invalid routing table ID")
 	}
 	if t.TableID == 0 {
 		// Auto assign FIB ID starting at 100
 		t.TableID = 100 + len(m.customTables)
 	}
 
-	for _, existing := range m.customTables {
-		if existing.Name == t.Name || existing.TableID == t.TableID {
+	for id, name := range m.getTableMap() {
+		if strings.EqualFold(name, t.Name) || id == t.TableID {
 			return fmt.Errorf("table with name %q or ID %d already exists", t.Name, t.TableID)
 		}
 	}
 
+	before := append([]RoutingTableItem(nil), m.customTables...)
 	m.customTables = append(m.customTables, t)
-	_ = m.save()
-	m.syncCustomTablesToSystem()
+	if err := m.save(); err != nil {
+		m.customTables = before
+		return errors.Join(fmt.Errorf("saving routing table: %w", err), m.save())
+	}
+	if err := m.syncCustomTablesToSystem(); err != nil {
+		m.customTables = before
+		restoreErr := m.save()
+		if restoreErr != nil {
+			restoreErr = fmt.Errorf("restoring saved routing tables: %w", restoreErr)
+		}
+		return errors.Join(fmt.Errorf("registering routing table: %w", err), restoreErr)
+	}
 	return nil
 }
 

internal/routing/policy_transaction_test.go

@@ -0,0 +1,122 @@
+package routing
+
+import (
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+func TestIsolatedPolicyActionsAndExactDeletion(t *testing.T) {
+	if os.Getenv("DEBTIK_TEST_ISOLATED_NETNS") != "1" {
+		t.Skip("requires a dedicated network namespace")
+	}
+	useTempRouteStores(t)
+	ip := func(args ...string) string {
+		t.Helper()
+		out, err := exec.Command("ip", args...).CombinedOutput()
+		if err != nil {
+			t.Fatalf("ip %v: %s %v", args, out, err)
+		}
+		return string(out)
+	}
+	ip("rule", "add", "pref", "11200", "from", "203.0.113.0/24", "lookup", "main")
+	m := &RoutingManager{}
+	for _, tc := range []struct {
+		priority int
+		action   string
+	}{{11200, "blackhole"}, {11201, "unreachable"}, {11202, "lookup"}} {
+		p := PolicyRuleItem{ID: tc.action, Priority: tc.priority, Action: tc.action, SrcAddress: "198.51.100.0/24", Table: "main"}
+		if err := m.SavePolicyRule(p); err != nil {
+			t.Fatalf("save %s: %v", tc.action, err)
+		}
+	}
+	listed := m.ListPolicyRules()
+	seen := map[string]bool{}
+	unmanaged := false
+	for _, rule := range listed {
+		if rule.ID == "blackhole" || rule.ID == "unreachable" || rule.ID == "lookup" {
+			if rule.Action != rule.ID || rule.IsSystem {
+				t.Fatalf("incorrect rule: %+v", rule)
+			}
+			seen[rule.ID] = true
+		}
+		if rule.Priority == 11200 && rule.SrcAddress == "203.0.113.0/24" {
+			unmanaged = rule.IsSystem
+		}
+	}
+	if len(seen) != 3 || !unmanaged {
+		t.Fatalf("incorrect mapping: %v unmanaged=%v listed=%+v", seen, unmanaged, listed)
+	}
+	if err := m.DeletePolicyRule("blackhole"); err != nil {
+		t.Fatal(err)
+	}
+	show := ip("rule", "show")
+	if !strings.Contains(show, "from 203.0.113.0/24 lookup main") || strings.Contains(show, "from 198.51.100.0/24 blackhole") {
+		t.Fatalf("wrong rule deleted: %s", show)
+	}
+	if err := m.DeletePolicyRule("rule-11200-254"); err == nil {
+		t.Fatal("unmanaged rule deletion accepted")
+	}
+	for _, id := range []string{"unreachable", "lookup"} {
+		if err := m.DeletePolicyRule(id); err != nil {
+			t.Fatal(err)
+		}
+	}
+}
+func TestIsolatedPolicySaveFailureRemovesKernelRule(t *testing.T) {
+	if os.Getenv("DEBTIK_TEST_ISOLATED_NETNS") != "1" {
+		t.Skip("requires a dedicated network namespace")
+	}
+	useTempRouteStores(t)
+	bad := filepath.Join(t.TempDir(), "dir")
+	if err := os.Mkdir(bad, 0700); err != nil {
+		t.Fatal(err)
+	}
+	StaticRoutesStorePath = bad
+	m := &RoutingManager{}
+	err := m.SavePolicyRule(PolicyRuleItem{ID: "r", Priority: 11300, Action: "blackhole", SrcAddress: "198.51.100.0/24"})
+	if err == nil {
+		t.Fatal("save should fail")
+	}
+	out, e := exec.Command("ip", "rule", "show").CombinedOutput()
+	if e != nil {
+		t.Fatal(e)
+	}
+	if strings.Contains(string(out), "11300:") {
+		t.Fatalf("kernel rule not rolled back: %s", out)
+	}
+	if len(m.policyRules) != 0 {
+		t.Fatal("memory not restored")
+	}
+}
+func TestTableRegistryFailureRestoresMemory(t *testing.T) {
+	useTempRouteStores(t)
+	old := RTTableFilePath
+	t.Cleanup(func() { RTTableFilePath = old })
+	RTTableFilePath = filepath.Join(t.TempDir(), "missing", "rt_tables")
+	m := &RoutingManager{}
+	if err := m.SaveTable(RoutingTableItem{Name: "test", TableID: 501}); err == nil {
+		t.Fatal("registry failure ignored")
+	}
+	if len(m.customTables) != 0 {
+		t.Fatalf("table not rolled back: %+v", m.customTables)
+	}
+}
+
+func TestTableRejectsReservedAndInvalidNames(t *testing.T) {
+	useTempRouteStores(t)
+	old := RTTableFilePath
+	t.Cleanup(func() { RTTableFilePath = old })
+	RTTableFilePath = filepath.Join(t.TempDir(), "rt_tables")
+	m := &RoutingManager{}
+	for _, table := range []RoutingTableItem{{Name: "main", TableID: 501}, {Name: "bad name", TableID: 502}, {Name: "bad\nentry", TableID: 503}, {Name: "test", TableID: 254}} {
+		if err := m.SaveTable(table); err == nil {
+			t.Fatalf("accepted table %+v", table)
+		}
+	}
+	if len(m.customTables) != 0 {
+		t.Fatal("invalid tables saved")
+	}
+}

internal/vpn/ppp_server.go

@@ -3,6 +3,7 @@ package vpn
 import (
 	"bufio"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"log"
 	"net"
@@ -234,12 +235,32 @@ func (m *PPPServerManager) loadAll() {
 }
 
 func (m *PPPServerManager) save(filename string, v any) error {
-	_ = os.MkdirAll(m.baseDir, 0755)
+	if err := os.MkdirAll(m.baseDir, 0755); err != nil {
+		return err
+	}
 	data, err := json.MarshalIndent(v, "", "  ")
 	if err != nil {
 		return err
 	}
-	return os.WriteFile(filepath.Join(m.baseDir, filename), data, 0644)
+	f, err := os.CreateTemp(m.baseDir, ".ppp-*.tmp")
+	if err != nil {
+		return err
+	}
+	name := f.Name()
+	defer os.Remove(name)
+	if err = f.Chmod(0600); err == nil {
+		_, err = f.Write(data)
+	}
+	if err == nil {
+		err = f.Sync()
+	}
+	if closeErr := f.Close(); err == nil {
+		err = closeErr
+	}
+	if err != nil {
+		return err
+	}
+	return os.Rename(name, filepath.Join(m.baseDir, filename))
 }
 
 // ======================== PPP Profiles CRUD ========================
@@ -295,16 +316,25 @@ func (m *PPPServerManager) SaveProfile(p *PPPProfile) error {
 	}
 
 	// Re-apply servers if their configured DefaultProfile was updated
+	var applyErrors []error
 	if m.l2tpServer.Enabled && m.l2tpServer.DefaultProfile == p.Name {
-		_ = m.applyL2TPServer()
+		if err := m.applyL2TPServer(); err != nil {
+			applyErrors = append(applyErrors, fmt.Errorf("L2TP: %w", err))
+		}
 	}
 	if m.pptpServer.Enabled && m.pptpServer.DefaultProfile == p.Name {
-		_ = m.applyPPTPServer()
+		if err := m.applyPPTPServer(); err != nil {
+			applyErrors = append(applyErrors, fmt.Errorf("PPTP: %w", err))
+		}
 	}
 	if m.pppoeServer.Enabled && m.pppoeServer.DefaultProfile == p.Name {
-		_ = m.applyPPPoEServer()
+		if err := m.applyPPPoEServer(); err != nil {
+			applyErrors = append(applyErrors, fmt.Errorf("PPPoE: %w", err))
+		}
+	}
+	if err := errors.Join(applyErrors...); err != nil {
+		return fmt.Errorf("profile saved but server reapply failed: %w", err)
 	}
-
 	return nil
 }
 
@@ -341,10 +371,43 @@ func (m *PPPServerManager) ListSecrets() []*PPPSecret {
 	return list
 }
 
+func (m *PPPServerManager) snapshotSecretsLocked() map[string]*PPPSecret {
+	before := make(map[string]*PPPSecret, len(m.secrets))
+	for id, secret := range m.secrets {
+		copy := *secret
+		before[id] = &copy
+	}
+	return before
+}
+func (m *PPPServerManager) saveSecretsLocked() error {
+	list := make([]*PPPSecret, 0, len(m.secrets))
+	for _, secret := range m.secrets {
+		list = append(list, secret)
+	}
+	return m.save("ppp_secrets.json", list)
+}
+func (m *PPPServerManager) commitSecretsLocked(before map[string]*PPPSecret) error {
+	if err := m.saveSecretsLocked(); err != nil {
+		m.secrets = before
+		return fmt.Errorf("saving PPP secrets: %w", err)
+	}
+	if err := m.syncSecretsToSystemLocked(); err != nil {
+		m.secrets = before
+		saveErr := m.saveSecretsLocked()
+		if saveErr != nil {
+			saveErr = fmt.Errorf("restoring saved PPP secrets: %w", saveErr)
+		}
+		syncErr := m.syncSecretsToSystemLocked()
+		if syncErr != nil {
+			syncErr = fmt.Errorf("reapplying previous PPP secrets: %w", syncErr)
+		}
+		return errors.Join(fmt.Errorf("applying PPP secrets: %w", err), saveErr, syncErr)
+	}
+	return nil
+}
 func (m *PPPServerManager) SaveSecret(s *PPPSecret) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
-
 	if s.Name == "" || s.Password == "" {
 		return fmt.Errorf("username and password are required")
 	}
@@ -357,49 +420,31 @@ func (m *PPPServerManager) SaveSecret(s *PPPSecret) error {
 	if s.ID == "" {
 		s.ID = fmt.Sprintf("secret-%d", time.Now().UnixNano())
 	}
-
-	m.secrets[s.ID] = s
-
-	var list []*PPPSecret
-	for _, sc := range m.secrets {
-		list = append(list, sc)
-	}
-	if err := m.save("ppp_secrets.json", list); err != nil {
-		return err
-	}
-
-	return m.syncSecretsToSystemLocked()
+	before := m.snapshotSecretsLocked()
+	copy := *s
+	m.secrets[s.ID] = &copy
+	return m.commitSecretsLocked(before)
 }
-
 func (m *PPPServerManager) SetSecretState(id string, disabled bool) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
-
-	s, ok := m.secrets[id]
+	secret, ok := m.secrets[id]
 	if !ok {
 		return fmt.Errorf("secret not found")
 	}
-	s.Disabled = disabled
-
-	var list []*PPPSecret
-	for _, sc := range m.secrets {
-		list = append(list, sc)
-	}
-	_ = m.save("ppp_secrets.json", list)
-	return m.syncSecretsToSystemLocked()
+	before := m.snapshotSecretsLocked()
+	secret.Disabled = disabled
+	return m.commitSecretsLocked(before)
 }
-
 func (m *PPPServerManager) DeleteSecret(id string) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
-
-	delete(m.secrets, id)
-	var list []*PPPSecret
-	for _, sc := range m.secrets {
-		list = append(list, sc)
+	if _, ok := m.secrets[id]; !ok {
+		return fmt.Errorf("secret not found")
 	}
-	_ = m.save("ppp_secrets.json", list)
-	return m.syncSecretsToSystemLocked()
+	before := m.snapshotSecretsLocked()
+	delete(m.secrets, id)
+	return m.commitSecretsLocked(before)
 }
 
 // Helper IP math utilities for dynamic pool allocation per profile
@@ -545,6 +590,8 @@ func (m *PPPServerManager) SetPPPoEServer(cfg PPPoEServerConfig) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
+	previous := m.pppoeServer
+
 	m.pppoeServer.Enabled = cfg.Enabled
 	if cfg.ServiceName != "" {
 		m.pppoeServer.ServiceName = cfg.ServiceName
@@ -563,8 +610,14 @@ func (m *PPPServerManager) SetPPPoEServer(cfg PPPoEServerConfig) error {
 		m.pppoeServer.MaxMRU = cfg.MaxMRU
 	}
 
-	_ = m.save("pppoe_server.json", m.pppoeServer)
-	return m.applyPPPoEServer()
+	if err := m.save("pppoe_server.json", m.pppoeServer); err != nil {
+		m.pppoeServer = previous
+		return fmt.Errorf("saving PPPoE configuration: %w", err)
+	}
+	if err := m.applyPPPoEServer(); err != nil {
+		return fmt.Errorf("PPPoE configuration saved but could not be applied: %w", err)
+	}
+	return nil
 }
 
 func (m *PPPServerManager) applyPPPoEServer() error {
@@ -623,7 +676,10 @@ default-mru
 default-asyncmap
 proxyarp
 `, dns1, dns2)
-	_ = os.WriteFile("/etc/ppp/pppoe-server-options", []byte(opts), 0644)
+	if err := os.WriteFile("/etc/ppp/pppoe-server-options", []byte(opts), 0644); err != nil {
+		m.pppoeServer.LastError = err.Error()
+		return fmt.Errorf("writing PPPoE options: %w", err)
+	}
 
 	// Launch pppoe-server
 	// pppoe-server -I <if> -L <local_ip> -R <start_ip> -N <count> -S <service> -O /etc/ppp/pppoe-server-options -X <pidfile>
@@ -649,6 +705,10 @@ proxyarp
 
 	time.Sleep(300 * time.Millisecond)
 	m.pppoeServer.Running = m.isProcessRunning(m.pppoePIDFile)
+	if !m.pppoeServer.Running {
+		m.pppoeServer.LastError = "PPPoE server did not become active"
+		return errors.New(m.pppoeServer.LastError)
+	}
 	m.pppoeServer.LastError = ""
 	return nil
 }
@@ -681,6 +741,8 @@ func (m *PPPServerManager) SetPPTPServer(cfg PPTPServerConfig) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
+	previous := m.pptpServer
+
 	m.pptpServer.Enabled = cfg.Enabled
 	if cfg.Port > 0 {
 		m.pptpServer.Port = cfg.Port
@@ -690,13 +752,22 @@ func (m *PPPServerManager) SetPPTPServer(cfg PPTPServerConfig) error {
 	}
 	m.pptpServer.RequireMPPE = cfg.RequireMPPE
 
-	_ = m.save("pptp_server.json", m.pptpServer)
-	return m.applyPPTPServer()
+	if err := m.save("pptp_server.json", m.pptpServer); err != nil {
+		m.pptpServer = previous
+		return fmt.Errorf("saving PPTP configuration: %w", err)
+	}
+	if err := m.applyPPTPServer(); err != nil {
+		return fmt.Errorf("PPTP configuration saved but could not be applied: %w", err)
+	}
+	return nil
 }
 
 func (m *PPPServerManager) applyPPTPServer() error {
 	if !m.pptpServer.Enabled {
-		_ = exec.Command("systemctl", "stop", "pptpd").Run()
+		if out, err := exec.Command("systemctl", "stop", "pptpd").CombinedOutput(); err != nil {
+			m.pptpServer.LastError = strings.TrimSpace(string(out))
+			return fmt.Errorf("stopping pptpd: %s: %w", m.pptpServer.LastError, err)
+		}
 		m.pptpServer.Running = false
 		m.pptpServer.LastError = ""
 		return nil
@@ -724,7 +795,10 @@ delegate
 localip %s
 remoteip %s
 `, localIP, remotePool)
-	_ = os.WriteFile("/etc/pptpd.conf", []byte(pptpdConf), 0644)
+	if err := os.WriteFile("/etc/pptpd.conf", []byte(pptpdConf), 0644); err != nil {
+		m.pptpServer.LastError = err.Error()
+		return fmt.Errorf("writing /etc/pptpd.conf: %w", err)
+	}
 
 	// Write /etc/ppp/pptpd-options
 	dns1 := "1.1.1.1"
@@ -760,13 +834,23 @@ novj
 novjccomp
 nologfd
 `, mppeRule, dns1, dns2)
-	_ = os.WriteFile("/etc/ppp/pptpd-options", []byte(pptpdOpts), 0644)
+	if err := os.WriteFile("/etc/ppp/pptpd-options", []byte(pptpdOpts), 0644); err != nil {
+		m.pptpServer.LastError = err.Error()
+		return fmt.Errorf("writing /etc/ppp/pptpd-options: %w", err)
+	}
 
-	_ = exec.Command("systemctl", "restart", "pptpd").Run()
+	if out, err := exec.Command("systemctl", "restart", "pptpd").CombinedOutput(); err != nil {
+		m.pptpServer.LastError = strings.TrimSpace(string(out))
+		return fmt.Errorf("restarting pptpd: %s: %w", m.pptpServer.LastError, err)
+	}
 	time.Sleep(300 * time.Millisecond)
 
 	out, _ := exec.Command("systemctl", "is-active", "pptpd").Output()
 	m.pptpServer.Running = strings.TrimSpace(string(out)) == "active"
+	if !m.pptpServer.Running {
+		m.pptpServer.LastError = "pptpd is not active"
+		return errors.New(m.pptpServer.LastError)
+	}
 	m.pptpServer.LastError = ""
 	return nil
 }
@@ -786,6 +870,8 @@ func (m *PPPServerManager) SetL2TPServer(cfg L2TPServerConfig) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
+	previous := m.l2tpServer
+
 	m.l2tpServer.Enabled = cfg.Enabled
 	if cfg.Port > 0 {
 		m.l2tpServer.Port = cfg.Port
@@ -798,15 +884,27 @@ func (m *PPPServerManager) SetL2TPServer(cfg L2TPServerConfig) error {
 	}
 	m.l2tpServer.IPSecSecret = cfg.IPSecSecret
 
-	_ = m.save("l2tp_server.json", m.l2tpServer)
-	return m.applyL2TPServer()
+	if err := m.save("l2tp_server.json", m.l2tpServer); err != nil {
+		m.l2tpServer = previous
+		return fmt.Errorf("saving L2TP configuration: %w", err)
+	}
+	if err := m.applyL2TPServer(); err != nil {
+		return fmt.Errorf("L2TP configuration saved but could not be applied: %w", err)
+	}
+	return nil
 }
 
 func (m *PPPServerManager) applyL2TPServer() error {
 	if !m.l2tpServer.Enabled {
 		// When disabled, don't stop xl2tpd completely as client may need it, just reload without LNS
-		_ = os.WriteFile("/etc/ppp/options.xl2tpd.server", []byte("# disabled\n"), 0644)
-		_ = exec.Command("systemctl", "reload-or-restart", "xl2tpd").Run()
+		if err := os.WriteFile("/etc/ppp/options.xl2tpd.server", []byte("# disabled\n"), 0644); err != nil {
+			m.l2tpServer.LastError = err.Error()
+			return fmt.Errorf("writing L2TP options: %w", err)
+		}
+		if out, err := exec.Command("systemctl", "reload-or-restart", "xl2tpd").CombinedOutput(); err != nil {
+			m.l2tpServer.LastError = strings.TrimSpace(string(out))
+			return fmt.Errorf("reloading xl2tpd: %s: %w", m.l2tpServer.LastError, err)
+		}
 		m.l2tpServer.Running = false
 		m.l2tpServer.LastError = ""
 		return nil
@@ -832,7 +930,10 @@ func (m *PPPServerManager) applyL2TPServer() error {
 	}
 
 	// Write /etc/xl2tpd/xl2tpd.conf with [lns default]
-	_ = os.MkdirAll("/etc/xl2tpd", 0755)
+	if err := os.MkdirAll("/etc/xl2tpd", 0755); err != nil {
+		m.l2tpServer.LastError = err.Error()
+		return fmt.Errorf("creating L2TP directory: %w", err)
+	}
 	xl2tpdConf := fmt.Sprintf(`[global]
 port = %d
 
@@ -846,7 +947,10 @@ name = l2tpd
 pppoptfile = /etc/ppp/options.xl2tpd.server
 length bit = yes
 `, m.l2tpServer.Port, remotePool, localIP)
-	_ = os.WriteFile("/etc/xl2tpd/xl2tpd.conf", []byte(xl2tpdConf), 0644)
+	if err := os.WriteFile("/etc/xl2tpd/xl2tpd.conf", []byte(xl2tpdConf), 0644); err != nil {
+		m.l2tpServer.LastError = err.Error()
+		return fmt.Errorf("writing /etc/xl2tpd/xl2tpd.conf: %w", err)
+	}
 
 	// Write /etc/ppp/options.xl2tpd.server
 	dns1 := "1.1.1.1"
@@ -872,7 +976,10 @@ mru 1400
 proxyarp
 nodefaultroute
 `, dns1, dns2)
-	_ = os.WriteFile("/etc/ppp/options.xl2tpd.server", []byte(opts), 0644)
+	if err := os.WriteFile("/etc/ppp/options.xl2tpd.server", []byte(opts), 0644); err != nil {
+		m.l2tpServer.LastError = err.Error()
+		return fmt.Errorf("writing /etc/ppp/options.xl2tpd.server: %w", err)
+	}
 
 	// Clean restart of xl2tpd to avoid stale PID locks
 	_ = exec.Command("systemctl", "stop", "xl2tpd").Run()
@@ -880,11 +987,18 @@ nodefaultroute
 	_ = exec.Command("pkill", "-9", "-x", "xl2tpd").Run()
 	_ = os.Remove("/var/run/xl2tpd.pid")
 	_ = os.Remove("/run/xl2tpd.pid")
-	_ = exec.Command("systemctl", "start", "xl2tpd").Run()
+	if out, err := exec.Command("systemctl", "start", "xl2tpd").CombinedOutput(); err != nil {
+		m.l2tpServer.LastError = strings.TrimSpace(string(out))
+		return fmt.Errorf("starting xl2tpd: %s: %w", m.l2tpServer.LastError, err)
+	}
 	time.Sleep(300 * time.Millisecond)
 
 	out, _ := exec.Command("systemctl", "is-active", "xl2tpd").Output()
 	m.l2tpServer.Running = strings.TrimSpace(string(out)) == "active"
+	if !m.l2tpServer.Running {
+		m.l2tpServer.LastError = "xl2tpd is not active"
+		return errors.New(m.l2tpServer.LastError)
+	}
 	m.l2tpServer.LastError = ""
 	return nil
 }
@@ -904,6 +1018,8 @@ func (m *PPPServerManager) SetIPSecServer(cfg IPSecServerConfig) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
+	previous := m.ipsecServer
+
 	m.ipsecServer.Enabled = cfg.Enabled
 	if cfg.PoolAddress != "" {
 		m.ipsecServer.PoolAddress = cfg.PoolAddress
@@ -918,21 +1034,42 @@ func (m *PPPServerManager) SetIPSecServer(cfg IPSecServerConfig) error {
 		m.ipsecServer.DNSServers = cfg.DNSServers
 	}
 
-	_ = m.save("ipsec_server.json", m.ipsecServer)
-	return m.applyIPSecServer()
+	if err := m.save("ipsec_server.json", m.ipsecServer); err != nil {
+		m.ipsecServer = previous
+		return fmt.Errorf("saving IPSec configuration: %w", err)
+	}
+	if err := m.applyIPSecServer(); err != nil {
+		return fmt.Errorf("IPSec configuration saved but could not be applied: %w", err)
+	}
+	return nil
 }
 
 func (m *PPPServerManager) applyIPSecServer() error {
 	confFile := "/etc/swanctl/conf.d/rw-server.conf"
 	if !m.ipsecServer.Enabled {
-		_ = os.Remove(confFile)
-		_ = exec.Command("swanctl", "--load-all").Run()
+		if err := os.Remove(confFile); err != nil && !os.IsNotExist(err) {
+			return fmt.Errorf("removing IPsec server configuration: %w", err)
+		}
+		if out, err := exec.Command("swanctl", "--load-all").CombinedOutput(); err != nil {
+			return fmt.Errorf("reloading IPsec configuration: %w: %s", err, strings.TrimSpace(string(out)))
+		}
+		out, err := exec.Command("swanctl", "--list-conns").CombinedOutput()
+		if err != nil {
+			return fmt.Errorf("checking IPsec connections: %w: %s", err, strings.TrimSpace(string(out)))
+		}
+		for _, line := range strings.Split(string(out), "\n") {
+			if strings.HasPrefix(strings.TrimSpace(line), "rw-server:") {
+				return fmt.Errorf("IPsec server connection remains loaded after reload")
+			}
+		}
 		m.ipsecServer.Running = false
 		m.ipsecServer.LastError = ""
 		return nil
 	}
 
-	_ = os.MkdirAll("/etc/swanctl/conf.d", 0755)
+	if err := os.MkdirAll("/etc/swanctl/conf.d", 0755); err != nil {
+		return fmt.Errorf("creating IPsec configuration directory: %w", err)
+	}
 	swanConf := fmt.Sprintf(`connections {
     rw-server {
         local_addrs = %%any

internal/vpn/ppp_server_test.go

@@ -1,6 +1,7 @@
 package vpn
 
 import (
+	"encoding/json"
 	"os"
 	"path/filepath"
 	"strings"
@@ -122,3 +123,109 @@ func TestSyncSecretsReportsWriteFailure(t *testing.T) {
 		t.Fatal("expected write error")
 	}
 }
+
+func TestSecretPersistenceFailureDoesNotChangeState(t *testing.T) {
+	root := t.TempDir()
+	bad := filepath.Join(root, "not-dir")
+	if err := os.WriteFile(bad, []byte("x"), 0600); err != nil {
+		t.Fatal(err)
+	}
+	original := &PPPSecret{ID: "s", Name: "alice", Password: "p", Disabled: false}
+	mgr := &PPPServerManager{baseDir: bad, pppDir: root, secrets: map[string]*PPPSecret{"s": original}, profiles: map[string]*PPPProfile{}}
+	if err := mgr.SetSecretState("s", true); err == nil {
+		t.Fatal("expected persistence error")
+	}
+	if mgr.secrets["s"].Disabled {
+		t.Fatal("disabled state changed despite save failure")
+	}
+	if err := mgr.DeleteSecret("s"); err == nil {
+		t.Fatal("expected deletion persistence error")
+	}
+	if _, ok := mgr.secrets["s"]; !ok {
+		t.Fatal("secret removed despite save failure")
+	}
+}
+func TestSecretApplyFailureRestoresStoredState(t *testing.T) {
+	root := t.TempDir()
+	ppp := filepath.Join(root, "ppp")
+	if err := os.Mkdir(ppp, 0700); err != nil {
+		t.Fatal(err)
+	}
+	mgr := &PPPServerManager{baseDir: filepath.Join(root, "config"), pppDir: ppp, secrets: map[string]*PPPSecret{}, profiles: map[string]*PPPProfile{}}
+	secret := &PPPSecret{ID: "s", Name: "alice", Password: "p"}
+	if err := mgr.SaveSecret(secret); err != nil {
+		t.Fatal(err)
+	}
+	mgr.pppDir = filepath.Join(root, "missing")
+	if err := mgr.SetSecretState("s", true); err == nil {
+		t.Fatal("expected apply failure")
+	}
+	if mgr.secrets["s"].Disabled {
+		t.Fatal("memory not restored")
+	}
+	data, err := os.ReadFile(filepath.Join(mgr.baseDir, "ppp_secrets.json"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	var saved []PPPSecret
+	if err = json.Unmarshal(data, &saved); err != nil {
+		t.Fatal(err)
+	}
+	if len(saved) != 1 || saved[0].Disabled {
+		t.Fatalf("saved state not restored: %+v", saved)
+	}
+	info, err := os.Stat(filepath.Join(mgr.baseDir, "ppp_secrets.json"))
+	if err != nil || info.Mode().Perm() != 0600 {
+		t.Fatalf("permissions: %v %v", info, err)
+	}
+}
+func TestServerSaveFailureDoesNotStartOrChangeState(t *testing.T) {
+	root := t.TempDir()
+	bad := filepath.Join(root, "file")
+	if err := os.WriteFile(bad, []byte("x"), 0600); err != nil {
+		t.Fatal(err)
+	}
+	mgr := &PPPServerManager{baseDir: bad}
+	for _, test := range []struct {
+		name    string
+		run     func() error
+		enabled func() bool
+	}{
+		{"PPPoE", func() error { return mgr.SetPPPoEServer(PPPoEServerConfig{Enabled: true}) }, func() bool { return mgr.pppoeServer.Enabled }},
+		{"PPTP", func() error { return mgr.SetPPTPServer(PPTPServerConfig{Enabled: true}) }, func() bool { return mgr.pptpServer.Enabled }},
+		{"L2TP", func() error { return mgr.SetL2TPServer(L2TPServerConfig{Enabled: true}) }, func() bool { return mgr.l2tpServer.Enabled }},
+		{"IPSec", func() error { return mgr.SetIPSecServer(IPSecServerConfig{Enabled: true}) }, func() bool { return mgr.ipsecServer.Enabled }},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			if err := test.run(); err == nil {
+				t.Fatal("expected save error")
+			}
+			if test.enabled() {
+				t.Fatal("state not restored")
+			}
+		})
+	}
+}
+
+func TestIsolatedPPPDaemonFailuresAreReported(t *testing.T) {
+	if os.Getenv("DEBTIK_TEST_ISOLATED_PPP") != "1" {
+		t.Skip("requires isolated mounts and process command fakes")
+	}
+	bin := t.TempDir()
+	t.Setenv("PATH", bin)
+	systemctl := "#!/bin/sh\ncase \"$1\" in stop) exit 0 ;; is-active) echo inactive; exit 3 ;; *) echo daemon-start-failed >&2; exit 1 ;; esac\n"
+	if err := os.WriteFile(filepath.Join(bin, "systemctl"), []byte(systemctl), 0755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(bin, "pkill"), []byte("#!/bin/sh\nexit 0\n"), 0755); err != nil {
+		t.Fatal(err)
+	}
+	m := &PPPServerManager{baseDir: t.TempDir(), profiles: map[string]*PPPProfile{}, secrets: map[string]*PPPSecret{}, pppoePIDFile: filepath.Join(t.TempDir(), "pppoe.pid")}
+	m.initDefaults()
+	if err := m.SetPPTPServer(PPTPServerConfig{Enabled: true}); err == nil || !strings.Contains(err.Error(), "restarting pptpd") {
+		t.Fatalf("PPTP failure ignored: %v", err)
+	}
+	if err := m.SetL2TPServer(L2TPServerConfig{Enabled: true}); err == nil || !strings.Contains(err.Error(), "starting xl2tpd") {
+		t.Fatalf("L2TP failure ignored: %v", err)
+	}
+}

internal/web/static/app.js

@@ -7982,6 +7982,8 @@ async function deletePolicyRule(id) {
     if (res.ok) {
       showToast('Rule Removed', 'Policy rule deleted', 'success');
       loadRoutes();
+    } else {
+      throw new Error(await responseError(res));
     }
   } catch (err) {
     showToast('Error', err.message, 'error');