Skip to content

Improve url parser & token authentication - #4044

Open
disarticulate wants to merge 5 commits into
volcengine:mainfrom
disarticulate:main
Open

Improve url parser & token authentication#4044
disarticulate wants to merge 5 commits into
volcengine:mainfrom
disarticulate:main

Conversation

@disarticulate

@disarticulate disarticulate commented Aug 16, 2026

Copy link
Copy Markdown

Description

Weird. I filled this out previously. Simple fix proposed for user authenticated git repos and a simple dev environment using docker.

The url parser is cleaned up and tests created to demonstrate no regression in capabilities.

Human Involvement

  • A human participated in the implementation or review loop
  • This PR was generated entirely by AI agents without human participation in the loop

Related Issue

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update

Changes Made

  • Upgraded git-url parser to handle authenticated urls. Does not appear to break internal fetch requests, as the AI assured me the full url is kept, and the parser is only trying to keep a unique id of the repo.
  • A simple dev environment for a running container allows you to relaunch/reinstall as needed

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this on the following platforms:
    • Linux
    • macOS
    • Windows

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Screenshots (if applicable)

Additional Notes

DeerFlow added 3 commits August 16, 2026 17:46
…niqueness

normalize_repo_url rejected authenticated URLs like
https://oauth2:password@host/path/repo.git due to ':' not in user check,
leaving '@' in the normalized locator and breaking repo tracking.

This affects any Git provider (GitLab, GitHub, self-hosted) using
username:password or username:token authentication patterns.

This change:
- Fixes parsing to correctly strip userinfo with colons (e.g. oauth2:token@)
- Maintains existing normalized locator format (host/path) as unique identifier
- Extends repo tracking to support authenticated URLs across all Git providers
- Adds regression tests proving zero degradation on existing URLs

Moves normalize_repo_url to openviking/utils/url_parser.py alongside
parse_url() for shared URL handling.

AI: FF-27B-Q8
Detects /app/dev_openviking directory and runs uv pip install -e
with --force-reinstall --no-deps before starting the server.
Enables seamless local development against running containers.
Detects /app/dev_openviking and sets PYTHONPATH for seamless
local development. Uses PYTHONPATH instead of pip install to
avoid native build dependencies.
DeerFlow and others added 2 commits August 29, 2026 07:26
A lock token file left behind empty (e.g. a crash between file creation
and token write) permanently wedged the path: try_create_token returned
an Io error which is non-retryable in the acquire loop, so every
subsequent acquire on that path failed forever with
'lock I/O error: failed to create lock token'.

The filesystem provider now removes the orphaned empty file and retries
the CreateNew write, falling back to Conflict if another owner claimed
the lock in between.
@disarticulate

Copy link
Copy Markdown
Author

Ran into broken lock files preventing sync; entirely AI patched.

┃ # Wrote /tmp/opencode/PR-DESCRIPTION.md

┃ 1 # fix(lock): self-heal orphaned empty lock token files on acquire
┃ 2
┃ 3 ## Problem
┃ 4
┃ 5 An empty (0-byte) lock token file permanently wedges its path: every subsequent
┃ 6 pathlock acquire on that path fails fatally, stalling resource adds, semantic
┃ 7 processing, and watch refreshes until the file is removed by hand.
┃ 8
┃ 9 ### Symptoms observed in production
┃ 10
┃ 11 - RuntimeError: lock I/O error: failed to create lock token at /local/default/resources/<org>/<repo>/.path.ovlock
┃ 12 spamming the semantic processor, which treated it as transient and re-enqueued
┃ 13 the same messages every ~30s, opening its circuit breaker permanently.
┃ 14 - lock acquire timed out after 0ms on add-resource / watch-scheduler paths
┃ 15 whose tree lock was wedged by one of the empty files.
┃ 16 - 79 queued AddResource messages + 4 Semantic messages never processed; no
┃ 17 resources written to disk.
┃ 18
┃ 19 ### Root cause
┃ 20
┃ 21 1. FilesystemPathLockProvider::try_create_token (crates/ragfs/src/lock/provider.rs)
┃ 22 writes the token with WriteFlag::CreateNew.
┃ 23 2. If the lock file already exists but is empty, CreateNew fails and
┃ 24 read_token returns Ok(None) (empty content decodes to no token).
┃ 25 3. The provider then returns PathLockError::Io("failed to create lock token").
┃ 26 4. LockManager::is_retryable_error treats only Conflict/Busy as retryable,
┃ 27 so the Io error is returned immediately — no retry, no cleanup.
┃ 28 5. The stale-token cleanup branch only runs for Conflict errors, so the empty
┃ 29 file is never reclaimed.
┃ 30
┃ 31 Result: a single empty file = a permanent, non-retryable failure for every
┃ 32 lock operation touching that path.
┃ 33
┃ 34 Empty files arise from a crash/SIGKILL between creating the lock file and
┃ 35 writing the token (interrupted CreateNew write), or any other incomplete
┃ 36 write. Our incident had 14 such files across old resources.
┃ 37
┃ 38 ## Fix
┃ 39
┃ 40 In try_create_token, when the file exists but carries no decodable token,
┃ 41 remove the orphaned file and retry the CreateNew write once:
┃ 42
┃ 43 - remove fails → keep the previous Io error (genuinely broken FS).
┃ 44 - retry write fails → re-read: return Conflict if another owner claimed the
┃ 45 lock in the meantime (retryable, correct semantics), else Io.
┃ 46
┃ 47 This is safe because a token-less lock file holds no ownership state.
┃ 48
┃ 49 ## Tests
┃ 50
┃ 51 - Added lock::provider::tests::try_create_token_recovers_from_orphaned_empty_lock_file
┃ 52 (empty lock file present → try_create_token removes it and writes the new token).
┃ 53 - cargo test -p ragfs --lib lock → 36 passed, 0 failed.
┃ 54
┃ 55 ## Verification (deployed)
┃ 56
┃ 57 Rebuilt ragfs_python.abi3.so and restarted the server:
┃ 58
┃ 59 - 0 lock I/O error occurrences after restart (was: infinite loop).
┃ 60 - The 4 stuck Semantic messages completed (dead sources correctly dropped as
┃ 61 permanent errors instead of looping).
┃ 62 - Stuck AddResource queue drained; git resources cloned and written to disk.
┃ 63 - Watch scheduler recovered.
┃ 64
┃ 65 ## Files changed
┃ 66
┃ 67 - crates/ragfs/src/lock/provider.rs (+53 / −3): self-heal logic + unit test.
┃ 68
┃ 69 ## Minimal reproduction
┃ 70
┃ 71 1. Create an empty lock file for a resource path:
┃ 72 touch <data>/viking/<acct>/resources/<org>/<repo>/.path.ovlock
┃ 73 2. Trigger any operation that acquires a lock on that path (e.g. re-add the
┃ 74 resource, run a watch refresh, or a semantic sync).
┃ 75 3. Observe the immediate fatal lock I/O error: failed to create lock token
┃ 76 with no retries; repeat steps 2/3 to confirm it never recovers.

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