carlos / platform Public

Download this file

1	REJECT
2
3	Read-only review at `1a8a8037`; no files changed or tests run. “Plan” below means [the implementation plan](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-flag-readiness-3/docs/superpowers/plans/2026-10-13-zero-downtime-flag-readiness-3.md).
4
5	1. **High — Tasks 1–2: the protocol cannot establish its execution claims.**
6
7	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.
8
9	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.
10
11	**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.
12
13	2. **High — Task 1: the final ownership check moves before substantial launch preparation.**
14
15	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.
16
17	Current `internal/activator/genbackend.go:69` explicitly requires the check after preparation, immediately before execution; `backend_exec.go:2156` supplies that gate.
18
19	**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.
20
21	3. **Medium — Task 1: passing `SysProcAttr` through does not preserve all direct-exec semantics.**
22
23	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](https://www.man7.org/linux/man-pages/man7/signal.7.html)
24
25	The current preservation contract explicitly includes `Pdeathsig` and `Setpgid` (`internal/cgroup/cgroup_linux.go:18`).
26
27	**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.
28
29	4. **Medium — Tasks 1–2: Ruling 6 misses population readers and needs an explicit synchronization boundary.**
30
31	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.
32
33	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.
34
35	**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.
36
37	5. **Medium — Task 1: failure cleanup is neither fully specified nor bounded.**
38
39	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.
40
41	Production already recognizes this distinction through `teeStd` and seed drain handling (`internal/activator/backend_exec.go:375`, :1034).
42
43	**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.
44
45	6. **Medium — Task 4: the positive control does not prove the advertised hardening.**
46
47	`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.
48
49	Furthermore, accepted systemd properties are not proof of enforcement. Systemd 257 documents unavailable sandbox functionality in user services, particularly filesystem namespaces. [Systemd 257 documentation](https://raw.githubusercontent.com/systemd/systemd/v257/man/systemd.exec.xml)
50
51	The omitted identity/capability settings and filesystem protections matter to executable visibility and cgroup access (`internal/host/units/carlos-edge.service:28`, :32, :35).
52
53	**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.
54
55	7. **Medium — Tasks 1–2: the proposed fake-placement seam cannot pass existing activator tests.**
56
57	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`).
58
59	Thus `TestSpawnInCgroupAtPutsAGenerationUnderItsParent`, which calls the helper at `internal/activator/containment_test.go:113`, is refused before reaching its fake.
60
61	**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.
62
63	8. **Medium — Tasks 1, 3 and 5: several prescribed tests cannot prove their assertions.**
64
65	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](https://www.man7.org/linux/man-pages/man2/execve.2.html) Keep that value in the encoding test, but test equivalent rejection at execution.
66
67	Plan:140 makes the probe print a newline; :542 requires exactly the pathname without one. Specify `expected+"\n"`.
68
69	`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.
70
71	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`).
72
73	**Smallest plan fix:** correct these expectations, require explicit execution receipts, and synchronize the drain test with handler entry.
74
75	9. **Low — Task 6: another successful generation-stop path retains failed-unit state.**
76
77	`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`.
78
79	**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.
80
81	10. **Low — Tasks 3 and 7: the older-console consequence is more than different wording.**
82
83	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.
84
85	**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.
86
87	11. **Low — Verification instructions: cache, scratch and commit commands contradict the required workflow.**
88
89	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.
90
91	Task 1’s explicit `git add` at :437 also omits its new `inflight.go`, `inflight_linux_test.go`, and changed `cgroup.go`.
92
93	**Smallest plan fix:** use the default cache, uniquely owned disk scratch with scoped cleanup, and stage every Task 1 file before its commit.
94
95	**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.
96
97	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.