Skip to content

fix(storage): harden session append for missing files#1377

Open
chethanuk wants to merge 1 commit intovolcengine:mainfrom
chethanuk:fix/1358-session-append
Open

fix(storage): harden session append for missing files#1377
chethanuk wants to merge 1 commit intovolcengine:mainfrom
chethanuk:fix/1358-session-append

Conversation

@chethanuk
Copy link
Copy Markdown
Contributor

Summary

  • handle raw backend not-found errors during session message appends without crashing
  • serialize appends so concurrent first writes do not drop messages
  • add focused storage regressions for missing-file and concurrent append behavior

Testing

  • pytest tests/storage/test_viking_fs_append.py -v

Notes:

  • broader session/server add-message tests are currently blocked here by unrelated existing config/native-binding issues in this environment

Fixes #1358

@github-actions
Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1358 - Partially compliant

Compliant requirements:

  • Fix RuntimeError when appending to missing session files
  • Add regression tests for missing-file and concurrent append behavior

Non-compliant requirements:

  • Ensure concurrent appends do not drop messages (due to race condition in fallback lock creation)
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🏅 Score: 75
🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Race Condition in Fallback Lock Creation

Concurrent calls to _append_lock_context for the same path may create multiple asyncio.Lock instances because access to _append_fallback_locks is not synchronized. This breaks the serialization guarantee for concurrent appends, potentially leading to lost messages.

def _append_lock_context(self, path: str):
    try:
        from openviking.storage.transaction import LockContext, get_lock_manager

        return LockContext(get_lock_manager(), [path], lock_mode="point")
    except RuntimeError:
        lock = self._append_fallback_locks.get(path)
        if lock is None:
            lock = asyncio.Lock()
            self._append_fallback_locks[path] = lock
        return _asyncio_lock_context(lock)
Unused Dead Code

The _noop_async_lock async context manager is defined but never used.

@asynccontextmanager
async def _noop_async_lock() -> Any:
    yield

@github-actions
Copy link
Copy Markdown

PR Code Suggestions ✨

No code suggestions found for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

[Bug]: openviking.server RuntimeError: not found

1 participant