Skip to content

feat(deploy): Slice B2 — peer-side blob reads + install-line streaming - #760

Merged
kriszyp merged 7 commits into
mainfrom
feat/deployment-tracking-slice-b2
May 30, 2026
Merged

feat(deploy): Slice B2 — peer-side blob reads + install-line streaming#760
kriszyp merged 7 commits into
mainfrom
feat/deployment-tracking-slice-b2

Conversation

@kriszyp

@kriszyp kriszyp commented May 23, 2026

Copy link
Copy Markdown
Member

Slice B2 of the deployment-tracking redesign in #641. Closes #758.

Summary

What changed

Origin side (components/operations.js):

  • Strip req.payload before replicateOperation so peers don't receive a consumed Readable.
  • After replicateOperation returns, normalize the per-peer outcomes and write them to row.peer_results via the new DeploymentRecorder.recordPeers().

Peer side (components/operations.js + components/deploymentRecorder.ts):

  • New branch in deployComponent: when req._deploymentId is set and req.payload is absent, look up hdb_deployment[deployment_id] via the new awaitDeploymentRow() helper, then extract from row.payload_blob.stream(). The Blob API already handles in-flight BLOB_CHUNK writes by blocking until chunks arrive.
  • awaitDeploymentRow polls with exponential backoff (5ms → 100ms) so the fast path — replication has already caught up — sees no human-perceptible latency.

Install-line streaming (components/Application.ts):

  • nonInteractiveSpawn gains an optional onLine(stream, line) callback with proper line buffering and StringDecoder so multi-byte UTF-8 characters split across chunk boundaries are reassembled, not corrupted into U+FFFD.
  • installApplication threads an onLine closure through all three install paths (custom command, devEngines packageManager, default npm).
  • deployComponent wires onInstallLine to emit('install', {manager, stream, line}) so the SSE channel carries npm install output as it happens.

Defensive write (components/deploymentRecorder.ts):

  • recordPeers swallows audit-write failures with a warn log — peer_results is observability, not critical path. A disk-full on the audit table shouldn't fail a deploy that successfully replicated.

Where to look

  • components/operations.js lines ~420 (peer-side branch + strip) and ~480 (peer_results capture)
  • components/deploymentRecorder.tsrecordPeers, awaitDeploymentRow, normalizePeerResult
  • components/Application.tscreateLineSplitter, nonInteractiveSpawn onLine param, three install paths threading

Cross-model review

agy (Google's model) reviewed the diff and flagged three real issues that are addressed in this PR:

  1. UTF-8 multi-byte char split across chunks was being corrupted by naive chunk.toString(). Fixed with StringDecoder.
  2. recordPeers would have crashed the deploy on an audit table write error. Now logged-and-swallowed.
  3. awaitDeploymentRow had a 100ms fixed polling interval that introduced a latency floor on the fast path. Now exponential backoff starting at 5ms.

Test plan

  • unitTests/components/applicationSpawn.test.js — 6 line-buffering scenarios including UTF-8 split reassembly
  • unitTests/components/deploymentRecorder.test.js — 12 scenarios (recordPeers normalization, finish-then-recordPeers no-op, audit-write resilience, awaitDeploymentRow timeout + fast path)
  • integrationTests/deploy/deploy-tracking-peer-branch.test.ts — exercises the peer-side branch on a single node by seeding an hdb_deployment row first
  • No regression on existing deploy-tracking.test.ts (11 tests) and deploy-tracking-events.test.ts (5 tests)
  • True 3-node verification lives in the matching harper-pro PR — replicateOperation is implemented there

Follow-ups (out of scope)

  • Slice B3: delete_deployment_payload operation + 3-node cluster integration test in harper-pro
  • Slice C: rollback (deploy_component {rollback_from}) + onStorageReclamation blob pruning
  • Cross-node install line forwarding (peers' npm install lines visible on origin SSE) — not yet wired; peers have no emitter

🤖 Generated by Claude

Companion PR

  • harper-pro#221: 3-node cluster integration test that verifies the full round trip. No code changes there — the existing replicateOperation already does generic operation forwarding (no deploy_component-specific path needed). The closed harper-pro#146's chunked-relay was working around a constraint the row-replication channel doesn't have.

Slice B2 of the deployment-tracking redesign in #641. Makes multi-node
deploy_component work after Slice A's payload_blob-on-replicated-row design.

Origin side (components/operations.js):
- Strip req.payload before replicateOperation so peers receive a clean
  operation body (the consumed Readable would otherwise leak).
- Capture per-peer outcomes from replicateOperation's `replicated` return
  into the origin row's peer_results via DeploymentRecorder.recordPeers().

Peer side (components/operations.js + deploymentRecorder.ts):
- When req._deploymentId is set and there's no req.payload, look up the
  hdb_deployment row (await replication arrival with exponential-backoff
  polling, 5ms → 100ms), then extract from row.payload_blob.stream().
- awaitDeploymentRow encapsulates the polling with a 30s default timeout.

Install-line streaming (components/Application.ts):
- nonInteractiveSpawn gains an optional onLine(stream, line) callback
  with proper line buffering (StringDecoder so a multi-byte UTF-8 char
  split across chunks is reassembled instead of corrupted into U+FFFD).
- installApplication threads an onLine closure through all three install
  paths (custom command, devEngines packageManager, default npm).
- deployComponent wires this to emit('install', {manager, stream, line})
  so the SSE channel carries `npm install` output live as it streams.
  Finishes the residual install-streaming work from the closed #531.

DeploymentRecorder.recordPeers swallows audit-write failures so an
hdb_deployment write error doesn't fail a deploy that successfully
replicated to peers (peer_results is observability, not critical path).

Tests:
- unit: 5 line-buffering scenarios including UTF-8 split-byte reassembly
- unit: 11 recordPeers / awaitDeploymentRow scenarios
- integration: peer-side branch via seeded row (single-node simulation
  of the operation shape origin produces for peers)

The true 3-node cluster verification lives in the matching harper-pro
PR — replicateOperation is implemented there.

Cross-model reviewed: agy flagged the UTF-8 split-chunk bug, the
recordPeers crash-on-DB-error, and the polling latency floor — all
addressed in this commit.

Closes #758

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude

claude Bot commented May 23, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

The deploy hangs after extraction when reading a Web ReadableStream from
a file-backed Blob inside the same Harper process on Bun — same code
passes on Node v22/v24 across Linux and Windows. The harper-pro 3-node
cluster test (HarperFast/harper-pro#221) covers the same code path
end-to-end with real replication, so this skip doesn't lose coverage.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kriszyp
kriszyp requested review from Ethan-Arrowood and heskew May 23, 2026 04:00
kriszyp and others added 3 commits May 23, 2026 08:39
recordPeers was issuing its own put() concurrent with coalesced
emitter-triggered puts from phase events. Each put captures `this.record`
as the serializer sees it, and we were observing peer_results=[] in the
persisted row even though recordPeers mutated the in-memory state to a
populated array. The race: an earlier scheduleFlush's put (called when
peer_results was still []) sometimes completes AFTER recordPeers' direct
put, overwriting our write with the stale snapshot.

Fix: recordPeers no longer puts. It stashes the input on the recorder
and mutates the in-memory record (so live SSE readers still see the
update immediately). finish() re-applies the stash right before the
terminal status-transition put, bundling peer_results with status=success
into one atomic write. No concurrent puts after this point.

Unit test rewritten to reflect the new contract: recordPeers populates
the in-memory record but persistence happens at finish().

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The .catch in recordPeers' put was removed when peer_results moved into
finish(). The logger import is no longer needed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lback

Two design improvements landed together:

1. Per-peer real-time progress
   - replicateOperation accepts an optional `onPeerResult` callback that
     fires as each peer settles, in addition to returning the aggregate.
   - DeploymentRecorder gains `recordPeer(result)` that upserts a single
     peer entry by node name and triggers a coalesced flush, so the row
     reflects per-peer progress rather than only the final aggregate.
     `recordPeers(results)` becomes a thin wrapper over recordPeer for
     bulk fallback.
   - deployComponent wires onPeerResult to recorder.recordPeer + emits a
     `peer` SSE event so the CLI/Studio sees each peer transition live.

2. REPLICATION_DATABASES fallback
   - Origin checks whether the `system` database is replicated locally
     (mirroring shouldReplicateFromNode's database-level filter) before
     stripping req.payload. If `system` doesn't replicate, the payload
     stays in the operation body so peers can deploy via the legacy
     direct-payload path. Cross-node config asymmetry is intentionally
     not addressed here — local view is the canonical signal.

Defensive fixes from cross-model review (agy):
- isSystemDatabaseReplicated handles single-string config (a non-'*'
  string is treated as "only that one database replicates", not
  wildcard).
- recordPeer lazily initializes peer_results when absent on a row read
  back from replication.
- finish() now drains the FULL coalesced-flush chain, not just the
  first pendingPut — a chained re-flush from `dirty` could otherwise
  capture pre-mutation state and overwrite status=success.

Also strips transient slice naming ("Slice A/B/B1/B2 of #641") from
comments and test titles; the codebase shouldn't carry the
PR-succession terminology that was only meaningful during development.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kriszyp

kriszyp commented May 26, 2026

Copy link
Copy Markdown
Member Author

Round of review-driven changes pushed (commit 64d4ec4):

1. Per-peer real-time tracking (your first review point)

  • DeploymentRecorder.recordPeer(result) now upserts a single peer entry by node name and triggers scheduleFlush() — peer outcomes land on the row as each peer settles, not just at the aggregate end. recordPeers(results) becomes a thin wrapper over recordPeer for bulk fallback.
  • deployComponent passes an onPeerResult callback into replicateOperation that invokes recorder.recordPeer(r) and emits a peer SSE event. Studio / get_deployment SSE consumers now see per-node deploy progress live.
  • Companion change in harper-pro #221 adds the per-peer .then/.catch wiring in replicateOperation.
  • Caveat in comments: cross-node install line forwarding is still aggregate-only — would need extra plumbing.

2. REPLICATION_DATABASES fallback (your second review point)

  • New isSystemDatabaseReplicated() helper mirrors shouldReplicateFromNode's database-level filter (handles undefined, '*', single-string, array-with-{name} entries).
  • Origin only strips req.payload when system actually replicates locally. Otherwise the payload stays in the operation body so peers can deploy via the legacy direct-payload path.
  • Peer-side branch already tolerates this: when both _deploymentId AND req.payload are present, the row lookup is skipped and we extract from payload.
  • Per your guidance, we only check the local node's config — partial system-replication asymmetry is out of scope.

3. Slice naming stripped (your third review point)

Cross-model review (agy) caught three real issues, all addressed in this commit:

  • isSystemDatabaseReplicated mishandled a non-'*' string (would incorrectly treat 'data' as wildcard). Fixed.
  • recordPeer could throw on a row read back from replication where peer_results is absent. Lazy-init added.
  • finish() only drained the first pendingPut — a chained re-flush from dirty could capture pre-mutation state and overwrite status=success. Now drains the full chain.

15 unit tests + 11 integration tests pass locally.

🤖 Generated by Claude

@kriszyp
kriszyp marked this pull request as ready for review May 26, 2026 16:47
kriszyp and others added 2 commits May 28, 2026 22:37
File came in from main with stale formatting that fails the Format Check
workflow. Format Check has been failing on main too — this fix lets our
PR pass independently while main is sorted out separately.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kriszyp

kriszyp commented May 29, 2026

Copy link
Copy Markdown
Member Author

Post-merge CI noise from main, addressed:

Format Check (fixed)unitTests/resources/models/Models.test.js came in from main with stale formatting; ran prettier on it (commit efcd324). Note: Format Check has been failing on main too, so the source there needs the same fix at some point.

Unit Test (v20) / Integration API (v20)Error: Unable to locate rocksdb-js native binding on Node v20. Same failure on main (last 3 runs all failed). Looks like rocksdb-js dropped Node v20 support somewhere recently. v22 and v24 pass cleanly on this PR. Not addressing here since it's pre-existing on main and unrelated to deployment tracking.

🤖 Generated by Claude

@kriszyp

kriszyp commented May 30, 2026

Copy link
Copy Markdown
Member Author

I'm going to merge this so I can do more comprehensive testing of the merged state of harper, post-merge comments are still welcome.

@kriszyp
kriszyp merged commit ab804df into main May 30, 2026
35 of 37 checks passed
@kriszyp
kriszyp deleted the feat/deployment-tracking-slice-b2 branch May 30, 2026 13:08
kriszyp added a commit to HarperFast/harper-pro that referenced this pull request Jun 2, 2026
… B2) (#221)

* test(cluster): multi-node deployment-tracking integration test

3-node cluster test verifying Slice B2 of HarperFast/harper#641: payload
travels to peers via the replicated hdb_deployment.payload_blob row
attribute through Harper's existing BLOB_CHUNK channel, not via the
operations API body.

Asserts:
- deploy_component from node 0 succeeds, component loads on all 3 nodes
- hdb_deployment row replicates and is queryable from any node
- origin row's peer_results is populated with success entries for both
  peer nodes (proves origin captured per-peer outcomes from
  replicateOperation's return)

The OSS-side counterpart (HarperFast/harper#760) exercises the peer-side
branch on a single node by seeding the row first; this test verifies the
full multi-node round trip the new design depends on.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* chore(core): bump core to feat/deployment-tracking-slice-b2

Temporary bump so the new deployTrackingReplication.test.mjs cluster test
runs against harper's Slice B2 changes (HarperFast/harper#760). Re-target
to harper main HEAD once #760 merges.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test(cluster): drop restart:true so peer_results write is durable

The fullyConnectedReplication test uses restart:true to verify the new
component routes are loaded on peers, but for verifying B2's peer_results
tracking we need a clean deploy. Restarting HTTP workers mid-flow cycles
the worker that owns the recorder before recorder.finish() can flush the
final put with peer_results.

Removed the /Location/2 component-reachability check since it requires
restart:true; the harper #760 single-node test already verifies the peer
branch extracts from the row blob. This cluster test focuses on what's
unique: row replication + peer_results capture.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test(cluster): drop unused import + surface replicate-dispatch via assertion

- Remove unused `fetchWithRetry` import that broke lint after dropping the
  /Location/2 component-reachability check.
- Add explicit assertion that `deployResponse.replicated` is a non-empty
  array. If `replicateOperation` doesn't dispatch to peers (origin's
  `server.nodes` empty), the failure now surfaces here instead of as a
  silent empty `peer_results` later — clearer signal for triage.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* chore(core): bump core to pick up peer_results race fix

Includes harper commits up to 6d7cd99c (fix to eliminate the put race
where peer_results was lost — now bundled into the terminal finish()
put).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test(cluster): poll for replicated row instead of fixed sleep

The 1s sleep after deploy wasn't enough on slower CI shards (Node v22
shard 3) for the terminal-state finish() put to propagate via table
replication. Poll up to 15s with 250ms cadence — fast in the happy case,
patient enough for slower environments. peer_results assertion test
already passing after the race fix landed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* ci: force-trigger pull_request workflows on the polling-fix commit

* ci: trigger workflows after PR ready-for-review

* feat(replication): per-peer onPeerResult callback in replicateOperation

Adds optional `options.onPeerResult(result)` to replicateOperation.
Each peer's result fires the callback as soon as it settles (success
or failure), letting callers surface per-peer progress in real time
instead of waiting for the aggregate Promise.allSettled to return.

Backward compatible: the callback is optional, all existing call sites
that pass just `(req)` continue to work. The aggregate response.replicated
array is still produced as before.

Companion to harper#760 — deployComponent uses this to populate
hdb_deployment.peer_results in real time so Studio / get_deployment SSE
can show per-node deploy progress live rather than only at the end.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* refactor(replicator): use captured node variable, not server.nodes[index] in async callbacks

Address claude-review nit: `.then()` and `.catch()` callbacks were re-reading
`server.nodes[index]?.name` asynchronously. If `server.nodes` is replaced (not
mutated in-place) between the synchronous `.map()` and a callback settling,
the index would point at a different node or be undefined. Use the `node`
variable already captured by the outer closure instead.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(cluster): bump deploy-tracking replication poll timeout to 30s

Integration Tests 1/4 on Node v26 intermittently fails with peer node 2
seeing the row in `replicating` state at the 15s deadline — origin's final
`success` write hadn't propagated yet. v22/v24 of the same shard pass, so
this is v26-only slowness, not a logic regression. Doubling the polling
budget so slower v26 cold-start runs have headroom.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deployment tracking — Slice B2: peer-side blob reads + install-line streaming

1 participant