mcp/developer: Refactor to use tokio SplitStream

[cgwalters]

I was working on a previous PR with a lot larger output set sizes and I think I was seeing the last chunk of output sometimes being lost.

That said I asked Gemini to try to reproduce the failure with a case and I couldn't get it to fail offhand. In looking at the original code here it looks OK...

Async rust is hard; tokio::select! in particular can cause subtle bugs. For a lot more on this see
https://blog.yoshuawuyts.com/futures-concurrency-3 and various previous and future blog entries.

Anyways this code is I think cleaner, deduplicating the code to send the notification message.

[michaelneale]

@cgwalters very interesting - I know from past experience stderro/out for shell and commands can be tricky when on different platform and when running bundled in the electron app (signed) for macos - are you able to verify it works well in that scenario as well? (lost a day or 2 to that once, different ways to spawn). How can we validate that?

edit: seems fine to me - doesn't change how the streams are started.

[michaelneale]

I do like this and does look better, I am sure there are latent bugs in the old code here so I think this one is worth a real close look, nice one @cgwalters - how has it been performing for you?

[michaelneale]

this seems to work really well, hard to know what to look for but so far so good, and much much better than old code (and I think the dependency was already pulled in from another crate so no extra cost really). This could wipe out a few tricky bugs - and yes I think the old way could lose the end of a stream if there is no newline and it ends. @DOsinga I think this may be worth getting in ASAP

very nice @cgwalters

edit, if goose is right about this, very big:

1. Deadlock Scenario with tokio::select!

The old code could hang if one stream closes while the other still has data:

// Old code
loop {
    tokio::select! {
        n = stdout_reader.read_until(b'\n', &mut stdout_buf), if !stdout_done => {
            if n? == 0 {
                stdout_done = true;
            }
            // ... process stdout
        }
        
        n = stderr_reader.read_until(b'\n', &mut stderr_buf), if !stderr_done => {
            if n? == 0 {
                stderr_done = true;
            }
            // ... process stderr
        }
        
        else => break,
    }
}

[cgwalters]

and yes I think the old way could lose the end of a stream if there is no newline and it ends.

You're right! I asked AI to generate a test case based on that and indeed it fails with the old code and passes with the new.

[michaelneale]

yeah have given this a good run and seems solid.