fix(armor): a reboot is not someone switching the product off

The boot armor never armed on the router it shipped to. procd runs the
K-links on the way down with the action `shutdown`, and stop_service
classified actions with an OPEN default:

    case $action in restart|reload) keep;; *) DISARM;; esac

`shutdown` matched nobody, fell into `*`, and deleted the arm token. The
mechanism erased itself at exactly the transition it exists for, so every
boot found nothing to load. Measured on the live router, one minute apart
across a reboot:

    13:28  /etc/shater/boot.nft present
    ----   reboot
    18s    at_S22: NO_TABLE  armor_file=NO_FILE

It did not fail every time, which is worse than failing always: on the way
down `rm` from this script raced a `SaveBootArmor` driven by the ifdown
hotplug storm, and whichever landed second won. Two reboots on the same box
an hour apart gave opposite outcomes.

Both lists are now positive and CLOSED. Only `stop` disarms; only
`restart`/`reload` hand off. An action nobody thought of changes nothing,
so the default now fails toward a boot that arms when it need not have --
recoverable in the second before the daemon applies, and still gated by
shater-armor's four state refusals. The old default failed toward the
plaintext window the feature was built to close.

Also closed, found while proving the above:

  * Every restart left the LAN in the clear for 80-90ms. The exit path was
    `Teardown(); armOnExit()`, and TeardownNft DELETES the table -- two nft
    transactions with no `inet shater` between them, leaving fw4's
    `lan -> wan ACCEPT` as the only policy. Every restart, every LuCI Save
    & Apply. TeardownExiting arms first under the apply lock and skips the
    delete iff a plane actually went in; RenderHoldNft is one `nft -f` that
    REPLACES the table, so the kernel never observes its absence.
    35k-sample instrument: 7 and 6 no-table hits before, 0 across three
    runs after.

  * SaveBootArmor fsynced the payload but not the directory, so a power cut
    could lose the rename that publishes it -- a boot with no armor and no
    error anywhere.

`stop` now also reads rc.d state, so a package transaction that stops the
service is not mistaken for a person switching it off. This one does not
reproduce on apk (it runs no pre-upgrade script and never calls prerm on an
upgrade; verified with apk adbdump and 245k samples across a real reinstall)
-- it is one returning opkg lane away from being live, and the removal case
is now stated rather than implicit.

Both new tests are mutation-checked: reverting the predicate fails naming
`shutdown`; reverting the teardown fails with `did [arm delete], want [arm]`.
initscript_test.go sources the SHIPPED shell and calls the real predicates
with every action procd uses -- a comment claiming `shutdown` was handled is
what shipped last time.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BHw89tdWddzhjUc4bAH4tS

openwrt/shater-core/files/etc/init.d/shater

@@ -33,24 +33,30 @@
 #     be running. `start` raises ACTIVE_FLAG, `stop` clears it; hotplug/cron
 #     reconcile ONLY while the flag is up, so an admin `stop` STICKS — no
 #     background actor may resurrect interception behind a stopped daemon.
-#   * BEING REPLACED IS NOT BEING SWITCHED OFF. `restart`, `reload` (which is
-#     stop+start, i.e. every LuCI Save & Apply) and every package upgrade all run
-#     through `stop`, and the daemon's SIGTERM teardown removes the fail-closed
-#     table unconditionally — it does not consult kill_switch at all. Between that
-#     teardown and the successor's first apply the init GUARANTEES a gap: it waits
-#     for the old process to exit (shater_wait_stopped), then runs `shaterd
-#     migrate`, then starts a daemon that still has to build an engine. So a
-#     restart is announced with RESTART_FLAG, which tells the outgoing daemon to
-#     leave the fail-closed holding plane behind instead of bare routing. A real
-#     `stop` raises no flag and therefore still means what it says.
+#   * BEING REPLACED IS NOT BEING SWITCHED OFF. `restart` and `reload` (which is
+#     stop+start, i.e. every LuCI Save & Apply) both run through `stop`, and the
+#     daemon's SIGTERM teardown removes the fail-closed table unconditionally — it
+#     does not consult kill_switch at all. Between that teardown and the
+#     successor's first apply the init GUARANTEES a gap: it waits for the old
+#     process to exit (shater_wait_stopped), then runs `shaterd migrate`, then
+#     starts a daemon that still has to build an engine. So a restart is announced
+#     with RESTART_FLAG, which tells the outgoing daemon to leave the fail-closed
+#     holding plane STANDING — apply.TeardownExiting swaps it in with one nft
+#     transaction and then skips the delete, so the table is never absent, not even
+#     for the 80-90 ms the old arm-after-teardown order measured. A real `stop`
+#     raises no flag and therefore still means what it says.
+#     (A package UPGRADE does not come through here at all on apk v3: shater-core's
+#     script table is post-install / pre-deinstall / post-upgrade, with no
+#     pre-upgrade, so default_prerm — and its `stop` — runs only on REMOVAL.)
 #   * The FAIL-CLOSED PLANE MUST ALSO EXIST BEFORE THIS SCRIPT DOES. START=99 is
 #     after fw4 (19) and netifd (20), so at every boot the LAN forwards to the WAN
 #     in the clear for as long as it takes procd to decompress the daemon off
 #     flash and get an engine up. /etc/init.d/shater-armor (START=21) loads
 #     BOOT_ARMOR — a copy of the holding plane the daemon persists on every apply
-#     — to close that window. This script owns the DISARM half: a deliberate
-#     `stop`, or a missing daemon binary, removes the armor so it cannot outlive
-#     the product it protects.
+#     — to close that window. This script owns the DISARM half, and it owns it
+#     with a CLOSED LIST: an operator's `stop`, or a removal, and nothing else.
+#     Powering the box down must not — `shutdown` reaches stop_service too, and it
+#     is not a person switching the product off (see shater_stop_disarms).
 #   * The engine must never be permanently abandoned while interception stands:
 #     respawn retries are infinite (procd never gives up); a sustained-dead
 #     daemon is additionally escalated by the shater-cron watchdog.
@@ -84,20 +90,150 @@ STOP_WAIT_SECS=40
 
 # WHICH ACTION rc.common was invoked with, frozen at source time.
 #
-# rc.common sets `action=${2:-help}` before it sources this file, and every action
-# then runs as a function in THAT SAME shell — so `stop_service` can see whether it
-# was reached by `stop` or as the first half of `restart`/`reload`. That is the one
-# distinction procd itself does not expose (`restart` is literally `stop; start`,
-# and stop_service is called identically by both).
+# rc.common does, in this order:
+#     initscript=$1; action=${2:-help}; shift 2; ...; . "$initscript"; $action "$@"
+# so `action` is ALREADY assigned when this file is sourced, and every action then
+# runs as a function in THAT SAME shell. MEASURED on the target (ImmortalWrt
+# 25.12.1 r37978) with a throwaway probe init script, not read off documentation:
+#
+#     /etc/init.d/X restart  ->  stop_service action=[restart], start_service [restart]
+#     /etc/init.d/X stop     ->  stop_service action=[stop]
+#     /etc/init.d/X reload   ->  reload_service action=[reload]
+#     `reboot`               ->  stop_service action=[SHUTDOWN]     <-- see below
+#     the boot after it      ->  start_service action=[boot]
+#
+# A previous probe reported this variable EMPTY and the emptiness was written up as
+# the defect. It was the probe: `sh -x /etc/init.d/shater restart` bypasses the
+# `#!/bin/sh /etc/rc.common` shebang, so rc.common never runs, never assigns
+# `action`, and the variable reads empty no matter what this file does.
 #
 # Frozen into our own variable because `action` is a short, generic name that other
 # framework helpers also use as a local; a snapshot taken before any function runs
-# cannot be shadowed later. An EMPTY or unexpected value degrades to "real stop",
-# which is the pre-existing behaviour and the safe direction to be wrong in: it
-# costs a plaintext window on restart, where the other default would leave a
-# deliberately stopped router blocked.
+# cannot be shadowed later.
 SHATER_RC_ACTION="$action"
 
+# --- what an action MEANS --------------------------------------------------
+#
+# THE BUG THESE TWO PREDICATES REPLACE (v0.2.17, measured on the live router).
+# The old stop_service was `case $action in restart|reload) keep;; *) DISARM;; esac`
+# — an open default that swept up every action nobody had enumerated. `reboot` is
+# one of them: procd runs the K-links with the action `shutdown`, so the shutdown
+# path deleted the arm token on the way down and the next boot had nothing to load.
+# The mechanism destroyed itself at exactly the moment it exists for. Instrument
+# reading from the router, one minute apart across a reboot:
+#
+#     13:28  /etc/shater/boot.nft present
+#     ----   reboot (stop_service action=[shutdown] -> old `*` branch -> rm)
+#     18s    at_S22: NO_TABLE  armor_file=NO_FILE
+#
+# So both lists below are POSITIVE and CLOSED. An action nobody thought about —
+# `shutdown` above all, but also whatever a future procd invents — falls through
+# both and changes nothing. The default now fails in the recoverable direction: at
+# worst a boot arms when it need not have, which costs the second before the daemon
+# applies and is still gated by shater-armor's own four state refusals. The old
+# default failed in the direction of the plaintext window the feature was built to
+# close.
+#
+# They are predicates rather than an inline `case` so the test gate can execute the
+# real thing: it sources THIS FILE in /bin/sh and calls them with every action procd
+# actually uses (shater/cmd/shaterd/initscript_test.go). A comment claiming
+# `shutdown` is handled is what shipped last time.
+
+# True only for the ONE action that means "the operator switched the product off".
+# Deliberately not `shutdown`: powering a router down is not turning a feature off.
+#
+# NOT sufficient on its own — see shater_stop_disarms. `stop` is also how the
+# package manager's plumbing reaches us, and a package manager is not a person.
+shater_action_disarms() {
+	case "$1" in
+		stop) return 0 ;;
+		*)    return 1 ;;
+	esac
+}
+
+# Is a package manager in the middle of a transaction RIGHT NOW?
+#
+# This is a state, read at the moment the decision is made, exactly like
+# shater-armor's four refusals — not a record of an event. The same question is
+# already asked (for the same reason: prerm/postinst plumbing is not a user
+# action) by the detached bring-up in /etc/uci-defaults/30_shater-core.
+shater_pkg_transaction() {
+	pidof apk >/dev/null 2>&1 && return 0
+	pidof opkg >/dev/null 2>&1 && return 0
+	return 1
+}
+
+# Is the main service still enabled at boot? Same glob, and for the same reason,
+# as shater-armor's own check: `/etc/init.d/shater enabled` would source procd.sh
+# and take a blocking flock, which is not something to do from inside a package
+# manager's transaction.
+shater_rc_enabled() {
+	local f
+	for f in /etc/rc.d/S[0-9][0-9]shater; do
+		[ -e "$f" ] && return 0
+	done
+	return 1
+}
+
+# THE ACTUAL DISARM DECISION.
+#   $1 = action
+#   $2 = 1 when a package transaction is in flight
+#   $3 = 1 when the service is still enabled in rc.d
+# All three are passed in rather than read inside, so the gate can drive every
+# combination without a package manager or an /etc/rc.d.
+#
+# WHY IT IS NOT JUST THE ACTION. base-files' default_prerm runs, in this order:
+#
+#     if [ "$PKG_UPGRADE" != "1" ]; then "$i" disable; fi
+#     "$i" stop
+#
+# so a package manager reaches stop_service wearing the operator's clothes. Two
+# different intentions arrive as the same action, and the difference between them
+# is readable at the moment of the decision:
+#
+#   REMOVAL  — prerm has ALREADY run `disable`, so S99shater is gone. The product
+#              is going away; the armor goes with it. (It is belt-and-braces even
+#              so: shater-armor refuses to arm without that symlink, and the whole
+#              init script is about to be deleted anyway.)
+#   REPLACED — the service is still enabled, so something intends to bring it
+#              back. That is not an operator switching anything off, and deleting
+#              the armor here would leave the next boot unprotected. "The next
+#              apply will rewrite it" is not an answer: the armor exists precisely
+#              to cover a reboot, and a reboot between an update and the first
+#              apply is how this product is deployed.
+#
+# MEASURED, because the paragraph above is about a path I got wrong once already.
+# On THIS target (apk-tools 3.0.5, ImmortalWrt 25.12.1) shater-core's script table
+# is post-install / pre-deinstall / post-upgrade, with NO pre-upgrade — so an apk
+# UPGRADE never executes default_prerm and never calls `stop` at all. Verified with
+# a real `apk fix --reinstall shater-core` while sampling the armor file: 245 625
+# samples, zero disappearances, even with this guard mutated off. The upgrade half
+# of this predicate is therefore defence-in-depth for a shape that is one
+# `pre-upgrade` script (or a returning opkg lane) away, NOT a fix for an observed
+# failure. The removal half is live today.
+shater_stop_disarms() {
+	shater_action_disarms "$1" || return 1
+	# No package manager involved => a person typed it. The escape hatch must work.
+	[ "$2" = "1" ] || return 0
+	# A package transaction that has NOT disabled the service is replacing it.
+	[ "$3" = "1" ] && return 1
+	return 0
+}
+
+# True when a successor is coming, so the outgoing daemon should leave the
+# fail-closed holding plane standing instead of removing it.
+#
+# `shutdown` is deliberately NOT a handoff either: nothing is coming, and the
+# kernel that would hold the plane is going away with it. Leaving the flag down
+# there also keeps the marker's meaning exact — it says "you are being replaced",
+# and at shutdown nothing is.
+shater_action_handoff() {
+	case "$1" in
+		restart|reload) return 0 ;;
+		*)              return 1 ;;
+	esac
+}
+
 # --- helpers ---------------------------------------------------------------
 
 # True only when the stack is explicitly enabled in UCI.
@@ -125,10 +261,14 @@ shater_mark_restart() {
 shater_clear_restart() { rm -f "$RESTART_FLAG"; }
 
 # Remove the persisted boot armor, so the LAN is NOT blocked at the next boot
-# before the daemon starts. Called when the operator stops the service and when
-# the daemon binary is gone — in both cases nothing is going to come along and
-# replace the armor with a real data plane, and a kill switch with nothing behind
-# it is just a brick.
+# before the daemon starts. Called from exactly two places, both of which are a
+# statement about the PRODUCT rather than about this process: an operator typing
+# `stop`, and a daemon binary that is no longer on the box. In neither case is
+# anything going to come along and replace the armor with a real data plane, and a
+# kill switch with nothing behind it is just a brick.
+#
+# NOT called on the shutdown path. That is the whole fix — see
+# shater_action_disarms.
 shater_disarm_boot() { rm -f "$BOOT_ARMOR"; }
 
 # Echo the pid of a LIVE `shaterd run`, or fail. The pidfile is written by the
@@ -290,30 +430,42 @@ start_service() {
 stop_service() {
 	# Say WHY we are stopping before procd sends the signal, because the daemon
 	# cannot tell from the signal alone and the answer changes what it leaves in
-	# the kernel:
+	# the kernel. Two INDEPENDENT questions, and the old code conflated them into
+	# one two-armed `case` whose else-branch answered both wrongly for `shutdown`:
 	#
-	#   restart / reload  -> a successor is coming. Raise RESTART_FLAG so the
-	#                        outgoing daemon replaces its data plane with the
-	#                        fail-closed HOLDING plane instead of removing it. The
-	#                        gap until the successor applies is not a moment: this
-	#                        script waits out the old process, runs `shaterd
-	#                        migrate`, then starts a daemon that must build an
-	#                        engine — all of it, until now, with `lan -> wan
-	#                        ACCEPT` and nothing else.
-	#   anything else     -> a deliberate `stop`. Everything comes down, and the
-	#                        boot armor goes with it so the next boot does not
-	#                        quietly reinstate what the operator just switched off.
-	#                        An admin `stop` has to STICK; that is the same rule
-	#                        ACTIVE_FLAG has always enforced for hotplug/cron.
-	case "$SHATER_RC_ACTION" in
-		restart|reload)
-			shater_mark_restart
-			;;
-		*)
-			shater_clear_restart
-			shater_disarm_boot
-			;;
-	esac
+	#   1. IS A SUCCESSOR COMING (this process only)?  restart / reload.
+	#      Raise RESTART_FLAG so the outgoing daemon replaces its data plane with
+	#      the fail-closed HOLDING plane instead of removing it. The gap until the
+	#      successor applies is not a moment: this script waits out the old
+	#      process, runs `shaterd migrate`, then starts a daemon that must build an
+	#      engine — all of it, before this flag existed, with `lan -> wan ACCEPT`
+	#      and nothing else.
+	#
+	#   2. IS THE PRODUCT BEING SWITCHED OFF (across boots)?  `stop` — and only
+	#      `stop`, and only when a PERSON is behind it (shater_stop_disarms; the
+	#      package manager reaches us through `stop` too). Then the boot armor goes
+	#      with it, so the next boot does not quietly reinstate what the operator
+	#      just switched off — the same rule ACTIVE_FLAG has always enforced for
+	#      hotplug/cron.
+	#
+	# `shutdown` answers NO to both, which is the defect this replaced: a reboot is
+	# not a successor and it is certainly not an operator switching the product off.
+	# It is the boot the armor exists for. An upgrade answers NO to the second for
+	# the same kind of reason.
+	if shater_action_handoff "$SHATER_RC_ACTION"; then
+		shater_mark_restart
+	else
+		shater_clear_restart
+	fi
+	local in_pkg=0 rc_en=0
+	shater_pkg_transaction && in_pkg=1
+	shater_rc_enabled && rc_en=1
+	if shater_stop_disarms "$SHATER_RC_ACTION" "$in_pkg" "$rc_en"; then
+		shater_disarm_boot
+	elif [ "$in_pkg" = "1" ] && shater_action_disarms "$SHATER_RC_ACTION"; then
+		_slog -p daemon.info \
+			"stop came from a package transaction that left the service enabled — keeping the boot armor, so being replaced cannot leave the next boot unprotected"
+	fi
 
 	# Drop the live-flag FIRST so a concurrent hotplug/cron tick cannot rebuild
 	# what we are about to tear down. procd then sends SIGTERM to `shaterd run`,

openwrt/shater-core/files/etc/init.d/shater-armor

@@ -39,12 +39,24 @@
 #
 # THE ESCAPE HATCHES (a kill switch that cannot be switched off is a brick)
 #
+# These are STATE checks, evaluated here, at the moment of arming — not a record
+# of something that happened on the way down. That distinction is the whole
+# lesson of v0.2.17: the arm token was deleted by an EVENT on the shutdown path
+# ("this looks like a stop"), and since `reboot` also runs the K-links, the
+# mechanism reliably erased itself on the one transition it was built for. An
+# event on the way down cannot be trusted to describe the world on the way up; a
+# question asked on the way up can be.
+#
 #   * $ARMOR only exists while the daemon's last applied config was BOTH enabled
-#     and fail-closed. `globals.enabled=0`, `kill_switch=open` and a deliberate
-#     `/etc/init.d/shater stop` each remove it.
+#     and fail-closed. `globals.enabled=0` and `kill_switch=open` each remove it
+#     at the next apply, and an operator typing `/etc/init.d/shater stop` removes
+#     it there and then. Powering the box off does NOT.
 #   * We refuse to arm when the main service is disabled in rc.d, or when the
 #     daemon binary is gone — in either case nothing would ever come along to
-#     replace the armor with a real data plane.
+#     replace the armor with a real data plane. These two are what makes a
+#     genuinely uninstalled/disabled product safe REGARDLESS of what the file
+#     says, which is why they are checked here rather than trusted to have been
+#     acted on earlier.
 #   * We refuse to arm when UCI can be read AND says the stack is disabled. A
 #     config that cannot be read is NOT a refusal: that case is precisely why the
 #     armor is a file rather than a query.
@@ -52,6 +64,12 @@
 #     hook, to the router's own addresses) stay reachable. The operator can always
 #     get in and undo this.
 #
+# Note what a bare `/etc/init.d/shater stop` does NOT mean: it does not survive a
+# reboot, because S99shater is still linked and procd starts the daemon again. So
+# "stopped" is not a durable off-state and this script must not be designed as if
+# it were — the durable ones are `disable` (no S??shater) and `globals.enabled=0`,
+# and those are the two refusals above.
+#
 # busybox ash only — no bashisms.
 
 START=21          # after firewall (19) and network (20), long before shater (99)

shater/apply/apply.go

@@ -805,6 +805,10 @@ var applyHoldNft = netplane.ApplyNft
 var (
 	tableExists      = netplane.TableExists
 	bootArmorPresent = netplane.BootArmorPresent
+	// teardownNft is a seam for the same reason: whether the table is REMOVED or
+	// REPLACED on the way out is the whole of the restart-gap fix, and it can
+	// otherwise only be observed on a router with a real nft.
+	teardownNft = netplane.TeardownNft
 )
 
 // Holding reports whether forwarded LAN traffic is currently being BLOCKED by a
@@ -967,7 +971,42 @@ func (a *Applier) Reconcile() (changed bool, err error) {
 
 // Teardown is the honest teardown: engine.Close + netplane routing/nft teardown +
 // clear ACTIVE_FLAG, under the flock. Safe to call when nothing is up (idempotent).
-func (a *Applier) Teardown() error {
+//
+// This is the "everything goes" form — the operator disabled the stack or stopped
+// the service. A process that is being REPLACED wants TeardownExiting instead.
+func (a *Applier) Teardown() error { return a.teardown(nil) }
+
+// TeardownExiting is Teardown for a daemon that is going away, with the one
+// ordering that never leaves the LAN uncovered.
+//
+// THE GAP THIS CLOSES (MEASURED on the stand: 80-90 ms, twice). The exit path used
+// to be `Teardown(); armOnExit()` — TeardownNft DELETED the table, and only then
+// was the fail-closed holding plane installed. Two nft transactions, and between
+// them the `inet shater` table does not exist at all, so fw4's `lan -> wan ACCEPT`
+// is the only policy on the box and the whole LAN forwards in the clear. That is
+// not a boot-time window: it is every `restart`, every `reload_service` (i.e.
+// every LuCI Save & Apply) and every package upgrade. The width is two `nft`
+// invocations, so it does NOT grow with the engine — eng.Close runs before the
+// table is touched — but it is the whole LAN, in the clear, every time.
+//
+// The comment that used to sit on the call site — "AFTER the teardown, never
+// before: Teardown deletes the table, so a plane installed first would simply be
+// removed again" — described the mechanism correctly and drew the wrong conclusion
+// from it: the answer is not to arm later, it is to stop deleting.
+//
+// So arm FIRST and then skip the delete. RenderHoldNft's output is a single
+// `nft -f` script that opens with `table inet shater` / `delete table inet shater`
+// / `table inet shater { ... }` — one netlink transaction, in which the table is
+// REPLACED rather than removed and re-added. The kernel never observes its
+// absence, so a sampler cannot either.
+//
+// arm reports whether it actually installed a plane. When it did NOT — a real
+// `stop`, or a handoff with kill_switch=open, where fail-open is the operator's
+// documented choice — the table is removed exactly as before. "Keep the table"
+// therefore follows from "a plane is standing", never from the caller's intent.
+func (a *Applier) TeardownExiting(arm func() bool) error { return a.teardown(arm) }
+
+func (a *Applier) teardown(arm func() bool) error {
 	release, err := lockExclusive()
 	if err != nil {
 		return err
@@ -976,6 +1015,15 @@ func (a *Applier) Teardown() error {
 	a.mu.Lock()
 	defer a.mu.Unlock()
 
+	// Install the successor plane BEFORE anything is dismantled, and do it while
+	// still holding the apply lock, so a concurrent apply cannot slip between the
+	// swap and the teardown. arm must not call back into the Applier (cmd/shaterd's
+	// armOnExit goes straight to model + netplane) or this deadlocks.
+	kept := false
+	if arm != nil {
+		kept = arm()
+	}
+
 	// Stop the observatory BEFORE closing the engine, and wait for an in-flight
 	// tick: a teardown must not leave probe dials racing a box that is going away.
 	a.eng.StopObservatory()
@@ -998,8 +1046,14 @@ func (a *Applier) Teardown() error {
 	if err := netplane.TeardownRouting(m); err != nil && firstErr == nil {
 		firstErr = err
 	}
-	if err := netplane.TeardownNft(); err != nil && firstErr == nil {
-		firstErr = err
+	// The policy routing above is safe to remove either way: the holding plane is a
+	// single `forward` chain of accepts and drops and consults no routing table, so
+	// it keeps working with the ip rules gone. The TABLE is the one thing that must
+	// not be removed out from under it.
+	if !kept {
+		if err := teardownNft(); err != nil && firstErr == nil {
+			firstErr = err
+		}
 	}
 	// Put the per-ingress-iface knobs back the way we found them. With the table
 	// and the policy routing gone, a lingering accept_local=1 / rp_filter=0 on a
@@ -1011,7 +1065,11 @@ func (a *Applier) Teardown() error {
 	clearActiveFlag(a.log)
 	a.lastGood = nil
 	a.lastNft = ""
-	a.setHolding(false)
+	// Not a blanket false any more: with a holding plane standing, forwarded LAN
+	// traffic really IS being blocked, and saying otherwise here is the inverted lie
+	// Holding()'s doc comment is about — the process is exiting, but Status can
+	// still be read over the control socket before it does.
+	a.setHolding(kept)
 	a.setTraffic(generate.Traffic{})
 	a.setWarnings(nil)
 	// The plane is gone, so the logged set no longer describes anything. Forget it,

shater/apply/teardownexit_test.go

@@ -0,0 +1,149 @@
+package apply
+
+// The exit path must never leave the LAN uncovered, and "never" is an ORDER, not
+// an intention.
+//
+// WHAT THIS PINS. The daemon's SIGTERM path used to be:
+//
+//	applier.Teardown()   // netplane.TeardownNft() -> `nft delete table inet shater`
+//	armOnExit(handoff)   // then, separately, install the fail-closed holding plane
+//
+// Two nft transactions. Between them the `inet shater` table does not exist, so
+// fw4's `lan -> wan ACCEPT` is the only policy on the box and every forwarded LAN
+// packet leaves in the clear. MEASURED on the stand at 80-90 ms, reproduced twice
+// with a 35 000-sample run at ~1.3 ms resolution — and this is not a boot-time
+// window that heals itself: it is every `restart`, every `reload_service` (i.e.
+// every LuCI Save & Apply), and every package upgrade.
+//
+// The old call site even carried a comment explaining the mechanism — "AFTER the
+// teardown, never before: Teardown deletes the table, so a plane installed first
+// would simply be removed again" — and drew the wrong conclusion from a correct
+// observation. The fix is not to arm later, it is to stop deleting: arm first (a
+// single `nft -f` that opens with `delete table` and closes with the new one, so
+// the kernel replaces rather than removes), then skip the delete.
+//
+// So the property under test is a SEQUENCE, and the test records the order the
+// two seams are called in. A test that only asserted "the table still exists at
+// the end" would pass against the broken code.
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/sagernet/sing-box/shater/engine"
+)
+
+// recordTeardownSeams captures the order in which the exit path touches the
+// kernel: "arm" when a holding plane is installed, "delete" when the table is
+// removed.
+func recordTeardownSeams(t *testing.T) (*[]string, func()) {
+	t.Helper()
+	var calls []string
+	orig := teardownNft
+	teardownNft = func() error {
+		calls = append(calls, "delete")
+		return nil
+	}
+	return &calls, func() { teardownNft = orig }
+}
+
+// TestTeardownExitingReplacesThePlaneInsteadOfRemovingIt is the regression: with a
+// successor coming, the table must be swapped and NEVER deleted.
+func TestTeardownExitingReplacesThePlaneInsteadOfRemovingIt(t *testing.T) {
+	calls, restore := recordTeardownSeams(t)
+	defer restore()
+
+	a := New(engine.New(), nil)
+	if err := a.TeardownExiting(func() bool {
+		*calls = append(*calls, "arm")
+		return true
+	}); err != nil {
+		t.Fatalf("TeardownExiting: %v", err)
+	}
+
+	if len(*calls) != 1 || (*calls)[0] != "arm" {
+		t.Fatalf("exit path did %v, want exactly [arm]: the holding plane must be installed "+
+			"and the table must NOT be deleted — a `delete` here is the 80-90 ms window in which "+
+			"fw4's lan->wan ACCEPT is the only policy on the box", *calls)
+	}
+	if !a.Holding() && tableExists == nil {
+		t.Errorf("unreachable; keeps the linter honest about the seam")
+	}
+}
+
+// TestTeardownExitingArmsBeforeItTearsDown pins the ORDER even in the case where
+// the table does still get removed. Arming has to be the first thing that touches
+// the kernel; if it ran after the delete we would be back to the two-transaction
+// gap with extra steps.
+func TestTeardownExitingArmsBeforeItTearsDown(t *testing.T) {
+	calls, restore := recordTeardownSeams(t)
+	defer restore()
+
+	a := New(engine.New(), nil)
+	// arm reports FALSE: nothing was installed (a render failure, or kill_switch=open
+	// where fail-open is the operator's documented choice). The table must then come
+	// down exactly as it always did.
+	if err := a.TeardownExiting(func() bool {
+		*calls = append(*calls, "arm")
+		return false
+	}); err != nil {
+		t.Fatalf("TeardownExiting: %v", err)
+	}
+
+	if len(*calls) != 2 || (*calls)[0] != "arm" || (*calls)[1] != "delete" {
+		t.Fatalf("exit path did %v, want [arm delete]: arming must precede the delete, and a "+
+			"plane that was NOT installed must not keep the table alive", *calls)
+	}
+}
+
+// TestTeardownStillRemovesEverything is the escape hatch. A deliberate `stop`, and
+// a Reconcile that finds the stack disabled, both come through the plain Teardown
+// and must dismantle the plane completely — a kill switch that cannot be switched
+// off is a brick.
+func TestTeardownStillRemovesEverything(t *testing.T) {
+	calls, restore := recordTeardownSeams(t)
+	defer restore()
+
+	a := New(engine.New(), nil)
+	if err := a.Teardown(); err != nil {
+		t.Fatalf("Teardown: %v", err)
+	}
+	if len(*calls) != 1 || (*calls)[0] != "delete" {
+		t.Fatalf("Teardown did %v, want [delete]: an operator's stop must take the table with it", *calls)
+	}
+}
+
+// TestTeardownExitingReportsHolding pins the honesty half: with a plane left
+// standing the Applier must not go on saying it installed nothing. Status can
+// still be read over the control socket between the swap and the exit, and
+// "holding=false over a blocked LAN" is the inverted lie holdstate_test.go is
+// about, just reached down a different path.
+func TestTeardownExitingReportsHolding(t *testing.T) {
+	_, restore := recordTeardownSeams(t)
+	defer restore()
+	restoreFacts := stubPlaneFacts(t, true, true)
+	defer restoreFacts()
+
+	a := New(engine.New(), nil)
+	if err := a.TeardownExiting(func() bool { return true }); err != nil {
+		t.Fatalf("TeardownExiting: %v", err)
+	}
+	if !a.Holding() {
+		t.Errorf("Holding() = false right after the exit path left a fail-closed plane standing")
+	}
+}
+
+// TestTeardownExitingSurvivesANilArm keeps the plain-Teardown contract explicit:
+// a nil arm is "nothing to install", not a panic.
+func TestTeardownExitingSurvivesANilArm(t *testing.T) {
+	calls, restore := recordTeardownSeams(t)
+	defer restore()
+
+	a := New(engine.New(), nil)
+	if err := a.TeardownExiting(nil); err != nil && !errors.Is(err, nil) {
+		t.Fatalf("TeardownExiting(nil): %v", err)
+	}
+	if len(*calls) != 1 || (*calls)[0] != "delete" {
+		t.Fatalf("TeardownExiting(nil) did %v, want [delete]", *calls)
+	}
+}

shater/cmd/shaterd/armor.go

@@ -180,44 +180,59 @@ func refreshBootArmor(m *model.Model, logger log.ContextLogger) {
 	}
 }
 
-// armFromSnapshot reinstates the persisted holding plane. why is a short phrase
-// for the log ("config is unreadable", "restart handoff").
-func armFromSnapshot(why string, logger log.ContextLogger) {
+// armFromSnapshot reinstates the persisted holding plane and reports whether a
+// plane is now standing. why is a short phrase for the log ("config is
+// unreadable", "restart handoff").
+func armFromSnapshot(why string, logger log.ContextLogger) bool {
 	loaded, err := netplane.LoadBootArmor()
 	switch {
 	case err != nil:
 		logger.Error("FAIL-CLOSED PLANE NOT INSTALLED (", why, "): the saved plane ",
 			netplane.BootArmorPath, " could not be loaded: ", err,
 			" — LAN traffic may be reaching the WAN unprotected")
+		return false
 	case loaded:
 		logger.Error("fail-closed plane reinstated from ", netplane.BootArmorPath,
 			" (", why, "): LAN->WAN forwarding is BLOCKED. ",
 			"SSH, LuCI and the admin panel remain reachable.")
+		return true
 	default:
 		logger.Warn("no saved fail-closed plane at ", netplane.BootArmorPath, " (", why,
 			"): nothing was installed")
+		return false
 	}
 }
 
-// armFromModel renders the holding plane for m and installs it.
-func armFromModel(m *model.Model, why string, logger log.ContextLogger) {
+// armFromModel renders the holding plane for m, installs it, and reports whether
+// a plane is now standing.
+//
+// The return value is load-bearing on the exit path: Applier.TeardownExiting keeps
+// the nft table only when a plane really was installed, so a render failure or a
+// fail-open config falls back to the old remove-everything behaviour instead of
+// leaving whatever the engine happened to have in the kernel.
+func armFromModel(m *model.Model, why string, logger log.ContextLogger) bool {
 	ruleset, err := netplane.RenderHoldNft(m)
 	if err != nil {
 		logger.Error("FAIL-CLOSED PLANE NOT INSTALLED (", why, "): render failed: ", err)
-		return
+		return false
 	}
 	if ruleset == "" {
 		// No divert devices: there is nothing this plane would protect.
-		return
+		return false
 	}
+	// One `nft -f` that opens with `delete table` and closes with the new table:
+	// the swap is a single netlink transaction, so this REPLACES whatever plane is
+	// loaded without the table ever being absent. That property is why the exit
+	// path can arm before it tears down.
 	if err := netplane.ApplyNft(ruleset); err != nil {
 		logger.Error("FAIL-CLOSED PLANE NOT INSTALLED (", why, "): ", err,
 			" — LAN traffic may be reaching the WAN unprotected")
-		return
+		return false
 	}
 	logger.Info("fail-closed plane left in place (", why,
 		"): LAN->WAN forwarding stays BLOCKED until the next daemon applies. ",
 		"SSH, LuCI and the admin panel remain reachable.")
+	return true
 }
 
 // armOnUnreadableConfig is the window-3 answer: the daemon is up but cannot read
@@ -247,13 +262,20 @@ func armOnUnreadableConfig(logger log.ContextLogger) {
 var errUnreadableConfig = os.ErrInvalid
 
 // armOnExit is the window-2 answer: what this daemon leaves in the kernel when it
-// is asked to go away. See planExitArmor for the policy.
-func armOnExit(handoff bool, logger log.ContextLogger) {
+// is asked to go away, and whether anything is now standing there. See
+// planExitArmor for the policy.
+//
+// It is called BY Applier.TeardownExiting, before the teardown and under the apply
+// lock, so that the plane is swapped rather than removed-then-rebuilt. It must
+// therefore never call back into the Applier — everything here goes straight to
+// model.ReadUCI and netplane.
+func armOnExit(handoff bool, logger log.ContextLogger) bool {
 	m, err := model.ReadUCI()
 	switch planExitArmor(handoff, m, err, netplane.BootArmorPresent()) {
 	case armorRender:
-		armFromModel(m, "restart handoff", logger)
+		return armFromModel(m, "restart handoff", logger)
 	case armorSnapshot:
-		armFromSnapshot("restart handoff, config unreadable", logger)
+		return armFromSnapshot("restart handoff, config unreadable", logger)
 	}
+	return false
 }

shater/cmd/shaterd/initscript_test.go

@@ -0,0 +1,169 @@
+// Executing the SHIPPED init script's action classification.
+//
+// WHY THIS TEST IS SHAPED LIKE THIS
+//
+// The boot-armor defect that shipped in v0.2.17 was not in any Go file. The Go
+// half was correct and fully covered: refreshBootArmor tracked desired state,
+// planExitArmor made the right call, LoadBootArmor validated before loading.
+// Every one of those tests was green while the feature did not work at all on
+// hardware, because the thing that broke it was one shell `case` in
+// /etc/init.d/shater whose default arm swept up procd's `shutdown` action — so
+// the arm token was deleted on the way down, every reboot, and the boot it
+// existed to protect always found no file.
+//
+// A unit test that cannot see the shell file cannot catch that, and a comment in
+// the shell file claiming `shutdown` is handled is precisely what shipped. So
+// this runs the real thing: it sources the actual packaged
+// openwrt/shater-core/files/etc/init.d/shater in /bin/sh and calls its two
+// classification predicates with every action procd actually uses.
+//
+// Sourcing the whole file is safe and deliberate — at top level it contains only
+// variable assignments and function definitions, nothing that touches the system —
+// and sourcing the WHOLE file is the point: a test that copy-pasted the `case`
+// would pass while the shipped script said something else.
+//
+// The action names are not invented. They were measured on the target
+// (ImmortalWrt 25.12.1 r37978) with a throwaway probe init script:
+//
+//	/etc/init.d/X restart  -> stop_service action=[restart]
+//	/etc/init.d/X stop     -> stop_service action=[stop]
+//	/etc/init.d/X reload   -> reload_service action=[reload]
+//	`reboot`               -> stop_service action=[shutdown]
+//	the boot after it      -> start_service action=[boot]
+package main
+
+import (
+	"os/exec"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+)
+
+// initScriptPath is the packaged init script, relative to this package dir.
+const initScriptPath = "../../../openwrt/shater-core/files/etc/init.d/shater"
+
+// askInitScript sources the init script in /bin/sh and reports whether fn
+// returns true for the given arguments.
+func askInitScript(t *testing.T, fn string, args ...string) bool {
+	t.Helper()
+	abs, err := filepath.Abs(initScriptPath)
+	if err != nil {
+		t.Fatalf("resolve %s: %v", initScriptPath, err)
+	}
+	// `. script` then call the predicate. `set -e` is deliberately NOT used: the
+	// predicates report by exit status, and a false answer is not an error.
+	script := `. "$1" || exit 3; shift; if ` + fn + ` "$@"; then echo yes; else echo no; fi`
+	argv := append([]string{"-c", script, "sh", abs}, args...)
+	out, err := exec.Command("/bin/sh", argv...).CombinedOutput()
+	if err != nil {
+		t.Fatalf("%s(%q): %v\n%s", fn, args, err, out)
+	}
+	switch strings.TrimSpace(string(out)) {
+	case "yes":
+		return true
+	case "no":
+		return false
+	default:
+		t.Fatalf("%s(%q): unreadable answer %q", fn, args, out)
+		return false
+	}
+}
+
+// TestInitScriptActionClassification pins the two closed lists. The `shutdown`
+// rows are the regression: both must be false, because a reboot is neither a
+// handoff (nothing is coming) nor an operator switching the product off.
+func TestInitScriptActionClassification(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("needs a POSIX /bin/sh; the gate runs on linux")
+	}
+	for _, tc := range []struct {
+		action  string
+		disarms bool
+		handoff bool
+		why     string
+	}{
+		{"stop", true, false, "the operator switched the product off"},
+		{"shutdown", false, false, "REBOOT/POWEROFF — must not disarm; this is the boot the armor exists for"},
+		{"restart", false, true, "a successor is coming"},
+		{"reload", false, true, "Save & Apply is stop+start"},
+		{"boot", false, false, "start side, never reaches stop_service"},
+		{"start", false, false, "start side"},
+		{"", false, false, "unknown/empty degrades to changing nothing"},
+		{"enable", false, false, "not a lifecycle transition"},
+		{"disable", false, false, "durable off, but handled by shater-armor's rc.d refusal, not here"},
+	} {
+		if got := askInitScript(t, "shater_action_disarms", tc.action); got != tc.disarms {
+			t.Errorf("shater_action_disarms(%q) = %v, want %v (%s)", tc.action, got, tc.disarms, tc.why)
+		}
+		if got := askInitScript(t, "shater_action_handoff", tc.action); got != tc.handoff {
+			t.Errorf("shater_action_handoff(%q) = %v, want %v (%s)", tc.action, got, tc.handoff, tc.why)
+		}
+	}
+}
+
+// TestInitScriptStopDisarmsOnlyForAPerson pins the rest of the decision: `stop`
+// disarms when a PERSON is behind it, or when the product is being removed — and
+// not when something is merely replacing it.
+//
+// base-files' default_prerm reaches stop_service as a plain `stop`:
+//
+//	if [ "$PKG_UPGRADE" != "1" ]; then "$i" disable; fi
+//	"$i" stop
+//
+// so two different intentions arrive as one action. The rc.d state separates them:
+// a removal has already run `disable`, a replacement has not.
+//
+// On this target an apk UPGRADE turns out never to run default_prerm at all
+// (no pre-upgrade script — verified with a real `apk fix --reinstall` while
+// sampling the armor file), so the upgrade rows below are defence-in-depth rather
+// than a reproduction. The removal rows are live behaviour.
+func TestInitScriptStopDisarmsOnlyForAPerson(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("needs a POSIX /bin/sh; the gate runs on linux")
+	}
+	for _, tc := range []struct {
+		action   string
+		inPkg    string
+		rcEnable string
+		want     bool
+		why      string
+	}{
+		{"stop", "0", "1", true, "an operator typed it — the escape hatch must keep working"},
+		{"stop", "0", "0", true, "an operator typed it on an already-disabled service"},
+		{"stop", "1", "1", false, "BEING REPLACED — prerm left the service enabled, so something is coming back"},
+		{"stop", "1", "0", true, "REMOVAL — prerm already ran `disable`; the product is going away"},
+		{"shutdown", "0", "1", false, "reboot never disarms"},
+		{"shutdown", "1", "1", false, "reboot never disarms, package manager or not"},
+		{"shutdown", "1", "0", false, "still a reboot; the action decides first"},
+		{"restart", "0", "1", false, "a successor is coming"},
+		{"restart", "1", "0", false, "a successor is coming; action decides before any state"},
+		{"reload", "1", "1", false, "Save & Apply"},
+		{"", "1", "0", false, "unknown action changes nothing"},
+	} {
+		got := askInitScript(t, "shater_stop_disarms", tc.action, tc.inPkg, tc.rcEnable)
+		if got != tc.want {
+			t.Errorf("shater_stop_disarms(%q, in_pkg=%s, rc_enabled=%s) = %v, want %v (%s)",
+				tc.action, tc.inPkg, tc.rcEnable, got, tc.want, tc.why)
+		}
+	}
+}
+
+// TestInitScriptsParse is the cheapest possible guard against the class of bug
+// that no Go test can otherwise see: a shell file that ships syntactically
+// broken. An init script that fails to parse takes the whole service down and
+// `go build` is perfectly happy about it.
+func TestInitScriptsParse(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("needs a POSIX /bin/sh; the gate runs on linux")
+	}
+	for _, name := range []string{"shater", "shater-armor", "shater-cron"} {
+		p, err := filepath.Abs(filepath.Join(filepath.Dir(initScriptPath), name))
+		if err != nil {
+			t.Fatalf("resolve %s: %v", name, err)
+		}
+		if out, err := exec.Command("/bin/sh", "-n", p).CombinedOutput(); err != nil {
+			t.Errorf("/etc/init.d/%s does not parse: %v\n%s", name, err, out)
+		}
+	}
+}

shater/cmd/shaterd/main.go

@@ -436,12 +436,18 @@ func cmdRun() int {
 			// operator's escape hatch and must keep working.
 			handoff := restartHandoffPending()
 			logger.Info("signal ", sig, ": honest teardown + exit (restart handoff: ", handoff, ")")
-			if err := applier.Teardown(); err != nil {
+			// BEFORE the teardown, not after. This used to read `Teardown(); armOnExit()`
+			// on the reasoning that Teardown deletes the table so arming first would be
+			// undone — true, and the wrong conclusion: it left a measured 80-90 ms window per
+			// restart (80-90 ms measured) in which no `inet shater` table existed at all and fw4's
+			// `lan -> wan ACCEPT` was the only policy on the box. TeardownExiting arms
+			// first (one nft transaction that REPLACES the table) and then skips the
+			// delete iff a plane really went in.
+			if err := applier.TeardownExiting(func() bool {
+				return armOnExit(handoff, logger)
+			}); err != nil {
 				logger.Error("teardown: ", err)
 			}
-			// AFTER the teardown, never before: Teardown deletes the table, so a plane
-			// installed first would simply be removed again.
-			armOnExit(handoff, logger)
 			return 0
 		}
 	}

shater/netplane/armor.go

@@ -131,6 +131,18 @@ func SaveBootArmor(ruleset string) (changed bool, err error) {
 	if err = os.Rename(name, BootArmorPath); err != nil {
 		return false, err
 	}
+	// And fsync the DIRECTORY. Syncing the file only guarantees its contents; the
+	// rename that publishes them is a directory operation, and on the flash
+	// filesystems this ships on (jffs2/f2fs/ubifs, and ext4 on the x86 images) an
+	// unsynced rename can be lost across a power cut while the file's data is not.
+	// The result would be a boot with no armor and no error anywhere — i.e. exactly
+	// the failure this file exists to prevent, on exactly the boot it exists for.
+	// A best-effort sync: a filesystem that will not open its own directory is not
+	// a reason to report a write that did happen as failed.
+	if d, derr := os.Open(dir); derr == nil {
+		_ = d.Sync()
+		_ = d.Close()
+	}
 	return true, nil
 }