feat(client): a SubmitQueue client library, with list and watch by behinddwalls · Pull Request #570 · uber/submitqueue

behinddwalls · 2026-08-11T21:06:31Z

Summary

Why?

Seeing what a queue was doing meant running the demo tool, which creates pull requests as a side effect. The live table it draws is the good part, and it was trapped inside a tool whose job is generating traffic. Meanwhile the gateway has exposed a paged List RPC that no client has ever called, and the CLI's status reads one request at a time by id — so there was no way to ask what a whole queue was doing without adding to it.

Underneath that sat a duplication problem heading somewhere worse. Three binaries dialled the gateway for themselves, parseStrategy existed twice verbatim, and the demo was growing a second copy of everything the CLI would eventually need. Extracting only the table would have treated the symptom.

What?

submitqueue/client is now the client for the domain: dialling, the calls made against a gateway, and the terminal view of a queue. Both binaries become thin over it — the CLI is flag parsing, and the demo is GitHub scaffolding plus calls into the library. The demo's main.go drops from 1069 lines to 369 with no change in what it does, and its GitHub REST helpers move to their own file, since they are not SubmitQueue client code.

list and watch. list draws a queue's recent requests once, following continuation tokens so a caller gets the answer rather than a cursor. watch seeds its rows from the same listing, or from named ids, and then follows them with the tracker that already existed. Its set is fixed when it starts: a watch that grew as its queue did would never finish, and finishing is what makes it usable from a script — it exits non-zero if anything settles anywhere other than landed, the contract the demo used to own alone.

Addressing. -addr is unchanged and passed to the dialler untouched, so dns:///host:port and unix:///path.sock work alongside a plain host:port. Transport security is a separate -tls flag rather than part of the address, because gRPC keeps target resolution and credentials apart — there is no scheme meaning "use TLS", and inventing one would only mislead. The demo's odd -gateway flag is renamed to -addr to match the three real clients.

The changes column. It was the one part of the table tied to having created the pull requests. A row now carries cells of text and an optional URL, supplied by the caller: the demo passes pull request numbers linked to their pages, and a client watching a queue it did not create passes the change URIs the gateway reports. The hyperlink and width handling is shared, including that padding counts on-screen width — a hyperlink is mostly escape bytes occupying no columns.

Credentials. The client can present a bearer token. TokenEnv names the variable holding it rather than carrying the token, so a secret never reaches a command line, and an unset variable sends nothing rather than failing. Nothing in this repository checks it — the gateway admits every caller — so it is there for a gateway reached through something that does: a proxy, a sidecar, an ingress terminating auth ahead of the service. gRPC refuses per-RPC credentials on an insecure connection unless they declare they do not need transport security, which is why the credential declares it and -tls stays a separate choice.

Test Plan

✅ bazel test //... — all 112 targets pass, including the Docker-backed integration and end-to-end suites.

✅ The extraction is behaviour-preserving by construction: all 23 view tests moved to submitqueue/client and pass unchanged there. None was lost — the demo had 28, and 23 plus the 5 file-layout tests that stayed accounts for all of them.

✅ TestCredentialsReachTheServerOverPlaintext runs a real gRPC server on a loopback port and asserts the token arrives as Bearer …. This is the case that silently breaks otherwise: gRPC refuses per-RPC credentials on an insecure connection unless they declare they do not need transport security, so a token that works over TLS can simply never be sent without it.

✅ List paging: pages are followed to the end, a limit cuts across pages without over-fetching, an empty page ends the walk so a server that never stops handing out tokens cannot spin, and -since becomes a receipt-time bound while its absence leaves the window open.

Not driven by hand: list and watch have not been pointed at a running gateway, so the end-to-end shape of the rendered table against real data is unverified. Their paging, settle and verdict logic is covered hermetically above.

Also in this PR

feat(client): wrap the stage at the terminal's width, never truncate
feat(demo): open independent pull requests in parallel
fix(demo): let demo-queue inherit the fake build runner

_{Each commit above carries its own rationale and test plan in its message.}

mnoah1 · 2026-08-12T22:57:54Z

+		return nil, fmt.Errorf("queue must not be empty")
+	}
+
+	req := &pb.ListRequest{Queue: q.Queue, PageSize: int32(q.PageSize)}


ReceivedBeforeMs is never set, but the gateway's list controller rejects any request where received_at_or_after_ms >= received_before_ms (list.go:85). With Since unset you send (0, 0); with -since 1h you send (now-1h, 0) — both invalid, so every list and watch call fails with InvalidArgument against a real gateway. Every other caller in the repo sets both bounds. Set req.ReceivedBeforeMs = time.Now().UnixMilli() once before the loop and keep it fixed across pages, since the continuation token pins both bounds.

## Summary ### Why? Seeing what a queue was doing meant running the demo tool, which creates pull requests as a side effect. The live table it draws is the good part, and it was trapped inside a tool whose job is generating traffic. Meanwhile the gateway has exposed a paged `List` RPC that no client has ever called, and the CLI's `status` reads one request at a time by id — so there was no way to ask what a whole queue was doing without adding to it. Underneath that sat a duplication problem heading somewhere worse. Three binaries dialled the gateway for themselves, `parseStrategy` existed twice verbatim, and the demo was growing a second copy of everything the CLI would eventually need. Extracting only the table would have treated the symptom. ### What? `submitqueue/client` is now the client for the domain: dialling, the calls made against a gateway, and the terminal view of a queue. Both binaries become thin over it — the CLI is flag parsing, and the demo is GitHub scaffolding plus calls into the library. The demo's `main.go` drops from 1069 lines to 369 with no change in what it does, and its GitHub REST helpers move to their own file, since they are not SubmitQueue client code. **`list` and `watch`.** `list` draws a queue's recent requests once, following continuation tokens so a caller gets the answer rather than a cursor. `watch` seeds its rows from the same listing, or from named ids, and then follows them with the tracker that already existed. Its set is fixed when it starts: a watch that grew as its queue did would never finish, and finishing is what makes it usable from a script — it exits non-zero if anything settles anywhere other than `landed`, the contract the demo used to own alone. **Addressing.** `-addr` is unchanged and passed to the dialler untouched, so `dns:///host:port` and `unix:///path.sock` work alongside a plain `host:port`. Transport security is a separate `-tls` flag rather than part of the address, because gRPC keeps target resolution and credentials apart — there is no scheme meaning "use TLS", and inventing one would only mislead. The demo's odd `-gateway` flag is renamed to `-addr` to match the three real clients. **The changes column.** It was the one part of the table tied to having created the pull requests. A row now carries cells of text and an optional URL, supplied by the caller: the demo passes pull request numbers linked to their pages, and a client watching a queue it did not create passes the change URIs the gateway reports. The hyperlink and width handling is shared, including that padding counts on-screen width — a hyperlink is mostly escape bytes occupying no columns. **Credentials.** The client can present a bearer token. `TokenEnv` names the variable holding it rather than carrying the token, so a secret never reaches a command line, and an unset variable sends nothing rather than failing. Nothing in this repository checks it — the gateway admits every caller — so it is there for a gateway reached through something that does: a proxy, a sidecar, an ingress terminating auth ahead of the service. gRPC refuses per-RPC credentials on an insecure connection unless they declare they do not need transport security, which is why the credential declares it and `-tls` stays a separate choice. ## Test Plan ✅ `bazel test //...` — all 112 targets pass, including the Docker-backed integration and end-to-end suites. ✅ The extraction is behaviour-preserving by construction: all 23 view tests moved to `submitqueue/client` and pass unchanged there. None was lost — the demo had 28, and 23 plus the 5 file-layout tests that stayed accounts for all of them. ✅ `TestCredentialsReachTheServerOverPlaintext` runs a real gRPC server on a loopback port and asserts the token arrives as `Bearer …`. This is the case that silently breaks otherwise: gRPC refuses per-RPC credentials on an insecure connection unless they declare they do not need transport security, so a token that works over TLS can simply never be sent without it. ✅ `List` paging: pages are followed to the end, a limit cuts across pages without over-fetching, an empty page ends the walk so a server that never stops handing out tokens cannot spin, and `-since` becomes a receipt-time bound while its absence leaves the window open. Not driven by hand: `list` and `watch` have not been pointed at a running gateway, so the end-to-end shape of the rendered table against real data is unverified. Their paging, settle and verdict logic is covered hermetically above.

## Summary ### Why? The stage column was cut at a hard-coded 120 columns, and the cut fell at the end of the trail — which is exactly where the request currently is. Once the pipeline began reporting its finer stages, an ordinary trail outgrew the line and the run read like this, with the interesting part missing: ``` demo-queue/1 #147 22s accepted → started → validating → validated → batched → speculating → speculated → la… ``` The 120 was never a measurement. The renderer redraws in place by moving the cursor back over the lines it emitted, and a line that wraps physically occupies two rows, which desyncs every redraw after it — so the width had to be bounded somehow, and a constant was cheaper than asking. That trade was invisible while trails were short. ### What? The renderer now asks the terminal how wide it is, and wraps rather than cuts when a trail still will not fit. Asking first is what matters: on a wide window the whole trail simply fits on one line, and nobody is held to the narrowest window anyone might have. The fallback is the old constant, used whenever there is no size to discover — a pipe, a file, a CI log — where a stable width is what a log wants anyway. It asks before every draw, not once at startup. The width is not a property of the process: a watch runs for minutes, and a window dragged narrower inside them leaves every later frame wrapped to a width the window no longer has. The terminal then wraps those lines itself — mid-word, ignoring the column alignment — and because the redraw counts the lines it emitted rather than the lines that appeared, it drifts further with every frame. Sampling once traded that away for an `ioctl` per second. Knowing the width is also what decides whether to redraw in place at all. Detecting a terminal and measuring it were two separate probes, so a terminal that answered the first and not the second got wrapped to the fallback constant — 120 columns of table in whatever window the reader actually had. They are now one question: no size, no wrapping, render as a log. When a trail is longer than the line even so, it wraps onto continuation lines indented under the stage column, the way a wrapped error already does. This keeps the redraw honest rather than working around it: every line is one the renderer produced and counted, so the cursor arithmetic still holds, and nothing is ever cut. Piped output is left on a single unwrapped line, since a log is easier to read and grep that way and has no width to respect. ## Test Plan ✅ `bazel test //submitqueue/client:go_default_test` — 75 cases pass, including the pre-existing redraw-accounting ones that pin the property this all rests on: no emitted line exceeds the width. ✅ Seven new cases: a long trail wraps instead of truncating and every status survives it, the end of the trail is on the last line, continuations align under the stage column, no line exceeds the width at 80/100/120/200 columns, a 240-column terminal needs no wrapping at all, piped output stays on one line, and a window too narrow for the columns still wraps to a readable floor rather than one word per line. ✅ The zero-value renderer that tests construct directly falls back to the default width rather than collapsing, which is what keeps the moved tests working unchanged. ✅ A resize is followed: a table drawn at 200 columns and redrawn after the window narrows to 94 keeps every line inside 94. This is the case that reaches a reader as a trail cut mid-word at the window's edge, since the terminal breaks anything the renderer lets past it. ✅ A probe that fails once leaves the last known width alone rather than snapping to the fallback, which on a narrow window is wider than the window. ✅ A terminal whose size cannot be read does not redraw in place, so wrapping never runs against a guessed width.

## Summary ### Why? Opening a pull request is several round trips to the provider — cut a branch, commit each file, open the request — and the run did them one after another. For a large `-count` that was most of the run's wall time, spent waiting on the network rather than on the queue. Worse for a demo whose whole subject is contention: a request the queue has not been given yet cannot contend with anything. Serial creation delayed the overlap the tool exists to show, so the early part of every run was the least interesting part of it. Nothing about independent changes required the wait. Each branches from the same base and writes files no other change touches — the sharded paths guarantee that — so the ordering was an artifact of the loop. ### What? Independent pull requests are now created concurrently, bounded by `-concurrency` (default 5, `CONCURRENCY` on `make demo-pr`). Each is still enqueued the moment it exists, so the queue starts working sooner as well as being fed faster. The limit is deliberate rather than arbitrary. The provider is a shared service with its own opinion about burst rates, and the point of the tool is to feed the queue, not to discover how fast a repository can be hammered. Lower it if a provider starts refusing bursts. `createAndEnqueue` splits into the two shapes it always had, which were tangled together in one loop: - **independent** runs through a bounded group, collecting into an indexed slice so the run's own order survives workers finishing in whatever order the provider answers them. - **stacked** stays strictly sequential, and cannot be otherwise: each change is based on the branch of the one before it and must see its content, so the next branch cannot be cut until the previous head exists. It ignores `-concurrency` rather than pretending to honour it. The shared state the workers touch — the tracker's rows and the table — was already mutex-guarded, because the status poll has always run concurrently with creation. The GitHub client holds only immutable fields and builds a fresh request per call. ## Test Plan ✅ `bazel test //service/submitqueue/demo/pr:go_default_test` — configuration validation now covers the new flag: zero and negative concurrency are rejected, one is accepted as plain sequential rather than treated as invalid, and the run shape reports the limit only when it is above one. ✅ `bazel test //submitqueue/client:go_default_test --features=race` — clean under the race detector, including `TestTrackerConcurrentPollAndUpdate`, which drives concurrent updates against a running poll. That is the shared state these workers now contend for, and the reason no new locking was needed. ✅ `bazel test //...` — 103 packages pass. Not run against a live provider: this needs a real repository and token, and Docker image builds are failing in this environment. What a live run would add is the provider's own reaction to five concurrent creators — burst limits and secondary rate limits — which is exactly what the flag exists to turn down, and what no local test can tell us.

## Summary ### Why? `demo-queue` pinned the GitHub Actions build runner, so every land waited on a real CI run. That is not what the demo is for: it exists to show the queue batching, speculating and merging, and a walkthrough that spends most of its time watching a workflow spin says very little about any of that. The configuration had also drifted from its own documentation, which already describes the fake runner as the default and real CI as the thing you opt into. ### What? The queue no longer names a build runner, so it inherits `{type: fake}` from the defaults and every build succeeds instantly. A land now completes in seconds. The comment that offered the Actions block said to "replace the line above with the block below", which after this points at the change provider rather than at anything to do with builds. It now says to add the block, and explains what inheriting the default actually gets you. Real CI remains one uncommented block away, and the how-to still documents the three things it needs. ## Test Plan ✅ `make local-provider-start PROVIDER=github` parses the configuration on startup and refuses to start on an invalid one, so a malformed profile fails loudly rather than silently falling back. ✅ No code change: the fake runner is an existing implementation already used by every other queue in this file and by the local provider.

behinddwalls marked this pull request as ready for review August 11, 2026 21:08

behinddwalls requested review from a team and sbalabanov as code owners August 11, 2026 21:08

behinddwalls force-pushed the preetam/sq/status-client branch from c3ea776 to b4399e2 Compare August 11, 2026 22:40

behinddwalls force-pushed the preetam/sq/status-client branch from b4399e2 to dd197d9 Compare August 11, 2026 22:52

behinddwalls force-pushed the preetam/sq/status-client branch 2 times, most recently from 611c99b to 5b00125 Compare August 12, 2026 05:47

behinddwalls force-pushed the preetam/sq/status-client branch from 5b00125 to d80b62e Compare August 12, 2026 19:04

behinddwalls force-pushed the preetam/sq/status-client branch from d80b62e to ac3bee4 Compare August 12, 2026 20:24

behinddwalls force-pushed the preetam/sq/status-client branch from ac3bee4 to 97f0b52 Compare August 12, 2026 20:47

behinddwalls force-pushed the preetam/sq/status-client branch from 97f0b52 to 32d45ad Compare August 12, 2026 21:29

behinddwalls force-pushed the preetam/sq/status-client branch from 32d45ad to 3761445 Compare August 12, 2026 21:37

behinddwalls force-pushed the preetam/sq/status-client branch from 3761445 to dd0dd9a Compare August 12, 2026 21:41

mnoah1 approved these changes Aug 12, 2026

View reviewed changes

behinddwalls force-pushed the preetam/sq/status-client branch from dd0dd9a to ee4735b Compare August 13, 2026 16:31

behinddwalls added 3 commits August 13, 2026 09:55

behinddwalls force-pushed the preetam/sq/status-client branch from ee4735b to 0746c18 Compare August 13, 2026 16:55

behinddwalls mentioned this pull request Aug 13, 2026

feat(gateway): land and status subcommands for the client #547

Open

Navigation Menu

Uh oh!

feat(client): a SubmitQueue client library, with list and watch - #570

feat(client): a SubmitQueue client library, with list and watch#570
behinddwalls wants to merge 4 commits into
sq/demo-prfrom
preetam/sq/status-client

behinddwalls commented Aug 11, 2026 •

edited

Loading

Uh oh!

mnoah1 Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

behinddwalls commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why?

What?

Test Plan

Also in this PR

Uh oh!

mnoah1 Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

behinddwalls commented Aug 11, 2026 •

edited

Loading