From 62c44126232b371218b74a936bae7e5f4057797a Mon Sep 17 00:00:00 2001 From: nexxo Date: Sat, 25 Jul 2026 10:26:37 +0200 Subject: [PATCH] fix(engine): address Codex round 2 (poll budget, host error state, wg race) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - StepResult::poll — polling steps (reboot) wait without consuming the retry budget; the step owns its deadline. Reboot maxDuration > deadline. - Failed runs move a Host subject to 'error' via ProvisioningSubject hook (no host stuck 'onboarding'). - ConfigureWireguard allocates + reserves the wg_ip under a global lock; unique index on hosts.wg_ip as a backstop against duplicate addresses. Co-Authored-By: Claude Opus 4.8 --- app/Models/Host.php | 9 +++++- .../Contracts/ProvisioningSubject.php | 12 ++++++++ app/Provisioning/RunRunner.php | 30 ++++++++++++++----- app/Provisioning/StepResult.php | 10 +++++++ .../Steps/Host/ConfigureWireguard.php | 15 ++++++++-- .../Steps/Host/RebootIntoPveKernel.php | 8 +++-- ...070005_add_unique_wg_ip_to_hosts_table.php | 24 +++++++++++++++ tests/Feature/Provisioning/HostStepsTest.php | 4 +-- tests/Feature/Provisioning/RunRunnerTest.php | 25 ++++++++++++++++ tests/Support/Steps/FakePollStep.php | 30 +++++++++++++++++++ 10 files changed, 151 insertions(+), 16 deletions(-) create mode 100644 app/Provisioning/Contracts/ProvisioningSubject.php create mode 100644 database/migrations/2026_07_25_070005_add_unique_wg_ip_to_hosts_table.php create mode 100644 tests/Support/Steps/FakePollStep.php diff --git a/app/Models/Host.php b/app/Models/Host.php index f288b0d..a5b5b4a 100644 --- a/app/Models/Host.php +++ b/app/Models/Host.php @@ -3,11 +3,12 @@ namespace App\Models; use App\Models\Concerns\HasUuid; +use App\Provisioning\Contracts\ProvisioningSubject; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\MorphMany; -class Host extends Model +class Host extends Model implements ProvisioningSubject { /** @use HasFactory<\Database\Factories\HostFactory> */ use HasFactory, HasUuid; @@ -39,6 +40,12 @@ class Host extends Model return $this->morphMany(ProvisioningRun::class, 'subject'); } + /** Runner hook: a failed onboarding run moves the host to the error status. */ + public function onProvisioningFailed(): void + { + $this->update(['status' => 'error']); + } + /** Free committable storage: total minus reserve. Instance quotas subtract in B. */ public function freeGb(): int { diff --git a/app/Provisioning/Contracts/ProvisioningSubject.php b/app/Provisioning/Contracts/ProvisioningSubject.php new file mode 100644 index 0000000..ee372da --- /dev/null +++ b/app/Provisioning/Contracts/ProvisioningSubject.php @@ -0,0 +1,12 @@ +type) { StepResult::ADVANCE => $this->onAdvance($run, $step), StepResult::RETRY => $this->onRetry($run, $step, $result), - StepResult::FAIL => $this->onFail($run, $step, $result), + StepResult::POLL => $this->onPoll($run, $step, $result), + StepResult::FAIL => $this->failRun($run, $step, $result->reason), }; } @@ -105,10 +107,7 @@ class RunRunner $run->attempt += 1; if ($run->attempt >= $run->max_attempts) { - $run->status = ProvisioningRun::STATUS_FAILED; - $run->error = $result->reason; - $run->save(); - $this->record($run, $step, 'failed', $result->reason); + $this->failRun($run, $step, $result->reason); return; } @@ -119,12 +118,27 @@ class RunRunner $this->record($run, $step, 'retry', $result->reason); } - private function onFail(ProvisioningRun $run, ProvisioningStep $step, StepResult $result): void + /** Poll: wait and re-run without consuming the retry budget (step owns its deadline). */ + private function onPoll(ProvisioningRun $run, ProvisioningStep $step, StepResult $result): void + { + $run->status = ProvisioningRun::STATUS_WAITING; + $run->next_attempt_at = now()->addSeconds($result->afterSeconds); + $run->save(); + $this->record($run, $step, 'info', $result->reason); + } + + private function failRun(ProvisioningRun $run, ProvisioningStep $step, string $reason): void { $run->status = ProvisioningRun::STATUS_FAILED; - $run->error = $result->reason; + $run->error = $reason; $run->save(); - $this->record($run, $step, 'failed', $result->reason); + $this->record($run, $step, 'failed', $reason); + + // Let the subject react (e.g. a Host moves to the 'error' status). + $subject = $run->subject; + if ($subject instanceof ProvisioningSubject) { + $subject->onProvisioningFailed(); + } } private function record(ProvisioningRun $run, ProvisioningStep $step, string $outcome, ?string $message): void diff --git a/app/Provisioning/StepResult.php b/app/Provisioning/StepResult.php index 5b34288..043e84a 100644 --- a/app/Provisioning/StepResult.php +++ b/app/Provisioning/StepResult.php @@ -10,6 +10,7 @@ final class StepResult { public const ADVANCE = 'advance'; public const RETRY = 'retry'; + public const POLL = 'poll'; public const FAIL = 'fail'; private function __construct( @@ -28,6 +29,15 @@ final class StepResult return new self(self::RETRY, $afterSeconds, $reason); } + /** + * Wait and re-run WITHOUT consuming the retry budget — for polling long + * operations (e.g. a reboot). The step enforces its own deadline via fail(). + */ + public static function poll(int $afterSeconds, string $reason): self + { + return new self(self::POLL, $afterSeconds, $reason); + } + public static function fail(string $reason): self { return new self(self::FAIL, 0, $reason); diff --git a/app/Provisioning/Steps/Host/ConfigureWireguard.php b/app/Provisioning/Steps/Host/ConfigureWireguard.php index 8875d0d..e9add02 100644 --- a/app/Provisioning/Steps/Host/ConfigureWireguard.php +++ b/app/Provisioning/Steps/Host/ConfigureWireguard.php @@ -7,6 +7,7 @@ use App\Models\ProvisioningRun; use App\Provisioning\StepResult; use App\Services\Ssh\RemoteShell; use App\Services\Wireguard\WireguardHub; +use Illuminate\Support\Facades\Cache; /** * Installs WireGuard on the host, allocates a management IP, registers the host @@ -37,7 +38,17 @@ class ConfigureWireguard extends HostStep return StepResult::retry(20, 'could not read host WireGuard public key'); } - $wgIp = $host->wg_ip ?: $this->hub->allocateIp(); + // Allocate + reserve the management IP atomically so concurrent + // onboarding runs can never receive the same address. + $wgIp = Cache::lock('wireguard:allocate', 30)->block(10, function () use ($host) { + $ip = $host->wg_ip ?: $this->hub->allocateIp(); + if (blank($host->wg_ip)) { + $host->update(['wg_ip' => $ip]); + } + + return $ip; + }); + $privateKey = trim($this->shell->run('cat /etc/wireguard/privatekey')->stdout); $this->shell->putFile('/etc/wireguard/wg0.conf', $this->renderConfig($wgIp, $privateKey)); @@ -46,7 +57,7 @@ class ConfigureWireguard extends HostStep $this->hub->addPeer($publicKey, $wgIp); // Persist external identity BEFORE verifying/advancing (crash-safe). - $host->update(['wg_ip' => $wgIp, 'wg_pubkey' => $publicKey]); + $host->update(['wg_pubkey' => $publicKey]); $this->recordResource($run, $host, 'wg_peer', $publicKey); } diff --git a/app/Provisioning/Steps/Host/RebootIntoPveKernel.php b/app/Provisioning/Steps/Host/RebootIntoPveKernel.php index 08eb6b0..ac98a3f 100644 --- a/app/Provisioning/Steps/Host/RebootIntoPveKernel.php +++ b/app/Provisioning/Steps/Host/RebootIntoPveKernel.php @@ -24,7 +24,9 @@ class RebootIntoPveKernel extends HostStep public function maxDuration(): int { - return 900; + // Above the 15-min reboot deadline below, so the step's own deadline + // governs failure rather than the generic step timeout. + return 1800; } public function execute(ProvisioningRun $run): StepResult @@ -41,7 +43,7 @@ class RebootIntoPveKernel extends HostStep ]); $this->shell->run('systemctl reboot || reboot'); - return StepResult::retry(30, 'rebooting into the Proxmox kernel'); + return StepResult::poll(30, 'rebooting into the Proxmox kernel'); } try { @@ -60,6 +62,6 @@ class RebootIntoPveKernel extends HostStep return StepResult::fail('Host did not return on the Proxmox kernel before the deadline.'); } - return StepResult::retry(15, 'waiting for the Proxmox kernel'); + return StepResult::poll(15, 'waiting for the Proxmox kernel'); } } diff --git a/database/migrations/2026_07_25_070005_add_unique_wg_ip_to_hosts_table.php b/database/migrations/2026_07_25_070005_add_unique_wg_ip_to_hosts_table.php new file mode 100644 index 0000000..ffcd277 --- /dev/null +++ b/database/migrations/2026_07_25_070005_add_unique_wg_ip_to_hosts_table.php @@ -0,0 +1,24 @@ +unique('wg_ip'); + }); + } + + public function down(): void + { + Schema::table('hosts', function (Blueprint $table) { + $table->dropUnique(['wg_ip']); + }); + } +}; diff --git a/tests/Feature/Provisioning/HostStepsTest.php b/tests/Feature/Provisioning/HostStepsTest.php index 4a9a983..99cbbca 100644 --- a/tests/Feature/Provisioning/HostStepsTest.php +++ b/tests/Feature/Provisioning/HostStepsTest.php @@ -140,7 +140,7 @@ it('issues the reboot on first execution', function () { $s = fakeServices(); $run = hostRun(Host::factory()->create()); - expect(app(RebootIntoPveKernel::class)->execute($run)->type)->toBe('retry') + expect(app(RebootIntoPveKernel::class)->execute($run)->type)->toBe('poll') ->and($run->fresh()->context('reboot_issued'))->toBeTrue() ->and($s['shell']->ran('reboot'))->toBeTrue(); }); @@ -164,7 +164,7 @@ it('keeps waiting while the old kernel is still running', function () { 'reboot_deadline' => now()->addMinutes(10)->toIso8601String(), ]); - expect(app(RebootIntoPveKernel::class)->execute($run)->type)->toBe('retry'); + expect(app(RebootIntoPveKernel::class)->execute($run)->type)->toBe('poll'); }); it('fails when the host does not return before the reboot deadline', function () { diff --git a/tests/Feature/Provisioning/RunRunnerTest.php b/tests/Feature/Provisioning/RunRunnerTest.php index 3abadb0..0645132 100644 --- a/tests/Feature/Provisioning/RunRunnerTest.php +++ b/tests/Feature/Provisioning/RunRunnerTest.php @@ -1,5 +1,6 @@ status)->toBe('failed')->and($run->error)->toBe('boom'); }); +it('polls without consuming the retry budget', function () { + bindPipeline(['test' => [FakePollStep::class]]); + $run = ProvisioningRun::factory()->create(['pipeline' => 'test', 'max_attempts' => 1]); + + app(RunRunner::class)->advance($run); + expect($run->fresh()->status)->toBe('waiting')->and($run->fresh()->attempt)->toBe(0); + + // A second poll must not fail the run even with max_attempts = 1. + app(RunRunner::class)->advance($run->fresh()); + expect($run->fresh()->status)->toBe('waiting')->and($run->fresh()->attempt)->toBe(0); +}); + +it('marks a Host subject as error when the run fails', function () { + bindPipeline(['test' => [FakeFailStep::class]]); + $host = Host::factory()->create(['status' => 'onboarding']); + $run = ProvisioningRun::factory()->forHost($host)->create(['pipeline' => 'test']); + + app(RunRunner::class)->advance($run); + + expect($run->fresh()->status)->toBe('failed') + ->and($host->fresh()->status)->toBe('error'); +}); + it('treats a thrown exception as a retry', function () { bindPipeline(['test' => [FakeThrowStep::class]]); $run = ProvisioningRun::factory()->create(['pipeline' => 'test', 'max_attempts' => 5]); diff --git a/tests/Support/Steps/FakePollStep.php b/tests/Support/Steps/FakePollStep.php new file mode 100644 index 0000000..251e376 --- /dev/null +++ b/tests/Support/Steps/FakePollStep.php @@ -0,0 +1,30 @@ +