Merge pull request #480 from hookdeck/fix/mcp-tool-call-stall

fix(mcp): stop a slow display-name lookup stalling tool calls; harness returns at the response

Author: leggetter

pkg/hookdeck/auth.go

@@ -175,7 +175,13 @@ func (c *Client) withoutProjectScope() *Client {
 
 // ValidateAPIKey validates an API key and returns user/project information
 func (c *Client) ValidateAPIKey() (*ValidateAPIKeyResponse, error) {
-	res, err := c.clientForCLIAuthValidate().Get(context.Background(), APIPathPrefix+"/cli-auth/validate", "", nil)
+	return c.ValidateAPIKeyContext(context.Background())
+}
+
+// ValidateAPIKeyContext is ValidateAPIKey bounded by ctx, for callers that must
+// not wait indefinitely on the API.
+func (c *Client) ValidateAPIKeyContext(ctx context.Context) (*ValidateAPIKeyResponse, error) {
+	res, err := c.clientForCLIAuthValidate().Get(ctx, APIPathPrefix+"/cli-auth/validate", "", nil)
 	if err != nil {
 		return nil, err
 	}

pkg/hookdeck/projects.go

@@ -12,7 +12,13 @@ type Project struct {
 }
 
 func (c *Client) ListProjects() ([]Project, error) {
-	res, err := c.clientForCLIAuthValidate().Get(context.Background(), APIPathPrefix+"/projects", "", nil)
+	return c.ListProjectsContext(context.Background())
+}
+
+// ListProjectsContext is ListProjects bounded by ctx, for callers that must not
+// wait indefinitely on the API.
+func (c *Client) ListProjectsContext(ctx context.Context) ([]Project, error) {
+	res, err := c.clientForCLIAuthValidate().Get(ctx, APIPathPrefix+"/projects", "", nil)
 	if err != nil {
 		return []Project{}, err
 	}

pkg/mcpcore/project_display.go

@@ -1,15 +1,31 @@
 package mcpcore
 
 import (
+	"context"
+	"time"
+
 	"github.com/hookdeck/hookdeck-cli/pkg/hookdeck"
 	"github.com/hookdeck/hookdeck-cli/pkg/project"
 )
 
+// projectDisplayLookupTimeout bounds the whole display-name lookup. The lookup
+// runs before every tool call until it succeeds, including calls that never
+// reach the API (help, a write refused in read-only mode, a rejected argument),
+// and the API client has no deadline of its own. Without this bound one slow
+// API response held the tool call open for as long as the API took: the client
+// saw initialize answered and then nothing. The name is cosmetic, so giving up
+// costs only an id where a name would have been, and the next call tries again.
+// A variable so tests can shorten it.
+var projectDisplayLookupTimeout = 5 * time.Second
+
 // FillProjectDisplayNameIfNeeded sets target.ProjectOrg and target.ProjectName
 // when target has an API key and project id but no cached org/name (typical
 // after loading the profile from disk). Fails silently on API errors. Stdio MCP
 // invokes tools sequentially, so this is safe without locking.
 //
+// The lookup gives up after projectDisplayLookupTimeout, or sooner when ctx is
+// done, so it can never hold up the tool call it runs ahead of.
+//
 // lookup is the client account-level requests are made from, which is not always
 // target: a product API served from its own host does not answer them, so the
 // lookup has to go to the account API.
@@ -19,26 +35,36 @@ import (
 // projects only works for a user-associated key, so a project-scoped key from
 // `hookdeck ci` — a common way to configure an MCP server — would otherwise
 // leave the name empty and callers with nothing but an opaque id to show.
-func FillProjectDisplayNameIfNeeded(lookup, target *hookdeck.Client) {
+func FillProjectDisplayNameIfNeeded(ctx context.Context, lookup, target *hookdeck.Client) {
 	if lookup == nil || target == nil || target.APIKey == "" || target.ProjectID == "" {
 		return
 	}
 	if target.ProjectName != "" || target.ProjectOrg != "" {
 		return
 	}
+	if ctx == nil {
+		ctx = context.Background()
+	}
+	ctx, cancel := context.WithTimeout(ctx, projectDisplayLookupTimeout)
+	defer cancel()
 
 	// The key's own project, available whatever kind of key it is.
-	if response, err := lookup.ValidateAPIKey(); err == nil && response.ProjectID == target.ProjectID {
+	if response, err := lookup.ValidateAPIKeyContext(ctx); err == nil && response.ProjectID == target.ProjectID {
 		target.ProjectName = response.ProjectName
 		target.ProjectOrg = response.OrganizationName
 		if target.ProjectName != "" || target.ProjectOrg != "" {
 			return
 		}
 	}
 
+	// Out of time: the listing would fail too.
+	if ctx.Err() != nil {
+		return
+	}
+
 	// The active project differs from the key's, so it has to be looked up.
 	// Only a user-associated key can do this.
-	projects, err := lookup.ListProjects()
+	projects, err := lookup.ListProjectsContext(ctx)
 	if err != nil {
 		return
 	}

pkg/mcpcore/project_display_test.go

@@ -1,18 +1,111 @@
 package mcpcore
 
 import (
+	"context"
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"strings"
+	"sync/atomic"
 	"testing"
+	"time"
+
+	mcpsdk "github.com/modelcontextprotocol/go-sdk/mcp"
 
 	"github.com/hookdeck/hookdeck-cli/pkg/hookdeck"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+// stalledAPI answers nothing until the request is abandoned, which is what a
+// slow API looks like to a client with no deadline of its own.
+func stalledAPI(t *testing.T) (*hookdeck.Client, *atomic.Int64) {
+	t.Helper()
+	var requests atomic.Int64
+	release := make(chan struct{})
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		requests.Add(1)
+		select {
+		case <-r.Context().Done():
+		case <-release:
+		}
+	}))
+	t.Cleanup(func() {
+		close(release)
+		server.Close()
+	})
+	baseURL, err := url.Parse(server.URL)
+	require.NoError(t, err)
+	return &hookdeck.Client{BaseURL: baseURL, APIKey: "k", ProjectID: "tm_1"}, &requests
+}
+
+func shortenProjectDisplayLookup(t *testing.T, d time.Duration) {
+	t.Helper()
+	previous := projectDisplayLookupTimeout
+	projectDisplayLookupTimeout = d
+	t.Cleanup(func() { projectDisplayLookupTimeout = previous })
+}
+
+// TestFillProjectDisplayName_GivesUpOnAStalledAPI: the lookup is cosmetic, so a
+// slow API must cost at most the timeout, not however long the API takes.
+func TestFillProjectDisplayName_GivesUpOnAStalledAPI(t *testing.T) {
+	shortenProjectDisplayLookup(t, 200*time.Millisecond)
+	client, requests := stalledAPI(t)
+
+	start := time.Now()
+	FillProjectDisplayNameIfNeeded(context.Background(), client, client)
+
+	assert.Less(t, time.Since(start), 3*time.Second, "the lookup must give up at its timeout")
+	assert.GreaterOrEqual(t, requests.Load(), int64(1), "the lookup should have been attempted")
+	assert.Empty(t, client.ProjectName, "a lookup that gave up leaves the name for the next call to fill")
+	assert.Empty(t, client.ProjectOrg)
+}
+
+// TestToolCallIsNotHeldByAStalledDisplayNameLookup is the case the acceptance
+// suite kept hitting: a tool that never needs the API (here, one standing in for
+// a write refused in read-only mode) answered nothing, because the lookup ahead
+// of it waited on the API with no deadline.
+func TestToolCallIsNotHeldByAStalledDisplayNameLookup(t *testing.T) {
+	shortenProjectDisplayLookup(t, 200*time.Millisecond)
+	client, _ := stalledAPI(t)
+	s := &Server{client: client, accountClient: client}
+
+	refusal := mcpsdk.ToolHandler(func(ctx context.Context, req *mcpsdk.CallToolRequest) (*mcpsdk.CallToolResult, error) {
+		return &mcpsdk.CallToolResult{IsError: true}, nil
+	})
+
+	done := make(chan *mcpsdk.CallToolResult, 1)
+	go func() {
+		result, _ := s.wrapWithTelemetry("gateway_sources_read", refusal)(context.Background(), newCallToolRequest(`{"action":"delete"}`))
+		done <- result
+	}()
+
+	select {
+	case result := <-done:
+		require.NotNil(t, result)
+		assert.True(t, result.IsError)
+	case <-time.After(5 * time.Second):
+		t.Fatal("the tool call is still waiting on the display-name lookup")
+	}
+}
+
+// TestFillProjectDisplayName_StopsWhenTheCallIsCancelled: a client that cancels
+// the tool call must not leave the lookup running to its own timeout.
+func TestFillProjectDisplayName_StopsWhenTheCallIsCancelled(t *testing.T) {
+	shortenProjectDisplayLookup(t, time.Minute)
+	client, _ := stalledAPI(t)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond)
+	defer cancel()
+
+	start := time.Now()
+	FillProjectDisplayNameIfNeeded(ctx, client, client)
+
+	assert.Less(t, time.Since(start), 3*time.Second, "cancelling the call must end the lookup")
+	assert.Empty(t, client.ProjectName)
+}
+
 func TestFillProjectDisplayNameIfNeeded_SetsNameFromAPI(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if r.URL.Path != "/2026-09-01/projects" {
@@ -32,14 +125,14 @@ func TestFillProjectDisplayNameIfNeeded_SetsNameFromAPI(t *testing.T) {
 		APIKey:    "k",
 		ProjectID: "proj_x",
 	}
-	FillProjectDisplayNameIfNeeded(client, client)
+	FillProjectDisplayNameIfNeeded(context.Background(), client, client)
 	require.Equal(t, "Acme", client.ProjectOrg)
 	require.Equal(t, "production", client.ProjectName)
 }
 
 func TestFillProjectDisplayNameIfNeeded_NoOpWhenNameSet(t *testing.T) {
 	client := &hookdeck.Client{ProjectID: "p", ProjectName: "already"}
-	FillProjectDisplayNameIfNeeded(client, client)
+	FillProjectDisplayNameIfNeeded(context.Background(), client, client)
 	require.Equal(t, "already", client.ProjectName)
 }
 
@@ -72,7 +165,7 @@ func TestFillProjectDisplayNameIfNeeded_LooksUpThroughTheAccountClient(t *testin
 	account := &hookdeck.Client{BaseURL: accountURL, APIKey: "k", ProjectID: "proj_x"}
 	product := &hookdeck.Client{BaseURL: productURL, APIKey: "k", ProjectID: "proj_x"}
 
-	FillProjectDisplayNameIfNeeded(account, product)
+	FillProjectDisplayNameIfNeeded(context.Background(), account, product)
 	require.Equal(t, "Acme", product.ProjectOrg)
 	require.Equal(t, "production", product.ProjectName)
 }
@@ -100,7 +193,7 @@ func TestFillProjectDisplayName_ProjectScopedKey(t *testing.T) {
 	require.NoError(t, err)
 	client := &hookdeck.Client{BaseURL: baseURL, APIKey: "ci-key", ProjectID: "tm_1"}
 
-	FillProjectDisplayNameIfNeeded(client, client)
+	FillProjectDisplayNameIfNeeded(context.Background(), client, client)
 
 	assert.Equal(t, "cli outpost testing", client.ProjectName)
 	assert.Equal(t, "Automated Testing", client.ProjectOrg)
@@ -126,7 +219,7 @@ func TestFillProjectDisplayName_FallsBackToListing(t *testing.T) {
 	require.NoError(t, err)
 	client := &hookdeck.Client{BaseURL: baseURL, APIKey: "user-key", ProjectID: "tm_1"}
 
-	FillProjectDisplayNameIfNeeded(client, client)
+	FillProjectDisplayNameIfNeeded(context.Background(), client, client)
 
 	assert.Equal(t, "the active one", client.ProjectName, "the key's own project must not be used when it is not the active one")
 }

pkg/mcpcore/server.go

@@ -270,7 +270,7 @@ func (s *Server) wrapWithTelemetry(toolName string, handler mcpsdk.ToolHandler)
 			}
 		}()
 
-		FillProjectDisplayNameIfNeeded(s.accountClient, s.client)
+		FillProjectDisplayNameIfNeeded(ctx, s.accountClient, s.client)
 
 		return handler(ctx, req)
 	}