zombor/pergamum
1
0
Fork 0
Code Issues Pull requests 3 Projects Releases Packages 2 Wiki Activity Actions

fix(dbtest): comment accuracy + GET_LOCK/ctx deadline note (bookshelf-061u) #800

Merged
zombor merged 1 commit from bd-bookshelf-061u into main 2026-06-28 05:53:29 +00:00
Conversation 0 Commits 1 Files changed 1 +17 -3
zombor commented 2026-06-28 05:24:11 +00:00
Owner
Copy link

Summary

Three review minors from PR #587 code review (comment 6925):

  1. cloneSchemaFromTemplate schema_migrations error path: previously returned nil with a misleading comment claiming "migrations will re-apply". This is wrong — the table DDL was already cloned by the loop above, so re-running migrations would fail with "table already exists". Now returns the error to surface the real problem.

  2. ensureTemplateDB GET_LOCK/ctx deadline: added a comment explaining that the 120s GET_LOCK MySQL timeout and the caller ctx deadline are independent; whichever fires first wins (effective cap = min(ctx deadline, 120s)).

  3. templateIsReady comment accuracy: the error-swallow path says "absent or unreadable" but actually swallows all errors (network, context cancellation, absent table). Updated comment accurately describes the behaviour.

Test plan

  • go build ./internal/dbtest/... compiles clean
  • go build -tags integration ./internal/dbtest/... compiles clean
  • make test passes (unit suite, no DB required)
  • CI integration suite exercises NewSuiteDB + StartSharedMySQL (the paths that exercise the happy path through all three changed functions)

Closes bead bookshelf-061u on merge.

## Summary Three review minors from PR #587 code review (comment 6925): 1. **`cloneSchemaFromTemplate` schema_migrations error path**: previously returned `nil` with a misleading comment claiming "migrations will re-apply". This is wrong — the table DDL was already cloned by the loop above, so re-running migrations would fail with "table already exists". Now returns the error to surface the real problem. 2. **`ensureTemplateDB` GET_LOCK/ctx deadline**: added a comment explaining that the 120s GET_LOCK MySQL timeout and the caller `ctx` deadline are independent; whichever fires first wins (effective cap = `min(ctx deadline, 120s)`). 3. **`templateIsReady` comment accuracy**: the error-swallow path says "absent or unreadable" but actually swallows all errors (network, context cancellation, absent table). Updated comment accurately describes the behaviour. ## Test plan - [x] `go build ./internal/dbtest/...` compiles clean - [x] `go build -tags integration ./internal/dbtest/...` compiles clean - [x] `make test` passes (unit suite, no DB required) - [x] CI integration suite exercises `NewSuiteDB` + `StartSharedMySQL` (the paths that exercise the happy path through all three changed functions) Closes bead bookshelf-061u on merge.
fix(dbtest): address comment accuracy and GET_LOCK/ctx deadline note (bookshelf-061u)
All checks were successful
/ Lint (pull_request) Successful in 3m0s
Details
/ JS Unit Tests (pull_request) Successful in 3m39s
Details
/ E2E API (pull_request) Successful in 4m19s
Details
/ Integration (pull_request) Successful in 5m10s
Details
/ Test (pull_request) Successful in 5m15s
Details
/ E2E Browser (pull_request) Successful in 5m21s
Details
358534bc20
 
Three review minors from PR #587 code review (comment 6925):

1. cloneSchemaFromTemplate: return error instead of nil when querying
   schema_migrations from the template fails. The previous comment claimed
   "migrations will re-apply" but that is incorrect — the table DDL was
   already cloned by the tables loop, so golang-migrate would find existing
   tables and fail with "table already exists". Returning the error surfaces
   the real problem.

2. ensureTemplateDB: document that GET_LOCK's 120s MySQL-side timeout and
   the caller ctx deadline are independent; whichever fires first wins
   (min(ctx deadline, 120s) is the effective cap).

3. templateIsReady: fix comment on the schema_migrations query error path —
   it swallows ALL errors (network, context, absent table), not just the
   absent-table case. Updated comment accurately describes the behaviour.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
zombor merged commit 13cad36268 into main 2026-06-28 05:53:29 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
zombor/pergamum!800
No description provided.