-
Notifications
You must be signed in to change notification settings - Fork 36
fix(cli): stop reporting hook waits and status changes that never happened #1039
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,6 +54,9 @@ | |
| # Refresh token when less than this many seconds remain | ||
| _TOKEN_REFRESH_THRESHOLD_SECONDS = 120 | ||
|
|
||
| # Total time to wait for the beforeLease hook. | ||
| _HOOK_TIMEOUT: float = 300.0 | ||
|
|
||
|
|
||
| def _run_shell_only(lease, config, command, path: str, motd: str | None = None) -> int: | ||
| """Run just the shell command without log streaming.""" | ||
|
|
@@ -336,12 +339,24 @@ async def _run_shell_with_lease_async(lease, exporter_logs, config, command, can | |
| # Wait for beforeLease hook to complete while logs are streaming | ||
| # This allows hook output to be displayed in real-time | ||
| # Uses non-blocking polling instead of streaming for robustness | ||
| logger.info("Waiting for beforeLease hook to complete...") | ||
|
|
||
| # Wait for LEASE_READY or hook failure using background monitor | ||
| result = await monitor.wait_for_any_of( | ||
| [ExporterStatus.LEASE_READY, ExporterStatus.BEFORE_LEASE_HOOK_FAILED], timeout=300.0 | ||
| ) | ||
| targets = [ExporterStatus.LEASE_READY, ExporterStatus.BEFORE_LEASE_HOOK_FAILED] | ||
|
|
||
| # The monitor reports no status until its first poll, so | ||
| # wait for that observation before saying anything: | ||
| # attaching to a lease that is already LEASE_READY must | ||
| # not claim to be waiting on a hook that already ran. | ||
| # Waiting on the observation rather than a fixed settle | ||
| # time keeps that true on a slow or distant link, where a | ||
| # wall-clock probe would expire before the first answer. | ||
| deadline = anyio.current_time() + _HOOK_TIMEOUT | ||
| await monitor.wait_for_first_observation(timeout=_HOOK_TIMEOUT) | ||
| result = monitor.current_status if monitor.current_status in targets else None | ||
|
|
||
| if result is None and not monitor.connection_lost: | ||
| logger.info("Waiting for beforeLease hook to complete...") | ||
| result = await monitor.wait_for_any_of( | ||
| targets, timeout=max(0.0, deadline - anyio.current_time()) | ||
| ) | ||
|
Comment on lines
+352
to
+359
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The return value of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. Tracing through the code, here's the concrete scenario:
So if
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IDK, since this is mostly aesthetic. I'd propose that we leave this alone, and eventually tackle it when we have a FSM in rust , python or whatever :D WDYT?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the FSM work scheduled already or somewhere deeply nested in the backlog? I think if we implement the FSM soonish, its okay to leave it as is. |
||
|
|
||
| if result == ExporterStatus.BEFORE_LEASE_HOOK_FAILED: | ||
| reason = monitor.status_message or "beforeLease hook failed" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -995,3 +995,62 @@ async def test_long_after_hook_survives_deadline_exceeded(self) -> None: | |
|
|
||
| assert result == ExporterStatus.AVAILABLE | ||
| assert monitor.connection_lost is False | ||
|
|
||
|
|
||
| class TestWaitForFirstObservation: | ||
| async def test_waits_out_a_slow_first_answer(self) -> None: | ||
| """A distant or loaded exporter can take longer than any settle time. | ||
|
|
||
| The caller has to know whether a status has been observed, not whether | ||
| some number of seconds has passed, so the wait tracks the answer. | ||
| """ | ||
|
|
||
| class SlowStub(MockExporterStub): | ||
| async def GetStatus(self, request, timeout=None): | ||
| await anyio.sleep(0.4) | ||
| return await super().GetStatus(request, timeout=timeout) | ||
|
|
||
| stub = SlowStub([create_status_response(ExporterStatus.LEASE_READY, version=1)]) | ||
| monitor = StatusMonitor(stub, poll_interval=0.05) | ||
|
|
||
| async with anyio.create_task_group() as tg: | ||
| await monitor.start(tg) | ||
| # Shorter than the answer takes: no observation yet. | ||
| assert await monitor.wait_for_first_observation(timeout=0.1) is False | ||
| assert monitor.current_status is None | ||
| # Long enough: the answer lands and is reported as observed. | ||
| assert await monitor.wait_for_first_observation(timeout=2.0) is True | ||
| assert monitor.current_status == ExporterStatus.LEASE_READY | ||
| await monitor.stop() | ||
|
|
||
| async def test_returns_once_the_first_answer_lands(self) -> None: | ||
| stub = MockExporterStub([create_status_response(ExporterStatus.AVAILABLE, version=1)]) | ||
| monitor = StatusMonitor(stub, poll_interval=0.05) | ||
|
|
||
| async with anyio.create_task_group() as tg: | ||
| await monitor.start(tg) | ||
| assert await monitor.wait_for_first_observation(timeout=2.0) is True | ||
| assert monitor.current_status == ExporterStatus.AVAILABLE | ||
| await monitor.stop() | ||
|
|
||
| async def test_does_not_block_when_get_status_is_unsupported(self) -> None: | ||
| """LEASE_READY is assumed without polling, so there is nothing to wait for.""" | ||
| monitor = StatusMonitor(MockExporterStub([]), poll_interval=0.05, get_status_unsupported=True) | ||
|
|
||
| async with anyio.create_task_group() as tg: | ||
| await monitor.start(tg) | ||
| assert await monitor.wait_for_first_observation(timeout=2.0) is True | ||
| assert monitor.current_status == ExporterStatus.LEASE_READY | ||
| await monitor.stop() | ||
|
|
||
| async def test_releases_waiters_when_the_monitor_stops(self) -> None: | ||
| """A stopped monitor will never observe anything, so waiters must not | ||
| sit out their whole timeout.""" | ||
| stub = MockExporterStub([AioRpcError(StatusCode.UNAVAILABLE, None, None)]) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using |
||
| monitor = StatusMonitor(stub, poll_interval=0.05) | ||
|
|
||
| async with anyio.create_task_group() as tg: | ||
| await monitor.start(tg) | ||
| await monitor.stop() | ||
| with anyio.fail_after(2.0): | ||
| assert await monitor.wait_for_first_observation(timeout=30.0) is False | ||
Uh oh!
There was an error while loading. Please reload this page.