Capture server.Serve() error in OAuth authorization flow

cmd/admin/organizations.go

@@ -13,8 +13,8 @@ package admin
 import (
 	"fmt"
 
+	"github.com/adobe/imscli/cmd/pretty"
 	"github.com/adobe/imscli/ims"
-	"github.com/adobe/imscli/output"
 	"github.com/spf13/cobra"
 )
 
@@ -33,7 +33,7 @@ func OrganizationsCmd(imsConfig *ims.Config) *cobra.Command {
 			if err != nil {
 				return fmt.Errorf("error in get admin organizations cmd: %w", err)
 			}
-			output.PrintPrettyJSON(resp)
+			fmt.Println(pretty.JSON(resp))
 			return nil
 		},
 	}

cmd/admin/profile.go

@@ -13,8 +13,8 @@ package admin
 import (
 	"fmt"
 
+	"github.com/adobe/imscli/cmd/pretty"
 	"github.com/adobe/imscli/ims"
-	"github.com/adobe/imscli/output"
 	"github.com/spf13/cobra"
 )
 
@@ -31,7 +31,7 @@ func ProfileCmd(imsConfig *ims.Config) *cobra.Command {
 			if err != nil {
 				return fmt.Errorf("error in get admin profile cmd: %w", err)
 			}
-			output.PrintPrettyJSON(resp)
+			fmt.Println(pretty.JSON(resp))
 			return nil
 		},
 	}

cmd/decode.go

@@ -13,6 +13,7 @@ package cmd
 import (
 	"fmt"
 
+	"github.com/adobe/imscli/cmd/pretty"
 	"github.com/adobe/imscli/ims"
 	"github.com/spf13/cobra"
 )
@@ -32,9 +33,9 @@ func decodeCmd(imsConfig *ims.Config) *cobra.Command {
 				return fmt.Errorf("error decoding the token: %w", err)
 			}
 
-			fmt.Println(decoded.Header)
+			fmt.Println(pretty.JSON(decoded.Header))
 			fmt.Println()
-			fmt.Println(decoded.Payload)
+			fmt.Println(pretty.JSON(decoded.Payload))
 
 			return nil
 		},

cmd/organizations.go

@@ -13,8 +13,8 @@ package cmd
 import (
 	"fmt"
 
+	"github.com/adobe/imscli/cmd/pretty"
 	"github.com/adobe/imscli/ims"
-	"github.com/adobe/imscli/output"
 	"github.com/spf13/cobra"
 )
 
@@ -33,7 +33,7 @@ func organizationsCmd(imsConfig *ims.Config) *cobra.Command {
 			if err != nil {
 				return fmt.Errorf("error in get organizations cmd: %w", err)
 			}
-			output.PrintPrettyJSON(resp)
+			fmt.Println(pretty.JSON(resp))
 			return nil
 		},
 	}

output/output.go → cmd/pretty/json.go

@@ -8,23 +8,19 @@
 // OF ANY KIND, either express or implied. See the License for the specific language
 // governing permissions and limitations under the License.
 
-package output
+package pretty
 
 import (
 	"bytes"
 	"encoding/json"
-	"fmt"
 )
 
-// PrintPrettyJSON prints a JSON string with indentation.
-// If the input is not valid JSON, it prints the original string.
-func PrintPrettyJSON(jsonStr string) {
-	var prettyBuf bytes.Buffer
-	err := json.Indent(&prettyBuf, []byte(jsonStr), "", "  ")
-	if err != nil {
-		// Not valid JSON, print as-is
-		fmt.Println(jsonStr)
-		return
+// JSON returns the input with JSON indentation applied.
+// If the input is not valid JSON, it returns the original string.
+func JSON(jsonStr string) string {
+	var buf bytes.Buffer
+	if err := json.Indent(&buf, []byte(jsonStr), "", "  "); err != nil {
+		return jsonStr
 	}
-	fmt.Println(prettyBuf.String())
+	return buf.String()
 }

output/output_test.go → cmd/pretty/json_test.go

@@ -8,49 +8,25 @@
 // OF ANY KIND, either express or implied. See the License for the specific language
 // governing permissions and limitations under the License.
 
-package output
+package pretty
 
 import (
-	"bytes"
-	"io"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 )
 
-// captureStdout captures the output of a function that writes to stdout.
-func captureStdout(t *testing.T, fn func()) string {
-	t.Helper()
-	old := os.Stdout
-	r, w, err := os.Pipe()
-	if err != nil {
-		t.Fatalf("failed to create pipe: %v", err)
-	}
-	os.Stdout = w
-
-	fn()
-
-	w.Close()
-	os.Stdout = old
-
-	var buf bytes.Buffer
-	if _, err := io.Copy(&buf, r); err != nil {
-		t.Fatalf("failed to read pipe: %v", err)
-	}
-	return buf.String()
-}
-
-// loadExpected reads the expected output from a golden file in testdata/.
 func loadExpected(t *testing.T, filename string) string {
 	t.Helper()
 	data, err := os.ReadFile(filepath.Join("testdata", filename))
 	if err != nil {
 		t.Fatalf("failed to read golden file %s: %v", filename, err)
 	}
-	return string(data)
+	return strings.TrimSuffix(string(data), "\n")
 }
 
-func TestPrintPrettyJSON(t *testing.T) {
+func TestJSON(t *testing.T) {
 	tests := []struct {
 		name  string
 		input string
@@ -62,20 +38,16 @@ func TestPrintPrettyJSON(t *testing.T) {
 		{name: "empty object", input: `{}`, file: "empty_object.json"},
 		{name: "empty array", input: `[]`, file: "empty_array.json"},
 		{name: "nested objects", input: `{"a":{"b":{"c":1}}}`, file: "nested_objects.json"},
-		{name: "non-JSON is printed as-is", input: "this is not JSON", file: "non_json.txt"},
-		{name: "empty string is printed as-is", input: "", file: "empty_string.txt"},
+		{name: "non-JSON is returned as-is", input: "this is not JSON", file: "non_json.txt"},
+		{name: "empty string is returned as-is", input: "", file: "empty_string.txt"},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			expected := loadExpected(t, tt.file)
-
-			got := captureStdout(t, func() {
-				PrintPrettyJSON(tt.input)
-			})
-
+			got := JSON(tt.input)
 			if got != expected {
-				t.Errorf("PrintPrettyJSON(%q)\ngot:\n%s\nexpected output is in testdata/%s", tt.input, got, tt.file)
+				t.Errorf("JSON(%q)\ngot:\n%s\nexpected:\n%s", tt.input, got, expected)
 			}
 		})
 	}

cmd/profile.go

@@ -13,8 +13,8 @@ package cmd
 import (
 	"fmt"
 
+	"github.com/adobe/imscli/cmd/pretty"
 	"github.com/adobe/imscli/ims"
-	"github.com/adobe/imscli/output"
 	"github.com/spf13/cobra"
 )
 
@@ -32,7 +32,7 @@ func profileCmd(imsConfig *ims.Config) *cobra.Command {
 			if err != nil {
 				return fmt.Errorf("error in get profile cmd: %w", err)
 			}
-			output.PrintPrettyJSON(resp)
+			fmt.Println(pretty.JSON(resp))
 			return nil
 		},
 	}

cmd/validate/access_token.go

@@ -13,8 +13,8 @@ package validate
 import (
 	"fmt"
 
+	"github.com/adobe/imscli/cmd/pretty"
 	"github.com/adobe/imscli/ims"
-	"github.com/adobe/imscli/output"
 	"github.com/spf13/cobra"
 )
 
@@ -35,7 +35,7 @@ func AccessTokenCmd(imsConfig *ims.Config) *cobra.Command {
 			if !resp.Valid {
 				return fmt.Errorf("invalid token: %v", resp.Info)
 			}
-			output.PrintPrettyJSON(resp.Info)
+			fmt.Println(pretty.JSON(resp.Info))
 			return nil
 		},
 	}

cmd/validate/authorization_code.go

@@ -13,8 +13,8 @@ package validate
 import (
 	"fmt"
 
+	"github.com/adobe/imscli/cmd/pretty"
 	"github.com/adobe/imscli/ims"
-	"github.com/adobe/imscli/output"
 	"github.com/spf13/cobra"
 )
 
@@ -35,7 +35,7 @@ func AuthzCodeCmd(imsConfig *ims.Config) *cobra.Command {
 			if !resp.Valid {
 				return fmt.Errorf("invalid token: %v", resp.Info)
 			}
-			output.PrintPrettyJSON(resp.Info)
+			fmt.Println(pretty.JSON(resp.Info))
 			return nil
 		},
 	}

cmd/validate/device_token.go

@@ -13,8 +13,8 @@ package validate
 import (
 	"fmt"
 
+	"github.com/adobe/imscli/cmd/pretty"
 	"github.com/adobe/imscli/ims"
-	"github.com/adobe/imscli/output"
 	"github.com/spf13/cobra"
 )
 
@@ -35,7 +35,7 @@ func DeviceTokenCmd(imsConfig *ims.Config) *cobra.Command {
 			if !resp.Valid {
 				return fmt.Errorf("invalid token: %v", resp.Info)
 			}
-			output.PrintPrettyJSON(resp.Info)
+			fmt.Println(pretty.JSON(resp.Info))
 			return nil
 		},
 	}

cmd/validate/refresh_token.go

@@ -13,8 +13,8 @@ package validate
 import (
 	"fmt"
 
+	"github.com/adobe/imscli/cmd/pretty"
 	"github.com/adobe/imscli/ims"
-	"github.com/adobe/imscli/output"
 	"github.com/spf13/cobra"
 )
 
@@ -35,7 +35,7 @@ func RefreshTokenCmd(imsConfig *ims.Config) *cobra.Command {
 			if !resp.Valid {
 				return fmt.Errorf("invalid token: %v", resp.Info)
 			}
-			output.PrintPrettyJSON(resp.Info)
+			fmt.Println(pretty.JSON(resp.Info))
 			return nil
 		},
 	}

docs/oauth-serve-error.md

@@ -0,0 +1,84 @@
+# OAuth Local Server: Unhandled Serve() Error
+
+## The Problem
+
+`go server.Serve(listener)` (`ims/authz_user.go:130`) launches a goroutine. The `error`
+return from `Serve` is discarded.
+
+`http.Server.Serve` (`ims-go/login/server.go:136-138`) delegates to the standard
+library's `http.Server.Serve`. If the listener fails, `Serve` returns an error
+immediately. No HTTP handler ever runs.
+
+Since no handler runs, nothing writes to `resCh` or `errCh`
+(`ims-go/login/server.go:91-92`).
+
+The `select` (`ims/authz_user.go:137-145`) is waiting on `server.Error()`,
+`server.Response()`, and a 5-minute timeout. Neither channel will ever receive a value.
+
+The user waits 5 minutes for the timeout to fire, then gets a generic "user timed out"
+error instead of the actual listener failure.
+
+## Practical Likelihood
+
+In practice, this is hard to trigger. The most obvious cause — port already in use — is
+caught earlier at `net.Listen` (`ims/authz_user.go:112-114`), before `Serve` is called.
+Once `net.Listen` succeeds, `Serve` with a valid listener will block on `Accept()` until
+`Shutdown()` is called. A failure would require something unusual like the file descriptor
+being invalidated between `Listen` and `Serve`.
+
+The fix is applied as a defensive pattern: capturing goroutine errors and reacting to
+them is good practice regardless of how likely the failure is, and it has no cost.
+
+## The Fix
+
+Capture the `Serve` error via a buffered channel and add it as a case in the `select`:
+
+```go
+serveCh := make(chan error, 1)
+go func() {
+    serveCh <- server.Serve(listener)
+}()
+```
+
+The new `select` case:
+
+```go
+case serr = <-serveCh:
+    log.Println("The local server stopped unexpectedly.")
+```
+
+This gives the user an immediate, accurate error instead of a 5-minute wait.
+
+## Why the Channel Must Be Buffered
+
+In the normal flow, `Serve` runs in the background while the `select`
+(`ims/authz_user.go:137-145`) waits for one of three things: response, error, or
+timeout.
+
+The browser callback arrives. The handler writes to `resCh`
+(`ims-go/login/result.go:35-36`). The `select` reads from `server.Response()` and
+proceeds.
+
+We reach `Shutdown()` (`ims/authz_user.go:159-163`). `Shutdown` closes the listener
+and waits for handlers to finish, then returns.
+
+Once the listener is closed, `server.Serve(listener)` (`ims-go/login/server.go:136-138`)
+returns `http.ErrServerClosed`.
+
+Our goroutine tries to write this error to `serveCh`:
+
+```go
+go func() {
+    serveCh <- server.Serve(listener)
+}()
+```
+
+Nobody is reading from `serveCh` — we already left the `select` back when the browser
+callback arrived.
+
+If `serveCh` were **unbuffered**, the goroutine blocks on the write forever — a goroutine
+leak (the same class of problem described in `docs/oauth-shutdown-deadlock.md`).
+
+With `serveCh` buffered at capacity 1, the write succeeds immediately into the buffer.
+The goroutine exits. The channel and its buffered value are cleaned up by GC when nothing
+references them anymore.

ims/authz_user.go

@@ -127,7 +127,13 @@ func (i Config) AuthorizeUser() (string, error) {
 		fmt.Fprintf(os.Stderr, "error launching the browser, open it and visit %s\n", localUrl)
 	}
 
-	go server.Serve(listener)
+	// Capture Serve errors via a buffered channel. Buffered so the goroutine
+	// can always write and exit, even if nobody reads (e.g., a response arrived
+	// first). See docs/oauth-serve-error.md for a detailed explanation.
+	serveCh := make(chan error, 1)
+	go func() {
+		serveCh <- server.Serve(listener)
+	}()
 
 	var (
 		serr error
@@ -139,6 +145,8 @@ func (i Config) AuthorizeUser() (string, error) {
 		log.Println("The IMS HTTP handler returned an error message.")
 	case resp = <-server.Response():
 		log.Println("The IMS HTTP handler returned a message.")
+	case serr = <-serveCh:
+		log.Println("The local server stopped unexpectedly.")
 	case <-time.After(time.Minute * 5):
 		fmt.Fprintf(os.Stderr, "Timeout reached waiting for the user to finish the authentication ...\n")
 		serr = fmt.Errorf("user timed out")