fix: Allow archived sketches with failed timelines to be unarchived #3656
+616
−67
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
Collaborator
Author
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request correctly implements the ability to force-delete archived sketches. The changes are applied consistently across the API client, the server-side resource, and the end-to-end tests. The logic is sound, preventing soft-deletion of archived sketches while allowing force-deletion, and the error messages have been updated to guide the user. The accompanying test modifications properly validate the new functionality. I've added a couple of comments regarding outdated docstrings that should be updated to reflect these changes.
* Server-side (`timesketch/api/v1/resources/archive.py`): Modified the unarchive logic to allow
sketches with failed or processing timelines to be unarchived. Instead of aborting with a 500
error, the system now logs a warning and proceeds to open all healthy indices. This enables
users to recover the sketch and then fix or delete the problematic timelines.
* `test_unarchive_sketch_with_failed_timeline`: Updated the docstring to clarify that the test
verifies unarchiving is allowed for sketches with failed timelines to enable recovery and subsequent
fixing.
* `test_unarchive_sketch_with_mixed_states`: Enhanced the docstring to explain that the test confirms
robustness against multiple problematic timeline states (fail and processing) during unarchive.
jaegeral
commented
Jan 6, 2026
Collaborator
Author
|
/gemini review |
jaegeral
changed the title
force_delete to also delete archived sketches
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses an issue preventing sketches with timelines in 'fail' or 'processing' states from being unarchived, which is a great improvement. However, the pull request title and description are misleading. They claim that force_delete is now allowed on archived sketches, but the updated tests confirm the opposite: an archived sketch cannot be force-deleted and must be unarchived first. Please update the title and description to accurately reflect the changes, which are mainly about improving the unarchiving logic.
Collaborator
Author
|
/gemini summary |
Contributor
Summary of ChangesThis pull request resolves an issue where archived sketches containing failed timelines could not be unarchived, thereby preventing their proper management and deletion. The changes enable administrators to unarchive these sketches, allowing them to address the underlying timeline issues and ultimately facilitate the removal of the sketches. The update involves a modification to the unarchive API logic and the addition of new tests to ensure robustness across different timeline states. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
|
jkppr
requested changes
Jan 6, 2026
Collaborator
jkppr
left a comment
jkppr
left a comment
There was a problem hiding this comment.
First round. Please take a look below.
| "Timeline '%s' status set to 'ready'.", | ||
| timeline.id, | ||
| ) | ||
| # Only set timeline to ready if its index was successfully opened |
Collaborator
There was a problem hiding this comment.
I ran into the following issue:
- A searchindex can be shared between a "ready" and a "fail" timeline. E.g. when you have 1 Plaso file that works and 1 that does not.
- Somehow this situation leads to a "fail" timeline after unarchive where it was "ready" before.
Steps to reproduce:
- Create new Sketch
- Upload a working timeline
- Archive & unarchive work without issue.
- Now add a failing timeline.
- Try to archive the sketch will be prevented (as expected)
- Delete the broken timeline
- Archive sketch (now works)
- Unarchive sketch. But now the timeline that was working before is in a fail state! This should not happen.
Collaborator
Author
There was a problem hiding this comment.
Ok I tried to reproduce it and it should be fixed now.
Collaborator
There was a problem hiding this comment.
I'm still able to reproduce this issue. Again here are the exact steps I did:
- Create new Sketch
- Upload a working Plaso timeline
- Upload a broken Plaso timeline (I have shared the one I use via DM)
- Delete the broken timeline
- Archive the sketch
- Unarchive sketch. But now the timeline that was working before is in a fail state! This should not happen.
Collaborator
Author
There was a problem hiding this comment.
ok will try again, sorry for wasting your time :-/
jkppr
requested changes
Jan 8, 2026
Collaborator
jkppr
left a comment
jkppr
left a comment
There was a problem hiding this comment.
Still having the same issue.
| "Timeline '%s' status set to 'ready'.", | ||
| timeline.id, | ||
| ) | ||
| # Only set timeline to ready if its index was successfully opened |
Collaborator
There was a problem hiding this comment.
I'm still able to reproduce this issue. Again here are the exact steps I did:
- Create new Sketch
- Upload a working Plaso timeline
- Upload a broken Plaso timeline (I have shared the one I use via DM)
- Delete the broken timeline
- Archive the sketch
- Unarchive sketch. But now the timeline that was working before is in a fail state! This should not happen.
Collaborator
Author
|
jkppr
approved these changes
Jan 8, 2026
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.
This pull request resolves an issue where
archivedsketches containing failed timelines could not be unarchived, thereby preventing their proper management and deletion. The changes enable administrators tounarchivethese sketches, allowing them to address the underlying timeline issues and ultimately facilitate the removal of the sketches. The update involves a modification to the unarchive API logic and the addition of new tests to ensure robustness across different timeline states.It changes how to tackle unarchiving indexes that have a OpenSearchIndex that is not found. I tiwll continue to unarchive but set the corresponding Timeline etc to
failso the Sketch is not in a blocked mode.