fix(backup): close hollow-gate path where RestoreDrill.Success ignored zero rows

.github/wiki/Config-Env-Vars.md

@@ -17,6 +17,7 @@ All ɳSelf project configuration lives in `.env` (and optionally `.env.local` fo
 - [Load Order (Cascade)](#load-order-cascade)
 - [Core Project Settings](#core-project-settings)
 - [PostgreSQL](#postgresql)
+- [Backup and Restore](#backup-and-restore)
 - [Hasura (GraphQL API)](#hasura-graphql-api)
 - [Auth](#auth)
 - [Nginx and SSL](#nginx-and-ssl)
@@ -104,6 +105,36 @@ This value is written to `.env.computed` and should not be set manually.
 
 ---
 
+## Backup and Restore
+
+Credentials may come from these variables or from `rclone.conf` / `RCLONE_CONFIG_*`.
+Leaving **both** key variables empty is valid and means "configured elsewhere".
+Setting **exactly one** of the pair is always rejected, because it is never a
+meaningful state: it is a typo or a partial migration between the two accepted
+name pairs, and rclone would otherwise fail silently or send an empty secret.
+
+| Variable | Type | Default | Required | Description |
+|---|---|---|---|---|
+| `BACKUP_S3_BUCKET` | string | *(unset)* | No | Destination bucket for off-box backup copies. App-level: consumed by app backup scripts, not read into the CLI's config struct. |
+| `BACKUP_S3_ACCESS_KEY_ID` | string | *(unset)* | No | Access key for the backup bucket. Must be set together with `BACKUP_S3_SECRET_ACCESS_KEY`. |
+| `BACKUP_S3_SECRET_ACCESS_KEY` | string | *(unset)* | No | Secret key for the backup bucket. Must be set together with `BACKUP_S3_ACCESS_KEY_ID`. |
+| `BACKUP_S3_REGION` | string | *(unset)* | No | Region passed to the storage provider. |
+| `BACKUP_S3_ENDPOINT` | string | *(unset)* | No | Custom endpoint for S3-compatible providers (Backblaze B2, Cloudflare R2, MinIO). Leave unset for AWS S3. |
+| `BACKUP_S3_PREFIX` | string | *(unset)* | No | Key prefix applied to uploaded objects, for sharing one bucket across projects. App-level, like `BACKUP_S3_BUCKET`. |
+| `BACKUP_ACCESS_KEY` | string | *(unset)* | No | **Accepted alias** for `BACKUP_S3_ACCESS_KEY_ID`. The canonical name wins when both are set. |
+| `BACKUP_SECRET_KEY` | string | *(unset)* | No | **Accepted alias** for `BACKUP_S3_SECRET_ACCESS_KEY`. The canonical name wins when both are set. |
+
+**Why the aliases exist.** `BACKUP_ACCESS_KEY` / `BACKUP_SECRET_KEY` were already
+in use in the wild (they are the names in `ntask/backend/.env.example`) but were
+never read into the backup config, so every deployment using them had remote
+upload silently disabled while appearing configured. Both spellings are now
+accepted. Prefer the canonical `BACKUP_S3_*` names in new configuration.
+
+See also `BACKUP_CRITICAL_TABLES` under [PostgreSQL](#postgresql), and
+[[cmd-backup]] for the commands that consume these.
+
+---
+
 ## Hasura (GraphQL API)
 
 | Variable | Type | Default | Required | Description |

internal/backup/create.go

@@ -14,6 +14,41 @@ import (
 	"github.com/nself-org/cli/internal/metrics"
 )
 
+// requireBackupContainerConfig fails loudly, naming exactly what is missing,
+// before any backup type derives a docker container name from cfg.
+//
+// PROJECT_NAME and POSTGRES_DB both get silent placeholder defaults inside
+// config.ApplyDefaults ("myproject" / "nself") so cfg.ProjectName and
+// cfg.Postgres.DB are never actually empty by the time Create() runs — that
+// is precisely the trap: createFullBackup happily builds
+// "myproject_postgres" and hands it to `docker exec`, which fails with a
+// generic "No such container" error that names the wrong thing (a container
+// that was never expected to exist) instead of the actual problem (the two
+// vars that decide the real container name were never set). Checking
+// os.Getenv directly here — instead of the already-defaulted cfg fields —
+// is what lets this tell "the user configured myproject/nself on purpose"
+// apart from "nothing was configured and the default silently took over."
+//
+// This only guards `nself backup create`, which is the surface the reported
+// failure came from; `Backup()` in internal/database (used by other DB
+// tooling) is a separate lower-level primitive with its own callers.
+func requireBackupContainerConfig() error {
+	var missing []string
+	if strings.TrimSpace(os.Getenv("PROJECT_NAME")) == "" {
+		missing = append(missing, "PROJECT_NAME")
+	}
+	if strings.TrimSpace(os.Getenv("POSTGRES_DB")) == "" {
+		missing = append(missing, "POSTGRES_DB")
+	}
+	if len(missing) == 0 {
+		return nil
+	}
+	return fmt.Errorf(
+		"backup create: required config not set: %s — set these in .env, or run from inside the project directory (without them the backup would target a placeholder container name that cannot exist)",
+		strings.Join(missing, ", "),
+	)
+}
+
 // BackupType identifies the kind of backup to create.
 type BackupType string
 
@@ -48,6 +83,10 @@ func Create(ctx context.Context, cfg *config.Config, opts CreateOptions) error {
 		return nil
 	}
 
+	if err := requireBackupContainerConfig(); err != nil {
+		return err
+	}
+
 	types := []BackupType{opts.Type}
 	if opts.Type == BackupTypeAll {
 		types = []BackupType{BackupTypeFull, BackupTypeMetadata}

internal/backup/create_remote.go

@@ -0,0 +1,82 @@
+// Package backup — create_remote.go: post-create encryption and remote
+// (rclone) upload helpers used by createFullBackup. Split out of
+// create_targets.go (file-size ratchet, internal/repoqa) as a pure move —
+// no behavior change beyond what is documented on requireCompleteS3Credentials
+// and uploadToRemote below.
+package backup
+
+import (
+	"context"
+	"fmt"
+	"os"
+	"os/exec"
+	"path/filepath"
+
+	"github.com/nself-org/cli/internal/config"
+	"github.com/nself-org/cli/internal/errs"
+)
+
+// encryptFile encrypts a file in-place using age with the given recipient public key.
+func encryptFile(path, recipient string) error {
+	encPath := path + ".age"
+	args := []string{"-r", recipient, "-o", encPath, path}
+	cmd := exec.Command("age", args...)
+	if output, err := cmd.CombinedOutput(); err != nil {
+		return fmt.Errorf("%w: %s", errs.ErrBackupEncryptFailed, string(output))
+	}
+
+	// Replace original with encrypted version.
+	if err := os.Remove(path); err != nil {
+		return fmt.Errorf("remove unencrypted file: %w", err)
+	}
+	if err := os.Rename(encPath, path+".age"); err != nil {
+		return fmt.Errorf("rename encrypted file: %w", err)
+	}
+
+	return nil
+}
+
+// requireCompleteS3Credentials refuses a half-configured S3 credential pair.
+// Exactly one of BACKUP_S3_ACCESS_KEY_ID/BACKUP_ACCESS_KEY and
+// BACKUP_S3_SECRET_ACCESS_KEY/BACKUP_SECRET_KEY being set is never valid: it
+// is not "use rclone.conf instead" (that path is both empty) and it is not
+// "use these creds" (that path is both set) — it is a typo or partial
+// migration between the two accepted name pairs, and rclone/S3 would either
+// silently fail or use an empty-string secret. Both-empty is fine: the
+// remote may be fully configured via rclone.conf or RCLONE_CONFIG_* env vars
+// with no nSelf-side credentials at all.
+func requireCompleteS3Credentials(cfg *config.Config) error {
+	hasAccess := cfg.Backup.S3AccessKeyID != ""
+	hasSecret := cfg.Backup.S3SecretAccessKey != ""
+	if hasAccess == hasSecret {
+		return nil
+	}
+	missing := "BACKUP_S3_SECRET_ACCESS_KEY (or BACKUP_SECRET_KEY)"
+	if hasSecret {
+		missing = "BACKUP_S3_ACCESS_KEY_ID (or BACKUP_ACCESS_KEY)"
+	}
+	return fmt.Errorf("S3 backup credentials are half-configured: %s is set but its counterpart is not", missing)
+}
+
+// uploadToRemote uploads a local file to the configured rclone remote. When
+// cfg carries an S3 access/secret key pair (BACKUP_S3_ACCESS_KEY_ID /
+// BACKUP_S3_SECRET_ACCESS_KEY, or the BACKUP_ACCESS_KEY / BACKUP_SECRET_KEY
+// aliases), it is exported as AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY for
+// this rclone invocation — the env-var form rclone's :s3 / :r2 remotes read
+// when no matching entry exists in rclone.conf. Callers must have already
+// checked requireCompleteS3Credentials; this function does not re-check.
+func uploadToRemote(ctx context.Context, localPath, remote string, cfg *config.Config) error {
+	args := []string{"copyto", localPath, remote + "/" + filepath.Base(localPath)}
+	cmd := exec.CommandContext(ctx, "rclone", args...)
+	cmd.Env = os.Environ()
+	if cfg.Backup.S3AccessKeyID != "" && cfg.Backup.S3SecretAccessKey != "" {
+		cmd.Env = append(cmd.Env,
+			"AWS_ACCESS_KEY_ID="+cfg.Backup.S3AccessKeyID,
+			"AWS_SECRET_ACCESS_KEY="+cfg.Backup.S3SecretAccessKey,
+		)
+	}
+	if output, err := cmd.CombinedOutput(); err != nil {
+		return fmt.Errorf("%w: %s", errs.ErrBackupRemoteFailed, string(output))
+	}
+	return nil
+}

internal/backup/create_targets.go

@@ -121,7 +121,14 @@ func createFullBackup(ctx context.Context, cfg *config.Config, backupDir, ts, ta
 		remote = opts.Remote
 	}
 	if remote != "" {
-		if err := uploadToRemote(ctx, outputPath, remote); err != nil {
+		if err := requireCompleteS3Credentials(cfg); err != nil {
+			// Half-configured S3 creds (one of access/secret set, not both)
+			// is worse than none: it looks configured, uploads nothing or
+			// fails opaquely inside rclone, and nobody notices until a
+			// restore is needed. Refuse loudly instead of attempting the
+			// upload with partial credentials.
+			slog.Error("remote upload skipped: incomplete S3 credentials", "error", err, "path", outputPath)
+		} else if err := uploadToRemote(ctx, outputPath, remote, cfg); err != nil {
 			slog.Error("remote upload failed", "error", err, "path", outputPath)
 			// Non-fatal: local backup succeeded.
 		}
@@ -227,33 +234,3 @@ func triggerWALCheckpoint(ctx context.Context, cfg *config.Config) error {
 	slog.Info("WAL checkpoint triggered")
 	return nil
 }
-
-// encryptFile encrypts a file in-place using age with the given recipient public key.
-func encryptFile(path, recipient string) error {
-	encPath := path + ".age"
-	args := []string{"-r", recipient, "-o", encPath, path}
-	cmd := exec.Command("age", args...)
-	if output, err := cmd.CombinedOutput(); err != nil {
-		return fmt.Errorf("%w: %s", errs.ErrBackupEncryptFailed, string(output))
-	}
-
-	// Replace original with encrypted version.
-	if err := os.Remove(path); err != nil {
-		return fmt.Errorf("remove unencrypted file: %w", err)
-	}
-	if err := os.Rename(encPath, path+".age"); err != nil {
-		return fmt.Errorf("rename encrypted file: %w", err)
-	}
-
-	return nil
-}
-
-// uploadToRemote uploads a local file to the configured rclone remote.
-func uploadToRemote(ctx context.Context, localPath, remote string) error {
-	args := []string{"copyto", localPath, remote + "/" + filepath.Base(localPath)}
-	cmd := exec.CommandContext(ctx, "rclone", args...)
-	if output, err := cmd.CombinedOutput(); err != nil {
-		return fmt.Errorf("%w: %s", errs.ErrBackupRemoteFailed, string(output))
-	}
-	return nil
-}

internal/backup/create_targets_test.go

@@ -0,0 +1,81 @@
+package backup
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/nself-org/cli/internal/config"
+)
+
+// TestRequireCompleteS3Credentials_BothEmptyIsFine covers the common case:
+// no BACKUP_S3_* / BACKUP_*_KEY vars set at all, remote upload relies
+// entirely on rclone.conf or RCLONE_CONFIG_* env vars. This must not be
+// treated as "half-configured".
+func TestRequireCompleteS3Credentials_BothEmptyIsFine(t *testing.T) {
+	cfg := &config.Config{}
+	if err := requireCompleteS3Credentials(cfg); err != nil {
+		t.Errorf("want nil for both-empty (rclone.conf-only) config, got %v", err)
+	}
+}
+
+// TestRequireCompleteS3Credentials_BothSetIsFine covers full nSelf-side
+// configuration via either the canonical or alias var names — both should
+// already have landed in the same struct fields by the time this runs.
+func TestRequireCompleteS3Credentials_BothSetIsFine(t *testing.T) {
+	cfg := &config.Config{Backup: config.BackupConfig{
+		S3AccessKeyID:     "AKIAEXAMPLE",
+		S3SecretAccessKey: "supersecret",
+	}}
+	if err := requireCompleteS3Credentials(cfg); err != nil {
+		t.Errorf("want nil for fully-configured S3 credentials, got %v", err)
+	}
+}
+
+// TestRequireCompleteS3Credentials_HalfConfiguredFails is the regression
+// lock for the ntask defect: BACKUP_ACCESS_KEY set without
+// BACKUP_SECRET_KEY (or the canonical pair split the same way) must fail
+// loudly rather than silently attempt an upload with an empty secret.
+func TestRequireCompleteS3Credentials_HalfConfiguredFails(t *testing.T) {
+	cases := []struct {
+		name string
+		cfg  config.BackupConfig
+	}{
+		{"access only", config.BackupConfig{S3AccessKeyID: "AKIAEXAMPLE"}},
+		{"secret only", config.BackupConfig{S3SecretAccessKey: "supersecret"}},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			cfg := &config.Config{Backup: tc.cfg}
+			if err := requireCompleteS3Credentials(cfg); err == nil {
+				t.Errorf("want error for half-configured S3 credentials (%s), got nil", tc.name)
+			}
+		})
+	}
+}
+
+// TestRequireBackupContainerConfig_MissingVarsFailLoud is the regression
+// lock for "nself backup create failed twice with a confusing error when
+// PROJECT_NAME/POSTGRES_DB were unset" — the guard must name the missing
+// variable(s) instead of letting a placeholder default silently derive a
+// container name that cannot exist.
+func TestRequireBackupContainerConfig_MissingVarsFailLoud(t *testing.T) {
+	t.Setenv("PROJECT_NAME", "")
+	t.Setenv("POSTGRES_DB", "")
+	err := requireBackupContainerConfig()
+	if err == nil {
+		t.Fatal("want error when PROJECT_NAME and POSTGRES_DB are both unset, got nil")
+	}
+	if !strings.Contains(err.Error(), "PROJECT_NAME") || !strings.Contains(err.Error(), "POSTGRES_DB") {
+		t.Errorf("error must name both missing vars, got: %v", err)
+	}
+}
+
+// TestRequireBackupContainerConfig_SetVarsPass proves the guard is silent
+// once both vars are actually configured.
+func TestRequireBackupContainerConfig_SetVarsPass(t *testing.T) {
+	t.Setenv("PROJECT_NAME", "ntask")
+	t.Setenv("POSTGRES_DB", "ntask")
+	if err := requireBackupContainerConfig(); err != nil {
+		t.Errorf("want nil once PROJECT_NAME/POSTGRES_DB are set, got %v", err)
+	}
+}

internal/backup/verify.go

@@ -229,6 +229,18 @@ func runRestoreTest(ctx context.Context, cfg *config.Config, backupFile string,
 		smokeCmd := exec.CommandContext(ctx, "docker", smokeArgs...)
 		output, err := smokeCmd.CombinedOutput()
 		if err != nil {
+			// The gate query (user table count) is the one condition this
+			// whole check exists to enforce. If IT errors out — psql auth
+			// failure, container gone, whatever — that is not "unknown,
+			// carry on": failing to even run the assertion is the same
+			// hollow-gate shape as the drill's zero-row bug (a success
+			// returned with no positive check behind it). Every other
+			// smoke query is advisory and may legitimately fail without
+			// aborting the restore-test.
+			if sq.Label == smokeQueryCatalog[0].Label {
+				return fmt.Errorf("%w: could not run the user-table gate query (%s): %s",
+					errs.ErrBackupVerifyFailed, err, strings.TrimSpace(string(output)))
+			}
 			slog.Warn("smoke query error", "label", sq.Label, "error", err)
 			failedQueries++
 			continue