Skip to content

fix: show errors when running a detached process #940

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Conversation

yrobla
Copy link
Contributor

@yrobla yrobla commented Jul 3, 2025

This was raised by this issue, but is a common problem, when toolhive is run in detached mode, all the errors are masked, only can be seen in foreground.
This PR populates error logs in detached mode, so problems are visible to the end user

Closes: #704

@yrobla yrobla requested review from dmjb, rdimitrov and danbarr July 3, 2025 13:57
Copy link
Collaborator

@danbarr danbarr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the error happening now for example if I run with a bad secret key reference. But It's a simple text output, doesn't have the same format as other log output, should we be expecting a red ERR level? Here's full output:

./bin/thv run --secret badsecretkey,target=GITHUB_PERSONAL_ACCESS_TOKEN github
A new version of ToolHive is available: v0.1.2
Currently running: v0.1.2-5-g0e353a0
3:12PM INF MCP server ghcr.io/github/github-mcp-server:latest is verified successfully
3:12PM INF Image ghcr.io/github/github-mcp-server:latest has 'latest' tag, pulling to ensure we have the most recent version...
3:12PM INF Pulling image: ghcr.io/github/github-mcp-server:latest
Pulling from github/github-mcp-server: latest
Digest: sha256:db17de30c03103dd8b40ba63686a899270a79a1487d2304db0314e5c716118cf
Status: Image is up to date for ghcr.io/github/github-mcp-server:latest
3:12PM INF Successfully pulled image: ghcr.io/github/github-mcp-server:latest
3:12PM INF Logging to: /Users/dan/Library/Application Support/toolhive/logs/github.log
Error: startup error (PID 57967): A new version of ToolHive is available: v0.1.2
Currently running: v0.1.2-5-g0e353a0
3:12PM WRN MCP server ghcr.io/github/github-mcp-server:latest has no provenance information set, skipping image verification
3:12PM INF Image ghcr.io/github/github-mcp-server:latest has 'latest' tag, pulling to ensure we have the most recent version...
3:12PM INF Pulling image: ghcr.io/github/github-mcp-server:latest
3:12PM INF Successfully pulled image: ghcr.io/github/github-mcp-server:latest
3:12PM INF OIDC validation disabled, using local user authentication
3:12PM INF Using local user authentication for user: dan
3:12PM INF MCP parsing middleware enabled for transport
3:12PM INF Saved run configuration for github
Error: failed to get secrets: secret not found: badsecretkey
image

@yrobla
Copy link
Contributor Author

yrobla commented Jul 4, 2025

I see the error happening now for example if I run with a bad secret key reference. But It's a simple text output, doesn't have the same format as other log output, should we be expecting a red ERR level? Here's full output:

./bin/thv run --secret badsecretkey,target=GITHUB_PERSONAL_ACCESS_TOKEN github
A new version of ToolHive is available: v0.1.2
Currently running: v0.1.2-5-g0e353a0
3:12PM INF MCP server ghcr.io/github/github-mcp-server:latest is verified successfully
3:12PM INF Image ghcr.io/github/github-mcp-server:latest has 'latest' tag, pulling to ensure we have the most recent version...
3:12PM INF Pulling image: ghcr.io/github/github-mcp-server:latest
Pulling from github/github-mcp-server: latest
Digest: sha256:db17de30c03103dd8b40ba63686a899270a79a1487d2304db0314e5c716118cf
Status: Image is up to date for ghcr.io/github/github-mcp-server:latest
3:12PM INF Successfully pulled image: ghcr.io/github/github-mcp-server:latest
3:12PM INF Logging to: /Users/dan/Library/Application Support/toolhive/logs/github.log
Error: startup error (PID 57967): A new version of ToolHive is available: v0.1.2
Currently running: v0.1.2-5-g0e353a0
3:12PM WRN MCP server ghcr.io/github/github-mcp-server:latest has no provenance information set, skipping image verification
3:12PM INF Image ghcr.io/github/github-mcp-server:latest has 'latest' tag, pulling to ensure we have the most recent version...
3:12PM INF Pulling image: ghcr.io/github/github-mcp-server:latest
3:12PM INF Successfully pulled image: ghcr.io/github/github-mcp-server:latest
3:12PM INF OIDC validation disabled, using local user authentication
3:12PM INF Using local user authentication for user: dan
3:12PM INF MCP parsing middleware enabled for transport
3:12PM INF Saved run configuration for github
Error: failed to get secrets: secret not found: badsecretkey
image

Well, it's because those logs are not part of the flow itself, but come from the container. But i'll see if i can format them better.

taskbot added 2 commits July 4, 2025 10:27
This was raised by this issue, but is a common problem, when toolhive
is run in detached mode, all the errors are masked, only can be seen
in foreground.
This PR populates error logs in detached mode, so problems are visible
to the end user

Closes: #704
@yrobla yrobla requested a review from danbarr July 4, 2025 11:37
Copy link
Member

@dmjb dmjb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the approach we want to take to the secrets problem. We want to check all the secrets in the foreground process before starting the background process.

return
}
// Print the log file contents
// This is done in a deferred function to ensure it runs after the log file is closed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the log file not closed in the block above?

@yrobla
Copy link
Contributor Author

yrobla commented Jul 8, 2025

This is not the approach we want to take to the secrets problem. We want to check all the secrets in the foreground process before starting the background process.

I thought about it. But is true that generally, we are informing to the user about "MCP server is up and running" when it's not the case. So this PR corrects it. I could do another for the checks, but i thought this could be useful as well. At a minimum, remove the sentence of "MCP server is up and running" because it's confusing and we are not informing the user correctly. So we either show the logs here, or rephrase the sentence telling "MCP server deployment process has finished. Please check logs to get the status" or something around those lines

@yrobla yrobla closed this Jul 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

thv run doesn't error on bad or inaccessible secrets
4 participants