From b7830b88a8db21f316faabdf18cd038f9e94dbeb Mon Sep 17 00:00:00 2001 From: Stuart Date: Mon, 28 Sep 2026 16:46:17 +1000 Subject: [PATCH 1/3] fix(deploy): keep gateway 5xx, repeated sightings and new deploys truthful A 5xx from the provisioning endpoint no longer classifies as a definitive pre-write refusal: a gateway can answer 502/503/504 after forwarding the request, so the outcome stays UNCONFIRMED and the possibly-written files stay dirty until an export proves them (only 4xx is still FAILED). An export sighting already explained by a verified write is spent evidence: a second export of the same unchanged content can no longer retire an earlier uncertain write whose identical content may still land, so the snapshot stays uncached until that attempt reports its own result. A new deploy starting inside the terminal state's 1s hide delay cancels the previous deploy's hide timer instead of having its progress overlay closed by it. --- app.js | 46 ++++++++++---- e2e/stu380-deploy.spec.js | 119 ++++++++++++++++++++++++++++++++++-- src/provisionStream.js | 35 +++++++---- src/provisionStream.test.js | 37 ++++++++--- 4 files changed, 200 insertions(+), 37 deletions(-) diff --git a/app.js b/app.js index 75ddcf0..4d811d6 100644 --- a/app.js +++ b/app.js @@ -2671,14 +2671,16 @@ function getFileNameFromPath(filePath) { const projectSourceCache = new Map(); // File writes whose provisioning outcome is unconfirmed, per cache key — -// each dirty path maps content -> { uncertain, verified }: `uncertain` counts -// provision attempts that may still write that content (a lost/dropped -// request is not proof the runner stopped), and `verified` credits confirmed -// writes an export sighting must consume first. An export showing content X -// can retire an uncertain X-write only when no verified X-write explains the -// observation — so a confirmed retry can never clear an earlier attempt whose -// identical write may still land over a newer edit. While any entry is -// outstanding, fresh exports are never cached. +// each dirty path maps content -> { uncertain, verified, explainedByVerified }: +// `uncertain` counts provision attempts that may still write that content (a +// lost/dropped request is not proof the runner stopped), and `verified` +// credits confirmed writes an export sighting must consume first. A sighting +// of content X consumes a verified X-credit before it can retire an uncertain +// X-write, and once a verified X-write has explained an X-sighting, +// `explainedByVerified` marks the sighting as spent evidence: a later export +// of the same unchanged X is the same fact, not a new landing, so it can never +// retire an uncertain X-write whose identical content may still land. While +// any entry is outstanding, fresh exports are never cached. const projectSourceDirtyWrites = new Map(); // Unconfirmed sync pushes whose underlying request is still in flight, per @@ -3078,14 +3080,21 @@ async function resolveProjectPubspec(apiClient, newDependencies = {}) { // This fresh export is the authoritative read: an observed content // first consumes a verified write credit (the sighting is explained by // a confirmed provision), and only retires an uncertain write when no - // verified write could have produced it. + // verified write could have produced it. A sighting explained by a + // verified write is spent evidence — a later export of the same + // unchanged content is the same fact, so it must not retire the + // uncertain write whose identical content may still land. for (const [path, contents] of [...dirty]) { const observed = projectSource.files.get(path); if (observed !== undefined) { const record = contents.get(observed); if (record) { - if (record.verified > 0) record.verified -= 1; - else record.uncertain -= 1; + if (record.verified > 0) { + record.verified -= 1; + record.explainedByVerified = true; + } else if (!record.explainedByVerified) { + record.uncertain -= 1; + } if (record.uncertain <= 0 && record.verified <= 0) { contents.delete(observed); } @@ -6729,6 +6738,11 @@ document.addEventListener("DOMContentLoaded", async () => { } + // The terminal state hides the overlay one second later, but a new deploy + // can start inside that window: its progress must not be closed by the + // previous deploy's timer. + let commitProgressHideTimer = null; + window.addEventListener("commitStateChange", (event) => { const { state } = event.detail; @@ -6737,13 +6751,21 @@ document.addEventListener("DOMContentLoaded", async () => { state === CommitState.VALIDATING || state === CommitState.PUSHING ) { + if (commitProgressHideTimer) { + clearTimeout(commitProgressHideTimer); + commitProgressHideTimer = null; + } // Only open the overlay for flows that didn't open it themselves; // re-opening mid-commit would reset the progress back to step one. if (!commitProgress.phaseId) showCommitProgress(); updateProgressFromState(state); } else if (state === CommitState.SUCCESS || state === CommitState.ERROR) { updateProgressFromState(state); - setTimeout(hideCommitProgress, 1000); + if (commitProgressHideTimer) clearTimeout(commitProgressHideTimer); + commitProgressHideTimer = setTimeout(() => { + commitProgressHideTimer = null; + hideCommitProgress(); + }, 1000); } }); diff --git a/e2e/stu380-deploy.spec.js b/e2e/stu380-deploy.spec.js index 403dd54..b370b5a 100644 --- a/e2e/stu380-deploy.spec.js +++ b/e2e/stu380-deploy.spec.js @@ -235,6 +235,49 @@ test.describe("STU-380 deployment terminal outcomes", () => { await expect(page.locator("#commit-progress-overlay")).toBeHidden(); }); + test("a new deploy within the terminal hide delay keeps its progress overlay", async ({ page }) => { + await page.addInitScript(() => { + localStorage.setItem("hasSeenWalkthrough", "true"); + }); + await applyDefaultRoutes(page); + await page.goto("/"); + // The commitStateChange listener is attached at the end of the app's async + // startup, so wait for the real listener to answer: dispatching PREPARING + // opens the overlay. This is also deploy 1 starting. + await page.waitForFunction(() => { + window.dispatchEvent( + new CustomEvent("commitStateChange", { detail: { state: "PREPARING" } }), + ); + return document + .getElementById("commit-progress-overlay") + ?.classList.contains("open"); + }); + await expect(page.locator("#commit-progress-overlay")).toBeVisible(); + + // Deploy 1 ends: the terminal state schedules the overlay hide one second + // later, but the overlay is still up while that delay runs. + await page.evaluate(() => { + window.dispatchEvent( + new CustomEvent("commitStateChange", { detail: { state: "SUCCESS" } }), + ); + }); + + // Deploy 2 starts inside that second, exactly as a real confirm does by + // emitting PREPARING. The previous deploy's timer must not close the + // overlay that now belongs to the running deploy. + await page.evaluate(() => { + window.dispatchEvent( + new CustomEvent("commitStateChange", { detail: { state: "PREPARING" } }), + ); + }); + // Past the 1s hide delay and the overlay's 0.25s visibility transition: + // a stale timer has removed `.open` by now, so the class is the settled + // evidence — not a computed style that is mid-transition. + await page.waitForTimeout(1500); + await expect(page.locator("#commit-progress-overlay")).toHaveClass(/\bopen\b/); + await expect(page.locator("#commit-progress-overlay")).toBeVisible(); + }); + test("a partial/unconfirmed terminal keeps the deploy controls enabled (busy released)", async ({ page }) => { await render(page, { success: false, @@ -947,6 +990,51 @@ test.describe("STU-380 transport outcome regressions", () => { expect(exportCalls).toBe(2); }); + test("a gateway 502 keeps the project snapshot dirty instead of reporting a refusal", async ({ page }) => { + await loadDeployHooks(page); + + const zip = new JSZip(); + zip.file( + "pubspec.yaml", + "name: my_app\n\ndependencies:\n flutter:\n sdk: flutter\n", + ); + const projectZip = await zip.generateAsync({ type: "base64" }); + + let exportCalls = 0; + await page.route("**/exportCode", async (route) => { + exportCalls += 1; + await route.fulfill({ + status: 200, + contentType: "application/json", + body: JSON.stringify({ value: { project_zip: projectZip } }), + }); + }); + // A gateway 502 can arrive after the provisioning request was forwarded — + // the runner may still be deploying classes, so this is not a refusal. + await page.route(ENDPOINTS.deployCustomClasses, async (route) => { + await route.fulfill({ status: 502, body: "bad gateway" }); + }); + + // Seed the project-source cache exactly like a deploy's pubspec merge does. + await page.evaluate(() => window.__CCC_RESOLVE_PROJECT_PUBSPEC__({})); + expect(exportCalls).toBe(1); + + const outcome = await settleAsOutcome( + page, + "__CCC_PROVISION_CUSTOM_CLASSES__", + [{ fileMap: newClassFileMap }], + ); + // The UI must not report a definitive "nothing was written". + expect(outcome.name).toBe("UnconfirmedDeployError"); + expect(outcome.outcome).toBe("unconfirmed"); + + // The possibly-written class keeps the snapshot dirty: the next deploy + // must re-export instead of planning against the cached pre-write snapshot + // and silently provisioning the class a second time. + await page.evaluate(() => window.__CCC_RESOLVE_PROJECT_PUBSPEC__({})); + expect(exportCalls).toBe(2); + }); + test("a runner failure after deploy began also drops the cached snapshot", async ({ page }) => { await loadDeployHooks(page); @@ -1214,12 +1302,18 @@ test.describe("STU-380 transport outcome regressions", () => { await page.evaluate(() => { const realFetch = window.fetch; let provisionCalls = 0; + window.__CCC_PROVISION_STREAM__ = null; window.fetch = (url, ...args) => { if (String(url).includes("deployCustomClasses")) { provisionCalls += 1; if (provisionCalls === 1) { + const body = new ReadableStream({ + start(controller) { + window.__CCC_PROVISION_STREAM__ = controller; + }, + }); return Promise.resolve( - new Response(new ReadableStream({ start() {} }), { + new Response(body, { status: 200, headers: { "Content-Type": "application/x-ndjson" }, }), @@ -1261,11 +1355,28 @@ test.describe("STU-380 transport outcome regressions", () => { await page.evaluate(() => window.__CCC_RESOLVE_PROJECT_PUBSPEC__({})); expect(exportCalls).toBe(2); await page.evaluate(() => window.__CCC_RESOLVE_PROJECT_PUBSPEC__({})); - // Second sighting has no verified credit left — it retires A's write and - // this export becomes the cached snapshot. + // The second sighting shows the same unchanged X: it is the same fact as + // the first, so it cannot retire A's uncertain write. While A can still + // land, no export may be cached. expect(exportCalls).toBe(3); await page.evaluate(() => window.__CCC_RESOLVE_PROJECT_PUBSPEC__({})); - expect(exportCalls).toBe(3); + expect(exportCalls).toBe(4); + + // A's own late result is what resolves its write: the uncertain credit + // converts to a verified one, and the next sighting consumes it — only + // then does caching resume. + await page.evaluate(() => { + const encoder = new TextEncoder(); + window.__CCC_PROVISION_STREAM__.enqueue( + encoder.encode('{"event":"result","success":true}\n'), + ); + window.__CCC_PROVISION_STREAM__.close(); + }); + await page.waitForTimeout(50); + await page.evaluate(() => window.__CCC_RESOLVE_PROJECT_PUBSPEC__({})); + expect(exportCalls).toBe(5); + await page.evaluate(() => window.__CCC_RESOLVE_PROJECT_PUBSPEC__({})); + expect(exportCalls).toBe(5); }); test("a push still in flight keeps exports uncached until it settles", async ({ page }) => { diff --git a/src/provisionStream.js b/src/provisionStream.js index 6fa09fa..a408871 100644 --- a/src/provisionStream.js +++ b/src/provisionStream.js @@ -93,13 +93,12 @@ export async function readProvisionResponse(response, handlers = {}) { return { success: false, finalResultReceived: false, - // An explicit non-2xx HTTP response is a definitive server refusal, not an - // unknown outcome. The caller must render it as FAILED; only an ok response - // whose stream dropped before a result leaves the remote state genuinely - // unknown (UNCONFIRMED). This holds for the deploy runner: once streaming - // begins it emits every post-work failure as a `result` event under the - // already-sent 200 status, so a non-2xx can only arrive before the first - // remote write. + // An explicit non-2xx HTTP response. A 4xx proves the request was turned + // away before any work, so the caller renders it as FAILED. A 5xx proves + // no such thing: a gateway can answer 502/503/504 after forwarding the + // request to the runner, which may already be deploying (or may have + // deployed) classes — so a 5xx is UNCONFIRMED, exactly like an ok response + // whose stream dropped before a result. httpRejected: !response.ok, httpStatus: response.status, runnerPhase: lastPhase, @@ -150,11 +149,13 @@ export function isPreWriteRunnerFailure(result) { * Maps a provisioning stream result to the single truthful terminal outcome. * * When the runner delivered a definitive result, the ordinary classifier - * applies. When it did not (`finalResultReceived === false`), the two cases the - * caller must never flatten are separated: an explicit HTTP rejection (403/5xx) - * is a definitive refusal — the write did not happen — so it is FAILED, while an - * ok response whose stream simply dropped (or a client wait expiry) leaves the - * remote outcome unknown and is UNCONFIRMED. + * applies. When it did not (`finalResultReceived === false`), the cases the + * caller must never flatten are separated: a 4xx response is a definitive + * refusal — the request was turned away before any write — so it is FAILED, + * while a 5xx is not proof of anything. A gateway can return 502/503/504 + * after forwarding the request to the runner, so classes may already be + * deploying; that outcome, an ok response whose stream simply dropped, and a + * client wait expiry all leave the remote outcome unknown (UNCONFIRMED). * * This is the decision `provisionMissingCodeFiles` uses in the real deploy * path, so a test of this function drives the same code that renders a 403 @@ -175,7 +176,15 @@ export function deployOutcomeOfStreamResult(result) { : outcome; } if (result?.httpRejected) { - return DeployOutcome.FAILED; + // Only a 4xx proves the write was refused before it began. A 5xx can come + // from a gateway after the request reached the runner, so it must stay + // UNCONFIRMED: reporting it as FAILED would tell the user nothing was + // written while the runner is still deploying. + return typeof result.httpStatus === "number" && + result.httpStatus >= 400 && + result.httpStatus < 500 + ? DeployOutcome.FAILED + : DeployOutcome.UNCONFIRMED; } return DeployOutcome.UNCONFIRMED; } diff --git a/src/provisionStream.test.js b/src/provisionStream.test.js index eff319c..5b5a5f6 100644 --- a/src/provisionStream.test.js +++ b/src/provisionStream.test.js @@ -103,12 +103,11 @@ test("treats an empty body as a failure", async () => { // --- STU-380: an explicit HTTP rejection is FAILED, never UNCONFIRMED -------- // These drive the real stream handler (`readProvisionResponse`) against a true -// Response object with a non-2xx status and no streamed "result" event — the -// exact shape the deploy runner produces when it refuses the request before -// doing any work (API key lacks write access, transient 5xx, …). The -// mislabelling bug lived here: that shape was previously thrown as an -// UnconfirmedDeployError, telling the user to reconcile in FlutterFlow when the -// server had plainly refused the write. +// Response object with a non-2xx status and no streamed "result" event. A 4xx +// is the shape the deploy runner produces when it refuses the request before +// doing any work (API key lacks write access, bad request, …): the write was +// turned away, so it is FAILED. A 5xx is not a refusal — a gateway can fail +// after forwarding the request to the runner — so it is UNCONFIRMED. function provisionAtStatus(chunks, status) { const body = new ReadableStream({ @@ -173,7 +172,7 @@ test("an explicit HTTP rejection (403) is a failure, not unconfirmed", async () assert.notEqual(DeployOutcome.FAILED, DeployOutcome.UNCONFIRMED); }); -test("a 5xx server error with no streamed result is also FAILED, not unconfirmed", async () => { +test("a 5xx server error with no streamed result is UNCONFIRMED — a gateway can fail after forwarding", async () => { const result = await readProvisionResponse( new Response( '{"event":"phase","phase":"connecting","message":"Connecting..."}\n', @@ -181,12 +180,34 @@ test("a 5xx server error with no streamed result is also FAILED, not unconfirmed ), ); assert.equal(result.httpRejected, true); + assert.equal(result.httpStatus, 502); + // A 502 can come from a gateway after the request reached the runner, which + // may be deploying classes already — so it cannot prove nothing was written. assert.equal( deployOutcomeOfStreamResult(result), - DeployOutcome.FAILED, + DeployOutcome.UNCONFIRMED, + ); + + // Every 5xx shares that uncertainty: the server failed, but none of them + // proves the request was turned away before a write. + const unavailable = await readProvisionResponse( + new Response("", { status: 503 }), + ); + assert.equal( + deployOutcomeOfStreamResult(unavailable), + DeployOutcome.UNCONFIRMED, ); }); +test("a 4xx with no streamed result stays a definitive refusal, not unconfirmed", async () => { + const result = await readProvisionResponse( + new Response("no", { status: 400 }), + ); + // A 4xx is validation/auth — the request was turned away before any work. + assert.equal(result.httpRejected, true); + assert.equal(deployOutcomeOfStreamResult(result), DeployOutcome.FAILED); +}); + test("a dropped stream on an OK response stays UNCONFIRMED (outcome truly unknown)", async () => { const result = await readProvisionResponse( provisionAtStatus( From 81c5982700543b32229280d424048709b4175333 Mon Sep 17 00:00:00 2001 From: Stuart Date: Mon, 28 Sep 2026 17:09:03 +1000 Subject: [PATCH 2/3] docs(deploy): align provisioning caller comment with the 4xx/5xx split provisionMissingCodeFiles' no-final-result branch still described a 403/5xx `response.ok === false` as a definitive pre-write refusal. Only a 4xx is FAILED now; a 5xx stays UNCONFIRMED because a gateway can answer 502/503/504 after forwarding the request to the runner. Comment-only: no behavior change. --- app.js | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/app.js b/app.js index 4d811d6..59b072d 100644 --- a/app.js +++ b/app.js @@ -2924,12 +2924,13 @@ async function provisionMissingCodeFiles( ); // A stream that ended (or a body that carried) no definitive result leaves - // the remote decision ambiguous. There are two distinct cases which must not - // be flattened: a server that explicitly refused the request (a 403/5xx - // `response.ok === false`) has made a definitive decision — the write did not - // happen — so that is a FAILURE, not an unknown outcome; only an ok response - // whose stream dropped before a result (or a client wait expiry) leaves the - // remote state genuinely unknown and must be reported UNCONFIRMED. + // the remote decision ambiguous. An explicit 4xx is a definitive pre-write + // refusal — the request was turned away before any write — so it is a + // FAILURE, not an unknown outcome. A 5xx is no such proof: a gateway can + // answer 502/503/504 after forwarding the request to the runner, which may + // already be deploying, so a 5xx — like an ok response whose stream dropped + // before a result, or a client wait expiry — leaves the remote state + // genuinely unknown and must be UNCONFIRMED. if (!result.finalResultReceived) { if (deployOutcomeOfStreamResult(result) === DeployOutcome.FAILED) { // Not marked remoteRefusal: the refusal comes from our deploy runner, From b888f6fabe4876072b76d87c714c8463d442e021 Mon Sep 17 00:00:00 2001 From: Stuart Date: Mon, 28 Sep 2026 18:12:18 +1000 Subject: [PATCH 3/3] fix(deploy): name the received HTTP status when a 5xx deploy has no result MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit provisionMissingCodeFiles reported a no-result 5xx as "the connection to the FlutterFlow deploy runner dropped before it reported a result" — but an HTTP 502 was actually received, and the status was discarded. The cause sentence was false for exactly the case the 5xx handling exists for. The unconfirmed message now says the runner returned HTTP without reporting a result when a non-ok status arrived, and keeps the dropped-connection wording only when an ok response stream ended with no result. The reconcile-before-retry advice is unchanged in both. The 502 e2e test now pins the truthful text: it must name HTTP 502 and must not claim the connection dropped. --- app.js | 11 +++++++++-- e2e/stu380-deploy.spec.js | 6 ++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/app.js b/app.js index 59b072d..f917572 100644 --- a/app.js +++ b/app.js @@ -2940,9 +2940,16 @@ async function provisionMissingCodeFiles( `FlutterFlow custom class provisioning failed (HTTP ${result.httpStatus}).`, ); } + // The cause must match what actually happened. A non-ok status (a 5xx, + // typically a gateway) was received, so the runner answered — it did not + // drop the connection — and the message names the status. Only an ok + // response whose stream ended without a result is a dropped connection. + const cause = + result.httpRejected && typeof result.httpStatus === "number" + ? `The FlutterFlow deploy runner returned HTTP ${result.httpStatus} without reporting a result.` + : "The connection to the FlutterFlow deploy runner dropped before it reported a result."; throw new UnconfirmedDeployError( - "The connection to the FlutterFlow deploy runner dropped before it reported a result. " + - "The deploy may still be finishing on the server; open your FlutterFlow project to reconcile " + + `${cause} The deploy may still be finishing on the server; open your FlutterFlow project to reconcile ` + "before retrying.", ); } diff --git a/e2e/stu380-deploy.spec.js b/e2e/stu380-deploy.spec.js index b370b5a..c417c7d 100644 --- a/e2e/stu380-deploy.spec.js +++ b/e2e/stu380-deploy.spec.js @@ -1027,6 +1027,12 @@ test.describe("STU-380 transport outcome regressions", () => { // The UI must not report a definitive "nothing was written". expect(outcome.name).toBe("UnconfirmedDeployError"); expect(outcome.outcome).toBe("unconfirmed"); + // The cause must be truthful: an HTTP 502 was actually received, so the + // message names the status and may not claim the connection dropped. The + // reconcile-before-retry advice stays in both cases. + expect(outcome.message).toContain("HTTP 502"); + expect(outcome.message).not.toContain("dropped"); + expect(outcome.message).toContain("reconcile"); // The possibly-written class keeps the snapshot dirty: the next deploy // must re-export instead of planning against the cached pre-write snapshot