Repository navigation
ref: the window's start marker lands at its line, not at the stream's end - #369
Conversation
There was a problem hiding this comment.
Verdict: MUST_FIX: the marker hides output write failures, its new test accepts an early marker, and the sampler duplicates existing readers.
Must fix
1. Propagate failures when forwarding output
tools/ref/cold-blocks.sh:166: a failed printf is discarded, and the final if returns success for newline-terminated input. A read-only stdout descriptor returns rc 2 at base and rc 0 at head. If the log fills after a complete throughput row, tools/ref/lease.sh:581 receives success. tools/ref/depth-ds41.sh:1656 then accepts the row, which can enter sums at line 1699.
Fixing the filter now avoids adding output-integrity checks to every result parser. Replace the forwarding write at line 166 with:
printf '%s\n' "$line" || return2. Assert that unmatched input creates no marker
tools/ref/cold-blocks.sh:462 sends the prefix and matching line together. Removing the regex predicate at line 162 creates the marker on ahead, before the measured window starts, yet all four new marker assertions pass. This is a kind B test defect with a mechanical fix.
Checking the prefix separately now prevents the test from accepting an incorrectly placed window boundary. Replace line 462 with the following; keep the existing wait and assertions afterwards:
printf 'ahead\n' >&9
for ((i=0; i<40; i++)); do [ -s "$d/out" ] && break; sleep 0.01; done
check marker-before-match "$(cat "$d/out")|$([ -e "$d/mark" ] && echo yes || echo no)" 'ahead|no'
printf 'window-start\n' >&93. Reuse the existing counter reader
tools/ref/cold-blocks.sh:146 reimplements tools/ref/cold-blocks.sh:137 (majflt_now). The new sampler overrides in tools/ref/depth-ds41-stub.sh:223, tools/ref/depth-qwen3moe-stub.sh:257 and tools/ref/depth-glm5next-stub.sh:250 also repeat their adjacent majflt_now overrides (R14).
Removing these readers now keeps counter changes and stub injection at the existing owner. Delete majflt_sample, its comment, and the three generated sampler overrides. Keep the existing majflt_now overrides. Replace the marker with this definition, which also shares the complete-line and final-line processing:
majflt_mark() {
local f=$1 re=$2 line s=''
while IFS= read -r line || [ -n "$line" ]; do
if [ -z "$s" ] && [ -n "$re" ] && [[ $line =~ $re ]]; then
majflt_now > "$f" || return
s=1
fi
printf '%s\n' "$line" || return
done
}Checked
Duplicates: 4 new reader definitions checked; each duplicates an existing owner.
Complexity: the changed marker has separate complete-line and EOF branches.
Silent failure: both forwarding writes and the lease_filter-to-ref_arm status path checked. Only the loop write loses its failure status.
Tests: Bash syntax passes for all four changed files. Extracted-function probes confirm stream passthrough, partial-line handling, the lost write status, and the surviving predicate-deletion mutant. A separate prefix assertion detects that mutant; the proposed forwarding fix returns failure.
Gates: no existing threshold, tolerance or test was removed or relaxed.
Escape catalog: no C1–C4 pattern matched. No engine hot path changed.
Notes
No Rust file changed. Full self-test and stub suites were not run: they read host state and create temporary files outside the permitted trees. Validation used extracted functions and in-memory pipes.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
| majflt_sample "$f" | ||
| s=1 | ||
| fi | ||
| printf '%s\n' "$line" |
There was a problem hiding this comment.
Propagate the forwarding error so lease_filter rejects a truncated output log.
| printf '%s\n' "$line" | |
| printf '%s\n' "$line" || return |
|
All three items are addressed in e8e13f2.
The three stub suites still pass whole on the fixed tree: DS41 84/0, qwen3moe 86/0, glm5next 49/0, all rc 0. |
There was a problem hiding this comment.
Since the last review:
- Forwarding failures: fixed; failed writes return failure.
- Prefix-only marker assertion: fixed; it detects a marker created before the matching line.
- Sampler duplication: fixed; the marker and stubs reuse
majflt_now.
Verdict: MUST_FIX: the new commits add an identical second definition of majflt_require.
Must fix
1. Keep one majflt_require definition
tools/ref/cold-blocks.sh:145 duplicates tools/ref/cold-blocks.sh:139 exactly (R14, kind B). Sourcing the file replaces the first definition with the second.
Removing the copy now prevents future changes to the first definition from having no effect. Replace lines 139–149 with this single definition, keeping the comment at line 138:
majflt_require() {
[ -n "$(majflt_now 2> /dev/null)" ] && return 0
echo "$1: /proc/vmstat gives no pgmajfault: every row's majflt column and cold tag would read 0" >&2
exit 2
}Checked
Duplicates: 1 new definition checked; it duplicates the existing preflight owner. The earlier sampler copies are removed.
Complexity: the marker now handles complete and final partial lines in one loop; no crapper report.
Silent failure: 2 changed operations followed, sampling and forwarding. Their failures reach lease_filter, then a failed arm.
Tests: all four changed files pass Bash syntax checks. The new prefix assertion rejects a marker created on ahead before the matching line. Full self-test and stub suites were not run.
Gates: no threshold or tolerance moved; no test removed or relaxed.
Escape catalog: no C1–C4 pattern matched. No engine hot path changed.
Notes
Dryer and crapper did not run because no Rust file changed.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
… end The marker's sample ran in an awk whose stdin is buffered (mawk 1.3.4, gawk and the Mac's awk alike): the match's sample waited for the buffer's fill or the stream's end, and faults inside the measured window landed under its start — the warm and warm-fail checks of the DS41 stub suite have carried it. The mark is now one line at a time through the shell, with the sampler itself (majflt_sample) the part the stub suites replace, and the self-test pins the marker against a pipe that stays open.
…s owner A failed forwarding write was dropped and the marker's own sampler was a second reader of the counter the owners already read. The forward now returns a printf failure, the sample is the counter's owner (majflt_now, whose stub overrides stand), and the self-test asserts that input before the window's line marks nothing — the marker check the matched line's predicate needs.
e8e13f2 to
25d8a83
Compare
|
Addressed the latest MUST_FIX in source The added forwarding regression uses a real FIFO with stdin held open and read-only stdout. It requires an error before upstream EOF. A simpler EOF-only probe was insufficient because the loop's last failed printf could accidentally preserve the status even after the guard was removed. Actual complete-self-test negative controls on this exact source:
Exact-head validation: four Bash syntax checks and helper self-test pass; DS41 84/0 failed, Qwen 86/0, GLM 49/0 with corpus/dry-same skips explicitly retained. Full The branch is rebased on upstream Immutable full logs, source identities, environment, live-stream evidence and mutants: Ready for re-review. No CUDA execution, model-accuracy or timing result is claimed. |
There was a problem hiding this comment.
Verdict: CLEAN: The shared marker samples at its first matching line and propagates sampling and forwarding failures to failed runner rows.
Checked
- Duplicates: 0 new production functions, types or constants. One production marker and counter owner remain; all three stub marker overrides are removed.
- Complexity: no Rust functions changed. The shell change adds a line-reading loop with explicit error exits.
- Silent failure: the two guards at tools/ref/cold-blocks.sh:155 and tools/ref/cold-blocks.sh:158 preserve counter, marker-write and forwarding failures.
lease_filterreturns the filter status, and the DS41, Qwen3MoE and GLM callers handle nonzero status as failed arms. - Tests: static mutation review maps premature sampling at L154 to
marker-before-match, buffering at L153 tomarker-at-line, a forced failure after L159 tomarker-rc, dropped output at L158 tomarker-pass, removal of the empty-regex guard at L154 tomarker-empty-re, and removal of the forwarding guard at L158 tomarker-forward-failure. The forwarding test keeps upstream stdin open while checking failure. - Validation: each of the four changed shell files passes
bash -n. - Gates: no bands, thresholds, pins or existing assertions moved or were removed. The PR body records FAIL-first results for buffering, predicate and forwarding mutations.
- Escape catalog: no C1–C4 patterns matched the changes. Failed marker and output captures produce failing assertions.
- Adversarial: the reader processes a matching line before waiting for another line, samples only once, skips an empty regex and processes a final partial line through the nonempty-line fallback.
- Performance: only runner/tooling code changed; no engine hot path changed.
Notes
The original review reports that dryer and crapper did not run because no Rust files changed. Runtime self-tests, stub suites and isolated probes were not rerun during validation.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
|
@Burntgogi Thanks for the contribution. The review is done, so I'm merging this. |
Problem
Fixes #362. The awk-based fault-window marker can delay its
pgmajfaultsample until buffered input is consumed, excluding faults from the intended measured window. The marker must sample at its first matching line while the engine's output stream is still open, and failed sampling or forwarding must fail the arm.Source
25d8a832ae55e4da35baf5e288dc2aba30564aa0, rebased onto fetched upstream mainc14c46115ae487b3472e6c8ce80d2d109fd599bb.Change and review resolution
majflt_nowas the counter owner for the preflight, marker and all three stub suites. Nomajflt_samplecopy remains.|| return; retain the prefix-only assertion that rejects an early marker.majflt_requiredefinition reported by the latest review. There is one preflight owner.docs/performance.md.No engine, model, threshold, cold policy or retry rule is changed; existing fixture waits are not lengthened. This session newly edits only the shared helper and documentation; the three stub-counter migrations already present in the PR are retained. The other session's
tools/box.shwork is untouched.Exercised exact-head verification
Linux x86_64, Bash 5.2.21, mawk 1.3.4;
LANG=ko_KR.UTF-8, no locale override. No model or GPU is used.bash -non the helper and all three changed stub scriptsbash tools/ref/cold-blocks.sh --self-testbash tools/ref/depth-ds41-stub.shbash tools/ref/depth-qwen3moe-stub.shbash tools/ref/depth-glm5next-stub.shbash tools/check-recipes.shSeven additional actual-source smoke cases exercise a real open pipe and
/proccounter, first-match-only sampling through the existing counter override boundary, byte-preserving prefixes/final-fragment handling, empty regex, read-only stdout, counter failure, marker-file failure and unavailable-counter preflight. Controlled counter failures preserve exit 9; unavailable counters exit 2; I/O failures are nonzero.Negative controls
Run the unchanged actual self-test with one production mutation at a time; each exits 1:
marker-forward-failurefails.marker-before-matchfails.c14c461: the before-EOF marker checks fail.An EOF-only write-failure probe was insufficient because the loop's final command could preserve the failing status accidentally. The retained regression keeps stdin open and detects the lost immediate failure propagation. All three retained mutant patches pass
git apply --checkon the stated source.Coverage limits and base-specific history
corpus-prose.idsis unavailable locally; no replacement IDs or SHA change was used.dry-samealso skips becauseDEPTH_GLM5NEXT_BASEis unset./proc/vmstat.98839fafailed the old Englishmkfifo.*File existsassertion under the Korean locale. Rebasing inherits main's already-merged tests: assert FIFO failure outcomes without localized stderr #367 correction; the final full recipe run passes without changing locale or re-pinning the wording.Immutable evidence
Exact-head result, full logs, live-stream smoke, environment and applicable mutant patches.
Separately labelled pre-rebase failure and verification scope.