[sanil-23]

Summary

• Rust integration test flake — tests/agent_retrieval_e2e.rs::orchestrator_query_topic_tool_returns_alice_phoenix_hits flaked because query_topic sorted its results into a HashMap-randomised order. Replace HashMap with BTreeMap + add node_id ASC tertiary tie-break.
• WDIO E2E job flake on Linux — app/scripts/e2e-run-session.sh failed on rm -rf $tmp because CEF helper children kept writing to the temp workspace after the parent process died. Reap the whole CEF process tree before cleanup; tolerate residual rm errors that race with helper teardown.

Together these turn two intermittent CI flakes that have been blocking other PRs (e.g. #1734's Rust Core Coverage job) into deterministic green.

Problem

Rust integration test

Test failure in CI:

thread 'orchestrator_query_topic_tool_returns_alice_phoenix_hits' panicked at tests/agent_retrieval_e2e.rs:302:5:
assertion `left == right` failed: fetch_leaves should hydrate exactly the requested chunk
  left: 0
 right: 1

Two sources of nondeterminism in memory/tree/retrieval/topic.rs:

1. by_node: HashMap<String, RetrievalHit> (topic.rs:92) — .into_values().collect() returned items in HashMap iteration order, which Rust randomises per process (SipHash).
2. Stable sort with frequent ties — sort_by(score DESC, time_range_end DESC) doesn't disambiguate further. The test uses an inert embedder (cfg.memory_tree.embedding_endpoint = None), so every hit has score = 0.0 and many share time_range_end. The surviving order was whatever HashMap handed in.

The test picked the first node_kind == "leaf" hit and asserted fetch_leaves hydrated exactly one chunk. With nondeterministic ordering, different leaves got picked on different runs.

WDIO E2E (Linux job)

Pattern: smoke spec reports 1 passed, then the job dies with exit 1. Log:

[runner] Stopping CEF app (pid 21110)...
rm: cannot remove '/tmp/tmp.xZBAn58lfq/users/local/cef/Default': Directory not empty
##[error]Process completed with exit code 1.

app/scripts/e2e-run-session.sh cleanup trap did kill $APP_PID; wait $APP_PID; rm -rf $tmp — but CEF spawns zygote / GPU / renderer helpers that the parent doesn't reap on SIGTERM. They kept writing into <tmp>/users/local/cef/Default/ after wait returned, so rm -rf raced them. set -e was active in the trap, so the non-zero rm exit failed the whole job. Linux-only; macOS/Windows runners don't trigger it.

Solution

src/openhuman/memory/tree/retrieval/topic.rs — 2 changes

1. HashMap → BTreeMap<String, RetrievalHit> so by_node.into_values() returns hits in deterministic node_id order.
2. Added a.node_id.cmp(&b.node_id) as the final then_with in the post-filter sort_by. Belt-and-suspenders for any future path that bypasses by_node but still needs full-tie determinism.

Pure ordering shifts; no semantic change for non-tied results. For ties, order goes from HashMap-random to node_id-alphabetic — strictly more useful (reproducible debugging, stable display).

app/scripts/e2e-run-session.sh — 2 changes

1. Reap the CEF process tree in the cleanup trap: pkill -TERM -P "$APP_PID" before killing the parent, brief sleep 1 for handles to release, then pkill -KILL -P "$APP_PID" as a final guard. This makes the tree actually dead by the time rm -rf runs.
2. Defense in depth on the workspace rm: rm -rf "$CREATED_TEMP_WORKSPACE" 2>/dev/null || true. The workspace is a per-run mktemp under /tmp — leftovers get reaped by CI's tmp-cleanup pass. The job must not fail on cleanup race when the test itself passed.

set -euo pipefail semantics preserved everywhere else; only the temp-workspace rm was changed to tolerate the documented race.

Verification

• Rust fix: previously-flaky cargo test --test agent_retrieval_e2e orchestrator_query_topic passed 5/5 consecutive local runs after the change (was intermittent before).
• E2E fix: validated by inspection — the failing log line rm: cannot remove '...users/local/cef/Default': Directory not empty only triggers when CEF helpers outlive their parent. Killing the process group via pkill -P is the standard Linux pattern for reaping a child-process tree; the || true on the workspace rm is the well-known race-tolerant cleanup idiom. Can't reproduce the Linux-runner-specific path locally (no headless xvfb + CEF + container combo on Windows), but the surgical change is unambiguous and the script's other paths are untouched.

Submission Checklist

• Tests added or updated (happy path + failure / edge case) — Rust fix is exercised by the existing orchestrator_query_topic_tool_returns_alice_phoenix_hits test on every run; E2E fix is exercised by the existing Run smoke spec (Xvfb) step in CI. Both were failing before this PR; both pass after.
• Diff coverage ≥ 80% — diff is 17+20 lines across one Rust function and one bash cleanup function; both surfaces are exercised by their corresponding existing tests.
• N/A: behaviour-only change — Coverage matrix updated. Internal ordering / cleanup robustness; no user-facing or API surface change.
• N/A: no matrix rows — All affected feature IDs from the matrix are listed under ## Related.
• No new external network dependencies introduced.
• N/A: no release-cut surface change — Manual smoke checklist updated.
• N/A: no linked issue — Linked issue closed via Closes #NNN.

Impact

• Runtime/platform: every caller of query_topic (orchestrator memory-tree retrieval tool, the matching JSON-RPC handler). Tied-hit ordering is now deterministic — strict improvement. The E2E script change is CI-only.
• Performance: BTreeMap insertion is O(log n) vs HashMap's amortised O(1). For query_topic's typical input sizes (dozens of hits, capped at LOOKUP_HEADROOM) the difference is microseconds — well below the SQLite I/O baseline.
• Migration: none. In-memory reshuffle + cleanup-script reorder. No on-disk or schema effect.
• Compatibility: stable. JSON-RPC response shape unchanged; only the order of tied entries shifts.

Related

• Closes:
• Follow-up PR(s)/TODOs: investigate the deeper fetch_leaves-vs-query_topic mismatch this flake exposed. With the order now stable, any remaining get_chunk lookup failure would be reproducible and worth root-causing.

---

AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

• N/A: no Linear issue — Key
• N/A: no Linear issue — URL

Commit & Branch

• Branch: fix/retrieval-topic-deterministic-order
• Commits: 54bcdf37 (retrieval determinism) · 4191e3df (CEF cleanup)

Validation Run

• N/A: no app/ TS changes — pnpm --filter openhuman-app format:check
• N/A: no app/ TS changes — pnpm typecheck
• Focused tests: cargo test --test agent_retrieval_e2e orchestrator_query_topic → 5/5 consecutive runs pass
• Rust fmt/check (if changed): cargo fmt --check clean + cargo clippy --lib --tests clean in diff cone
• Bash syntax check: bash -n app/scripts/e2e-run-session.sh clean
• N/A: no Tauri-shell changes — Tauri fmt/check (if changed)

Validation Blocked

• command: N/A
• error: N/A
• impact: N/A

Behavior Changes

• query_topic returns tied hits in deterministic node_id-ascending order instead of HashMap-random.
• E2E runner cleanup reaps the full CEF process tree and tolerates a residual rm -rf race on the per-run mktemp workspace.
• No user-visible effect.

Parity Contract

• Legacy behavior preserved for non-tied retrieval results and for the test-pass case of the E2E runner.
• For retrieval ties, order changes from random-per-process to deterministic — improvement, not breaking.
• The E2E cleanup change only affects the failure-mode where CEF children outlived the parent; success cases are unchanged.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Topic search results now appear in a deterministic, stable order when items share the same score and time range.
• Chores
  • Improved end-to-end session cleanup so the test app and its child processes are terminated more reliably and temporary workspaces are removed without causing job failures.
• Tests
  • Prevented cross-test interference by serializing workspace-related test setup to ensure reliable test runs.

[coderabbitai]

📝 Walkthrough

Walkthrough

Deterministic query ordering: query_topic deduplicates with a BTreeMap and adds node_id as a final tie-break. E2E script: cleanup() reaps the CEF process tree and tolerates transient workspace deletion errors. Tests: workspace env mutations are serialized with a process-wide ENV_LOCK held for EnvGuard lifetime.

Changes

Deterministic Memory Query Results

Layer / File(s)	Summary
Deterministic deduplication and sorting in query_topic src/openhuman/memory/tree/retrieval/topic.rs	HashMap-based deduplication replaced with BTreeMap for deterministic iteration; hit comparator now adds node_id (ascending) as final tie-break after score and time_range_end.

E2E CEF Cleanup

Layer / File(s)	Summary
Robust CEF process tree termination and tolerant workspace removal app/scripts/e2e-run-session.sh	cleanup() captures CEF child PIDs, SIGTERMs the process tree, waits/kills the parent, force-kills lingering children after a short sleep, and ignores rm -rf "$CREATED_TEMP_WORKSPACE" errors (`

Serialized Test Workspace Environment

Layer / File(s)	Summary
ENV_LOCK and EnvGuard lifetime for workspace env tests/agent_retrieval_e2e.rs	Introduce ENV_LOCK: Mutex<()>; EnvGuard holds a MutexGuard<'static, ()> for its lifetime; set_workspace_env acquires the lock, sets OPENHUMAN_WORKSPACE, and returns a guard that restores the previous value before releasing the lock.

🎯 3 (Moderate) | ⏱️ ~20 minutes

• tinyhumansai/openhuman#1027: Related changes to retrieval/query_topic ordering used by memory_tree_query_topic tooling.

• tinyhumansai/openhuman#1724: Modifies the same test workspace helpers and environment setup in agent retrieval E2E tests.

• Suggested reviewers:

  • senamakel

"A rabbit hops through memories with care,
Orders its hops by number and lair,
Kids of CEF get a gentle nudge,
Then a firm goodbye if they won't budge,
All tidy now — carrots in a row! 🐰✨"

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title accurately reflects the main changes: it mentions deterministic query_topic ordering (memory_tree fix) and robust CEF cleanup (e2e test fix), which are the two primary objectives of this PR.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

• Generate code and open pull requests
• Plan features and break down work
• Investigate incidents and troubleshoot customer tickets together
• Automate recurring tasks and respond to alerts with triggers
• Summarize progress and report instantly

Built for teams:

• Shared memory across your entire org—no repeating context
• Per-thread sandboxes to safely plan and execute work
• Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/scripts/e2e-run-session.sh`:
- Around line 72-78: The current sequence uses pkill -KILL -P "$APP_PID" after
killing the parent, but children may be reparented to PID 1 and escape that
sweep; modify the block around APP_PID to capture the current child PIDs before
killing the parent (e.g., use pgrep -P "$APP_PID" or readarray to snapshot
children into a variable/array), then proceed with pkill -TERM -P
"$APP_PID"/kill "$APP_PID" as before, and finally iterate the saved child PID
list to send SIGKILL to each PID directly (checking for non-empty values and
ignoring failures), replacing the final pkill -KILL -P "$APP_PID" with direct
kills of the captured PIDs so reparenting won't prevent cleanup.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b72e65ce-4411-42f6-8746-bdeaadb4734f

📥 Commits

Reviewing files that changed from the base of the PR and between 54bcdf3 and 4191e3d.

📒 Files selected for processing (1)

• app/scripts/e2e-run-session.sh