Skip to content

Commit 9c6354e

Browse files
committed
WorkspaceBuilder(fix[here]): Preserve reused sessions on startup failures
why: --here and --append reuse an existing tmux session. Startup failures must abort without destroying that live session, and duplicate target names must stop before any plugin hooks or before_script side effects run. what: - track whether build created the session before cleaning it up on failure - move the --here duplicate-session check ahead of startup hooks and script execution - add builder coverage for reused-session failures and pre-hook rename conflicts
1 parent 2b8a511 commit 9c6354e

2 files changed

Lines changed: 142 additions & 27 deletions

File tree

src/tmuxp/workspace/builder.py

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,8 @@ def build(
431431
here : bool
432432
reuse current window for first window and rename session
433433
"""
434+
session_created = session is None
435+
434436
if not session:
435437
if not self.server:
436438
msg = (
@@ -495,6 +497,19 @@ def build(
495497

496498
assert isinstance(session, Session)
497499

500+
# Check --here rename conflicts before plugin hooks, before_script,
501+
# or any session/window mutation with user-visible side effects.
502+
if here:
503+
session_name = self.session_config["session_name"]
504+
if session.name != session_name:
505+
existing = self.server.sessions.get(
506+
session_name=session_name,
507+
default=None,
508+
)
509+
if existing is not None:
510+
msg = f"cannot rename to {session_name!r}: session already exists"
511+
raise exc.TmuxpException(msg)
512+
498513
for plugin in self.plugins:
499514
plugin.before_workspace_builder(self.session)
500515

@@ -529,22 +544,16 @@ def build(
529544
),
530545
},
531546
)
532-
self.session.kill()
547+
if session_created:
548+
self.session.kill()
533549
raise
534550
finally:
535551
if self.on_build_event:
536552
self.on_build_event({"event": "before_script_done"})
537553

538-
# Check for rename conflicts early, before any session mutation
539554
if here:
540555
session_name = self.session_config["session_name"]
541556
if session.name != session_name:
542-
existing = self.server.sessions.get(
543-
session_name=session_name, default=None
544-
)
545-
if existing is not None:
546-
msg = f"cannot rename to {session_name!r}: session already exists"
547-
raise exc.TmuxpException(msg)
548557
session.rename_session(session_name)
549558

550559
if "options" in self.session_config:

tests/workspace/test_builder.py

Lines changed: 125 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1224,18 +1224,17 @@ def test_before_script_throw_error_if_retcode_error(
12241224

12251225
builder = WorkspaceBuilder(session_config=workspace, server=server)
12261226

1227-
with temp_session(server) as sess:
1228-
session_name = sess.name
1229-
assert session_name is not None
1227+
session_name = workspace["session_name"]
1228+
assert isinstance(session_name, str)
12301229

1231-
with (
1232-
caplog.at_level(logging.ERROR, logger="tmuxp.workspace.builder"),
1233-
pytest.raises(exc.BeforeLoadScriptError),
1234-
):
1235-
builder.build(session=sess)
1230+
with (
1231+
caplog.at_level(logging.ERROR, logger="tmuxp.workspace.builder"),
1232+
pytest.raises(exc.BeforeLoadScriptError),
1233+
):
1234+
builder.build()
12361235

1237-
result = server.has_session(session_name)
1238-
assert not result, "Kills session if before_script exits with errcode"
1236+
result = server.has_session(session_name)
1237+
assert not result, "Kills created session if before_script exits with errcode"
12391238

12401239
error_records = [r for r in caplog.records if r.levelno == logging.ERROR]
12411240
assert len(error_records) >= 1
@@ -1259,17 +1258,124 @@ def test_before_script_throw_error_if_file_not_exists(
12591258

12601259
builder = WorkspaceBuilder(session_config=workspace, server=server)
12611260

1262-
with temp_session(server) as session:
1263-
session_name = session.name
1261+
session_name = workspace["session_name"]
1262+
assert isinstance(session_name, str)
1263+
1264+
with pytest.raises((exc.BeforeLoadScriptNotExists, OSError)):
1265+
builder.build()
1266+
result = server.has_session(session_name)
1267+
assert not result, "Kills created session if before_script doesn't exist"
1268+
1269+
1270+
class ReusedSessionBeforeScriptFailureFixture(t.NamedTuple):
1271+
"""Fixture for before_script failures on reused sessions."""
1272+
1273+
test_id: str
1274+
fixture_name: str
1275+
build_kwargs: dict[str, bool]
1276+
expected_exception: type[BaseException] | tuple[type[BaseException], ...]
1277+
1278+
1279+
REUSED_SESSION_BEFORE_SCRIPT_FAILURE_FIXTURES: list[
1280+
ReusedSessionBeforeScriptFailureFixture
1281+
] = [
1282+
ReusedSessionBeforeScriptFailureFixture(
1283+
test_id="append-retcode-error-keeps-session",
1284+
fixture_name="workspace/builder/config_script_fails.yaml",
1285+
build_kwargs={"append": True},
1286+
expected_exception=exc.BeforeLoadScriptError,
1287+
),
1288+
ReusedSessionBeforeScriptFailureFixture(
1289+
test_id="here-retcode-error-keeps-session",
1290+
fixture_name="workspace/builder/config_script_fails.yaml",
1291+
build_kwargs={"here": True},
1292+
expected_exception=exc.BeforeLoadScriptError,
1293+
),
1294+
ReusedSessionBeforeScriptFailureFixture(
1295+
test_id="here-missing-script-keeps-session",
1296+
fixture_name="workspace/builder/config_script_not_exists.yaml",
1297+
build_kwargs={"here": True},
1298+
expected_exception=(exc.BeforeLoadScriptNotExists, OSError),
1299+
),
1300+
]
1301+
1302+
1303+
@pytest.mark.parametrize(
1304+
list(ReusedSessionBeforeScriptFailureFixture._fields),
1305+
REUSED_SESSION_BEFORE_SCRIPT_FAILURE_FIXTURES,
1306+
ids=[f.test_id for f in REUSED_SESSION_BEFORE_SCRIPT_FAILURE_FIXTURES],
1307+
)
1308+
def test_before_script_failure_on_reused_session_keeps_session(
1309+
server: Server,
1310+
test_id: str,
1311+
fixture_name: str,
1312+
build_kwargs: dict[str, bool],
1313+
expected_exception: type[BaseException] | tuple[type[BaseException], ...],
1314+
) -> None:
1315+
"""before_script failures do not kill reused sessions for append or here."""
1316+
fixture_template = test_utils.read_workspace_file(fixture_name)
1317+
workspace_yaml = fixture_template.format(
1318+
script_failed=FIXTURE_PATH / "script_failed.sh",
1319+
script_not_exists=FIXTURE_PATH / "script_not_exists.sh",
1320+
)
1321+
workspace = ConfigReader._load(fmt="yaml", content=workspace_yaml)
1322+
workspace = loader.expand(workspace)
1323+
workspace = loader.trickle(workspace)
1324+
1325+
builder = WorkspaceBuilder(session_config=workspace, server=server)
12641326

1327+
with temp_session(server) as current_session:
1328+
session_name = current_session.name
12651329
assert session_name is not None
1266-
temp_session_exists = server.has_session(session_name)
1267-
assert temp_session_exists
1268-
with pytest.raises((exc.BeforeLoadScriptNotExists, OSError)) as excinfo:
1269-
builder.build(session=session)
1270-
excinfo.match(r"No such file or directory")
1271-
result = server.has_session(session_name)
1272-
assert not result, "Kills session if before_script doesn't exist"
1330+
workspace["session_name"] = session_name
1331+
builder.session_config = workspace
1332+
1333+
with pytest.raises(expected_exception):
1334+
builder.build(session=current_session, **build_kwargs)
1335+
1336+
assert server.has_session(session_name), test_id
1337+
1338+
1339+
def test_here_mode_duplicate_session_name_fails_before_startup_hooks(
1340+
server: Server,
1341+
tmp_path: pathlib.Path,
1342+
) -> None:
1343+
"""--here rename conflicts abort before plugins or before_script run."""
1344+
before_script_marker = tmp_path / "before-script-ran"
1345+
plugin_marker = tmp_path / "plugin-ran"
1346+
1347+
workspace: dict[str, t.Any] = {
1348+
"session_name": "existing-blocker",
1349+
"before_script": f"touch {before_script_marker}",
1350+
"windows": [{"window_name": "main", "panes": [{"shell_command": []}]}],
1351+
}
1352+
workspace = loader.expand(workspace)
1353+
workspace = loader.trickle(workspace)
1354+
1355+
class PluginProbe:
1356+
"""Minimal plugin stub for startup-hook ordering tests."""
1357+
1358+
def before_workspace_builder(self, session: Session) -> None:
1359+
plugin_marker.touch()
1360+
1361+
builder = WorkspaceBuilder(
1362+
session_config=workspace,
1363+
server=server,
1364+
plugins=[PluginProbe()],
1365+
)
1366+
1367+
with (
1368+
temp_session(server) as current_session,
1369+
temp_session(server) as existing_session,
1370+
):
1371+
existing_session.rename_session("existing-blocker")
1372+
with pytest.raises(exc.TmuxpException, match="session already exists"):
1373+
builder.build(session=current_session, here=True)
1374+
1375+
assert current_session.name is not None
1376+
assert not before_script_marker.exists()
1377+
assert not plugin_marker.exists()
1378+
assert server.has_session(current_session.name)
12731379

12741380

12751381
def test_before_script_true_if_test_passes(

0 commit comments

Comments
 (0)