Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 109 additions & 1 deletion apps/cli/src/commands/functions/deploy/deploy.integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ import { BunServices } from "@effect/platform-bun";
import { describe, expect, it } from "@effect/vitest";
import {
Cause,
Clock,
Deferred,
Duration,
Effect,
Exit,
Fiber,
Expand All @@ -11,10 +13,12 @@ import {
Option,
Path,
Predicate,
Queue,
Schema,
Stdio,
} from "effect";
import * as HttpClientResponse from "effect/http/HttpClientResponse";
import { TestClock } from "effect/testing";

import { YesFlag } from "../../../command-internal/global-flags.ts";
import { stripControlSequences } from "../../../shared/output/strip-control-sequences.ts";
Expand Down Expand Up @@ -710,13 +714,38 @@ describe("functions deploy", () => {
function apiDeployHarness(
handler: ApiHandler,
args: ReadonlyArray<string> = ["functions", "deploy", "hello-world", "--use-api"],
rateLimitNotices?: Queue.Enqueue<string>,
) {
const out = mockOutput({ format: "text" });
const api = mockCommandPlatformApi({ handler });
const layer = Layer.mergeAll(
cliConfigValuesTestLayer,
buildTestRuntime({
out,
out:
rateLimitNotices === undefined
? out
: {
...out,
layer: Layer.effect(
Output,
Effect.gen(function* () {
const output = yield* Output;
return {
...output,
raw: (text: string, stream?: "stdout" | "stderr") =>
output
.raw(text, stream)
.pipe(
Effect.tap(() =>
text.startsWith("Rate limit exceeded")
? Queue.offer(rateLimitNotices, text)
: Effect.void,
),
),
};
}),
).pipe(Layer.provide(out.layer)),
},
api,
cliSettings: mockCommandSettings({ workdir: tempRoot.current }),
runtimeInfo: mockRuntimeInfo({ cwd: tempRoot.current }),
Expand Down Expand Up @@ -1145,6 +1174,85 @@ describe("functions deploy", () => {
}).pipe(Effect.provide(layer), Effect.ensuring(removeTempRoot));
});

it.effect("gives up on a persistent rate limit after the command's backoff schedule", () =>
Effect.gen(function* () {
const notices = yield* Queue.unbounded<string>();
const deployTimes: Array<number> = [];
const { layer } = apiDeployHarness(
(request) =>
request.method === "GET"
? Effect.succeed(jsonResponse(request, 200, []))
: Effect.gen(function* () {
deployTimes.push(yield* Clock.currentTimeMillis);
return HttpClientResponse.fromWeb(request, new Response("{}", { status: 429 }));
}),
undefined,
notices,
);

yield* Effect.gen(function* () {
yield* writeCliConfig(tempRoot.current);
yield* writeLocalFunction(tempRoot.current, "hello-world");

const deploy = yield* Effect.forkChild(functionsDeploy(baseFlags));
for (const seconds of [1, 2, 4, 8, 16, 32, 32, 32]) {
expect(yield* Queue.take(notices)).toBe(
`Rate limit exceeded while deploying function hello-world. Retrying in ${seconds}s.\n`,
);
yield* TestClock.adjust(Duration.seconds(seconds));
}
const error = yield* Fiber.join(deploy).pipe(Effect.flip);

expect(error).toBeInstanceOf(FunctionsApiStatusError);
expect(error).toMatchObject({ status: 429 });
expect(deployTimes).toEqual([
0, 1_000, 3_000, 7_000, 15_000, 31_000, 63_000, 95_000, 127_000,
]);
Comment on lines +1204 to +1210

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ NIT · test-coverage · source: claude

The persistent-rate-limit test consumes eight retry notices but never checks that no additional notice remains after deployment fails. A misleading final retry notice could therefore pass this test.

Evidence: The harness queues every rate-limit notice at lines 735-742. The test creates an unbounded queue at line 1179, consumes eight notices at lines 1198-1203, then checks only the error and request timestamps at lines 1204-1210.

Suggested fix: After joining the deployment fiber, assert that the notices queue is empty.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This persistent-rate-limit test came from develop and the merge did not change it, so the missing empty-queue check was not introduced by the merge. It is out of scope for the sync and can be handled in a follow-up on develop.

}).pipe(Effect.provide(layer), Effect.ensuring(removeTempRoot));
}),
);

it.effect("waits for the server's retry-after before retrying a rate-limited deploy", () =>
Effect.gen(function* () {
const notices = yield* Queue.unbounded<string>();
let deployAttempts = 0;
const { layer, api } = apiDeployHarness(
(request, recorded) => {
if (request.method === "GET") return Effect.succeed(jsonResponse(request, 200, []));
deployAttempts += 1;
if (deployAttempts === 1) {
return Effect.succeed(
HttpClientResponse.fromWeb(
request,
new Response("{}", { status: 429, headers: { "retry-after": "3" } }),
),
);
}
return deployedHelloWorld(request, recorded);
},
undefined,
notices,
);
const deployRequests = () => api.requests.filter((request) => request.method === "POST");

yield* Effect.gen(function* () {
yield* writeCliConfig(tempRoot.current);
yield* writeLocalFunction(tempRoot.current, "hello-world");

const deploy = yield* Effect.forkChild(functionsDeploy(baseFlags));
expect(yield* Queue.take(notices)).toBe(
"Rate limit exceeded while deploying function hello-world. Retrying in 3s.\n",
);
yield* TestClock.adjust(Duration.millis(2_999));
expect(deployRequests()).toHaveLength(1);
yield* TestClock.adjust(Duration.millis(1));
yield* Fiber.join(deploy);

expect(deployRequests()).toHaveLength(2);
}).pipe(Effect.provide(layer), Effect.ensuring(removeTempRoot));
}),
);

it.effect("reports a malformed functions list with the schema decoder's message", () => {
const { layer } = apiDeployHarness((request) =>
Effect.succeed(
Expand Down
6 changes: 3 additions & 3 deletions apps/cli/src/shared/auth/jwks.integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { resolveRemoteJwks } from "./jwks.ts";
const issuer = "https://issuer.example";

describe("remote signing keys", () => {
it.live("discovers the key URL and preserves the provider's key payload", () =>
it.effect("discovers the key URL and preserves the provider's key payload", () =>
Effect.gen(function* () {
const requested: string[] = [];
const keys = [{ kty: "RSA", kid: "key-1", custom: true }];
Expand All @@ -31,7 +31,7 @@ describe("remote signing keys", () => {
{ name: "missing discovery key URL", status: 200, body: {} },
{ name: "empty key document", status: 200, body: { keys: [] } },
]) {
it.live(`rejects ${response.name}`, () =>
it.effect(`rejects ${response.name}`, () =>
Effect.gen(function* () {
const client = HttpClient.make((request) =>
Effect.succeed(
Expand Down Expand Up @@ -73,7 +73,7 @@ describe("remote signing keys", () => {
}),
);

it.live("caller interruption aborts the owned request without becoming a remote error", () =>
it.effect("caller interruption aborts the owned request without becoming a remote error", () =>
Effect.gen(function* () {
const started = yield* Deferred.make<AbortSignal>();
const client = HttpClient.make((_request, _url, signal) =>
Expand Down
5 changes: 4 additions & 1 deletion apps/cli/src/shared/cli/command-docs.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import type { Command, HelpDoc } from "effect/cli";
import { helpDocFor } from "./subcommand-flag-suggestions.ts";

// Get HelpDoc from a command (uses internal buildHelpDoc)
export function getHelpDoc(
command: Command.Command.Any,
commandPath: ReadonlyArray<string>,
): HelpDoc.HelpDoc {
return (command as any).buildHelpDoc(commandPath);
const helpDoc = helpDocFor(command, commandPath);
if (helpDoc === undefined) throw new Error(`Command ${command.name} has no help doc`);
return helpDoc;
}

// Navigate to a subcommand by path segments
Expand Down
4 changes: 2 additions & 2 deletions apps/cli/src/shared/cli/markdown-formatter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ function formatTable(headers: string[], rows: string[][]): string {
const widths = headers.map((h, i) => Math.max(h.length, ...rows.map((r) => (r[i] ?? "").length)));
const pad = (s: string, w: number) => s + " ".repeat(w - s.length);

const headerLine = `| ${headers.map((h, i) => pad(h, widths[i]!)).join(" | ")} |`;
const headerLine = `| ${headers.map((h, i) => pad(h, widths[i] ?? h.length)).join(" | ")} |`;
const separatorLine = `| ${widths.map((w) => "-".repeat(w)).join(" | ")} |`;
const dataLines = rows.map(
(row) => `| ${row.map((cell, i) => pad(cell, widths[i]!)).join(" | ")} |`,
(row) => `| ${row.map((cell, i) => pad(cell, widths[i] ?? cell.length)).join(" | ")} |`,
);

return [headerLine, separatorLine, ...dataLines].join("\n");
Expand Down
24 changes: 16 additions & 8 deletions apps/cli/src/shared/cli/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
Layer,
Path,
PlatformError,
Predicate,
Runtime,
Scope,
Stdio,
Expand Down Expand Up @@ -118,7 +119,8 @@ const selfManagedSignalCommands: ReadonlyArray<ReadonlyArray<string>> = [["funct
export function extractCommandPath(args: ReadonlyArray<string>): ReadonlyArray<string> {
const commandArgs: Array<string> = [];
for (let index = 0; index < args.length; index += 1) {
const arg = args[index]!;
const arg = args[index];
if (arg === undefined) continue;
if (arg === "--") return commandArgs;
if (arg.startsWith("-")) {
const [flag] = arg.split("=", 1);
Expand Down Expand Up @@ -152,7 +154,8 @@ export function* rootFlagTokens(
isValueTakingToken: (token: string) => boolean = isGlobalValueFlagToken,
): Generator<{ readonly token: string; readonly index: number }> {
for (let index = 0; index < args.length; index += 1) {
const arg = args[index]!;
const arg = args[index];
if (arg === undefined) continue;
if (arg === "--") return;
if (!arg.startsWith("-")) continue;
yield { token: arg, index };
Expand Down Expand Up @@ -206,7 +209,7 @@ function* shortClusterFlagNames(
): Generator<string> {
if (!token.startsWith("-") || token.startsWith("--")) return;
for (let rest = token.slice(1); rest.length > 0; rest = rest.slice(1)) {
const short = `-${rest[0]!}`;
const short = `-${rest.charAt(0)}`;
if (isValueTakingToken(short)) return;
if (rest[1] === "=") {
yield short;
Expand All @@ -223,7 +226,7 @@ function shortClusterConsumesNextToken(
): boolean {
if (!token.startsWith("-") || token.startsWith("--")) return false;
for (let rest = token.slice(1); rest.length > 0; rest = rest.slice(1)) {
if (isValueTakingToken(`-${rest[0]!}`)) return rest.length === 1;
if (isValueTakingToken(`-${rest.charAt(0)}`)) return rest.length === 1;
if (rest[1] === "=") return false;
}
return false;
Expand All @@ -239,7 +242,8 @@ function firstPositionalIndex(
isValueTakingToken: (token: string) => boolean,
): number {
for (let index = 0; index < args.length; index += 1) {
const arg = args[index]!;
const arg = args[index];
if (arg === undefined) continue;
if (arg === "--") return index + 1;
if (!arg.startsWith("-")) return index;
const [flag] = arg.split("=", 1);
Expand Down Expand Up @@ -442,7 +446,11 @@ export function classifyParseErrorConsoleOutput(
context: CliErrorSuggestionContext,
): ParseErrorConsoleDisposition {
const error = Cause.squash(cause);
if (!CliError.isCliError(error) || error._tag !== "ShowHelp" || error.errors.length === 0) {
if (
!CliError.isCliError(error) ||
!Predicate.isTagged(error, "ShowHelp") ||
error.errors.length === 0
) {
return "flush-unchanged";
}
// `isValueTakingFlagTokenFor` only inspects the resolved leaf command's own flags, so
Expand All @@ -453,7 +461,7 @@ export function classifyParseErrorConsoleOutput(
const isValueTakingToken = (token: string) =>
globalFlagsWithValues.has(token) || isLeafValueTakingToken(token);
const isSuppressedMissingFlag = (inner: (typeof error.errors)[number]) =>
inner._tag === "MissingOption" &&
Predicate.isTagged(inner, "MissingOption") &&
!isMissingFlagTokenPresent(
inner.option,
context.args,
Expand Down Expand Up @@ -659,7 +667,7 @@ export const runCli = Effect.fnUntraced(function* <
.pipe(Effect.map((signal) => ({ _tag: "signal" as const, signal }))),
);

if (outcome._tag === "signal") {
if (Predicate.isTagged(outcome, "signal")) {
// SIGHUP must also stay held once cleanup begins.
yield* Effect.scoped(
processControl.holdSignals(["SIGHUP"]).pipe(Effect.andThen(Fiber.interrupt(cliFiber))),
Expand Down
12 changes: 8 additions & 4 deletions apps/cli/src/shared/cli/subcommand-flag-suggestions.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { Predicate } from "effect";
import type { CliError, Command, HelpDoc } from "effect/cli";
import { formatInvalidValueMessage } from "./invalid-value-message.ts";

Expand Down Expand Up @@ -36,7 +37,7 @@ function hasHelpDoc(command: Command.Command.Any): command is CommandWithHelpDoc
return "buildHelpDoc" in command && typeof command.buildHelpDoc === "function";
}

function helpDocFor(
export function helpDocFor(
command: Command.Command.Any,
commandPath: ReadonlyArray<string>,
): HelpDoc.HelpDoc | undefined {
Expand Down Expand Up @@ -281,7 +282,7 @@ export function formatCliErrorsForDisplay(
let changed = false;

for (const error of errors) {
if (error._tag === "UnrecognizedOption" && context) {
if (Predicate.isTagged(error, "UnrecognizedOption") && context) {
const hint = buildSubcommandFlagHint(error, context);
if (hint) {
if (hint.consumedValue) suppressedUnknownSubcommands.add(hint.consumedValue);
Expand All @@ -296,12 +297,15 @@ export function formatCliErrorsForDisplay(
}
}

if (error._tag === "UnknownSubcommand" && suppressedUnknownSubcommands.has(error.subcommand)) {
if (
Predicate.isTagged(error, "UnknownSubcommand") &&
suppressedUnknownSubcommands.has(error.subcommand)
) {
changed = true;
continue;
}

if (error._tag === "InvalidValue") {
if (Predicate.isTagged(error, "InvalidValue")) {
const message = formatInvalidValueMessage(error);
if (message !== undefined) {
changed = true;
Expand Down
6 changes: 1 addition & 5 deletions apps/cli/src/shared/cli/success-trailer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,7 @@ export const successTrailerLayer = Layer.effect(

return SuccessTrailer.of({
append: (text) => Ref.update(pending, (trailers) => [...trailers, text]),
takeAll: Effect.gen(function* () {
const trailers = yield* Ref.get(pending);
yield* Ref.set(pending, empty);
return trailers;
}),
takeAll: Ref.getAndSet(pending, empty),
setWorkingDirectory: (cwd) => Ref.set(workingDirectory, cwd),
workingDirectory: Ref.get(workingDirectory),
});
Expand Down
5 changes: 3 additions & 2 deletions apps/cli/src/shared/cli/version.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ export function parseSemver(version: string): ParsedSemver | undefined {
/^(0|[1-9]\d*)(?:\.(0|[1-9]\d*))?(?:\.(0|[1-9]\d*)(?:-([0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*))?(?:\+[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?)?(?![\s\S])/.exec(
version,
);
if (match === null) return undefined;
const major = match?.[1];
if (match === null || major === undefined) return undefined;
const prerelease = match[4] ?? "";
if (
prerelease
Expand All @@ -40,7 +41,7 @@ export function parseSemver(version: string): ParsedSemver | undefined {
return undefined;
}
return {
nums: [match[1]!, match[2] ?? "0", match[3] ?? "0"],
nums: [major, match[2] ?? "0", match[3] ?? "0"],
prerelease,
};
}
Expand Down
Loading
Loading