Repository navigation
fix(daemon): bound pending sessions and prune on completion - #1091
PierrunoYT wants to merge 1 commit into
Conversation
Reject excess unfinished sessions before registration and dispatch, independently of retained history. Release admission and prune on every terminal path. Amp-Thread-ID: https://ampcode.com/threads/T-01a0dcbe-87ec-74e6-bdd0-6341b0c26d9a Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: Gitlawb/zero/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. WalkthroughThe session manager now limits unfinished sessions separately from retained history. It defaults ChangesSession admission and retention
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The pending-session limit and completion cleanup show no identified issue requiring a fix before merge. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new limit bounds unfinished sessions and releases capacity when they finish. Remote authentication remains in place, and no new security finding was established. The limit is shared by callers, while the exposure and identity policy of a deployed remote listener remain uncertain. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Reviewed at 53ba8759. Admission is checked and counted under the manager lock before anything is registered or started, and released on every completion path. Moving finish under the manager lock is safe. finish only flips state under the session lock and closes channels, so it can't block, and every path takes the manager lock before the session lock, so there's no inversion.
Each piece is load-bearing:
- Dropping the admission check fails
TestSessionManagerAdmissionBoundandTestSessionManagerCancellationReleasesAdmission. - Dropping the release on completion fails both of those too.
- Dropping the prune on completion fails
TestSessionManagerPrunesOnCompletion. - Registering an already-canceled start fails the cancellation test.
internal/daemon passes natively on Windows, and under -race three times over for the session tests. CI is 9 of 9 at head. Approving.
euxaristia
left a comment
euxaristia
left a comment
There was a problem hiding this comment.
The admission bound is correct (counter under lock, refuses with a typed error, decrement plus prune after the run). One gap worth a follow-up: the decrement is not panic-safe (no defer around pool.Run), so a panicking worker would leak a pending slot until the 256 cap bricks admission.
This branch has not been deployed
4 participants
What and why
Fixes #1084 (issue-approved).
Add a separate admission limit before registering a session or spawning its goroutine.
MaxPendingbounds queued plus running sessions, defaults to 256, and is independent of completed-history retention (MaxSessions). Excess starts returnErrSessionOverloaded, which the existing control protocol sends through its error response. Already-canceled starts are rejected without registration.Completion holds the manager lock while finishing the session, releasing admission, and pruning history. This covers success, failure, and queued cancellation; active sessions are never evicted. No new CLI flag or protocol message is introduced.
Regression evidence
Before the implementation, the new blocked-worker tests failed on upstream main:
accepted 300 unfinished sessions, want default bound 256retained 3 completed sessions without another Start, want 2Coverage includes concurrent starts at the default bound, a configured bound independent of retention, overload without registration, duplicate IDs at capacity, queued cancellation, canceled input, capacity reuse, permanent worker failure, and completion-time pruning without another submission.
Validation
Linux amd64, Go 1.26.6:
make fmt-check— passedgo vet ./...— passedgo test ./...— passedgo test -race -count=20 ./internal/daemon/...— passedgo run ./cmd/zero-release build— passedgo run ./cmd/zero-release smoke— passedmake vulncheck— No vulnerabilities foundgit diff HEAD --check— passed before commitmake lint-static— advisory failure: four pre-existing staticcheck style findings in unchanged files:internal/installtest/workflow_permissions_test.go:20(QF1001),internal/proxydial/proxydial.go:67,72andinternal/tools/web_fetch.go:315(QF1008). No findings in changed files.Branch created from current upstream main; native macOS and Windows execution was not performed locally.
Summary by CodeRabbit