Refactor `zstash` for improved maintainability

[forsyth2]

Request criteria

• I searched the zstash GitHub Discussions to find a similar question and didn't find it.
• I searched the zstash documentation.
• This issue does not match the other templates (i.e., it is not a bug report, documentation request, feature request, or a question.)

Issue description

@TonyB9000 and I made substantial improvements to zstash's structure in #416 in an effort to better support transfer tracking, but we've also noted things beyond the scope of that PR that could further improve zstash's maintainability. I'm starting this issue to track ideas for such a future refactor (whose purpose would be solely better maintainability, not just in service of fixing a bug).

[forsyth2]

Ideas from @TonyB9000:

Idea 1

Presently, we have functions hpss_get() and hpss_put(). They serve only to feed the word “get” or “put” to hpss_transfer(). They extend the call-stack needlessly. BUT: Rather than eliminate them, I want to make them REAL, and destroy “hpss_transfer()”. This function must juggle “put”, “get”, “either” throughout its body, both extending its size and complicating its analysis. We should:

• Place everything “get” into hpss_get()
• Place everything “put” into hpss_put()
• Place everything non-trivial common to BOTH “get” and “put” into one or more hpss_util functions that get called in hpss_get() and hpss_put(). If trivial, we simply repeat it.

Then, hpss_get() and hpss_put() will actually contain the codes supporting hpss get, and hpss put. I believe this will facilitate code analysis and maintenance. For starters, it will eliminate codes like this:

        transfer_word: str
        transfer_command: str
        if transfer_type == "put":
            transfer_word = "to"
            transfer_command = "put"
        elif transfer_type == "get":
            transfer_word = "from"
            transfer_command = "get"
        else:
            raise ValueError("Invalid transfer_type={}".format(transfer_type))
        logger.info("Transferring file {} HPSS: {}".format(transfer_word, file_path))

Idea 2

If these “transfer_batch” objects exist only for file deletion management, they should be called “deletion_batch” objects. As it stands, one would naturally think that a “transfer_batch” is used by transfer-processing to know what to transfer and when, AND its status. If, in a future refactor, they serve a large role in configuring actual transfer activity, THEN we can name them “transfer_batch” objects.

(This one might make sense to include in #416, but in an effort to get that PR merged by the RC deadline and leave open the option to expand its functionality later, let's leave it as is for now).

[TonyB9000]

@forsyth2 "whose purpose would be solely better readability, not just in service of fixing a bug",

More than just readability, maintainability should also improve when complexity can be reduced.

[forsyth2]

@TonyB9000 Good point, I changed the wording to reflect this. It better captures the importance.

[forsyth2]

This function must juggle “put”, “get”, “either” throughout its body, both extending its size and complicating its analysis.

I originally implemented it this way so as to not repeat code ("Don't Repeat Yourself" DRY principle). But it is a valid criticism that it produces some awkward blocks like:

        if transfer_type == "put":
            transfer_word = "to"
            transfer_command = "put"

But I suppose my counterpoint is we're always going to need to separate based on type of transfer. We have 3 dimensions/axes that come up in if/else blocks here:

• put or get?
• No hpss, hpss, or globus?
• Keep or not keep?

So, that's 2x3x2 combinations = 12 potential functions that could be written out of this.

[forsyth2]

I did a preliminary refactor of hpss_transfer() -- see here, but I don't think it's particularly straightforward to completely remove that function in place of only using hpss_put() and hpss_get().

(Again, this is just an early start; we can do more post-Unified release).

[TonyB9000]

True, we would not want 12 separate functions - but "get vs put" seems major. If we keep it as is, we should simply eliminate "hpss_get()" and "hpss_put()", and instead call "hpss_transfer()" directly..

Thing is, when I see a large "if" block with "if keep and put", and down in that block there are some "return" calls, I am unsure whether we are still in a "get" or "put" state beyond the block. A comment to the effect, "we are still doing get or put" would help clarify. But separate functions makes that unnecessary as well.

I would try to support "Don't Repeat Yourself" by extracting the repeated part into a separate callable function - unless it is a very trivial call.

[TonyB9000]

All of this is post-Unified commentary, just for capture. My plan would be:

1. make two copies of hpss_transfer, one for "get" and one for "put"
2. in "get", remove any code that is purely for "put"
3. in "put" remove any code that is purely for "get"
4. whatever remains in each that is common, seek to extract to separate function(s).

Paraphrasing some famous programmer, "Most problems can be solved with another layer of indirection", but I would add, the layer should do something more significant then merely introduce new function names.

[forsyth2]

Showing Claude the zstash source files, it has this to offer:

---

Looking at the codebase, here are the most impactful clarity improvements I'd suggest, roughly in priority order:

---

1. The TransferBatch / TransferManager abstraction is doing too much and is confusingly named

TransferBatch conflates two concerns: "a unit of work submitted to Globus" and "a list of local files to delete after transfer." These are only loosely related, and the deletion logic (checking file_paths, pruning empty batches, etc.) scattered across globus.py, hpss.py, and transfer_tracking.py is hard to follow as a result.

Suggestion: Separate file-deletion tracking from transfer-batch tracking. The simplest version: just keep a flat files_to_delete: List[str] on TransferManager and remove the deletion responsibility from TransferBatch entirely.

---

2. globus_transfer() mixes "submit this file" with "check/wait on the previous transfer" — and the non-blocking path is especially hard to follow

The function's real job is: add a file to a pending transfer, submit it when ready, and optionally block. But it reads like a state machine without that framing being stated. The comment # DIVING: Submit Transfer is doing a lot of work that the structure should do.

Suggestion: Split into:

• _ensure_transfer_submitted(...) — handles the "is the previous one still active? should I wait?" logic
• _add_file_to_pending_transfer(...) — the actual file-batching

Then globus_transfer becomes a short orchestrator calling those two.

---

3. hpss_transfer() is a single function handling four distinct cases via conditionals

The four cases are: hpss=none/put, hpss=none/get, hpss=globus, hpss=hsi. Each has different behavior. The shared function body obscures this.

Suggestion: The hpss=none path is really just "do nothing (or chmod)" — it shouldn't share a code path with actual transfers. Consider dispatching early:

def hpss_put(...):
    if hpss == "none":
        _handle_local_put(file_path, cache, is_index)
        return
    if urlparse(hpss).scheme == "globus":
        _globus_put(...)
    else:
        _hsi_put(...)

---

4. The TaskStatus plumbing is passed around but callers often ignore the return value

globus_transfer returns a TaskStatus, hpss_transfer doesn't return it, hpss_put/hpss_get don't either — but then hpss_transfer manually reads mrb.task_status after the call anyway. The return value contract is unclear.

Suggestion: Either commit to the return-value pattern (and have callers use it) or commit to the side-effect-on-batch pattern — not both. The side-effect pattern is probably simpler here.

---

5. globus_finalize orchestrates five distinct phases but they're not labeled

The phases are: (1) submit any pending unsent transfer, (2) collect all task IDs, (3) wait for all of them, (4) delete transferred files, (5) prune empty batches. The helper functions help, but the top-level function would be clearer with inline comments labeling each phase explicitly — or even better, if the helper names were more verb-forward (submit_pending, wait_for_all, delete_transferred, prune_empty).

---

6. construct_tars / TarWrapper.process_tar have a 4-step comment block that's doing the documentation job that method decomposition should do

The numbered comments # 1. Close the tar, # 2. Submit..., etc. are a sign the method wants to be split. Each numbered block could be its own method on TarWrapper or a module-level function.

---

7. Minor but pervasive: config is a global singleton mutated from many call sites

update_config(cur) silently overwrites global state mid-execution, and config.hpss gets overwritten in extract_database, update_database, and ls_database with "command line arg takes precedence" logic repeated each time. This is a common source of confusion when reading execution flow.

Suggestion: Pass a resolved config object into functions rather than mutating the global. Even a small dataclass passed explicitly would make data flow visible.

---

Summary table

Issue	Effort	Clarity gain
Split TransferBatch deletion from transfer tracking	Medium	High
Split globus_transfer into submit + check	Medium	High
Dispatch early in hpss_transfer	Low	Medium
Commit to one TaskStatus return pattern	Low	Medium
Label phases in globus_finalize	Very low	Medium
Decompose process_tar numbered steps	Low	Medium
Pass config explicitly instead of mutating global	High	High (long-term)

The biggest bang-for-buck is probably #1 and #2 — the Globus transfer state machine is where most readers get lost, and those two changes would make the before/during/after of a transfer much more apparent.

[forsyth2]

My initial attempt at further refactoring: #436 (as noted here; only covers "Refactor hpss_transfer()")

Claude's attempt at further refactoring: #445