Skip to content

refactor(supervisor): retype VM identity from ItemHash to VmId#982

Merged
odesenfans merged 1 commit into
devfrom
od/supervisor-vmid-identity
Jun 18, 2026
Merged

refactor(supervisor): retype VM identity from ItemHash to VmId#982
odesenfans merged 1 commit into
devfrom
od/supervisor-vmid-identity

Conversation

@odesenfans

Copy link
Copy Markdown
Contributor

What

Removes the last aleph_message symbol from src/aleph/vm/pool.py by retyping the daemon's VM-identity from aleph_message.models.ItemHash to the message-free VmId = NewType("VmId", str) (defined in supervisor/types.py). VmExecution.vm_hash is retyped to match so the pool's dict[VmId, VmExecution] key type stays honest.

This is the VM-identity slice of the "remove aleph_message from the supervisor" follow-up flagged during Phase 2. It is intentionally scoped: models.py keeps its message-content imports (ExecutableContent/InstanceContent/ProgramContent), which drive the message path. Removing the message object from VmExecution entirely is a separate, larger refactor.

Why it's safe

  • ItemHash is a str subclass and VmId a str NewType, so stored keys and lookups stay compatible (hash/== identical). local.py already looked the pool up with VmId values; the create path now stores spec.vm_id directly instead of re-wrapping it as ItemHash.
  • No daemon consumer relies on ItemHash-specific behavior (no isinstance, no .item_hash, no pydantic coercion); consumers either str() the value or use it as a plain string dict key.
  • The only behavioral delta is dropping ItemHash format validation on the spec path. Content-consuming helpers (cloudinit.get_hostname_from_hash, hostnetwork.allocate_vm_ipv6_subnet) still assume a valid hex hash; the agent always supplies one, so a malformed id now fails inside those consumers rather than eagerly at ItemHash(). Out of scope, flagged for awareness.

Verification

  • pool.py confirmed free of ItemHash/aleph_message.
  • ruff format + isort clean; mypy adds zero new errors (2 pre-existing env-noise at the same spot on the base).
  • Tests: supervisor 901 passed (7 pre-existing environment failures: jailman chown subprocess + pyroute2 netlink), migration+network 26 passed.
  • Reviewed by an adversarial code-quality pass: APPROVED, no defects.

Base

Stacked on #981 (od/grpc-only-supervisor-phase2); pool.py here is already the Phase 2 pool. Merge after #981.

@foxpatch-aleph foxpatch-aleph left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean, mechanically sound refactor that replaces ItemHash with VmId in pool.py and models.py, completing the removal of aleph_message from the pool. The change is type-safe because both types are str-compatible at runtime (same hash/eq semantics), leaving dict key behavior unaffected. The only behavioral delta — deferred ItemHash format validation — is correctly acknowledged in the description as out of scope.

The pool keyed executions by aleph_message's ItemHash, the last
aleph_message symbol in pool.py. Replace it with the message-free VmId
(a NewType str) from supervisor.types, drop the import so the pool is
fully message-free, and retype VmExecution.vm_hash to match so the
pool's dict key type stays honest.

ItemHash is a str subclass and VmId a str NewType, so stored keys and
lookups stay compatible; the spec path now stores spec.vm_id directly
instead of re-wrapping it. models.py keeps its message-content imports
(ExecutableContent and friends), which drive the message path and are
out of scope here.
@odesenfans odesenfans force-pushed the od/supervisor-vmid-identity branch from b328af3 to 8c73728 Compare June 18, 2026 13:23

@foxpatch-aleph foxpatch-aleph left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR cleanly retypes VM identity from ItemHash to VmId in pool.py and models.py, removing the last aleph_message symbol from the pool module. The change is mechanically safe (ItemHash is a str subclass, VmId is a str NewType — dict key semantics are identical) and correctly propagates the type through all pool method signatures. The only cosmetic point is the redundant str() wrapping on VmId(str(config.vm_hash)) where config.vm_hash is already a str, which is harmless.

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.30%. Comparing base (db13f89) to head (8c73728).

Files with missing lines Patch % Lines
src/aleph/vm/pool.py 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #982      +/-   ##
==========================================
- Coverage   80.30%   80.30%   -0.01%     
==========================================
  Files         201      201              
  Lines       23083    23082       -1     
  Branches     1511     1511              
==========================================
- Hits        18536    18535       -1     
  Misses       4114     4114              
  Partials      433      433              

☔ 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.

@odesenfans odesenfans merged commit 259421c into dev Jun 18, 2026
19 checks passed
@odesenfans odesenfans deleted the od/supervisor-vmid-identity branch June 18, 2026 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants