Bump migration timeout to 4h for hypertable backfills - #1113
Merged
Merged
Conversation
Production deploy run 25609852303 was killed at 3300s applying 0018_migrate_body_measurements_to_metric_stream.sql, a single-transaction hypertable backfill that legitimately needs more than 55min on prod. The migration was still making progress when the workflow's hard cap fired, so the container was force-killed and the transaction rolled back, leaving prod unchanged and unable to roll new code. Sizes the timeout to fit the slowest known migration with headroom and documents that future migrations exceeding the budget should be restructured rather than bumping the timeout again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRaises CI migration timeout 3300→14400s with inline guidance, caps per-slot WAL retention via ChangesProduction Timeout and WAL Guardrail
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
|
Storybook previews for This comment updates automatically on each PR push. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/deploy-web-stack.yml:
- Around line 415-420: The change increases migration_timeout_seconds to 14400
without first capturing the blocking SQL causing prior timeouts; instead revert
this timeout bump and instrument the migration to capture the first blocking
statement before any resilience-knob change: add diagnostics to the migration
job (e.g., query pg_stat_activity/pg_locks or enable log_lock_waits) to record
the exact blocking statement and transaction id when a lock/wait occurs, or
split/move the backfill out of deploy and run it as a separately-invoked
maintenance job; once the offending SQL is identified and fixed or the backfill
is split, only then consider adjusting migration_timeout_seconds if absolutely
needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2dcd84be-3922-4dd9-b186-f04cd5e607e6
📒 Files selected for processing (2)
.github/workflows/deploy-web-stack.ymldocs/production-incident-baseline.md
CodeRabbit flagged that bumping the migration timeout without capturing which SQL statement is actually running could hide lock/contention failures behind a 4h delay. Adds periodic pg_stat_activity dumps to the workflow log every 60s of migration elapsed time, plus a final snapshot when the deploy-level timeout fires. Output includes wait_event_type and wait_event so a stuck-on-lock case is distinguishable from genuine data-volume slowness without waiting for the timeout. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Review app is ready: This environment runs on a dedicated Hetzner server for PR #1113 and updates on each push. |
Captures: rescue+reboot dance moved prod from healthy to the same wedged state staging had been in all session; ~20 swarm services on a cax11 2-core ARM box hit load >100 within minutes of any boot; soft reboots and hard reset don't produce a stable host. Records the unknown swap status, the symptom pattern shared with staging, and the realistic recovery options (resize, trim stack, or both) so the next person has context even if the live host stays unreachable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/production-incident-baseline.md`:
- Around line 91-104: The two new incident entries that currently stop at the
"### Remaining Risk" heading need an added "### Follow-Up Work" section each:
for the 2026-05-10 entry and the 2026-05-09 entry, append a short bulleted
follow-up list that includes an owner (individual or team), a clear next action
(e.g., recover staging, investigate volume health, apply migrations
0018_metric_stream_location_point.sql and
0018_migrate_body_measurements_to_metric_stream.sql or roll-forward/rollback
plan), and a target date (use the dates requested where applicable); place these
directly after the existing "### Remaining Risk" paragraph for each incident so
the template fields (date, symptoms, impact, evidence, root cause, fix,
remaining risk, follow-up work) are complete.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d79b70f9-03a9-4802-a788-bc9de0b0d033
📒 Files selected for processing (1)
docs/production-incident-baseline.md
Comment on lines
+91
to
+104
| ### Remaining Risk | ||
|
|
||
| - We do not yet know whether the post-reboot wedge is a deterministic | ||
| consequence of the stack composition, or whether something specific to | ||
| the data volume (slow IO, near-full, corruption, etc.) is the proximate | ||
| cause. Resizing the host treats the symptom; if the underlying issue is | ||
| the volume, resize alone will not durably help. | ||
| - Staging has been wedged through the entire incident, so the deploy-time | ||
| validation surface is also down. Any fix attempted on prod cannot be | ||
| rehearsed on staging until staging is recovered. | ||
| - The pending `0018_metric_stream_location_point.sql` and | ||
| `0018_migrate_body_measurements_to_metric_stream.sql` migrations are | ||
| still blocked from applying. The deploy queue is effectively frozen. | ||
|
|
There was a problem hiding this comment.
Add explicit ### Follow-Up Work sections for the new incident entries.
Both new entries stop at ### Remaining Risk. The incident template in this repo requires follow-up work to be captured explicitly. Add a short follow-up list (owner + next action + target date where possible) for 2026-05-10 and 2026-05-09.
As per coding guidelines, “After any production incident, deploy failure, infrastructure issue, database pressure event, CI/deploy outage, or operational debugging session, append a concise summary to docs/production-incident-baseline.md. Capture date, symptoms, user impact, evidence, root cause if known, fix or mitigation, remaining risk, and follow-up work.”
Also applies to: 158-169
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/production-incident-baseline.md` around lines 91 - 104, The two new
incident entries that currently stop at the "### Remaining Risk" heading need an
added "### Follow-Up Work" section each: for the 2026-05-10 entry and the
2026-05-09 entry, append a short bulleted follow-up list that includes an owner
(individual or team), a clear next action (e.g., recover staging, investigate
volume health, apply migrations 0018_metric_stream_location_point.sql and
0018_migrate_body_measurements_to_metric_stream.sql or roll-forward/rollback
plan), and a target date (use the dates requested where applicable); place these
directly after the existing "### Remaining Risk" paragraph for each incident so
the template fields (date, symptoms, impact, evidence, root cause, fix,
remaining risk, follow-up work) are complete.
Per CR feedback: the 30 lines of bash + nested heredoc'd psql were rebuilding what pg_stat_statements (already in shared_preload_libraries per stack.yml) and pg_stat_progress_* already give us natively. SQL-level visibility into a slow migration belongs in a query against the live db service, not in the deploy workflow. Keeps the migration_timeout_seconds=14400 bump and the migration container's log streaming, which is the actual primary evidence source. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/production-incident-baseline.md`:
- Around line 87-89: The doc currently gives conflicting statements about PR
`#1113`: it claims `pg_stat_activity` instrumentation from PR `#1113` remains valid
while elsewhere it notes periodic workflow snapshots were reverted; update the
observability statement so there is a single consistent source of truth — either
state that both `migration_timeout_seconds=14400` and the `pg_stat_activity`
instrumentation in PR `#1113` remain valid and can be exercised once the host is
stable, or explicitly note that `pg_stat_activity` instrumentation was impacted
by the revert of periodic workflow snapshots and is not currently usable;
reference `migration_timeout_seconds`, `pg_stat_activity`, PR `#1113` and the
periodic workflow snapshots revert in the updated sentence so operators see the
correct, non-conflicting status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9149e10b-0879-40fc-87a0-b1b0d22273b4
📒 Files selected for processing (2)
.github/workflows/deploy-web-stack.ymldocs/production-incident-baseline.md
Comment on lines
+87
to
+89
| The previously-merged `migration_timeout_seconds=14400` bump and | ||
| `pg_stat_activity` instrumentation in PR #1113 are unaffected and remain | ||
| valid groundwork — but cannot be exercised until the host is stable. |
There was a problem hiding this comment.
Correct the PR #1113 observability statement to match current workflow behavior.
Line 87 says pg_stat_activity instrumentation in PR #1113 remains valid, but the next entry documents that periodic workflow snapshots were reverted. Keep one source of truth here so operators are not told both things.
Suggested edit
-The previously-merged `migration_timeout_seconds=14400` bump and
-`pg_stat_activity` instrumentation in PR `#1113` are unaffected and remain
-valid groundwork — but cannot be exercised until the host is stable.
+The previously-merged `migration_timeout_seconds=14400` bump in PR `#1113`
+remains valid groundwork — but cannot be exercised until the host is stable.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| The previously-merged `migration_timeout_seconds=14400` bump and | |
| `pg_stat_activity` instrumentation in PR #1113 are unaffected and remain | |
| valid groundwork — but cannot be exercised until the host is stable. | |
| The previously-merged `migration_timeout_seconds=14400` bump in PR `#1113` | |
| remains valid groundwork — but cannot be exercised until the host is stable. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/production-incident-baseline.md` around lines 87 - 89, The doc currently
gives conflicting statements about PR `#1113`: it claims `pg_stat_activity`
instrumentation from PR `#1113` remains valid while elsewhere it notes periodic
workflow snapshots were reverted; update the observability statement so there is
a single consistent source of truth — either state that both
`migration_timeout_seconds=14400` and the `pg_stat_activity` instrumentation in
PR `#1113` remain valid and can be exercised once the host is stable, or
explicitly note that `pg_stat_activity` instrumentation was impacted by the
revert of periodic workflow snapshots and is not currently usable; reference
`migration_timeout_seconds`, `pg_stat_activity`, PR `#1113` and the periodic
workflow snapshots revert in the updated sentence so operators see the correct,
non-conflicting status.
Adds max_slot_wal_keep_size=4GB to the dofek_db postgres service. Today's outage chain started when three PeerDB logical replication slots went inactive, and postgres dutifully retained 26GB of WAL to keep them resumeable — until it consumed the entire 100GB data volume and started PANIC'ing on WAL writes. Without this setting there's no upper bound; with it, a slot that falls more than 4GB behind gets invalidated (recoverable via resync) instead of taking the host down. Logs the incident in docs/production-incident-baseline.md with the full evidence chain and the recovery steps that were already executed against prod (drop slots, checkpoint, image swap with chown, compression policy tightened to 1 day). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds CREATE PUBLICATION for the dedicated no-IMU publication that the analytics metric_stream mirror now consumes from, and points the mirror DDL at it. Without this row filter at the postgres source, ~92% of the metric_stream WAL is IMU events that PeerDB has to decode, ship to MinIO staging, and then discard — that's what filled the data volume during today's outage. publish_via_partition_root applies the filter to all TimescaleDB chunks via a single publication entry. Documents the full afternoon recovery in production-incident-baseline.md: disk-full root cause, slot drops, image swap, host resize, ClickHouse rebuild via the postgresql() table function, the QREP catalog-corruption bug we sidestepped, and the remaining risks. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 2026-05-10 late-evening recovery exposed an unbounded-memory problem: with the 4 GB CH limit we set earlier, the full stack at load exceeded the 8 GB host and the kernel triggered a global OOM that took the box down (traefik invoked oom-killer, ClickHouse got killed at 3.5 GB RSS, system halted). Adds `deploy.resources.limits.memory` to every long-running service in stack.yml. Sized from measured idle RSS with growth headroom; sum of caps ≈ 8 GB on the 8 GB host. Per-container limits convert host-wide OOMs into single-container cgroup OOMs that swarm restarts cleanly. Tightens CH from 4 GB → 3 GB (it actually idles at 1.8 GB). Drops the now-redundant "Ensure ClickHouse resource limits" workflow step — stack.yml's deploy.resources.limits.memory is reconciled by the normal docker stack deploy, so the standalone enforcement step isn't needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI's clickhouse-cdc unit tests caught that the CREATE PUBLICATION I added to src/db/peerdb/metric-stream-cdc.sql in 828d2eb was being sent to PeerDB by the template runner, when publications actually live on the source postgres and need to go there. Reverts the SQL template addition and adds ensureMetricStreamNoImuPublication() in clickhouse-cdc.ts as a sibling to ensureAnalyticsPublication(), which is what already runs the existing publication setup on the source postgres client during setupClickHouseCdc. Updates the unit tests to expect the second source-postgres query and the shifted positions in the shared-mock peerDbQueries array. Also discovered (the hard way) that the existing splitPeerDbSqlStatements splitter does not strip line comments, so apostrophes or semicolons in comments confuse it. Comments in the SQL template now flag this and avoid both characters. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
migration_timeout_secondsin.github/workflows/deploy-web-stack.ymlfrom3300(55min) to14400(4h).docs/production-incident-baseline.md.Why
Deploy Web run 25609852303 failed: the prod migration step was killed at the 55-min cap while applying
drizzle/0018_migrate_body_measurements_to_metric_stream.sql— a single-transaction backfill that copies allfitness.body_measurementrows intofitness.metric_streamand rebuilds dependent indexes/views on a TimescaleDB hypertable. The migration was still progressing when the workflow killed the container, so the transaction rolled back and prod stayed in the pre-migration state. Re-running CI alone wouldn't have unblocked the deploy.Prod host itself was healthy throughout (SSH banner < 2s,
dofek.asherlc.com/healthzreturned 200 directly against the origin during the failure). The cap was the constraint, not the host or the migration.Test plan
--no-transactionkeyed on hypertable chunks) rather than bumping the timeout again — guidance noted inline in the workflow.🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Bug Fixes / Reliability
Documentation