Fix-Runde 1: writeAtomic prueft Rueckgabewerte, ceiling() faengt Throwable
Zwei Important-Befunde aus dem Code-Review zu Task 3: - writeAtomic() ignorierte den Rueckgabewert von File::put()/File::move() und meldete setCeiling() als "true", selbst wenn ein I/O-Fehler (volle Platte, Rechteproblem) nichts geschrieben oder eine .tmp liegen gelassen hatte. Beide Rueckgabewerte werden jetzt geprueft, eine liegen gebliebene .tmp wird im Fehlerfall aufgeraeumt, und der Fehlschlag wird bis zu setCeiling() durchgereicht (Rueckgabe false). - ceiling() konnte state() doch werfen lassen: zwischen File::exists() und File::get() liegt ein Zeitfenster, und File::get() wirft eine FileNotFoundException, wenn die Datei dazwischen verschwindet. readJson() und lastLog() kapseln genau dieses Muster schon in try/catch(Throwable); ceiling() zieht jetzt nach. Beide Befunde tragen einen eigenen Test: ein Verzeichnis an der Ceiling- Datei-Stelle (exists() wahr, get() wirft) fuer den zweiten, eine echte Rechteverweigerung (chmod 0500 als nicht-root Testbenutzer) fuer den ersten -- kein Facade-Mock noetig.feat/versandtakt
parent
367198459c
commit
c93510ffe4
|
|
@ -593,8 +593,10 @@ final class UpdateChannel
|
||||||
/**
|
/**
|
||||||
* Den Server auf eine Version festnageln — oder die Decke abnehmen.
|
* Den Server auf eine Version festnageln — oder die Decke abnehmen.
|
||||||
*
|
*
|
||||||
* `null` nimmt sie ab. Rückgabe `false` heißt: die Form stimmt nicht, es
|
* `null` nimmt sie ab. Rückgabe `false` heißt: die Form stimmt nicht, oder
|
||||||
* wurde nichts geschrieben.
|
* das Schreiben ist fehlgeschlagen (volle Platte, Rechteproblem auf
|
||||||
|
* storage/app/deploy) — in beiden Fällen wurde nichts Halbes hinterlassen,
|
||||||
|
* das der Agent falsch lesen müsste.
|
||||||
*
|
*
|
||||||
* Die Decke wirkt NICHT dadurch, dass hier etwas ausgelöst wird. Der Agent
|
* Die Decke wirkt NICHT dadurch, dass hier etwas ausgelöst wird. Der Agent
|
||||||
* liest die Datei bei jedem Takt und rechnet `behind` dagegen; Knopf und
|
* liest die Datei bei jedem Takt und rechnet `behind` dagegen; Knopf und
|
||||||
|
|
@ -619,7 +621,10 @@ final class UpdateChannel
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
$this->writeAtomic(self::CEILING, $tag);
|
if (! $this->writeAtomic(self::CEILING, $tag)) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
$this->requestCheck($by);
|
$this->requestCheck($by);
|
||||||
|
|
||||||
return true;
|
return true;
|
||||||
|
|
@ -631,18 +636,28 @@ final class UpdateChannel
|
||||||
* Nicht aus der Statusdatei des Agenten: die ist seine Rückmeldung und
|
* Nicht aus der Statusdatei des Agenten: die ist seine Rückmeldung und
|
||||||
* hinkt bis zu einem Takt hinterher. Was die Konsole selbst geschrieben
|
* hinkt bis zu einem Takt hinterher. Was die Konsole selbst geschrieben
|
||||||
* hat, muss sie sofort anzeigen können.
|
* hat, muss sie sofort anzeigen können.
|
||||||
|
*
|
||||||
|
* Zwischen `File::exists()` und `File::get()` liegt ein Zeitfenster —
|
||||||
|
* verschwindet die Datei genau dort (ein zweiter Aufruf nimmt die Decke
|
||||||
|
* ab, während dieser noch liest), wirft `File::get()` eine
|
||||||
|
* FileNotFoundException. Dasselbe Muster wie readJson() und lastLog()
|
||||||
|
* weiter unten: state() darf unter keinen Umständen werfen.
|
||||||
*/
|
*/
|
||||||
private function ceiling(): ?string
|
private function ceiling(): ?string
|
||||||
{
|
{
|
||||||
$path = storage_path('app/'.self::CEILING);
|
try {
|
||||||
|
$path = storage_path('app/'.self::CEILING);
|
||||||
|
|
||||||
if (! File::exists($path)) {
|
if (! File::exists($path)) {
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
|
$tag = trim((string) File::get($path));
|
||||||
|
|
||||||
|
return $tag === '' ? null : $tag;
|
||||||
|
} catch (Throwable) {
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|
||||||
$tag = trim((string) File::get($path));
|
|
||||||
|
|
||||||
return $tag === '' ? null : $tag;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|
@ -651,13 +666,34 @@ final class UpdateChannel
|
||||||
* `write()` daneben benutzt File::put und taugt dafür nicht: der Agent
|
* `write()` daneben benutzt File::put und taugt dafür nicht: der Agent
|
||||||
* liest diese Datei bei jedem Takt, und eine halbe Zeile ist genau die
|
* liest diese Datei bei jedem Takt, und eine halbe Zeile ist genau die
|
||||||
* Art Decke, die er als kaputt ablehnen müsste.
|
* Art Decke, die er als kaputt ablehnen müsste.
|
||||||
|
*
|
||||||
|
* Prüft beide Rückgabewerte, statt ihnen zu glauben: `File::move()` ruft
|
||||||
|
* nur `rename()` auf, und ein fehlgeschlagenes `rename()` liefert `false`
|
||||||
|
* ohne von sich aus eine Ausnahme zu werfen. Schlägt einer der beiden
|
||||||
|
* Schritte fehl — volle Platte, Rechteproblem — räumt diese Methode die
|
||||||
|
* `.tmp` weg und meldet `false`, statt einen Erfolg zu behaupten, der
|
||||||
|
* nicht stattfand.
|
||||||
*/
|
*/
|
||||||
private function writeAtomic(string $relative, string $contents): void
|
private function writeAtomic(string $relative, string $contents): bool
|
||||||
{
|
{
|
||||||
$path = storage_path('app/'.$relative);
|
$path = storage_path('app/'.$relative);
|
||||||
File::ensureDirectoryExists(dirname($path));
|
$tmp = $path.'.tmp';
|
||||||
File::put($path.'.tmp', $contents);
|
|
||||||
File::move($path.'.tmp', $path);
|
try {
|
||||||
|
File::ensureDirectoryExists(dirname($path));
|
||||||
|
|
||||||
|
if (File::put($tmp, $contents) === false || ! File::move($tmp, $path)) {
|
||||||
|
File::delete($tmp);
|
||||||
|
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
return true;
|
||||||
|
} catch (Throwable) {
|
||||||
|
File::delete($tmp);
|
||||||
|
|
||||||
|
return false;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|
|
||||||
|
|
@ -81,3 +81,44 @@ it('does not mark a ceiling that is still ahead', function () {
|
||||||
|
|
||||||
expect(app(UpdateChannel::class)->state()['ceiling_passed'])->toBeFalse();
|
expect(app(UpdateChannel::class)->state()['ceiling_passed'])->toBeFalse();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('does not throw when the ceiling file exists but cannot be read as a file', function () {
|
||||||
|
// Kein echtes Wettrennen im Test moeglich, aber derselbe Codepfad wie ein
|
||||||
|
// Verschwinden zwischen File::exists() und File::get(): ein Verzeichnis
|
||||||
|
// existiert (File::exists() prueft file_exists(), das ist fuer
|
||||||
|
// Verzeichnisse wahr), ist aber keine Datei (File::get() prueft
|
||||||
|
// isFile() und wirft eine FileNotFoundException). Ohne Absicherung
|
||||||
|
// wuerfe state() genau hier — die Vorgabe, die es unter keinen
|
||||||
|
// Umstaenden darf.
|
||||||
|
//
|
||||||
|
// Bewusst kein expect(fn () => ...)->not->toThrow(Throwable::class):
|
||||||
|
// Throwable ist ein Interface, class_exists() verneint es, und die
|
||||||
|
// Pruefung faellt intern auf einen Substring-Vergleich zurueck, der
|
||||||
|
// nichts mehr ueber ein tatsaechliches Werfen aussagt. Ein direkter
|
||||||
|
// Aufruf laesst eine durchschlagende Exception den Test regulaer als
|
||||||
|
// fehlgeschlagen melden — das ist der verlaessliche Nachweis.
|
||||||
|
File::ensureDirectoryExists(storage_path('app/deploy/release-ceiling'));
|
||||||
|
|
||||||
|
$state = app(UpdateChannel::class)->state();
|
||||||
|
|
||||||
|
expect($state['ceiling'])->toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('does not claim success when writing the ceiling fails', function () {
|
||||||
|
// Kein File-Facade-Mock: eine echte Rechteverweigerung auf einem
|
||||||
|
// Verzeichnis, das dieser (nicht-root) Testbenutzer selbst besitzt.
|
||||||
|
// File::put() auf die .tmp darin schlaegt wirklich fehl, nicht nur dem
|
||||||
|
// Namen nach.
|
||||||
|
$deployDir = storage_path('app/deploy');
|
||||||
|
chmod($deployDir, 0500);
|
||||||
|
|
||||||
|
try {
|
||||||
|
$result = app(UpdateChannel::class)->setCeiling('chef@example.com', 'v1.8.0');
|
||||||
|
} finally {
|
||||||
|
chmod($deployDir, 0755);
|
||||||
|
}
|
||||||
|
|
||||||
|
expect($result)->toBeFalse()
|
||||||
|
->and(File::exists($deployDir.'/release-ceiling.tmp'))->toBeFalse()
|
||||||
|
->and(File::exists($deployDir.'/release-ceiling'))->toBeFalse();
|
||||||
|
});
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue