Repository navigation
Implement get_deployment_payload and delete_deployment_payload operations - #1898
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for retrieving and deleting deployment payloads. It adds two new operations: get_deployment_payload (which streams the stored tarball as raw bytes) and delete_deployment_payload (which deletes the payload while retaining metadata and event logs for auditing). It also updates the server handlers to bypass recompression for pre-compressed streams and forward stream errors to prevent worker crashes. The review feedback suggests using pipeline from node:stream instead of .pipe() when chaining streams in serverHandlers.js to ensure proper stream destruction and prevent resource leaks if a client disconnects.
|
Reviewed; no blockers found. |
recordCommitLatency() only uses commitResolution.then() for timing and never touches the resolved value, but its parameter was typed as Promise<void>. TS's control-flow narrowing widens an assignment of Promise<void> to a Promise<number | void> variable back to Promise<number | void> (the union member it's assignable to, not the literal assigned type), so passing that variable at the call site failed to typecheck. Widen the parameter to Promise<unknown>, which matches what the function actually needs, and drop the now-unnecessary `as Promise<void>` cast on the commit() call. Fixes the "Next.js adapter integration" CI failures on PR #1898 (all Node versions), which build harper fresh via `npm run build` and hit this compile error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Reply to the Good catch, and confirmed the mechanism exactly: Fixed in 9ba73f1 with your first option: Thread marked resolved. — Claude (Opus 4.8) |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reply to the claude-bot finding on Investigated by instrumenting the live code path rather than trusting either the finding or my own earlier smoke test at face value, since they seemed to disagree. Booted a scratch instance from this branch, deployed a payload, and logged
No code change from this thread. — Claude (Opus 4.8) |
…ions Both operations have been documented (and enum-declared) since 5.1 but never registered, so they returned "Operation not found" (#1893). - get_deployment_payload streams the stored tarball as raw bytes (never base64: V8's ~512MiB string cap makes large payloads hard-fail) via the existing Readable+headers pipe path, marked preCompressed since the payload is already a gzipped tar. - delete_deployment_payload nulls payload_blob on terminal-status rows, retaining row metadata/event_log as audit; the replicated null unlinks the blob file locally and on every peer (the #1495 reclaim mechanism). Idempotent; 409 on non-terminal rows whose blob may still be the replication source. - serverHandlers gzip pipe now forwards source-stream errors; an async blob read error previously fired 'error' with no listener and killed the whole worker via uncaughtException (also latent for get_backup). - MCP operations adapter fails cleanly (isError) on streaming results instead of JSON.stringify-ing stream internals, and destroys the stream; delete_deployment_payload added to DESTRUCTIVE_OPERATIONS. Fixes #1893 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Client disconnect destroys only the stream Fastify was handed (the gzip transform); without the reverse hook the source file/blob read stays open. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Kris caught in review: destroying an already-returned stream isn't sufficient cleanup for get_backup's LMDB path — it opens an fd and read transaction, then returns Readable.from(asyncGenerator). Per spec, calling .return() (which .destroy() triggers) on a generator that was never iterated resolves it to done and skips its body, including the readTxn.done() cleanup at the tail — so the fd/transaction leaked on every rejected MCP call. STREAMING_OPERATIONS (get_backup, get_deployment_payload) now short-circuits before chooseOperation/processLocalTransaction ever run, so those resources are never opened. The prior data instanceof Readable check remains as a defense-in-depth backstop for an operation outside that set unexpectedly returning a stream, with its own message so the two paths are distinguishable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
9ba73f1 to
0afdc1f
Compare
cb1kenobi (review on #1898, responding to the PR's own flagged open question): get_deployment_payload streams the full tarball, which can embed secrets — unlike get_deployment/list_deployments, which only ever return stripped metadata. Registering it with get_deployment's permission shape lets an SU delegate it to a non-SU role via the operations allowlist (gate-2), handing that role full source+secret read. That's exactly the property that makes the secrets-store ops self-enforce super_user in the handler so they can't be gate-2 delegated. get_deployment_payload now does the same (mirroring secretOperations' requireSuperUser). delete_deployment_payload is intentionally left as-is — cb1kenobi's recommendation was scoped to the payload-read op; the #1893 non-SU cleanup-automation use case only needs delete, which doesn't expose source/secrets. Verified live: a non-SU role granted get_deployment_payload via add_role({operations:[...]}) now gets 403; the same role's delete_deployment_payload call still succeeds; an SU caller still gets a byte-identical download. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Reply to cb1kenobi's finding on Agreed, and this was exactly the open question I flagged in the PR description ("Happy to tighten if reviewers disagree"). Fixed in fba0fcb: Verified live against a real non-SU role granted both ops via Thanks for catching this — nice catch on the asymmetry with |
…contracts (#600) * Document get_deployment_payload / delete_deployment_payload response contracts Companion to HarperFast/harper#1898 (which implements the two operations — they were documented but returned "Operation not found", harper#1893): raw-bytes response for get_deployment_payload, and the terminal-status requirement, idempotency, response shape, and audit event for delete_deployment_payload. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: note get_deployment_payload's stricter super_user enforcement get_deployment_payload checks requireSuperUser() directly in the handler, unlike delete_deployment_payload and nearly every other super_user op, which can be granted to a non-super_user role via that role's operations allowlist. Document the asymmetry so admins don't hit an unexplained 403. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…oyment payload operations (#2431) * Promote two QA anchors: TLS cert-table swap reachability and the deployment payload ops Adds two integration suites, no product change. integrationTests/deploy/qa701-deployment-payload-ops.test.ts pins the two operations added by #1898, which shipped with unit coverage only. It asserts that delete_deployment_payload performs a real on-disk unlink with freed_bytes exactly equal to payload_size at ~4 KB and again at 12 MB (rather than the metadata-only drop seen on drop_attribute/drop_table), and that the authorization asymmetry is the contract: a non-super_user role explicitly granted both operations can delete but is still 403 on get, because that handler self-enforces super_user over and above the registered permission. Boundaries covered alongside: unknown id 404 with no download headers on the error path, 409 on a non-terminal deployment, 404 after delete, idempotent second delete, a running component's route surviving its payload delete, and redeploy independence. integrationTests/security/qa877-tls-cert-swap.test.ts pins the narrowing behind #2004: an ordinary create_attribute reaches every worker's resetDatabases() yet does not swap the hdb_certificate table object, so a cert rotation immediately afterwards still propagates to every worker with zero stale certificates across 30 fresh handshakes, at threads=1 and threads=4. If that leg ever goes red, #2004 has become reachable from a far more common trigger than currently understood. Co-Authored-By: Claude Opus <noreply@anthropic.com> * Address pre-push review: unique payload sizes, a real 409 window, and worker-count/convergence guards Codex and Cursor/Grok independently flagged the deployment suite's disk oracle: it identifies the blob under test by size within a 64-byte tolerance, and every fixture was built at 4 KB, so an unrelated surviving blob could read as a leak of the one just deleted. Each fixture now has a distinct size and the pre-delete precondition requires exactly one match, so a future collision fails loudly on the precondition instead of false-failing as DEFECT-LEAK. The 409 probe never reached the guard: deploy_component only responds once the row is terminal, so deleting after awaiting the deploy always hit a settled row and the assertion was skipped on every run. The deploy now runs unawaited and the in-flight row is caught through list_deployments, which makes the 409 deterministic; the refused delete is then confirmed to have left payload_blob in place once the deployment settles. For the TLS suite: the threads=4 arm never verified four workers actually started, so it could pass vacuously against a single survivor — it now carries cert-reload.test.ts's system_information guard. Both rotation legs waited for the FIRST worker to serve the new serial and then immediately required every worker to be current, which can false-fail on a loaded runner; they now poll the whole fan-out for convergence. create_attribute's result is read back through describe_table rather than checked for truthiness. Also from the review: an error listener and request teardown on the multipart helper, the real request path instead of a hardcoded '/', a clear error when the poll helper never ran, and trimmed comment narration. Co-Authored-By: Claude Opus <noreply@anthropic.com> * Round 2: outlast the updateTLS debounce, and stop three silent-skip paths The schema-change leg asserts a non-event (the served cert does not change) but waited exactly 1500ms — the same interval each worker debounces its updateTLS() rebuild with. A swap landing just after the probe would have read as "unchanged", so the leg could have passed on the behavior it exists to catch. The wait is now a named constant clearly past the debounce. Three paths could skip work silently: the 409 window poll ended on a transient list_deployments failure instead of retrying to its deadline; both readiness polls treated a 5xx as ready, turning a broken fixture into a cryptic downstream timeout; and the cert directory was removed after teardownHarper rather than in a finally, so a teardown throw leaked it. Also destroys the upload body when the request errors, matching the reverse direction already there. Co-Authored-By: Claude Opus <noreply@anthropic.com> * Round 3: make the 409 assertion unconditional, and fix a busy-loop and a mislabeled check The 409 probe still depended on winning a race against local I/O, so on a fast host it skipped its assertion and a regression in the guard could have merged green. deploy_component writes the deployment row before it ingests the payload stream, so the upload is now paced in chunks: the row is held non-terminal for a known ~2s regardless of host speed, and the assertion runs unconditionally. Verified over three consecutive runs. The fixture shrinks from 6 MB to 256 KB in the process, since pacing rather than payload size now supplies the window. The round-2 fix that stopped the poll bailing out on a transient error left it spinning without a backoff; it now sleeps between polls. The super_user sanity check at the end of the delegation probe claimed to prove a byte-identical download still worked, but called get_deployment (metadata) against a row whose payload had already been deleted two probes earlier. It now downloads the redeployed row's still-present payload, which is what it always claimed to do. The suite's readiness poll fell through silently when its deadline expired, so a never-routed instance would have surfaced as confusing downstream failures instead of a setup error. Comment volume was raised by both reviewers in every round: headers are trimmed to the invariant, the proof boundary, and the cross-references, with narration and reviewer-directed framing removed. Co-Authored-By: Claude Opus <noreply@anthropic.com> * Do not UTF-8 decode a binary operations response callOperationAs decoded every response body as UTF-8 before trying JSON.parse. The current callers read binary downloads through `raw`, so nothing was corrupted, but the helper was a trap for the next test that reached for `body` on a get_deployment_payload success. Octet-stream responses now leave `body` undefined. Co-Authored-By: Claude Opus <noreply@anthropic.com> --------- Co-authored-by: Claude Opus <noreply@anthropic.com>
2 participants
Both operations have been documented (operations-api reference + 5.1 release notes) and enum-declared since 5.1, but were never registered — calling them returned
Operation 'delete_deployment_payload' not found(#1893, reported from a Fabric quota-cleanup workflow).What this adds
get_deployment_payload(components/deploymentOperations.ts) — streams the stored deployment tarball back as raw bytes withcontent-type: application/octet-stream+ acontent-dispositionfilename, via the existingReadable+headers pipe path inserverHandlers.js(theget_backupmechanism). Deliberately never base64: payloads can be hundreds of MB, and a base64 round-trip materializes multi-hundred-MB V8 strings (hardERR_STRING_TOO_LONGnear ~400MB). The stream is markedpreCompressedso the already-gzipped tar isn't re-gzipped.delete_deployment_payload— nullspayload_blobon the row and re-puts it; the RecordEncoder retained-blob check unlinks the local blob file and the replicated null makes every peer drop its copy too (the same cluster-wide reclaim mechanism as the automatic post-deploy retention drop from feat(deploy): auto-drop large deployment payload blobs after a successful deploy #1496). Row metadata +event_logare retained as the audit trail, with apayload_droppedevent recordingdeleted_by. Guards: 409 on non-terminal deployments (their blob may still be the replication channel peers install from); idempotent success (freed_bytes: 0) when the payload is already gone; 404 for unknown ids.get_deployment's super_user permission shape.Hardening found by review (in this PR)
serverHandlers.jsgzip pipe now forwards source-stream errors..pipe()doesn't propagate them, so an async read error on a returned stream (e.g.get_backup's file stream) fired'error'with no listener →uncaughtException→ whole worker exits. Latent before this PR; blob streams made it much more reachable. Now the error destroys the gzip stream and Fastify aborts just that response.isErrorfor streaming (Readable) results instead ofJSON.stringify-ing stream internals, and destroys the stream so file-backed sources release fds.delete_deployment_payloadadded toDESTRUCTIVE_OPERATIONS. (Neither payload op is in the MCP default-allow surface; this covers explicitmcp.operations.allowopt-ins, andget_backupas well.)Considered decisions (flagging for review)
get_deployment's permission shape, meaning an SU can grant them to a role via itsoperationsallowlist (gate-2). Considered self-enforcing super_user like the secrets ops (tarballs can embed secrets), but delegation requires a deliberate per-op SU grant, and a non-SU cleanup-automation principal is exactly the delete_deployment_payload operation not implemented #1893 use case. Happy to tighten if reviewers disagree.Verification
get_deployment_payloadbyte-identical round-trip (sha256 match) with and withoutAccept-Encoding: gzip;delete_deployment_payload→freed_bytescorrect, blob file physically unlinked,payload_blob_present:false,payload_droppedaudit event, idempotent second call, 404s on reclaimed/unknown.Cross-model coverage: codex ✓ / gemini ✓ / Harper-domain pass ✓ (thorough), + final-artifact pass after the review fixes. All blockers/concerns from review addressed above.
Docs: companion PR documentation#600 adds the delete response shape, terminal-status requirement, and raw-bytes (not base64) clarification.
Fixes #1893
🤖 Generated with Claude Code (Opus 4.8)