fix(api): honour the script's own timeoutSeconds in the stale reaper (#3190)

[bdunncompany]

Fixes #3190.

The defect

reapStaleScriptExecutions used a flat deadline for every execution:

const defaultTimeoutMs = 300 * 1000 + 5 * 60 * 1000; // 300s script + 300s grace

It never read the script's own timeoutSeconds, so it is wrong in both directions:

• a long-timeout script is reaped and stamped "no response from agent" while it is still executing correctly;
• a short-timeout script sits pending far past its own contract.

The fix

The deadline now comes from the script row through getCommandTimeoutMs, which is the same helper reapStaleDeviceCommands already uses a few lines above. That leaves one source of truth for "how long may a script take" instead of a second hardcoded copy that can drift from it.

Both the query pre-filter and the per-row re-check use it. That second one is the part worth calling out: leaving the re-check on a fixed constant would keep enforcing the old floor and make the whole change inert for exactly the short-timeout case this issue names. The tests below fail in precisely that way if it is reverted.

The pre-filter uses SCRIPT_GRACE_BUFFER_MS as a conservative floor — timeoutSeconds is a non-negative integer, so every per-script deadline is at least the grace buffer and nothing younger than that can be due. Rows are then re-checked individually, mirroring the device-command reaper's SHORTEST_TIMEOUT_MS pattern.

Safety

• script_executions.script_id is NOT NULL (db/schema/scripts.ts:114), so the innerJoin to scripts cannot silently drop rows.
• Executions whose script uses the default 300s are completely unaffected — the old constant was exactly what getCommandTimeoutMs returns for the default. Existing behaviour is preserved where it was already correct.
• SCRIPT_GRACE_BUFFER_MS is newly exported; its value is unchanged.
• No migration, no schema change, no new table or column, so there is nothing to add to the cascade or export-policy registries.

Relationship to #3097

These are separate axes of the same function and do not overlap, as discussed on the issue. #3097 (merged as #3172) decides what the reaper concludes about a row it already selected; this decides which rows are selected and when. #3172's terminal-command guard is untouched here and its tests still pass.

Worth noting the interaction runs one way: a tighter per-script deadline selects those rows sooner and in greater number, so the #3172 guard gets more load, not less.

Test evidence

Two new cases, one per direction of the defect:

Case	Script timeout	Age	Expected
short-timeout is reaped once its own deadline passes	30s (→ 5.5 min)	7 min	reaped
long-timeout is left alone while still within its deadline	1h (→ 65 min)	30 min	not reaped

Verified in both directions. Replacing only the per-script deadline with the old flat constant, leaving everything else in place:

	Result
flat deadline restored	2 failed / 34 passed — exactly the two new cases
fix in place	36 passed

Full runs on the final tree, Node 22.23.2:

• tsc --noEmit -p apps/api/tsconfig.json → exit 0, no output
• apps/api full unit suite → 1273 files passed / 5 skipped, 20199 tests passed / 61 skipped
• Targeted: staleCommandReaper, commandTimeouts → 2 files / 38 tests passed

https://claude.ai/code/session_01RUup17Z6KMH9jSkhBBhRJ1

[ToddHebebrand]

Approving. The fix is correct, the tests are genuine, and the reasoning in the body holds up everywhere I checked it independently. One comment-placement fix I'd like before merge, and one test gap worth considering.

What I verified rather than took on faith:

The conservative floor is genuinely conservative. getCommandTimeoutMs returns timeoutSeconds * 1000 + SCRIPT_GRACE_BUFFER_MS (commandTimeouts.ts:125-131) and scripts.timeout_seconds is NOT NULL DEFAULT 300 (db/schema/scripts.ts:28), so every per-script deadline is at least the buffer and no due row can fall outside the pre-filter.

The widened window doesn't crowd out due rows. This was the way the change could have gone wrong and it's worth stating explicitly: dropping the pre-filter floor from 10min to 5min makes the SELECT return strictly more rows, and it's capped by .limit(MAX_REAP_PER_RUN). What saves it is .orderBy(scriptExecutions.createdAt) ascending — the newly-included younger rows sort last, so they can only ever be truncated away, never displace an older row that is actually due. Worth knowing that the ordering is load-bearing for correctness now and not just for determinism.

The innerJoin can't silently drop executions. The FK script_executions_script_id_scripts_id_fk (migrations/0001-baseline.sql:14272) declares no ON DELETE, so it's NO ACTION, and script deletion is a soft delete via deletedAt (routes/scripts.ts:739) which you correctly do not filter on here. No orphan path exists.

Also confirmed: the #3172 terminal-command guard is untouched and still runs after the deadline check; the function runs under runWithSystemDbAccess, so joining the RLS-forced scripts table doesn't change row visibility; and the two new tests do flip when the per-script deadline is reverted — they assert reap counts against ages chosen to sit on the right side of only the new logic, not mock plumbing.

One fix before merge — a misplaced comment. Inserting the new describe block split the #3097 comment from the block it documents. staleCommandReaper.test.ts:939-943 is the #3097 terminal-guard rationale ("...89 executions read timeout while their command had completed with output"), and it now reads as the preamble to describe('reapStaleScriptExecutions per-script timeout (#3190)') at line 948, while the actual describe('reapStaleScriptExecutions terminal-command guard (#3097)') at line 1004 has nothing above it. Moving lines 939-943 down to sit above 1004 is the whole fix. Small, but it's the exact shape that has future readers attributing one issue's reasoning to another's tests.

Nice-to-have: the running reference-time branch has no coverage at all. Both new fixtures use status: 'pending' with startedAt: null, so referenceTime comes from createdAt in both. In fact grep "status: 'running'" returns nothing across the entire staleCommandReaper.test.ts — the exec.status === 'running' && exec.startedAt branch has never been tested. That was tolerable when the deadline was one flat constant; now that the deadline is per-script, "which timestamp do we measure from" and "how long is the budget" interact, and a regression in that branch would ship green. A third case — a running execution with a recent startedAt and an old createdAt, under a short script timeout — would pin it. Your call whether it belongs in this PR or a follow-up.

One note, no action needed: the header comment's "timeoutSeconds is a non-negative integer" is true today, but it's held up by app-layer Zod (routes/scripts.ts:173,188 and routes/partnerApi/schemas.ts), not a DB CHECK constraint. The invariant is real, its enforcement just lives further away than the sentence implies. Fine as is — worth knowing if anyone ever adds a write path that skips those validators.

Push the comment move and I'll merge.

[bdunncompany]

Both addressed, pushed.

Comment placement — moved the #3097 rationale back above its own describe. You were right that it's the exact shape that misattributes reasoning later; the new block landed between the comment and the tests it belonged to.

The running branch — added, and it belongs in this pull request rather than a follow-up for the reason you gave: the reference-time choice and the budget length only started interacting when the deadline became per-script, so leaving it uncovered would let a regression in either ship green. Fixture is old createdAt, recent startedAt, 30s script — measuring from createdAt reaps it, measuring from startedAt correctly does not. Control run: forcing referenceTime to createdAt fails exactly that case and leaves the other 36 passing.

Now 37 tests, tsc exit 0.

On your three verification notes — all three are things I had checked but had not written down, and two of them are worth more than the check itself:

• The ordering is load-bearing now. I had reasoned that .limit() truncation was safe because .orderBy(createdAt) ascending puts the newly-included younger rows last, but I left that out of the body. Stating it explicitly is better: that ordering is now a correctness property, not just determinism, and someone "tidying" it into a different sort would reintroduce starvation silently.
• The NO ACTION FK plus soft-delete is the part I am glad you traced independently. I verified script_id is NOT NULL and concluded the join was safe, which is necessary but not sufficient — it would still have been wrong if script deletion were a hard delete. It isn't, so no orphan path exists, but my stated reason was narrower than the real one.
• The non-negative timeoutSeconds invariant living in Zod rather than a DB CHECK is a fair correction to my comment's phrasing. The floor holds today; it just isn't enforced where the sentence implies.

[ToddHebebrand]

Re-approving — the comment now sits with its own describe, and thanks for taking the running case too. That test is well-chosen: old createdAt puts the row through the pre-filter, recent startedAt keeps it inside its own deadline, so it fails if the reference-time branch ever regresses to measuring from createdAt. Delta is test-file-only. Merging.