fix: address review on #221

Four things the review caught.

The MCP conf resource handed out the live config with no redaction. It
is the one surface whose full text leaves the machine by design — the
client, and from there a model provider — and a witness node's config
carries its signing key. Redact at the resource, not in the shared
helper: the drift tool reads the same helper and needs the raw text,
since a redacted side would report every witness node as drifted.

RedactWitnessLines decided array membership from line shapes, and two
shapes defeated it. A `]` inside the comment on the opening line ended
the array before it began and let the key through; an element that
closed the array left the scanner inside it, so every following line
came out as <REDACTED> and the diff said nothing. Find the values by
parsing instead — the config parser is already a dependency for
dbfork — and replace those literal strings wherever they appear, which
no formatting can dodge. The scan stays as the fallback for text that
does not parse, hardened to strip comments before the bracket checks.

Every nodeContext command was holding the exclusive state lock until
Close, including ones that never write: logs, wait, exec, files,
health, diagnose, verify-config. A `trond logs -f` left running would
block every other trond process on the host, and Acquire had no
deadline, so the wait was indefinite and unexplained. Split the resolve
path — readers take nothing, writers keep the lock across
load-modify-save — and bound the wait at 30s with an error that says
which process to look for.

Enabling provisioning mode made a discarded error live: ensureServiceUser
ignored useradd's result, which was invisible while the whitelist
refused the call. Check it, tolerating only "already exists" so
bootstrap stays re-runnable.

Author: Little-Peony

cmd/bootstrap.go

@@ -77,7 +77,18 @@ func runBootstrap(cmd *cobra.Command, args []string) error {
 		if len(parsed.Nodes) > 0 && parsed.Nodes[0].SystemUser != "" {
 			user = parsed.Nodes[0].SystemUser
 		}
-		tgt.Exec(ctx, "useradd", "--system", "--no-create-home", "--shell", "/usr/sbin/nologin", user)
+		// Until provisioning mode existed this call was refused by the
+		// SSH whitelist and failed on every remote target, so discarding
+		// the error was invisible. Now that it runs, a real failure —
+		// no permission, a conflicting uid, no /usr/sbin/nologin — would
+		// otherwise be reported as a created user.
+		if out, err := tgt.Exec(ctx, "useradd", "--system", "--no-create-home",
+			"--shell", "/usr/sbin/nologin", user); err != nil {
+			if !userAlreadyExists(out) {
+				return exitWithError("BOOTSTRAP_ERROR", output.ExitGeneralError,
+					fmt.Sprintf("Failed to create system user %q: %v: %s", user, err, strings.TrimSpace(string(out))))
+			}
+		}
 		installed = append(installed, "user:"+user)
 	}
 
@@ -138,3 +149,16 @@ func installJDK(ctx context.Context, tgt target.Target) error {
 
 	return fmt.Errorf("unsupported package manager; install JDK 17 manually")
 }
+
+// userAlreadyExists reports whether a useradd failure was only the user
+// being there already, which bootstrap has to tolerate: it is expected
+// to be re-runnable, and the second run finds the user from the first.
+//
+// useradd exits 9 for "name already in use", but the exit status does
+// not survive target.Exec's error, so the message is what is left to
+// match on. Both util-linux and busybox wording are covered.
+func userAlreadyExists(out []byte) bool {
+	msg := strings.ToLower(string(out))
+	return strings.Contains(msg, "already exists") ||
+		strings.Contains(msg, "already in use")
+}

cmd/bootstrap_test.go

@@ -0,0 +1,31 @@
+package cmd
+
+import "testing"
+
+// bootstrap has to be re-runnable, so a useradd that fails only because
+// the user is already there must not abort the run — but anything else
+// must, now that provisioning mode lets the call actually execute.
+func TestUserAlreadyExists(t *testing.T) {
+	tolerated := []string{
+		"useradd: user 'tron' already exists",
+		"useradd: UID 999 is not unique\nuseradd: name tron already in use",
+		"ALREADY EXISTS",
+	}
+	for _, out := range tolerated {
+		if !userAlreadyExists([]byte(out)) {
+			t.Errorf("should tolerate: %q", out)
+		}
+	}
+
+	fatal := []string{
+		"useradd: Permission denied.",
+		"useradd: cannot open /etc/passwd",
+		"useradd: invalid shell '/usr/sbin/nologin'",
+		"",
+	}
+	for _, out := range fatal {
+		if userAlreadyExists([]byte(out)) {
+			t.Errorf("should not tolerate: %q", out)
+		}
+	}
+}

cmd/heal.go

@@ -102,7 +102,7 @@ func runAutoHeal(cmd *cobra.Command, args []string) error {
 		}
 	}
 
-	nc, err := resolveNodeContext(name)
+	nc, err := resolveNodeContextForWrite(name)
 	if err != nil {
 		return err
 	}

cmd/remove.go

@@ -49,7 +49,7 @@ func runRemove(cmd *cobra.Command, args []string) error {
 	}
 
 	start := time.Now()
-	nc, err := resolveNodeContext(name)
+	nc, err := resolveNodeContextForWrite(name)
 	if err != nil {
 		return err
 	}

cmd/resolve.go

@@ -115,23 +115,43 @@ func requirePrivateForNodes(names ...string) error {
 	return guard.EnforceNodes(refs)
 }
 
-// resolveNodeContext loads a node from state and constructs its target and runtime.
+// resolveNodeContext loads a node from state without keeping the state
+// lock. Use it for commands that only read — logs, wait, exec, files,
+// health, diagnose, verify-config. Holding the exclusive lock across
+// those buys nothing, and `logs -f` or a long `wait` would keep every
+// other trond process on the host blocked for as long as it runs.
 func resolveNodeContext(name string) (*nodeContext, error) {
+	return resolveNode(name, false)
+}
+
+// resolveNodeContextForWrite loads a node and keeps the state lock until
+// Close. Commands that write the node list back — start, stop, restart,
+// upgrade, rollback, heal, remove — need the read and the write inside
+// one lock, or a concurrent trond drops one of the two updates.
+func resolveNodeContextForWrite(name string) (*nodeContext, error) {
+	return resolveNode(name, true)
+}
+
+func resolveNode(name string, forWrite bool) (*nodeContext, error) {
 	store, err := state.NewStore(statePath())
 	if err != nil {
 		return nil, err
 	}
 
-	// Take the lock before the read: the caller writes the same state back
-	// through SaveState once its operation finishes.
-	lock := state.NewLock(stateDir())
-	if err := lock.Acquire(); err != nil {
-		return nil, exitWithError("LOCK_ERROR", output.ExitGeneralError,
-			"Failed to acquire state lock: "+err.Error(),
-			"Check if another trond process is running")
+	// A writer takes the lock before the read and holds it until Close,
+	// so the load-modify-save cycle is atomic. A reader takes nothing:
+	// the load below is a single Load() and nothing is written back.
+	var lock *state.Lock
+	if forWrite {
+		lock = state.NewLock(stateDir())
+		if err := acquireStateLock(lock); err != nil {
+			return nil, err
+		}
 	}
 	release := func() {
-		lock.Release()
+		if lock != nil {
+			lock.Release()
+		}
 	}
 
 	deployState, err := store.Load()
@@ -241,3 +261,36 @@ func writeAudit(ev auditEvent) {
 		Log().Warn("audit log write failed", "error", writeErr)
 	}
 }
+
+// stateLockTimeout bounds how long a command waits for the state lock.
+// Long enough that a normal deploy finishing up is simply waited out,
+// short enough that a stuck or forgotten process is reported rather
+// than leaving the caller staring at a hung terminal.
+const stateLockTimeout = 30 * time.Second
+
+// acquireStateLock waits for the lock, but not forever. syscall.Flock
+// with LOCK_EX blocks with no deadline, so the wait happens on a
+// goroutine and the caller gives up after stateLockTimeout with an
+// error that says what to do about it.
+func acquireStateLock(lock *state.Lock) error {
+	done := make(chan error, 1)
+	go func() { done <- lock.Acquire() }()
+
+	select {
+	case err := <-done:
+		if err != nil {
+			return exitWithError("LOCK_ERROR", output.ExitGeneralError,
+				"Failed to acquire state lock: "+err.Error(),
+				"Check if another trond process is running")
+		}
+		return nil
+	case <-time.After(stateLockTimeout):
+		// The goroutine keeps waiting and will release on process exit;
+		// the lock file is a shared resource, so abandoning the attempt
+		// is safe.
+		return exitWithError("LOCK_TIMEOUT", output.ExitGeneralError,
+			fmt.Sprintf("Another trond process has held the state lock for %s", stateLockTimeout),
+			"Find it with: ps aux | grep trond",
+			"A stuck process can be ended; the lock is released when it exits")
+	}
+}

cmd/restart.go

@@ -28,7 +28,7 @@ func runRestart(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	nc, err := resolveNodeContext(name)
+	nc, err := resolveNodeContextForWrite(name)
 	if err != nil {
 		return err
 	}

cmd/rollback.go

@@ -29,7 +29,7 @@ func runRollback(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	nc, err := resolveNodeContext(name)
+	nc, err := resolveNodeContextForWrite(name)
 	if err != nil {
 		return err
 	}

cmd/start.go

@@ -28,7 +28,7 @@ func runStart(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	nc, err := resolveNodeContext(name)
+	nc, err := resolveNodeContextForWrite(name)
 	if err != nil {
 		return err
 	}

cmd/stop.go

@@ -28,7 +28,7 @@ func runStop(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	nc, err := resolveNodeContext(name)
+	nc, err := resolveNodeContextForWrite(name)
 	if err != nil {
 		return err
 	}

cmd/upgrade.go

@@ -34,7 +34,7 @@ func runUpgrade(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	nc, err := resolveNodeContext(name)
+	nc, err := resolveNodeContextForWrite(name)
 	if err != nil {
 		return err
 	}