fix(backup): close hollow-gate path where RestoreDrill.Success ignored zero rows - #414
Merged
Merged
Conversation
…d zero rows RestoreDrill() unconditionally set RestoreDrillResult.Success = true once restore/verify/critical-table steps returned no Go error, never consulting RowsVerified itself — only the separate Drill()/smokeCheck wrapper enforced the zero-row hard-fail (added in bddfc1c). Anything calling RestoreDrill() directly, chiefly `nself db restore-drill` (cmd/commands/db_pitr_ops.go), could restore genuinely nothing on a multi-schema stack and still print "Drill status: PASS" with "Rows sampled: 0" — the same hollow-gate shape proven live on staging 2026-08-31, one call-frame lower. Move the smoke-gate assertion (smokeCheck, already covered by TestSmokeCheck_ZeroRowsFails et al.) into RestoreDrill() itself so Success=true is impossible to produce without RowsVerified > 0 and the table-count floor, regardless of caller. Drill()'s own smokeCheck call stays as a second, now largely redundant, layer. Added multi-schema Docker/Postgres integration tests (restore_drill_integration_test.go) reproducing nself-web's shape — tables split across `public` + a second schema: - MultiSchemaRowsCounted: real rows in both schemas restore and count correctly (RowsVerified sums across every non-system schema, not just public). - ZeroRowsNeverSucceeds: same table count, zero rows in all of them — proven to fail against the pre-fix code (Success=true, tables=8 rows=0) and pass against the fix. Also, same hollow-gate shape found elsewhere on the backup path: - internal/backup/verify.go: the restore-test's user-table gate query silently continued past its own psql/docker error instead of failing — only a returned "0" was treated as failure, an error running the check at all was not. Now a hard fail for the gate query specifically. - internal/backup/create.go: `nself backup create` derived a docker container name from PROJECT_NAME/POSTGRES_DB without checking they were actually configured (both silently default via ApplyDefaults), so an unset .env produced a confusing "no such container" error instead of naming the missing variable. Added requireBackupContainerConfig(). - internal/config: BACKUP_ACCESS_KEY/BACKUP_SECRET_KEY (used verbatim by ntask/backend/.env.example) were listed as "known" env vars but never read into BackupConfig — only BACKUP_S3_ACCESS_KEY_ID/ BACKUP_S3_SECRET_ACCESS_KEY were. Remote backup upload was silently non-functional. Both names now accepted (canonical wins if both set), wired into the actual rclone upload's environment, and requireCompleteS3Credentials() refuses a half-configured pair instead of silently proceeding. internal/database/restore_drill.go and internal/backup/create_targets.go split (restore_drill_verify.go, create_remote.go) to stay under the 300-line file cap after the above additions — pure moves, no behavior change beyond what's described above.
acamarata
enabled auto-merge (squash)
September 12, 2026 14:39
The doc-sync gate flagged this PR for changing env vars without updating the env-var inventory, and it was right: BACKUP_ACCESS_KEY and BACKUP_SECRET_KEY are now read as aliases for BACKUP_S3_ACCESS_KEY_ID and BACKUP_S3_SECRET_ACCESS_KEY, and none of the BACKUP_S3_* family was documented at all. Adds a Backup and Restore section covering the whole family, the alias precedence (canonical wins when both are set), why the aliases exist, and the half-configured-pair rejection. Notes that both-empty is valid because the remote may be configured via rclone.conf, and that BACKUP_S3_BUCKET and BACKUP_S3_PREFIX are app-level rather than CLI config-struct fields.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RestoreDrill()itself soSuccess=trueis now structurally impossible withoutRowsVerified > 0and the table-count floor, regardless of caller.public+ a second schema, matching nself-web's shape): one proving real multi-schema rows are counted correctly, one proving a zero-row restore can never report success (fails against pre-fix code, passes against the fix).verify.go's restore-test gate query silently continued past its own error instead of failing;create.goderived a docker container name from unsetPROJECT_NAME/POSTGRES_DBinstead of naming the missing variable;BACKUP_ACCESS_KEY/BACKUP_SECRET_KEY(used byntask/backend/.env.example) were recognized as known env vars but never read into config, silently disabling remote backup upload.Test plan
go build ./...go vet ./...gofmt -lcleangolangci-lint runclean on touched packagesgo test ./...— full suite green, includinginternal/repoqa(file-size + gofmt gates)INTEGRATION=1 go test -tags integration ./internal/database/... -run TestRestoreDrill_Integration— both new tests pass against the fixSuccess = truefortables=8 rows=0) and passes against the fixPROJECT_NAME/POSTGRES_DBguard (no Docker required)