diff --git a/host-setup/menu.ps1 b/host-setup/menu.ps1 index 0f0e1f475..57e61fdf8 100644 --- a/host-setup/menu.ps1 +++ b/host-setup/menu.ps1 @@ -257,10 +257,23 @@ function Invoke-FetchHubLocked { step "Fetching $script:HUB_REPO at $script:REF" if (-not (Test-HubRemovable)) { return $false } $hubPath = Join-Path $script:DIR 'hub' - if (Test-Path -LiteralPath $hubPath) { Remove-Item -LiteralPath $hubPath -Recurse -Force } + if (Test-Path -LiteralPath $hubPath) { + try { + Remove-Item -LiteralPath $hubPath -Recurse -Force + } catch { + fail "Could not remove $hubPath to fetch it again. Check that nothing holds a file under it: $($_.Exception.Message)" + return $false + } + } # Marked as ours before git can create anything under $hubPath, not only once the clone also succeeds: git can leave a partial directory behind on a failed or interrupted clone, and an unmarked one would then block every retry until removed by hand. - $script:HUB_FETCH_TOKEN = [guid]::NewGuid().ToString('N') - Set-Content -LiteralPath (Get-MarkerPath) -Value $script:HUB_FETCH_TOKEN + $token = [guid]::NewGuid().ToString('N') + try { + Set-Content -LiteralPath (Get-MarkerPath) -Value $token + } catch { + fail "Could not write $(Get-MarkerPath). Check that $script:DIR is writable: $($_.Exception.Message)" + return $false + } + $script:HUB_FETCH_TOKEN = $token # A full clone of the default branch first, whatever -Ref names: spec\audit.py walks the hub's own history to judge whether a carried copy is trailing the file it was copied from, and a shallow clone would read every file as changed at the truncation boundary. # Piped to Out-Host rather than left bare: an unassigned native call's stdout otherwise joins this function's own return value, which return $true/$false below would then be appended to instead of replacing. & git clone --quiet --branch $script:DEFAULT_REF --single-branch $script:HUB_URL $hubPath | Out-Host diff --git a/host-setup/menu.sh b/host-setup/menu.sh index d94696ffc..4961a8f2a 100755 --- a/host-setup/menu.sh +++ b/host-setup/menu.sh @@ -170,14 +170,23 @@ fetch_hub() { fetch_hub_locked() { step "Fetching $HUB_REPO at $REF" - if ! HUB_FETCH_TOKEN=$(od -An -N16 -tx1 /dev/urandom | tr -d ' \n') || [[ -z $HUB_FETCH_TOKEN ]]; then + local token + if ! token=$(od -An -N16 -tx1 /dev/urandom | tr -d ' \n') || [[ -z $token ]]; then fail "Could not read /dev/urandom for the token marking this fetch as this session's." return 1 fi remove_unowned_hub_check || return 1 rm -rf "$DIR/hub" + if [[ -e "$DIR/hub" || -L "$DIR/hub" ]]; then + fail "Could not remove $DIR/hub to fetch it again. Check that nothing holds a file under it." + return 1 + fi # Marked as ours before git can create anything under $DIR/hub, not only once the clone also succeeds: git can leave a partial directory behind on a failed or interrupted clone, and an unmarked one would then block every retry until removed by hand. - printf '%s\n' "$HUB_FETCH_TOKEN" >"$(marker_path)" + if ! printf '%s\n' "$token" >"$(marker_path)"; then + fail "Could not write $(marker_path). Check that $DIR is writable." + return 1 + fi + HUB_FETCH_TOKEN="$token" # A full clone of the default branch first, whatever $REF names: spec/audit.py walks the hub's own history to judge whether a carried copy is trailing the file it was copied from, and a shallow clone would read every file as changed at the truncation boundary and misreport every repo as stale. git clone --quiet --branch "$DEFAULT_REF" --single-branch "$HUB_URL" "$DIR/hub" || { diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py index 0235b6970..f581593f1 100755 --- a/tests/test_bootstrap.py +++ b/tests/test_bootstrap.py @@ -1451,14 +1451,40 @@ def test_a_token_that_cannot_be_read_fails_the_fetch_before_it_changes_anything( self.make_cache(self.TOKEN) result = self.run_body( 'git() { mkdir -p "${!#}"; }\nod() { return 1; }\n' - 'rc=0\nfetch_hub_locked || rc=$?\nprintf "rc=%s\\n" "$rc"\n' + f"HUB_FETCH_TOKEN={self.TOKEN}\nrc=0\nfetch_hub_locked || rc=$?\n" + 'printf "rc=%s token=%s\\n" "$rc" "$HUB_FETCH_TOKEN"\n' ) self.assertEqual(result.returncode, 0, result.stderr) - self.assertIn("rc=1", result.stdout) + self.assertIn(f"rc=1 token={self.TOKEN}", result.stdout) self.assertIn("Could not read /dev/urandom", result.stderr) self.assertTrue((self.dir / "hub" / "README.md").exists()) self.assertEqual((self.dir / "hub.owned").read_text(encoding="utf-8"), self.TOKEN) + def test_a_tree_that_cannot_be_removed_fails_the_fetch_and_keeps_its_marker(self) -> None: + """A clone into the surviving tree would fail, and its cleanup would then drop the marker.""" + self.make_cache(self.TOKEN) + result = self.run_body( + 'git() { mkdir -p "${!#}"; }\nrm() { return 0; }\n' + f"HUB_FETCH_TOKEN={self.TOKEN}\nrc=0\nfetch_hub_locked || rc=$?\n" + 'printf "rc=%s token=%s\\n" "$rc" "$HUB_FETCH_TOKEN"\n' + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn(f"rc=1 token={self.TOKEN}", result.stdout) + self.assertIn("Could not remove", result.stderr) + self.assertEqual((self.dir / "hub.owned").read_text(encoding="utf-8"), self.TOKEN) + + def test_a_marker_that_cannot_be_written_fails_the_fetch_without_cloning(self) -> None: + """A directory at the marker's name refuses the write, which errexit would not catch here.""" + (self.dir / "hub.owned").mkdir() + result = self.run_body( + f'git() {{ mkdir -p "${{!#}}"; }}\nHUB_FETCH_TOKEN={self.TOKEN}\nrc=0\n' + 'fetch_hub_locked || rc=$?\nprintf "rc=%s token=%s\\n" "$rc" "$HUB_FETCH_TOKEN"\n' + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn(f"rc=1 token={self.TOKEN}", result.stdout) + self.assertIn("Could not write", result.stderr) + self.assertFalse((self.dir / "hub").exists()) + def run_body(self, body: str) -> subprocess.CompletedProcess[str]: script = self.scripts / "body.sh" script.write_text( @@ -1658,6 +1684,13 @@ class TestPowerShellMenuHubCleanup(HubCleanupCases, unittest.TestCase): """`menu.ps1`'s `Invoke-Cleanup`, under its real lock, its marker ending CRLF as on Windows.""" LINE_BREAK = "\r\n" + FETCH_SETUP = ( + "$script:HUB_REPO = 'owner/hub'\n$script:HUB_URL = 'https://example.invalid/hub'\n" + "$script:DEFAULT_REF = 'main'\n$script:REF = 'main'\n$script:HUB_ROOT = ''\n" + "function step { param([string]$Message) }\n" + "function git { New-Item -ItemType Directory -Path $args[-1] -Force | Out-Null; " + "$global:LASTEXITCODE = 0 }\n" + ) def setUp(self) -> None: self.dir = Path(self.enterContext(tempfile.TemporaryDirectory())) @@ -1674,13 +1707,46 @@ def fetch_body(self, refetched: bool) -> str: "$script:HUB_FETCH_TOKEN = $mine\n" ) return ( - "$script:HUB_REPO = 'owner/hub'\n$script:HUB_URL = 'https://example.invalid/hub'\n" - "$script:DEFAULT_REF = 'main'\n$script:REF = 'main'\n$script:HUB_ROOT = ''\n" - "function step { param([string]$Message) }\n" - "function git { New-Item -ItemType Directory -Path $args[-1] -Force | Out-Null; " - "$global:LASTEXITCODE = 0 }\n" - "[void](Invoke-FetchHubLocked)\n" + (again if refetched else "") + "Invoke-Cleanup\n" + self.FETCH_SETUP + + "[void](Invoke-FetchHubLocked)\n" + + (again if refetched else "") + + "Invoke-Cleanup\n" + ) + + def test_a_tree_that_cannot_be_removed_fails_the_fetch_and_keeps_its_marker(self) -> None: + """It fails the one task rather than ending the menu's whole session. + + The refusal is stubbed, since what holds a file open differs by platform. + """ + self.make_cache(self.TOKEN) + result = self.run_body( + self.FETCH_SETUP + + f"$script:HUB_FETCH_TOKEN = '{self.TOKEN}'\n" + + "function Remove-Item { throw 'held' }\n" + "$ok = Invoke-FetchHubLocked\n" + '[Console]::Out.WriteLine("ok=$ok token=$script:HUB_FETCH_TOKEN")\n' + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout.strip().splitlines()[-1], f"ok=False token={self.TOKEN}") + self.assertIn("Could not remove", result.stderr) + self.assertEqual((self.dir / "hub.owned").read_text(encoding="utf-8"), self.TOKEN) + + def test_a_marker_that_cannot_be_written_fails_the_fetch_without_cloning(self) -> None: + """It fails the one task rather than ending the menu's whole session. + + The refusal is stubbed, since what refuses a write differs by platform and by account. + """ + result = self.run_body( + self.FETCH_SETUP + + f"$script:HUB_FETCH_TOKEN = '{self.TOKEN}'\n" + + "function Set-Content { throw 'refused' }\n" + "$ok = Invoke-FetchHubLocked\n" + '[Console]::Out.WriteLine("ok=$ok token=$script:HUB_FETCH_TOKEN")\n' ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(result.stdout.strip().splitlines()[-1], f"ok=False token={self.TOKEN}") + self.assertIn("Could not write", result.stderr) + self.assertFalse((self.dir / "hub").exists()) def run_body(self, body: str) -> subprocess.CompletedProcess[str]: body_file = self.scripts / "body.ps1"