Skip to content

chore(container-runner): tighten comments - #5589

Merged
abcxff merged 5 commits into
mainfrom
stack/chore-container-runner-tighten-comments-zrxxyxls
Aug 31, 2026
Merged

chore(container-runner): tighten comments#5589
abcxff merged 5 commits into
mainfrom
stack/chore-container-runner-tighten-comments-zrxxyxls

Conversation

@abcxff

@abcxff abcxff commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Review: chore(container-runner): tighten comments

Confirmed this is a comments/docs-only change — every hunk touches //////!/// doc comments, README prose, or comments in scripts/Dockerfiles. No behavioral code changed.

Finding: stale doc left inconsistent with the fix in this same PR (README.md:11-12)

The PR tightens container-runner/src/main.rs's module doc from "the instance stays warm after its last actor stops and never self-exits; the engine reaps it by draining the /start connection..." to "once the last child stops the process exits so the platform reaps the instance" — which actually matches the real behavior (stop_child in actor.rs calls request_exit when the child registry is empty, which cancels EXIT and lets main return/exit the process).

However, container-runner/README.md:11-12 still has the old, now-contradicted claim:

"The instance stays warm after its last actor stops; the engine reaps it by draining the /start connection after the request lifespan."

Since this PR is specifically about tightening/correcting these comments and already fixed the same inaccuracy in main.rs, it would be good to fix the README in the same pass so the two do not disagree.

Other observations (non-blocking)

  • Em dash cleanup is consistent and complete — grepped all added lines and found no remaining em dashes, matching the CLAUDE.md rule against em dashes in prose/comments.
  • The monitor.rs module doc trims out the old "memory/CPU sources in preference order" lists, but that detail is not lost — it now lives on the MemSource/CpuSource enum variant doc comments, a better place for it.
  • The "Cloud Run" to "serverless"/"Rivet Compute" rewording is applied consistently across README, Dockerfile, docker-compose, and scripts. Good for not baking a specific vendor's product name into the code comments.
  • One pre-existing em dash at README.md:187 ("— so thousands can run cheaply") is not touched by this PR and is out of scope, but since the PR is already doing comment cleanup in this file, it could be swept up too.

No functional, security, or test-coverage concerns — there is no logic change here, so no new tests are needed.

@abcxff
abcxff force-pushed the stack/feat-container-runner-exit-process-when-the-last-child-stops-ssuvmoso branch from 332c1a8 to 87b61cb Compare August 24, 2026 14:46
@abcxff
abcxff force-pushed the stack/chore-container-runner-tighten-comments-zrxxyxls branch from 102498a to 2e9fde1 Compare August 24, 2026 14:46
@abcxff
abcxff force-pushed the stack/chore-container-runner-tighten-comments-zrxxyxls branch from 2e9fde1 to 5b11c04 Compare August 25, 2026 19:36
@abcxff
abcxff force-pushed the stack/feat-container-runner-exit-process-when-the-last-child-stops-ssuvmoso branch from 87b61cb to 7531a39 Compare August 25, 2026 19:36
@abcxff
abcxff force-pushed the stack/chore-container-runner-tighten-comments-zrxxyxls branch from 5b11c04 to 985b564 Compare August 28, 2026 19:21
@abcxff
abcxff force-pushed the stack/feat-container-runner-exit-process-when-the-last-child-stops-ssuvmoso branch from 7531a39 to 42e69ff Compare August 28, 2026 19:21
@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Review

This PR is purely a comment/prose tightening pass over container-runner — I confirmed every hunk touches only ////// doc comments, the README, and script/Dockerfile comments; no executable code paths changed. Given that scope, risk is low, but a few things worth calling out:

Good

  • Complies with the CLAUDE.md comment style rules: no em dashes remain in any added comment/prose (verified via grep across the whole diff), and the dash-parenthetical fragments from the originals (e.g. (if configured) - regardless of...-style phrasing) are gone in favor of complete sentences.
  • The main.rs module doc change (removing "the instance ... never self-exits; the engine reaps it by draining the /start connection") actually fixes a stale doc, not just trims it — the branch stacks on feat-container-runner-exit-process-when-the-last-child-stops, and the new text ("once the last child stops the process exits so the platform reaps the instance") now matches actor.rs::stop_child, which calls request_exit when the child registry is empty. The old wording would have been actively misleading post-rebase.
  • Similarly, dropping "failed start" from the EXIT static's doc comment ("actor stopped, failed start, or signal""last child stopped or a signal") is correct: on_start's error path explicitly does not tear down the instance ("A failed start is this actor's alone; it does not take down others").
  • The monitor.rs module doc drops the detailed "memory/CPU sources in preference order" lists, but that detail is preserved per-variant on MemSource/CpuSource (src/monitor.rs:47-82), and the new comment explicitly points there (see [\MemSource`]/[`CpuSource`]`) — no net information loss.
  • The Cloud Runserverless/serverless platform terminology swap (README, Dockerfile, e2e scripts, main.rs) is applied consistently everywhere it appears, not just in the files that happened to get comment edits.

Minor / worth a second look

  • A few trims do lose some genuinely load-bearing nuance rather than just wordiness. For example, in main.rs::async_main's exit-handling comment, the original explained why the signal path bounds the drain concurrently with notifying the engine ("Bounding the drain means an unreachable engine cannot eat the budget the children need") — that rationale is gone in the shortened version. Similarly drain_then_stop_child's doc drops the explicit tie-back to "the reclaim's SIGTERM→SIGKILL budget is honored." Neither is wrong, just thinner; worth confirming the team is fine losing that context in a low-traffic file like this one.
  • Since this is a comment-only PR, there isn't (and doesn't need to be) new test coverage — nothing to flag there.

No functional, security, or performance concerns since no code semantics changed.

@abcxff
abcxff force-pushed the stack/feat-container-runner-exit-process-when-the-last-child-stops-ssuvmoso branch from 42e69ff to 57f7115 Compare August 30, 2026 18:16
@abcxff
abcxff force-pushed the stack/chore-container-runner-tighten-comments-zrxxyxls branch from 985b564 to 1ef5e54 Compare August 30, 2026 18:16
@abcxff
abcxff force-pushed the stack/feat-container-runner-exit-process-when-the-last-child-stops-ssuvmoso branch from 57f7115 to 94d4dcc Compare August 31, 2026 14:13
@abcxff
abcxff force-pushed the stack/chore-container-runner-tighten-comments-zrxxyxls branch from 1ef5e54 to 0a1e431 Compare August 31, 2026 14:13
@abcxff
abcxff changed the base branch from stack/feat-container-runner-exit-process-when-the-last-child-stops-ssuvmoso to main August 31, 2026 14:13
@abcxff
abcxff merged commit 0a1e431 into main Aug 31, 2026
3 of 8 checks passed
@abcxff
abcxff deleted the stack/chore-container-runner-tighten-comments-zrxxyxls branch August 31, 2026 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant