langgenius/dify
1
0
Fork 0
mirror of https://github.com/langgenius/dify.git synced 2026-07-21 12:05:30 -04:00
Code Issues 775 Packages Projects Releases 10 Wiki Activity

[PR #32299] fix(api): make DB migration Redis lock TTL configurable and prevent LockNotOwnedError from masking failures #33654

New Issue
Closed
opened 2026-02-21 20:53:40 -05:00 by yindo · 0 comments
yindo commented 2026-02-21 20:53:40 -05:00
Owner
Copy Link
Copy Source

Original Pull Request: https://github.com/langgenius/dify/pull/32299

State: closed
Merged: Yes

Important

  1. Make sure you have read our contribution guidelines
  2. Ensure there is an associated issue and you have been assigned to it
  3. Use the correct syntax to link this PR: Fixes #<issue number>.

Fixes #32297

This PR hardens startup database migrations (flask upgrade-db) in multi-replica environments (e.g. Kubernetes) to prevent concurrent migrations and ensure failures are observable.

Background / Problems

  • The migration command uses a Redis lock (db_upgrade_lock) to avoid concurrent upgrades, but long-running migrations can outlive a short lock TTL, allowing a second instance to acquire the lock and start a concurrent migration.
  • When the lock expires or ownership changes, lock cleanup can raise LockNotOwnedError, which can distract from or mask the real Alembic/DB migration failure in logs.
  • Simply increasing the TTL is not sufficient: if a pod is killed mid-migration and cannot release the lock, a long TTL can leave a stale lock and block future upgrades for too long.

Changes

  • Use a short base lock TTL (fixed in code) and keep it alive via heartbeat renewal while migrations are running:
    • Introduce a migration-specific helper DbMigrationAutoRenewLock that renews the redis-py lock TTL using lock.reacquire() from a daemon heartbeat thread.
    • The underlying lock is created with thread_local=False so the renewal thread can access the token.
    • If the process is killed, renewal stops and the lock naturally expires soon, avoiding long stale-lock windows.
  • Make lock release best-effort and non-masking:
    • Lock renewal/release errors (LockNotOwnedError / RedisError) are logged at warning level with stack traces (exc_info=True) for debugging.
    • These errors never change the migration exit code and do not mask the real migration failure (Alembic/DB error still results in exit 1).
  • Remove TTL env/config plumbing:
    • Drop the MIGRATION_LOCK_TTL environment/config option and any docker compose exposure, to keep behavior consistent and avoid stale-lock tuning pitfalls.
  • Tests:
    • Unit tests cover skip-on-lock-busy, failure not masked by release errors, heartbeat renewal behavior, and renewal error tolerance.
    • Add a TestContainers integration test using real Redis to validate TTL renewal and release end-to-end.

No new dependencies.

Screenshots

Before After
N/A (backend-only, no UI change) N/A

Checklist

  • This change requires a documentation update, included: Dify Document
  • I understand that this PR may be closed in case there was no previous discussion or issues. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.
  • I ran make lint and make type-check (backend) and cd web && npx lint-staged (frontend) to appease the lint gods
**Original Pull Request:** https://github.com/langgenius/dify/pull/32299 **State:** closed **Merged:** Yes --- > [!IMPORTANT] > > 1. Make sure you have read our [contribution guidelines](https://github.com/langgenius/dify/blob/main/CONTRIBUTING.md) > 1. Ensure there is an associated issue and you have been assigned to it > 1. Use the correct syntax to link this PR: `Fixes #<issue number>`. Fixes #32297 This PR hardens startup database migrations (`flask upgrade-db`) in multi-replica environments (e.g. Kubernetes) to prevent concurrent migrations and ensure failures are observable. ### Background / Problems - The migration command uses a Redis lock (`db_upgrade_lock`) to avoid concurrent upgrades, but long-running migrations can outlive a short lock TTL, allowing a second instance to acquire the lock and start a concurrent migration. - When the lock expires or ownership changes, lock cleanup can raise `LockNotOwnedError`, which can distract from or mask the real Alembic/DB migration failure in logs. - Simply increasing the TTL is not sufficient: if a pod is killed mid-migration and cannot release the lock, a long TTL can leave a stale lock and block future upgrades for too long. ### Changes - Use a short base lock TTL (fixed in code) and keep it alive via heartbeat renewal while migrations are running: - Introduce a migration-specific helper `DbMigrationAutoRenewLock` that renews the redis-py lock TTL using `lock.reacquire()` from a daemon heartbeat thread. - The underlying lock is created with `thread_local=False` so the renewal thread can access the token. - If the process is killed, renewal stops and the lock naturally expires soon, avoiding long stale-lock windows. - Make lock release best-effort and non-masking: - Lock renewal/release errors (`LockNotOwnedError` / `RedisError`) are logged at `warning` level with stack traces (`exc_info=True`) for debugging. - These errors never change the migration exit code and do not mask the real migration failure (Alembic/DB error still results in exit 1). - Remove TTL env/config plumbing: - Drop the `MIGRATION_LOCK_TTL` environment/config option and any docker compose exposure, to keep behavior consistent and avoid stale-lock tuning pitfalls. - Tests: - Unit tests cover skip-on-lock-busy, failure not masked by release errors, heartbeat renewal behavior, and renewal error tolerance. - Add a TestContainers integration test using real Redis to validate TTL renewal and release end-to-end. No new dependencies. ## Screenshots | Before | After | |--------|-------| | N/A (backend-only, no UI change) | N/A | ## Checklist - [ ] This change requires a documentation update, included: [Dify Document](https://github.com/langgenius/dify-docs) - [x] I understand that this PR may be closed in case there was no previous discussion or issues. (This doesn't apply to typos!) - [x] I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change. - [x] I've updated the documentation accordingly. - [ ] I ran `make lint` and `make type-check` (backend) and `cd web && npx lint-staged` (frontend) to appease the lint gods
yindo added the pull-request label 2026-02-21 20:53:40 -05:00
yindo closed this issue 2026-02-21 20:53:40 -05:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Knowledge Pipeline
Proposal
WIP
cloud
dependencies
feat:webapp
good first issue
outdated
performance
pull-request
Mirrored from GitHub Pull Request
 python:uv
refactor
size:L
stale
status: accepting prs
status: blocked
status: in discussion
stt & tts
vulnerablity
web
wont-fix
🌊 feat:workflow
🌚 invalid
🐞 bug
👻 feat:rag
💪 enhancement
📚 documentation
🔨 feat:tools
🙋‍♂️ question
🙏 help wanted
🤔 cant-reproduce
🤖 feat:agent
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
yindo
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: langgenius/dify#33654
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?