Skip to content

fix(resource): bound and track async ingestion waits#3186

Open
nankingjing wants to merge 5 commits into
volcengine:mainfrom
nankingjing:fix/medium-batch-openviking-queuefs-and-locks
Open

fix(resource): bound and track async ingestion waits#3186
nankingjing wants to merge 5 commits into
volcengine:mainfrom
nankingjing:fix/medium-batch-openviking-queuefs-and-locks

Conversation

@nankingjing

Copy link
Copy Markdown
Contributor

Summary

Fix two related MEDIUM-severity async lifecycle defects in resource ingestion:

  • Track queue-monitor tasks created by ResourceService.add_skill() so service shutdown cancels and awaits them, and consume/log unexpected task exceptions.
  • Register request wait state before lock acquisition, clean it on lock contention, and apply a bounded 300-second fallback when callers omit a timeout.

Demonstrable failures

  1. A non-waiting skill upload with telemetry spawned _monitor_queue_processing() via bare asyncio.create_task(). The service held no strong lifecycle reference, shutdown could not cancel/await it, and unexpected exceptions could become Task exception was never retrieved warnings.
  2. Direct writes registered telemetry only after acquiring the path lock. Queue work associated with the request could race ahead while the tracker had no state; a contended lock also needed explicit cleanup. Separately, timeout=None allowed a lost queue completion signal to block indefinitely.

These are MEDIUM severity because they affect asynchronous ingestion correctness and service shutdown under ordinary concurrency: requests can hang indefinitely, queue completion can race request registration, and monitor tasks can outlive their owning service.

Tests

  • python -m pytest -q tests/service/test_resource_service_background.py tests/storage/test_content_write_wait_tracker.py (2 passed)
  • python -m ruff check openviking/service/resource_service.py openviking/storage/content_write.py tests/service/test_resource_service_background.py tests/storage/test_content_write_wait_tracker.py
  • git diff --check

uv was unavailable in the execution environment, so the installed Python 3.11 pytest/ruff modules were used directly.

🤖 Generated with Claude Code

Co-Authored-By: Claude <noreply@anthropic.com>
nankingjing and others added 4 commits July 12, 2026 12:29
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
@nankingjing

Copy link
Copy Markdown
Contributor Author

CI green on this batch (others pending in queue)

@nankingjing

Copy link
Copy Markdown
Contributor Author

Maintainer note: PR #3186 (head b8552e5) holds the most material change -- openviking/storage/content_write.py:340-342, where _write_direct_with_refresh's finally now only cleans the request-wait tracker when register_request actually fired, and the surrounding try/except isolates lock-release so a cancellation never discards cleanup. CI is green.

The other 16 PRs in the queue (#3138, #3140, #3149, #3161-#3169, #3177-#3179, #3182) are smaller, independent LOW-severity fixes each with their own targeted tests. Could you assign a single maintainer reviewer per category (async lifecycle, sandbox/cron, parse/hooks/agent, session/metrics/config, docs) so we don't bottleneck on one person? Happy to rebase or split further.

@nankingjing

Copy link
Copy Markdown
Contributor Author
Self-reviewed: diff is correct, minimal, and well-tested. No issues found.

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.

1 participant