fix(cli): report a killed edge runtime container instead of a lost log stream - #6615
Prashansa-K wants to merge 2 commits into
Conversation
Adds a resource_limit category under user_actionable and its out_of_memory / container_killed fingerprint suffixes, so an edge runtime container killed for exceeding its memory limit can be classified separately from an internal bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Exit 137 re-attached to the log stream until the re-attach cap gave up, reporting an out-of-memory kill as a lost log stream against a container the message claimed was still running. `State.OOMKilled` separates a memory-limit kill, which carries a memory-allocation remediation, from a kill the CLI cannot attribute to either side. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Prashansa-K
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Review
Both independent reviews completed. Six distinct findings were adjudicated: five confirmed and one refuted. No critical or major defects were found.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/src/shared/functions/serve.ts:1424 |
behavior-change |
claude | Removing the non-OOM exit-137 retry introduces a race where supabase stop can force-kill the container and functions serve fails before the subsequent prune removes it. |
| 🟡 MINOR | apps/cli/src/command-internal/docker-suggest.ts:13 |
correctness |
codex | The remediation overstates OOMKilled as proof that the container exceeded its own configured memory limit. |
| 🟡 MINOR | apps/cli/src/shared/functions/serve.ts:1464 |
error-handling |
claude | When docker logs -f exits non-zero and inspection finds a stopped OOM-killed container, the code reports a generic log-stream error instead of the container crash and memory remediation. |
| ⚪ NIT | apps/cli/src/commands/functions/serve/serve.integration.test.ts:2366 |
test-coverage |
claude | The five-second timeout does not provide the stated retry-regression detection and unnecessarily includes potentially slow command bring-up. |
| ⚪ NIT | apps/cli/src/commands/functions/serve/SIDE_EFFECTS.md:75 |
documentation |
claude | The exit-code documentation incorrectly restricts the memory-limit classification to exit 137 even though the implementation classifies any OOMKilled: true non-zero exit as a resource limit. |
Findings outside the diff
- 🟡 MINOR
apps/cli/src/shared/functions/serve.ts:1464— Whendocker logs -fexits non-zero and inspection finds a stopped OOM-killed container, the code reports a generic log-stream error instead of the container crash and memory remediation.
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/shared/telemetry/error-actionability.ts:365(telemetry): Usingsuggestion_type: update_configfor runtime memory allocation misleadingly categorizes the remediation as a CLI configuration edit.
Refuted: The closed vocabulary definesUpdateConfiggenerically, not specifically asconfig.tomlor CLI-owned configuration. Raising the container runtime's memory allocation is a configuration change, and the separateresource_limiterror category preserves the cause.
Stats
Claude findings: 5 · Codex findings: 1 · Confirmed: 5 · Refuted: 1 · Uncertain: 0
Models: claude-opus-5 + gpt-5.6-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
| if (state.exitCode === 0) { | ||
| return { _tag: "containerExited" } satisfies ContainerLogsEndReason; | ||
| } | ||
| if (state.exitCode === 137) { | ||
| yield* reattach; | ||
| continue; | ||
| } | ||
| return yield* Effect.fail( | ||
| new EdgeRuntimeContainerCrashedError({ | ||
| message: `error running container ${containerId}: exit ${state.exitCode}`, | ||
| containerId, | ||
| exitCode: state.exitCode, | ||
| oomKilled: state.oomKilled, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🟡 MINOR · behavior-change · source: claude
Removing the non-OOM exit-137 retry introduces a race where supabase stop can force-kill the container and functions serve fails before the subsequent prune removes it.
Evidence: apps/cli/src/shared/functions/serve.ts:1418-1434 now immediately fails for stopped exit 137, while apps/cli/src/command-internal/docker-remove-all.ts:130-163 waits for docker stop and only then runs container prune.
Suggested fix: After a non-OOM exit 137, briefly re-inspect for removal or otherwise distinguish forced supervisor shutdown before failing.
| // A container killed for exceeding its memory limit never comes back, so a | ||
| // regression to re-attaching burns the whole cap before failing; bound the | ||
| // wait so that shows up as a timeout rather than a slow pass. | ||
| const error = yield* functionsServe(baseFlags()).pipe( | ||
| Effect.provide(layer), | ||
| Effect.timeout(Duration.seconds(5)), | ||
| Effect.flip, | ||
| ); |
There was a problem hiding this comment.
⚪ NIT · test-coverage · source: claude
The five-second timeout does not provide the stated retry-regression detection and unnecessarily includes potentially slow command bring-up.
Evidence: apps/cli/src/shared/functions/serve.ts:131 and 145-147 define five 400ms retry sleeps before EdgeRuntimeLogStreamLostError, while apps/cli/src/commands/functions/serve/serve.integration.test.ts:2375 and 2385 already assert the error type and single inspection.
Suggested fix: Remove the timeout, or correct its rationale and give cold command bring-up sufficient headroom.
| | `1` | the edge-runtime container is killed for exceeding its memory limit — exit `137` with `State.OOMKilled` | | ||
| | `1` | the edge-runtime container is killed from outside the CLI — exit `137` without `State.OOMKilled` | |
There was a problem hiding this comment.
⚪ NIT · documentation · source: claude
The exit-code documentation incorrectly restricts the memory-limit classification to exit 137 even though the implementation classifies any OOMKilled: true non-zero exit as a resource limit.
Evidence: apps/cli/src/shared/functions/serve.errors.ts:29-35 checks oomKilled before exit code, and apps/cli/src/shared/functions/serve.errors.unit.test.ts:100-111 explicitly verifies OOMKilled: true with exit code 1; SIDE_EFFECTS.md:75-76 describes only exit 137.
Suggested fix: Describe State.OOMKilled as authoritative and exit 137 as the typical exit code.
|
|
||
| /** Remediation hint shown when a container is killed for exceeding its memory limit. */ | ||
| export const SUGGEST_CONTAINER_MEMORY_LIMIT = | ||
| "The container was killed for exceeding its memory limit. Raise the memory allocated to your container runtime and try again."; |
There was a problem hiding this comment.
🟡 MINOR · correctness · source: codex
The remediation overstates OOMKilled as proof that the container exceeded its own configured memory limit.
Evidence: apps/cli/src/command-internal/docker-suggest.ts:13 attributes the kill to "its memory limit," and apps/cli/src/command-internal/docker-lifecycle.ts:269-271 derives that conclusion from only the boolean State.OOMKilled, without inspecting any configured limit or broader runtime memory pressure.
Suggested fix: Describe the event as an OOM-killer termination and advise checking both container limits and available host/runtime memory.
1 participant
TL;DR
An edge runtime container that gets killed during
supabase functions serve— most often by the out-of-memory killer — is retried until the re-attach cap gives up, and then reported as a lost log stream against a container the message claims is still running. This reports the kill instead, and separates a memory-limit kill from one the CLI cannot attribute.Refs CLI-2427 —
functions servemisreports a killed edge runtime container as a lost log stream (exit 137). This is the137gap #6594 left open.What was wrong
The retry can never succeed.
streamContainerLogstreats container exit137as retriable, but the container is created with no--restartpolicy, so once it is killed it stays dead.docker logs -fis respawned, exits, the container is inspected,137comes back.The resulting message says the opposite of what happened. After the cap the command fails with
lost the Edge Runtime log stream 5 times; container … is still running. The container is not running — that is the one fact the precedingdocker container inspectestablished.The classification sends the user the wrong way.
EdgeRuntimeLogStreamLostErrordeclaresexternal_service/networkwith a "rerun with--debug" remediation. An out-of-memory kill is not a network problem, and that remediation leads nowhere.The retry also contradicts both of its neighbours:
apps/cli-go/internal/utils/docker.gomaps137toErrContainerKilledwith no retry, anddb difftreats137as a genuine failure.What changed
137falls through toEdgeRuntimeContainerCrashedError, which now carriesState.OOMKilledfrom thedocker container inspectthe code already makes. That call uses--format "{{json .State}}", so this is one more field read inparseContainerState— no format string changes.error_kind/error_category137,OOMKilled: trueuser_actionable/resource_limit137,OOMKilled: falseunknown/unknown129/130/131/143internal_bug/runtime_crashresource_limitis a newuser_actionablecategory. Both137rows get their own fingerprint suffix, so they stay separable from each other and from the untagged bulk of their categories.Two calls worth reviewing
An out-of-memory kill is not
internal_bug/runtime_crash. It is Docker's memory allocation or the user's own function, and the user can act on it. Filing it against the internal-bug counter would be wrong in the direction this work exists to correct.A non-OOM
137isunknown, notuser_cancelled. Docker setsOOMKilledreliably only for container-limit OOMs; a host or VM out-of-memory kill on macOS lands here withOOMKilled: false. Calling that "the user cancelled" asserts something we do not know.unknownwith a named fingerprint keeps it separable so it can be reclassified once there is data on which cause dominates — at the cost of knowingly adding to theunknownbucket.Behaviour change to be aware of
A session whose container is killed now fails immediately rather than after five re-attaches, and fails with a different tagged error than before. A
functions serve &CI step that was passing through the re-attach window will now fail at the kill.🤖 Generated with Claude Code