Skip to content

Commit 26eae66

Browse files
committed
fix: default to ~/.memxt/palace.db + serialize embedder init
Bare memxt commands no longer create empty memxt.db in random cwds. Concurrent mine no longer races global Embedder init and drops chunks.
1 parent f48ea5b commit 26eae66

3 files changed

Lines changed: 158 additions & 6 deletions

File tree

build.zig.zon

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
.{
22
.name = .memxt,
3-
.version = "0.3.0",
3+
.version = "0.3.1",
44
.fingerprint = 0x5bf95d78bca15787,
55
.minimum_zig_version = "0.16.0",
66
.paths = .{

src/config.zig

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,10 +125,35 @@ pub fn applyEnvOverrides(cfg: *Config, allocator: std.mem.Allocator) void {
125125
overrideZ(&cfg.database_path, "memxt.db", "MEMXT_DB", allocator);
126126
overrideZ(&cfg.model_path, "lib/minilm.gguf", "MEMXT_MODEL", allocator);
127127
overrideZ(&cfg.default_wing, "default", "MEMXT_WING", allocator);
128+
resolveDatabasePath(cfg, allocator);
128129
resolveModelPath(cfg, allocator);
129130
resolveDefaultWing(cfg, allocator);
130131
}
131132

133+
/// The `database_path` default is the *relative* literal "memxt.db", so a bare
134+
/// `memxt search` / `inspect` / `mine` run from any project directory used to
135+
/// silently CREATE a brand-new empty palace right there — littering repos with
136+
/// stray memxt.db files and, far worse, showing the user an empty memory
137+
/// ("0 wings, 0 drawers") as if everything they'd stored was gone. Nothing
138+
/// warned; it just looked like data loss.
139+
///
140+
/// Anchor the unconfigured default to the installer's canonical palace
141+
/// (~/.memxt/palace.db) so bare commands find the real memory from any cwd.
142+
/// An existing ./memxt.db still wins — that's a deliberate project-local
143+
/// palace, and silently re-pointing it at the global one would orphan real data.
144+
fn resolveDatabasePath(cfg: *Config, allocator: std.mem.Allocator) void {
145+
// Explicitly configured (yaml or MEMXT_DB) — never second-guess it.
146+
if (!std.mem.eql(u8, cfg.database_path, "memxt.db")) return;
147+
// A project-local palace already exists here: keep using it.
148+
if (fileExists(cfg.database_path)) return;
149+
150+
const home_raw = c.getenv("HOME") orelse return;
151+
const home = std.mem.span(home_raw);
152+
const tmp = std.fmt.allocPrint(allocator, "{s}/.memxt/palace.db", .{home}) catch return;
153+
defer allocator.free(tmp);
154+
cfg.database_path = allocator.dupeZ(u8, tmp) catch return;
155+
}
156+
132157
/// If nothing (yaml nor MEMXT_WING) picked a wing, scope the default wing to
133158
/// the current project instead of dumping every project on the machine into
134159
/// one shared "default" wing. Prefers the basename of the git repository
@@ -236,6 +261,38 @@ test "MEMXT_WING env override wins over derived default" {
236261
try std.testing.expectEqualStrings("explicit-wing", cfg.default_wing);
237262
}
238263

264+
test "unconfigured database_path anchors to ~/.memxt/palace.db, not a stray cwd file" {
265+
const allocator = std.testing.allocator;
266+
_ = c.unsetenv("MEMXT_DB");
267+
268+
var cfg = Config{};
269+
applyEnvOverrides(&cfg, allocator);
270+
defer cfg.deinit(allocator);
271+
272+
// Regression: the default used to stay the relative literal "memxt.db",
273+
// so running memxt from any project dir created an empty palace there and
274+
// reported "0 wings, 0 drawers" — indistinguishable from losing everything.
275+
// (Guarded: only assert the rewrite when there's no project-local palace in
276+
// cwd, since an existing ./memxt.db is legitimately preferred.)
277+
if (!fileExists("memxt.db")) {
278+
try std.testing.expect(!std.mem.eql(u8, cfg.database_path, "memxt.db"));
279+
try std.testing.expect(std.mem.endsWith(u8, cfg.database_path, "/.memxt/palace.db"));
280+
try std.testing.expect(std.fs.path.isAbsolute(cfg.database_path));
281+
}
282+
}
283+
284+
test "MEMXT_DB env override is never second-guessed" {
285+
const allocator = std.testing.allocator;
286+
_ = c.setenv("MEMXT_DB", "/tmp/explicit-palace.db", 1);
287+
defer _ = c.unsetenv("MEMXT_DB");
288+
289+
var cfg = Config{};
290+
applyEnvOverrides(&cfg, allocator);
291+
defer cfg.deinit(allocator);
292+
293+
try std.testing.expectEqualStrings("/tmp/explicit-palace.db", cfg.database_path);
294+
}
295+
239296
test "default wing derives from project when nothing is configured" {
240297
const allocator = std.testing.allocator;
241298
_ = c.unsetenv("MEMXT_WING");

src/embedder.zig

Lines changed: 100 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,12 @@ const c = @cImport({
1717
@cInclude("llama.h");
1818
});
1919

20+
/// libc bits used only to locate the installed model in tests.
21+
const libc = @cImport({
22+
@cInclude("stdlib.h");
23+
@cInclude("unistd.h");
24+
});
25+
2026
// MiniLM-L6-v2 embedding width. The vec_drawers virtual table is declared
2127
// float[384], so the active model MUST match this. Validated in init().
2228
pub const EMBEDDING_DIM = 384;
@@ -193,36 +199,71 @@ var global_emb: ?Embedder = null;
193199
/// Points at config/env memory — not freed here.
194200
var pending_model_path: ?[:0]const u8 = null;
195201

202+
/// `global_emb` is a by-value `?Embedder`, so *assigning* it rewrites the
203+
/// struct in place — including `lock_flag`, the very spinlock `embed()` uses to
204+
/// serialize the llama_context. Lazy-init therefore MUST be serialized itself:
205+
/// the miner calls `embed()` from one concurrent task per file, and an
206+
/// unguarded `if (global_emb == null) global_emb = init()` let every task see
207+
/// null, build its own Embedder, and overwrite the global underneath the
208+
/// others. In-flight calls had their `ctx` swapped mid-encode and their lock
209+
/// reset, which surfaced as a storm of EncodeFailed/NoEmbedding and silently
210+
/// dropped ~90% of chunks when mining a directory (single-file mining has one
211+
/// task, so it never raced and always looked fine).
212+
///
213+
/// `emb_ready` is the atomic publication flag — `global_emb` itself is a wide
214+
/// struct and can't be loaded atomically. Once published, `global_emb` is never
215+
/// reassigned until `deinitGlobal`, so `&global_emb.?` stays stable.
216+
var emb_ready: std.atomic.Value(bool) = .init(false);
217+
var init_lock: std.atomic.Value(bool) = .init(false);
218+
219+
/// Initialize the process-global embedder exactly once. Safe to call from many
220+
/// threads concurrently; losers of the race wait and observe the winner's.
221+
fn initGlobalOnce(model_path: [:0]const u8) !void {
222+
if (emb_ready.load(.acquire)) return;
223+
224+
while (init_lock.cmpxchgWeak(false, true, .acquire, .monotonic) != null) {
225+
std.atomic.spinLoopHint();
226+
}
227+
defer init_lock.store(false, .release);
228+
229+
// Double-check under the lock: another thread may have published while we
230+
// were spinning.
231+
if (emb_ready.load(.acquire)) return;
232+
233+
global_emb = try Embedder.init(model_path);
234+
emb_ready.store(true, .release);
235+
}
236+
196237
/// Remember where the model lives without loading it. Mine can then skip the
197238
/// ~0.5s Metal init entirely when every chunk is already stored (incremental).
198239
pub fn setModelPath(model_path: [:0]const u8) void {
199240
pending_model_path = model_path;
200241
}
201242

202243
pub fn initGlobal(model_path: [:0]const u8) !void {
203-
if (global_emb != null) return;
204244
pending_model_path = model_path;
205-
global_emb = try Embedder.init(model_path);
245+
try initGlobalOnce(model_path);
206246
}
207247

208248
pub fn deinitGlobal() void {
209249
if (global_emb != null) {
210250
global_emb.?.deinit();
211251
global_emb = null;
212252
}
253+
emb_ready.store(false, .release);
213254
pending_model_path = null;
214255
}
215256

216257
/// True once a model is loaded. Lets callers degrade gracefully (e.g. keyword
217258
/// search) instead of hard-failing when no model is configured.
218259
pub fn isReady() bool {
219-
return global_emb != null;
260+
return emb_ready.load(.acquire);
220261
}
221262

222263
fn ensureReady() !void {
223-
if (global_emb != null) return;
264+
if (emb_ready.load(.acquire)) return;
224265
const path = pending_model_path orelse return error.EmbedderNotInitialized;
225-
global_emb = try Embedder.init(path);
266+
try initGlobalOnce(path);
226267
}
227268

228269
pub fn embed(text: []const u8, allocator: Allocator) ![]f32 {
@@ -232,3 +273,57 @@ pub fn embed(text: []const u8, allocator: Allocator) ![]f32 {
232273
}
233274
return error.EmbedderNotInitialized;
234275
}
276+
277+
// ═══════════════════════════════════════════════════════════════════
278+
// Regression tests
279+
// ═══════════════════════════════════════════════════════════════════
280+
281+
test "concurrent embed() does not race the lazy global init" {
282+
const allocator = std.testing.allocator;
283+
284+
// Needs the real model. Skip where it isn't installed (clean CI checkout).
285+
const home_raw = libc.getenv("HOME") orelse return error.SkipZigTest;
286+
const home = std.mem.span(home_raw);
287+
const path = std.fmt.allocPrintSentinel(allocator, "{s}/.memxt/lib/minilm.gguf", .{home}, 0) catch return error.SkipZigTest;
288+
defer allocator.free(path);
289+
if (libc.access(path.ptr, 0) != 0) return error.SkipZigTest;
290+
291+
deinitGlobal();
292+
setModelPath(path);
293+
294+
// The miner spawns one concurrent task per file, all landing in embed().
295+
// Pre-fix, each saw `global_emb == null`, built its own Embedder and
296+
// overwrote the global — swapping the llama_context (and resetting
297+
// `lock_flag`, the spinlock guarding it) underneath in-flight calls. Most
298+
// chunks then died with EncodeFailed/NoEmbedding and were silently dropped:
299+
// mining src/ui (22 files) stored 4 drawers instead of 504, and still
300+
// exited 0. Hammer the cold-start path from many threads at once; every
301+
// call must succeed.
302+
const N = 8;
303+
const Worker = struct {
304+
fn run(ok: *std.atomic.Value(u32), bad: *std.atomic.Value(u32), idx: usize) void {
305+
var buf: [64]u8 = undefined;
306+
const text = std.fmt.bufPrint(&buf, "regression sentence number {d}", .{idx}) catch return;
307+
const a = std.testing.allocator;
308+
if (embed(text, a)) |vec| {
309+
a.free(vec);
310+
_ = ok.fetchAdd(1, .monotonic);
311+
} else |_| {
312+
_ = bad.fetchAdd(1, .monotonic);
313+
}
314+
}
315+
};
316+
317+
var ok: std.atomic.Value(u32) = .init(0);
318+
var bad: std.atomic.Value(u32) = .init(0);
319+
var threads: [N]std.Thread = undefined;
320+
for (&threads, 0..) |*t, i| {
321+
t.* = std.Thread.spawn(.{}, Worker.run, .{ &ok, &bad, i }) catch return error.SkipZigTest;
322+
}
323+
for (threads) |t| t.join();
324+
325+
try std.testing.expectEqual(@as(u32, 0), bad.load(.monotonic));
326+
try std.testing.expectEqual(@as(u32, N), ok.load(.monotonic));
327+
328+
deinitGlobal();
329+
}

0 commit comments

Comments
 (0)