ENG-1113: -f/--follow for sandbox logs (client-side polling)

[davixcky]

What

Adds -f/--follow to signadot logs for the sandbox path (ENG-1113).

Stacked on #351 (ENG-1112) — base is feat/sandbox-logs; will retarget to main once #351 merges.

The sandbox-scoped endpoint is a point-in-time snapshot, so follow tails by polling on an interval (2s) and advancing sinceTime. Native pod-log streaming is a fast-follow (ENG-1115). The job path already streams via SSE and is unchanged.

Correctness

• Per-container de-dup. All containers share a single sinceTime per request, so a global high-water mark would drop lines from a lagging container. Instead each container is de-duped against its own last-printed timestamp, and the next poll uses the oldest per-container high-water mark — no lost or repeated lines across multiple containers.
• Error handling. The first poll surfaces selector errors (unknown workload/container/resource/step) so a typo fails fast; subsequent transient errors (pod not running yet, restarts) are reported once to stderr and retried.
• Graceful stop on SIGINT/SIGTERM.
• Rejected with -o json|yaml (follow streams plain lines only).

Testing (live, local cluster)

Followed a 2-container fork (app + sidecar, each logging every 1s):

$ signadot logs --sandbox follow-test --workload multi -f
[app] APP 0
[sidecar] SIDE 0
[app] APP 1
...

• ~10s of -f: 70 lines, 70 unique (0 duplicates), 35 [app] + 35 [sidecar], sequential with no gaps.
• -f -c app → single container, raw (unprefixed), tails live.
• -f -o json → Error: --follow cannot be combined with -o json.
• -f --workload nope → first poll surfaces 400 … workload "nope" not found ….
• SIGTERM/Ctrl-C → stops promptly.

🤖 Generated with Claude Code

[cursor]

Risk: medium. Cursor Bugbot was not present on this PR; evaluated using remaining signals. Approved — latest commit only re-pins go-sdk; bounded CLI follow feature unchanged. Reviewers already assigned (scott-cotton, daniel-de-vera).

  

_{Sent by Cursor Approval Agent: Pull Request Approver}

[scott-cotton]

Code Review

Thoughtful, well-scoped change. Follow is implemented client-side by polling the snapshot endpoint every 2s, advancing sinceTime, and de-duping per container. The per-container high-water-mark design (use the oldest container mark as the shared sinceTime, dedup each container against its own last-printed timestamp) is the correct approach and avoids the naive-global-watermark bug. The fetchSandboxLogs/printLogLine refactor is clean and preserves JSON/YAML behavior.

A few things worth resolving before merge:

Correctness / risks

1. Dedup can drop lines when timestamps aren't unique (main risk). The boundary filter is !t.After(last) — it skips any line <= the last-printed timestamp. If two distinct lines share the exact timestamp of the last-printed one and the snapshot boundary falls between them, the second is silently dropped:

Poll 1: prints line@T2, last["app"]=T2, since=T2
Poll 2 (sinceTime=T2) returns [T2(dup), T2b(new, same ts), T3]
        -> T2b is skipped because !T2b.After(T2), though it was never printed

At 1 line/sec (as tested) timestamps are distinct so this never fires — but with bursty logs or coarse server-side timestamp precision (second/ms rather than ns) many lines share a timestamp and lines will be lost at every poll boundary. Worth confirming the endpoint's timestamp precision. Robust fix: remember the set/count of messages seen at exactly the max timestamp and only suppress those, rather than everything <= last. At minimum, add a comment noting the limitation.

2. Catastrophic duplication if Time is empty. LogsLogItem.Time is omitempty. If the server ever returns lines without timestamps, time.Parse fails, the dedup branch is skipped so every line prints, and the high-water mark never advances — so oldestSince returns current unchanged and the entire window reprints every 2s indefinitely. Live testing implies Time is always populated, but there's no guard. Consider a message-based fallback, or at least don't reprint when timestamps are missing.

3. First-poll fail-fast also rejects not-yet-running pods. On first, any error returns immediately. Ideal for a typo'd --workload, but a user running -f right after creating a sandbox (pod not scheduled yet) gets a hard error instead of the "waiting for logs…" retry that follow implies. If the API distinguishes 4xx selector errors from transient "not running", consider hard-failing only on the former on the first poll.

Minor / style

• --follow with --job is silently ignored. logs.go explicitly documents "Reject cross-source flags rather than silently ignoring them," yet -f is accepted with --job and does nothing (the job path never reads Follow). Either wire it in, add it to the mutual-exclusion set with --job, or document the no-op.
• multi prefix can flap mid-stream. multi := len(resp) > 1 is recomputed each poll, so if a second container starts logging later, early lines print unprefixed and later ones gain [container] prefixes. Consider latching multi once true, or keying off cfg.Container == "".
• Missing SIGHUP. Almost every other command in the repo (sandbox/apply.go, plan/run.go, traffic/record.go, …) also traps syscall.SIGHUP; this long-running follow doesn't.

Tests

No tests added, and the package has no *_test.go. oldestSince is a pure function and the dedup loop is deterministic given a fixed []*models.LogsContainerLogs sequence — table-driven tests for "duplicate timestamp across poll boundary" (#1) and "lagging second container" would directly de-risk the trickiest part here and guard against regressions.

None of these block the happy path that was tested live, but #1 and #2 are the difference between "works in my test" and "correct under load." Also a reminder to retarget/rebase onto main now that #351 has landed.

🤖 Generated with Claude Code