Skip to content

fix(openviking): don't spawn a second server onto a live port (#74846) - #74959

Closed
jeff-mettel wants to merge 1 commit into
NousResearch:mainfrom
jeff-mettel:fix/openviking-spawn-loop
Closed

fix(openviking): don't spawn a second server onto a live port (#74846)#74959
jeff-mettel wants to merge 1 commit into
NousResearch:mainfrom
jeff-mettel:fix/openviking-spawn-loop

Conversation

@jeff-mettel

@jeff-mettel jeff-mettel commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

What & why

Fixes #74846.

_start_local_openviking_server() spawns openviking-server unconditionally. Both call sites reach it from a health probe:

Call site When
initialize() gateway startup
_handle_runtime_openviking_unreachable() every health-check cycle that classifies unreachable

_VikingClient.health() can time out client-side while the server is genuinely up (curl /health returns 200). That produces unreachable, which spawns a second server; the new process loses the data-directory lock and exits immediately with DataDirectoryLocked. Since the probe keeps timing out, this repeats every cooldown window — the reporter measured a spawn roughly every 5 minutes, ~18 DataDirectoryLocked per day.

The existing 30s _failed_refresh cooldown paces the loop but can't stop it: it expires while the underlying condition persists. A longer cooldown would only slow the bleeding, so this fixes the spawn decision itself.

The change

Probe the target host:port before spawning; an occupied port means "already started". Both call sites converge on this function, so one guard covers every path.

Two deliberate details:

  • The probe tests liveness, not health. A successful TCP connect proves a listener owns the port, which is all that's needed to know a second server would lose the lock. Whether that listener is healthy is a different question, already handled by the existing waiter/cooldown machinery — this change doesn't touch it.
  • Parse + probe now precede the shutil.which() lookup. A reachable server is reported as running even when openviking-server isn't on PATH (e.g. started by systemd or another venv). Previously that returned "not found on PATH" despite a perfectly good server listening.

No change to the fallback-to-disabled behavior when nothing is listening and no binary exists.

Tests

tests/plugins/memory/test_openviking_provider.py:

  • ..._does_not_spawn_when_port_already_openPopen raises if called
  • ..._reports_running_server_without_cli_on_path — probe outranks PATH
  • ..._rejects_unparseable_url_before_probing — no probe on a bad endpoint
  • _local_openviking_port_is_open against a real bound loopback socket, then against the same port after close (true/false without mocking the syscall)

The pre-existing ..._uses_endpoint_host_and_port test now stubs the probe explicitly rather than depending on nothing happening to listen on port 1934 in CI.

51 passed  # tests/plugins/memory/test_openviking_provider.py + tests/openviking_plugin/

Platforms

Tested on macOS 15 (Darwin 25.5.0), Python 3.11. socket.create_connection + OSError handling is platform-neutral; the 2s budget only bounds a wedged listener, since a loopback connect resolves or refuses far below it.

Duplicate check

gh search prs for DataDirectoryLocked returns no open or closed PRs, and no open PR references #74846.


Authored by an AI agent (Claude Opus 5) operating autonomously on @jeff-mettel's behalf: the defect was traced, the patch written, and the tests run and verified end-to-end before submission.

`_start_local_openviking_server()` spawned `openviking-server`
unconditionally. Both callers — `initialize()` and the runtime
unreachable handler — reach it from a health probe, and that probe can
time out client-side while the server is up and serving. The spawned
process then loses the data-directory lock and exits immediately with
`DataDirectoryLocked`; because the probe keeps timing out, the cycle
repeats every cooldown window (~5 min observed).

The existing 30s `_failed_refresh` cooldown paces the loop but cannot
stop it, since it expires while the underlying condition persists.

Probe the target host:port before spawning and treat an occupied port as
already-started. This guards both call sites at their single convergence
point. The probe deliberately tests only that a listener owns the port —
enough to know a second server would lose the lock — and says nothing
about that listener's health.

The parse/probe now precedes the PATH lookup, so a reachable server is
reported as running even when `openviking-server` is not on PATH.

Fixes NousResearch#74846

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers labels Jul 30, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused fix. Current main still calls subprocess.Popen without checking whether the local endpoint is already occupied (plugins/memory/openviking/__init__.py:1217-1235), and both runtime recovery (:2254) and initialization (:2328-2333) reach that helper. The PR's pre-spawn TCP listener guard addresses that shared convergence point without changing the existing health/waiter behavior.

Automated hermes-sweeper review.

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Merged via #77747 — your commits cherry-picked with authorship preserved (rebase merge).

This PR consolidated your port-occupancy guard and compression re-arm fixes together with 4 other OpenViking fixes into one coherent integration so the complete runtime behavior could be validated as a unit.

Thanks for the contributions!

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

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(openviking): missing cooldown on unreachable health state causes spawn loop (DataDirectoryLocked every 2-5 min)

4 participants