Skip to content

fix: add twirp catch-all with proxy support for unmatched paths (#237)#238

Open
yinheli wants to merge 3 commits into
falcondev-oss:devfrom
yinheli:fix/twirp-catchall-proxy
Open

fix: add twirp catch-all with proxy support for unmatched paths (#237)#238
yinheli wants to merge 3 commits into
falcondev-oss:devfrom
yinheli:fix/twirp-catchall-proxy

Conversation

@yinheli

@yinheli yinheli commented Jun 6, 2026

Copy link
Copy Markdown

GitHub Actions Cache Server only has Twirp routes for CacheService. Unmatched paths (e.g. ArtifactService) under /twirp/ return 404 because Nitro marks the segment as handled but no catch-all exists inside it.

Changes:

  • Add routes/twirp/[...path].ts — proxies unmatched Twirp paths to DEFAULT_ACTIONS_RESULTS_URL, falls back to stub 200 on failure
  • Add lib/proxy-agent.ts — reads HTTP_PROXY/HTTPS_PROXY/NO_PROXY env vars and creates an undici ProxyAgent; inject via fetchOptions.dispatcher
  • Add undici as a direct dependency (global fetch uses Node.js built-in undici which is incompatible with the locally-installed ProxyAgent)
  • Strip content-length/host/connection from forwarded headers to avoid undici header validation errors during CONNECT tunneling

yinheli added 2 commits June 6, 2026 15:19
Unmatched Twirp paths (e.g., ArtifactService) currently return 404
because Nitro marks the /twirp/ segment as handled without a catch-all.
Add routes/twirp/[...path].ts to proxy them to DEFAULT_ACTIONS_RESULTS_URL,
matching the pattern of the root catch-all in routes/[...path].ts.

Fixes falcondev-oss#237
Node.js/undici does not respect HTTP_PROXY env vars by default. Add
lib/proxy-agent.ts to create an undici ProxyAgent from env vars and
inject it via fetchOptions.dispatcher. Also add undici as direct dep.
@DrJume

DrJume commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator

Thank you for your contribution.
Please submit a failing test, which this PR is aiming to resolve, so we can verify the issue and prevent future regressions.

@yinheli

yinheli commented Jun 6, 2026

Copy link
Copy Markdown
Author

@DrJume Thanks for the review!

Here's the failure context and logs that motivate this PR.

Our CI failure (artifact upload step):

Attempt 1 of 5 failed with error: Failed request: (500) Server Error. Retrying request in 3000 ms...
Attempt 2 of 5 failed with error: Failed request: (500) Server Error. Retrying request in 5246 ms...
Attempt 3 of 5 failed with error: Failed request: (500) Server Error. Retrying request in 7013 ms...
Attempt 4 of 5 failed with error: Failed request: (500) Server Error. Retrying request in 10395 ms...

Warning: Failed to CreateArtifact: Failed to make request after 5 attempts: Failed request: (500) Server Error

This happens during the Post Build and push image step when the runner tries to upload the docker build record as an artifact.

Reproduction from #237:

$ curl -i -X POST -H 'Content-Type: application/json' --data '{}' \
    http://cache-server:3000/twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact

HTTP/1.1 404 Not Found
{"statusCode":404,"statusMessage":"Cannot find any route matching /twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact."}

Our environment shows 500 instead of 404 (likely because the @actions/artifact client retries and the aggregated failure appears as 500), but the root cause is the same: unmatched /twirp/* paths are not proxied to DEFAULT_ACTIONS_RESULTS_URL.

Verifies that unmatched /twirp/* paths (e.g. ArtifactService)
are proxied instead of returning 404.
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.

2 participants