feat(workers push): make the build wait opt-out with --no-wait flag - #6371
Conversation
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@539986a3518db401ac620864e423e19bdb30a171Preview package for commit |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdff3a52a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Makes the `name` argument to `supabase workers new` optional and prompts for it when it is omitted, so a bare `supabase workers new` walks through name, runtime and size rather than failing the parse. The name is the one input this command cannot default — it is the directory, the `[workers.<name>]` key and the hostname all at once. So where the runtime and size prompts fall back to a default when there is nowhere to ask, the name prompt has nothing to fall back to: with `-o json|yaml|toml|env` or no interactive terminal, the command fails with a new `MissingWorkerNameError` pointing at `supabase workers new api`. The prompt validates against everything the command would otherwise refuse a moment later — a non-DNS-label name, and a name `config.toml` already records — so a typo is corrected in place instead of ending the run. That also means the project has to be loaded before the first prompt, and the machine-output check moves up with it: `-o` leaves `output.format` as `text`, and Clack writes its terminal UI to stdout, so a name prompt would land in front of the payload for the same reason the runtime prompt would. The handler's inline name validation is replaced by the shared `legacyValidateWorkerName`, which the rest of the command family already uses, so an explicitly-passed name and a prompted one are refused on identical terms. `mockOutput` now records `promptTextCalls` so tests can assert on the prompt's message and exercise its `validate` callback.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
`output.interactive` is derived solely from `tty.stdoutIsTty`, so with stdin piped or redirected and stdout still on a terminal it stayed true. A bare `printf 'api\n' | supabase workers new` therefore opened Clack's name prompt and read the worker name off the pipe instead of taking the documented `MissingWorkerNameError` path — and the runtime and size prompts consumed whatever followed rather than falling back to their defaults. The three resolvers now share one `canPromptFor` decision, made once before the first prompt, which pairs `output.interactive` with `tty.stdinIsTty` the way `workers delete` already guards its confirmation. A prompt is only answerable from a keyboard, so both streams have to be a terminal.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a6e00c91e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
6a6e00c to
8026a04
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5072b3ec0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
5072b3e to
6010d9f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6010d9fcd1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
6010d9f to
22d3cfb
Compare
💡 Codex Reviewcli/apps/cli/src/legacy/commands/workers/logs/logs.handler.ts Lines 137 to 139 in 22d3cfb  Decouple live polling from the history tail limit
When cli/apps/cli/src/legacy/commands/workers/logs/logs.handler.ts Lines 181 to 183 in 22d3cfb Derive source choices without a type assertion
This assertion suppresses the compiler check that every value accepted by AGENTS.md reference: AGENTS.md:L220-L220 cli/apps/cli/src/legacy/commands/workers/logs/logs.handler.ts Lines 315 to 318 in 22d3cfb  Retry only transient log-read failures
Under AGENTS.md reference: AGENTS.md:L181-L187 cli/apps/cli/src/legacy/commands/workers/logs/logs.handler.ts Lines 327 to 329 in 22d3cfb Let interruption unwind before exiting the process
When a followed command receives SIGINT or SIGTERM in the real runtime, AGENTS.md reference: apps/cli/AGENTS.md:L286-L290 cli/apps/cli/src/legacy/commands/workers/logs/logs.handler.ts Lines 237 to 239 in 22d3cfb Honor the legacy text format before output-format
When callers combine AGENTS.md reference: apps/cli/AGENTS.md:L298-L300 cli/apps/cli/src/legacy/commands/workers/logs/logs.handler.ts Lines 165 to 171 in 22d3cfb Preserve request and build details in followed JSON logs
With ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Makes the `name` argument to `supabase workers new` optional and prompts for it when it is omitted, so a bare `supabase workers new` walks through name, runtime and size rather than failing the parse. The name is the one input this command cannot default — it is the directory, the `[workers.<name>]` key and the hostname all at once. So where the runtime and size prompts fall back to a default when there is nowhere to ask, the name prompt has nothing to fall back to: with `-o json|yaml|toml|env` or no interactive terminal, the command fails with a new `MissingWorkerNameError` pointing at `supabase workers new api`. The prompt validates against everything the command would otherwise refuse a moment later — a non-DNS-label name, and a name `config.toml` already records — so a typo is corrected in place instead of ending the run. That also means the project has to be loaded before the first prompt, and the machine-output check moves up with it: `-o` leaves `output.format` as `text`, and Clack writes its terminal UI to stdout, so a name prompt would land in front of the payload for the same reason the runtime prompt would. The handler's inline name validation is replaced by the shared `legacyValidateWorkerName`, which the rest of the command family already uses, so an explicitly-passed name and a prompted one are refused on identical terms. `mockOutput` now records `promptTextCalls` so tests can assert on the prompt's message and exercise its `validate` callback.
`output.interactive` is derived solely from `tty.stdoutIsTty`, so with stdin piped or redirected and stdout still on a terminal it stayed true. A bare `printf 'api\n' | supabase workers new` therefore opened Clack's name prompt and read the worker name off the pipe instead of taking the documented `MissingWorkerNameError` path — and the runtime and size prompts consumed whatever followed rather than falling back to their defaults. The three resolvers now share one `canPromptFor` decision, made once before the first prompt, which pairs `output.interactive` with `tty.stdinIsTty` the way `workers delete` already guards its confirmation. A prompt is only answerable from a keyboard, so both streams have to be a terminal.
22d3cfb to
3bf251a
Compare
Comments explaining why code is shaped a certain way now state the constraint directly instead of narrating what an earlier version did. The reasoning is unchanged; only the framing is.
The workers commands each grew their own way of saying "here is what happened" and "here is what to run next". This settles them on the shapes the rest of the legacy shell already uses, with no change to what any command does. - "What to run next" lines in `new`, `push`, `delete` and `status` move to `emitSuccessTrailer`, the way `stop`, `bootstrap`, `migration repair` and `gen signing-key` already emit theirs: printed once at the end of the run rather than inline, so a multi-worker push does not bury each worker's hint under the next worker's output. The commands within them are aqua'd, as every other follow-up hint in this shell writes them. - `list`'s two advisories take the yellow `WARNING:` prefix and the two-line consequence shape `start`'s Docker notice uses. Each was one long sentence that re-flowed at a different width under a table that lines its columns up. - `list` drops the URL column. Every worker's URL is the same host and prefix with the name on the end, and carrying it pushed the table past 130 columns for one derivable field, since `renderGlamourTable` sizes to the widest cell and never wraps. `status` still renders it vertically, and every machine format still carries `url` per worker. - `push` counts its per-worker announcements (`Deploying Worker 1/2:`) and closes a multi-worker run with a summary line. Each worker takes minutes; the name alone said nothing about how much of the run was left. - `push` names the workers a failed run never attempted. The loop stops at the first failure and the error only names the worker that broke, leaving the rest to be reconstructed from argument order. On stderr in every format, machine ones included: that run is a CI run. - Both of `push`'s retry suggestions carry an explicit `--project-ref` when the flag supplied the ref, via the `legacyWorkersProjectRefSuffix` helper `status` and `delete` already use. A suggestion is copy-pasted verbatim, so one that dropped it re-resolved against whatever this checkout was linked to. Adds unit coverage for `legacyRenderWorkerDetails`'s padding and empty-row dropping, and pins the shared `-o env` refusal so a new command that forgets its own up-front check cannot silently emit TOML instead.
`Flag.boolean(name)` builds a bare `Single` param, and a bare `Single` is *required* — omitting it fails the whole command with a missing-flag error before the handler ever runs. Every boolean flag has to be closed off with `Flag.withDefault(false)` or `Flag.optional`, and nothing in the existing suites notices when one is not. Handler integration tests build their flags record directly, so they never touch the parser, and the required-ness is invisible to the type checker because a required boolean flag still infers as `boolean`. The flag only misbehaves when a real invocation omits it, which is exactly the invocation no handler test makes. So this walks the whole legacy command tree, including global flags, and asserts every boolean param carries a default or is optional. It reads the primitive kind through `Primitive.getTypeName` rather than `_tag`, since this repo forbids inspecting effect's runtime representation in tests as well as in source.
Comments explaining why code is shaped a certain way now state the constraint directly instead of narrating what an earlier version did. The reasoning is unchanged; only the framing is.
`status` reports the deployment; nothing reported the runtime. Once `push` succeeded and `status` said `active`, a misbehaving worker was a black box from the CLI. Reads the project's unified logs stream rather than a worker route — there is no worker-scoped log endpoint — via `v1GetProjectLogs`, which the generated client already carries. `--source app|requests|builds` narrows to one of the three streams; without it all three are returned. `--tail` caps the rows. Three things about that endpoint are load-bearing and non-obvious, so they are documented at each site: - **The filter is `log_attributes`, not the `source` column.** Worker rows carry an empty top-level `source`, because the Workers Logflare source is not enrolled as a category in the generic logs path. `where source = 'worker_guest_logs'` matches nothing. The `in (...)` list over the three known streams is therefore a tenancy guard, not a convenience — with `source` empty it is the only thing excluding a non-worker row that happens to carry a `worker` attribute. - **Both timestamp bounds are always sent, spanning under 24h.** One bound alone yields a one-minute window, silently; neither is an outright error; and a span over 24h is clamped to `start + 24h`, which returns an *older* slice than the one asked for rather than a truncated one. - **A failed query can arrive as HTTP 200** with a populated `error`, so the envelope is checked before `result`. The response is decoded against a local schema rather than the generated `V1GetProjectLogsOutput`: that schema marks `result`/`error` optional but allows neither to be `null`, while the endpoint always sends one of them as an explicit `null`, so decoding any real response against it fails. Rendering is per-stream, because `event_message` differs in kind — on the request stream it is only `"GET /"`, with status and duration in `log_attributes`, so the request line is composed. `severity_text` is ignored: it is `INFO` on every row of every stream, so the level is derived, and guest lines report none rather than a guess. A guest message is tenant-controlled bytes, so escape sequences are stripped before it reaches a terminal while a stack trace's newlines and indentation survive. `mapRequestError`/`unexpectedStatus`/`decodeBody` move out of `workers-api.ts` into `workers-api-status.ts`, unchanged, now that a second seam needs them. The test helper records `urlParams`: `HttpClientRequest` keeps them off the URL, so without this no test could assert the emitted SQL or window.
Matches the only other log-line format this shell prints — the `--debug` HTTP logger, which uses Go's `log.LstdFlags` (`legacy-debug-logger.layer.ts`). Someone reading a tail is asking "what just happened", and the answer gets compared against their own clock. Text output only. The machine payload keeps both unambiguous forms, so nothing that is parsed, sorted, or pasted into an issue depends on the reader's zone: `timestamp` stays ISO-8601 UTC and `timestamp_ms` the raw epoch value. The unit tests derive their expected prefix from the same instant with the same field accessors, rather than hardcoding one: a literal `"14:45:32"` would have passed only on a UTC machine. One case additionally pins the zone choice itself — asserting the output is *not* the UTC rendering — guarded so it stays meaningful on a UTC machine, where the two coincide. Verified green under `TZ=Asia/Tokyo`, `TZ=UTC`, and the ambient zone.
Opt-in, matching `workers push --wait`: long-running behaviour in this family is asked for, never defaulted. **The poll interval is set by the rate limit, not by responsiveness.** The v1 analytics endpoints allow 10 requests per 60 seconds, so the two-second poll a live tail suggests would spend the whole allowance in ten seconds. Six seconds is the arithmetic floor; ten leaves room for the history query, the deployed-worker check, and a retry in the same window. Measured at ~7 requests in the worst 60-second window. The interval is in `--follow`'s help text, because a 10-second tail is visibly not a live stream and would otherwise look broken. The cursor deliberately lags 60 seconds behind the newest line printed. Guest lines are relayed CloudWatch -> subscription filter -> Lambda -> Logflare and arrive late and out of order, so a cursor sitting on the newest timestamp would drop every straggler permanently. Overlap is therefore guaranteed and expected; dedupe on the Logflare-minted `id` is what makes it invisible, bounded so a long tail does not grow the set forever. `followWindow` clamps to the same sub-24h span as a bounded read, so a tail resumed after a laptop suspend cannot ask for a wider window — the server answers those by returning an *older* slice. Every poll sends both timestamp bounds. Advancing only `iso_timestamp_start` is the obvious implementation and is wrong: it yields a one-minute window. Output: - `-o json|yaml|toml` and `--output-format json` are refused up front, beside the `-o env` refusal and for the same reason — each promises one terminal payload and a tail has no last element. - `--output-format stream-json` emits one `log-entry` event per line instead of a single `result`, reusing the existing variant. `stream` splits error/warn to `stderr`; `source` separates backlog from live. - SIGINT exits 130, matching the local `supabase logs` command. - `--tail 0` skips the backlog and makes no history request, since the endpoint rejects `limit 0`. It also suppresses the not-deployed check, which would otherwise read "no rows" as "no worker" when no query was made at all. Both schedules are injectable, as `awaitWorkerBuild`'s are, so the cursor, dedupe and retry paths are tested without a wall clock. The SIGINT test forks the handler and synchronises on the mock's `awaitExit` — `exit` never returns, so the handler cannot be awaited. Stressed over five consecutive runs.
…/supabase/cli into FUNC-851/general-output-polish
Three separate breaks, all from the same PR: - `WORKER_LOG_CURSOR_GRACE_SECONDS` is only read by `logWindow` in its own file, so the `export` was dead and knip failed the quality job. Dropped. - `--kind` is a value-consuming long flag, and the repo-wide completeness guard in `legacy-db-target-flags` requires every one of them to be registered or the DB-target scanner mis-reads the token after it. - The colour test supplied a fake stream but not a fake environment. The gate checks `CI` before it asks the stream, so the assertion only held on a developer machine and failed in CI. Stubbed the same four variables `legacy-colors.unit.test.ts` stubs.
…into FUNC-853/workers-logs-command
…ploy-wait-flag # Conflicts: # apps/cli/src/legacy/commands/experimental/workers/push/push.integration.test.ts
`git diff-tree --stdin` given a cwd outside any repository exits before it reads a hash, so the write that follows can land on a dead process and raise `EPIPE: broken pipe, send` instead of the exit-code error the caller reports. Which one surfaces is a race against process startup — green on an idle machine, red on a loaded CI runner. The exit code and stderr are the diagnosis, so a broken pipe on stdin is dropped and the reporting left to them.
kanadgupta
left a comment
kanadgupta
left a comment
There was a problem hiding this comment.
LGTM, confirmed this works as expected! claude left three pieces of feedback that seem worth looking into, but happy with whatever call you make
[!NOTE]
This review was drafted by an AI (Claude) and is pending human vetting. Refreshed after new commits landed (head6a0bab9; the delta was mostly a merge of the stacked base, plus a few in-PR touches reviewed below).What this PR does
Makes
workers pushreturn as soon as the platform accepts the deploy, instead of blocking minutes on the server-side container build;--waitopts back into the old blocking behavior. A deploy answered withbuild_state: failedstill exits 1 in both modes. The no-wait text output leads the details block with aStaterow, drops the (nonexistent)Imagerow, and emits a success trailer pointing atworkers statusand--wait; machine callers readbuild_statefrom the payload. SIDE_EFFECTS.md and the exit-code table are updated to match. Since the last review round the PR also gained aoutput.format === "text"gate on the per-workerDeploying Worker n/Nprogress line (keeping it out of--output-format json, with a test), a telemetry span rename tolegacy.experimental.workers.push, and a comment reword in the boolean-flag-defaults guard — all clean, no new findings.Does it deliver FUNC-848?
Yes. The ticket asks for exit-on-optimism by default and blocking under
--wait, and that is exactly what ships. The default flip (previously push blocked and failed on a bad build; now it exits 0 while the build runs) is handled carefully: theStaterow, the trailer hint,build_statein the machine payload, the flag description naming the default, and updated exit-code docs. Anyone gating CI on the old blocking behavior needs to add--wait, but on an experimental-namespace alpha command that is an acceptable, well-signposted change. Tests sit at the right places on the pyramid (integration-level, schedule seams injected, no sleeps), and the polling itself still runs through the pre-existingSchedule-basedawaitWorkerBuild.Findings
Graded for an experimental command: "fix before merge" is reserved for correctness/data-loss/security issues of real note; everything else is a follow-up candidate.
Fix before merge: none.
Follow-up candidates (inline):
- With
--wait, a deploy response that already carries a terminalbuild_state(failed/active) still enters the poll loop; in the 404-race / flaky-read cases this can convert an already-reported failure into a 10-minuteWorkerBuildTimeoutError. One-line fix (input.wait && accepted.buildState === "building"). This amplifies an open Codex thread that I verified is correct and still unaddressed.- In a multi-worker no-wait push, an accepted worker's trailer hint is silently dropped if a later worker fails —
run.tsdrainsSuccessTraileronly on exit code 0. Also an open, correct Codex thread; at minimum worth documenting as a tradeoff.- On a re-push of an existing worker, the deploy response may echo the previous
image_versionwhilebuild_stateisbuilding, so the no-wait output (and machine payload) can show a stale image. Question-grade; depends on API behavior the fixtures don't cover.CI
Green. The three failures noted in the earlier round (knip unused export and a color-tinting unit test, both in files owned by the logs PR below this one) were inherited from the stacked base and have since been fixed there.
Existing review threads
The author addressed the earlier Codex rounds thoroughly (the
_tagguard, the--project-refsuffix in hints, the text-only trailer docs). Two open Codex P2s onpush.handler.ts(lines 345 and 416) remain unaddressed and are both correct — my inline comments 1 and 2 amplify them. Three other unresolved Codex threads (delete/status SIDE_EFFECTS.md,new.handler.ts) point at files under the old pre-experimental/paths that are not part of this PR's incremental diff — stack drift; they belong to the PRs below this one. (#6432 now stacks on top of this branch; its changes are out of scope here.)Overall: 0 fix-before-merge issue(s), 3 follow-up candidate(s).
## Summary Makes the `name` argument to `supabase experimental workers new` optional and prompts for it when it is omitted, so a bare `supabase experimental workers new` walks through name, runtime and size rather than failing the parse. The name is the one input this command cannot default — it is the directory, the `[workers.<name>]` key and the hostname all at once. So where the runtime and size prompts fall back to a default when there is nowhere to ask, the name prompt has nothing to fall back to: with `-o json|yaml|toml|env` or no interactive terminal, the command fails with a new `MissingWorkerNameError` pointing at `supabase experimental workers new api`. The prompt validates against everything the command would otherwise refuse a moment later — a non-DNS-label name, and a name `config.toml` already records — so a typo is corrected in place instead of ending the run. That also means the project has to be loaded before the first prompt, and the machine-output check moves up with it: `-o` leaves `output.format` as `text`, and Clack writes its terminal UI to stdout, so a name prompt would land in front of the payload for the same reason the runtime prompt would. The handler's inline name validation is replaced by the shared `legacyValidateWorkerName`, which the rest of the command family already uses, so an explicitly-passed name and a prompted one are refused on identical terms. `mockOutput` now records `promptTextCalls` so tests can assert on the prompt's message and exercise its `validate` callback. ## Stack Bottom of the workers stack, on `develop`. Above it: output polish (supabase#6389), `workers logs` (supabase#6410), and `push --wait` (supabase#6371). ## Linked issue FUNC-840 (Linear). Supabase maintainer, exempt from the `open-for-contribution` flow. ## Checklist - [x] The PR title follows [Conventional Commits](https://www.conventionalcommits.org/)
…base#6389) ## Summary The workers commands each grew their own way of saying "here is what happened" and "here is what to run next". This settles them on the shapes the rest of the legacy shell already uses. **No command changes what it does** — this is output, plus the coverage that pins it. - **Success trailers.** "What to run next" lines in `new`, `push`, `delete` and `status` move to `emitSuccessTrailer`, the way `stop`, `bootstrap`, `migration repair` and `gen signing-key` already emit theirs: printed once at the end of the run rather than inline, so a multi-worker push does not bury each worker's hint under the next worker's output. The commands within them are aqua'd. - **`list` advisories.** Both take the yellow `WARNING:` prefix and the two-line consequence shape `start`'s Docker notice uses. Each was one long sentence that re-flowed at a different width, directly under a table that lines its columns up. - **`list` drops the URL column.** Every worker's URL is the same host and prefix with the name on the end, and carrying it pushed the table past 130 columns for one derivable field — `renderGlamourTable` sizes each column to its widest cell and never wraps. `status` still renders it vertically, and every machine format still carries `url` per worker. - **`push` progress.** Per-worker announcements are counted (`Deploying Worker 1/2:`) and a multi-worker run closes with a summary. Each worker takes minutes; the name alone said nothing about how much of the run was left. - **`push` names what it never attempted.** The loop stops at the first failure and the error only names the worker that broke, leaving the rest to be reconstructed from argument order. On stderr in every format, machine ones included: that run is a CI run. - **`--project-ref` survives into `push`'s retry suggestions**, via the `legacyWorkersProjectRefSuffix` helper `status` and `delete` already use. A suggestion is copy-pasted verbatim, so one that dropped it re-resolved against whatever this checkout was linked to. Also adds unit coverage for `legacyRenderWorkerDetails`, pins the shared `-o env` refusal, and adds a guard (own commit) asserting no legacy boolean flag ships required — `Flag.boolean` alone builds a *required* param, and nothing in the existing suites notices. ## Stack On top of the `workers new` name prompt (supabase#6349). Above it: `workers logs` (supabase#6410), then `push --wait` (supabase#6371) last, so the output work can ship independently of both. ## Linked issue [FUNC-851](https://linear.app/supabase/issue/FUNC-851/general-output-polish). Supabase maintainer, exempt from the `open-for-contribution` flow. ## Checklist - [x] The PR title follows [Conventional Commits](https://www.conventionalcommits.org/) --------- Co-authored-by: kanad <git@kanad.dev>
Waiting on the server-side build goes back to being the default, so a plain `supabase experimental workers push` still reports the build's verdict and existing invocations keep their behaviour. `--no-wait` is the opt-out for an inner-loop redeploy or a CI step that only needs the deploy accepted. The early-return path is unchanged: it still leads the details block with `State`, drops `Image`, reports a deploy answered with `build_state: failed` as a failure, and emits the `workers status` success trailer. The trailer no longer suggests adding `--wait`, since reaching it means the caller asked not to wait.
`V2DeployAWorkerOutput` permits a terminal `active` or `failed` on the deploy response itself, and that verdict belongs to this deploy. Polling on top of it could only contradict it: `awaitWorkerBuild` reads a post-deploy 404 as "still building", so an already-failed deploy could burn the full poll budget and surface as `WorkerBuildTimeoutError` rather than the failure the platform had already reported, and a concurrent deployment could answer with a state from someone else's build. The wait now runs only when the deploy response left `build_state` at `building`, which also drops a redundant `GET` on the common terminal cases.
johnstonmatt
changed the title
--wait flag--no-wait flag
…ogs-command # Conflicts: # apps/cli/src/shared/workers/workers-api.ts
…indow `followWindow`'s `graceSeconds`/`spanMinutes` options and `logWindow`'s `spanMinutes` parameter had no caller and no test — both call sites pass nothing — so they were configurability for its own sake over the module's trickiest arithmetic. Both now read the module constants directly. `followWindow` had no direct unit tests at all. It has four now, including the suspend case the 24h clamp exists for: a cursor left days behind would otherwise ask for an over-wide span, which the server answers by rewriting `end` to `start + 24h` — returning an older slice rather than a truncated one, so a resumed tail would silently replay yesterday. The expectations are written as literals rather than read from the constants under test, so they cannot stay green through the change they exist to catch.
The `FOLLOW_MAX_PAGES` comment claimed rows past the bound were re-asked for on
the next poll. They are not. The drain walks `end` backwards, so the pages it
does fetch are the newest ones, and the cursor then advances to the newest row
printed — past a region it never reached. Only the part of that region inside
the next window's grace comes back.
Lowering the cursor cannot fix it: `followWindow` moves the window's floor, not
its ceiling, so a poll anchored at `now` would re-fetch the same newest pages
and never walk down to the gap. The bound stays, and the loss is now reported
instead of silent — once per run, on stderr, in every output format, since a
`stream-json` consumer cannot infer a hole from the events it receives.
Corrects the rate arithmetic in the same pass. Both the handler comment ("~7
requests in the worst 60-second window") and SIDE_EFFECTS ("6 requests a
minute") predate the multi-page drain and assume one request per poll. A quiet
tail does spend 6 a minute; a poll draining a burst spends up to 5, so a
sustained backlog reaches 30 against a limit of 10 and is throttled by the
retry rather than budgeted for.
…gnosis" This reverts b772491. The fix is sound but has nothing to do with workers logs; it landed here to stabilise this branch's CI. Moved to #6455 off develop so it does not merge or revert with the workers work. Raised in review on #6410. Until #6455 lands, the `packages/config` release-script test it stabilises can flake on a loaded runner.
Two findings from review, both reachable only under `--no-wait`. `runCli` drains success trailers on exit code 0 only, so a multi-worker run whose later worker fails discarded the follow-up hint for every worker already accepted — while their builds carried on running on the platform. The failure path now names them itself: `Still building: api`, a sibling to `Not attempted: web` and on the same terms, stderr in every format, because a machine-format run is a CI run where nobody watched the loop. `image_version` is optional-but-permitted on the deploy response, so a re-push of a worker that is already serving can echo the image it is serving now. Shown beside `State building` that named an image this deploy did not produce, and a script reading `image_version` next to `build_state: "building"` would take it for the new one. Both the row and the payload field are now omitted while the build is still running.
…eploy-wait-flag # Conflicts: # apps/cli/src/legacy/commands/experimental/workers/push/push.integration.test.ts
…ivate-exposure #6371 was squash-merged into develop, so develop's copy of the workers push work shares no ancestry with the merge of it this branch already carries. A plain merge therefore re-conflicted all four push files against content the branch already had. Recorded with an explicit merge base of 539986a — the FUNC-848 tip that squash came from, verified content-identical to develop — which resolves cleanly and leaves the tree unchanged.
…e#6410) ## Summary Reads the project's unified logs stream rather than a worker route — there is no worker-scoped log endpoint — via `v1GetProjectLogs`, which the generated client already carries. `--kind app|requests|builds` narrows to one of the three streams; without it all three are returned, tagged per line. `--tail` caps the rows. `--follow` keeps printing until interrupted. ## Three non-obvious things about that endpoint Each is documented at its call site, because none is guessable from the API surface: - **The filter is `log_attributes`, not the `source` column.** Worker rows carry an empty top-level `source`, because the Workers Logflare source is not enrolled as a category in the generic logs path, so `where source = 'worker_guest_logs'` matches nothing. The `in (...)` list over the three known streams is a tenancy guard rather than a convenience — with `source` empty it is the only thing excluding a non-worker row that happens to carry a `worker` attribute. - **Both timestamp bounds are always sent, spanning under 24h.** One bound alone yields a one-minute window, silently; neither is an outright error; and a span over 24h is clamped to `start + 24h`, returning an *older* slice than the one asked for rather than a truncated one. - **A failed query can arrive as HTTP 200** with a populated `error`, so the envelope is checked before `result`. The response is decoded against a local schema rather than the generated `V1GetProjectLogsOutput`: that schema marks `result`/`error` optional but permits neither to be `null`, while the endpoint always sends one of them as an explicit `null`. Decoding a real response against it always fails — worth fixing in the spec separately. ## Rendering Per-stream, because `event_message` differs in kind: on the request stream it is only `"GET /"`, with status and duration in `log_attributes`, so the request line is composed. `severity_text` is ignored — it is `INFO` on every row of every stream — so the level is derived, and app lines report none rather than a guess. An app message is tenant-controlled bytes, so escape sequences are stripped before it reaches a terminal while a stack trace's newlines and indentation survive. ## `--follow` The poll interval is set by the rate limit, not by responsiveness: the v1 analytics endpoints allow 10 requests per 60 seconds, so the two-second poll a live tail suggests would spend the whole allowance in ten seconds. It polls every 10 seconds, measured at ~7 requests in the worst 60-second window. The cursor deliberately lags 60 seconds behind the newest line printed. Guest lines are relayed CloudWatch → subscription filter → Lambda → Logflare and arrive late and out of order, so a cursor sitting on the newest timestamp would drop every straggler permanently. Overlap is therefore guaranteed; dedupe on the Logflare-minted `id` is what makes it invisible. `-o json|yaml|toml` and `--output-format json` are refused up front — each promises one terminal payload and a tail has no last element. `--output-format stream-json` emits one `log-entry` event per line. SIGINT exits 130. ## Stack On top of the workers output polish (supabase#6389), with `push --wait` (supabase#6371) stacked above so it can be rejected independently. Below those: the `workers new` name prompt (supabase#6349). ## Note Replaces supabase#6408, which GitHub marked merged during a stack reorder. It was never merged to `develop`; the branch and its commits are intact here.
2 participants
Summary
supabase experimental workers pushblocks on the server-side container build. That build routinely runs for minutes, so the common case — a deploy that builds fine — is the slowest thing in the loop.--no-waitreturns once the platform accepts the deploy, which is the last thing the command can learn without waiting: the deploy response arrives only after the spec and the uploaded context are accepted, and it carries the accepted spec back. Waiting stays the default, so a plain push still reports the build's verdict and existing invocations are unchanged.--no-waitthe details block leads with aStaterow — the one row that says the worker is not serving yet — and dropsImage, since no image exists until the build produces one.experimental workers statusfor the build's outcome. Text output only; machine callers readbuild_statefrom the payload.failedis reported as a failure whether or not the build was waited on, rather than exiting zero on a worker that will never come up.The second commit is a separate fix that fell out of the review: the wait now runs only when the deploy response left
build_stateatbuilding.V2DeployAWorkerOutputpermits a terminalactiveorfailedon the deploy response itself, and polling on top of that could only contradict it —awaitWorkerBuildreads a post-deploy 404 as "still building", so an already-failed deploy could burn the full poll budget and surface as a timeout rather than the failure the platform had already reported.Stack
Top of the workers stack, on top of
workers logs(#6410). Everything below it — the name prompt (#6349), the output polish (#6389) andworkers logs— is independent of this flag, so this PR can be rejected on its own without holding any of them up.Linked issue
FUNC-848 (Linear). Supabase maintainer, exempt from the
open-for-contributionflow.Checklist