Skip to content

Commit bacdd54

Browse files
kriszypclaude
andcommitted
fix(deploy): address cross-model review findings for Slice A
- Skip recording on replicated executions: peer nodes receiving deploy_component via replicateOperation already have req._deploymentId set by origin, so they no longer spin up a fresh recorder + UUID + row. Prevents N duplicate rows per N-node cluster. - Cap payload at 200 MiB while Slice A buffers in memory. Throws a clear ClientError pointing users at the package-identifier path or Slice B's streaming variant. - Register list_deployments and get_deployment in utility/operation_authorization.ts. Pattern matches get_components: requires_su=true with the operation enum as the named exception so a role can be granted it without SU rights (per the design's permission model). - Add "audit": true to hdb_deployment in systemSchema.json so fresh installs match the audit setting the 5-2-0 upgrade directive applies. - Drop two now-unused imports (Transform from recorder, existsSync from test). - Auto-format pass via npm run format:write. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 136a10e commit bacdd54

6 files changed

Lines changed: 94 additions & 29 deletions

File tree

components/deploymentOperations.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,15 +47,18 @@ export async function handleListDeployments(req: ListRequest = {}): Promise<{ de
4747
const conditions: any[] = [];
4848
if (req.project) conditions.push({ attribute: 'project', value: req.project });
4949
if (req.status) conditions.push({ attribute: 'status', value: req.status });
50-
if (req.since != null) conditions.push({ attribute: 'started_at', value: req.since, comparator: 'greater_than_equal' });
50+
if (req.since != null)
51+
conditions.push({ attribute: 'started_at', value: req.since, comparator: 'greater_than_equal' });
5152
if (req.until != null) conditions.push({ attribute: 'started_at', value: req.until, comparator: 'less_than_equal' });
5253

5354
const collected: any[] = [];
5455
for await (const row of table.search(conditions)) {
5556
collected.push(stripBlob(row));
5657
}
5758
// Newest first by started_at; ties broken by deployment_id for stability.
58-
collected.sort((a, b) => (b.started_at ?? 0) - (a.started_at ?? 0) || String(a.deployment_id).localeCompare(b.deployment_id));
59+
collected.sort(
60+
(a, b) => (b.started_at ?? 0) - (a.started_at ?? 0) || String(a.deployment_id).localeCompare(b.deployment_id)
61+
);
5962

6063
const total = collected.length;
6164
const offset = Math.max(0, req.offset ?? 0);

components/deploymentRecorder.ts

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,29 @@
1010

1111
import { randomUUID } from 'node:crypto';
1212
import { createHash, Hash } from 'node:crypto';
13-
import { Transform } from 'node:stream';
1413
import type { Readable } from 'node:stream';
1514
import { databases } from '../resources/databases.ts';
1615
import { createBlob } from '../resources/blob.ts';
1716
import * as terms from '../utility/hdbTerms.ts';
17+
import { ClientError } from '../utility/errors/hdbError.ts';
1818
import { hostname } from 'node:os';
1919

20-
type DeploymentStatus = 'pending' | 'extracting' | 'installing' | 'loading' | 'replicating' | 'restarting' | 'success' | 'failed' | 'rolled_back';
20+
// Slice A buffers the entire payload in memory before computing the hash and persisting.
21+
// This cap prevents an OOM on accidentally-huge uploads while Slice B is in flight. Slice B
22+
// replaces the buffer with a streaming hash + Blob-source pattern that lifts this limit
23+
// back to whatever the replication path supports.
24+
const SLICE_A_PAYLOAD_LIMIT_BYTES = 200 * 1024 * 1024;
25+
26+
type DeploymentStatus =
27+
| 'pending'
28+
| 'extracting'
29+
| 'installing'
30+
| 'loading'
31+
| 'replicating'
32+
| 'restarting'
33+
| 'success'
34+
| 'failed'
35+
| 'rolled_back';
2136

2237
interface CreateOptions {
2338
project?: string;
@@ -88,11 +103,27 @@ export class DeploymentRecorder {
88103
buffer = Buffer.from(source, 'base64');
89104
} else {
90105
const chunks: Buffer[] = [];
106+
let collected = 0;
91107
for await (const chunk of source as AsyncIterable<Buffer | string>) {
92-
chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk as any));
108+
const buf = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk as any);
109+
collected += buf.length;
110+
if (collected > SLICE_A_PAYLOAD_LIMIT_BYTES) {
111+
(source as Readable).destroy?.();
112+
throw new ClientError(
113+
`Deploy payload exceeds Slice A's interim ${SLICE_A_PAYLOAD_LIMIT_BYTES} byte cap. ` +
114+
`Use a package identifier (npm:/file:/git:) or wait for Slice B's streaming path.`
115+
);
116+
}
117+
chunks.push(buf);
93118
}
94119
buffer = Buffer.concat(chunks);
95120
}
121+
if (buffer.length > SLICE_A_PAYLOAD_LIMIT_BYTES) {
122+
throw new ClientError(
123+
`Deploy payload (${buffer.length} bytes) exceeds Slice A's interim ${SLICE_A_PAYLOAD_LIMIT_BYTES} byte cap. ` +
124+
`Use a package identifier (npm:/file:/git:) or wait for Slice B's streaming path.`
125+
);
126+
}
96127
hash.update(buffer);
97128
byteCount = buffer.length;
98129
this.record.payload_blob = createBlob(buffer, { type: 'application/gzip' });

components/operations.js

Lines changed: 25 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -390,21 +390,29 @@ async function deployComponent(req) {
390390
// observable and auditable even if the CLI disconnects. The row also holds the payload
391391
// in a Blob attribute — Slice B will use that for peer delivery; for now it's the
392392
// audit record and the rollback source.
393-
const recorder = await DeploymentRecorder.create({
394-
project: req.project,
395-
package_identifier: req.package ?? null,
396-
user: req.hdb_user?.username,
397-
restart_mode: req.restart === 'rolling' ? 'rolling' : req.restart ? 'immediate' : null,
398-
});
399-
req._deploymentId = recorder.deploymentId;
393+
//
394+
// Only the origin node records — peers receiving a replicated deploy_component skip
395+
// recording so we don't accumulate one row per node for the same deploy. The row will
396+
// reach peers via the table's replication once Slice B has them consume it.
397+
const isReplicatedExecution = typeof req._deploymentId === 'string';
398+
const recorder = isReplicatedExecution
399+
? null
400+
: await DeploymentRecorder.create({
401+
project: req.project,
402+
package_identifier: req.package ?? null,
403+
user: req.hdb_user?.username,
404+
restart_mode: req.restart === 'rolling' ? 'rolling' : req.restart ? 'immediate' : null,
405+
});
406+
if (recorder) req._deploymentId = recorder.deploymentId;
400407

401408
let extractionPayload = req.payload;
402409
try {
403-
// If a tarball came in (Buffer or Readable from the multipart parser), tee it through
404-
// a hash-and-size tap into the row's payload_blob, then re-source extraction from the
405-
// persisted blob. This means we read the upload exactly once into local storage; the
406-
// blob is the staging area and (in Slice B) the channel peers will replicate from.
407-
if (req.payload != null) {
410+
// On the origin, tee the tarball (Buffer or Readable from the multipart parser)
411+
// through a hash-and-size tap into the row's payload_blob, then re-source extraction
412+
// from the persisted blob. This is the staging area and (in Slice B) the channel
413+
// peers will replicate from. On peer nodes we skip recording entirely and use the
414+
// raw payload as-is.
415+
if (recorder && req.payload != null) {
408416
await recorder.ingestPayload(req.payload);
409417
extractionPayload = recorder.row.payload_blob.stream();
410418
}
@@ -462,11 +470,13 @@ async function deployComponent(req) {
462470
response.message = `Successfully deployed: ${application.name}, restarting Harper`;
463471
} else response.message = `Successfully deployed: ${application.name}`;
464472

465-
response.deployment_id = recorder.deploymentId;
466-
await recorder.finish('success');
473+
if (recorder) {
474+
response.deployment_id = recorder.deploymentId;
475+
await recorder.finish('success');
476+
}
467477
return response;
468478
} catch (err) {
469-
await recorder.finish('failed', err);
479+
if (recorder) await recorder.finish('failed', err);
470480
throw err;
471481
}
472482
}

integrationTests/deploy/deploy-tracking.test.ts

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
import { suite, test, before, after } from 'node:test';
1010
import { ok, strictEqual } from 'node:assert/strict';
1111
import { join } from 'node:path';
12-
import { existsSync, mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs';
12+
import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs';
1313
import { tmpdir } from 'node:os';
1414
import { setTimeout as sleep } from 'node:timers/promises';
1515
import { request } from 'node:http';
@@ -37,7 +37,7 @@ function postMultipart(
3737
headers: {
3838
'Content-Type': contentType,
3939
'Transfer-Encoding': 'chunked',
40-
Authorization: 'Basic ' + Buffer.from(`${auth.username}:${auth.password}`).toString('base64'),
40+
'Authorization': 'Basic ' + Buffer.from(`${auth.username}:${auth.password}`).toString('base64'),
4141
},
4242
},
4343
(res) => {
@@ -58,11 +58,10 @@ async function callOperation(
5858
op: Record<string, unknown>
5959
): Promise<{ status: number; body: any }> {
6060
const url = new URL(ctx.harper.operationsAPIURL);
61-
const auth =
62-
'Basic ' + Buffer.from(`${ctx.harper.admin.username}:${ctx.harper.admin.password}`).toString('base64');
61+
const auth = 'Basic ' + Buffer.from(`${ctx.harper.admin.username}:${ctx.harper.admin.password}`).toString('base64');
6362
const res = await fetch(url, {
6463
method: 'POST',
65-
headers: { 'Content-Type': 'application/json', Authorization: auth },
64+
headers: { 'Content-Type': 'application/json', 'Authorization': auth },
6665
body: JSON.stringify(op),
6766
});
6867
const text = await res.text();
@@ -133,10 +132,16 @@ suite('Deployment tracking', (ctx: ContextWithHarper) => {
133132
strictEqual(row.project, project);
134133
strictEqual(row.status, 'success');
135134
strictEqual(row.payload_blob_present, true, 'payload_blob should have been persisted');
136-
ok(typeof row.payload_hash === 'string' && /^[0-9a-f]{64}$/i.test(row.payload_hash), 'payload_hash should be a sha256 hex string');
135+
ok(
136+
typeof row.payload_hash === 'string' && /^[0-9a-f]{64}$/i.test(row.payload_hash),
137+
'payload_hash should be a sha256 hex string'
138+
);
137139
ok(typeof row.payload_size === 'number' && row.payload_size > 0, 'payload_size should be a positive integer');
138140
ok(typeof row.started_at === 'number' && row.started_at > 0, 'started_at should be set');
139-
ok(typeof row.completed_at === 'number' && row.completed_at >= row.started_at, 'completed_at should be >= started_at');
141+
ok(
142+
typeof row.completed_at === 'number' && row.completed_at >= row.started_at,
143+
'completed_at should be >= started_at'
144+
);
140145
});
141146

142147
test('list_deployments surfaces the row, supports project filter', async () => {
@@ -148,7 +153,10 @@ suite('Deployment tracking', (ctx: ContextWithHarper) => {
148153
ok(deploymentId && ids.includes(deploymentId), `listed deployments should include ${deploymentId}`);
149154
// blob bytes must NOT travel back in the list response — only the presence boolean.
150155
ok(!('payload_blob' in listed.body.deployments[0]), 'list_deployments must not include payload_blob bytes');
151-
ok('payload_blob_present' in listed.body.deployments[0], 'list_deployments should include payload_blob_present flag');
156+
ok(
157+
'payload_blob_present' in listed.body.deployments[0],
158+
'list_deployments should include payload_blob_present flag'
159+
);
152160
});
153161

154162
test('a failed deploy is recorded with status=failed and error.message', async () => {
@@ -190,7 +198,10 @@ suite('Deployment tracking', (ctx: ContextWithHarper) => {
190198
const failed = listed.body.deployments.find((d: any) => d.project === project);
191199
ok(failed, `expected to find a deployment for ${project} in list`);
192200
strictEqual(failed.status, 'failed');
193-
ok(failed.error && typeof failed.error.message === 'string' && failed.error.message.length > 0, 'failed deployment should have error.message');
201+
ok(
202+
failed.error && typeof failed.error.message === 'string' && failed.error.message.length > 0,
203+
'failed deployment should have error.message'
204+
);
194205
} finally {
195206
try {
196207
rmSync(brokenDir, { recursive: true, force: true });

json/systemSchema.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,7 @@
374374
"hash_attribute": "deployment_id",
375375
"name": "hdb_deployment",
376376
"schema": "system",
377+
"audit": true,
377378
"attributes": [
378379
{
379380
"attribute": "deployment_id"

utility/operation_authorization.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ import PermissionResponseObject from '../security/data_objects/PermissionRespons
4040
import { handleHDBError, hdbErrors } from '../utility/errors/hdbError.ts';
4141

4242
import * as regDeprecated from '../resources/registrationDeprecated.ts';
43+
import * as deploymentOperations from '../components/deploymentOperations.ts';
4344

4445
const requiredPermissions = new Map();
4546
const DELETE_PERM = 'delete';
@@ -241,6 +242,14 @@ requiredPermissions.set(functionsOperations.addComponent.name, new (permission a
241242
requiredPermissions.set(functionsOperations.dropCustomFunctionProject.name, new (permission as any)(true, []));
242243
requiredPermissions.set(functionsOperations.packageComponent.name, new (permission as any)(true, []));
243244
requiredPermissions.set(functionsOperations.deployComponent.name, new (permission as any)(true, []));
245+
requiredPermissions.set(
246+
deploymentOperations.handleListDeployments.name,
247+
new (permission as any)(true, [], terms.OPERATIONS_ENUM.LIST_DEPLOYMENTS)
248+
);
249+
requiredPermissions.set(
250+
deploymentOperations.handleGetDeployment.name,
251+
new (permission as any)(true, [], terms.OPERATIONS_ENUM.GET_DEPLOYMENT)
252+
);
244253

245254
//Below are functions that are currently open to all roles
246255
requiredPermissions.set(regDeprecated.getRegistrationInfo.name, new (permission as any)(false, []));

0 commit comments

Comments
 (0)