fix(memory): fall back to a writable session dir instead of losing it

[Forrest-ly]

What

start_session used the configured session directory unconditionally and propagated the first create_dir_all error. The shipped default that lands there — /run/anolisa/sessions, which is also the exact value the OpenClaw plugin forwards as MEMORY_SESSION_DIR on every spawn — is only writable by root, while the server always runs unprivileged. So on a stock install the session log never existed.

Resolve the base through a preference chain instead and take the first candidate that can be created and written to.

Why this is a bug, not a configuration preference

Install path	/run/anolisa/sessions state	Non-root server
RPM	created 0700 root:root by config/systemd/anolisa-memory-tmpfiles.conf (system instance, %tmpfiles_create in %post)	cannot traverse the parent (no x for others), cannot create <sid>
make install / container / dev box	absent — no tmpfiles snippet is installed, and /run is drwxr-xr-x root root	create_dir_all fails on the first component with EACCES
systemctl --user anolisa-memory@$USER	as above	unit declares RuntimeDirectory=anolisa/sessions/%i but never tells the server about it, and ReadOnlyPaths=/ makes it unwritable from inside the namespace anyway

Failure was silent: MemoryService::new degraded to session = None behind a single warn! in the child's stderr, and every mem_promote returned SessionUnavailable while every mem_session_log returned NotImplemented. Session-scoped consolidation had no log to read either. mem_promote is the capability the plugin's sessionId pinning exists to support (see a3e641a3), so this removes the one property the user guide says a fixed session id is for.

Fix

resolve_session_base tries, in order, and takes the first usable candidate:

1. the configured directory (resolved_session_dir()), unchanged;
2. $XDG_RUNTIME_DIR/anolisa/sessions — already 0700 and user-owned, and what the user unit's RuntimeDirectory= creates;
3. <$TMPDIR|/tmp>/anolisa-sessions-<uid> — uid-suffixed so two users on one host never share a base.

A fallback is reported with warn! naming both directories; only when every candidate fails does the service degrade, and the aggregated error then lists each one with its reason. Treating the configured value as a preference rather than a requirement is what makes this work for every caller — including the plugin, which always sends an explicit MEMORY_SESSION_DIR, so changing only the compiled-in default would not have reached it.

Two details worth calling out:

• Existence is not a usable probe. A pre-existing read-only directory passes create_dir_all and then fails on the first session, so each candidate gets a real create-and-remove.
• Directories we pick are hardened; the operator's are not. Chosen candidates must be non-symlinks owned by us and are created 0700. Without that, any local user could plant /tmp/anolisa-sessions-<victim uid> as a symlink and redirect the probe, every session root, log.jsonl and the mem_promote source tree into a directory of their choosing. The configured directory is exempt from those two checks so a symlink or a group-shared mount there keeps working.

The user unit is fixed alongside: Environment=MEMORY_SESSION_DIR=%t/anolisa/sessions/%i makes the RuntimeDirectory= it already creates live, and %t/anolisa is added to ReadWritePaths (ReadOnlyPaths=/ would otherwise make it read-only inside the namespace). /run/anolisa is left listed for operators who point MEMORY_SESSION_DIR back at it.

Verification

Driven over stdio exactly as the plugin does (initialize → notifications/initialized → tools/call mem_session_log), non-root uid 1359527, no RPM installed, /run/anolisa absent, MEMORY_SESSION_DIR=/run/anolisa/sessions:

build	mem_session_log	server stderr
main @ 69b6869a	isError: true — session log unavailable; check MEMORY_SESSION_DIR / /run/anolisa permissions	WARN session log unavailable (io: Permission denied (os error 13)); mem_promote / mem_session_log will return errors
this branch	isError: false — (session log is empty)	WARN session dir /run/anolisa/sessions is not usable by uid 1359527; using /run/user/1359527/anolisa/sessions instead (set MEMORY_SESSION_DIR to override)

With XDG_RUNTIME_DIR unset the same fixed build falls through to $TMPDIR/anolisa-sessions-1359527 and still answers isError: false.

Suites:

Gate	Result
cargo test --locked	353 passed / 0 failed, 16 suites (lib 191 → 198)
cargo fmt --all --check	clean
cargo clippy --all-targets --locked -- -D warnings	clean
systemd-analyze verify anolisa-memory@.service	identical warning set before and after (only line numbers shift); %t/anolisa accepted as absolute

New service::session_base_tests (7): preference order, uncreatable configured dir, existing read-only dir, symlink refused for chosen dirs but honoured for configured ones, 0700 on a created fallback, all-candidates-failed error text, and the shape of the candidate chain.

tests/session_test.rs::session_log_degrades_gracefully_when_session_dir_unavailable pinned the old behaviour with assert!(svc.session.is_none()) and now fails by design. It is rewritten as unusable_session_dir_falls_back_instead_of_losing_the_session, which keeps the "service still builds" half of the old contract and adds: session root is outside the unusable path, and mem_promote + mem_session_log work end to end through the fallback. Reverting only src/service/mod.rs makes it fail, so it reproduces the bug rather than describing it.

Scope

Three files, no overlap with the in-flight component:memory PRs — #3228 (anolisa-core adapter), #3238 (plugin config.ts / index.ts / manifest / user guide), #3256 (Makefile), #3222 (adapter install.sh).

Deliberately left for follow-ups:

• config/systemd/anolisa-memory-tmpfiles.conf still creates /run/anolisa{,/sessions} root-only. That is now unused rather than broken; changing its mode is a security-relevant packaging decision that should not ride along here.
• docs/user-guide/{en,zh}/token-saving/agent-memory.md and the plugin's DEFAULT_SESSION_DIR / sessionDir descriptions still present /run/anolisa/sessions as the effective location. Both files are being edited by fix(memory): refuse expert profile at plugin boot #3238, so touching them here would only create a conflict.

Related to Multica issue AGE-7763.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e334264dd5

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[Forrest-ly]

Round 1 review handled — all three findings confirmed valid, fixed in a13fdab5, and each thread answered in place and resolved.

Finding	Fix	Test that fails without it
P1 · owner check exempted root	fallback_owner_is_us(owner, me) = owner == me, no root branch	fallback_owner_check_has_no_root_exemption, rejects_a_foreign_owned_fallback_even_as_root
P1 · probe followed planted symlinks	probe_writable opens create_new (O_CREAT|O_EXCL) mode 0600; symlink ⇒ reject candidate, stale regular file ⇒ next of PROBE_ATTEMPTS = 8 reserved names	probe_refuses_to_follow_a_planted_symlink, probe_steps_over_a_stale_file_and_removes_only_its_own
P2 · chmod result swallowed	tighten to 0700 after the owner check, propagate failure as a candidate rejection, re-stat and reject if still loose	tightens_a_pre_existing_fallback_to_0700, leaves_an_operator_configured_dir_mode_alone

Two things worth calling out beyond the literal asks:

• The O_EXCL probe applies to the operator-configured base too, not just the fallbacks. A group-writable MEMORY_SESSION_DIR is explicitly allowed, which is the case where a neighbour can plant the predictable probe name; only the symlink / owner / mode rules on the base itself remain the operator's business.
• The 0700 tightening is re-verified after chmod, because some FUSE and network mounts return success without applying it — the same "silently listable base" outcome, reached with no error left to propagate.

Every fix was mutation-checked: reverting it individually turns the corresponding test red (493 != 448 for the mode case, Ok(()) where an Err was expected for the symlink case, and the predicate assertion for the owner case).

Scope is unchanged — still src/agent-memory/src/service/mod.rs, tests/session_test.rs and the systemd user unit. Verified on the branch: cargo test --locked 359 passed / 0 failed (6 new), cargo fmt --all -- --check clean, cargo clippy --all-targets --locked -- -D warnings clean.

Ready for re-review.

[kongche-jbw]

Reviewed the full merge-base-to-head diff, the e334264 -> a13fdab follow-up, affected callers, tests, and all existing review threads and replies. The three previously reported issues have direct fixes, but three additional findings remain below. All three were already present in e334264; they are gaps in the previous review, not regressions introduced by a13fdab.

Baseline: declared base 2e1ee65; merge base 69b6869; reviewed head a13fdab. The current agent-memory CI check passed.

Validation was primarily static. git diff --check passed. A bounded Python simulation in a private temporary directory confirmed the filesystem-operation interleaving described in the first finding; this was not a cross-user exploit or an end-to-end execution of the Rust server. No services, namespaces, mounts, or network/security configuration changes were performed. The temporary directory was removed and its absence verified.

[Forrest-ly]

Thanks for the second pass, @kongche-jbw — all three findings confirmed and fixed in d259a8e2. Each thread has the detail; the short version:

[P1] service/mod.rs — validation and use are now bound to one descriptor. probe_session_base resolves the candidate exactly once (O_RDONLY|O_DIRECTORY|O_CLOEXEC[|O_NOFOLLOW]) and does every check with fstat/fchmod on that descriptor; the probe uses openat/unlinkat/fstatat. The winner is returned as a new SessionBase (src/agent-memory/src/session/base.rs) that owns the descriptor and hands out /proc/self/fd/<n> — the kernel resolves that magic symlink to the descriptor's inode rather than re-walking the pathname, so the rename-then-symlink swap you reproduced cannot redirect the session any more. SessionLogService::start_in keeps the handle alive next to the paths derived from it, and creates <sid> with mkdirat + an AT_SYMLINK_NOFOLLOW verification. New display_root() carries the operator-facing pathname for logs.

[P2] service/mod.rs — the tmp suffix uses the host uid. New crate::host captures the launching uid into a OnceLock before any unshare (from main, from LinuxUserNsMount::enter, and from the top of MemoryService::new), and falls back to recovering it from /proc/self/uid_map when nothing was captured. The ownership check deliberately keeps using the current uid, since that is the namespace st_uid is reported in — the split is now "host uid for the name, namespace uid for the metadata".

[P2] tests/session_test.rs — the fallback case runs in an isolated child. Private XDG_RUNTIME_DIR, private TMPDIR, test-owned MEMORY_SESSION_ID, everything else passed in as ANOLISA_TEST_*. The parent plants a decoy live session in the fallback base and asserts it survives the child's cleanup, that the child's own session is gone, and that the tmp fallback was never needed. The child must print a completion marker, so a child that matched no test cannot pass by exiting 0.

Verification:

check	result
cargo fmt --all --check	clean
cargo clippy --all-targets -- -D warnings	clean
cargo test --all-targets	368 passed, 0 failed (lib 213, up from 208)
agent-memory serve, non-root, inside userns, XDG_RUNTIME_DIR unset, MEMORY_SESSION_DIR=/run/anolisa/sessions	falls back to <$TMPDIR>/anolisa-sessions-1359527 (host uid, not 0), base 0700, session tree written, log reports the operator-facing path

One deliberate behaviour change to flag: a directory can only be opened read-only, so the base is opened O_RDONLY|O_DIRECTORY. A session base the server cannot even list is now rejected where it previously might have scraped through; write access — the property that actually matters — is still proven by the O_EXCL probe.

Ball back with you and @ikunkun-sys for re-review.

[kongche-jbw]

LGTM. Re-reviewed a13fdab -> d259a8e, including the author replies, affected callers, descriptor lifetime, and session cleanup. The three findings from my previous review are addressed:

• Fallback validation, probing, and subsequent session operations stay anchored to the validated directory descriptor.
• Temporary-directory naming uses the UID captured before namespace entry, while ownership validation uses the current namespace's UID.
• The fallback integration test runs in a child process with test-owned runtime/tmp directories and a pinned test session ID, and verifies that a neighbouring session survives cleanup.

No new actionable findings in this revision. git diff --check passed, and a bounded local filesystem simulation confirmed that probing, writes, and cleanup remain in the original directory after its pathname is replaced. The current agent-memory CI check passed. Local validation was primarily static; I did not run the full Rust suite or namespace/mount tests. The documented requirement for usable /proc/self/fd and a readable session base was considered in this review.