Add rescue mode, execution state tracking, and runtimes aggregate

[aliel]

Rescue mode lets users boot a persistent instance from a minimal
rescue image while their original rootfs is attached as a secondary
drive for repair. The original rootfs is never moved or renamed.

POST /control/machine/{ref}/rescue - enter rescue mode
DELETE /control/machine/{ref}/rescue - exit and restore normal boot

Reinstall and erase are blocked while in rescue mode (409 conflict).

Execution state tracking:

• New mode column on ExecutionRecord ("normal" or "rescue"), persisted across supervisor restarts.
• New execution_events audit table records lifecycle events (created, stopped, reinstalled, rescue_entered, rescue_exited, etc.)
• list_executions_v2 response now includes mode.

Runtimes aggregate:

• Fetch and cache logic for the runtimes aggregate from the governance address, same pattern as the settings aggregate.
• RuntimeEntry schema with id, name, type, item_hash, default fields.
• Lookup helpers: get_runtime_by_id, get_default_runtime.
• Rescue image hash is resolved from the aggregate (type "rescue") instead of a hardcoded config value.

[codecov]

Codecov Report

❌ Patch coverage is 81.15502% with 124 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.68%. Comparing base (9b1875d) to head (dc14d4c).
⚠️ Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
src/aleph/vm/orchestrator/views/operator.py	59.64%	62 Missing and 7 partials ⚠️
...ons/versions/0005_add_mode_and_execution_events.py	0.00%	24 Missing ⚠️
src/aleph/vm/orchestrator/utils.py	72.54%	14 Missing ⚠️
src/aleph/vm/controllers/qemu/instance.py	55.55%	2 Missing and 2 partials ⚠️
src/aleph/vm/pool.py	33.33%	3 Missing and 1 partial ⚠️
src/aleph/vm/storage.py	50.00%	3 Missing ⚠️
src/aleph/vm/controllers/qemu/backup.py	0.00%	2 Missing ⚠️
src/aleph/vm/models.py	66.66%	1 Missing ⚠️
src/aleph/vm/orchestrator/metrics.py	95.23%	1 Missing ⚠️
src/aleph/vm/orchestrator/run.py	50.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #932      +/-   ##
==========================================
+ Coverage   72.27%   72.68%   +0.41%     
==========================================
  Files         117      120       +3     
  Lines       14316    14957     +641     
  Branches     1143     1187      +44     
==========================================
+ Hits        10347    10872     +525     
- Misses       3668     3774     +106     
- Partials      301      311      +10     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[foxpatch-aleph]

The PR introduces a well-structured rescue mode feature with execution event tracking and runtimes aggregate. The core logic is sound, but there are two genuine issues that need addressing: (1) the Alembic migration uses placeholder revision IDs that will break the migration chain, and (2) operate_rescue_exit has a race condition where it can conflict with an in-progress rescue background task. Additionally, test coverage for the rescue API endpoints is completely missing.

src/aleph/vm/orchestrator/migrations/versions/0005_add_mode_and_execution_events.py (line 17): The revision IDs 'b3c4d5e6f7a8' and 'a1b2c3d4e5f6' are placeholder values. These must be replaced with real Alembic revision IDs (generated via alembic revision --rev-id=<real-id>) or the migration will fail to apply in the chain. Use alembic revision to generate proper IDs that match the actual previous revision.

src/aleph/vm/orchestrator/views/operator.py (line 1017): operate_rescue_exit does not check whether a background rescue task is in-flight for this VM. If a rescue is still downloading/restarting (task in state.tasks), this exit call could delete the rescue rootfs that the task is about to use, or leave the VM in an inconsistent state. Add a check: if vm_hash_str in state.tasks, return 409 with 'Rescue operation in progress'.

src/aleph/vm/orchestrator/views/operator.py (line 826): The rescue image download via download_file() has no timeout. If the network is slow or the server is unresponsive, this call could hang indefinitely, leaving the rescue task stuck. Consider passing a timeout to download_file or wrapping it with asyncio.wait_for.

src/aleph/vm/orchestrator/views/operator.py (line 865): execution.mode is set to 'rescue' on line 865 before _restart_persistent_vm succeeds. If the restart fails, the in-memory state says 'rescue' but the DB still says 'normal'. The next GET /rescue status call would incorrectly report rescue mode. Consider setting mode only after _restart_persistent_vm completes, or persisting the mode atomically with the restart.

tests/supervisor/test_rescue.py (line 1): Missing test coverage for the three rescue API endpoints: operate_rescue (POST), operate_rescue_status (GET), and operate_rescue_exit (DELETE). At minimum, add tests for: 409 when already in rescue mode, 409 when not in rescue mode (exit), 202 in_progress response, 409 conflict blocking on erase/reinstall while in rescue mode, and the happy path for enter/exit.

src/aleph/vm/orchestrator/utils.py (line 116): fetch_runtimes_aggregate accesses resp_data['data']['runtimes']['entries'] without defensive checks. If the API returns an unexpected structure, this raises a KeyError. While get_runtimes catches Exception, a malformed response silently returns [] which may mask configuration issues. Consider logging the raw response structure on error for debugging.

[foxpatch-aleph]

The PR is a well-structured addition implementing rescue mode, execution event auditing, and runtimes aggregate. The architecture is sound and the code follows existing patterns. However, there is one genuine bug: operate_rescue_exit is missing a check for an in-progress rescue task (state.tasks), which creates a race condition allowing exit and rescue to run concurrently on the same VM. There are also several minor issues worth addressing.

src/aleph/vm/orchestrator/views/operator.py (line 1017): Missing race condition guard: operate_rescue_exit does not check state.tasks for an in-progress rescue task. Compare with operate_rescue_status (line 994) and operate_rescue (line 937). If a rescue is still downloading/restarting, operate_rescue_exit could delete the rescue rootfs and reset to normal mode while _run_rescue_work is mid-flight, causing data corruption or a crash. Add: if vm_hash_str in state.tasks: return web.json_response({'status': 'in_progress'}, status=202)

src/aleph/vm/orchestrator/utils.py (line 116): Missing key validation: fetch_runtimes_aggregate accesses resp_data['data']['runtimes']['entries'] without checking these keys exist. If the API returns an unexpected structure (e.g., empty runtimes key), this raises a KeyError that is caught by the outer try/except but silently swallowed. Consider adding explicit validation: if 'runtimes' not in resp_data.get('data', {}): return []

src/aleph/vm/orchestrator/metrics.py (line 84): SQLAlchemy default vs server_default: Column(String, nullable=False, default='normal', server_default='normal') — the Python default is ignored when using server_default. Remove the redundant default='normal' parameter to avoid confusion.

src/aleph/vm/orchestrator/migrations/versions/0005_add_mode_and_execution_events.py (line 17): The down_revision = 'a1b2c3d4e5f6' appears to be a placeholder. Verify this matches the actual latest migration revision before merging, otherwise Alembic will fail to apply the migration.

src/aleph/vm/orchestrator/views/operator.py (line 1048): In operate_rescue_exit, the tap interface is deleted and recreated only if pool.network and execution.vm and pool.network.interface_exists(...). However, if execution.vm is None (possible if the VM was never fully started), the tap reset is silently skipped. This is probably fine, but consider logging a warning if execution.vm is None since it means the rescue exit is incomplete.

src/aleph/vm/orchestrator/views/operator.py (line 997): In operate_rescue_status, state.results.pop(vm_hash_str) removes the result on first read. This means if the caller refreshes the status page after a successful rescue, they get a 409 instead of the rescue status. The fallback to execution.mode == 'rescue' (line 1003) handles this, but the result data (which includes the full message about /dev/vdb) is lost. Consider not popping the result, or caching the result alongside the mode check.

src/aleph/vm/orchestrator/views/operator.py (line 865): In _run_rescue_work, execution.mode = 'rescue' is set before _restart_persistent_vm. If the restart fails (e.g., systemd error), the execution record is left in rescue mode even though the VM is not actually in rescue. Consider setting the mode after _restart_persistent_vm succeeds, or rolling back on failure.

src/aleph/vm/orchestrator/utils.py (line 98): The RuntimeEntry TypedDict uses total=False which makes all fields optional. However, id, name, type, item_hash are effectively required by consumers. Consider using a dataclass or a Pydantic model for stricter validation, or at least document which fields are required.

tests/supervisor/test_rescue.py (line 88): The _patch_session_maker fixture patches AsyncSessionMaker on the module-level aleph.vm.orchestrator.metrics which is shared across all tests. While this works because tests are isolated by in-memory DB, consider using autouse=False explicitly and ensuring tests that don't need the DB don't accidentally interfere. The fixture is fine as-is, but worth noting.

[foxpatch-aleph]

The PR implements a well-structured rescue mode feature with execution state tracking and a runtimes aggregate. The core logic is sound: rescue mode boots from a .rescue overlay image while attaching the original rootfs as /dev/vdb, the audit trail via ExecutionEvent is useful, and the runtimes aggregate follows the existing settings aggregate pattern. However, there are a few issues worth addressing before merge.

src/aleph/vm/orchestrator/views/operator.py (line 848): When expected_sha256 is None (user-provided item_hash without a sha256 in the aggregate), the rescue image is downloaded without any integrity verification. Consider always requiring sha256 verification, or at minimum logging a warning when verification is skipped.

src/aleph/vm/orchestrator/views/operator.py (line 833): _run_rescue_work calls pool.stop_vm() which records a 'stopped' event, then _restart_persistent_vm is called. This means a 'stopped' event is recorded even though the VM immediately restarts. The event log will show stop/start pairs for rescue transitions. Consider recording 'stopped' only for explicit stops, not for restarts within _run_rescue_work, to avoid polluting the audit trail.

src/aleph/vm/orchestrator/views/operator.py (line 998): state.results.pop(vm_hash_str) removes the result after first read. This means if a user polls GET /rescue after success, they won't see the result from state.results and will fall through to the execution.mode == 'rescue' check. This is intentional (prevents replay), but the dual-path behavior (result from cache vs. live mode check) could return slightly different response bodies. Consider unifying the response format.

src/aleph/vm/orchestrator/views/operator.py (line 1033): operate_rescue_exit calls pool.stop_vm() which records a 'stopped' event, then calls _restart_persistent_vm which restarts the VM. Same issue as line 833: the 'stopped' event is recorded for a restart, not an actual stop. This creates misleading audit trail entries.

src/aleph/vm/orchestrator/metrics.py (line 84): The mode column uses default="normal" and server_default="normal". In SQLAlchemy 2.x, Column.default is only used when constructing a new Python object, while server_default is used for INSERT statements. Both being set is correct for this use case, but note that existing rows created before this column was added will have NULL until the migration's server_default constraint takes effect on future updates.

src/aleph/vm/orchestrator/migrations/versions/0005_add_mode_and_execution_events.py (line 12): The revision IDs b3c4d5e6f7a8 and a1b2c3d4e5f6 appear to be placeholder values. These should be verified against the actual alembic revision chain before merge.

src/aleph/vm/orchestrator/utils.py (line 111): The URL construction on this line has an unnecessary line break: f"{settings.API_SERVER}/api/v0/aggregates/" f"{settings.SETTINGS_AGGREGATE_ADDRESS}.json?keys=runtimes". While this works due to Python string concatenation, a single line would be cleaner.

tests/supervisor/test_rescue.py (line 88): The _patch_session_maker fixture monkeypatches metrics_mod.AsyncSessionMaker in-place. Since pytest runs tests sequentially and the patch is not undone between tests, this could cause test isolation issues if a test imports the module before the fixture runs. Consider using yield in the fixture to restore the original value after the test.

tests/supervisor/test_rescue.py (line 1): Missing test coverage for the rescue API endpoints themselves (operate_rescue, operate_rescue_status, operate_rescue_exit). The underlying functions (_run_rescue_work, RescueState) are tested indirectly through the metrics and utils tests, but the HTTP endpoints should have integration tests covering: 409 conflict when already in rescue mode, 409 when not in rescue mode, 403 unauthorized, and 202/in_progress flow.

src/aleph/vm/controllers/qemu/instance.py (line 216): In rescue mode, the original rootfs is inserted as a host volume with read_only=False. This allows writes to the original rootfs during rescue, which is the intended behavior for repair. However, consider whether data volumes should also be read-only in rescue mode to prevent accidental modification while the filesystem is mounted elsewhere.

[foxpatch-aleph]

Well-structured feature adding rescue mode, execution state tracking, and runtimes aggregate. The code is clean, well-tested (643 lines of new tests across 2 files), and the design decisions are well-documented. The main concern is the migration using make_db_url() (which returns an async URL sqlite+aiosqlite://) with SQLAlchemy's synchronous create_engine, which would fail at migration time — should use make_sync_db_url() instead (returning sqlite://). The QEMU volume format auto-detection by file extension is fragile but acceptable given current usage patterns.

src/aleph/vm/orchestrator/migrations/versions/0005_add_mode_and_execution_events.py (line 23): Uses create_engine(make_db_url()) but make_db_url() returns sqlite+aiosqlite:// (async driver URL). The synchronous create_engine will fail with this URL. Should use make_sync_db_url() which returns sqlite:// (sync driver URL). This pattern is copied from migration 0004 which has the same issue.

src/aleph/vm/hypervisors/qemu/qemuvm.py (line 201): Volume format auto-detection by .qcow2 file extension is fragile. A raw image not ending in .qcow2 will get format=raw and may fail to boot, while a qcow2 image named without the extension will also fail. The existing codebase always names qcow2 volumes with the extension, so this works in practice, but QEMU's auto-detection (format=auto or omitting format) would be more robust.

src/aleph/vm/orchestrator/views/operator.py (line 216): The download_file and get_content_url imports are inside the function body. Consider moving them to module-level imports for consistency with the rest of the file.

tests/supervisor/views/test_rescue_endpoints.py (line 289): The test_rescue_exit_success test patches _restart_persistent_vm to a no-op AsyncMock but doesn't verify the rescue rootfs cleanup or the tap interface reset logic. Consider adding a test that validates these side effects.

[foxpatch-aleph]

Well-designed feature with thorough tests and clean code. The migration has a correctness issue where it creates a synchronous engine with what may be an async database URL, which will break on PostgreSQL in production. Recommend fixing the migration to properly derive a sync URL before merging.

src/aleph/vm/orchestrator/migrations/versions/0005_add_mode_and_execution_events.py (line 23): create_engine(make_db_url()) will fail if make_db_url() returns an async driver URL like postgresql+asyncpg://... or sqlite+aiosqlite://.... Alembic migrations run synchronously and need a sync-compatible URL. Either parse and strip the async driver prefix or hardcode the sync URL separately.

src/aleph/vm/hypervisors/qemu/qemuvm.py (line 213): Format detection via .endswith('.qcow2') is heuristic. Consider adding a comment that this assumes all qcow2 files carry the extension and all raw files do not.

src/aleph/vm/orchestrator/views/operator.py (line 1064): In rescue exit, execution.mode is set to 'normal' before _restart_persistent_vm succeeds. If the restart fails, the mode will be 'normal' but the VM isn't running normally — the rescue state is lost. Consider rolling back mode to 'rescue' on failure, similar to the rollback in _run_rescue_work.

src/aleph/vm/controllers/qemu/instance.py (line 187): The configure() method checks Path(rescue_rootfs_path).exists() to decide whether to boot from the rescue image. If mode == 'rescue' but the file doesn't exist, it silently falls through to the original rootfs. Consider logging a warning in this case.

tests/supervisor/test_rescue.py (line 85): The _patch_session_maker fixture monkeypatches a module-level global (AsyncSessionMaker). This works because it's scoped to the test function via usefixtures, but it's worth noting that tests using this pattern cannot safely run in parallel with other DB-testing sessions.

[foxpatch-aleph]

Well-structured feature with clean code organization and good test coverage. Two bugs need fixing: the migration uses the async DB URL with a sync engine, which will fail at runtime, and the SHA256 integrity check uses read_bytes() which will OOM on multi-GB rescue images. A few minor concerns: fragile QEMU format detection via string matching, an unnecessary retry window in tap recreation during rescue exit, and unusual use of HTTP 409 for 'no rescue operation'.

src/aleph/vm/orchestrator/migrations/versions/0005_add_mode_and_execution_events.py (line 16): Bug: Uses make_db_url() which returns sqlite+aiosqlite://... but sqlalchemy.create_engine() is a sync engine. The +aiosqlite suffix is not a valid sync DBAPI driver and will raise ValueError: Could not parse rfc1738 URL. Must use make_sync_db_url() (which returns sqlite://...) instead.

src/aleph/vm/orchestrator/views/operator.py (line 854): Memory issue: rescue_rootfs_path.read_bytes() loads the entire rescue image (potentially multiple GB) into memory. Use hashlib.sha256() with incremental file reading (sh256.update(chunk) in a loop) to avoid OOM. This occurs in two places: the cached check on line 854 and the post-download verification on line 872.

src/aleph/vm/hypervisors/qemu/qemuvm.py (line 212): Fragile format detection: str(volume.path_on_host).endswith('.qcow2') would also match paths like volumes/rootfs.qcow2.backup or any path where .qcow2 appears as a substring before the actual extension. Consider using suffix from PurePath or checking the first few bytes of the file (QCOW2 magic: QFI\xfb) for correctness.

src/aleph/vm/orchestrator/views/operator.py (line 1041): Race window: The tap interface is deleted and recreated between stop_vm and _restart_persistent_vm. If the periodic allocation loop runs in this window and sees the execution re-registered but without a tap, it may attempt to interfere. Consider recreating the tap before calling stop_vm and swapping it atomically, or documenting that the re-registration at line 1030 protects this window.

src/aleph/vm/orchestrator/views/operator.py (line 936): HTTP semantics: Returning 409 Conflict for 'No rescue operation in progress' is unusual — 409 typically means the request conflicts with current state (e.g. resource already exists). A 404 Not Found would be more appropriate here since the 'rescue operation' resource simply doesn't exist.

src/aleph/vm/orchestrator/views/operator.py (line 867): Minor: When cached is False but the file doesn't exist yet, rescue_rootfs_path.unlink(missing_ok=True) is called unnecessarily. The file was already either non-existent or deleted in the cached check branch above. This is harmless but slightly redundant.

src/aleph/vm/orchestrator/views/operator.py (line 868): Potential timeout edge case: asyncio.wait_for wraps download_file, which already has 10 retry attempts internally. If the overall timeout fires mid-download, the .part file is left behind. The retry mechanism in download_file would find it on next attempt and treat it as existing (.is_file() returns True). Consider cleaning up .part files associated with rescue downloads in the timeout handler.

[foxpatch-aleph]

Well-structured PR for rescue mode with good separation, thorough tests, and clear code. However, the rescue exit flow has a real bug: the tap interface reset code is dead (execution.vm is None after stop_vm), meaning stale tap state is inherited by the normal-boot QEMU, which the code's own comment warns can cause TUNSETOFFLOAD EBADFD crashes. Two approaches to fix: (1) extract the tap interface into a local variable before stopping, or (2) move tap recreation into _restart_persistent_vm so it happens regardless of entry point.

src/aleph/vm/orchestrator/views/operator.py (line 954): Bug: execution.vm is None after stop_vm above, so this condition is always false and the tap interface is never recreated. The comment warns this leads to TUNSETOFFLOAD EBADFD crashes. Either: (a) save execution.vm to a local variable before stopping, then use that for the tap deletion; or (b) move the tap recreation into _restart_persistent_vm so it always happens. Option (b) is safer and avoids the same bug appearing in other callers like reboot/reinstall.

src/aleph/vm/orchestrator/views/operator.py (line 954): If you decide to keep the tap reset here, use a local variable before stop_vm: vm = execution.vm; await pool.stop_vm(...); if vm and pool.network and pool.network.interface_exists(vm.vm_id): await vm.tap_interface.delete(); await pool.network.create_tap(vm.vm_id, vm.tap_interface)

[foxpatch-aleph]

Well-architected PR that adds rescue mode, lifecycle event auditing, and runtimes aggregate support. The design is clean: rescue images come from the on-chain aggregate, mode is persisted across restarts, and tests cover the major paths. Two issues require attention before merging: the QEMU volume format detection bug that will cause .rescue files to always be treated as raw format (even if they're qcow2), and the ordering race in _restart_persistent_vm where the execution is registered in the pool before its controller JSON is updated.

src/aleph/vm/hypervisors/qemu/qemuvm.py (line 213): The format detection volume.path_on_host.suffix == ".qcow2" will always return "raw" for rescue rootfs files. The rescue file path is {original_rootfs}.rescue, so its suffix is .rescue, not .qcow2. A rescue image downloaded via get_content_url / download_file could be either qcow2 or raw. Use qemu-img info for reliable format detection, or strip a known suffix like .rescue before checking, or document that rescue images must be raw format and validate at download time.

src/aleph/vm/orchestrator/views/operator.py (line 250): _restart_persistent_vm registers the execution in pool.executions and calls _schedule_forget_on_stop before vm.configure(mode=...) regenerates the controller JSON. Between these calls, a concurrent request (status check, rescue status) or a systemd restart could read stale controller config. Move vm.configure(mode=execution.mode) to execute before the pool.executions[execution.vm_hash] = execution line.

src/aleph/vm/orchestrator/views/operator.py (line 1092): In operate_rescue_exit, the rollback sets execution.mode = "rescue" if the restart fails. But if the rescue rootfs was already deleted before the failed restart, the mode is inconsistent with reality — the rescue image is gone. Either delete the rescue rootfs after confirming the restart succeeded, or accept this minor inconsistency with a comment.

src/aleph/vm/orchestrator/metrics.py (line 120): Consider using a DB-side default for the timestamp column (e.g., server_default=func.now()) instead of setting it from application code. This protects against clock skew between supervisor instances and makes the audit trail more reliable.

src/aleph/vm/controllers/qemu/instance.py (line 187): The rescue mode check if mode == "rescue" and Path(rescue_rootfs_path).exists() silently falls back to the original rootfs if the rescue image is missing. Consider logging a warning when this fallback occurs so operators can detect configuration drift.