f2cf0821
refactor: Session.freeOwned owns the memory teardown
a73x 2026-09-01 13:13
Commit message
src/server/server.zig
| Old | New | ||
|---|---|---|---|
| @@ -397,6 +397,21 @@ pub const Session = struct { | |||
| 397 | } | 397 | } |
| 398 | } | 398 | } |
| 399 | 399 | ||
| 400 | /// Free every allocation this Session owns EXCEPT two: the pty and the | ||
| 401 | /// agent socket. Those two are not memory, and each caller ends them | ||
| 402 | /// differently — `Server.deinit` reaps its child against a shared | ||
| 403 | /// deadline, `SessionTable.reap` deinits one pty on its own, and the | ||
| 404 | /// upgrade tests deliberately leave both descriptors open for the | ||
| 405 | /// adopting Server. So a caller pairs this with whatever pty and agent | ||
| 406 | /// teardown its path calls for, and anything else the session allocated | ||
| 407 | /// is handled here for all of them. | ||
| 408 | pub fn freeOwned(self: *Session, alloc: std.mem.Allocator) void { | ||
| 409 | self.tracker.deinit(alloc); | ||
| 410 | if (self.title_sent) |t| alloc.free(t); | ||
| 411 | self.freePending(alloc); | ||
| 412 | self.eng.deinit(); | ||
| 413 | } | ||
| 414 | |||
| 400 | /// Give up this session's agent socket. Called from BOTH teardown | 415 | /// Give up this session's agent socket. Called from BOTH teardown |
| 401 | /// paths — a session reaped mid-life and the whole daemon exiting — | 416 | /// paths — a session reaped mid-life and the whole daemon exiting — |
| 402 | /// and idempotent, because a session that never got one is the | 417 | /// and idempotent, because a session that never got one is the |
| @@ -872,11 +887,10 @@ pub const Server = struct { | |||
| 872 | for (&self.sessions.table) |*slot| { | 887 | for (&self.sessions.table) |*slot| { |
| 873 | if (slot.* == null) continue; | 888 | if (slot.* == null) continue; |
| 874 | const s = &slot.*.?; | 889 | const s = &slot.*.?; |
| 875 | s.tracker.deinit(self.alloc); | 890 | // `reap` only signals and waits on the child, so it is free to |
| 876 | if (s.title_sent) |t| self.alloc.free(t); | 891 | // run either side of the memory frees. |
| 877 | s.freePending(self.alloc); | ||
| 878 | s.pty.reap(reap_by); | 892 | s.pty.reap(reap_by); |
| 879 | s.eng.deinit(); | 893 | s.freeOwned(self.alloc); |
| 880 | s.closeAgent(self.alloc); | 894 | s.closeAgent(self.alloc); |
| 881 | } | 895 | } |
| 882 | // After the ptys, so every shell is gone before the files it was | 896 | // After the ptys, so every shell is gone before the files it was |
src/server/server_sessions.zig
| Old | New | ||
|---|---|---|---|
| @@ -199,14 +199,13 @@ pub const SessionTable = struct { | |||
| 199 | const c = srv.clients[i] orelse continue; | 199 | const c = srv.clients[i] orelse continue; |
| 200 | if (c.session == si) srv.dropClient(i); | 200 | if (c.session == si) srv.dropClient(i); |
| 201 | } | 201 | } |
| 202 | // Teardown in `deinit`'s order. Nulling the slot frees the name | 202 | // Nulling the slot frees the name for re-creation under a NEW |
| 203 | // for re-creation under a NEW epoch, so a client quoting this | 203 | // epoch, so a client quoting this instance's seqs resyncs by |
| 204 | // instance's seqs resyncs by snapshot. | 204 | // snapshot. What still has to be ordered is the slot itself: |
| 205 | s.tracker.deinit(srv.alloc); | 205 | // it goes null only after everything below has run, so no |
| 206 | if (s.title_sent) |t| srv.alloc.free(t); | 206 | // pass over the table can find a half-freed session. |
| 207 | s.freePending(srv.alloc); | 207 | s.freeOwned(srv.alloc); |
| 208 | s.pty.deinit(); | 208 | s.pty.deinit(); |
| 209 | s.eng.deinit(); | ||
| 210 | s.closeAgent(srv.alloc); | 209 | s.closeAgent(srv.alloc); |
| 211 | // After the listener, so nothing can be accepted into a session | 210 | // After the listener, so nothing can be accepted into a session |
| 212 | // being torn down. Normally a no-op, but a client that reattached | 211 | // being torn down. Normally a no-op, but a client that reattached |
src/server/server_test_upgrade.zig
| Old | New | ||
|---|---|---|---|
| @@ -130,10 +130,9 @@ test "initFromManifest: an adopted session answers a status_req without having b | |||
| 130 | // are left alone here and torn down once, by srv2. | 130 | // are left alone here and torn down once, by srv2. |
| 131 | { | 131 | { |
| 132 | const s = &srv.sessions.table[0].?; | 132 | const s = &srv.sessions.table[0].?; |
| 133 | s.tracker.deinit(alloc); | 133 | s.freeOwned(alloc); |
| 134 | s.freePending(alloc); | 134 | // The path string only. The listener behind it stays bound, because |
| 135 | if (s.title_sent) |t| alloc.free(t); | 135 | // the fd is one of the things srv2 adopts. |
| 136 | s.eng.deinit(); | ||
| 137 | if (s.agentPath()) |p| alloc.free(p); | 136 | if (s.agentPath()) |p| alloc.free(p); |
| 138 | } | 137 | } |
| 139 | if (srv.agents.dir) |d| alloc.free(d); | 138 | if (srv.agents.dir) |d| alloc.free(d); |
| @@ -206,10 +205,9 @@ test "initFromManifest: a session whose agent socket file vanished loses forward | |||
| 206 | // demolish exactly what srv2 is about to inherit. | 205 | // demolish exactly what srv2 is about to inherit. |
| 207 | { | 206 | { |
| 208 | const s = &srv.sessions.table[0].?; | 207 | const s = &srv.sessions.table[0].?; |
| 209 | s.tracker.deinit(alloc); | 208 | s.freeOwned(alloc); |
| 210 | s.freePending(alloc); | 209 | // The path string only. The listener behind it stays bound, because |
| 211 | if (s.title_sent) |t| alloc.free(t); | 210 | // the fd is one of the things srv2 adopts. |
| 212 | s.eng.deinit(); | ||
| 213 | if (s.agentPath()) |p| alloc.free(p); | 211 | if (s.agentPath()) |p| alloc.free(p); |
| 214 | } | 212 | } |
| 215 | if (srv.agents.dir) |d| alloc.free(d); | 213 | if (srv.agents.dir) |d| alloc.free(d); |
| @@ -280,10 +278,9 @@ test "initFromManifest: the return watermark is re-stamped, never carried across | |||
| 280 | // Memory only; the descriptors and the child are srv2's to tear down. | 278 | // Memory only; the descriptors and the child are srv2's to tear down. |
| 281 | { | 279 | { |
| 282 | const s = &srv.sessions.table[0].?; | 280 | const s = &srv.sessions.table[0].?; |
| 283 | s.tracker.deinit(alloc); | 281 | s.freeOwned(alloc); |
| 284 | s.freePending(alloc); | 282 | // The path string only. The listener behind it stays bound, because |
| 285 | if (s.title_sent) |t| alloc.free(t); | 283 | // the fd is one of the things srv2 adopts. |
| 286 | s.eng.deinit(); | ||
| 287 | if (s.agentPath()) |p| alloc.free(p); | 284 | if (s.agentPath()) |p| alloc.free(p); |
| 288 | } | 285 | } |
| 289 | if (srv.agents.dir) |d| alloc.free(d); | 286 | if (srv.agents.dir) |d| alloc.free(d); |
| @@ -405,10 +402,9 @@ test "sealAdoptedFds: the adopted fds are CLOEXEC again, and not one step before | |||
| 405 | // demolish exactly what srv2 is about to inherit. | 402 | // demolish exactly what srv2 is about to inherit. |
| 406 | { | 403 | { |
| 407 | const s = &srv.sessions.table[0].?; | 404 | const s = &srv.sessions.table[0].?; |
| 408 | s.tracker.deinit(alloc); | 405 | s.freeOwned(alloc); |
| 409 | s.freePending(alloc); | 406 | // The path string only. The listener behind it stays bound, because |
| 410 | if (s.title_sent) |t| alloc.free(t); | 407 | // the fd is one of the things srv2 adopts. |
| 411 | s.eng.deinit(); | ||
| 412 | if (s.agentPath()) |p| alloc.free(p); | 408 | if (s.agentPath()) |p| alloc.free(p); |
| 413 | } | 409 | } |
| 414 | if (srv.agents.dir) |d| alloc.free(d); | 410 | if (srv.agents.dir) |d| alloc.free(d); |