Skip to content

fix(storage): propagate VikingFS.mkdir backend errors instead of swallowing them - #3731

Open
hobostay wants to merge 1 commit into
volcengine:mainfrom
hobostay:fix/mkdir-propagate-backend-errors
Open

fix(storage): propagate VikingFS.mkdir backend errors instead of swallowing them#3731
hobostay wants to merge 1 commit into
volcengine:mainfrom
hobostay:fix/mkdir-propagate-backend-errors

Conversation

@hobostay

@hobostay hobostay commented Aug 4, 2026

Copy link
Copy Markdown

Problem

VikingFS.mkdir() (openviking/storage/viking_fs.py) catches every exception from the backend mkdir call, but the except block has no re-raise:

try:
    await self._async_agfs.mkdir(path, fs_ctx=self._pathlock_fs_ctx(ctx, lease_ref))
except Exception as exc:
    message = str(exc).lower()
    already_exists = "exist" in message or "already" in message
    if exist_ok and already_exists:
        return
    # <- anything else falls through and is silently discarded

Any failure that is not an already-exists error tolerated by exist_ok=True — permission denied, quota/resource-exhausted, I/O errors, lock-lease violations, and even already-exists errors with the default exist_ok=False — is silently swallowed, and mkdir() returns as if the directory had been created.

mkdir is on the write hot path (ovpack import, parsers, session, privacy service, and mv's internal copy). Callers reasonably assume a returned mkdir means the directory exists; downstream writes then fail later with confusing secondary errors, and the original, actionable error is lost.

Fix

Re-raise the original exception unless it is an already-exists error tolerated by exist_ok=True:

            if exist_ok and already_exists:
                return
            raise

One-line change; no behavior change for the already-exists + exist_ok=True path.

Tests

tests/misc/test_mkdir.py was stale: it mocked fs.agfs.mkdir, but mkdir() goes through the AsyncAGFSClient wrapper (self._async_agfs). Four of its five tests failed on main, and the fifth passed only because the swallowed AttributeError from the missing mock made mkdir() return early — the very bug this PR fixes.

  • Updated the existing tests to mock _async_agfs.mkdir and to match the current create-then-tolerate-exists semantics.
  • Added regression tests: backend errors propagate with both exist_ok=False and exist_ok=True (non-exists errors), and exist_ok=False surfaces already-exists errors.
tests/misc/test_mkdir.py: 8 passed

Full tests/storage, tests/misc, and tests/service runs show no new failures versus main (the only delta is the 4 previously-failing stale mkdir tests now passing).

…lowing them

The except block in VikingFS.mkdir() had no re-raise, so any backend
failure that was not an already-exists error (permission denied, quota
exceeded, I/O errors, lock-lease violations) — and even already-exists
errors with exist_ok=False — was silently discarded and mkdir() returned
as if the directory had been created. Callers on the write hot path
(ovpack import, parsers, session, privacy) then write into a directory
that may not exist, and the original actionable error is lost.

Re-raise the original exception unless it is an already-exists error
tolerated by exist_ok=True.

Also update tests/misc/test_mkdir.py, which still mocked fs.agfs.mkdir
even though mkdir() now goes through the AsyncAGFSClient wrapper
(self._async_agfs) — the swallowed-attribute-error made the stale tests
pass/fail for the wrong reasons. Add regression tests covering error
propagation for both exist_ok values.
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