Open
Conversation
Garavitey
requested review from
AtMrun,
firecast and
inishchith
as code owners
📜 Docstring Coverage ReportRESULT: PASSED (minimum: 30.0%, actual: 76.0%) Detailed Coverage Report |
📦 Trivy Vulnerability Scan Results
Report Summary
Scan Result Details✅ No vulnerabilities found during the scan for |
|
🛠 Docs available at: https://k.atlan.dev/application-sdk/event-push-bug-fix |
📦 Trivy Secret Scan Results
Report Summary
Scan Result Details✅ No secrets found during the scan for |
Collaborator
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
|
🛠 Full Test Coverage Report: https://k.atlan.dev/coverage/application-sdk/pr/893 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
application_sdk/clients/temporal.py
Outdated
| # Recalculate refresh interval each time in case token expiry changes | ||
| refresh_interval = self.auth_manager.calculate_refresh_interval() | ||
| # Publish token refresh event | ||
| await self._publish_token_refresh_event() |
There was a problem hiding this comment.
Bug: Token refresh event published before actual refresh occurs
The _publish_token_refresh_event() call was moved to execute before the actual token refresh and sleep. This causes the event to be published with stale data: token_expiry_time and time_until_expiry reflect the old token's values, and refresh_timestamp is set before the refresh actually occurs. On the first loop iteration, the event fires before any refresh has happened at all. The event model explicitly documents these fields as representing the "new token" and "when the refresh occurred," which no longer matches reality.
Contributor
Author
There was a problem hiding this comment.
that is the intention
Collaborator
There was a problem hiding this comment.
I don't get it, why before?
Contributor
Author
There was a problem hiding this comment.
We need to send the this event before the function goes to sleep, this is the reason why we dont see "active" state as soon as the worker starts, if we send it before then the function gets sent and we should be able to see the active state in the ui as soon as the worker is up.
Collaborator
There was a problem hiding this comment.
Ahh, hmm, no way to send first event elsewhere on startup?
Contributor
Author
There was a problem hiding this comment.
why send one single event from a different place? redundancy in code
There was a problem hiding this comment.
Bug: Test assertions don't match updated log messages
The implementation at _publish_heartbeat_event was updated to log "Published heartbeat event" and "Failed to publish heartbeat event", but the test assertions still check for the old messages "Published token refresh event" and "Failed to publish token refresh event". These tests will fail because the expected strings don't match the actual log output.
tests/unit/clients/test_temporal_client.py#L384-L385
application-sdk/tests/unit/clients/test_temporal_client.py
Lines 384 to 385 in f35a5de
tests/unit/clients/test_temporal_client.py#L416-L417
application-sdk/tests/unit/clients/test_temporal_client.py
Lines 416 to 417 in f35a5de
tests/unit/clients/test_temporal_client.py#L446-L449
application-sdk/tests/unit/clients/test_temporal_client.py
Lines 446 to 449 in f35a5de
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog
Additional context (e.g. screenshots, logs, links)
Checklist
Copyleft License Compliance
Note
Switches from publishing a token refresh event to a heartbeat event and emits it at the start of each refresh cycle; updates models, client, and tests accordingly.
WorkerTokenRefreshEventDatatoWorkerHeartbeatEventDatainapplication_sdk/events/models.py.application_sdk/clients/temporal.py):_publish_token_refresh_eventwith_publish_heartbeat_eventthat publishes heartbeat data while keepingevent_nameastoken_refresh.tests/unit/clients/test_temporal_client.py):Written by Cursor Bugbot for commit c368892. This will update automatically on new commits. Configure here.