[WenYuLuo]

Cold OCI preparation could start runc after the create deadline, and runtime deletion errors could be mistaken for successful absence. Pin sandboxd official-main commit d5886b42ceff89bf0bf9baf7f73f5c534f689dee, including merged sandboxd #72 and #74. Runtime deletion failure retains metadata, mounts/private storage and resource ownership for subsequent attempts.

The opt-in fault-runc-delete-retry case now fails exactly one selected runc delete invocation. An atomic default-pass launcher delegates to an unchanged copy of real runc. A five-second token matches the exact physical sandbox/delete arguments and original sandboxd process identity, and is consumed before returning the injected error. Other commands and later deletes execute real runc; a daemon restart invalidates any pending token. Installation atomically replaces a valid executable path with a ready launcher, so an unknown Docker completion does not require moving a missing binary back. The exclusive fixture is destroyed after validation.

The case captures retained metadata/private-file hashes, namespace/cgroup and ADX held/assignment evidence at the following real delete, then only polls physical/Redis cleanup without another public DELETE. A final explicit operation retires the remaining catalog record separately. The runner rejects this group with under 120 seconds of remaining budget; the child checks at least 100 seconds before arming. Final evidence is written even when disarming or catalog cleanup raises. Unit regressions cover exact matching, concurrent single consumption, expiry, daemon replacement, a controller killed after injection and insufficient campaign budget.

Validation of the maintained fault code at fe2ae123972bc382352a54679c6be53d6b407cbe; final gitlink-only update at 579646f6f2f85e21f7d212a324d879716218b993:

• SDK checks pass 298 unit tests, 35 benchmark-driver tests, Ruff, mypy (22 source files) and deployment syntax checks. New injector contract tests were exercised RED before the one-shot claim implementation and GREEN afterward. A first full check selected a virtualenv missing adx_environment; the complete passing check used the established #161 validation virtualenv, with the failed environment log retained.
• Fresh task-owned Linux AMD64 native-runc fixture used real Redis/ADX API Server/Platform/sandboxd/Execd. The fault group passed 1 test, zero skips. Exactly one injected deletion failure was followed by a real delegated delete. Before that retry, sandboxd metadata, private-file hash, namespace/cgroup and Failed/held assignment remained intact.
• Upstream retry completed physical cleanup in 0.40 seconds. The single public DELETE returned 200 after internal recovery; no second public DELETE was issued before cleanup verification. Official Redis audit is cluster-clean, and independent final inspection found no runtime states, metadata/storage, netns, mounts or audited runtime processes. The exact test fixture was removed; the original service/container remained running.
• The verified existing native-runc image 9ed03b... was used as the base, replacing only sandboxd with a binary built from exact commit 701887a; test image sha256:e70dad071ee239041c61bf863a21e8ca05780f64624d73e2b303973688ac1d4d. Sandboxd and unchanged #161 ADX/Execd binary hashes were verified. Canonical official-main submodule checkout succeeded. Its tracked tree ceb0e3ec035fe7db3922626db405a1a10f39d5b5 is identical to the tested 701887a tree; the final pin changes only commit identity. All five tested Python file fingerprints match this commit.
• Final-head GitHub CI passes all 10 checks on 579646f, with publishing skipped as expected. Sandboxd feat: configure shared ChunkDB capacity in node deployments #74's reviewed head 701887a has all 14 CI checks passing at attempt 2, and the merged dependency d5886b4 has the identical tracked tree; the initial stats-output failure and denied initial rerun are retained in its historical evidence.

Sandboxd #74 is merged. Related to #96; its Delete-ACK timing and original large-image/macOS/guest acceptance remain independent. ADX #161 artifact/source pins and default-disabled TLS verification are unchanged. This Linux/native-runc validation does not claim macOS/QEMU guest acceptance.

[rcyw]

@WenYuLuo Please address these two P2 recovery gaps in the new fault test before merging. This review covers HEAD cb90d6f5ac059e4927da5eadd7345ede4871926f, including the full 305-line test, runner registration and documentation. This is comment-only feedback with a request-changes recommendation, not a submitted approval/review event.

1. A completed rename with a failed Docker completion skips restoration. At test_runc_delete_retry.py:213–221, hidden = True is assigned only after the mutating Docker command returns. If the container-side rename succeeds but Docker reports an error/lost completion, execution enters finally with hidden == False, and restore():144–153 does nothing.

I executed the actual reviewed test method with Docker/Redis/HTTP replaced by controlled fakes. The Docker substitute renamed a task-local dummy file and then raised subprocess.CalledProcessError, representing an unknown command outcome. Observed:

test error recorded = true
runc present after finally = false
backup present after finally = true
restoration called = false

Please persist recovery intent before mutation, including the exact container ID and original/backup paths, and make restoration idempotent based on the actual checked filesystem state rather than only a post-success Boolean. Setting the Boolean earlier alone is insufficient when the rename never happened. Use the full verified container ID for recovery actions, preserve unexpected replacements, and ensure final evidence is written even if restoration or catalog cleanup raises; the current JSON write at lines 299–301 can also be skipped by an earlier cleanup exception.

2. The runner can kill the only restoration owner while runc is hidden. The new case registration at run.py:204–208 uses the existing min(case.timeout_seconds, remaining) / subprocess.run(timeout=...) path. The runner will start the case with very little aggregate budget remaining, even though the fault-period observation loop alone lasts 12 seconds. On timeout, the child is killed and its finally restoration cannot run.

Reproduced with the actual runner and actual test method, a two-second aggregate budget, fully simulated endpoints and task-local dummy files:

runner result = timed-out
runc present after runner returns = false
backup present after runner returns = true

Please reject destructive injection when insufficient execution/restoration budget remains, and provide restoration outside the child that may be killed, such as a parent-side recovery guardian/hook using the exact fixture ID and durable recovery record. Record restoration completion/failure separately from the timed-out test result. A child-only finally cannot supply that guarantee under the maintained runner's timeout policy.

Suggested regressions for these paths:

• Rename succeeds but its completion is lost; rename never happens; restoration itself fails; catalog cleanup raises. Verify either checked restoration or an explicit retained recovery record, and always preserve final error/evidence state.
• Expire the campaign budget after injection. The runner must recover the task-local executable or explicitly report an unresolved recovery failure, rather than returning only timed-out with the fixture still silently altered.
• Keep the successful path's separation: retention assertions during failure, read-only polling after restoration with no further public delete, then explicitly recorded Failed-catalog retirement. Continue the missing-fixture and wrong-container-ID gates.

The tightened gate correctly skips when required fixture/integration/fault variables are missing; six independent missing-variable checks made no Docker/API calls. The normal source control flow correctly separates automatic-cleanup observations from final catalog retirement. These positives do not cover the exceptional recovery paths above.

Evidence boundaries: the recovery reproductions are simulated control-flow tests, not a live native-runc fault-E2E pass. Exact-head AKernel CI has 10 successful checks and the expected publish skip; local SDK unit/benchmark/Ruff/mypy and deployment syntax checks pass. Default Standalone E2E builds with runc disabled and does not exercise this opt-in full-level case.

I independently fetched sandboxd 794318afbde0299579cb77d5c87e1d03998c2f0b and verified its tracked tree equals dependency b47728a9bf690d7dabb431e7795af1b95254f85d: 96f07231890ff80a8d934e7ad475afceae83543e. Canonical gitlink fetch and actual ADX archive/source/hash checks passed. The final 305-line test SHA-256 is 085ce9027aee3c0fbd96640b4e7fedd5188b39e7f53dc3db7232928fdf0fc740; the author-reported live replay remains separate from this review's locally executed evidence. No live image or guest acceptance was reused without verification.

Please fix and merge sandboxd #74 first, then verify this PR's final dependency SHA/code equivalence, final test/recovery-file fingerprints and updated CI, and rerun the corrected fault test on a verified exclusive fixture. AKernel #96 continues tracking the remaining cleanup semantics and real-workload/guest acceptance. This round did not execute cold imports, live cancellation cleanup, KVM/QEMU or macOS guest acceptance.

[rcyw]

@WenYuLuo Follow-up review against the still-current HEAD cb90d6f5ac059e4927da5eadd7345ede4871926f: both previously reported P2 recovery gaps remain reproducible; please fix them and update the final sandboxd dependency before merging #99. This is ordinary comment-only feedback, not a formal GitHub review submission or merge action. The current-head CI has 10 successful checks and the expected publish skip; that does not cover these opt-in fault-recovery paths.

The 305-line fault test is unchanged, SHA-256 085ce9027aee3c0fbd96640b4e7fedd5188b39e7f53dc3db7232928fdf0fc740; the runner is unchanged as well. I independently reran the actual reviewed test method and actual runner with controlled Docker/HTTP/Redis substitutes and task-local dummy executable files. No live Docker container, ADX service, VM or original disk was faulted.

P2 — a successful rename with a failed Docker completion still skips restoration

At test_runc_delete_retry.py:213–221, hidden = True is set only after the mutating Docker command returns. The substitute applies the rename and then raises subprocess.CalledProcessError, simulating a lost/failed command completion after the side effect. restore():144–153 sees hidden == False and skips recovery. Observed again this round:

test errors = 1
runc present after finally = false
backup present after finally = true
restoration called = false

Please persist recovery intent before mutation, including the full verified container ID and original/backup paths. Restore idempotently from the checked filesystem state, preserve unexpected replacements, and verify the restoration outcome. Merely setting the Boolean earlier does not cover the opposite case where the rename never happened. Recovery actions should use the full verified ID rather than relying on the mutable container name.

P2 — the runner still kills the sole restoration owner on timeout

The new case registration at run.py:204–208 uses the existing remaining-budget / subprocess.run(timeout=...) path at lines 455–480. The case can start with two seconds remaining, although the fault-period observations alone hold runc unavailable for 12 seconds. A runner timeout kills the child and prevents its finally from restoring the executable. This round's actual-runner reproduction with the actual test method and simulated endpoints produced:

aggregate budget = 2 seconds
runner result = timed-out
elapsed = 2.002 seconds
runc present after runner returns = false
backup present after runner returns = true

Please reject destructive injection when insufficient execution/restoration budget remains and add restoration outside the child that may be killed, such as a parent-side recovery hook/guardian backed by the exact fixture ID and durable recovery intent. Record restoration success or unresolved recovery failure separately from the timed-out test result. A child-only finally cannot satisfy this runner's timeout policy.

Evidence preservation and required regressions

I additionally simulated restoration failure and final catalog-cleanup failure separately. Both correctly raise a unittest error, but either can bypass the final JSON write at test_runc_delete_retry.py:290–301; the last saved snapshot lacks the final cleanup outcome/proof. Please put final evidence persistence on an independent cleanup path and preserve the original test error together with recovery/catalog errors.

Please add maintained regressions that cover:

• Rename applied but completion lost; rename never applied; a replacement unexpectedly appears at the original path. Check safe, idempotent recovery of the exact owned fixture and explicit unresolved-recovery evidence when restoration cannot complete.
• Runner budget expires after injection. Exercise the maintained runner policy and require recovery outside the killed child, with the restoration result recorded separately.
• Restoration raises and catalog retirement raises. Ensure final evidence is still saved and errors are not masked.
• Retain the fixture gates: each missing integration/fault/server/Redis/container-name/container-ID variable must skip injection, and a wrong container ID must prevent mutation. This round's six independent missing-variable checks made no Docker/API calls.
• Keep the successful path's proof order: retain metadata/private file hashes/runtime process/netns/cgroup/ADX held resources during failed deletion; restore runc; issue no further public deletion while observing automatic physical cleanup and resource release; only afterward explicitly retire the Failed catalog record. The simulated normal path preserves this ordering, but is not live cleanup acceptance.

Final dependency and merge order

The gitlink still pins sandboxd b47728a9bf690d7dabb431e7795af1b95254f85d, which I fetched successfully from the .gitmodules canonical URL. Its tracked tree equals the author-reported image source 794318afbde0299579cb77d5c87e1d03998c2f0b (96f07231890ff80a8d934e7ad475afceae83543e). However, the corrected sandboxd #74 HEAD 701887a61433a1b9237ef1675ee4ab64262168b5 has tree ceb0e3ec035fe7db3922626db405a1a10f39d5b5: there are real classification-code and Client/Handler regression changes in four files. The current #99 dependency therefore does not yet include the now-verified classification fix; this is more than a SHA traceability difference.

The #74 follow-up review passed independent local format/vet/full unit/runc-shim-server race checks and verified real-runc diagnostic probes; its current CI has all 14 checks successful. I now recommend Approve for #74. Please merge it first, then pin its final squash commit in #99 or verify actual tracked-tree equivalence of the chosen dependency. Fix the two recovery gaps, verify final test/recovery-file fingerprints and updated-head CI, and rerun the corrected opt-in native-runc fault case on a verified exclusive deployment. No duplicate repair PR is needed from our side.

The ADX source pin and actual platform/Execd archive inputs remain consistent; the previously downloaded exact official archive bytes were rehashed this round. No new complete AKernel image build or live fault E2E was performed. The historical author-reported Linux/native-runc campaign remains author evidence for the earlier source tree, not this round's live acceptance. Default Standalone CI explicitly builds with runc disabled and does not exercise this full-level opt-in group.

AKernel #96 continues tracking remaining cleanup semantics, integration and real-workload/guest acceptance. Cold OCI import, real cancellation cleanup, AKernel KVM/QEMU/macOS guest acceptance were not run in this round. Delete ACK, logical Deleted, empty inventory and completed physical release are separate evidence levels; neither this PR's green CI nor final explicit catalog deletion closes that acceptance scope.

[rcyw]

@WenYuLuo Dependency update: sandboxd #74 has now been squash-merged as d5886b42ceff89bf0bf9baf7f73f5c534f689dee. I verified through the canonical repository's Git Data API that its tracked tree, ceb0e3ec035fe7db3922626db405a1a10f39d5b5, exactly matches the reviewed #74 HEAD 701887a61433a1b9237ef1675ee4ab64262168b5.

#99 is still at cb90d6f5ac059e4927da5eadd7345ede4871926f, with src/sandboxd pinned to the older b47728a9bf690d7dabb431e7795af1b95254f85d. Please update it to the final squash commit above and address the two P2 recovery gaps and evidence-preservation regressions in the follow-up review. Then verify the final dependency and test versions, rerun CI on the updated HEAD, and run the corrected opt-in native-runc fault case on a verified exclusive fixture. The current green CI is for the existing HEAD and does not validate those pending changes.

The #74 dependency is now merged; the remaining #99 review requirements are unchanged. This is an integration-status update only, not a formal approval or a new runtime acceptance claim. AKernel #96 continues tracking remaining cleanup semantics, integration and real-workload/guest acceptance; default CI does not replace the opt-in fault case, cold-import/cancellation cleanup or KVM/QEMU/macOS guest validation.

[WenYuLuo]

@rcyw Updated in fe2ae12, with the final merged sandboxd dependency pinned by 579646f. This addresses the fault scope and dependency requests in the follow-up review.

Fault scope and cleanup

The injector now fails exactly one selected runc delete --force invocation. A five-second token matches the exact physical sandbox arguments and the original sandboxd PID/start-time/boot identity, and is atomically consumed before returning the injected error. Unrelated commands, subsequent deletes and restarted daemons execute real runc normally.

The fixture installs a ready, default-pass launcher atomically and preserves an unchanged copy of native runc. Both possible installation outcomes leave a working executable path, so the test does not depend on its child process moving a missing executable back. The runner rejects this group with less than 120 seconds remaining; the child checks at least 100 seconds before arming. Docker mutations use the verified full container ID. Disarming and catalog-cleanup errors are collected before the final JSON evidence write.

Dependency

src/sandboxd now points to official-main squash commit d5886b42ceff89bf0bf9baf7f73f5c534f689dee, after sandboxd #74 merged. Canonical submodule initialization/checkout succeeded. Its tracked tree ceb0e3ec035fe7db3922626db405a1a10f39d5b5 exactly matches tested #74 head 701887a61433a1b9237ef1675ee4ab64262168b5; the classification fixes are included.

Regression results

• Real subprocess regressions cover exact command/ID matching, concurrent single consumption, unrelated commands, expiry, daemon replacement and a controller killed after injection. The maintained runner is also checked with a two-second budget and does not start the fault group. SDK checks pass 298 unit tests, 35 benchmark-driver tests, Ruff, mypy (22 source files) and deployment syntax checks.
• Corrected opt-in Linux AMD64/native-runc E2E passed 1 test, zero skips on a fresh exclusive fixture with real Redis, ADX API Server/Platform, sandboxd and Execd. Events recorded exactly one injected failure followed by one delegated real delete. At that following delete, metadata, private-file hash, namespace/cgroup, Failed/held ownership and assignment were retained.
• Automatic retry completed physical cleanup in 0.3998 seconds. The single public DELETE returned 200 after internal recovery; no additional public DELETE was issued before cleanup verification. Final catalog retirement was recorded separately. Official Redis audit was cluster-clean; independent physical inspection found no runtime/metadata/storage/netns/mount/process residuals. The exact test fixture was removed and the original service preserved.
• The live fixture used verified image sha256:e70dad071ee239041c61bf863a21e8ca05780f64624d73e2b303973688ac1d4d: an existing verified native-runc image with only sandboxd rebuilt from exact source 701887a. Sandboxd and unchanged ADX #161/Execd hashes were checked. The final gitlink-only update to d5886b4 has the identical source tree.
• Final-head CI passes all 10 checks on 579646f, with publishing skipped. Sandboxd #74 CI passes all 14 checks at attempt 2.

Tested SHA-256 fingerprints match the final Python sources:

• test_runc_delete_retry.py: 2268dc38b3ff9aee063d9105fefecc245e90079ef63695aac53f80deb3b9be5b
• runc_delete_fault.py: 03ec077e2afb8f3d738b70ee6d3728d4c9920596d7f48af7b1d6f79efe49da0c

#96 remains open for its separate cleanup-timing, original-workload and guest/macOS acceptance scope. This report distinguishes the actual opt-in native-runc run from default CI; it does not claim fresh cold-import/cancellation or KVM/QEMU/macOS guest acceptance.