Skip to content

fix: return confirmed daemon termination outcomes - #3124

Open
thymikee wants to merge 7 commits into
refactor/shared-daemon-registrationfrom
fix/daemon-termination-outcomes
Open

thymikee wants to merge 7 commits into
refactor/shared-daemon-registrationfrom
fix/daemon-termination-outcomes

Conversation

@thymikee

@thymikee thymikee commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

Summary

Daemon stopping now returns an explicit outcome. One function verifies the captured process lifetime, sequences TERM/KILL, and waits for exit. PID reuse and a verified zombie prove that lifetime ended; unreadable identity, failed signals, and exhausted waits do not.

Manual stop refuses success when exit is unconfirmed. Startup cleanup preserves normalized error reasons, and the transitional timeout fallback reports rejection instead of leaving it unhandled. Integration cleanup retains unconfirmed state and preserves the primary test failure.

18 files changed. Builds on #3123; part of #3116. The dependent client migration replaces the transitional cleanup paths.

Validation

Tested 1b2e47efb5:

  • AGENT_DEVICE_REQUIRE_LOOPBACK_TESTS=1 pnpm check:affected --base refactor/shared-daemon-registration --run passed; all runnable checks completed.
  • Real cleanup, HTTP daemon and replacement smoke passed. Actual children are joined before private test directories are removed.
  • Restoring the old zombie predicate, nested error details, or uncaught timeout fallback fails the corresponding regression. The timeout regression exercises the socket route and reproduces the unhandled rejection.
  • Independent review confirmed termination and response projection. GitHub CI and coverage remain authoritative on the new head.

@github-actions

github-actions Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.93 MB 4.93 MB +344 B
Package (unpacked) 4.93 MB 4.93 MB +344 B
Package (download) 1.48 MB 1.48 MB +116 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 23.2 ms 23.6 ms +0.4 ms
CLI --help 70.4 ms 68.5 ms -1.9 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 14 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/daemon-client/daemon-client-metadata.ts Outdated
Comment thread test/integration/smoke-web-platform.test.ts Outdated
Comment thread src/daemon/__tests__/daemon-stop.test.ts
Comment thread test/integration/smoke-daemon-clean.test.ts

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 6 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/daemon-client/daemon-client-metadata.ts
Comment thread src/daemon-client/daemon-client-metadata.ts Outdated
Comment thread src/daemon-client/daemon-client-metadata.ts
Comment thread test/integration/smoke-daemon-http.test.ts
Comment thread test/integration/smoke-daemon-clean.test.ts Outdated
@thymikee

thymikee commented Oct 2, 2026

Copy link
Copy Markdown
Member Author

The code at d4a7be6 looks right, but the PR is not ready to merge yet. I could not run the tests, and I judged behavior only by reading the code against the merge-base. All 14 checks pass, and there are no conflicts.

Not blocking, and you can take or leave these. In scripts/clean-daemon.ts the check termination.status !== 'exited' also fails on not-running. That status can happen when daemon.json has no processStartTime and the pid is dead, and daemon-runtime.ts:350 can write such a file. The script then throws and leaves daemon.json and daemon.lock behind, where before it cleaned up. Cleanup should remove the metadata whenever no live process holds the recorded pid, and throw only on retained, as stopDaemon does. A subprocess case with a dead pid and no processStartTime would cover it. I could not measure how often the daemon writes that file. Three sites build the daemon_exit_unconfirmed error with three different details shapes: daemon-client-metadata.ts:273, clean-daemon.ts:33, and daemon-stop.ts:53. One helper next to DaemonTerminationResult in daemon-process.ts could own that projection. Also, nothing in production passes mode: 'force' (daemon-process.ts:109). A process that is already a zombie when the stop starts gets a 0 ms wait and returns retained identity-unverified, so the takeover at lifecycle.ts:213 now fails loudly where it used to be silent. A zombie with a matching start time could count as exiting and use the wait budget. I did not confirm this race is reachable for detached daemons.

Could the PR be smaller still? It already removes stopProcessForTakeover and the duplicate TERM/KILL sequence, so I see no further owner to absorb it. Dropping the unused force mode and moving the error projection into daemon-process.ts would remove the three ad-hoc builders. The retirement layer in #3116 has to decide what lifecycle.ts:447 and daemon-client-timeout.ts:189 do with a retained result, and until then force has no consumer.

Before merge, please resolve the open P1 thread: the void stopDaemonProcessForTakeover call at daemon-client-timeout.ts:189 now rejects unhandled on a retained result.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 11 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread test/integration/support/daemon-test-cleanup.ts
Comment thread test/integration/support/daemon-test-cleanup.ts
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

This PR is ready at 1b2e47e. The zombie-pid and timeout fixes from the earlier review (#3124 (comment)) are in, and the regression tests now cover them.

Not blocking: the hasExited check in https://github.com/callstack/agent-device/blob/1b2e47e/src/daemon-process.ts#L78 now repeats the rule in classifyOwnerLivenessFromObservation (packages/host-kit/src/internal/owner-identity.ts), which host-kit/process already exports. The earlier zombie bug came from a local copy drifting away from that helper, so using classifyOwnerLivenessFromObservation({ owner: identity }, observations.get(pid) ?? null) !== 'live' would keep one owner for the rule. You can take this or leave it.

All 14 checks pass on 1b2e47e, and the unit tests and the real-process smokes (clean, HTTP, replace) cover the changed files. I did not run the tests myself. I judged the regression tests by reading the old code against their mocked inputs. I also did not confirm that Vitest ties the unhandled rejection to the timeout test, but the run fails without the fix either way. The timeout fallback still removes daemon.json and daemon.lock in its finally block even if the async stop comes back retained. That predates this PR and is left for the #3116 work, so I did not raise it again.

Nothing in this change blocks merge. Please resolve or answer the two open Cubic threads on test/integration/support/daemon-test-cleanup.ts, then merge after #3123. No conflicts.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 3, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant