Skip to content

Fix #614: verify daemon lifecycle and stale server state - #705

Closed
Axiya3749 wants to merge 2 commits into
Metabuilder-Labs:mainfrom
Axiya3749:fix/614-daemon-lifecycle
Closed

Fix #614: verify daemon lifecycle and stale server state#705
Axiya3749 wants to merge 2 commits into
Metabuilder-Labs:mainfrom
Axiya3749:fix/614-daemon-lifecycle

Conversation

@Axiya3749

@Axiya3749 Axiya3749 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Verifies that tj onboard actually registers the generated launchd/systemd unit instead of treating a written file or successful load command as proof. Extends tj doctor to report unloaded services, multiple live tj serve processes, stale or mismatched server.state, and the existing OTLP endpoint reachability result in one diagnostic surface.

The endpoint probe and shell-host correction had already landed on main in commit c76e1b7c; this PR keeps that implementation and adds the missing service-manager/process/state lifecycle checks from #614.

Related issue

Closes #614

Validation

  • pytest tests/unit/test_server_state.py tests/unit/test_onboard_daemon.py tests/unit/test_doctor_daemon_lifecycle.py tests/unit/test_doctor_otlp_endpoint.py -q — 49 passed
  • ruff check tokenjam/ — passed
  • mypy tokenjam/ — passed
  • Full local CI test scope — 5,404 passed; 11 failures and 8 setup errors were limited to sandbox-denied socket/ps access and Python 3.14 Rich-color behavior outside the CI Python 3.10-3.12 matrix
  • GitHub CI — lint/type checking, Python 3.10, Python 3.11, Python 3.12, test-ts, version-lockstep, and contributor guard all passed

Checklist

  • Tests pass (pytest tests/unit/ tests/synthetic/ tests/agents/ tests/integration/) in GitHub CI
  • Lint clean (ruff check tokenjam/)
  • Type check clean (mypy tokenjam/)
  • CLAUDE.md updated (not required; no architecture contract changed)
  • Test spans use tests/factories.py (not applicable; no spans added)
  • Requested @anilmurty as reviewer (mentioned here: @anilmurty)

Note: this PR changes tj doctor's exit code behavior: the new daemon-lifecycle checks (Daemon service, Daemon instances, Server state) can emit warnings that make doctor exit 1 in situations that previously exited 0. Anything scripting on tj doctor's exit code should account for this.

@anilmurty

@Axiya3749
Axiya3749 requested a review from anilmurty as a code owner August 6, 2026 14:57
@greptile-apps

greptile-apps Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR strengthens daemon lifecycle verification during onboarding and adds consolidated doctor diagnostics for service registration, duplicate server processes, and stale or mismatched server state.

  • Independently verifies launchd and systemd registration after installation.
  • Distinguishes enabled, active, unloaded, and unsupported daemon states.
  • Reports duplicate tj serve processes and validates the PID, port, and resolved configuration recorded in server.state.
  • Adds focused unit and integration coverage for the lifecycle checks.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
tokenjam/cli/cmd_doctor.py Adds daemon service, process inventory, and server-state diagnostics; the previously reported config-path mismatch is now checked.
tokenjam/cli/cmd_onboard.py Adds independent post-registration verification for launchd and systemd onboarding.
tokenjam/core/server_state.py Adds cross-platform unit inspection and process inventory while correctly excluding linked and aliased systemd states from enabled status.
tests/unit/test_doctor_daemon_lifecycle.py Covers unloaded units, duplicate processes, stale PIDs, wrong ports, and mismatched configuration paths.
tests/unit/test_onboard_daemon.py Covers failed systemd enable verification and launchd registration replacement behavior.
tests/unit/test_server_state.py Covers service-manager state classification and machine-wide tj serve process discovery.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  Onboard[tj onboard] --> Write[Write launchd or systemd unit]
  Write --> Register[Register and start unit]
  Register --> Verify[Verify service-manager state]
  Doctor[tj doctor] --> Unit[Inspect daemon unit]
  Doctor --> Processes[Inventory tj serve processes]
  Doctor --> State[Read server.state]
  State --> Identity[Validate PID, process identity, port, and config path]
Loading

Reviews (2): Last reviewed commit: "Rebase onto main, fix Server state confi..." | Re-trigger Greptile

Comment thread tokenjam/cli/cmd_doctor.py Outdated
Comment thread tokenjam/core/server_state.py
@anshss

anshss commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Thanks for this. The doctor checks and the process-listing helper are genuinely useful and I want them in.

The problem is main moved under you. The launchd install path now uses bootstrap/bootout/enable and verifies by comparing the running unit's program args, so it already covers the "load reported success but nothing registered" case, and it also catches a resident-but-stale registration that a launchctl list <label> check can't see (list returns 0 for a resident job regardless of whether its args are current). The branch is conflicting against main now too, so the green checks here predate that. Can you rebase and rework the launchd piece around what's there? I'd expect most of that hunk to fall away; the systemd, doctor and server_state additions should survive fine.

Two things to fix while you're in there:

  1. The Server state check only compares the port. ServerState already carries config_path, so a daemon running under a different config on the same port reads as healthy and doctor ends up validating the wrong database and secret. Worth one more comparison.
  2. _SYSTEMD_ENABLED_STATES includes linked, linked-runtime and alias. Those don't autostart at login, so reporting them as enabled tells the user something untrue. We only ever install via enable --now so it can't bite today, but it's a one-line tightening.

One thing I want to flag rather than change: a new warning here flips tj doctor's exit code from 0 to 1. I think that's correct, it's just a real behavior change for anything scripting on it, so worth a line in the PR body.

Also noticed the launchctl/systemctl/ps calls have no timeout, so a wedged service manager hangs doctor with no output. That's pre-existing and not your job here, I'm tracking it separately.

Verify launchd and systemd registration after onboarding, and teach doctor to report unloaded units, duplicate serve processes, and stale server state.

:wq#
Rebased fix/614-daemon-lifecycle onto the launchd bootstrap/bootout/enable
rework that landed on main since this branch was opened, dropping the
_install_launchd verification hunk this PR added -- main's program-args
comparison already covers what it checked, plus a resident-but-stale
registration case it couldn't see. The systemd and doctor/server_state
additions carried over unchanged.

Also addresses two review comments:
- Server state check now also compares config_path, not just port, so a
  same-port daemon running under a different config warns instead of
  reading healthy.
- _SYSTEMD_ENABLED_STATES no longer treats linked/linked-runtime/alias as
  enabled -- those states don't autostart at login.

Added a regression test for each.
@Axiya3749
Axiya3749 force-pushed the fix/614-daemon-lifecycle branch from 97f8902 to 81a5184 Compare August 10, 2026 15:24
@Axiya3749

Copy link
Copy Markdown
Contributor Author

Thanks for this. The doctor checks and the process-listing helper are genuinely useful and I want them in.

The problem is main moved under you. The launchd install path now uses bootstrap/bootout/enable and verifies by comparing the running unit's program args, so it already covers the "load reported success but nothing registered" case, and it also catches a resident-but-stale registration that a launchctl list <label> check can't see (list returns 0 for a resident job regardless of whether its args are current). The branch is conflicting against main now too, so the green checks here predate that. Can you rebase and rework the launchd piece around what's there? I'd expect most of that hunk to fall away; the systemd, doctor and server_state additions should survive fine.

Two things to fix while you're in there:

  1. The Server state check only compares the port. ServerState already carries config_path, so a daemon running under a different config on the same port reads as healthy and doctor ends up validating the wrong database and secret. Worth one more comparison.
  2. _SYSTEMD_ENABLED_STATES includes linked, linked-runtime and alias. Those don't autostart at login, so reporting them as enabled tells the user something untrue. We only ever install via enable --now so it can't bite today, but it's a one-line tightening.

One thing I want to flag rather than change: a new warning here flips tj doctor's exit code from 0 to 1. I think that's correct, it's just a real behavior change for anything scripting on it, so worth a line in the PR body.

Also noticed the launchctl/systemctl/ps calls have no timeout, so a wedged service manager hangs doctor with no output. That's pre-existing and not your job here, I'm tracking it separately.

@Axiya3749 Axiya3749 closed this Aug 10, 2026
@Axiya3749

Copy link
Copy Markdown
Contributor Author

Rebased onto main and dropped the _install_launchd verification hunk entirely. Your bootstrap/bootout/enable rework already covers what it checked, and catches the resident-but-stale case mine couldn't. Systemd, doctor, and server_state additions carried over clean.

Fixed both: Server state now compares config_path too (a same-port daemon under a different config warns instead of reading healthy), and _SYSTEMD_ENABLED_STATES drops linked/linked-runtime/alias. Added a regression test for each.

Added a note to the PR body about the exit-code change per your flag.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tj serve daemon lifecycle: launchd unit written but never loaded, stale OTLP endpoints lose telemetry silently

2 participants