Skip to content

Move DB migrations off the request/connect path into a one-shot deploy-time runner #31

New issue
New issue
Open
Open
Move DB migrations off the request/connect path into a one-shot deploy-time runner#31
Labels
effort/lWeeksimpact/all-usersAffects every userpriority/p2Backlog-worthyseverity/highSignificant harmtriagedItem has been triagedtype/choreMaintenance / non-user-visibleurgency/this-quarterWithin the quarter

Description

@cristim
cristim
opened on Jun 9, 2026

Problem

Database migrations currently run lazily on DB connect, inside the
per-instance ensureDB path (internal/server/app.go), bounded by a
timeout (CUDLY_MIGRATION_TIMEOUT, default now 120s). This couples schema
evolution to request/cold-start traffic and has several structural failure
modes:

  • Per-instance race. Every Lambda cold start (and every Fargate task)
    independently tries to run migrations. Concurrent cold starts race on the
    same schema_migrations row; a migration interrupted mid-run (Lambda
    timeout, ENI drop) leaves dirty = true, which is terminal — every
    later boot then fail-opens and serves 500s on any query needing the
    unapplied columns.
  • No fail-the-deploy signal. Because the app intentionally fail-opens on
    migration failure (so liveness probes pass), a broken migration is invisible
    to the deploy pipeline and to the AWS/Lambda Errors metric. The bad
    build ships and the failure only surfaces as user-facing 500s.
  • Timeout is a band-aid. Raising the bound (PR fix(migrations): opt-in dirty auto-heal, raise timeout default, failure alarm cloud-commitments-cli#1124 took it 20s -> 120s)
    buys headroom but the migration still competes with request latency budgets
    and Lambda's hard 300s ceiling. A genuinely long DDL on a large table is
    still at risk.

This was the root cause of the recent prod outage (migrations 070-073 never
applied; purchase_delay_hours / revocation_window_closes_at /
executed_by_user_id missing). PR LeanerCloud/cloud-commitments-cli#1124 hardened the in-place path
(opt-in dirty auto-heal, higher timeout, a CloudWatch failure alarm) but did
not change where/when migrations run.

Proposed structural fix

Move migrations off the request/connect path into a one-shot, deploy-time
runner:

  • A single dedicated runner (CI deploy step, init container, or a one-shot
    Lambda/Job invoked by the deploy pipeline) runs RunMigrations exactly once
    per deploy, before the new app version starts taking traffic.
  • Long timeout / no request-latency coupling — the runner can take minutes
    for a large index build without competing with request budgets or the
    serving Lambda's 300s ceiling.
  • Fail the deploy on migration failure — a dirty/failed migration aborts
    the rollout instead of fail-opening into a half-broken serving fleet.
  • No per-instance race — the serving Lambda / Fargate task no longer runs
    migrations at all (DB_AUTO_MIGRATE=false on the workload), so concurrent
    cold starts can't race or dirty the row.

Scope / out of scope

Related

Activity

  1. cristim
    added
    triagedItem has been triaged
    priority/p2Backlog-worthy
    severity/highSignificant harm
    urgency/this-quarterWithin the quarter
    impact/all-usersAffects every user
    effort/lWeeks
    type/choreMaintenance / non-user-visible
    on Jun 9, 2026
  2. cristim
    added 2 commits that reference this issue on Jun 9, 2026

    test(migrations): enforce all migrations are transactional + address CR

    7ff2c77

    fix(migrations): opt-in dirty auto-heal, raise timeout default, failu…

    9a4d01c

  3. cristim commented on Jul 28, 2026

    @cristim
    cristim
    on Jul 28, 2026
    MemberAuthor

    Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.

    The 2026-07-28 full-repo review re-derived this and confirmed it is still present on be11bdcb5. Adding two mechanisms this issue does not currently name, both of which widen the blast radius beyond "migrations compete with request latency".

    Where

    • internal/server/app.go:577-597 (ensureDB holds app.dbMu for its full duration, taken at :583)
    • internal/server/app.go:621 (runMigrationsBounded)
    • internal/server/app.go:199 (runMigrationsBoundedWith builds its own context.WithTimeout(context.Background(), timeout))
    • internal/server/app.go:140 (migrationsTimeout, default 120s)
    • Callers: internal/server/http.go:162, internal/server/http.go:228, internal/server/lambda.go:27

    What the review adds

    1. ensureDB holds dbMu for the entire migration, so migrations do not merely compete with request latency, they serialise every other request behind them. This issue describes the per-instance race and the fail-open behaviour, but not the mutex. Every concurrent request on that instance, including /api/scheduled/*, blocks on dbMu for up to the full migrationsTimeout.

    2. The migration runner deliberately ignores the caller's deadline. runMigrationsBoundedWith builds a fresh context.Background() bounded by migrationsTimeout (default 120s, app.go:140), so the caller's 30s HTTP deadline has no effect on the DDL. The caller therefore cannot abandon it; it can only time out itself while the migration continues.

    Failure scenario

    A deploy adds an index build that takes 90s.

    The first request after the cold start enters ensureDB, takes dbMu, and blocks in runMigrationsBounded. Every concurrent request, API traffic and /api/scheduled/* (all of which call ensureDB first), blocks on dbMu, hits its own 30s ceiling, and returns 503 Service temporarily unavailable.

    Two consequences beyond the latency:

    • On Lambda, the invocation is killed at the function timeout while the migration goroutine is still mid-DDL in a container that is then frozen. That is precisely the state that leaves schema_migrations.dirty = true, the terminal failure mode the 120s default was raised to avoid. So the band-aid can itself produce the outcome it was meant to prevent, because the two timeouts (function and migration) are set independently and the longer one wins the DDL.
    • The scheduled tick that fires during this window returns 503 and is simply lost. There is no retry on the /api/scheduled/* path, so a 90s migration window silently drops whatever collection or analytics tick landed in it, with no record that it was skipped.

    Fix direction

    Consistent with the one-shot deploy-time runner already proposed here. If lazy migration must stay as a transitional safety net, one cheap intermediate improvement: do the connect under dbMu and the migration outside it, so a long DDL degrades throughput rather than blocking every request behind a mutex, and report pending / failed through /health as it already does.

  4. cristim
    transferred this issue fromLeanerCloud/cloud-commitments-clion Sep 27, 2026
  5. IAmJSD
    added a commit that references this issue on Oct 26, 2026

    docs(db): clarify migration transaction and recovery comments (#1281)

    a9b7eb8

  6. cristim commented on Nov 9, 2026

    @cristim
    cristim
    on Nov 9, 2026
    MemberAuthor

    Claimed by claude-cc-platform: premise re-checked on main (lazy runMigrationsBounded still on the connect path); writing a plan first (runner shape, deploy ordering, fail-the-deploy signal), no deploy or terraform apply.

  7. cristim commented on Nov 9, 2026

    @cristim
    cristim
    on Nov 9, 2026
    MemberAuthor

    Claimed by claude-cc-platform: plan reviewed (2 adversarial passes, same cold reviewer, all findings applied). Nothing is applied to any cloud.

    Shape. A one-shot runner (cmd/server --migrate, plus a Lambda direct-invoke event handled before ensureDB) that the deploy workflow calls after terraform apply. The app stops migrating lazily once its stack's runner is proven. A broken migration then fails the deploy step.

    First slice (needs no decision, no apply): runner core plus the --migrate flag. I'll start there.

    Owner decisions, recommended default first:

    • D1 prod: keep prod out of the in-deploy migrate step; migrate on demand through a documented invoke (prod tfvars say "manual migrations").
    • D2 ordering: apply, then migrate (the code tolerates an older schema via the existing schema-version gates).
    • D3 failure semantics: app stays fail-open; a dirty row is recovered manually through a guarded force-version option (only when dirty, version equals the dirty one or dirty-1).
    • D4 lazy path: remove only after dev and staging prove the runner, and only after GCP and Azure have their own runner (their tfvars still set auto_migrate=true on the shared image).
    • D5 Fargate: needs ecs:RunTask and ecs:DescribeTasks added in ci-cd-permissions (human bootstrap apply); default Lambda first, Fargate optional.
    • D6 endpoint: runner uses the app's endpoint; long DDL goes through Fargate, since the Lambda ceiling is below the runner default.
    • D7 scope: rescope Move DB migrations off the request/connect path into a one-shot deploy-time runner #31 to AWS plus docs, and file a follow-up for GCP/Azure.
    • D8 live schema check in /health: defer.
    • D9 destructive-migration lint: docs and checklist only.
  8. cristim
    added 5 commits that reference this issue on Nov 10, 2026

    feat(server): add a one-shot deploy-time migration runner

    321b9e0

    feat(server): add a --migrate mode to the server binary

    6373688

    test(server): prove the migration runner against a real database

    af5ca8a

    fix(server): make the runner force guard gap-aware and refuse a stale…

    539fbe0

    feat(server): handle a Lambda migrate event before ensureDB

    7b092dd

  9. cristim commented on Nov 10, 2026

    @cristim
    cristim
    on Nov 10, 2026
    MemberAuthor

    Claimed by claude-cc-platform: S1-S3 landed on main (runner, --migrate flag, Lambda migrate event) plus the review fixes (gap-aware force target, stale force env refused). Main CI green at 7b092dd. Nothing is applied to any cloud. Next: S5 (Lambda deploy step), then docs. Owner decisions D1, D5 and D7 are in the plan comment above.

  10. cristim
    added a commit that references this issue on Nov 12, 2026

    feat(deploy): run database migrations through the Lambda migrate even…

    e61a531

  11. cristim commented on Nov 12, 2026

    @cristim
    cristim
    on Nov 12, 2026
    MemberAuthor

    S5 (the deploy-time migrate step for dev and staging) is on main at e61a531. Decided (owner go 2026-11-12, approval A) by claude-platform: rebased onto current main, reviewed in two passes (approve), pushed. The first dev deploy (run 43285168692, success) ran the migrate step against the dev function: 'migrations ok: from 167 to 167', so the database was already at the latest migration. Prod remains excluded from the step, as planned. #31 stays open for the remaining slices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    effort/lWeeksimpact/all-usersAffects every userpriority/p2Backlog-worthyseverity/highSignificant harmtriagedItem has been triagedtype/choreMaintenance / non-user-visibleurgency/this-quarterWithin the quarter

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions