Repository navigation
fix(daemon): close startup and shutdown ownership gaps - #3795
Conversation
|
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.
2 issues found across 11 files
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="packages/daemon/src/server/http/daemon-runtime-bounded-close.test.ts">
<violation number="1" location="packages/daemon/src/server/http/daemon-runtime-bounded-close.test.ts:200">
P3: This race leaves its one-second `timers/promises` timer running after `closing` wins, adding an unnecessary one-second live handle to every passing test run and potentially delaying worker shutdown. Abort the timeout when the race settles.</violation>
</file>
<file name="packages/daemon/src/server/http/server-shutdown.ts">
<violation number="1" location="packages/daemon/src/server/http/server-shutdown.ts:201">
P1: This deadline returns while the platform-owner shutdown is still running, so the daemon can report pending and continue the rest of shutdown while the owner still mutates platform resources; retaining the registration and runtime state does not cancel or join that work. Pass a cancellation/deadline signal through the owner shutdown, or keep the close synchronized with the owner operation before proceeding with dependent cleanup.
(Based on your team's feedback about cancelling timed-out asynchronous work.)</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
|
||
| async function closeWithinBudget( | ||
| close: () => Promise<readonly PluginCleanupOutcome[]>, | ||
| /** Settles one shutdown operation at its deadline without cancelling its unfinished work. */ |
There was a problem hiding this comment.
P1: This deadline returns while the platform-owner shutdown is still running, so the daemon can report pending and continue the rest of shutdown while the owner still mutates platform resources; retaining the registration and runtime state does not cancel or join that work. Pass a cancellation/deadline signal through the owner shutdown, or keep the close synchronized with the owner operation before proceeding with dependent cleanup.
(Based on your team's feedback about cancelling timed-out asynchronous work.)
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 packages/daemon/src/server/http/server-shutdown.ts, line 201:
<comment>This deadline returns while the platform-owner shutdown is still running, so the daemon can report pending and continue the rest of shutdown while the owner still mutates platform resources; retaining the registration and runtime state does not cancel or join that work. Pass a cancellation/deadline signal through the owner shutdown, or keep the close synchronized with the owner operation before proceeding with dependent cleanup.
(Based on your team's feedback about cancelling timed-out asynchronous work.) </comment>
<file context>
@@ -182,27 +188,30 @@ export function createPluginClose(params: {
-async function closeWithinBudget(
- close: () => Promise<readonly PluginCleanupOutcome[]>,
+/** Settles one shutdown operation at its deadline without cancelling its unfinished work. */
+export async function settleWithinBudget<T>(
+ close: () => Promise<T>,
</file context>
| }); | ||
| const closing = runtime.close(); | ||
| try { | ||
| const report = await Promise.race([closing, delay(1_000, null)]); |
There was a problem hiding this comment.
P3: This race leaves its one-second timers/promises timer running after closing wins, adding an unnecessary one-second live handle to every passing test run and potentially delaying worker shutdown. Abort the timeout when the race settles.
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 packages/daemon/src/server/http/daemon-runtime-bounded-close.test.ts, line 200:
<comment>This race leaves its one-second `timers/promises` timer running after `closing` wins, adding an unnecessary one-second live handle to every passing test run and potentially delaying worker shutdown. Abort the timeout when the race settles.</comment>
<file context>
@@ -163,3 +169,49 @@ test('the executable writes unfinished work to stderr after a close, and a clean
+ });
+ const closing = runtime.close();
+ try {
+ const report = await Promise.race([closing, delay(1_000, null)]);
+ expect(report).not.toBeNull();
+ expect(report!.platformOwners).toBe('pending');
</file context>
| const report = await Promise.race([closing, delay(1_000, null)]); | |
| const report = await (async () => { | |
| const timeout = new AbortController(); | |
| try { | |
| return await Promise.race([closing, delay(1_000, null, { signal: timeout.signal })]); | |
| } finally { | |
| timeout.abort(); | |
| } | |
| })(); |
|
I reviewed 50ae4e5 and found no blocking problems in the code. Not blocking: The Coverage job failed, and its log is cut off. No vitest summary or failing test name survives. It runs the full unit suite, which includes the three new runtime tests and the close path many daemon tests use, so I cannot say if the failure is from this PR. Main CI at 4893253 is green, but this PR is based on c966724. Please rerun the job, or run I did not run any tests. I judged that the new regression tests would fail on base by reading the old code. The live iOS On the other review threads, the lower-priority thread on the test timer in |
thymikee
added
the
ready-for-human
76b3086 to
5d3e7f1
Compare
|
The code looks good at 5d3e7f1. The delta since 50ae4e5 only changes when All 20 checks pass on 5d3e7f1, and the platform smoke checks start the daemon through the deferred server-startup import, so CI covers the new route. I did not run the eager-import gate or any tests myself, so the budget claim rests on CI and your counts. The live iOS run was at 50ae4e5; since the delta only changes load timing, I do not ask for a new one. There are no conflicts. The Cubic P1 on shutdown past the deadline does not apply, and you can resolve it: the shutdown reports platformOwners as pending and keeps registration, lock and runtime state while work is unfinished, and the bounded-owner test asserts this (#3795 (comment)). Not blocking: the Cubic timer thread on |
1 participant
Summary
Follow up on #3781 across 12 files, including daemon lifecycle and an upstream help-assertion repair.
Validation
5d3e7f1fad65546d99a8b18e32e510a96ffdd17d, rebased ontofeca8cb2fc; daemon patches preserved by range-diff.pnpm check:affected --run --head 5d3e7f1fad: 36 checks passed (exactCommit=true,checkoutChanged=false).c966724bea→50ae4e57ab: runtime 4 failures → 10 passes; entry checks 2 failures → 11 passes.50ae4e57ab, then 849 passes. Upstream foldable-help assertion: 1 failure before repair, then 22 passes. Combined focused suite: 871 passes.50ae4e57ab: Settings open, native snapshot (18 nodes), close, gracefuldaemon stop --clean; runner cleanup completed, registration/lock removed, simulator powered off.GitHub CI passed on this head, including Coverage and platform smoke checks.