fix(deploy): make peer deployment-row wait configurable, default 120s - #1338
Merged
Conversation
Replicated deploy_component intermittently timed out on peers since 5.1.x (harper-pro#402). The deployment-tracking feature moved the payload off the operation body and into the system.hdb_deployment row's payload_blob; peers wait for that row to replicate via awaitDeploymentRow, which hardcoded a 30s deadline. Under system-table replication backlog the row arrives later than 30s, so the peer threw and the deploy failed — with no parameter to extend it. - awaitDeploymentRow: default deadline 30s -> 120s (DEFAULT_AWAIT_ROW_TIMEOUT_MS). A deploy is rare and heavyweight; a longer peer-convergence budget is cheap, and the row replicates on a path independent of the waiting op (the initial deploy already proves the row arrives mid-wait), so the longer wait extends patience without risk of deadlock. - New optional `deployment_timeout` deploy operation parameter (ms), threaded into the peer branch and added to the Joi schema, so operators can extend the budget per-deploy on heavily-backlogged clusters. - Coerce timeoutMs defensively (Number + finite/non-negative guard): the Joi validator's coerced value is discarded by validateBySchema, so a numeric string from a JSON/multipart client would otherwise concatenate into a far-future deadline (Date.now() + "120000") and silently defeat the timeout. - Poll loop now runs at least once (handles deployment_timeout: 0) and caps the final sleep to the remaining budget instead of overshooting the deadline. - Timeout error distinguishes "row never replicated" (dead channel) from "row arrived but payload_blob pending" (origin's ingestPayload write still in flight) — they point at different root causes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
|
Reviewed; no blockers found. |
kriszyp
marked this pull request as ready for review
June 17, 2026 03:20
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the peer-side timeout in replicated
deploy_componentreported in harper-pro#402. Since 5.1.x the deployment-tracking feature moved the payload off the operation body into thesystem.hdb_deploymentrow'spayload_blob; peers wait for that row to replicate viaawaitDeploymentRow, which hardcoded a 30s deadline with no way to extend it. Undersystem-table replication backlog the row arrives later than 30s, so the peer threwTimed out … waiting for hdb_deployment row …and the deploy failed.30s → 120s(DEFAULT_AWAIT_ROW_TIMEOUT_MS).deployment_timeoutdeploy operation parameter (ms) to extend the budget per-deploy on heavily-backlogged clusters; added to the Joi schema.deployment_timeout: 0performs a single lookup rather than no-op'ing) and caps the final sleep to the remaining budget.ingestPayloadwrite still in flight).Where to look / what to weigh
deploymentRecorder.ts— the 120s default. Rationale: a deploy is rare and heavyweight, and the issue's evidence (the initial deploy always succeeds because the row beats 30s; rows arrive mid-wait) shows the row replicates on a path independent of the waiting op — so the longer wait extends patience without deadlock risk. A peer that will ultimately fail now takes up to 120s (or the operator'sdeployment_timeout) to report it, instead of 30s. Worth a sanity check that 120s is the right default vs. something larger/smaller.deployment_timeoutpropagation. The param is set on the origin call and carried to peers via the replicated operation body (the origin only deletespayload/progressbefore replicating), which is what lets peers honor it. Confirm that's the intended delivery mechanism.Number()coercion oftimeoutMs.validateBySchemadiscards Joi's coerced value, so a numeric-stringdeployment_timeoutfrom a JSON/multipart client would otherwise reach the arithmetic as a string and concatenate into a far-future deadline. Coerced + finite/non-negative guarded in the helper (the single chokepoint where the timestamp math lives).Follow-up (not in this PR)
This is the pragmatic Option-1 fix from the issue. The progress-based / no-progress timeout (issue suggestion #2) is deferred — I'll file a follow-up to track it; the create()→ingestPayload() writes happen back-to-back on the origin, so the intermediate "row present, null blob" progress signal is unreliable in practice and the marginal benefit is uncertain.
Tests
Unit coverage added for the configurable timeout, the numeric-string/NaN coercion, the
timeoutMs: 0single-poll behavior, the two distinct timeout messages, and validator accept/reject ofdeployment_timeout.🤖 Generated by Claude (Opus 4.8, 1M context). Reviewed cross-model (Codex + Gemini) before opening; the findings they surfaced are already addressed in the diff.