unregister from controller only on proper exit#659
unregister from controller only on proper exit#659mangelajo merged 1 commit intojumpstarter-dev:mainfrom
Conversation
WalkthroughExporter.stop gained a new parameter to record an intent to unregister; CLI signal handlers now call stop(..., should_unregister=True). Unregistration during cleanup is performed only when the exporter was registered and the new internal _unregister flag is set. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant OS as OS Signal
participant CLI as CLI Runner
participant EXP as Exporter
participant CTRL as Controller
OS->>CLI: SIGHUP/SIGTERM
CLI->>EXP: stop(wait_for_lease_exit=?, should_unregister=true)
alt immediate stop (wait_for_lease_exit=false)
EXP->>EXP: set _unregister = true
EXP->>EXP: cancel tasks / cleanup
opt registered && _unregister
EXP->>CTRL: unregister()
CTRL-->>EXP: ok
end
EXP-->>CLI: stopped
else lease-exit path (wait_for_lease_exit=true)
EXP->>EXP: wait for lease exit
EXP->>EXP: stop(..., should_unregister=true)
opt registered && _unregister
EXP->>CTRL: unregister()
CTRL-->>EXP: ok
end
EXP-->>CLI: stopped
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-cli/jumpstarter_cli/run.py(1 hunks)packages/jumpstarter/jumpstarter/exporter/exporter.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#584
File: packages/jumpstarter/jumpstarter/client/grpc.py:38-54
Timestamp: 2025-08-18T07:13:37.619Z
Learning: User michalskrivanek prefers inline code over small helper function extractions when the abstraction doesn't significantly improve readability or reduce complexity. They value straightforward, readable code over reducing minor duplication.
🧬 Code graph analysis (1)
packages/jumpstarter-cli/jumpstarter_cli/run.py (1)
packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
stop(46-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
🔇 Additional comments (3)
packages/jumpstarter-cli/jumpstarter_cli/run.py (1)
64-64: Confirm intent: unregister on all signals (not just SIGHUP).You pass should_unregister=True for every signal, including SIGINT/SIGTERM/SIGQUIT. If that’s the intended “proper exit” definition, LGTM. If you only want unregister on specific signals, gate it accordingly.
packages/jumpstarter/jumpstarter/exporter/exporter.py (2)
69-85: Cleanup logic looks solid.Conditional unregister + 10s timeout with shielded close is good for reliability.
205-207: Good: unregister only after lease exit path.This ensures graceful SIGHUP flow: defer cancellation until not leased, then request unregister.
exceptions raised duing serve() and lease changes keep the exporter registered with controller to avoid quick online/offline transitions since the internal logic restarts the jumpstarter child process within few seconds. On termination signal the unregistration still happens as the parent process also terminates. Note it still causes offline/online bounce if it is immediatelly restarted via systemd service for example.
michalskrivanek
force-pushed
the
keep-registered
branch
from
9068ace to
8587858
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter/jumpstarter/exporter/exporter.py (2)
46-52: Make stop() params keyword‑only and typed to avoid call‑site ambiguity.Prevents accidental positional mix‑ups now that a second boolean was added.
Apply:
- def stop(self, wait_for_lease_exit=False, should_unregister=False): + def stop(self, *, wait_for_lease_exit: bool = False, should_unregister: bool = False) -> None:(Non‑breaking for current call sites that already use keywords.) Based on learnings.
69-85: Differentiate timeout vs error; clear state post‑unregister.Two small tweaks improve observability and state hygiene:
- Log when the 10s timeout triggers (distinct from exceptions).
- Mark
registered = Falseafter a successfulUnregisterto avoid stale state if the instance is reused.Apply:
- with move_on_after(10): # 10 second timeout + with move_on_after(10) as cs: # 10s timeout channel = await self.channel_factory() try: controller = jumpstarter_pb2_grpc.ControllerServiceStub(channel) await controller.Unregister( jumpstarter_pb2.UnregisterRequest( reason="Exporter shutdown", ) ) logger.info("Controller unregistration completed successfully") + self.registered = False finally: with CancelScope(shield=True): await channel.close() + if cs.cancel_called: + logger.warning("Controller unregistration timed out after 10s")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-cli/jumpstarter_cli/run.py(1 hunks)packages/jumpstarter/jumpstarter/exporter/exporter.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-cli/jumpstarter_cli/run.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#584
File: packages/jumpstarter/jumpstarter/client/grpc.py:38-54
Timestamp: 2025-08-18T07:13:37.619Z
Learning: User michalskrivanek prefers inline code over small helper function extractions when the abstraction doesn't significantly improve readability or reduce complexity. They value straightforward, readable code over reducing minor duplication.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
🔇 Additional comments (3)
packages/jumpstarter/jumpstarter/exporter/exporter.py (3)
41-41: Explicit unregister gate — good change.Private
_unregistercleanly separates transient restarts from intentional shutdowns and prevents controller flapping.
56-58: Order of operations LGTM.Setting
_unregisterbefore cancelling the task group ensures the intent survives cancellation.
206-207: AllExporter.stopcalls explicitly useshould_unregister; no positional booleans remain.
|
Successfully created backport PR for |
2 participants
exceptions raised duing serve() and lease changes keep the exporter registered with controller to avoid quick online/offline transitions since the internal logic restarts the jumpstarter child process within few seconds. On termination signal the unregistration still happens as the parent process also terminates. Note it still causes offline/online bounce if it is immediatelly restarted via systemd service for example.
Summary by CodeRabbit