From f70afbee7159bcf969d8a5956e6c439a00523602 Mon Sep 17 00:00:00 2001 From: EtienneLescot Date: Fri, 21 Aug 2026 16:36:59 +0200 Subject: [PATCH 1/2] ci(nix): run record before sources, to test whether position is the cause An A/B, not a fix. Across four runs record has succeeded 1 time in 10 while sources managed 13 in 20, and the durations added last week rule out the obvious reading: when record works it returns in 17ms, exactly like sources, and when either fails the call does not return at all. Same mechanism, same timing, different frequency. The one difference between the two was position. run_cli spawns a fresh `xvfb-run -a` per invocation, so record was always invocations 6-8 of the step -- after five Xvfb servers had been started and torn down -- while sources ran at 4-8. That is enough to explain the gap without postulating anything about record at all, and it is the cheaper hypothesis, so it goes first. Swapping the blocks makes the prediction symmetric, which is the point: if display churn drives the failures, record should improve AND sources should get worse. If record stays near 1 in 10 from the front of the step, position was never the cause and the search moves into the record path itself -- it mounts the recording pipeline before pickSource, where the sources runner enumerates almost immediately. Note for whoever reads the next run: one run is one sample, and the failures are plainly correlated within a runner rather than independent across attempts. A single green record proves nothing on its own; what would move this is the pair of rates moving together, in opposite directions. Co-Authored-By: Claude --- .github/workflows/nix-build.yml | 156 +++++++++++++++++--------------- 1 file changed, 85 insertions(+), 71 deletions(-) diff --git a/.github/workflows/nix-build.yml b/.github/workflows/nix-build.yml index 6806ad745..8ac18af3e 100644 --- a/.github/workflows/nix-build.yml +++ b/.github/workflows/nix-build.yml @@ -204,6 +204,87 @@ jobs: echo "--- openscreen --help ---" run_cli $SANDBOX $CHROME_FLAGS --help + # The real acceptance test, and now the first one to run. Nothing above + # touches the compositor addon, which is what actually renders output: + # record a couple of seconds, export it, and look at what came out. + # + # ORDERING EXPERIMENT -- this block used to sit after the sources loop, + # and moving it is the whole point of the change that put it here. + # Across four runs, record succeeded 1 time in 10 while sources managed + # 13 in 20, and when record does work it returns in 17ms, exactly like + # sources: same mechanism, different frequency. The one difference the + # two had was position. run_cli spawns a fresh `xvfb-run -a` per + # invocation, so record was always invocations 6-8 of the step, after + # five Xvfb servers had been started and torn down, while sources ran at + # 4-8. If display churn is what drives the failures, the rates should now + # swap: record improving AND sources getting worse. If record stays at + # roughly 1 in 10 from this position, position was never the cause and + # something in the record path itself is, which is a different search. + # + # Read the next run as that A/B, not as a verdict on the package. Put it + # back if the rates do not move. + # + # Up to three goes, because screen capture on this host is unreliable in + # its own right. One success is enough for the question being asked here. + echo "--- record then export ---" + EXPORTED="" + # Tracked apart from EXPORTED so the verdict can name the stage that + # actually failed. Every run so far has died in record without export + # ever executing, while the annotation said "the export path does not + # work" -- an accusation aimed at the one component the run never + # reached, and the compositor addon is precisely what this step exists + # to vouch for. + RECORDED=0 + for i in 1 2 3; do + echo "=== export attempt $i/3 ===" + rm -f /tmp/demo.openscreen /tmp/demo.mp4 + RC=0 + CLI_TIMEOUT=120 OPENSCREEN_DIAGNOSTIC=1 run_cli $SANDBOX $CHROME_FLAGS record --duration 2 --project /tmp/demo.openscreen >"/tmp/rec.$i.out" 2>&1 || RC=$? + # Outside the failure branch for the same reason as above: a record that + # works is exactly the measurement missing from the comparison, since + # this path has never yet produced one. + grep -a "get-sources\]" "/tmp/rec.$i.out" || true + if [ "$RC" -ne 0 ] || [ ! -f /tmp/demo.openscreen ]; then + echo "record failed (rc=$RC); last lines:" + tail -5 "/tmp/rec.$i.out" || true + continue + fi + RECORDED=1 + echo "recorded. project:" + head -c 200 /tmp/demo.openscreen; echo + + RC=0 + CLI_TIMEOUT=180 OPENSCREEN_DIAGNOSTIC=1 run_cli $SANDBOX $CHROME_FLAGS export /tmp/demo.openscreen -o /tmp/demo.mp4 >"/tmp/exp.$i.out" 2>&1 || RC=$? + if [ "$RC" -ne 0 ] || [ ! -f /tmp/demo.mp4 ]; then + echo "export failed (rc=$RC); last lines:" + tail -15 "/tmp/exp.$i.out" || true + continue + fi + EXPORTED=/tmp/demo.mp4 + break + done + + EXPORT_OK=0 + if [ -z "$EXPORTED" ] && [ "$RECORDED" -eq 0 ]; then + echo "::error::No attempt got past record, so export never ran and the compositor addon is unproven. This is a capture failure on this host, not an export failure." + elif [ -z "$EXPORTED" ]; then + echo "::error::record produced a project but no attempt produced an MP4. The compositor addon is packaged and the export path does not work." + else + SIZE=$(wc -c < "$EXPORTED") + # An MP4 opens with a 4-byte length then 'ftyp'. A zero-length or + # truncated file would otherwise pass a mere existence check. + MAGIC=$(dd if="$EXPORTED" bs=1 skip=4 count=4 2>/dev/null || true) + echo "exported $SIZE bytes, magic at offset 4: $MAGIC" + if [ "$MAGIC" != "ftyp" ]; then + echo "::error::output is not an MP4 (no ftyp box)" + elif [ "$SIZE" -lt 10000 ]; then + echo "::error::MP4 is only $SIZE bytes, too small to hold two seconds of video" + else + echo "Export works: $SIZE bytes of MP4." + EXPORT_OK=1 + fi + fi + # `sources` enumerates displays, windows and microphones, so it # exercises the capture stack rather than proving that a usage string # can be printed. @@ -294,10 +375,10 @@ jobs: PY fi - # Noted, not acted on yet. Enumeration is flaky here and the export - # assertion below is the more important question; letting one flaky - # attempt exit the step meant the run never reached it. Verdict at the - # end, once both have had their say. + # Noted, not acted on. Enumeration is flaky here and the export + # assertion -- which now runs above rather than below -- is the more + # important question; a flaky attempt costs an annotation rather than + # the answer. Verdict at the end, once both have had their say. if [ "$HUNG" -gt 0 ]; then echo "::warning::openscreen sources had to be killed on $HUNG of $ATTEMPTS attempts. Check the milestones above for how far it got before the outer bound fired." fi @@ -305,73 +386,6 @@ jobs: echo "::warning::openscreen sources failed on $FAILED of $ATTEMPTS attempts. The capture path is not reliable on this host." fi - # The real acceptance test. Everything above proves the package starts - # and can list a screen; none of it touches the compositor addon, which - # is what actually renders output. Record a couple of seconds, export - # it, and look at what came out. - # - # Up to three goes, because screen capture on this host is unreliable - # in its own right -- `sources` fails a couple of times in five. One - # success is enough to answer the question being asked here; repeated - # capture failures are a separate problem, already reported above. - echo "--- record then export ---" - EXPORTED="" - # Tracked apart from EXPORTED so the verdict can name the stage that - # actually failed. Every run so far has died in record without export - # ever executing, while the annotation said "the export path does not - # work" -- an accusation aimed at the one component the run never - # reached, and the compositor addon is precisely what this step exists - # to vouch for. - RECORDED=0 - for i in 1 2 3; do - echo "=== export attempt $i/3 ===" - rm -f /tmp/demo.openscreen /tmp/demo.mp4 - RC=0 - CLI_TIMEOUT=120 OPENSCREEN_DIAGNOSTIC=1 run_cli $SANDBOX $CHROME_FLAGS record --duration 2 --project /tmp/demo.openscreen >"/tmp/rec.$i.out" 2>&1 || RC=$? - # Outside the failure branch for the same reason as above: a record that - # works is exactly the measurement missing from the comparison, since - # this path has never yet produced one. - grep -a "get-sources\]" "/tmp/rec.$i.out" || true - if [ "$RC" -ne 0 ] || [ ! -f /tmp/demo.openscreen ]; then - echo "record failed (rc=$RC); last lines:" - tail -5 "/tmp/rec.$i.out" || true - continue - fi - RECORDED=1 - echo "recorded. project:" - head -c 200 /tmp/demo.openscreen; echo - - RC=0 - CLI_TIMEOUT=180 OPENSCREEN_DIAGNOSTIC=1 run_cli $SANDBOX $CHROME_FLAGS export /tmp/demo.openscreen -o /tmp/demo.mp4 >"/tmp/exp.$i.out" 2>&1 || RC=$? - if [ "$RC" -ne 0 ] || [ ! -f /tmp/demo.mp4 ]; then - echo "export failed (rc=$RC); last lines:" - tail -15 "/tmp/exp.$i.out" || true - continue - fi - EXPORTED=/tmp/demo.mp4 - break - done - - EXPORT_OK=0 - if [ -z "$EXPORTED" ] && [ "$RECORDED" -eq 0 ]; then - echo "::error::No attempt got past record, so export never ran and the compositor addon is unproven. This is a capture failure on this host, not an export failure." - elif [ -z "$EXPORTED" ]; then - echo "::error::record produced a project but no attempt produced an MP4. The compositor addon is packaged and the export path does not work." - else - SIZE=$(wc -c < "$EXPORTED") - # An MP4 opens with a 4-byte length then 'ftyp'. A zero-length or - # truncated file would otherwise pass a mere existence check. - MAGIC=$(dd if="$EXPORTED" bs=1 skip=4 count=4 2>/dev/null || true) - echo "exported $SIZE bytes, magic at offset 4: $MAGIC" - if [ "$MAGIC" != "ftyp" ]; then - echo "::error::output is not an MP4 (no ftyp box)" - elif [ "$SIZE" -lt 10000 ]; then - echo "::error::MP4 is only $SIZE bytes, too small to hold two seconds of video" - else - echo "Export works: $SIZE bytes of MP4." - EXPORT_OK=1 - fi - fi # One verdict, after both questions have been asked. Enumeration being # flaky must not hide whether export works, which is the whole point of From ced6ba7c6dc74f558cedac95dc9d3e4de7dd9ce0 Mon Sep 17 00:00:00 2001 From: EtienneLescot Date: Fri, 21 Aug 2026 16:45:55 +0200 Subject: [PATCH 2/2] ci(nix): count run_cli invocations, so the A/B is readable at all The review caught a hole in the protocol, and it is the half I was most confident about. I proposed reading a change in the aggregate sources rate as evidence that position matters. It cannot be: the workload ahead of sources is not fixed. The sandbox probe costs one invocation or two depending on the host, and the record loop breaks on first success, so it costs anywhere between two and six. Sources starts somewhere different on every run, and an average over runs confounds position with whatever happened before it. Counting is the cheap repair. Every attempt now prints which run_cli invocation it is, and the verdict prints the total, so the rates can be stratified by position instead of averaged across it. No wall clock added, and nothing about the test itself changes -- record still runs first, which is the arm that was sound. What this also fixes is the reading. One run was never going to settle this; with labels, several runs add up to something, and without them they would not have. Co-Authored-By: Claude --- .github/workflows/nix-build.yml | 43 +++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/.github/workflows/nix-build.yml b/.github/workflows/nix-build.yml index 8ac18af3e..3bb79037e 100644 --- a/.github/workflows/nix-build.yml +++ b/.github/workflows/nix-build.yml @@ -187,7 +187,19 @@ jobs: # the capture failure remains unexplained. XVFB_SCREEN="-screen 0 1920x1080x24" CHROME_FLAGS="--disable-dev-shm-usage" - run_cli() { timeout --signal=TERM --kill-after=10s "${CLI_TIMEOUT:-120}" xvfb-run -a -s "$XVFB_SCREEN" ./result/bin/openscreen "$@"; } + # Counted, because position is the variable under test and it is not + # fixed. The sandbox probe costs one invocation or two depending on the + # host, and the record loop breaks on first success, so it costs anywhere + # between two and six. An aggregate success rate therefore cannot be + # attributed to position on its own -- the workload ahead of it moved + # too. Labelling every block with the invocation it starts at is what + # makes the rates comparable across runs; reading them unlabelled is the + # mistake this counter exists to prevent. + RUN_CLI_N=0 + run_cli() { + RUN_CLI_N=$((RUN_CLI_N + 1)) + timeout --signal=TERM --kill-after=10s "${CLI_TIMEOUT:-120}" xvfb-run -a -s "$XVFB_SCREEN" ./result/bin/openscreen "$@" + } SANDBOX="" echo "--- probe: as shipped ---" @@ -216,17 +228,28 @@ jobs: # two had was position. run_cli spawns a fresh `xvfb-run -a` per # invocation, so record was always invocations 6-8 of the step, after # five Xvfb servers had been started and torn down, while sources ran at - # 4-8. If display churn is what drives the failures, the rates should now - # swap: record improving AND sources getting worse. If record stays at - # roughly 1 in 10 from this position, position was never the cause and - # something in the record path itself is, which is a different search. + # 4-8. If display churn is what drives the failures, record should now + # improve. If it stays at roughly 1 in 10 from this position, position + # was never the cause and something in the record path itself is, which + # is a different search. # - # Read the next run as that A/B, not as a verdict on the package. Put it + # The other half of that prediction -- sources getting worse -- is NOT + # readable from its aggregate rate, and an earlier draft of this comment + # claimed it was. The workload ahead of sources is not fixed: the sandbox + # probe costs one invocation or two, and the loop below breaks on first + # success, so it costs between two and six. Sources therefore starts at a + # different position on different runs, and a rate averaged over runs + # confounds the two. Hence the run_cli counter: every attempt now prints + # the invocation it is, so the rates can be stratified by position across + # several runs. One run does not settle this; the counter is what makes + # several of them add up. + # + # Read the next runs as that A/B, not as a verdict on the package. Put it # back if the rates do not move. # # Up to three goes, because screen capture on this host is unreliable in # its own right. One success is enough for the question being asked here. - echo "--- record then export ---" + echo "--- record then export (first run_cli here is #$((RUN_CLI_N + 1))) ---" EXPORTED="" # Tracked apart from EXPORTED so the verdict can name the stage that # actually failed. Every run so far has died in record without export @@ -236,7 +259,7 @@ jobs: # to vouch for. RECORDED=0 for i in 1 2 3; do - echo "=== export attempt $i/3 ===" + echo "=== export attempt $i/3 (run_cli #$((RUN_CLI_N + 1))) ===" rm -f /tmp/demo.openscreen /tmp/demo.mp4 RC=0 CLI_TIMEOUT=120 OPENSCREEN_DIAGNOSTIC=1 run_cli $SANDBOX $CHROME_FLAGS record --duration 2 --project /tmp/demo.openscreen >"/tmp/rec.$i.out" 2>&1 || RC=$? @@ -313,7 +336,7 @@ jobs: FAILED=0 LAST_OK="" for i in $(seq 1 "$ATTEMPTS"); do - echo "=== attempt $i/$ATTEMPTS ===" + echo "=== attempt $i/$ATTEMPTS (run_cli #$((RUN_CLI_N + 1))) ===" RC=0 # 120, matching record: the renderer's own bounds can legitimately # spend 35s here (20s on getSources, then up to 3x5s on the audio @@ -400,7 +423,7 @@ jobs: # per-attempt warnings above keep that flakiness visible without letting # it decide the build; tighten this to $ATTEMPTS once the capture failure # is understood and fixed. - echo "=== verdict: enumeration $OK/$ATTEMPTS ok, record $RECORDED, export $EXPORT_OK ===" + echo "=== verdict: enumeration $OK/$ATTEMPTS ok, record $RECORDED, export $EXPORT_OK, $RUN_CLI_N run_cli invocations ===" if [ "$EXPORT_OK" -ne 1 ] || [ "$OK" -eq 0 ]; then exit 1 fi