Skip to content

feat(service): support deferred configuration updates - #130

Open
rustatian wants to merge 4 commits into
masterfrom
feature/2119
Open

rustatian wants to merge 4 commits into
masterfrom
feature/2119

Conversation

@rustatian

Copy link
Copy Markdown
Member

Add service.Update for partial configuration changes while current processes continue running.

  • Apply new settings on subsequent executions, with process-count reconciliation and environment replacement.
  • Use api-go/v6 v6.0.0-beta.15 and align startup and shutdown with Endure.
  • Cover each parameter, concurrent updates, and process cleanup.

closes: roadrunner-server/roadrunner#2119

API: roadrunner-server/api#80.
Docs: roadrunner-server/docs#80.

Copilot AI lite review requested due to automatic review settings September 16, 2026 08:33
@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.13953% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.67%. Comparing base (ba05b00) to head (95a4a27).

Files with missing lines Patch % Lines
group.go 99.03% 1 Missing ⚠️
plugin.go 96.15% 1 Missing ⚠️
process.go 95.83% 1 Missing ⚠️
rpc.go 97.82% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
+ Coverage   97.96%   98.67%   +0.70%     
==========================================
  Files           5        6       +1     
  Lines         295      377      +82     
==========================================
+ Hits          289      372      +83     
+ Misses          6        5       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Resolve the critical build, cleanup, and unbounded reconciliation issues before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds deferred service.Update support for partial configuration changes while active processes continue running.

Changes:

  • Adds runtime updates with environment replacement and process-count reconciliation.
  • Refactors process-group and lifecycle management.
  • Adds unit/integration coverage and dependency updates.
File summaries
File Reviewed changes Final findings
update_test.go Update tests
tests/update_test.go Integration and concurrency tests Critical (1 vote): new(tt.target) does not compile at lines 572 and 582.
tests/mock/observer.go Structured log capture Moderate (1 vote): Returned attribute maps alias stored records; deep-copy them on reads.
tests/helpers/rpc.go Update RPC helper
tests/go.sum Test dependency checksums
tests/go.mod Test dependency updates
rpc.go Update and lifecycle RPCs
rpc_test.go RPC lifecycle tests
process.go Process snapshots and shutdown handling Critical (1 vote): stop can dereference a nil command for an unstarted process.
process_test.go Process cleanup tests
plugin.go Plugin lifecycle integration
plugin_test.go Plugin lifecycle tests
group.go Deferred updates and process reconciliation Critical (1 vote): math.MaxInt can trigger unbounded worker/timer reconciliation and resource exhaustion.
go.work.sum Workspace checksums
go.sum Dependency checksums
go.mod API dependency updates
config.go Configuration cloning and validation
Review details

Suppressed comments (2)

tests/mock/observer.go:23

  • All only clones the slice, so each returned Attrs map still aliases the stored record; FilterMessage and FilterMessageSnippet return the same aliases. A caller that mutates an attribute can change historical records and race with Handle, contradicting the “Attribute maps are immutable” contract; deep-copy the maps on every read or remove that claim.
// All returns the captured records. Attribute maps are immutable.
func (o *ObservedLogs) All() []LoggedEntry {
	o.mu.RLock()
	defer o.mu.RUnlock()
	return slices.Clone(o.logs)

tests/update_test.go:582

  • new(tt.target) cannot compile because new accepts a type, not a value. This prevents the test package from building; take the address of the int64 field instead (for example, &tt.target).
				helpers.Update(t, client, &serviceV1.Update{Name: "update", ProcessNum: new(tt.target)})
  • Files reviewed: 15/17 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread group.go
Comment thread process.go
Comment thread tests/update_test.go
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.

[💡 FEATURE REQUEST]: Support updating service parameters using gRPC

2 participants