Skip to content

Fix periodic config update crash under Twisted asyncio reactor - #69

Merged
sgallagher merged 2 commits into
fedora-eln:mainfrom
sgallagher:asyncio_twisted_config_update
Sep 4, 2026
Merged

Fix periodic config update crash under Twisted asyncio reactor#69
sgallagher merged 2 commits into
fedora-eln:mainfrom
sgallagher:asyncio_twisted_config_update

Conversation

@sgallagher

@sgallagher sgallagher commented Sep 3, 2026

Copy link
Copy Markdown
Member

Fix periodic config update crash under Twisted asyncio reactor.

LoopingCall invoked update_config() directly as an async coroutine, which
Twisted ran through its deferred/coroutine bridge. When get_distro_packages()
retried via tenacity during dynamic config reload, await on tenacity's
asyncio.sleep() failed with RuntimeError: await wasn't used with future
because the retry slept outside a proper asyncio task.

Add schedule_update_config() as the LoopingCall entry point; it returns
Deferred.fromFuture(asyncio.ensure_future(update_config())) so the full
update path (including Content Resolver fetches and tenacity retries) runs
in a dedicated asyncio task, matching the pattern used in web.py.

Co-authored-by: Cursor cursoragent@cursor.com

Catch unexpected errors in update_config() so LoopingCall keeps running.

Periodic config reload could raise outside the known UnknownRefError and
ConfigError paths (for example tenacity/asyncio integration bugs or network
faults during Content Resolver fetches). Those exceptions propagated out of
update_config(), producing "Unhandled error in Deferred" and leaving the
config LoopingCall in a failed state with no further scheduled updates.

Wrap the body of update_config() in a catch-all handler that logs the
traceback, records a critical message that the timer will retry, and returns
normally so schedule_update_config() completes and the LoopingCall continues
on its interval.

Summary by CodeRabbit

  • Bug Fixes
    • Improved resilience of configuration updates by catching failures, logging them, and reporting a retry delay.
    • Configuration updates now run asynchronously, helping periodic daemon tasks continue operating when an update encounters an error.

@sgallagher sgallagher self-assigned this Sep 3, 2026
@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Review Change Stack

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 6a282956-ebce-4725-9222-c2eae2ca3957

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 22e472fe-0066-4c96-8973-5e6b70e18e84

📥 Commits

Reviewing files that changed from the base of the PR and between 7dfa171 and b4bb25b.

📒 Files selected for processing (2)
  • elnbuildsync/config/__init__.py
  • elnbuildsync/daemon.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The configuration updater now logs unexpected failures, runs through a dedicated asyncio task wrapped in a Twisted Deferred, and uses this scheduling entry point for periodic daemon updates.

Changes

Configuration update scheduling

Layer / File(s) Summary
Configuration update error handling
elnbuildsync/config/__init__.py
update_config catches unexpected exceptions, logs the failure, and reports the retry delay.
Async update entry point
elnbuildsync/config/__init__.py, elnbuildsync/daemon.py
schedule_update_config wraps update_config in an asyncio task and a Twisted Deferred. The daemon uses it for periodic updates.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to b4bb2

Periodic configuration updates now run in an asyncio task and log unexpected failures so later update attempts continue. No concrete merge-blocking risk remains.

Sequence Diagram(s)

sequenceDiagram
  participant LoopingCall
  participant schedule_update_config
  participant asyncio
  participant update_config
  participant Deferred
  LoopingCall->>schedule_update_config: Start periodic update
  schedule_update_config->>asyncio: Create task for update_config
  asyncio->>update_config: Run configuration update
  schedule_update_config->>Deferred: Wrap task future
  Deferred-->>LoopingCall: Return Deferred
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preventing periodic configuration update crashes under the Twisted asyncio reactor.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@yselkowitz yselkowitz linked an issue Sep 3, 2026 that may be closed by this pull request
@sgallagher
sgallagher force-pushed the asyncio_twisted_config_update branch from b4bb25b to 07f9f99 Compare September 4, 2026 13:20
sgallagher and others added 2 commits September 4, 2026 09:35
LoopingCall invoked update_config() directly as an async coroutine, which
Twisted ran through its deferred/coroutine bridge. When get_distro_packages()
retried via tenacity during dynamic config reload, await on tenacity's
asyncio.sleep() failed with RuntimeError: await wasn't used with future
because the retry slept outside a proper asyncio task.

Add schedule_update_config() as the LoopingCall entry point; it returns
Deferred.fromFuture(asyncio.ensure_future(update_config())) so the full
update path (including Content Resolver fetches and tenacity retries) runs
in a dedicated asyncio task, matching the pattern used in web.py.

Fixes: fedora-eln#68

Co-authored-by: Cursor <cursoragent@cursor.com>
Periodic config reload could raise outside the known UnknownRefError and
ConfigError paths (for example tenacity/asyncio integration bugs or network
faults during Content Resolver fetches). Those exceptions propagated out of
update_config(), producing "Unhandled error in Deferred" and leaving the
config LoopingCall in a failed state with no further scheduled updates.

Wrap the body of update_config() in a catch-all handler that logs the
traceback, records a critical message that the timer will retry, and returns
normally so schedule_update_config() completes and the LoopingCall continues
on its interval.
@sgallagher
sgallagher force-pushed the asyncio_twisted_config_update branch from 07f9f99 to 4565e57 Compare September 4, 2026 13:35
@sgallagher
sgallagher merged commit f84b8ff into fedora-eln:main Sep 4, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dynamic config is not updating

1 participant