Skip to content

fix: propagate migration body read errors to the database driver - #1417

Open
milanobrtlik wants to merge 1 commit into
golang-migrate:masterfrom
milanobrtlik:fix/buffer-propagate-read-error
Open

fix: propagate migration body read errors to the database driver#1417
milanobrtlik wants to merge 1 commit into
golang-migrate:masterfrom
milanobrtlik:fix/buffer-propagate-read-error

Conversation

@milanobrtlik

Copy link
Copy Markdown

Follow-up to #1308.

Problem

Migration.bufferWriter is the write half of an io.Pipe and Migration.BufferedBody is the read half that runMigrations passes to database.Driver.Run.

#1308 correctly made Buffer() always close bufferWriter via a deferred call, which fixed the reported case of a driver blocking forever on BufferedBody. However, io.PipeWriter.Close() closes with a nil error, so the reader observes a clean io.EOF — including when reading the migration body actually failed.

Both error returns in Buffer() are affected:

if _, err := b.Peek(int(m.BufferSize)); err != nil && err != io.EOF {
    return err          // nothing written to the pipe yet
}
...
n, err := b.WriteTo(m.bufferWriter)
if err != nil {
    return err          // partial body already written to the pipe
}

The error returned by Buffer() does not compensate for this, because Buffer() runs in a goroutine started by readUp/readDown whose only handling is m.logErr(err) — and logErr is a no-op when no logger is configured, which is the default for library use:

go func() {
    if err := migr.Buffer(); err != nil {
        m.logErr(err)
    }
}()

So when a source fails mid-read (S3, GCS, GitHub and other network-backed sources are the realistic trigger), runMigrations does:

SetVersion(target, dirty=true)
Run(BufferedBody)   -> reads EOF, executes a truncated or empty body, returns nil
SetVersion(target, dirty=false)

The result is that a partially-read migration is executed and then recorded as fully applied and clean. Nothing fails and nothing is logged, so the next up continues from a schema that was never fully migrated. That is worse than the migration failing outright, where the dirty flag at least stops the next run.

Fix

Close the pipe with the read error instead of a nil error, so the failure reaches the driver:

if berr != nil {
    if err := m.bufferWriter.CloseWithError(berr); err != nil {
        berr = errors.Join(berr, err)
    }
} else if err := m.bufferWriter.Close(); err != nil {
    berr = errors.Join(berr, err)
}

Run then returns the error, runMigrations propagates it to the caller, and the version row is correctly left dirty. The writer is still closed unconditionally, so the guarantee added in #1308 is preserved.

This required changing the unexported field bufferWriter from io.WriteCloser to *io.PipeWriter to reach CloseWithError. The field is private and only referenced inside migration.go; NewMigration already assigns it from io.Pipe(). There is no change to the exported API.

Tests

TestBufferPropagatesReadError covers a body that fails before any data is read, after a partial read, and after the buffer is filled — the last one exercising the WriteTo return site rather than Peek. Each asserts that the error surfaces to a reader of BufferedBody, which is what a database driver does with it.

The test fails on master (the read returns <nil>, i.e. a clean EOF with truncated content) and passes with this change. No Docker required.

Migration.Buffer closes bufferWriter in a deferred call so that readers of
BufferedBody can never block (golang-migrate#1308). Because io.PipeWriter.Close closes with
a nil error, the reader observes a clean io.EOF even when reading the
migration body failed.

The read error itself only reaches m.logErr in the goroutine started by
readUp/readDown, and logErr is a no-op when no logger is configured. So on a
failed body read runMigrations hands the driver a truncated (or empty) body,
Run returns nil, and the migration is recorded as applied and clean:

	SetVersion(target, dirty=true)
	Run(BufferedBody)  -> reads EOF, executes partial SQL, returns nil
	SetVersion(target, dirty=false)

Close the pipe with the read error instead, so the driver fails, the error
reaches the caller, and the version is correctly left dirty.
@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 54.471% (+0.06%) from 54.412% — milanobrtlik:fix/buffer-propagate-read-error into golang-migrate:master

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.

2 participants