Skip to content

Commit fefb54d

Browse files
stalepstalep
committed
Address PR Hyperfoil#265 review feedback
- Always track pwd in postRun even when exit code checking is disabled, so queue-download relative paths work correctly in all contexts - Revert lazy pwd fetch in QueueDownload to preserve watch/observer context compatibility (shSync cannot be called from observers) - Use indexOf instead of split for parsing "exitCode:::pwd" delimiter to handle the unlikely case of ::: appearing in directory names - Handle !parsed case explicitly by logging error and aborting to avoid running in an unknown execution state - Remove benchmark test classes (ShCommandOverheadTest, ShellOverheadBenchmarkTest) as they are profiling tools not suitable for the main test suite
1 parent 77b52a8 commit fefb54d

4 files changed

Lines changed: 104 additions & 673 deletions

File tree

‎qDup-core/src/main/java/io/hyperfoil/tools/qdup/cmd/impl/QueueDownload.java‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -72,12 +72,7 @@ public void run(String input, Context context) {
7272
resolvedPath = homeDir+resolvedPath.substring("~/".length());
7373
logger.debug("resolved local env to "+resolvedPath);
7474
}else if(!resolvedPath.startsWith("/")){//relative path
75-
String pwd = context.getCwd();
76-
// Lazily fetch pwd if not yet tracked (e.g. when exit code checking is disabled)
77-
if (pwd == null || pwd.isEmpty()) {
78-
pwd = context.getShell().shSync("pwd");
79-
context.setCwd(pwd);
80-
}
75+
String pwd = context.getCwd();//use cwd instead of pwd because queue-download can be in a watch:
8176
logger.debug("resolved local env to "+resolvedPath);
8277
if(resolvedPath.startsWith("./")){
8378
resolvedPath = resolvedPath.substring("./".length());

‎qDup-core/src/main/java/io/hyperfoil/tools/qdup/cmd/impl/Sh.java‎

Lines changed: 103 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -131,100 +131,130 @@ public void postRun(String output,Context context){
131131
if(context.getShell()!=null &&
132132
context.getShell().isOpen() &&
133133
context.getShell().isPromptShell(getPreviousPrompt()) &&
134-
context.getShell().getHost().isShell() &&
135-
shouldCheckExit(context))
134+
context.getShell().getHost().isShell())
136135
{
136+
boolean checkExit = shouldCheckExit(context);
137137

138-
// Combined exit code capture + pwd + exit code restore in a single shSync
139-
// to reduce per-command overhead from ~300ms (3 round-trips) to ~100ms (1 round-trip)
140-
String combined = context.getShell().shSync("export __qdup_ec=$?; echo \"${__qdup_ec}:::$(pwd)\"; (exit $__qdup_ec)");
138+
if (checkExit) {
139+
// Combined exit code capture + pwd + exit code restore in a single shSync
140+
// to reduce per-command overhead from ~300ms (3 round-trips) to ~100ms (1 round-trip)
141+
String combined = context.getShell().shSync("export __qdup_ec=$?; echo \"${__qdup_ec}:::$(pwd)\"; (exit $__qdup_ec)");
141142

142-
String response = "";
143-
String pwd = "";
144-
boolean parsed = false;
143+
String response = "";
144+
String pwd = "";
145+
boolean parsed = false;
145146

146-
// Parse combined response: "exitCode:::pwd"
147-
if (combined != null && !combined.isBlank() && combined.contains(":::")) {
148-
String[] parts = combined.trim().split(":::", 2);
149-
if (parts.length == 2 && parts[0].matches("-?\\d+")) {
150-
response = parts[0].trim();
151-
pwd = parts[1].trim();
152-
parsed = true;
147+
// Parse combined response: "exitCode:::pwd" using indexOf to avoid
148+
// issues if ::: appears in the pwd path
149+
if (combined != null && !combined.isBlank()) {
150+
String trimmed = combined.trim();
151+
int delimIndex = trimmed.indexOf(":::");
152+
if (delimIndex > 0) {
153+
String exitPart = trimmed.substring(0, delimIndex).trim();
154+
String pwdPart = trimmed.substring(delimIndex + 3).trim();
155+
if (exitPart.matches("-?\\d+")) {
156+
response = exitPart;
157+
pwd = pwdPart;
158+
parsed = true;
159+
}
160+
}
153161
}
154-
}
155162

156-
// Fallback: if parsing failed due to noisy output from concurrent processes,
157-
// retry by re-echoing the saved variable
158-
int retry = 0;
159-
while (!parsed && retry < 5 && combined != null && !combined.isBlank() && context.getShell().isReady() && !context.isAborted()) {
160-
combined = context.getShell().shSync("echo \"${__qdup_ec}:::$(pwd)\"");
161-
if (combined != null && combined.contains(":::")) {
162-
String[] parts = combined.trim().split(":::", 2);
163-
if (parts.length == 2 && parts[0].matches("-?\\d+")) {
164-
response = parts[0].trim();
165-
pwd = parts[1].trim();
166-
parsed = true;
163+
// Fallback: if parsing failed due to noisy output from concurrent processes,
164+
// retry by re-echoing the saved variable
165+
int retry = 0;
166+
while (!parsed && retry < 5 && combined != null && !combined.isBlank() && context.getShell().isReady() && !context.isAborted()) {
167+
combined = context.getShell().shSync("echo \"${__qdup_ec}:::$(pwd)\"");
168+
if (combined != null) {
169+
String trimmed = combined.trim();
170+
int delimIndex = trimmed.indexOf(":::");
171+
if (delimIndex > 0) {
172+
String exitPart = trimmed.substring(0, delimIndex).trim();
173+
String pwdPart = trimmed.substring(delimIndex + 3).trim();
174+
if (exitPart.matches("-?\\d+")) {
175+
response = exitPart;
176+
pwd = pwdPart;
177+
parsed = true;
178+
}
179+
}
167180
}
181+
retry++;
182+
}
183+
// If we had to retry, $? was changed by the retries, so restore it
184+
if (retry > 0 && parsed) {
185+
context.getShell().shSync("(exit $__qdup_ec)");
168186
}
169-
retry++;
170-
}
171-
// If we had to retry, $? was changed by the retries, so restore it
172-
if (retry > 0 && parsed) {
173-
context.getShell().shSync("(exit $__qdup_ec)");
174-
}
175187

176-
if (parsed && context.getShell().isReady() && !context.isAborted()) {
177-
context.setCwd(pwd);
178-
if(response.matches("\\d+")){
179-
try {
180-
context.getCommandTimer().getData().set("exit_code", Integer.parseInt(response));
181-
}catch (NumberFormatException e){
188+
if (!parsed) {
189+
// All retries failed — log warning and abort to avoid running in an unknown state
190+
context.error("failed to parse exit code and pwd from postRun output [" + combined + "] for " + this);
191+
context.abort(false);
192+
} else if (context.getShell().isReady() && !context.isAborted()) {
193+
context.setCwd(pwd);
194+
if(response.matches("\\d+")){
195+
try {
196+
context.getCommandTimer().getData().set("exit_code", Integer.parseInt(response));
197+
}catch (NumberFormatException e){
198+
context.getCommandTimer().getData().set("exit_code", response);
199+
}
200+
} else {
182201
context.getCommandTimer().getData().set("exit_code", response);
183202
}
184-
} else {
185-
context.getCommandTimer().getData().set("exit_code", response);
203+
context.getShell().flushAndResetBuffer();
186204
}
187-
context.getShell().flushAndResetBuffer();
188-
}
189-
//log the output if not using stream logging
190-
if(!context.getCoordinator().getGlobals().getSetting(Globals.STREAM_LOGGING,false)){
191-
String toLog = getLogOutput(output,context);
192-
if (toLog != null && !toLog.isBlank()) {
193-
if ("0".equals(response)) {
194-
context.log(toLog);
195-
} else {
196-
context.error(toLog);
205+
//log the output if not using stream logging
206+
if(!context.getCoordinator().getGlobals().getSetting(Globals.STREAM_LOGGING,false)){
207+
String toLog = getLogOutput(output,context);
208+
if (toLog != null && !toLog.isBlank()) {
209+
if ("0".equals(response)) {
210+
context.log(toLog);
211+
} else {
212+
context.error(toLog);
213+
}
197214
}
198215
}
199-
}
200-
//abort on non-zero exit if needed
201-
if(!"0".equals(response)){
202-
boolean couldBeCtrlC = walk(CmdLocation.createTmp(), (cmd) -> {
203-
return cmd instanceof CtrlC;
204-
}).stream().anyMatch(Boolean::booleanValue);
216+
//abort on non-zero exit if needed
217+
if(parsed && !"0".equals(response)){
218+
boolean couldBeCtrlC = walk(CmdLocation.createTmp(), (cmd) -> {
219+
return cmd instanceof CtrlC;
220+
}).stream().anyMatch(Boolean::booleanValue);
205221

206-
if( !couldBeCtrlC) {
207-
Cmd cmd = this;
208-
StringBuilder stack = new StringBuilder();
209-
while(cmd!=null){
210-
if( !(cmd instanceof ScriptCmd) ){
211-
stack.append(System.lineSeparator());
212-
stack.append((cmd instanceof Script ? "script: ":"") + cmd.toString());
222+
if( !couldBeCtrlC) {
223+
Cmd cmd = this;
224+
StringBuilder stack = new StringBuilder();
225+
while(cmd!=null){
226+
if( !(cmd instanceof ScriptCmd) ){
227+
stack.append(System.lineSeparator());
228+
stack.append((cmd instanceof Script ? "script: ":"") + cmd.toString());
229+
}
230+
cmd = cmd.getParent();
231+
}
232+
if(response.matches("-?\\d+")){
233+
context.error("aborting run due to exit code ["+response+"]\n host: "+context.getShell().getHost()+"\n command: "+ this +(stack.length()>0?"\nstack:"+stack.toString():""));
234+
}else{
235+
context.error("aborting run due to unexpected characters in exit code ["+response+"] The prompt may have changed\n host: "+context.getShell().getHost()+"\n command: "+ this +(stack.length()>0?"\nstack:"+stack.toString():""));
213236
}
214-
cmd = cmd.getParent();
237+
238+
context.abort(false);
215239
}
216-
if(response.matches("-?\\d+")){
217-
context.error("aborting run due to exit code ["+response+"]\n host: "+context.getShell().getHost()+"\n command: "+ this +(stack.length()>0?"\nstack:"+stack.toString():""));
218-
}else{
219-
context.error("aborting run due to unexpected characters in exit code ["+response+"] The prompt may have changed\n host: "+context.getShell().getHost()+"\n command: "+ this +(stack.length()>0?"\nstack:"+stack.toString():""));
240+
}
241+
} else {
242+
// Exit code checking disabled — still track pwd for queue-download relative paths
243+
String pwd = context.getShell().shSync("pwd");
244+
if (pwd != null && !pwd.isBlank() && context.getShell().isReady() && !context.isAborted()) {
245+
context.setCwd(pwd.trim());
246+
}
247+
context.getShell().flushAndResetBuffer();
248+
//log the output if not using stream logging
249+
if(!context.getCoordinator().getGlobals().getSetting(Globals.STREAM_LOGGING,false)){
250+
String toLog = getLogOutput(output,context);
251+
if (toLog != null && !toLog.isBlank()) {
252+
context.log(toLog);
220253
}
221-
222-
223-
context.abort(false);
224254
}
225255
}
226256
}else{
227-
//exit code checking is disabled or shell is not a prompt shell, just log the output
257+
//shell is not a prompt shell, just log the output
228258
if(!context.getCoordinator().getGlobals().getSetting(Globals.STREAM_LOGGING,false)){
229259
String toLog = getLogOutput(output,context);
230260
if (toLog != null && !toLog.isBlank()) {

0 commit comments

Comments
 (0)