Fix backup import across Docker mounts

[dngrtech]

Summary

• stage each restored file tree inside its own managed filesystem so Docker bind mounts no longer fail with EXDEV
• make partial swaps rollback-safe, reserve internal restore paths from imports/exports, and keep cleanup errors from misreporting committed restores
• serialize backup maintenance with entity tasks through owner-checked Redis leases and acquire entity locks before creation/rename mutations
• publish v1.25.1 with the approved Bug fixes and improvements release entry

Root cause

Global backup restore staged files under /app and then used atomic rename into separately bind-mounted SSH key, Terraform state, inventory, and config directories. Linux rejects those cross-filesystem renames with Errno 18.

Test plan

• TDD regressions reproduced the production EXDEV failure and partial-swap failures before implementation
• full backend baseline before the final lock-order fix: 1,487 passed
• fresh post-fix affected matrix: 225 passed
• Python compilation and git diff whitespace checks passed
• GitHub Actions will run the complete post-fix suite

Release

• VERSION: 1.25.1
• docs/user/version.json: 1.25.1
• docs/user/releases.md: v1.25.1 / PR Fix backup import across Docker mounts #170 / Bug fixes and improvements

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[github-actions]

Code Review — Fix backup import across Docker mounts (v1.25.1)

Strengths

Cross-filesystem fix is elegant and correct (ui/task_logic/backup_import.py:restore_backup_archive). Moving each tree's staging directory inside its own managed root guarantees every os.rename stays on one filesystem. This directly fixes the EXDEV production failure with zero risk of subtle correctness regressions.

_swap_tree is now locally exception-safe (backup_import.py:160–189). The displaced/installed bookkeeping with inner rollback before re-raise is exactly the right shape — primary exception preserved, secondary rollback errors logged without obscuring it.

Maintenance lock protocol is well-designed (ui/task_lock.py). Owner-token pattern, bounded TTL with daemon keepalive, and owner-only refresh/release all follow distributed-lock best practices. The separation between acquire_maintenance_lock (takes explicit client) and backup_maintenance_lock (fetches client internally) is clean.

flush-then-lock-then-commit pattern (ui/database.py, host/instance routes). Flushing to obtain an ORM-assigned ID before acquiring the entity lock, then committing only after the lock is held, closes the window where a row could be committed without a corresponding Redis lock.

Post-commit cleanup semantics are correct (backup_import.py:finally). Pre-commit cleanup is on the error path; post-commit cleanup is best-effort with per-path logging and no HTTP-500 promotion. The committed flag separates the two phases cleanly.

Reserved namespace protection (backup_files.py:is_restore_child, _swap_tree:excluded). .qlsm-restore-* children are excluded from exports, restore scans, and staged-child installation. An adversarially crafted archive hiding data under that prefix would be silently dropped — correct behaviour.

Test coverage is comprehensive — fault injection tests for both displacement and installation failures, a maintenance-contention test with a real ScriptRedis double that evaluates Lua semantics, post-commit cleanup failure with export-leakage verification, and the Docker-boundary regression using a monkeypatched os.rename guard. The design+findings+assessment paper trail is unusually thorough.

---

Issues

Critical (Must Fix)

None identified.

---

Important (Should Fix)

1. SCAN loop inside Lua blocks Redis atomically — unbounded for large key counts

ui/task_lock.py:_ACQUIRE_MAINTENANCE_SCRIPT (~line 37)

repeat
    local result = redis.call('SCAN', cursor, 'MATCH', ARGV[2], 'COUNT', 100)
    cursor = result[1]
    if #result[2] > 0 then return 0 end
until cursor == '0'

Redis executes Lua scripts atomically — no other commands run while this loop iterates. SCAN must page through the full keyspace to confirm zero task_lock:* entries. At typical entity-lock counts (a handful) the loop completes in microseconds. But if the application scales to hundreds of concurrent lock keys, or if the Redis keyspace is large for other reasons, this loop holds the global Redis command lock for milliseconds per iteration, stalling every concurrent caller.

Why it matters: This is the only path that could introduce Redis-level latency into non-backup request handling — any entity-lock acquisition that races with a maintenance acquisition attempt triggers this script.

How to fix: The safest alternative that avoids a full keyspace scan is to maintain a counter key (e.g., task_lock:active_count) incremented/decremented atomically alongside each entity SET/DEL. The maintenance script then checks GET task_lock:active_count == 0 instead of scanning. This eliminates the loop entirely and keeps the Lua scripts to O(1). If changing the entity-lock protocol is out of scope for this PR, document the bounded expected count and add a monitoring note.

---

2. keepalive.join() in backup_maintenance_lock has no timeout

ui/task_lock.py:backup_maintenance_lock (~line 128)

finally:
    stop_event.set()
    try:
        if started:
            keepalive.join()   # no timeout
    finally:
        ...release...

stop_event.wait(MAINTENANCE_REFRESH_INTERVAL) in the keepalive thread wakes immediately once the event is set, so under normal Redis conditions join() returns in milliseconds. But if Redis is unreachable, refresh_maintenance_lock may block for the full socket-connect/read timeout (potentially tens of seconds), and join() blocks the Flask worker for the same duration.

Why it matters: A Redis hiccup at the end of a large restore could stall the Flask worker thread until the TCP timeout fires, preventing it from serving other requests.

How to fix: Add a short timeout to join() (e.g., 5 seconds) and log a warning if the thread hasn't exited, then proceed to release:

keepalive.join(timeout=5)
if keepalive.is_alive():
    log.warning("Backup maintenance keepalive thread did not exit cleanly")

---

3. Stale log.debug hides keepalive expiry during release

ui/task_lock.py:release_maintenance_lock (~line 100)

else:
    log.debug("Backup maintenance lock not released (not owner or expired)")

If the 300-second TTL expires during an unusually long restore (or if the keepalive thread silently fails to refresh), release_maintenance_lock returns False and the condition is logged at DEBUG — invisible in production log levels. The next backup operation will simply time out and reacquire after TTL, so this is not a correctness bug, but an operator would have no visibility into why a backup lock expired mid-operation.

How to fix: Promote to log.warning with a message that makes the expiry diagnosis obvious:

log.warning("Backup maintenance lock not released: token mismatch or TTL expired — "
            "lock may have already recovered via TTL")

---

4. ScriptRedis test double inspects Lua source strings — fragile

tests/test_task_lock.py:ScriptRedis._acquire_maintenance (~line 42)

checks_owner = "'EXISTS', KEYS[1]" in script
checks_tasks = "'SCAN'" in script and '#result[2] > 0' in script

The test double validates behaviour by string-matching the Lua source. Any whitespace change, quote style change, or refactor of the Lua constants will silently break the double's behaviour without a test failure — the test would pass while the double no longer models the real script.

Why it matters: These tests are the primary evidence that entity and maintenance acquisition cannot both win. A silently broken double means that guarantee goes untested.

How to fix: Execute the Lua scripts directly using redis.StrictRedis against a real or embedded Redis (e.g., fakeredis), or restructure ScriptRedis to simulate Redis semantics from its own in-memory state rather than inspecting source text. The _acquire_entity branch already uses "'NX'" in script for the same fragility.

---

5. Redis Cluster: _ACQUIRE_ENTITY_SCRIPT passes keys from two hash slots

ui/task_lock.py:acquire_lock (~line 85)

redis_client.execute_command(
    'EVAL', _ACQUIRE_ENTITY_SCRIPT, 2,
    key,                   # task_lock:host:1   → hash-slot of "host:1"
    MAINTENANCE_LOCK_KEY,  # maintenance_lock:backup → different hash slot
    ...
)

Redis Cluster requires all keys touched by a Lua script to reside in the same hash slot. task_lock:host:1 and maintenance_lock:backup will almost certainly land in different slots. The _ACQUIRE_MAINTENANCE_SCRIPT has the same issue with SCAN.

Why it matters: If the deployment ever moves to Redis Cluster (or a managed Redis Cluster-mode service), all lock acquisitions will raise CROSSSLOT errors and the application will be unable to create hosts, instances, or run backups.

How to fix: Use hash tags to pin all lock keys to a single slot: {lock}:task:host:1, {lock}:maintenance:backup, {lock}:task:* (for SCAN). Alternatively, document that Redis Cluster is unsupported for this application.

---

Minor (Nice to Have)

6. walk_tree lambda captures skip in a closure that could be confusing

ui/task_logic/backup_files.py:walk_tree (~line 63)

excluded = lambda name: is_restore_child(name) or (skip and skip(name))
dirs[:] = [name for name in dirs if not excluded(name)]
files = [name for name in files if not excluded(name)]

The lambda is reassigned at each iteration of os.walk at the current_root == root branch. Moving it outside the loop (since skip and is_restore_child are invariant) would make the intent clearer and avoid creating a new closure every walk iteration:

def _excluded(name):
    return is_restore_child(name) or bool(skip and skip(name))

This is entirely cosmetic but the current lambda also shadows Python's lambda-in-loop capture pitfall in ways that could confuse future readers.

---

7. Two copies of _managed_tree_contents across test files

tests/test_backup_import.py and tests/test_backup_import_validation.py

Both files define _managed_tree_contents independently with slightly different structure (one returns {path: bytes}, the other returns {root: {relpath: bytes}}). If the helper needs updating (e.g., when a new managed tree is added), two files need changing.

Consider extracting to tests/helpers.py or a shared conftest.py fixture — out of scope for this PR if the 300-line file limit is already a constraint, but worth a follow-up.

---

8. _restore_displaced logs at WARNING but continues on rename failure

ui/task_logic/backup_import.py:_restore_displaced

except Exception as error:
    logger.warning(
        'Failed to roll back restore path %s to %s: %s',
        backup_path, target, error,
    )

A rollback rename failure during pre-commit error handling leaves the managed tree in a partial state. Logging at WARNING is appropriate, but the message doesn't indicate this is a rollback failure (as opposed to a cleanup failure), making log triage harder.

Suggested wording: 'Rollback failed — could not restore displaced path %s from %s: %s'

---

Assessment

Ready to merge? Yes, with the keepalive join() timeout fix (issue 2) as the only blocker I'd consider worth gating on before merge.

Reasoning: The core correctness fix (per-tree staging directories + locally exception-safe _swap_tree) is sound and well-tested. The maintenance lock is a significant improvement over the previous racy any_lock_held() check. Issue 1 (SCAN in Lua) is a known Redis pattern that is acceptable at expected scale; issue 4 (Cluster) is not a concern given the application's deployment profile. Issue 2 is a latency risk on a code path that matters (worker stall at cleanup), making it the one fix worth prioritising before merge.