fix(api): stop the reaper claiming "no response from agent" when the agent replied (#3097) - #3172
Merged
ToddHebebrand merged 1 commit intoAug 7, 2026
Conversation
…agent replied (LanternOps#3097) Script results submitted over the HTTP path never reach `script_executions` — only `agentWs` registers `script: handleScriptResult` — so the row stays pending, ages past the cutoff, and lands in `reapStaleScriptExecutions`, which stamped it `timeout` with "Server-side timeout: no response from agent". That claim is false whenever a terminal `device_commands` row exists: the agent did respond, the result simply was never mirrored onto the execution. On one live instance 89 executions read `timeout` while their command had completed successfully with output captured in `device_commands.result`. The reaper now reads the related command BEFORE deciding. When that row is terminal it records the outcome the command actually reached — mapped the same way `handleScriptResult` maps it, so a reaped row agrees with what the WS path would have written — with an error message saying the result was delivered but never recorded. When the command is non-terminal, or there is no command row at all, the original wording stands, because there the claim is true. This does NOT persist stdout/stderr. Mirroring the result belongs with the shared handler-registry work; this only stops the reaper asserting something it cannot know, which is where the false `timeout` labels come from. The command lookup already existed a few lines further down, used only to find `batchId`. It moved above the update and now serves both purposes, so this adds no extra round-trip. Two consequences worth flagging in review: - The batch counter followed the same false assumption: it incremented `devicesFailed` unconditionally. A recovered success now increments `devicesCompleted` instead. Slightly beyond a literal reading of "add a guard", but leaving it would have kept the same wrong claim one level up. - `reapStaleScriptExecutions` is exported so it can be tested directly. Four tests: command completed (recovers as completed, message no longer claims silence), command failed (records failed, not timeout), command non-terminal (genuine silence still reported as timeout), and no command row (same). The last two matter most — a guard that swallowed real agent silence would be worse than the bug. Confirmed the two recovery cases FAIL against the previous behaviour. Local gate, green on node 22.23.2: `tsc --noEmit` clean; `vitest run` in apps/api 1271 files / 20146 tests passed, 0 failed; eslint clean on both changed files.
ToddHebebrand
pushed a commit
that referenced
this pull request
Aug 7, 2026
…3190) (#3213) Fixes #3190. ## The defect `reapStaleScriptExecutions` used a flat deadline for every execution: ```ts 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
pushed a commit
that referenced
this pull request
Aug 8, 2026
…ort (#3097) (#3237) Step 2 of 2 on #3097, as sequenced in that thread. #3172 (the reaper guard) landed in v0.104.0; this is the extraction. ## The bug The per-command-type result handlers lived inside `agentWs.ts` and were only ever reachable over the websocket, so a result submitted to `POST /agents/:id/commands/:commandId/result` reached none of them. For `script` that means `script_executions` is never updated on the HTTP path. Measured on a live instance: **394 of 1070 executions (37%)** carried a `timeout` status that did not match what happened, **89** of them having completed successfully with the output sitting in `device_commands.result` the whole time. #3172 stopped the reaper mislabelling those; this stops them being stranded. ## Handlers byte-identical The handler block and its `commandResultHandlers` registry move verbatim from `routes/agentWs.ts` to `services/commandResultHandlers.ts`. Everything they touch is an importable service or job, so there is no edge back into `agentWs`. The moved block is **385 lines before and after**, and the diff between the two is **eleven lines**: - four handler signatures take `commandId` as a parameter - the four `result.commandId` reads that fed them become that parameter - two dynamic `./agents/helpers` imports become `../routes/agents/helpers` - the registry gains `export` Nothing else in the block changed. The two dynamic imports stay dynamic. Reproduce the proof with: ``` git show origin/main:apps/api/src/routes/agentWs.ts # slice handleDiscoveryResult..registry diff -u <that slice> <the same slice of services/commandResultHandlers.ts> ``` Rebased onto a3dc568 so this carries your #3223 SNMP backoff change rather than reverting it — that landed inside the moved block, and the diff above is exactly what caught it. ## `commandId` as a parameter Per your ruling on the issue: option 1. The two transports disagree structurally, not just cosmetically — the websocket envelope carries `commandId` inline while REST takes it from the path and authorizes against that path value. A body-supplied id read inside a handler would be the confused-deputy shape you described, so the id is supplied by whichever transport authorized it and is never read off the payload. Your grep warning was right: 4 of ~45 `result.commandId` reads were inside the moved block and became the parameter; the other 41 stay in `agentWs` reading the parsed envelope. ## Schema unification (the one intentional behaviour change) The two `commandResultSchema` definitions diverged on the 1 MB cap: `routes/agents/schemas.ts` measured `Buffer.byteLength`, `agentWs.ts` used `.length` (UTF-16 code units), so the websocket accepted roughly 3x the intended budget for CJK-heavy output while REST rejected at 1 MB. The byte-accurate one is now canonical; `agentWs` extends it with the two envelope fields it adds (`type`, `commandId`). As you noted, the cap is a `.refine()` on the result **field**, not the object — so the shared base stays a plain `ZodObject` that REST consumes directly and the websocket can still `.extend()`. ## Which types the HTTP path now dispatches Five: `network_discovery`, `hyperv_backup`, `mssql_backup`, `snmp_poll`, `script` — the ones this route never post-processed at all. **Deliberately not the whole registry.** Thirteen of its eighteen keys already have an equivalent inline block in the route (backup verification, restore, vault sync, sensitive-data, CIS); dispatching those as well would run each of them twice. Converging the overlapping thirteen is left as follow-up rather than folded in here, because they are *nearly* but not exactly equivalent: the CIS and sensitive-data handlers forward the **derived** stdout while the route's inline blocks forward `normalizedData` verbatim. Replacing them would change what those two post-processors receive for a result sent as `result: {...}` with no `stdout` — a real behaviour change, and beyond the one this PR claims. Happy to do that convergence as its own PR if you want it. One related asymmetry I did **not** touch: on a validation failure REST returns early, while the websocket still dispatches the handler for `TERMINAL_TRANSITION_FAMILIES_ON_VALIDATION_FAILURE` so backup/restore records transition to failed. Pre-existing, separate question. ## No DB-context wrap on the HTTP side `agentAuthMiddleware` already holds an org-scoped `withDbAccessContext` open across the request, with the same shape `runWithAgentOrgDbAccess` builds (scope `organization`, the device's orgId, `accessibleOrgIds: [orgId]`, no partner access). The websocket needs its own wrap only because that path deliberately runs contextless (#3021). This route is not one of the `SELF_MANAGED_DB_CONTEXT_ACTIONS` — those match on the final path segment, which here is `result`, not `commands` — so the request-long wrap is active. A nested wrap would be a no-op regardless (`withDbAccessContext` returns `fn()` unchanged when a context is already on the ALS), and opening a second real transaction is the #1105 double-hold this route was cleaned up to avoid. The registry is imported dynamically here, like the DR handler below it: it pulls in the discovery and SNMP workers and through them the Drizzle schema module, which is more than this hot route should carry statically — and enough to break suites that partially mock `db/schema`. ## Tests - a script result over HTTP reaches the shared handler with the path-derived `commandId`, the resolved device and the derived stdout - a type the route handles inline is **not** re-dispatched through the registry Both were run against the un-fixed code first: emptying the dispatch set fails the first and only the first; adding `cis_benchmark` to it fails the second. The #1105 enqueue contract scan now covers `services/commandResultHandlers.ts` as well as `agentWs.ts`. The pipeline's seven enqueue sites are now five plus two, and the contract is about the pipeline rather than the file — leaving the scan pinned to `agentWs.ts` would have silently dropped the two that moved. Confirmed by unwrapping one of the moved enqueues and watching the scan name it by file and line. ## Local gate (on a3dc568) - `tsc --noEmit` across `apps/api` — exit 0, no output - `vitest run` across `apps/api` — **1284 files, 20500 tests, 0 failures**, 5 files / 48 tests skipped - `eslint` on all five changed files — exit 0, clean Closes #3097.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Step 1 of the sequencing you set on #3097 — the reaper guard, on its own, ahead of the extraction. Leaves #3097 open for the extraction PR.
What it fixes
Results submitted over the HTTP path never reach
script_executions, so the row stays pending, ages past the cutoff, and lands inreapStaleScriptExecutions— which stamped ittimeoutwith "Server-side timeout: no response from agent".That's false whenever a terminal
device_commandsrow exists. The agent did respond; the result was simply never mirrored. 89 executions on one live instance readtimeoutwhile their command had completed successfully with output.The reaper now reads the related command before deciding, and when that row is terminal it records the outcome the command actually reached — mapped the same way
handleScriptResultmaps it, so a reaped row agrees with what the WS path would have written. Non-terminal command, or no command row at all: original wording stands, because there it's true.It does not persist stdout/stderr. That's the extraction's job. This only stops the reaper asserting something it cannot know.
The command lookup already existed a few lines below, used only for
batchId. It moved above the update and serves both purposes now, so no extra round-trip.Two things to look at in review
The batch counter carried the same false assumption — it incremented
devicesFailedunconditionally, so a recovered success was still counted as a batch failure. It now incrementsdevicesCompletedwhen the command completed. That's slightly beyond a literal "add a guard", and I'd rather you saw it flagged than buried: leaving it would have kept the same wrong claim one level up.reapStaleScriptExecutionsis now exported so it can be tested directly.Tests
Four cases, and the two I care most about are the negative ones — a guard that swallowed real agent silence would be worse than the bug it fixes:
completed, message no longer claims silencefailed, nottimeoutsent) → stilltimeout, still "no response from agent"Confirmed the two recovery cases fail against the previous behaviour before the guard went in.
Local gate
Node 22.23.2, all green:
tsc --noEmitclean;vitest runinapps/api1271 files / 20146 tests passed, 0 failed; eslint clean on both changed files.Extraction next, once this lands —
services/home for the handler block and registry, canonical byte-lengthcommandResultSchemaexported fromschemas.tsand imported by both transports, the two dynamic imports left as-is, and a body that can honestly say the handlers are byte-identical with the schema unification as the one intentional behaviour change.