Make VM expiry and provisioning recovery durable

[Svaag]

VM expiry, account administration and provisioning recovery previously had races that could lose paid time, resume restricted guests or release network identities before provider deletion was established. This integrates the admin control plane, serialized expiry/deletion and durable guest completion from #88, #113 and #116.

• Browser-session-only admin operations enforce CSRF, recent password step-up and current actor authorization, with durable audit and explicitly separate waiver accounting. Waivers remain disabled by default.
• Purchased extensions commit time and an application receipt together, reconcile lost acknowledgements, and preserve a durable resume obligation. Deletion claims and verified provider UUIDs prevent premature prefix release; worker retries recover failed final commits.
• Guest completion gates READY. Durable dispatch receipts preserve provisioning across process loss. Account lifecycle fences serialize payment, delivery, disable, deletion and transfer; independent manual/expiry suspensions survive re-enable.
• The ordered migration chain is 020→021→022→023. Downgrades reject unresolved lifecycle evidence, including paid resume markers; admin audit history must be preserved before any schema downgrade.

Validation: exact head c9ba187 has test, PR-Agent, Semgrep and Semgrep OSS green. All 88 review threads are resolved. The completed latest PR-Agent review had partial file coverage; its five findings have explicit full-source/test dispositions in comments 5644758881 and 5644785974. No claim is made that excluded files were covered by that reviewer. Fresh focused review verification passed all 5 cases; the preceding delete-commit/disabled-transfer set passed 10, full admin set passed 124, and actual PostgreSQL migration/nested-lock proofs passed 2. Earlier concurrency, guest receipt and authorization regression evidence remains in review history. A sandbox-only focused run timed out; the bounded approved local rerun passed.

Deployment is separate: use network-operations SHA promotion and the merged PR547 quiescing/preflight barriers, then verify schema, API/worker, session/CSRF and read-only admin behavior. No production schema or app pin is changed by this app merge. Prefer compatible application rollback retaining schema; never discard admin audit or retired VM data.

Retention, owner notices and restore validation remain separate in draft PR119 / issue110. Application receipts do not prove external refund payment or make repeated paid HTTP requests idempotent.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-09-09T17:18:05.983330Z	b8b8fb7	Manual request

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 50834e9c30

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Svaag]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f873b4483f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 38a2bebe4b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Svaag]

@codex review

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Svaag]

@codex review

[chatgpt-codex-connector]

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

Provided git ref b8b8fb729efb0af069a2e4d349dfc1fc276c54fa does not exist

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Svaag]

@codex review

[chatgpt-codex-connector]

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

Provided git ref b8b8fb729efb0af069a2e4d349dfc1fc276c54fa does not exist

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Svaag]

/review

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit c9ba187)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 88 - Partially compliant Compliant requirements: Admin audit tables (AdminAuditRow, AdminOperationRow) added. Billing mode columns (billing_mode, retail_cost_total) distinguish waived/charged. Account disabled_at column added. Admin bypass usage table (AdminBypassUsageRow) for rate-limiting. hyrule-admin CLI entry point added. Non-compliant requirements: Admin API routes and authentication not visible in diff; cannot verify session/CSRF/step-up enforcement. Private route exclusion from OpenAPI not visible. Waiver default disabled state not confirmed in diff. Requires further human verification: Admin API authentication and authorization implementation. CSRF token handling and step-up logic. Waiver default disabled configuration. 113 - Partially compliant Compliant requirements: locked_vm context manager shares row lock across operations. deletion_started_at column added and set before provider call. check_expiries uses locked_vm and deletion claim logic. release_destroyed_prefix checks provider_deleted_uuid. Non-compliant requirements: Worker recheck and resume logic not fully visible in diff (worker file not included). Provider delete retry with UUID absence confirmation not visible. Requires further human verification: Worker implementation for deletion claim recovery. Provider delete retry logic. 116 - Partially compliant Compliant requirements: VMGuestResultRow table added for guest receipts. _wait_for_guest_result and _poll_guest_result enforce receipt before READY. prepare_provisioning_dispatch persists receipt before task creation. recover_tracked_provisioning method added. _recover_pre_uuid_guest handles orphaned guests. destroy_vm checks deletion_started_at for transfer rejection. Account disable handling in _stop_restricted_provisioned_guest. Non-compliant requirements: Worker recovery loop not fully visible (worker file not included). Serialization of provider attempts across processes not fully visible. Requires further human verification: Worker implementation for provisioning recovery. Cross-process serialization mechanism.

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🏅 Score: 70

🧪 PR contains tests

🔒 Security concerns Potential unauthenticated admin endpoints: The diff adds admin-related database tables and a CLI entry point but does not include the admin API routes or authentication middleware. If the admin API is deployed without proper session-based authentication, CSRF protection, and step-up for sensitive operations, it would expose management functionality to unauthorized actors. Additionally, the external_admin_waiver_guard in tunnel.py may allow bypassing payment verification if not properly gated; the implementation appears to track usage but the guard's validation logic is not fully visible. No secrets or credentials are committed in the diff.

⚡ Recommended focus areas for review Trust boundary risk The admin control plane tables and CLI are added, but the admin API routes and authentication are not visible in this diff. If admin endpoints are deployed without proper session authentication, CSRF protection, and step-up enforcement, they become unauthenticated management endpoints that could be exploited. The diff shows no authentication middleware for admin operations; the external_admin_waiver_guard in tunnel.py suggests some waiver handling but not full admin auth. """ Missing test coverage The PR introduces complex state transitions for VM expiry, extension, provisioning recovery, and admin operations (e.g., extend_vm with receipt reconciliation, check_expiries with deletion claims, recover_tracked_provisioning). No test files are included in the diff. Without tests for idempotency, race conditions, and failure recovery, these critical paths are at risk of undetected bugs in production. """ Potential race in extension reconciliation In extend_vm, after a commit failure, the code attempts to reconcile by reacquiring the lock and checking for the receipt. However, if the original transaction partially committed (e.g., the receipt was written but the VM row update was lost), the reconciliation may incorrectly return None (no receipt found) and the extension is silently lost. The receipt_id is generated before commit, so a lost commit could leave the receipt orphaned. This could lead to a paid extension not being applied. try: await session.commit() except Exception as commit_error: try: await session.rollback() # A fresh connection and lifecycle lock wait for the original # transaction to resolve before absence is treated as failure. async with self.locked_vm(vm_id) as (check_session, current): receipt = await check_session.get(PaymentEventRow, receipt_id) if current is None: raise ExtensionOutcomeUnknownError("VM missing during reconciliation") if receipt is None: return None except Exception as reconcile_error: log.error("vm_extension_outcome_unknown", vm_id=vm_id, receipt_id=receipt_id, exc_info=True) raise ExtensionOutcomeUnknownError( "Unable to establish whether purchased time committed" ) from reconcile_error log.warning("vm_extension_commit_ack_recovered", vm_id=vm_id, receipt_id=receipt_id, error_type=type(commit_error).__name__)

---

⚠️ Review coverage: The following files were not included in this review because of the token budget:

• tests/test_admin_control_plane.py
• hyrule_cloud/api/admin.py
• tests/test_guest_provisioning_gate.py
• hyrule_cloud/domains/service.py
• hyrule_cloud/api/routes.py
• hyrule_cloud/api/auth.py
• tests/test_domains_v1.py
• hyrule_cloud/middleware/x402.py
• tests/test_refunds.py
• tests/test_vm_expiry_renewal.py
• tests/test_domain_disable_postgres.py
• hyrule_cloud/services/admin_operations.py
• alembic/versions/023_admin_console.py
• hyrule_cloud/services/intents.py
• tests/test_auth.py
• hyrule_cloud/domains/wallet_auth.py
• hyrule_cloud/api/dns.py
• tests/test_account_delete_transfer_postgres.py
• hyrule_cloud/api/bgp.py
• tests/test_admin_revocation_postgres.py
• tests/test_guest_result_postgres.py
• tests/test_admin_expiry_postgres.py
• hyrule_cloud/api/registry.py
• tests/test_guest_observer.py
• tests/test_customer_ipv6.py
• tests/test_waiver_acceptance_postgres.py
• tests/test_intent_engine.py
• tests/test_vm_events.py
• hyrule_cloud/app.py
• tests/test_vm_management_identity.py
• tests/test_vm_lifecycle_postgres.py
• tests/test_credential_disable_postgres.py
• hyrule_cloud/providers/cloudinit.py
• tests/test_vm_waiver_acceptance.py
• hyrule_cloud/domains/api.py
• tests/test_admin_migration_postgres.py
• tests/test_vm_quote.py
• hyrule_cloud/providers/guest_observer.py
• tests/test_launch_blockers.py
• hyrule_cloud/services/discovery.py
• tests/test_guest_result_store.py
• tests/test_admin_migration.py
• hyrule_cloud/middleware/auth.py
• hyrule_cloud/api/mx.py
• hyrule_cloud/services/payments_ledger.py
• hyrule_cloud/admin_cli.py
• hyrule_cloud/api/ip.py
• hyrule_cloud/services/sessions.py
• hyrule_cloud/services/guest_result.py
• hyrule_cloud/config.py
  ... and 36 more

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Svaag]

Reviewed the two focus findings against the implementation and added regression evidence in c9ba187:

Deletion remains recoverable when the provider succeeds and the final database commit fails. check_expiries selects durable deletion claims regardless of original expiry and retries destroy_vm. XCP-ng repeated deletion succeeds only after a successful exact-UUID absence query. New tests cover rollback before commit and lost acknowledgement after commit, and verify DESTROYED, provider_deleted_uuid, and prefix release. Existing provider tests reject unknown or failed inventory.

Keeping the transfer handoff while its recipient is disabled preserves the durable resume obligation. The disabled path makes no provider call and emits no failed-transfer log. The new test demonstrates retention without start, then one start and marker removal after re-enable. Normal transfer entrypoints reject disabled recipients; this handles a post-commit disable race.

Ten targeted tests pass. The worker schedules expiry checks every five minutes and invokes both resume reconcilers. Earlier full admin, PostgreSQL concurrency, and migration validation is recorded. The review explicitly excluded files; omitted code needs inspection before treating an alleged absence as a defect. This latest commit changes tests only.

[Svaag]

/review

[github-actions]

Persistent review updated to latest commit c9ba187

[Svaag]

Checked the latest review findings against the full source at c9ba187:

• The receipt and VM expiry are written by the same AsyncSession transaction and one commit; a receipt-only partial database commit is not a supported outcome. The existing parametrized test_extension_reconciles_lost_commit_acknowledgment covers both rollback and committed-but-lost acknowledgement, asserting receipt and expiry together. test_extension_does_not_authorize_refund_when_reconciliation_fails covers unresolved outcomes.
• Tests are present in the PR, including tests/test_vm_expiry_renewal.py, tests/test_admin_control_plane.py and the recorded PostgreSQL concurrency suites. Their exclusion from this partial review is a coverage limitation, not missing tests.
• hyrule_cloud/api/admin.py imports and applies require_admin_session, require_admin_csrf and require_admin_step_up. The router is excluded from OpenAPI. The browser-only and CSRF/bearer-composition tests exercise the requested boundary.

Fresh focused validation: 5 passed (both extension commit outcomes, unknown-outcome handling, browser-only admin entry, CSRF/bearer separation). The first sandbox invocation timed out without results; the bounded normally approved local rerun passed in 5.61s. No product changes were needed for these findings. The two earlier deletion/transfer findings are addressed with precise recovery/test evidence in my preceding response. Review coverage remains explicitly partial; these dispositions come from direct full-source inspection and tests, not a claim that omitted files were reviewed by PR-Agent.