Skip to content

fix(migration): stop a half-applied startup migration from committing silently - #6182

Open
n0ctal wants to merge 1 commit into
MHSanaei:mainfrom
n0ctal:upstream-migration-commit-errors
Open

fix(migration): stop a half-applied startup migration from committing silently#6182
n0ctal wants to merge 1 commit into
MHSanaei:mainfrom
n0ctal:upstream-migration-commit-errors

Conversation

@n0ctal

@n0ctal n0ctal commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Return and check the error from MigrationRequirements, stop inner := assignments from shadowing it, and propagate commit failures in both the startup migration and OutboundService.AddTraffic.

Why

MigrationRequirements wraps the startup migration in a transaction whose deferred handler commits when err == nil. Two separate things defeat that guard:

  1. Shadowing. Several steps inside the loop declare their own error with :=modifiedSettings, err := json.MarshalIndent(...), modelClients, err := s.GetClients(...). A failure there returns early while the outer err is still nil, so the deferred handler commits a migration that stopped halfway.

  2. Discarded commit error. tx.Commit()'s result was dropped. A commit that failed was indistinguishable from one that succeeded.

MigrateDB then ignored the outcome entirely, so a startup migration could fail, partially commit, and leave no trace. On the next start the panel runs against a schema state nobody intended, and the original failure is unrecoverable from the logs.

OutboundService.AddTraffic had the same discarded-commit shape in its own hand-rolled transaction.

Scope

  • MigrationRequirements returns error; the inner assignments use the named return instead of declaring new variables.
  • The commit result is checked and propagated rather than dropped.
  • MigrateDB logs a failed migration instead of ignoring it.
  • AddTraffic uses database.GetDB().Transaction(...), which surfaces the commit error, in place of the manual begin/defer/commit block.

Validation

  • go build ./internal/... clean.
  • go test ./internal/web/service/ ./internal/web/service/outbound/ green on main at ece16559 with this applied.
  • Cherry-picks onto current main without conflict.

Risk

Low, and it is a behaviour change only in the failing case: a migration that would previously have committed half-applied now rolls back and is logged. No schema, API, or configuration change. AddTraffic's signature and its (error, bool) contract are unchanged.

@n0ctal
n0ctal force-pushed the upstream-migration-commit-errors branch from fdaf2e7 to 8ce4d2f Compare August 8, 2026 02:10
@n0ctal
n0ctal force-pushed the upstream-migration-commit-errors branch from 8ce4d2f to 4e4d934 Compare August 8, 2026 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant