fix(docker): harden apt.llvm.org bootstrap

[davidtaikocha]

Summary

• fetch llvm.sh and its signing key with bounded GET retries
• remove the hash-pinned installer repository HEAD preflight and let signed apt-get update validate the repository
• pre-seed the key so upstream skips its key HEAD and unbounded key GET
• test both failing HEAD probes deterministically in the arm64 LLVM CI job

Root cause

Three arm64 jobs exposed separate network failure signatures against apt.llvm.org:

• job 94359650498: the /trixie/ HEAD preflight timed out
• job 94366430808: key HEAD timed out twice and /trixie/ HEAD timed out once
• job 94383071812: the key GET stalled for 900 seconds, followed by two /trixie/ HEAD timeouts

The pinned upstream installer treats a transient repository HEAD failure as an unsupported distribution and performs its key GET with the default 900-second wget timeout.

The wrapper now verifies the upstream script hash, disables only its redundant repository HEAD check, and relies on the signed apt-get update already protected by Acquire::Retries. It also downloads the signing key atomically using four bounded GET attempts, which eliminates both upstream key operations.

Regression coverage

The Docker integration test runs the real wrapper in Debian Trixie and makes both repository and signing-key HEAD requests fail while bounded GETs succeed.

• before the repository-probe fix: all three installer attempts failed and the test exited 1
• after the fix: no upstream HEAD request is required and the LLVM command contract completes

Validation

• red-green Docker regression test
• native linux/arm64 Debian Trixie install: LLVM 22.1.8
• ShellCheck
• actionlint
• just fmt-check
• just clippy
• just test: 294 passed

[claude]

Claude finished @davidtaikocha's task —— View job

---

PR Review: Docker LLVM Key Download Fix

Tasks:

• Read and analyze changed files
• Review code quality and best practices
• Check for bugs and security issues
• Evaluate test coverage
• Provide comprehensive feedback

---

Summary

Strong fix that addresses a real production issue with excellent test coverage. The solution is well-engineered with proper retry logic, atomic operations, and comprehensive validation.

Code Quality & Best Practices ✅

install_llvm_ubuntu.sh

• Clean shell scripting with proper error handling (set -eo pipefail)
• Good variable naming and documentation
• Appropriate use of temporary files with atomic operations (line 63: install -m 0644)
• Bounded retry logic with exponential backoff is well-implemented
• Proper cleanup of temporary files

install_llvm_ubuntu_test.sh

• Creative test design using symlinks for command interception
• Tests the actual failure scenario (HEAD fails, GET succeeds)
• Validates end-to-end behavior including installer contract

ci.yml

• Test properly integrated into the arm64 job where the issue manifests

Security Concerns ✅

• SHA256 verification preserved (line 42 in install_llvm_ubuntu.sh) - ensures installer integrity
• Atomic key installation (line 63) - prevents partial writes from being used
• File permissions properly set (0644 for GPG key)
• No secrets or credentials exposed
• Temporary file handling is secure

Potential Issues 🟡

1. Hardcoded retry attempts inconsistency (minor):

   • Installer/key download: 4 attempts (line 28, 51)
   • Installer execution: 3 attempts (line 72)
   • No clear rationale for the difference. Consider documenting why or unifying.

2. Sleep timing could overflow (edge case):

   • Lines 38, 61: sleep $((attempt * 5))
   • With 4 attempts: 5s, 10s, 15s (reasonable)
   • But the pattern is fragile if someone increases attempts significantly
   • Consider capping: sleep $((attempt * 5 < 60 ? attempt * 5 : 60))

3. Test environment coupling (minor):

   • Line 43: test script hardcodes /work/install_llvm_ubuntu_test.sh as the fake installer
   • Works fine but creates circular dependency (test script copies itself as fake installer)
   • Not a bug, just slightly confusing on first read

4. Missing stderr redirect check:

   • Line 21: 2>/dev/null || true silently swallows errors
   • While intentional (software-properties-common is optional), a comment explaining why would help

Performance Considerations ✅

• Retry delays are reasonable (5s, 10s, 15s for downloads; 10s, 20s for installer)
• Avoids the previous 15-minute timeout issue
• Only downloads key once and checks for existing installation (line 49)
• Efficient use of wget options (--tries=1 with manual retry gives better control)

Test Coverage ✅

Excellent regression coverage:

• ✅ Simulates the exact failure mode (HEAD fails, GET succeeds)
• ✅ Validates key is installed via GET
• ✅ Verifies installer contract (symlinks created)
• ✅ Runs on actual arm64 hardware where issue occurred
• ✅ Tests both the wrapper and real installer in Docker

Minor gap:

• No test for the installer retry loop (lines 72-89)
• Not critical since it's similar pattern to the key download retry

Recommendations

1. Add comment for software-properties-common suppression:

   # software-properties-common is needed on bookworm but not newer releases
   apt-get install -y --no-install-recommends software-properties-common 2>/dev/null || true

2. Document retry count difference or unify to 4 attempts for consistency

3. Consider capping sleep times if you expect to potentially increase retry counts in future

4. ShellCheck validation: Assuming this passed (mentioned in PR description) ✅

Verdict

Approve with minor suggestions. This is a well-researched fix that:

• Correctly identifies and solves the root cause
• Uses appropriate defensive techniques (bounded retries, atomic operations)
• Includes regression tests that reproduce the failure
• Maintains security properties (SHA256, permissions)

The suggestions above are minor refinements, not blockers.

---