fix(memory): edge cases, friendly errors, fail-fast lock perms - #240
Merged
Conversation
Contributor
|
🎉 PR Validation ✅ PASSED Commit: Checks:
Ready to merge! ✨ 🔗 View workflow run |
vreshch
marked this pull request as ready for review
July 7, 2026 22:08
vreshch
added a commit
that referenced
this pull request
Jul 7, 2026
## What README overhaul plus a hand-authored architecture diagram, aligned to master after #238-#245. Ready as the npm landing page. Touches only `README.md` and `docs/**` - no `src/`, `package.json`, or workflows. ## Changes - **`docs/architecture.svg`** - hand-written SVG (no render tooling). Neutral background with dark text on light boxes, so it reads in both light and dark GitHub themes and on npm. Shows AI tools -> MCP (stdio `agentage mcp` + daemon HTTP `/mcp` on `127.0.0.1:4243`) -> CLI + local daemon (single writer) -> local vaults (git-per-vault, `@agentage/memory-core`) -> sync out to git remotes and account sync (labeled protocol-neutrally, no internal tech named). - **`README.md`** restructured for a first-time reader: What is this -> Install -> Quickstart (local-only first, then optional `agentage setup`) -> Architecture (diagram) -> Command reference -> Sync -> MCP integration -> Daemon -> Env vars -> Development. - **`docs/architecture.md`** - short walk-through of the diagram. ## Reality-check vs master Built the CLI (`npm run build`) and verified every command and flag against the live `--help` output. Corrected drift from the merged PRs: - `vault add` now defaults to an **account** vault; `--local [path]`, `--git <remote>`, `--path <dir>` are the alternatives. - `vault sync [name]` now covers git and the account channel; progress prints per vault. - Documented the `update` command and `--no-daemon` global flag. - Daemon section notes the loopback-only, token-guarded, cross-origin-rejecting API and port-in-use reporting (#243, #244). - Friendly memory errors + non-zero exits (#240); 64 KB read clamp and secret refusal. - Describes no specific `src/` file paths (a sibling PR is moving `src/sync/` paths). ## Verification - `npm run verify` green: 405 unit tests pass, type-check + lint + format:check + build clean. - Ran the offline quickstart end-to-end against the built `dist/cli.js` (vault add --local, write via stdin, list, search, read `@vault/path`) - all pass. - Rendered `docs/architecture.svg` to PNG and eyeballed legibility. Note: `format:check` only globs `{src,e2e}/**/*.ts`, so it does not lint markdown; README changes are prose-only.
vreshch
added a commit
that referenced
this pull request
Jul 7, 2026
) ## Root cause (issue #249) `src/lib/file-lock.ts` decided lock takeover from **age alone**. A holder that acquires the lock, then gets descheduled by the OS for longer than `LOCK_TTL_MS` (10s) - realistic on a 2-core GitHub runner running 20 child processes **plus** V8 coverage (added to both lanes in #245; `NODE_V8_COVERAGE` is inherited by the `execFile` children, so every child pays the tax) - still has its `<pid> <timestamp>` lock look *stale*. `acquireFileLock` (old `file-lock.ts:79-81`) then treated that live holder as crashed: ``` const held = heldAt(lockPath(target)); if (held !== null && now - held < LOCK_TTL_MS) return false; // fresh -> refuse if (!takeOverStale(target, now)) return false; // else STEAL it ``` `takeOverStale` deletes the "stale" lock and a second process enters the critical section while the original holder is still mid read-modify-write. Both `readFileSync` the array, both `push`, the later `writeFileSync` clobbers the earlier - exactly one append is lost. The holder was never dead, so it still prints `ok`: hence **all 20 children succeed yet the array is 19/20** (`expected [0..18] to deeply equal Array(20)`). This is a **product bug**, not a test bug. ## Mechanism, reproduced Standalone harness: the 20-process append race with process 0's critical section paused 11s (simulating the CPU-starvation the runner produces), against the two `file-lock.ts`: ``` ORIG pause=11000 iters=8 lostWrites=8 throwPath=0 # incl. an exact 19/20 iter FIX pause=11000 iters=10 lostWrites=0 throwPath=0 ``` The original loses an append on **every** iteration (all children still report `ok`, matching the CI signature); the fix loses none. ## The fix Takeover is now gated on **same-host pid liveness**, not age alone (`file-lock.ts` `isCrashed`): - A holder is stolen only once it is past the TTL **and** provably gone - its pid returns `ESRCH` from `process.kill(pid, 0)`, or it is our own leftover pid (we cannot be inside our own synchronous section while trying to acquire), or it is past a 60s hard backstop (guards against pid reuse after a genuine crash). - A live-but-CPU-starved holder is **waited out** by the caller's bounded backoff, never stolen. If it is genuinely wedged past `MAX_WAIT_MS`, the caller throws a loud `could not acquire lock` - a safe, visible failure, never silent corruption. Why correct: the only way two processes can be in the critical section is if a takeover fires while the first is alive. Liveness makes that impossible for any alive holder, regardless of how long the scheduler pauses it. Crash recovery is preserved (dead pid -> takeover) and the ABA takeover guard is unchanged. The lock file format stays `<pid> <timestamp>`; parsing is tolerant of legacy single-token files (parsed as pid 0 -> ages out on the TTL, so old on-disk locks still recover). Same hardening applied to `src/lib/update-lock.ts` (same lock family). It is far less exposed (10-min TTL, single-writer), but the fix is symmetric and its parser is now backward-compatible with existing bare-timestamp `update.lock` files. ## Evidence / verify - `npm run verify` green (406 tests, 48 files). - `file-lock.test.ts` looped **20x = 20/20**; **5x pinned to 2 cores** (`taskset -c 0,1`) = 5/5. - All existing tests kept, including the fail-fast-on-EACCES cases (#240) and the 20-process zero-lost-writes proof. The crashed-holder test now seeds a reaped (deterministically dead) pid instead of a hardcoded `999`, so the liveness check is exercised reliably. Added a test proving a stale-looking lock whose holder is **alive** is refused, not stolen. Closes #249
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Memory-command UX + edge-case hardening, all verified against the built CLI.
memory edit <ref>with none of--old/--new/--body: was a silent no-op that printed "Edited ..." and exited 0. Now errors "specify --old/--new for a replacement or --body to overwrite" and exits 1.--frontmatterJSON: was a raw parser stack. Now "--frontmatter must be a JSON object, e.g. '{\"tags\":[\"x\"]}'" plus the parse detail; also rejects non-object JSON (arrays/scalars). Exits 1.memory write <ref>with no--bodyon an interactive TTY: blocked forever waiting on stdin. Now errors "provide --body, or pipe content on stdin". Pipe fallback unchanged.agentage vault listto see available vaults." - engine not forked.file-lockspun the full 15s MAX_WAIT_MS on EACCES/EPERM/EROFS (an unwritable config dir). Now non-EEXIST write failures rethrow immediately with "permission denied: ".vault listnow uses the--localform, matchingmemory list); top-level program description is now "The offline-first terminal client for agentage Memory"; bareagentagehelp gains a "New here?" start-here pointer.Why
These were the sharp edges a first-time terminal user hits: silent no-ops, raw parser dumps, an indefinite hang, MCP-internal vocabulary, and a 15s stall on a perms problem.
Tests
Unit tests added for every behavior change (
src/commands/memory.test.ts,src/lib/file-lock.test.ts). Permission tests skip under root (chmod is a no-op there).Verified: npm run verify green locally.