diff --git a/apps/desktop/fastlane/Fastfile b/apps/desktop/fastlane/Fastfile index e755a2b7..b5788546 100644 --- a/apps/desktop/fastlane/Fastfile +++ b/apps/desktop/fastlane/Fastfile @@ -31,10 +31,15 @@ end load_local_secrets -def post_release_notes(max_chars:) +# `build` is the macOS build number just uploaded. post-release-notes.mjs needs +# it to target THAT build: macOS and iOS share one App Store Connect app, and iOS +# builds also carry the macOS-only fields the script used to match on. +def post_release_notes(max_chars:, build:) Dir.chdir(PROJECT_ROOT) do notes = sh("bash", "scripts/generate-release-notes.sh", "--max-chars", max_chars.to_s).strip - sh("node", "scripts/post-release-notes.mjs", "--platform", "macos", "--notes", notes) unless notes.empty? + next if notes.empty? + + sh("node", "scripts/post-release-notes.mjs", "--platform", "macos", "--notes", notes, "--build", build.to_s) end rescue StandardError => e UI.important("Release notes posting failed (non-fatal): #{e.message}") @@ -291,7 +296,12 @@ platform :mac do upload_to_testflight( api_key: api_key, pkg: pkg_path, - skip_waiting_for_build_processing: false + # Don't block on Apple's processing (matches mobile). Waiting hung the + # #623 pre-merge lane for 3 days: build 56 went VALID in App Store + # Connect, but fastlane's poll never saw it, so the lane never exited + # and the factory never announced the build. post-release-notes.mjs + # polls for the exact build itself. + skip_waiting_for_build_processing: true ) else upload_to_app_store( @@ -304,7 +314,7 @@ platform :mac do end # Post release notes - post_release_notes(max_chars: 4000) + post_release_notes(max_chars: 4000, build: new_build_number) # Phase G: comment "Now live" on closed support issues for this build. # TestFlight (beta) and the App Store (production) ship the same macOS diff --git a/apps/desktop/scripts/post-release-notes.mjs b/apps/desktop/scripts/post-release-notes.mjs index 388ee532..228d177e 100644 --- a/apps/desktop/scripts/post-release-notes.mjs +++ b/apps/desktop/scripts/post-release-notes.mjs @@ -3,7 +3,13 @@ * Post release notes to App Store Connect (TestFlight) for the macOS desktop app. * * Usage: - * node post-release-notes.mjs --platform macos --notes "Release notes text" + * node post-release-notes.mjs --platform macos --notes "Release notes text" --build N + * + * --build is the macOS build number just uploaded. It is REQUIRED: macOS and iOS + * ship under the same App Store Connect app (eu.drafto.mobile), and iOS builds + * ALSO carry computedMinMacOsVersion (iPhone apps run on Apple Silicon Macs), so + * "the newest build with macOS fields" resolved to iOS build 42 and overwrote + * its notes. Match the exact number on the MAC_OS preReleaseVersion instead. * * Environment variables: * ASC_API_KEY_ID - App Store Connect API Key ID @@ -14,14 +20,17 @@ import { createSign } from "node:crypto"; import { readFileSync } from "node:fs"; +import { pathToFileURL } from "node:url"; -const args = process.argv.slice(2); -const notesIdx = args.indexOf("--notes"); -const notes = notesIdx !== -1 ? args[notesIdx + 1] : ""; - -if (!notes) { - console.error("Error: --notes is required"); - process.exit(1); +export function parseArgs(argv) { + // Return the token after `flag`, but treat a missing value or the next flag as + // absent — otherwise `--notes --build 29` would swallow `--build` as the notes. + const valueAfter = (flag) => { + const index = argv.indexOf(flag); + const value = index === -1 ? undefined : argv[index + 1]; + return value && !value.startsWith("--") ? value : ""; + }; + return { notes: valueAfter("--notes"), build: valueAfter("--build") }; } // --- App Store Connect --- @@ -98,7 +107,52 @@ export const ascFetch = async (url, options = {}) => { throw lastError; }; -async function postTestFlightNotes(releaseNotes) { +// CFBundleVersion: one to three period-separated integers. Checked up front so a +// typo fails fast instead of polling App Store Connect for 5 minutes. +export const isValidBuildNumber = (build) => /^\d+(\.\d+){0,2}$/.test(String(build)); + +/** + * Flatten an App Store Connect `/builds` response (data + included) into + * `{ id, version, uploadedDate, platform }` records. `platform` comes ONLY from + * the build's preReleaseVersion ("IOS" | "MAC_OS"). The macOS-only build fields + * are not a usable fallback here: iOS builds report computedMinMacOsVersion too. + */ +export function normalizeBuilds(buildsResponse) { + const preReleaseById = new Map( + (buildsResponse.included || []) + .filter((item) => item.type === "preReleaseVersions") + .map((item) => [item.id, item.attributes]), + ); + return (buildsResponse.data || []).map((build) => { + const preReleaseId = build.relationships?.preReleaseVersion?.data?.id; + return { + id: build.id, + version: build.attributes?.version, + uploadedDate: build.attributes?.uploadedDate, + platform: preReleaseId ? preReleaseById.get(preReleaseId)?.platform : undefined, + }; + }); +} + +/** + * Pick the macOS build with exactly `buildNumber`. Build numbers are not unique + * across platforms (iOS and macOS count independently), so filter to MAC_OS + * first; among survivors the most recently uploaded wins. + */ +export function selectMacBuild(builds, { buildNumber }) { + const candidates = builds.filter( + (build) => build.platform === "MAC_OS" && String(build.version) === String(buildNumber), + ); + if (candidates.length === 0) { + return null; + } + const uploadedAt = (build) => (build.uploadedDate ? Date.parse(build.uploadedDate) : 0); + return candidates.reduce((newest, build) => + uploadedAt(build) > uploadedAt(newest) ? build : newest, + ); +} + +async function postTestFlightNotes(releaseNotes, buildNumber) { const keyId = process.env.ASC_API_KEY_ID; const issuerId = process.env.ASC_API_ISSUER_ID; const privateKeyP8 = @@ -132,34 +186,41 @@ async function postTestFlightNotes(releaseNotes) { "Content-Type": "application/json", }; - // 1. Find the latest macOS build (filter by platform-identifying fields for multi-platform app). - // Uses the top-level /v1/builds endpoint because the relationship endpoint /v1/apps/{id}/builds - // no longer accepts the `sort` query parameter. - const buildsRes = await ascFetch( - `${baseUrl}/builds?filter[app]=${appId}&sort=-uploadedDate&limit=10&fields[builds]=version,processingState,computedMinMacOsVersion,lsMinimumSystemVersion`, - { headers }, - ); - if (!buildsRes.ok) { - throw new Error(`List builds failed: ${buildsRes.status} ${await buildsRes.text()}`); - } - const buildsData = await buildsRes.json(); - - if (!buildsData.data || buildsData.data.length === 0) { - console.error("Skipping TestFlight: no builds found"); - return; + // 1. Find the exact macOS build. The lane uploads with + // skip_waiting_for_build_processing, so a fresh build may not be indexed yet — + // poll until it appears. filter[version] rather than sort-by-uploadedDate: a + // still-processing build has a null uploadedDate and sorts last. + // `preReleaseVersion` MUST be in fields[builds] or ASC omits the relationship + // linkage and the platform can't be resolved from the include. + const buildsUrl = + `${baseUrl}/builds?filter[app]=${appId}&filter[version]=${encodeURIComponent(buildNumber)}` + + `&fields[builds]=version,uploadedDate,processingState,preReleaseVersion` + + `&include=preReleaseVersion&fields[preReleaseVersions]=platform`; + const maxAttempts = 15; + let target = null; + for (let attempt = 1; attempt <= maxAttempts; attempt++) { + const buildsRes = await ascFetch(buildsUrl, { headers }); + if (!buildsRes.ok) { + throw new Error(`List builds failed: ${buildsRes.status} ${await buildsRes.text()}`); + } + target = selectMacBuild(normalizeBuilds(await buildsRes.json()), { buildNumber }); + if (target || attempt === maxAttempts) { + break; + } + console.log( + `TestFlight: macOS build ${buildNumber} not indexed yet (attempt ${attempt}/${maxAttempts}); retrying in 20s…`, + ); + await sleep(20_000); } - // Filter to macOS builds only (have computedMinMacOsVersion or lsMinimumSystemVersion) - const macosBuild = buildsData.data.find( - (b) => b.attributes.computedMinMacOsVersion || b.attributes.lsMinimumSystemVersion, - ); - - if (!macosBuild) { - console.error("Skipping TestFlight: no macOS builds found (only iOS builds present)"); + if (!target) { + console.error( + `Skipping TestFlight: macOS build ${buildNumber} not found for app ${appId} after ${maxAttempts} attempts`, + ); return; } - const buildId = macosBuild.id; + const buildId = target.id; // 2. Check if a betaBuildLocalization already exists for en-US const locRes = await ascFetch( @@ -208,20 +269,34 @@ async function postTestFlightNotes(releaseNotes) { } } - console.log( - `TestFlight: "What to Test" updated for macOS build ${macosBuild.attributes.version}`, - ); + console.log(`TestFlight: "What to Test" updated for macOS build ${target.version}`); } // --- Main --- async function main() { + const { notes, build } = parseArgs(process.argv.slice(2)); + if (!notes) { + console.error("Error: --notes is required"); + process.exit(1); + } + if (!build) { + console.error("Error: --build is required so notes target THAT macOS build"); + process.exit(1); + } + if (!isValidBuildNumber(build)) { + console.error(`Error: --build "${build}" is not a CFBundleVersion (e.g. 56 or 1.2.3)`); + process.exit(1); + } try { - await postTestFlightNotes(notes); + await postTestFlightNotes(notes, build); } catch (err) { console.error(`TestFlight error: ${err.message}`); process.exit(1); } } -main(); +// Only run when invoked directly (not when imported by tests). +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + main(); +} diff --git a/docs/operations/factory-runbook.md b/docs/operations/factory-runbook.md index 64101d89..7e953fc8 100644 --- a/docs/operations/factory-runbook.md +++ b/docs/operations/factory-runbook.md @@ -164,6 +164,7 @@ A card in **In Test** needs a build a human can actually install. `--watch` ther | `FACTORY_INTEST_BETA_DESKTOP` | `0` | Additionally dispatch the **macOS** lane. Only turn on after the fossil validation below. | | `FACTORY_INTEST_TIMEOUT_SEC` | `600` | Wall-clock cap for the In Test scenario writer (read-only stage). | | `FACTORY_LANE_STALE_MIN` | `120` | A lane silent this long with no exit code is declared dead and retried. | +| `FACTORY_LANE_MAX_MIN` | `240` | A lane still running this long after ITS dispatch is killed (whole process group) and retried. | | `FACTORY_LANE_MAX_ATTEMPTS` | `3` | Retry budget per lane per commit; a new commit resets it. | | `DRAFTO_BETA_MOBILE_ROOT` | `/Users/jakub/code/drafto-beta-mobile` | Dedicated mobile build root. | | `DRAFTO_DESKTOP_BUILD_ROOT` | `/Users/jakub/code/drafto-beta-desktop` | Dedicated macOS build root (clonefile replica of the fossil). | @@ -233,6 +234,7 @@ cat logs/factory/beta-lane-mobile-463-b243d8196fa2.log.exit # its exit cod | absent, log written recently | still building | left alone | | absent, log silent > `FACTORY_LANE_STALE_MIN` | wrapper killed before it could record | treated as failed, retried | | absent, **no log at all**, dispatched > `FACTORY_LANE_STALE_MIN` | killed before it could even open its log | treated as failed, retried (judged against `intestBetaAt`) | +| absent, log active, dispatched > `FACTORY_LANE_MAX_MIN` | hung but chatty (e.g. a stuck poll loop) | the lane's process group is killed (TERM, then KILL); treated as failed, retried | | present but empty / non-numeric | caught mid-write | left alone; the staleness rows above are the backstop | | `0` | succeeded | stays suppressed; if a failure was announced for this lane+commit, a retraction is posted once (``) | | non-zero, attempt < `FACTORY_LANE_MAX_ATTEMPTS` | failed, budget remaining | dropped from `intestBetaLanes`, retried next tick, reported once (``) | diff --git a/scripts/__tests__/factory-agent-intest.test.mjs b/scripts/__tests__/factory-agent-intest.test.mjs index f8255f1f..2d61d5e2 100644 --- a/scripts/__tests__/factory-agent-intest.test.mjs +++ b/scripts/__tests__/factory-agent-intest.test.mjs @@ -600,6 +600,8 @@ describe("intest_check_lane_outcomes (extracted, real bash)", () => { attempt = 1, decoys = null, markers = [], + holdLog = false, + laneStartedAgoMin = null, }) => { const dir = mkdtempSync(join(tmpdir(), "drafto-outcome-")); const stateFile = join(dir, "state.json"); @@ -611,6 +613,11 @@ describe("intest_check_lane_outcomes (extracted, real bash)", () => { intestBetaSha: stateSha, intestBetaLanes: lanes, intestBetaAttempts: attempts, + ...(laneStartedAgoMin === null + ? {} + : { + intestBetaLaneAt: `mobile:${Math.floor(Date.now() / 1000) - laneStartedAgoMin * 60}`, + }), ...(dispatchedAgoMin === null ? {} : { @@ -635,8 +642,23 @@ set -uo pipefail eval "$(awk '/^iso_age_min\(\)/{f=1} f{print} f&&/^}/{exit}' "${agentPath}")" eval "$(awk '/^lane_attempt_of\(\)/{f=1} f{print} f&&/^}/{exit}' "${agentPath}")" eval "$(awk '/^lane_attempt_set\(\)/{f=1} f{print} f&&/^}/{exit}' "${agentPath}")" +eval "$(awk '/^kill_lane_holding_log\(\)/{f=1} f{print} f&&/^}/{exit}' "${agentPath}")" eval "$(awk '/^intest_check_lane_outcomes\(\)/{f=1} f{print} f&&/^}/{exit}' "${agentPath}")" log() { echo "[log] $*"; } +# A stand-in for a hung lane, in its OWN process group as dispatch-release.mjs's +# detached spawn gives it (set -m). The leader holds this attempt's log; its +# child does not, like xcodebuild whose output fastlane pipes back to ruby. +HELD_PID= KID_PID= +${ + holdLog + ? `set -m +bash -c 'sleep 30 >/dev/null 2>&1 & echo $! >"$0"; exec sleep 30' "${base}.kid" >>"${base}" & +HELD_PID=$! +set +m +sleep 0.3 +KID_PID=$(cat "${base}.kid")` + : "" +} # Models markers already on the issue ($2 is the marker). Default: none present. issue_has_marker() { case "$2" in @@ -655,10 +677,17 @@ STATE_FILE=${JSON.stringify(stateFile)} LOG_DIR=${JSON.stringify(dir)} LOG_FILE=${JSON.stringify(join(dir, "agent.log"))} FACTORY_LANE_STALE_MIN=120 +FACTORY_LANE_MAX_MIN=240 +FACTORY_LANE_KILL_GRACE_SEC=1 FACTORY_LANE_MAX_ATTEMPTS=3 FACTORY_INTEST_BETA=1 DRY_RUN=${dryRun} intest_check_lane_outcomes 463 ${JSON.stringify(sha)} 591 +if [[ -n "$HELD_PID" ]]; then + sleep 0.3 + if kill -0 "$HELD_PID" 2>/dev/null; then echo "HELD_ALIVE"; kill "$HELD_PID"; else echo "HELD_DEAD"; fi + if kill -0 "$KID_PID" 2>/dev/null; then echo "KID_ALIVE"; kill "$KID_PID"; else echo "KID_DEAD"; fi +fi cat "$STATE_FILE" `; const r = spawnSync("bash", ["-c", snippet], { encoding: "utf8" }); @@ -777,6 +806,74 @@ cat "$STATE_FILE" rmSync(r.dir, { recursive: true, force: true }); }); + it("kills and re-arms a lane that keeps logging past the wall-clock cap", () => { + // #623's desktop lane logged "Waiting for App Store Connect to finish + // processing" every 32 s for 3 days. Its log never went silent, so the + // silence check never fired and it held the desktop build root throughout. + const r = run({ exitContent: undefined, laneStartedAgoMin: 300, holdLog: true, dryRun: 0 }); + assert.equal(r.status, 0, r.stderr); + assert.match(r.stdout, /still running 240\+ min after dispatch/); + assert.match(r.stdout, /HELD_DEAD/, "the process holding the lane's log must be terminated"); + assert.match( + r.stdout, + /KID_DEAD/, + "its piped children must die too, or they outlive the retry", + ); + const state = JSON.parse(readFileSync(r.stateFile, "utf8")); + assert.ok(!state.issues["463"].intestBetaLanes, "a hung lane must be re-armed"); + rmSync(r.dir, { recursive: true, force: true }); + }); + + it("leaves a chatty lane alone while it is under the wall-clock cap", () => { + const r = run({ exitContent: undefined, laneStartedAgoMin: 30, holdLog: true, dryRun: 0 }); + assert.equal(r.status, 0, r.stderr); + assert.ok(!/re-arming/.test(r.stdout), "a 30-minute-old lane is still building"); + assert.match(r.stdout, /HELD_ALIVE/, "a healthy lane must not be killed"); + rmSync(r.dir, { recursive: true, force: true }); + }); + + it("clocks the cap per lane, so a sibling's re-dispatch cannot reset it", () => { + // intestBetaAt is shared and restamped by every dispatch. A mobile retry 5 + // minutes ago must not buy a desktop lane hung for 300 minutes more time. + const r = run({ + exitContent: undefined, + dispatchedAgoMin: 5, + laneStartedAgoMin: 300, + holdLog: true, + dryRun: 0, + }); + assert.equal(r.status, 0, r.stderr); + assert.match(r.stdout, /HELD_DEAD/); + rmSync(r.dir, { recursive: true, force: true }); + }); + + it("does not kill a young lane just because the shared dispatch stamp is old", () => { + const r = run({ + exitContent: undefined, + dispatchedAgoMin: 300, + laneStartedAgoMin: 10, + holdLog: true, + dryRun: 0, + }); + assert.equal(r.status, 0, r.stderr); + assert.match(r.stdout, /HELD_ALIVE/); + rmSync(r.dir, { recursive: true, force: true }); + }); + + it("falls back to the shared dispatch stamp for state predating intestBetaLaneAt", () => { + const r = run({ exitContent: undefined, dispatchedAgoMin: 300, holdLog: true, dryRun: 0 }); + assert.equal(r.status, 0, r.stderr); + assert.match(r.stdout, /HELD_DEAD/); + rmSync(r.dir, { recursive: true, force: true }); + }); + + it("does not kill anything on a dry run", () => { + const r = run({ exitContent: undefined, laneStartedAgoMin: 300, holdLog: true, dryRun: 1 }); + assert.equal(r.status, 0, r.stderr); + assert.match(r.stdout, /HELD_ALIVE/, "DRY_RUN must never terminate a process"); + rmSync(r.dir, { recursive: true, force: true }); + }); + it("declares a lane dead when it left NO log and NO exit code (openSync fell back)", () => { // Log missing AND wrapper killed: without the intestBetaAt fallback this // lane would be "still building" for ever — the same silent non-delivery @@ -789,6 +886,21 @@ cat "$STATE_FILE" rmSync(r.dir, { recursive: true, force: true }); }); + it("uses the per-lane clock for a traceless lane too", () => { + // A sibling's re-dispatch restamps the shared intestBetaAt; it must not + // keep a lane with no log and no exit code alive for ever. + const r = run({ + exitContent: undefined, + writeLog: false, + dispatchedAgoMin: 5, + laneStartedAgoMin: 300, + dryRun: 0, + }); + assert.equal(r.status, 0, r.stderr); + assert.match(r.stdout, /left no log and no exit code/); + rmSync(r.dir, { recursive: true, force: true }); + }); + it("does NOT declare a traceless lane dead while it is still young", () => { const r = run({ exitContent: undefined, writeLog: false, dispatchedAgoMin: 5, dryRun: 0 }); assert.equal(r.status, 0, r.stderr); diff --git a/scripts/__tests__/post-release-notes-select.test.mjs b/scripts/__tests__/post-release-notes-select.test.mjs index b115d1ce..a742bfa3 100644 --- a/scripts/__tests__/post-release-notes-select.test.mjs +++ b/scripts/__tests__/post-release-notes-select.test.mjs @@ -356,3 +356,76 @@ describe("mobile/desktop mirror invariant", () => { }); } }); + +describe("desktop post-release-notes build selection", () => { + // Live shape that broke it: iOS build 42 reports computedMinMacOsVersion + // (iPhone apps run on Apple Silicon Macs) and was uploaded AFTER macOS 56, so + // "newest build with macOS fields" picked iOS 42 and overwrote its notes. + const response = { + data: [ + { + id: "build-ios-56-old", + attributes: { + version: "56", + // Uploaded AFTER the macOS build, so a selector that ignored platform + // and took the newest exact match would pick this one. + uploadedDate: "2026-09-16T10:00:00-07:00", + computedMinMacOsVersion: "11.0", + }, + relationships: { preReleaseVersion: { data: { id: "pr-ios" } } }, + }, + { + id: "build-mac-56", + attributes: { version: "56", uploadedDate: "2026-09-15T05:18:13-07:00" }, + relationships: { preReleaseVersion: { data: { id: "pr-mac" } } }, + }, + ], + included: [ + { type: "preReleaseVersions", id: "pr-mac", attributes: { platform: "MAC_OS" } }, + { type: "preReleaseVersions", id: "pr-ios", attributes: { platform: "IOS" } }, + ], + }; + + it("picks the MAC_OS build with the exact number, never an iOS one", async () => { + const { normalizeBuilds: norm, selectMacBuild } = + await import("../../apps/desktop/scripts/post-release-notes.mjs"); + assert.equal(selectMacBuild(norm(response), { buildNumber: 56 })?.id, "build-mac-56"); + }); + + it("does not treat macOS-only build fields as proof of a macOS build", async () => { + const { normalizeBuilds: norm, selectMacBuild } = + await import("../../apps/desktop/scripts/post-release-notes.mjs"); + const iosOnly = { data: [response.data[0]], included: response.included }; + assert.equal(selectMacBuild(norm(iosOnly), { buildNumber: "56" }), null); + const noLinkage = { data: [{ ...response.data[0], relationships: {} }] }; + assert.equal(selectMacBuild(norm(noLinkage), { buildNumber: "56" }), null); + }); + + it("parses --notes and --build without swallowing a flag as a value", async () => { + const { parseArgs: parse } = await import("../../apps/desktop/scripts/post-release-notes.mjs"); + assert.deepEqual(parse(["--platform", "macos", "--notes", "hi", "--build", "56"]), { + notes: "hi", + build: "56", + }); + assert.deepEqual(parse(["--notes", "--build", "56"]), { notes: "", build: "56" }); + }); + + it("accepts only CFBundleVersion-shaped build numbers", async () => { + const { isValidBuildNumber } = + await import("../../apps/desktop/scripts/post-release-notes.mjs"); + for (const ok of ["0", "56", "1.2", "10.14.1"]) assert.ok(isValidBuildNumber(ok), ok); + for (const bad of ["abc", "-1", "1.", ".1", "1.2.3.4", "1..2", " 56"]) { + assert.ok(!isValidBuildNumber(bad), bad); + } + }); + + it("the desktop lane does not block on App Store processing and passes --build", () => { + // Blocking on processing hung #623's lane for 3 days after build 56 was + // already VALID. Mobile has always skipped the wait. + const src = readFileSync(resolve(HERE, "..", "..", "apps/desktop/fastlane/Fastfile"), "utf8"); + // Require an explicit true: dropping the option restores fastlane's blocking default. + assert.match(src, /skip_waiting_for_build_processing:\s*true/); + assert.match(src, /post_release_notes\(max_chars: 4000, build: new_build_number\)/); + assert.match(src, /"--build", build\.to_s/); + }); +}); diff --git a/scripts/factory-agent.sh b/scripts/factory-agent.sh index 297095ba..5548c26b 100755 --- a/scripts/factory-agent.sh +++ b/scripts/factory-agent.sh @@ -1670,6 +1670,37 @@ if ! [[ "$FACTORY_LANE_STALE_MIN" =~ ^[1-9][0-9]*$ ]]; then FACTORY_LANE_STALE_MIN=120 fi +# Hard wall-clock cap on one lane, however much it logs (see +# intest_check_lane_outcomes). Observed builds take 4-40 min. +FACTORY_LANE_MAX_MIN="${FACTORY_LANE_MAX_MIN:-240}" +if ! [[ "$FACTORY_LANE_MAX_MIN" =~ ^[1-9][0-9]*$ ]]; then + echo "WARNING: invalid FACTORY_LANE_MAX_MIN='$FACTORY_LANE_MAX_MIN'; defaulting to 240" >&2 + FACTORY_LANE_MAX_MIN=240 +fi + +# Terminate a hung lane: its whole process GROUP. dispatch-release.mjs spawns +# each lane detached, so the wrapper leads its own group. That group holds +# fastlane's piped children too (xcodebuild, productbuild, the uploader), which +# never hold the log, and they must die before a retry resets the build root +# under them. The group is found through THIS attempt's log, which only this +# lane holds open, so it can never reach another card's lane. The per-root +# .lock pid could: it belongs to whoever claimed the root last. TERM first, +# then KILL whatever ignored it. +kill_lane_holding_log() { + local log_path="$1" pid pgid pgids="" + for pid in $(lsof -t -- "$log_path" 2>/dev/null || true); do + pgid=$(ps -o pgid= -p "$pid" 2>/dev/null | tr -d ' ' || true) + # Never signal group 0/1 or our own group: that would take down the factory. + [[ "$pgid" =~ ^[0-9]+$ && "$pgid" -gt 1 && "$pgid" != "$(ps -o pgid= -p $$ | tr -d ' ')" ]] || continue + [[ " $pgids " == *" $pgid "* ]] || pgids="$pgids $pgid" + done + [[ -n "${pgids// /}" ]] || return 0 + log "Killing hung lane process group(s) holding $log_path:$pgids" + for pgid in $pgids; do kill -TERM -- "-$pgid" 2>/dev/null || true; done + sleep "${FACTORY_LANE_KILL_GRACE_SEC:-10}" + for pgid in $pgids; do kill -KILL -- "-$pgid" 2>/dev/null || true; done +} + # Whole minutes since an ISO-8601 UTC timestamp, or a huge number if it can't be # parsed (an unparseable stamp must not read as "just dispatched" and suppress a # lane for ever). BSD date on macOS; -j -f parses rather than sets. @@ -1716,7 +1747,7 @@ intest_check_lane_outcomes() { local issue_num="$1" sha="$2" pr_num="${3:-}" local state_json prior_sha prior_lanes prior_attempts dispatched_at lane exit_file local log_file_path code lane_attempt kept="" changed=0 give_up - local reason new_attempts attempts_changed=0 + local reason new_attempts attempts_changed=0 lane_at_csv lane_started lane_age_min state_json=$(node "$SCRIPT_DIR/lib/state-cli.mjs" factory:get-issue "$issue_num" \ --state-file "$STATE_FILE" 2>>"$LOG_FILE" || echo "{}") @@ -1724,6 +1755,7 @@ intest_check_lane_outcomes() { prior_lanes=$(echo "$state_json" | jq -r '.intestBetaLanes // ""' 2>/dev/null || echo "") dispatched_at=$(echo "$state_json" | jq -r '.intestBetaAt // ""' 2>/dev/null || echo "") prior_attempts=$(echo "$state_json" | jq -r '.intestBetaAttempts // ""' 2>/dev/null || echo "") + lane_at_csv=$(echo "$state_json" | jq -r '.intestBetaLaneAt // ""' 2>/dev/null || echo "") new_attempts="$prior_attempts" # Only meaningful for the commit currently under test. [[ -n "$sha" && "$prior_sha" == "$sha" && -n "$prior_lanes" ]] || return 0 @@ -1750,6 +1782,17 @@ intest_check_lane_outcomes() { log_file_path="$LOG_DIR/beta-lane-${lane}-${issue_num}-${sha:0:12}-a${lane_attempt}.log" exit_file="${log_file_path}.exit" reason="" + # This lane's own age. intestBetaAt is shared and reset by any lane's + # re-dispatch, so it is only the fallback for state written before + # intestBetaLaneAt existed. + lane_started=$(lane_attempt_of "$lane_at_csv" "$lane") + if [[ "$lane_started" =~ ^[1-9][0-9]*$ ]]; then + lane_age_min=$(( ($(date +%s) - lane_started) / 60 )) + elif [[ -n "$dispatched_at" ]]; then + lane_age_min=$(iso_age_min "$dispatched_at") + else + lane_age_min=0 + fi if [[ ! -f "$exit_file" ]]; then # No outcome yet. Still building, or dead without a trace? # @@ -1759,14 +1802,27 @@ intest_check_lane_outcomes() { # openSync fell back to "ignore"), fall back to how long ago the dispatch # was recorded; otherwise a lane with neither artefact would be suppressed # for ever — the exact silent non-delivery this mechanism exists to end. + # + # Silence alone misses a lane that is alive but stuck in a loop that + # keeps logging: #623's desktop lane logged "Waiting for App Store Connect + # to finish processing" every 32 s for 3 days, so its log never went + # stale. It held the desktop build root the whole time. So a lane still + # running FACTORY_LANE_MAX_MIN after dispatch is killed and counts as + # failed, however chatty it is. if [[ -f "$log_file_path" ]]; then if [[ -n "$(find "$log_file_path" -mmin "+$FACTORY_LANE_STALE_MIN" 2>/dev/null)" ]]; then + # Usually already dead, but a silent group that is still alive would + # hold the root lock, or outlive a retry that resets the root under it. + [[ "$DRY_RUN" -eq 0 ]] && kill_lane_holding_log "$log_file_path" reason="produced no output for over ${FACTORY_LANE_STALE_MIN} min and never recorded an exit code (killed?)" + elif [[ "$lane_age_min" -gt "$FACTORY_LANE_MAX_MIN" ]]; then + [[ "$DRY_RUN" -eq 0 ]] && kill_lane_holding_log "$log_file_path" + reason="was still running ${FACTORY_LANE_MAX_MIN}+ min after dispatch with no exit code (hung; killed)" else kept="${kept:+$kept,}$lane" # still building continue fi - elif [[ -n "$dispatched_at" ]] && [[ "$(iso_age_min "$dispatched_at")" -gt "$FACTORY_LANE_STALE_MIN" ]]; then + elif [[ -n "$lane_started$dispatched_at" ]] && [[ "$lane_age_min" -gt "$FACTORY_LANE_STALE_MIN" ]]; then reason="left no log and no exit code ${FACTORY_LANE_STALE_MIN}+ min after dispatch (killed before it could write?)" else kept="${kept:+$kept,}$lane" # too early to call @@ -1909,7 +1965,7 @@ intest_dispatch_betas() { # and later failed (see intest_check_lane_outcomes, which removes it), is # re-dispatched while a healthy sibling is left alone. A single shared SHA # used to suppress both: mobile succeeding hid a desktop failure entirely. - local prior_lanes="" prior_attempts="" state_json="" want to_dispatch="" new_attempts="" + local prior_lanes="" prior_attempts="" state_json="" want to_dispatch="" new_attempts="" new_lane_at="" local lane_attempt log_key state_json=$(node "$SCRIPT_DIR/lib/state-cli.mjs" factory:get-issue "$issue_num" \ --state-file "$STATE_FILE" 2>>"$LOG_FILE" || echo "{}") @@ -2025,6 +2081,15 @@ intest_dispatch_betas() { done node "$SCRIPT_DIR/lib/state-cli.mjs" factory:set-issue-field "$issue_num" \ intestBetaAttempts "$new_attempts" --state-file "$STATE_FILE" >>"$LOG_FILE" 2>&1 || true + # Per-lane start time, for the FACTORY_LANE_MAX_MIN cap. Only the lanes + # started NOW are stamped; a sibling still building keeps its own clock. + new_lane_at=$(echo "$state_json" | jq -r '.intestBetaLaneAt // ""' 2>/dev/null || echo "") + for want in $(echo "$confirmed_csv" | tr ',' ' '); do + [[ -n "$want" ]] || continue + new_lane_at=$(lane_attempt_set "$new_lane_at" "$want" "$(date +%s)") + done + node "$SCRIPT_DIR/lib/state-cli.mjs" factory:set-issue-field "$issue_num" \ + intestBetaLaneAt "$new_lane_at" --state-file "$STATE_FILE" >>"$LOG_FILE" 2>&1 || true else # Nothing started — deliberately record NO SHA, so the next tick retries. logerr "WARNING: pre-merge beta dispatch started no lanes for #$issue_num; leaving retry armed" diff --git a/scripts/lib/factory-state.mjs b/scripts/lib/factory-state.mjs index 1032e006..29d388e5 100644 --- a/scripts/lib/factory-state.mjs +++ b/scripts/lib/factory-state.mjs @@ -111,6 +111,10 @@ function emptyIssue() { // cap and the per-ATTEMPT artefact path — keying the log/.exit on the // commit alone let a retry share files with the attempt it replaced. intestBetaAttempts: null, + // Per-lane dispatch time, "mobile:,desktop:". Clocks the + // FACTORY_LANE_MAX_MIN cap per lane: intestBetaAt is shared and reset by + // any lane's re-dispatch, which would let a hung sibling outlive the cap. + intestBetaLaneAt: null, // Head SHA the code-review stage last ran against, so --watch reviews each // commit exactly once. Re-armed by every new push, which is what makes a // fix-then-re-review cycle converge instead of repeating. See ADR-0035. @@ -412,6 +416,7 @@ const MUTABLE_ISSUE_FIELDS = new Set([ "intestBetaAt", "intestBetaLanes", "intestBetaAttempts", + "intestBetaLaneAt", // Code-review stage idempotency key (ADR-0035): the head SHA the review stage // last ran against. Same silent-failure hazard as the In Test keys above — a // rejected field would make --watch re-review (and re-comment on) every tick. diff --git a/scripts/lib/state-cli.mjs b/scripts/lib/state-cli.mjs index a3a2ed6b..7b23ae89 100644 --- a/scripts/lib/state-cli.mjs +++ b/scripts/lib/state-cli.mjs @@ -103,6 +103,7 @@ // lastFeedbackAt, intestCommentSha, // intestBetaSha, intestBetaAt, // intestBetaLanes, intestBetaAttempts, +// intestBetaLaneAt, // lastReviewSha, crConvergedSha, // crConvergedAt, crCoverageSha, crCoverage, // crLastCoveredSha, crCliRuns,