diff --git a/CHANGELOG.md b/CHANGELOG.md index facb023..4c36b17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -106,6 +106,46 @@ to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). above tests it rather than leaving the reader to notice. Verified against a live tenant that accepts `baselinePolicyId`. +- **`grants list --wait`, with `--wait-stable`, `--wait-min` and + `--wait-timeout`.** Grant provisioning is asynchronous, so a read taken right + after a grant or revoke can catch the set mid-change. `--wait` re-reads every + page every 5s until the same grants come back `--wait-stable` times in a row + (default 3, minimum 2), then prints that settled set; progress goes to + stderr, so stdout stays pure NDJSON. `--wait-timeout` (default 4m) bounds the + whole wait. The 5s poll interval is fixed and not a flag, so `--wait-stable` + and `--wait-timeout` are the tunable parts; a `--wait-stable` that cannot fit + inside `--wait-timeout` is rejected up front rather than left to time out. + `--wait` is also rejected with `--page-token`, since a pinned cursor is not a + stable set. + + `--wait-min` sets a floor on how many grants must be present before the wait + can settle (default `0` = today's behavior). It exists because an empty + result is perfectly stable: a filter matching nothing settles at the first + opportunity, roughly 10s at the defaults, and exits `0` with zero rows. Run + right after a grant -- the workflow `--wait` advertises -- that reads as "it + did not happen" when the truth is "not yet", since provisioning runs about a + minute. The default stays `0` because empty-and-stable is the *correct* + answer when waiting for a revoke; `--wait-min 1` is how you say which of the + two you meant. + + `--wait-stable` defaults to 3, not 2, deliberately. Two equal reads cannot be + told apart from a pause mid-change. The case that motivated it is reported + from the field rather than measured here: MCP tool discovery streams + (0 -> 40 -> 101 with pauses between batches), so a presence check -- "at + least one row exists" -- approved 20 of 28 tools and reported success. Three + is still a heuristic, not a proof: raise `--wait-stable` past the longest + mid-stream pause you have actually seen, remembering that `n` reads span + `(n-1)` five-second intervals. + + Two behaviors worth knowing before scripting against it. `--wait` settles on + the WHOLE matching set, so it fetches every page on every poll regardless of + `--limit`; `--limit` only truncates what is finally printed. And a server + that re-issues one `nextPageToken` forever is now an error ("API returned the + same nextPageToken twice in a row..."), exit 1, matching the guard + `api --paginate` already carried -- without it each poll would become a + request storm bounded only by `--wait-timeout`, then blame provisioning for + the timeout. + - **`apps owners`, `apps add-owner`, and `apps remove-owner`.** `apps get`'s `appOwners` field was empty on every app checked, while `GET .../owners` returns the owners `apps set-owners` had already written, but reading @@ -156,6 +196,32 @@ to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). stops matching the one it enforces, or if either history command's 200 ceiling is dropped to the default 100. +- **The `--wait` poll loop is now one shared primitive (`cmd/wait.go`).** It + was written for `apps set-owners` and was the only polling loop in `cmd/`; + it is now `runWait(cmd, waitOp{...})` with a pluggable `Done` predicate, plus + `addWaitFlags` so `--wait`/`--wait-timeout` are declared identically + everywhere. `apps set-owners` behavior and output are unchanged -- its + `appOwners` wording was corrected separately, see Fixed below. + + Two predicates ship: `untilPresent` (what `set-owners` always did -- every + requested id has shown up) and `untilStable` (the polled value held steady + across N consecutive reads), which presence-waiting cannot express and which + `grants list --wait` uses. + +- **The `request-access` guide's Verify step now waits instead of telling you + to.** `c1i docs guide request-access` handed an agent a bare `grants list` + plus prose about re-running it in a minute or two -- exactly the poll-by-hand + workflow `--wait` replaces, and exactly the case where an empty read is + mistaken for a denial. It now shows `--wait --wait-min 1` after a grant and + says which exit code means what. + + The revoke direction is documented, not solved. `--wait` settles on whatever + is steady, and a grant that has not been deprovisioned yet is perfectly + steady, so exit 0 can still list the row -- meaning "not yet, re-run", not + "the revoke failed". `--wait-min` is a floor and cannot express a ceiling, + and the guide now says so rather than implying a bare `--wait` waits *for* + empty. + ### Fixed - **Flags that shipped undocumented are now documented, and a guard keeps it @@ -274,6 +340,14 @@ to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). `clientInfo.version` -- and a `git add -A` would have committed one as a gitlink (mode 160000), an embedded-repo pointer no clone can resolve. + Re-measured while extracting the shared wait primitive, and the help text now + states the discriminating half of the observation rather than just the + coverage: `appOwners` was `[]` on all 46 apps then in the tenant -- + *including the 45 that `GET .../ownerids` reported owners for* -- and on a + freshly created app immediately after `set-owners --wait` confirmed two + owners had provisioned. (The 47/46 counts above were the earlier pass; the + apps it created for the test have since been deleted.) + ## [0.5.0] - 2026-08-21 ### Added diff --git a/README.md b/README.md index 4bc0d54..8d9ca10 100644 --- a/README.md +++ b/README.md @@ -108,6 +108,9 @@ c1i grants list --app-id --entitlement-id # who holds an entitlement c1i grants list --user-id # what a C1 identity has, across apps c1i grants list --app-user-id # what an app account holds c1i grants list --app-id # every grant in an app + +# After a grant, wait for the set to stop changing rather than polling by hand +c1i grants list --app-id --entitlement-id --wait --wait-min 1 ``` Grants are the bindings between accounts/users and entitlements. At least one @@ -116,6 +119,31 @@ filter is required. Each NDJSON row includes the entitlement, the account `deprovision_at`), and `grant_source_count` — `0` for a direct grant, or the number of groups/roles the access is inherited through. +Grant provisioning is asynchronous, so a read taken moments after a grant or +revoke can catch the set mid-change. `--wait` re-reads every page every 5s and +prints nothing until the same grants come back `--wait-stable` times running +(default `3`, minimum `2` -- one read cannot show that anything held steady). +Progress goes to stderr, so stdout stays pure NDJSON. The 5s interval is fixed +and is not a flag; `--wait-stable` must fit inside `--wait-timeout` (default +`4m`), and a combination that cannot fit is rejected as a usage error rather +than left to time out. + +**An empty result is stable.** A filter matching nothing settles at the first +opportunity -- about 10s at the defaults -- and exits `0` with zero rows. +Waiting on a grant you just made, that reads as "it did not happen" when the +truth is "not yet". Pass `--wait-min 1` (or the count you expect) to hold out +for that many grants and time out instead; exit `1` then means "did not +converge in time", not "absent". The default of `0` is deliberate: +empty-and-stable is the correct answer when you are waiting for a revoke -- +and in that direction there is no flag that helps, since `--wait-min` is a +floor and cannot express a ceiling. `--wait` settles on whatever is steady, so +exit `0` still listing the row means "not yet, re-run", not "the revoke +failed". + +`--wait` settles on the whole matching set, fetching every page on every poll +regardless of `--limit`; `--limit` only truncates what is printed. Filter +narrowly. `--wait` and `--page-token` are mutually exclusive. + A grant outlives the entitlement or account it points at, so rows also carry `entitlement_deleted_at` and `app_user_deleted_at`: `jq 'select(.entitlement_deleted_at)'` finds grants whose backing object is gone. diff --git a/cmd/agents.md b/cmd/agents.md index a81e99c..122b448 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -227,7 +227,10 @@ Two things are irreversible in ways their `--help` doesn't make obvious: observed at 45-150s across set-owners, add-owner, remove-owner and the owner "apps create" assigns; grants: up to a couple of minutes). Verify owners with `c1i apps owners `, not `apps get`'s `appOwners` - field -- observed empty on every app checked in testing. `apps owners` also + field -- it was [] on all 46 apps in the test tenant on the second + measurement pass, including the 45 that GET .../ownerids reported owners + for, so an empty appOwners is not evidence an app has no owners. + `apps owners` also returns zero rows at exit 0 for a well-formed but nonexistent app id, so an empty result is either "no owners" or "wrong id"; `apps add-owner` on the same id exits 4. Don't write your own poll loop for this: `apps set-owners` @@ -235,6 +238,24 @@ Two things are irreversible in ways their `--help` doesn't make obvious: requested owner appears. A `--wait` timeout exits `1` and does not mean the write failed — provisioning may still be in flight, so re-check with `apps owners` instead of re-issuing the write. +- `grants list --wait` can report success with zero rows. An empty result is + stable, so a filter matching nothing settles in ~10s and exits `0` -- which + looks identical to "the grant did not happen" but usually means "not yet". + After a write, pass `--wait-min 1`; exit `1` then means "did not converge in + time", not "definitely absent". Without a minimum, treat `--wait` plus zero + rows as inconclusive, never as a negative answer. The reverse has no flag: + `--wait` settles on whatever is steady, and an undeprovisioned grant is + steady, so after a revoke exit `0` still listing the row means "not yet, + re-run", not "the revoke failed". +- `grants list --wait` buffers: nothing reaches stdout until the set settles, + unlike every other list command. Progress goes to stderr, so stdout stays + pure NDJSON, and a timeout exits `1` printing no rows. `--wait` fetches every + page on every poll regardless of `--limit`, which only truncates what is + printed, and it is mutually exclusive with `--page-token`. +- `--wait-stable` counts consecutive identical reads (default `3`, minimum + `2`). Two is not enough: a pause mid-change is indistinguishable from + completion. Three is a heuristic, not a proof -- size it past the longest + pause you have actually observed. The 5s poll interval is fixed, not a flag. - `accounts list --unmapped-only` filters after each page is fetched, not server-side. With `--page-token` (which turns off auto-pagination) a page can come back empty while unmapped accounts exist further along. diff --git a/cmd/apps_owners_wording_test.go b/cmd/apps_owners_wording_test.go index 15dacca..93102ae 100644 --- a/cmd/apps_owners_wording_test.go +++ b/cmd/apps_owners_wording_test.go @@ -7,8 +7,9 @@ import ( ) // appOwnersPopulateClaimTriggers are phrases that, paired with "appOwners" -// nearby, assert owners become visible there. Measured against the lab -// tenant: appOwners never populates (0 of 47 apps had a non-empty value), +// nearby, assert owners become visible there. Measured against the lab tenant +// twice: 0 of 47 apps on the first pass, 0 of 46 on the second (the first +// pass's scratch apps had been deleted by then). appOwners never populates, // while GET .../ownerids does converge (on the order of minutes). This // wording drifted, wrong, across five files before anything caught it. var appOwnersPopulateClaimTriggers = []string{"appear", "show up", "shows up", "populat"} diff --git a/cmd/apps_set_owners.go b/cmd/apps_set_owners.go index 5c6aaf1..234083c 100644 --- a/cmd/apps_set_owners.go +++ b/cmd/apps_set_owners.go @@ -18,7 +18,8 @@ import ( const ownerWaitPollInterval = 12 * time.Second // setOwnersSuccessFmt is the PUT-accepted confirmation. Points at "apps -// owners", not "apps get": appOwners was empty on every app checked. +// owners", not "apps get": appOwners was [] on all 46 apps measured, 45 of +// which did have owners. const setOwnersSuccessFmt = "Set %d owner(s) on app %s (provisioning is async; check with \"c1i apps owners %s\" in a minute or two).\n" var appsSetOwnersCmd = &cobra.Command{ @@ -30,8 +31,9 @@ any existing owners. Provide one or more --user-id (C1 user IDs, 27 chars each). Owner changes are provisioned ASYNCHRONOUSLY: this call returns immediately, but the new owners take a couple of minutes to show up in GET .../ownerids. A success here means the request was accepted, not that the owner list is -already live. The "appOwners" field in "apps get" was observed empty on -every app checked in testing -- don't use it to check ownership. +already live. Don't check ownership via the "appOwners" field in "apps get": +it was [] on all 46 apps in the test tenant on the second measurement pass, +including the 45 that GET .../ownerids reported owners for. Pass --wait to block and poll GET .../ownerids until every requested --user-id appears (or --wait-timeout elapses). Without --wait, behavior is @@ -52,10 +54,9 @@ polls).`, return &usageError{fmt.Errorf("--user-id values must be non-empty")} } } - wait, _ := cmd.Flags().GetBool("wait") - waitTimeout, _ := cmd.Flags().GetDuration("wait-timeout") - if wait && waitTimeout <= 0 { - return &usageError{fmt.Errorf("--wait-timeout must be positive")} + wait, waitTimeout, err := waitFlagValues(cmd) + if err != nil { + return err } baseURL, err := GetBaseURL() @@ -87,61 +88,33 @@ polls).`, }, } -// waitForOwners polls GET .../ownerids on appID every ownerWaitPollInterval -// until every id in wantUserIDs is present, timeout elapses, or cmd's context -// is canceled. It writes progress lines to cmd's stdout as it goes. +// waitForOwners blocks until every id in wantUserIDs appears in +// GET .../ownerids on appID, timeout elapses, or cmd's context is canceled. func waitForOwners(cmd *cobra.Command, c *client.Client, appID string, wantUserIDs []string, timeout time.Duration) error { - out := cmd.OutOrStdout() - ownerIDsPath := client.Path("/api/v1/apps/%s/ownerids", appID) + _, err := runWait(cmd, ownersWaitOp(c, appID, wantUserIDs, timeout)) + return err +} - ctx, cancel := context.WithTimeout(cmd.Context(), timeout) - defer cancel() - - start := time.Now() - ticker := time.NewTicker(ownerWaitPollInterval) - defer ticker.Stop() - - // firstPoll suppresses the "still waiting" line on the very first check: - // that poll happens immediately after the PUT, before any real waiting has - // elapsed, so printing it there would misleadingly imply time has already - // passed. Starting with the second poll, real time (>= one tick) has - // actually elapsed, so the message is accurate. - firstPoll := true - for { - got, err := fetchOwnerIDs(ctx, c, ownerIDsPath) - if err != nil { - if ctx.Err() != nil { - break // fall through to the timeout/cancellation report below +// ownersWaitOp is waitForOwners' operation, built separately so a test can +// drive the loop at a poll interval shorter than ownerWaitPollInterval. +func ownersWaitOp(c *client.Client, appID string, wantUserIDs []string, timeout time.Duration) waitOp[[]string] { + ownerIDsPath := client.Path("/api/v1/apps/%s/ownerids", appID) + return waitOp[[]string]{ + Poll: func(ctx context.Context) ([]string, error) { + got, err := fetchOwnerIDs(ctx, c, ownerIDsPath) + if err != nil { + return nil, fmt.Errorf("API error: %w", err) } - return fmt.Errorf("API error: %w", err) - } - if allOwnersPresent(wantUserIDs, got) { - _, _ = fmt.Fprintf(out, "Owners provisioned on app %s after %s.\n", - appID, time.Since(start).Round(time.Second)) - return nil - } - if !firstPoll { - _, _ = fmt.Fprintf(out, "Still waiting for owners to provision on app %s (%s elapsed)...\n", - appID, time.Since(start).Round(time.Second)) - } - firstPoll = false - - select { - case <-ctx.Done(): - case <-ticker.C: - continue - } - break - } - - if cmd.Context().Err() != nil { - return fmt.Errorf("canceled while waiting for owners to provision on app %s", appID) + return got, nil + }, + Done: untilPresent(wantUserIDs), + Interval: ownerWaitPollInterval, + Timeout: timeout, + Subject: fmt.Sprintf("owners to provision on app %s", appID), + Success: fmt.Sprintf("Owners provisioned on app %s", appID), + Slow: "provisioning can take several minutes", + Recheck: fmt.Sprintf("c1i apps owners %s", appID), } - return fmt.Errorf( - "timed out after %s waiting for owners to provision on app %s; "+ - "this is not necessarily a failure — provisioning can take several minutes, "+ - "check again later with: c1i apps owners %s", - timeout, appID, appID) } // fetchOwnerIDs GETs .../ownerids and returns the current owner user IDs. @@ -159,21 +132,10 @@ func fetchOwnerIDs(ctx context.Context, c *client.Client, path string) ([]string return parsed.UserIDs, nil } -// allOwnersPresent reports whether every id in want is present in got. Pure -// (no I/O) so the poll's success condition is unit-testable without a fake -// server: --wait's loop calls this after each GET .../ownerids. -func allOwnersPresent(want, got []string) bool { - gotSet := make(map[string]struct{}, len(got)) - for _, id := range got { - gotSet[id] = struct{}{} - } - for _, id := range want { - if _, ok := gotSet[id]; !ok { - return false - } - } - return true -} +// allOwnersPresent reports whether every id in want is present in got: the +// set-owners --wait success condition (untilPresent's) as a pure function, so +// it stays unit-testable without a fake server. +func allOwnersPresent(want, got []string) bool { return untilPresent(want)(got) } // buildSetOwnersBody assembles the PUT .../owners request body. Pure, so the // dry-run preview and a unit test pin the exact wire shape (userIds, not @@ -185,7 +147,6 @@ func buildSetOwnersBody(userIDs []string) map[string]any { func init() { appsSetOwnersCmd.Flags().StringSlice("user-id", nil, "C1 user ID to set as owner (repeatable; replaces the full owner list)") markRequired(appsSetOwnersCmd, "user-id") - appsSetOwnersCmd.Flags().Bool("wait", false, "block and poll GET .../ownerids until the requested owners appear, or --wait-timeout elapses") - appsSetOwnersCmd.Flags().Duration("wait-timeout", 4*time.Minute, "max time to wait with --wait (e.g. 30s, 5m)") + addWaitFlags(appsSetOwnersCmd, "GET .../ownerids until the requested owners appear", 4*time.Minute) appsCmd.AddCommand(appsSetOwnersCmd) } diff --git a/cmd/docs_guide.go b/cmd/docs_guide.go index e01b5c3..f52171f 100644 --- a/cmd/docs_guide.go +++ b/cmd/docs_guide.go @@ -579,14 +579,35 @@ you'd take it back. ## Verify - c1i grants list --app-id "$APP_ID" --entitlement-id "$ENTITLEMENT_ID" --user-id "$USER_ID" +After an approved GRANT, wait for the row to appear rather than polling by +hand: + + c1i grants list --app-id "$APP_ID" --entitlement-id "$ENTITLEMENT_ID" --user-id "$USER_ID" --wait --wait-min 1 + +After an approved REVOKE, re-read until the set is steady, then check it: + + c1i grants list --app-id "$APP_ID" --entitlement-id "$ENTITLEMENT_ID" --user-id "$USER_ID" --wait Grants are eventually consistent in both directions: after an approved grant, expect up to a couple of minutes before the row appears; after an -approved revoke, the same delay before it disappears. An empty result -immediately after approval isn't a failure — wait and re-run the same -command. A revoke task still sitting in TASK_STATE_OPEN past that window -means it's waiting on an approver, not that the revoke failed. +approved revoke, the same delay before it disappears. + +"--wait-min 1" is what makes the grant case trustworthy. An empty result is +stable, so a bare "--wait" settles on zero rows in about 10s and exits 0 -- +indistinguishable from "the grant did not happen" when the truth is "not +yet". With "--wait-min 1", exit 0 means the grant is really there and exit 1 +means it had not arrived within "--wait-timeout" (default 4m), which is a +timeout, not a denial. + +The revoke direction has no such flag, and "--wait" does NOT wait for empty: +it settles on whatever is steady, and a grant that has not been deprovisioned +yet is perfectly steady. So exit 0 still listing the row means "not yet, re-run +in a minute" -- NOT "the revoke failed". Only zero rows is confirmation. +"--wait-min" is a floor, not a ceiling, so leave it off here; it cannot express +"wait until this is gone". + +A revoke task still sitting in TASK_STATE_OPEN past that window means it's +waiting on an approver, not that the revoke failed. ## Common failures @@ -596,7 +617,7 @@ means it's waiting on an approver, not that the revoke failed. | 409 duplicate ticket found, with a task id in the error details (exit 1) | An open task for this exact app + entitlement + user already exists | Act on that task id ("tasks list --state open") instead of creating another | | required flag(s) "app-id", "entitlement-id" not set (exit 2) | Both are required on "requests create grant"/"revoke" | Fix the invocation | | An auth failure resolving the caller's own id when "--user-id" is omitted (exit 3) | Surfaces before the request call itself runs | Re-authenticate rather than retry as-is | -| "grants list" returns nothing right after approval | Eventual consistency | Wait roughly a minute or two and re-run the same filter | +| "grants list" returns nothing right after approval | Eventual consistency | Re-run with "--wait --wait-min 1" so it blocks until the grant lands; a bare "--wait" settles on the empty set in ~10s and exits 0, which reads as a denial | Approve/deny/comment failures, and everything about stepApproverIds, the actions gate, and the current policy step, live in diff --git a/cmd/grants_list.go b/cmd/grants_list.go index 0a65a5d..5f21ef6 100644 --- a/cmd/grants_list.go +++ b/cmd/grants_list.go @@ -1,9 +1,15 @@ package cmd import ( + "context" "encoding/json" "fmt" + "sort" + "strconv" + "strings" + "time" + "github.com/ConductorOne/c1i/internal/client" "github.com/spf13/cobra" ) @@ -87,7 +93,31 @@ var grantsListCmd = &cobra.Command{ c1i grants list --user-id USER # Every grant in an app - c1i grants list --app-id APP`, + c1i grants list --app-id APP + +Grant provisioning is asynchronous, so a read taken right after a grant or +revoke can catch the set mid-change. Pass --wait to poll instead: every 5s it +re-reads every page of the match, and once the same grants come back +--wait-stable times in a row it prints that settled set. Nothing reaches stdout +until it settles, unlike the default streaming output; progress goes to stderr, +so stdout stays pure NDJSON. + +AN EMPTY RESULT IS STABLE. A filter matching nothing settles at the first +opportunity -- roughly 10s at the defaults -- and exits 0 with zero rows. If +you are waiting for a grant you just made, that reads as "it did not happen" +when the truth is "not yet": provisioning runs about a minute. Pass +--wait-min 1 (or the count you expect) to make the wait hold out for that many +grants and time out instead of settling empty. The default of 0 is deliberate: +empty-and-stable is the correct answer when you are waiting for a revoke. + +--wait settles on the WHOLE matching set, so it fetches every page on every +poll regardless of --limit; --limit only truncates what is printed at the end. +Filter narrowly. The poll interval is fixed at 5s -- --wait-stable, --wait-min +and --wait-timeout are the tunable parts. + +--wait-stable defaults to 3 rather than 2 because two equal reads cannot be +told apart from a pause mid-change. Even 3 is a heuristic, not a proof: on a +set that keeps changing, --wait times out rather than printing anything.`, RunE: func(cmd *cobra.Command, args []string) error { appID, _ := cmd.Flags().GetString("app-id") userID, _ := cmd.Flags().GetString("user-id") @@ -101,6 +131,35 @@ var grantsListCmd = &cobra.Command{ return &usageError{fmt.Errorf("--entitlement-id requires --app-id (entitlements are scoped to an app)")} } + wait, waitTimeout, err := waitFlagValues(cmd) + if err != nil { + return err + } + stableReads := getIntFlag(cmd, "wait-stable") + minGrants := getIntFlag(cmd, "wait-min") + if wait { + if minGrants < 0 { + return &usageError{fmt.Errorf("--wait-min cannot be negative")} + } + if cmd.Flags().Changed("page-token") { + // --page-token pins one page of a cursor the server is free to + // re-issue as the set changes; "the same page twice" would not + // mean the set settled. + return &usageError{fmt.Errorf("--wait cannot be combined with --page-token; --wait re-reads every page")} + } + if stableReads < 2 { + return &usageError{fmt.Errorf("--wait-stable must be at least 2 (one read cannot show that anything held steady)")} + } + // The first read is immediate, so n reads need (n-1) intervals. + // Past that the wait cannot succeed, and the timeout would blame + // slow provisioning for what is really bad arithmetic. + if need := time.Duration(stableReads-1) * grantsWaitPollInterval; need >= waitTimeout { + return &usageError{fmt.Errorf( + "--wait-stable=%d needs %s at the fixed %s poll interval, which --wait-timeout=%s can never allow; raise --wait-timeout or lower --wait-stable", + stableReads, need, grantsWaitPollInterval, waitTimeout)} + } + } + baseURL, err := GetBaseURL() if err != nil { return err @@ -116,30 +175,30 @@ var grantsListCmd = &cobra.Command{ manualPaging := cmd.Flags().Changed("page-token") limit := getIntFlag(cmd, "limit") + q := grantsQuery{appID: appID, userID: userID, appUserID: appUserID, entitlementID: entitlementID} + + if wait { + settled, err := waitForGrants(cmd, c, q, requestedPageSize, stableReads, minGrants, waitTimeout) + if err != nil { + return err + } + enc := newEmitter(cmd) + for _, item := range settled { + _ = enc.Encode(grantRow(item)) + if limitReached(enc.Written(), limit) { + break + } + } + return nil + } + enc := newEmitter(cmd) for !limitReached(enc.Written(), limit) { pageSize := requestedPageSize if !enc.Filtered() { pageSize = effectivePageSize(requestedPageSize, limit, enc.Written()) } - body := map[string]any{ - "pageSize": pageSize, - } - if pageToken != "" { - body["pageToken"] = pageToken - } - if appID != "" { - body["appIds"] = []string{appID} - } - if userID != "" { - body["userId"] = userID - } - if appUserID != "" { - body["appUserIds"] = []string{appUserID} - } - if entitlementID != "" { - body["entitlementRefs"] = []map[string]string{{"appId": appID, "id": entitlementID}} - } + body := q.searchBody(pageSize, pageToken) data, err := c.Post(cmd.Context(), "/api/v1/search/grants", body) if err != nil { @@ -171,11 +230,160 @@ var grantsListCmd = &cobra.Command{ }, } +// grantsQuery is the filter set shared by the streaming and --wait paths, so +// both send byte-identical search bodies. +type grantsQuery struct { + appID string + userID string + appUserID string + entitlementID string +} + +func (q grantsQuery) searchBody(pageSize int, pageToken string) map[string]any { + body := map[string]any{"pageSize": pageSize} + if pageToken != "" { + body["pageToken"] = pageToken + } + if q.appID != "" { + body["appIds"] = []string{q.appID} + } + if q.userID != "" { + body["userId"] = q.userID + } + if q.appUserID != "" { + body["appUserIds"] = []string{q.appUserID} + } + if q.entitlementID != "" { + body["entitlementRefs"] = []map[string]string{{"appId": q.appID, "id": q.entitlementID}} + } + return body +} + +// grantsSnapshot is one full read of the matching grants. fingerprint is the +// comparable part runWait's Done predicate settles on; items is what gets +// printed once it does. +type grantsSnapshot struct { + fingerprint string + items []grantListItem +} + +// grantsWaitPollInterval is how often --wait re-reads. Each poll walks every +// page, so this is deliberately not tight. A var, not a const, so a test can +// drive the loop faster than real time. +var grantsWaitPollInterval = 5 * time.Second + +// grantSetFingerprint identifies the set of grants independently of the page +// order the server happens to return them in: a grant is (entitlement, +// account), and a re-ordered page is not a change. +func grantSetFingerprint(items []grantListItem) string { + keys := make([]string, 0, len(items)) + for _, it := range items { + keys = append(keys, it.Entitlement.AppEntitlement.ID+"\x00"+it.AppEntitlementUserBinding.AppUser.AppUser.ID) + } + sort.Strings(keys) + return strconv.Itoa(len(keys)) + "\x01" + strings.Join(keys, "\x02") +} + +// fetchAllGrants pages POST /api/v1/search/grants to completion. --wait +// fingerprints the whole set, so a first-page-only read would call a set +// settled while later pages were still moving. +func fetchAllGrants(ctx context.Context, c *client.Client, q grantsQuery, pageSize int) ([]grantListItem, error) { + var all []grantListItem + pageToken, prevToken := "", "" + for { + data, err := c.Post(ctx, "/api/v1/search/grants", q.searchBody(pageSize, pageToken)) + if err != nil { + return nil, fmt.Errorf("API error: %w", err) + } + var resp struct { + List []grantListItem `json:"list"` + NextPageToken string `json:"nextPageToken"` + } + if err := json.Unmarshal(data, &resp); err != nil { + return nil, fmt.Errorf("failed to parse response: %w", err) + } + all = append(all, resp.List...) + if resp.NextPageToken == "" { + return all, nil + } + // Same guard as "api --paginate" (cmd/api.go): a server that re-issues + // one token forever would otherwise turn each poll into a request storm + // bounded only by --wait-timeout, then blame provisioning for it. + if resp.NextPageToken == prevToken { + return nil, fmt.Errorf("API returned the same nextPageToken twice in a row while --wait was re-reading grants; the cursor is not advancing") + } + prevToken = resp.NextPageToken + pageToken = resp.NextPageToken + } +} + +// waitForGrants blocks until the matching grant set comes back identical +// stableReads times running, and returns that settled set. +func waitForGrants(cmd *cobra.Command, c *client.Client, q grantsQuery, pageSize, stableReads, minGrants int, timeout time.Duration) ([]grantListItem, error) { + done := stableAndAtLeast( + stableReads, + func(s grantsSnapshot) string { return s.fingerprint }, + func(s grantsSnapshot) bool { return len(s.items) >= minGrants }, + ) + settled, err := runWait(cmd, waitOp[grantsSnapshot]{ + Poll: func(ctx context.Context) (grantsSnapshot, error) { + items, err := fetchAllGrants(ctx, c, q, pageSize) + if err != nil { + return grantsSnapshot{}, err + } + return grantsSnapshot{fingerprint: grantSetFingerprint(items), items: items}, nil + }, + Done: done, + Interval: grantsWaitPollInterval, + Timeout: timeout, + Subject: grantsWaitSubject(minGrants), + Success: "Grants settled", + Slow: "grant provisioning can take several minutes", + Recheck: "c1i grants list " + strings.Join(q.rescanFlags(), " "), + // stdout is this command's NDJSON stream; progress prose there would + // land mid-stream and break a caller's jq. + Out: cmd.ErrOrStderr(), + }) + if err != nil { + return nil, err + } + return settled.items, nil +} + +// grantsWaitSubject names what the wait is for, so the progress and timeout +// lines say which of the two conditions is outstanding. +func grantsWaitSubject(minGrants int) string { + if minGrants > 0 { + return fmt.Sprintf("at least %d matching grant(s) to appear and stop changing", minGrants) + } + return "the matching grants to stop changing" +} + +// rescanFlags reproduces the filters this query was built from, so the +// timeout message hands back a command that reruns the same search. +func (q grantsQuery) rescanFlags() []string { + var out []string + for _, f := range []struct{ name, value string }{ + {"--app-id", q.appID}, + {"--user-id", q.userID}, + {"--app-user-id", q.appUserID}, + {"--entitlement-id", q.entitlementID}, + } { + if f.value != "" { + out = append(out, f.name+"="+f.value) + } + } + return out +} + func init() { grantsListCmd.Flags().String("app-id", "", "Filter to grants in this application") grantsListCmd.Flags().String("user-id", "", "Filter to grants held by this C1 identity user") grantsListCmd.Flags().String("app-user-id", "", "Filter to grants held by this app account (app user)") grantsListCmd.Flags().String("entitlement-id", "", "Filter to grants of this entitlement (requires --app-id)") addPaginationFlags(grantsListCmd) + addWaitFlags(grantsListCmd, "every page until the same grants come back --wait-stable times running", 4*time.Minute) + grantsListCmd.Flags().Int("wait-stable", 3, "Consecutive identical reads --wait requires before printing (min 2)") + grantsListCmd.Flags().Int("wait-min", 0, "Minimum matching grants --wait requires before it can settle (0 = an empty result may settle)") grantsCmd.AddCommand(grantsListCmd) } diff --git a/cmd/grants_wait_test.go b/cmd/grants_wait_test.go new file mode 100644 index 0000000..0274930 --- /dev/null +++ b/cmd/grants_wait_test.go @@ -0,0 +1,521 @@ +package cmd + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/ConductorOne/c1i/internal/client" + "github.com/spf13/cobra" +) + +// TestGrantSetFingerprint pins what "the grant set changed" means: identity is +// (entitlement, account), page order is not a change, and a differing count +// always is. +func TestGrantSetFingerprint(t *testing.T) { + mk := func(pairs ...[2]string) []grantListItem { + items := make([]grantListItem, 0, len(pairs)) + for _, p := range pairs { + var it grantListItem + it.Entitlement.AppEntitlement.ID = p[0] + it.AppEntitlementUserBinding.AppUser.AppUser.ID = p[1] + items = append(items, it) + } + return items + } + a := mk([2]string{"e1", "u1"}, [2]string{"e2", "u2"}) + reordered := mk([2]string{"e2", "u2"}, [2]string{"e1", "u1"}) + added := mk([2]string{"e1", "u1"}, [2]string{"e2", "u2"}, [2]string{"e3", "u3"}) + swapped := mk([2]string{"e1", "u2"}, [2]string{"e2", "u1"}) + + if grantSetFingerprint(a) != grantSetFingerprint(reordered) { + t.Error("page order changed the fingerprint; a re-ordered page is not a change") + } + if grantSetFingerprint(a) == grantSetFingerprint(added) { + t.Error("an added grant did not change the fingerprint") + } + if grantSetFingerprint(a) == grantSetFingerprint(swapped) { + t.Error("re-pairing the same ids did not change the fingerprint") + } + if grantSetFingerprint(nil) == grantSetFingerprint(mk([2]string{"e1", "u1"})) { + t.Error("an empty set and a one-grant set share a fingerprint") + } +} + +// TestGrantsQuerySearchBody pins that both the streaming and --wait paths +// build the same request body -- the reason the builder is shared. +func TestGrantsQuerySearchBody(t *testing.T) { + q := grantsQuery{appID: "app1", userID: "u1", appUserID: "au1", entitlementID: "e1"} + b, err := json.Marshal(q.searchBody(25, "tok")) + if err != nil { + t.Fatal(err) + } + want := `{"appIds":["app1"],"appUserIds":["au1"],"entitlementRefs":[{"appId":"app1","id":"e1"}],"pageSize":25,"pageToken":"tok","userId":"u1"}` + if string(b) != want { + t.Errorf("body = %s\nwant %s", b, want) + } + + b, err = json.Marshal(grantsQuery{appID: "app1"}.searchBody(50, "")) + if err != nil { + t.Fatal(err) + } + if want := `{"appIds":["app1"],"pageSize":50}`; string(b) != want { + t.Errorf("empty filters leaked into the body: %s, want %s", b, want) + } +} + +// grantsFake is the fake grants-search server plus what it observed. Recording +// the page sizes it was sent is what ties the --page-size flag to the request +// the wait actually issues; without it, pinning that argument at the call site +// is invisible to every test. +type grantsFake struct { + srv *httptest.Server + fullReads int32 + + mu sync.Mutex + pageSizes []int +} + +func (f *grantsFake) reads() int { return int(atomic.LoadInt32(&f.fullReads)) } + +func (f *grantsFake) observedPageSizes() []int { + f.mu.Lock() + defer f.mu.Unlock() + return append([]int(nil), f.pageSizes...) +} + +// grantsWaitServer serves POST /api/v1/search/grants. pages[i] is the list of +// (entitlement,account) id pairs returned by the i-th *full read*, split into +// two pages so each poll exercises pagination; the last entry repeats. +func grantsWaitServer(t *testing.T, reads [][][2]string) *grantsFake { + t.Helper() + f := &grantsFake{} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v1/search/grants" { + t.Errorf("path = %q", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + return + } + var body map[string]any + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Errorf("decoding body: %v", err) + } + tok, _ := body["pageToken"].(string) + if ps, ok := body["pageSize"].(float64); ok { + f.mu.Lock() + f.pageSizes = append(f.pageSizes, int(ps)) + f.mu.Unlock() + } + + idx := int(atomic.LoadInt32(&f.fullReads)) + if idx >= len(reads) { + idx = len(reads) - 1 + } + pairs := reads[idx] + + // First page carries the first item, second page the rest, so a + // first-page-only implementation would fingerprint a partial set. + var page [][2]string + next := "" + if tok == "" { + if len(pairs) > 0 { + page, next = pairs[:1], "p2" + } + } else { + page = pairs[1:] + } + if next == "" { + atomic.AddInt32(&f.fullReads, 1) + } + + items := make([]string, 0, len(page)) + for _, p := range page { + items = append(items, fmt.Sprintf( + `{"appEntitlementUserBinding":{"appUser":{"appUser":{"id":%q,"appId":"app1"}}},"entitlement":{"appEntitlement":{"id":%q,"appId":"app1"}}}`, + p[1], p[0])) + } + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprintf(w, `{"list":[%s],"nextPageToken":%q}`, strings.Join(items, ","), next) + })) + t.Cleanup(srv.Close) + f.srv = srv + return f +} + +func runGrantsWait(t *testing.T, srv *httptest.Server, stableReads, minGrants int, timeout time.Duration) ([]grantListItem, string, string, error) { + t.Helper() + c := client.NewForTesting(srv.URL, srv.Client()) + cmd := &cobra.Command{Use: "list"} + var out, errOut bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&errOut) + cmd.SetContext(context.Background()) + + origInterval := grantsWaitPollInterval + grantsWaitPollInterval = 5 * time.Millisecond + t.Cleanup(func() { grantsWaitPollInterval = origInterval }) + + items, err := waitForGrants(cmd, c, grantsQuery{appID: "app1"}, 50, stableReads, minGrants, timeout) + return items, out.String(), errOut.String(), err +} + +// TestWaitForGrantsSettles drives the wired --wait path: it must keep polling +// through a change, paginate each poll to completion, and return the set that +// held steady -- not the first one it saw. +func TestWaitForGrantsSettles(t *testing.T) { + fake := grantsWaitServer(t, [][][2]string{ + {{"e1", "u1"}}, + {{"e1", "u1"}, {"e2", "u2"}}, + {{"e1", "u1"}, {"e2", "u2"}}, + {{"e1", "u1"}, {"e2", "u2"}}, + }) + items, out, errOut, err := runGrantsWait(t, fake.srv, 3, 0, 10*time.Second) + if err != nil { + t.Fatalf("waitForGrants returned %v, want nil", err) + } + if len(items) != 2 { + t.Errorf("settled on %d grants, want 2 (both pages of the settled read)", len(items)) + } + if got := fake.reads(); got != 4 { + t.Errorf("made %d full reads, want 4 (one changing + three identical)", got) + } + if out != "" { + t.Errorf("progress went to stdout, which is the NDJSON stream:\n%s", out) + } + if !strings.Contains(errOut, "Grants settled after ") { + t.Errorf("stderr missing the success line:\n%s", errOut) + } +} + +// TestWaitForGrantsTimesOut covers the timeout path over HTTP and pins that +// the recheck command reproduces the filters the search was run with. +func TestWaitForGrantsTimesOut(t *testing.T) { + // Every read differs, so the set never settles. + var reads [][][2]string + for i := range 50 { + reads = append(reads, [][2]string{{"e1", "u1"}, {fmt.Sprintf("e%d", i+2), "u2"}}) + } + fake := grantsWaitServer(t, reads) + items, out, _, err := runGrantsWait(t, fake.srv, 3, 0, 40*time.Millisecond) + if err == nil { + t.Fatal("waitForGrants returned nil, want a timeout error") + } + if items != nil { + t.Errorf("returned %d grants on timeout, want none printed", len(items)) + } + if out != "" { + t.Errorf("timed-out wait wrote to stdout:\n%s", out) + } + want := "check again later with: c1i grants list --app-id=app1" + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not contain %q", err.Error(), want) + } +} + +// TestGrantsListWaitEndToEnd drives the user-visible path -- grantsListCmd.RunE +// with --wait -- rather than waitForGrants directly, so every flag is tied to +// the outcome it is supposed to produce. Calling the helper directly leaves the +// plumbing untested: replacing an argument at the call site with a literal, or +// transposing two adjacent ints, both compile and change behavior. +func TestGrantsListWaitEndToEnd(t *testing.T) { + threeGrants := [][2]string{{"e1", "u1"}, {"e2", "u2"}, {"e3", "u3"}} + oneGrant := [][2]string{{"e1", "u1"}} + + for _, tc := range []struct { + name string + reads [][][2]string + flags map[string]string + // parentTimeout bounds the command's own context. A case that expects + // a --wait-timeout to fire sets this well above it, so pinning + // waitTimeout to its 4m default fails promptly with a cancellation + // instead of hanging the suite. + parentTimeout time.Duration + wantErr string + wantRows int + wantReads int + wantPageSize int + }{ + { + name: "settles and prints the whole set", + reads: [][][2]string{oneGrant, threeGrants, threeGrants}, + flags: map[string]string{"wait-stable": "2"}, + wantRows: 3, + wantReads: 3, + wantPageSize: 50, + }, + { + name: "limit truncates what is printed, not what it settles on", + reads: [][][2]string{oneGrant, threeGrants, threeGrants}, + flags: map[string]string{"wait-stable": "2", "limit": "2"}, + wantRows: 2, + wantReads: 3, + wantPageSize: 50, + }, + { + // Three empty reads, so --wait-min=0 would settle on them at the + // third; the correct wait must outlast them and take the grant. + // Transposing --wait-stable and --wait-min turns this into a floor + // of 3 that one grant can never clear, so it times out instead. + name: "wait-min outlasts an empty prefix", + reads: [][][2]string{nil, nil, nil, oneGrant, oneGrant, oneGrant}, + flags: map[string]string{"wait-stable": "3", "wait-min": "1"}, + // Bounded for the same reason as the case below: under the + // transposition mutation this becomes a floor of 3 that one grant + // never clears, and the 4m default would grind out ~48k requests + // at the test poll interval before failing. + parentTimeout: 5 * time.Second, + wantRows: 1, + wantReads: 6, + wantPageSize: 50, + }, + { + name: "wait-min times out rather than settling empty", + reads: [][][2]string{nil}, + flags: map[string]string{"wait-stable": "2", "wait-min": "1", "wait-timeout": "60ms"}, + parentTimeout: 3 * time.Second, + wantErr: "timed out after 60ms waiting for at least 1 matching grant(s) to appear and stop changing; this is not necessarily a failure \u2014 grant provisioning can take several minutes, check again later with: c1i grants list --app-id=app1", + }, + { + name: "page-size reaches the request the wait issues", + reads: [][][2]string{threeGrants, threeGrants}, + flags: map[string]string{"wait-stable": "2", "page-size": "37"}, + wantRows: 3, + wantReads: 2, + wantPageSize: 37, + }, + } { + t.Run(tc.name, func(t *testing.T) { + fake := grantsWaitServer(t, tc.reads) + orig := newListClient + newListClient = func(*cobra.Command, string) (*client.Client, error) { + return client.NewForTesting(fake.srv.URL, fake.srv.Client()), nil + } + t.Cleanup(func() { newListClient = orig }) + t.Setenv("C1I_URL", "https://example.invalid") + + origInterval := grantsWaitPollInterval + grantsWaitPollInterval = 5 * time.Millisecond + t.Cleanup(func() { grantsWaitPollInterval = origInterval }) + + resetCmdFlags(t, grantsListCmd) + mustSet(t, grantsListCmd.Flags(), "app-id", "app1") + mustSet(t, grantsListCmd.Flags(), "wait", "true") + for name, val := range tc.flags { + mustSet(t, grantsListCmd.Flags(), name, val) + } + + ctx := context.Background() + if tc.parentTimeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, tc.parentTimeout) + defer cancel() + } + + var out, errOut bytes.Buffer + grantsListCmd.SetOut(&out) + grantsListCmd.SetErr(&errOut) + grantsListCmd.SetContext(ctx) + err := grantsListCmd.RunE(grantsListCmd, nil) + + if tc.wantErr != "" { + if err == nil { + t.Fatalf("RunE returned nil, want %q", tc.wantErr) + } + if err.Error() != tc.wantErr { + t.Fatalf("RunE error =\n%q\nwant\n%q", err.Error(), tc.wantErr) + } + if out.String() != "" { + t.Errorf("wrote rows to stdout despite not settling:\n%s", out.String()) + } + return + } + if err != nil { + t.Fatalf("RunE returned %v, want nil", err) + } + + lines := strings.Split(strings.TrimRight(out.String(), "\n"), "\n") + if len(lines) != tc.wantRows { + t.Fatalf("printed %d rows, want %d:\n%s", len(lines), tc.wantRows, out.String()) + } + for i, line := range lines { + var row map[string]any + if err := json.Unmarshal([]byte(line), &row); err != nil { + t.Fatalf("stdout line %d is not JSON (%v): %q", i, err, line) + } + if row["entitlement_id"] == "" || row["app_user_id"] == "" { + t.Errorf("row %d is missing its ids: %v", i, row) + } + } + if got := fake.reads(); got != tc.wantReads { + t.Errorf("made %d full reads, want %d", got, tc.wantReads) + } + observed := fake.observedPageSizes() + if len(observed) == 0 { + t.Fatal("no request carried a pageSize; the assertion below would pass vacuously") + } + for i, ps := range observed { + if ps != tc.wantPageSize { + t.Errorf("request %d asked for pageSize %d, want %d", i, ps, tc.wantPageSize) + } + } + if !strings.Contains(errOut.String(), "Grants settled after ") { + t.Errorf("stderr missing the success line:\n%s", errOut.String()) + } + }) + } +} + +// TestFetchAllGrantsRejectsAStuckCursor pins the same-token guard: without it a +// server re-issuing one cursor turns every poll into a request storm bounded +// only by --wait-timeout. +func TestFetchAllGrantsRejectsAStuckCursor(t *testing.T) { + var requests int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&requests, 1) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"list":[],"nextPageToken":"stuck"}`)) + })) + t.Cleanup(srv.Close) + + // Bounded, so removing the guard fails this test promptly instead of + // hanging it until go test's own panic timeout. + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + c := client.NewForTesting(srv.URL, srv.Client()) + _, err := fetchAllGrants(ctx, c, grantsQuery{appID: "app1"}, 50) + if err == nil { + t.Fatal("fetchAllGrants returned nil; a stuck cursor must be an error, not a loop") + } + if !strings.Contains(err.Error(), "same nextPageToken twice in a row") { + t.Errorf("err = %q, want it to name the stuck cursor", err.Error()) + } + if got := atomic.LoadInt32(&requests); got != 2 { + t.Errorf("made %d requests before giving up, want 2", got) + } +} + +// TestGrantsListWaitUsageErrors pins the combinations --wait rejects, all as +// exit-2 usage errors. +func TestGrantsListWaitUsageErrors(t *testing.T) { + cases := []struct { + name string + flags map[string]string + want string + }{ + {"page-token", map[string]string{"app-id": "app1", "wait": "true", "page-token": "tok"}, "--wait cannot be combined with --page-token"}, + {"wait-stable below 2", map[string]string{"app-id": "app1", "wait": "true", "wait-stable": "1"}, "--wait-stable must be at least 2"}, + {"wait-stable can never fit in wait-timeout", map[string]string{"app-id": "app1", "wait": "true", "wait-stable": "4", "wait-timeout": "10s"}, "can never allow"}, + {"negative wait-min", map[string]string{"app-id": "app1", "wait": "true", "wait-min": "-1"}, "--wait-min cannot be negative"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + resetCmdFlags(t, grantsListCmd) + for k, v := range tc.flags { + mustSet(t, grantsListCmd.Flags(), k, v) + } + grantsListCmd.SetContext(context.Background()) + err := grantsListCmd.RunE(grantsListCmd, nil) + var ue *usageError + if !errors.As(err, &ue) { + t.Fatalf("err = %v, want a *usageError (exit 2)", err) + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("err = %q, want it to contain %q", err.Error(), tc.want) + } + }) + } +} + +// TestWaitForGrantsSettlesEmptyWithoutAMinimum is the reproduction, kept as a +// test so the default's behavior is a choice on record and not an accident: an +// empty result IS stable, so with no floor the wait settles fast and exits 0 +// with zero rows. That is correct for a revoke and wrong for a grant, which is +// what --wait-min exists to say. +func TestWaitForGrantsSettlesEmptyWithoutAMinimum(t *testing.T) { + fake := grantsWaitServer(t, [][][2]string{nil}) + items, _, errOut, err := runGrantsWait(t, fake.srv, 3, 0, 10*time.Second) + if err != nil { + t.Fatalf("wait returned %v, want nil (empty-and-stable is a settle)", err) + } + if len(items) != 0 { + t.Errorf("settled on %d grants, want 0", len(items)) + } + if got := fake.reads(); got != 3 { + t.Errorf("made %d full reads, want 3 (it settles as soon as the streak fills)", got) + } + if !strings.Contains(errOut, "Grants settled after ") { + t.Errorf("stderr missing the success line:\n%s", errOut) + } +} + +// TestWaitForGrantsMinimumOutwaitsAnEmptySet is the fix: with --wait-min the +// wait must NOT settle on the empty reads that precede the grant landing, and +// must return the grant once it appears. +func TestWaitForGrantsMinimumOutwaitsAnEmptySet(t *testing.T) { + // Empty for the first four reads, then the grant lands and holds. + reads := [][][2]string{nil, nil, nil, nil, + {{"e1", "u1"}}, {{"e1", "u1"}}, {{"e1", "u1"}}} + fake := grantsWaitServer(t, reads) + items, _, _, err := runGrantsWait(t, fake.srv, 3, 1, 10*time.Second) + if err != nil { + t.Fatalf("wait returned %v, want nil", err) + } + if len(items) != 1 { + t.Fatalf("settled on %d grants, want 1; an empty prefix was accepted", len(items)) + } +} + +// TestWaitForGrantsMinimumTimesOutRatherThanSettlingEmpty pins the other half: +// if the grant never lands, --wait-min must produce a timeout, not a confident +// empty success. +func TestWaitForGrantsMinimumTimesOutRatherThanSettlingEmpty(t *testing.T) { + fake := grantsWaitServer(t, [][][2]string{nil}) + items, out, _, err := runGrantsWait(t, fake.srv, 3, 1, 40*time.Millisecond) + if err == nil { + t.Fatal("wait returned nil; an unmet --wait-min must time out, not settle empty") + } + if items != nil { + t.Errorf("returned %d grants, want none", len(items)) + } + if out != "" { + t.Errorf("wrote to stdout despite not settling:\n%s", out) + } + if !strings.Contains(err.Error(), "at least 1 matching grant(s) to appear and stop changing") { + t.Errorf("timeout error %q does not say the minimum was the unmet condition", err.Error()) + } +} + +// TestStableAndAtLeastFeedsStabilityOnEveryPoll pins the ordering rule the +// combinator exists for. The set dips below the floor and comes back to the +// SAME value; short-circuiting past the stateful predicate on the low poll +// would hide that dip and settle on a stale streak. +func TestStableAndAtLeastFeedsStabilityOnEveryPoll(t *testing.T) { + done := stableAndAtLeast( + 3, + func(n int) int { return n }, + func(n int) bool { return n >= 1 }, + ) + // 5,5 builds a streak of 2; the 0 must reset it; the run after must build + // a fresh streak of 3, so the first true is the final read. + seq := []int{5, 5, 0, 5, 5, 5} + firstTrue := -1 + for i, v := range seq { + if done(v) { + firstTrue = i + break + } + } + if firstTrue != 5 { + t.Errorf("first satisfied at index %d over %v, want 5; the dip below the floor was not fed to the stability predicate", firstTrue, seq) + } +} diff --git a/cmd/wait.go b/cmd/wait.go new file mode 100644 index 0000000..fdd4b67 --- /dev/null +++ b/cmd/wait.go @@ -0,0 +1,211 @@ +package cmd + +import ( + "context" + "fmt" + "io" + "time" + + "github.com/spf13/cobra" +) + +// waitOp is one "poll until a condition holds" operation. Every --wait in this +// CLI is built from it, so the loop's shape -- immediate first poll, progress +// lines, and a timeout that says so without claiming failure -- exists once. +// +// The four string fields are what the loop cannot infer: it prints them, it +// does not compose English. +type waitOp[T any] struct { + // Poll reads the current state. It must wrap its errors with %w so + // cmd/errors.go can still classify them. + Poll func(context.Context) (T, error) + // Done reports whether the polled state satisfies the wait. It may be + // stateful across calls (see untilStable), so runWait calls it exactly + // once per poll. + Done func(T) bool + // Interval must be positive: time.NewTicker panics otherwise. Callers set + // it from a package constant, so this is a programming error, not input. + Interval time.Duration + Timeout time.Duration + + // Subject reads after "waiting for": "owners to provision on app X". + Subject string + // Success is the completion sentence stem, printed as " after 12s.". + Success string + // Slow says why a timeout is not necessarily a failure: "provisioning can + // take several minutes". + Slow string + // Recheck is the command to suggest on timeout: "c1i apps owners X". + Recheck string + + // Out receives the progress and success lines; nil means cmd's stdout. + // A list command must set this to stderr, or its prose lines land in the + // middle of the NDJSON stream a caller is piping to jq. + Out io.Writer +} + +// runWait polls op until op.Done is satisfied, op.Timeout elapses, or cmd's +// context is canceled, writing progress lines to op.Out. It returns the +// satisfying poll's value, so a caller that needs to print what it settled on +// does not have to smuggle it out of the Poll closure. +// +// A timeout is an error (so scripts can branch on it) but deliberately not +// phrased as a failure: the write it is waiting on was already accepted. It is +// a bare error, so it exits 1 -- the same code set-owners' --wait has always +// returned. Giving it a code of its own would change that, and belongs with +// the README/agents.md exit-code table, not here. +func runWait[T any](cmd *cobra.Command, op waitOp[T]) (T, error) { + var zero T + out := op.Out + if out == nil { + out = cmd.OutOrStdout() + } + + ctx, cancel := context.WithTimeout(cmd.Context(), op.Timeout) + defer cancel() + + start := time.Now() + ticker := time.NewTicker(op.Interval) + defer ticker.Stop() + + // firstPoll suppresses the "still waiting" line on the very first check: + // that poll happens immediately after the write, before any real waiting + // has elapsed, so printing it there would misleadingly imply time has + // already passed. Starting with the second poll, real time (>= one tick) + // has actually elapsed, so the message is accurate. + firstPoll := true + for { + got, err := op.Poll(ctx) + if err != nil { + if ctx.Err() != nil { + break // fall through to the timeout/cancellation report below + } + return zero, err + } + if op.Done(got) { + _, _ = fmt.Fprintf(out, "%s after %s.\n", op.Success, time.Since(start).Round(time.Second)) + return got, nil + } + if !firstPoll { + _, _ = fmt.Fprintf(out, "Still waiting for %s (%s elapsed)...\n", + op.Subject, time.Since(start).Round(time.Second)) + } + firstPoll = false + + select { + case <-ctx.Done(): + case <-ticker.C: + continue + } + break + } + + if cmd.Context().Err() != nil { + return zero, fmt.Errorf("canceled while waiting for %s", op.Subject) + } + return zero, fmt.Errorf( + "timed out after %s waiting for %s; "+ + "this is not necessarily a failure — %s, "+ + "check again later with: %s", + op.Timeout, op.Subject, op.Slow, op.Recheck) +} + +// untilPresent is the Done predicate for "every wanted id has shown up". +// Extras in the polled set, ordering, and duplicates don't matter; an empty +// want list is trivially satisfied. Stateless. +func untilPresent(want []string) func([]string) bool { + return func(got []string) bool { + gotSet := make(map[string]struct{}, len(got)) + for _, id := range got { + gotSet[id] = struct{}{} + } + for _, id := range want { + if _, ok := gotSet[id]; !ok { + return false + } + } + return true + } +} + +// untilStable is the Done predicate for "the value stopped changing": it is +// satisfied once n consecutive polls read the same value. n <= 1 settles on +// the very first read -- a footgun, so a command exposing n as a flag should +// reject anything below 2 itself. The returned closure is stateful -- one per +// wait, called once per poll. +// +// Presence-waiting cannot express this. MCP tool discovery streams (reported +// from the field: 0 -> 40 -> 101, with pauses mid-stream), so "at least one +// tool exists" reports success on a partial result. Two equal reads is not +// enough either: a pause between batches is indistinguishable from completion, +// which is also this predicate's limit -- it is a heuristic, not a proof. Pick +// n so that (n-1) poll intervals exceed the longest mid-stream pause actually +// observed for that endpoint. +func untilStable[T comparable](n int) func(T) bool { + var last T + streak := 0 + return func(v T) bool { + if streak > 0 && v == last { + streak++ + } else { + last = v + streak = 1 + } + return streak >= n + } +} + +// stableAndAtLeast gates a stability predicate behind a floor: the value must +// hold steady AND clear floor before the wait settles. It exists because +// "stopped changing" alone answers "did my write land?" with a confident, +// fast "no" -- an empty result is perfectly stable, so a query matching +// nothing settles at the first opportunity and exits 0 with no rows, which +// reads as "it did not happen" rather than "it has not happened yet". +// +// key extracts the comparable part untilStable settles on; floor is checked +// against the whole value. The stability predicate is fed on EVERY poll, before +// the floor is consulted: writing this as "floor(v) && stable(key(v))" would +// short-circuit past a stateful predicate on any poll below the floor, so a set +// that dipped below it and came back would look like it never changed. +// +// PRECONDITION: key must determine floor -- two values with equal keys must +// agree on floor. Otherwise "the key held steady" and "this value clears the +// floor" can be true of different reads, and the wait settles on a value it +// never actually saw hold. grantSetFingerprint satisfies this by prefixing the +// count, so an equal fingerprint implies an equal grant count. +func stableAndAtLeast[T any, K comparable](n int, key func(T) K, floor func(T) bool) func(T) bool { + stable := untilStable[K](n) + return func(v T) bool { + settled := stable(key(v)) + return settled && floor(v) + } +} + +// waitTimeoutFlagUsage is --wait-timeout's help text everywhere. Shared +// because the pair's wording has drifted between commands before; +// TestWaitFlagsAreRegisteredIdentically fails CI if a command hand-rolls it. +const waitTimeoutFlagUsage = "max time to wait with --wait (e.g. 30s, 5m)" + +const ( + waitFlagUsagePrefix = "block and poll " + waitFlagUsageSuffix = ", or --wait-timeout elapses" +) + +// addWaitFlags declares the --wait/--wait-timeout pair. until is the only +// per-command part of --wait's help: what is polled and what ends the wait, +// e.g. "GET .../ownerids until the requested owners appear". +func addWaitFlags(cmd *cobra.Command, until string, defaultTimeout time.Duration) { + cmd.Flags().Bool("wait", false, waitFlagUsagePrefix+until+waitFlagUsageSuffix) + cmd.Flags().Duration("wait-timeout", defaultTimeout, waitTimeoutFlagUsage) +} + +// waitFlagValues reads the pair addWaitFlags declared. A non-positive timeout +// is only an error when --wait was actually asked for. +func waitFlagValues(cmd *cobra.Command) (bool, time.Duration, error) { + wait, _ := cmd.Flags().GetBool("wait") + timeout, _ := cmd.Flags().GetDuration("wait-timeout") + if wait && timeout <= 0 { + return false, 0, &usageError{fmt.Errorf("--wait-timeout must be positive")} + } + return wait, timeout, nil +} diff --git a/cmd/wait_test.go b/cmd/wait_test.go new file mode 100644 index 0000000..453c066 --- /dev/null +++ b/cmd/wait_test.go @@ -0,0 +1,516 @@ +package cmd + +import ( + "bytes" + "context" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/ConductorOne/c1i/internal/client" + "github.com/spf13/cobra" + "github.com/spf13/pflag" +) + +// newWaitTestCmd returns a command whose stdout is captured, so runWait's +// progress lines can be asserted on. +func newWaitTestCmd(ctx context.Context) (*cobra.Command, *bytes.Buffer) { + cmd := &cobra.Command{Use: "test"} + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SetContext(ctx) + return cmd, &out +} + +// TestUntilStable pins the predicate presence-waiting cannot express: a value +// must hold steady across n consecutive reads. The n=2 case is the shipped +// bug it exists to prevent -- a pause mid-stream reads as completion. +func TestUntilStable(t *testing.T) { + cases := []struct { + name string + n int + seq []int + // wantFirstTrue is the index of the first read at which the + // predicate is satisfied, or -1 if it never is. + wantFirstTrue int + }{ + {"n=3 over a streaming discovery, settles at the third equal read", 3, []int{0, 40, 40, 101, 101, 101}, 5}, + {"n=2 fires on a mid-stream pause -- why two equal reads is not enough", 2, []int{0, 40, 40, 101}, 2}, + {"n=3 does not fire on that same pause", 3, []int{0, 40, 40, 101}, -1}, + {"n=1 accepts the very first read", 1, []int{7}, 0}, + {"n=0 also accepts the first read, it does not hang", 0, []int{7}, 0}, + {"negative n also accepts the first read", -5, []int{7}, 0}, + {"a change resets the streak", 3, []int{5, 5, 7, 5, 5}, -1}, + {"streak resumes after a reset", 2, []int{5, 7, 5, 5}, 3}, + {"zero is a value like any other", 2, []int{0, 0}, 1}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + done := untilStable[int](tc.n) + got := -1 + for i, v := range tc.seq { + if done(v) { + got = i + break + } + } + if got != tc.wantFirstTrue { + t.Errorf("untilStable(%d) over %v first satisfied at index %d, want %d", + tc.n, tc.seq, got, tc.wantFirstTrue) + } + }) + } +} + +// TestUntilStableOnStrings pins that untilStable works on any comparable +// value, not just counts -- a fingerprint string is the other intended use. +func TestUntilStableOnStrings(t *testing.T) { + done := untilStable[string](2) + for i, v := range []string{"a", "b", "b"} { + got := done(v) + want := i == 2 + if got != want { + t.Errorf("read %d (%q): got %v, want %v", i, v, got, want) + } + } +} + +// TestUntilPresentIsStateless guards the difference between the two +// predicates: untilStable is deliberately stateful, untilPresent must not be, +// or a transient disappearance would be remembered. +func TestUntilPresentIsStateless(t *testing.T) { + done := untilPresent([]string{"a"}) + for i, tc := range []struct { + got []string + want bool + }{ + {[]string{"a"}, true}, + {nil, false}, + {[]string{"b", "a"}, true}, + {[]string{"b"}, false}, + } { + if got := done(tc.got); got != tc.want { + t.Errorf("call %d: untilPresent([a])(%v) = %v, want %v", i, tc.got, got, tc.want) + } + } +} + +// TestRunWaitSucceedsAfterSeveralPolls covers the loop's happy path and the +// first-poll suppression: the "still waiting" line must not appear for the +// poll that happens immediately, before any time has elapsed. +func TestRunWaitSucceedsAfterSeveralPolls(t *testing.T) { + cmd, out := newWaitTestCmd(context.Background()) + var polls int + _, err := runWait(cmd, waitOp[int]{ + Poll: func(context.Context) (int, error) { polls++; return polls, nil }, + Done: func(v int) bool { return v >= 3 }, + Interval: time.Millisecond, + Timeout: 10 * time.Second, + Subject: "widgets to settle on thing X", + Success: "Widgets settled on thing X", + Slow: "settling can take several minutes", + Recheck: "c1i widgets get X", + }) + if err != nil { + t.Fatalf("runWait returned %v, want nil", err) + } + if polls != 3 { + t.Errorf("polled %d times, want 3", polls) + } + lines := strings.Split(strings.TrimRight(out.String(), "\n"), "\n") + if len(lines) != 2 { + t.Fatalf("got %d output lines, want 2 (one progress + one success):\n%s", len(lines), out.String()) + } + if !strings.HasPrefix(lines[0], "Still waiting for widgets to settle on thing X (") { + t.Errorf("progress line = %q", lines[0]) + } + if !strings.HasPrefix(lines[1], "Widgets settled on thing X after ") || !strings.HasSuffix(lines[1], ".") { + t.Errorf("success line = %q", lines[1]) + } +} + +// TestRunWaitReturnsTheSatisfyingValue pins that runWait hands back the poll +// that satisfied Done, so a caller printing what it settled on does not have to +// smuggle it out of the Poll closure. +func TestRunWaitReturnsTheSatisfyingValue(t *testing.T) { + cmd, _ := newWaitTestCmd(context.Background()) + var polls int + got, err := runWait(cmd, waitOp[string]{ + Poll: func(context.Context) (string, error) { polls++; return fmt.Sprintf("read-%d", polls), nil }, + Done: func(v string) bool { return v == "read-3" }, + Interval: time.Millisecond, + Timeout: 10 * time.Second, + Subject: "s", + Success: "Done", + Slow: "slow", + Recheck: "c1i check", + }) + if err != nil { + t.Fatalf("runWait returned %v, want nil", err) + } + if got != "read-3" { + t.Errorf("runWait returned %q, want the satisfying poll %q", got, "read-3") + } +} + +// (A "timeout returns the zero value" test lives at the caller instead -- +// TestWaitForGrantsTimesOut. runWait cannot fabricate a non-zero T, so such a +// test here would be true by construction and could never fail.) + +// TestRunWaitSuppressesProgressOnImmediateSuccess pins that a wait satisfied +// by its very first poll prints only the success line. +func TestRunWaitSuppressesProgressOnImmediateSuccess(t *testing.T) { + cmd, out := newWaitTestCmd(context.Background()) + if _, err := runWait(cmd, waitOp[int]{ + Poll: func(context.Context) (int, error) { return 1, nil }, + Done: func(int) bool { return true }, + Interval: time.Millisecond, + Timeout: time.Second, + Subject: "s", + Success: "Done", + Slow: "it can be slow", + Recheck: "c1i check", + }); err != nil { + t.Fatalf("runWait returned %v, want nil", err) + } + if strings.Contains(out.String(), "Still waiting") { + t.Errorf("first-poll success printed a progress line:\n%s", out.String()) + } +} + +// TestRunWaitHonorsOutWriter pins that op.Out redirects every line runWait +// prints. A list command sets it to stderr; if runWait ignored it, prose would +// land in the middle of that command's NDJSON stream. +func TestRunWaitHonorsOutWriter(t *testing.T) { + cmd, stdout := newWaitTestCmd(context.Background()) + var elsewhere bytes.Buffer + var polls int + if _, err := runWait(cmd, waitOp[int]{ + Poll: func(context.Context) (int, error) { polls++; return polls, nil }, + Done: func(v int) bool { return v >= 3 }, + Interval: time.Millisecond, + Timeout: time.Second, + Subject: "s", + Success: "Done", + Slow: "slow", + Recheck: "c1i check", + Out: &elsewhere, + }); err != nil { + t.Fatalf("runWait returned %v, want nil", err) + } + if stdout.String() != "" { + t.Errorf("runWait wrote to stdout despite op.Out:\n%s", stdout.String()) + } + if !strings.Contains(elsewhere.String(), "Still waiting for s (") || + !strings.Contains(elsewhere.String(), "Done after ") { + t.Errorf("op.Out did not receive both lines:\n%s", elsewhere.String()) + } +} + +// TestRunWaitTimeout pins the timeout error: it must name the subject, say it +// is not necessarily a failure, and hand back a recheck command. +func TestRunWaitTimeout(t *testing.T) { + cmd, _ := newWaitTestCmd(context.Background()) + _, err := runWait(cmd, waitOp[int]{ + Poll: func(context.Context) (int, error) { return 0, nil }, + Done: func(int) bool { return false }, + Interval: time.Millisecond, + Timeout: 25 * time.Millisecond, + Subject: "widgets to settle on thing X", + Success: "Widgets settled on thing X", + Slow: "settling can take several minutes", + Recheck: "c1i widgets get X", + }) + if err == nil { + t.Fatal("runWait returned nil, want a timeout error") + } + want := "timed out after 25ms waiting for widgets to settle on thing X; " + + "this is not necessarily a failure — settling can take several minutes, " + + "check again later with: c1i widgets get X" + if err.Error() != want { + t.Errorf("timeout error =\n%q\nwant\n%q", err.Error(), want) + } +} + +// TestRunWaitPollErrorIsReturnedUnwrapped pins that a poll failure surfaces +// the Poll closure's own error unchanged, so cmd/errors.go can still classify +// it via errors.As. runWait must not add a layer of its own. +func TestRunWaitPollErrorIsReturnedUnwrapped(t *testing.T) { + sentinel := errors.New("boom") + cmd, _ := newWaitTestCmd(context.Background()) + _, err := runWait(cmd, waitOp[int]{ + Poll: func(context.Context) (int, error) { return 0, fmt.Errorf("API error: %w", sentinel) }, + Done: func(int) bool { return true }, + Interval: time.Millisecond, + Timeout: time.Second, + Subject: "s", + Success: "Done", + Slow: "slow", + Recheck: "c1i check", + }) + if err == nil || err.Error() != "API error: boom" { + t.Fatalf("err = %v, want \"API error: boom\"", err) + } + if !errors.Is(err, sentinel) { + t.Error("poll error lost its %w chain; cmd/errors.go could not classify it") + } +} + +// TestRunWaitCanceled pins that a canceled parent context is reported as a +// cancellation, not as a timeout -- the two mean different things to a script. +func TestRunWaitCanceled(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cmd, _ := newWaitTestCmd(ctx) + go func() { + time.Sleep(10 * time.Millisecond) + cancel() + }() + defer cancel() + + _, err := runWait(cmd, waitOp[int]{ + Poll: func(context.Context) (int, error) { return 0, nil }, + Done: func(int) bool { return false }, + Interval: 5 * time.Millisecond, + Timeout: 10 * time.Second, + Subject: "widgets to settle on thing X", + Success: "Widgets settled on thing X", + Slow: "slow", + Recheck: "c1i widgets get X", + }) + want := "canceled while waiting for widgets to settle on thing X" + if err == nil || err.Error() != want { + t.Fatalf("err = %v, want %q", err, want) + } +} + +// TestRunWaitCallsDoneOncePerPoll is what makes a stateful predicate safe: +// untilStable counts consecutive reads, so a second Done call on the same +// poll would inflate the streak and settle early. +func TestRunWaitCallsDoneOncePerPoll(t *testing.T) { + cmd, _ := newWaitTestCmd(context.Background()) + var polls, dones int + _, _ = runWait(cmd, waitOp[int]{ + Poll: func(context.Context) (int, error) { polls++; return polls, nil }, + Done: func(int) bool { dones++; return polls >= 4 }, + Interval: time.Millisecond, + Timeout: 10 * time.Second, + Subject: "s", + Success: "Done", + Slow: "slow", + Recheck: "c1i check", + }) + if polls != dones { + t.Errorf("Done called %d times for %d polls; must be once per poll", dones, polls) + } +} + +// ownerIDsServer returns an httptest server that answers GET .../ownerids +// with responses[i] on the i-th request, repeating the last one thereafter. +func ownerIDsServer(t *testing.T, wantPath string, responses []string) *httptest.Server { + t.Helper() + var n int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != wantPath { + t.Errorf("request path = %q, want %q", r.URL.Path, wantPath) + w.WriteHeader(http.StatusNotFound) + return + } + i := int(atomic.AddInt32(&n, 1)) - 1 + if i >= len(responses) { + i = len(responses) - 1 + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(responses[i])) + })) + t.Cleanup(srv.Close) + return srv +} + +// runOwnersWait drives the real set-owners wait -- fetchOwnerIDs, the shared +// client, and runWait -- against srv, at a test-speed poll interval. +func runOwnersWait(t *testing.T, srv *httptest.Server, appID string, want []string, timeout time.Duration) (string, error) { + t.Helper() + c := client.NewForTesting(srv.URL, srv.Client()) + op := ownersWaitOp(c, appID, want, timeout) + op.Interval = 5 * time.Millisecond + cmd, out := newWaitTestCmd(context.Background()) + _, err := runWait(cmd, op) + return out.String(), err +} + +// TestWaitForOwnersConvergesOverHTTP drives the wired end-to-end wait: the +// ownerids response starts empty and converges, and the messages must be +// exactly the ones set-owners printed before the wait loop was extracted. +func TestWaitForOwnersConvergesOverHTTP(t *testing.T) { + srv := ownerIDsServer(t, "/api/v1/apps/app1/ownerids", []string{ + `{"userIds":[]}`, + `{"userIds":["u1"]}`, + `{"userIds":["u1","u2"]}`, + }) + out, err := runOwnersWait(t, srv, "app1", []string{"u1", "u2"}, 10*time.Second) + if err != nil { + t.Fatalf("wait returned %v, want nil", err) + } + if !strings.Contains(out, "Still waiting for owners to provision on app app1 (") { + t.Errorf("missing the progress line:\n%s", out) + } + if !strings.Contains(out, "Owners provisioned on app app1 after ") { + t.Errorf("missing the success line:\n%s", out) + } +} + +// TestWaitForOwnersTimesOutOverHTTP covers the timeout path end-to-end and +// pins the recheck command the operator is handed. +func TestWaitForOwnersTimesOutOverHTTP(t *testing.T) { + srv := ownerIDsServer(t, "/api/v1/apps/app1/ownerids", []string{`{"userIds":[]}`}) + _, err := runOwnersWait(t, srv, "app1", []string{"u1"}, 30*time.Millisecond) + if err == nil { + t.Fatal("wait returned nil, want a timeout error") + } + want := "timed out after 30ms waiting for owners to provision on app app1; " + + "this is not necessarily a failure — provisioning can take several minutes, " + + "check again later with: c1i apps owners app1" + if err.Error() != want { + t.Errorf("timeout error =\n%q\nwant\n%q", err.Error(), want) + } +} + +// TestWaitForOwnersAPIErrorOverHTTP pins that a mid-wait API failure keeps its +// "API error:" prefix and its typed cause, so it exits 4 rather than 1. +func TestWaitForOwnersAPIErrorOverHTTP(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"not found"}`)) + })) + t.Cleanup(srv.Close) + + _, err := runOwnersWait(t, srv, "app1", []string{"u1"}, 5*time.Second) + if err == nil { + t.Fatal("wait returned nil, want an API error") + } + if !strings.HasPrefix(err.Error(), "API error: ") { + t.Errorf("error = %q, want an \"API error: \" prefix", err.Error()) + } + var apiErr *client.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("error %v does not unwrap to *client.APIError; exit-code classification would fall back to 1", err) + } + if apiErr.StatusCode != http.StatusNotFound { + t.Errorf("status = %d, want 404", apiErr.StatusCode) + } +} + +// wantWaitTimeoutDefaults is every command that declares --wait, with the +// --wait-timeout default it ships. Routing one value through a shared helper is +// exactly when a default gets "tidied" uniformly across callers, so each is +// pinned by name here; a new --wait caller has to add itself. +var wantWaitTimeoutDefaults = map[string]time.Duration{ + "c1i apps set-owners": 4 * time.Minute, + "c1i grants list": 4 * time.Minute, +} + +// TestWaitFlagsAreRegisteredIdentically walks the whole command tree and +// fails if any --wait pair was hand-rolled instead of going through +// addWaitFlags. The pair's help text has drifted between commands before; +// this is the guard that keeps one wording -- and, via +// wantWaitTimeoutDefaults, one set of defaults. +func TestWaitFlagsAreRegisteredIdentically(t *testing.T) { + var walk func(*cobra.Command) + seen := map[string]bool{} + walk = func(c *cobra.Command) { + waitFlag := c.Flags().Lookup("wait") + timeoutFlag := c.Flags().Lookup("wait-timeout") + if waitFlag != nil || timeoutFlag != nil { + path := c.CommandPath() + seen[path] = true + if waitFlag == nil || timeoutFlag == nil { + t.Errorf("%s declares only one of --wait/--wait-timeout; use addWaitFlags", path) + } else { + if !strings.HasPrefix(waitFlag.Usage, waitFlagUsagePrefix) || !strings.HasSuffix(waitFlag.Usage, waitFlagUsageSuffix) { + t.Errorf("%s --wait usage %q was hand-rolled; use addWaitFlags", path, waitFlag.Usage) + } + if timeoutFlag.Usage != waitTimeoutFlagUsage { + t.Errorf("%s --wait-timeout usage %q drifted from %q", path, timeoutFlag.Usage, waitTimeoutFlagUsage) + } + d, err := time.ParseDuration(timeoutFlag.DefValue) + switch { + case err != nil || d <= 0: + t.Errorf("%s --wait-timeout default %q must be a positive duration", path, timeoutFlag.DefValue) + default: + want, pinned := wantWaitTimeoutDefaults[path] + if !pinned { + t.Errorf("%s declares --wait but is not in wantWaitTimeoutDefaults; add it with the default it ships", path) + } else if d != want { + t.Errorf("%s --wait-timeout default = %s, want %s", path, d, want) + } + } + } + } + for _, sub := range c.Commands() { + walk(sub) + } + } + walk(rootCmd) + if len(seen) == 0 { + t.Fatal("found no --wait flags in the command tree; this guard is inert") + } + for path := range wantWaitTimeoutDefaults { + if !seen[path] { + t.Errorf("wantWaitTimeoutDefaults names %q, which no longer declares --wait", path) + } + } +} + +// TestWaitFlagValues pins that --wait-timeout is only validated when --wait +// was actually asked for, and that a bad value is a usage error (exit 2). +func TestWaitFlagValues(t *testing.T) { + newCmd := func() *cobra.Command { + c := &cobra.Command{Use: "x"} + addWaitFlags(c, "something until it happens", 4*time.Minute) + return c + } + + t.Run("defaults", func(t *testing.T) { + wait, timeout, err := waitFlagValues(newCmd()) + if err != nil || wait || timeout != 4*time.Minute { + t.Errorf("got (%v, %v, %v), want (false, 4m0s, nil)", wait, timeout, err) + } + }) + + t.Run("non-positive timeout without --wait is accepted", func(t *testing.T) { + c := newCmd() + mustSet(t, c.Flags(), "wait-timeout", "0s") + if _, _, err := waitFlagValues(c); err != nil { + t.Errorf("err = %v, want nil (no --wait, so the timeout is unused)", err) + } + }) + + for _, bad := range []string{"0s", "-1s"} { + t.Run("--wait with "+bad, func(t *testing.T) { + c := newCmd() + mustSet(t, c.Flags(), "wait", "true") + mustSet(t, c.Flags(), "wait-timeout", bad) + _, _, err := waitFlagValues(c) + var ue *usageError + if !errors.As(err, &ue) { + t.Fatalf("err = %v, want a *usageError (exit 2)", err) + } + if !strings.Contains(err.Error(), "--wait-timeout must be positive") { + t.Errorf("err = %q, want it to name --wait-timeout", err.Error()) + } + }) + } +} + +func mustSet(t *testing.T, fs *pflag.FlagSet, name, value string) { + t.Helper() + if err := fs.Set(name, value); err != nil { + t.Fatalf("setting --%s=%s: %v", name, value, err) + } +}