p24-infra Docs

plan-7532-serialize-n8n-restarts

13 min read

Plan #7532 — Serialize & gate n8n container restarts on bms-4

Status: draft (design) · Issue: #7532 · Type: hardening (not a bug fix) Related: #7510 (root cause — fixed by PR #7511), #7509 (Redis rotation), #2489 (crashed-vs-error exporter split), #5817 (keepalive-timeout → recovery sweep).

1. Problem

n8n on bms-4 runs in queue mode (main + 3 workers, image n8n:2.26.3). When the n8n main container is recreated, any execution that has not yet had its terminal status written is reaped to status=crashed by the recovery sweep on the next startup. A single recreate therefore orphans every in-flight execution at once.

Fresh evidence (from #7532): 1729 executions marked crashed in a single instant at 2026-10-06 20:20:17 UTC — the signature of one container recreate catching a large in-flight set. The work itself had completed; only the DB status was wrong.

The original crash mechanism (throttled main event loop missing the DB keepalive, 5817) is fixed — PR #7511 raised the main CPU cap 0.5→2.0 and the self-inflicted recovery sweeps stopped (0 Database ping failed, 0 stuck/crashed over 24 h). What remains is the operator-/deploy-driven class: a restart issued on purpose (a deploy, a manual bounce) still orphans whatever is in flight, and nothing prevents two restart sources from overlapping.

1.1 Restart sources that recreate n8n containers on bms-4 (ground truth)

#SourceWhat it doesSerialized today?
1secrets-sync.yml → sync-bms-4 jobdocker compose stop n8n n8n-worker-1/2/3 → restart redis → docker compose up -d n8n n8n-worker-1/2/3 (.github/workflows/secrets-sync.yml ~L1683–1701). Triggers on any change to secrets/n8n-bms4.env.sops or secrets/n8n-bms4-gh.env.sops.Only vs other GH Actions runs of the same workflow (concurrency: secrets-sync). Not vs on-box ops.
2bms-4/start.shdocker compose up -d "$@" (manual full/partial bring-up).No.
3Ad-hoc operator / infra-task / sys-admin sessiondocker compose up -d …, docker restart bms-4-n8n-1, docker compose up -d --force-recreate ….No.

The workflow-level concurrency: secrets-sync guard only prevents two GitHub Actions runs from overlapping. It does nothing when a sync run and a human docker compose up -d on the box (or two humans) race — which is exactly the “multiple restart sources overlap” half of #7532. There is no on-host mutex today.

1.2 Current graceful-shutdown posture

  • No stop_grace_period on the n8n services in bms-4/docker-compose.yml → Docker’s default 10 s SIGTERM→SIGKILL window applies.
  • No N8N_GRACEFUL_SHUTDOWN_TIMEOUT set → n8n’s own graceful-shutdown window is at its default.
  • restart: always, command: start.

So a recreate gives the main process ~10 s to finalize in-flight work before SIGKILL — often not enough under load, which is why in-flight runs get orphaned instead of finalized.

1.3 Alerting claim — must be validated, likely already handled

The issue states false crashed rows trip N8nWorkflowPersistentlyFailing / failure-spike alerts. Grounding the code suggests this is already mitigated and should be re-verified before any work:

  • monitoring/exporters/n8n-bms4-exporter/app.py (#2489) labels each execution by its actual status, so crashed is its own series and never inflates status="error" (test_app.py:: test_crashed_not_counted_as_error).
  • N8nWorkflowPersistentlyFailing (monitoring/prometheus/rules/n8n.yml L143) fires on n8n_bms4_executions_1h{status="error"} > 3 — crashed is excluded by construction (the rule comment at L133 says so explicitly).
  • N8nWorkflowFailureSpike (L95) counts the native n8n_workflow_execution_duration_seconds_count {status="failed"} metric; recovery-sweep crashes are written straight to the DB and do not emit that native duration metric.

Design consequence: Option D below (suppress/annotate crashed for alerting) is low priority and possibly a no-op. The genuine harm is (a) wrong execution history and (b) operator confusion / on-call noise when someone eyeballs the executions list — not a tripping Prometheus alert. This plan treats D as “verify + document”, not “build”.

2. Goal / success criteria

  1. A deliberate n8n restart on bms-4 finalizes in-flight executions (or waits for them) instead of orphaning them to crashed.
  2. No two restart sources can recreate the n8n containers concurrently — secrets-sync, start.sh, and manual ops all serialize on a single on-host lock.
  3. secrets-sync stops recreating n8n when the change did not touch an n8n-relevant key (fewer needless restarts → fewer orphan windows).
  4. The crashed-vs-alerting behaviour is verified against the live rules and documented, so a future operator does not re-derive it.

3. Options evaluated

Option A — on-host flock mutex around every recreate path ✅ adopt

A single lock file (/var/lock/p24-n8n-restart.lock) guarding every path that recreates n8n containers on bms-4. The repo already uses this exact idiom (scripts/run-nightly-triage.sh L89, flock -n 200). Introduce one wrapper script, bms-4/n8n-restart.sh, that:

  • takes an exclusive flock on the lock file (blocking, with a timeout — not -n, because a deploy should wait for an in-progress deploy rather than skip),
  • performs the ordered recreate (the redis-first → n8n sequence already in secrets-sync),
  • releases the lock on exit.

secrets-sync.yml and bms-4/start.sh call this wrapper instead of open-coding docker compose up -d n8n …. Manual ops are documented to call it too.

Concrete lock parameters (resolves Risk gap 1): block with a bounded timeout rather than -n skip — flock -w 600 <fd>. The 600 s (10 min) ceiling is chosen to exceed the worst-case serialized recreate: main graceful drain (≤120 s, §B) + worker drain (≤60 s) + redis bounce + the existing 12×5 s (≈60 s) n8n verification loop + the 12×5 s pinbox-mongo-proxy loop ≈ 5 min, doubled for margin. On timeout the wrapper does not force through: it logs, fires the Discord infra-scripts error webhook, and exits non-zero (the caller — secrets-sync — then fails loud, same posture as the existing n8n-verification failure at .github/workflows/secrets-sync.yml ~L1738). The lock fd is released via trap '…' EXIT so a crashed wrapper never wedges the lock. flock on a lock file (not rm-ing it) means a dead holder’s lock auto-releases when its fd closes — no stale lock file to clean up.

Pros: cheap, proven idiom, directly closes the “sources overlap” gap the GH concurrency guard can’t. Cons: manual docker restart outside the wrapper still bypasses it — mitigated by docs + by making the wrapper the single obvious entry point (see the emergency-bypass note in §4.6).

Option B — graceful drain before recreate ✅ adopt (highest value)

Give n8n time to finalize in-flight executions before SIGKILL:

  • Add stop_grace_period to the n8n main (and workers) in bms-4/docker-compose.yml, large enough to cover a normal execution tail (candidate: 120 s main, 60 s workers — tune against observed execution durations).
  • Confirm n8n honours SIGTERM graceful shutdown in queue mode and finalizes/leaves-replayable the in-flight set; set N8N_GRACEFUL_SHUTDOWN_TIMEOUT consistently with stop_grace_period if the version respects it.
  • The wrapper from Option A issues docker compose stop -t <grace> (or relies on the compose stop_grace_period) so the drain window is actually honoured on recreate.

Pros: attacks the actual root of the 1729-crashed event — orphaning on a deliberate restart. Cons: longer restart wall-time; a pathologically long-running execution can still exceed the grace window (acceptable — bounded, and far better than an unconditional 10 s).

Option C — reduce needless recreates in secrets-sync ✅ adopt (scoped)

Today sync-bms-4 recreates the whole n8n set on any n8n-bms4*.env.sops change. Mirror the fingerprint-and-skip pattern already used in the same job for claude-session-manager (.github/workflows/secrets-sync.yml ~L1490–1534: “restart the service ONLY when a value actually changed”).

Fingerprint the RENDERED env, not a single SOPS file (resolves Risk gap 2). The running n8n containers’ env is assembled from five sources, so a fingerprint keyed on n8n-bms4.env.sops alone would silently miss a changed credential and ship it stale — exactly the #7152 drift class. The correct key set to fingerprint is the n8n-relevant keys in the final rendered /opt/p24-infra/bms-4/.env and /opt/p24-infra/bms-4/n8n-bms4-gh.env, which sync-bms-4 builds from:

Source SOPS filen8n-relevant key(s) it contributes
secrets/n8n-bms4.env.sopsREDIS_PASSWORD, N8N_ENCRYPTION_KEY, N8N_DB_PASSWORD, TELEGRAM_BOT_TOKEN, GROQ_API_KEY, ATRAX_*, CLAUDE_SESSION_MANAGER_KEY, … (primary)
secrets/monitoring.env.sops (append ~L1248)PROMETHEUS_BASIC_AUTH_USER, PROMETHEUS_BASIC_AUTH_PASSWORD
secrets/pinbox24-w4-auth.env.sops (append ~L1265)REDIS_BRIDGE_KEY
secrets/et-operational-platform.env.sops (re-assert ~L1292)PINBOX24_MONGODB_URI (canonical)
secrets/n8n-bms4-gh.env.sops (separate install)GH_TOKEN, GITHUB_PAT_*

Implementation: compute a hash of the n8n-relevant lines of the final .env + n8n-bms4-gh.env after all appends/re-asserts complete and before the install overwrites the live files; compare to a hash of the current live files; recreate n8n only when they differ. Keyed on the rendered output, the guard is source-agnostic — adding a 6th append later needs no allow-list edit, because whatever lands in the rendered .env is fingerprinted. Print key NAMES / the hash only, never values (mirrors the existing claude-session-manager block). Prefer targeted --no-deps <service> where only one service’s key changed.

Pros: removes a large fraction of restarts outright; robust to the multi-source assembly. Cons: fingerprinting the whole rendered n8n-key set is slightly coarser than per-service precision — acceptable, and far safer than an under-inclusive per-file allow-list.

Option D — suppress / annotate post-restart crashed rows for alerting ⚠️ verify only, do not build

Per §1.3 the exporter and rules already exclude crashed from the failing/spike alerts. Action: verify this on the live stack (confirm no alert actually fired on the 2026-10-06 20:20 event) and, if confirmed, document it. Only if a real alert is found firing on crashed do we design a suppression — e.g. a Prometheus annotation/inhibition keyed on a restart marker. No build work is planned unless the verification contradicts the code.

Adopt B + A + C as the core; D = verify + document.

Ordering by value: B (graceful drain) removes the orphaning on any single restart; A (flock) removes the overlap between sources; C (fewer recreates) shrinks how often the window is opened at all. Each is independently useful and independently shippable.

4.1 Changes required

All rows below ship in the SINGLE implementation PR/task (resolves Completeness gap) — there is no separate follow-up. The dev_r_services row is written as part of the same implementation task (compliance is mandatory in the same change per CLAUDE.md §Compliance, not deferred), and every doc edit is committed in the implementation PR alongside the code.

File / artefactChangeShips in impl. PR?Why
bms-4/docker-compose.ymlAdd stop_grace_period: to n8n (and each n8n-worker-*); set N8N_GRACEFUL_SHUTDOWN_TIMEOUT consistently if honoured by 2.26.3yesOption B — finalize in-flight before SIGKILL
bms-4/n8n-restart.sh (new)flock -w 600-guarded wrapper performing the ordered redis→n8n recreate with docker compose stop -t <grace>; trap EXIT fd release; Discord error-notify on failure/timeout (CLAUDE.md §Error Notification Standard)yesOptions A + B — single serialized entry point
.github/workflows/secrets-sync.yml (sync-bms-4)Call bms-4/n8n-restart.sh instead of the inline stop/up block; wrap in the rendered-env fingerprint-and-skip guard (§C)yesOptions A + C
bms-4/start.shRoute the n8n bring-up through bms-4/n8n-restart.sh (or share the lock)yesOption A — close the manual path
docs/n8n-operations.mdDocument the single restart entry point, the lock + flock -w 600 timeout, the grace window, the §4.6 emergency-bypass + rollback, and the verified crashed-vs-alerting behaviouryesOption D + operability
dev_r_services (Supabase) rowRegister bms-4/n8n-restart.sh as a new operational script (compliance_workbook='yes', workbook_url → docs/n8n-operations.md)yes (same task)CLAUDE.md §Compliance — mandatory in-change

Note: pushing the .github/workflows/secrets-sync.yml edit requires the workflow-scope token path (docs/playbooks/worker-push-workflow-files.md) — the implementer (an infra-task/sys-admin worker on bms-4) must use the GH_TOKEN PAT push helper, not the default gh OAuth login.

4.2 Regression risks

  • Longer deploy time. A graceful drain + serialized lock lengthens sync-bms-4. The existing verification loop (12×5 s) already tolerates slow startup; confirm the GH job timeout covers the new worst case.
  • Lock starvation / stuck lock. If the wrapper dies holding the lock, later restarts block. Use a blocking flock with a timeout and release via trap … EXIT; log + Discord-notify on timeout.
  • Grace window too long for a hung execution. stop_grace_period is an upper bound — SIGKILL still fires after it, so a hung run cannot wedge a deploy forever.
  • secrets-sync skip-guard too aggressive. If the n8n-relevant key allow-list misses a key, a rotated credential won’t reach the running container until the next real n8n restart. Keep the list broad and documented; a missed-key case is a stale-credential bug, not an outage.
  • restart: always interaction. Verify the wrapper’s stop + up sequence doesn’t race Docker’s own restart policy.

4.3 DB schema changes

None.

4.4 Rollback & operator emergency path (resolves Risk gap 3)

Emergency restart when the lock is held / the wrapper hangs. The wrapper is the normal entry point, not a cage. An operator who needs n8n back now and finds the lock held has two documented escapes (both to be written into docs/n8n-operations.md):

  1. Inspect the holder first: fuser /var/lock/p24-n8n-restart.lock (or lsof) → if it is a live, legitimate deploy, wait (that is the intended serialization).
  2. Genuine emergency only: run the recreate directly, bypassing the lock — cd /opt/p24-infra/bms-4 && docker compose up -d --force-recreate n8n n8n-worker-1 n8n-worker-2 n8n-worker-3. This knowingly forgoes serialization; it is the last resort and is logged as such. (flock on an fd auto-releases when the holder dies, so there is normally nothing to “clear”; only use a manual bypass when a holder is genuinely wedged.)

Rollback of the changes themselves (each independently revertible):

  • Compose change: revert the stop_grace_period / N8N_GRACEFUL_SHUTDOWN_TIMEOUT lines in bms-4/docker-compose.yml, then docker compose up -d --force-recreate n8n n8n-worker-*. Returns to the current Docker-default 10 s window immediately.
  • Wrapper + start.sh: revert the two files; the inline stop/up block restored in start.sh keeps bring-up working with zero wrapper dependency.
  • secrets-sync workflow: revert the sync-bms-4 edit to restore the always-recreate inline block, then re-run secrets-sync.yml. Because the fingerprint guard only skips a recreate, the fallback of the old behaviour is strictly “recreate more often”, never “fail to deploy”.
  • No DB migration to roll back; the dev_r_services row is a benign metadata entry.

4.5 Alerting during the longer drain (addresses Risk sub-point)

The graceful drain lengthens the window where n8n containers are briefly down during a recreate. N8nWorkerDown is already for: 10m / warning and N8nAllWorkersDown is for: 3m / critical (monitoring/prometheus/rules/n8n.yml). A 120 s main + 60 s worker drain stays inside the 3 m N8nAllWorkersDown window, so no new false page is expected — but the implementer must confirm the total serialized recreate wall-time (drain + verify loops) stays under 3 m, or raise N8nAllWorkersDown’s for: accordingly. Noted as an implementation acceptance check.

4.6 Manual test checklist

  • Trigger bms-4/n8n-restart.sh while a known long execution is in flight → it finalizes (success), is not marked crashed.
  • Launch two n8n-restart.sh invocations concurrently → the second waits for the first (lock held), neither leaves orphaned/hash-suffixed containers.
  • secrets-sync run that changes a non-n8n key in n8n-bms4.env.sops → no n8n recreate.
  • secrets-sync run that changes an n8n key → n8n recreated exactly once, all 4 containers reach running (existing verification loop still passes).
  • Confirm against live Prometheus that the 2026-10-06 20:20 crashed burst did not fire N8nWorkflowPersistentlyFailing / N8nWorkflowFailureSpike (validates §1.3 / Option D).
  • Discord error webhook fires if the wrapper fails or the lock times out.

4.5 Dependencies

  • Independent of #7509 (Redis rotation). The redis-first ordering already lives in secrets-sync and is preserved by the wrapper.

5. Scope / role boundary

This is a design plan (this issue is plan-labelled). The deliverable here is this document + the ## Code-change-design comment + /review-plan. The implementation lands server-side changes (docker-compose.yml on bms-4, a new on-host wrapper script, a secrets-sync.yml workflow edit) and is therefore an infra-task/sys-admin job, not a dev-coder one — the implementation issue must be dispatched with server_preference='bms-4' so it runs where production SSH + Docker live.

File-overlap signature: docs/plans/plan-7532-serialize-n8n-restarts.md

files_to_change (implementation): bms-4/docker-compose.yml, bms-4/n8n-restart.sh, .github/workflows/secrets-sync.yml, bms-4/start.sh, docs/n8n-operations.md

Graph View