From dfd7e8919adde45577e98ccbd084f11ce1f2eb4b Mon Sep 17 00:00:00 2001 From: Rich Thanki Date: Thu, 4 Jun 2026 12:48:50 +0000 Subject: [PATCH] adds regression test --- src/fibers/io/exec.lua | 81 ++++++++++++++++++++++++++++++++++----- tests/test_io-exec.lua | 87 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 159 insertions(+), 9 deletions(-) diff --git a/src/fibers/io/exec.lua b/src/fibers/io/exec.lua index b48c45d..6d5cea9 100644 --- a/src/fibers/io/exec.lua +++ b/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) diff --git a/tests/test_io-exec.lua b/tests/test_io-exec.lua index 294c5ae..40be8f8 100644 --- a/tests/test_io-exec.lua +++ b/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()