fix(service): report the wait that was actually spent, not the budget (rebase of #3138) - #3186
Conversation
Follow-up to #3134, which named this as the one piece of #3039 it deliberately did not carry. reportServiceServing printed Math.trunc(healthBudgetMs / 1000) — the budget it allowed — while confirmServiceServing knocks once more after a 500ms grace sleep whenever it waited at all. So a Windows run that gives up reports "within 45s" after spending 45.5s. The gap is half a second today. It is the expression that is wrong, not the number: it states a figure the run did not spend, and it understates every future grace the loop grows. The reader is using it to judge whether the service was merely still coming up, which is a question about elapsed time. The wait is now timed in reportServiceServing, through the same injected `now` the deps already carry, so confirmServiceServing's contract and its existing tests are untouched. Two tests: - the Windows budget case asserts the clock reached 45_000 + 500 and that the line says "after 46s" — the rounded real wait — and no longer says 45s; - a zero budget takes the single probe, skips the grace, and reports "after 0s", so a caller that asked not to wait is not told it waited. Reverting to the budget expression fails both. tests/service.test.ts: 183 pass, 6 fail. Those six fail identically on a clean `dev` checkout on this host (Windows) and are untouched by this change; compared by test name rather than by count.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
![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: 76d142365a
ℹ️ 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".
| const now = deps.now ?? Date.now; | ||
| const startedAt = now(); | ||
| const serving = await confirmServiceServing({ ...deps, timeoutMs: healthBudgetMs }); | ||
| const waitedMs = Math.max(0, now() - startedAt); |
There was a problem hiding this comment.
When the system clock is corrected while the 20–45s probe runs, such as after resume or initial time synchronization, Date.now() can jump. The warning then reports the clock correction rather than elapsed time—a forward jump can produce “after 3600s” almost immediately, while a backward jump can hide an hour actually spent—defeating the purpose of this fix. Measure the displayed duration with a monotonic clock such as performance.now(), while retaining an injectable clock for tests.
Useful? React with 👍 / 👎.
| console.error( | ||
| `⚠️ Service ${verb}, but no proxy answered on port ${serving.port} within ` | ||
| + `${Math.trunc(healthBudgetMs / 1000)}s.\n` | ||
| `⚠️ Service ${verb}, but no proxy answered on port ${serving.port} after ` | ||
| + `${Math.round(waitedMs / 1000)}s.\n` |
There was a problem hiding this comment.
A normal non-Windows failure now renders after 21s because the 20s budget is followed by the 500ms grace and rounded, but the exact CLI transcripts in docs-site/src/content/docs/reference/cli/lifecycle.md and the French, Turkish, and Traditional Chinese translations still show within 20s. Update those snippets so the user-facing reference matches the output introduced here and the translations remain consistent.
AGENTS.md reference: AGENTS.md:L343-L344
Useful? React with 👍 / 👎.
2 participants
Summary
Maintainer rebase of #3138 by @ntdatt812 onto current
dev— cherry-picked with author credit preserved, one auto-merge intests/service.test.ts, no conflicts.ocx servicereported the wait budget rather than the wait actually spent, so a probe that settled in 2s of a 30s budget still told the operator it had waited 30s. It now reports elapsed time.Verification
Exact head
76d142365:bun test ./tests/service.test.ts— 193 pass, 0 fail, 626 expect() calls.The PR body noted 6 failures in this file on the author's machine and said they reproduce on clean
dev. They do not appear here at all: this run is on macOS, and the six are the systemd-dependent cases thatAGENTS.mddocuments as environment-only failures in containers and non-systemd hosts. Either way they are not this diff's.Full-suite and typecheck coverage is left to CI on this exact head.
Checklist
dev