mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-20 00:02:19 +00:00
fix(cli-tests): stabilize flow lock-gen race + Windows path (#9080)
* fix(cli-tests): stabilize flow lock-gen race + Windows path Three CLI test failures on the latest main, all flaky on CI: 1. `Mixed Case Paths: pull and push flow with capitalized folder` and `Integration: Mixed scripts and flows with nonDottedPaths are idempotent`: flow create/update queues an async FlowDependencies job that fills inline-script lockfiles and rewrites flow.value. The tests pulled/pushed before the worker finished, so dry-run idempotency saw phantom `*.inline_script.lock` adds and `flow.yaml` edits. Added a `waitForFlowDependencyJob` helper that polls `/flows/get` for the latest `dependency_job` and `/jobs_u/completed/get` until it lands, and called it after each API/CLI flow write in both tests. 2. `HEADERS env var is forwarded on every CLI fetch` (Windows-only, added in #9075): the new test built the CLI entrypoint via `new URL("..", import.meta.url).pathname`, which yields `/C:/...` on Windows and `Bun.spawn` rejected before reaching the proxy, leaving `rejectedRequests.length` at 0. Switched to `fileURLToPath` + `node:path.join` to match `cargo_backend.ts`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cli-tests): use /flows/deployment_status to actually wait for dep job CI reviewers (Claude, Codex) flagged the prior `waitForFlowDependencyJob` as a no-op: it read `flow.dependency_job` from `/api/w/{ws}/flows/get`, but `Flow` / `FlowWithStarred` (backend/windmill-types/src/flows.rs:20-60) do not include that field. The helper exited on the first iteration without polling. Switch to `/api/w/{ws}/flows/deployment_status/p/{path}`, which returns `{ lock_error_logs, job_id }`. `job_id` is the FlowDependencies UUID written into `deployment_metadata` in the same tx as the dep-job push (backend/windmill-api-flows/src/flows.rs:660-672 and :1275-1292), so by the time the create/update API call returns, the response carries the latest dep-job UUID. Then poll `/jobs_u/completed/get/{job_id}` as before. Local runtime for `mixed_case_paths.test.ts` jumps from ~9s to ~32s, confirming the helper now actually waits instead of returning immediately. The 404 short-circuit in `sync_pull_push.test.ts` still works — `get_deployment_status` returns 404 when the flow is absent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -18,6 +18,8 @@
|
||||
|
||||
import { expect, test } from "bun:test";
|
||||
import { mkdir, writeFile } from "node:fs/promises";
|
||||
import { dirname, join } from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import type { Server } from "bun";
|
||||
import { withTestBackend } from "./test_backend.ts";
|
||||
|
||||
@@ -113,12 +115,16 @@ async function runCliThroughProxy(
|
||||
cwd: string,
|
||||
env: Record<string, string>,
|
||||
): Promise<{ stdout: string; stderr: string; code: number }> {
|
||||
const cliDir = new URL("..", import.meta.url).pathname;
|
||||
// Use fileURLToPath + node:path so the CLI entrypoint is a real OS path on
|
||||
// Windows. `new URL(..).pathname` yields `/C:/...` which Bun.spawn fails to
|
||||
// resolve, so the negative case "rejected" on launch instead of reaching
|
||||
// the proxy and the assertion on rejectedRequests > 0 flaked.
|
||||
const cliDir = join(dirname(fileURLToPath(import.meta.url)), "..");
|
||||
const useNode = process.env["TEST_CLI_RUNTIME"] === "node";
|
||||
const runtime = useNode ? "node" : "bun";
|
||||
const entrypoint = useNode
|
||||
? `${cliDir}/npm/esm/main.js`
|
||||
: `${cliDir}/src/main.ts`;
|
||||
? join(cliDir, "npm", "esm", "main.js")
|
||||
: join(cliDir, "src", "main.ts");
|
||||
const runtimeArgs = useNode ? [entrypoint] : ["run", entrypoint];
|
||||
|
||||
const fullArgs = [
|
||||
|
||||
@@ -130,6 +130,57 @@ async function createFlow(
|
||||
throw new Error(`Failed to create flow ${flowPath}: ${error}`);
|
||||
}
|
||||
await response.text();
|
||||
|
||||
// Flow creation queues an async FlowDependencies job that generates the
|
||||
// inline-script lockfile and rewrites flow.value. If we don't wait, a
|
||||
// subsequent pull/push races the worker and the dry-run idempotency
|
||||
// assertion sees a phantom lock-add + flow.yaml-edit diff (CI-only flake).
|
||||
await waitForFlowDependencyJob(backend, flowPath);
|
||||
}
|
||||
|
||||
async function waitForFlowDependencyJob(
|
||||
backend: any,
|
||||
flowPath: string,
|
||||
timeoutMs: number = 30000,
|
||||
): Promise<void> {
|
||||
// /flows/get does not return `dependency_job`. The deployment_status route
|
||||
// joins flow_version against deployment_metadata, which is populated in the
|
||||
// same tx as the FlowDependencies push, so by the time the create/update
|
||||
// API call returns, job_id is already the latest dep-job UUID.
|
||||
const start = Date.now();
|
||||
while (Date.now() - start < timeoutMs) {
|
||||
const statusResp = await backend.apiRequest!(
|
||||
`/api/w/${backend.workspace}/flows/deployment_status/p/${flowPath}`,
|
||||
);
|
||||
if (statusResp.status === 404) {
|
||||
await statusResp.text().catch(() => {});
|
||||
return;
|
||||
}
|
||||
if (!statusResp.ok) {
|
||||
await statusResp.text().catch(() => {});
|
||||
throw new Error(
|
||||
`Failed to fetch deployment status for ${flowPath}: ${statusResp.status}`,
|
||||
);
|
||||
}
|
||||
const status = await statusResp.json();
|
||||
const depJobId: string | undefined = status?.job_id;
|
||||
if (!depJobId) {
|
||||
await new Promise((r) => setTimeout(r, 100));
|
||||
continue;
|
||||
}
|
||||
const completed = await backend.apiRequest!(
|
||||
`/api/w/${backend.workspace}/jobs_u/completed/get/${depJobId}`,
|
||||
);
|
||||
if (completed.ok) {
|
||||
await completed.text();
|
||||
return;
|
||||
}
|
||||
await completed.text().catch(() => {});
|
||||
await new Promise((r) => setTimeout(r, 100));
|
||||
}
|
||||
throw new Error(
|
||||
`Flow dependency job for ${flowPath} did not complete within ${timeoutMs}ms`,
|
||||
);
|
||||
}
|
||||
|
||||
async function createApp(
|
||||
@@ -384,6 +435,10 @@ excludes: []
|
||||
const updatedFlow = await getFlow(backend, flowPath);
|
||||
expect(updatedFlow.summary).toEqual("Modified Data Processor Flow from test");
|
||||
|
||||
// The CLI push enqueues a fresh FlowDependencies job. Wait for it before
|
||||
// the dry-run pull so the lock/flow.value writes have committed.
|
||||
await waitForFlowDependencyJob(backend, flowPath);
|
||||
|
||||
// Verify no diff on subsequent pull (idempotency)
|
||||
await verifyNoDiffOnPull(backend, tempDir);
|
||||
});
|
||||
|
||||
@@ -493,6 +493,58 @@ async function cleanupTempDir(dir: string): Promise<void> {
|
||||
}
|
||||
}
|
||||
|
||||
// Polls /flows/deployment_status/p/{path} + /jobs_u/completed/get until the
|
||||
// most recent flow dependency job has completed. After a flow create/update
|
||||
// the API queues a FlowDependencies job that asynchronously fills inline-
|
||||
// script lockfiles and rewrites flow.value; tests that round-trip through
|
||||
// sync push/pull must wait for it or they race the worker (CI-only flake).
|
||||
// `deployment_status` is the right read here — `/flows/get` does not return
|
||||
// `dependency_job`, but the `deployment_metadata.job_id` row is written in
|
||||
// the same tx as the dep-job push, so the returned `job_id` is the latest
|
||||
// dep-job UUID by the time the create/update API call has returned. Returns
|
||||
// silently if the flow doesn't exist on the server (treats it as a no-op
|
||||
// push, since the route returns 404 when the flow row is missing).
|
||||
async function waitForFlowDependencyJob(
|
||||
backend: any,
|
||||
flowPath: string,
|
||||
timeoutMs: number = 30000,
|
||||
): Promise<void> {
|
||||
const start = Date.now();
|
||||
while (Date.now() - start < timeoutMs) {
|
||||
const statusResp = await backend.apiRequest!(
|
||||
`/api/w/${backend.workspace}/flows/deployment_status/p/${flowPath}`,
|
||||
);
|
||||
if (statusResp.status === 404) {
|
||||
await statusResp.text().catch(() => {});
|
||||
return;
|
||||
}
|
||||
if (!statusResp.ok) {
|
||||
await statusResp.text().catch(() => {});
|
||||
throw new Error(
|
||||
`Failed to fetch deployment status for ${flowPath}: ${statusResp.status}`,
|
||||
);
|
||||
}
|
||||
const status = await statusResp.json();
|
||||
const depJobId: string | undefined = status?.job_id;
|
||||
if (!depJobId) {
|
||||
await new Promise((r) => setTimeout(r, 100));
|
||||
continue;
|
||||
}
|
||||
const completed = await backend.apiRequest!(
|
||||
`/api/w/${backend.workspace}/jobs_u/completed/get/${depJobId}`,
|
||||
);
|
||||
if (completed.ok) {
|
||||
await completed.text();
|
||||
return;
|
||||
}
|
||||
await completed.text().catch(() => {});
|
||||
await new Promise((r) => setTimeout(r, 100));
|
||||
}
|
||||
throw new Error(
|
||||
`Flow dependency job for ${flowPath} did not complete within ${timeoutMs}ms`,
|
||||
);
|
||||
}
|
||||
|
||||
// =============================================================================
|
||||
// Tests
|
||||
// =============================================================================
|
||||
@@ -1797,6 +1849,12 @@ excludes: []
|
||||
|
||||
expect(pushResult.code).toEqual(0);
|
||||
|
||||
// sync push of the flow enqueues an async FlowDependencies job that
|
||||
// generates the inline-script lockfile and rewrites flow.value. Wait for
|
||||
// it before pulling back, otherwise pull races the worker and the
|
||||
// dry-run push idempotency check sees phantom diffs (CI-only flake).
|
||||
await waitForFlowDependencyJob(backend, flowName);
|
||||
|
||||
// Pull back
|
||||
const pullResult = await backend.runCLICommand(
|
||||
["sync", "pull", "--yes", "--includes", `f/test/mixed_*_${uniqueId}**`],
|
||||
|
||||
Reference in New Issue
Block a user