Let SIGTERM cancel a running pass and bound nfs-server's clean-up at 10 s, so a hanging command cannot keep the export file past the stop timeout

modules/nfs-server/README.md

@@ -102,6 +102,13 @@ its config file, so on unassignment the config file is gone when the process is
 removes its export file — the one removal its code makes — and runs `exportfs -ra`, so the kernel lets the
 shares go. A plain restart, a push or a shutdown leaves the config file in place, and the exports stay.
 
+**Within the stop timeout.** The service manager kills a process that has not stopped 90 s after SIGTERM
+(mesh-host #62), and a killed process cleans up nothing. So SIGTERM ends a running pass at once (its
+commands and its look below a share are cancelled, and it writes nothing), and the clean-up has its own
+bound of 10 s: the export file is removed first, which needs no command, then `exportfs -ra` runs within
+that bound. Whatever it could not finish is logged as `nfs-server: UNASSIGNED, NOT CLEAN: …`, with what a
+person must do.
+
 ## What it leaves
 - **A subfolder export shares its filesystem.** A share that is a folder inside a larger filesystem (not
   a filesystem of its own) is exported with `no_subtree_check`, so a client that forges file handles could

modules/nfs-server/cmd/nfs-server/machine.go

@@ -108,6 +108,21 @@ type Machine struct {
 	// clean is when a look below each real path last found no backup repository (LookKept).
 	clean   map[string]time.Time
 	cleanMu sync.Mutex
+	// walk is the running pass's context, which ends the look below a share when the process stops.
+	walk   context.Context
+	walkMu sync.Mutex
+}
+
+func (m *Machine) setWalk(ctx context.Context) {
+	m.walkMu.Lock()
+	m.walk = ctx
+	m.walkMu.Unlock()
+}
+
+func (m *Machine) walking() context.Context {
+	m.walkMu.Lock()
+	defer m.walkMu.Unlock()
+	return m.walk
 }
 
 // Here is the machine this process runs on.
@@ -272,8 +287,17 @@ func (m *Machine) Escalated(name string, args ...string) (string, []string) {
 
 // AsRoot runs what only root may do, and names a refusal by how it failed.
 func (m *Machine) AsRoot(name string, args ...string) (Ran, error) {
+	return m.AsRootWithin(context.Background(), name, args...)
+}
+
+// AsRootWithin is AsRoot ended with its context: the process's pass is cancelled by SIGTERM, and Stopped
+// runs within StopTimeout.
+func (m *Machine) AsRootWithin(ctx context.Context, name string, args ...string) (Ran, error) {
 	program, argv := m.Escalated(name, args...)
-	r := m.Run(context.Background(), program, argv...)
+	r := m.Run(ctx, program, argv...)
+	if err := ctx.Err(); err != nil && r.Code != 0 {
+		return r, fmt.Errorf("%s %s did not finish: %v", name, strings.Join(args, " "), err)
+	}
 	if r.Code != 0 {
 		why := r.Err
 		if why == "" {

modules/nfs-server/cmd/nfs-server/process.go

@@ -56,7 +56,14 @@ func (m *Machine) Pass(ctx context.Context, last *Status) Status {
 	if _, note := m.Backup(); note != "" {
 		s.Said = append(s.Said, note)
 	}
+	m.setWalk(ctx)
 	exports, refused, said := m.Judge(c, roster, wants)
+	m.setWalk(nil)
+	if ctx.Err() != nil {
+		// Stopping: write nothing and reload nothing, so Stopped starts at once.
+		s.Problems = append(s.Problems, "the pass was ended by the process stopping; nothing was written")
+		return s
+	}
 	s.Refused = refused
 	s.Said = append(s.Said, said...)
 
@@ -80,7 +87,7 @@ func (m *Machine) Pass(ctx context.Context, last *Status) Status {
 		}
 	}
 	if reload != "" {
-		if _, err := m.AsRoot("exportfs", "-ra"); err != nil {
+		if _, err := m.AsRootWithin(ctx, "exportfs", "-ra"); err != nil {
 			s.ReloadError = err.Error()
 		} else {
 			at := m.Now().UTC()
@@ -102,6 +109,10 @@ func (m *Machine) Pass(ctx context.Context, last *Status) Status {
 	return s
 }
 
+// StopTimeout bounds Stopped and the commands it runs: well under the service manager's stop timeout (90 s,
+// then SIGKILL), so the export file is gone before the process could be killed (mesh-host #62).
+var StopTimeout = 10 * time.Second
+
 // RunExports is the process: a pass now and every Every until the context ends, then Stopped.
 func (m *Machine) RunExports(ctx context.Context) error {
 	last := m.Pass(ctx, nil)
@@ -184,11 +195,25 @@ func (m *Machine) Stopped() {
 	if _, err := m.Read(ConfigFile); err == nil || !os.IsNotExist(err) {
 		return
 	}
+	ctx, cancel := context.WithTimeout(context.Background(), StopTimeout)
+	defer cancel()
+	// The file first: removing it needs no command, so a hanging sudo or exportfs cannot keep it.
 	if err := m.unexport(); err != nil {
-		fmt.Fprintln(os.Stderr, "nfs-server:", err)
+		loud("the module was unassigned and its export file is still there: " + err.Error() +
+			"; remove " + ExportFile + " and run exportfs -ra as root")
 		return
 	}
-	if _, err := m.AsRoot("exportfs", "-ra"); err != nil {
-		fmt.Fprintln(os.Stderr, "nfs-server: the export file is gone, and exportfs -ra failed:", err)
+	fmt.Fprintln(os.Stderr, "nfs-server: unassigned: the export file "+ExportFile+" is removed")
+	if _, err := m.AsRootWithin(ctx, "exportfs", "-ra"); err != nil {
+		loud("the module was unassigned and its export file is removed, but the kernel may still export its " +
+			"shares until exportfs -ra runs as root: " + err.Error())
+		return
 	}
+	fmt.Fprintln(os.Stderr, "nfs-server: unassigned: the kernel let the shares go (exportfs -ra)")
+}
+
+// loud says, on the process's log where the service manager's journal keeps it, what an unassignment left
+// undone, so a person finds it rather than an export that outlived its module.
+func loud(what string) {
+	fmt.Fprintln(os.Stderr, "nfs-server: UNASSIGNED, NOT CLEAN: "+what)
 }

modules/nfs-server/cmd/nfs-server/review_test.go

@@ -379,3 +379,56 @@ func TestTheLookBelowIsKeptForAWhileAndThenTakenAgain(t *testing.T) {
 		t.Errorf("a repository put there later was not found once the kept look expired: %+v", s.Refused)
 	}
 }
+
+// mesh-host #62 found that a stop the service manager ends with SIGKILL after 90 s leaves the export file
+// behind. A pass that hangs in a command (sudo, exportfs: 20 s each) must not hold the cleanup: SIGTERM
+// cancels the running pass, and Stopped has its own short bound, so the file is gone well inside the stop
+// timeout even when every command hangs.
+func TestAHangingCommandStillLetsStoppedRemoveTheFileInTime(t *testing.T) {
+	f := newFake(t)
+	f.mountinfo("/")
+	f.dir("/srv/library")
+	f.file(ConfigFile, grantsConfig("library=/srv/library", "library=ws:rw"))
+	f.roster("ws 10.0.0.3")
+	f.wants(map[string][]Want{"ws": {{Share: "library", Access: "rw"}}})
+	m := f.machine()
+	inner := m.Run
+	hung := make(chan struct{}, 8)
+	m.Run = func(ctx context.Context, name string, args ...string) Ran {
+		line := strings.Join(append([]string{name}, args...), " ")
+		if strings.Contains(line, "exportfs") {
+			hung <- struct{}{}
+			<-ctx.Done() // hangs until its context ends, as a stuck sudo or exportfs would
+			return Ran{Code: -1, Err: ctx.Err().Error()}
+		}
+		return inner(ctx, name, args...)
+	}
+	saved := StopTimeout
+	StopTimeout = 300 * time.Millisecond
+	defer func() { StopTimeout = saved }()
+
+	ctx, cancel := context.WithCancel(context.Background())
+	done := make(chan error, 1)
+	go func() { done <- m.RunExports(ctx) }()
+	select {
+	case <-hung: // the first pass is stuck in exportfs
+	case <-time.After(5 * time.Second):
+		t.Fatal("the first pass never reached exportfs")
+	}
+	if err := os.Remove(filepath.Join(f.root, ConfigFile)); err != nil { // unassigned: config removed first
+		t.Fatal(err)
+	}
+	start := time.Now()
+	cancel() // SIGTERM
+	select {
+	case <-done:
+	case <-time.After(3 * time.Second):
+		t.Fatal("the process did not stop within 3 s of SIGTERM while a command hung")
+	}
+	if took := time.Since(start); took > 2*time.Second {
+		t.Errorf("stopping took %s with a hanging command; the bound is StopTimeout (%s)", took, StopTimeout)
+	}
+	if _, err := os.Stat(filepath.Join(f.root, ExportFile)); err == nil {
+		t.Errorf("the export file stays after an unassignment whose commands hung")
+	}
+}

modules/nfs-server/cmd/nfs-server/shares.go

@@ -281,9 +281,13 @@ func (m *Machine) walkBelow(dir string) (found string, complete bool) {
 	}
 	seen := 0
 	level := []string{dir}
+	walk := m.walking()
 	for depth := 1; depth <= WalkDepth && len(level) > 0; depth++ {
 		var next []string
 		for _, d := range level {
+			if walk != nil && walk.Err() != nil {
+				return "", false // ended by the process stopping: unfinished, so refused
+			}
 			for _, child := range m.Children(d) {
 				seen++
 				if seen > limit {