From bb7f655a02db60eabe7012f9d505896430f3be65 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Mon, 10 Aug 2026 23:39:52 -0700 Subject: [PATCH 1/5] feat(client): a SubmitQueue client library, with list and watch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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. --- Makefile | 12 +- doc/howto/PROVIDER-E2E.md | 34 + service/submitqueue/demo/pr/BUILD.bazel | 13 +- service/submitqueue/demo/pr/github.go | 125 +++ service/submitqueue/demo/pr/main.go | 792 ++---------------- service/submitqueue/demo/pr/main_test.go | 641 +------------- .../submitqueue/gateway/client/BUILD.bazel | 6 +- service/submitqueue/gateway/client/main.go | 226 +++-- submitqueue/client/BUILD.bazel | 40 + submitqueue/client/README.md | 33 + submitqueue/client/conn.go | 166 ++++ submitqueue/client/conn_test.go | 119 +++ submitqueue/client/land.go | 72 ++ submitqueue/client/query.go | 140 ++++ submitqueue/client/query_test.go | 189 +++++ submitqueue/client/view.go | 463 ++++++++++ submitqueue/client/view_test.go | 658 +++++++++++++++ submitqueue/client/watch.go | 198 +++++ 18 files changed, 2476 insertions(+), 1451 deletions(-) create mode 100644 service/submitqueue/demo/pr/github.go create mode 100644 submitqueue/client/BUILD.bazel create mode 100644 submitqueue/client/README.md create mode 100644 submitqueue/client/conn.go create mode 100644 submitqueue/client/conn_test.go create mode 100644 submitqueue/client/land.go create mode 100644 submitqueue/client/query.go create mode 100644 submitqueue/client/query_test.go create mode 100644 submitqueue/client/view.go create mode 100644 submitqueue/client/view_test.go create mode 100644 submitqueue/client/watch.go diff --git a/Makefile b/Makefile index 6d4a5eed..7dc465c9 100644 --- a/Makefile +++ b/Makefile @@ -57,6 +57,8 @@ DEMO_REPO ?= behinddwalls/sq-demo COUNT ?= 3 FILES ?= 3 STACKED ?= false +SINCE ?= 1h +LIMIT ?= 50 LAND ?= true WATCH ?= true QUEUE ?= demo-queue @@ -159,7 +161,7 @@ demo-pr: ## Create N PRs in the demo repo, enqueue each as it is created, and wa -count $(COUNT) \ -files $(FILES) \ -stacked=$(STACKED) \ - -gateway $(GATEWAY_ADDR) \ + -addr $(GATEWAY_ADDR) \ -queue $(QUEUE) \ -strategy $(STRATEGY) \ -land=$(LAND) -watch=$(WATCH) @@ -227,6 +229,14 @@ land-status: ## Read a landed request's status (SQID=... [QUEUE=demo-queue]) @$(BAZEL) run //service/submitqueue/gateway/client:gateway -- \ -addr $(GATEWAY_ADDR) status -queue $(QUEUE) -sqid $(SQID) +land-list: ## Show a queue's recent requests as a table (QUEUE=demo-queue SINCE=1h LIMIT=50) + @$(BAZEL) run //service/submitqueue/gateway/client:gateway -- \ + -addr $(GATEWAY_ADDR) list -queue $(QUEUE) -since $(SINCE) -limit $(LIMIT) + +land-watch: ## Follow a queue's requests until they settle (QUEUE=demo-queue SINCE=15m LIMIT=50) + @$(BAZEL) run //service/submitqueue/gateway/client:gateway -- \ + -addr $(GATEWAY_ADDR) watch -queue $(QUEUE) -since $(SINCE) -limit $(LIMIT) + license-fix: ## Add missing license headers to source files @$(BAZEL) run //tool/linter/licenseheader -- --fix diff --git a/doc/howto/PROVIDER-E2E.md b/doc/howto/PROVIDER-E2E.md index 5c9a4542..8873ec50 100644 --- a/doc/howto/PROVIDER-E2E.md +++ b/doc/howto/PROVIDER-E2E.md @@ -129,6 +129,40 @@ The command exits non-zero if any request settles anywhere other than `landed`, ## Watching it work +The queue itself is readable without creating any traffic: + +```bash +make land-list # a table of recent requests +make land-list SINCE=24h LIMIT=200 # a wider window +make land-watch # follow them until they settle +``` + +Both draw the same table `make demo-pr` does — the demo tool and the CLI share it — but against whatever the queue already holds, so watching a queue no longer means adding to it. `land-watch` fixes its set when it starts and exits non-zero if any request in that set finishes anywhere other than `landed`, which makes it usable from a script. A request accepted after the watch begins is not picked up: a watch that grew as the queue did would never finish. + +Under the hood these are `client list` and `client watch`, which take a queue and reach any gateway: + +```bash +bazel run //service/submitqueue/gateway/client:gateway -- \ + -addr sq.example.com:443 -tls list -queue my-queue -since 1h +``` + +`-addr` is passed to the dialler untouched, so `dns:///host:port` and `unix:///path.sock` work as well as a plain `host:port`. Transport security is a separate flag rather than part of the address, because gRPC keeps target resolution and credentials apart — there is no `grpcs://` to write. + +Bear in mind that a request reads `batched` for the whole of its active life (see above), so a listing of a busy queue is mostly `batched` rows until the pipeline reports its finer stages. + +### Authentication + +The gateway admits every caller. It is a sandbox stack, and nothing in it checks a credential. + +The client can still present one, for a gateway reached through something that does — a proxy, a mesh sidecar, an ingress that terminates auth ahead of the service. It reads `SQ_TOKEN` by default and sends it as `Authorization: Bearer …`; `-token-env` names a different variable, and an unset one sends nothing rather than failing, which is how it stays usable against a stack that wants no credential. + +```bash +SQ_TOKEN=$(cat ~/.sq-token) bazel run //service/submitqueue/gateway/client:gateway -- \ + -addr sq.example.com:443 -tls list -queue my-queue +``` + +### Service logs + ```bash docker compose -p submitqueue-provider logs -f runway-service ``` diff --git a/service/submitqueue/demo/pr/BUILD.bazel b/service/submitqueue/demo/pr/BUILD.bazel index 28729f35..23f1b641 100644 --- a/service/submitqueue/demo/pr/BUILD.bazel +++ b/service/submitqueue/demo/pr/BUILD.bazel @@ -2,17 +2,16 @@ load("@rules_go//go:def.bzl", "go_binary", "go_library", "go_test") go_library( name = "go_default_library", - srcs = ["main.go"], + srcs = [ + "github.go", + "main.go", + ], importpath = "github.com/uber/submitqueue/service/submitqueue/demo/pr", visibility = ["//visibility:private"], deps = [ - "//api/base/change/protopb:go_default_library", "//api/base/mergestrategy/protopb:go_default_library", - "//api/submitqueue/gateway/protopb:go_default_library", "//platform/base/change/github:go_default_library", - "//submitqueue/entity:go_default_library", - "@org_golang_google_grpc//:go_default_library", - "@org_golang_google_grpc//credentials/insecure:go_default_library", + "//submitqueue/client:go_default_library", ], ) @@ -27,9 +26,7 @@ go_test( srcs = ["main_test.go"], embed = [":go_default_library"], deps = [ - "//api/submitqueue/gateway/protopb:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", - "@org_golang_google_grpc//:go_default_library", ], ) diff --git a/service/submitqueue/demo/pr/github.go b/service/submitqueue/demo/pr/github.go new file mode 100644 index 00000000..53e78039 --- /dev/null +++ b/service/submitqueue/demo/pr/github.go @@ -0,0 +1,125 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "bytes" + "context" + "encoding/base64" + "encoding/json" + "fmt" + "net/http" + "strings" +) + +// githubClient is the slice of GitHub's REST API this tool needs: read a +// branch, create a branch, commit a file, open a pull request. +type githubClient struct { + root string + token string + owner string + repo string +} + +func (g *githubClient) branchSHA(ctx context.Context, branch string) (string, error) { + var out struct { + Object struct { + SHA string `json:"sha"` + } `json:"object"` + } + if err := g.do(ctx, http.MethodGet, "/git/ref/heads/"+branch, nil, &out); err != nil { + return "", err + } + return out.Object.SHA, nil +} + +func (g *githubClient) createBranch(ctx context.Context, branch, fromSHA string) error { + return g.do(ctx, http.MethodPost, "/git/refs", + map[string]string{"ref": "refs/heads/" + branch, "sha": fromSHA}, nil) +} + +// commitFile writes a file on a branch and returns the resulting commit SHA — +// the commit a change URI pins the pull request to. +func (g *githubClient) commitFile(ctx context.Context, branch, path, content, message string) (string, error) { + body := map[string]string{ + "message": message, + "content": base64.StdEncoding.EncodeToString([]byte(content)), + "branch": branch, + } + var out struct { + Commit struct { + SHA string `json:"sha"` + } `json:"commit"` + } + if err := g.do(ctx, http.MethodPut, "/contents/"+path, body, &out); err != nil { + return "", err + } + return out.Commit.SHA, nil +} + +func (g *githubClient) openPR(ctx context.Context, title, head, base string) (int, string, error) { + body := map[string]string{"title": title, "head": head, "base": base, "body": "Opened by service/submitqueue/demo/pr."} + var out struct { + Number int `json:"number"` + HTMLURL string `json:"html_url"` + } + if err := g.do(ctx, http.MethodPost, "/pulls", body, &out); err != nil { + return 0, "", err + } + return out.Number, out.HTMLURL, nil +} + +// do issues one authenticated request against the repository, decoding into out +// when it is non-nil. +func (g *githubClient) do(ctx context.Context, method, path string, body any, out any) error { + endpoint := fmt.Sprintf("%s/repos/%s/%s%s", g.root, g.owner, g.repo, path) + + var payload []byte + if body != nil { + var err error + if payload, err = json.Marshal(body); err != nil { + return fmt.Errorf("encode request for %s: %w", endpoint, err) + } + } + + req, err := http.NewRequestWithContext(ctx, method, endpoint, bytes.NewReader(payload)) + if err != nil { + return fmt.Errorf("build request for %s: %w", endpoint, err) + } + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("Authorization", "Bearer "+g.token) + if payload != nil { + req.Header.Set("Content-Type", "application/json") + } + + resp, err := http.DefaultClient.Do(req) + if err != nil { + return fmt.Errorf("%s %s: %w", method, endpoint, err) + } + defer resp.Body.Close() + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + var detail bytes.Buffer + _, _ = detail.ReadFrom(resp.Body) + return fmt.Errorf("%s %s returned %s: %s", method, endpoint, resp.Status, strings.TrimSpace(detail.String())) + } + if out == nil { + return nil + } + if err := json.NewDecoder(resp.Body).Decode(out); err != nil { + return fmt.Errorf("decode response from %s: %w", endpoint, err) + } + return nil +} diff --git a/service/submitqueue/demo/pr/main.go b/service/submitqueue/demo/pr/main.go index 7d401124..c0f1ffbc 100644 --- a/service/submitqueue/demo/pr/main.go +++ b/service/submitqueue/demo/pr/main.go @@ -45,55 +45,19 @@ package main import ( - "bytes" "context" "crypto/sha256" - "encoding/base64" - "encoding/json" "flag" "fmt" - "net/http" "os" - "sort" "strings" - "sync" "time" - "unicode/utf8" - changepb "github.com/uber/submitqueue/api/base/change/protopb" mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb" - pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" githubchange "github.com/uber/submitqueue/platform/base/change/github" - "github.com/uber/submitqueue/submitqueue/entity" - "google.golang.org/grpc" - "google.golang.org/grpc/credentials/insecure" + "github.com/uber/submitqueue/submitqueue/client" ) -const ( - // pollInterval bounds how often the watcher re-reads every request's history. - pollInterval = 2 * time.Second - - // maxLineWidth caps a redrawn line. A line that wraps occupies two physical - // rows, which permanently desyncs the cursor arithmetic the in-place redraw - // depends on; capping is cheaper than asking the terminal how wide it is. - maxLineWidth = 120 - - // absent is what a cell shows before there is anything to put in it. - absent = "—" - - // minNoteWidth keeps a wrapped error readable even when the columns before - // it have eaten most of the line. - minNoteWidth = 40 -) - -// terminalStatuses are the states a land request settles on. They are keyed off -// the gateway's own vocabulary so this tool cannot quietly drift from it. -var terminalStatuses = map[string]bool{ - string(entity.RequestStatusLanded): true, - string(entity.RequestStatusError): true, - string(entity.RequestStatusCancelled): true, -} - func main() { cfg := parseFlags() if err := run(context.Background(), cfg); err != nil { @@ -112,7 +76,9 @@ type config struct { prefix string land bool watch bool - gateway string + addr string + tls bool + tokenEnv string queue string strategy string token string @@ -130,7 +96,9 @@ func parseFlags() config { flag.StringVar(&c.prefix, "prefix", "demo", "branch name prefix") flag.BoolVar(&c.land, "land", true, "enqueue each pull request as it is created") flag.BoolVar(&c.watch, "watch", true, "watch the requests until they all settle") - flag.StringVar(&c.gateway, "gateway", "localhost:8081", "gateway address") + flag.StringVar(&c.addr, "addr", "localhost:8081", "gateway address") + flag.BoolVar(&c.tls, "tls", false, "dial the gateway with transport security") + flag.StringVar(&c.tokenEnv, "token-env", client.DefaultTokenEnv, "environment variable holding the gateway bearer token") flag.StringVar(&c.queue, "queue", "demo-queue", "queue to land on") flag.StringVar(&c.strategy, "strategy", "SQUASH_REBASE", "merge strategy") flag.Parse() @@ -152,7 +120,7 @@ func run(ctx context.Context, cfg config) error { if !ok || owner == "" || repo == "" { return fmt.Errorf("-repo %q must be owner/name", cfg.repo) } - strategy, err := parseStrategy(cfg.strategy) + strategy, err := client.ParseStrategy(cfg.strategy) if err != nil { return err } @@ -163,14 +131,13 @@ func run(ctx context.Context, cfg config) error { return fmt.Errorf("read %s: %w", cfg.base, err) } - var client pb.SubmitQueueGatewayClient + var sq *client.Client if cfg.land { - conn, err := grpc.NewClient(cfg.gateway, grpc.WithTransportCredentials(insecure.NewCredentials())) + sq, err = client.New(client.Options{Addr: cfg.addr, TLS: cfg.tls, TokenEnv: cfg.tokenEnv}) if err != nil { - return fmt.Errorf("connect to gateway %s: %w", cfg.gateway, err) + return err } - defer conn.Close() - client = pb.NewSubmitQueueGatewayClient(conn) + defer sq.Close() } // A run tag keeps repeated invocations from colliding on branch names, and @@ -181,8 +148,8 @@ func run(ctx context.Context, cfg config) error { // Every row is known before anything is created: one per pull request, or a // single one for a stack, since the whole chain lands as one request. The // table is therefore complete from the first draw and only ever fills in. - t := newTracker(newRows(cfg)) - t.note("starting") + t := client.NewTracker(client.NewRows(rowCount(cfg))) + t.Note("starting") // Statuses are read on their own clock, concurrently with creation. A run // that only started polling once every pull request existed would show an @@ -192,31 +159,40 @@ func run(ctx context.Context, cfg config) error { if cfg.land { polling, stop := context.WithCancel(ctx) defer stop() - go t.poll(polling, client, cfg.queue) + go t.Poll(polling, sq.Gateway(), cfg.queue) } - created, err := createAndEnqueue(ctx, gh, client, cfg, strategy, tag, baseSHA, t) + created, err := createAndEnqueue(ctx, gh, sq, cfg, strategy, tag, baseSHA, t) if err != nil { return err } - t.seal() + t.Seal() if !cfg.land { - t.note("created %d pull request(s), not enqueued", len(created)) + t.Note("created %d pull request(s), not enqueued", len(created)) fmt.Printf("\nEnqueue them with:\n make land PRS=\"%s\"\n", strings.Join(urlsOf(created), " ")) return nil } if !cfg.watch { - t.note("enqueued, not watching") + t.Note("enqueued, not watching") return nil } select { case <-ctx.Done(): return ctx.Err() - case <-t.settled: + case <-t.Settled(): } - return t.conclude() + return t.Conclude() +} + +// rowCount is how many rows the table needs: one per pull request, or a single +// one for a stack, which lands as one request however many changes it carries. +func rowCount(cfg config) int { + if cfg.stacked { + return 1 + } + return cfg.count } func shape(cfg config) string { @@ -243,68 +219,6 @@ func urlsOf(cs []change) []string { return out } -// row is one land request and everything shown about it. A row exists from the -// first draw, before the pull request it will carry has been opened, so the -// table never changes shape while the run is in progress. -type row struct { - // changes are the pull requests the request carries, in caller order. A - // stacked run puts every change on one row. - changes []change - - // sqid is empty until the gateway accepts the request. - sqid string - // submitted is when the gateway accepted it, and starts the elapsed clock. - submitted time.Time - // settled is when a terminal status was first observed, and stops it. - settled time.Time - - // trail is the ordered set of statuses the gateway recorded for the request. - trail []string - status string - note string - done bool -} - -// newRows allocates the rows the run will fill: one per pull request, or a -// single row for a stack, which lands as one request. -func newRows(cfg config) []*row { - n := cfg.count - if cfg.stacked { - n = 1 - } - rows := make([]*row, n) - for i := range rows { - rows[i] = &row{} - } - return rows -} - -// elapsed is how long the request has been with the queue: absent until it is -// accepted, running while it is in flight, and frozen once it settles. -func (rw *row) elapsed() string { - if rw.submitted.IsZero() { - return absent - } - end := time.Now() - if !rw.settled.IsZero() { - end = rw.settled - } - return fmt.Sprintf("%ds", int(end.Sub(rw.submitted).Seconds())) -} - -// stage is the path the request has taken, as the gateway recorded it. The -// waiting marker covers the gap between acceptance and the first recorded -// event, so an accepted request is never shown as though nothing happened. -func (rw *row) stage() string { - if len(rw.trail) > 0 { - return strings.Join(rw.trail, " → ") - } - if rw.sqid != "" { - return "…" - } - return absent -} - // shardDirs is how many nested bucket directories a path carries under the demo // root. Two levels of 256 buckets spread a run's files widely enough that no // directory becomes a dumping ground, while staying shallow enough to read in a @@ -369,24 +283,25 @@ func changeFileCount(tag string, change, min int) int { func createAndEnqueue( ctx context.Context, gh *githubClient, - client pb.SubmitQueueGatewayClient, + sq *client.Client, cfg config, strategy mergestrategypb.Strategy, tag, baseSHA string, - t *tracker, + t *client.Tracker, ) ([]change, error) { created := make([]change, 0, cfg.count) + rows := t.Rows() parentBranch, parentSHA := cfg.base, baseSHA for i := 1; i <= cfg.count; i++ { // A stack is one request, so every change lands on the single row. - target := t.rows[0] + target := rows[0] if !cfg.stacked { - target = t.rows[i-1] + target = rows[i-1] } branch := fmt.Sprintf("%s/%s/%d", cfg.prefix, tag, i) - t.note("creating branch %s", branch) + t.Note("creating branch %s", branch) if err := gh.createBranch(ctx, branch, parentSHA); err != nil { return nil, fmt.Errorf("create branch %s: %w", branch, err) } @@ -399,7 +314,7 @@ func createAndEnqueue( for k := 1; k <= fileCount; k++ { path := changeFilePath(tag, i, k) body := fmt.Sprintf("change %d of run %s\nfile %d of %d\n", i, tag, k, fileCount) - t.note("committing %s (%d/%d)", path, k, fileCount) + t.Note("committing %s (%d/%d)", path, k, fileCount) message := fmt.Sprintf("demo change %d (run %s): file %d of %d", i, tag, k, fileCount) sha, err := gh.commitFile(ctx, branch, path, body, message) @@ -409,7 +324,7 @@ func createAndEnqueue( headSHA = sha } - t.note("opening pull request for %s", branch) + t.Note("opening pull request for %s", branch) number, url, err := gh.openPR(ctx, fmt.Sprintf("demo change %d (run %s)", i, tag), branch, parentBranch) if err != nil { return nil, fmt.Errorf("open pull request for %s: %w", branch, err) @@ -423,7 +338,10 @@ func createAndEnqueue( }.String(), } created = append(created, c) - t.update(func() { target.changes = append(target.changes, c) }) + // The cell is what the table shows for this change: the pull request + // number, clickable where the terminal allows it. + cell := client.Cell{Text: fmt.Sprintf("#%d", number), URL: url} + t.Update(func() { target.Cells = append(target.Cells, cell) }) if cfg.stacked { // The next change builds on this one, so it sees this change's @@ -434,636 +352,32 @@ func createAndEnqueue( if !cfg.land { continue } - t.note("enqueuing #%d", number) - sqid, err := enqueue(ctx, client, cfg, strategy, []change{c}) + t.Note("enqueuing #%d", number) + sqid, err := sq.Land(ctx, cfg.queue, urisOf([]change{c}), strategy) if err != nil { return nil, err } - t.update(func() { target.sqid, target.submitted = sqid, time.Now() }) + t.Update(func() { target.SQID, target.Submitted = sqid, time.Now() }) } // The stack goes in as one request, which is only possible now that every // change in it exists. if cfg.stacked && cfg.land { - t.note("enqueuing the stack") - sqid, err := enqueue(ctx, client, cfg, strategy, created) + t.Note("enqueuing the stack") + sqid, err := sq.Land(ctx, cfg.queue, urisOf(created), strategy) if err != nil { return nil, err } - t.update(func() { t.rows[0].sqid, t.rows[0].submitted = sqid, time.Now() }) + t.Update(func() { rows[0].SQID, rows[0].Submitted = sqid, time.Now() }) } return created, nil } -// enqueue submits one land request carrying the given changes, in order, and -// returns the identifier the gateway assigned it. -func enqueue( - ctx context.Context, - client pb.SubmitQueueGatewayClient, - cfg config, - strategy mergestrategypb.Strategy, - changes []change, -) (string, error) { - uris := make([]string, 0, len(changes)) - for _, c := range changes { - uris = append(uris, c.uri) - } - - resp, err := client.Land(ctx, &pb.LandRequest{ - Queue: cfg.queue, - Change: &changepb.Change{Uris: uris}, - Strategy: strategy, - }) - if err != nil { - return "", fmt.Errorf("land %s failed: %w", labelsOf(changes), err) - } - return resp.Sqid, nil -} - -// labelsOf names the pull requests on a row the way they are shown. -func labelsOf(cs []change) string { - labels := make([]string, 0, len(cs)) +// urisOf is the change URIs the run pinned, in caller order. +func urisOf(cs []change) []string { + out := make([]string, 0, len(cs)) for _, c := range cs { - labels = append(labels, fmt.Sprintf("#%d", c.number)) - } - return strings.Join(labels, ",") -} - -// tracker owns the rows for the duration of the run. Two goroutines touch -// them — creation fills in pull requests and identifiers, polling fills in -// statuses — and both draw the same table, so the mutex is what keeps one from -// redrawing halfway through the other's update. -type tracker struct { - mu sync.Mutex - rows []*row - r *renderer - status string - // sealed records that every request that will be enqueued has been. Without - // it, polling would find nothing outstanding before creation had begun and - // call the run finished. - sealed bool - - // settled closes once every request has reached a terminal status. - settled chan struct{} - once sync.Once -} - -func newTracker(rows []*row) *tracker { - return &tracker{rows: rows, r: newRenderer(), settled: make(chan struct{})} -} - -// note replaces the line under the table and redraws. -func (t *tracker) note(format string, args ...any) { - t.mu.Lock() - defer t.mu.Unlock() - t.status = fmt.Sprintf(format, args...) - t.r.draw(t.rows, t.status) -} - -// update applies a change to the rows and redraws with it. -func (t *tracker) update(fn func()) { - t.mu.Lock() - defer t.mu.Unlock() - fn() - t.r.draw(t.rows, t.status) -} - -// conclude draws the verdict and reports whether everything landed. It reads -// the rows under the lock because a poll may still be applying its last round. -func (t *tracker) conclude() error { - t.mu.Lock() - defer t.mu.Unlock() - t.status = outcome(t.rows) - t.r.draw(t.rows, t.status) - return summarize(t.rows) -} - -// seal declares that nothing further will be enqueued, which is what lets an -// otherwise-finished run conclude. -func (t *tracker) seal() { - t.mu.Lock() - defer t.mu.Unlock() - t.sealed = true - t.signalLocked() -} - -// signalLocked closes settled once there is nothing left to wait for. -func (t *tracker) signalLocked() { - if !t.sealed { - return - } - for _, rw := range t.rows { - if rw.sqid == "" || !rw.done { - return - } - } - t.once.Do(func() { close(t.settled) }) -} - -// poll re-reads statuses until the run finishes or the context ends. -func (t *tracker) poll(ctx context.Context, client pb.SubmitQueueGatewayClient, queue string) { - ticker := time.NewTicker(pollInterval) - defer ticker.Stop() - for { - select { - case <-ctx.Done(): - return - case <-t.settled: - return - case <-ticker.C: - } - t.refresh(ctx, client, queue) - } -} - -// refresh re-reads every request that has been accepted but has not settled. -// -// The reads happen outside the lock. Holding it across a round of RPCs would -// stall creation behind the network, and creation racing ahead is the whole -// point of enqueuing each pull request the moment it exists. -func (t *tracker) refresh(ctx context.Context, client pb.SubmitQueueGatewayClient, queue string) { - t.mu.Lock() - outstanding := make([]*row, 0, len(t.rows)) - for _, rw := range t.rows { - if rw.sqid != "" && !rw.done { - outstanding = append(outstanding, rw) - } - } - total := len(t.rows) - t.mu.Unlock() - - type reading struct { - rw *row - trail []string - status string - note string - } - readings := make([]reading, 0, len(outstanding)) - for _, rw := range outstanding { - // sqid is written once, before the row becomes outstanding, so reading - // it here without the lock is safe. - resp, err := client.GetRequestHistoryByID(ctx, &pb.GetRequestHistoryByIDRequest{Sqid: rw.sqid, Queue: queue}) - if err != nil || resp == nil || len(resp.Events) == 0 { - // A history that is not readable yet is normal right after Land; - // the next tick picks it up. - continue - } - trail, status, note := digest(resp.Events) - readings = append(readings, reading{rw: rw, trail: trail, status: status, note: note}) - } - - t.mu.Lock() - defer t.mu.Unlock() - - settled := 0 - for _, got := range readings { - got.rw.trail, got.rw.status, got.rw.note = got.trail, got.status, got.note - if terminalStatuses[got.status] && !got.rw.done { - // Stamped from the local clock rather than the event timestamp so - // the elapsed column is measured end to end against one clock. - got.rw.done, got.rw.settled = true, time.Now() - } - } - for _, rw := range t.rows { - if rw.done { - settled++ - } - } - - t.status = fmt.Sprintf("%d of %d settled", settled, total) - t.r.draw(t.rows, t.status) - t.signalLocked() -} - -// digest reduces a request's recorded history to the trail worth showing, the -// status it currently holds, and the error the latest event carried. A status -// recorded more than once in a row is one step in the trail, not several. -func digest(events []*pb.HistoryEvent) (trail []string, status, note string) { - if len(events) == 0 { - return nil, "", "" - } - for _, e := range events { - if e == nil || e.Status == "" { - continue - } - if len(trail) > 0 && trail[len(trail)-1] == e.Status { - continue - } - trail = append(trail, e.Status) - } - if last := events[len(events)-1]; last != nil { - status, note = last.Status, last.LastError - } - if status == "" && len(trail) > 0 { - status = trail[len(trail)-1] - } - return trail, status, note -} - -// outcome is the one-line verdict shown under the finished table. -func outcome(rows []*row) string { - landed := 0 - for _, rw := range rows { - if rw.status == string(entity.RequestStatusLanded) { - landed++ - } - } - if landed == len(rows) { - return fmt.Sprintf("all %d request(s) landed", len(rows)) - } - return fmt.Sprintf("%d of %d request(s) did not land", len(rows)-landed, len(rows)) -} - -// summarize fails the run if anything did not land, so a scripted demo notices. -func summarize(rows []*row) error { - var failed []string - for _, rw := range rows { - if rw.status != string(entity.RequestStatusLanded) { - failed = append(failed, fmt.Sprintf("%s=%s", rw.sqid, rw.status)) - } - } - if len(failed) > 0 { - sort.Strings(failed) - return fmt.Errorf("%d of %d request(s) did not land: %s", len(failed), len(rows), strings.Join(failed, ", ")) - } - return nil -} - -// renderer draws the status table, redrawing in place on a terminal and -// appending a fresh block otherwise, so piping the output to a file stays -// readable instead of filling with escape codes. -// -// Column widths only ever grow, so a value that turns out to be wider than the -// header does not make the table jitter as rows fill in. -type renderer struct { - inPlace bool - - wRequest int - wChanges int - wElapsed int - wStage int - - // lastLines is how many lines the previous draw actually emitted, which is - // how far the cursor has to move back to overwrite them. - lastLines int - drawn bool - - // lastBody is the signature of the previous table, so piped output can skip - // a redundant reprint when a step moved but the table did not. - lastBody string -} - -func newRenderer() *renderer { - info, err := os.Stdout.Stat() - tty := err == nil && info.Mode()&os.ModeCharDevice != 0 - return &renderer{ - inPlace: tty, - wRequest: len("REQUEST"), - wChanges: len("CHANGES"), - wElapsed: len("ELAPSED"), - wStage: len("STAGE"), + out = append(out, c.uri) } -} - -func (r *renderer) draw(rows []*row, status string) { - body := r.body(rows) - - if !r.inPlace { - sig := signature(rows) - if sig == r.lastBody { - // Nothing in the table moved; the step that prompted this draw is a - // terminal affordance and has no place in a log. - return - } - r.lastBody = sig - fmt.Println(strings.Join(body, "\n")) - fmt.Println() - return - } - - if r.drawn { - fmt.Printf("\033[%dA", r.lastLines) - } - for _, line := range body { - fmt.Printf("\033[K%s\n", line) - } - fmt.Printf("\033[K\n") - fmt.Printf("\033[K ▸ %s\n", truncate(status, maxLineWidth-4)) - // Every draw emits the body, one blank line, and the status line; moving - // back by exactly this many lines is what keeps the redraw from drifting. - r.lastLines = len(body) + 2 - r.drawn = true -} - -// body renders the header and one line per row. -func (r *renderer) body(rows []*row) []string { - r.fit(rows) - - lines := make([]string, 0, len(rows)+2) - lines = append(lines, - fmt.Sprintf(" %-*s %-*s %*s %s", - r.wRequest, "REQUEST", r.wChanges, "CHANGES", r.wElapsed, "ELAPSED", "STAGE"), - fmt.Sprintf(" %s %s %s %s", - rule(r.wRequest), rule(r.wChanges), rule(r.wElapsed), rule(r.wStage))) - - for _, rw := range rows { - lines = append(lines, r.rowLine(rw)) - lines = append(lines, r.noteLines(rw)...) - } - return lines -} - -// fit grows the columns to hold what the rows now contain. Widths never shrink, -// so the table does not shift under a value that has already been printed — but -// on a terminal the stage column stays inside the line, since its rule would -// otherwise wrap on a long trail and take the redraw with it. -func (r *renderer) fit(rows []*row) { - for _, rw := range rows { - r.wRequest = max(r.wRequest, utf8.RuneCountInString(rw.sqid)) - _, visible := r.changesCell(rw) - r.wChanges = max(r.wChanges, visible) - r.wStage = max(r.wStage, utf8.RuneCountInString(rw.stage())) - } - if r.inPlace { - r.wStage = min(r.wStage, max(len("STAGE"), maxLineWidth-r.prefixWidth())) - } -} - -// prefixWidth is the space every row spends before the stage column. -func (r *renderer) prefixWidth() int { - return 2 + r.wRequest + 2 + r.wChanges + 2 + r.wElapsed + 2 -} - -func (r *renderer) rowLine(rw *row) string { - sqid := rw.sqid - if sqid == "" { - sqid = absent - } - changes, visible := r.changesCell(rw) - - prefix := fmt.Sprintf(" %-*s %s %*s ", - r.wRequest, sqid, pad(changes, visible, r.wChanges), r.wElapsed, rw.elapsed()) - - tail := rw.stage() - if r.inPlace { - // Only the tail can overflow, and unlike the changes cell it never holds - // escape sequences, so it is the one part safe to cut. The budget comes - // from the column widths rather than the rendered prefix, which counts a - // hyperlink's escape bytes that take up no space on screen. - tail = truncate(tail, maxLineWidth-r.prefixWidth()) - } - return prefix + tail -} - -// noteLines renders a request's error under its row, wrapped and indented to -// the stage column. An error is the one thing in the table worth reading in -// full — truncating it to the width of a cell hides the part that says what -// went wrong — so it gets as many lines as it needs instead of an ellipsis. -func (r *renderer) noteLines(rw *row) []string { - if rw.note == "" { - return nil - } - - indent := r.prefixWidth() - // A piped run spends most of the line on URLs, so the wrap width is floored - // rather than allowed to collapse to nothing. - width := max(minNoteWidth, maxLineWidth-indent-2) - - wrapped := wrap(rw.note, width) - lines := make([]string, 0, len(wrapped)) - for i, text := range wrapped { - marker := " " - if i == 0 { - marker = "↳ " - } - lines = append(lines, strings.Repeat(" ", indent)+marker+text) - } - return lines -} - -// wrap breaks text into lines no wider than width, splitting on spaces and -// hard-splitting any single token too long to fit on a line of its own. -func wrap(s string, width int) []string { - if width < 1 { - return nil - } - - var lines []string - current := "" - flush := func() { - if current != "" { - lines = append(lines, current) - current = "" - } - } - - for _, word := range strings.Fields(s) { - for utf8.RuneCountInString(word) > width { - flush() - runes := []rune(word) - lines = append(lines, string(runes[:width])) - word = string(runes[width:]) - } - switch { - case current == "": - current = word - case utf8.RuneCountInString(current)+1+utf8.RuneCountInString(word) <= width: - current += " " + word - default: - flush() - current = word - } - } - flush() - return lines -} - -// changesCell renders the pull requests on a row and reports the width they -// occupy on screen. The two differ on a terminal, where a hyperlink is mostly -// escape bytes that take up no space. -func (r *renderer) changesCell(rw *row) (string, int) { - if len(rw.changes) == 0 { - return absent, utf8.RuneCountInString(absent) - } - - parts := make([]string, 0, len(rw.changes)) - visible := 0 - for _, c := range rw.changes { - label := fmt.Sprintf("#%d", c.number) - if r.inPlace { - parts = append(parts, hyperlink(label, c.url)) - visible += len(label) - continue - } - // Piped output has nothing to click, so the address itself has to be - // readable — and copyable out of a log. - parts = append(parts, c.url) - visible += len(c.url) - } - separator := "," - if !r.inPlace { - separator = " " - } - return strings.Join(parts, separator), visible + len(separator)*(len(parts)-1) -} - -// hyperlink wraps text in an OSC 8 escape so terminals that understand it make -// the text clickable, and the rest simply show the text. -func hyperlink(text, url string) string { - if url == "" { - return text - } - return "\033]8;;" + url + "\033\\" + text + "\033]8;;\033\\" -} - -// pad right-pads a cell to a column width using its on-screen width, which is -// not its length whenever it carries escape sequences. -func pad(s string, visible, width int) string { - if visible >= width { - return s - } - return s + strings.Repeat(" ", width-visible) -} - -func rule(n int) string { - return strings.Repeat("─", n) -} - -// signature is what a piped run treats as the table having moved. The elapsed -// clock is left out on purpose: it advances every second, and a log that -// reprinted the table for that alone would say nothing while saying it often. -func signature(rows []*row) string { - var b strings.Builder - for _, rw := range rows { - fmt.Fprintf(&b, "%s|%s|%s|%s\n", rw.sqid, labelsOf(rw.changes), rw.stage(), rw.note) - } - return b.String() -} - -func truncate(s string, n int) string { - s = strings.ReplaceAll(s, "\n", " ") - if n < 1 { - return "" - } - if utf8.RuneCountInString(s) <= n { - return s - } - return string([]rune(s)[:n-1]) + "…" -} - -func parseStrategy(name string) (mergestrategypb.Strategy, error) { - switch strings.ToUpper(strings.TrimSpace(name)) { - case "", "DEFAULT": - return mergestrategypb.Strategy_DEFAULT, nil - case "REBASE": - return mergestrategypb.Strategy_REBASE, nil - case "SQUASH_REBASE": - return mergestrategypb.Strategy_SQUASH_REBASE, nil - case "MERGE": - return mergestrategypb.Strategy_MERGE, nil - case "PROMOTE": - return mergestrategypb.Strategy_PROMOTE, nil - default: - return mergestrategypb.Strategy_DEFAULT, fmt.Errorf("unknown strategy %q", name) - } -} - -// githubClient is the slice of GitHub's REST API this tool needs: read a -// branch, create a branch, commit a file, open a pull request. -type githubClient struct { - root string - token string - owner string - repo string -} - -func (g *githubClient) branchSHA(ctx context.Context, branch string) (string, error) { - var out struct { - Object struct { - SHA string `json:"sha"` - } `json:"object"` - } - if err := g.do(ctx, http.MethodGet, "/git/ref/heads/"+branch, nil, &out); err != nil { - return "", err - } - return out.Object.SHA, nil -} - -func (g *githubClient) createBranch(ctx context.Context, branch, fromSHA string) error { - return g.do(ctx, http.MethodPost, "/git/refs", - map[string]string{"ref": "refs/heads/" + branch, "sha": fromSHA}, nil) -} - -// commitFile writes a file on a branch and returns the resulting commit SHA — -// the commit a change URI pins the pull request to. -func (g *githubClient) commitFile(ctx context.Context, branch, path, content, message string) (string, error) { - body := map[string]string{ - "message": message, - "content": base64.StdEncoding.EncodeToString([]byte(content)), - "branch": branch, - } - var out struct { - Commit struct { - SHA string `json:"sha"` - } `json:"commit"` - } - if err := g.do(ctx, http.MethodPut, "/contents/"+path, body, &out); err != nil { - return "", err - } - return out.Commit.SHA, nil -} - -func (g *githubClient) openPR(ctx context.Context, title, head, base string) (int, string, error) { - body := map[string]string{"title": title, "head": head, "base": base, "body": "Opened by service/submitqueue/demo/pr."} - var out struct { - Number int `json:"number"` - HTMLURL string `json:"html_url"` - } - if err := g.do(ctx, http.MethodPost, "/pulls", body, &out); err != nil { - return 0, "", err - } - return out.Number, out.HTMLURL, nil -} - -// do issues one authenticated request against the repository, decoding into out -// when it is non-nil. -func (g *githubClient) do(ctx context.Context, method, path string, body any, out any) error { - endpoint := fmt.Sprintf("%s/repos/%s/%s%s", g.root, g.owner, g.repo, path) - - var payload []byte - if body != nil { - var err error - if payload, err = json.Marshal(body); err != nil { - return fmt.Errorf("encode request for %s: %w", endpoint, err) - } - } - - req, err := http.NewRequestWithContext(ctx, method, endpoint, bytes.NewReader(payload)) - if err != nil { - return fmt.Errorf("build request for %s: %w", endpoint, err) - } - req.Header.Set("Accept", "application/vnd.github+json") - req.Header.Set("Authorization", "Bearer "+g.token) - if payload != nil { - req.Header.Set("Content-Type", "application/json") - } - - resp, err := http.DefaultClient.Do(req) - if err != nil { - return fmt.Errorf("%s %s: %w", method, endpoint, err) - } - defer resp.Body.Close() - - if resp.StatusCode < 200 || resp.StatusCode >= 300 { - var detail bytes.Buffer - _, _ = detail.ReadFrom(resp.Body) - return fmt.Errorf("%s %s returned %s: %s", method, endpoint, resp.Status, strings.TrimSpace(detail.String())) - } - if out == nil { - return nil - } - if err := json.NewDecoder(resp.Body).Decode(out); err != nil { - return fmt.Errorf("decode response from %s: %w", endpoint, err) - } - return nil + return out } diff --git a/service/submitqueue/demo/pr/main_test.go b/service/submitqueue/demo/pr/main_test.go index 4b1c9988..dccad12d 100644 --- a/service/submitqueue/demo/pr/main_test.go +++ b/service/submitqueue/demo/pr/main_test.go @@ -15,648 +15,14 @@ package main import ( - "context" "fmt" - "io" - "os" "strings" - "sync" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" - "google.golang.org/grpc" ) -func TestDigest(t *testing.T) { - tests := []struct { - name string - events []*pb.HistoryEvent - wantTrail []string - wantStatus string - wantNote string - }{ - { - name: "no events yet", - }, - { - name: "one event", - events: []*pb.HistoryEvent{{Status: "accepted"}}, - wantTrail: []string{"accepted"}, - wantStatus: "accepted", - }, - { - name: "trail keeps the order it was recorded in", - events: []*pb.HistoryEvent{ - {Status: "accepted"}, {Status: "started"}, {Status: "batched"}, {Status: "landed"}, - }, - wantTrail: []string{"accepted", "started", "batched", "landed"}, - wantStatus: "landed", - }, - { - name: "a status recorded twice in a row is one step", - events: []*pb.HistoryEvent{ - {Status: "accepted"}, {Status: "started"}, {Status: "started"}, {Status: "batched"}, - }, - wantTrail: []string{"accepted", "started", "batched"}, - wantStatus: "batched", - }, - { - name: "a status revisited later is a step again", - events: []*pb.HistoryEvent{ - {Status: "speculating"}, {Status: "batched"}, {Status: "speculating"}, - }, - wantTrail: []string{"speculating", "batched", "speculating"}, - wantStatus: "speculating", - }, - { - name: "the error on the latest event is the one shown", - events: []*pb.HistoryEvent{ - {Status: "started", LastError: "transient"}, {Status: "error", LastError: "merge conflict"}, - }, - wantTrail: []string{"started", "error"}, - wantStatus: "error", - wantNote: "merge conflict", - }, - { - name: "events without a status do not become steps", - events: []*pb.HistoryEvent{{Status: ""}, {Status: "accepted"}, {Status: ""}}, - wantTrail: []string{"accepted"}, - wantStatus: "accepted", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - trail, status, note := digest(tt.events) - assert.Equal(t, tt.wantTrail, trail) - assert.Equal(t, tt.wantStatus, status) - assert.Equal(t, tt.wantNote, note) - }) - } -} - -func TestRowElapsed(t *testing.T) { - now := time.Now() - - tests := []struct { - name string - row row - want string - }{ - { - name: "absent before the gateway accepts it", - row: row{}, - want: absent, - }, - { - name: "running while in flight", - row: row{submitted: now.Add(-5 * time.Second)}, - want: "5s", - }, - { - name: "frozen once settled, however long ago that was", - row: row{submitted: now.Add(-90 * time.Second), settled: now.Add(-60 * time.Second)}, - want: "30s", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, tt.row.elapsed()) - }) - } -} - -// TestRowElapsedStopsAtSettle pins the behavior the clock exists for: a settled -// row reads the same however much later it is drawn, while an unsettled one -// does not. -func TestRowElapsedStopsAtSettle(t *testing.T) { - start := time.Now().Add(-time.Minute) - settled := row{submitted: start, settled: start.Add(10 * time.Second)} - inFlight := row{submitted: start} - - first := settled.elapsed() - time.Sleep(time.Millisecond) - assert.Equal(t, first, settled.elapsed()) - assert.NotEqual(t, first, inFlight.elapsed()) -} - -func TestRowStage(t *testing.T) { - tests := []struct { - name string - row row - want string - }{ - { - name: "nothing to report before the request exists", - row: row{}, - want: absent, - }, - { - name: "accepted but nothing recorded yet", - row: row{sqid: "demo-queue/17"}, - want: "…", - }, - { - name: "the states it passed through", - row: row{sqid: "demo-queue/17", trail: []string{"accepted", "started", "landed"}}, - want: "accepted → started → landed", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, tt.row.stage()) - }) - } -} - -func TestChangesCell(t *testing.T) { - one := []change{{number: 41, url: "https://github.com/o/r/pull/41"}} - two := []change{ - {number: 41, url: "https://github.com/o/r/pull/41"}, - {number: 421, url: "https://github.com/o/r/pull/421"}, - } - - t.Run("a terminal gets short clickable labels", func(t *testing.T) { - r := &renderer{inPlace: true} - text, visible := r.changesCell(&row{changes: two}) - - assert.Contains(t, text, "https://github.com/o/r/pull/41") - assert.Contains(t, text, "\033]8;;") - // "#41,#421" occupies eight columns however many escape bytes carry it. - assert.Equal(t, len("#41,#421"), visible) - assert.Greater(t, len(text), visible, "the escapes should not be counted as width") - }) - - t.Run("a pipe gets the addresses themselves", func(t *testing.T) { - r := &renderer{inPlace: false} - text, visible := r.changesCell(&row{changes: one}) - - assert.Equal(t, "https://github.com/o/r/pull/41", text) - assert.Equal(t, len(text), visible) - assert.NotContains(t, text, "\033") - }) - - t.Run("no pull requests yet", func(t *testing.T) { - r := &renderer{inPlace: true} - text, visible := r.changesCell(&row{}) - - assert.Equal(t, absent, text) - assert.Equal(t, 1, visible) - }) -} - -// TestPadCountsVisibleWidth guards the alignment trap: a hyperlinked cell is -// mostly escape bytes, so padding by length would push the columns apart. -func TestPadCountsVisibleWidth(t *testing.T) { - linked := hyperlink("#41", "https://github.com/o/r/pull/41") - - assert.Equal(t, linked+" ", pad(linked, len("#41"), 10)) - assert.Equal(t, "#41 ", pad("#41", 3, 10)) - assert.Equal(t, "#41", pad("#41", 3, 3), "a cell at the column width is not padded") - assert.Equal(t, "#41", pad("#41", 3, 2), "a cell wider than the column is left alone") -} - -func TestTruncate(t *testing.T) { - tests := []struct { - name string - in string - n int - want string - }{ - {name: "short enough to keep", in: "accepted", n: 20, want: "accepted"}, - {name: "exactly the limit", in: "accepted", n: 8, want: "accepted"}, - {name: "cut with a marker", in: "accepted → started", n: 10, want: "accepted …"}, - {name: "newlines flattened", in: "line\nbreak", n: 20, want: "line break"}, - {name: "no room at all", in: "accepted", n: 0, want: ""}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, truncate(tt.in, tt.n)) - }) - } -} - -// TestTruncateSplitsOnRunes checks that a cut lands between characters. The -// trail is joined with a multi-byte arrow, so cutting by bytes would leave -// mojibake in the middle of the table. -func TestTruncateSplitsOnRunes(t *testing.T) { - got := truncate("accepted → started → batched", 12) - assert.True(t, utf8ValidAndCounted(got, 12), "got %q", got) -} - -func utf8ValidAndCounted(s string, n int) bool { - return len([]rune(s)) <= n && strings.ToValidUTF8(s, "?") == s -} - -// TestDrawLineAccounting is the invariant the in-place redraw rests on: the -// cursor moves back exactly as far as the previous draw reached. Off by one and -// every subsequent draw leaves a stale row on screen. -func TestDrawLineAccounting(t *testing.T) { - r := newRenderer() - r.inPlace = true - rows := []*row{ - {sqid: "demo-queue/17", changes: []change{{number: 41, url: "https://github.com/o/r/pull/41"}}, - submitted: time.Now(), trail: []string{"accepted", "started"}}, - {sqid: "demo-queue/18", changes: []change{{number: 42, url: "https://github.com/o/r/pull/42"}}, - submitted: time.Now(), trail: []string{"accepted"}}, - {}, - } - - first := captureStdout(t, func() { r.draw(rows, "watching") }) - emitted := strings.Count(first, "\n") - assert.Equal(t, emitted, r.lastLines, "the first draw must record how far it reached") - assert.True(t, strings.HasPrefix(first, "\033[K"), "the first draw has nothing to move back over") - - second := captureStdout(t, func() { r.draw(rows, "still watching") }) - assert.True(t, strings.HasPrefix(second, fmt.Sprintf("\033[%dA", emitted)), - "the redraw must move back over exactly the %d lines it wrote, got %q", emitted, head(second, 12)) - assert.Equal(t, strings.Count(second, "\n"), r.lastLines) -} - -// TestDrawStaysWithinLineWidth checks the other half of the redraw contract: a -// line that wraps occupies two physical rows and desyncs the cursor for good. -func TestDrawStaysWithinLineWidth(t *testing.T) { - r := newRenderer() - r.inPlace = true - rows := []*row{{ - sqid: "demo-queue/17", - submitted: time.Now(), - trail: strings.Split(strings.Repeat("speculating ", 30), " "), - note: strings.Repeat("a very long error message ", 10), - }} - - out := captureStdout(t, func() { r.draw(rows, strings.Repeat("status ", 40)) }) - for _, line := range strings.Split(out, "\n") { - line = strings.ReplaceAll(line, "\033[K", "") - assert.LessOrEqual(t, len([]rune(line)), maxLineWidth, "line too wide: %q", line) - } -} - -// TestDrawPipedSkipsClockOnlyRedraws keeps a redirected run's log readable: the -// table is reprinted when it moves, not once a second because the clock did. -func TestDrawPipedSkipsClockOnlyRedraws(t *testing.T) { - r := newRenderer() - r.inPlace = false - rows := []*row{{sqid: "demo-queue/17", submitted: time.Now().Add(-5 * time.Second), trail: []string{"accepted"}}} - - first := captureStdout(t, func() { r.draw(rows, "watching") }) - require.NotEmpty(t, first) - assert.NotContains(t, first, "\033", "a redirected run must not emit escape codes") - - rows[0].submitted = time.Now().Add(-30 * time.Second) - assert.Empty(t, captureStdout(t, func() { r.draw(rows, "watching") }), - "only the clock moved, so there is nothing new to say") - - rows[0].trail = append(rows[0].trail, "started") - assert.NotEmpty(t, captureStdout(t, func() { r.draw(rows, "watching") }), - "the request moved, so the table should be reprinted") -} - -// TestFitGrowsColumnsOnly checks that a value wider than its header widens the -// column and that a later, narrower table does not pull it back in — a column -// that shrank would make the table jitter as rows fill in. -func TestFitGrowsColumnsOnly(t *testing.T) { - r := newRenderer() - wide := []*row{{sqid: "some-very-long-queue-name/1234"}} - r.fit(wide) - grown := r.wRequest - assert.Equal(t, len("some-very-long-queue-name/1234"), grown) - - r.fit([]*row{{}}) - assert.Equal(t, grown, r.wRequest) -} - -func TestNewRows(t *testing.T) { - assert.Len(t, newRows(config{count: 3}), 3, "independent changes are one request each") - assert.Len(t, newRows(config{count: 3, stacked: true}), 1, "a stack is a single request") -} - -func TestOutcome(t *testing.T) { - landed := &row{status: "landed"} - failed := &row{status: "error"} - - assert.Equal(t, "all 2 request(s) landed", outcome([]*row{landed, landed})) - assert.Equal(t, "1 of 2 request(s) did not land", outcome([]*row{landed, failed})) -} - -func TestSummarize(t *testing.T) { - assert.NoError(t, summarize([]*row{{sqid: "q/1", status: "landed"}})) - assert.Error(t, summarize([]*row{{sqid: "q/1", status: "landed"}, {sqid: "q/2", status: "error"}})) -} - -// TestRowLineAlignment is the column contract: on every row the stage begins at -// exactly the same screen column, whatever the cells before it contain. A -// hyperlinked row is the interesting one, since its changes cell is mostly -// escape bytes that occupy no width — measuring those as if they did would both -// shove the column sideways and eat the stage's room to render. -// fakeGateway answers history lookups from a table the test controls. Only the -// one method is reachable; the embedded interface satisfies the rest. -type fakeGateway struct { - pb.SubmitQueueGatewayClient - mu sync.Mutex - events map[string][]*pb.HistoryEvent -} - -func (f *fakeGateway) set(sqid string, statuses ...string) { - f.mu.Lock() - defer f.mu.Unlock() - if f.events == nil { - f.events = map[string][]*pb.HistoryEvent{} - } - events := make([]*pb.HistoryEvent, 0, len(statuses)) - for _, s := range statuses { - events = append(events, &pb.HistoryEvent{Status: s}) - } - f.events[sqid] = events -} - -func (f *fakeGateway) GetRequestHistoryByID( - _ context.Context, in *pb.GetRequestHistoryByIDRequest, _ ...grpc.CallOption, -) (*pb.GetRequestHistoryByIDResponse, error) { - f.mu.Lock() - defer f.mu.Unlock() - return &pb.GetRequestHistoryByIDResponse{Events: f.events[in.Sqid]}, nil -} - -func isClosed(ch <-chan struct{}) bool { - select { - case <-ch: - return true - default: - return false - } -} - -// TestTrackerSettlesOnlyWhenSealedAndTerminal covers the condition that ends a -// run. Polling starts while pull requests are still being created, so "nothing -// outstanding" is true before creation has begun — sealing is what separates -// that from actually being finished. -func TestTrackerSettlesOnlyWhenSealedAndTerminal(t *testing.T) { - tr := newTracker(newRows(config{count: 2})) - tr.r.inPlace = false - gw := &fakeGateway{} - ctx := context.Background() - - captureStdout(t, func() { - tr.update(func() { tr.rows[0].sqid = "demo-queue/1" }) - tr.seal() - }) - assert.False(t, isClosed(tr.settled), "a row that was never enqueued is not settled") - - captureStdout(t, func() { tr.update(func() { tr.rows[1].sqid = "demo-queue/2" }) }) - gw.set("demo-queue/1", "accepted", "started") - gw.set("demo-queue/2", "accepted") - captureStdout(t, func() { tr.refresh(ctx, gw, "demo-queue") }) - - assert.Equal(t, []string{"accepted", "started"}, tr.rows[0].trail) - assert.False(t, isClosed(tr.settled), "requests still in flight") - - gw.set("demo-queue/1", "accepted", "started", "landed") - gw.set("demo-queue/2", "accepted", "error") - captureStdout(t, func() { tr.refresh(ctx, gw, "demo-queue") }) - - assert.True(t, isClosed(tr.settled), "every request reached a terminal status") - assert.True(t, tr.rows[0].done) - assert.False(t, tr.rows[0].settled.IsZero(), "settling stops the clock") -} - -// TestTrackerSealBeforeEnqueueDoesNotSettle guards the ordering hazard the seal -// exists for: polling that ran before anything was enqueued must not conclude -// the run just because it found nothing outstanding. -func TestTrackerSealBeforeEnqueueDoesNotSettle(t *testing.T) { - tr := newTracker(newRows(config{count: 1})) - tr.r.inPlace = false - - captureStdout(t, func() { tr.refresh(context.Background(), &fakeGateway{}, "demo-queue") }) - assert.False(t, isClosed(tr.settled), "nothing has been enqueued yet") -} - -// TestTrackerPollsWhileCreating is the behavior the tracker exists for: a run -// that only polled after every pull request was created would show an empty -// trail for the whole creation phase. Here a row enqueued first picks up its -// trail while a later row has not been enqueued at all. -func TestTrackerPollsWhileCreating(t *testing.T) { - tr := newTracker(newRows(config{count: 3})) - tr.r.inPlace = false - gw := &fakeGateway{} - gw.set("demo-queue/1", "accepted", "started", "batched") - - captureStdout(t, func() { - tr.update(func() { tr.rows[0].sqid = "demo-queue/1" }) - tr.refresh(context.Background(), gw, "demo-queue") - }) - - assert.Equal(t, "accepted → started → batched", tr.rows[0].stage()) - assert.Equal(t, absent, tr.rows[2].stage(), "a row not yet enqueued has nothing to show") -} - -// TestTrackerConcurrentPollAndUpdate exercises the two writers against each -// other so the race detector has something to find. Creation fills in rows from -// one goroutine while polling reads and redraws from another. -func TestTrackerConcurrentPollAndUpdate(t *testing.T) { - tr := newTracker(newRows(config{count: 8})) - tr.r.inPlace = false - gw := &fakeGateway{} - ctx := context.Background() - - captureStdout(t, func() { - var wg sync.WaitGroup - wg.Add(2) - - go func() { - defer wg.Done() - for i := range tr.rows { - sqid := fmt.Sprintf("demo-queue/%d", i) - gw.set(sqid, "accepted", "landed") - i := i - tr.update(func() { - tr.rows[i].changes = append(tr.rows[i].changes, change{number: 100 + i, url: "https://example.test/pull/1"}) - tr.rows[i].sqid, tr.rows[i].submitted = sqid, time.Now() - }) - } - tr.seal() - }() - - go func() { - defer wg.Done() - for range 20 { - tr.refresh(ctx, gw, "demo-queue") - } - }() - - wg.Wait() - tr.refresh(ctx, gw, "demo-queue") - }) - - assert.True(t, isClosed(tr.settled)) - require.NoError(t, tr.conclude()) -} - -func TestWrap(t *testing.T) { - tests := []struct { - name string - in string - width int - want []string - }{ - {name: "nothing to wrap", in: "short", width: 20, want: []string{"short"}}, - { - name: "breaks on spaces", - in: "queue name must not be empty", - width: 12, - want: []string{"queue name", "must not be", "empty"}, - }, - { - name: "a token longer than the line is split", - in: "aaaaaaaaaa bb", - width: 4, - want: []string{"aaaa", "aaaa", "aa", "bb"}, - }, - {name: "newlines are just whitespace", in: "one\ntwo", width: 20, want: []string{"one two"}}, - {name: "no width", in: "anything", width: 0}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := wrap(tt.in, tt.width) - assert.Equal(t, tt.want, got) - for _, line := range got { - assert.LessOrEqual(t, len([]rune(line)), tt.width) - } - }) - } -} - -// TestNoteLinesRenderErrorInFull is the point of wrapping rather than -// truncating: the interesting part of a pipeline error is usually at the end, -// so an ellipsis in the stage column hides exactly what the reader needs. -func TestNoteLinesRenderErrorInFull(t *testing.T) { - r := newRenderer() - r.inPlace = true - failed := &row{ - sqid: "demo-queue/1", - changes: []change{{number: 75, url: "https://github.com/behinddwalls/sq-demo/pull/75"}}, - submitted: time.Now(), - trail: []string{"accepted", "started", "validated", "batched", "error"}, - note: `speculator failed for queue demo-queue: score dependency "demo-queue/batch/1": ` + - `failed to resolve storage for queue "": queue name must not be empty`, - } - r.fit([]*row{failed}) - - lines := r.noteLines(failed) - require.NotEmpty(t, lines) - - var text strings.Builder - for i, line := range lines { - assert.LessOrEqual(t, len([]rune(line)), maxLineWidth, "a wrapped note still has to fit the line") - trimmed := strings.TrimLeft(line, " ") - if i == 0 { - assert.True(t, strings.HasPrefix(trimmed, "↳ "), "the first line is marked") - } - text.WriteString(strings.TrimPrefix(strings.TrimPrefix(trimmed, "↳ "), " ")) - text.WriteString(" ") - } - - assert.Contains(t, text.String(), "queue name must not be empty", - "the tail of the error is what says what went wrong; it must survive") - - // The row itself keeps only the trail, so the columns stay aligned. - assert.NotContains(t, r.rowLine(failed), "speculator failed") - assert.NotContains(t, r.rowLine(failed), "…") -} - -// TestNoteLinesIndentToStageColumn keeps a wrapped error visually attached to -// its row rather than looking like a new column. -func TestNoteLinesIndentToStageColumn(t *testing.T) { - r := newRenderer() - r.inPlace = true - rows := []*row{{sqid: "demo-queue/1", submitted: time.Now(), trail: []string{"error"}, note: "boom"}} - r.fit(rows) - - lines := r.noteLines(rows[0]) - require.Len(t, lines, 1) - assert.Equal(t, strings.Repeat(" ", r.prefixWidth())+"↳ boom", lines[0]) -} - -func TestRowLineAlignment(t *testing.T) { - r := newRenderer() - r.inPlace = true - rows := []*row{ - { - sqid: "demo-queue/17", - changes: []change{{number: 41, url: "https://github.com/behinddwalls/sq-demo/pull/41"}}, - submitted: time.Now(), - trail: []string{"accepted", "started", "batched", "speculating", "landed"}, - }, - {sqid: "demo-queue/1234", submitted: time.Now(), trail: []string{"accepted"}}, - {}, - } - r.fit(rows) - - for _, rw := range rows { - shown := []rune(visible(r.rowLine(rw))) - require.GreaterOrEqual(t, len(shown), r.prefixWidth()) - assert.Equal(t, rw.stage(), string(shown[r.prefixWidth():]), - "the stage should start at column %d and be rendered whole", r.prefixWidth()) - } -} - -// visible strips OSC 8 hyperlink sequences, leaving what the terminal draws. -func visible(s string) string { - for { - start := strings.Index(s, "\033]8;;") - if start < 0 { - return s - } - end := strings.Index(s[start:], "\033\\") - if end < 0 { - return s - } - s = s[:start] + s[start+end+len("\033\\"):] - } -} - -// captureStdout collects what fn writes to stdout. The renderer writes there -// directly, which is the thing under test. The pipe is drained as fn runs, so a -// test that draws more than the pipe buffer holds does not deadlock. -func captureStdout(t *testing.T, fn func()) string { - t.Helper() - - rd, wr, err := os.Pipe() - require.NoError(t, err) - - collected := make(chan string, 1) - go func() { - out, readErr := io.ReadAll(rd) - if readErr != nil { - collected <- "" - return - } - collected <- string(out) - }() - - original := os.Stdout - os.Stdout = wr - defer func() { os.Stdout = original }() - - fn() - require.NoError(t, wr.Close()) - return <-collected -} - -func head(s string, n int) string { - if len(s) <= n { - return s - } - return s[:n] -} - func TestChangeFilePath_IsUniquePerFileAcrossChangesAndRuns(t *testing.T) { // Uniqueness is the property the whole layout rests on: two changes writing // the same path would collide on content, and the run would measure conflict @@ -737,3 +103,10 @@ func TestChangeFileCount_VariesButIsReproducible(t *testing.T) { } assert.Greater(t, len(counts), 1, "the count should vary across changes, not be constant") } + +func TestRowCount(t *testing.T) { + // A stack lands as one request however many pull requests it chains, so it + // gets one row; independent changes get one each. + assert.Equal(t, 3, rowCount(config{count: 3}), "independent changes are one request each") + assert.Equal(t, 1, rowCount(config{count: 3, stacked: true}), "a stack is a single request") +} diff --git a/service/submitqueue/gateway/client/BUILD.bazel b/service/submitqueue/gateway/client/BUILD.bazel index 3d935b7e..ea6834da 100644 --- a/service/submitqueue/gateway/client/BUILD.bazel +++ b/service/submitqueue/gateway/client/BUILD.bazel @@ -6,12 +6,8 @@ go_library( importpath = "github.com/uber/submitqueue/service/submitqueue/gateway/client", visibility = ["//visibility:private"], deps = [ - "//api/base/change/protopb:go_default_library", - "//api/base/mergestrategy/protopb:go_default_library", - "//api/submitqueue/gateway/protopb:go_default_library", "//platform/base/change/github:go_default_library", - "@org_golang_google_grpc//:go_default_library", - "@org_golang_google_grpc//credentials/insecure:go_default_library", + "//submitqueue/client:go_default_library", ], ) diff --git a/service/submitqueue/gateway/client/main.go b/service/submitqueue/gateway/client/main.go index d52444e5..9bad7093 100644 --- a/service/submitqueue/gateway/client/main.go +++ b/service/submitqueue/gateway/client/main.go @@ -13,12 +13,16 @@ // limitations under the License. // Command client is a small operator CLI for the SubmitQueue gateway: submit a -// land request, read a request's status, and ping the service. +// land request, read a request's status, and see what a queue is doing. // // It exists so that driving a real queue does not require hand-assembling // protobuf with grpcurl — in particular, `land -pr` turns a pull request URL // into the change URI the pipeline wants, so nobody has to paste a 40-character // commit SHA by hand. +// +// Everything it does beyond parsing flags lives in submitqueue/client, so this +// file stays a thin front end and the same behaviour is available to any other +// tool that wants it. package main import ( @@ -34,12 +38,7 @@ import ( "time" githubchange "github.com/uber/submitqueue/platform/base/change/github" - - changepb "github.com/uber/submitqueue/api/base/change/protopb" - mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb" - pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" - "google.golang.org/grpc" - "google.golang.org/grpc/credentials/insecure" + "github.com/uber/submitqueue/submitqueue/client" ) const usage = `Usage: client [global flags] [command flags] @@ -48,10 +47,14 @@ Commands: ping Check that the gateway is reachable land Submit a change, or an ordered stack of changes, to a queue status Read a request's current status + list Show a queue's recent requests as a table + watch Follow a queue's requests until they settle Global flags: - -addr gateway address (default "localhost:8081") - -timeout request timeout (default 10s) + -addr gateway address (default "localhost:8081") + -tls dial with transport security (default false) + -token-env environment variable holding the bearer token (default "SQ_TOKEN") + -timeout request timeout, 0 for none (default 10s; watch ignores it) Examples: client ping @@ -59,11 +62,18 @@ Examples: client land -queue my-queue -uri github://github.com/uber/r/pull/7/ -strategy SQUASH_REBASE client land -queue my-queue -pr -pr client status -queue my-queue -sqid my-queue/12 + client list -queue my-queue -since 1h + client watch -queue my-queue + client -addr sq.example.com:443 -tls list -queue my-queue ` func main() { - addr := flag.String("addr", "localhost:8081", "gateway server address") - timeout := flag.Duration("timeout", 10*time.Second, "request timeout") + var opts client.Options + flag.StringVar(&opts.Addr, "addr", "localhost:8081", "gateway server address") + flag.BoolVar(&opts.TLS, "tls", false, "dial the gateway with transport security") + flag.StringVar(&opts.TokenEnv, "token-env", client.DefaultTokenEnv, + "environment variable holding the bearer token; empty disables authentication") + timeout := flag.Duration("timeout", 10*time.Second, "request timeout; 0 for none") flag.Usage = func() { fmt.Fprint(os.Stderr, usage) } flag.Parse() @@ -73,46 +83,54 @@ func main() { os.Exit(2) } - if err := run(*addr, *timeout, args[0], args[1:]); err != nil { + if err := run(opts, *timeout, args[0], args[1:]); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) } } -func run(addr string, timeout time.Duration, command string, args []string) error { - conn, err := grpc.NewClient(addr, grpc.WithTransportCredentials(insecure.NewCredentials())) +func run(opts client.Options, timeout time.Duration, command string, args []string) error { + sq, err := client.New(opts) if err != nil { - return fmt.Errorf("failed to connect to %s: %w", addr, err) + return err } - defer conn.Close() + defer sq.Close() - client := pb.NewSubmitQueueGatewayClient(conn) - ctx, cancel := context.WithTimeout(context.Background(), timeout) + // A watch runs until its queue settles or the operator stops it, so it is + // the one command that must not inherit a per-call deadline. + if command == "watch" { + timeout = 0 + } + ctx, cancel := client.WithTimeout(context.Background(), timeout) defer cancel() switch command { case "ping": - return runPing(ctx, client, args) + return runPing(ctx, sq, args) case "land": - return runLand(ctx, client, args) + return runLand(ctx, sq, args) case "status": - return runStatus(ctx, client, args) + return runStatus(ctx, sq, args) + case "list": + return runList(ctx, sq, args) + case "watch": + return runWatch(ctx, sq, args) default: fmt.Fprint(os.Stderr, usage) return fmt.Errorf("unknown command %q", command) } } -func runPing(ctx context.Context, client pb.SubmitQueueGatewayClient, args []string) error { +func runPing(ctx context.Context, sq *client.Client, args []string) error { fs := flag.NewFlagSet("ping", flag.ExitOnError) message := fs.String("message", "", "message to echo back") if err := fs.Parse(args); err != nil { return err } - resp, err := client.Ping(ctx, &pb.PingRequest{Message: *message}) + resp, err := sq.Ping(ctx, *message) if err != nil { - return fmt.Errorf("ping failed: %w", err) + return err } fmt.Printf("Message: %s\n", resp.Message) @@ -122,7 +140,7 @@ func runPing(ctx context.Context, client pb.SubmitQueueGatewayClient, args []str return nil } -func runLand(ctx context.Context, client pb.SubmitQueueGatewayClient, args []string) error { +func runLand(ctx context.Context, sq *client.Client, args []string) error { fs := flag.NewFlagSet("land", flag.ExitOnError) queue := fs.String("queue", "", "queue to land on (required)") strategy := fs.String("strategy", "REBASE", "REBASE, SQUASH_REBASE, MERGE, PROMOTE, or DEFAULT") @@ -151,7 +169,7 @@ func runLand(ctx context.Context, client pb.SubmitQueueGatewayClient, args []str // Explicit URIs first, then resolved ones, each in the order given. all := append(append([]string{}, uris...), resolved...) - parsedStrategy, err := parseStrategy(*strategy) + parsedStrategy, err := client.ParseStrategy(*strategy) if err != nil { return err } @@ -160,22 +178,18 @@ func runLand(ctx context.Context, client pb.SubmitQueueGatewayClient, args []str fmt.Printf("Change %d: %s\n", i+1, uri) } - resp, err := client.Land(ctx, &pb.LandRequest{ - Queue: *queue, - Change: &changepb.Change{Uris: all}, - Strategy: parsedStrategy, - }) + sqid, err := sq.Land(ctx, *queue, all, parsedStrategy) if err != nil { - return fmt.Errorf("land failed: %w", err) + return err } fmt.Printf("\nLanded request submitted.\n") - fmt.Printf(" sqid: %s\n", resp.Sqid) - fmt.Printf("\nFollow it with: client status -queue %s -sqid %s\n", *queue, resp.Sqid) + fmt.Printf(" sqid: %s\n", sqid) + fmt.Printf("\nFollow it with: client status -queue %s -sqid %s\n", *queue, sqid) return nil } -func runStatus(ctx context.Context, client pb.SubmitQueueGatewayClient, args []string) error { +func runStatus(ctx context.Context, sq *client.Client, args []string) error { fs := flag.NewFlagSet("status", flag.ExitOnError) sqid := fs.String("sqid", "", "request id returned by land (required)") // A sqid is only resolvable within its own queue, so the server needs both. @@ -190,26 +204,128 @@ func runStatus(ctx context.Context, client pb.SubmitQueueGatewayClient, args []s return fmt.Errorf("-queue is required") } - resp, err := client.GetRequestSummaryByID(ctx, &pb.GetRequestSummaryByIDRequest{Sqid: *sqid, Queue: *queue}) + request, err := sq.Summary(ctx, *queue, *sqid) if err != nil { - return fmt.Errorf("status failed: %w", err) - } - if resp.Request == nil { - return fmt.Errorf("no request found for %q", *sqid) + return err } - fmt.Printf("sqid: %s\n", resp.Request.Sqid) - fmt.Printf("queue: %s\n", resp.Request.Queue) - fmt.Printf("status: %s\n", resp.Request.Status) - if resp.Request.LastError != "" { - fmt.Printf("error: %s\n", resp.Request.LastError) + fmt.Printf("sqid: %s\n", request.Sqid) + fmt.Printf("queue: %s\n", request.Queue) + fmt.Printf("status: %s\n", request.Status) + if request.LastError != "" { + fmt.Printf("error: %s\n", request.LastError) } - for i, uri := range resp.Request.ChangeUris { + for i, uri := range request.ChangeUris { fmt.Printf("change %d: %s\n", i+1, uri) } return nil } +// runList draws a queue's recent requests once and returns. +func runList(ctx context.Context, sq *client.Client, args []string) error { + fs := flag.NewFlagSet("list", flag.ExitOnError) + queue := fs.String("queue", "", "queue to list (required)") + since := fs.Duration("since", 0, "only requests received within this window; 0 for all retained history") + limit := fs.Int("limit", 50, "most requests to show; 0 for every one in the window") + if err := fs.Parse(args); err != nil { + return err + } + if *queue == "" { + return fmt.Errorf("-queue is required") + } + + summaries, err := sq.List(ctx, client.ListQuery{Queue: *queue, Since: *since, Limit: *limit}) + if err != nil { + return err + } + if len(summaries) == 0 { + fmt.Printf("no requests in %s%s\n", *queue, within(*since)) + return nil + } + + rows := client.RowsFromSummaries(summaries) + client.Draw(rows, fmt.Sprintf("%d request(s) in %s%s", len(rows), *queue, within(*since))) + return nil +} + +// runWatch follows a queue's requests until every one of them settles. +// +// The set is fixed when the watch starts: a request accepted afterwards is not +// picked up, because a watch that grew as the queue did would never finish, and +// finishing is what makes the command usable from a script. +func runWatch(ctx context.Context, sq *client.Client, args []string) error { + fs := flag.NewFlagSet("watch", flag.ExitOnError) + queue := fs.String("queue", "", "queue to watch (required)") + since := fs.Duration("since", 15*time.Minute, "how far back to pick requests up from") + limit := fs.Int("limit", 50, "most requests to watch; 0 for every one in the window") + var sqids repeatable + fs.Var(&sqids, "sqid", "watch only this request; repeat for several, and -since is then ignored") + if err := fs.Parse(args); err != nil { + return err + } + if *queue == "" { + return fmt.Errorf("-queue is required") + } + + rows, err := watchRows(ctx, sq, *queue, *since, *limit, sqids) + if err != nil { + return err + } + if len(rows) == 0 { + fmt.Printf("no requests in %s%s\n", *queue, within(*since)) + return nil + } + + t := client.NewTracker(rows) + // Nothing further will join the set, so the tracker can conclude as soon as + // what it holds has settled. + t.Seal() + t.Note("watching %d request(s) in %s", len(rows), *queue) + + go t.Poll(ctx, sq.Gateway(), *queue) + + select { + case <-ctx.Done(): + return ctx.Err() + case <-t.Settled(): + } + return t.Conclude() +} + +// watchRows is the set a watch will follow: the named requests, or whatever the +// queue holds in the window. +func watchRows( + ctx context.Context, + sq *client.Client, + queue string, + since time.Duration, + limit int, + sqids []string, +) ([]*client.Row, error) { + if len(sqids) > 0 { + rows := make([]*client.Row, 0, len(sqids)) + for _, sqid := range sqids { + rows = append(rows, &client.Row{SQID: sqid, Submitted: time.Now()}) + } + return rows, nil + } + + summaries, err := sq.List(ctx, client.ListQuery{Queue: queue, Since: since, Limit: limit}) + if err != nil { + return nil, err + } + return client.RowsFromSummaries(summaries), nil +} + +// within describes a time window for a message, or nothing at all when the +// window is the whole of retained history. +func within(since time.Duration) string { + if since <= 0 { + return "" + } + return fmt.Sprintf(" in the last %s", since) +} + // repeatable collects a flag given more than once, preserving the order it was // given in — which for a stack of changes is the order they must be applied. type repeatable []string @@ -217,24 +333,6 @@ type repeatable []string func (r *repeatable) String() string { return strings.Join(*r, ",") } func (r *repeatable) Set(v string) error { *r = append(*r, v); return nil } -// parseStrategy maps the -strategy value onto the wire enum. -func parseStrategy(name string) (mergestrategypb.Strategy, error) { - switch strings.ToUpper(strings.TrimSpace(name)) { - case "", "DEFAULT": - return mergestrategypb.Strategy_DEFAULT, nil - case "REBASE": - return mergestrategypb.Strategy_REBASE, nil - case "SQUASH_REBASE": - return mergestrategypb.Strategy_SQUASH_REBASE, nil - case "MERGE": - return mergestrategypb.Strategy_MERGE, nil - case "PROMOTE": - return mergestrategypb.Strategy_PROMOTE, nil - default: - return mergestrategypb.Strategy_DEFAULT, fmt.Errorf("unknown strategy %q", name) - } -} - // resolvePullRequests turns each pull request URL into the change URI the // pipeline expects, in the order given. func resolvePullRequests(ctx context.Context, urls []string) ([]string, error) { diff --git a/submitqueue/client/BUILD.bazel b/submitqueue/client/BUILD.bazel new file mode 100644 index 00000000..929e7ec2 --- /dev/null +++ b/submitqueue/client/BUILD.bazel @@ -0,0 +1,40 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = [ + "conn.go", + "land.go", + "query.go", + "view.go", + "watch.go", + ], + importpath = "github.com/uber/submitqueue/submitqueue/client", + visibility = ["//visibility:public"], + deps = [ + "//api/base/change/protopb:go_default_library", + "//api/base/mergestrategy/protopb:go_default_library", + "//api/submitqueue/gateway/protopb:go_default_library", + "//submitqueue/entity:go_default_library", + "@org_golang_google_grpc//:go_default_library", + "@org_golang_google_grpc//credentials:go_default_library", + "@org_golang_google_grpc//credentials/insecure:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = [ + "conn_test.go", + "query_test.go", + "view_test.go", + ], + embed = [":go_default_library"], + deps = [ + "//api/submitqueue/gateway/protopb:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + "@org_golang_google_grpc//:go_default_library", + "@org_golang_google_grpc//metadata:go_default_library", + ], +) diff --git a/submitqueue/client/README.md b/submitqueue/client/README.md new file mode 100644 index 00000000..facafe2d --- /dev/null +++ b/submitqueue/client/README.md @@ -0,0 +1,33 @@ +# client + +The SubmitQueue client: dialling a gateway, the calls made against it, and the terminal view of what a queue is doing. + +It exists so the tools are thin. A binary under `service/` is flag parsing over this package, which is what keeps the gateway CLI and the demo from each growing their own dialling, their own strategy parsing, and their own status table — as they had begun to. + +## Connecting + +`Options` is a plain struct, not a set of flags: binaries parse their own flags and fill it, so the package stays usable from a test or another program. + +`Addr` is handed to the dialler untouched, so the full gRPC target syntax works — a plain `host:port`, but also `dns:///host:port` or `unix:///path.sock`. Transport security is a separate option rather than something encoded in the address, because gRPC keeps target resolution and transport credentials apart; there is no scheme that means "use TLS", and inventing one would only mislead. + +`TokenEnv` names the variable holding a bearer token rather than carrying the token, so a credential never reaches a command line. An unset variable is not an error — it is how a client against a gateway that wants no credential runs, which today is every gateway in this repository. The token is for one reached through something that does check it: a proxy, a sidecar, an ingress terminating auth ahead of the service. + +## The view + +A `Row` is one land request and everything shown about it. A `Tracker` owns a set of rows, polls their histories, and redraws as they move; `Draw` renders once, for a listing that is not following anything. + +Two things move a run forward at once — whatever is producing requests, and the poll reading their statuses — and both draw the same table, so the tracker's mutex is what keeps one from redrawing halfway through the other's update. Reads happen outside that lock: holding it across a round of RPCs would stall the producer behind the network, and letting the producer run ahead is the whole point of watching each request the moment it exists. + +The renderer draws in place on a terminal and appends a fresh block when piped, so redirecting to a file gives something readable rather than escape codes. Piped output skips a redraw when nothing in the table moved, which is why the signature it compares leaves out the elapsed clock: that advances every second, and a log reprinting the table for it alone would say nothing while saying it often. + +Column widths only ever grow, so a value wider than its header does not make the table jitter as rows fill in. + +### The changes column + +Each row carries `[]Cell`, and a cell is text with an optional URL. The column is supplied by the caller rather than derived, because what identifies a change depends on who is watching: a tool that just opened the pull requests knows their numbers and can link to them, while a client watching a queue it did not create knows only the change URIs the gateway reports. `RowsFromSummaries` builds the second kind from a listing. + +On a terminal a cell with a URL is rendered as an OSC 8 hyperlink; piped, the address itself is printed, since a log has nothing to click and the address is the part worth copying. Padding counts on-screen width rather than string length — a hyperlink is mostly escape bytes that occupy no columns, and padding by length would shove the table sideways. + +## Settling + +`Tracker.Seal` declares that nothing further will join the set, which is what lets an otherwise-finished run conclude; without it a poll finding nothing outstanding before the first request existed would call the run finished. `Conclude` draws the verdict and returns an error naming every request that ended anywhere other than `landed`, so a scripted caller notices. diff --git a/submitqueue/client/conn.go b/submitqueue/client/conn.go new file mode 100644 index 00000000..ce523858 --- /dev/null +++ b/submitqueue/client/conn.go @@ -0,0 +1,166 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package client is the SubmitQueue client: dialling a gateway, the calls a +// caller makes against it, and the terminal view of what a queue is doing. +// +// It exists so the tools are thin. A binary here is flag parsing over this +// package, which is what keeps the gateway CLI and the demo from growing their +// own dialling, their own strategy parsing, and their own status table. +package client + +import ( + "context" + "crypto/tls" + "fmt" + "os" + "time" + + "google.golang.org/grpc" + "google.golang.org/grpc/credentials" + "google.golang.org/grpc/credentials/insecure" + + pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" +) + +// DefaultTokenEnv is the environment variable a client reads its bearer token +// from unless told otherwise. +const DefaultTokenEnv = "SQ_TOKEN" + +// Options is how a caller reaches a gateway. +// +// It is a plain struct rather than a set of flags so the package stays usable +// from a test or another program; binaries parse their own flags and fill it. +type Options struct { + // Addr is the gRPC target. It is passed to the dialler untouched, so the + // full target syntax works — a plain host:port, but also dns:///host:port + // or unix:///path/to.sock. + Addr string + + // TLS dials with transport security instead of plaintext. Off by default, + // which is what a local stack wants and nothing else should. + TLS bool + + // TokenEnv names the environment variable holding a bearer token. The + // variable is named rather than the token passed directly, so a credential + // never reaches a command line, where it would be visible in the shell + // history and to anyone running ps. Empty sends no credential. + TokenEnv string +} + +// Client is a connected gateway client. +type Client struct { + conn *grpc.ClientConn + gw pb.SubmitQueueGatewayClient +} + +// New dials the gateway described by opts. +// +// The caller closes the returned client. Dialling is lazy, as gRPC prefers, so +// an unreachable address surfaces on the first call rather than here. +func New(opts Options) (*Client, error) { + if opts.Addr == "" { + return nil, fmt.Errorf("address must not be empty") + } + + dialOpts := []grpc.DialOption{grpc.WithTransportCredentials(transportCredentials(opts.TLS))} + if creds, ok := bearerFrom(opts.TokenEnv); ok { + dialOpts = append(dialOpts, grpc.WithPerRPCCredentials(creds)) + } + + conn, err := grpc.NewClient(opts.Addr, dialOpts...) + if err != nil { + return nil, fmt.Errorf("failed to dial gateway at %s: %w", opts.Addr, err) + } + return &Client{conn: conn, gw: pb.NewSubmitQueueGatewayClient(conn)}, nil +} + +// Close releases the connection. +func (c *Client) Close() error { + return c.conn.Close() +} + +// Gateway is the generated client underneath, for a call this package does not +// wrap yet. Prefer the wrappers; this is the escape hatch, not the interface. +func (c *Client) Gateway() pb.SubmitQueueGatewayClient { + return c.gw +} + +// Ping checks the gateway answers, returning its reply. +func (c *Client) Ping(ctx context.Context, message string) (*pb.PingResponse, error) { + resp, err := c.gw.Ping(ctx, &pb.PingRequest{Message: message}) + if err != nil { + return nil, fmt.Errorf("ping failed: %w", err) + } + return resp, nil +} + +func transportCredentials(useTLS bool) credentials.TransportCredentials { + if useTLS { + return credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12}) + } + return insecure.NewCredentials() +} + +// bearer sends a token as an Authorization header on every call. +// +// Nothing in this repository checks it: the gateway admits every caller. It is +// here for a gateway reached through something that does — a proxy, a sidecar, +// an ingress terminating auth ahead of the service. +type bearer struct { + token string +} + +// GetRequestMetadata renders the credential as the header a server would read. +func (b bearer) GetRequestMetadata(context.Context, ...string) (map[string]string, error) { + return map[string]string{"authorization": "Bearer " + b.token}, nil +} + +// RequireTransportSecurity reports false so a token can be sent over a +// plaintext connection. +// +// gRPC otherwise refuses to attach per-RPC credentials without transport +// security, which is the right default and the wrong one for a local stack +// that has no certificates. The token is only as protected as the transport +// carrying it, so a deployment reachable by anyone else should also set TLS. +func (b bearer) RequireTransportSecurity() bool { + return false +} + +// bearerFrom reads the token out of the named variable, reporting whether there +// is one to send. An unset or empty variable is not an error: it is how a client +// against a gateway that wants no credential runs. +func bearerFrom(tokenEnv string) (bearer, bool) { + if tokenEnv == "" { + return bearer{}, false + } + token := os.Getenv(tokenEnv) + if token == "" { + return bearer{}, false + } + return bearer{token: token}, true +} + +// WithTimeout derives a context carrying timeout, or the parent unchanged when +// timeout is not positive. +// +// A non-positive timeout means "no deadline", which is what a watch needs: it +// runs until its queue settles or the operator stops it, and a deadline meant +// for a single call would cut it short. +func WithTimeout(ctx context.Context, timeout time.Duration) (context.Context, context.CancelFunc) { + if timeout <= 0 { + return context.WithCancel(ctx) + } + return context.WithTimeout(ctx, timeout) +} diff --git a/submitqueue/client/conn_test.go b/submitqueue/client/conn_test.go new file mode 100644 index 00000000..cde4aa99 --- /dev/null +++ b/submitqueue/client/conn_test.go @@ -0,0 +1,119 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package client + +import ( + "context" + "net" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/grpc" + "google.golang.org/grpc/metadata" + + pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" +) + +func TestNewRejectsAnEmptyAddress(t *testing.T) { + _, err := New(Options{}) + require.Error(t, err) +} + +// TestCredentialsReachTheServerOverPlaintext is the one that matters for a +// local stack: gRPC refuses to attach per-RPC credentials to an insecure +// connection unless they say they do not need transport security, so a token +// that works over TLS can silently never be sent without it. +func TestCredentialsReachTheServerOverPlaintext(t *testing.T) { + tests := []struct { + name string + tokenEnv string + token string + want string + }{ + { + name: "a token is sent as a bearer credential", + tokenEnv: "SQ_TEST_CONN_TOKEN", + token: "s3cret", + want: "Bearer s3cret", + }, + { + name: "an unset variable sends nothing", + tokenEnv: "SQ_TEST_CONN_TOKEN", + }, + { + name: "no variable named sends nothing", + token: "s3cret", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.tokenEnv != "" { + t.Setenv(tt.tokenEnv, tt.token) + } + + gw := &recordingGateway{} + addr, stop := serve(t, gw) + defer stop() + + sq, err := New(Options{Addr: addr, TokenEnv: tt.tokenEnv}) + require.NoError(t, err) + defer sq.Close() + + _, err = sq.Ping(context.Background(), "hello") + require.NoError(t, err) + + assert.Equal(t, tt.want, gw.authorization()) + }) + } +} + +// recordingGateway answers Ping and remembers what metadata the call carried. +type recordingGateway struct { + pb.UnimplementedSubmitQueueGatewayServer + md metadata.MD +} + +func (g *recordingGateway) Ping(ctx context.Context, req *pb.PingRequest) (*pb.PingResponse, error) { + g.md, _ = metadata.FromIncomingContext(ctx) + return &pb.PingResponse{Message: req.GetMessage()}, nil +} + +// authorization is the single Authorization header the last call carried, or +// empty when it carried none. +func (g *recordingGateway) authorization() string { + values := g.md.Get("authorization") + if len(values) == 0 { + return "" + } + return values[0] +} + +// serve starts a real gRPC server on a loopback port and returns its address. +// A real listener rather than an in-memory pipe, so the dialling path under +// test is the one a caller actually uses. +func serve(t *testing.T, gw pb.SubmitQueueGatewayServer) (string, func()) { + t.Helper() + + lis, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + + srv := grpc.NewServer() + pb.RegisterSubmitQueueGatewayServer(srv, gw) + go func() { _ = srv.Serve(lis) }() + + return lis.Addr().String(), srv.Stop +} diff --git a/submitqueue/client/land.go b/submitqueue/client/land.go new file mode 100644 index 00000000..d401ccce --- /dev/null +++ b/submitqueue/client/land.go @@ -0,0 +1,72 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package client + +import ( + "context" + "fmt" + "strings" + + changepb "github.com/uber/submitqueue/api/base/change/protopb" + mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb" + pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" +) + +// Land puts a change on a queue and returns the request id tracking it. +// +// The URIs are one change, in caller order: several of them are a stack landing +// as a single request, not several requests. +func (c *Client) Land( + ctx context.Context, + queue string, + uris []string, + strategy mergestrategypb.Strategy, +) (string, error) { + if queue == "" { + return "", fmt.Errorf("queue must not be empty") + } + if len(uris) == 0 { + return "", fmt.Errorf("at least one change URI is required") + } + + resp, err := c.gw.Land(ctx, &pb.LandRequest{ + Queue: queue, + Change: &changepb.Change{Uris: uris}, + Strategy: strategy, + }) + if err != nil { + return "", fmt.Errorf("land %s failed: %w", strings.Join(uris, ","), err) + } + return resp.GetSqid(), nil +} + +// ParseStrategy maps a strategy name to its wire value. An empty name selects +// the queue's configured default. +func ParseStrategy(name string) (mergestrategypb.Strategy, error) { + switch strings.ToUpper(strings.TrimSpace(name)) { + case "", "DEFAULT": + return mergestrategypb.Strategy_DEFAULT, nil + case "REBASE": + return mergestrategypb.Strategy_REBASE, nil + case "SQUASH_REBASE": + return mergestrategypb.Strategy_SQUASH_REBASE, nil + case "MERGE": + return mergestrategypb.Strategy_MERGE, nil + case "PROMOTE": + return mergestrategypb.Strategy_PROMOTE, nil + default: + return mergestrategypb.Strategy_DEFAULT, fmt.Errorf("unknown strategy %q", name) + } +} diff --git a/submitqueue/client/query.go b/submitqueue/client/query.go new file mode 100644 index 00000000..73706f70 --- /dev/null +++ b/submitqueue/client/query.go @@ -0,0 +1,140 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package client + +import ( + "context" + "fmt" + "time" + + pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" +) + +// ListQuery selects a page range of a queue's receipt history. +type ListQuery struct { + // Queue is the exact queue to read. Required: the gateway has no + // cross-queue listing, because a request id is only resolvable within its + // own queue. + Queue string + + // Since bounds the window to requests received within it. Zero reads from + // the beginning of retained history. + Since time.Duration + + // Limit caps how many requests are returned across all pages. Zero means + // every request in the window, which for a busy queue is a lot of paging. + Limit int + + // PageSize is what each page requests. Zero takes the server default. + PageSize int +} + +// List reads a queue's requests, newest first, following continuation tokens +// until the limit is reached or the queue is exhausted. +// +// Paging is followed here rather than exposed, because a caller asking what a +// queue is doing wants the answer, not a cursor. A caller that needs the cursor +// can reach the generated client through Gateway. +func (c *Client) List(ctx context.Context, q ListQuery) ([]*pb.RequestSummary, error) { + if q.Queue == "" { + return nil, fmt.Errorf("queue must not be empty") + } + + req := &pb.ListRequest{Queue: q.Queue, PageSize: int32(q.PageSize)} + if q.Since > 0 { + req.ReceivedAtOrAfterMs = time.Now().Add(-q.Since).UnixMilli() + } + + var out []*pb.RequestSummary + for { + resp, err := c.gw.List(ctx, req) + if err != nil { + return nil, fmt.Errorf("list %s failed: %w", q.Queue, err) + } + + out = append(out, resp.GetRequests()...) + if q.Limit > 0 && len(out) >= q.Limit { + return out[:q.Limit], nil + } + + // An empty token is the last page. An empty page with a token would + // otherwise spin, so a page that added nothing also ends the walk. + if resp.GetNextPageToken() == "" || len(resp.GetRequests()) == 0 { + return out, nil + } + req.PageToken = resp.GetNextPageToken() + } +} + +// Summary reads one request's current status. +func (c *Client) Summary(ctx context.Context, queue, sqid string) (*pb.RequestSummary, error) { + if queue == "" || sqid == "" { + return nil, fmt.Errorf("queue and sqid are both required") + } + resp, err := c.gw.GetRequestSummaryByID(ctx, &pb.GetRequestSummaryByIDRequest{Sqid: sqid, Queue: queue}) + if err != nil { + return nil, fmt.Errorf("status of %s failed: %w", sqid, err) + } + if resp.GetRequest() == nil { + return nil, fmt.Errorf("no request found for %q", sqid) + } + return resp.GetRequest(), nil +} + +// History reads the events recorded for one request, oldest first. +func (c *Client) History(ctx context.Context, queue, sqid string) ([]*pb.HistoryEvent, error) { + if queue == "" || sqid == "" { + return nil, fmt.Errorf("queue and sqid are both required") + } + resp, err := c.gw.GetRequestHistoryByID(ctx, &pb.GetRequestHistoryByIDRequest{Sqid: sqid, Queue: queue}) + if err != nil { + return nil, fmt.Errorf("history of %s failed: %w", sqid, err) + } + return resp.GetEvents(), nil +} + +// RowsFromSummaries builds watchable rows from a listing, newest first. +// +// The changes column carries each request's change URIs, which is all a client +// watching a queue it did not create knows about them. A caller that knows +// more — a tool that just opened the pull requests, say — sets richer cells of +// its own. +// +// A summary records when a request was received but not when it settled, so a +// row built here has no settle time and its elapsed column reads as age since +// receipt. For a request still in flight that is the same number; for one that +// finished long ago it is how long ago, which is the useful thing to show in a +// listing anyway. +func RowsFromSummaries(summaries []*pb.RequestSummary) []*Row { + rows := make([]*Row, 0, len(summaries)) + for _, s := range summaries { + if s == nil { + continue + } + cells := make([]Cell, 0, len(s.GetChangeUris())) + for _, uri := range s.GetChangeUris() { + cells = append(cells, Cell{Text: uri}) + } + rows = append(rows, &Row{ + SQID: s.GetSqid(), + Cells: cells, + Status: s.GetStatus(), + Note: s.GetLastError(), + Submitted: time.UnixMilli(s.GetReceivedAtMs()), + Done: terminalStatuses[s.GetStatus()], + }) + } + return rows +} diff --git a/submitqueue/client/query_test.go b/submitqueue/client/query_test.go new file mode 100644 index 00000000..e062eea3 --- /dev/null +++ b/submitqueue/client/query_test.go @@ -0,0 +1,189 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package client + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" +) + +func TestListRequiresAQueue(t *testing.T) { + sq, stop := dial(t, &pagingGateway{}) + defer stop() + + _, err := sq.List(context.Background(), ListQuery{}) + require.Error(t, err) +} + +func TestListFollowsPages(t *testing.T) { + gw := &pagingGateway{pages: [][]string{ + {"q/1", "q/2"}, + {"q/3", "q/4"}, + {"q/5"}, + }} + sq, stop := dial(t, gw) + defer stop() + + got, err := sq.List(context.Background(), ListQuery{Queue: "q"}) + require.NoError(t, err) + + assert.Equal(t, []string{"q/1", "q/2", "q/3", "q/4", "q/5"}, sqidsOf(got)) + assert.Equal(t, 3, gw.calls, "every page is fetched") +} + +func TestListStopsAtTheLimit(t *testing.T) { + // The limit is across pages, not per page, so it has to cut a page short + // and stop asking for more rather than over-fetching the whole queue. + gw := &pagingGateway{pages: [][]string{ + {"q/1", "q/2"}, + {"q/3", "q/4"}, + {"q/5"}, + }} + sq, stop := dial(t, gw) + defer stop() + + got, err := sq.List(context.Background(), ListQuery{Queue: "q", Limit: 3}) + require.NoError(t, err) + + assert.Equal(t, []string{"q/1", "q/2", "q/3"}, sqidsOf(got)) + assert.Equal(t, 2, gw.calls, "the third page is never asked for") +} + +func TestListStopsOnAnEmptyPage(t *testing.T) { + // A server that keeps handing back a token with nothing in the page would + // otherwise spin forever. + gw := &pagingGateway{pages: [][]string{{"q/1"}, {}}, alwaysToken: true} + sq, stop := dial(t, gw) + defer stop() + + got, err := sq.List(context.Background(), ListQuery{Queue: "q"}) + require.NoError(t, err) + + assert.Equal(t, []string{"q/1"}, sqidsOf(got)) + assert.Equal(t, 2, gw.calls) +} + +func TestListSinceBecomesAReceiptBound(t *testing.T) { + gw := &pagingGateway{pages: [][]string{{"q/1"}}} + sq, stop := dial(t, gw) + defer stop() + + before := time.Now().Add(-time.Hour).UnixMilli() + _, err := sq.List(context.Background(), ListQuery{Queue: "q", Since: time.Hour}) + require.NoError(t, err) + after := time.Now().Add(-time.Hour).UnixMilli() + + assert.GreaterOrEqual(t, gw.lastRequest.GetReceivedAtOrAfterMs(), before) + assert.LessOrEqual(t, gw.lastRequest.GetReceivedAtOrAfterMs(), after) +} + +func TestListWithoutSinceLeavesTheWindowOpen(t *testing.T) { + gw := &pagingGateway{pages: [][]string{{"q/1"}}} + sq, stop := dial(t, gw) + defer stop() + + _, err := sq.List(context.Background(), ListQuery{Queue: "q"}) + require.NoError(t, err) + + assert.Zero(t, gw.lastRequest.GetReceivedAtOrAfterMs(), + "no window means all retained history, not a bound of zero-time") +} + +func TestRowsFromSummaries(t *testing.T) { + received := time.Now().Add(-5 * time.Minute) + rows := RowsFromSummaries([]*pb.RequestSummary{ + { + Sqid: "q/1", + Status: "landed", + ChangeUris: []string{"github://h/o/r/pull/1/abc"}, + ReceivedAtMs: received.UnixMilli(), + }, + nil, // a hole in the page is skipped rather than becoming a blank row + {Sqid: "q/2", Status: "batched", LastError: "still going"}, + }) + + require.Len(t, rows, 2) + + assert.Equal(t, "q/1", rows[0].SQID) + assert.Equal(t, []Cell{{Text: "github://h/o/r/pull/1/abc"}}, rows[0].Cells) + assert.True(t, rows[0].Done, "a terminal status arrives already settled") + assert.Equal(t, received.UnixMilli(), rows[0].Submitted.UnixMilli()) + + assert.Equal(t, "q/2", rows[1].SQID) + assert.False(t, rows[1].Done, "an active request is still outstanding") + assert.Equal(t, "still going", rows[1].Note) +} + +// pagingGateway answers List from a fixed set of pages, handing out a +// continuation token until they run out. +type pagingGateway struct { + pb.UnimplementedSubmitQueueGatewayServer + pages [][]string + // alwaysToken keeps returning a token even past the last page, standing for + // a server that never signals the end. + alwaysToken bool + + calls int + lastRequest *pb.ListRequest +} + +func (g *pagingGateway) List(_ context.Context, req *pb.ListRequest) (*pb.ListResponse, error) { + g.lastRequest = req + page := g.calls + g.calls++ + + if page >= len(g.pages) { + return &pb.ListResponse{}, nil + } + + requests := make([]*pb.RequestSummary, 0, len(g.pages[page])) + for _, sqid := range g.pages[page] { + requests = append(requests, &pb.RequestSummary{Sqid: sqid, Queue: req.GetQueue()}) + } + + token := "" + if g.alwaysToken || page+1 < len(g.pages) { + token = fmt.Sprintf("page-%d", page+1) + } + return &pb.ListResponse{Requests: requests, NextPageToken: token}, nil +} + +func dial(t *testing.T, gw pb.SubmitQueueGatewayServer) (*Client, func()) { + t.Helper() + + addr, stop := serve(t, gw) + sq, err := New(Options{Addr: addr}) + require.NoError(t, err) + + return sq, func() { + _ = sq.Close() + stop() + } +} + +func sqidsOf(summaries []*pb.RequestSummary) []string { + out := make([]string, 0, len(summaries)) + for _, s := range summaries { + out = append(out, s.GetSqid()) + } + return out +} diff --git a/submitqueue/client/view.go b/submitqueue/client/view.go new file mode 100644 index 00000000..e28c4a27 --- /dev/null +++ b/submitqueue/client/view.go @@ -0,0 +1,463 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package client + +import ( + "fmt" + "os" + "sort" + "strings" + "time" + "unicode/utf8" + + pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" + "github.com/uber/submitqueue/submitqueue/entity" +) + +const ( + // pollInterval bounds how often the watcher re-reads every request's history. + pollInterval = 2 * time.Second + + // maxLineWidth caps a redrawn line. A line that wraps occupies two physical + // rows, which permanently desyncs the cursor arithmetic the in-place redraw + // depends on; capping is cheaper than asking the terminal how wide it is. + maxLineWidth = 120 + + // absent is what a cell shows before there is anything to put in it. + absent = "—" + + // minNoteWidth keeps a wrapped error readable even when the columns before + // it have eaten most of the line. + minNoteWidth = 40 +) + +// terminalStatuses are the states a land request settles on. They are keyed off +// the gateway's own vocabulary so this view cannot quietly drift from it. +var terminalStatuses = map[string]bool{ + string(entity.RequestStatusLanded): true, + string(entity.RequestStatusError): true, + string(entity.RequestStatusCancelled): true, +} + +// Cell is one entry in a row's changes column: what to show, and optionally +// where it points. +// +// The column is caller-supplied rather than derived, because what identifies a +// change depends on who is watching. A tool that just created pull requests +// knows their numbers and can link to them; a client watching a queue it did +// not create knows only the change URIs the gateway reports. Both render +// through the same cell. +type Cell struct { + // Text is what the reader sees on a terminal. + Text string + // URL is where the text points. Empty when the caller has no address for + // it, in which case Text is shown as-is in both modes. + URL string +} + +// Row is one land request and everything shown about it. A row exists from the +// first draw, before the request it will carry has been accepted, so the table +// never changes shape while a run is in progress. +type Row struct { + // Cells are what the changes column shows for this request, in caller order. + Cells []Cell + + // SQID is empty until the gateway accepts the request. + SQID string + // Submitted is when the gateway accepted it, and starts the elapsed clock. + Submitted time.Time + // Settled is when a terminal status was first observed, and stops it. + Settled time.Time + + // Trail is the ordered set of statuses the gateway recorded for the request. + Trail []string + Status string + Note string + Done bool +} + +// NewRows allocates n empty rows, so the table has its final shape before +// anything has been accepted. +func NewRows(n int) []*Row { + rows := make([]*Row, n) + for i := range rows { + rows[i] = &Row{} + } + return rows +} + +// elapsed is how long the request has been with the queue: absent until it is +// accepted, running while it is in flight, and frozen once it settles. +func (rw *Row) elapsed() string { + if rw.Submitted.IsZero() { + return absent + } + end := time.Now() + if !rw.Settled.IsZero() { + end = rw.Settled + } + return fmt.Sprintf("%ds", int(end.Sub(rw.Submitted).Seconds())) +} + +// stage is the path the request has taken, as the gateway recorded it. The +// waiting marker covers the gap between acceptance and the first recorded +// event, so an accepted request is never shown as though nothing happened. +func (rw *Row) stage() string { + if len(rw.Trail) > 0 { + return strings.Join(rw.Trail, " → ") + } + if rw.SQID != "" { + return "…" + } + return absent +} + +// Draw renders the table once and returns. It is what a one-shot listing wants; +// a caller following requests as they move uses a Tracker instead, which owns +// the rows and redraws them. +func Draw(rows []*Row, status string) { + newRenderer().draw(rows, status) +} + +// digest reduces a request's recorded history to the trail worth showing, the +// status it currently holds, and the error the latest event carried. A status +// recorded more than once in a row is one step in the trail, not several. +func digest(events []*pb.HistoryEvent) (trail []string, status, note string) { + if len(events) == 0 { + return nil, "", "" + } + for _, e := range events { + if e == nil || e.Status == "" { + continue + } + if len(trail) > 0 && trail[len(trail)-1] == e.Status { + continue + } + trail = append(trail, e.Status) + } + if last := events[len(events)-1]; last != nil { + status, note = last.Status, last.LastError + } + if status == "" && len(trail) > 0 { + status = trail[len(trail)-1] + } + return trail, status, note +} + +// outcome is the one-line verdict shown under the finished table. +func outcome(rows []*Row) string { + landed := 0 + for _, rw := range rows { + if rw.Status == string(entity.RequestStatusLanded) { + landed++ + } + } + if landed == len(rows) { + return fmt.Sprintf("all %d request(s) landed", len(rows)) + } + return fmt.Sprintf("%d of %d request(s) did not land", len(rows)-landed, len(rows)) +} + +// summarize fails the run if anything did not land, so a scripted caller +// notices. +func summarize(rows []*Row) error { + var failed []string + for _, rw := range rows { + if rw.Status != string(entity.RequestStatusLanded) { + failed = append(failed, fmt.Sprintf("%s=%s", rw.SQID, rw.Status)) + } + } + if len(failed) > 0 { + sort.Strings(failed) + return fmt.Errorf("%d of %d request(s) did not land: %s", len(failed), len(rows), strings.Join(failed, ", ")) + } + return nil +} + +// renderer draws the status table, redrawing in place on a terminal and +// appending a fresh block otherwise, so piping the output to a file stays +// readable instead of filling with escape codes. +// +// Column widths only ever grow, so a value that turns out to be wider than the +// header does not make the table jitter as rows fill in. +type renderer struct { + inPlace bool + + wRequest int + wChanges int + wElapsed int + wStage int + + // lastLines is how many lines the previous draw actually emitted, which is + // how far the cursor has to move back to overwrite them. + lastLines int + drawn bool + + // lastBody is the signature of the previous table, so piped output can skip + // a redundant reprint when a step moved but the table did not. + lastBody string +} + +func newRenderer() *renderer { + info, err := os.Stdout.Stat() + tty := err == nil && info.Mode()&os.ModeCharDevice != 0 + return &renderer{ + inPlace: tty, + wRequest: len("REQUEST"), + wChanges: len("CHANGES"), + wElapsed: len("ELAPSED"), + wStage: len("STAGE"), + } +} + +func (r *renderer) draw(rows []*Row, status string) { + body := r.body(rows) + + if !r.inPlace { + sig := signature(rows) + if sig == r.lastBody { + // Nothing in the table moved; the step that prompted this draw is a + // terminal affordance and has no place in a log. + return + } + r.lastBody = sig + fmt.Println(strings.Join(body, "\n")) + fmt.Println() + return + } + + if r.drawn { + fmt.Printf("\033[%dA", r.lastLines) + } + for _, line := range body { + fmt.Printf("\033[K%s\n", line) + } + fmt.Printf("\033[K\n") + fmt.Printf("\033[K ▸ %s\n", truncate(status, maxLineWidth-4)) + // Every draw emits the body, one blank line, and the status line; moving + // back by exactly this many lines is what keeps the redraw from drifting. + r.lastLines = len(body) + 2 + r.drawn = true +} + +// body renders the header and one line per row. +func (r *renderer) body(rows []*Row) []string { + r.fit(rows) + + lines := make([]string, 0, len(rows)+2) + lines = append(lines, + fmt.Sprintf(" %-*s %-*s %*s %s", + r.wRequest, "REQUEST", r.wChanges, "CHANGES", r.wElapsed, "ELAPSED", "STAGE"), + fmt.Sprintf(" %s %s %s %s", + rule(r.wRequest), rule(r.wChanges), rule(r.wElapsed), rule(r.wStage))) + + for _, rw := range rows { + lines = append(lines, r.rowLine(rw)) + lines = append(lines, r.noteLines(rw)...) + } + return lines +} + +// fit grows the columns to hold what the rows now contain. Widths never shrink, +// so the table does not shift under a value that has already been printed — but +// on a terminal the stage column stays inside the line, since its rule would +// otherwise wrap on a long trail and take the redraw with it. +func (r *renderer) fit(rows []*Row) { + for _, rw := range rows { + r.wRequest = max(r.wRequest, utf8.RuneCountInString(rw.SQID)) + _, visible := r.changesCell(rw) + r.wChanges = max(r.wChanges, visible) + r.wStage = max(r.wStage, utf8.RuneCountInString(rw.stage())) + } + if r.inPlace { + r.wStage = min(r.wStage, max(len("STAGE"), maxLineWidth-r.prefixWidth())) + } +} + +// prefixWidth is the space every row spends before the stage column. +func (r *renderer) prefixWidth() int { + return 2 + r.wRequest + 2 + r.wChanges + 2 + r.wElapsed + 2 +} + +func (r *renderer) rowLine(rw *Row) string { + sqid := rw.SQID + if sqid == "" { + sqid = absent + } + changes, visible := r.changesCell(rw) + + prefix := fmt.Sprintf(" %-*s %s %*s ", + r.wRequest, sqid, pad(changes, visible, r.wChanges), r.wElapsed, rw.elapsed()) + + tail := rw.stage() + if r.inPlace { + // Only the tail can overflow, and unlike the changes cell it never holds + // escape sequences, so it is the one part safe to cut. The budget comes + // from the column widths rather than the rendered prefix, which counts a + // hyperlink's escape bytes that take up no space on screen. + tail = truncate(tail, maxLineWidth-r.prefixWidth()) + } + return prefix + tail +} + +// noteLines renders a request's error under its row, wrapped and indented to +// the stage column. An error is the one thing in the table worth reading in +// full — truncating it to the width of a cell hides the part that says what +// went wrong — so it gets as many lines as it needs instead of an ellipsis. +func (r *renderer) noteLines(rw *Row) []string { + if rw.Note == "" { + return nil + } + + indent := r.prefixWidth() + // A piped run spends most of the line on URLs, so the wrap width is floored + // rather than allowed to collapse to nothing. + width := max(minNoteWidth, maxLineWidth-indent-2) + + wrapped := wrap(rw.Note, width) + lines := make([]string, 0, len(wrapped)) + for i, text := range wrapped { + marker := " " + if i == 0 { + marker = "↳ " + } + lines = append(lines, strings.Repeat(" ", indent)+marker+text) + } + return lines +} + +// wrap breaks text into lines no wider than width, splitting on spaces and +// hard-splitting any single token too long to fit on a line of its own. +func wrap(s string, width int) []string { + if width < 1 { + return nil + } + + var lines []string + current := "" + flush := func() { + if current != "" { + lines = append(lines, current) + current = "" + } + } + + for _, word := range strings.Fields(s) { + for utf8.RuneCountInString(word) > width { + flush() + runes := []rune(word) + lines = append(lines, string(runes[:width])) + word = string(runes[width:]) + } + switch { + case current == "": + current = word + case utf8.RuneCountInString(current)+1+utf8.RuneCountInString(word) <= width: + current += " " + word + default: + flush() + current = word + } + } + flush() + return lines +} + +// changesCell renders a row's cells and reports the width they occupy on +// screen. The two differ on a terminal, where a hyperlink is mostly escape +// bytes that take up no space. +func (r *renderer) changesCell(rw *Row) (string, int) { + if len(rw.Cells) == 0 { + return absent, utf8.RuneCountInString(absent) + } + + parts := make([]string, 0, len(rw.Cells)) + visible := 0 + for _, c := range rw.Cells { + if c.URL == "" { + // Nothing to point at, so the text is all there is in either mode. + parts = append(parts, c.Text) + visible += utf8.RuneCountInString(c.Text) + continue + } + if r.inPlace { + parts = append(parts, hyperlink(c.Text, c.URL)) + visible += utf8.RuneCountInString(c.Text) + continue + } + // Piped output has nothing to click, so the address itself has to be + // readable — and copyable out of a log. + parts = append(parts, c.URL) + visible += utf8.RuneCountInString(c.URL) + } + separator := "," + if !r.inPlace { + separator = " " + } + return strings.Join(parts, separator), visible + len(separator)*(len(parts)-1) +} + +// hyperlink wraps text in an OSC 8 escape so terminals that understand it make +// the text clickable, and the rest simply show the text. +func hyperlink(text, url string) string { + if url == "" { + return text + } + return "\033]8;;" + url + "\033\\" + text + "\033]8;;\033\\" +} + +// pad right-pads a cell to a column width using its on-screen width, which is +// not its length whenever it carries escape sequences. +func pad(s string, visible, width int) string { + if visible >= width { + return s + } + return s + strings.Repeat(" ", width-visible) +} + +func rule(n int) string { + return strings.Repeat("─", n) +} + +// signature is what a piped run treats as the table having moved. The elapsed +// clock is left out on purpose: it advances every second, and a log that +// reprinted the table for that alone would say nothing while saying it often. +func signature(rows []*Row) string { + var b strings.Builder + for _, rw := range rows { + fmt.Fprintf(&b, "%s|%s|%s|%s\n", rw.SQID, labelsOf(rw.Cells), rw.stage(), rw.Note) + } + return b.String() +} + +// labelsOf is the cells' text, for identifying a row without its addresses. +func labelsOf(cells []Cell) string { + parts := make([]string, 0, len(cells)) + for _, c := range cells { + parts = append(parts, c.Text) + } + return strings.Join(parts, ",") +} + +func truncate(s string, n int) string { + s = strings.ReplaceAll(s, "\n", " ") + if n < 1 { + return "" + } + if utf8.RuneCountInString(s) <= n { + return s + } + return string([]rune(s)[:n-1]) + "…" +} diff --git a/submitqueue/client/view_test.go b/submitqueue/client/view_test.go new file mode 100644 index 00000000..b483f49b --- /dev/null +++ b/submitqueue/client/view_test.go @@ -0,0 +1,658 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package client + +import ( + "context" + "fmt" + "io" + "os" + "strings" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" + "google.golang.org/grpc" +) + +func TestDigest(t *testing.T) { + tests := []struct { + name string + events []*pb.HistoryEvent + wantTrail []string + wantStatus string + wantNote string + }{ + { + name: "no events yet", + }, + { + name: "one event", + events: []*pb.HistoryEvent{{Status: "accepted"}}, + wantTrail: []string{"accepted"}, + wantStatus: "accepted", + }, + { + name: "trail keeps the order it was recorded in", + events: []*pb.HistoryEvent{ + {Status: "accepted"}, {Status: "started"}, {Status: "batched"}, {Status: "landed"}, + }, + wantTrail: []string{"accepted", "started", "batched", "landed"}, + wantStatus: "landed", + }, + { + name: "a status recorded twice in a row is one step", + events: []*pb.HistoryEvent{ + {Status: "accepted"}, {Status: "started"}, {Status: "started"}, {Status: "batched"}, + }, + wantTrail: []string{"accepted", "started", "batched"}, + wantStatus: "batched", + }, + { + name: "a status revisited later is a step again", + events: []*pb.HistoryEvent{ + {Status: "speculating"}, {Status: "batched"}, {Status: "speculating"}, + }, + wantTrail: []string{"speculating", "batched", "speculating"}, + wantStatus: "speculating", + }, + { + name: "the error on the latest event is the one shown", + events: []*pb.HistoryEvent{ + {Status: "started", LastError: "transient"}, {Status: "error", LastError: "merge conflict"}, + }, + wantTrail: []string{"started", "error"}, + wantStatus: "error", + wantNote: "merge conflict", + }, + { + name: "events without a status do not become steps", + events: []*pb.HistoryEvent{{Status: ""}, {Status: "accepted"}, {Status: ""}}, + wantTrail: []string{"accepted"}, + wantStatus: "accepted", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + trail, status, note := digest(tt.events) + assert.Equal(t, tt.wantTrail, trail) + assert.Equal(t, tt.wantStatus, status) + assert.Equal(t, tt.wantNote, note) + }) + } +} + +func TestRowElapsed(t *testing.T) { + now := time.Now() + + tests := []struct { + name string + row Row + want string + }{ + { + name: "absent before the gateway accepts it", + row: Row{}, + want: absent, + }, + { + name: "running while in flight", + row: Row{Submitted: now.Add(-5 * time.Second)}, + want: "5s", + }, + { + name: "frozen once settled, however long ago that was", + row: Row{Submitted: now.Add(-90 * time.Second), Settled: now.Add(-60 * time.Second)}, + want: "30s", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, tt.row.elapsed()) + }) + } +} + +// TestRowElapsedStopsAtSettle pins the behavior the clock exists for: a settled +// row reads the same however much later it is drawn, while an unsettled one +// does not. +func TestRowElapsedStopsAtSettle(t *testing.T) { + start := time.Now().Add(-time.Minute) + settled := Row{Submitted: start, Settled: start.Add(10 * time.Second)} + inFlight := Row{Submitted: start} + + first := settled.elapsed() + time.Sleep(time.Millisecond) + assert.Equal(t, first, settled.elapsed()) + assert.NotEqual(t, first, inFlight.elapsed()) +} + +func TestRowStage(t *testing.T) { + tests := []struct { + name string + row Row + want string + }{ + { + name: "nothing to report before the request exists", + row: Row{}, + want: absent, + }, + { + name: "accepted but nothing recorded yet", + row: Row{SQID: "demo-queue/17"}, + want: "…", + }, + { + name: "the states it passed through", + row: Row{SQID: "demo-queue/17", Trail: []string{"accepted", "started", "landed"}}, + want: "accepted → started → landed", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, tt.row.stage()) + }) + } +} + +func TestChangesCell(t *testing.T) { + one := []Cell{{Text: "#41", URL: "https://github.com/o/r/pull/41"}} + two := []Cell{ + {Text: "#41", URL: "https://github.com/o/r/pull/41"}, + {Text: "#421", URL: "https://github.com/o/r/pull/421"}, + } + + t.Run("a terminal gets short clickable labels", func(t *testing.T) { + r := &renderer{inPlace: true} + text, visible := r.changesCell(&Row{Cells: two}) + + assert.Contains(t, text, "https://github.com/o/r/pull/41") + assert.Contains(t, text, "\033]8;;") + // "#41,#421" occupies eight columns however many escape bytes carry it. + assert.Equal(t, len("#41,#421"), visible) + assert.Greater(t, len(text), visible, "the escapes should not be counted as width") + }) + + t.Run("a pipe gets the addresses themselves", func(t *testing.T) { + r := &renderer{inPlace: false} + text, visible := r.changesCell(&Row{Cells: one}) + + assert.Equal(t, "https://github.com/o/r/pull/41", text) + assert.Equal(t, len(text), visible) + assert.NotContains(t, text, "\033") + }) + + t.Run("no pull requests yet", func(t *testing.T) { + r := &renderer{inPlace: true} + text, visible := r.changesCell(&Row{}) + + assert.Equal(t, absent, text) + assert.Equal(t, 1, visible) + }) +} + +// TestPadCountsVisibleWidth guards the alignment trap: a hyperlinked cell is +// mostly escape bytes, so padding by length would push the columns apart. +func TestPadCountsVisibleWidth(t *testing.T) { + linked := hyperlink("#41", "https://github.com/o/r/pull/41") + + assert.Equal(t, linked+" ", pad(linked, len("#41"), 10)) + assert.Equal(t, "#41 ", pad("#41", 3, 10)) + assert.Equal(t, "#41", pad("#41", 3, 3), "a cell at the column width is not padded") + assert.Equal(t, "#41", pad("#41", 3, 2), "a cell wider than the column is left alone") +} + +func TestTruncate(t *testing.T) { + tests := []struct { + name string + in string + n int + want string + }{ + {name: "short enough to keep", in: "accepted", n: 20, want: "accepted"}, + {name: "exactly the limit", in: "accepted", n: 8, want: "accepted"}, + {name: "cut with a marker", in: "accepted → started", n: 10, want: "accepted …"}, + {name: "newlines flattened", in: "line\nbreak", n: 20, want: "line break"}, + {name: "no room at all", in: "accepted", n: 0, want: ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, truncate(tt.in, tt.n)) + }) + } +} + +// TestTruncateSplitsOnRunes checks that a cut lands between characters. The +// trail is joined with a multi-byte arrow, so cutting by bytes would leave +// mojibake in the middle of the table. +func TestTruncateSplitsOnRunes(t *testing.T) { + got := truncate("accepted → started → batched", 12) + assert.True(t, utf8ValidAndCounted(got, 12), "got %q", got) +} + +func utf8ValidAndCounted(s string, n int) bool { + return len([]rune(s)) <= n && strings.ToValidUTF8(s, "?") == s +} + +// TestDrawLineAccounting is the invariant the in-place redraw rests on: the +// cursor moves back exactly as far as the previous draw reached. Off by one and +// every subsequent draw leaves a stale row on screen. +func TestDrawLineAccounting(t *testing.T) { + r := newRenderer() + r.inPlace = true + rows := []*Row{ + {SQID: "demo-queue/17", Cells: []Cell{{Text: "#41", URL: "https://github.com/o/r/pull/41"}}, + Submitted: time.Now(), Trail: []string{"accepted", "started"}}, + {SQID: "demo-queue/18", Cells: []Cell{{Text: "#42", URL: "https://github.com/o/r/pull/42"}}, + Submitted: time.Now(), Trail: []string{"accepted"}}, + {}, + } + + first := captureStdout(t, func() { r.draw(rows, "watching") }) + emitted := strings.Count(first, "\n") + assert.Equal(t, emitted, r.lastLines, "the first draw must record how far it reached") + assert.True(t, strings.HasPrefix(first, "\033[K"), "the first draw has nothing to move back over") + + second := captureStdout(t, func() { r.draw(rows, "still watching") }) + assert.True(t, strings.HasPrefix(second, fmt.Sprintf("\033[%dA", emitted)), + "the redraw must move back over exactly the %d lines it wrote, got %q", emitted, head(second, 12)) + assert.Equal(t, strings.Count(second, "\n"), r.lastLines) +} + +// TestDrawStaysWithinLineWidth checks the other half of the redraw contract: a +// line that wraps occupies two physical rows and desyncs the cursor for good. +func TestDrawStaysWithinLineWidth(t *testing.T) { + r := newRenderer() + r.inPlace = true + rows := []*Row{{ + SQID: "demo-queue/17", + Submitted: time.Now(), + Trail: strings.Split(strings.Repeat("speculating ", 30), " "), + Note: strings.Repeat("a very long error message ", 10), + }} + + out := captureStdout(t, func() { r.draw(rows, strings.Repeat("status ", 40)) }) + for _, line := range strings.Split(out, "\n") { + line = strings.ReplaceAll(line, "\033[K", "") + assert.LessOrEqual(t, len([]rune(line)), maxLineWidth, "line too wide: %q", line) + } +} + +// TestDrawPipedSkipsClockOnlyRedraws keeps a redirected run's log readable: the +// table is reprinted when it moves, not once a second because the clock did. +func TestDrawPipedSkipsClockOnlyRedraws(t *testing.T) { + r := newRenderer() + r.inPlace = false + rows := []*Row{{SQID: "demo-queue/17", Submitted: time.Now().Add(-5 * time.Second), Trail: []string{"accepted"}}} + + first := captureStdout(t, func() { r.draw(rows, "watching") }) + require.NotEmpty(t, first) + assert.NotContains(t, first, "\033", "a redirected run must not emit escape codes") + + rows[0].Submitted = time.Now().Add(-30 * time.Second) + assert.Empty(t, captureStdout(t, func() { r.draw(rows, "watching") }), + "only the clock moved, so there is nothing new to say") + + rows[0].Trail = append(rows[0].Trail, "started") + assert.NotEmpty(t, captureStdout(t, func() { r.draw(rows, "watching") }), + "the request moved, so the table should be reprinted") +} + +// TestFitGrowsColumnsOnly checks that a value wider than its header widens the +// column and that a later, narrower table does not pull it back in — a column +// that shrank would make the table jitter as rows fill in. +func TestFitGrowsColumnsOnly(t *testing.T) { + r := newRenderer() + wide := []*Row{{SQID: "some-very-long-queue-name/1234"}} + r.fit(wide) + grown := r.wRequest + assert.Equal(t, len("some-very-long-queue-name/1234"), grown) + + r.fit([]*Row{{}}) + assert.Equal(t, grown, r.wRequest) +} + +func TestNewRows(t *testing.T) { + assert.Len(t, NewRows(3), 3, "independent changes are one request each") + assert.Len(t, NewRows(1), 1, "a stack is a single request") +} + +func TestOutcome(t *testing.T) { + landed := &Row{Status: "landed"} + failed := &Row{Status: "error"} + + assert.Equal(t, "all 2 request(s) landed", outcome([]*Row{landed, landed})) + assert.Equal(t, "1 of 2 request(s) did not land", outcome([]*Row{landed, failed})) +} + +func TestSummarize(t *testing.T) { + assert.NoError(t, summarize([]*Row{{SQID: "q/1", Status: "landed"}})) + assert.Error(t, summarize([]*Row{{SQID: "q/1", Status: "landed"}, {SQID: "q/2", Status: "error"}})) +} + +// TestRowLineAlignment is the column contract: on every row the stage begins at +// exactly the same screen column, whatever the cells before it contain. A +// hyperlinked row is the interesting one, since its changes cell is mostly +// escape bytes that occupy no width — measuring those as if they did would both +// shove the column sideways and eat the stage's room to render. +// fakeGateway answers history lookups from a table the test controls. Only the +// one method is reachable; the embedded interface satisfies the rest. +type fakeGateway struct { + pb.SubmitQueueGatewayClient + mu sync.Mutex + events map[string][]*pb.HistoryEvent +} + +func (f *fakeGateway) set(sqid string, statuses ...string) { + f.mu.Lock() + defer f.mu.Unlock() + if f.events == nil { + f.events = map[string][]*pb.HistoryEvent{} + } + events := make([]*pb.HistoryEvent, 0, len(statuses)) + for _, s := range statuses { + events = append(events, &pb.HistoryEvent{Status: s}) + } + f.events[sqid] = events +} + +func (f *fakeGateway) GetRequestHistoryByID( + _ context.Context, in *pb.GetRequestHistoryByIDRequest, _ ...grpc.CallOption, +) (*pb.GetRequestHistoryByIDResponse, error) { + f.mu.Lock() + defer f.mu.Unlock() + return &pb.GetRequestHistoryByIDResponse{Events: f.events[in.Sqid]}, nil +} + +func isClosed(ch <-chan struct{}) bool { + select { + case <-ch: + return true + default: + return false + } +} + +// TestTrackerSettlesOnlyWhenSealedAndTerminal covers the condition that ends a +// run. Polling starts while pull requests are still being created, so "nothing +// outstanding" is true before creation has begun — sealing is what separates +// that from actually being finished. +func TestTrackerSettlesOnlyWhenSealedAndTerminal(t *testing.T) { + tr := NewTracker(NewRows(2)) + tr.r.inPlace = false + gw := &fakeGateway{} + ctx := context.Background() + + captureStdout(t, func() { + tr.Update(func() { tr.rows[0].SQID = "demo-queue/1" }) + tr.Seal() + }) + assert.False(t, isClosed(tr.settled), "a row that was never enqueued is not settled") + + captureStdout(t, func() { tr.Update(func() { tr.rows[1].SQID = "demo-queue/2" }) }) + gw.set("demo-queue/1", "accepted", "started") + gw.set("demo-queue/2", "accepted") + captureStdout(t, func() { tr.refresh(ctx, gw, "demo-queue") }) + + assert.Equal(t, []string{"accepted", "started"}, tr.rows[0].Trail) + assert.False(t, isClosed(tr.settled), "requests still in flight") + + gw.set("demo-queue/1", "accepted", "started", "landed") + gw.set("demo-queue/2", "accepted", "error") + captureStdout(t, func() { tr.refresh(ctx, gw, "demo-queue") }) + + assert.True(t, isClosed(tr.settled), "every request reached a terminal status") + assert.True(t, tr.rows[0].Done) + assert.False(t, tr.rows[0].Settled.IsZero(), "settling stops the clock") +} + +// TestTrackerSealBeforeEnqueueDoesNotSettle guards the ordering hazard the seal +// exists for: polling that ran before anything was enqueued must not conclude +// the run just because it found nothing outstanding. +func TestTrackerSealBeforeEnqueueDoesNotSettle(t *testing.T) { + tr := NewTracker(NewRows(1)) + tr.r.inPlace = false + + captureStdout(t, func() { tr.refresh(context.Background(), &fakeGateway{}, "demo-queue") }) + assert.False(t, isClosed(tr.settled), "nothing has been enqueued yet") +} + +// TestTrackerPollsWhileCreating is the behavior the tracker exists for: a run +// that only polled after every pull request was created would show an empty +// trail for the whole creation phase. Here a row enqueued first picks up its +// trail while a later row has not been enqueued at all. +func TestTrackerPollsWhileCreating(t *testing.T) { + tr := NewTracker(NewRows(3)) + tr.r.inPlace = false + gw := &fakeGateway{} + gw.set("demo-queue/1", "accepted", "started", "batched") + + captureStdout(t, func() { + tr.Update(func() { tr.rows[0].SQID = "demo-queue/1" }) + tr.refresh(context.Background(), gw, "demo-queue") + }) + + assert.Equal(t, "accepted → started → batched", tr.rows[0].stage()) + assert.Equal(t, absent, tr.rows[2].stage(), "a row not yet enqueued has nothing to show") +} + +// TestTrackerConcurrentPollAndUpdate exercises the two writers against each +// other so the race detector has something to find. Creation fills in rows from +// one goroutine while polling reads and redraws from another. +func TestTrackerConcurrentPollAndUpdate(t *testing.T) { + tr := NewTracker(NewRows(8)) + tr.r.inPlace = false + gw := &fakeGateway{} + ctx := context.Background() + + captureStdout(t, func() { + var wg sync.WaitGroup + wg.Add(2) + + go func() { + defer wg.Done() + for i := range tr.rows { + sqid := fmt.Sprintf("demo-queue/%d", i) + gw.set(sqid, "accepted", "landed") + i := i + tr.Update(func() { + tr.rows[i].Cells = append(tr.rows[i].Cells, Cell{Text: fmt.Sprintf("#%d", 100+i), URL: "https://example.test/pull/1"}) + tr.rows[i].SQID, tr.rows[i].Submitted = sqid, time.Now() + }) + } + tr.Seal() + }() + + go func() { + defer wg.Done() + for range 20 { + tr.refresh(ctx, gw, "demo-queue") + } + }() + + wg.Wait() + tr.refresh(ctx, gw, "demo-queue") + }) + + assert.True(t, isClosed(tr.settled)) + require.NoError(t, tr.Conclude()) +} + +func TestWrap(t *testing.T) { + tests := []struct { + name string + in string + width int + want []string + }{ + {name: "nothing to wrap", in: "short", width: 20, want: []string{"short"}}, + { + name: "breaks on spaces", + in: "queue name must not be empty", + width: 12, + want: []string{"queue name", "must not be", "empty"}, + }, + { + name: "a token longer than the line is split", + in: "aaaaaaaaaa bb", + width: 4, + want: []string{"aaaa", "aaaa", "aa", "bb"}, + }, + {name: "newlines are just whitespace", in: "one\ntwo", width: 20, want: []string{"one two"}}, + {name: "no width", in: "anything", width: 0}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := wrap(tt.in, tt.width) + assert.Equal(t, tt.want, got) + for _, line := range got { + assert.LessOrEqual(t, len([]rune(line)), tt.width) + } + }) + } +} + +// TestNoteLinesRenderErrorInFull is the point of wrapping rather than +// truncating: the interesting part of a pipeline error is usually at the end, +// so an ellipsis in the stage column hides exactly what the reader needs. +func TestNoteLinesRenderErrorInFull(t *testing.T) { + r := newRenderer() + r.inPlace = true + failed := &Row{ + SQID: "demo-queue/1", + Cells: []Cell{{Text: "#75", URL: "https://github.com/behinddwalls/sq-demo/pull/75"}}, + Submitted: time.Now(), + Trail: []string{"accepted", "started", "validated", "batched", "error"}, + Note: `speculator failed for queue demo-queue: score dependency "demo-queue/batch/1": ` + + `failed to resolve storage for queue "": queue name must not be empty`, + } + r.fit([]*Row{failed}) + + lines := r.noteLines(failed) + require.NotEmpty(t, lines) + + var text strings.Builder + for i, line := range lines { + assert.LessOrEqual(t, len([]rune(line)), maxLineWidth, "a wrapped note still has to fit the line") + trimmed := strings.TrimLeft(line, " ") + if i == 0 { + assert.True(t, strings.HasPrefix(trimmed, "↳ "), "the first line is marked") + } + text.WriteString(strings.TrimPrefix(strings.TrimPrefix(trimmed, "↳ "), " ")) + text.WriteString(" ") + } + + assert.Contains(t, text.String(), "queue name must not be empty", + "the tail of the error is what says what went wrong; it must survive") + + // The row itself keeps only the trail, so the columns stay aligned. + assert.NotContains(t, r.rowLine(failed), "speculator failed") + assert.NotContains(t, r.rowLine(failed), "…") +} + +// TestNoteLinesIndentToStageColumn keeps a wrapped error visually attached to +// its row rather than looking like a new column. +func TestNoteLinesIndentToStageColumn(t *testing.T) { + r := newRenderer() + r.inPlace = true + rows := []*Row{{SQID: "demo-queue/1", Submitted: time.Now(), Trail: []string{"error"}, Note: "boom"}} + r.fit(rows) + + lines := r.noteLines(rows[0]) + require.Len(t, lines, 1) + assert.Equal(t, strings.Repeat(" ", r.prefixWidth())+"↳ boom", lines[0]) +} + +func TestRowLineAlignment(t *testing.T) { + r := newRenderer() + r.inPlace = true + rows := []*Row{ + { + SQID: "demo-queue/17", + Cells: []Cell{{Text: "#41", URL: "https://github.com/behinddwalls/sq-demo/pull/41"}}, + Submitted: time.Now(), + Trail: []string{"accepted", "started", "batched", "speculating", "landed"}, + }, + {SQID: "demo-queue/1234", Submitted: time.Now(), Trail: []string{"accepted"}}, + {}, + } + r.fit(rows) + + for _, rw := range rows { + shown := []rune(visible(r.rowLine(rw))) + require.GreaterOrEqual(t, len(shown), r.prefixWidth()) + assert.Equal(t, rw.stage(), string(shown[r.prefixWidth():]), + "the stage should start at column %d and be rendered whole", r.prefixWidth()) + } +} + +// visible strips OSC 8 hyperlink sequences, leaving what the terminal draws. +func visible(s string) string { + for { + start := strings.Index(s, "\033]8;;") + if start < 0 { + return s + } + end := strings.Index(s[start:], "\033\\") + if end < 0 { + return s + } + s = s[:start] + s[start+end+len("\033\\"):] + } +} + +// captureStdout collects what fn writes to stdout. The renderer writes there +// directly, which is the thing under test. The pipe is drained as fn runs, so a +// test that draws more than the pipe buffer holds does not deadlock. +func captureStdout(t *testing.T, fn func()) string { + t.Helper() + + rd, wr, err := os.Pipe() + require.NoError(t, err) + + collected := make(chan string, 1) + go func() { + out, readErr := io.ReadAll(rd) + if readErr != nil { + collected <- "" + return + } + collected <- string(out) + }() + + original := os.Stdout + os.Stdout = wr + defer func() { os.Stdout = original }() + + fn() + require.NoError(t, wr.Close()) + return <-collected +} + +func head(s string, n int) string { + if len(s) <= n { + return s + } + return s[:n] +} diff --git a/submitqueue/client/watch.go b/submitqueue/client/watch.go new file mode 100644 index 00000000..289ddc63 --- /dev/null +++ b/submitqueue/client/watch.go @@ -0,0 +1,198 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package client + +import ( + "context" + "fmt" + "sync" + "time" + + "google.golang.org/grpc" + + pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" +) + +// HistorySource is the part of the gateway a watch reads from. +// +// It is narrowed to the one call rather than taking the generated client whole, +// so a caller watching requests can be handed something that only knows how to +// answer for them. The generated client satisfies it as it stands. +type HistorySource interface { + GetRequestHistoryByID( + ctx context.Context, + in *pb.GetRequestHistoryByIDRequest, + opts ...grpc.CallOption, + ) (*pb.GetRequestHistoryByIDResponse, error) +} + +// Tracker owns the rows and the table drawn from them. +// +// Two things move a run forward at once — whatever is producing requests, and +// the poll that reads their statuses — and both draw the same table, so the +// mutex is what keeps one from redrawing halfway through the other's update. +type Tracker struct { + mu sync.Mutex + rows []*Row + r *renderer + status string + // sealed records that every request that will be watched is known. Without + // it, polling would find nothing outstanding before the first request + // existed and call the run finished. + sealed bool + + // settled closes once every request has reached a terminal status. + settled chan struct{} + once sync.Once +} + +// NewTracker returns a Tracker over the given rows. +func NewTracker(rows []*Row) *Tracker { + return &Tracker{rows: rows, r: newRenderer(), settled: make(chan struct{})} +} + +// Rows are the rows the tracker draws. Mutate them only from inside Update, +// which holds the lock the poll also takes. +func (t *Tracker) Rows() []*Row { + return t.rows +} + +// Settled closes once every request has reached a terminal status. +func (t *Tracker) Settled() <-chan struct{} { + return t.settled +} + +// Note replaces the line under the table and redraws. +func (t *Tracker) Note(format string, args ...any) { + t.mu.Lock() + defer t.mu.Unlock() + t.status = fmt.Sprintf(format, args...) + t.r.draw(t.rows, t.status) +} + +// Update applies a change to the rows and redraws with it. +func (t *Tracker) Update(fn func()) { + t.mu.Lock() + defer t.mu.Unlock() + fn() + t.r.draw(t.rows, t.status) +} + +// Conclude draws the verdict and reports whether everything landed. It reads +// the rows under the lock because a poll may still be applying its last round. +func (t *Tracker) Conclude() error { + t.mu.Lock() + defer t.mu.Unlock() + t.status = outcome(t.rows) + t.r.draw(t.rows, t.status) + return summarize(t.rows) +} + +// Seal declares that nothing further will be watched, which is what lets an +// otherwise-finished run conclude. +func (t *Tracker) Seal() { + t.mu.Lock() + defer t.mu.Unlock() + t.sealed = true + t.signalLocked() +} + +// signalLocked closes settled once there is nothing left to wait for. +func (t *Tracker) signalLocked() { + if !t.sealed { + return + } + for _, rw := range t.rows { + if rw.SQID == "" || !rw.Done { + return + } + } + t.once.Do(func() { close(t.settled) }) +} + +// Poll re-reads statuses until the run finishes or the context ends. +func (t *Tracker) Poll(ctx context.Context, src HistorySource, queue string) { + ticker := time.NewTicker(pollInterval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-t.settled: + return + case <-ticker.C: + } + t.refresh(ctx, src, queue) + } +} + +// refresh re-reads every request that has been accepted but has not settled. +// +// The reads happen outside the lock. Holding it across a round of RPCs would +// stall whatever is producing requests behind the network, and letting that run +// ahead is the whole point of watching each request the moment it exists. +func (t *Tracker) refresh(ctx context.Context, src HistorySource, queue string) { + t.mu.Lock() + outstanding := make([]*Row, 0, len(t.rows)) + for _, rw := range t.rows { + if rw.SQID != "" && !rw.Done { + outstanding = append(outstanding, rw) + } + } + total := len(t.rows) + t.mu.Unlock() + + type reading struct { + rw *Row + trail []string + status string + note string + } + readings := make([]reading, 0, len(outstanding)) + for _, rw := range outstanding { + // SQID is written once, before the row becomes outstanding, so reading + // it here without the lock is safe. + resp, err := src.GetRequestHistoryByID(ctx, &pb.GetRequestHistoryByIDRequest{Sqid: rw.SQID, Queue: queue}) + if err != nil || resp == nil || len(resp.Events) == 0 { + // A history that is not readable yet is normal right after Land; + // the next tick picks it up. + continue + } + trail, status, note := digest(resp.Events) + readings = append(readings, reading{rw: rw, trail: trail, status: status, note: note}) + } + + t.mu.Lock() + defer t.mu.Unlock() + + settled := 0 + for _, got := range readings { + got.rw.Trail, got.rw.Status, got.rw.Note = got.trail, got.status, got.note + if terminalStatuses[got.status] && !got.rw.Done { + // Stamped from the local clock rather than the event timestamp so + // the elapsed column is measured end to end against one clock. + got.rw.Done, got.rw.Settled = true, time.Now() + } + } + for _, rw := range t.rows { + if rw.Done { + settled++ + } + } + + t.status = fmt.Sprintf("%d of %d settled", settled, total) + t.r.draw(t.rows, t.status) + t.signalLocked() +} From 278b93f4fb7caf9bab35b811526a425259618134 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Tue, 11 Aug 2026 12:29:38 -0700 Subject: [PATCH 2/5] feat(client): wrap the stage at the terminal's width, never truncate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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. --- MODULE.bazel | 1 + go.mod | 1 + go.sum | 2 + submitqueue/client/BUILD.bazel | 1 + submitqueue/client/view.go | 135 +++++++++++++++++++++---- submitqueue/client/view_test.go | 173 ++++++++++++++++++++++++++++++-- 6 files changed, 287 insertions(+), 26 deletions(-) diff --git a/MODULE.bazel b/MODULE.bazel index 1bd3809c..ed503983 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -66,6 +66,7 @@ use_repo( "org_golang_google_protobuf", "org_golang_x_oauth2", "org_golang_x_sync", + "org_golang_x_term", "org_uber_go_fx", "org_uber_go_mock", "org_uber_go_yarpc", diff --git a/go.mod b/go.mod index 2b361b94..ca0a0657 100644 --- a/go.mod +++ b/go.mod @@ -14,6 +14,7 @@ require ( go.uber.org/zap v1.27.1 golang.org/x/oauth2 v0.34.0 golang.org/x/sync v0.19.0 + golang.org/x/term v0.39.0 google.golang.org/grpc v1.68.1 google.golang.org/protobuf v1.36.10 gopkg.in/yaml.v3 v3.0.1 diff --git a/go.sum b/go.sum index 199e1c87..5ac50f6e 100644 --- a/go.sum +++ b/go.sum @@ -249,6 +249,8 @@ golang.org/x/sys v0.0.0-20211019181941-9d821ace8654/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.40.0 h1:DBZZqJ2Rkml6QMQsZywtnjnnGvHza6BTfYFWY9kjEWQ= golang.org/x/sys v0.40.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= +golang.org/x/term v0.39.0 h1:RclSuaJf32jOqZz74CkPA9qFuVTX7vhLlpfj/IGWlqY= +golang.org/x/term v0.39.0/go.mod h1:yxzUCTP/U+FzoxfdKmLaA0RV1WgE0VY7hXBwKtY/4ww= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk= golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= diff --git a/submitqueue/client/BUILD.bazel b/submitqueue/client/BUILD.bazel index 929e7ec2..5e8e07ad 100644 --- a/submitqueue/client/BUILD.bazel +++ b/submitqueue/client/BUILD.bazel @@ -19,6 +19,7 @@ go_library( "@org_golang_google_grpc//:go_default_library", "@org_golang_google_grpc//credentials:go_default_library", "@org_golang_google_grpc//credentials/insecure:go_default_library", + "@org_golang_x_term//:go_default_library", ], ) diff --git a/submitqueue/client/view.go b/submitqueue/client/view.go index e28c4a27..554461fc 100644 --- a/submitqueue/client/view.go +++ b/submitqueue/client/view.go @@ -24,16 +24,16 @@ import ( pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" "github.com/uber/submitqueue/submitqueue/entity" + "golang.org/x/term" ) const ( // pollInterval bounds how often the watcher re-reads every request's history. pollInterval = 2 * time.Second - // maxLineWidth caps a redrawn line. A line that wraps occupies two physical - // rows, which permanently desyncs the cursor arithmetic the in-place redraw - // depends on; capping is cheaper than asking the terminal how wide it is. - maxLineWidth = 120 + // defaultLineWidth is the width assumed when the terminal will not say how + // wide it is — piped output, or a terminal that answers no size at all. + defaultLineWidth = 120 // absent is what a cell shows before there is anything to put in it. absent = "—" @@ -41,6 +41,11 @@ const ( // minNoteWidth keeps a wrapped error readable even when the columns before // it have eaten most of the line. minNoteWidth = 40 + + // minStageWidth is the narrowest the stage column is allowed to wrap to. A + // window narrow enough to force this is already unreadable; the floor keeps + // the wrap from degenerating into one word per line. + minStageWidth = 24 ) // terminalStatuses are the states a land request settles on. They are keyed off @@ -192,9 +197,24 @@ func summarize(rows []*Row) error { // // Column widths only ever grow, so a value that turns out to be wider than the // header does not make the table jitter as rows fill in. +// +// No line the renderer emits may exceed the terminal's width. A line that wraps +// occupies two physical rows, and the in-place redraw moves the cursor back by +// the number of lines it *emitted* — so one wrapped line desyncs every redraw +// after it. Everything wide is therefore wrapped deliberately, into lines the +// renderer counts itself. type renderer struct { inPlace bool + // width is the terminal's width, re-read before every draw. A watch runs for + // minutes and a window can be resized inside them, so this is not a property + // the process can sample once — see resize. + width int + + // size reports the terminal's width and whether it could be read. Held as a + // field so a test can drive a resize without a terminal. + size func() (int, bool) + wRequest int wChanges int wElapsed int @@ -211,10 +231,16 @@ type renderer struct { } func newRenderer() *renderer { - info, err := os.Stdout.Stat() - tty := err == nil && info.Mode()&os.ModeCharDevice != 0 + width, sized := terminalSize() return &renderer{ - inPlace: tty, + // Redrawing in place requires knowing the width to wrap to. A terminal + // that will not report its size is therefore treated as a log: emitting + // a guessed width into a narrower window is what produces a physically + // wrapped line, and the redraw counts the lines it emitted rather than + // the lines that appeared, so one of those desyncs every frame after it. + inPlace: sized, + width: width, + size: terminalSize, wRequest: len("REQUEST"), wChanges: len("CHANGES"), wElapsed: len("ELAPSED"), @@ -222,7 +248,54 @@ func newRenderer() *renderer { } } +// terminalSize is how wide the output is, in columns, and whether that came +// from the terminal rather than from the fallback. +// +// Asking the terminal is what lets a wide window show a long trail in full, +// rather than everyone being held to the narrowest window anyone might have. +// Anything that is not a sized terminal — a pipe, a file, a CI log — falls back +// to a fixed width, since there is no width to discover and a log wants a +// stable one anyway. +func terminalSize() (int, bool) { + w, _, err := term.GetSize(int(os.Stdout.Fd())) + if err != nil || w <= 0 { + return defaultLineWidth, false + } + return w, true +} + +// lineWidth is the width to render to. It tolerates a renderer built without +// one — a zero-value renderer in a test — rather than collapsing to nothing. +func (r *renderer) lineWidth() int { + if r.width <= 0 { + return defaultLineWidth + } + return r.width +} + +// resize re-reads the terminal's width before a draw. +// +// The width is not a property of the process: a watch runs for minutes and the +// window can be dragged narrower at any point in them. Sampling once at startup +// means every frame after a resize is wrapped to a width the window no longer +// has, and the terminal wraps those lines itself — mid-word, ignoring the +// column alignment, and without telling the redraw, which then moves the cursor +// back by fewer lines than actually appeared. +// +// Only the width is re-read. Whether output is a terminal at all cannot change +// under a running process, and re-deciding it per frame would let a transient +// probe failure switch rendering modes mid-run. +func (r *renderer) resize() { + if !r.inPlace || r.size == nil { + return + } + if w, sized := r.size(); sized { + r.width = w + } +} + func (r *renderer) draw(rows []*Row, status string) { + r.resize() body := r.body(rows) if !r.inPlace { @@ -245,7 +318,7 @@ func (r *renderer) draw(rows []*Row, status string) { fmt.Printf("\033[K%s\n", line) } fmt.Printf("\033[K\n") - fmt.Printf("\033[K ▸ %s\n", truncate(status, maxLineWidth-4)) + fmt.Printf("\033[K ▸ %s\n", truncate(status, r.lineWidth()-4)) // Every draw emits the body, one blank line, and the status line; moving // back by exactly this many lines is what keeps the redraw from drifting. r.lastLines = len(body) + 2 @@ -264,7 +337,7 @@ func (r *renderer) body(rows []*Row) []string { rule(r.wRequest), rule(r.wChanges), rule(r.wElapsed), rule(r.wStage))) for _, rw := range rows { - lines = append(lines, r.rowLine(rw)) + lines = append(lines, r.rowLines(rw)...) lines = append(lines, r.noteLines(rw)...) } return lines @@ -282,7 +355,7 @@ func (r *renderer) fit(rows []*Row) { r.wStage = max(r.wStage, utf8.RuneCountInString(rw.stage())) } if r.inPlace { - r.wStage = min(r.wStage, max(len("STAGE"), maxLineWidth-r.prefixWidth())) + r.wStage = min(r.wStage, r.stageWidth()) } } @@ -291,7 +364,14 @@ func (r *renderer) prefixWidth() int { return 2 + r.wRequest + 2 + r.wChanges + 2 + r.wElapsed + 2 } -func (r *renderer) rowLine(rw *Row) string { +// rowLines renders one row: its columns, and the stage wrapped onto indented +// continuation lines when the trail does not fit the width. +// +// Wrapping rather than cutting is what keeps a long trail readable — the end of +// it is where the request actually is, so a cut there hides the interesting +// part. Continuations align under the stage column so the wrapped text reads as +// one field rather than as new rows. +func (r *renderer) rowLines(rw *Row) []string { sqid := rw.SQID if sqid == "" { sqid = absent @@ -302,14 +382,31 @@ func (r *renderer) rowLine(rw *Row) string { r.wRequest, sqid, pad(changes, visible, r.wChanges), r.wElapsed, rw.elapsed()) tail := rw.stage() - if r.inPlace { - // Only the tail can overflow, and unlike the changes cell it never holds - // escape sequences, so it is the one part safe to cut. The budget comes - // from the column widths rather than the rendered prefix, which counts a - // hyperlink's escape bytes that take up no space on screen. - tail = truncate(tail, maxLineWidth-r.prefixWidth()) + if !r.inPlace { + // A log has no width to respect and is easier to read and grep on one + // line, so it takes the trail whole. + return []string{prefix + tail} + } + + indent := r.prefixWidth() + segments := wrap(tail, r.stageWidth()) + if len(segments) == 0 { + return []string{prefix + tail} + } + + lines := make([]string, 0, len(segments)) + lines = append(lines, prefix+segments[0]) + for _, segment := range segments[1:] { + lines = append(lines, strings.Repeat(" ", indent)+" "+segment) } - return prefix + tail + return lines +} + +// stageWidth is the room a wrapped stage has. The two columns subtracted are +// the indent a continuation line carries, so every line of a wrapped stage +// fits the same budget as the first. +func (r *renderer) stageWidth() int { + return max(minStageWidth, r.lineWidth()-r.prefixWidth()-2) } // noteLines renders a request's error under its row, wrapped and indented to @@ -324,7 +421,7 @@ func (r *renderer) noteLines(rw *Row) []string { indent := r.prefixWidth() // A piped run spends most of the line on URLs, so the wrap width is floored // rather than allowed to collapse to nothing. - width := max(minNoteWidth, maxLineWidth-indent-2) + width := max(minNoteWidth, r.lineWidth()-indent-2) wrapped := wrap(rw.Note, width) lines := make([]string, 0, len(wrapped)) diff --git a/submitqueue/client/view_test.go b/submitqueue/client/view_test.go index b483f49b..688eef50 100644 --- a/submitqueue/client/view_test.go +++ b/submitqueue/client/view_test.go @@ -294,12 +294,68 @@ func TestDrawStaysWithinLineWidth(t *testing.T) { out := captureStdout(t, func() { r.draw(rows, strings.Repeat("status ", 40)) }) for _, line := range strings.Split(out, "\n") { line = strings.ReplaceAll(line, "\033[K", "") - assert.LessOrEqual(t, len([]rune(line)), maxLineWidth, "line too wide: %q", line) + assert.LessOrEqual(t, len([]rune(line)), r.lineWidth(), "line too wide: %q", line) } } -// TestDrawPipedSkipsClockOnlyRedraws keeps a redirected run's log readable: the -// table is reprinted when it moves, not once a second because the clock did. +// TestDrawFollowsAResize is the regression for a table that wraps correctly at +// startup and then stops. The width is not fixed for the life 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 the +// redraw, which counts the lines it emitted rather than the lines that appeared, +// drifts a little further with every frame. +func TestDrawFollowsAResize(t *testing.T) { + width := 200 + r := newRenderer() + r.inPlace = true + r.width = width + r.size = func() (int, bool) { return width, true } + + rows := []*Row{{SQID: "demo-queue/72", Submitted: time.Now(), Trail: longTrail}} + + wide := captureStdout(t, func() { r.draw(rows, "watching") }) + for _, line := range strings.Split(wide, "\n") { + assert.LessOrEqual(t, len([]rune(visible(strings.ReplaceAll(line, "\033[K", "")))), width) + } + + // The window is dragged in. Nothing tells the process; it has to look. + width = 94 + narrow := captureStdout(t, func() { r.draw(rows, "watching") }) + require.NotEmpty(t, narrow) + for _, line := range strings.Split(narrow, "\n") { + clean := visible(strings.ReplaceAll(line, "\033[K", "")) + assert.LessOrEqual(t, len([]rune(clean)), width, + "a line wider than the window is wrapped by the terminal, which desyncs the redraw: %q", clean) + } +} + +// TestDrawKeepsTheLastWidthWhenTheTerminalStopsAnswering guards the other +// direction: a probe that fails once must not snap the table to the fallback +// width, which on a narrow window is wider than the window itself. +func TestDrawKeepsTheLastWidthWhenTheTerminalStopsAnswering(t *testing.T) { + r := newRenderer() + r.inPlace = true + r.width = 94 + r.size = func() (int, bool) { return defaultLineWidth, false } + + r.resize() + assert.Equal(t, 94, r.width, "an unanswered probe leaves the last known width alone") +} + +// TestNewRendererNeedsASizeToRedrawInPlace pins the two probes together. Drawing +// in place means wrapping, and wrapping to a guessed width is what puts a line +// past the edge of a narrower window. A terminal that will not report its size +// is therefore rendered as a log, which needs no width at all. +func TestNewRendererNeedsASizeToRedrawInPlace(t *testing.T) { + // Test stdout is not a sized terminal, which is exactly the case at issue. + r := newRenderer() + width, sized := terminalSize() + require.False(t, sized, "test stdout is not expected to be a sized terminal") + assert.Equal(t, defaultLineWidth, width) + assert.False(t, r.inPlace, "without a known width the renderer must not wrap and redraw") +} + func TestDrawPipedSkipsClockOnlyRedraws(t *testing.T) { r := newRenderer() r.inPlace = false @@ -554,7 +610,7 @@ func TestNoteLinesRenderErrorInFull(t *testing.T) { var text strings.Builder for i, line := range lines { - assert.LessOrEqual(t, len([]rune(line)), maxLineWidth, "a wrapped note still has to fit the line") + assert.LessOrEqual(t, len([]rune(line)), r.lineWidth(), "a wrapped note still has to fit the line") trimmed := strings.TrimLeft(line, " ") if i == 0 { assert.True(t, strings.HasPrefix(trimmed, "↳ "), "the first line is marked") @@ -567,8 +623,8 @@ func TestNoteLinesRenderErrorInFull(t *testing.T) { "the tail of the error is what says what went wrong; it must survive") // The row itself keeps only the trail, so the columns stay aligned. - assert.NotContains(t, r.rowLine(failed), "speculator failed") - assert.NotContains(t, r.rowLine(failed), "…") + assert.NotContains(t, strings.Join(r.rowLines(failed), "\n"), "speculator failed") + assert.NotContains(t, strings.Join(r.rowLines(failed), "\n"), "…") } // TestNoteLinesIndentToStageColumn keeps a wrapped error visually attached to @@ -600,7 +656,7 @@ func TestRowLineAlignment(t *testing.T) { r.fit(rows) for _, rw := range rows { - shown := []rune(visible(r.rowLine(rw))) + shown := []rune(visible(r.rowLines(rw)[0])) require.GreaterOrEqual(t, len(shown), r.prefixWidth()) assert.Equal(t, rw.stage(), string(shown[r.prefixWidth():]), "the stage should start at column %d and be rendered whole", r.prefixWidth()) @@ -656,3 +712,106 @@ func head(s string, n int) string { } return s[:n] } + +// longTrail is the shape that prompted wrapping: every status the pipeline now +// publishes, which no longer fits a default-width line. +var longTrail = []string{ + "accepted", "started", "validating", "validated", "batched", + "speculating", "speculated", "building", "built", "landing", "landed", +} + +func TestRowLinesWrapsRatherThanTruncates(t *testing.T) { + r := newRenderer() + r.inPlace = true + rw := &Row{SQID: "demo-queue/1", Submitted: time.Now(), Trail: longTrail} + r.fit([]*Row{rw}) + + lines := r.rowLines(rw) + require.Greater(t, len(lines), 1, "a trail this long has to wrap") + + joined := strings.Join(lines, " ") + assert.NotContains(t, joined, "…", "nothing is cut, so there is no ellipsis") + for _, status := range longTrail { + assert.Contains(t, joined, status, "every status survives the wrap") + } + assert.Contains(t, lines[len(lines)-1], "landed", + "the end of the trail is where the request is; it must be the part that shows") +} + +func TestRowLinesContinuationsAlignUnderTheStage(t *testing.T) { + r := newRenderer() + r.inPlace = true + rw := &Row{SQID: "demo-queue/1", Submitted: time.Now(), Trail: longTrail} + r.fit([]*Row{rw}) + + lines := r.rowLines(rw) + require.Greater(t, len(lines), 1) + + for _, line := range lines[1:] { + leading := len(line) - len(strings.TrimLeft(line, " ")) + assert.GreaterOrEqual(t, leading, r.prefixWidth(), + "a continuation sits under the stage column, not under the request id") + } +} + +func TestRowLinesFitTheWidth(t *testing.T) { + // The redraw moves the cursor back by the number of lines it emitted, so a + // line wide enough to wrap physically would desync every redraw after it. + widths := []int{80, 100, 120, 200} + for _, width := range widths { + t.Run(fmt.Sprintf("width %d", width), func(t *testing.T) { + r := newRenderer() + r.inPlace = true + r.width = width + rw := &Row{SQID: "demo-queue/1", Submitted: time.Now(), Trail: longTrail} + r.fit([]*Row{rw}) + + for _, line := range r.rowLines(rw) { + assert.LessOrEqual(t, len([]rune(visible(line))), width, "line too wide: %q", line) + } + }) + } +} + +func TestRowLinesUseTheWholeWidthBeforeWrapping(t *testing.T) { + // The point of asking the terminal how wide it is: a window with room for + // the whole trail should show it on one line. + r := newRenderer() + r.inPlace = true + r.width = 240 + rw := &Row{SQID: "demo-queue/1", Submitted: time.Now(), Trail: longTrail} + r.fit([]*Row{rw}) + + lines := r.rowLines(rw) + assert.Len(t, lines, 1, "a wide terminal needs no wrapping") + assert.Contains(t, lines[0], "landed") +} + +func TestRowLinesPipedStayOnOneLine(t *testing.T) { + // A log has no width to respect and is easier to read and grep unwrapped. + r := newRenderer() + r.inPlace = false + rw := &Row{SQID: "demo-queue/1", Submitted: time.Now(), Trail: longTrail} + r.fit([]*Row{rw}) + + lines := r.rowLines(rw) + require.Len(t, lines, 1) + assert.Contains(t, lines[0], "landed") + assert.NotContains(t, lines[0], "…") +} + +func TestStageWidthHasAFloor(t *testing.T) { + r := newRenderer() + r.inPlace = true + r.width = 20 + r.wRequest, r.wChanges, r.wElapsed = 40, 40, 40 + + assert.Equal(t, minStageWidth, r.stageWidth(), + "a window too narrow to hold the columns still wraps to something readable") +} + +func TestLineWidthFallsBackWhenUnset(t *testing.T) { + // Tests build renderers directly; a zero width must not collapse the table. + r := &renderer{inPlace: true} + assert.Equal(t, defaultLineWidth, r.lineWidth()) +} From 81242efbdce6c1558f2553f8811e961c6055e88d Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Tue, 11 Aug 2026 12:30:06 -0700 Subject: [PATCH 3/5] feat(demo): open independent pull requests in parallel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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. --- Makefile | 4 +- doc/howto/PROVIDER-E2E.md | 3 + service/submitqueue/demo/pr/BUILD.bazel | 1 + service/submitqueue/demo/pr/main.go | 273 ++++++++++++++++------- service/submitqueue/demo/pr/main_test.go | 40 ++++ 5 files changed, 238 insertions(+), 83 deletions(-) diff --git a/Makefile b/Makefile index 7dc465c9..bf70bd00 100644 --- a/Makefile +++ b/Makefile @@ -56,6 +56,7 @@ export SQ_PROVIDER_CONFIG_DIR ?= $(REPO_ROOT)/service/submitqueue/demo/provider/ DEMO_REPO ?= behinddwalls/sq-demo COUNT ?= 3 FILES ?= 3 +CONCURRENCY ?= 5 STACKED ?= false SINCE ?= 1h LIMIT ?= 50 @@ -155,11 +156,12 @@ clean-proto: ## Clean generated proto files @rm -f $(foreach p,$(PROTO_PACKAGES),$(p)/protopb/*.pb.go $(p)/protopb/*.pb.yarpc.go) @echo "Proto clean complete!" -demo-pr: ## Create N PRs in the demo repo, enqueue each as it is created, and watch (COUNT=3 FILES=3; needs GITHUB_TOKEN) +demo-pr: ## Create N PRs in the demo repo, enqueue each as it is created, and watch (COUNT=3 FILES=3 CONCURRENCY=5; needs GITHUB_TOKEN) @$(BAZEL) run //service/submitqueue/demo/pr -- \ -repo $(DEMO_REPO) \ -count $(COUNT) \ -files $(FILES) \ + -concurrency $(CONCURRENCY) \ -stacked=$(STACKED) \ -addr $(GATEWAY_ADDR) \ -queue $(QUEUE) \ diff --git a/doc/howto/PROVIDER-E2E.md b/doc/howto/PROVIDER-E2E.md index 8873ec50..e7cf15b1 100644 --- a/doc/howto/PROVIDER-E2E.md +++ b/doc/howto/PROVIDER-E2E.md @@ -97,12 +97,15 @@ Opening pull requests by hand gets old fast. `demo-pr` creates them, enqueues th make demo-pr # 3 independent PRs, each enqueued as it is created make demo-pr COUNT=8 # more traffic make demo-pr FILES=8 # wider changes, more files per PR +make demo-pr CONCURRENCY=1 # create them one at a time make demo-pr STACKED=true # one stack, enqueued as a single request make demo-pr LAND=false # create only, print the land command ``` Each pull request is enqueued the moment it exists, so the queue is already working on the first while the last is still being opened. That overlap is the point: a queue holding one request at a time never batches, never analyzes a conflict against another batch, and never speculates. Nothing is awaited until every request is in. +Independent pull requests are created **five at a time** by default (`CONCURRENCY`). Opening one is several round trips — a branch, a commit per file, the pull request itself — so creating them serially was most of what a large run spent its time on, and it delayed the overlap the demo exists to show. A stack ignores the setting: each of its changes is based on the branch before it, so the next cannot be cut until the previous head exists. Lower it if the provider starts refusing bursts. + The table is there from the start — one row per land request, drawn before the first pull request exists and filled in as the run proceeds. Whatever is happening right now is a single line underneath it, so creating and enqueuing does not scroll the table away: ``` diff --git a/service/submitqueue/demo/pr/BUILD.bazel b/service/submitqueue/demo/pr/BUILD.bazel index 23f1b641..0917fa7a 100644 --- a/service/submitqueue/demo/pr/BUILD.bazel +++ b/service/submitqueue/demo/pr/BUILD.bazel @@ -12,6 +12,7 @@ go_library( "//api/base/mergestrategy/protopb:go_default_library", "//platform/base/change/github:go_default_library", "//submitqueue/client:go_default_library", + "@org_golang_x_sync//errgroup:go_default_library", ], ) diff --git a/service/submitqueue/demo/pr/main.go b/service/submitqueue/demo/pr/main.go index c0f1ffbc..e898e3f9 100644 --- a/service/submitqueue/demo/pr/main.go +++ b/service/submitqueue/demo/pr/main.go @@ -23,6 +23,11 @@ // batch, and never speculates; those behaviors only appear when requests // overlap. The table watches all of them at once. // +// Independent pull requests are opened several at a time (-concurrency), since +// each is several round trips to the provider and nothing about them depends on +// the others. A stack cannot be: every change in it is based on the branch +// before it, so the next cannot be cut until the previous head exists. +// // Two shapes of change, because the pipeline treats them differently: // // - independent (default): each pull request targets the base branch and is @@ -56,6 +61,7 @@ import ( mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb" githubchange "github.com/uber/submitqueue/platform/base/change/github" "github.com/uber/submitqueue/submitqueue/client" + "golang.org/x/sync/errgroup" ) func main() { @@ -68,22 +74,23 @@ func main() { // config is everything the run needs, resolved from flags and the environment. type config struct { - repo string - base string - count int - files int - stacked bool - prefix string - land bool - watch bool - addr string - tls bool - tokenEnv string - queue string - strategy string - token string - apiRoot string - host string + repo string + base string + count int + files int + concurrency int + stacked bool + prefix string + land bool + watch bool + addr string + tls bool + tokenEnv string + queue string + strategy string + token string + apiRoot string + host string } func parseFlags() config { @@ -92,6 +99,8 @@ func parseFlags() config { flag.StringVar(&c.base, "base", "main", "branch the changes target") flag.IntVar(&c.count, "count", 3, "how many pull requests to create") flag.IntVar(&c.files, "files", 3, "fewest files each pull request touches; the actual count varies a little above it") + flag.IntVar(&c.concurrency, "concurrency", 5, + "how many pull requests to create at once; a stack ignores it, being sequential by nature") flag.BoolVar(&c.stacked, "stacked", false, "chain the pull requests and enqueue them as one stack") flag.StringVar(&c.prefix, "prefix", "demo", "branch name prefix") flag.BoolVar(&c.land, "land", true, "enqueue each pull request as it is created") @@ -110,14 +119,11 @@ func parseFlags() config { } func run(ctx context.Context, cfg config) error { - if cfg.token == "" { - return fmt.Errorf("GITHUB_TOKEN is not set; it is the same credential the stack uses") - } - if cfg.count < 1 { - return fmt.Errorf("-count must be at least 1") + if err := cfg.validate(); err != nil { + return err } - owner, repo, ok := strings.Cut(cfg.repo, "/") - if !ok || owner == "" || repo == "" { + owner, repo, _ := strings.Cut(cfg.repo, "/") + if owner == "" || repo == "" { return fmt.Errorf("-repo %q must be owner/name", cfg.repo) } strategy, err := client.ParseStrategy(cfg.strategy) @@ -199,14 +205,40 @@ func shape(cfg config) string { if cfg.stacked { return "stacked, enqueued as one request once the chain exists" } + if cfg.concurrency > 1 { + return fmt.Sprintf("independent, %d at a time, each enqueued as soon as it is created", cfg.concurrency) + } return "independent, each enqueued as soon as it is created" } +// validate rejects a configuration the run cannot proceed with. +func (c config) validate() error { + if c.token == "" { + return fmt.Errorf("GITHUB_TOKEN is not set; it is the same credential the stack uses") + } + if c.count < 1 { + return fmt.Errorf("-count must be at least 1") + } + if c.concurrency < 1 { + return fmt.Errorf("-concurrency must be at least 1") + } + if c.files < 1 { + return fmt.Errorf("-files must be at least 1") + } + if _, _, ok := strings.Cut(c.repo, "/"); !ok { + return fmt.Errorf("-repo %q must be owner/name", c.repo) + } + return nil +} + // change is one pull request this run created. type change struct { number int url string branch string + // headSHA is the commit the pull request now points at, which the next + // change in a stack branches from. + headSHA string // uri is the SubmitQueue change URI pinning the pull request to its head. uri string } @@ -289,80 +321,102 @@ func createAndEnqueue( tag, baseSHA string, t *client.Tracker, ) ([]change, error) { - created := make([]change, 0, cfg.count) + if cfg.stacked { + return createStack(ctx, gh, sq, cfg, strategy, tag, baseSHA, t) + } + return createIndependent(ctx, gh, sq, cfg, strategy, tag, baseSHA, t) +} + +// createIndependent opens the pull requests concurrently, up to the configured +// limit, enqueuing each the moment it exists. +// +// Independent changes have nothing to say to each other: each branches from the +// same base and writes files no other change touches, so the only reason to +// create them one at a time was that the loop did. Creating a pull request is +// several round trips to the provider — a branch, a commit per file, the pull +// request itself — and doing that serially is most of what a large run spends +// its time on. It also delays the overlap the demo exists to show, since the +// queue cannot work on requests that have not been submitted yet. +// +// The limit is there because the provider is a shared service with its own +// opinion about burst rates, and because the point is to feed the queue, not to +// find out how fast a repository can be hammered. +func createIndependent( + ctx context.Context, + gh *githubClient, + sq *client.Client, + cfg config, + strategy mergestrategypb.Strategy, + tag, baseSHA string, + t *client.Tracker, +) ([]change, error) { rows := t.Rows() + // Indexed rather than appended: the workers finish in whatever order the + // provider answers them, and the caller still wants the run's own order. + created := make([]change, cfg.count) - parentBranch, parentSHA := cfg.base, baseSHA - for i := 1; i <= cfg.count; i++ { - // A stack is one request, so every change lands on the single row. - target := rows[0] - if !cfg.stacked { - target = rows[i-1] - } + group, groupCtx := errgroup.WithContext(ctx) + group.SetLimit(cfg.concurrency) - branch := fmt.Sprintf("%s/%s/%d", cfg.prefix, tag, i) - t.Note("creating branch %s", branch) - if err := gh.createBranch(ctx, branch, parentSHA); err != nil { - return nil, fmt.Errorf("create branch %s: %w", branch, err) - } + for i := 1; i <= cfg.count; i++ { + group.Go(func() error { + c, err := createOne(groupCtx, gh, cfg, tag, baseSHA, cfg.base, i, t, rows[i-1]) + if err != nil { + return err + } + created[i-1] = c - // Each file is its own commit, so the pull request arrives as a range of - // commits rather than a single edit. The last one is the head the change - // URI pins. - var headSHA string - fileCount := changeFileCount(tag, i, cfg.files) - for k := 1; k <= fileCount; k++ { - path := changeFilePath(tag, i, k) - body := fmt.Sprintf("change %d of run %s\nfile %d of %d\n", i, tag, k, fileCount) - t.Note("committing %s (%d/%d)", path, k, fileCount) - - message := fmt.Sprintf("demo change %d (run %s): file %d of %d", i, tag, k, fileCount) - sha, err := gh.commitFile(ctx, branch, path, body, message) + if !cfg.land { + return nil + } + t.Note("enqueuing #%d", c.number) + sqid, err := sq.Land(groupCtx, cfg.queue, urisOf([]change{c}), strategy) if err != nil { - return nil, fmt.Errorf("commit %s to %s: %w", path, branch, err) + return err } - headSHA = sha - } + t.Update(func() { rows[i-1].SQID, rows[i-1].Submitted = sqid, time.Now() }) + return nil + }) + } - t.Note("opening pull request for %s", branch) - number, url, err := gh.openPR(ctx, fmt.Sprintf("demo change %d (run %s)", i, tag), branch, parentBranch) - if err != nil { - return nil, fmt.Errorf("open pull request for %s: %w", branch, err) - } + if err := group.Wait(); err != nil { + return nil, err + } + return created, nil +} - c := change{ - number: number, url: url, branch: branch, - uri: githubchange.ChangeID{ - Scheme: "github", Host: cfg.host, Org: gh.owner, Repo: gh.repo, - PRNumber: number, HeadCommitSHA: headSHA, - }.String(), - } - created = append(created, c) - // The cell is what the table shows for this change: the pull request - // number, clickable where the terminal allows it. - cell := client.Cell{Text: fmt.Sprintf("#%d", number), URL: url} - t.Update(func() { target.Cells = append(target.Cells, cell) }) - - if cfg.stacked { - // The next change builds on this one, so it sees this change's - // content and its pull request is based on this branch. - parentBranch, parentSHA = branch, headSHA - continue - } - if !cfg.land { - continue - } - t.Note("enqueuing #%d", number) - sqid, err := sq.Land(ctx, cfg.queue, urisOf([]change{c}), strategy) +// createStack opens the pull requests one after another, each based on the one +// before it, and submits the whole chain as a single request. +// +// This one cannot be parallelized, and not for want of trying: a change is +// based on the branch of the change before it and must see its content, so the +// next branch cannot be cut until the previous head exists. +func createStack( + ctx context.Context, + gh *githubClient, + sq *client.Client, + cfg config, + strategy mergestrategypb.Strategy, + tag, baseSHA string, + t *client.Tracker, +) ([]change, error) { + rows := t.Rows() + created := make([]change, 0, cfg.count) + + parentBranch, parentSHA := cfg.base, baseSHA + for i := 1; i <= cfg.count; i++ { + // A stack is one request, so every change lands on the single row. + c, err := createOne(ctx, gh, cfg, tag, parentSHA, parentBranch, i, t, rows[0]) if err != nil { return nil, err } - t.Update(func() { target.SQID, target.Submitted = sqid, time.Now() }) + created = append(created, c) + parentBranch, parentSHA = c.branch, c.headSHA } // The stack goes in as one request, which is only possible now that every // change in it exists. - if cfg.stacked && cfg.land { + if cfg.land { t.Note("enqueuing the stack") sqid, err := sq.Land(ctx, cfg.queue, urisOf(created), strategy) if err != nil { @@ -373,6 +427,61 @@ func createAndEnqueue( return created, nil } +// createOne cuts a branch from parentSHA, writes the change's files to it, and +// opens a pull request against parentBranch, recording it on the given row. +func createOne( + ctx context.Context, + gh *githubClient, + cfg config, + tag, parentSHA, parentBranch string, + i int, + t *client.Tracker, + target *client.Row, +) (change, error) { + branch := fmt.Sprintf("%s/%s/%d", cfg.prefix, tag, i) + t.Note("creating branch %s", branch) + if err := gh.createBranch(ctx, branch, parentSHA); err != nil { + return change{}, fmt.Errorf("create branch %s: %w", branch, err) + } + + // Each file is its own commit, so the pull request arrives as a range of + // commits rather than a single edit. The last one is the head the change + // URI pins. + var headSHA string + fileCount := changeFileCount(tag, i, cfg.files) + for k := 1; k <= fileCount; k++ { + path := changeFilePath(tag, i, k) + body := fmt.Sprintf("change %d of run %s\nfile %d of %d\n", i, tag, k, fileCount) + t.Note("committing %s (%d/%d)", path, k, fileCount) + + message := fmt.Sprintf("demo change %d (run %s): file %d of %d", i, tag, k, fileCount) + sha, err := gh.commitFile(ctx, branch, path, body, message) + if err != nil { + return change{}, fmt.Errorf("commit %s to %s: %w", path, branch, err) + } + headSHA = sha + } + + t.Note("opening pull request for %s", branch) + number, url, err := gh.openPR(ctx, fmt.Sprintf("demo change %d (run %s)", i, tag), branch, parentBranch) + if err != nil { + return change{}, fmt.Errorf("open pull request for %s: %w", branch, err) + } + + c := change{ + number: number, url: url, branch: branch, headSHA: headSHA, + uri: githubchange.ChangeID{ + Scheme: "github", Host: cfg.host, Org: gh.owner, Repo: gh.repo, + PRNumber: number, HeadCommitSHA: headSHA, + }.String(), + } + // The cell is what the table shows for this change: the pull request + // number, clickable where the terminal allows it. + cell := client.Cell{Text: fmt.Sprintf("#%d", number), URL: url} + t.Update(func() { target.Cells = append(target.Cells, cell) }) + return c, nil +} + // urisOf is the change URIs the run pinned, in caller order. func urisOf(cs []change) []string { out := make([]string, 0, len(cs)) diff --git a/service/submitqueue/demo/pr/main_test.go b/service/submitqueue/demo/pr/main_test.go index dccad12d..cd13684d 100644 --- a/service/submitqueue/demo/pr/main_test.go +++ b/service/submitqueue/demo/pr/main_test.go @@ -110,3 +110,43 @@ func TestRowCount(t *testing.T) { assert.Equal(t, 3, rowCount(config{count: 3}), "independent changes are one request each") assert.Equal(t, 1, rowCount(config{count: 3, stacked: true}), "a stack is a single request") } + +func TestConfigValidate(t *testing.T) { + valid := config{token: "t", repo: "owner/name", count: 3, files: 3, concurrency: 5} + + tests := []struct { + name string + mutate func(*config) + wantErr bool + }{ + {name: "a usable configuration", mutate: func(*config) {}}, + {name: "concurrency of one is sequential, not invalid", mutate: func(c *config) { c.concurrency = 1 }}, + {name: "no token", mutate: func(c *config) { c.token = "" }, wantErr: true}, + {name: "no changes to make", mutate: func(c *config) { c.count = 0 }, wantErr: true}, + {name: "no files to write", mutate: func(c *config) { c.files = 0 }, wantErr: true}, + {name: "zero concurrency would never start", mutate: func(c *config) { c.concurrency = 0 }, wantErr: true}, + {name: "negative concurrency", mutate: func(c *config) { c.concurrency = -1 }, wantErr: true}, + {name: "repo without an owner", mutate: func(c *config) { c.repo = "name" }, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := valid + tt.mutate(&cfg) + err := cfg.validate() + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + }) + } +} + +func TestShapeReportsConcurrency(t *testing.T) { + assert.Contains(t, shape(config{count: 10, concurrency: 5}), "5 at a time") + assert.NotContains(t, shape(config{count: 10, concurrency: 1}), "at a time", + "one at a time is just sequential; saying so adds nothing") + assert.Contains(t, shape(config{count: 10, concurrency: 5, stacked: true}), "stacked", + "a stack is sequential whatever the limit says") +} From aa406099cc18c34028fc57823ba3f1a13d95aab6 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Tue, 11 Aug 2026 12:30:28 -0700 Subject: [PATCH 4/5] fix(demo): let demo-queue inherit the fake build runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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. # Conflicts: # service/submitqueue/demo/provider/github/profiles.yaml # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # interactive rebase in progress; onto ff81ffbf # Last commands done (4 commands done): # pick f4315648 # feat(demo): open independent pull requests in parallel # pick 0746c189 # fix(demo): let demo-queue inherit the fake build runner # Next command to do (1 remaining command): # pick 06080b1a # fix(client): bound the list window at the call time # You are currently rebasing. # # Changes to be committed: # modified: service/submitqueue/demo/provider/github/profiles.yaml # --- .../demo/provider/github/profiles.yaml | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/service/submitqueue/demo/provider/github/profiles.yaml b/service/submitqueue/demo/provider/github/profiles.yaml index 1cf4cf56..d210837d 100644 --- a/service/submitqueue/demo/provider/github/profiles.yaml +++ b/service/submitqueue/demo/provider/github/profiles.yaml @@ -25,19 +25,13 @@ queues: # detection. analyzer: {type: pathoverlap, by: directory} - # Every build succeeds instantly, so a land completes in seconds and the - # demo exercises the merge rather than waiting on CI. - buildRunner: - type: githubactions - owner: behinddwalls - repo: sq-demo - workflow: ci.yml # file name or numeric workflow id - ref: main # the branch the workflow definition is read from + # The build runner is inherited from the defaults above, so every build + # succeeds instantly. A land then completes in seconds, and the demo shows + # the queue and the merge rather than spending its time waiting on CI. # - # To run real CI instead, replace the line above with the block below. It - # needs a workflow in the target repository that is triggerable by - # workflow_dispatch and accepts the sq_base_uris / sq_head_uris inputs, and - # a token with the `workflow` scope. + # To run real CI instead, add the block below. It needs a workflow in the + # target repository that is triggerable by workflow_dispatch and accepts the + # sq_base_uris / sq_head_uris inputs, and a token with the `workflow` scope. # # buildRunner: # type: githubactions From 60b23c9604e02a511a8072f17268b2fd670a1da4 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Thu, 13 Aug 2026 10:04:41 -0700 Subject: [PATCH 5/5] fix(client): bound the list window at the call time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `List` never set `ReceivedBeforeMs`, and the gateway rejects any list where `received_at_or_after_ms >= received_before_ms` (`gateway/controller/list.go:85`). With `Since` unset the client sent `(0, 0)` and with `-since 1h` it sent `(now-1h, 0)` — both invalid, so every `list` call, and every `watch` that did not name its requests with `-sqid`, failed with InvalidArgument against a real gateway. Both bounds are now taken from a single `now` before the first page. Fixing them up front is required rather than tidy: the continuation token pins both bounds (`list.go:111`), so a bound recomputed per page would be rejected from the second page on. --- submitqueue/client/query.go | 17 ++++++++++---- submitqueue/client/query_test.go | 38 ++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/submitqueue/client/query.go b/submitqueue/client/query.go index 73706f70..97ebeeb0 100644 --- a/submitqueue/client/query.go +++ b/submitqueue/client/query.go @@ -29,8 +29,8 @@ type ListQuery struct { // own queue. Queue string - // Since bounds the window to requests received within it. Zero reads from - // the beginning of retained history. + // Since bounds the window to requests received within it, ending at the + // time of the call. Zero reads from the beginning of retained history. Since time.Duration // Limit caps how many requests are returned across all pages. Zero means @@ -52,9 +52,18 @@ func (c *Client) List(ctx context.Context, q ListQuery) ([]*pb.RequestSummary, e return nil, fmt.Errorf("queue must not be empty") } - req := &pb.ListRequest{Queue: q.Queue, PageSize: int32(q.PageSize)} + // Both bounds are fixed before the first page. The gateway requires + // received_at_or_after_ms < received_before_ms, so an unset upper bound + // rejects every call, and its continuation token pins both bounds, so one + // recomputed per page would be rejected from the second page on. + now := time.Now() + req := &pb.ListRequest{ + Queue: q.Queue, + PageSize: int32(q.PageSize), + ReceivedBeforeMs: now.UnixMilli(), + } if q.Since > 0 { - req.ReceivedAtOrAfterMs = time.Now().Add(-q.Since).UnixMilli() + req.ReceivedAtOrAfterMs = now.Add(-q.Since).UnixMilli() } var out []*pb.RequestSummary diff --git a/submitqueue/client/query_test.go b/submitqueue/client/query_test.go index e062eea3..785ffdf7 100644 --- a/submitqueue/client/query_test.go +++ b/submitqueue/client/query_test.go @@ -108,6 +108,42 @@ func TestListWithoutSinceLeavesTheWindowOpen(t *testing.T) { "no window means all retained history, not a bound of zero-time") } +func TestListClosesTheWindowAtTheCallTime(t *testing.T) { + // The gateway rejects a list whose lower bound is not strictly below its + // upper one, so leaving the upper bound unset fails every call. + gw := &pagingGateway{pages: [][]string{{"q/1"}}} + sq, stop := dial(t, gw) + defer stop() + + before := time.Now().UnixMilli() + _, err := sq.List(context.Background(), ListQuery{Queue: "q"}) + require.NoError(t, err) + after := time.Now().UnixMilli() + + got := gw.lastRequest.GetReceivedBeforeMs() + assert.GreaterOrEqual(t, got, before) + assert.LessOrEqual(t, got, after) + assert.Less(t, gw.lastRequest.GetReceivedAtOrAfterMs(), got, + "the gateway requires received_at_or_after_ms < received_before_ms") +} + +func TestListKeepsTheWindowFixedAcrossPages(t *testing.T) { + // The continuation token pins both bounds, so a bound recomputed per page + // would be rejected from the second page on. + gw := &pagingGateway{pages: [][]string{{"q/1"}, {"q/2"}, {"q/3"}}} + sq, stop := dial(t, gw) + defer stop() + + _, err := sq.List(context.Background(), ListQuery{Queue: "q", Since: time.Hour}) + require.NoError(t, err) + + require.Len(t, gw.requests, 3) + for _, req := range gw.requests[1:] { + assert.Equal(t, gw.requests[0].GetReceivedAtOrAfterMs(), req.GetReceivedAtOrAfterMs()) + assert.Equal(t, gw.requests[0].GetReceivedBeforeMs(), req.GetReceivedBeforeMs()) + } +} + func TestRowsFromSummaries(t *testing.T) { received := time.Now().Add(-5 * time.Minute) rows := RowsFromSummaries([]*pb.RequestSummary{ @@ -143,10 +179,12 @@ type pagingGateway struct { alwaysToken bool calls int + requests []*pb.ListRequest lastRequest *pb.ListRequest } func (g *pagingGateway) List(_ context.Context, req *pb.ListRequest) (*pb.ListResponse, error) { + g.requests = append(g.requests, req) g.lastRequest = req page := g.calls g.calls++