Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .env_example
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ OELEO_BASE_DIR_TO=<dir to copy to>
OELEO_FILTER_EXTENSION=<file extension to filter on (with the .)>
OELEO_LOG_DIR=<dir to put log-files in>
OELEO_DB_NAME=<name of the data-base>
# OELEO_RECONNECT=true # opt-in: reconnect before each changed file (default off; failed copies still retry once)
OELEO_DB_HOST=<db host>
OELEO_DB_PORT=<db port>
OELEO_DB_USER=<db user>
Expand Down
20 changes: 20 additions & 0 deletions .issueflows/03-solved-issues/issue17_original.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Issue #17: Stop reconnecting SSH before every file by default

Source: https://github.com/ife-bat/oeleo/issues/17

## Original issue text

## Summary
`Worker` defaults `reconnect=True` and reconnects the external connector before every file in `_process_file`. For SSH this thrash-opens Fabric sessions (plus retries inside `move_func`) — slow and fragile.

**Finding ID:** REL-01
**Size:** standard (confirm ops preference)
**Design doc:** `.issueflows/04-designs-and-guides/code-review-correctness-and-bugs.md`

## Fix direction
Default `reconnect=False`; reconnect only after a failed move (partially already done). Keep an opt-in for flaky networks if needed.

## Acceptance
- [ ] Successful multi-file SSH/local runs do not reconnect per file by default
- [ ] Failed move still retries with reconnect
- [ ] Behavior change noted for operators (changelog / README if relevant)
60 changes: 60 additions & 0 deletions .issueflows/03-solved-issues/issue17_plan.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# Plan: Issue #17 — Stop reconnecting SSH before every file by default

## Goal

Stop thrashing SSH sessions by making per-file reconnect opt-in (`Worker.reconnect=False` by default), while keeping reconnect-and-retry after a failed move. Document the behavior change for operators.

## Constraints

- Finding **REL-01** only — do not fix BUG-06 (broken `SharePointConnection.reconnect`), SEC-01 (path quoting), or SSH `move_func` inner retry loop in this PR.
- Keep public composition API: factories + `Worker` methods; constructors remain the opt-in surface unless we add a small env toggle (see Open questions).
- Behavior change is intentional and must be noted (README / env docs). No project `CHANGELOG` file today.
- Tests via `uv run pytest -m "not ssh"` (unit path); no new SSH Docker requirement for the happy-path mock test.
- Cite REL-01 in status when closing; update `code-review-correctness-and-bugs.md` / backlog when the finding is done (close step or this PR if touching docs anyway).

### Prior art

- `Worker._process_file` — [`oeleo/workers.py`](oeleo/workers.py): per-file `if self.reconnect: external_connector.reconnect()` then `move_func`; on failure, unconditional `reconnect()` + second `move_func` (keep).
- `Worker.reconnect: bool = True` — dataclass default; docstring already describes “if connection is lost” (out of sync with always-on behavior).
- `simple_worker` / `ssh_worker` — hardcode `reconnect=True`; `sharepoint_worker` relies on dataclass default.
- `Connector.reconnect` protocol + `SSHConnector.reconnect` — close/connect; `LocalConnector.reconnect` is a no-op.
- `SSHConnector.move_func` — own `CONNECTION_RETRIES` loop with `reconnect()` on failure (leave alone; complements Worker-level retry).
- Design: [`.issueflows/04-designs-and-guides/code-review-correctness-and-bugs.md`](.issueflows/04-designs-and-guides/code-review-correctness-and-bugs.md) (REL-01 + recommended test #5).
- Toolbox: none. Graph: none. Grep: no existing reconnect tests.

## Approach

1. **Flip default** — `Worker.reconnect = False`. Clarify docstring: when `True`, reconnect before each changed file; failed moves always reconnect once and retry regardless.
2. **Factories** — Remove hard-coded `reconnect=True` from `simple_worker` and `ssh_worker` (inherit `False`), or pass through an optional `reconnect=` / env (per Open questions).
3. **Leave failure path** — Keep the post-failure `external_connector.reconnect()` + second `move_func` unconditional on the flag.
4. **Opt-in for flaky networks** — At minimum, `Worker(..., reconnect=True)` and factory kwarg. Optionally honor `OELEO_RECONNECT` (`1`/`true`/`yes`) in factories so scheduled `app/oa.pyw` / `.env` can opt in without code changes.
5. **Docs** — README env section (and `.env_example` if env is added): default is no per-file reconnect; how to opt in; note that a failed copy still reconnects once.
6. **Design memory** — Mark REL-01 done in correctness + backlog docs (small durable update).

## Files to touch

| Path | Change |
|------|--------|
| [`oeleo/workers.py`](oeleo/workers.py) | Default `reconnect=False`; docstring; factories stop forcing `True` (+ optional env/kwarg) |
| [`README.md`](README.md) | Operator note + env if added |
| [`.env_example`](.env_example) | Optional `OELEO_RECONNECT` line if env is added |
| [`tests/test_oeleo.py`](tests/test_oeleo.py) (or small new test module) | Mock `external_connector`: multi-file success with `reconnect=False` → `reconnect()` only on failures (0 on all-success); `reconnect=True` → called before each changed file; failed `move_func` still triggers reconnect when flag is `False` |
| [`.issueflows/04-designs-and-guides/code-review-correctness-and-bugs.md`](.issueflows/04-designs-and-guides/code-review-correctness-and-bugs.md) | Mark REL-01 addressed |
| [`.issueflows/04-designs-and-guides/code-review-improvement-backlog.md`](.issueflows/04-designs-and-guides/code-review-improvement-backlog.md) | Strike/complete #17 |
| [`.issueflows/04-designs-and-guides/code-review-overview.md`](.issueflows/04-designs-and-guides/code-review-overview.md) | Adjust severity table for REL-01 if still listed as open |

## Test strategy

- Command: `uv run pytest -m "not ssh"` (and the new test(s) specifically).
- Prefer a focused unit test with a fake/mock connector counting `reconnect` / `move_func` — matches recommended test #5 in the correctness doc; no Fabric needed.
- Existing `simple_worker` integration-style tests should keep passing (local reconnect is already a no-op).

## Open questions

1. **Confirm default flip to `False`?** (Recommended: **yes** — matches issue acceptance and REL-01. Alternative: keep default `True` and only document opt-out — weaker fix.)
2. **Opt-in surface:** Python kwarg only on `Worker` / factories, or also **`OELEO_RECONNECT`** for `.env` / Windows scheduled runs? (Recommended: **both** — tiny, matches how ops configure oeleo today.)
3. **Version bump on close?** Behavior change for SSH operators — recommend **minor** note in PR; bump only if `/iflow-close` release policy says so for this class of change (likely patch/docs-level unless you want a minor for “operator-visible default”).

## Scope check

Single concern, few files, no unrelated refactors. Fits one PR. Does **not** need splitting.
18 changes: 18 additions & 0 deletions .issueflows/03-solved-issues/issue17_status.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Status: Issue #17

- [x] Done

## What's done

- Plan confirmed (default `reconnect=False`; kwarg + `OELEO_RECONNECT` opt-in).
- `Worker.reconnect` default flipped to `False`; docstring clarified.
- `resolve_reconnect()` + factory kwarg/`OELEO_RECONNECT` on `simple_worker`, `ssh_worker`, `sharepoint_worker`.
- Failure-path reconnect+retry left unconditional.
- README + `.env_example` operator docs.
- Unit tests for per-file skip/opt-in, fail-path reconnect, env/kwarg resolution.
- Design docs: REL-01 marked done (overview, correctness, backlog).
- `uv run pytest -m "not ssh"` — 25 passed (incl. 5 new reconnect tests).

## Remaining work

- None.
Original file line number Diff line number Diff line change
Expand Up @@ -55,21 +55,9 @@ log.debug(f"*A2O* connecting to db ({worker.db_path})")

---

### REL-01 — Reconnect before every file
### ~~REL-01 — Reconnect before every file~~ (done in #17)

`Worker` defaults `reconnect=True`. In `_process_file`:

```python
if self.reconnect:
self.external_connector.reconnect()
success = self.external_connector.move_func(...)
```

For SSH this closes/opens Fabric sessions per file (plus retries inside `move_func`). Slow and fragile under rate limits / server MaxStartups.

**Fix direction:** Default `reconnect=False`; reconnect only after failed move (already partially done). Keep env/flag for flaky networks. Add test or at least document.

**Issue size:** small behavior change — confirm with operators; **yolo-fit** if default change is accepted, else gated behind flag.
~~`Worker` defaults `reconnect=True`.~~ **Resolved:** default is `reconnect=False`; opt-in via `Worker(..., reconnect=True)`, factory `reconnect=`, or `OELEO_RECONNECT`. Failed moves still reconnect once and retry. Per-file reconnect remains available for flaky networks.

## Medium severity

Expand Down Expand Up @@ -162,4 +150,4 @@ Tray quit → hard process exit from deep inside worker. Hard to test; surprisin
2. Two same basenames in different subdirs get two DB rows and two destinations.
3. `OA_SINGLE_RUN` path does not AttributeError.
4. Failed SSH list does not look like “empty directory” without error status.
5. `reconnect=False` performs one connect for N successful copies (mock Fabric Connection).
5. ~~`reconnect=False` performs one connect for N successful copies (mock Fabric Connection).~~ — covered by unit tests in #17.
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ Do this as a **dedicated PR**, not mixed with BUG-* logic fixes. Details: [`code
| [#15](https://github.com/ife-bat/oeleo/issues/15) | Honor bookkeeping `code=2` (locked) during `Worker.run` | BUG-01 | **yolo-fit** | Unit test required · label `yolo` |
| [#14](https://github.com/ife-bat/oeleo/issues/14) | Fix `OA_SINGLE_RUN` crash: replace `worker.db_path` | BUG-03 | **yolo-fit** | Log `bookkeeper.db_name` · label `yolo` |
| [#16](https://github.com/ife-bat/oeleo/issues/16) | Peewee `processed_date` default must be callable | BUG-04 | **yolo-fit** | One-liner + test · label `yolo` |
| [#17](https://github.com/ife-bat/oeleo/issues/17) | Stop reconnecting SSH before every file by default | REL-01 | **standard** | Confirm ops preference; reconnect-on-failure only |
| ~~[#17](https://github.com/ife-bat/oeleo/issues/17)~~ | ~~Stop reconnecting SSH before every file by default~~ | ~~REL-01~~ | **done** | Default `False`; opt-in kwarg / `OELEO_RECONNECT`; fail-path retry kept |

## P1 — Data model & identity

Expand Down Expand Up @@ -103,7 +103,7 @@ Filed on GitHub (2026-07-16). Two parallel tracks (separate branches/PRs):
1. [#14](https://github.com/ife-bat/oeleo/issues/14) BUG-03 — yolo
2. [#15](https://github.com/ife-bat/oeleo/issues/15) BUG-01 — yolo
3. [#16](https://github.com/ife-bat/oeleo/issues/16) BUG-04 — yolo
4. [#17](https://github.com/ife-bat/oeleo/issues/17) REL-01 — plan + confirm
4. ~~[#17](https://github.com/ife-bat/oeleo/issues/17) REL-01~~done
5. [#18](https://github.com/ife-bat/oeleo/issues/18) SEC-01 — plan
6. [#19](https://github.com/ife-bat/oeleo/issues/19) DOC-01 — yolo

Expand Down
2 changes: 1 addition & 1 deletion .issueflows/04-designs-and-guides/code-review-overview.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ env (.env) → factories (simple_worker / ssh_worker / sharepoint_worker)
| BUG-02 | DB keys files by **basename only** → collisions when `include_subdirs=True` | correctness |
| BUG-03 | `app/oa.pyw` logs `worker.db_path` which **does not exist** on `Worker` | correctness |
| SEC-01 | SSH `find` / `md5sum` / `mkdir` built via string interpolation (shell injection risk) | security |
| REL-01 | `Worker.reconnect=True` reconnects **before every file** (SSH thrashing) | correctness |
| ~~REL-01~~ | ~~`Worker.reconnect=True` reconnects **before every file**~~ — **resolved in #17** (`reconnect=False` default; opt-in via kwarg / `OELEO_RECONNECT`) | correctness |

### Medium

Expand Down
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ OELEO_BASE_DIR_TO=C:\data\pub
OELEO_FILTER_EXTENSION=.csv
OELEO_DB_NAME=local2pub.db
OELEO_LOG_DIR=C:\oeleo\logs
# OELEO_RECONNECT=true

## only needed for advanced connectors:
# OELEO_DB_HOST=<db host>
Expand All @@ -125,6 +126,7 @@ Core transfer settings:
- `OELEO_FILTER_EXTENSION`: file extension filter (include the dot, e.g. `.csv`).
- `OELEO_DB_NAME`: sqlite database filename used for bookkeeping.
- `OELEO_LOG_DIR`: directory for log files; defaults to the current working directory.
- `OELEO_RECONNECT`: when `true` / `1` / `yes`, reconnect the destination connector before each changed file (useful on flaky networks). Default is off so SSH runs keep one session across files. A failed copy still reconnects once and retries regardless of this setting. Factories also accept a `reconnect=` kwarg that overrides the env var.

SSH connector settings:
- `OELEO_EXTERNAL_HOST`: SSH host (optionally with port, e.g. `host:2222`).
Expand Down
33 changes: 28 additions & 5 deletions oeleo/workers.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,23 @@
from oeleo.console import console
from oeleo.models import DbHandler, MockDbHandler, SimpleDbHandler
from oeleo.reporters import Reporter, ReporterBase
from oeleo.utils import to_bool

T = TypeVar("T")

log = logging.getLogger("oeleo")


def resolve_reconnect(reconnect: Union[bool, None] = None) -> bool:
"""Resolve per-file reconnect: explicit kwarg, else OELEO_RECONNECT, else False."""
if reconnect is not None:
return reconnect
raw = os.environ.get("OELEO_RECONNECT")
if raw is None or raw == "":
return False
return to_bool(raw)


def chunkify(file_list: Iterable[T], n: int = 10) -> Iterable[List[T]]:
"""Split a file-list into chunks of size n"""
file_list = iter(file_list)
Expand Down Expand Up @@ -128,11 +139,13 @@ class Worker(WorkerBase):
local_connector: Connector = None
external_connector: Connector = None

Additional optinal arguments:
Additional optional arguments:
dry_run: Bool
extension: str (with the .)
reporter: Reporter
reconnect: Bool (reconnect to the external server if connection is lost)
reconnect: Bool — when True, reconnect the external connector before each
changed file (opt-in for flaky networks). Default False. A failed move
always reconnects once and retries regardless of this flag.
external_name_generator: Callable that accepts the class instance and a string
"""

Expand All @@ -144,7 +157,7 @@ class Worker(WorkerBase):
extension: str = None
reporter: ReporterBase = Reporter()
external_name_generator: Callable[[Any, Path], Path] = field(default=None)
reconnect: bool = True
reconnect: bool = False
file_names: Iterable[Path] = field(init=False, default_factory=list)
subdirs: bool = False
external_subdirs: bool = False
Expand Down Expand Up @@ -490,6 +503,7 @@ def simple_worker(
reporter=None,
include_subdirs=False,
external_subdirs=False,
reconnect: Union[bool, None] = None,
):
"""Create a Worker for copying files locally.

Expand All @@ -503,6 +517,8 @@ def simple_worker(
include_subdirs: set to True if you want to include subdirectories.
external_subdirs: set to True if you want to include subdirectory structure in the external directory
(only makes sense if include_subdirs is True).
reconnect: when True, reconnect before each changed file. When None (default),
use OELEO_RECONNECT if set, otherwise False.

Returns:
simple worker that can copy files between two local folder.
Expand Down Expand Up @@ -541,7 +557,7 @@ def simple_worker(
extension=extension,
dry_run=dry_run,
reporter=reporter,
reconnect=True,
reconnect=resolve_reconnect(reconnect),
subdirs=include_subdirs,
external_subdirs=external_subdirs,
)
Expand All @@ -559,6 +575,7 @@ def ssh_worker(
is_posix: bool = True,
include_subdirs: bool = False,
external_subdirs: bool = False,
reconnect: Union[bool, None] = None,
):
"""Create a Worker with SSHConnector.

Expand All @@ -573,6 +590,8 @@ def ssh_worker(
reporter: reporter to use. If None, a default reporter will be used.
include_subdirs: include subdirectories when filtering local files.
external_subdirs: include subdirectories when filtering remote files.
reconnect: when True, reconnect before each changed file. When None (default),
use OELEO_RECONNECT if set, otherwise False. Failed moves still reconnect once.

Returns:
worker with SSHConnector attached to it.
Expand Down Expand Up @@ -610,7 +629,7 @@ def ssh_worker(
extension=extension,
dry_run=dry_run,
reporter=reporter,
reconnect=True,
reconnect=resolve_reconnect(reconnect),
subdirs=include_subdirs,
external_subdirs=external_subdirs,
)
Expand All @@ -626,6 +645,7 @@ def sharepoint_worker(
extension: str = None,
reporter: ReporterBase = None,
dry_run: bool = False,
reconnect: Union[bool, None] = None,
):
"""Create a Worker with SharePointConnector.

Expand All @@ -638,6 +658,8 @@ def sharepoint_worker(
extension: file extension to filter on (for example '.csv').
reporter: reporter to use. If None, a default reporter will be used.
dry_run: set to True if you would like to run without updating or moving anything.
reconnect: when True, reconnect before each changed file. When None (default),
use OELEO_RECONNECT if set, otherwise False.

Returns:
worker with SharePoint attached to it.
Expand Down Expand Up @@ -682,5 +704,6 @@ def external_name_generator(con, name):
external_name_generator=external_name_generator,
dry_run=dry_run,
reporter=reporter,
reconnect=resolve_reconnect(reconnect),
)
return worker
Loading
Loading