fix(service): bake WinSW admin env and retry stop-path probes - #805
Conversation
Close the remaining #764 gaps: native WinSW now inherits install-time OPENCODEX_ADMIN_AUTH_TOKEN / OPENCODEX_ACL_TIMEOUT_MS, and service stop liveness retries short transport timeouts instead of treating a just-bound proxy as absent.
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe change adds bounded retries to proxy identity checks, applies a shared liveness policy to service stop and orphan cleanup, and updates WinSW XML to include configured environment values while excluding authentication tokens. ChangesService lifecycle updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant service
participant proxyIdentityAt
participant healthz
participant sleepFn
service->>proxyIdentityAt: findLiveProxy(SERVICE_STOP_LIVENESS)
proxyIdentityAt->>healthz: request /healthz
healthz-->>proxyIdentityAt: transport failure
proxyIdentityAt->>sleepFn: wait 100 ms
sleepFn-->>proxyIdentityAt: retry
proxyIdentityAt->>healthz: request /healthz
healthz-->>proxyIdentityAt: proxy identity
proxyIdentityAt-->>service: live proxy result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7a74d096d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex P1: uninstall retains the WinSW XML, so baking the admin secret would leave it on disk. Keep ACL timeout + always-bake OPENCODEX_HOME so file-backed admin auth still resolves under SCM.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/service.test.ts`:
- Around line 664-668: Extend the service source assertions in
tests/service.test.ts to verify the default findProxy closure uses
proxyStillLiveAfterStop, in addition to the existing orphan-cleanup assertions.
Add a focused assertion near the checks for findLiveProxy and
SERVICE_STOP_LIVENESS, targeting the default stop-verification path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f3bfcac-e94b-4fdf-a28f-76191a0e4e53
📒 Files selected for processing (6)
src/lib/winsw.tssrc/server/proxy-liveness.tssrc/service.tstests/proxy-liveness.test.tstests/service.test.tstests/winsw.test.ts
CodeRabbit: pin the proxyStillLiveAfterStop default findProxy closure, not only the orphan-cleanup call site.
Windows CI was timing out at Bun's 5s default while AUTH_TIMEOUT waited on a hung gh spawn — inject StarDeps so route tests stay hermetic.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/proxy-liveness.ts`:
- Around line 92-107: Bound proxy discovery by one aggregate caller deadline:
update findLiveProxy and proxyIdentityAt to propagate the remaining budget and
derive each probe’s timeout/retries from it, so multi-candidate discovery cannot
exceed proxyStillLiveAfterStop or stopTrackedProxyIfRunning verification
windows. Preserve immediate returns for successful or definitive non-transport
responses, and add a focused all-transport-failure regression test near the
existing service tests in tests/service.test.ts.
- Line 90: Update the attempts normalization in proxyIdentityAt so NaN is
converted to the documented one-attempt default before applying Math.trunc and
the 1–5 bounds. Preserve the existing clamping behavior for valid numeric values
and ensure the loop still performs one fetch attempt when attempts is NaN.
In `@tests/winsw.test.ts`:
- Around line 33-37: Update the WinSW XML test around the OPENCODEX_HOME
assertions to parse the generated XML, extract and decode the OPENCODEX_HOME
environment value, and compare it with getConfigDir(). Keep the existing
assertions that secret token values are absent, and apply the same
resolved-value validation to the additional assertion range.
- Around line 39-49: Add a unique sentinel value for OPENCODEX_API_AUTH_TOKEN in
the env passed to buildWinswXml within the test, then assert that this sentinel
does not occur in the generated XML, alongside the existing token-name
assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2bc9965b-f27b-45a3-a50e-a077ce8aaf15
📒 Files selected for processing (6)
src/lib/winsw.tssrc/server/proxy-liveness.tssrc/service.tstests/proxy-liveness.test.tstests/service.test.tstests/winsw.test.ts
Stop-path discovery now shares a wall-clock deadline so multi-candidate SERVICE_STOP_LIVENESS retries cannot overrun the verification window, and tests assert resolved OPENCODEX_HOME plus absent API/admin token values.
The preview route test was failing at Bun's 5s default under runner contention while siblings already used 20s.
|
Merging this. Why it helps: WinSW services now see OPENCODEX_HOME (and ACL timeout when set) without embedding admin tokens in XML, and stop-path /healthz probes retry briefly so a just-bound proxy is not treated as dead. |
1 participant
Summary
Fixes #764
Closes the remaining gaps after #780:
OPENCODEX_HOMEinto WinSW XML and pass install-timeOPENCODEX_ACL_TIMEOUT_MSso the native SCM service sees them (SCM does not inherit the interactive user env). Admin auth stays file-backed (admin-api-token) — never embedOPENCODEX_ADMIN_AUTH_TOKENin the XML./healthztransport timeouts on the service-stop liveness path (SERVICE_STOP_LIVENESS: 1500ms x 3 attempts) so a just-bound proxy is not treated as absent.Test plan
bun test tests/winsw.test.ts tests/proxy-liveness.test.ts tests/service.test.ts tests/service-stop-verification.test.tsbun run typecheckSummary by CodeRabbit
Bug Fixes
Tests