Wait for proxy readiness before starting updater - #1757
Conversation
There was a problem hiding this comment.
Pull request overview
Adds proxy readiness gating before updater startup and preserves the primary failure during cleanup.
Changes:
- Waits up to 60 seconds for proxy port 1080.
- Improves cleanup error handling.
- Adds unit and integration coverage.
Show a summary per file
| File | Description |
|---|---|
src/updater.ts |
Gates updater startup and preserves failures. |
src/proxy.ts |
Adds readiness probing and robust cleanup. |
dist/main.js |
Updates bundled action code. |
dist/cleanup.js |
Updates bundled cleanup code. |
__tests__/updater.test.ts |
Tests startup sequencing and cleanup. |
__tests__/proxy.test.ts |
Tests readiness and shutdown behavior. |
__tests__/proxy-integration.test.ts |
Exercises readiness in Docker. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/7 changed files
- Comments generated: 0
- Review effort level: Balanced
jakecoffman
left a comment
jakecoffman
left a comment
There was a problem hiding this comment.
Sounds like the Proxy also needs a performance pass, maybe some concurrency setting things up. But this change should go in regardless.
Probe port 1080 from inside the proxy container before starting the updater, with a bounded timeout and deterministic coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c7891dcd-6a84-4edc-a6e7-47e7a4b9c37d
Treat an already-stopped proxy as idempotent cleanup, remove resources in dependency order, and report cleanup failures without masking the update error. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c7891dcd-6a84-4edc-a6e7-47e7a4b9c37d
Regenerate the checked-in action bundles from a clean dependency install. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c7891dcd-6a84-4edc-a6e7-47e7a4b9c37d
a0bf430 to
45f2998
Compare
|
I was curious why it was taking so long, and Copilot turned up that we're waiting on Nuget registries to load before the proxy comes up, rather than doing it later on-demand: I think this might also solve some customer issues around slow startup times... at least one customer who is having slow startup on GHES has Nuget registries. |
The proxy container can report as running before it starts listening on port 1080. This is more pronounced for jobs with private registry configuration because the proxy performs synchronous initialization, including fetching a NuGet service index, before opening its listener.
Previously, the action started the updater immediately after Docker started the proxy container. The affected run shows the updater's connectivity check being refused, a subsequent clone failing through the unavailable proxy, and the proxy only beginning to listen about 2.26 seconds later:
The failed clone was then reported as
job_repo_not_found, obscuring the startup race.This waits for port 1080 from inside the proxy container before starting the updater. It also preserves the readiness error if cleanup encounters an already-stopped container or another Docker error.