Skip to content

ref: the window's start marker lands at its line, not at the stream's end - #369

Merged
juliankang4 merged 3 commits into
midagedev:mainfrom
Burntgogi:fix/cold-marker-line-responsive
Oct 17, 2026
Merged

juliankang4 merged 3 commits into
midagedev:mainfrom
Burntgogi:fix/cold-marker-line-responsive

Conversation

@Burntgogi

@Burntgogi Burntgogi commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Fixes #362. The awk-based fault-window marker can delay its pgmajfault sample 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 main c14c46115ae487b3472e6c8ce80d2d109fd599bb.

Change and review resolution

  • Read the stream one line at a time; sample only the first matching line. Empty regexes create no marker, and final partial lines use the same processing path.
  • Keep majflt_now as the counter owner for the preflight, marker and all three stub suites. No majflt_sample copy remains.
  • Propagate failed sampling and forwarding with || return; retain the prefix-only assertion that rejects an early marker.
  • Remove the identical second majflt_require definition reported by the latest review. There is one preflight owner.
  • Add a behavioral regression using a real FIFO and read-only stdout: the filter must return failure while upstream stdin is still open, not wait for EOF. Clean up the owned fixture and document the contract in 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.sh work 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.

Check Observed result
bash -n on the helper and all three changed stub scripts pass
bash tools/ref/cold-blocks.sh --self-test 0 failures
bash tools/ref/depth-ds41-stub.sh 84 checks, 0 failed; warm/warm-fail/warm-bad pass
bash tools/ref/depth-qwen3moe-stub.sh 86 checks, 0 failed; warm passes
bash tools/ref/depth-glm5next-stub.sh 49 checks, 0 failed; explicit skips below
bash tools/check-recipes.sh pass; 785 card/runner-fixture checks, 59 tool self-tests, 2,725 tests with gate ownership

Seven additional actual-source smoke cases exercise a real open pipe and /proc counter, 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:

  • Remove the forwarding guard: marker-forward-failure fails.
  • Remove the regex predicate: marker-before-match fails.
  • Restore the buffered awk marker from main 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 --check on the stated source.

Coverage limits and base-specific history

  • GLM corpus-dependent cases skip because the configured pinned corpus-prose.ids is unavailable locally; no replacement IDs or SHA change was used. dry-same also skips because DEPTH_GLM5NEXT_BASE is unset.
  • Native Mac execution was not run; the Linux-counter-specific self-test clause is conditional on /proc/vmstat.
  • The pre-rebase local head 98839fa failed the old English mkfifo.*File exists assertion 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.
  • No CUDA execution, model-accuracy, throughput or timing result is claimed. These are host/tooling and stub checks.

Immutable evidence

Exact-head result, full logs, live-stream smoke, environment and applicable mutant patches.

Separately labelled pre-rebase failure and verification scope.

@github-actions github-actions Bot added the tools Tools, recipes, CI and the toolchain: tools/, justfile, .github/, Cargo label Oct 16, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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" || return

2. 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' >&9

3. 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)

Comment thread tools/ref/cold-blocks.sh Outdated
majflt_sample "$f"
s=1
fi
printf '%s\n' "$line"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Propagate the forwarding error so lease_filter rejects a truncated output log.

Suggested change
printf '%s\n' "$line"
printf '%s\n' "$line" || return

@Burntgogi

Copy link
Copy Markdown
Contributor Author

All three items are addressed in e8e13f2.

  • Must fix 1: the forwarding write is printf ... || return, so a failed forward is the marker's failure, and the loop's exit status carries it (bash -c on a read-only stdout now returns rc 1 where head returned 0; base awk returned 2; re-verified locally both ways).
  • Must fix 2: the self-test sends the prefix alone first, asserts marker-before-match — text out, no marker — then sends the window line and keeps the wait and the following assertions. The predicate-deletion mutant the previous checks survived now fails this one (FAIL marker-before-match: got [ahead|yes], want [ahead|no], re-verified locally and restored).
  • Must fix 3: majflt_sample and its comment are gone, as are the three generated sampler overrides; the marker samples through majflt_now, whose majflt_now-reading stub overrides keep standing.

The three stub suites still pass whole on the fixed tree: DS41 84/0, qwen3moe 86/0, glm5next 49/0, all rc 0.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@Burntgogi
Burntgogi force-pushed the fix/cold-marker-line-responsive branch from e8e13f2 to 25d8a83 Compare October 16, 2026 23:37
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Oct 16, 2026
@Burntgogi

Copy link
Copy Markdown
Contributor Author

Addressed the latest MUST_FIX in source 25d8a832ae55e4da35baf5e288dc2aba30564aa0: the identical second majflt_require definition is removed. The previous forwarding/prefix/counter-owner corrections remain intact.

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:

  • remove the forwarding guard: exit 1, marker-forward-failure fails;
  • remove the regex predicate: exit 1, marker-before-match fails;
  • restore main's buffered awk marker: exit 1, before-EOF checks fail.
    All three retained patches pass git apply --check; all seven actual-source runtime smoke cases pass.

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 tools/check-recipes.sh passes: 785 card/runner-fixture checks, 59 tool self-tests, 2,725 owned tests. No model or GPU is used and no threshold, retry policy or existing fixture wait is relaxed.

The branch is rebased on upstream c14c461. This inherits already-merged #367 instead of changing locale or re-pinning its obsolete English strerror assertion; validation remains under LANG=ko_KR.UTF-8. Prior heads are preserved on the fork's archive refs. The original worktree and other session's #365 scope were not modified.

Immutable full logs, source identities, environment, live-stream evidence and mutants:
https://github.com/Burntgogi/bloomery/tree/d4b19d14d5a64dca5dbff73dae75fa72c77539b0/docs/contrib/evidence/pr369-marker-review/final-25d8a832

Ready for re-review. No CUDA execution, model-accuracy or timing result is claimed.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_filter returns 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 to marker-at-line, a forced failure after L159 to marker-rc, dropped output at L158 to marker-pass, removal of the empty-regex guard at L154 to marker-empty-re, and removal of the forwarding guard at L158 to marker-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)

@juliankang4
juliankang4 merged commit 7228ebf into midagedev:main Oct 17, 2026
7 checks passed
@juliankang4

Copy link
Copy Markdown
Collaborator

@Burntgogi Thanks for the contribution. The review is done, so I'm merging this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation tools Tools, recipes, CI and the toolchain: tools/, justfile, .github/, Cargo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cold marker samples at EOF under buffered mawk instead of window start

2 participants