Repository navigation
feat(hub): replace a listener whose mail the hub cannot route - #683
TheGreatAxios merged 5 commits into
Conversation
|
Triage: Fail @TheGreatAxios, please address the following:
Changed since the last verdict (Pass at |
There was a problem hiding this comment.
Replaces a GitHub or Bitbucket listener whose connected sidecar has no mail route in the hub's router, and reports replaced listeners that stock never stops. The most important risk is that the new readiness and wait paths call router and lifecycle methods unbound and may misjudge listeners that are still starting, which could disable the check or replace a listener that is not broken.
| async cancel(tenantId, runId) { | ||
| await composition.workflowLifecycleService.requestCancellation(tenantId, runId, "replaced"); | ||
| }, | ||
| stop: composition.workflowLifecycleService.getStatus, |
There was a problem hiding this comment.
P1 · getStatus is passed unbound and may lose this
stop: composition.workflowLifecycleService.getStatus hands over the method without binding it. The neighbouring cancel wraps the service call, so this one is inconsistent with it.
If getStatus is a class method that reads this, every call from waitUntilStopped throws. Each roll's settle then fails after the new listener has already deployed.
Trigger: any roll that replaces a listener and then calls deps.stop.
Suggested fix: wrap it like cancel, for example stop: (runId) => composition.workflowLifecycleService.getStatus(runId), or bind it explicitly.
| allocation: function allocationOf(runId) { | ||
| return deployedAllocation(composition.db, runId); | ||
| }, | ||
| routable: composition.sidecarRouter.isAllocatedWorkflowActive, |
There was a problem hiding this comment.
P1 · isAllocatedWorkflowActive is passed unbound
routable: composition.sidecarRouter.isAllocatedWorkflowActive loses this if the router defines it as a prototype method. The resulting TypeError is caught in watch and logged at debug as listener_disconnected, so the whole unroutable check would be silently disabled.
Trigger: every reconcile pass with a deployed listener.
Suggested fix: wrap the call, for example routable: (target) => composition.sidecarRouter.isAllocatedWorkflowActive(target), and add a test that calls the real router method through the wiring.
| for (;;) { | ||
| const run = await deps.stop(tenantId, runId); | ||
| if (!run || !schema.isLiveWorkflowRunStatus(run.status)) return; | ||
| if (run.cancellationDeadline === null) throw new Error(`Deployment ${runId} is live with no stop requested.`); |
There was a problem hiding this comment.
P2 · Roll fails after deploying when a cancelled run has no deadline yet
settle runs after the new listener is live. If stock's status still shows a just-cancelled run as live with a null cancellationDeadline, the throw fails the roll. The host is then left unrecorded and the roll is retried.
Trigger: requestCancellation completes before the status read reflects a deadline.
Suggested fix: treat a null deadline as not yet settled and keep polling, or retry the read, instead of throwing. Only throw after a bounded number of attempts.
| /** Whether a deployment whose stop was requested is still live after stock stopped trying to stop it. */ | ||
| export function stopOverdue(run: RunStop, now: Date): boolean { | ||
| if (!schema.isLiveWorkflowRunStatus(run.status) || run.cancellationDeadline === null) return false; | ||
| return now.getTime() >= Date.parse(run.cancellationDeadline) + STOP_GRACE_MS; |
There was a problem hiding this comment.
P2 · stopOverdue ignores when stock starts its give-up clock
Stock gives up on an unreachable stop at max(cancellationDeadline, startedAt) + unreachableStopGraceMs, where startedAt is the lifecycle service's start time. The hub computes deadline + 60s and ignores startedAt.
After a hub restart, or when the service started after the deadline, stock keeps retrying past the hub's threshold. The hub then logs listener_cancel_stuck and proceeds, while stock still intends to stop the run and later force-release it.
Trigger: the lifecycle service starts after a stop's deadline has passed and the old sidecar stays unreachable.
Suggested fix: use the same max(cancellationDeadline, startedAt) base that stock uses, or read stock's own give-up time from the status if it is exposed.
| eq(schema.workflowRun.id, runId), | ||
| isNull(schema.workflowRun.cancellationRequestedAt), | ||
| isNotNull(schema.workflowRun.publicKey), | ||
| eq(schema.sidecarAllocation.status, "allocated"), |
There was a problem hiding this comment.
P2 · deployedAllocation counts allocations still retrying their start as deployed
deploymentState treats an allocated sidecar with a non-null nextAttemptAt as still starting. deployedAllocation filters only on status allocated and ensureAcceptedGeneration, so it returns targets whose first deploy has not completed. The watch then tests routability on them, and the route may not be installed yet.
Trigger: an allocation is allocated with nextAttemptAt set during a retried start or ensure, and no roll is running. The router reports no route and the hub replaces a listener that is still starting.
Suggested fix: also require nextAttemptAt to be null in the deployedAllocation query, or reuse the deploymentState check, so the docstring's 'once its first deploy completed' holds.
| else deps.log({ level: "debug", msg: "listener_disconnected", ...entry, reason: errorText(error) }); | ||
| continue; | ||
| } | ||
| if (routed || !deps.replace(tenantId, source)) continue; | ||
| deps.log({ level: "warn", msg: "listener_unroutable", ...entry, reason: "its sidecar is connected but the hub holds no route to it" }); |
There was a problem hiding this comment.
P3 · The readiness probe can undeploy a listener, which the diff does not mention
isAllocatedWorkflowActive calls getProvisionedConnection(target, 'readiness'). When the sidecar identity is no longer current, that path calls detachBinding(..., { undeploy: true }) and fences cleanup bindings. The watch now runs this on every reconcile pass, so a periodic check can stop a live listener.
The description says a disconnected or unvalidated listener is left to stock, but this path is not covered by that claim.
Trigger: the listener's sidecar identity stops validating as current, for example after a credential or identity change. The next pass undeploys the listener.
Suggested fix: give the router a read-only check that never detaches or undeploys, or pass a flag that disables the undeploy side effect for this call, and document the behaviour.
| for (const workflow of [TRIAGE_LISTENERS[source], ...REPLACED[source]]) { | ||
| for (const live of await liveOf(tenantId, workflow)) { | ||
| if (live.runId !== kept) await deps.cancel(tenantId, live.runId); | ||
| if (live.runId === kept) continue; |
There was a problem hiding this comment.
P3 · settle reports replaced non-listener workflows as listener_cancel_stuck
settle waits on every cancelled run, including replaced workflows such as pr-triage and pr-triage-historical, not only the triage listener. waitUntilStopped then logs listener_cancel_stuck for them, so the log name no longer matches what was stopped.
Trigger: a roll cancels an old pr-triage-historical deployment that stock never stops. The hub logs listener_cancel_stuck for a workflow that is not a listener.
Suggested fix: either filter the wait to listener deployments, or use a generic line such as deployment_cancel_stuck with the workflow name and reserve listener_cancel_stuck for listeners.
There was a problem hiding this comment.
Replaces a GitHub or Bitbucket listener whose sidecar is connected but which the hub has no mail route to, and reports replaced listeners that stock never stops. It adds a stop-wait loop after roll cancellations and a 15-second triage reconcile interval. The most important defect is that the stop wait has no upper bound when the cancellation deadline is never set, which can hold a workspace's roll lock and a roll slot indefinitely.
| for (;;) { | ||
| const run = await deps.stop(tenantId, runId); | ||
| if (!run || !schema.isLiveWorkflowRunStatus(run.status)) return; | ||
| if (stopOverdue(run, deps.now())) { | ||
| if (workflow === TRIAGE_LISTENERS[source]) deps.log({ level: "warn", msg: "listener_cancel_stuck", tenantId, source, deployment: runId }); | ||
| else deps.log({ level: "warn", msg: "rollout_cancel_stuck", tenantId, source, workflow, deployment: runId }); | ||
| return; | ||
| } | ||
| await deps.sleep(START_POLL_MS); | ||
| } | ||
| } |
There was a problem hiding this comment.
P1 · Stop wait has no upper bound while the cancellation deadline stays null
stopOverdue returns false whenever cancellationDeadline is null, so the polling loop in rollout.ts never exits. While it runs, settle holds the workspace advisory lock and one of the two hub-wide roll slots, so that workspace cannot roll again and other workspaces lose roll capacity.
Trigger: a replaced run stays live and stock never sets its cancellationDeadline, for example because requestCancellation was a no-op for that run.
Fix: add a hard upper bound on the wait, measured from when the cancel was requested, and treat reaching it as stuck. For example, if (!run.cancellationDeadline && Date.now() - requestedAt > MAX_WAIT_MS) return 'stuck';.
| stopped.push({ workflow, runId: live.runId }); | ||
| } | ||
| } | ||
| await Promise.all(stopped.map((run) => waitUntilStopped(tenantId, source, run.workflow, run.runId))); |
There was a problem hiding this comment.
P2 · A transient status read error fails a roll whose new listener is already live
Promise.all in rollout.ts rejects on any error from deps.stop (getStatus). settle then throws before record() and the success log run, even though the old deployments were already cancelled. The host ends up on the new listener without being recorded on the release, and the roll reports failure.
Trigger: getStatus throws once during the wait, for example on a DB hiccup, or because a run row is gone and stock throws rather than returning null.
Fix: catch and retry status reads inside waitUntilStopped, and only treat the wait as failed after the bounded retry budget is spent. Record the successful replacement before surfacing any stop-wait error.
| return now.getTime() >= Date.parse(run.cancellationDeadline) + STOP_GRACE_MS; | ||
| } |
There was a problem hiding this comment.
P2 · Hub's overdue test ignores the start time stock uses for its forced release
Stock's forced release fires at max(cancellationDeadline, startedAt) + unreachableStopGraceMs, where startedAt is the lifecycle service's boot time. stopOverdue in rollout.ts counts only from the deadline. If the hub restarts after a stop's deadline has passed, stock still has a full minute of retries from its boot, but the hub already reports the listener or workflow as stuck and stops waiting.
Trigger: a cancelled listener or replaced workflow whose deadline passed before the hub booted. stuckListeners and waitUntilStopped then log *_cancel_stuck while stock is still inside its retry grace.
Fix: compute the overdue point as max(cancellationDeadline, startedAt) + STOP_GRACE_MS, using the same start time stock uses, so the hub's stuck verdict matches stock's forced release.
| if (!listener) { | ||
| deps.ensure(tenantId); | ||
| return; | ||
| } | ||
| const target = await deps.allocation(listener.runId); |
There was a problem hiding this comment.
P3 · Stuck listeners on later hosts are not reported when an earlier host has no listener
The early return after ensure in triggers.ts exits the whole pass as soon as one host has no live listener. The remaining sources are skipped, so their listener_cancel_stuck lines and unroutable checks are lost for that pass. This contradicts the description's claim that the line is logged on every pass.
Trigger: GitHub has no live listener and the Bitbucket host has a stuck cancelling listener. The Bitbucket stuck line is never emitted.
Fix: replace the early return with continue so each source is still evaluated on every pass.
7b9c44b to
eccca3e
Compare
…p deadline stock has not set yet Pass the lifecycle status and router readiness calls bound to their owners. A replaced deployment whose stop deadline is not set yet is still waited on instead of failing the roll, and a replaced workflow that stays live is logged as rollout_cancel_stuck with its name.
…l bounded A replaced deployment counts as stuck once stock's lifecycle status shows its allocation no longer dispatchable, instead of the hub re-timing stock's deadline and grace. The stop wait is bounded at five minutes from the request, retries a failed status read three times, and logs rather than fails the roll. The listener watch checks every connected host on each pass. The rollout's status dependency is typed from stock's lifecycle response, its callbacks are named, and the queue reuses schedule for a replace.
eccca3e to
ac998cb
Compare
The hub now replaces a GitHub or Bitbucket listener whose sidecar is connected but which the hub holds no mail route to. It also reports a replaced listener that stock never manages to stop. Before this, both kinds of listener stayed live and nothing said so.
What changed:
isAllocatedWorkflowActiveabout each host's live listener once its first deploy has completed. This is stock's deploy-readiness check, and it reads the same per-connection route table that mail is routed by. The router is the instance the hub already holds. The watch uses stock's readiness check, which undeploys a sidecar whose identity is no longer current, the same as stock's reconciler.listener_unroutableline with the workspace, host, run, allocation, generation and reason.maxDisconnected. Each pass logslistener_disconnectedat debug level for it, orlistener_identity_unknownwhen the router could not validate the sidecar's identity.listener_cancel_stuckwith the workspace, host and run, and every reconcile pass logs that line again while it stays live. A replaced workflow still live is logged once asrollout_cancel_stuckwith the workspace, host, workflow and run.How it was verified:
a listener whose connected sidecar the router holds no route to is started again once, and one whose sidecar is not connected is left to stock. It uses a fake router that answers false for one host and throws for the other, and asserts one roll, for the false host only, and the log lines on each pass.a replaced listener or workflow that stock never stops is waited on until its stop is overdue, then logged as stuck. It uses a fake lifecycle status that never ends the old listener or a replacedpr-triage-historicalrun, and asserts onelistener_cancel_stuckand onerollout_cancel_stuckline.a replaced listener whose stop deadline stock sets late is waited on until 60 s past that deadline, and the roll succeeds. The fake sets the deadline on the third poll.a connected host found with no live listener rolls its workspace, and a listener stock gave up stopping is reported on every pass.a listener whose sidecar disconnected is left to stock, and once it reconnects the next pull request gets its verdict. It freezes the listener's sidecar with SIGSTOP until a pass logslistener_disconnected, then resumes it. It asserts that no listener was replaced and that the next pull request gets a Pass verdict.bun run check: passes.bun run lint: exit 0.bun --conditions=intx-src test apps/hub:431 pass, 0 fail, Ran 431 tests across 78 files.bun run test:chain:8 pass, 0 fail, Ran 8 tests across 1 file. [192.45s]Closes CL-11589