fix(task): flush a completion record and the log when a lifecycle limit kills a task

[scttfrdmn]

Fixes #632. Also fixes #642, found while testing this.

The bug

A spawn task run task killed by its own TTL left nothing: no completion.json, no command.log, no task prefix in the results bucket. Verified in code — both are written by the wrapper after the user command, in sequence:

• pkg/taskproto/wrapper.go:253-256 — the log upload
• pkg/taskproto/wrapper.go:259+ — the completion.json heredoc

and grep -n trap pkg/taskproto/wrapper.go finds nothing. So they only ever run if the command returns. When a lifecycle limit fires mid-command, the log dies with the disk — and that log is exactly what you need, because spawn_phase() has already written timestamped stage-in start / stage-in done / command start markers into it.

This is distinct from the two already-fixed neighbours, both of which assume the command returns: #566 (a command that fails writes no record) and #561 (a lost output recorded as success). Here it neither succeeded nor failed — it was still running.

The fix, and why it differs from the issue's suggestions

The issue offered three shapes. I used a fourth, because a mechanism already existed that is strictly better than all three:

func (a *Agent) terminate(ctx context.Context, reason string, why taskproto.ExitReason) {
	log.Printf("Terminating instance (reason: %s)", reason)
	a.runPreStop(false, why)   // ← synchronous, bounded, BEFORE the API call
	...
	err := a.provider.Terminate(ctx, reason)

• Option 1 (a trap) only fires during OS shutdown, racing an S3 upload against network teardown — the worst possible moment to need the network. runPreStop runs while the instance is still fully healthy.
• Option 3 (agent-side record) can't work alone: the agent doesn't know $RESULTS_PREFIX. Here the hook is generated with it baked in.

Hanging it off runPreStop also means it covers every exit path spored mediates, not just TTL — agent.go:1326 (spot interruption), :1581 (stop), :1602 (hibernate), :1799 (terminate). So the identical diagnostic hole is closed for a cost-limit kill and an idle stop, which the issue doesn't mention but which lose the log the same way.

Deliberate deviation: no state: "terminated"

The issue asked for state: "terminated". I used state: "failed" instead, because CompletionRecord.State is documented completed | failed and six workflow adapters parse this exact object — a third value would send every one of them down a default branch, making this a cross-repo wire-contract change. "failed" is also true: the task produced no outputs.

The why, which is what you actually needed, rides in two places instead:

• retry_class — now populating RetryTTLExpired and RetrySpotInterruption, which completion.go:85-89 already documents as "defined but not yet populated — these need spored-side signals". This is that signal.
• terminal_reason — a new additive, omitempty field naming the limit that fired (ttl_expired, cost_limit_exceeded, idle_timeout, spot_interruption). Needed because retry_class alone can't express cost/idle, which have no matching class. Additive exactly as run_id was for task run --wait can return the PREVIOUS attempt's completion.json when re-running the same task_id (false negative after a fix) #608: adapters ignore unknown fields, and pre-A task killed by its own TTL leaves no completion record and no command.log, so there is nothing to re-size from #632 records still parse.

exit_code is -1, to distinguish "never returned a status" from a real 0 or non-zero exit.

The dangerous part, and its interlock

The success path also reaches this code: command returns → wrapper writes completion.json → SPAWN_COMPLETE → spored acts on on_complete → terminate() → runPreStop() → hook. Without a guard, the hook would relabel every normally-completed task failed/exit -1 — far worse than the bug being fixed. So the hook bails first on the wrapper's record existing, and TestFlushHookNeverOverwritesTheWrapperRecord was confirmed to fail with that guard removed.

Security

The hook is installed to /etc/spawn/task-flush.sh, root-owned 0700 — not /tmp. spored execs it as root, so a world-writable location would let any local user choose what root runs. The existing /tmp/SPAWN_COMPLETE channel is only ever read as data, which is why /tmp is fine for that and not for this. Pinned by TestTaskFlushHookIsInstalledRootOwnedAndNotWorldWritable (fails at 0755).

The exit reason is a typed token, not the human log string, so rewording "cost limit reached ($%.2f)" can never silently change a recorded retry class. A drift gate asserts every lifecycle exit site passes one.

Two real bugs caught by the tests themselves

1. The hook's PATH export was substituting, not appending — which made any aws outside the six system dirs unreachable. Not hypothetical: Ubuntu's snap package installs it to /snap/bin. A hook that can't find aws salvages nothing. TestFlushHookFindsAwsOutsideTheSystemDirs pins it.
2. Exec-based wrapper tests race on a single hardcoded /tmp/spawn-completion.json across packages #642, the cross-package test race, filed separately and fixed here since my own interlock test would otherwise have made it worse.

Not covered, stated plainly

An instance killed without spored's involvement — a direct terminate-instances, an out-of-band reaper, or host failure — still leaves nothing. spored never runs its pre-stop path there, so there is nothing to hang this off; closing it needs a signal from the terminating side (e.g. the reaper's graceful SSM path). I chose not to ship a trap as a token backstop for it, since I can't verify it works in the case it's meant for. The pooled worker path (pkg/taskpool) is also out of scope — its per-job script arrives over SSM, so the hook would need rewriting per job.

Verification

• Full suite green; gofmt, go vet, make check-docs, race detector on pkg/agent, all 11 lambda modules build.
• staticcheck diffed by message: 45 findings on main, 45 on the branch, 0 new. (My first attempt used git stash without -u, so the baseline didn't compile and reported nothing for pkg/agent — the resulting 7 "new" U1000s were pre-existing findings merely unmasked. Redone with -u.)
• Exec-based wrapper tests race on a single hardcoded /tmp/spawn-completion.json across packages #642 regression: the full suite failed 3/3 on main and passes 4/4 here.
• Every new test confirmed to fail against the pre-fix behaviour — interlock removed, PATH substituted, a token dropped, the flush moved after the PreStop gate, and 0755 installation all produce the expected red.
• No new dependencies; go.mod/go.sum untouched.

[codecov]

Codecov Report

❌ Patch coverage is 83.45324% with 23 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/agent/agent.go	50.00%	8 Missing ⚠️
cmd/task.go	0.00%	5 Missing ⚠️
pkg/taskproto/flush.go	91.80%	5 Missing ⚠️
pkg/agent/taskflush.go	88.23%	2 Missing and 2 partials ⚠️
pkg/taskproto/recorddir.go	85.71%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[scttfrdmn]

Pushed 980a0a5 — Semgrep's dangerous-exec-command was blocking.

The cause was real, not a false positive to wave through. Making the hook path testable turned it from a constant into a package variable, and the rule can't know the only writer is a test helper.

So rather than suppress it alone, I added the check the constant never provided. A constant tells you where you're about to exec; it says nothing about whether the file there is still trustworthy — and spored execs it as root, which is the whole game. verifyHookTrustworthy now refuses unless the target is:

• a regular file (not a directory, device or socket),
• not a symlink — os.Lstat rather than os.Stat, so a link planted at the path is rejected instead of silently followed,
• writable by nobody but its owner (perm & 0o022 == 0).

The root-owned /etc/spawn directory is still the primary protection; this is the residual guard for a file something else has loosened. Deliberately portable rather than a uid-0 check via syscall.Stat_t, which would need the linux/windows/other build-tag trio (sys_*.go) for a hook that only exists on Linux. Builds verified for all three.

Tests: five permission cases (0700/0750 accepted, 0770/0777/0702 refused) plus a symlinked hook. Confirmed to fail with the check removed — 0770, 0777 and 0702 all got executed.

One gotcha worth recording, since it cost a CI round-trip: a nosemgrep directive is only honoured on the offending line or the one immediately above it. My first attempt put it at the top of an explanatory comment block and it was silently ignored — semgrep reported the finding exactly as before, with no hint the suppression hadn't applied.

Verified locally against the exact CI command this time rather than guessing: semgrep scan --config=auto --error . → 0 findings across 811 files.