fix(task run): --wait must not return a previous attempt's completion (#608)

[scttfrdmn]

The symptom, and why it is severe

spawn task run --spec <f> --wait could report the previous attempt's verdict
when a task was re-run with the same task_id. The reporter fixed a spec, re-ran,
and was told failed 141 with the old timestamps while the new instance was
still booting — and that instance went on to exit 0. It happened twice in one
session, and both times the natural read was "my fix didn't work", so working code
got re-debugged.

This is a false negative in the one place that is supposed to be authoritative,
and the failure mode is quiet: the stale record is a real record of a real
earlier failure, so nothing looks corrupt — it just describes the wrong run.
Re-running after a fix is the single most common reason to invoke --wait, which
is precisely when a leftover record exists and is most likely to mislead.

Root cause

Two facts combine:

• The on-instance wrapper writes to $RESULTS_PREFIX/completion.json =
  s3://<bucket>/tasks/<task_id>/completion.json — the same key for every run
  of the same task_id (and the same for the sibling .exitcode).
• CompletionRecord had task_id/started_at/ended_at but no run
  identity, so attempt N was indistinguishable from attempt N−1. pollCompletion
  fetched the key and returned whatever was there.

The fix — both layers

1. Clear the stale record at launch (structural). runTaskReal deletes
tasks/<task_id>/completion.json and tasks/<task_id>/.exitcode before
launcher.Provision, so there is no stale record to mis-read in the first place.
Best-effort by design: deleting an absent key is a no-op success in S3 (the normal
first-run case), and a delete failure logs a warning to stderr and launches
anyway — the run-id check is the suspenders. New aws.Client.DeleteS3Object
normalizes NoSuchKey/NotFound to nil for the same reason.

2. Stamp a run id and verify it (detectable). CompletionRecord gains an
additive run_id; runTaskReal mints one per launch (github.com/google/uuid,
already in the tree) and passes it to the wrapper generator; --wait ignores
any record whose run_id is not the one it just launched and keeps polling,
announcing the skip. If the only record that ever appears belongs to an earlier
attempt, --wait now times out with an error that says so instead of returning
it. The launched run id is printed in the launch summary and in
printCompletion, so "which attempt is this?" is answerable from the output.

Explicitly NOT renaming the completion key

The issue floats completion-<runid>.json. Rejected:
tasks/<task_id>/completion.json is the published workflow-adapter contract
polled by six adapter repos — nf-spawn, miniwdl-spawn, cwl-spawn,
snakemake-executor-plugin-spawn, airflow-spawn, pegasus-spawn. Renaming it is a
cross-repo breaking change for no gain, whereas an additive JSON field is
invisible to them (unknown fields are ignored in every one of those parsers).

Back-compat: what an empty run_id means

An empty run_id can only come from a pre-fix wrapper (the wrapper now emits
the field unconditionally, and the pooled path mints its own id). Such a record is
accepted, with a loud warning that it predates run-id stamping and cannot be
attributed to this run. Rationale: treating "unverifiable" as "keep polling" would
hang every legitimate old-wrapper run until its TTL and then fail it — turning a
rare misattribution risk into a guaranteed outage for anyone on a cached
wrapper/AMI. Layer 1 already deleted both keys moments earlier, so a record
present now is overwhelmingly likely to be this run's. An announced unverifiable
answer beats an unexplained timeout. The warning is asserted in a test so it can't
quietly disappear.

Pooled path

GeneratePooledJobScript (the #70 per-job path) stamps a run id too, minted per
execution in pkg/taskpool's ScriptExecer — a pooled worker overwrites the
same key on every execution of a task_id, so its records need attempt identity
for the same reason. No pooled submitter verifies it today (it never learns the
id, because the dispatcher has no launch step to mint it in); what it buys now is
that "run_id is empty" keeps its single meaning of "written by a pre-fix
wrapper". When the dispatch path grows a submitter-side identity, threading it in
gets verification for free.

Adapter blast radius (note, not a blocker)

The adapters poll the same key, so any adapter that reuses a task_id
inherits the same false negative — they do not check run_id. In practice they
mint a unique id per run (per-task/per-attempt work-dir naming), so this is a note
rather than a blocker. The new field is available to them whenever they want to
harden the same way.

Tests (unit only, zero AWS, zero launches)

A narrow taskResultStore interface (GetS3Object/DeleteS3Object, satisfied by
*aws.Client) is the only new seam; runTaskReal is untouched in shape.

• a record with a different run_id is not accepted — polling continues, the
  skip is announced exactly once, and the matching record is what gets returned
• a record with a matching run_id is accepted, with no warning
• an empty run_id record is accepted and warns (asserted)
• stale-only → timeout error naming the earlier attempt
• runID == "" caller (no run identity) keeps the pre-fix accept-anything behavior
• the pre-launch delete is issued for both keys, and a delete error is
  reported (for logging) without skipping the second key
• the wrapper emits run_id into the completion heredoc (one-instance and
  pooled), before the upload; an exec-based test runs the generated script with a
  stand-in aws and asserts the record it writes is valid JSON and parses with the
  right run_id
• CompletionRecord additive guard: a record with no run_id still parses
  (RunID ""), a set one round-trips, and unset stays omitted

Gates: gofmt clean, go vet ./..., go build ./..., go test ./... all green;
all 11 lambda/* modules still build+vet. staticcheck ./... finding list is
byte-identical to the pre-change baseline (61 findings, all pre-existing).
make gen-docs re-run and docs-gen/task.md committed (the task run long help
gained a paragraph on the re-run guarantee).

Notes

• One new permission expectation for the CLI caller (not the instance role):
  s3:DeleteObject on the per-account results bucket. Without it the pre-launch
  clear prints a warning and the launch proceeds, with the run-id check covering
  correctness.
• CHANGELOG.md updated under ## [Unreleased] → Fixed. No version promoted;
  recommended bump is PATCH.

Fixes #608

[codecov]

Codecov Report

❌ Patch coverage is 64.38356% with 26 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/aws/s3.go	0.00%	14 Missing ⚠️
cmd/task.go	76.92%	12 Missing ⚠️

📢 Thoughts on this report? Let us know!