carlos / platform Public

REJECT

Read-only review at 1a8a8037; no files changed or tests run. “Plan” below means the implementation plan.

1. High — Tasks 1–2: the protocol cannot establish its execution claims.

Plan:114–115 and :136 accept R followed by EOF as successful exec. A shim killed between writing R and execve produces exactly that result. Conversely, a deadline can race successful exec: the tenant starts and forks, but the parent observes a timeout, kills only the leader, and reports a placement failure.

This breaks an existing safety assumption: internal/activator/backend_exec.go:1037 treats a start error as spawned=false; :1072 then skips descendant proof, and :1106 can discharge the seed obligation and delete its database. Flat fallback can also run the tenant twice.

Smallest plan fix: distinguish proven pre-exec failure from uncertain execution. Require an execution-observation mechanism that satisfies the owner’s pre-exec classification rule. Never retry an uncertain launch or discharge its obligations without descendant termination proof. Add deterministic tests immediately before exec and after exec but before parent acknowledgment.

1. High — Task 1: the final ownership check moves before substantial launch preparation.

Parent step 5 asks before before starting the shim; placement, decoding and the eventual tenant exec happen afterward, potentially seconds later. A fence, lost lease or superseded attempt during that interval does not prevent tenant execution.

Current internal/activator/genbackend.go:69 explicitly requires the check after preparation, immediately before execution; backend_exec.go:2156 supplies that gate.

Smallest plan fix: use a placement-ready handshake followed by the parent’s final authorization and an explicit exec release. Specify cancellation and lock ordering: no stop may hold a lock needed by that final gate while waiting for the registered placement. Add a held-shim test that revokes authorization before release.

1. Medium — Task 1: passing SysProcAttr through does not preserve all direct-exec semantics.

A Go runtime executes between the two execs. It installs signal handlers and changes signal masks before main: the installed toolchain’s runtime/signal_unix.go:155 preserves inherited ignored dispositions selectively, and :1354 changes masks. syscall/exec_unix.go:284 does not restore them before exec. For example, an ignored SIGTERM can become the tenant’s default disposition. Linux preserves the executing thread’s mask across exec. Signal documentation

The current preservation contract explicitly includes Pdeathsig and Setpgid (internal/cgroup/cgroup_linux.go:18).

Smallest plan fix: specify signal-state restoration on the executing OS thread, and test non-default masks/dispositions, Pdeathsig, process groups, capabilities and rlimits against a direct start. Comparing two freshly initialized Go children can hide inherited-state differences.

1. Medium — Tasks 1–2: Ruling 6 misses population readers and needs an explicit synchronization boundary.

Task 2 updates Cleanup.Populated, Members, and the direct-parent count. However, internal/cgroup/gen.go:213 separately calls parentPopulated; an in-flight descendant still looks empty there, permitting parent-limit initialization at :229.

Also, KillAndEmpty calls private populationLocked and membersLocked directly (internal/cgroup/cleanup_linux.go:334, :338). Updating only the public methods does not cover those reads. Waiting for zero once is insufficient if another placement can register afterward.

Smallest plan fix: put registry awareness in the shared population/member primitives, include descendant placements in ensureParent, and document the existing spawn-exclusion barrier required across an emptiness proof. Test placement registration during proof, not only a count established beforehand.

1. Medium — Task 1: failure cleanup is neither fully specified nor bounded.

Parent steps 5–8 do not specify closing statusR on success or both remaining pipe ends on every early return. More seriously, cmd.Wait() is not necessarily bounded by killing the process. Go’s os/exec/exec.go:979 waits indefinitely for copying goroutines with zero WaitDelay; even its timeout arm waits for those goroutines at :1001. An arbitrary blocking stdin reader or output writer can therefore keep the placement registered forever.

Production already recognizes this distinction through teeStd and seed drain handling (internal/activator/backend_exec.go:375, :1034).

Smallest plan fix: enumerate descriptor ownership and deferred closes for every return; define one absolute handshake deadline; and specify how failed-start reaping avoids unbounded caller I/O. Preserve command errors such as failed cmd.Dir setup instead of classifying every initial Start error as placement failure.

1. Medium — Task 4: the positive control does not prove the advertised hardening.

ENOSYS inside the unit also occurs when clone3 is unavailable outside it. Both proposed checks, and the old-clone3 mutation, can pass that condition without proving this unit introduced the restriction.

Furthermore, accepted systemd properties are not proof of enforcement. Systemd 257 documents unavailable sandbox functionality in user services, particularly filesystem namespaces. Systemd 257 documentation

The omitted identity/capability settings and filesystem protections matter to executable visibility and cgroup access (internal/host/units/carlos-edge.service:28, :32, :35).

Smallest plan fix: require an unrestricted delegated positive control, fail an explicitly requested inner test on delegation skips, and verify effective restrictions inside the unit. Label unavailable protections as untested even when systemd-run accepts them. Keep their real-unit checks deferred as authorized.

1. Medium — Tasks 1–2: the proposed fake-placement seam cannot pass existing activator tests.

Required starts check shimAvailable() before invoking placeStart (Plan:457). FakePlacementForTest changes only placeStart (:461). But the activator’s existing TestMain has no shim hook (internal/activator/activator_test.go:5894).

Thus TestSpawnInCgroupAtPutsAGenerationUnderItsParent, which calls the helper at internal/activator/containment_test.go:113, is refused before reaching its fake.

Smallest plan fix: make the test helper satisfy the availability check within its restored test scope, or install the real hook in every applicable test binary. Do not merely mark an unhooked executable available: that defeats the recursion guard.

1. Medium — Tasks 1, 3 and 5: several prescribed tests cannot prove their assertions.

Plan:326 requires successfully executing an environment entry containing a 1 MiB value. Linux limits each argument/environment string to 32 pages; on a 4 KiB-page machine this must fail with E2BIG. Exec documentation Keep that value in the encoding test, but test equivalent rejection at execution.

Plan:140 makes the probe print a newline; :542 requires exactly the pathname without one. Specify expected+"\n".

genKernelManager skips without CARLOS_GEN_KERNEL=1 and on delegation failures (internal/cgroup/gen_kernel_linux_test.go:29, :45). A reported outer PASS must not substitute for executed assertions.

Task 5’s fixed 10 ms sleep does not establish an in-flight request. The actual launch arguments are correctly identified (internal/activator/backend_exec.go:2147; internal/vet/vet.go:135).

Smallest plan fix: correct these expectations, require explicit execution receipts, and synchronize the drain test with handler entry.

1. Low — Task 6: another successful generation-stop path retains failed-unit state.

abandonGen successfully calls killGen and removes generation files without passing through stopGen (internal/instancectl/gen.go:189, :197). A failed start cleaned up there can still remain in systemctl --failed.

Smallest plan fix: share the best-effort reset operation between successful explicit stops and successful abandonment cleanup. Keep unproven stops unchanged. The two missing logf initializers are correctly identified at strict_kernel_linux_test.go:160 and :190.

1. Low — Tasks 3 and 7: the older-console consequence is more than different wording.

Ruling 7 says older consoles display the unknown-code phrase. They also classify unknown codes as non-retryable: internal/rolloutwords/words.go:109 rejects them, and internal/console/instancepage.go:43 uses that result for retry eligibility and hold presentation.

Smallest plan fix: state that an older console cannot offer/sign host retries for this newly declined state until updated. Reusing unavailableDeclined itself introduces no new member-facing phrase.

1. Low — Verification instructions: cache, scratch and commit commands contradict the required workflow.

Plan:26 and repeated commands override the default Go cache and use shared /tmp/k paths, including recursive deletion at :699. The supplied machine rules require the default cache and task-owned disk scratch.

Task 1’s explicit git add at :437 also omits its new inflight.go, inflight_linux_test.go, and changed cgroup.go.

Smallest plan fix: use the default cache, uniquely owned disk scratch with scoped cleanup, and stage every Task 1 file before its commit.

Ruling dispositions: 2—agree with conditions: fallback only after proven non-execution. 3—agree: strict placement remains unchanged (strict_linux.go:346). 4—agree with conditions: repair test-hook integration. 5—agree: empty shim environment and byte-preserving transport are appropriate. 6—agree with conditions: findings 2, 4 and 5. 7—agree with conditions: correct mixed-version consequences. 8—agree: box-wide decline is consistent with swapArming (host/rollout.go:150). 9—disagree as written: ten seconds does not bound cleanup or prove non-execution. 10—agree: reset failure is cosmetic after successful stop. 11—agree: a tested probe closes the argv defect. 12—agree with conditions: retain focused protocol/concurrency tests and reconsider the sweep if fixes touch modeled behavior.

All enumerated ordinary launch paths converge on the two Manager entry points (cgroup.go:383; gen.go:272), including generation/role selection (activator/containment.go:91). Successful exec preserves the leader PID; OOM adjustment and leader signaling remain compatible (cgroup.go:652; backend_exec.go:3144). I found no additional required-path PID-move escape, demonstrated new privilege grant, or inherent cross-compile break. The blocking defects concern execution classification, authorization timing, and proof completeness.