Skip to content

More improvements #161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 26 commits into from
Aug 27, 2025
Merged

More improvements #161

merged 26 commits into from
Aug 27, 2025

Conversation

@marcelklehr
Copy link
Member

No description provided.

This comment was marked as outdated.

Sign in to view

@marcelklehr marcelklehr force-pushed the fix/more-improvements branch from fa55375 to 861b9ea Compare August 7, 2025 10:55
kyteinsky
kyteinsky reviewed Aug 7, 2025
View reviewed changes
@marcelklehr marcelklehr requested a review from julien-nc as a code owner August 7, 2025 12:58
Copilot AI reviewed Aug 7, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements several improvements to the context chat application, focusing on enhanced queue processing prioritization, better error handling, and duplicate event cleanup. The changes prioritize new files over updates in the indexing queue and add comprehensive exception handling throughout various service layers.

  • Adds prioritization logic to process new files before updates in the queue system
  • Implements comprehensive exception handling across service layers with proper logging
  • Introduces a migration step to remove duplicate filesystem events

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/psalm-baseline.xml Updates psalm version and adds baseline for undefined class warning
src/components/ViewAdmin.vue Updates admin view text to show unseen files count alongside sent files
lib/Settings/AdminSettings.php Adds queued new files count to admin statistics
lib/Service/QueueService.php Implements prioritization logic and new file counting functionality
lib/Service/FsEventService.php Adds exception handling for file access operations
lib/Service/FsEventScheduler.php Improves error handling in owner retrieval logic
lib/Service/ActionScheduler.php Adds exception documentation and imports
lib/Public/ContentManager.php Wraps action service calls with exception handling
lib/Migration/RemoveDuplicateFsEvents.php Adds migration step to clean up duplicate filesystem events
lib/Listener/FileListener.php Improves error message clarity in filesystem event listener
lib/Db/QueueMapper.php Adds new file filtering and counting capabilities
lib/Db/QueueContentItemMapper.php Adds ordering to queue retrieval queries
lib/Db/QueueActionMapper.php Adds ordering to queue retrieval queries
lib/Db/FsEventMapper.php Implements duplicate prevention logic and adds ordering
lib/Command/Statistics.php Adds new file queue statistics to command output
lib/BackgroundJobs/FileSystemListenerJob.php Groups events by user and adds memory management

@marcelklehr marcelklehr force-pushed the fix/more-improvements branch from 5fb6f45 to aa387da Compare August 7, 2025 13:01
julien-nc
julien-nc approved these changes Aug 8, 2025
View reviewed changes
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good code wise. I'm not deep enough in the app so I can't judge the logic.

kyteinsky
kyteinsky reviewed Aug 8, 2025
View reviewed changes
nickvergessen
nickvergessen reviewed Aug 8, 2025
View reviewed changes
marcelklehr and others added 16 commits August 19, 2025 15:22
@marcelklehr @kyteinsky
fix(QueueMappers): Ensure the queues are FIFO, prioritize
7da349f 
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@marcelklehr @kyteinsky
fix: improve error handling
2f1b7dd 
Signed-off-by: Marcel Klehr <mklehr@gmx.net>

# Conflicts:
#	lib/Public/ContentManager.php
#	lib/Service/FsEventService.php
@marcelklehr @kyteinsky
fix(FsEventMapper): Prevent inserting duplicates
dc3ef71 
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@marcelklehr @kyteinsky
fix(FileSystemListenerJob): Prevent OOM
5f2ebbb 
by working per user and tearing down FS in between

Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@marcelklehr @kyteinsky
fix: Fix psalm issues
ba1d05c 
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@marcelklehr @kyteinsky
fix(FileSystemListenerJob) Use SetupManager to tear down fs
cf20fa7 
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@marcelklehr @kyteinsky
feat: Add stats entry for queued unseen files
be6b2fa 
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@marcelklehr @kyteinsky
fix: Add RemoveDuplicateFsEvents repair step
3469a42 
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@marcelklehr @kyteinsky
fix: psalm issues
dd95303 
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@marcelklehr @kyteinsky
tests: Kill workers manually after each test case
7313a30 
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@marcelklehr @kyteinsky
fix: Add comments to RemoveDuplicateFsEvents
1c7b87e 
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@marcelklehr @kyteinsky
fix(IndexerJob): Make sure too large files are removed from queue
ecb847c 
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@marcelklehr @kyteinsky
fix(REUSE): Add copyright to psalm-baseline.xml
e4af538 
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@kyteinsky
fix: consider only new files for first indexing complete check
c4abdff 
Signed-off-by: Anupam Kumar <kyteinsky@gmail.com>
@kyteinsky
wrap remove duplicate fs events db query in a transaction
e69479b 
Signed-off-by: Anupam Kumar <kyteinsky@gmail.com>
@kyteinsky
fix: adjustments to admin stats page
8da95af 
Signed-off-by: Anupam Kumar <kyteinsky@gmail.com>
@kyteinsky kyteinsky force-pushed the fix/more-improvements branch from 086bb78 to d9e9382 Compare August 19, 2025 09:53
@kyteinsky
fix: remove unopenable files from queue + add more logs
5547865 
Signed-off-by: Anupam Kumar <kyteinsky@gmail.com>
@kyteinsky
fix: exclude trashbin and versions from total file estimate
7cd57ff 
Signed-off-by: Anupam Kumar <kyteinsky@gmail.com>
@kyteinsky
fix: improve logging and exception handling
26424a9 
Signed-off-by: Anupam Kumar <kyteinsky@gmail.com>
@kyteinsky kyteinsky force-pushed the fix/more-improvements branch from d9e9382 to 26424a9 Compare August 19, 2025 09:56
@kyteinsky
fix: handle empty list returned in ccb doc count
10a7e32 
Signed-off-by: Anupam Kumar <kyteinsky@gmail.com>
@kyteinsky
fix: better db error handling in fs listener job
23c09bc 
Signed-off-by: Anupam Kumar <kyteinsky@gmail.com>
@kyteinsky
do not deduplicate at once
5208412 
Signed-off-by: Anupam Kumar <kyteinsky@gmail.com>
@kyteinsky kyteinsky force-pushed the fix/more-improvements branch from 58b01b1 to fb9cc9e Compare August 22, 2025 15:05
@kyteinsky
fix: progressive de-duplication instead of all at once
cee6ea6 
- add an index over the rows to speed up de-duplication, and uniqueness
  check upon insertion
- delete all instances of the row when the event is processed
- add more logging

Signed-off-by: Anupam Kumar <kyteinsky@gmail.com>
@kyteinsky kyteinsky force-pushed the fix/more-improvements branch from fb9cc9e to cee6ea6 Compare August 22, 2025 15:07
@kyteinsky
Copy link
Contributor

Switched to progressive de-duplication as advised by Julien instead of doing it at once in a repair step since it was difficult to do come up with a query that limited the no. of entries deleted/inserted in one DB transation, which can cause issues in certain setups like Galera setups as Joas and Christoph noted.

marcelklehr
marcelklehr commented Aug 25, 2025
View reviewed changes
marcelklehr
marcelklehr commented Aug 25, 2025
View reviewed changes
marcelklehr and others added 3 commits August 25, 2025 10:17
@marcelklehr
fix(IndexerJob): Fix warning message
aa4502f 
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@kyteinsky
fix: only set last_indexed_file_id for the last successful file
e9abb6b 
Signed-off-by: Anupam Kumar <kyteinsky@gmail.com>
@marcelklehr
tests: background-job:worker -v
92cef6b 
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@marcelklehr
Copy link
Member Author

See nextcloud/server#54620

@marcelklehr
Copy link
Member Author

Thanks for improving the migration step @kyteinsky !

kyteinsky
kyteinsky approved these changes Aug 26, 2025
View reviewed changes
Copy link
Contributor

@kyteinsky kyteinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for fixing the issue 🚀

@marcelklehr
Copy link
Member Author

@kyteinsky Now the backend embedder is failing

@kyteinsky
Copy link
Contributor

This PR also has the same issue so it's not a one-off at least: nextcloud/context_chat_backend#200
testing some changes there

@kyteinsky
Copy link
Contributor

Fixed the issue in context_chat_backend, see the PR linked above for details.

@kyteinsky kyteinsky merged commit d2d6ef6 into main Aug 27, 2025
76 of 85 checks passed
@kyteinsky kyteinsky deleted the fix/more-improvements branch August 27, 2025 12:50
@kyteinsky kyteinsky mentioned this pull request Aug 29, 2025
Merged
@kyteinsky kyteinsky mentioned this pull request Sep 23, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

Copilot code review Copilot Copilot left review comments

@julien-nc julien-nc julien-nc approved these changes

@kyteinsky kyteinsky kyteinsky approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@marcelklehr @kyteinsky @nickvergessen @julien-nc