exec: detaches scope finaliser on termination

src/fibers/io/exec.lua

@@ -64,6 +64,8 @@ local DEFAULT_SHUTDOWN_GRACE = 1.0
 ---@field _code integer|nil
 ---@field _signal integer|nil
 ---@field _err string|nil
+---@field _finalizer_detach fun():boolean|nil
+---@field _cleaned boolean
 local Command = {}
 Command.__index = Command
 
@@ -160,6 +162,49 @@ function Command:_record_exit(code, signal, err)
 	self._code, self._signal = code, signal
 end
 
+--- Detach the scope finaliser once command-owned resources have been retired.
+---
+--- Long-lived service scopes may create many short-lived Command objects.
+--- The finaliser closure captures the Command, so leaving it installed until
+--- scope shutdown retains every completed command. Terminal commands therefore
+--- detach their finaliser once their status has been recorded.
+function Command:_detach_finalizer()
+	local detach = self._finalizer_detach
+	if detach then
+		self._finalizer_detach = nil
+		pcall(detach)
+	end
+end
+
+--- Release command-owned resources after the backend has reached a terminal
+--- state. This path is deliberately immediate and non-yielding so it is safe
+--- from Op wrap callbacks and from scope finalisers.
+---
+--- The terminal status/code/signal/error fields are left intact. run_op() and
+--- shutdown_op() check _done before touching the backend, so repeated waits
+--- remain idempotent after cleanup.
+function Command:_cleanup_terminal()
+	if self._cleaned then return end
+	if not self._done then return end
+
+	self:_detach_finalizer()
+
+	for _, name in ipairs { 'stdin', 'stdout', 'stderr' } do
+		local cfg = self['_' .. name]
+		if cfg and cfg.stream and cfg.owned then
+			pcall(function () cfg.stream:terminate('exec_terminal') end)
+			cfg.stream = nil
+		end
+	end
+
+	if self._proc and self._proc.backend then
+		pcall(function () self._proc.backend:close() end)
+		self._proc.backend = nil
+	end
+	self._proc = nil
+	self._cleaned = true
+end
+
 --- Ensure the process has been started and a ProcHandle exists.
 ---@return boolean ok
 ---@return ProcHandle|nil proc
@@ -194,6 +239,7 @@ function Command:_ensure_started()
 		self._status = 'failed'
 		self._done   = true
 		self._err    = start_err
+		self:_cleanup_terminal()
 		return false, nil, start_err
 	end
 
@@ -389,30 +435,38 @@ end
 
 function Command:run_op()
 	return op.guard(function ()
+		-- Repeated waits must remain idempotent even after terminal cleanup has
+		-- detached the finaliser and released the backend handle.
+		if self._done then
+			return op.always(self._status, self._code, self._signal, self._err)
+		end
+
 		local ok, proc, err = self:_ensure_started()
 		if not ok or not proc then
 			return op.always('failed', nil, nil, err)
 		end
-		if self._done then
-			return op.always(self._status, self._code, self._signal, self._err)
-		end
 
 		return proc.backend:wait_op():wrap(function (...)
 			self:_record_exit(...)
-			return self._status, self._code, self._signal, self._err
+			local status, code, signal, wait_err = self._status, self._code, self._signal, self._err
+			self:_cleanup_terminal()
+			return status, code, signal, wait_err
 		end)
 	end)
 end
 
 function Command:shutdown_op(grace)
 	return op.guard(function ()
+		-- Repeated shutdown/wait calls after terminal cleanup should report the
+		-- cached terminal status without requiring a backend handle.
+		if self._done then
+			return op.always(self._status, self._code, self._signal, self._err)
+		end
+
 		local ok, proc, err = self:_ensure_started()
 		if not (ok and proc) then
 			return op.always('failed', nil, nil, err)
 		end
-		if self._done then
-			return op.always(self._status, self._code, self._signal, self._err)
-		end
 
 		local g = grace or self._shutdown_grace or DEFAULT_SHUTDOWN_GRACE
 
@@ -457,8 +511,10 @@ function Command:shutdown_op(grace)
 			-- otherwise falling back to raw waiting.
 			local code2, signal2, err2 = perform_with_scope_or_raw(proc.backend:wait_op())
 			self:_record_exit(code2, signal2, err2)
+			local status, code, signal = self._status, self._code, self._signal
 			local err_final = kill_err or err2
-			return self._status, self._code, self._signal, err_final
+			self:_cleanup_terminal()
+			return status, code, signal, err_final
 		end)
 	end)
 end
@@ -547,6 +603,7 @@ function Command:_shutdown_uninterruptible(grace)
 		-- Ensure the process is waited for (uninterruptible).
 		local code2, signal2, err2 = op.perform_raw(proc.backend:wait_op())
 		self:_record_exit(code2, signal2, err2)
+		self:_cleanup_terminal()
 
 		-- Preserve any earlier status data if present.
 		-- (status/code/signal/e are unused here by design.)
@@ -563,6 +620,8 @@ end
 ----------------------------------------------------------------------
 
 function Command:_on_scope_exit()
+	if self._cleaned then return end
+
 	if self._started and not self._done then
 		-- Non-interruptible best-effort shutdown.
 		self:_shutdown_uninterruptible(self._shutdown_grace)
@@ -586,6 +645,8 @@ function Command:_on_scope_exit()
 		end
 		self._proc.backend = nil
 	end
+	self._proc = nil
+	self._cleaned = true
 end
 
 ----------------------------------------------------------------------
@@ -624,9 +685,11 @@ local function command_from_spec(spec)
 		_code           = nil,
 		_signal         = nil,
 		_err            = nil,
+		_finalizer_detach = nil,
+		_cleaned        = false,
 	}, Command)
 
-	scope:finally(function ()
+	cmd._finalizer_detach = scope:finally(function ()
 		cmd:_on_scope_exit()
 	end)
 

tests/test_io-exec.lua

@@ -13,6 +13,7 @@ local fibers = require 'fibers'
 local exec   = require 'fibers.io.exec'
 local op     = require 'fibers.op'
 local sleep  = require 'fibers.sleep'
+local scope  = require 'fibers.scope'
 local poller = require 'fibers.io.poller'
 
 ----------------------------------------------------------------------
@@ -257,6 +258,91 @@ local function wait_op_with_timeout_pattern()
 	assert(werr2 == nil, 'unexpected error on second wait: ' .. tostring(werr2))
 end
 
+-- Regression: completed commands must detach their scope finalisers.
+--
+-- Historically exec.command registered a scope finaliser that captured the
+-- Command object, but successful completion left that finaliser attached until
+-- the surrounding scope exited.  Long-lived service scopes therefore retained
+-- every completed command.  The test wraps one child scope's finally method so
+-- it can count finalisers registered by exec.command without adding any test
+-- hooks to the library.
+local function completed_commands_detach_scope_finalizers()
+	print('running: completed_commands_detach_scope_finalizers')
+
+	local N = 20
+	local attached = 0
+	local detached = 0
+	local finalisers_ran = 0
+
+	local st, _, primary = scope.run(function (s)
+		local original_finally = s.finally
+
+		s.finally = function (self, f)
+			attached = attached + 1
+			local detach = original_finally(self, function (...)
+				finalisers_ran = finalisers_ran + 1
+				return f(...)
+			end)
+
+			local detached_once = false
+			return function ()
+				if not detached_once then
+					detached_once = true
+					detached = detached + 1
+				end
+				return detach()
+			end
+		end
+
+		for i = 1, N do
+			local expected = i % 13
+			local proc = exec.command {
+				'sh', '-c', ('exit %d'):format(expected),
+				stdin  = 'null',
+				stdout = 'null',
+				stderr = 'null',
+			}
+			assert(proc, ('command creation failed at iteration %d'):format(i))
+
+			local status, code, sig, err = fibers.perform(proc:run_op())
+			assert(err == nil,
+				('wait error at iteration %d: %s'):format(i, tostring(err)))
+			assert(status == 'exited',
+				("status not 'exited' at iteration %d: %s"):format(i, tostring(status)))
+			assert(code == expected,
+				('exit code mismatch at iteration %d: got %s, expected %d')
+				:format(i, tostring(code), expected))
+			assert(sig == nil,
+				('signal not nil at iteration %d: %s'):format(i, tostring(sig)))
+
+			-- The terminal result must remain cached after cleanup.  This also
+			-- guards against regressions where cleanup releases the backend and a
+			-- later wait reports a synthetic failure.
+			local status2, code2, sig2, err2 = fibers.perform(proc:run_op())
+			assert(status2 == status,
+				('status changed between waits at iteration %d'):format(i))
+			assert(code2 == code,
+				('code changed between waits at iteration %d'):format(i))
+			assert(sig2 == sig,
+				('signal changed between waits at iteration %d'):format(i))
+			assert(err2 == nil,
+				('unexpected error on second wait at iteration %d: %s')
+				:format(i, tostring(err2)))
+		end
+	end)
+
+	assert(st == 'ok', 'scope.run failed: ' .. tostring(primary))
+	assert(attached == N,
+		('expected %d command finalisers to be attached, got %d')
+		:format(N, attached))
+	assert(detached == N,
+		('completed commands retained scope finalisers: attached=%d detached=%d')
+		:format(attached, detached))
+	assert(finalisers_ran == 0,
+		('completed command finalisers ran at scope exit instead of being detached: %d')
+		:format(finalisers_ran))
+end
+
 -- 6. shutdown: terminate a long-running process (TERM then KILL if needed).
 local function shutdown_long_running_process()
 	print('running: shutdown_long_running_process')
@@ -416,6 +502,7 @@ local function main()
 	stderr_pipe_vs_stderr_is_stdout()
 	output_op_normal_completion()
 	wait_op_with_timeout_pattern()
+	completed_commands_detach_scope_finalizers()
 	shutdown_long_running_process()
 	spawn_op_basic_usage()
 	many_short_lived_processes_stress()