Repository navigation
regen-round-cost: re-exec once when the seed build replaces the running executable - #13580
gunbai-bot[bot] wants to merge 10 commits into
Conversation
…ng executable; file failure-mode row Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 86b42a14cd6a3d12665dcf28af7ba7ac08d22219. The bounded handoff addresses the stale running-image symptom, but the successful re-exec path loses the very seed-build cost this instrument promises to report. No full compiler self-hosting redesign or new CI lane is requested.
P1 — Re-exec erases paid work and then emits a complete-round cost receipt
run_regen_round_cost arms the trace ledger, measures round.seed_build, and retains its CargoBuildObservation in seed_build. On replacement it execs a new image without transferring either the ledger or that observation. The restarted invocation arms a new ledger and replaces the observation with the no-op rebuild. At the terminal it supplies only the restarted marks and seed_build.compiled_crates to render_round_cost_receipt.
This contradicts the unchanged gunbc.regen_round_cost contract: one round is build seed -> emit -> install -> rebuild -> readback; the SeedBuild phase is part of that round, and unreadable work is not zero. regen_round_total sums only the marks it receives. The PR's own reported successful run makes the discrepancy concrete: the first seed build takes about three minutes, while the persisted SeedBuild reports the restarted ~96ms check and the earlier compilation is absent. The original crate-build count is lost as well. The old implementation refused instead of publishing a successful receipt for this invocation; automating the old run-twice workaround turns its accounting omission into the instrument's normal successful output.
The PR-body caveat is honest prose but does not repair the machine-consumed receipt or its total. Transfer the first build observation and its phase measurements across the handoff, bound to this invocation/source/build artifact, and compose them exactly once with the resumed round. A separate bootstrap/controller that retains the ledger while running the built seed is also a sound solution. If continuity cannot be established, the receipt must explicitly mark the affected measurement/total unavailable or incomplete rather than rendering the warm rebuild as the whole SeedBuild. Do not call a deliberately narrowed post-bootstrap measurement the unchanged whole-round instrument.
Add a bounded discriminator around the actual production handoff/receipt composition: a nonzero pre-handoff build measurement and compiled-crate count must survive; neither may disappear or be counted twice. This can use supplied build observations or a tiny controlled child, not a live-corpus compiler rebuild in the unit lane. Keep the normal no-replacement control and a control for the second-replacement refusal.
P2 — Correct the refusal and ceiling claims in the committed RFM
The new branches are Err(format!(...)) through Result<RegenRoundCostOutcome, String>. They are loud, bounded errors, but there is no distinct typed ReexecFailed/SeedReplacedAgain cause introduced at this boundary. The RFM's 'correct and typed' characterization is stronger than this implementation. Either carry the actual structured cause through a matching boundary or explicitly record the legacy String refusal rather than crediting a type that is not there.
Nor does replacement number two prove 'the build is not idempotent over an unchanged tree'. The first invocation's source/tree identity is not handed over and compared: the marker contains a digest but is only checked for presence, and the restarted invocation recomputes tree/dirty state. The observed fact is that the executable path changed again. Keep refusing, but avoid assigning non-idempotence/unchanged inputs as an established cause without that join.
Record re-exec as bounded runtime mitigation (rung 1), not the construction that establishes ceiling 3. The future capability should be an emitter bound to the actual admitted build artifact, with a complete or explicitly unavailable cost receipt. Merely moving the step to .dag or declaring a different output pathname does not establish that relation. The row already correctly identifies output-path/running-image coupling as the earlier boundary; preserve it.
Answer to the root-cause question
Replacing an executable pathname and changing the code already executing are different operations. An explicit exec/handoff is legitimate when a bootstrap intentionally transitions to a newly built seed; merely building elsewhere and then continuing to emit in the old process would be equally wrong. This patch therefore addresses a real transition requirement, not an arbitrary retry of a semantic failure, and its one-reexec limit is preferable to an unbounded loop.
My preferred root shape is a bootstrap/controller that builds a separately identified artifact, verifies/selects that artifact, and executes it while retaining the round's observations. A stable artifact path/content identity also avoids mistaking 'what currently occupies my old pathname' for the built seed. The current current_exe_digest explicitly hashes the on-disk pathname, not the mapped running image; its name must not be read as proof of running-image provenance. Rust's current_exe documentation likewise cautions that it returns a path, with platform-specific behavior and replacement risks. A bounded re-exec can remain the immediate realization if the identity/accounting handoff is honest; there is no requirement to finish the entire .dag build modeling migration in this PR.
Verification and coverage
Commit-filtered workflow 37743010470 completed successfully. Seed, generated, floor, emit-build and witnesses succeeded; rust-unit-tests was skipped. The generated job did execute all-target lint and its one-emission mirror check. These results do not execute --regen-round-cost or test the new re-exec branch.
The forced-relink RED, second build ~96ms and full 164/164 green round under memory.max remain author-reported dispatch-time evidence. I did not retrieve/replay their raw run or independently rerun this flag, and the two-file diff adds no enrolled regression control for the handoff. Green general CI must not be credited as that missing execution.
Inspected the complete two-file diff, pinned run_regen_round_cost before/after handoff and receipt construction, current_exe helpers, the unchanged .dag measurement contract, RFM, and exact-head CI metadata. No local compiler build, re-exec, mutation, merge or enqueue. Fix the accounting continuity and the durable overclaims; no new hardware operation, full-corpus timing lane or broad v1 improvement program is requested.
…c, typed handoff refusals, controls Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head aba309cc5e91837ca992acf263dff17b99c20693, re-reviewing 5454194241. The ordinary single-row resumed path now retains the paid seed build; that specific omission is repaired. Two remaining P2 boundary issues prevent the stronger exactly-once/continuity claim requested for this review. No new full-round CI lane or complete bootstrap redesign is requested.
Credited repair and handoff-arm trace
SeedHandoff carries the first image's measured wall time, optional process-tree CPU and compiled-crate count. On first replacement, the process execs instead of falling through to emit or render. On a successful resume, carried_cost reaches the one terminal compose_seed_build_cost call before the production renderer. The newly measured warm-build cost is not also passed as a second carry. With the expected one round.seed_build row, the result is first-build + resumed-build time and crates, once. With no handoff, composition is identity. An unreadable CPU in either input remains unreadable.
Malformed handoff decoding, a changed carried commit, a replacement during the resumed build, or failed exec returns an error rather than a newly successful cost receipt. This is not a claim that those failures persist all paid costs in a separate failure receipt; they currently return through the legacy String error boundary.
SeedHandoffRefusal is a real enum, used by the decision/decoder tests, and runtime mitigation is the correct standing. Rendering it into the existing String-returning CLI boundary does not invalidate the local typed decision; it is not an end-to-end typed CLI result either.
P2 — Composition silently drops or multiplies the carry unless the ledger has exactly one matching row
src/v1/stage0/src/required_regen_host.rs::compose_seed_build_cost adds the carried duration to EVERY row whose label equals round.seed_build, then adds the carried crate count once. It neither requires a matching row nor refuses duplicates. For N matching rows, the duration contribution is N times the carried duration, while the crate contribution is always once.
Concrete accepted inputs, using the test's 180000ms carry:
- No matching row: all 180000ms disappear, while the carried crate count remains in the header.
- Two matching rows at 96ms and 4ms: the seed-build rows total 360100ms, not 180100ms, while crates are still added once.
The production renderer maps the resulting list directly to TraceMark values, and gunbc.regen_round_cost::regen_round_total sums them without enforcing this population. It therefore does not repair or refuse either case. The new discriminator supplies precisely one row, so it tests the useful happy case but not omission or multiplicity.
Scope precision: the inspected normal driver calls seed_cargo_build(..., "round.seed_build") once, and its successful begin/done trace ordinarily supplies one row. I have NOT reproduced a missing/duplicate ledger in the author's full dispatch or claimed that the reported green round has either defect. This finding is the unchecked receipt-composition input boundary: a missing or duplicated measurement is accepted as a complete total rather than diagnosed, contrary to the requested all-arm accounting guarantee.
Require exactly one corresponding current-image seed-build measurement before merging, with a located typed refusal on missing/duplicate input, or represent the two image measurements explicitly so the carried measurement is included independently exactly once. Extend the bounded production-composition control with missing-row and duplicate-row cases, alongside the existing positive/no-handoff/unreadable-CPU controls. No compiler rebuild or new test lane is needed for those cases.
P2 — The carried identity does not establish which built artifact/source this cost belongs to
The driver's tree is git_head_sha, implemented as git rev-parse HEAD. The dirty flag is read separately and is not carried or compared. Equal HEAD values do not establish equal worktree inputs across the handoff.
SeedHandoff also stores only exe_before from the sending image. That field is encoded/decoded but is not consulted by decide_seed_handoff; the digest of the newly built executable (exe_after at the sender) is not carried at all. A handoff produced for A -> B at commit T is accepted by decide_seed_handoff(Some(h), "C", "C", "T", ...) as Proceed. The decision establishes only that the receiving pathname did not change during its own build, not that it is the artifact to which the first image handed this cost. A same-HEAD worktree change is likewise outside TreeChangedAcrossHandoff as implemented.
This is a source-derived discriminator of the missing relation, not a reproduced concurrent replacement or an allegation of hostile environment injection. The earlier request was to bind the carried observation to the invocation/source/build artifact, not merely retain an unused executable string beside a commit string.
Carry and validate the expected BUILT artifact identity at the receiving boundary and the actual relevant source/worktree identity, reusing the existing build/source identity authority. Reject a foreign artifact or changed source before accepting the carry into the round. Preserve legitimate dirty-worktree operation rather than replacing this with a blanket clean-tree restriction. Add bounded wrong-artifact and same-commit/different-source controls beside the valid handoff. The full immutable-build-output/.dag migration can remain the future capability.
Finish the committed P2 accuracy correction
The RFM still says the historical refusal 'was correct and typed', although the pre-repair refusal was the legacy String error identified in the previous review. Its ceiling/trigger still names only a distinct output pathname in .dag. Update the same row to distinguish the historical String refusal from the new local enum, and name artifact-bound execution plus complete-or-explicitly-unavailable accounting as the capability. A different pathname alone does not force the emitter to execute that artifact. Current rung 1 remains appropriate. The PR body's older 'non-idempotent build' and 'not run: unit tests' summaries also conflict with its update; align them without adding more receipt history.
Evidence and limitations
Workflow 37758146600 associated with this exact head is green: seed, generated, floor, emit-build and witnesses succeeded; rust-unit-tests was skipped. The generated job executed all-target lint and the one-emission mirror check. These do not execute the flag or the four Rust tests.
I verified that required_regen_host.rs at the author's named Rust revision 8ee4c3f6 and this head has the same Git blob 234553234b39ee12f748697a1a76f5f19309a822. The four-test group is present: the two new handoff/composition controls plus the two retained controls. The new composition control invokes the actual composition and actual .dag renderer, not a copied formatter. The BuildBuddy amd64 command, 4-pass report, and cgroup round with crates=1 and seed_build wall_ms=208817/cpu_ms=354770 remain author-run evidence; I did not retrieve their raw transcript or replay them.
Reviewed the exact source, relevant trace-ledger producer, .dag receipt/total consumer, prior review, RFM and current CI metadata. No local compiler build, new mutation, re-exec experiment, merge or enqueue. Preserve the repaired normal-path accounting and typed decisions; close the two narrow input/identity boundaries rather than opening a wider v1 improvement programme.
…d source identity, typed refusals, controls; RFM wording
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 8d732ad2157d3703904e5d6b1fcc68be220c6c38, re-reviewing 5456694753 and the preceding accounting finding. The missing/duplicate-ledger-row repair is accepted. One P2 remains in the newly extended source-identity construction.
Accepted: the cost composition and supplied handoff decisions
compose_seed_build_cost now counts round.seed_build rows before mutation and returns SeedBuildMeasurementCount { found } unless a carry has exactly one destination. Its zero-row and two-row controls assert those exact causes. On the one-row arm it adds the carry's wall/CPU once and the crate count once; unavailable CPU remains unavailable. With no incoming carry the old observation is unchanged. The real driver drains its ledger, calls this composer once, propagates Err before rendering/writing a new successful cost receipt, and passes the resulting crate count and marks to render_round_cost_receipt. There is no second terminal addition in the inspected driver.
decide_seed_handoff now compares the carried source and built_exe before accepting the resumed branch, retains the second-replacement refusal, and never returns another ReExec on an incoming carry. The controls discriminate the supplied wrong-artifact and different-source values. This closes the previous unused-exe-before-field problem at that decision boundary. The one-row control also exercises the real receipt renderer.
P2 — untracked file framing lets different source populations share an identity
compose_source_identity serializes each untracked entry as path + '=' + bytes_digest(content) + '\n'. The preceding observer correctly uses Git's NUL-delimited listing, but then drops that framing. Neither the observer nor composer rejects '=' or newline in the path.
Let D(x) be bytes_digest(x). These two distinct untracked populations produce IDENTICAL bytes before the final digest:
- A:
[("a.dag", x), ("b.dag", y)] - B:
[("a.dag=" + D(x) + "\nb.dag", y)]
Both serialize to a.dag=D(x)\nb.dag=D(y)\n. HEAD and the tracked diff can be identical (all these files are untracked), so the complete source identity is also identical. No collision in the hash function is required. A carry for A can therefore pass the source-equality arm after the population becomes B, when the supplied artifact readings remain the expected built artifact.
I executed a small temporary-Git-repository reproduction of the NUL listing and this delimiter construction: two files versus the single newline-containing filename, same HEAD and tracked diff, identical pre-hash payload. That was an algorithm-independent framing reproducer, not a gunbc build or a run of the repository's Rust tests; its result does not depend on the particular digest algorithm.
Use unambiguous framing of path bytes and content identities (for example length prefixes or a delimiter the admitted path/digest domains cannot contain). Preserve actual path identity or refuse unsupported path encodings rather than silently rewriting them. Add this distinct-population discriminator through the production composer and require SourceChangedAcrossHandoff; retain the ordinary unchanged-source positive. This needs neither a full-round CI job nor a different bootstrap architecture.
Receipt wording
The RFM's SPECIMEN now correctly calls the old refusal Err(String), and its capability trigger is substantially repaired. But DISTINGUISHING FACTS still calls that historical refusal 'typed', and RUNG FOUND still says 'a typed refusal'. Remove those two stale historical descriptions. Current rung 1/runtime mitigation remains appropriate; the new enum does not retroactively type the old implementation. The body also retains historical summaries superseded by its UPDATE sections; do not read them as current implementation claims.
Verification
Exact-head workflow 37781073906 is green: seed, generated, floor, emit-build and witnesses succeeded, including all-target lint and the one-emission mirror check. Rust unit tests were skipped. I inspected the five-control module; two tests pre-existed, and the new source test supplies rows directly to the composer rather than executing Git discovery. The reported remote cgroup five-pass run and older full-round result remain author-run evidence, not transcripts I retrieved or runs I replayed. I did not run the full compiler, a new re-exec round, or a concurrent executable-replacement experiment. Nothing merged or enqueued.
…from the exec'd descriptor, RFM wording
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 9446c5461809477bdbd3a728400480f402a5d8c3, re-reviewing 5459498802. The untracked-path framing and historical-refusal wording findings are closed. The previously accepted cost composition remains intact. One P2 remains in the new executable-identity boundary: an open descriptor pins an inode, not its bytes, and the receiver still identifies the running image by rereading the installed pathname.
Accepted: unambiguous path framing at this head
source_identity now retains Git's NUL-delimited path bytes, uses OsStr::from_bytes for the file read, and passes raw byte vectors to compose_source_identity. No lossy UTF-8 conversion remains on that path.
Each untracked entry is encoded as an eight-byte little-endian path length, exactly that many path bytes, and the content digest. The current bytes_digest encoding is fixed-width (fnv1a64: plus the 16 hexadecimal digits produced by v1_rt::bytes_identity_hash), so the end of each entry is uniquely determined. Newlines, equals signs and invalid UTF-8 in an admitted Unix filename cannot become entry boundaries. The framing argument is at path/content-digest grain, not a collision-free guarantee for the underlying hash. The two-population control runs the production composer and requires SourceChangedAcrossHandoff; the identical-source control still proceeds. This closes my delimiter-alias counterexample.
The RFM now consistently calls the historical refusal Err(String), keeps current standing at runtime mitigation/rung 1, and retains the capability-based ceiling trigger. The exactly-one-row count, exact typed zero/two-row refusals, one terminal composition call, crate accounting and unavailable-CPU propagation are not weakened by this revision.
P2 — Hashing an ordinary read-only fd immediately before exec does not close the content-change window
open_exec_target does File::open plus read_to_end. It neither seals the file nor establishes an authority that excludes writes to that inode. Executing /proc/self/fd/N prevents a later pathname replacement from redirecting that descriptor, but another writer can change the SAME inode after read_to_end and close its write fd before exec. The exec then loads the changed bytes. Linux man-pages fexecve(3), NOTES, explicitly distinguishes pathname replacement protection from this checksum-to-exec contents race.
The new exec_target_digest_is_of_the_opened_inode_not_the_path is useful for its stated unlink/replacement case, but it only rereads the descriptor. It does not execute it, mutate its inode in place, or exercise the receiving identity check.
The receiver does not repair the gap: current_exe_digest still calls current_exe_on_disk, strips (deleted) from current_exe's pathname, and reopens that PATH. run_regen_round_cost supplies those installed-path readings as exe_before/exe_after to decide_seed_handoff. The running field in WrongArtifactAcrossHandoff consequently does not necessarily describe the running image.
A concrete interleaving is:
- Open inode I containing built image B and hash B; carry.built_exe names B.
- Before exec, rewrite I in place with image C and close the writer. Then replace the installed pathname with a different inode containing B.
- Exec
/proc/self/fd/N: it executes C from I. The new process's current_exe path is the unlinked old name; the PR's suffix stripping/reopen reads the installed B. - With the same source identity and a no-op build on that installed B, the receiving predicate sees carried=B, exe_before=B, exe_after=B and returns Proceed, although the running image is C.
I executed a standalone Linux ELF reproducer of these file operations and the receiving-side comparisons. Two tiny C receiver images used the same comparison logic and FNV-1a digest spelling, with a Python launcher doing open/hash/interleaving/exec through /proc/self/fd/N. Results:
- unchanged: running B, installed B, Proceed;
- pathname replacement after open: running B, installed C, WrongArtifactAcrossHandoff (the descriptor correctly pins B);
- same-inode change after hashing: running C, installed C, WrongArtifactAcrossHandoff, but C has already executed;
- same-inode change followed by restoring B at the pathname: running C, installed B, Proceed.
This was NOT a gunbc build, not the repository's Rust tests, and not an observed bad result in the author's successful full round. It establishes the OS behavior and the inadequacy of this receiving predicate under the specifically requested hash-to-exec window. It requires an allowed writer to the build artifact; this head establishes no exclusion of such a writer.
Bounded repair
Make the bytes immutable for the verification-to-exec interval, or establish an enforced write-exclusion mechanism. A sealed executable snapshot is one possible implementation, not a requirement to introduce a new bootstrap program. Merely hashing again closer to exec moves the race. If a snapshot is used, keep its running-image identity separate from the install/build pathname.
Validate the receiving image against the actual running executable, not a stripped pathname reopened on disk. Preserve the distinction between installed-output observation, the admitted exec target and running-image observation. The new carry.built_exe = target_digest also replaces the earlier build-output observation with whatever the later open found; do not silently re-admit a changed target where a comparison to the expected artifact is required.
Extend the small exec-boundary control so mutation after verification either cannot alter what executes or causes a refusal, and a running C cannot be accepted under B merely because B occupies the pathname. Retain a successful same-artifact exec and the existing pathname-replacement control. No new full-round CI lane or broad compiler rewrite is requested.
Verification
Exact-head PR workflow 37810545548 succeeded: seed, generated, floor, emit-build and witnesses passed; rust-unit-tests was skipped. The generated job includes all-target lint and the one-emission mirror check. The PR body reports seven remote unit tests and a forced-relink GREEN_EXIT=0 round at this head, including seed_build_compiled_crates=1 and wall_ms=216005. I inspected the source of the new controls but did not retrieve the raw remote transcripts or replay that round. The PR correctly states that no required lane executes --regen-round-cost. Those happy-path results do not cover the interleaving above.
Local execution was limited to the standalone OS/identity reproducer; no live infrastructure changes, repository modifications, merge or enqueue. No further framing or accounting change is requested by this review.
…self/exe; modify-after-verify and installed/running mismatch controls
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 6b3e8d6fd5ff41988ac03d0e6b0fd74443576884, re-reviewing 5461063791. The receiver-side installed/running confusion is repaired, and the earlier B/C interleaving now refuses. One P2 from the earlier review remains: the sender overwrites its observed build-output identity with the later-opened target instead of joining them. The normal same-artifact handoff is not over-refused by the new receiver comparison.
Accepted: direct running-image observation and the previous B/C interleaving
running_image_digest opens /proc/self/exe directly and hashes through digest_open_file; it does not readlink, strip (deleted), or reopen the installation pathname. run_regen_round_cost now supplies that observation as exe_before. For an incoming handoff, decide_seed_handoff checks source, carried built_exe versus that running image, and running image versus the installed-output observation after the resumed build, in that order.
For the previous counterexample (hash B, rewrite the opened inode to C, restore B at the installed pathname, execute the retained fd), the receiver now sees carried B / running C / installed B and returns WrongArtifactAcrossHandoff. B on the pathname no longer conceals C in the process. This closes that specific acceptance hole.
There is no false refusal of the ordinary A -> B transition at this predicate: the first invocation without a carry returns ReExec for A/B; the resumed invocation compares B to the carried B and installed B, not to the old A. Byte-identical replacement of the installed inode likewise retains B/B and proceeds. An incoming carry B with running B but installed C is deliberately refused as ExecutableReplacedAfterHandoff; that is not evidence that the B executable is wrong, but that the protocol's installed/running agreement no longer holds. I found no comparison-induced over-refusal on the unchanged-source, same-artifact path.
The RFM now correctly distinguishes after-the-fact detection from prevention. I credit this as runtime consistency mitigation for a receiver executing this checking code, not prevention of C executing, protection against arbitrary replacement code that omits the check, or ceiling-3 artifact admission. This review does not require introducing an immutable-exec mechanism to claim only that bounded mitigation. Note the receiver performs its seed build before the handoff decision; 'after the fact' is material.
P2 — build output and opened target are still collapsed at the sender
decide_seed_handoff(None, running_A, installed_B, source, cost) constructs SeedHandoff { built_exe: B, ... }. But the production ReExec arm then does:
let (target, target_digest) = open_exec_target(&on_disk)?;
let mut carry = carry;
carry.built_exe = target_digest;
No equality check connects target_digest to the already observed build-output B. The original expected identity is lost. This is the remaining part of my previous request not to silently re-admit a later-opened target.
Concrete interleaving: observe B after the paid seed build; replace the installation with C before open_exec_target opens it; open/hash C; overwrite the carry with C; execute C. If the resumed build leaves C installed and the source identity remains equal, the receiver sees carry C / running C / installed C and returns Proceed. The cost still belongs to the preceding B build, but the agreement has been reset to whatever the later open happened to find. Three named observations are not three joined identities.
This is distinct from the after-hash inode rewrite now detected by /proc/self/exe. The latest tests cover that receiving decision and the earlier path-replacement-after-open case; none requires the opened target to equal the prior build-output observation.
Bounded correction
Keep the expected build-output digest in the carry. Compare the opened target's digest against it before execution and return a typed refusal on disagreement; do not assign the observation over the expectation. Continue executing the checked descriptor and checking the actual running image on receipt. A small production admission helper/decision, with build=B/opened=C negative and build=B/opened=B positive, is sufficient. Retain byte-identical inode replacement as an allowed case. No new full-round CI lane, another accounting redesign, or broad bootstrap rewrite is requested.
Local verification and evidence limits
I ran a standalone Linux ELF reproducer with two tiny C receiver images and a Python fd-exec launcher. The receiver directly opens /proc/self/exe and mirrors this head's incoming equality checks with unchanged source and a simulated no-op resumed build. Results:
- unchanged: running B, installed B, carry B -> Proceed;
- byte-identical pathname replacement after opening B: running B, installed B, carry B -> Proceed;
- different pathname replacement after opening B: running B, installed C, carry B -> ExecutableReplacedAfterHandoff;
- previous after-hash inode rewrite, then B restored at pathname: running C, installed B, carry B -> WrongArtifactAcrossHandoff;
- replacement between observing build-output B and opening the target, with the production carry overwrite: running C, installed C, carry C -> Proceed, despite the earlier build-output observation B.
This was not a gunbc build, not execution of the repository's nine Rust tests, and not a replay of the forced-relink round. It verifies OS behavior and the boundary comparison, not the full instrument. The source of the new unit tests is present and inspected. modify_after_verify_and_installed_running_mismatch_refuse supplies the receiver digests after a small file rewrite; it neither execs that file nor actually restores B at its path. running_image_is_read_from_proc_self_exe_not_the_installed_path runs in the unchanged test process. These are useful checks within that scope, not an executing test of every production interleaving.
The exact-head PR workflow 37824671664 passed seed, generated, floor, emit-build and witnesses; rust-unit-tests was skipped. Generated includes all-target lint and the one-emission mirror check. The PR body reports nine remote unit tests and a forced-relink GREEN_EXIT=0 round (seed_build_compiled_crates=1, wall_ms=220306); I did not retrieve their raw transcripts or rerun the instrument. No required lane executes --regen-round-cost, as the PR states.
The compare from 9446c54618 is one commit in the same three files: running-image reader and tests in required_regen_host.rs, plus the RFM and seed-growth wording. The previously accepted length framing, exact-one-row accounting, unavailable-CPU handling, and bounded one-reexec refusal remain credited. Nothing merged or enqueued.
briansrls
left a comment
There was a problem hiding this comment.
APPROVE at exact head 336ecb0d3651233266d752119aaaff281e31917c, re-reviewing REQUEST_CHANGES 5462949909. The remaining carry-overwrite P2 is resolved. No new blocking finding; the previously accepted framing, cost-composition and receiver repairs remain credited.
The build-B/opened-C interleaving now refuses BEFORE exec
decide_seed_handoff(None, old, B, source, cost) still constructs a carry whose built_exe is the post-build installed-output observation B. The ReExec arm opens the target once, obtaining the descriptor and its digest, then calls:
let carry =
admit_exec_target(carry, &target_digest).map_err(|cause| cause.to_string())?;
That statement precedes construction of the /proc/self/fd/N command and its .exec(). admit_exec_target returns OpenedTargetNotBuildOutput { built, opened } when the opened digest differs. The ? propagates that refusal out of run_regen_round_cost; there is no fallback, new carry construction, or exec on that arm. The typed cause is rendered through the existing String CLI error boundary rather than discarded.
Therefore observe build B -> replace installation with C -> open C now produces OpenedTargetNotBuildOutput { built: B, opened: C } before the descriptor is executed. The earlier assignment carry.built_exe = target_digest and its mutable carry are gone. On agreement the helper returns the original carry unchanged, preserving not just B but its source and paid-build observations.
Positive and discriminator are at the requested production boundary
exec_target_admission_compares_with_the_build_output_and_never_overwrites obtains its carry from the real first-image decide_seed_handoff, checks B is the expected artifact, requires the specific OpenedTargetNotBuildOutput variant for opened C, and then admits opened B. It checks equality of the ENTIRE admitted carry with the original and passes that result to the real receiving decision, which must Proceed for source S / running B / installed B.
This is the bounded supplied-digest production pairing requested in the last review, not a duplicate local implementation of the admission. An unconditional-accept/overwrite mutation fails the B/C negative; an unconditional refusal fails the B/B positive. A byte-identical inode replacement is allowed at this content-identity boundary because its digest still equals B; neither this helper nor the receiver imposes inode/path identity equality. The existing descriptor-versus-replaced-path control remains.
The new admission control itself does not physically exec a replaced file or execute the complete driver. I am not describing its supplied B/C strings as a new independent OS interleaving experiment. The inspected production call site establishes that the same admission precedes exec, and the reported full round is the separate driver execution evidence.
Accepted repairs are unchanged
The comparison from 6b3e8d6fd5ff41988ac03d0e6b0fd74443576884 contains exactly two commits and the same three files. Production changes add the refusal variant/renderer and admission helper and replace the carry-overwrite statement with the checked call. The other edits add the control, remove a redundant preliminary test fragment, and update the RFM/seed-growth account. No unrelated production change was found.
The raw-path, length-framed untracked-source encoding is unchanged. So are source continuity checks, direct /proc/self/exe observation, descriptor execution, and the incoming source -> built/running -> running/installed checks. The previous after-hash B/C example therefore still refuses at the receiver; the normal same-source A->B handoff compares B/B/B on receipt and still proceeds.
Cost accounting is unchanged: an incoming carry requires exactly one round.seed_build ledger row; missing/duplicate rows refuse; carried crate count and durations are composed once through the existing terminal composition before rendering; unavailable CPU is not converted into zero. The accepted no-handoff case and second-replacement refusal remain intact. No cost row, budget, or lane was relaxed by this revision.
The committed RFM retains rung 1/runtime mitigation and correctly distinguishes the new pre-exec opened-target check from detection of later inode modification by a receiver running the checking code. This approval does not turn the handoff into immutable execution or an adversarial code-authentication boundary, nor claim prevention of a changed image starting. The named future artifact/admission/accounting capability remains a ceiling trigger. admit_exec_target is added to the seed-growth declaration roster and description.
Verification and evidence limits
Exact-head workflow 37849845538 passed seed, generated, floor, emit-build and witnesses. Generated passed all-target lint and the one-emission mirror check. Rust unit tests were skipped, not passed. Those CI results do not independently establish execution of the ten Rust controls or --regen-round-cost.
The PR body reports ten passing tests in the remote cgroup command and a forced-relink GREEN_EXIT=0 round at this head, with seed_build_compiled_crates=1 and seed-build wall_ms=221671. I inspected the committed controls and call sites but did not retrieve the remote transcripts, replay the tests/full round, or run a new compiler or OS mutant. Those reported executions remain author-run evidence. The instrument's dispatch-time coverage is not promoted to a required whole-round regression lane by this approval.
Non-blocking metadata cleanup: the PR body's opening still describes a second replacement as a non-idempotent build, and its final 'Not run' footer is stale relative to its updates and exact-head lint. The committed code/RFM and Update 6 express the narrower facts; align the summary without another code change.
No further code change, accounting redesign, immutable-exec project or new test lane is requested. Land through normal required composed-revision checks. Nothing merged or enqueued.
|
Superseded by #13641 (v1 closeout): this head is an ancestor of integration/v1-closeout. |
Root cause (DESIGN §6b):
round.seed_buildbuildsclaim_executorto the path this process is running from, so a relinking build replaces the running image; the process then refuses (correctly) because its emit would speak for the old seed. The refusal fires on a precondition the flag itself manufactured; "run it twice" is a §5 workaround.Fix:
run_regen_round_costre-execs the built binary exactly once (env markerGUNBC_REGEN_ROUND_COST_REEXEC); the receiving image refuses with the typedSeedHandoffRefusalwhen the opened target is not the build output, when the running image or the source differs from what was carried, when the path is replaced again, or when the ledger has other than exactly oneround.seed_buildrow. A second replacement shows another path replacement, not that the build is non-idempotent over an unchanged tree. Row filed:recurring_failure_mode/a_build_replaces_the_executable_that_is_running_it.Evidence (one
ctrl-build --remotedispatch, head 86b42a1, build and run together, amd64):RED_EXIT=1,refused: ... the seed build replaced the running executable (fnv1a64:2e4b... -> fnv1a64:2b55...).round.seed_build done in 92ms, refusal gone.POSITIVE CONTROL (full single-run green round, head 86b42a1, one
ctrl-build --remotedispatch: build, forced relink, create cgroup/sys/fs/cgroup/rrcwithmemory.max=17179869184, move the shell in, run once):GREEN_EXIT=0. Excerpt:The earlier HostBudgetUnreadable panic was my harness (no cgroup memory limit bound the process), not a BuildBuddy defect.
UPDATE (review P1/P2, head 8ee4c3f for the Rust; later heads touch only .dag rows):
GUNBC_REGEN_ROUND_COST_REEXECandcompose_seed_build_costsums it into theround.seed_buildledger row and crate count once. Full single round (same cgroup method,GREEN_EXIT=0): pre-exec build 3 min, re-exec no-op 88ms, receiptseed_build_compiled_crates=1,phase=seed_build wall_ms=208817 cpu_ms=354770(was ~96ms).SeedHandoffRefusal(ExecutableReplacedAfterHandoff, TreeChangedAcrossHandoff, HandoffUndecodable, ReExecFailed); the claim is narrowed to "another path replaced the executable after the handoff"; tree identity is carried and compared. Standing: runtime mitigation, rung 1; ceiling 3 on modeling the seed build output path in .dag.ctrl-build --remote -- bash -lc 'mkdir /sys/fs/cgroup/rrc; echo 17179869184 > .../memory.max; echo $$ > .../cgroup.procs; cargo test --release -p v1-compiler --lib regen_round_cost_tests'on a BuildBuddy amd64 runner: 4 passed, includingpre_handoff_seed_build_cost_is_composed_exactly_once_into_the_receipt(nonzero carried cost and crate count survive the production composition and renderer, once) andhandoff_decisions_are_typed_and_round_trip. The merge-queue rust-unit-tests lane will also run them.regen_round_cost_instrument_seed_growth.UPDATE 2 (review of aba309c, head b6ca191):
compose_seed_build_costnow returns a Result and refuses with typedSeedBuildMeasurementCount{found}unless exactly oneround.seed_buildrow exists; controls cover zero rows and two rows.built_exe(digest of the artifact the first image built) andsource(HEAD + digest ofgit diff HEAD --binary, so a dirty tree stays legal). The re-exec'd image checks source (SourceChangedAcrossHandoff), then that it is the built artifact (WrongArtifactAcrossHandoff), then no second replacement. Negatives: same-commit/different-source, wrong artifact, second replacement; positive: built artifact + same source proceeds.Err(String); ceiling-3 trigger is execution of the admitted build artifact with complete, or explicitly unavailable, accounting. Standing stays rung 1.ctrl-build --remote -- bash -lc 'mkdir -p /sys/fs/cgroup/rrc; echo +memory > /sys/fs/cgroup/cgroup.subtree_control; echo 17179869184 > /sys/fs/cgroup/rrc/memory.max; echo $$ > /sys/fs/cgroup/rrc/cgroup.procs; cargo test --release -p v1-compiler --lib regen_round_cost_tests'at b6ca191: 4 passed, TEST_EXIT=0. The full-round green above predates this change (head 8ee4c3f Rust); the handoff logic it exercised is unchanged apart from the added checks.UPDATE 3 (head 8d732ad): the source identity also folds path+content digests of untracked, non-ignored files (
git ls-files --others --exclude-standard); new controlsource_identity_sees_untracked_files(added and edited untracked file each change the identity and refuse via SourceChangedAcrossHandoff). Same remote cgroup command: 5 passed, TEST_EXIT=0.UPDATE 4 (head 9446c54): untracked entries are framed as u64-LE length + raw path bytes + content digest (no
=/newline ambiguity); controluntracked_framing_distinguishes_ambiguous_populationsruns the two-population case throughcompose_source_identity+decide_seed_handoff(SourceChangedAcrossHandoff; unchanged-source positive kept).built_exeis now the digest of the descriptor that is exec'd:open_exec_targetopens the path once after the build, hashes the bytes read from that fd, and the re-exec runs/proc/self/fd/Nimmediately after; controlexec_target_digest_is_of_the_opened_inode_not_the_pathreplaces the path after the open and shows the digest still names the opened bytes. Historical-refusal wording aligned with the Err(String) sentence; rung 1. Controls: same cgroup command, 7 passed. Full single round at this head (forced relink, cgroup, one dispatch, exec-by-fd path): GREEN_EXIT=0, seed_build 3 min then 98ms, seed_build_compiled_crates=1, phase=seed_build wall_ms=216005 cpu_ms=362740.UPDATE 5 (head 6b3e8d6): the receiving image digests the running image via
/proc/self/exeopened directly (running_image_digest; no pathname reopen, no(deleted)strip) asexe_before, and compares it with the carriedbuilt_exe(WrongArtifactAcrossHandoff) and with the installed path after its own build (ExecutableReplacedAfterHandoff). Three identities stay separate: build output (installed path), opened exec target (carried), running image. A rewrite of the inode between hash and exec is detected after the fact, not prevented (rung 1). New controls:modify_after_verify_and_installed_running_mismatch_refuse(rewrite inode after hashing, B put back at the path: refuses; installed/running mismatch refuses; same-artifact proceeds),running_image_is_read_from_proc_self_exe_not_the_installed_path; path-replacement control kept. Same cgroup command: 9 passed. Full forced-relink round at this head: GREEN_EXIT=0, seed_build 3 min then 93ms, seed_build_compiled_crates=1, wall_ms=220306.UPDATE 6 (head 336ecb0): the carry keeps the BUILD-OUTPUT digest (
built_exe= installed-path digest after the build) and is never overwritten from the open.admit_exec_targetcompares the opened descriptor's digest with it before the exec; mismatch is the typedOpenedTargetNotBuildOutput. Controlexec_target_admission_compares_with_the_build_output_and_never_overwritesgoes through the productiondecide_seed_handoff+admit_exec_target: build=B/opened=C refuses; build=B/opened=B proceeds with the carry intact and the receiver then proceeds (a byte-identical replacement is digest-equal, so allowed). Same cgroup command: 10 passed. Full forced-relink round at this head: GREEN_EXIT=0, seed_build 3 min then 93ms, seed_build_compiled_crates=1, wall_ms=221671.CI coverage: no required lane executes
--regen-round-cost; it runs only viainstrument-dispatch.yml(workflow_dispatch, label regen-round-cost). The RED and control above are therefore dispatch-time local evidence, not an enrolled regression control. Rung 1 (runtime mitigation): the historical refusal was an untypedErr(String); the new refusals are typed. It is not mechanically enforced; an enrolled discriminating witness would need a lane that runs the instrument. Executed: the 10 Rust unit tests in a cgroup on a BuildBuddy runner (UPDATE 6), a full forced-relink round, and PR CI (seed, emit-build, floor, generated and witnesses all green; clippy runs in the generated lane). The PR rust-unit-tests lane is skipped on pull_request; the merge queue runs it.🤖 Generated with Claude Code