feat: supervise portable custom box services - #25
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesAdds portable custom service drop-ins with supervised reconciliation, restart and quiesce behavior, Portable custom services
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/regressions-logs.sh (2)
300-311: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInline zombie-aware polling loop duplicates
wait_for_process_exit.This loop reimplements the same "kill -0 / zombie-state" polling logic just added as
wait_for_process_exit()intests/regressions-services.sh. See consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/regressions-logs.sh` around lines 300 - 311, Replace the duplicated kill-0 and zombie-state polling loop with the existing wait_for_process_exit() helper from regressions-services.sh, passing pdeath_child_pid and preserving the subsequent failure check and error message.
291-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrack
pdeath_child_pidfor harness cleanup.
pdeath_capture_pidis added topids(line 290) butpdeath_child_pid— the actual foreground process under test — is not. If the survival check at lines 307-311 trips (i.e., the fix being validated is broken), the test exits without this process being reaped. See consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/regressions-logs.sh` around lines 291 - 293, Update the PID setup around pdeath_capture_pid and pdeath_child_pid so the child PID read from pdeath_pid_file is also appended to the pids cleanup list. Preserve the existing kill -0 survival check and ensure the foreground process is tracked before the test can exit on failure.tests/regressions-services.sh (2)
294-345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrack
old_pdeath_service/new_pdeath_servicePIDs for cleanup.
old_pdeath_capture/new_pdeath_captureare added topids(lines 325, 334) butold_pdeath_serviceandnew_pdeath_service— the actualguardedscript processes — never are. If an assertion between lines 335-339 fails (i.e., exactly the scenario this test guards against: the new pair not replacing the old one), the test exits without these processes being reaped by the harness's cleanup trap, leaking apython3 -c 'signal.pause()'process. See consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/regressions-services.sh` around lines 294 - 345, Add both old_pdeath_service and new_pdeath_service to the pids cleanup list immediately after each PID is captured, alongside the corresponding capture PID. Keep the existing assertions and process lifecycle checks unchanged so the guarded service processes are reaped if any later assertion fails.
47-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate polling logic vs.
regressions-logs.sh.This helper duplicates the zombie-aware exit-polling loop added inline in
tests/regressions-logs.sh(lines 300-311) for the same class of "did the process die" check. Worth sharing viatests/lib.sh— see consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/regressions-services.sh` around lines 47 - 57, Move the shared zombie-aware process-exit polling logic from wait_for_process_exit in regressions-services.sh and the inline loop in regressions-logs.sh into a common helper in tests/lib.sh. Update both callers to reuse that helper while preserving the existing timeout, polling, and return-status behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/regressions-logs.sh`:
- Around line 300-311: Replace the duplicated kill-0 and zombie-state polling
loop with the existing wait_for_process_exit() helper from
regressions-services.sh, passing pdeath_child_pid and preserving the subsequent
failure check and error message.
- Around line 291-293: Update the PID setup around pdeath_capture_pid and
pdeath_child_pid so the child PID read from pdeath_pid_file is also appended to
the pids cleanup list. Preserve the existing kill -0 survival check and ensure
the foreground process is tracked before the test can exit on failure.
In `@tests/regressions-services.sh`:
- Around line 294-345: Add both old_pdeath_service and new_pdeath_service to the
pids cleanup list immediately after each PID is captured, alongside the
corresponding capture PID. Keep the existing assertions and process lifecycle
checks unchanged so the guarded service processes are reaped if any later
assertion fails.
- Around line 47-57: Move the shared zombie-aware process-exit polling logic
from wait_for_process_exit in regressions-services.sh and the inline loop in
regressions-logs.sh into a common helper in tests/lib.sh. Update both callers to
reuse that helper while preserving the existing timeout, polling, and
return-status behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 83b14d1e-6b8e-4073-b045-7a53f75843dd
📒 Files selected for processing (3)
guest/tx9-logstests/regressions-logs.shtests/regressions-services.sh
The all-source query asserted an early healthy-log event survived an unfiltered --tail 100 while the intentionally broken service kept crash-looping and appending records, so the check raced the flood. Filter with --grep before the tail so eviction of unrelated events cannot fail the assertion. Both service log assertions also relied on jq -e select() over a multi-record JSONL stream, whose exit status tracks the final record rather than whether any record matched. Slurp and assert any() instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
~/.config/hermes-box/services.d/service-<name>logshb up, reconciliation, pause/resume, status, doctor guidance, backup/import/upgrade persistence, and container shutdowntx9 logs --source service-<name>Important behavior
hb up/hb resumestill return failure when a configured service cannot be reconciledhb pausesynchronously stops services before backup state is checkpointedValidation
make syntaxmake lintmake testgo vet -buildvcs=false ./...go test -buildvcs=false ./...git diff --checkNo explicit build command was run, per repository instructions.
Closes #24
Note
Add supervision of portable custom box services via
tx9-servicesguest/tx9-services, a new OS-level supervisor for user-provided service drop-ins defined as executables inservices.d/. Services are restarted automatically, logged underservice-<name>sources, and managed with durable state and safe process identity checks.hb upandhb resumenow reconcile custom services before starting Executor and the gateway, failing if reconciliation fails.hb pausestops services, andhb reconcilestops services while quiesced but continues core reconciliation on service failure.hb servicesandhb services-reloadcommands that delegate to thetx9-serviceshelper.tx9-logscapture gains a parent-death SIGKILL shim (via re-exec) and precise startup failure reporting (process_start_failedevents for exec/setup errors). Log source filtering now acceptsservice-<name>patterns.tx9-servicesat$OPT/bin/tx9-serviceswith mode 0755, and the build context marks it executable.hb up/hb resumewill fail if service reconciliation fails, which is a new failure mode for those commands.Macroscope summarized 3e7089b.
Greptile Summary
This PR adds supervision for portable custom services inside a box. The main changes are:
hb, reconciliation, and container shutdown.service-<name>filtering.Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (6): Last reviewed commit: "fix: make service log regression asserti..." | Re-trigger Greptile