Repository navigation
Replace silent-failure handling, fix resource leaks and import-time side effects #123
Description
Activity
- added a parent issue
on Aug 17, 2026 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
sitecustomizethat logs every connection attempt with a full stack trace. Result: 3,078 connection attempts, of which 3,072 arepip installsubprocesses 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 resolveThis 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
ClusterConfigtriggers 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
- added 5 commits that reference this issue
on Aug 18, 2026 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 ispassorreturn None, with no logging and no re-raise:37 handlers that swallow with no log and no re-raiseclustrix/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 1So 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:363is in__del__, where an exception is unraisable anyway and swallowing is the correct behaviour.loop_analysis.py:332_evaluate_binopreturnsNoneto mean "this expression cannot be evaluated statically", which is a design, not a failure — and that design is what replaced theeval()that used to guess. - Probably fine but worth a log line. The
utils.pyserialization 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_existsswallowed 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 theNoneas if it were an answer belongs here.
What this means for scope
The current fix covers
executor_core.pyandexecutor_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.pyholds 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.- Legitimate.
Fixed in
917c053and69f135e— surviving two adversarial roundsThe defect that mattered most
remote_file_existscaught everything and returnedFalse: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.statraises for a missing file — which is this method's expected result, not an error — sosftp.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.pypolls this method to decide whether a job has produced its result. A permission error, a dead transport, a missing binary — all returnedFalse, 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 workssh_client is Noneraise — no connection is no evidence about the file open_sftp()failureraise — a channel that failed to open is not one to close sftp.stat()narrowed to FileNotFoundError_verify_result_signaturealready 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 bothClusterExecutorandConnectionManager. 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) == 0and 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_clientmade lazy, and the two defects that introducedConnecting now opens zero channels. Per-call sites keep their own channels deliberately:
SFTPClientmultiplexes 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 — leftsftp_clientbound to the dead transport:same object? True,usable: NO -> OSError: Socket is closed. Fixed by having it calldisconnect()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 indisconnect().
The race test is deterministic by construction rather than by timing: a wrapper counts entries into the real
open_sftpand 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.pyholds 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.- A stale channel survived a reconnect.
Closed after five fix rounds and four adversarial reviews; the merge train is done.
- Rounds 1–3 (
917c053,69f135e+ successors): bareexcept Exceptionfrom 36 sites to near-zero, each recorded site either fixed or justified by name;remote_file_existsno 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.disablethrough 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 = Trueis caught while the widget's sixbutton.disabledassignments 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.
- Rounds 1–3 (
- added a commit that references this issue
on Aug 23, 2026 6 remaining items
- added 15 commits that reference this issue
on Aug 23, 2026
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 Nonesites. 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 errorsdecorator.py:508—_detect_remote_gpu_countreturnsNoneon any failuredecorator.py:708-716— swallows theTypeErrorfrom broken auto-parallelization and "falls back to sequential"filesystem.py:159— prints a warning and continuesEstablish and document the project's error policy (the
.claude/CLAUDE.mdphilosophy — 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 atwarning+ with the exception attached, or is justified in a commentStatus functions never report success on error
2. Resource leaks
SFTP channel churn —
upload_file/download_file/create_remote_file/remote_file_existseach open a new SFTP channel per call (executor_connections.py:234,244,254,264), whileself.sftp_clientcreated insetup_ssh_connection(:79) is opened and never used.remote_file_existsleaks the channel on exception (:264-269).No context managers — neither
ClusterExecutornorConnectionManagerimplements__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 atdecorator.py:182and:200, allocating aThreadPoolExecutor(async_executor_simple.py:95) thatshutdown()is never called on.ClusterExecutorandConnectionManagerare context managers; documented usage useswithOne 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 scopeconfig.py:351—_load_default_config()executes on importconfig.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 orderImporting
clustrixhas no side effects beyond defining namesNo 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-414build remote scripts by concatenating strings.utils.py:1123emits unquotedexport {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.shlex.quotefor every interpolated value5. 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 testingutils.py(1,788 lines) — AST analysis + environment capture + remote venv provisioning + shell templatingConfig split into composable per-backend sections
utils.pysplit along its four existing seams6. 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_resultbusy-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).