feat(notifications): notify when a container exits on its own - #384
feat(notifications): notify when a container exits on its own#384AprilNEA wants to merge 1 commit into
Conversation
Greptile SummaryThe PR adds default-enabled notifications for unexpected container exits, using Docker event attributes and timestamps to distinguish intentional shutdowns, suppress crash-loop noise, and batch simultaneous failures.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| Packages/DockerClient/Sources/DockerClient/DockerClient/DockerClient+Events.swift | Extends parsed Docker events with actor attributes and daemon timestamps needed by container-exit classification. |
| ArcBox/Services/Notifications/ContainerNotificationRules.swift | Implements intentional-shutdown suppression, exact action matching, per-container cooldowns, and crash extraction; the previously discussed residual suppression window is explicitly accepted. |
| ArcBox/Services/Notifications/ContainerCrashReporter.swift | Collects qualifying crashes into a fixed three-second batch and safely clears pending work when the runtime stops. |
| ArcBox/Services/Notifications/ContainerCrashDigest.swift | Deduplicates crashes by container and renders single- or multi-container notifications with appropriate destinations. |
| ArcBox/Services/DockerEventMonitor.swift | Forwards raw Docker events to notification handling before the existing refresh whitelist and debounce logic. |
| ArcBox/App/ApplicationCoordinator.swift | Connects Docker event delivery to the notification coordinator and clears pending crash reports when the runtime stops. |
| ArcBox/App/AppPreferences.swift | Registers defaults for all notification categories so the new container category is enabled unless explicitly disabled. |
| ArcBox/Views/Settings/GeneralSettingsView.swift | Adds a General settings toggle for container crash notifications. |
| ArcBoxTests/ContainerNotificationRulesTests.swift | Covers crash classification, intentional shutdowns, signal handling, cooldown behavior, and health-check noise suppression. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Docker["Docker /events stream"] --> Parser["DockerClient event parser"]
Parser --> Monitor["DockerEventMonitor"]
Monitor --> Rules["ContainerNotificationRules"]
Rules -->|"intentional, clean, or cooling down"| Ignore["Ignore"]
Rules -->|"unexpected non-zero exit"| Reporter["ContainerCrashReporter"]
Reporter -->|"3-second batch"| Digest["ContainerCrashDigest"]
Digest --> Service["UserNotificationService"]
Reviews (3): Last reviewed commit: "feat(notifications): notify when a conta..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cff057870
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case "kill", "stop": | ||
| intentionalExits.insert(id) |
There was a problem hiding this comment.
When a user sends a non-terminating signal such as docker kill --signal=SIGHUP and the container handles it without exiting, this inserts the container ID into intentionalExits indefinitely; the next unrelated crash is then silently consumed as intentional. The repository's Docker API spec explicitly allows ContainerKill to send an arbitrary POSIX signal (Packages/DockerClient/Sources/DockerClient/openapi.yaml:1792-1798,1846-1849), so only signals that actually imply termination should establish this intent, or the intent needs to expire when no death follows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in the latest push.
Intent now expires after ten seconds (ContainerNotificationRules.intentWindow) instead of being a set membership that lives forever. Signal classification would not have been enough: a container that traps SIGTERM and keeps running has exactly the same problem as one sent SIGHUP, and neither is visible from the signal number.
Ten seconds is enough because a fatal signal kills within milliseconds, and a long docker stop -t 60 stays covered regardless — Docker emits stop immediately before the die, so the intent is refreshed no matter how long the grace period ran. Both cases have tests (testNonTerminatingSignalDoesNotSuppressALaterCrash, testLongGracePeriodStopIsStillSuppressed).
Residual gap, deliberately accepted: a genuine crash within ten seconds of a non-terminating signal is still suppressed. That errs toward silence, which is the safe direction here.
229d0c9 to
73faa6f
Compare
3cff057 to
3a9b744
Compare
There was a problem hiding this comment.
Pull request overview
Adds end-user notifications for containers that exit unexpectedly by enriching Docker event data (attributes + timestamp) and introducing container-crash-specific rule/digest/reporting components, wired through the app’s notification pipeline and settings.
Changes:
- Extend
DockerClient.DockerEventto carryActor.Attributesand the daemon-recorded event time. - Add container-crash notification logic (rules + batching + digest) with targeted noise suppression and rate limiting.
- Wire Docker events into
NotificationCoordinator, add a new notification category, and expose a user toggle with defaults registered for all categories.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Packages/DockerClient/Sources/DockerClient/DockerClient/DockerClient+Events.swift | Enrich Docker events with attributes and timestamps when streaming /events. |
| ArcBoxTests/DockerEventMonitorTests.swift | Update helper to construct DockerEvent with the new required date. |
| ArcBoxTests/ContainerNotificationRulesTests.swift | Add coverage for intent suppression, cooldown, and noise cases (exec_die, restart loops). |
| ArcBoxTests/ContainerCrashDigestTests.swift | Add coverage for digest rendering, dedupe, identifiers, and category. |
| ArcBox/Views/Settings/GeneralSettingsView.swift | Add UI toggle for container-crash notifications in General settings. |
| ArcBox/Services/Notifications/NotificationCoordinator.swift | Route Docker events into the new container crash reporter; stop on runtime shutdown. |
| ArcBox/Services/Notifications/ContainerNotificationRules.swift | Implement stateful rules to decide which container deaths are notify-worthy. |
| ArcBox/Services/Notifications/ContainerCrashReporter.swift | Batch crash events into a single notification per window. |
| ArcBox/Services/Notifications/ContainerCrashDigest.swift | Render a batch of crashes into a single user notification. |
| ArcBox/Services/Notifications/AppNotification.swift | Add .container category and preference-key mapping. |
| ArcBox/Services/DockerEventMonitor.swift | Add onEvent callback to deliver raw Docker events to notification rules before whitelisting. |
| ArcBox/App/AppPreferences.swift | Register notification defaults via Category.allCases to avoid silent “off by default” shipping. |
| ArcBox/App/ApplicationCoordinator.swift | Wire Docker event monitor to notification coordinator and stop container reporting on runtime stop. |
| ArcBox.xcodeproj/project.pbxproj | Add new source/test files to the Xcode project. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // `timeNano` is more precise but overflows a Double | ||
| // past millisecond range; `time` is whole seconds, | ||
| // which is all any consumer needs. | ||
| let date = (json["time"] as? TimeInterval).map(Date.init(timeIntervalSince1970:)) |
There was a problem hiding this comment.
Fixed. The comment now says what actually happens: timeNano at present-day values is past the range where a Double holds every integer, so reading it back loses the sub-second precision it existed for. Not overflow.
| } | ||
|
|
||
| let names = unique.prefix(namesShown).map(\.name).joined(separator: ", ") | ||
| let remainder = unique.count - namesShown |
There was a problem hiding this comment.
Fixed. remainder is now measured against the prefix that was actually taken (unique.count - shown.count), so it cannot go negative however the count compares to namesShown. testShortBatchListsEveryName covers the two-crash case.
The follow-up ABXD-132 left open, now that DockerEvent can carry the attributes it needs. Exit codes cannot tell a crash from a shutdown: `docker stop` produces the same `die`/`exitCode=137` a killed container does. What separates them is that Docker announces intent first — captured against a live 29.6 daemon, `docker stop` emits kill(SIGTERM), kill(SIGKILL), stop, then die — so a `kill` or `stop` seen for a container suppresses one following death, and a `start` clears it. Two things keep that intent honest. `docker kill` delivers whatever signal it is asked to, so only the four that ask a container to end — SIGINT, SIGQUIT, SIGKILL, SIGTERM — establish it; SIGHUP to reload configuration is addressed to a container expected to keep running and explains no death that follows. And it expires after ten seconds, because a container that traps SIGTERM and carries on would otherwise leave intent sitting there to swallow the next genuine crash. Neither an image's own STOPSIGNAL (httpd's is SIGWINCH) nor a long `docker stop -t N` needs a special case: `stop` arrives immediately before the `die` whichever signal was sent, and refreshes the intent. Two noise sources found the same way. A failing health check emits `exec_die` with a non-zero exit code every few seconds on a `type=container` event, so the action is matched exactly rather than by suffix. And a container under `restart: always` with a broken image dies forever, so the same container stays quiet for five minutes after being announced. Deaths inside a three-second window are announced together: a failing `compose up` takes several containers down at once. The window opens on the first crash rather than sliding, so a crash loop cannot defer the report indefinitely. DockerEvent now carries Actor.Attributes and the event time; both were parsed and dropped before.
3a9b744 to
9df27ba
Compare
The first follow-up ABXD-132 left open. #366 has landed, so this is now rebased straight onto
masterand stands alone.DockerClient.DockerEventkept onlytype/action/actorIDand threw awayActor.Attributes, which is where the exit code lives. It now carries the attributes and the event time.Telling a crash from a shutdown
Exit codes cannot do it. Captured against a live Docker 29.6 daemon:
docker stopproduces the samedie/exitCode=137a killed container does. What separates them is that Docker announces the intent first — note the ordering,stopprecedesdie, not the other way round. So akillorstopseen for a container suppresses exactly one following death, and astartclears any intent that no death consumed.Two things keep that intent honest, both from review:
docker killdelivers whatever signal it is given. Only the four that ask a container to end — SIGINT, SIGQUIT, SIGKILL, SIGTERM — establish intent. SIGHUP to reload configuration is addressed to a container expected to keep running, so a death that follows it is the container's own doing and gets announced. Numbers rather than names because that is what the daemon writes (strconv.Itoa(int(stopSignal)), mobydaemon/kill.go). An image's ownSTOPSIGNALis deliberately not in the set — httpd's is SIGWINCH — and needs no special case, becausedocker stopemitsstopbefore thediewhichever signal it sent.Intent expires after ten seconds. A container that traps SIGTERM and carries on is indistinguishable, by signal number, from one that dies of it; without an expiry its intent would sit there and swallow the next genuine crash. A long
docker stop -t 60stays covered regardless, sincestoparrives immediately before thedieand refreshes it.Noise, found the same way
Failing health checks. A container whose probe fails emits
exec_diewith a non-zero exit code every few seconds, on atype=containerevent — three of them were streaming on the test machine throughout. Matching the action by prefix or substring would turn a single unhealthy container into a permanent banner source. The action is matched exactly, and there is a test that says so.Crash loops. A container under
restart: alwayswith a broken image dies forever. The same container stays quiet for five minutes after being announced;destroyclears that, since a recreated container is a different container.Simultaneous deaths. A failing
compose uptakes several containers down at once, so deaths inside a three-second window are announced together — "3 containers exited: api, worker, db". The window opens on the first crash rather than sliding, because a sliding window lets a crash loop defer the report indefinitely.Shape
ContainerNotificationRules— pure and stateful (intent, cooldown); decides whether a death is news.ContainerCrashDigest— pure rendering of a batch into one notification, deduplicated by container.ContainerCrashReporter— owns the batch window and nothing else, so the coordinator stays thin..containercategory, registered default-on and toggleable in General settings.AppPreferencesnow registers fromCategory.allCases, so a future category cannot silently ship switched off.Verification
make lint(0 violations) andmake testpass on the rebase: 470 passed, 0 failed. 26 cases acrossContainerNotificationRulesTestsandContainerCrashDigestTests, confirmed by name in the result bundle.make generate-xcodeprojreproduces the committedproject.pbxprojbyte for byte. The event shapes in the tests are transcribed from the live capture above, not assumed.Test containers created during the capture (
arcbox-evt-crash,arcbox-evt-stop) were removed.Still open from ABXD-132
Long image operations, memory pressure, and disk usage. Deliberately not bundled here: each adds its own noise surface and deserves its own judgement, and disk needs a filesystem-usage field in
stats.protofirst, which is a daemon-side change.