Skip to content

fix(backup): stop locked databases from hanging updates - #84826

Open
fangliquanflq wants to merge 2 commits into
NousResearch:mainfrom
fangliquanflq:fix/backup-locked-db-timeout
Open

fangliquanflq wants to merge 2 commits into
NousResearch:mainfrom
fangliquanflq:fix/backup-locked-db-timeout

Conversation

@fangliquanflq

Copy link
Copy Markdown
Contributor

What does this PR do?

hermes update --backup now stops waiting after a bounded interval when a database under HERMES_HOME remains locked, instead of hanging indefinitely before the update begins. The backup still uses SQLite's snapshot API for WAL consistency, fails closed for the locked file, removes the partial destination, and lets the existing backup failure handling continue safely.

Symptom

When another process holds a persistent write lock on any *.db under HERMES_HOME, the update remains at Creating pre-update backup... with no further output or progress. The reported case required SIGKILL after more than 15 minutes.

Impact

Affected users cannot complete hermes update --backup while a database remains locked. Their only available workaround is to disable the pre-update backup, which removes the safety snapshot for the update.

Bug Cause

Trigger: hermes_cli/backup.py:346 / _safe_copy_db() when sqlite3.Connection.backup() receives repeated SQLITE_BUSY or SQLITE_LOCKED results.

Causal chain:

  1. hermes update --backup includes a live SQLite database under HERMES_HOME in the pre-update archive.
  2. _safe_copy_db() calls Connection.backup() without a deadline, so CPython keeps sleeping and retrying while another process retains the lock.
  3. The backup archive never finishes and the update never reaches the installation step.

Why it is wrong: The backup path promises to fail closed when it cannot create a consistent snapshot, but an unbounded internal retry prevents it from returning a failure.

Working sibling / contrast: An unlocked database completes through the same SQLite snapshot path and preserves committed WAL data. --no-backup also avoids the blocked path, confirming that update execution itself is not the source of the hang.

Ruled out: Falling back to copying only the main database file is not safe because it can omit committed WAL data. The fix therefore retains sqlite3.backup() and bounds only continuous busy or locked periods.

Fix

Use incremental SQLite backup progress callbacks with a 10-second continuous-busy deadline and a zero connection busy timeout. Reset the deadline after successful progress, close the destination before cleanup for Windows compatibility, and remove any partial snapshot on failure.

Related Issue

Fixes #84790

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/backup.py - bound continuous locked-source waits while preserving WAL-consistent snapshot behavior and fail-closed cleanup.
  • tests/hermes_cli/test_backup.py - cover the timeout, connection configuration, close-before-delete behavior, and normal database copying.

How to Test

  1. Hold an exclusive SQLite lock from a separate process and call _safe_copy_db() for that database; verify it returns False after the bounded wait and leaves no destination file.
  2. Copy an unlocked WAL-backed database and verify the snapshot contains the committed data.
  3. Run the focused automated tests:
scripts/run_tests.sh tests/hermes_cli/test_backup.py -k TestSafeCopyDb -v

Result: 3 passed. Real-environment verification also reproduced the prior locked-backup hang and confirmed that the final default deadline returns in approximately 10 seconds with cleanup.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the repository test entry on the relevant tests and all focused tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 10, including a real cross-process SQLite lock

Documentation & Housekeeping

  • Relevant documentation update: N/A - no user-facing configuration or workflow changed
  • cli-config.yaml.example update: N/A - no configuration keys changed
  • CONTRIBUTING.md or AGENTS.md update: N/A - no architecture or workflow changed
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide
  • Tool descriptions/schemas update: N/A - no model tool behavior changed

Screenshots / Logs

Not applicable. The focused test result and real-environment timing are documented above.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard area/install-update Installer, updater, packaging, wheels, doctor sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades duplicate This issue or pull request already exists labels Aug 12, 2026
@alt-glitch

Copy link
Copy Markdown

This was generated by AI during triage.

Duplicate of #68868: both bound SQLite snapshot stalls through a progress callback and no-progress deadline while preserving fail-closed WAL-safe backups.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(backup): stop locked databases from hanging updates

  1. hermes_cli/backup.py _check_backup_progress — the deadline is reset on every non-busy callback (busy_deadline = now + timeout_seconds), so the timeout only bounds consecutive SQLITE_BUSY stretches, not total wall-clock. A source that alternates brief busy states with small progress chunks can keep the copy alive far beyond timeout_seconds. If total-time bounding is intended, document that; otherwise reset only on meaningful page progress (e.g. when _remaining decreases).
  2. timeout=0.0 on the source connection disables sqlite3's internal busy wait entirely, which is correct here since the progress callback owns the deadline — but worth a comment near sqlite3.connect (or confirm one exists) so a future reader doesn't "fix" it back to a default timeout and reintroduce the hang.
  3. Fail-closed cleanup looks right (close backup_conn before dst.unlink on Windows). Minor: the except Exception around the close swallows silently — logging at debug level would aid diagnosis if the partial destination then fails to unlink.
  4. The new test is well-structured, but it fakes both connections; a small integration-style test that uses a real sqlite3 source held by a writer in a second connection (with a short timeout) would guard the actual conn.backup(..., progress=...) interaction.

@fangliquanflq

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. I checked each point against the PR diff, tests, and the version that was salvaged to main as 076b8a5aa:

  1. The timeout intentionally bounds a continuous SQLITE_BUSY/SQLITE_LOCKED period rather than total backup wall time. Successful backup progress resets the deadline so a healthy large or concurrently changing database is not aborted merely for taking longer than ten seconds.
  2. Already addressed: the comment above sqlite3.connect(..., timeout=0.0) explains that the zero timeout prevents SQLite's implicit busy wait from extending the progress-callback deadline.
  3. The existing warning records the snapshot failure, while close failures are handled best-effort consistently with the existing finally cleanup. Extra debug logging could improve diagnostics but does not change the fail-closed result.
  4. A real cross-process SQLite lock was used for the documented Windows verification, while the committed tests deterministically cover the callback deadline, connection configuration, close-before-delete ordering, and a real SQLite success path. A committed lock integration test would add coverage depth, but this review did not identify a failing behavior in the salvaged implementation.

No further branch change is needed for these points.

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

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hermes update --backup hangs forever when a HERMES_HOME *.db is locked by another process (sqlite backup has no timeout)

3 participants