Skip to content

Commit db5ad13

Browse files
fix(server): report what a timed-out stream limit change actually did
Bounding a stream that has outgrown its new limit sheds the excess before the broker replies, so the call takes as long as the deletion. Startup gave up first and logged that the stream was "left at its live configuration" while the broker went on to apply the change — telling the operator the opposite of what had happened. Startup now waits on a handle of its own, separate from the one the health indicator and monitor share, because the two want opposite timeouts: a probe has to fail fast to be worth alerting on, and an admin call has to outlast the bytes it is deleting. When it still cannot get an answer it re-reads the stream and reports the limits actually in force rather than assuming its own update failed. Found by deploying #1501 to staging, where a 32.3 GB GitHub stream took longer to shed than the client would wait. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LK2yxSEfNL5Q5xwbAqAMWr
1 parent 86aeefa commit db5ad13

14 files changed

Lines changed: 173 additions & 4 deletions

File tree

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
"hephaestus": patch
3+
---
4+
5+
A webhook stream limit change that takes longer than the broker's reply now says what actually happened. Bounding a stream that has outgrown its new limit deletes the excess before the broker answers, so on a large stream the wait is proportional to the data being shed — long enough that startup gave up and reported the stream "left at its live configuration" while the broker went on to apply the change. An operator reading that was told the opposite of the truth: that nothing had happened, when tens of gigabytes had just been deleted. Startup now waits long enough for the change to land, and if it still cannot get an answer it reports the limits the stream actually has rather than assuming its own update failed.
6+
7+
**Operators:** the new `HEPHAESTUS_WEBHOOK_STREAM_LIMIT_UPDATE_TIMEOUT` defaults to `5m` and applies only at startup. Raise it if a stream is large enough that shedding the excess takes longer than that; it is deliberately separate from the health-check timeouts, which must stay short to be worth alerting on.

docker/.env.example

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,9 @@ HEPHAESTUS_WEBHOOK_STREAM_MAX_BYTES_GITHUB=10GB
128128
# deliberate start-up to apply it — for example when first bounding a stream that has already grown
129129
# past the new limit.
130130
HEPHAESTUS_WEBHOOK_STREAM_ALLOW_DESTRUCTIVE_LIMIT_UPDATES=false
131+
# How long one stream limit change may take. A bound that sheds messages deletes them before the
132+
# broker answers, so raise this on a stream too large to shed inside five minutes.
133+
HEPHAESTUS_WEBHOOK_STREAM_LIMIT_UPDATE_TIMEOUT=5m
131134

132135
# -----------------------------------------------------------------------------
133136
# MONITORING & SYNC

docker/compose.core.yaml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,9 @@ services:
4242
# Lowering a bound on a stream that has already outgrown it deletes the excess. Set true for
4343
# one deliberate start-up to apply it; startup otherwise logs what it would cost and declines.
4444
HEPHAESTUS_WEBHOOK_STREAM_ALLOW_DESTRUCTIVE_LIMIT_UPDATES: ${HEPHAESTUS_WEBHOOK_STREAM_ALLOW_DESTRUCTIVE_LIMIT_UPDATES:-false}
45+
# Applying that bound sheds the excess before the broker replies, so the wait scales with the
46+
# bytes deleted. Raise it on a stream large enough that 5 minutes is not enough.
47+
HEPHAESTUS_WEBHOOK_STREAM_LIMIT_UPDATE_TIMEOUT: ${HEPHAESTUS_WEBHOOK_STREAM_LIMIT_UPDATE_TIMEOUT:-5m}
4548
# Shared webhook secret used for incoming HMAC / token verification.
4649
# WEBHOOK_EXTERNAL_URL is only used by auto-registration (server role) and is intentionally
4750
# not set on the webhook-server pod.

docs/admin/webhook-ingestion-operations.mdx

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,19 @@ To apply it deliberately:
124124
Leaving that flag set turns a one-time decision into standing permission for any future deploy to
125125
delete stored messages.
126126

127+
Shedding the excess happens **before** the broker replies, so on a large stream the call takes as
128+
long as the deletion does. `HEPHAESTUS_WEBHOOK_STREAM_LIMIT_UPDATE_TIMEOUT` (default `5m`) is how
129+
long startup waits; raise it if your stream is too large to shed inside that. If it still runs out,
130+
startup reports the limits the stream actually has rather than assuming its own update failed —
131+
which matters, because a timed-out update is one the broker may well have applied:
132+
133+
```text
134+
Failed to reconcile JetStream stream limits: name=github changed=[maxBytes -1 -> 10737418240]
135+
— live configuration is now maxAge=PT4320H maxBytes=10737418240 maxMessages=-1
136+
```
137+
138+
That line says the change **did** land. Confirm with `nats stream info github` before retrying.
139+
127140
If instead you see a shape-drift error, the stream is not what this deployment expects and no bound
128141
is written onto it at all:
129142

@@ -213,6 +226,29 @@ retention, so a deployment offline past the stream's byte or age bound comes bac
213226
at messages the stream no longer holds — `webhook.stream.unacknowledged.deletions` will say so on the
214227
first poll.
215228

229+
## Making a change actually reach the broker
230+
231+
Two things in the deployment path will silently leave your change unapplied.
232+
233+
**The webhook receiver and NATS are in the core stack, and staging CD deploys the app only.** A
234+
change to `hephaestus.webhook.stream.*` ships with the application server but does not reach the
235+
container that bootstraps the streams until core is deployed. CI warns when `docker/compose.core.yaml`
236+
changes; deploy it with **Deploy to Staging** and `deploy-core: true`.
237+
238+
**Compose does not recreate NATS when only its config changes.** The broker's configuration is a
239+
Compose `configs:` block whose content renders to a bind-mounted file. Changing the content does not
240+
change the container's spec, so `docker compose up -d` reports no drift and the running broker keeps
241+
the config it started with — `max_payload` and `max_file` included. Force it:
242+
243+
```bash
244+
docker compose -f compose.core.yaml up -d --force-recreate nats-server
245+
docker exec core-nats-server-1 cat /etc/nats/nats-server.conf # confirm the value you set
246+
```
247+
248+
Recreating the broker is safe for stored messages — JetStream data lives in the volume — but
249+
in-flight deliveries during the restart are lost, and webhooks are not redeliverable. Reconciliation
250+
covers `MONITORING_TIMEFRAME` days afterwards.
251+
216252
## Where these settings live
217253

218254
Stream bounds are read by beans that only exist on the **webhook** runtime role. Setting them on the

scripts/check-env-roles.test.mjs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,9 +166,12 @@ test("a compose file that yields no services is a failure, not a pass", () => {
166166
});
167167

168168
test("compose defaults resolve the way an operator who sets nothing gets them", () => {
169+
// biome-ignore lint/suspicious/noTemplateCurlyInString: Compose interpolation syntax under test, not a template literal
169170
assert.equal(composeDefault("${HEPHAESTUS_RUNTIME_WEBHOOK_ENABLED:-false}"), "false");
171+
// biome-ignore lint/suspicious/noTemplateCurlyInString: Compose interpolation syntax under test, not a template literal
170172
assert.equal(composeDefault("${HEPHAESTUS_RUNTIME_WEBHOOK_ENABLED-false}"), "false");
171173
assert.equal(composeDefault('"false"'), "false");
174+
// biome-ignore lint/suspicious/noTemplateCurlyInString: Compose interpolation syntax under test, not a template literal
172175
assert.equal(composeDefault("${WEBHOOK_SECRET}"), "");
173176
});
174177

server/src/main/java/de/tum/cit/aet/hephaestus/core/webhook/WebhookProperties.java

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,14 @@ public record Stream(
162162
* immediately, so it is a decision an operator makes rather than one a deploy makes for them.
163163
*/
164164
@DefaultValue("false") boolean allowDestructiveLimitUpdates,
165+
/**
166+
* How long startup waits for one stream limit update. Bounding a stream that has outgrown
167+
* the new limit deletes the excess before the broker answers, so the work is proportional to
168+
* the bytes being shed rather than to the size of the request — tens of GB take far longer
169+
* than the request timeout the health probes want. This is deliberately separate from the
170+
* consumer request timeout so a slow admin call cannot slow a readiness answer.
171+
*/
172+
@DefaultValue("5m") Duration limitUpdateTimeout,
165173
/** How often the stream monitor reads stream and consumer state. */
166174
@DefaultValue("60s") Duration monitorInterval
167175
) {
@@ -216,6 +224,11 @@ public record Stream(
216224
requirePositive("stream.maxBytesByStream." + e.getKey(), e.getValue());
217225
}
218226
requirePositive("stream.storageBudget", storageBudget);
227+
if (limitUpdateTimeout.isZero() || limitUpdateTimeout.isNegative()) {
228+
throw new IllegalArgumentException(
229+
"stream.limitUpdateTimeout must be positive, got: " + limitUpdateTimeout
230+
);
231+
}
219232
if (monitorInterval.isZero() || monitorInterval.isNegative()) {
220233
throw new IllegalArgumentException("stream.monitorInterval must be positive, got: " + monitorInterval);
221234
}

server/src/main/java/de/tum/cit/aet/hephaestus/integration/core/webhook/WebhookJetStreamBootstrap.java

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -248,17 +248,39 @@ private boolean applyLimits(String name, LimitPlan plan) {
248248
}
249249
return true;
250250
} catch (JetStreamApiException | IOException ex) {
251-
// The stream exists and publishes still land at its live limits, so this is not worth
252-
// failing a deploy over — the receiver stays up and the operator gets a loud record.
251+
// A timeout here says the broker did not answer in time, not that it did nothing: shedding
252+
// the excess a new bound deletes happens before the reply, so the update can land while the
253+
// client gives up. Report the limits the stream actually has rather than assert an outcome.
253254
log.error(
254-
"Failed to reconcile JetStream stream limits: name={} — stream left at its live configuration",
255+
"Failed to reconcile JetStream stream limits: name={} changed={} — live configuration is now {}",
255256
name,
257+
plan.applied,
258+
describeLiveLimits(name),
256259
ex
257260
);
258261
return false;
259262
}
260263
}
261264

265+
/**
266+
* The stream's limits as the broker reports them right now, for a failure that cannot say whether
267+
* its own update landed. Never throws: it runs on a path that is already handling a failure, and
268+
* an unreadable broker is itself the answer.
269+
*/
270+
private String describeLiveLimits(String name) {
271+
try {
272+
StreamConfiguration live = jsm.getStreamInfo(name).getConfiguration();
273+
return String.format(
274+
"maxAge=%s maxBytes=%d maxMessages=%d",
275+
live.getMaxAge(),
276+
live.getMaxBytes(),
277+
live.getMaxMsgs()
278+
);
279+
} catch (JetStreamApiException | IOException | RuntimeException ex) {
280+
return "unreadable (" + ex.getClass().getSimpleName() + ") — check the broker directly";
281+
}
282+
}
283+
262284
/**
263285
* Fields that define what the stream <em>is</em>. Changing any of them reshapes or re-homes the
264286
* data, so they are reported and never written — {@code nats stream edit} with a human deciding

server/src/main/java/de/tum/cit/aet/hephaestus/integration/core/webhook/WebhookProducerBeans.java

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,11 @@
99
import io.nats.client.Connection;
1010
import io.nats.client.JetStream;
1111
import io.nats.client.JetStreamManagement;
12+
import io.nats.client.JetStreamOptions;
1213
import java.io.IOException;
1314
import org.springframework.beans.factory.annotation.Qualifier;
1415
import org.springframework.context.annotation.Bean;
16+
import org.springframework.context.annotation.Primary;
1517

1618
/**
1719
* The NATS <em>producer</em> cluster contributed by {@link WebhookConfiguration} on the
@@ -34,11 +36,30 @@ JetStream webhookJetStream(@Qualifier("natsConnection") Connection natsConnectio
3436
}
3537

3638
@Bean
39+
@Primary
3740
JetStreamManagement webhookJetStreamManagement(@Qualifier("natsConnection") Connection natsConnection)
3841
throws IOException {
3942
return natsConnection.jetStreamManagement();
4043
}
4144

45+
/**
46+
* The handle {@link WebhookJetStreamBootstrap} applies limit changes through. Separate from
47+
* {@link #webhookJetStreamManagement} because the two want opposite timeouts: the health
48+
* indicator and the monitor share that one and need it to fail fast, while bounding a stream
49+
* that has outgrown its new limit sheds the excess before the broker answers and takes as long
50+
* as the bytes require. Sharing one handle means either readiness waits on a delete or the
51+
* delete reports a failure the broker is still completing.
52+
*/
53+
@Bean
54+
JetStreamManagement webhookAdminJetStreamManagement(
55+
@Qualifier("natsConnection") Connection natsConnection,
56+
WebhookProperties properties
57+
) throws IOException {
58+
return natsConnection.jetStreamManagement(
59+
JetStreamOptions.builder().requestTimeout(properties.stream().limitUpdateTimeout()).build()
60+
);
61+
}
62+
4263
@Bean
4364
Retry webhookPublishRetry(WebhookProperties properties) {
4465
WebhookProperties.Publish p = properties.publish();
@@ -63,7 +84,10 @@ JetStreamPublisher jetStreamPublisher(
6384
}
6485

6586
@Bean
66-
WebhookJetStreamBootstrap webhookJetStreamBootstrap(JetStreamManagement jsm, WebhookProperties properties) {
87+
WebhookJetStreamBootstrap webhookJetStreamBootstrap(
88+
@Qualifier("webhookAdminJetStreamManagement") JetStreamManagement jsm,
89+
WebhookProperties properties
90+
) {
6791
return new WebhookJetStreamBootstrap(jsm, properties);
6892
}
6993

server/src/main/resources/application.yml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -612,6 +612,10 @@ hephaestus:
612612
# messages the stream already holds is withheld and logged rather than applied. Set this
613613
# once, deliberately, to let that through.
614614
allow-destructive-limit-updates: ${HEPHAESTUS_WEBHOOK_STREAM_ALLOW_DESTRUCTIVE_LIMIT_UPDATES:false}
615+
# A bound that sheds messages deletes them before the broker replies, so the wait scales
616+
# with the bytes shed, not with the request. Startup-only, and deliberately not the
617+
# timeout the health probes use — those have to fail fast to be worth alerting on.
618+
limit-update-timeout: ${HEPHAESTUS_WEBHOOK_STREAM_LIMIT_UPDATE_TIMEOUT:5m}
615619
monitor-interval: 60s
616620
shutdown:
617621
# Drain budget for in-flight publishes after HTTP closes. Docker stop_grace_period

server/src/test/java/de/tum/cit/aet/hephaestus/core/webhook/WebhookPropertiesFixture.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ public static WebhookProperties.Stream stream() {
2525
Map.of(),
2626
gibibytes(12),
2727
false,
28+
Duration.ofMinutes(5),
2829
Duration.ofSeconds(60)
2930
);
3031
}

0 commit comments

Comments
 (0)