services updates#1422
Conversation
📝 WalkthroughWalkthroughService-on-demand lifecycle handling now supports asynchronous restart, explicit ChangesService lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ServiceRestartHandler
participant C2DEngineDocker
participant C2DDatabase
participant Docker
Client->>ServiceRestartHandler: SERVICE_RESTART
ServiceRestartHandler->>C2DEngineDocker: resolve and restart service
C2DEngineDocker->>C2DDatabase: acquire lease and persist Restarting
C2DEngineDocker->>Docker: pull image and replace container
Docker-->>C2DEngineDocker: replacement container
C2DEngineDocker->>C2DDatabase: persist Running and release lease
Client->>ServiceRestartHandler: poll status
ServiceRestartHandler-->>Client: current service status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
This PR effectively addresses race conditions in service lifecycle management by introducing an in-memory per-service lifecycle lock (serviceOpsInFlight). The implementation accurately utilizes synchronous lock acquisition (safe in Node.js event loop) and robust try...finally blocks to guarantee lock release. The background loop now correctly respects these locks, preventing destructive operations like orphan-recovery from tearing down networks mid-creation. The inclusion of thorough unit and integration tests is highly commendable. LGTM!
Comments:
• [INFO][style] Excellent implementation of the per-service lifecycle lock. Using a synchronous check (has) and insertion (add) ensures there are no asynchronous yields where a race condition could slip through.
• [INFO][bug] Good use of the try...finally pattern. This guarantees that the lock is always released, even if the underlying Docker API throws an unexpected error, avoiding a permanently locked state for the service.
• [INFO][bug] Catching the error here and continuing the loop is a great defensive practice. It ensures that an error in stopping a single locked/expired service does not abort the entire expiry sweep for other services.
• [INFO][other] Great approach to testing the race condition. Intercepting pullImageRef to force a targeted delay (await sleep(5000)) reliably reproduces the state necessary to test this async bug without introducing excessive test flakiness.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/c2d/compute_engine_docker.ts (1)
3787-3816: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOrdering race in
doRestartService: old container is torn down before the DB status is flipped away fromRunning, letting a concurrent health-check flip it toError.
doRestartServicestops/removes the old container and network (lines 3804-3810) before persistingjob.status = Starting(lines 3812-3816).doStopServicedoes the opposite — it persistsStoppingfirst, then touches Docker. During that window inrestartService,checkRunningServices()/checkServiceContainerHealth()(run every InternalLoop tick, not gated byserviceOpsInFlight) can see the job asRunningin the DB while its container is already gone, conclude "container lost", and callmarkServiceFailed→ persistsError. With a 2s cron tick and up to a 10s container-stop timeout, this window is realistically reachable in production.This is a real race, though it self-heals:
doRestartServicekeeps mutating its own in-memoryjoband unconditionally overwrites the DB on each subsequent step, so the strayErrorwrite gets clobbered once the restart proceeds. The visible impact is a transient falseErrorstatus to a client pollingserviceStatusmid-restart — undermining the exact class of race this PR sets out to close.Mirror
doStopService's ordering: persist the status flip away fromRunningbefore touching Docker.🔧 Proposed fix: flip status before tearing down
- // 1. Tear down existing container + network (best-effort) - if (job.containerId) { - const c = this.docker.getContainer(job.containerId) - await c.stop({ t: 10 }).catch(() => {}) - await c.remove({ force: true }).catch(() => {}) - } - await this.removeServiceNetwork(serviceId, job.networkId).catch(() => {}) - - job.status = ServiceStatusNumber.Starting - job.statusText = ServiceStatusText[ServiceStatusNumber.Starting] - job.containerId = '' - job.networkId = '' - await this.db.updateServiceJob(job) + // Flip status away from Running (and persist) BEFORE touching docker, so a concurrent + // InternalLoop tick's checkRunningServices() can no longer see this service as "Running" + // while its container is mid-teardown. + const oldContainerId = job.containerId + const oldNetworkId = job.networkId + job.status = ServiceStatusNumber.Starting + job.statusText = ServiceStatusText[ServiceStatusNumber.Starting] + job.containerId = '' + job.networkId = '' + await this.db.updateServiceJob(job) + + // 1. Tear down the previous container + network (best-effort) + if (oldContainerId) { + const c = this.docker.getContainer(oldContainerId) + await c.stop({ t: 10 }).catch(() => {}) + await c.remove({ force: true }).catch(() => {}) + } + await this.removeServiceNetwork(serviceId, oldNetworkId).catch(() => {})As defense-in-depth,
checkRunningServices()could additionally skip serviceIds present inserviceOpsInFlight, matching the guard already applied to the pendingStarts and expiry loops.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/c2d/compute_engine_docker.ts` around lines 3787 - 3816, Update doRestartService to set the job status to Starting, clear its containerId and networkId, and persist the job with updateServiceJob before stopping or removing the existing container and network. Preserve the existing expiry validation and teardown behavior, and do not rely on the optional checkRunningServices defense-in-depth change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/c2d/compute_engine_docker.ts`:
- Around line 3697-3707: Update the lifecycle shutdown flow in stop() to track
and await in-flight direct stopService and restartService calls, in addition to
internalLoopPromise and serviceStartPromises. Use the existing service lifecycle
operation tracking around stopService and its corresponding restartService path,
and ensure stop() drains these promises before returning so no service teardown
or restart remains active.
---
Outside diff comments:
In `@src/components/c2d/compute_engine_docker.ts`:
- Around line 3787-3816: Update doRestartService to set the job status to
Starting, clear its containerId and networkId, and persist the job with
updateServiceJob before stopping or removing the existing container and network.
Preserve the existing expiry validation and teardown behavior, and do not rely
on the optional checkRunningServices defense-in-depth change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 97c7e03d-6f72-4265-98a6-a267b36e9d5c
📒 Files selected for processing (6)
docs/services.mdsrc/components/c2d/compute_engine_docker.tssrc/test/integration/services.test.tssrc/test/unit/compute.test.tssrc/test/unit/service/serviceNetworkCleanup.test.tssrc/test/unit/service/serviceRestartRace.test.ts
|
Commit 50af212 description: Service lifecycle hardening: cross-process locking, async restart, paid-window reservationsWhyThe 1. Cross-process lifecycle lease (
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
What was fixed (insufficient-funds follow-up) in b3f38e11. Free-restart vulnerability (the serious one). The restart guard in 2. Unpaid resource squatting. 3. Fail-fast funds check. New tests: never-paid restart rejection, unpaid- The end-to-end behavior for a user without funds is now: an immediate |
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/c2d/compute_engine_docker.ts`:
- Around line 3864-3878: Update tryAcquireServiceLifecycleLock to recheck
this.stopped immediately after the asynchronous db.acquireServiceLock resolves;
if shutdown occurred, release the acquired service lock and return false, while
preserving the existing in-flight cleanup behavior.
- Around line 3846-3848: Update the lock checks around isServiceLocked in the
affected lifecycle paths to fail closed: do not convert database errors to
false/unlocked. When lease state cannot be verified, skip or reject the
restart/stop operation, preserving normal behavior only when the database check
succeeds; apply this consistently to both referenced paths.
- Around line 1847-1885: Update the expiry sweep around stopService and the
ServiceExtendHandler flow to use the same lifecycle lock/CAS contract,
serializing expiry with SERVICE_EXTEND. Re-read the current expiresAt while
holding the lease immediately before teardown and skip expiry if the service was
extended. Ensure the extension’s final ServiceJob write cannot overwrite a newer
stop/restart/Expired state, and preserve the existing teardown and port-release
behavior only for a valid expiry transition.
- Around line 4037-4040: Update the missing-job branch in the restartService
flow to release the lifecycle lock and DB lease before returning null. Ensure
the cleanup used by the surrounding catch/finally path is invoked explicitly or
restructure control flow so the background operation’s finally executes, while
preserving the existing debug log and null result.
- Around line 1831-1838: Update the fire-and-forget promise created around
processServiceStart in the service-start loop to attach a rejection handler,
while preserving the existing finally cleanup that deletes startPromise and
releases the lifecycle lock. Ensure start failures are consumed or routed
through the established error-handling path so the tracked promise cannot
produce an unhandled rejection.
In `@src/components/database/sqliteCompute.ts`:
- Around line 187-215: Update acquireServiceLock and its callers to return and
propagate a fencing generation/token rather than only a boolean. Require the
current holder to validate that token immediately before destructive or
state-changing Docker/job operations, and abort when validation fails after a
stale takeover; ensure release remains holder-scoped and cannot bypass fencing.
- Around line 418-426: Update the expirableStatuses definition near the existing
Running, Error, and Stopped entries to also include
ServiceStatusNumber.Stopping, ensuring crashed jobs persisted in Stopping are
recovered by the same expiry sweep.
In `@src/test/integration/services.test.ts`:
- Around line 786-823: Update the mid-restart wait in the SERVICE_RESTART test
around restartPromise and the PullImage polling loop to assert that the job
reaches ServiceStatusNumber.PullImage before proceeding, rather than silently
timing out. Also synchronize with and assert that an InternalLoop tick occurs
while the job remains in PullImage, then issue the concurrent commands only
after both conditions are confirmed.
In `@src/test/unit/service/serviceRestartRace.test.ts`:
- Around line 364-384: Ensure host-port reservations are always released via
finally blocks. In src/test/unit/service/serviceRestartRace.test.ts lines
364-384, wrap all work after reserveHostPort(PORT) in try/finally and release
the port in finally; in src/test/unit/compute.test.ts lines 1637-1643, move
releaseHostPort(reservedPort) into finally while preserving the existing test
assertions and cleanup behavior.
- Around line 144-156: Update the restartService failure test to assert that
updateServiceJob receives the Error status and pull failure text, rather than
relying only on the mutable job object. In
src/test/unit/service/serviceRestartRace.test.ts lines 144-156, add that
persistence assertion; in src/test/unit/compute.test.ts lines 1747-1755,
1794-1807, 1820-1827, and 1840-1847, assert that each restart failure or
replacement/reused/cleared command and entrypoint value is passed through the
database persistence call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 24a0a66d-5f88-451e-b2a1-beaa67fe31b9
📒 Files selected for processing (16)
docs/services.mdsrc/@types/C2D/ServiceOnDemand.tssrc/components/c2d/compute_engine_docker.tssrc/components/core/service/extendService.tssrc/components/core/service/getStreamableLogs.tssrc/components/core/service/restartService.tssrc/components/core/service/stopService.tssrc/components/core/service/utils.tssrc/components/database/C2DDatabase.tssrc/components/database/sqliteCompute.tssrc/test/integration/services.test.tssrc/test/unit/compute.test.tssrc/test/unit/service/serviceHandlers.test.tssrc/test/unit/service/serviceJobsDatabase.test.tssrc/test/unit/service/serviceNetworkCleanup.test.tssrc/test/unit/service/serviceRestartRace.test.ts
| // Atomically takes the lock for serviceId: inserts a fresh row, or steals one whose | ||
| // acquiredAt is older than staleMs (crashed holder). The single upsert statement is | ||
| // the atomicity guarantee — two processes racing it can never both see success. | ||
| acquireServiceLock( | ||
| serviceId: string, | ||
| holder: string, | ||
| staleMs: number | ||
| ): Promise<boolean> { | ||
| const now = Date.now() | ||
| const upsertSQL = ` | ||
| INSERT INTO service_locks (serviceId, holder, acquiredAt) VALUES (?, ?, ?) | ||
| ON CONFLICT(serviceId) DO UPDATE | ||
| SET holder = excluded.holder, acquiredAt = excluded.acquiredAt | ||
| WHERE service_locks.acquiredAt <= ?; | ||
| ` | ||
| return new Promise<boolean>((resolve, reject) => { | ||
| this.db.run( | ||
| upsertSQL, | ||
| [serviceId, holder, now, now - staleMs], | ||
| function (this: RunResult, err: Error | null) { | ||
| if (err) { | ||
| DATABASE_LOGGER.error(`Could not acquire service lock: ${err.message}`) | ||
| reject(err) | ||
| } else { | ||
| resolve(this.changes === 1) | ||
| } | ||
| } | ||
| ) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Add fencing before allowing stale lease takeover.
After process B steals a stale row, process A can resume and continue modifying the same Docker resources and job because acquisition returns only a boolean. Holder-scoped release does not stop A’s ongoing operation. Use a fencing generation verified before destructive/state-changing steps, or avoid takeover until the previous process is proven dead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/database/sqliteCompute.ts` around lines 187 - 215, Update
acquireServiceLock and its callers to return and propagate a fencing
generation/token rather than only a boolean. Require the current holder to
validate that token immediately before destructive or state-changing Docker/job
operations, and abort when validation fails after a stale takeover; ensure
release remains holder-scoped and cannot bypass fencing.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/test/unit/service/serviceRestartRace.test.ts (1)
144-168: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssert DB lease release on restart failure.
This verifies only
serviceOpsInFlightcleanup; the DB lease assertion covers success elsewhere. Also assertreleaseServiceLock(SERVICE_ID, 'test-holder')after the failed background operation settles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/unit/service/serviceRestartRace.test.ts` around lines 144 - 168, Add an assertion to the restartService failure-path test after the background operation settles, verifying releaseServiceLock was called with SERVICE_ID and 'test-holder'. Keep the existing serviceOpsInFlight cleanup and persisted Error assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/c2d/compute_engine_docker.ts`:
- Around line 4058-4072: Move the CPU allocation release currently performed
before Docker cleanup into the successful teardown branch of stopService,
alongside clearing containerId and networkId. Keep the allocation retained when
cleanupError is present, so cores remain reserved while the old container may
still be running.
- Around line 3862-3878: Update the health-failure transition around
isServiceLocked and updateServiceJob so it is serialized with lifecycle
operations: acquire the service’s lifecycle lease before persisting Error, or
use an atomic conditional update that only changes the record if it remains in
the expected Running state. Preserve the fail-closed behavior when lease
acquisition or validation fails, and prevent a concurrent Restarting transition
from being overwritten.
In `@src/components/core/service/extendService.ts`:
- Around line 204-247: The service extension flow around claimLock and
updateServiceJob must persist an idempotent extension intent before claiming
payment, then recover or finalize that intent if persistence or later processing
fails. Ensure retries detect the existing intent and complete the expiresAt,
duration, and extendPayments update without charging again; only claim payment
after the intent is durable.
In `@src/components/core/service/utils.ts`:
- Around line 128-133: Update reserveHostPort to persist reservations in SQLite
rather than only adding to the process-local allocatedPorts set. Store a unique
reservation keyed by both port and service, and ensure concurrent processes
cannot select the same port while preserving idempotent behavior for repeated
reservations and restart flows.
In `@src/test/unit/service/serviceJobsDatabase.test.ts`:
- Around line 179-184: Update the refreshServiceLocks test so the lock becomes
older than the five-second staleness threshold before calling
refreshServiceLocks, using the test’s existing clock or time-control utilities.
Then keep the proc-B acquisition assertion to verify refreshServiceLocks
re-stamps proc-A’s row and prevents takeover.
---
Nitpick comments:
In `@src/test/unit/service/serviceRestartRace.test.ts`:
- Around line 144-168: Add an assertion to the restartService failure-path test
after the background operation settles, verifying releaseServiceLock was called
with SERVICE_ID and 'test-holder'. Keep the existing serviceOpsInFlight cleanup
and persisted Error assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bc0a991-ec99-4eb5-bc3d-a3ae49a11c8f
📒 Files selected for processing (18)
docs/services.mdsrc/@types/C2D/ServiceOnDemand.tssrc/components/c2d/compute_engine_base.tssrc/components/c2d/compute_engine_docker.tssrc/components/core/service/extendService.tssrc/components/core/service/getStreamableLogs.tssrc/components/core/service/restartService.tssrc/components/core/service/startService.tssrc/components/core/service/stopService.tssrc/components/core/service/utils.tssrc/components/database/C2DDatabase.tssrc/components/database/sqliteCompute.tssrc/test/integration/services.test.tssrc/test/unit/compute.test.tssrc/test/unit/service/serviceHandlers.test.tssrc/test/unit/service/serviceJobsDatabase.test.tssrc/test/unit/service/serviceNetworkCleanup.test.tssrc/test/unit/service/serviceRestartRace.test.ts
| // Marks an already-assigned port as reserved (idempotent). Used by restart, which | ||
| // re-binds the ports recorded on the job: after a stop (or an Error path) released | ||
| // them, they must go back into the set before the container binds them again. | ||
| export function reserveHostPort(port: number): void { | ||
| allocatedPorts.add(port) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Persist host-port reservations across processes.
The new lifecycle model supports multiple processes sharing one Docker daemon, but allocatedPorts remains process-local. Different services can concurrently select the same port; one paid service then fails to bind and cannot restart successfully on its recorded endpoint. Use a SQLite-backed unique reservation keyed by port and service.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/core/service/utils.ts` around lines 128 - 133, Update
reserveHostPort to persist reservations in SQLite rather than only adding to the
process-local allocatedPorts set. Store a unique reservation keyed by both port
and service, and ensure concurrent processes cannot select the same port while
preserving idempotent behavior for repeated reservations and restart flows.
Fixed (5 of 6) in beb981d
Skipped (1)
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/components/c2d/compute_engine_docker.ts (1)
1870-1904: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the
Expiredtransition under the lifecycle lease.
stopService()releases the lease before this fresh read and write. A concurrent extension can updateexpiresAtin that gap, then be overwritten byExpiredand lose its ports. Move the expiry recheck, terminal update, and port release into the lockedonlyIfExpiredoperation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/c2d/compute_engine_docker.ts` around lines 1870 - 1904, The expiry recheck and Expired transition in the service sweep currently occur after stopService releases the lifecycle lease, allowing a concurrent extension to be overwritten. Move the fresh expiresAt validation, terminal status update, and releaseHostPort calls into the locked onlyIfExpired operation, ensuring the operation revalidates expiration and performs the update and port release atomically under the lifecycle lease.src/components/core/service/utils.ts (1)
128-133: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPersist host-port reservations across processes.
This process-local
Setallows separate node processes to reserve the same port for different services. Docker then rejects one paid service’s bind. Use a SQLite reservation with a unique port constraint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/core/service/utils.ts` around lines 128 - 133, Update reserveHostPort to persist reservations in SQLite rather than only adding to the process-local allocatedPorts Set. Use a reservations table with a unique constraint on the port and insert the requested port atomically, while preserving idempotent behavior when the port is already reserved; ensure all host-port allocation checks use the same database-backed reservation.src/test/integration/services.test.ts (1)
816-829: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that an
InternalLooptick occurs duringPullImage.The test can pass without exercising orphan recovery if no loop tick runs during the delay. Spy on and assert the pending-start query or equivalent loop activity before issuing concurrent commands.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/integration/services.test.ts` around lines 816 - 829, Update the restart integration test’s PullImage wait around getServiceJob to also spy on the pending-start query or equivalent InternalLoop activity, and assert that at least one loop tick occurs while the service remains in PullImage before issuing concurrent commands. Preserve the existing sawPullImage timeout assertion and ensure the new assertion fails if no loop activity is observed during that phase.
🧹 Nitpick comments (1)
src/test/unit/service/serviceJobsDatabase.test.ts (1)
328-365: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover expired
Stoppingjobs.
getExpiredServiceJobs()now includesStoppingspecifically for crash-mid-stop recovery, but this test only exercisesRunning,Error, andStopped. Add expired and futureStoppingfixtures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/unit/service/serviceJobsDatabase.test.ts` around lines 328 - 365, Extend the getExpiredServiceJobs test with expired and future fixtures using ServiceStatusNumber.Stopping, persist both through db.newServiceJob, and assert the expired job is returned while the future job is excluded. Keep the existing status coverage and expectations unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/c2d/compute_engine_docker.ts`:
- Around line 3902-3909: Update the service lock acquisition catch block around
the lease logic to fail closed instead of setting acquired = true. Roll back any
local reservation made before acquisition, then reject or defer the lifecycle
operation so Docker and database mutations cannot proceed without the SQLite
lease; preserve the warning log and ensure the failure is propagated to the
caller.
In `@src/components/core/service/extendService.ts`:
- Around line 241-254: Update the intent persistence flow in the extension
payment logic around freshJob.extendPayments and updateServiceJob so that, when
updateServiceJob throws, it calls cancelExpiredLock for the confirmed lockTx.
Confirm the cancellation succeeded; if cancellation fails or cannot be
confirmed, return HTTP 409, while preserving the existing error handling for
successful compensation.
---
Duplicate comments:
In `@src/components/c2d/compute_engine_docker.ts`:
- Around line 1870-1904: The expiry recheck and Expired transition in the
service sweep currently occur after stopService releases the lifecycle lease,
allowing a concurrent extension to be overwritten. Move the fresh expiresAt
validation, terminal status update, and releaseHostPort calls into the locked
onlyIfExpired operation, ensuring the operation revalidates expiration and
performs the update and port release atomically under the lifecycle lease.
In `@src/components/core/service/utils.ts`:
- Around line 128-133: Update reserveHostPort to persist reservations in SQLite
rather than only adding to the process-local allocatedPorts Set. Use a
reservations table with a unique constraint on the port and insert the requested
port atomically, while preserving idempotent behavior when the port is already
reserved; ensure all host-port allocation checks use the same database-backed
reservation.
In `@src/test/integration/services.test.ts`:
- Around line 816-829: Update the restart integration test’s PullImage wait
around getServiceJob to also spy on the pending-start query or equivalent
InternalLoop activity, and assert that at least one loop tick occurs while the
service remains in PullImage before issuing concurrent commands. Preserve the
existing sawPullImage timeout assertion and ensure the new assertion fails if no
loop activity is observed during that phase.
---
Nitpick comments:
In `@src/test/unit/service/serviceJobsDatabase.test.ts`:
- Around line 328-365: Extend the getExpiredServiceJobs test with expired and
future fixtures using ServiceStatusNumber.Stopping, persist both through
db.newServiceJob, and assert the expired job is returned while the future job is
excluded. Keep the existing status coverage and expectations unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f4c8de2f-b072-47be-8325-ff9b0a2702d4
📒 Files selected for processing (18)
docs/services.mdsrc/@types/C2D/ServiceOnDemand.tssrc/components/c2d/compute_engine_base.tssrc/components/c2d/compute_engine_docker.tssrc/components/core/service/extendService.tssrc/components/core/service/getStreamableLogs.tssrc/components/core/service/restartService.tssrc/components/core/service/startService.tssrc/components/core/service/stopService.tssrc/components/core/service/utils.tssrc/components/database/C2DDatabase.tssrc/components/database/sqliteCompute.tssrc/test/integration/services.test.tssrc/test/unit/compute.test.tssrc/test/unit/service/serviceHandlers.test.tssrc/test/unit/service/serviceJobsDatabase.test.tssrc/test/unit/service/serviceNetworkCleanup.test.tssrc/test/unit/service/serviceRestartRace.test.ts
| } catch (e: any) { | ||
| // DB trouble must not brick every lifecycle op on a single-process node — fall | ||
| // back to in-process-only locking (the pre-lease behavior). | ||
| CORE_LOGGER.warn( | ||
| `service lock DB acquire failed for ${serviceId} (falling back to in-process lock): ${e.message}` | ||
| ) | ||
| acquired = true | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Fail closed when SQLite lease acquisition fails.
Setting acquired = true allows Docker and database mutations while another process may own the lease, recreating the lifecycle race this PR fixes. Roll back the local reservation and reject or defer the operation.
Proposed fix
} catch (e: any) {
- CORE_LOGGER.warn(
- `service lock DB acquire failed for ${serviceId} (falling back to in-process lock): ${e.message}`
- )
- acquired = true
+ this.serviceOpsInFlight.delete(serviceId)
+ CORE_LOGGER.error(`service lock DB acquire failed for ${serviceId}: ${e.message}`)
+ return false
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/c2d/compute_engine_docker.ts` around lines 3902 - 3909, Update
the service lock acquisition catch block around the lease logic to fail closed
instead of setting acquired = true. Roll back any local reservation made before
acquisition, then reject or defer the lifecycle operation so Docker and database
mutations cannot proceed without the SQLite lease; preserve the warning log and
ensure the failure is propagated to the caller.
| // Persist the extension INTENT (lockTx recorded, claim pending) BEFORE | ||
| // claiming: a crash between claim and the final write is then auditable — the | ||
| // consumer's money can never be taken without a durable record of why — and a | ||
| // retry finds the intent (unresolved branch above) instead of charging twice. | ||
| const intent = { | ||
| chainId: task.payment.chainId, | ||
| token: task.payment.token, | ||
| lockTx, | ||
| claimTx: '', | ||
| cancelTx: '', | ||
| cost: costExtend | ||
| } | ||
| freshJob.extendPayments = [...(freshJob.extendPayments ?? []), intent] | ||
| await engine.db.updateServiceJob(freshJob) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Cancel the escrow lock when intent persistence fails.
If updateServiceJob() throws here, the outer catch returns without cancelling the confirmed lock or recording its intent. A retry can create another lock for the same extension. Compensate this failed write with cancelExpiredLock; return 409 if cancellation cannot be confirmed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/core/service/extendService.ts` around lines 241 - 254, Update
the intent persistence flow in the extension payment logic around
freshJob.extendPayments and updateServiceJob so that, when updateServiceJob
throws, it calls cancelExpiredLock for the confirmed lockTx. Confirm the
cancellation succeeded; if cancellation fails or cannot be confirmed, return
HTTP 409, while preserving the existing error handling for successful
compensation.
New command:
|
3 participants
Fix: service restart/stop race with the InternalLoop (follow-up to #1416)
Problem
Service restarts still failed on live nodes after #1416, always with:
Live-node logs showed the smoking gun — the loop's orphan recovery ran for the very
service being restarted, seconds before the failure:
Root cause
A race between
restartService()and the engine'sInternalLoop(2 s tick):restartService()runs synchronously from the handler and persists intermediatestatuses (
Starting→PullImage/BuildImage) while it pulls the image.(
getPendingServiceStarts) and launchesprocessServiceStart()for any serviceId notin the in-memory guard set — which only ever covered loop-launched starts.
restartService()never registered itself.PullImage≠Startingsent it intothe orphan-recovery branch (crash cleanup), which removes the service network by
its deterministic name
ocean-svc-<serviceId>, releases the host ports and flips thejob to
Error.container.start()→ 404 "network not found".The race predates #1416, but was silent (clobbered status / double-released ports).
#1416's removal-by-name made it deterministic: any restart whose image pull spans a
2-second tick lost the race — i.e. essentially every restart.
stopService()had the same family of races in both directions (a stop tearing down anin-flight start/restart's fresh network; a restart racing an in-flight stop or the expiry
sweep).
Fix
src/components/c2d/compute_engine_docker.ts:(
servicesBeingStarted→serviceOpsInFlight): at most one lifecycle operation — theloop-driven start pipeline,
restartService, orstopService— runs per service.restartService()andstopService()acquire the lock at entry(
acquireServiceLifecycleLock) and release it infinally. If the lock is held theythrow
Service <id> has a start/stop/restart operation in progress — retry shortly;both handlers already map throws to an HTTP error, so concurrent/duplicate requests get
a clear rejection instead of corrupting each other.
sweep no longer marks a service
Expiredwhen the stop was deferred by the lock(that would leak the container/ports) — it retries on the next tick instead.
empty at boot, so the loop still orphan-cleans stale
PullImage-state jobs.stop()now drains handler-driven ops too (review finding): the promise setthe loop already used for its fire-and-forget starts was generalized
(
serviceStartPromises→serviceOpPromises) andrestartService/stopServiceregister themselves in it, so
stop()returns only once no lifecycle op is stilltouching Docker/escrow on the shared DB.
checkRunningServicesskipsservices with a lifecycle op in flight (a restart intentionally kills the container —
not an "unexpected death"), and
markServiceFailedbails when the job's containerIdchanged since the health snapshot (a completed restart replaced the container), so a
healthy restarted service can't be flagged
Errorby a stale check.docs/services.md: documented the "lifecycle operations are exclusive per service"behavior and the retry semantics.
Tests
src/test/unit/service/serviceRestartRace.test.ts(7 tests):would run orphan recovery when unlocked — the pre-fix behavior)
stop()drains an in-flight handler-drivenstopServicebefore returning(l3)insrc/test/integration/services.test.ts: reproducesthe live-node scenario end-to-end — delays
pullImageRefby 5 s so real InternalLoopticks land mid-restart, asserts concurrent SERVICE_STOP / SERVICE_RESTART are rejected
meanwhile, and that the restart completes to
Runningon the same host port with theservice answering HTTP. Fails pre-fix exactly like the live node.
compute.test.ts,serviceNetworkCleanup.test.ts) seededwith the new lock field (their
Object.create(prototype)engines skip fieldinitializers).
Review findings addressed / rejected
stop()not draining directrestartService/stopServicecalls(see Fix above).
doRestartServiceto persistStarting+ clearedcontainerId/networkId before tearing down the old container/network. That stretches
the crash window in which the DB holds a bare
Startingrecord from milliseconds (twoconsecutive DB writes) to 10+ seconds of Docker teardown I/O. On reboot the loop treats
Startingas a brand-new start and runs the full pipeline — creating and claiming asecond escrow lock (double-charging the consumer), allocating new host ports
(silently changing the consumer's endpoints), and — since the crash was mid-teardown —
potentially leaving the old container running as an unpaid zombie no longer
referenced by the job record. The current order crashes into a benign state instead
(job still
Runningwith stale ids → the pre-existing boot health check flips it toError→ the consumer re-issues restart, whose teardown handles the missing resourcesvia benign 404s on the same ports/payment — no reliance on the new health-check
guards). The ordering constraint is now documented in a comment at the teardown site.
The reviewer's underlying concern (the health check racing a restart) is closed by the
targeted
checkRunningServices/markServiceFailedhardening instead.Verification
npm run type-checkclean,npm run lintgreen.npm run test:servicesintegration) requires Barge — validated in CI.Summary by CodeRabbit