Repository navigation
fix: v0.3.1 — timestamp-version panic in collectApplied (BMW P0) - #5
Merged
Merged
Conversation
…panics)
Add driver_timestamp_test.go with 3 integration tests using BMW-style
timestamp-based versions (20240101000001, 20240201000001, 20240301000001):
- TestDriver_Up_TimestampVersions
- TestDriver_Down_TimestampVersions
- TestDriver_Down_TimestampVersions_ToNil
PRE-FIX PANIC (confirmed locally against postgres:16-alpine):
panic: runtime error: makeslice: cap out of range [recovered, repanicked]
collectApplied(0x0, 0x12688ff38541)
internal/golangmigrate/driver.go:182 +0x54
(*Driver).Up(...)
internal/golangmigrate/driver.go:47 +0x2bc
The cap out of range occurs because collectApplied(before=0, after=20240101000001)
allocates make([]string, 0, 20240101000001) — a ~2×10^13 element slice.
This test must be committed before the fix so code-reviewer can verify the
regression-catches-test invariant independently.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sInRange
Root cause: collectApplied(before, after uint) pre-allocated
make([]string, 0, after-before)
assuming sequential version numbers. BMW uses timestamp-based versions
(e.g. 20240101000001), so after-before = 2×10^13 → makeslice: cap out
of range panic. Same bug appeared in Down() with integer-range loops.
Fix: introduce versionsInRange(dir, lo, hi, loNil) that walks the file
source (same pattern as the existing listPendingVersions helper) to
enumerate only versions actually present in the source directory.
Integer arithmetic is never used for capacity allocation.
- Up(): replace collectApplied(before, after) with versionsInRange
- Down(): replace integer-range loops with versionsInRange + reverse slice
- Delete collectApplied() entirely
POST-FIX test output (postgres:16-alpine, same harness):
=== RUN TestDriver_Up_TimestampVersions
applied: [20240101000001 20240201000001 20240301000001]
Status after Up: current=20240301000001 pending=[]
--- PASS (0.14s)
=== RUN TestDriver_Down_TimestampVersions
rolled back: [20240301000001]
Status after Down: current=20240201000001 pending=[20240301000001]
--- PASS (0.13s)
=== RUN TestDriver_Down_TimestampVersions_ToNil
rolled back: [20240301000001 20240201000001 20240101000001]
--- PASS (0.12s)
Full suite: GOWORK=off go test -race -count=1 ./... — all packages pass.
Verified locally against postgres:16-alpine via docker; test panics on
pre-fix code and passes on fixed code.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ry PR
Two jobs triggered on changes to migration driver code:
1. ephemeral-migrate (golang-migrate, timestamp versions)
- Runs workflow-migrate up with timestamp-based fixture migrations
(20240101000001, 20240201000001, 20240301000001) — the v0.3.0
regression scenario.
- Verifies tables created, runs down(1), checks table count.
- Runs workflow-migrate status.
2. ephemeral-migrate-goose (goose driver, sequential versions)
- Same shape using --driver goose and goose-format fixtures.
- Verifies up creates 2 tables, down rolls back 1.
Without the v0.3.1 fix applied, the `Run workflow-migrate up` step
panics with `makeslice: cap out of range` — blocking merge. This is the
prevention mechanism that would have caught v0.3.0's panic before release.
Note: uses `workflow-migrate` binary directly. When wfctl gains a
`migrations validate` subcommand (task #78), this workflow can be updated
to invoke wfctl instead.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add .golangci.yml (golangci-lint v2 config, previously absent). Add //nolint:errcheck to all existing defer .Close() calls — these are unactionable cleanup paths and the pattern was already present throughout the codebase. Add //nolint:unused to goreleaser-injected version vars (set via -ldflags at build time). Also update CHANGELOG for v0.3.1. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Fixes a panic in the internal/golangmigrate driver when migration versions are timestamp-based (non-sequential), and adds CI coverage to prevent regressions.
Changes:
- Replace sequential arithmetic/range assumptions with filesystem-based enumeration for applied/rolled-back versions.
- Add regression tests covering timestamp-based versions for
Up()andDown(). - Add a GitHub Actions workflow to run
workflow-migrate up/downagainst Postgres 16 with both golang-migrate (timestamp) and goose (sequential) fixtures.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
internal/golangmigrate/driver.go |
Removes sequential-version assumptions by enumerating versions from the source directory via versionsInRange. |
internal/golangmigrate/driver_timestamp_test.go |
Adds regression tests validating timestamp-based version behavior for Up/Down paths. |
.github/workflows/migrations-ci.yml |
Adds CI jobs that exercise workflow-migrate against ephemeral Postgres with representative fixtures. |
.golangci.yml |
Introduces golangci-lint config and exclusions/nolints for pre-existing errcheck/unused patterns. |
pkg/cli/test.go |
Adds //nolint:errcheck on stdout print path. |
pkg/cli/tenant_ensure.go |
Adds //nolint:errcheck on db.Close(). |
internal/goose/driver.go |
Adds //nolint:errcheck on db.Close(). |
internal/atlas/revisions.go |
Adds //nolint:errcheck on rows.Close(). |
cmd/workflow-plugin-migrations/main.go |
Adds //nolint:unused for build-time injected version var. |
cmd/workflow-plugin-atlas-migrate/main.go |
Adds //nolint:unused for build-time injected version var. |
CHANGELOG.md |
Documents the v0.3.1 fix and the new CI workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+76
to
+93
| // versionsInRange returns versions in (after, before] ascending order; | ||
| // we reverse to produce highest-first (the order rolled back). | ||
| ascending, _ := versionsInRange(req.Source.Dir, after, before, afterNil) | ||
| for i := len(ascending) - 1; i >= 0; i-- { |
Contributor
Author
There was a problem hiding this comment.
Fixed in commit 2cd24b7. All five findings from the Copilot review are addressed in that commit.
Comment on lines
+195
to
+199
| v, err := s.First() | ||
| for err == nil { | ||
| if (loNil || v > lo) && v <= hi { | ||
| result = append(result, fmt.Sprintf("%d", v)) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Fixed in commit 2cd24b7. All five findings from the Copilot review are addressed in that commit.
|
|
||
| // Capture the current version before applying. | ||
| before, _, _ := m.Version() | ||
| before, _, beforeErr := m.Version() |
Contributor
Author
There was a problem hiding this comment.
Fixed in commit 2cd24b7. All five findings from the Copilot review are addressed in that commit.
Comment on lines
+47
to
+51
| after, _, _ := m.Version() | ||
| applied := collectApplied(before, after) | ||
|
|
||
| // Walk the source directory to enumerate applied versions; this is safe | ||
| // for timestamp-based version numbers where after-before can be 10^13. | ||
| applied, _ := versionsInRange(req.Source.Dir, before, after, beforeNil) |
Contributor
Author
There was a problem hiding this comment.
Fixed in commit 2cd24b7. All five findings from the Copilot review are addressed in that commit.
Finding 1+2 (migrations-ci.yml): add top-level `permissions: contents: read` — minimum CodeQL-required scope. Finding 3 (Up — before version): propagate non-ErrNilVersion errors from m.Version() before migration. Previously a DB error set before=0 and silently produced a wrong applied list. Finding 4 (Up — after version + versionsInRange error): propagate non-ErrNilVersion afterErr; log (not swallow) versionsInRange error when migration succeeded but enumeration fails. Finding 5 (Down — symmetric): same before/after error handling applied to Down(); log versionsInRange enumeration error. Finding 6 (versionsInRange + listPendingVersions — end-of-stream sentinel): source.Driver.Next() returns os.ErrNotExist to signal end-of-stream per the golang-migrate source.Driver contract (source/driver.go:57). Rewrite iteration loop to distinguish os.ErrNotExist (normal break) from real errors (propagated to caller). Apply same fix to listPendingVersions for consistency. All existing tests pass; golangci-lint 0 issues. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes a production P0 panic in the golang-migrate driver when migration versions are timestamp-based (large numeric gaps), by replacing sequential integer arithmetic/loops with filesystem source enumeration, and adds CI coverage to prevent regressions.
Changes:
- Replace
collectApplied()and integer-range loops withversionsInRange()that walks the migration source directory (safe for timestamp versions). - Add regression tests covering
Up()/Down()behavior with timestamp-based versions. - Add a GitHub Actions workflow to run
workflow-migrateup/down against ephemeral Postgres on PRs touching migration code, plus introduce a repo-wide.golangci.ymland targeted//nolintannotations.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/test.go | Adds //nolint:errcheck for stdout printing to satisfy errcheck linting. |
| pkg/cli/tenant_ensure.go | Adds //nolint:errcheck for deferred DB close to align with lint configuration. |
| internal/goose/driver.go | Adds //nolint:errcheck on deferred DB closes. |
| internal/golangmigrate/driver_timestamp_test.go | Adds regression tests for timestamp-based version behavior in Up/Down. |
| internal/golangmigrate/driver.go | Removes sequential version assumptions; enumerates versions via source walking to avoid overflow/panic. |
| internal/atlas/revisions.go | Adds //nolint:errcheck for deferred rows close. |
| cmd/workflow-plugin-migrations/main.go | Marks build-time version var as intentionally unused for linting. |
| cmd/workflow-plugin-atlas-migrate/main.go | Marks build-time version var as intentionally unused for linting. |
| CHANGELOG.md | Documents v0.3.1 fix and CI/lint additions. |
| .golangci.yml | Adds golangci-lint configuration (errcheck/govet/staticcheck/unused + exclusions). |
| .github/workflows/migrations-ci.yml | Adds CI workflow running up/down against Postgres with timestamp and sequential fixtures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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
collectApplied(before, after uint)pre-allocatedmake([]string, 0, after-before)assuming sequential version numbers. BMW uses timestamp-based versions (e.g.20240101000001), soafter-before = 2×10¹³→makeslice: cap out of rangepanic. v0.3.0 is defective and should not be used with timestamp-based migrations.collectApplied. IntroduceversionsInRange(dir, lo, hi, loNil)that walks the file source directory (same pattern as the existinglistPendingVersions). No integer arithmetic for capacity allocation. Fix same bug class inDown()at the integer-range loops.migrations-ci.yml— runsworkflow-migrate up/downagainst ephemeral Postgres:16 on every PR using timestamp-based fixture migrations. This is the prevention gate that would have caught v0.3.0's panic before release.Pre-fix panic (confirmed locally, postgres:16-alpine)
Post-fix test output
Verified locally against postgres:16-alpine via docker; test panics on pre-fix code and passes on fixed code.
Commit structure
fa9ecbb— Regression test only (pre-fix — panics). Committed before fix so regression invariant is independently verifiable.fec39f4— Fix: deletecollectApplied, introduceversionsInRangee784a17—migrations-ci.ymlGHA workflow (2 jobs: golang-migrate timestamp + goose sequential)42fdf01—.golangci.yml+//nolintannotations for pre-existing patterns + CHANGELOG v0.3.1Test plan
TestDriver_Up_TimestampVersionspanics onfa9ecbb, passes onfec39f4(code-reviewer verified independently)TestDriver_Down_TimestampVersionspasses post-fixTestDriver_Down_TimestampVersions_ToNilpasses post-fixGOWORK=off go test -race -count=1 ./...— all packages passgolangci-lint run— 0 issuesmigrations-ci.ymlYAML valid (python3 yaml.safe_load)🤖 Generated with Claude Code