fix(worker): force worker mode onto main thread#1392
Open
fyzanshaik-atlan wants to merge 4 commits intomainfrom
Open
fix(worker): force worker mode onto main thread#1392fyzanshaik-atlan wants to merge 4 commits intomainfrom
fyzanshaik-atlan wants to merge 4 commits intomainfrom
Conversation
fyzanshaik-atlan
requested review from
OnkarVO7,
cmgrote and
louisnow
as code owners
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
📜 Docstring Coverage ReportRESULT: PASSED (minimum: 30.0%, actual: 80.2%) Detailed Coverage Report |
Contributor
Author
|
@sdk-review |
Contributor
|
🔄 SDK Review starting (review) — ~10 min. Watch live progress |
Contributor
📦 Trivy Vulnerability Scan Results
Report SummaryCould not generate summary table (data length mismatch: 9 vs 8). Scan Result Detailsrequirements.txtVulnerabilities
uv.lockVulnerabilities
|
Contributor
📦 Trivy Secret Scan Results
Report SummaryCould not generate summary table (data length mismatch: 9 vs 8). Scan Result Detailsrequirements.txtuv.lock |
Collaborator
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
Contributor
|
🛠 Full Test Coverage Report: https://k.atlan.dev/coverage/application-sdk/pr/1392 |
Member
|
v3 relevance check: ✅ Still relevant — worker mode main thread fix. Please rebase on latest main (significant changes merged recently) and verify the fix still applies. |
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
4 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.
Summary
This hardens the v2 SDK worker startup path for apps that still use the older manual
setup_workflow() -> start_worker() -> setup_server()entrypoint pattern.When
APPLICATION_MODE=WORKER, the SDK now forcesWorker.start()to run withdaemon=Falseeven if the caller passeddaemon=True.Why
We hit a production outage on
cbrands-povafteratlan-fabric-appupgraded to SDK2.8.xand continued using a custommain.pythat always calledstart_worker()and then server startup. On worker pods,ATLAN_APP_HTTP_PORT=-1caused the process to fall through into server startup and crash before the pod could stay healthy.The immediate outage was app-side, but v2 makes this class of bug too easy:
BaseApplication.start()mode pathThis PR adds an SDK-side safety net for v2 without changing the behavior of apps that already use
BaseApplication.start()correctly.Behavior change
APPLICATION_MODE=WORKER+Worker.start(daemon=True)now logs a warning and behaves asdaemon=FalseLOCALbehavior is unchangedThis means worker pods keep the worker on the main thread and do not fall through into later server startup code in older/manual entrypoints like Fabric's.
Scope
This is a v2 hardening fix only.
refactor-v3already handles startup correctly via the unifiedapplication_sdk.mainentrypoint with explicitworker/handler/combinedmodes, so this failure mode should not exist there.Validation
uv run pytest tests/unit/test_worker.py -quv run pytest tests/unit/application/test_application.py -quv run pre-commit run --files application_sdk/worker.py tests/unit/test_worker.pyTests added
APPLICATION_MODE=WORKERprevents daemon thread startup even if the caller requesteddaemon=TrueFollow-up
Apps on v2 should consume the patched SDK release rather than carrying app-local
APP_PORT == -1/asyncio.Event().wait()workarounds.