Repository navigation
fix(helm): crash dumps outlive the pod, under retention and a headroom gate - #6316
Conversation
…m gate The portal wrote createdump output to a pod-local memex-dumps emptyDir, so every roll deleted it (the 09-26 SIGSEGV dump and the 10-07 memex-cloud dump; blocks #4654, #1605, #5555). Dumps now go to /data/dumps/<pod>/ on the /data volume (the shared /data claim on AKS). A postStart hook (files/crash-dump-retention.sh) runs on every container start: delete dumps older than crashDumps.maxAgeDays, keep the newest crashDumps.keep across all pods, and arm this pod's directory only when the volume has room for headroomDumps x the memory limit + reserveMiB. Otherwise it HOLDS (no directory, so createdump writes nothing) so a dump can never fill /data. Always exits 0 and logs every outcome as [crash-dumps]. Chart invariant #20 asserts the wiring in every values combination; test-crash-dump-retention.sh executes the hook in Chart Gate. Documented in Doc/Architecture/DebuggingNativeCrashes. Takes effect on each instance's next Reconcile, not on an image Roll. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved correctness and storage-safety issues can lose crash evidence or exhaust shared storage.
6 open findings
What changed in this PR
Moves portal crash dumps onto /data so persistent claims can preserve diagnostic evidence across pod replacements.
Changes:
- Adds per-pod dump paths and configurable retention and headroom checks.
- Extends chart validation and CI with retention checks.
- Documents dump recovery and activation through Reconcile.
| File | Description |
|---|---|
src/MeshWeaver.Documentation/Data/Architecture/OnboardingNewEnvironment.md |
Updates dump-verification guidance. |
src/MeshWeaver.Documentation/Data/Architecture/DebuggingNativeCrashes.md |
Documents storage, retention, and recovery. |
src/MeshWeaver.Documentation/Data/Architecture/CollectibleThreadStaticHandleReuse.md |
Links historical dump loss to the new approach. |
deploy/helm/values.yaml |
Adds crash-dump settings. |
deploy/helm/templates/memex-portal/deployment.yaml |
Configures dump paths and lifecycle hook. |
deploy/helm/files/crash-dump-retention.sh |
Implements retention and headroom checks. |
deploy/aks/scripts/test-crash-dump-retention.sh |
Adds executable retention cases. |
deploy/aks/scripts/check-chart-invariants.sh |
Documents the new invariant. |
deploy/aks/scripts/check-chart-invariants.py |
Checks dump-path and hook wiring. |
deploy/aks/README.md |
Updates operational guidance. |
.github/workflows/chart-gate.yml |
Runs retention checks in CI. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| ls -1t "$root"/*/coredump.* 2>/dev/null | tail -n +"$((keep + 1))" | | ||
| while IFS= read -r f; do | ||
| if rm -f "$f" 2>/dev/null; then log "deleted (beyond the newest $keep): $f"; else log "ERROR: could not delete $f"; fi |
There was a problem hiding this comment.
Agreed. Fixed in 490503e. Retention now never deletes a file modified within crashDumps.activeMinutes (default 30), even when it is beyond keep. A dump that is still being written keeps advancing its mtime, so this check is about recency, not completeness: once a file's mtime is older than the window, nothing is writing it. Skipped files still count toward keep and are logged as kept (… possibly still being written).
New case 3 in test-crash-dump-retention.sh: keep=1, with two writers whose mtimes are 1 and 2 minutes ago, plus one finished 100-minute-old dump. Both writers survive and the finished dump is deleted. Removing the check (mutation) reds the case.
| # 3. Empty directories of other pods (a rolled-away pod that never crashed). | ||
| for d in "$root"/*/; do | ||
| [ -d "$d" ] || continue | ||
| d="${d%/}" | ||
| [ "$d" = "$root/$pod" ] && continue | ||
| rmdir "$d" 2>/dev/null || : | ||
| done |
There was a problem hiding this comment.
Agreed. Fixed in 490503e. Step 3 now removes only EMPTY parked directories (<pod>.<epoch>, <pod>.held-<epoch>). Another pod's armed directory is never removed, empty or not. Case 4 is replaced by case 6, which starts pod-a, pod-b, then pod-a again and asserts both targets remain. Restoring the old "$root"/*/ glob (mutation) reds it.
Cost: departed pods leave empty armed directories behind (one inode each). The doc names this.
| free=$((free_kib * 1024)) | ||
| need=$((limit * factor + reserve_mib * 1024 * 1024)) | ||
| count="$(ls -1 "$root"/*/coredump.* 2>/dev/null | wc -l | tr -d ' ')" | ||
| if [ "$free" -ge "$need" ]; then | ||
| if mkdir -p "$root/$pod" 2>/dev/null; then |
There was a problem hiding this comment.
Agreed. The claim was too strong. Fixed in 490503e in three ways:
- A dedicated dump claim (
persistence.dumps, mounted ATcrashDumps.root) is now supported and documented as the recommended shape. With it, the worst a dump can fill is the dump claim. A record declares it as adumpsvolume, and a new fixture renders it (invariants 12 and 20 cover it). - On the shared
/dataclaim,headroomDumpsnow defaults to the replica ceiling (keda.maxReplicasunder KEDA, elsereplicas.portal), so every pod that could crash at once is counted: 8 on the AKS overlay, 2 on the two-replica shape. On a dedicated claim it defaults to 1. - Docs, values and script header now say the gate is a startup snapshot, not a reservation. On
/datait bounds, but cannot guarantee, the space other writers leave. Only a dedicated claim isolates dumps from/data.
Not done here: declaring the dumps volume on the production records. That is a record change on the control instance.
| mv "$root/$pod" "$held" 2>/dev/null || log "ERROR: could not move $root/$pod aside" | ||
| fi | ||
| log "HELD: dumps are DISABLED for this container start: free $((free / 1048576)) MiB < needed $((need / 1048576)) MiB ($factor x $((limit / 1048576)) MiB limit + $reserve_mib MiB reserve). A crash now writes no dump. Free space on the volume or lower crashDumps.keep; $count dump(s) retained." |
There was a problem hiding this comment.
Agreed. Fixed in 490503e. A single disarm path now does rmdir, falls back to a rename, and then checks [ -e target ]. Only an absent target logs HELD. If the target still exists it logs ERROR: could not remove or move aside … — dumps REMAIN ARMED there …. The unreadable-df and bad-setting branches use the same path.
A restart also parks the previous non-empty directory FIRST (step 0), so a low-headroom restart does not keep writing into the earlier start's directory.
New case 7: a read-only parent with an existing target. It asserts REMAIN ARMED and no HELD, and has a control that the directory really survived. Mutating the check to always print HELD reds it.
On "disable through the startup path": that would mean overriding the image entrypoint. See the reply on the postStart thread.
| that outlives the pod (the shared /data claim on AKS). A root anywhere else lands on the | ||
| container's writable layer, which a roll deletes exactly as it deleted the old emptyDir. */}} | ||
| {{- $dumps := dict | ||
| "root" (trimSuffix "/" (((.Values.crashDumps).root) | default "/data/dumps")) |
There was a problem hiding this comment.
Agreed. Fixed in 490503e.
- The chart now requires
^/data(/[A-Za-z0-9_-][A-Za-z0-9._-]*)+$: no segment may start with., and//and a trailing/are refused. That rejects/data/../tmp/dumps,/data/dumps/..,/data//and/dataitself. - Three new refusal controls in
check-chart-invariants.shcover exactly these inputs. - Invariant 20 now requires
posixpath.normpath(root) == rootand a/data/prefix. - The script itself also refuses a non-normalised or relative root before touching anything (case 8; removing that refusal reds it).
| postStart: | ||
| exec: |
There was a problem hiding this comment.
Partly fixed in 490503e. A crash-dump-prepare init container now runs the same script synchronously before the portal process exists. It uses the portal image, so the user and tools are the same, and it reads the portal's limits.memory. That covers every pod's first start. postStart still covers restarts in place, and on a restart the script now parks the previous directory first.
What remains: a native crash in the first moments of a restart in place, before postStart has run. Closing that needs the gate to run inside the container's own start, which means the chart overriding the image's entrypoint. The image is built by the SDK container publish and the chart does not own its entrypoint, so coupling to it here would trade this window for a boot failure on any entrypoint change. The gap is stated in DebuggingNativeCrashes → "What it does not cover", and invariant 20 asserts both runs are present.
… init container, writer-safe retention - A dedicated dump claim (persistence.dumps, mounted AT crashDumps.root) is the recommended shape: a dump can then fill only the dump claim. Without one the dump shares /data and the gate is documented as a snapshot, not a guarantee. - headroomDumps defaults to the replica ceiling on /data (every pod that could crash at once), 1 on a dedicated claim. - crash-dump-prepare init container runs the script synchronously before the portal process exists; postStart keeps covering restarts in place. - A restart parks the previous directory first; disarm verifies the target is gone and otherwise logs REMAIN ARMED (never HELD). - Retention never deletes a file modified within activeMinutes (a write in progress) and never removes another pod's armed directory. - crashDumps.root must match ^/data(/segment)+$ (no ., .., //); three refusal controls; invariant 20 normalises and checks both runs. - test-crash-dump-retention.sh: 11 cases, each behaviour caught by a mutation. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test Results 22 files + 1 22 suites +1 48m 42s ⏱️ + 3m 12s Results for commit 490503e. ± Comparison against base commit 3fc485c. This pull request removes 29 and adds 1338 tests. Note that renamed tests count towards both. |
Problem
The portal's crash dumps went to a pod-local
memex-dumpsemptyDir, which is deleted with the pod, and every roll replaces the pod. That is how the 09-26 SIGSEGV dump and the 10-07 memex-cloud dump were lost. Without them, #4654, #1605 and #5555 are blocked.Change (deploy/helm)
DOTNET_DbgMiniDumpName = <crashDumps.root>/$(MEMEX_POD_NAME)/coredump.%p.%t(root defaults to/data/dumps). It lands on a volume that outlives the pod:persistence.dumps.claimNameis set. This is the recommended shape: the claim is mounted AT the root, so a dump can fill only that claim. A record declares it as adumpsvolume, withcreate/size/storageClassif the chart should create it./datavolume (the sharedmemex-dataclaim).^/data(/segment)+$with no segment starting with.. That refuses..,.,//and/dataitself.memex-dumpsemptyDir is removed. Volumes 0 and 1 are untouched.files/crash-dump-retention.shruns twice: as thecrash-dump-prepareinit container (synchronously, before the portal process exists) and as the portal's postStart hook (every container start, so restarts in place are covered too). In order, it:maxAgeDays(14);keep(3) across all pods, but never deletes a file modified withinactiveMinutes(30), because that file may still be being written;headroomDumps× the portal memory limit +reserveMiB.headroomDumpsdefaults to the replica ceiling on/dataand to 1 on a dedicated claim. Otherwise the directory is left absent, which is verified before the script printsHELD.[crash-dumps] <init|postStart> …./dataclaim the gate is a startup snapshot, not a reservation. It bounds, but cannot guarantee, the space other writers leave. Only a dedicated claim isolates dumps from/data. This is documented.Gates (all run locally, green)
Chart invariant Update release-packages.yml #20, checked in every combination:
/data;MEMEX_POD_NAMEcomes frommetadata.nameand is defined first;memex-dataat/data;limits.memory;memex-dumpsvolume is rendered.New fixture: AKS overlay + a dedicated dump claim. New refusal controls: the three bad roots. Result:
All 18 values combinations render a self-consistent deployment, and all 10 refusal controls hold.Negative control: rendering main'sdeployment.yamlfails all six portal findings in all 18 combinations, and all three refusals.test-crash-dump-retention.sh(new Chart Gate step) executes the script undershin 11 cases: keep-newest, max-age, active writer protected, headroom HOLD, restart parking, two live pods, unremovable target reported as REMAIN ARMED, path refusal, two refusal paths, and a negative control. Five script mutations each red at least one case: no active window, removing foreign dirs, always-HELD, no parking, no path refusal.check-values-are-read.sh,test-chart-drift-compare.shandtest-chart-drift-render.share green.MeshWeaver.Documentation.Testbuilt in Release with-warnaserror(0 warnings) and passed 721/721.Docs
Doc/Architecture/DebuggingNativeCrasheshas a new section, "Production: where a dump lands, and how long it stays". It covers the wiring, the script steps, what it does not cover, the current production numbers, and how the change takes effect.OnboardingNewEnvironment,CollectibleThreadStaticHandleReuseanddeploy/aks/README.md.🚨 Takes effect on the next Reconcile, not on a Roll
This is a chart change, so it reaches an instance only through a
helm upgradefrom its record (a Reconcile). Until then, a roll still deletes the dump.On the current records there is no
dumpsvolume, so dumps go to the shared/dataclaim:Where
/datahas less free space, every pod logsHELDand no dump is written. The live free space was not measured. Declaring adumpsvolume on each record is the follow-up that makes dumps reliable. After the Reconcile, read the first[crash-dumps]line of each pod (aLogsaction, query|= "[crash-dumps]").Nothing to recycle: no NodeType or hub-served content changed.
Pairs-with: none — no public C# surface touched (chart, scripts and docs only)
🤖 Generated with Claude Code