Standardize wait and progress handling behind one shared waiter - #1946
Conversation
Every `--wait` in doctl polled the API its own way. Some printed a trail of dots, some printed nothing at all, several would poll forever if a resource never settled, and an action that ended in `errored` was reported as a successful wait, which left `--wait` claiming a resource was ready when the API had already given up on it. Route them all through a single waiter. It reports the stage the API is in, bounds every wait with --wait-timeout, and says on timeout that the operation itself is unaffected and how to wait longer, because otherwise a timeout reads like a failed create. Progress degrades by stream rather than being suppressed. A terminal gets one updating line; a pipe or a CI log gets a plain line per stage change, with no glyphs, no escape sequences, and no repeat of a stage that has not moved. Glyphs are a terminal affordance, and a build log is read by grep and by people who never saw the terminal it would have been drawn on. Also replaces the tabwriter-based text displayer with a width-aware renderer, so a table narrows to the terminal attached to stdout instead of wrapping, while piped output stays unconstrained. Co-authored-by: Cursor <cursoragent@cursor.com>
The agents work on the beta line renders its wait and status glyphs as ✓ and ✗, while internal/ui had settled on the heavier ✔ and ✘. Two spinners drawn from the same palette but different checkmarks read as a bug when they sit in the same scrollback, so adopt the agents set. The characters move into consts because commands/charm/text was restating them: it already sourced its colours from charm.Colors, which now resolves to internal/ui, and glyphs were the last hardcoded half. Without this, `auth init` would keep printing ✔ while a --wait printed ✓ - exactly the drift the shared layer exists to prevent. Pending and Bullet are aligned to ⟳ and • for the same reason. Neither has a caller yet, so only the success and failure glyphs change output. Co-authored-by: Cursor <cursoragent@cursor.com>
Styling was decided by two process-wide stacks that both made their
choice before doctl knew anything about the invocation: lipgloss from a
default renderer bound to os.Stdout, and fatih/color from an init-time
check of whether *stdout* was a terminal. Because the check looked at
the wrong stream, `doctl ... 2>log` wrote escape sequences into the log
file and `doctl ... > data` stripped colour from a terminal well able to
show it.
installOutputPolicy runs in the root PersistentPreRunE, after flags and
config are read but before anything writes, resolves one ui.Env, and
points both stacks plus charm's template output at it. That is what lets
the older charm chrome and the remaining fatih/color call sites obey
--color, --output, and NO_COLOR without each of them knowing about
ui.Env.
--color takes auto, always, or never. auto detects per stream, so a
redirected stream losing colour while the other keeps it is expected:
which writer a caller was handed is exactly what decides whether
styling it is safe.
Errors move to stderr and gain one shared label, so a validation error
and an API error read as one voice. The --output json envelope is
unchanged - automation parsing {"errors":[{"detail":...}]} keeps working,
with plain text in detail.
Co-authored-by: Cursor <cursoragent@cursor.com>
These four call sites printed status lines with fmt.Println or a direct os.Stderr write, so they neither carried the Notice label nor followed the writer the invocation had resolved. Sending them through notice() puts them where every other diagnostic already goes. This changes what a script capturing stdout sees: `doctl auth switch` and `auth remove` previously wrote their confirmation to stdout and now write it to stderr, so `out=$(doctl auth switch foo)` comes back empty. The rule being applied is that Out carries data and Err carries chrome - for `keys create` that means the secret still goes to stdout with the table so it can be piped, while the warning about saving it does not. Needs a release note. Co-authored-by: Cursor <cursoragent@cursor.com>
Text output went through text/tabwriter, which measures in bytes and never truncates, so a wide table wrapped into an unreadable block and a double-width or styled value pushed its column out of alignment. Columns are now measured in terminal cells and narrowed to fit, widest first, so the column with the most slack gives up space before the others. A column is never narrowed past its header, which means an unavoidably wide table still overflows rather than becoming useless. Only data on Out is constrained: DataWidth is zero unless stdout is itself a terminal, so a piped table is left exactly as wide as it was. Tone classifies the state words doctl receives from many APIs in three encodings - active, CREATING, SCENARIO_SET_STATUS_READY - and paints them from one palette. It is applied only where a column is named for state and the value is a word the vocabulary knows: state words also appear in prose columns like Message, which must not turn red just because a resource happens to be failing. Unrecognised values are left alone rather than guessed at, and displayers that the shared vocabulary would classify wrongly can override it through Toned. Co-authored-by: Cursor <cursoragent@cursor.com>
The palette now names slots in the terminal's own 16 colours instead of
carrying fixed hex values from the Kraken brand tokens. Every terminal
renders green a little differently, so a fixed value gambles that it stays
readable against whatever background the user picked; naming the slot lets
the user's own theme decide. It also keeps DigitalOcean blue out of the
palette, and it fixes muted text, which downsampled to pure blue on a
16-colour terminal and took the table rules and dim hints with it.
Colour is no longer combined with weight. Most terminals render bold plus
one of the eight base colours as that colour's bright variant, so a bold
label would be a visibly different red from a table cell painted in the
same slot. Bold is now reserved for Highlight, which carries no colour of
its own.
Alongside that:
- The glyph vocabulary matches the design system: "!" for warnings, "i"
for info, "." for cancelled, each with an ASCII fallback.
- Env gains DataTTY and ErrTTY, so box rules and glyphs are gated on the
stream they are written to rather than on whether colour is permitted.
- Text tables are fitted to the terminal, boxed on a screen and left as
plain columns for a pipeline, with state columns toned by meaning.
- A wait on a plain stream repeats an unmoved stage on a heartbeat rather
than falling silent through a long provision, and closes on the
sentence alone rather than on a "Success:" label the format lacks.
- FlagValidationError.Error() returns a one-line summary, so the JSON
error envelope carries a sentence and Display() keeps the rendered
block.
Co-authored-by: Cursor <cursoragent@cursor.com>
The error label carried its glyph whatever it was written to, so a redirected stderr got "✗ Error:" where every script and test matching on doctl's errors expects "Error:" - thirty integration expectations among them. Env.ErrTTY was added for exactly this decision, on the grounds that a symbol standing in for a word is a screen affordance and a log read by grep wants the word, but it had no callers. Gate the glyph on it. Colour is still decided separately, so a terminal under --color=never keeps the symbol and loses only the escape sequences. The process-wide lipgloss profile was pointed at Err. That stack backs the components not yet built on ui.Env - charm's templates, prompts and styled text - and those write to Out, so `doctl auth init > log` wrote escape sequences into the log whenever a terminal happened to be attached to stderr. That is the mirror image of the bug the policy was installed to fix. DataProfile resolves Out's profile for it, while fatih/color keeps following Err, because the one remaining call site writes to color.Error. --color is read back through viper now, the way --output already is, so a `color:` in config.yaml is honoured instead of silently losing to the flag's own default. Co-authored-by: Cursor <cursoragent@cursor.com>
Actions cover the operations whose duration is set by how much data has to be moved rather than by how long a control plane takes to come up: an image transferred between regions, a snapshot of a full disk, a Droplet rebuilt from a custom image. Those run past half an hour often enough that the shared thirty minute default would report a timeout on a perfectly healthy transfer, and a timeout that fires on success is worse than no timeout at all - it teaches the user to pass --wait-timeout reflexively and stop reading it. AddActionWaitFlags gives every wait that polls an action a two hour default instead. The value is carried by the flag rather than by the waiter so that --help states the deadline the command will actually apply, and it stays a default rather than becoming a per-operation ceiling: --wait-timeout overrides it and still means the same thing everywhere. TestEveryWaitIsBounded walks the command tree looking for a --wait without a --wait-timeout beside it, so the pairing AddWaitFlags exists to enforce is checked rather than trusted. Co-authored-by: Cursor <cursoragent@cursor.com>
waitForClusterRunning only recorded the cluster on the branches that ended the poll, so a timeout returned nil and the caller announced that the cluster had vanished while it was in fact still provisioning. Keep the cluster last read and return it however the wait ends, which is what the comment there already promised. RunDropletCreate assigned Droplets to their project after the wait. That was harmless while the wait discarded its own error and is not now that it can time out: a timeout left Droplets running outside the project the user asked for, with nothing recording that it had been asked. A Droplet belongs to a project from the moment it exists, so assign first and wait second. Spinner.Start wrote the animation channels outside the mutex halt reads them under. Nothing races them today, because wait starts and stops on one goroutine, but the type documents itself as safe for concurrent use and a deferred Stop is the pattern it invites. Also widens the integration helper that normalises elapsed times, which matched "(10s)" but not the "(1m40s)" that any wait crossing a minute reports. Co-authored-by: Cursor <cursoragent@cursor.com>
Nothing has imported shiena/ansicolor since the error path stopped writing through fatih/color's package-level Output, but it stayed behind in go.mod, go.sum and vendor/, where the next `make vendor` would have removed it as unrelated noise in someone else's diff. Removed by hand rather than with `go mod tidy`, which on go 1.25 also wants to record test-only dependencies of dependencies that the committed go.sum does not carry. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Manual testing of We should find a way to display these values in full. Truncating important resource details makes the output less user-friendly |
|
the waiting line is quite long: Could we shorten it, something like: |
|
IMO progress only appears with |
| for { | ||
| done, detail, err := poll() | ||
| if err != nil { | ||
| spinner.Fail("Gave up waiting for %s", op.Subject) |
There was a problem hiding this comment.
this path also covers actual failures (errored action, LB, app deploy), not just give-ups. Failed while waiting for %s (or similar) would read better. Timeout already has its own Timed out waiting... message.
There was a problem hiding this comment.
Agreed, that branch fires on an errored action, load balancer, or app deploy just as often as on anything doctl chose to stop waiting for. Changed it to Failed while waiting for %s; the timeout branch keeps its own Timed out waiting for %s so the two stay distinguishable. Updated the assertion in TestWaitReturnsPollError to match.
| } | ||
| }) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
when an action errors, we currently return nil, err even though the action has already been fetched. Returning action, err would allow callers to surface the failed action details in the output instead of only showing the error message
There was a problem hiding this comment.
Done, it returns action, err now, so callers can show the action's final status instead of only the error string. It is still nil when the Get itself failed and nothing was ever fetched. Test updated to expect the errored action back rather than nil.
| if remaining < interval { | ||
| interval = remaining | ||
| } |
There was a problem hiding this comment.
| if remaining < interval { | |
| interval = remaining | |
| } | |
| sleep := interval | |
| if remaining < sleep { | |
| sleep = remaining | |
| } |
instead of mutating interval for the final partial sleep, could we use a local sleep := interval? This keeps the polling interval unchanged for subsequent iterations and makes it clearer that we are only adjusting the current sleep to stay within the deadline
There was a problem hiding this comment.
Applied. The clamp writes to a per-iteration sleep now, so interval keeps its original value. It only ever triggered on the last pass before the deadline, so nothing changes in practice today, but the loop should not depend on that holding.
`droplet create --wait` on a terminal printed the Droplet it had just created as `5…` at `167.71.255…`. Fitting the table to the terminal narrowed every column down to its header, and a Droplet row carries fifteen columns of IDs, addresses and UUIDs, none of which survives being cut: half an ID is not a shorter ID, it is a different one, and the user cannot copy it out of the output or paste it into the next command. A column is now never narrowed past the longest run of its values that has nowhere to break. Prose and the comma-separated lists doctl prints for tags, features and volumes are series of short runs, so those columns still give up space to hold a table within the terminal; a column of names or addresses does not. When those floors leave the table wider than the terminal, it is written at its full width and without rules. Narrowing only the columns that happen to be cuttable would not make such a table fit, so it would cost values and buy nothing, and rules drawn around a row wider than the terminal are broken by the wrap regardless. A wide table therefore reads as it did before doctl fitted anything, with every value intact, and the box is kept for the tables that fit inside it - which is most of them once `--format` has picked the columns the user actually wanted. Co-authored-by: Cursor <cursoragent@cursor.com>
The progress line read `Waiting for Droplet (wait-test2) to become
active (0 of 1 active) (1s)`. Seventy characters, of which the only two
that ever changed were the elapsed seconds. `Waiting for` restates the
spinner, `to become active` restates the closing line, and `0 of 1
active` restates both - a count of one counts nothing.
waitOp gains an Activity: the short present-tense phrase a user reads
while the operation runs, `Creating Droplet (wait-test2)`. Subject stays
as it was and keeps its long form, because it is read after the fact and
out of context - in the timeout error and in the line reporting what
doctl gave up on - where the words the progress line drops are what make
the message actionable. Where one helper serves several commands the
verb comes from the caller, since only it knows whether a database is
being created, forked or migrated.
Detail is now reported only while it says something the activity does
not. The Droplet count appears once there is more than one Droplet to
count, and an action no longer echoes `in-progress`, which is the only
status an action in flight can report and so has never distinguished
anything. A CI log of an action wait is two lines rather than three.
⠹ Creating Droplet (wait-test2) (12s)
✓ Droplet (wait-test2) is active (35s)
Co-authored-by: Cursor <cursoragent@cursor.com>
The poll-error branch fires on an errored action, load balancer, or app deploy as much as on anything doctl chose to stop waiting for. Timeouts have their own line, so this one no longer has to speak for them. Co-authored-by: Cursor <cursoragent@cursor.com>
Clamping to the time left wrote the shortened value back over the poll interval, which every later pass would then have used. Nothing reaches a later pass today, but the loop should not depend on that. Co-authored-by: Cursor <cursoragent@cursor.com>
waitForAction already had the action when it decided the wait had failed, and dropped it. A caller that wants to show the status the API ended on can now do so instead of repeating the error string. Co-authored-by: Cursor <cursoragent@cursor.com>
Fixed in A column is now never narrowed past the longest run of characters in its values that has nowhere to break, so IDs, addresses and UUIDs keep their full width. Prose and the comma-separated lists we print for tags, features and volumes are series of short runs, so those columns still yield space to fit the table. When those floors still leave the table wider than the terminal, it prints at full width without rules, since narrowing only the cuttable columns would not make it fit anyway and box rules wrap into a mess at that width. So a wide table reads as it did before we fitted anything, with every value intact and copy-pasteable, and the box is kept for tables that do fit, which is most of them once |
Done in
Detail now appears only when it adds signal, per your point: the Droplet count shows only once there is more than one to count, and an action no longer echoes |
Not changed here, and I would like to keep it out of this PR, but I do not disagree with the direction. This PR is deliberately mechanical: one waiter, one timeout, one progress renderer, with each command's Two things worth settling before we do it. First, it is per-command rather than global: waiting on Happy to take it as a follow-up. Do you want it on everywhere the wait is typically short, or on for the create commands specifically? |
2 participants
Why
Every
--waitin doctl polled the API its own way:erroredwas reported as a successful wait, so--waitclaimed a resource was ready when the API had already given up on itdoctl compute droplet create --waitwaited insidedo/droplets.goviautil.WaitForActive, silently, unbounded, discarding its own errorWhat
One shared waiter.
commands/wait.goaddswaiter+waitOp, and every wait path now goes through it: actions (which covers nfs, volumes, images and droplet actions), droplets, databases, vector DBs, app deployments, Kubernetes, load balancers, VPC peerings, security scans and partner network connect.Each
waitOpcarries anActivity, the short present-tense phrase shown while the operation runs (Creating database (some-id)), and aSubject, the longer phrase used after the fact in the timeout and failure lines, where the message is read out of context and the extra words are what make it actionable. Where one helper serves several commands the verb comes from the caller, since only it knows whether a database is being created, forked or migrated.Stage detail is appended only where it adds something the activity does not: the raw API status for a database or cluster (
creating,provisioning), progress counts for droplet and deployment waits, where3 of 5 activebeats five separate statuses. A count of one is not reported, and an action no longer echoesin-progress, which is the only status an in-flight action can report.Every wait is bounded. New
--wait-timeout(default 30m), registered with--waitas a pair byAddWaitFlagsso a command cannot offer one without the other. The timeout error says the operation is unaffected and how to wait longer, because otherwise a timeout reads like a failed create.Progress degrades by stream instead of being suppressed. A terminal gets one updating line. A pipe or a CI log gets a plain line per stage change — no glyphs, no escape sequences, and no repeat of a stage that has not moved (a 10s poll across a 20m provision would otherwise write a hundred identical lines), with an unchanged stage repeated on a one-minute heartbeat so a slow job does not read as a hung one. Glyphs are a terminal affordance; a build log is read by grep and by people who never saw the terminal it would have been drawn on.
Terminal:
Pipe or CI:
Progress goes to stderr throughout, so piping data into another program keeps working.
Also in this diff
Two things that are separable from the wait work, called out so reviewers know they are here on purpose. Happy to split either into its own PR if you would rather review them apart:
commands/displayers/output.godropstext/tabwriterfor a renderer that fits a table to the terminal attached to stdout, measuring in terminal cells so styled and double-width values stay aligned. A column is never narrowed past the longest run in its values that has nowhere to break, so IDs, addresses and UUIDs keep their full width while prose and comma-separated lists give up space; half an ID is not a shorter ID, and it cannot be pasted into the next command. When those floors still leave the table wider than the terminal it is written at full width and without rules, since narrowing only the cuttable columns would cost values and still not fit. Piped output stays unconstrained, which is whyui.EnvgainsDataWidthseparately fromWidth.ui.Env.warn/noticewrote throughfatih/colorglobals that decide once, at package init, from whether stdout is a terminal. Sodoctl ... 2>logwrote escape sequences into the log file, anddoctl ... > datastripped colour from a terminal perfectly able to show it.go.modpromotescharmbracelet/x/ansiandspf13/pflagfrom// indirectto direct.vendor/modules.txtalready marked both## explicit, so go.mod was the stale file.Still open
No
Cancelledstage.waiter.waittakes no context and sleeps with a baretime.Sleep, so Ctrl-C kills the process before the deferredStop()runs and a terminal keeps a half-painted frame. Wiring a context throughwaitis a self-contained follow-up;commands/apps.goalready has thesignal.NotifyContextpattern to copy.Defaulting
--waiton for interactive terminals is worth doing but is deliberately not here — this PR leaves every command's default exactly where it was, so a change in when commands return is reviewable on its own. Discussion in the thread below.kubernetes cluster createwarns and continues when its wait fails, so a create that times out can still exit 0. That predates this PR in shape, but--wait-timeoutgives it a new way to happen, so it should be settled before this merges.Test plan
go test ./internal/ui/... ./commands/— passes. Newspinner_test.gocovers the plain path: no escapes, no glyphs, a line per stage change, and dedupe of an unchanged stage.wait_test.gocovers immediate first poll, poll error, timeout, and that progress never reaches stdout.go test ./integration/— all wait-related tests pass. Failure set is identical to the pre-change baseline (registry login and auth init need the macOS Keychain, gradient tests need network).Running action (10101)andDeploying app (...) (0 of 1 steps complete).droplet create --wait, which is what turned up the truncated table values and the over-long progress line fixed in this branch.Made with Cursor