fix: pdeathsig, namespace timeout, arping for remaining flaky tests

[ejc3]

Summary

Fixes 5 of 6 flaky tests from the latest main CI run (22565260533).

• PR_SET_PDEATHSIG fix: Switch rootless baselines from nsenter to pre_exec setns path. nsenter's internal setns(CLONE_NEWUSER) clears PR_SET_PDEATHSIG (kernel zeros task->pdeath_signal on credential changes), leaving Firecracker orphaned when fcvm is SIGKILL'd. The pre_exec setns path (already used by clones) sets pdeathsig AFTER entering the user namespace.

  • Fixes: test_sigkill_kills_firecracker_rootless

• Namespace timeout 5s→30s: unshare --user --net can take >5s under heavy parallel test load on CI runners. Increased from 5s to 30s.

  • Fixes: test_clone_connection_reset_rootless, test_clone_connection_timing_rootless

• arping replaces ping: Already on main from PR fc-agent: clock sync, ss -K fix, --no-swap cgroup isolation, --no-dirty-tracking #536. send_gratuitous_arp() used ping -c 1 -W 3 which always failed in rootless (3s timeout). Now uses arping -c 1 -w 1 -I eth0 <gateway> which works at L2.

  • Fixes: test_clone_port_forward_rootless

Not fixed in this PR:

• test_snapshot_clone_stress_100_bridged — Firecracker EINVAL from mount namespace with many baseline_dirs (separate issue)
• test_egress_clone_bridged — container wget timeout to host test server (pre-existing, not clone-related)

Test Results

$ make test-root FILTER="-E 'test(/sigkill|sanity|clone_connection|clone_port_forward|egress_clone|snapshot_clone_stress/)'" STREAM=1
16 tests run: 16 passed, 0 failed

[claude-claude]

🔍 Claude Review

SEVERITY: medium

Findings

[MEDIUM] Misleading commit message: The single commit 4843c83 is titled "fix: replace blocking ping with arping for gratuitous ARP" but the actual changes are the pdeathsig/setns fix and namespace timeout increase. The arping change was already merged to main via PR #536 (commit 2480c73). The commit message appears to have been copied from that earlier commit.

[MEDIUM] Dead nsenter code path in VmManager::start(): The else if let Some(holder_pid) = self.holder_pid branch (vm.rs:165-183) is now unreachable. The only call site that sets holder_pid on VmManager (setup_rootless_namespace in namespace.rs:229) now also sets user_namespace_path (line 220), which is checked first in the if chain. No other code path calls set_holder_pid() on VmManager without also calling set_user_namespace_path(). This dead code will confuse future readers into thinking nsenter is still used for Firecracker process launch.

[MEDIUM] Stale comments in vm.rs:

• Line 150: "2. holder_pid set (no user_namespace_path): use nsenter to enter existing namespace (rootless baseline)" — this describes the now-dead nsenter path as the rootless baseline path.
• Lines 91-98: set_user_namespace_path doc says "for rootless clones" and "Use this instead of set_holder_pid when mount namespace isolation is needed" — now misleading since it's used for ALL rootless VMs (baselines + clones), not just clones needing mount isolation.

[MEDIUM] DESIGN.md stale: Line 472 says "Run Firecracker via nsenter: nsenter -t HOLDER_PID -U -n -- firecracker ..." but Firecracker is now launched directly with pre_exec setns for rootless baselines. Steps 2/4/6 (setup scripts, health checks) still correctly use nsenter, but step 5 is now inaccurate.

[MEDIUM] PR description overclaims: The description lists 3 fixes, but the arping change (fix #3) is not part of this PR — it was already merged on main. The PR contains 2 actual changes: the pdeathsig fix and the timeout increase.

Summary

The core pdeathsig fix (switching rootless baselines from nsenter to pre_exec setns) is well-motivated and the implementation is correct. However, the PR leaves dead code (the unreachable nsenter branch), stale comments, and stale documentation that will mislead future readers about how rootless Firecracker launch works. The commit message also doesn't describe the actual changes.

---

Review by Claude | Run

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4843c836ff

ℹ️ 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]

Keep namespace readiness deadline consistent

setup_namespace_mappings() now waits up to 30s for /proc/<pid>/ns/user, but spawn_namespace_holder() still computes its absolute deadline from HOLDER_RETRY_TIMEOUT (5s) and passes that same deadline into wait_for_namespace_ready(). If namespace creation takes longer than 5s (the case this commit targets), wait_for_namespace_ready() starts with an already-expired deadline and will time out after the first failed probe, so rootless startup can still fail under load instead of getting the intended extended grace period.

Useful? React with 👍 / 👎.

[claude-claude]

🔧 Auto-Fix Created

I found issues and created a fix PR: #542

Please review and merge the fix PR first, then this PR.

View Claude Run

[claude-claude]

🔍 Claude Review

SEVERITY: medium

Findings

[MEDIUM] PR description does not cover second commit: Commit 0195816 ("fix: PR_SET_PDEATHSIG on namespace holders, add bench diagnostics") adds significant changes — PR_SET_PDEATHSIG on holder processes in common.rs, find_holder_for_fcvm() in tests, bench diagnostic improvements, and Makefile cleanup — but the PR description only covers the first commit's changes. The description should be updated to reflect the full scope.

[MEDIUM] Dead nsenter branch + stale comments/docs: As noted in previous review, the dead nsenter code path (vm.rs:165-183), stale struct comments (vm.rs:39-41), and stale DESIGN.md step 5 (line 472) remain unfixed. Fix PR #542 addresses these but has not been merged yet.

[LOW] find_holder_for_fcvm() uses broad pgrep pattern: The function uses pgrep -f "sleep infinity" which could match unrelated processes. The Makefile's cleanup already uses the more specific pattern unshare.*sleep infinity. The is_descendant_of() check mitigates false positives in practice, so this is low risk.

Summary

The second commit (0195816) is a solid addition — adding PR_SET_PDEATHSIG to holder processes fixes a real resource leak (orphaned namespace holders accumulating on the host), the test extension properly validates both Firecracker and holder cleanup, and the bench diagnostics will help root-cause future HTTP failures. The core implementation in both commits is correct. The main gap is the PR description not reflecting the second commit, and the stale code/docs tracked in PR #542.

---

Review by Claude | Run

[claude-claude]

🔍 Claude Review

SEVERITY: medium

Findings

[MEDIUM] PR description still does not cover commits 2 and 3: As noted in previous review, the PR description only covers the first commit's changes. Now there is a third commit (9ac5725 — stale cargo cache detection) that is also not described. The description should be updated to reflect all 3 commits.

[MEDIUM] Dead nsenter branch + stale comments/docs remain unfixed: As noted in first review, the dead nsenter code path (vm.rs:165-183), stale struct comments (vm.rs:39-41), and stale DESIGN.md step 5 (line 472) remain. Fix PR #542 addresses these but has not been merged yet.

New Commit Analysis: 9ac5725 (stale cargo cache detection)

The Makefile build target now detects missing build script output files and cleans the cargo cache before building. The logic is correct: if target/release/build exists but target/release/build/fcvm-*/output files are missing, the cache is stale (e.g., from force-push rewriting git history that cargo fingerprints depend on). The cleanup removes build/, deps/, and .fingerprint/ to force a full rebuild. No issues with this change.

Summary

The new commit (stale cargo cache detection) is a straightforward and correct CI reliability fix. The two previously reported medium issues remain: PR description is incomplete and stale code/docs tracked in PR #542 are still unmerged.

---

Review by Claude | Run

[claude-claude]

🔍 Claude Review

SEVERITY: medium

Findings

[MEDIUM] PR description still does not cover commits 2, 3, and 4: As noted in previous review, the PR description only covers the first commit's changes. There are now 4 total commits but only the first is described.

[MEDIUM] Dead nsenter branch + stale comments/docs remain unfixed: As noted in first review, these issues remain. Fix PR #542 is still open.

[MEDIUM] Commit 348aaaf message claims "single source of truth" but version is duplicated: The commit message says "CI: use dtolnay/rust-toolchain@master with toolchain='' so it reads rust-toolchain.toml" and "Single source of truth: rust-toolchain.toml controls everything". But the actual implementation uses dtolnay/rust-toolchain@1.93.0 (hardcoded in 3 places in ci.yml), not @master with an empty toolchain parameter. The @1.93.0 ref installs that specific Rust version regardless of rust-toolchain.toml. This means the Rust version is now specified in two places (rust-toolchain.toml AND ci.yml), contradicting the single-source-of-truth goal. The self-hosted runner jobs correctly use --default-toolchain none and let rust-toolchain.toml drive the version. The GitHub-hosted runner jobs do not.

New Commit Analysis: 348aaaf (pin Rust 1.93.0 in CI)

The changes are functionally correct — Rust 1.93.0 will be installed and components/targets from rust-toolchain.toml will be respected. The self-hosted runner improvements (retry flags on curl, --default-toolchain none, rustup show active-toolchain) are solid. The concern is the misleading commit message and the version duplication for GitHub-hosted runners. A future Rust version bump will need to update both rust-toolchain.toml and 3 lines in ci.yml.

Summary

The new commit is functionally correct but the commit message inaccurately describes the implementation (claims @master but uses @1.93.0, claims single-source-of-truth but the version is duplicated). Previously reported issues (dead code, stale docs, incomplete PR description) remain as tracked in PR #542.

---

Review by Claude | Run

[claude-claude]

🔍 Claude Review

SEVERITY: medium

Findings

No new commits since previous review. All previously reported issues remain:

• [MEDIUM] Dead nsenter branch + stale comments/docs: As noted in first review, the unreachable nsenter code path (vm.rs:165-183), stale comments, and stale DESIGN.md step 5 remain. Fix PR #542 is still open.

• [MEDIUM] PR description does not cover commits 2-4: As noted in previous reviews, the description only describes the first commit's changes. Commits for PR_SET_PDEATHSIG on holders, stale cargo cache detection, and Rust 1.93.0 pinning are not described.

• [MEDIUM] Commit 364759d message inaccuracy: As noted in previous review, the commit message claims @master with empty toolchain but actually uses @1.93.0, and claims single-source-of-truth while duplicating the version in 3 CI lines.

Summary

No changes since last review. The core implementation is correct. Outstanding issues are the incomplete PR description, stale code/docs (tracked in PR #542), and the misleading commit message on the CI commit.

---

Review by Claude | Run

[claude-claude]

🔍 Claude Review

SEVERITY: medium

Findings

[MEDIUM] New commit b46a392 — stale comment after timeout increase: The commit changes MAX_EXEC_MS from 2000 to 5000, but the doc comment on line 179 of test_vsock_connect_stress.rs still says "Every exec must succeed and complete in under 2 seconds." This should say 5 seconds to match the new constant.

[MEDIUM] Dead nsenter branch + stale comments/docs remain unfixed: As noted in first review, the unreachable nsenter code path (vm.rs:165-183), stale comments, and stale DESIGN.md step 5 remain. Fix PR #542 is still open.

[MEDIUM] PR description does not cover commits 2-5: As noted in previous reviews, the description only describes the first commit's changes. Commits for PR_SET_PDEATHSIG on holders, stale cargo cache detection, Rust 1.93.0 pinning, and vsock exec timeout are not described.

New Commit Analysis: b46a392 (vsock exec timeout 2s→5s)

The change aligns MAX_EXEC_MS with the existing tokio::time::timeout(Duration::from_secs(5)) on line 47, which was already 5s — so the original code had an inconsistency where the assertion (2s) was stricter than the operation timeout (5s). Making them consistent is reasonable. For CI runners under heavy parallel load, CPU contention can legitimately slow trivial operations. The test still catches real regressions — it logs "SLOW" at 500ms and the 5s cap prevents unbounded latency.

Summary

The new commit is a minor and consistent change. The only new issue is the stale "under 2 seconds" comment. Previously reported issues (dead code, stale docs, incomplete PR description) remain as tracked in PR #542.

---

Review by Claude | Run

[claude-claude]

🔧 Auto-Fix Created

I found issues and created a fix PR: #543

Please review and merge the fix PR first, then this PR.

View Claude Run

[claude-claude]

🔍 Claude Review

SEVERITY: medium

Findings

New commit 7b9a07e: Replaces filtered log-line diagnostics in benches/clone.rs and benches/exec.rs with comprehensive failure diagnostics (ss, connect_timeout probe, stale process counts, raw log tail). Pure observability improvement, no behavioral changes. No issues with this commit.

Previously reported issues still open:

• [MEDIUM] Dead nsenter branch + stale comments/docs: As noted in first review, these remain. Fix PR #542 is still open.

• [MEDIUM] Stale "under 2 seconds" comment in test_vsock_connect_stress.rs:179: As noted in previous review, the doc comment says "under 2 seconds" but MAX_EXEC_MS is now 5000. Fix PR #543 is still open.

• [MEDIUM] PR description does not cover commits 2-6: As noted in previous reviews, the description only describes the first commit. Commits for PR_SET_PDEATHSIG on holders, stale cargo cache detection, Rust 1.93.0 pinning, vsock exec timeout, and bench diagnostics are not described.

Summary

The new commit is a clean diagnostic improvement — no issues. The three previously reported medium issues remain tracked in PRs #542 and #543 (both still open). The core implementation across all 6 commits is correct.

---

Review by Claude | Run

[claude-claude]

🔍 Claude Review

SEVERITY: medium

Findings

New commits since last review: 282016f (clean stale network namespaces) and b45e17b (update stale comments from code review).

Commit 282016f — clean stale network namespaces: Well-motivated fix. The commit message documents the root cause (188 stale namespaces across 7 CI runners degrading pasta forwarding). The pgrep -x sleep -P 1 replacement for pkill -f is correct — avoids the self-match problem. The ip netns del loop with fcvm- prefix filter is safe. No issues.

Commit b45e17b — update stale comments: Fixes two previously reported issues (nsenter branch comment in vm.rs and "under 2 seconds" in test_vsock_connect_stress.rs). However, several related stale comments in the same file were missed:

[MEDIUM] Remaining stale comments/docs not addressed by b45e17b:

• vm.rs:39 — holder_pid field comment still says "use nsenter to run FC" but nsenter is no longer used to run FC for rootless VMs.
• vm.rs:40-41 — user_namespace_path and net_namespace_path comments still say "for rootless clones" but are now used for ALL rootless VMs (baselines + clones).
• vm.rs:91 — set_user_namespace_path doc says "for rootless clones" — same issue.
• vm.rs:97-98 — doc says "Use this instead of set_holder_pid when mount namespace isolation is needed, since nsenter wrapper runs AFTER pre_exec" — misleading since the real reason is now PR_SET_PDEATHSIG preservation, and it's always used (not just for mount isolation).
• vm.rs:150 — Block comment still says "2. holder_pid set (no user_namespace_path): use nsenter to enter existing namespace (rootless baseline)" — describes this as the normal rootless baseline path, but it's now a fallback.
• DESIGN.md:472 — Still says "Run Firecracker via nsenter" for step 5 of rootless setup.

[MEDIUM] PR description does not cover commits 2-8: As noted in previous review, the description only describes the first commit. There are now 8 total commits.

Summary

The two new commits are correct and well-motivated. Commit b45e17b addresses some of the stale comment issues from previous reviews, but several related stale comments and docs in the same files were missed. The core implementation across all 8 commits remains correct.

---

Review by Claude | Run

[claude-claude]

🔧 Auto-Fix Created

I found issues and created a fix PR: #544

Please review and merge the fix PR first, then this PR.

View Claude Run

[claude-claude]

🔍 Claude Review

SEVERITY: medium

Findings

New commit aa0e7cb (update stale comments from code review): Fixes two previously flagged issues — the nsenter branch comment in vm.rs:165-168 and the "under 2 seconds" comment in test_vsock_connect_stress.rs:179. Also adds iptables/ip6tables cleanup for stale MASQUERADE rules from fcvm VMs in clean-test-data. The iptables cleanup is correct (scoped to 172.30.x/10.0.x subnets, excludes NETAVARK rules for ip6tables). No issues with this commit.

[MEDIUM] Remaining stale comments/docs not addressed by aa0e7cb: As noted in previous review, several stale comments and docs remain. These are tracked in fix PR #544 which is still open:

• vm.rs:39 — holder_pid comment says "use nsenter to run FC"
• vm.rs:40-41 — says "for rootless clones" (now used for all rootless VMs)
• vm.rs:91-98 — set_user_namespace_path doc says "for rootless clones" and misleading reason for preferring over set_holder_pid
• vm.rs:150 — block comment says "2. holder_pid set: use nsenter ... (rootless baseline)"
• DESIGN.md:472 — says "Run Firecracker via nsenter"

[MEDIUM] PR description does not cover commits 2-8: As noted in previous reviews, the description only covers the first commit. There are now 8 total commits (pdeathsig on holders, stale cargo cache, Rust 1.93.0, vsock exec timeout, bench diagnostics, stale namespace cleanup, stale comment fixes).

Summary

The new commit is correct and addresses two previously flagged issues. The remaining stale comments/docs are tracked in open fix PR #544. Core implementation across all 8 commits remains correct — no new issues found.

---

Review by Claude | Run

[claude-claude]

🔍 Claude Review

SEVERITY: medium

Findings

No new commits since previous review. All previously reported issues remain:

• [MEDIUM] Stale comments/docs (vm.rs:39-41, 91-98, 150; DESIGN.md:472): As noted in first review, several stale comments and docs remain. Fix PR #544 is still open.

• [MEDIUM] PR description does not cover commits 2-8: As noted in previous reviews, the description only describes the first commit's changes.

• [MEDIUM] Commit 364759d message inaccuracy: As noted in previous review, the commit message claims @master with empty toolchain but actually uses @1.93.0.

Summary

No changes since last review. The core implementation across all 8 commits is correct — the pdeathsig fix, holder cleanup, namespace cleanup, bench diagnostics, and CI improvements are all well-motivated and properly implemented. The outstanding issues are tracked in open fix PR #544.

---

Review by Claude | Run

[claude-claude]

🔍 Claude Review

SEVERITY: medium

Findings

No new commits since previous review. All previously reported issues remain:

• [MEDIUM] Stale comments/docs (vm.rs:39-41, 91-98, 150; DESIGN.md:472): As noted in first review, several stale comments and docs remain. Fix PR #544 is still open.

• [MEDIUM] PR description does not cover commits 2-8: As noted in previous reviews, the description only describes the first commit's changes.

Summary

No changes since last review. The core implementation across all 8 commits is correct. Outstanding stale comments/docs are tracked in open fix PR #544.

---

Review by Claude | Run