Measure startup time - #1181
Open
hugo-ccabral wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis PR introduces phase-based instrumentation for daemon startup, tracking key initialization steps with timing and outcomes. A new readiness endpoint ( Changes
Sequence DiagramsequenceDiagram
participant Client
participant Daemon as Daemon (/_ready)
participant Phases as Phase Tracker
participant Worker
Note over Daemon: Daemon Startup
Daemon->>Phases: phaseStart("daemon_boot")
Daemon->>Phases: phaseEnd("daemon_boot", "ok")
Daemon->>Phases: phaseStart("git_setup")
Daemon->>Phases: phaseEnd("git_setup", "ok")
Daemon->>Phases: phaseStart("cache_download")
Daemon->>Phases: phaseEnd("cache_download", "skipped")
Daemon->>Phases: phaseStart("manifest_generation")
Daemon->>Phases: phaseEnd("manifest_generation", "ok")
Note over Worker: Worker Initialization
Worker->>Phases: phaseStart("worker_ready")
Worker->>Worker: dispatchWorkerState("ready")
Worker->>Phases: phaseEnd("worker_ready", "ok")
Note over Client: User Readiness Check
Client->>Daemon: GET /_ready?timeout=5000
Daemon->>Worker: isWorkerEverReady()
Daemon->>Phases: phaseSummary()
Daemon->>Client: 200 {ready: true, version: "X.Y.Z", phases: {...}}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Contributor
Tagging OptionsShould a new tag be published when this PR is merged?
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@daemon/main.ts`:
- Around line 314-320: The cache download errors are being swallowed and
phaseEnd("cache_download") is still called as if successful; update the
SANDBOX_MODE block around downloadCache(siteName) so the catch captures the
error, logs it, and calls phaseEnd with a non-ok outcome (e.g.,
phaseEnd("cache_download", "failure") or the appropriate failure payload your
phaseEnd accepts) instead of the default success; reference SANDBOX_MODE,
downloadCache, phaseStart, and phaseEnd to locate and change the code.
In `@daemon/worker.ts`:
- Around line 43-47: The ready phase is being ended without ever being started,
so change the block that handles state === "ready" && !everReady to call
phaseStart("worker_ready") before calling phaseEnd("worker_ready") (i.e., invoke
phaseStart("worker_ready") then phaseEnd("worker_ready") and only then set
everReady = true and call firstReady.resolve()); this ensures the ready phase
records a real duration instead of bootstrapping a zero-length record; reference
the variables/state names state, everReady, phaseStart, phaseEnd, and
firstReady.resolve in the worker.ts block.
- Around line 25-29: resetWorkerState() currently only recreates the worker
proxy (wp) but leaves the readiness latch variables firstReady and everReady
unchanged, causing /_ready to short-circuit after undeploy/redeploy; update
resetWorkerState() to reinitialize firstReady to a new
Promise.withResolvers<void>() and set everReady = false, and immediately attach
firstReady.promise.catch(() => {}) to prevent unhandled rejections so the
readiness logic (firstReady, everReady, and any code that resolves firstReady)
reflects the new worker lifecycle.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4eb4d6ed-786b-4d29-a19b-9d44d0fd334b
📒 Files selected for processing (3)
daemon/main.tsdaemon/phases.tsdaemon/worker.ts
Comment on lines
314
to
+320
| if (SANDBOX_MODE) { | ||
| start = performance.now(); | ||
| phaseStart("cache_download"); | ||
| await downloadCache(siteName).catch((err) => { | ||
| console.warn(`[cache] Failed to download build cache: ${err.message}`); | ||
| }); | ||
| phaseEnd("cache_download"); |
There was a problem hiding this comment.
Mark failed cache downloads explicitly.
The cache download error is swallowed, but the phase is still ended with the default ok outcome. That makes the startup metrics report success even when the cache was unavailable.
Suggested fix
- await downloadCache(siteName).catch((err) => {
- console.warn(`[cache] Failed to download build cache: ${err.message}`);
- });
- phaseEnd("cache_download");
+ const cacheOutcome = await downloadCache(siteName)
+ .then(() => "ok" as const)
+ .catch((err) => {
+ console.warn(
+ `[cache] Failed to download build cache: ${err.message}`,
+ );
+ return "fail" as const;
+ });
+ phaseEnd("cache_download", cacheOutcome);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@daemon/main.ts` around lines 314 - 320, The cache download errors are being
swallowed and phaseEnd("cache_download") is still called as if successful;
update the SANDBOX_MODE block around downloadCache(siteName) so the catch
captures the error, logs it, and calls phaseEnd with a non-ok outcome (e.g.,
phaseEnd("cache_download", "failure") or the appropriate failure payload your
phaseEnd accepts) instead of the default success; reference SANDBOX_MODE,
downloadCache, phaseStart, and phaseEnd to locate and change the code.
Comment on lines
+25
to
+29
| const firstReady = Promise.withResolvers<void>(); | ||
| // Prevent unhandled rejection if the daemon shuts down before the worker | ||
| // ever becomes ready — callers handle the race against their own timeouts. | ||
| firstReady.promise.catch(() => {}); | ||
| let everReady = false; |
There was a problem hiding this comment.
Reset the readiness state on undeploy.
resetWorkerState() only recreates wp; firstReady and everReady stay latched across deploys. After a sandbox undeploy/redeploy, / _ready will short-circuit to 200 before the new worker has actually become ready.
Suggested fix
-const firstReady = Promise.withResolvers<void>();
+let firstReady = Promise.withResolvers<void>();
...
export const resetWorkerState = () => {
workerInitFailed = false;
wp = makeWp();
+ firstReady = Promise.withResolvers<void>();
+ firstReady.promise.catch(() => {});
+ everReady = false;
};Also applies to: 72-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@daemon/worker.ts` around lines 25 - 29, resetWorkerState() currently only
recreates the worker proxy (wp) but leaves the readiness latch variables
firstReady and everReady unchanged, causing /_ready to short-circuit after
undeploy/redeploy; update resetWorkerState() to reinitialize firstReady to a new
Promise.withResolvers<void>() and set everReady = false, and immediately attach
firstReady.promise.catch(() => {}) to prevent unhandled rejections so the
readiness logic (firstReady, everReady, and any code that resolves firstReady)
reflects the new worker lifecycle.
Comment on lines
+43
to
+47
| if (state === "ready" && !everReady) { | ||
| everReady = true; | ||
| phaseEnd("worker_ready"); | ||
| firstReady.resolve(); | ||
| } |
There was a problem hiding this comment.
Start worker_ready before ending it.
phaseEnd("worker_ready") currently runs without a matching phaseStart(), and phaseEnd() bootstraps missing records with durationMs: 0. That makes the new readiness payload report a zero-length ready phase instead of time-to-ready.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@daemon/worker.ts` around lines 43 - 47, The ready phase is being ended
without ever being started, so change the block that handles state === "ready"
&& !everReady to call phaseStart("worker_ready") before calling
phaseEnd("worker_ready") (i.e., invoke phaseStart("worker_ready") then
phaseEnd("worker_ready") and only then set everReady = true and call
firstReady.resolve()); this ensures the ready phase records a real duration
instead of bootstrapping a zero-length record; reference the variables/state
names state, everReady, phaseStart, phaseEnd, and firstReady.resolve in the
worker.ts block.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="daemon/main.ts">
<violation number="1" location="daemon/main.ts:320">
P2: The cache download error is caught and swallowed, but `phaseEnd("cache_download")` still runs with the default `"ok"` outcome. This causes startup metrics to report a successful cache download even when it failed. Capture the outcome from the `.catch()` and pass it to `phaseEnd`.</violation>
<violation number="2" location="daemon/main.ts:605">
P2: `/_ready` can report a false positive after sandbox redeploys because it short-circuits on a global `everReady` flag that is never reset by `resetWorkerState()`.</violation>
</file>
<file name="daemon/worker.ts">
<violation number="1" location="daemon/worker.ts:45">
P2: `phaseEnd("worker_ready")` is called here without a corresponding `phaseStart("worker_ready")` anywhere. The `phaseEnd` implementation bootstraps a missing record with `durationMs: 0`, so the readiness phase will always report zero duration instead of actual time-to-ready. Add a `phaseStart("worker_ready")` call at the point the worker is created/started.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| { status: ready ? 200 : 503, headers }, | ||
| ); | ||
|
|
||
| if (isWorkerEverReady()) return respond(true); |
Contributor
There was a problem hiding this comment.
P2: /_ready can report a false positive after sandbox redeploys because it short-circuits on a global everReady flag that is never reset by resetWorkerState().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At daemon/main.ts, line 605:
<comment>`/_ready` can report a false positive after sandbox redeploys because it short-circuits on a global `everReady` flag that is never reset by `resetWorkerState()`.</comment>
<file context>
@@ -550,6 +564,54 @@ app.get("/_healthcheck", (c) => {
+ { status: ready ? 200 : 503, headers },
+ );
+
+ if (isWorkerEverReady()) return respond(true);
+
+ const ready = await Promise.race([
</file context>
| await downloadCache(siteName).catch((err) => { | ||
| console.warn(`[cache] Failed to download build cache: ${err.message}`); | ||
| }); | ||
| phaseEnd("cache_download"); |
Contributor
There was a problem hiding this comment.
P2: The cache download error is caught and swallowed, but phaseEnd("cache_download") still runs with the default "ok" outcome. This causes startup metrics to report a successful cache download even when it failed. Capture the outcome from the .catch() and pass it to phaseEnd.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At daemon/main.ts, line 320:
<comment>The cache download error is caught and swallowed, but `phaseEnd("cache_download")` still runs with the default `"ok"` outcome. This causes startup metrics to report a successful cache download even when it failed. Capture the outcome from the `.catch()` and pass it to `phaseEnd`.</comment>
<file context>
@@ -305,9 +313,11 @@ const createDeps = (
await downloadCache(siteName).catch((err) => {
console.warn(`[cache] Failed to download build cache: ${err.message}`);
});
+ phaseEnd("cache_download");
logs.push({
level: "info",
</file context>
| broadcast({ type: "worker-status", detail: workerState }); | ||
| if (state === "ready" && !everReady) { | ||
| everReady = true; | ||
| phaseEnd("worker_ready"); |
Contributor
There was a problem hiding this comment.
P2: phaseEnd("worker_ready") is called here without a corresponding phaseStart("worker_ready") anywhere. The phaseEnd implementation bootstraps a missing record with durationMs: 0, so the readiness phase will always report zero duration instead of actual time-to-ready. Add a phaseStart("worker_ready") call at the point the worker is created/started.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At daemon/worker.ts, line 45:
<comment>`phaseEnd("worker_ready")` is called here without a corresponding `phaseStart("worker_ready")` anywhere. The `phaseEnd` implementation bootstraps a missing record with `durationMs: 0`, so the readiness phase will always report zero duration instead of actual time-to-ready. Add a `phaseStart("worker_ready")` call at the point the worker is created/started.</comment>
<file context>
@@ -21,9 +22,29 @@ export type WorkerStatus = { state: "updating" | "ready" };
broadcast({ type: "worker-status", detail: workerState });
+ if (state === "ready" && !everReady) {
+ everReady = true;
+ phaseEnd("worker_ready");
+ firstReady.resolve();
+ }
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Adds phase-based startup timing and a true readiness signal. New
/_readywaits until the worker serves its first request and returns startup phase timings.New Features
[phase]logs andphaseSummary(); exposed via/_readyJSON (ready, version, phases, uptime).daemon_boot,git_setup,cache_download,manifest_gen,blocks_metadata,worker_ready./_readywaits for first worker-ready event; supports?timeout=<ms>(30s default, 120s max). CORS enabled; non-ready returns 503.Migration
/_readyfor readiness checks; keep/_healthcheckor/deco/_livenessfor liveness.Written for commit 89d90f6. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
GET /_ready) that returns daemon version, worker readiness state, and initialization phase timings in JSON format