diff --git a/.github/workflows/nix-build.yml b/.github/workflows/nix-build.yml index 6806ad745..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 ---" @@ -204,6 +216,98 @@ 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, 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. + # + # 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 (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 + # 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 (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=$? + # 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. @@ -232,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 @@ -294,10 +398,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 +409,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 @@ -386,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