mirror of
https://github.com/dtzp555-max/ocp.git
synced 2026-07-27 16:05:07 +00:00
92787dcced
* ci: add a manual flake hunt for #203 (Linux x Node version) #203 has been seen four times, always on Linux CI, never once on macOS across 1000+ runs in two independent experiments. The documented next step is a Linux + Node 24 reproduction, and there is currently no way to run one without reddening an unrelated PR and waiting for chance. workflow_dispatch only — it never runs on push or pull_request, so it costs nothing until someone asks for it. Inputs are the two suspected variables: - `node` is a choice (24 / 22 / 26) rather than pinned, because 22-vs-24 is a comparison worth running deliberately. A previous Linux VM attempt was invalidated by Node 22's `node:sqlite` ExperimentalWarning landing on stderr, which the boot gate read as "server did not start" — ~23 of 50 runs failed spuriously. The workflow says so, so the next person interprets a 22 result against that confound instead of rediscovering it. - `concurrency` is the knob that actually reproduces, not round count: test() is fire-and-forget for async bodies, so ONE suite run already spawns 15 concurrent server.mjs children across 11 ltBoot tests. Several suites at once is what multiplies cross-process contention. Classification is anchored on the failure marker AND the test name, which is not cosmetic. ltDiag samples the first 900B of child stdout (#204) — the boot banner — so a log where only the GATE test failed still contains "Local tools: ON". Validated against real logs rather than reasoned about: 3 clean runs + 1 gate-mutation run gave unanchored 'Local tools' -> 1 hit, attributed to the WRONG category anchored -> gate=1, announce=0 (correct) totals -> total=4 clean=3 (correct) My first draft had the unanchored form with a comment claiming the failure mode was "matches every log". That was wrong — the real defect is cross-category attribution, caused by the diagnostic quoting the child's own output. Found by running the logic on real logs; the comment now states the measured reason. The job does NOT fail on a reproduction: catching the flake is the goal, and a red X would read as "the hunt is broken" rather than "the flake was caught". Results go to the step summary; logs upload as an artifact. server.mjs: unchanged (CI-only; ALIGNMENT.md requires no cli.js citation). Verified: YAML parses, all three run blocks pass `bash -n`, classify logic exercised against real suite output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gbqUZ8HfBZpjjbzQ85oH8 * ci: fix the hunt's fatal shell bug and retract a wrong root-cause story Review found two HIGH defects. Both confirmed independently before fixing. 1. The Classify step would have failed on EVERY run and produced no summary. I wrote `set -uo pipefail # NOT -e`. GitHub's default shell is `bash -e {0}`, and that line does not turn errexit OFF — it only ADDS pipefail. So any category counting zero makes grep exit 1, pipefail propagates it through the pipeline, the command substitution inherits it, and errexit kills the step. The all-clean case — the result this workflow most wants to report — dies on the FIRST counter. bash -e -c 'set -uo pipefail; x=$(echo hi | grep -c nomatch); echo REACHED' -> exit=1, "REACHED" never printed bash -e -c 'set +e -u -o pipefail; ...; echo REACHED' -> REACHED, x=0 Now `set +e -u -o pipefail`, with the reason in the file so nobody "tidies" it back. Verified by EXECUTING the extracted step under `bash -e` against real logs: exit=0, 2836 bytes of summary. My stated verification was `bash -n`, which is a pure syntax check and structurally cannot catch this. And because workflow_dispatch requires the file on the default branch, the workflow could not have been run end-to-end before merge — so nothing else would have caught it either. 2. The Node 22 story was a misattribution, and I had propagated it four places. I claimed Node 22's `node:sqlite` ExperimentalWarning was read by the boot gate as "server did not start", invalidating an earlier Linux run. The warning is real; the causal claim is false. The predicate is ltWait(() => buf.out.includes("listening on") || buf.exit != null) stdout only. `buf.err` appears in the assertion MESSAGE, never in the condition, so a stderr warning cannot fail it. Review also ran the full suite on Node 22.23.1: 462 passed, 0 failed, with the warning present in the logs. What actually produced that noise floor is something I had already measured and then failed to connect: pre-#204 fixed ports gave 246 EADDRINUSE and only 42/200 clean runs on unmodified main. The warning was merely VISIBLE in the failure text — via buf.err.slice(0,200) — and I read presence in the error message as causation. That is the same error I have been correcting in others' findings all week. The cost was not cosmetic: the input description told the next person that Node 22 was confounded, which would have made them discard a perfectly usable arm. Node 22 is now offered plainly, and the file states the correction so the wrong story does not survive in the artifact that outlives this PR. Also from review: - `ref` input (MED-1): a null result on current main is uninterpretable, because #204 may already have fixed #203. Hunting `7f15921^` is the positive control. - inputs go through `env:` (MED-2): free-text inputs were interpolated straight into the script body. GitHub documents `inputs.*` as untrusted. Added `type: number`. - #203's SIGNATURE, not its test name (MED-3): a CPU-starved runner blows the 9s ltWait and emits the same "✗ boot gate REFUSES" line. #203 is closed=true + non-zero exit + EMPTY stderr. Both counters are reported; the difference is contention. Demonstrated live — a gate-mutation run scored gate=1, sig=0. - full histogram instead of a hand-maintained category table: an enumerated list silently drops the failure nobody thought of, which on a hunt is the interesting one. Bucketed on a 72-char name prefix, NOT `sed 's/:.*//'` as suggested — test names contain colons, so that collapses every `localToolsSafetyError: <case>` into one bucket (verified). - per-run `timeout 300` and step-level timeout (MED-4), `permissions: contents: read`, a `concurrency:` group, Node 25 in the choices. - "15 concurrent server.mjs children" was wrong: 15 is TOTAL spawns; peak is 11-12 (review measured 11 x3, I measured 12) because the gate test's 3 cases and the 2 epoch boots are awaited serially. Corrected. - dropped the ephemeral-port note: after #204 ports come from ltFreePort(), so it is stale and the inference now runs backwards. server.mjs: unchanged (CI-only; ALIGNMENT.md requires no cli.js citation). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gbqUZ8HfBZpjjbzQ85oH8 * ci: fix the ref the control arm depends on, and a signature that matched the opposite bug Delta review ofbe7c545. One new HIGH, one real defect in the signature, three LOWs. HIGH — `ref: 7f15921^` cannot be checked out, so MED-1's entire remedy was inert. actions/checkout does not rev-parse; it takes a branch, a tag, or a FULL 40-char SHA. The input description told the operator to type the one form that fails: git ls-remote origin '7f15921^' -> 0 rows git ls-remote origin '7f15921' -> 0 rows (abbreviated SHA also fails) git fetch --depth=1 origin '+7f15921^:refs/...' -> fatal: invalid refspec '+7f15921^:refs/remotes/origin/tmpcheck' git rev-parse 7f15921^ ->c180987376(fetches fine) The whole point of `ref` is the pre-#204 positive control, and it would have red-X'd on the first step. Full SHA now spelled out in both the description and the note, with the resolution rule stated so the next person doesn't retype `^`. The signature grep matched the OPPOSITE failure. The comment claimed it tested "closed, exited NON-ZERO, empty stderr". It didn't test the exit code at all — `.*` swallowed it: line old new [multi] expected a local-tools FATAL exit=1 1 1 <- true #203 [multi] must exit non-zero exit=0 1 0 <- gate DIDN'T refuse exit=0 is the second assertion in that test: the gate failed to refuse and the server came up and exited. That is the inverse of #203 and it was scoring as #203. Pinning the match to `expected a local-tools FATAL` binds it to the third assertion. Verified against four synthesised shapes drawn from real ltDiag text — true-#203, exit=0, contention, and the pre-ltDiag historic format: sig=1 (TRUE203 only) gate=4 (all four) clean 1/5 completed, 6 launched Also recorded, because it would otherwise look like a broken grep: all four historic sightings PREDATE #204's ltDiag — their text is `expected a local-tools FATAL, got: ` with no `closed=` and no `stderr(0B)` — so the signature can only ever match a future reproduction. Do not "validate" it against the archived CI logs. LOWs, all taken: - a round killed by the step timeout leaves logs with no Results line; they counted in `total` and could never be clean, silently depressing the rate. `completed` is now reported separately, so a shortfall reads as a timeout rather than as a flake. - `timeout 300 npm test` sends SIGTERM to `npm`, which need not reach the node child or the server.mjs grandchildren. Now `timeout -k 10 --foreground 300`. - `concurrency.group` keyed on `github.ref` let two dispatches from different branches run at once — contending for the runner class, the one variable this experiment holds still. Bare `flake-hunt` group serializes all of them. - noted the invariant that the `[ -z "$first" ] &&` list returns 1 when $first is set, so it must never become the last command in the step. Confirmed unchanged after review re-checked them: `set +e -u -o pipefail` is sufficient (exit 0 across all-clean, missing logs/, and a SIGPIPE'd histogram), and the 72-char histogram bucket stands — `sed 's/:.*//'` would collapse every `integration:` test into one row, which is worse than I had argued. Reviewer's 11 vs my 12 reconciles: 11 live children plus one SIGKILLed-not-yet-reaped, because the `finally` does not wait. "11-12" stays. server.mjs: unchanged (CI-only; ALIGNMENT.md requires no cli.js citation). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gbqUZ8HfBZpjjbzQ85oH8 * ci: drop --foreground — it does the opposite of what -k is there for Delta review caught this, and it was mine: I added `--foreground` while implementing the reviewer's own LOW about SIGTERM not reaching grandchildren, and the flag defeats exactly that. coreutils is explicit: -f, --foreground ... in this mode, children of COMMAND will not be timed out By default timeout makes itself process-group leader and signals the GROUP, so the node child and the server.mjs grandchildren go down with npm. Measured with the same shape the workflow uses (`timeout ... &` then `wait`), a script holding one direct child and one grandchild, 3 runs each: default survivors: 0 0 0 --foreground survivors: 2 2 2 My comment also claimed --foreground was needed "so the timeout applies to a backgrounded job at all". Also false — the default form fires on backgrounded jobs 3/3 above. The cost was not cosmetic. 50 rounds x 4 concurrent means every timeout would have leaked a batch of orphaned node/server.mjs processes that keep competing for the runner's 4 vCPUs — inflating the contention noise floor, which is precisely the column (`gate` minus `sig`) this workflow exists to separate from #203. server.mjs: unchanged (CI-only; ALIGNMENT.md requires no cli.js citation). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gbqUZ8HfBZpjjbzQ85oH8 --------- Co-authored-by: dtzp555 <dtzp555@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>