fix race bug#1422
Conversation
alexcos20
requested review from
andreip136,
bogdanfazakas,
dnsi0 and
giurgiur99
as code owners
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 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 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.
| it('(l3) SERVICE_RESTART survives InternalLoop ticks mid-restart (orphan-recovery race)', async function () { | ||
| this.timeout(DEFAULT_TEST_TIMEOUT * 4) | ||
| const engine: any = getDockerEngine() | ||
| const before = await getServiceJob(serviceId) | ||
| const oldContainerId = before.containerId | ||
|
|
||
| // Delay the image pull so the job sits in PullImage across several InternalLoop | ||
| // ticks (cronTime = 2 s) — the exact window in which the loop's orphan-recovery | ||
| // used to tear down the network the restart had just created, failing the restart | ||
| // with "network ocean-svc-<id> not found" (the live-node bug). | ||
| const originalPull = engine.pullImageRef | ||
| engine.pullImageRef = async (...args: any[]) => { | ||
| await sleep(5000) | ||
| return originalPull.apply(engine, args) | ||
| } | ||
| try { | ||
| const { | ||
| consumerAddress: addr, | ||
| nonce, | ||
| signature | ||
| } = await signFor(consumerAccount, PROTOCOL_COMMANDS.SERVICE_RESTART) | ||
| const task: ServiceRestartCommand = { | ||
| command: PROTOCOL_COMMANDS.SERVICE_RESTART, | ||
| consumerAddress: addr, | ||
| nonce, | ||
| signature, | ||
| serviceId | ||
| } | ||
| const restartPromise = new ServiceRestartHandler(oceanNode).handle(task) | ||
|
|
||
| // wait until the restart is actually mid-pull (deterministic, not sleep-based) | ||
| const deadline = Date.now() + 10_000 | ||
| while (Date.now() < deadline) { | ||
| const j = await getServiceJob(serviceId) | ||
| if (j?.status === ServiceStatusNumber.PullImage) break | ||
| await sleep(250) | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the orphan-recovery synchronization observable.
Lines 817-823 silently continue if PullImage is never reached, and the test never verifies that an InternalLoop tick occurred during that state. It can therefore pass without exercising the advertised race. Assert both conditions before issuing the 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 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.
|
@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
| // A fresh DB lease means ANOTHER process is mid-operation on this service (its | ||
| // teardown makes the container look dead); our in-memory serviceOpsInFlight can't | ||
| // see that. Skip — if the container is genuinely dead, the next tick catches it. | ||
| // Fail CLOSED: when the lease state can't be read, assume locked and skip this | ||
| // tick rather than risk flipping a mid-restart service to Error. | ||
| const lockedElsewhere = await this.db | ||
| .isServiceLocked(job.serviceId, SERVICE_LOCK_STALE_MS) | ||
| .catch(() => true) | ||
| if (lockedElsewhere) { | ||
| CORE_LOGGER.debug( | ||
| `markServiceFailed ${job.serviceId}: skipped — a lifecycle lease is held elsewhere` | ||
| ) | ||
| return | ||
| } | ||
| fresh.status = ServiceStatusNumber.Error | ||
| fresh.statusText = `service container exited unexpectedly: ${reason}` | ||
| await this.db.updateServiceJob(fresh) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Serialize the health-failure transition with lifecycle operations.
isServiceLocked() is only an observation. Another process can acquire the lease and persist Restarting after this check but before updateServiceJob(), allowing this stale Running snapshot to overwrite it with Error. Acquire the lifecycle lease or use an atomic conditional update.
🤖 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 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.
| if (cleanupError) { | ||
| await this.logServiceDockerState( | ||
| `stop ${serviceId}: FAILED (${cleanupError.message}) — docker state at failure`, | ||
| serviceId | ||
| ) | ||
| job.status = ServiceStatusNumber.Error | ||
| job.statusText = `stop failed: ${cleanupError.message}` | ||
| } else { | ||
| job.status = ServiceStatusNumber.Stopped | ||
| job.statusText = ServiceStatusText[ServiceStatusNumber.Stopped] | ||
| // The docker objects are confirmed gone — drop the stale ids so nothing later | ||
| // mistakes them for live resources (a restart re-creates and re-persists both). | ||
| job.containerId = '' | ||
| job.networkId = '' | ||
| CORE_LOGGER.debug(`stopService ${serviceId}: stopped, container + network removed`) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Retain CPU pinning when teardown fails.
Line 4033 releases the service’s CPU allocation before Docker cleanup is confirmed. If stopping fails and the old container remains alive, another workload can be assigned the same physical cores. Release the allocation only in the successful teardown branch.
🤖 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 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.
| let claimTx: string | null | ||
| try { | ||
| claimTx = await engine.escrow.claimLock( | ||
| task.payment.chainId, | ||
| task.serviceId, | ||
| task.payment.token, | ||
| task.consumerAddress, | ||
| costExtend, | ||
| `service-extend:${task.serviceId}` | ||
| ) | ||
| } catch (e: any) { | ||
| claimTx = null | ||
| CORE_LOGGER.error(`Service extend claimLock failed: ${e.message}`) | ||
| } | ||
| if (!claimTx) { | ||
| await engine.escrow | ||
| .cancelExpiredLock( | ||
| task.payment.chainId, | ||
| task.serviceId, | ||
| task.payment.token, | ||
| task.consumerAddress | ||
| ) | ||
| .catch((e) => CORE_LOGGER.error(`cancelExpiredLock failed: ${e.message}`)) | ||
| return { | ||
| stream: null, | ||
| status: { httpStatus: 402, error: 'Escrow claim failed — lock cancelled' } | ||
| } | ||
| } | ||
|
|
||
| CORE_LOGGER.logMessage( | ||
| `Service ${task.serviceId} extended by ${task.additionalDuration}s, new expiresAt: ${job.expiresAt}`, | ||
| true | ||
| ) | ||
| return { | ||
| stream: Readable.from(JSON.stringify([toPublicServiceJob(job)])), | ||
| status: { httpStatus: 200 } | ||
| // Payment successful — push expiresAt forward and record extension payment | ||
| freshJob.expiresAt += task.additionalDuration * 1000 | ||
| freshJob.duration += task.additionalDuration | ||
| freshJob.extendPayments = [ | ||
| ...(freshJob.extendPayments ?? []), | ||
| { | ||
| chainId: task.payment.chainId, | ||
| token: task.payment.token, | ||
| lockTx, | ||
| claimTx, | ||
| cancelTx: '', | ||
| cost: costExtend | ||
| } | ||
| ] | ||
| await engine.db.updateServiceJob(freshJob) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Make the extension durable before claiming payment.
If claimLock() succeeds but updateServiceJob() fails, the consumer is charged while expiresAt remains unchanged; the outer catch then encourages a retry. Persist an idempotent extension intent before claiming and recover/finalize it after failures.
🤖 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 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.
| // 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.
| it('refreshServiceLocks re-stamps a holder’s rows so they stay unstealable', async () => { | ||
| const serviceId = `lock-svc-${Date.now()}-d` | ||
| expect(await db.acquireServiceLock(serviceId, 'proc-A', STALE_MS)).to.equal(true) | ||
| await db.refreshServiceLocks('proc-A') | ||
| // B considers anything older than 5s stale — A's row was just re-stamped | ||
| expect(await db.acquireServiceLock(serviceId, 'proc-B', 5_000)).to.equal(false) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the heartbeat test fail when refreshServiceLocks is a no-op.
The lease is already fresh under the five-second threshold before refresh. Advance or fake time past the threshold before refreshing, then verify holder B cannot acquire.
🤖 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 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.
Fixed (5 of 6) in beb981d
Skipped (1)
|
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