Skip to content

fix(resources): clean up temp dir after incremental sync#1759

Open
arminzou wants to merge 1 commit intovolcengine:mainfrom
arminzou:fix/incremental-watch-sync-and-temp-cleanup
Open

fix(resources): clean up temp dir after incremental sync#1759
arminzou wants to merge 1 commit intovolcengine:mainfrom
arminzou:fix/incremental-watch-sync-and-temp-cleanup

Conversation

@arminzou
Copy link
Copy Markdown

Summary

When sync_diff_callback successfully completes an incremental update (watch-triggered or explicit re-add), the temp directory tree used as the diff source was never removed from viking://temp/. Over time this left behind one empty temp directory shell per watch tick, accumulating indefinitely.

This PR adds a cleanup step inside sync_diff_callback (in SemanticDagExecutor) that removes the temp parent directory immediately after a successful sync, mirroring the cleanup that already happens on the new-resource path in ResourceProcessor.

Root cause: delete_temp was only called on error paths or in ResourceProcessor for the first-add case. The incremental-update success path in SemanticDagExecutor had no corresponding cleanup.

Fix: After the logger.info(f"[SyncDiff] Diff computed: ...") line in sync_diff_callback, compute the parent URI from self._root_uri and call delete_temp if it lives under viking://temp/. Failures are caught and logged as warnings (non-fatal).

Type of Change

  • Bug fix (fix)

Testing

  • Manually verified with a watch task set to 5-minute intervals: new files added to the watched source directory sync to the resource URI, and no residual empty dirs accumulate under data/viking/<account>/temp/ after the sync completes.
  • Existing test suite passes (the one failing test — test_resource_processor_first_add_persist_does_not_await_agfs_mv — fails on origin/main as well due to an uninitialized LockManager in the test fixture; unrelated to this change).

Related Issues

N/A

Checklist

  • Code follows project style guidelines (ruff format + ruff check pass)
  • Manual testing completed
  • All tests pass (pre-existing failure unrelated to this change)

/cc @chuanbao666 @baojun-zhang @myysy

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 28, 2026

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🏅 Score: 85
🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Clean up temp dir after incremental sync

Relevant files:

  • openviking/storage/queuefs/semantic_dag.py

Sub-PR theme: Add account_id to VikingDB metrics

Relevant files:

  • openviking/metrics/collectors/vikingdb.py
  • openviking/metrics/datasources/observer_state.py

⚡ Recommended focus areas for review

Overly Broad Exception Handling

The _make_default_ctx method uses a broad except Exception: without logging the error, which could hide underlying configuration issues.

except Exception:
    account_id = "default"
    user_id = "default"
Temp Directory Cleanup Edge Case

The temp parent URI check uses startswith("viking://temp/"), which would fail to clean up if the parent URI is exactly viking://temp/<dir> (without a trailing slash in the check condition).

if temp_parent_uri and temp_parent_uri.startswith("viking://temp/"):

@github-actions
Copy link
Copy Markdown

PR Code Suggestions ✨

No code suggestions found for the PR.

After a successful sync_diff, the temp parent directory was left as an
empty shell on disk because delete_temp was never called for the
incremental path. Call delete_temp on the temp parent URI after
sync_diff succeeds.
@arminzou arminzou force-pushed the fix/incremental-watch-sync-and-temp-cleanup branch from 50b0931 to 635a0f5 Compare April 28, 2026 02:45
@qin-ctx qin-ctx requested a review from myysy April 28, 2026 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

3 participants