Replace silent-failure handling, fix resource leaks and import-time side effects

Part of #108 · Phase 4 · label: tech-debt

Structural problems that produce wrong answers rather than errors. Ranked by how badly each one lies to the user.

1. Silent failure is the default

40+ except Exception: -> pass/return None sites. Failures surface as wrong results, not errors:

executor_kubernetes.py:304-308 — returns "completed" on any API exception for a tracked job. A crashed job reports success.
config.py:206 — swallows all cloud-dependency install errors inside __init__
config.py:347 + :351 — _load_default_config() runs at import time and silently swallows load errors
decorator.py:508 — _detect_remote_gpu_count returns None on any failure
decorator.py:708-716 — swallows the TypeError from broken auto-parallelization and "falls back to sequential"
filesystem.py:159 — prints a warning and continues
Establish and document the project's error policy (the .claude/CLAUDE.md philosophy — fail fast for critical config, log and continue for optional features — is a good starting point but is not what the code does)
No bare except Exception: pass. Every swallow either re-raises, or logs at warning+ with the exception attached, or is justified in a comment
Status functions never report success on error

2. Resource leaks

SFTP channel churn — upload_file/download_file/create_remote_file/remote_file_exists each open a new SFTP channel per call (executor_connections.py:234,244,254,264), while self.sftp_client created in setup_ssh_connection (:79) is opened and never used. remote_file_exists leaks the channel on exception (:264-269).
No context managers — neither ClusterExecutor nor ConnectionManager implements __enter__/__exit__; cleanup relies on __del__ (executor_core.py:304, filesystem.py:222), which is not guaranteed to run.
Thread pool leaked per decorated call — AsyncClusterExecutor(config) is constructed fresh at decorator.py:182 and :200, allocating a ThreadPoolExecutor (async_executor_simple.py:95) that shutdown() is never called on.
ClusterExecutor and ConnectionManager are context managers; documented usage uses with
One SFTP channel per connection, reused
Executors are pooled or explicitly shut down
A test asserts no file-descriptor or thread growth across repeated submissions

3. Global mutable state with import-time side effects

config.py:244 — _config = ClusterConfig() at module scope
config.py:351 — _load_default_config() executes on import
config.py:195-208 — ClusterConfig.__init__ auto-installs pip packages. Importing a library should never mutate the environment.
decorator.py:109,113,117,... — the decorator mutates the global config at call time, so behavior depends on call order
Importing clustrix has no side effects beyond defining names
No pip installation at import or config-construction time
Config is passed explicitly; global mutation from the decorator is removed or made opt-in

4. Remote code assembled by string concatenation

utils.py:1149-1186, executor_kubernetes.py:152-243, executor_cloud.py:376-414 build remote scripts by concatenating strings. utils.py:1123 emits unquoted export {var}={value}; #SBATCH --mem={memory} interpolates config straight into shell. The K8s payload nests a base64 blob inside nested quoting. This is injection-prone and effectively untestable.

Templates (Jinja2 or equivalent) with proper escaping; shlex.quote for every interpolated value
Script generation is unit-testable in isolation and has tests covering hostile inputs

5. God objects

ClusterConfig — a ~150-field dataclass mixing SSH, Kubernetes, and five clouds' credentials (config.py:13-90+)
notebook_magic_widget.py (2,040 lines) — UI + cloud SDK calls + connectivity testing
utils.py (1,788 lines) — AST analysis + environment capture + remote venv provisioning + shell templating
Config split into composable per-backend sections
utils.py split along its four existing seams

6. Job routing by string prefix

Routing falls back to job_id.startswith("lambda_") / "clustrix-job-" in four separate methods (executor_core.py:134-145, 220-231, 258-269, 409-412). wait_for_cloud_result busy-polls a plain dict mutated by a daemon thread with no lock and no timeout (executor_cloud.py:438); scheduler waits loop forever with no timeout (executor_core.py:156).

A typed job handle carrying its backend, instead of prefix sniffing
All waits have timeouts; shared state is properly synchronized

New evidence from the #109 fix (PR #129): the test suite mutates the developer's environment.

While verifying that the suite makes no billable calls, I instrumented a full run with a socket-blocking sitecustomize that logs every connection attempt with a full stack trace. Result: 3,078 connection attempts, of which 3,072 are pip install subprocesses reaching PyPI.

Stack traces confirm the origin — every one bottoms out in pip's resolver:

File ".../pip/_internal/commands/install.py", line 392, in run
File ".../pip/_internal/resolution/resolvelib/resolver.py", line 99, in resolve

This is ClusterConfig.__init__ auto-installing packages (config.py:195-208), already listed under "Global mutable state with import-time side effects" in this issue. The measurement makes the impact concrete:

Running the unit test suite installs packages into whatever environment you are in
It generates ~3,000 network round-trips, making the suite slow and flaky offline
It happens during config construction, so effectively any test that builds a ClusterConfig triggers it

This strengthens the existing AC "No pip installation at import or config-construction time" — it is not merely a design smell, it is active behaviour on every run today.

Reproduce:

cat > /tmp/nb/sitecustomize.py <<PY
import socket, traceback, os
def _deny(*a, **k):
    open("/tmp/net.log","a").write("%r\n%s" % (a[1:2], "".join(traceback.format_stack())))
    raise OSError("blocked")
socket.socket.connect = _deny; socket.create_connection = _deny
PY
PYTHONPATH=/tmp/nb pytest tests/ -m "not real_world" -o addopts="" -q --tb=no
grep -c "^" /tmp/net.log

Issue #123: Give SSH connections a timeout so an unreachable host err…

Issue #123: reuse one async executor instead of building a pool per call

Fix three fabricated-result bugs, close the RCE, and overhaul the doc…

Issue #123: bound the job wait loop

Remove every backend that has never been run against real hardware (#149

Tracked as a sub-issue of #159 (defect in current functionality). Consolidated after a three-agent audit that verified every open issue against the code; see #159 for the plan and the ordering.

Measurement: the real count of silent exception handlers is 37, package-wide

This issue says "40+". The number closest to right depends on where you look, and the earlier audit's "15, now 7" was counting only executor_*.py. Measured across the whole package with an AST walk — handlers whose entire body is pass or return None, with no logging and no re-raise:

37 handlers that swallow with no log and no re-raise

clustrix/utils.py                 13    clustrix/staging.py                3
clustrix/dependency_analysis.py    4    clustrix/auth_methods.py           3
clustrix/executor_scheduler_status.py 2  clustrix/config.py                2
clustrix/loop_analysis.py          2    clustrix/filesystem.py             2
clustrix/executor_core.py          1    clustrix/local_executor.py         1
clustrix/ssh_utils.py              1    clustrix/notebook_magic_widget.py  1
clustrix/hf_jobs.py                1    clustrix/auth_manager.py           1

So the issue's estimate was closer than the follow-up audits suggested — it was the file list that was too narrow, not the count too high.

Not all 37 are defects, and treating them as one batch would be wrong

A blanket "log everything" sweep would add noise and hide the ones that matter. Three categories are visible from the call sites:

Legitimate. executor_core.py:363 is in __del__, where an exception is unraisable anyway and swallowing is the correct behaviour. loop_analysis.py:332 _evaluate_binop returns None to mean "this expression cannot be evaluated statically", which is a design, not a failure — and that design is what replaced the eval() that used to guess.
Probably fine but worth a log line. The utils.py serialization walk (_attribute_values, _walk_referenced_modules, _dumps_by_value) swallows while probing objects it does not control. The result is still correct; a debug line would make a future by-value bug findable instead of invisible.
Genuine defects. The pattern that produced the channel leak fixed on this branch: remote_file_exists swallowed the exception, which both hid the leak and made every failure — permission denied, host unreachable, missing binary — indistinguishable from "file not found". Anywhere a caller acts on the None as if it were an answer belongs here.

What this means for scope

The current fix covers executor_core.py and executor_connections.py — 2 of the 37. That is the right place to start, because it is where the one confirmed data-loss-shaped bug lived, but this issue should not close on that basis.

The remaining 35 need a per-site decision recorded in a comment, not a sweep. utils.py holds 13 of them and is the file where a swallowed exception has historically cost the most: the environment-replication bug that silently dropped 187 of 563 packages lived there.

Fixed in `917c053` and `69f135e` — surviving two adversarial rounds

The defect that mattered most

remote_file_exists caught everything and returned False:

try:
    sftp = self.ssh_client.open_sftp()
    sftp.stat(remote_path)
    sftp.close()
    return True
except Exception:
    return False

Two consequences, and the second is the serious one.

It leaked an SFTP channel on every negative answer. sftp.stat raises for a missing file — which is this method's expected result, not an error — so sftp.close() was skipped every time the answer was "no". Reproduced: 25 calls, 25 leaked channels, and the exception swallowed so nothing reached the log.

It made "I could not tell" indistinguishable from "not there". executor_scheduler_status.py polls this method to decide whether a job has produced its result. A permission error, a dead transport, a missing binary — all returned False, which the poller reads as not finished yet. A broken connection presented as a job that ran forever.

Now narrowed to FileNotFoundError, the only exception that is an answer. An adversarial pass confirmed the narrowing is correct rather than merely narrower: a dangling symlink, a symlink loop, permission-denied and a dead transport each raise something else, so none is misread as absence.

The five silent-failure sites, decided individually

The issue estimated "40+". In these two files the real count is 5. The 37 figure is repo-wide — measured and posted above — and the discrepancy was the file list, not the count.

Site	Decision
credential-manager load	log and continue — SSH agent and default keys follow, and `connect()` raises if none work
`ssh_client is None`	raise — no connection is no evidence about the file
`open_sftp()` failure	raise — a channel that failed to open is not one to close
`sftp.stat()`	narrowed to `FileNotFoundError`
`_verify_result_signature`	already re-raised; unchanged

__del__ keeps a deliberate swallow, justified in a comment: a finaliser at interpreter shutdown has no caller to mislead.

Deterministic teardown

__enter__/__exit__ on both ClusterExecutor and ConnectionManager. The tests assert against the kernel, not paramiko's bookkeeping — transport.sock.fileno() == -1.

That distinction was itself a finding. Two tests originally asserted len(transport._channels._map) == 0 and failed; the diagnosis was that the test was wrong, not the class. Channel._unlink() returns early when the channel is already closed, so the map entry outlives the resource it describes. Asking whether the socket is gone and asking whether paramiko tidied a dict are different questions.

12 of 23 tests fail against pre-fix code — a reviewer verified that count exactly.

`sftp_client` made lazy, and the two defects that introduced

Connecting now opens zero channels. Per-call sites keep their own channels deliberately: SFTPClient multiplexes by request id and is not thread-safe, so per-call channels are what make concurrent uploads on one connection correct. Sharing would trade a real correctness property for one saved channel.

Round two found two defects in that laziness, both fixed in 69f135e:

A stale channel survived a reconnect. setup_ssh_connection() — the method the new error message tells you to call — left sftp_client bound to the dead transport: same object? True, usable: NO -> OSError: Socket is closed. Fixed by having it call disconnect() first, which also closes the old channel rather than dropping it, and incidentally fixed a pre-existing leak where a reconnect abandoned the entire old transport.
A race on first access. Four threads produced four open_sftp() calls and three orphaned channels. Now serialised by a lock covering the property, the setter, and the clearing in disconnect().

The race test is deterministic by construction rather than by timing: a wrapper counts entries into the real open_sftp and holds the first caller inside it until the others have had every chance to enter.

Still open under this issue

The remaining 32 silent handlers elsewhere in the package, listed in the measurement above. utils.py holds 13 of them, and that is where a swallowed exception has historically cost the most — the environment-replication bug that silently dropped 187 of 563 packages lived there. Those need a per-site decision, not a sweep.

Closed after five fix rounds and four adversarial reviews; the merge train is done.

Rounds 1–3 (917c053, 69f135e + successors): bare except Exception from 36 sites to near-zero, each recorded site either fixed or justified by name; remote_file_exists no longer swallowed everything (and leaked an SFTP channel doing it).
Round four (d1db83c): position-independent count enforcement in the swallow lint, detection of hook-assignment silencing (sys.excepthook, logging.disable through aliases), flaky duplicate test removed with measurements recorded.
Round five (dde208e, found uncommitted on the branch and finished): logging silencers qualified by receiver — logging.getLogger().disabled = True is caught while the widget's six button.disabled assignments stay unflagged; 29 blind spots recorded.
Successor Three silent-failure sites left over from #123: a malformed config, an empty file list, a lost traceback #168's three deliberate leftovers: fixed (closed separately).
Merged-tree state: pytest tests/unit/test_no_silent_swallows.py tests/unit/test_import_has_no_side_effects.py -q → 217 passed; whole suite 2760/0.

The import-time side effects are gone entirely: importing clustrix opens no file in $HOME or cwd (pinned by test), and a config file that cannot be used raises at first use instead of silently running your job somewhere else.

Issue #123: remote_file_exists leaked a channel every time it said "no"

Issue #123: the narrowing was undone one frame out, by this issue's o…

Issue #123: the while-loop sibling, the source tuple, and prose nobod…

Issue #164: the probe names the host now, so the assertion must too

Issue #159: campaign status — merge plan re-derived, DoD bullet 4 ver…

Issue #123: five error reports, a prose test that read no prose, and …

Issue #123: record fix round three; correct my stray-config finding; …

Issue #167: the ssh-copy-id subprocess obeys the gate and the host ke…

Issue #123: a count above family A, a swallow with no handler, and a …

Issue #123: record round four — position-independent counts, global s…

Issue #123: qualify the logging silencers by receiver, and count the …

Merge work/silent-failures into work/priorities-and-docs

Merge work/widget-apply into work/priorities-and-docs

Issue #123: a profile bundle is declined and named, not adopted and n…

Merge work/named-env into work/priorities-and-docs

Merge work/credential-gate into work/priorities-and-docs

Navigation Menu

Repository navigation

Replace silent-failure handling, fix resource leaks and import-time side effects #123

1. Silent failure is the default

2. Resource leaks

3. Global mutable state with import-time side effects

4. Remote code assembled by string concatenation

5. God objects

6. Job routing by string prefix

Measurement: the real count of silent exception handlers is 37, package-wide

Not all 37 are defects, and treating them as one batch would be wrong

What this means for scope

Fixed in `917c053` and `69f135e` — surviving two adversarial rounds

The defect that mattered most

The five silent-failure sites, decided individually

Deterministic teardown

`sftp_client` made lazy, and the two defects that introduced

Still open under this issue

6 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Replace silent-failure handling, fix resource leaks and import-time side effects #123

Description

1. Silent failure is the default

2. Resource leaks

3. Global mutable state with import-time side effects

4. Remote code assembled by string concatenation

5. God objects

6. Job routing by string prefix

Activity

jeremymanning commented on Aug 17, 2026

jeremymanning commented on Aug 20, 2026

jeremymanning commented on Aug 20, 2026

Measurement: the real count of silent exception handlers is 37, package-wide

Not all 37 are defects, and treating them as one batch would be wrong

What this means for scope

jeremymanning commented on Aug 20, 2026

Fixed in 917c053 and 69f135e — surviving two adversarial rounds

The defect that mattered most

The five silent-failure sites, decided individually

Deterministic teardown

sftp_client made lazy, and the two defects that introduced

Still open under this issue

jeremymanning commented on Aug 22, 2026

6 remaining items

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Fixed in `917c053` and `69f135e` — surviving two adversarial rounds

`sftp_client` made lazy, and the two defects that introduced