amadan
carlos / platform Public
Verdict: REJECT
Reviewed commit 087e3380. Seven findings follow.
Findings
- P1 — A failed refresh lets a held instance execute a stale build. internal/host/buildstate.go:641
A recovered instance can have cached last_executed=A and live A while its durable build state names B. If the wake’s refresh fails transiently, it logs the error and selects A from the old copy. The subsequent pre-exec GET can succeed, but beforeExec:488 blindly merges A into that freshly read state. A then runs against B’s data.
This violates §4.3’s stale-link refusal. The later successful write does not validate the earlier selection.
Smallest fix: refuse held wakes when their required refresh fails. Revalidate the held target against the state read under the lease before recording execution.
- P1 — Settings protect recovery copies from deletion, but not replacement. internal/host/versionsweep.go:82, internal/release/pins.go:124
The new settings references reach the deletion sweep only. Staging’s BoxPins still reads just Version, PrevVersion, and GoodVersion.
Consider a sleeper that last executed V1, then received cold installs V2 and V3. With no GoodVersion or active generation, only its build state names V1. Another instance adopting a legitimately promoted reuse of V1 causes stageFrom:1994 to consider the old copy unreferenced and replace both executable and manifest sibling. Later retained recovery validates the replacement bytes; the original recovery copy is gone.
This breaks §4.1’s retained identity and the plan index’s explicit requirement to include settings references in the restaging pin set.
Smallest fix: include pins, LastExecuted, and resolved targets in BoxPins, qualified by account/app. Refuse replacement when those references cannot be read completely.
- P1 — A graceful move can reopen a completed-failed request. internal/release/adopt.go:2209, internal/host/movescut.go:986
A sleeping manual instance has last_executed=A. Root resolves request R to an already-failed T and records R as completed-failed in its local consumed file. Nothing executes, so nothing mirrors that completion into the build state. A graceful move seals the unchanged bucket object. On the destination, R is above the watermark and resolves again, potentially installing a newer head without another “Update now.”
This violates §4.3’s completed-request rule and §4.3a’s preservation of request state.
Smallest fix: merge local request metadata into the durable state under the lease before transfer/sealing, preserving last_executed. Cover legacy moves and return lanes too; successful completion must remain exclusive to pre-exec writes.
- P1 — A major hold is bypassed until root writes a matching reason. internal/host/buildstate.go:587
heldTarget recognizes a major hold through root’s consumed reason. An absent file or stale ReasonAdopt yields Held=false.
For example, a sleeper last ran B from major N but has a cold-installed A from major N+1. The member enables major N. Before root corrects the installation and writes its reason, a flag-off wake reads the new setting and fresh build state but still runs A. Target given the same current settings and head would choose B.
This violates §4.3 rule 3 and the required agreement between root and wake selection.
Smallest fix: determine current major eligibility in the wake decision, or hold and request root until a decision for the current settings/head is available. Missing or stale root output cannot establish that the instance is unheld.
- P1 — A failed installed build can run when no build-state object exists. internal/host/buildstate.go:597
For a legacy manual instance with Known=true, no state object, installed T, and root’s ReasonFailed/Detail=T, the final branch returns Held=false. With no planner, the wake executes T.
Root correctly applies failure precedence to the installed-build fallback at updatetarget.go:156. The wake never checks that fallback through rootFailed.
This violates §4.3’s failure precedence and §6, including the explicitly supported legacy-state case.
Smallest fix: select and failure-check the installed build when the state has no last_executed; return an explicit failed held target instead of falling through.
- P1 — The local sweep proceeds with an explicitly incomplete recovery set. internal/host/versionsweep.go:149
settingsPins ignores ReadErr when Known remains true. After a failed refresh, an old cached state naming A therefore permits deletion of other staged builds. If the recovered durable state names B, and no live link, version column, or generation names B, the sweep can delete its only recovery copy.
Keeping A does not make the protected set complete. This violates §4.4’s incomplete-set rule and the required preservation of local recovery copies.
Smallest fix: refuse sweeping when a relevant build-state copy has ReadErr, while retaining its previously known references. Add a test containing both cached A and an otherwise unreferenced recovery B.
- P2 — Verified local recovery unnecessarily depends on remote release objects. internal/release/adopt.go:2252
stageAuthorized tries the retained copy only after ErrNoPromotionRecord. An existing promotion record whose manifest GET fails—or a failed record GET—therefore blocks recovery despite a valid local executable and sibling.
A held instance with another live version cannot get its retained target installed. With a planner, its wake waits for root and times out. §4.1 permits recovery from the box’s verified staged copy independently of remote availability.
Smallest fix: accept verified retained evidence independently; require remote digest-bound evidence whenever downloading or replacing bytes.
Rulings
- R-SPEC-43A: disagree for completed-failed requests. The lost-request exception concerns a lost resolution before execution. It does not permit a graceful move to discard a completed outcome and reopen the request.
- R-INV1: disagree with excluding departed-box failed completions from the completion invariant. That makes finding 3 pass the gate. Lost resolutions and completed requests need distinct expectations.
- The Task 7 note claiming failed refreshes are effectively fail-closed is also incorrect: the refresh can fail while the subsequent pre-exec GET and PUT succeed, as in finding 1.
The simulation exercises the production decision and merge functions, but does not faithfully cover wake enforcement. Its wake:946 runs the live build directly, bypassing heldTarget and wakeCheck; its consistency check:993 excludes stale known copies that production preserves. The reported 3,000-seed result therefore does not establish these guarantees.
Tests could not start because the read-only sandbox refused Go’s build directory. Findings are based on verified code paths; no files were changed.