Repository navigation
Conversation
A client killed a healthy daemon that was still starting: the local startup used one 15 s wall-clock deadline, and a timeout never retried. On an overloaded host that is a failed run before any work starts (#3782, failure class #3342). - startLocalDaemon now waits while the spawned daemon stays alive, up to a 120 s ceiling; AGENT_DEVICE_DAEMON_STARTUP_TIMEOUT_MS overrides it for the CLI, MCP, and Node client alike. - The daemon records startup phase markers (modules-loaded, lock-acquired, listening, registered) with timestamps in daemon-startup-phases.jsonl beside its registration. A live daemon that publishes no marker for 60 s is retired as stalled; one that keeps progressing is waited for to the ceiling. - Failure details carry reason (startup-exited, startup-stalled, startup-ceiling), the observed phase timings, and the effective startupTimeoutMs. Each reason has its own hint. - help commands documents the variable and the three reasons.
EvidenceBase: Local gate: New regression tests ( Base/head proof: disposable checkout at base with only the behavioral change reverted → all 6 tests fail for the intended reasons (no ceiling, no reasons, no env override); at head all 6 pass. Focused suites at head: startup-budget + startup-race 31 passed; lifecycle + ensuring + explicit-lifecycle 77 passed; registration-owner + daemon-entrypoint + help 79 passed. Existing lifecycle/startup-race tests keep their meaning (15 s budget pinned via Live route proof: |
|
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
You’re at about 98% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Integration Tests failed at This is a PR defect, not a known CI flake. I assigned a lifecycle fix with red/green verification through the three failing integration tests. The leak oracle stays active. I did not rerun CI. The preview comment needs no action. The Size report shows +3.2 kB unpacked. The independent review must assess whether a smaller design can meet #3782. Review and repair are in progress; the PR is not ready. |
|
I checked all six cubic claims against
The current Apex repair worker now owns these items with the integration cleanup defect. This keeps one editor on the PR. Independent review continues. Threads remain open until a verified fix or assessment is posted. |
|
Independent review of The current repair worker owns these findings with the integration cleanup defect. It must also split the startup responsibility from The reviewer treats the long gap between phases as a risk, not a demonstrated defect. Completed-phase markers can improve observability. A heartbeat must not make stuck work appear to progress. The reviewer ran 270 daemon-client tests and the focused registration, entrypoint, close, and help suites. They passed at the reviewed head. This does not replace the failed Integration Tests lane. A new independent review and CI pass are required after repair. The PR remains not ready. |
…ts owner
The integration leak oracle failed because every daemon appended its
startup phase markers to one shared `daemon-startup-phases.jsonl` beside
the registration and nothing retired that file, so any completed launch
left state-dir residue the strict oracle names as a stray entry.
Ownership now follows the file name: each launch writes only to
`daemon-startup-phases-<pid>.jsonl` through
`resolveDaemonStartupPhasesPath(stateDir, pid)`. The writing daemon
removes its file once it has registered; the launching client removes it
once that process's exit is joined (in `retireStartupAttempt`, after the
exit is observed, never before); and each new registration-lock owner
sweeps the files of provably dead pids next to the shutdown-report
removal. A live pid's file is always kept, so a concurrent or replaced
launch can neither add to, hide behind, nor delete another launch's
evidence, and the client no longer needs `spawnedAtMs` to attribute
markers to its own launch.
Along the protocol boundary:
- `recordDaemonStartupMarker` takes an optional `reportFailure` callback
so producers surface a refused append on stderr (the daemon log tail
the waiting client already ships as failure evidence) instead of
swallowing it; the append stays non-fatal and a crash-truncated final
line is separated so it cannot glue two records together.
- The phase list gains real completed boundaries, `composition-loaded`
and `recovery-finished`, and no heartbeats, so silence still means a
stalled startup and progress extends the stall window exactly as far
as the work took.
- The launch-exit join drops its redundant `.catch(() => {})`: the
`runCmdDetachedMonitored` owner resolves the promise on error and
exit, so it never rejects.
- Startup code splits out of `daemon-client-lifecycle.ts` into
`daemon-client-startup.ts` (budget, wait loop, marker refresh, hints),
keeping both modules under the 1000-line tripwire; importers attach
and ensure point at the owning module directly.
Regression coverage: the budget suite now pins per-launch attribution,
including a fresh daemon producing its own markers while an earlier
launch's leftover file is swept at the new registration, and a new
protocol suite pins the per-pid paths, the dead-pid sweep, removal
idempotence, and the write-failure seam.
Refs #3782, #3791
The documented reasons overclaimed. `startup-ceiling` said "progressing, not registered before the ceiling", but the ceiling is reached by a daemon that is merely alive: a progressing one is only ever stopped by a ceiling above the stall window, so the promise was unverifiable from the client's evidence. `startup-stalled` named a 60 s window without saying that `AGENT_DEVICE_DAEMON_STARTUP_TIMEOUT_MS` does not change it, so raising the ceiling looked like a fix for silence. Both the CLI help and the configuration table now describe the two reasons as what they are, and the env var is specified as a positive integer number of milliseconds. Refs #3791
Follow-up: the Integration Tests leak failure on this PR is fixed (a2c6e74)Cause. The daemon wrote its #3782 startup phase markers to a single shared Fix — ownership follows the file name. Each launch writes only Red→green. Review threads.
Also in this push. Startup code split out of |
There was a problem hiding this comment.
All reported issues were addressed across 12 files (changes from recent commits).
You’re at about 98% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
The lifecycle and startup module headers only narrate each file's role, which AGENTS.md reserves for public APIs, tool directives, and constraints code cannot show. The split is owned by the imports and the file names. Fixes review thread PRRT_kwDORFGHaM6tkpef on #3791.
…or it The shared emit-startup-markers flag file applied to every later fixture in the same state dir, so the stale-history cleaner inherited the request, wrote its own live marker, and the sweep assertion raced it. The request is now a typed per-child option bound into that child's entry script, and readiness waits use a per-pid file so a stale registration-held from an earlier child cannot shortcut the await. Fixes review thread PRRT_kwDORFGHaM6tkpeV on #3791.
… exit A launch that exited busy, unproven, or lost the registration to a contending winner returned its retry, error, or winner handoff without joining its exact exited child, so the child's pid-named phase file sat in the state dir until some later registration sweep. Every terminal child outcome now joins the child first and removes only that child's own file, the same ownership rule a retired startup follows; the current winner's registration and record are untouched. Regression tests write markers at the real pre-acquisition producer seam (new emitStartupMarkerBeforeAcquisition fixture option) and pin the residue rule on the retry, winner-attach, and unproven-exit paths: red at the prior head, green with the fix. Fixes review thread PRRT_kwDORFGHaM6tkpec on #3791.
The per-pid file names its writer, not its lifetime: if client and daemon of an old launch both died before either cleanup ran and the host reused the pid, the old record survived under the new writer's name, the sweep skipped it as live, and the earliest-per-phase reader mixed two lifetimes into one observation. The producer now claims its own record (claimDaemonStartupPhasesFile) before its first marker, so stale same-pid content can neither mask nor extend the fresh startup's progress. A refused claim follows the marker contract: reported, never fatal. The test child repeats the real producer sequence against its own pid, which no outside test can arrange. Addresses review finding R2-1 on #3791.
…n out of the runtime Code ownership allows no exception: behavior was added to a module over 1000 lines, so the module is split at its existing boundary first. flushDaemonStartupDiagnostics, emitDaemonDiagnostic, finishDaemonRegistration, and armDaemonMetadataLossWatch form the daemon-log diagnostic and registration-completion edge the runtime already treats as one unit; they move with their callers updated, no behavior change. daemon-runtime.ts is back under the limit.
describeStartupPhaseTimings builds a Record<string, number> that always starts at spawned: 0; the intersection cast asserted what the literal already showed. No caller branched on it. Addresses review finding R2-2 on #3791.
There was a problem hiding this comment.
All reported issues were addressed across 12 files (changes from recent commits).
You’re at about 98% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Repairs at head Causes & fixes
Validation: Threads: peV/pec/pev fixed+resolved; marker-unavailable j3DD stays open — stall retirement still applies when the marker channel is unobservable; no exemption claimed. Pending: full CI at this head (Integration Tests, Coverage, native, device lanes are GitHub-owned), and the independent review of a2c6..62a09. Not ready for merge. |
registeredDaemonFixtureArgs rewrote one shared dist/src/internal/ daemon.js per construction, so a second build could replace the script before an earlier launched child loaded it, silently applying the wrong per-child marker switches and registration fields. Each construction now writes a unique fixture-entries/<uuid>/dist/src/internal/daemon.js: the per-launch directory keeps the daemon entry path shape the retirement identity checks require, and cleanup still removes the state dir. A regression test builds both arg sets before launching either child and proves child 1 executes its own switches while child 2 exits busy: red on the shared path, green per-launch. Fixes review thread PRRT_kwDORFGHaM6tk7ed on #3791.
|
Coverage failed at This is a PR regression, not a coverage-percentage failure or a known flake. The active repair worker will reproduce the tests with the same CI base and correct the import boundary. The budgets and assertions remain unchanged. I did not request a rerun. The PR remains not ready. |
The split moved the helpers next to the runtime but left a static edge, so both daemon entries evaluated one more module and 12-15 more top-level statements than the merge-base (Coverage lane red: 3 eager-closure-budgets failures at OUTPUT_ECONOMY_BASE=4a8abed7b9). startDaemonRuntime now loads them through a function-scoped await import at its first use point, the loading shape the gate prescribes; composition, publication, flush, and shutdown order are unchanged and the source-order guards still read them from this module. Red at 62a09bd (3 failures), green here: 846/846. Addresses the eager-closure failure reported at 62a09bd on #3791.
|
Repairs at head Causes & fixes
Validation: Threads: tk7ed fixed+resolved; peV/pec/pef fixed (replies at Pending: CI at this head (Coverage/Integration/native/device GitHub-owned) and independent review of the two new commits. Not ready for merge. |
|
At 62a09bd, one defect remains in the startup wait. When another live daemon holds the lock and has not published yet, the client waits to the ceiling and reports the wrong reason. In Not blocking, and you can take or leave each: the production marker emitters in Coverage failed at 62a09bd, likely because On the open threads: the two cubic-dev-ai P2 threads do not apply, so please resolve them. The first is #3791 (comment): Before merge, please make every wait that reaches the ceiling with a live startup pending report 'startup-ceiling' and add the failing-on-head test. Then get Coverage green at the head, and attach the measurement #3782 rule 3 asks for. That is the 'Preflight iOS runner through public CLI' step log from a run at this head, after |
The prepare step's daemon_startup diagnostic is not a cold-startup measurement: the CLI's ensured transport finishes ensureDaemon before sendToDaemon's timer wraps the attach connect, so the timer only sees an attach. Time the public ensureDaemon() directly after clean:daemon and record phase, durationMs, started, and pid, so the record counts as cold only when started is true. The prepare command keeps its original semantics and attaches to the same daemon. Corrects the measurement assumption behind 62a09bd on #3791; the cold-startup CI evidence for #3782 is still missing until a run reports this record with started:true.
|
CI measurement correction at head Cause: the prepare step's Fix: after Local proof (isolated temp state dir): cold run Validation: Pending: one iOS CI run here reporting |
|
Independent review found no actionable findings in I reviewed the workflow-only change I verified receipt |
|
Required checks pass at The raw iOS job log records a fresh public SDK start after {"phase":"daemon_cold_startup","durationMs":324,"started":true,"pid":60517}This is one successful CI sample. It does not establish the maximum startup time on overloaded hosts. The CLI diagnostic measured an attach after startup, so it cannot supply this cold-start evidence. The maintainer review identifies one remaining defect. If our contender exits busy while a verified live winner still starts, the deadline can produce The PR remains not ready until the repair, independent review, exact-head gate, and new-head CI pass. No merge requested. |
…at the deadline When the launched contender exited busy while another live daemon held the registration and deferred publication, the wait loop ended in an unlabeled 'timeout' result that mapped to startup-exited with the generic hint. Any deadline with a live startup still pending, this client's launch or a verified live lock owner, now reports startup-ceiling with the effective ceiling and the overload hint; startup-exited stays reserved for a real exit or no live startup. Remove the 'timeout' wait kind so the unlabeled state cannot exist and add the sendToDaemon-boundary regressions for both outcomes. Also compress this PR's narration and duplicate test setup to keep the gross diff within one coherent change, and add the Unreleased changelog entry for the startup budget, the stall window, and the three reasons.
Repair: live startup at the ceiling reports
|
There was a problem hiding this comment.
3 issues found across 12 files (changes from recent commits).
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/daemon-client/daemon-client-startup.ts">
<violation number="1" location="src/daemon-client/daemon-client-startup.ts:522">
P3: This deadline check treats any process that reuses the launch PID as the launched daemon. Compare the launch identity/start time before classifying it as a live startup, otherwise an observed child exit can be reported incorrectly as `startup-ceiling` and the caller receives the wrong recovery reason.</violation>
</file>
<file name="src/daemon-client/__tests__/daemon-client-startup-budget.test.ts">
<violation number="1" location="src/daemon-client/__tests__/daemon-client-startup-budget.test.ts:399">
P2: This test uses a fixed real PID to model a gone child, so it can collide with an unrelated process and become flaky or classify the deadline as a live startup. Use a guaranteed-dead child identity or mock the liveness seam for this scenario.</violation>
</file>
<file name="CHANGELOG.md">
<violation number="1" location="CHANGELOG.md:5">
P2: The changelog should say that any other value fails with `INVALID_ARGS` before a daemon spawns. Without it, readers can assume a bad value falls back to the 120 s default.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| vi.stubEnv('AGENT_DEVICE_DAEMON_STARTUP_TIMEOUT_MS', '2000'); | ||
| const paths = resolveDaemonPaths(mkdtempForTestSync('daemon-start-gone-at-deadline-')); | ||
| spawn.mockImplementation(() => ({ | ||
| pid: 997_842, |
There was a problem hiding this comment.
P2: This test uses a fixed real PID to model a gone child, so it can collide with an unrelated process and become flaky or classify the deadline as a live startup. Use a guaranteed-dead child identity or mock the liveness seam for this scenario.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/daemon-client/__tests__/daemon-client-startup-budget.test.ts, line 399:
<comment>This test uses a fixed real PID to model a gone child, so it can collide with an unrelated process and become flaky or classify the deadline as a live startup. Use a guaranteed-dead child identity or mock the liveness seam for this scenario.</comment>
<file context>
@@ -377,29 +339,75 @@ test("a startup progressed by a fresh launch is not judged stalled by an earlier
+ vi.stubEnv('AGENT_DEVICE_DAEMON_STARTUP_TIMEOUT_MS', '2000');
+ const paths = resolveDaemonPaths(mkdtempForTestSync('daemon-start-gone-at-deadline-'));
+ spawn.mockImplementation(() => ({
+ pid: 997_842,
+ exited: new Promise<ExecDetachedExit>(() => {}),
+ readIdentity: () => ({ pid: 997_842, startTime: null }),
</file context>
|
|
||
| ## Unreleased | ||
|
|
||
| - A client starting a local daemon now waits while the daemon process stays alive and keeps publishing startup progress, so an overloaded host makes startup slower instead of failing it. The wait stops at a ceiling of 120 s by default; set `AGENT_DEVICE_DAEMON_STARTUP_TIMEOUT_MS` to a positive integer number of milliseconds to override it for the CLI, MCP, and Node clients alike. A failed startup names the outcome in `error.details.reason`, and you should branch on it, never on the message: `startup-exited` (the process ended before registering), `startup-stalled` (alive but no startup progress for a fixed 60 s window, which the ceiling variable does not change; the daemon is stopped), or `startup-ceiling` (a startup was still live — this client's daemon or another client's — when the ceiling passed; the client stops its own launch and never signals another client's live daemon). `details.phases` carries the startup phase timings the client observed and `details.startupTimeoutMs` the effective ceiling. |
There was a problem hiding this comment.
P2: The changelog should say that any other value fails with INVALID_ARGS before a daemon spawns. Without it, readers can assume a bad value falls back to the 120 s default.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At CHANGELOG.md, line 5:
<comment>The changelog should say that any other value fails with `INVALID_ARGS` before a daemon spawns. Without it, readers can assume a bad value falls back to the 120 s default.</comment>
<file context>
@@ -2,6 +2,8 @@
## Unreleased
+- A client starting a local daemon now waits while the daemon process stays alive and keeps publishing startup progress, so an overloaded host makes startup slower instead of failing it. The wait stops at a ceiling of 120 s by default; set `AGENT_DEVICE_DAEMON_STARTUP_TIMEOUT_MS` to a positive integer number of milliseconds to override it for the CLI, MCP, and Node clients alike. A failed startup names the outcome in `error.details.reason`, and you should branch on it, never on the message: `startup-exited` (the process ended before registering), `startup-stalled` (alive but no startup progress for a fixed 60 s window, which the ceiling variable does not change; the daemon is stopped), or `startup-ceiling` (a startup was still live — this client's daemon or another client's — when the ceiling passed; the client stops its own launch and never signals another client's live daemon). `details.phases` carries the startup phase timings the client observed and `details.startupTimeoutMs` the effective ceiling.
+
- The `daemon-not-running` hint now always names `--state-dir <dir>`, and a daemon force-retired for outlasting its drain is reported as `Replaced unreachable daemon` instead of `Replaced idle daemon`. `daemon-drain-timeout` and the post-drain force retire are documented in the client API and `help`.
</file context>
There was a problem hiding this comment.
The configuration guide already states that an invalid value fails with INVALID_ARGS. The changelog states the positive-integer requirement. I assess this as an optional release-note detail, not a behavior defect. We have passed the agreed limit for small wording rounds. I will leave this thread open for the maintainer.
| await sleep(Math.min(100, remainingStartupBudget(deadline))); | ||
| } | ||
| // A deadline passing while any startup is live is the host running out of room, never an exit. | ||
| if (isProcessAlive(launch.pid) || isLiveLockOwner(inspectProcessLock(settings.paths.lockPath))) |
There was a problem hiding this comment.
P3: This deadline check treats any process that reuses the launch PID as the launched daemon. Compare the launch identity/start time before classifying it as a live startup, otherwise an observed child exit can be reported incorrectly as startup-ceiling and the caller receives the wrong recovery reason.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/daemon-client/daemon-client-startup.ts, line 522:
<comment>This deadline check treats any process that reuses the launch PID as the launched daemon. Compare the launch identity/start time before classifying it as a live startup, otherwise an observed child exit can be reported incorrectly as `startup-ceiling` and the caller receives the wrong recovery reason.</comment>
<file context>
@@ -544,16 +513,25 @@ async function waitForDaemonStartup(
- // failure, and the reason says the host, not the daemon, ran out of room.
- return { kind: 'ceiling' };
+ // A deadline passing while any startup is live is the host running out of room, never an exit.
+ if (isProcessAlive(launch.pid) || isLiveLockOwner(inspectProcessLock(settings.paths.lockPath)))
+ return { kind: 'ceiling' };
+ if (earlyExit) return { kind: 'early_exit', exit: earlyExit };
</file context>
This branch has not been deployed
1 participant
Summary
Closes #3782. A client killed a healthy daemon that was still starting: local startup used one 15 s wall-clock deadline, so an overloaded host failed the run before any work started (#3342).
startLocalDaemonnow waits while the spawned daemon stays alive, to a 120 s ceiling;AGENT_DEVICE_DAEMON_STARTUP_TIMEOUT_MSoverrides it for CLI, MCP, and Node. Each launch claims and records phase markers (modules-loaded…registered) in its owndaemon-startup-phases-<pid>.jsonl, attributed by file name and a fresh lifetime claim. Owners retire it: the daemon afterregistered, the client after any joined terminal exit, the registration owner on sweep of provably-dead pids. Silent 60 s failsstartup-stalled; an exit fails fast (startup-exited). Any deadline with a live startup still pending — this client's launch or a verified live lock owner — failsstartup-ceilingwithreason,phases,startupTimeoutMs; a live contender is never signaled. No CI auto-detection.Scope expansions: the iOS preflight times the public SDK
ensureDaemon()afterclean:daemon, logging adaemon_cold_startuprecord. The CLIdaemon_startuptimer wraps only attach, so it cannot be cold-startup evidence.Validation
Head
f45bc9d470, base4a8abed7b9, 18 files, gross 2498.check:affected --run --head f45bc9d470: passed, exactCommit=true, checkoutChanged=false (receipt run-Hh9oLH). Ceiling-defect regression red atbef883fbc1/ green at head throughsendToDaemon. Eager budgets (OUTPUT_ECONOMY_BASE=4a8abed7b9): 846/846. Leak integrations pass. Cold-startup CI evidence atbef883fbc1:durationMs:324, started:true(run 39076295261); new-head CI pending. Integration, Coverage, native, device lanes stay GitHub-owned at this head.