A pane's ctl takes acme's lock and unlock

A client doing an edit of several writes to addr and data had no way to
keep another client's from landing in between. acme's window ctl takes
lock and unlock for this (editors/acme/xfid.c:603-611): a qlock that
blocks a second locker, owned by the fid that wrote it and given up when
that fid is clunked, binding only clients that ask. pardes does the same:
the open's record holds it, a second lock parks until unlock, close or
the pane closing, and no other write is refused for it.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

7 files changed, 150 insertions, 19 deletions

diff --git a/docs/fs.md b/docs/fs.md
index 64ac9ba1..b19a0a71 100644
--- a/docs/fs.md
+++ b/docs/fs.md
@@ -153,8 +153,15 @@ appends to the directory's `+Errors` pane. Holding `event` open redirects the
 pane's Look and Exec clicks to that client; writing a record back performs the
 action. `ctl` reads acme's window status line — serial, tag length, body
 length, a reserved zero, the dirty flag, the width in cells, the font and the
-tab width — and takes one verb, `get`, which reloads the buffer from the name
-it carries.
+tab width — and takes `get`, which reloads the buffer from the name it
+carries, and acme's `lock` and `unlock` (editors/acme/xfid.c:603-611), for an
+edit of several writes to `addr` and `data` that another client must not
+land in the middle of. As in acme the lock binds only the clients that take
+it: a `lock` while another open holds it waits until that open writes
+`unlock` or closes (or the pane does), and nothing else is refused for it --
+not a write to any other file, not the person at the keyboard. It belongs to
+the open that wrote it, so only that open's `unlock` is taken; a write on an
+open that cannot write (or the editor's own, on none) cannot lock.
 
 The three range files `addr`, `dot` and `limit` each read the pair of offsets
 they also accept, so copying one onto another is all that acme's `addr=dot`,
diff --git a/src/fs-help.txt b/src/fs-help.txt
index 5f890d66..302e6538 100644
--- a/src/fs-help.txt
+++ b/src/fs-help.txt
@@ -42,4 +42,4 @@ Pitfalls, one each:
   Truncating tag clears the part you may edit; truncating dot or addr empties it.
   A terminal's body is a history snapshot frozen per open; pty/data is the live stream.
   A failing command is reported in the editor and in log, not as a write error; a bad line fails the write.
-  pane/<n>/ctl reads acme's window status line and takes one verb, get, which reloads from disk.
+  pane/<n>/ctl: acme's status line; takes get (reload), lock/unlock (a second lock waits for close/unlock).
diff --git a/src/ninep/ctl.zig b/src/ninep/ctl.zig
index 7625f501..83b1196f 100644
--- a/src/ninep/ctl.zig
+++ b/src/ninep/ctl.zig
@@ -2,8 +2,8 @@
 //! a right click on it and writing one to `exec` a middle click, at the
 //! active pane from the root and at that pane from /pane/<n>/; reading either
 //! answers the serials the last command made or touched. /status reports the
-//! editor, and a pane's ctl its acme status line and the one verb, `get`,
-//! that no file of its own would say any better.
+//! editor, and a pane's ctl its acme status line and the verbs no file of its
+//! own would say any better: `get`, `lock` and `unlock`.
 const std = @import("std");
 const pardes = @import("../pardes.zig");
 const panes = @import("../panes.zig");
@@ -179,17 +179,48 @@ pub fn readPane(p: *Pardes, req: Req, pane: *Pane) Reply {
     return tree.stagedReply(p, req);
 }
 
-/// `get` is the one thing here that no file of the pane's own would say: it
-/// reloads the buffer from the name it carries, wherever that name resolves.
-/// Repeating it in one write would only reload the same bytes, so it runs once.
+/// `get` reloads the buffer from the name it carries, wherever that name
+/// resolves. Repeating it in one write would only reload the same bytes, so
+/// it runs once.
+///
+/// `lock` and `unlock` are acme's (editors/acme/xfid.c:603-611), so that a
+/// client can make an edit of several writes to addr and data without
+/// another's landing in between. As in acme the lock binds only the clients
+/// that ask for it: a `lock` while another open holds it waits (parked, as
+/// acme's qlock blocks the writer) until that open unlocks or closes; a
+/// write to any other file is never refused for it, nor is the person at
+/// the keyboard. It belongs to the open that wrote it, which alone may
+/// `unlock`, and closing that open or the pane gives it up.
 pub fn writePane(p: *Pardes, req: Req, pane: *Pane) Reply {
+    // The record of this open is what holds the lock; a write that came on
+    // no writable open (the editor's own) has none.
+    const mine: ?u8 = if (tree.openOf(p, req)) |o| @intCast(o - &p.fs.opens[0]) else null;
+    const other = pane.fs.lock != null and pane.fs.lock != mine;
     var asked = false;
-    var it = std.mem.splitScalar(u8, req.data, '\n');
-    while (it.next()) |raw| {
-        const line = std.mem.trim(u8, raw, " \t\r");
-        if (line.len == 0) continue;
-        if (!std.mem.eql(u8, line, "get")) return tree.failText(req.tag, E.INVAL, tree.e_bad_ctl);
-        asked = true;
+    // Checked whole before anything applies, so a write that must wait for
+    // the lock has done nothing yet when it goes again.
+    for ([2]bool{ false, true }) |apply| {
+        var held = !other and pane.fs.lock != null;
+        var it = std.mem.splitScalar(u8, req.data, '\n');
+        while (it.next()) |raw| {
+            const line = std.mem.trim(u8, raw, " \t\r");
+            if (line.len == 0) continue;
+            if (std.mem.eql(u8, line, "get")) {
+                asked = true;
+            } else if (std.mem.eql(u8, line, "lock")) {
+                if (mine == null) return tree.failText(req.tag, E.INVAL, tree.e_bad_ctl);
+                if (other) return .{ .tag = req.tag, .status = .again };
+                held = true;
+                if (apply) pane.fs.lock = mine;
+            } else if (std.mem.eql(u8, line, "unlock")) {
+                if (!held) return tree.failText(req.tag, E.INVAL, e_not_locked);
+                held = false;
+                if (apply) {
+                    pane.fs.lock = null;
+                    pardes.turn.parked = true; // a `lock` waiting goes again
+                }
+            } else return tree.failText(req.tag, E.INVAL, tree.e_bad_ctl);
+        }
     }
     if (asked) {
         const errno = get(p, pane);
@@ -198,6 +229,8 @@ pub fn writePane(p: *Pardes, req: Req, pane: *Pane) Reply {
     return .{ .tag = req.tag, .written = @intCast(req.data.len) };
 }
 
+const e_not_locked = "window not locked by this open";
+
 fn get(p: *Pardes, pane: *Pane) u16 {
     const f = pane_files.fileOf(pane) orelse return 0;
     if (!panes.Output.fileTraits(f.output).saves) return 0;
@@ -251,13 +284,13 @@ test "pane ctl read is index's five fields plus width in cells, font and tab wid
     try testing.expectEqualStrings("'it''s'", w.buffered());
 }
 
-test "the pane ctl takes get, and nothing that a file of its own now answers" {
+test "the pane ctl takes get, lock and unlock, and nothing that a file of its own now answers" {
     const gpa = testing.allocator;
     const p = try withFile(gpa, "one\ntwo\n");
     defer p.deinit();
     const ctl_node = Node.of(serialOf(p), .ctl);
     for ([_][]const u8{
-        "menu",       "nomenu",     "dump echo hi", "font Go Mono", "lock",   "bogus",     "DEL",
+        "menu",       "nomenu",     "dump echo hi", "font Go Mono", "lock x", "bogus",     "DEL",
         "name x.txt", "put",        "del",          "delete",       "Look x", "Exec Save", "clean",
         "dirty",      "cleartag",   "dot=addr",     "addr=dot",     "show",   "mark",      "nomark",
         "scroll",     "limit=addr", "get x",        "look /tmp",    "exec Del",
@@ -265,6 +298,63 @@ test "the pane ctl takes get, and nothing that a file of its own now answers" {
     try testing.expect(p.paneBySerial(serialOf(p)) != null);
 }
 
+test "a second lock waits until the holder unlocks or closes, and binds nobody else" {
+    const p = try withFile(testing.allocator, "one\ntwo\n");
+    defer p.deinit();
+    const serial = serialOf(p);
+    const ctl_node = Node.of(serial, .ctl);
+    const w = struct {
+        fn ctl(pp: *Pardes, node: u64, h: u32, data: []const u8) th.Answer {
+            return call(pp, .{ .tag = 4, .op = .write, .node = node, .handle = h, .data = data });
+        }
+    };
+    const a = call(p, .{ .tag = 1, .op = .open, .node = ctl_node, .omode = 2 }).reply.handle;
+    const b = call(p, .{ .tag = 2, .op = .open, .node = ctl_node, .omode = 1 }).reply.handle;
+    try testing.expect(a != 0 and b != 0 and a != b);
+    // Reading the status line holds nothing, so it cannot lock either.
+    try testing.expectEqual(@as(u32, 0), call(p, .{ .tag = 3, .op = .open, .node = ctl_node }).reply.handle);
+    try testing.expectEqual(E.INVAL, wr(p, ctl_node, "lock\n").errno());
+
+    try testing.expectEqual(Status.ok, w.ctl(p, ctl_node, a, "lock\n").reply.status);
+    try testing.expectEqual(Status.ok, w.ctl(p, ctl_node, a, "lock\n").reply.status);
+    // Another open's lock waits, and its unlock is refused; writes to the
+    // pane's other files are not.
+    try testing.expectEqual(Status.again, w.ctl(p, ctl_node, b, "lock\n").reply.status);
+    try testing.expectEqualStrings(e_not_locked, w.ctl(p, ctl_node, b, "unlock\n").reply.ename);
+    try testing.expectEqual(Status.ok, wr(p, Node.of(serial, .addr), "1").reply.status);
+    try testing.expectEqual(Status.ok, wr(p, Node.of(serial, .data), "ONE\n").reply.status);
+    try testing.expectEqualStrings("ONE\ntwo\n", p.panes[0].?.file.?.content);
+
+    // Unlocking lets the parked lock go again, and now it takes the lock.
+    pardes.turn.parked = false;
+    try testing.expectEqual(Status.ok, w.ctl(p, ctl_node, a, "unlock\n").reply.status);
+    try testing.expect(pardes.turn.parked);
+    try testing.expectEqual(Status.ok, w.ctl(p, ctl_node, b, "lock\n").reply.status);
+    try testing.expectEqual(Status.again, w.ctl(p, ctl_node, a, "lock\nunlock\n").reply.status);
+    // Closing the holder gives it up.
+    pardes.turn.parked = false;
+    _ = call(p, .{ .tag = 5, .op = .release, .node = ctl_node, .handle = b });
+    try testing.expect(pardes.turn.parked);
+    try testing.expectEqual(Status.ok, w.ctl(p, ctl_node, a, "lock\nunlock\nlock\n").reply.status);
+    try testing.expectEqual(E.INVAL, w.ctl(p, ctl_node, a, "unlock\nunlock\n").errno());
+    try testing.expectEqual(@as(?u8, @intCast(a - 1)), p.panes[0].?.fs.lock);
+
+    // The lock lives and dies with the pane: closing it wakes whoever waits.
+    const other = try th.newPane(p);
+    const other_ctl = Node.of(other, .ctl);
+    const c = call(p, .{ .tag = 6, .op = .open, .node = other_ctl, .omode = 1 }).reply.handle;
+    try testing.expectEqual(Status.ok, w.ctl(p, other_ctl, c, "lock\n").reply.status);
+    pardes.turn.parked = false;
+    try testing.expectEqual(Status.ok, th.rmdir(p, Node.of(other, .dir)).reply.status);
+    try testing.expect(pardes.turn.parked);
+    pardes.turn.parked = false;
+    try testing.expectEqual(E.NOENT, w.ctl(p, other_ctl, c, "lock\n").errno());
+    for ([_]u64{ ctl_node, other_ctl }, [_]u32{ a, c }) |node, h|
+        _ = call(p, .{ .tag = 7, .op = .release, .node = node, .handle = h });
+    try testing.expect(p.panes[0].?.fs.lock == null);
+    for (p.fs.opens) |o| try testing.expect(o.node == 0);
+}
+
 test "exec runs a builtin at the pane and records the pane it acted on" {
     const gpa = testing.allocator;
     const p = try withFile(gpa, "Msg fs-ran\n");
diff --git a/src/ninep/events.zig b/src/ninep/events.zig
index 8f66cf3a..cb31e2aa 100644
--- a/src/ninep/events.zig
+++ b/src/ninep/events.zig
@@ -121,6 +121,8 @@ pub fn noteRetire(p: *Pardes, pane: *Pane) void {
     tree.pty.shellGone(p, pane);
     p.fs.news = true; // a read held on its event or pty/data hears it went
     p.fs.listeners -|= pane.fs.readers;
+    // A write parked on its lock goes again, and finds the pane gone.
+    if (pane.fs.lock != null) pardes.turn.parked = true;
     if (pane.fs.unannounced) {
         pane.fs.unannounced = false;
         return;
diff --git a/src/ninep/pane.zig b/src/ninep/pane.zig
index c03cd776..3d7834c4 100644
--- a/src/ninep/pane.zig
+++ b/src/ninep/pane.zig
@@ -46,6 +46,8 @@ pub const State = struct {
     /// The open record (tree.zig) of the pty/run waiting on this shell's
     /// current command.
     run: ?u8 = null,
+    /// The open record of the ctl open that wrote `lock` (ctl.zig).
+    lock: ?u8 = null,
     /// The host started a shell it could not teach to mark its prompts.
     unmarked: bool = false,
     /// Installed during this update; /log hears about it once the update ends
diff --git a/src/ninep/tree.zig b/src/ninep/tree.zig
index bc32ad33..c9e7b31a 100644
--- a/src/ninep/tree.zig
+++ b/src/ninep/tree.zig
@@ -122,6 +122,8 @@ pub const Open = struct {
         event: bool,
         /// The one open reading a pane's pty/data.
         pty_data,
+        /// An open that may write a pane's ctl, and so hold its lock.
+        ctl,
     } = .{ .snapshot = null },
     /// A read that found nothing yet (`.again`), kept to be answered when
     /// this open's file has something, the way factotum keeps its log's
@@ -141,7 +143,7 @@ pub const Open = struct {
             .snapshot => |bytes| if (bytes) |b| gpa.free(b),
             .log => |log| gpa.free(log.bytes),
             .run => |run| gpa.free(run.output),
-            .event, .pty_data => {},
+            .event, .pty_data, .ctl => {},
         }
         o.* = .{};
     }
@@ -577,6 +579,10 @@ fn open(p: *Pardes, req: Req, target: Target) Reply {
                 .event => if (reader and pn.fs.event_reader) return failText(req.tag, E.BUSY, e_in_use) else .{ .event = reader },
                 .pty_data => if (!reader) return .{ .tag = req.tag } else if (pn.fs.pty_reader) return failText(req.tag, E.BUSY, e_in_use) else .pty_data,
                 .pty_run => .{ .run = .{} },
+                .ctl => switch (req.omode & 3) {
+                    1, 2 => .ctl, // OWRITE, ORDWR
+                    else => return .{ .tag = req.tag },
+                },
                 else => return .{ .tag = req.tag },
             };
         },
@@ -605,7 +611,7 @@ fn open(p: *Pardes, req: Req, target: Target) Reply {
             pn.fs.event_reader = pn.fs.event_reader or reader;
         },
         .pty_data => pane_of.?.fs.pty_reader = true,
-        .run => {},
+        .run, .ctl => {},
     }
     return .{ .tag = req.tag, .handle = @intCast(i + 1) };
 }
@@ -652,6 +658,13 @@ fn releaseHandle(p: *Pardes, req: Req) void {
         .run => if (pn.fs.run == @as(u8, @intCast(req.handle - 1))) {
             pn.fs.run = null;
         },
+        // Closing the open that holds the lock gives it up, as acme's clunk
+        // of its ctlfid does (editors/acme/xfid.c:211); a write parked on
+        // `lock` goes again.
+        .ctl => if (pn.fs.lock == @as(u8, @intCast(req.handle - 1))) {
+            pn.fs.lock = null;
+            pardes.turn.parked = true;
+        },
         .snapshot, .log => {},
     };
     o.deinit(p.gpa);
diff --git a/test/fs.py b/test/fs.py
index b4b0315b..fe7e4c13 100644
--- a/test/fs.py
+++ b/test/fs.py
@@ -217,6 +217,23 @@ def discovery(binary, embedded=False):
                 client.write('/exec', b'Msg woken\n')
                 reader.join(5)
                 assert woke and woke[0].endswith(b' woken\n'), woke
+            # A pane's ctl takes acme's lock: a second open's lock waits until
+            # the holder unlocks, and nothing else waits on it meanwhile.
+            with Client(address) as other:
+                mine = client.open(f'/pane/{first}/ctl', 2)
+                client.rpc(118, struct.pack('<IQI', mine, 0, 5) + b'lock\n')
+                theirs = other.open(f'/pane/{first}/ctl', 1)
+                locked = []
+                locker = threading.Thread(target=lambda: locked.append(other.rpc(118, struct.pack('<IQI', theirs, 0, 5) + b'lock\n')), daemon=True)
+                locker.start()
+                time.sleep(.2)
+                assert not locked
+                client.write(f'/pane/{first}/addr', b'#0')
+                client.rpc(118, struct.pack('<IQI', mine, 0, 7) + b'unlock\n')
+                locker.join(5)
+                assert locked, 'a lock waiting on another open goes once it unlocks'
+                client.close(mine)
+                other.close(theirs)
             assert set(client.list('/pane')) == {'new', str(fixture), str(first), str(second)}
             client.write(f'/pane/{first}/body', b'first pane', truncate=True)
             assert client.read(f'/pane/{first}/body') == b'first pane'
@@ -287,7 +304,7 @@ def discovery(binary, embedded=False):
                 assert client.read('/src/pardes.zig').startswith(b'const std')
                 assert client.stat('/src/pardes.zig')['mode'] == 0o444
                 assert b'pub const Pardes' in client.read(f'/pane/{look(client, "/virtual/src/pardes.zig")}/body')
-    print('9P discovery: listing/stat/find are inert; new, remove, look, exec, name, sel and log behave')
+    print('9P discovery: listing/stat/find are inert; new, remove, look, exec, name, sel, log and ctl lock behave')
 
 
 def run_file(binary):