fix: terminate background tasks on restart / suspend - #106
Conversation
|
Warning Review limit reached
More reviews will be available in 29 minutes and 6 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe plugin now properly cleans up background state during lifecycle events. A new ChangesLifecycle cleanup for restart and suspend
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
grimmory.koplugin/main.lua (1)
171-179: ⚡ Quick winConsider clearing executor in
onPowerOff()for consistency.
onPowerOff()follows a similar pattern toonSuspend()(ending session, optionally syncing), but it does not clear the executor. For consistency with the new lifecycle cleanup approach and to prevent background tasks from running during power-off, consider addingself.executor:clear()here as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@grimmory.koplugin/main.lua` around lines 171 - 179, Add the same executor cleanup done in onSuspend() to onPowerOff(): inside function Grimmory:onPowerOff() after ending the reading session and before/after triggering onGrimmorySync(false), call self.executor:clear() to stop background tasks; update any related comments or ordering to match onSuspend() (i.e., self.reading_recorder:onSessionEnd(), then self.executor:clear(), then conditional self:onGrimmorySync(false)).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@grimmory.koplugin/main.lua`:
- Around line 142-147: The onRestart() method currently clears the scheduler and
executor but fails to close an active reading session; add a call to
self.reading_recorder:onSessionEnd() inside Grimmory:onRestart() (mirroring the
behavior in onSuspend() and onPowerOff()) so any active reading session is
properly ended before scheduler/executor are cleared.
- Around line 154-158: onSuspend currently starts a background sync via
self:onGrimmorySync(false) which uses self.executor:wrap(...) and then
immediately calls self.executor:clear(), causing the newly spawned subprocesses
to be killed; fix by performing the suspend sync synchronously instead of
wrapping it (call the executor's run path directly or await completion), or by
deferring/removing the immediate self.executor:clear() until after the sync
finishes; specifically update onSuspend to invoke the executor's blocking run
(use GrimmoryExecutor:run or the sync path used by onGrimmorySync when forcing
synchronous execution) or wait for the wrapped task to complete before calling
GrimmoryExecutor:clear(), and ensure running_subprocesses is not cleared while
the suspend sync is active.
---
Nitpick comments:
In `@grimmory.koplugin/main.lua`:
- Around line 171-179: Add the same executor cleanup done in onSuspend() to
onPowerOff(): inside function Grimmory:onPowerOff() after ending the reading
session and before/after triggering onGrimmorySync(false), call
self.executor:clear() to stop background tasks; update any related comments or
ordering to match onSuspend() (i.e., self.reading_recorder:onSessionEnd(), then
self.executor:clear(), then conditional self:onGrimmorySync(false)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a4cdc91-4ec3-4ca4-b064-d54d5d4f8387
📒 Files selected for processing (1)
grimmory.koplugin/main.lua
📜 Review details
🧰 Additional context used
🔀 Multi-repo context grimmory-tools/grimmory-docs
Findings (cross-repo search for KOReader sync / background tasks)
-
docs/integration/koreader.md — documents KOReader progress sync, auto-sync behavior, and instructs users how to enable automatic/background sync; implies user-visible expectations around stopping sync on restart/resume. [::grimmory-tools/grimmory-docs::docs/integration/koreader.md]
-
docs/tools/task-manager.md — describes background system tasks and notes some run asynchronously in the background (only one instance at a time). Relevant for user expectations around background task lifecycle. [::grimmory-tools/grimmory-docs::docs/tools/task-manager.md]
-
docs/tools/devices.md — describes KOReader integration and sync settings (enable toggle, API path, credentials) that tie to the sync behavior the plugin affects. [::grimmory-tools/grimmory-docs::docs/tools/devices.md]
No references were found to plugin lifecycle methods (onRestart/onSuspend) or to the specific executor/scheduler identifiers introduced/modified in the PR; I did not find any code in this docs repo that would directly call or depend on those new/changed methods. [::grimmory-tools/grimmory-docs::]
Conclusion: Documentation references KOReader background sync and background tasks (so the PR's intent aligns with documented expectations), but there are no direct cross-repo code consumers of the new Grimmory:onRestart / onSuspend behavior in this docs repository.
🔇 Additional comments (1)
grimmory.koplugin/main.lua (1)
142-147: ⚡ Quick winCheck whether KOReader dispatches
onRestart()to plugins
Current search ingrimmory.koplugin/main.luashows only thefunction Grimmory:onRestart()definition (no call sites in this repo), so it’s not established that KOReader actually invokesonRestart()on restart. Need KOReader core/framework confirmation (docs or source) thatonRestart()is a valid plugin lifecycle hook; otherwise this restart cleanup will never run.
| function Grimmory:onRestart() | ||
| logger:dbg("Restarting") | ||
|
|
||
| self.scheduler:clear() | ||
| self.executor:clear() | ||
| end |
There was a problem hiding this comment.
End the reading session on restart.
onRestart() should call self.reading_recorder:onSessionEnd() to properly close any active reading session, matching the pattern used in onSuspend() (line 152) and onPowerOff() (line 174). Without this, reading sessions may span across restart boundaries, leading to inconsistent or incomplete session data.
📝 Proposed fix to end reading session on restart
function Grimmory:onRestart()
logger:dbg("Restarting")
+ self.reading_recorder:onSessionEnd()
+
self.scheduler:clear()
self.executor:clear()
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function Grimmory:onRestart() | |
| logger:dbg("Restarting") | |
| self.scheduler:clear() | |
| self.executor:clear() | |
| end | |
| function Grimmory:onRestart() | |
| logger:dbg("Restarting") | |
| self.reading_recorder:onSessionEnd() | |
| self.scheduler:clear() | |
| self.executor:clear() | |
| end |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@grimmory.koplugin/main.lua` around lines 142 - 147, The onRestart() method
currently clears the scheduler and executor but fails to close an active reading
session; add a call to self.reading_recorder:onSessionEnd() inside
Grimmory:onRestart() (mirroring the behavior in onSuspend() and onPowerOff()) so
any active reading session is properly ended before scheduler/executor are
cleared.
| if self.settings:getSyncOnSuspend() then | ||
| self:onGrimmorySync(false) | ||
| end | ||
|
|
||
| self.executor:clear() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check GrimmoryExecutor implementation to verify wrap() vs run() behavior
# Find the executor module
fd -t f "executor.lua" grimmory.koplugin/
# Check the implementation of wrap and clear methods
rg -n -A 10 "function.*:wrap\(" grimmory.koplugin/
rg -n -A 10 "function.*:clear\(" grimmory.koplugin/
rg -n -A 10 "function.*:run\(" grimmory.koplugin/Repository: grimmory-tools/grimmory.koplugin
Length of output: 3369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show executor.lua implementations for wrap/clear/run with enough context
sed -n '60,170p' grimmory.koplugin/grimmory/executor.lua | cat -n
# Show main.lua onSuspend section and onGrimmorySync implementation call site(s)
# (use sed ranges around the referenced lines)
sed -n '120,220p' grimmory.koplugin/main.lua | cat -n
sed -n '420,520p' grimmory.koplugin/main.lua | cat -nRepository: grimmory-tools/grimmory.koplugin
Length of output: 8965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== executor.lua: running_subprocesses usage ==="
rg -n "running_subprocesses" grimmory.koplugin/grimmory/executor.lua
echo "=== executor.lua: tail of run() (track subprocess + returns) ==="
# Print a bit more around run() end
rg -n "function GrimmoryExecutor:run" grimmory.koplugin/grimmory/executor.lua
# show from start of run() through end (bounded)
sed -n '110,230p' grimmory.koplugin/grimmory/executor.lua | cat -n
echo "=== main.lua: onGrimmorySync definition and what it calls ==="
rg -n "function Grimmory:onGrimmorySync" grimmory.koplugin/main.lua
# show around the function
# (avoid dumping too much; this should cover the executor:wrap call and inner callback)
sed -n '430,540p' grimmory.koplugin/main.lua | cat -n
echo "=== main.lua: search where executor:run is used inside onGrimmorySync ==="
rg -n "executor:run\(" grimmory.koplugin/main.luaRepository: grimmory-tools/grimmory.koplugin
Length of output: 7524
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== main.lua: inspect sync_callback implementations around executor:run call sites ==="
# show enough context around executor:run at the known call sites
sed -n '230,310p' grimmory.koplugin/main.lua | cat -n
sed -n '310,390p' grimmory.koplugin/main.lua | cat -n
echo "=== main.lua: show executor:wrap call around onGrimmorySync(verbose) ==="
sed -n '280,370p' grimmory.koplugin/main.lua | cat -nRepository: grimmory-tools/grimmory.koplugin
Length of output: 9993
Fix sync-on-suspend being cancelled immediately in onSuspend()
In grimmory.koplugin/main.lua’s onSuspend(), when getSyncOnSuspend() is enabled it calls self:onGrimmorySync(false) and then immediately calls self.executor:clear().
onGrimmorySync() starts the sync via self.executor:wrap(...) (returns without waiting). The actual work runs in GrimmoryExecutor:run(), which spawns a subprocess and tracks its PID in running_subprocesses. GrimmoryExecutor:clear() then terminates all those subprocesses, so the sync-on-suspend operation can be interrupted before completion.
Defer executor:clear() until the suspend sync finishes, or run the suspend sync synchronously (use executor:run() directly/await completion for the suspend case) instead of wrap().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@grimmory.koplugin/main.lua` around lines 154 - 158, onSuspend currently
starts a background sync via self:onGrimmorySync(false) which uses
self.executor:wrap(...) and then immediately calls self.executor:clear(),
causing the newly spawned subprocesses to be killed; fix by performing the
suspend sync synchronously instead of wrapping it (call the executor's run path
directly or await completion), or by deferring/removing the immediate
self.executor:clear() until after the sync finishes; specifically update
onSuspend to invoke the executor's blocking run (use GrimmoryExecutor:run or the
sync path used by onGrimmorySync when forcing synchronous execution) or wait for
the wrapped task to complete before calling GrimmoryExecutor:clear(), and ensure
running_subprocesses is not cleared while the suspend sync is active.
|
Hmm.. this doesn't work the way I'd like it to based on testing. It'd be better to put the executor into a "shutdown" state that rejects new tasks & waits for the current task to complete. |
1 participant
on suspend or restart we should stop background tasks - otherwise unexpected behaviors may occur
on restart, it will lead to a background task that is completely un-controlled; on suspend and resume, there's undefined behavior that can happen
fixes #95
Summary by CodeRabbit