add deferred commands and relative path support for download & upload

[willr3]

No description provided.

[stalep]

1. Retry loop echo command (Sh.java)

In the retry loop, the shSync call has the closing quote wrapping the entire string including the (exit ...):

response = context.getShell().shSync("echo \"${__qdup_ec}; (exit $__qdup_ec)\"");

Wouldn't this echo the (exit ...) as literal text rather than executing it? Should the quote end before the semicolon so the exit subshell actually runs?

response = context.getShell().shSync("echo \"${__qdup_ec}\"; (exit $__qdup_ec)");

2. Removing pwd capture from postRun

Previously, postRun captured the exit code and pwd together in a single round-trip (echo "${__qdup_ec}:::$(pwd)"). With that removed, commands like QueueDownload now call shSync("pwd") individually when resolving relative paths. Could this become a regression for scripts that frequently use relative paths outside of observer contexts — where before the pwd was essentially free, now each one pays its own round-trip + delay?

3. Skipping the entire exit-code block when !shouldCheckExit

Moving shouldCheckExit(context) up to guard the entire block means that when exit code checking is disabled, no shSync runs at all — which also skips flushAndResetBuffer(). Could this leave stale output in the stream buffer and affect subsequent commands? Previously the exit code was always captured and the buffer always flushed; only the abort-on-non-zero part was conditional on shouldCheckExit.

[willr3]

1. Retry loop echo command (Sh.java)

Good catch, I will fix

2. Removing pwd capture from postRun

This is intentional. Right now we pay the performance price to check the current working directory after every sh. This only makes sense if there is a relative path queue-download observing every sh and only provides a performance gain if there are more than one relative path queue-downloads after every sh.

From my rough "napkin math" the proposed change will be a performance improvement if the ratio of queue-download that use relative paths to sh is less than 1:1. Put another way, this should improve performance for scripts with fewer relative path queue-download than sh

3. Skipping the entire exit-code block when !shouldCheckExit

Theoretically the flushAndResetBuffer() should not be needed because the buffer ended on the prompt or an abort but in reality some commands have dangling references to the standard out / err. I will move the flush to always run.

[stalep]

Great, I think this is good once those two fixes are pushed! 👍

[willr3]

1. Retry loop echo command (Sh.java)

Good catch, I will fix

2. Removing pwd capture from postRun

This is intentional. Right now we pay the performance price to check the current working directory after every sh. This only makes sense if there is a relative path queue-download observing every sh and only provides a performance gain if there are more than one relative path queue-downloads after every sh.

From my rough "napkin math" the proposed change will be a performance improvement if the ratio of queue-download that use relative paths to sh is less than 1:1. Put another way, this should improve performance for scripts with fewer relative path queue-download than sh

3. Skipping the entire exit-code block when !shouldCheckExit

Theoretically the flushAndResetBuffer() should not be needed because the buffer ended on the prompt or an abort but in reality some commands have dangling references to the standard out / err. I will move the flush to always run.

[stalep]

Exit code clobbered by deferred commands (Sh.java)

runDeferred(output, context) runs before the exit code capture (export __qdup_ec=$?). The deferred commands call shSync("pwd"), shSync("echo ~/"), etc., which overwrite $? on the remote shell. By the time we capture the exit code, it reflects the last deferred shSync — not the original command.

This would silently swallow a non-zero exit code from the original sh: command whenever deferred downloads/uploads are present.

Fix: save $? before running deferred commands:

context.getShell().shSync("export __qdup_ec=$?");
runDeferred(output, context);
// then later just echo the saved variable:
String response = context.getShell().shSync("echo \"${__qdup_ec}\"; (exit $__qdup_ec)");

Upload defer condition checks wrong variable (Upload.java)

The defer guard includes populatedPath.endsWith("/"), but populatedPath is the local source file. The shell access (mkdir -p) is needed when the destination ends with /. Should this be populatedDestination.endsWith("/")?

[willr3]

Exit code clobbered by deferred commands (Sh.java)

Great catch! The issue is bigger than the exit code checking in sh. A user could reasonably expect echo $? to work after sh and a few queue-downloads but the wrong queue-download path could break the user's sh: echo $?. This isn't isolated to queue-download, both download and upload have the same potential issue and honestly anything using the internal shSync could be a problem.

I think the broader solution is for shSync to automatically capture and preserve the exit code for any input command. WDYT?

[stalep]

I think the broader solution is for shSync to automatically capture and preserve the exit code for any input command. WDYT?

Agreed, this is a cleaner long-term solution. Making shSync transparent to $? would cover all current and future callers. The wrapping could look something like:

__qdup_saved=$?; (exit $__qdup_saved); <command>; (exit $__qdup_saved)

This works because:

1. __qdup_saved=$? saves the original exit code
2. (exit $__qdup_saved) restores $? so the command sees the correct value (e.g., export __qdup_ec=$? in the postRun exit code capture still gets the original)
3. The command runs — its output is captured normally since the wrapper produces no stdout
4. (exit $__qdup_saved) restores $? again for anything that follows

The existing exit code capture in Sh.postRun (export __qdup_ec=$?; echo "${__qdup_ec}"; (exit $__qdup_ec)) would still work correctly with this wrapping since $? is restored before the command runs.

The trade-off is a small overhead (3 extra shell operations) on every shSync call, but that seems acceptable given how much subtle breakage it prevents.

Would it make sense to split this into two steps — fix the immediate ordering issue in this PR (so we can unblock it), and then do the shSync wrapping as a follow-up? Or do you prefer to do it all here?

[stalep]

The exit code preservation via per-call shSync wrapping looks correct, and the new tests directly verify the scenario that was flagged ((exit 42) through deferred commands → assertEquals(42L, state.get("ec"))). Good coverage across all three command types and both deferred/non-deferred paths.

One item from the previous review is still open:

Upload defer condition checks wrong variable (Upload.java)

The defer guard still has populatedPath.endsWith("/") — that's the local source path. The shell access (mkdir -p) is needed when the destination ends with /. Should it be populatedDestination.endsWith("/")?

[willr3]

replaced populatedPath.endsWith("/") with populatedDestination

[stalep]

Upload defer condition fixed — LGTM. Thanks for addressing all the feedback!