Skip to content

invoices: bound rapid migration test workload - #11206

Merged
yyforyongyu merged 1 commit into
lightningnetwork:masterfrom
ziggie1984:fix-invoice-rapid-timeout
Sep 21, 2026
Merged

yyforyongyu merged 1 commit into
lightningnetwork:masterfrom
ziggie1984:fix-invoice-rapid-timeout

Conversation

@ziggie1984

@ziggie1984 ziggie1984 commented Sep 19, 2026 •

Copy link
Copy Markdown
Collaborator

Change

TestMigrateSingleInvoiceRapid performed Rapid's default 100 property checks,
but generated and migrated another 100 invoices inside every check for both
SQLite and Postgres. The resulting 20,000 migrations and delayed database
cleanup made the race build exceed the Postgres fixture's fixed ten-minute
lifetime.

  • Run SQLite and Postgres as separate subtests so the Postgres fixture lifetime
    covers only Postgres checks.
  • Close each database when its Rapid property check finishes instead of
    retaining every handle until the outer test exits.
  • Migrate a bounded batch of ten randomized invoices per property check. This
    retains multiple migrations in one transaction and post-commit lookup
    coverage while replacing the accidental 100-by-100 multiplier with 1,000
    generated invoices per backend.

This addresses the repeated race-job timeout observed after the Go 1.27.1
toolchain update in #11200 without increasing the fixture timeout.

Verification

  • Full Go 1.27.1 race run of TestMigrateSingleInvoiceRapid: pass in 142.330s.
  • Fixed-seed 20-check comparison: 30.884s, down from 65.900s.
  • make lint-native: 0 issues.

@ziggie1984 ziggie1984 self-assigned this Sep 19, 2026
@ziggie1984 ziggie1984 added the severity-override-low Manual override to low label Sep 19, 2026
Run the SQLite and Postgres property checks as isolated subtests, close each database after its check, and migrate a bounded batch of randomized invoices per Rapid iteration. This preserves multi-invoice transaction coverage while preventing the accidental 100-by-100 workload from exhausting the Postgres fixture under the race detector.
@ziggie1984
ziggie1984 force-pushed the fix-invoice-rapid-timeout branch from 9e98006 to 381faa6 Compare September 19, 2026 01:12
@github-actions github-actions Bot added the severity-low Best-effort review label Sep 19, 2026
@github-actions

Copy link
Copy Markdown

🟢 PR Severity: LOW

override label | 1 files | 132 lines changed

🟢 Low (1 files)
  • invoices/sql_migration_test.go - test-only file (*_test.go)

Analysis

A severity-override-low label is present on this PR, so automatic classification was skipped and the override was applied directly. For reference, the only file changed (invoices/sql_migration_test.go) is a test file, which would independently classify as LOW under the standard rules.


To override, add a severity-override-{critical,high,medium,low} label.

@Lrifton92 Lrifton92 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 381faa6.

The restructuring matches the description. Each backend runs as its own subtest, so the Postgres fixture only lives through the Postgres checks. Every Rapid check gets a fresh store, whose handle is closed in rt.Cleanup rather than kept until the outer test exits. The per-check batch goes from 100 invoices to 10, which removes the accidental 100×100 multiplier while still putting several migrations in one transaction.

Two changes are improvements beyond the timeout fix:

  • The ExecTx closure now returns the MigrateSingleInvoice error instead of calling require.NoError inside it. FailNow from inside the transaction callback would bypass the executor's rollback and retry handling.
  • The map becomes a slice, so the migration order within a check is deterministic, which helps when Rapid replays a failure.

What I ran on this head:

  • TestMigrateSingleInvoiceRapid/SQLite passes: "[rapid] OK, passed 100 tests". The early db.Close() plus the helper's own cleanup close is exercised on this path without error, since sql.DB.Close is idempotent.
  • The Postgres subtest needs Docker, which I don't have here, so that half is unverified locally.
  • To check that 10 invoices per check still gives useful coverage, I added a break after the first InsertInvoiceHTLCCustomRecord in sql_migration.go, so only one custom record per HTLC is migrated. Rapid fails immediately ("failed after 0 tests") and shrinks to a minimal invoice. So a data-loss bug on a common shape is still caught at the new batch size. Rarer combinations get fewer draws than before (1,000 generated invoices per backend instead of 10,000), which is the stated trade-off.
  • go vet ./invoices/ is clean and no added line exceeds 80 columns.

Minor, non-blocking: each check's Postgres CREATE DATABASE and each SQLite temp dir are still registered on the subtest's t (sqldb/postgres_fixture.go:158, sqldb/sqlite.go:236), so 100 of them accumulate until the subtest ends. The handles are closed early, which is what matters for the fixture, but the comment at lines 583-585 could say the databases themselves stay until the subtest ends.

LGTM.

@bhandras bhandras left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hieblmi
hieblmi self-requested a review September 21, 2026 16:19

@hieblmi hieblmi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@saubyk saubyk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

@yyforyongyu
yyforyongyu merged commit 6145051 into lightningnetwork:master Sep 21, 2026
44 of 46 checks passed
@ziggie1984 ziggie1984 added backport-v0.20.x-branch This label is used to trigger the creation of a backport PR to the branch `v0.20.x-branch`. backport-v0.21.x-branch This label triggers a backport to branch `v0.21.x-branch ` labels Sep 21, 2026
@ziggie1984
ziggie1984 deleted the fix-invoice-rapid-timeout branch September 21, 2026 18:56
@github-actions

Copy link
Copy Markdown

Successfully created backport PR for v0.20.x-branch:

@github-actions

Copy link
Copy Markdown

Successfully created backport PR for v0.21.x-branch:

ziggie1984 added a commit that referenced this pull request Sep 21, 2026
…21.x-branch

[v0.21.x-branch] Backport #11206: invoices: bound rapid migration test workload
ziggie1984 added a commit that referenced this pull request Sep 21, 2026
…20.x-branch

[v0.20.x-branch] Backport #11206: invoices: bound rapid migration test workload
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-v0.20.x-branch This label is used to trigger the creation of a backport PR to the branch `v0.20.x-branch`. backport-v0.21.x-branch This label triggers a backport to branch `v0.21.x-branch ` no-changelog severity-low Best-effort review severity-override-low Manual override to low

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants