Add new --follow mode for tower apps logs with resilient streaming and tests
#171
Conversation
|
Nice thanks @burakdede, we should make the default branch in this repo I'll review the content now.! |
There was a problem hiding this comment.
Besides the failure re: linting, this is generally really good! I'd really love to merge this with the following logic in the do_run function in crates/tower-cmd/src/run.rs -- but I can manage that on our side! All the stuff around retries, etc., is really good and missing from the other implementation.
I'll leave this open for a bit just in case you decide this is something you want to tackle, but otherwise I'll rebase this against develop and land this over the weekend!
| if is_run_finished(&run) { | ||
| if let Ok(resp) = api::describe_run_logs(&config, &name, seq).await { | ||
| for line in resp.log_lines { | ||
| emit_log_if_new(&line, &mut last_line_num); | ||
| } | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
This is good, we really should unify these endpoints at some point to make this easier 🙃
| let (cancel_tx, cancel_rx) = oneshot::channel(); | ||
| cancel_monitor = Some(cancel_tx); | ||
| let run_complete = monitor_run_completion(&config, &name, seq, cancel_rx); | ||
| match api::stream_run_logs(&config, &name, seq).await { |
There was a problem hiding this comment.
This should be fine functionally, but comparing it to our implementation for starting a remote run and following the logs, we wait for the run to start first...that may not be necessary, we should be able to just start listening? I'll double check that.
There was a problem hiding this comment.
I went ahead and added a small start‑state wait before opening the log stream to mirror the remote‑run flow, since we don’t have clear backend guarantees that pre‑start streaming is valid.
So the states aren’t consistent: in apps logs --follow, terminal states are not errors; in run, they are. That behavior difference might be fine because apps logs --follow is designed to show logs for completed runs, while run is about waiting for a live run to begin.
Feel free to cherry‑pick after you double‑check backend behavior.
- deduplicate log lines across reconnects / might be an issue - add unit test for deduplication and out of order logs
burakdede
force-pushed
the
feat/follow-logs
branch
from
518721d to
709d13e
Compare
|
Awesome, thanks @burakdede really appreciate it! |
2 participants
Addresses improvement required #164
--followtotower apps logswith SSE streaming, retry/backoff, Ctrl+C handling, dedupe across reconnects, and fatal‑error exits.Tests should cover most of the critical aspects of the implementation but I have also simulated some tests via local
towerbuild and deploying app for;Broken pipeand CLI panic #162 still fails when piping to head (known issue).Drain Logs & Clean Exit
Intermittent Failure Results & Slightly-Graceful Exit
Kill Log Stream & Clean Exit