Skip to content

fix(chat): fix shell command execution output markdown block issue #6995

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 10, 2025
Merged

fix(chat): fix shell command execution output markdown block issue #6995

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f02eba
fix shell command execution output markdown block issue
yueny2020 Apr 10, 2025
72a589b
Use a closure boolean value to check the first chunk
yueny2020 Apr 10, 2025
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
84 changes: 78 additions & 6 deletions packages/core/src/codewhispererChat/tools/executeBash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,14 @@ export interface CommandValidation {
warning?: string
}

// Interface for timestamped output chunks
interface TimestampedChunk {
timestamp: number
isStdout: boolean
content: string
isFirst: boolean
Copy link
Contributor

@justinmk3 justinmk3 Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be a field that actually needs a brief comment that gives insight into its purpose.

}

export class ExecuteBash {
private readonly command: string
private readonly workingDirectory?: string
Expand Down Expand Up @@ -207,8 +215,33 @@ export class ExecuteBash {
const stdoutBuffer: string[] = []
const stderrBuffer: string[] = []

let firstChunk = true
let firstStderrChunk = true
// Use a queue to maintain chronological order of chunks
// This ensures that the output is processed in the exact order it was generated by the child process.
const outputQueue: TimestampedChunk[] = []
let processingQueue = false
const firstChunk = new AtomicBoolean(true)
Copy link
Contributor

@justinmk3 justinmk3 Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I see that you are using this as a shared lock. That's different than an atomic boolean.

If you want a shared lock, use AsyncLock:

aws-toolkit-vscode/packages/core/src/amazonq/messages/chatMessageDuration.ts

Line 14 in 1da77e7

private static _asyncLock = new AsyncLock()

But this risks deadlock. It would be less risky to collect stdout and stderr separately, then combine them later, if that's important to you.

Copy link
Contributor Author

@yueny2020 yueny2020 Apr 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think as the javascript event loop is a single thread, we can use a simple boolean variable(isFirstChunk) in closure to replace the AsyncLock in my use case.

  1. The onStdout and onStderr callbacks are never truly executing simultaneously for one bash command execution - they're queued and executed one after another on the event loop.

  2. The only shared state is the simple boolean flag (isFirstChunk) that's only set once from true to false. and there is no any other async operations.

Overall, the usage of AsyncLock adds unnecessary overhead for such a simple operation.

I can remove the atomic inner class(as it causes confusion). just use var and a function.

            // Use a closure boolean value firstChunk and a function to get and set its value
            let isFirstChunk = true
            const getAndSetFirstChunk = (newValue: boolean): boolean => {
                const oldValue = isFirstChunk
                isFirstChunk = newValue
                return oldValue
            }

Copy link
Contributor

@justinmk3 justinmk3 Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set its value atomically

what is the purpose of mentioning this? there is no atomicity provided by this function.

Copy link
Contributor Author

@yueny2020 yueny2020 Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, remove it from the comment.


// Process the queue in order
const processQueue = () => {
if (processingQueue || outputQueue.length === 0) {
return
}

processingQueue = true

try {
// Sort by timestamp to ensure chronological order
outputQueue.sort((a, b) => a.timestamp - b.timestamp)

while (outputQueue.length > 0) {
const chunk = outputQueue.shift()!
ExecuteBash.handleTimestampedChunk(chunk, stdoutBuffer, stderrBuffer, updates)
}
} finally {
processingQueue = false
}
}

const childProcessOptions: ChildProcessOptions = {
spawnOptions: {
cwd: this.workingDirectory,
Expand All @@ -217,12 +250,26 @@ export class ExecuteBash {
collect: false,
waitForStreams: true,
onStdout: (chunk: string) => {
ExecuteBash.handleChunk(firstChunk ? '```console\n' + chunk : chunk, stdoutBuffer, updates)
firstChunk = false
const isFirst = firstChunk.getAndSet(false)
const timestamp = Date.now()
outputQueue.push({
timestamp,
isStdout: true,
content: chunk,
isFirst,
})
processQueue()
},
onStderr: (chunk: string) => {
ExecuteBash.handleChunk(firstStderrChunk ? '```console\n' + chunk : chunk, stderrBuffer, updates)
firstStderrChunk = false
const isFirst = firstChunk.getAndSet(false)
const timestamp = Date.now()
outputQueue.push({
timestamp,
isStdout: false,
content: chunk,
isFirst,
})
processQueue()
},
}

Expand Down Expand Up @@ -261,6 +308,17 @@ export class ExecuteBash {
})
}

private static handleTimestampedChunk(
chunk: TimestampedChunk,
stdoutBuffer: string[],
stderrBuffer: string[],
updates?: Writable
): void {
const buffer = chunk.isStdout ? stdoutBuffer : stderrBuffer
const content = chunk.isFirst ? '```console\n' + chunk.content : chunk.content
ExecuteBash.handleChunk(content, buffer, updates)
}

private static handleChunk(chunk: string, buffer: string[], updates?: Writable) {
try {
updates?.write(chunk)
Expand Down Expand Up @@ -307,3 +365,17 @@ export class ExecuteBash {
updates.end()
}
}

class AtomicBoolean {
Copy link
Contributor

@justinmk3 justinmk3 Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? javascript is single-thread, what is the purpose of this?

Copy link
Contributor Author

@yueny2020 yueny2020 Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, javascript is single-thread, but we have the use case that one process can send bash command execution output to stdout and stderr, which the streaming of stdout and stderr is async.

Copy link
Contributor

@justinmk3 justinmk3 Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

javascript has a lot of asynchrony, but each async execution context is executed fully until a async call, and is not "preemptible". getAndSet() is not doing anything useful here.

Copy link
Contributor

@justinmk3 justinmk3 Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see https://github.com/aws/aws-toolkit-vscode/pull/6995/files#r2037764657

private value: boolean

constructor(initialValue: boolean) {
this.value = initialValue
}

public getAndSet(newValue: boolean): boolean {
const oldValue = this.value
this.value = newValue
return oldValue
}
}