Skip to content

fix: vector search perf regression + Windows web server takeover loop - #259

Open
petra-dot wants to merge 4 commits into
tickernelz:mainfrom
petra-dot:fix/247-251-memory-robustness
Open

fix: vector search perf regression + Windows web server takeover loop#259
petra-dot wants to merge 4 commits into
tickernelz:mainfrom
petra-dot:fix/247-251-memory-robustness

Conversation

@petra-dot

Copy link
Copy Markdown
Contributor

Fixes two reported bugs.

#247: memory search takes ~8s when filtering by container_tag

searchKind() combined vector_top_k() with INNER JOIN memories m ON m.rowid = v.id. SQLite's planner drove from the memories table and evaluated the ANN index once per row (337 rows = 337 ANN calls). CROSS JOIN forces the table-function-first plan; semantics unchanged. Benchmarked in the issue: 8.4s -> ~29ms. Added a regression test asserting the query plan scans vector_top_k before the memories lookup.

#251: Web server takeover loop on Windows when an orphaned LISTEN socket holds the port

After a crash, an orphaned socket holds the configured port (EADDRINUSE) but answers no HTTP, so attemptTakeover() retried forever every 5s. After 3 consecutive failed takeovers the server now falls back to port+1 (bounded at port+10) and the takeover toast surfaces the actual URL. Added a unit test for the port-fallback policy.

Verification

  • tsc --noEmit clean
  • bun run build succeeds
  • Full test suite: 405 pass / 4 fail, identical failure set on clean main (pre-existing: onnxruntime-resolve shim, plugin-loader contract timeout, turso-shard recreate timeout, config parallel flake). No new failures.
  • Prettier clean on all touched files

CROSS JOIN forces SQLite to scan the vector_top_k table function first. The previous INNER JOIN let the planner drive from the memories table and evaluate the ANN index once per row (337 rows = 337 ANN calls, ~8s). Same semantics, one-word change. Closes tickernelz#247.
On Windows a crashed opencode can leave an orphaned LISTEN socket in the TCP table: the port refuses to bind (EADDRINUSE) yet nothing answers HTTP, so the takeover loop retried forever. After 3 consecutive failed takeovers the server now moves to port+1 (bounded at +10) and surfaces the actual URL in the takeover toast. Closes tickernelz#251.
@petra-dot

Copy link
Copy Markdown
Contributor Author

Note: this PR overlaps with #257, which makes the same CROSS JOIN change for #247. Either merge order works — the hunk is identical. This PR additionally includes the #251 web-server takeover fix and regression tests for both, so you can drop #257 if preferred, or take #257 and I'll rebase this one down to just the #251 fix.

@lindixu6-hash lindixu6-hash left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the #247 overlap and the new #251 path. The CROSS JOIN implementation matches #257 and the query-plan assertion is useful. One coverage difference to preserve if this PR supersedes #257: #257 exercises both searchKind() SQL branches (empty and non-empty containerTag), while this regression executes only the filtered query. Please retain coverage for both generated branches in whichever PR lands.

For #251, nextFallbackPort() is well bounded, but the current test exercises only that pure helper. The behavior depends on the surrounding takeover state machine: repeated _start()/EADDRINUSE, health-loop restart, owner transition, and callback URL after config.port changes. A focused integration-style test around those transitions would make the Windows fix substantially safer.

I attempted the two targeted tests locally, but the isolated worktree dependency install stalled and the partial environment could not resolve @libsql/client, so I am not claiming runtime verification from that attempt.

@petra-dot

Copy link
Copy Markdown
Contributor Author

Addressed both review points in d12f0d0:

  1. [Bug]: memory search takes ~8s when filtering by container_tag (vector_top_k + JOIN bad query plan) #247 both branches — the plan assertion now runs for both searchKind() SQL variants: "drives vector_top_k before the memories join when filtering by container_tag" and "...without a container tag". The empty-tag branch (WHERE m.vector IS NOT NULL, no container predicate) is covered alongside the filtered one.

  2. [Bug] Web server takeover loop on Windows when orphaned LISTEN socket holds the port #251 takeover state machine — new integration-style test "becomes owner on a neighbor port after repeated failed takeovers": a real listener holds the port (EADDRINUSE on every bind), checkServerAvailable reports unavailable, and three direct attemptTakeover() cycles run the real state machine — first two leave isServerOwner() false on the original port, the third bumps to the neighbor port, flips ownership, fires the takeover callback once, and getUrl() reports the new port with /api/health answering on it.

Verification: both test files 6 pass, tsc --noEmit clean, full suite 408 pass / 3 fail (same pre-existing flakes as clean main: onnxruntime shim, plugin-loader timeout, turso-shard-recreate timeout), prettier clean.

@petra-dot

Copy link
Copy Markdown
Contributor Author

Diagnosing the windows-latest failure on this run (no changes made to the diff).

Failure: turso legacy migrator dimension preflight > preserves legacy dimensions — a 10s afterEach hook timeout.

Not caused by this PR: the branch diff touches only src/services/turso/vector-search.ts, src/services/web-server.ts, src/index.ts, and two test files. The migrator and the shared test-cleanup path are untouched. The same test passes in ~145ms locally on Windows, and the other 5 OS jobs in this matrix passed.

Mechanism: the timeout is in cleanupTursoTestDirectory() (tests/turso-test-utils.ts), which runs withSqliteFileLockRetry(() => rmSync(...)) after closing Turso handles. On a busy Windows runner, an SQLite file handle can stay locked past the 10s hook window. This is the same flake class as the turso-shard-recreate hook timeout seen in earlier local runs — it lands on a different turso test each time.

If you can, re-run the failed job (gh run rerun <id> --failed); it should go green. Happy to make the cleanup hook more robust (e.g. raise the hook timeout or degrade the file-lock retry) as a separate PR if this keeps flaking.

@lindixu6-hash lindixu6-hash left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The follow-up at d12f0d0 resolves both points from my first review: both searchKind() SQL branches now assert the vector-first plan, and the integration-style test exercises repeated bind failures through owner transition, callback, active URL, and a live health response. I independently ran the two target files: 6/6 tests passed, and bun run typecheck passed. The command itself returned non-zero only when the TRAE sandbox blocked cleanup of macOS /var/folders/... temp directories after the tests completed.

I also inspected the Windows failure and triggered a failed-job rerun. Both new vector tests and the takeover integration test passed on Windows; the failure was the existing turso-migrate-dims-preflight SQLite EBUSY/10-second hook timeout in untouched code.

Two state-machine gaps still need changes before this is safe to merge:

  1. takeoverFailures is described as consecutive, but a successful availability probe does not reset it. Sequence: two failed takeovers, the original owner recovers and serves healthy checks, then dies later; the next single failure advances to a fallback port because the stale count is still 2. Reset the counter when the post-jitter checkServerAvailable() succeeds (and after a successful bind), and add a regression for failure -> recovery -> failure.

  2. Candidate exhaustion is bounded numerically but not behaviorally. At maxFallbackPort, nextFallbackPort() returns the same port forever and startHealthCheckLoop() retries every five seconds indefinitely. This recreates the loop #251 is intended to eliminate when all 11 candidates are unavailable. Add an explicit exhausted terminal/degraded state with one actionable log/callback (no endless retries), plus coverage for every candidate occupied/unavailable.

The existing health probe also accepts any HTTP 2xx as an opencode-mem owner. Since fallback expands adoption across 10 neighboring ports, validating the /api/health JSON shape (success: true, status: "ok") would prevent convergence on an unrelated local service; please include that in the candidate handling or explain a stronger existing identity guarantee.

…stion, validate owner identity

Addresses the three CHANGES_REQUESTED points on tickernelz#251:
- takeoverFailures now resets when the owner recovers (post-jitter probe succeeds) and after a successful bind, so a stale count can't bump the port on the next unrelated failure.
- At maxFallbackPort with repeated failures, the takeover loop stops and signals a terminal degraded state once (log + optional callback) instead of retrying every five seconds forever.
- checkServerAvailable now requires the opencode-mem API envelope (health: success+status ok; stats: success), so a 2xx from an unrelated local service on a fallback port is not mistaken for the owner.
@petra-dot

Copy link
Copy Markdown
Contributor Author

All three points addressed in af5dbaf:

  1. Stale takeover countertakeoverFailures now resets in both recovery paths: when the post-jitter checkServerAvailable() succeeds (owner recovered, we go passive again) and after a successful bind. Regression test "resets the takeover failure counter when the owner recovers" drives fail → fail → recover → die and asserts the next single failure does NOT bump the port.

  2. Port exhaustion — at maxFallbackPort, once takeoverFailures reaches the per-candidate threshold, the health loop is stopped and a terminal degraded state fires exactly once: a log plus an optional setOnPortsExhaustedCallback (wired in index.ts to a toast with the candidate range). No more 5s retry loop. Test "enters a terminal state when every candidate port is unavailable" asserts the loop is cleared and the callback fires once across subsequent attempts.

  3. Owner identitycheckServerAvailable now parses the response and requires our API envelope: /api/health needs success === true and status === "ok"; the token path (/api/stats) needs success === true. A 2xx HTML page or a { success: false } body from an unrelated service on a fallback port is treated as unavailable. Test "does not treat a 2xx from an unrelated service as an opencode-mem owner" covers both non-JSON and wrong-shape responses.

Verification: web-server-health.test.ts 6 pass, tsc --noEmit clean, prettier clean, full suite 411 pass / 3 fail — the remaining failures are the pre-existing environment flakes (onnxruntime shim fixture, and the Windows SQLite file-lock hook timeout that you re-ran green), none in this PR's code.

@lindixu6-hash lindixu6-hash left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The af5dbaf follow-up resolves all three requested state-machine issues:

  • recovery resets the consecutive-failure counter, including a fail/fail/recover/fail regression;
  • final-candidate exhaustion stops the health loop and emits one bounded degraded-state callback;
  • health ownership now requires the opencode-mem JSON envelope instead of accepting arbitrary HTTP 2xx responses.

Independent verification on exact head af5dbaf640e900cbb72074d1cb4494c0f3c67000:

  • tests/turso-vector-search.test.ts + tests/web-server-health.test.ts: 9 pass, 0 fail, 36 assertions;
  • bun run typecheck: pass;
  • Prettier check on all five changed source/test files: pass;
  • upstream six-platform package-smoke run 32288060862: Ubuntu, Windows, macOS 15/26 Intel, and macOS 15/26 Apple Silicon all pass.

The local test command reported non-zero only after all 9 tests passed because the TRAE sandbox blocked cleanup of temporary test directories; this is not a test or implementation failure.

Approved. If this PR lands first, its two-branch ANN coverage supersedes the overlapping query-plan portion of #257; #257 should then be closed or rebased rather than merged independently.

@lindixu6-hash

Copy link
Copy Markdown
Collaborator

@tickernelz, this is now approved at exact head af5dbaf: 9/9 focused tests, typecheck, Prettier, and all six package-smoke platforms pass.

One owner decision remains because its ANN hunk overlaps my authored #257. I will not use Write access to choose a merge order that benefits my own PR. Please choose either:

  1. independently review/merge fix: preserve vector-first ANN query plan #257 first, then have fix: vector search perf regression + Windows web server takeover loop #259 rebased to the Windows takeover changes; or
  2. merge fix: vector search perf regression + Windows web server takeover loop #259 first, after which I will close fix: preserve vector-first ANN query plan #257 as transparently superseded rather than duplicate the change.

#258 remains separate and independently review-gated. I will not self-approve or self-merge it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants