fix: avoid error log when daemon retries "exec" tasks on startup #714
Conversation
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
Exec tasks are not retryable (by design). However, if the Pebble daemon is hard-killed when an exec task is running, then when the daemon is restarted, the state engine tries to run that task (which was in Doing status) again, and returns an error from the task handler. Simply log this at debug level instead of returning a task error, as it's not an error. Before: 2025-10-20T22:29:41.286Z [pebble] Started daemon. 2025-10-20T22:29:41.287Z [pebble] Change 1 task (Execute command "sleep") failed: internal error: cannot get exec setup object for task "1" 2025-10-20T22:29:41.287Z [pebble] POST /v1/services 982.809µs 400 (http+unix) 2025-10-20T22:29:41.287Z [pebble] Cannot start default services: no default services After: 2025-10-20T22:33:28.862Z [pebble] Started daemon. 2025-10-20T22:33:28.865Z [pebble] POST /v1/services 2.696031ms 400 (http+unix) 2025-10-20T22:33:28.866Z [pebble] Cannot start default services: no default services Fixes canonical#587
tonyandrewmeyer
approved these changes
Oct 20, 2025
| setupObj := st.Cached(execSetupKey{task.ID()}) | ||
| st.Unlock() | ||
| setup, ok := setupObj.(*execSetup) | ||
| if !ok || setup == nil { |
There was a problem hiding this comment.
If I'm understanding things correctly, setupObj will be nil in the case we're interested in, and this will cover that case because nil can't be converted to an execSetup.
Would it be tighter to check if setupObj is nil in case there are other issues with the type check (that should error out)? Or do we want to just log in either case anyway?
There was a problem hiding this comment.
There are actually four cases:
- missing from cache (
!ok) - in cache, but not an
*execSetuptype (!ok) - in cache, but a nil
*execSetupvalue (ok && setup==nil) - in cache and a non-nil
*execSetupvalue (ok && setup!=nil)
So this branch will be executed on 1, 2, or 3. Which I think is fine. Realistically, only 1 will happen because we've only put non-nil *execSetups in the map. So we could handle 2 and 3 more "harshly", but I don't think it hurts to handle them the same way.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Exec tasks are not retryable (by design). However, if the Pebble daemon is hard-killed when an exec task is running, then when the daemon is restarted, the state engine tries to run that task (which was in Doing status) again, and returns an error from the task handler.
Simply log this at debug level instead of returning a task error, as it's not an error.
Before:
After:
To repro or test:
pebble runin one terminalpebble exec sleep 1000in another terminalkill -9 <pebble_run_pid>pebble runand (before the fix) you'll see the error log on startupFixes #587