Skip to content

fix(api): tolerate concurrent DNS record creation (fixes stuck deployments) - #45

Open
defang-sam[bot] wants to merge 5 commits into
mainfrom
sam/use-sam-mcp-tools-wkkamr
Open

fix(api): tolerate concurrent DNS record creation (fixes stuck deployments)#45
defang-sam[bot] wants to merge 5 commits into
mainfrom
sam/use-sam-mcp-tools-wkkamr

Conversation

@defang-sam

@defang-sam defang-sam Bot commented Sep 5, 2026

Copy link
Copy Markdown

Summary

Fixes the chain that left deployments stuck on the defanglabs.ca install. Three defects, one incident, all in the deployment apply path.

1. Concurrent app-route DNS creation 500'd the release fetch

GET /api/nodes/:id/deploy-release upserts every app-route DNS record before returning the node's release payload. upsertAppRouteDNSRecord was a check-then-act with an await in the gap, fanned out via Promise.all (deploy-release-callback.ts:306,328). Two callers both saw "no record", both POSTed, Cloudflare rejected the loser with 81058, and the throw propagated out of Promise.all:

12:55:58  source=api  node=01M1RS3F0SKBQSPMRVSJQSSMF2
  message : An identical record already exists.
  context : {"path":"/api/nodes/01M1RS3F…/deploy-release","method":"GET","status":500}
  stack   : at upsertAppRouteDNSRecord (index.js:137759)
            at async Promise.all (index 0)

The node never received its payload, so the deployment stalled upstream of anything cert-related. The missing certificate everyone could see was three layers downstream of the actual failure.

Fix: treat the duplicate-record codes on the create path as a lost race — re-resolve once, update in place. Scoped to 81057/81058 (81053, a cross-type collision, still throws), create-path only, bounded to one retry.

2. The same race in createNodeBackendDNSRecord, with a worse failure mode

Found by review against this PR's own new rule (§6: check sibling operations in the same module) — which this PR had not applied to its own module. It was a blind POST with no lookup at all. Two paths create that record; on conflict node-lifecycle.ts:460 stamps nodes.error_message and leaves backend_dns_record_id NULL, so every later heartbeat retried the same losing POST forever (no backfill job exists) and node deletion, which deletes by that id, orphaned the real record. Now resolves the winner so the id gets persisted, fixing both.

3. The duplicate spawn that caused the race

health.go spawned a new apply goroutine per heartbeat per pending release with no in-flight check. observed.AppliedSeq only advances after a fully successful apply, so any release slower than one heartbeat interval accumulated another concurrent apply per tick — each re-running the whole control-plane fetch.

4. Compose SIGKILLed mid-pull by the apply idle watchdog

The apply is bounded by a 15-minute idle timer reset only by ApplyProgressEvents. docker compose up emits exactly one (compose_up_started) and then nothing until it returns, so a legitimately slow image pull was indistinguishable from a hung apply. This is what produced compose up: … signal: killed (stderr: db Pulling …).

Compose streams pull progress to stderr, so that is now the liveness signal — runCompose wraps stderr in a livenessWriter that pokes the watchdog per write without persisting an event, mirroring newIdleProgressReader which already does this for artifact downloads. Also fixed the diagnostic that hid it: health.go overwrote the accurate "deployment apply stalled: no progress for 15m0s" with the child's signal: killed — a consequence of our own cancel — making a self-inflicted timeout indistinguishable from an OOM kill.

5. deployment_volumes.status frozen at the provider's transient snapshot

attachEnvironmentVolumes persisted attached.status straight from the provider's response. Hetzner commonly still reports creating at that instant, and nothing ever re-polls the row — the only other writer is the detach path. So an attached, mounted, working volume read creating forever.

Nothing reads the column, so this is not a functional bug. It is worse in a specific way: it is the single most obvious "here is your stuck deployment" signal in the table, and it is false. It cost real time in this investigation before code reading showed the heartbeat gate keys on attached_server_id, not status.

Now persists the settled SAM-side fact (attached). No migration, no constraint widening — the column is plain TEXT with no CHECK, and attached is already in the provider's VolumeStatus union. Rule 57 at the storage layer; polling was rejected as disproportionate since the fact is knowable locally the moment attach returns.

Live production evidence

From deployment_release_events for env pr18-preview-3, one node, while this branch was being written:

 18x deployment.apply.fetch_started
  9x deployment.apply.started
  9x deployment.apply.compose_up_started
  0x  (no compose_up_completed, ever)

An exact 2:1 fetch/apply ratio, still holding after 2.5 hours of looping — the duplicate's Apply() is rejected by the engine's applyMu.TryLock(), which is why applies are half the fetches, but the fetch has already run and the fetch is what upserts DNS. 18 chances to lose the race on one environment. And 9 compose starts with zero completions, cycling every ~14 minutes against a 15-minute idle timeout.

Validation

  • pnpm lint — clean on changed files
  • pnpm typecheck — clean
  • pnpm test — API unit suite 8054/8054, 0 collection errors, total reconciled 8043 → 8054 (+11).
  • Workers suite (vitest.workers.config.ts) — 761/761, 0 collection errors.

Both CI test jobs are now green. Two failures showed up in earlier runs on this branch (Test and Durable Object Workers). I initially reported them as "pre-existing, unrelated" — true, but incomplete: I had verified them as failing on a clean main baseline, but hadn't found why. Root-caused and fixed in a follow-up commit, test-only, no production code touched:

Test Root cause Fix
project-data-snapshot-recovery-wake.test.tsallows a stopped ProjectData session to wake… Fixture hardcodes NOW = 2026-08-26; expires_at seeded at NOW + 7 days = 2026-09-02. The production function (hasAuthorizedRestorableSnapshotWakeClaim) has no injectable clock and defaults to real new Date(). Once real time passed 2026-09-02, expires_at > now went false — a time bomb, not a flake. Freeze the test's clock to the fixture's NOW (vi.useFakeTimers/setSystemTime), so the production default resolves to the intended fixture time.
project-data-tool-payload-archive.test.tsretrieves archived tool payloads through the MCP tool (401 not 200) The file's FIXED_NOW (2026-08-26) is correctly threaded through the archival business logic's now/nowMs params — but this one test also stamped an MCP token's createdAt with that same fixed date. validateMcpToken has no injectable clock by design (auth boundary) and rejects tokens older than 24h against real Date.now(). Once real time passed 2026-08-27 the token looked ~10 days old → 401. Two independent clocks, wrongly conflated. Stamp the token with the real current time instead of the archival fixture's frozen date.

Both fixes verified discriminating: reverting either reproduces its original failure exactly, with no effect on the other tests in the same file. Neither touches DNS, deployment apply, compose, or volumes — the actual scope of this PR. Filed and closed as idea 01M1S79XZMF4Q9H53MQTCEQM0M (originally mis-filed as "pre-existing failures" before the root cause was found).

  • vm-agent: go build, go vet, gofmt clean on all touched files; 23/23 packages pass; deploy + server pass -race
  • N/A: does not change candidate selection for any sweep/cron/alarm loop

Every guard proven discriminating

Each mutation applied once, observed, reverted:

Mutation Result
DNS tolerance disabled 4 race tests red, 10 controls green
!existing removed (widen to update path) exactly the update-path control red
sibling conflict-recovery disabled exactly the 2 sibling recovery tests red
code: v.optional(v.number()) restored exactly the 2 message-preservation tests red
pre-fix dns.ts + route suite the new route test red; the other 31 all passed
dedup guard disabled exactly the duplicate test red, both controls green
stderr reverted to a plain buffer exactly the liveness test red
head-retention restored exactly the 2 tail tests red

The fifth row is the one that matters most: the existing route suite could not observe the production 500. Only a test at the real entry point can.

Staging Verification (REQUIRED — merge-blocking)

  • Staging deployment green
  • Live app verified via Playwright
  • Existing workflows confirmed working
  • New feature/fix verified on staging
  • Infrastructure verification (VM provisioned, heartbeat confirmed)
  • N/A: no UI changes

Staging Verification Evidence

Not performed. This is a merge blocker under .claude/rules/13 and /22, and a human must decide how to proceed. I am deliberately not checking these boxes rather than rationalizing them (.claude/rules/30).

Two reasons:

  1. deploy-staging.yml targets the upstream sammy.party environment, not this fork's defanglabs.ca deployment.
  2. This PR changes packages/vm-agent/, so .claude/rules/27 applies: VM agent binaries are downloaded once at cloud-init. Verifying the Go changes requires deleting all nodes, deploying so the new binary reaches R2, and provisioning a fresh node. Testing against an existing node would exercise the old binary and prove nothing.

What is verified: the production failure was root-caused from live D1 and Cloudflare state (the error row, the zone's DNS records, the duplicated seq=4, the 2:1 fetch/apply ratio), and every fix has a regression test proven to fail against the pre-fix code.

Suggested verification once deployed: delete all nodes → deploy → provision fresh → confirm deployment_release_events shows a 1:1 fetch/apply ratio (not 2:1), compose_up_completed appears, and a grey-cloud r{N}-…apps.{BASE_DOMAIN} A record resolves to the node IP.

Post-Mortem

What broke

Deployments stalled indefinitely with no app-route DNS record and no TLS certificate; later attempts failed with signal: killed during docker compose up.

Root cause

Check-then-act against an external API that enforces its own uniqueness constraint, with the conflict treated as fatal on a path that gates an entire deployment — plus a duplicate-spawn bug that made the race far likelier, and a watchdog blind to the longest step it was guarding.

Class of bug

Two, from one incident:

  1. Check-then-act against a remote uniqueness constraint. Cross-isolate sibling of rule 45 (DO await interleaving). A mutex isn't proportionate when the remote API adjudicates in one round trip — converge on its answer. Review then showed the overlap was also preventable one layer up, so this PR does both: remove the cause, and survive it anyway (old agents keep retrying until replaced — rule 54).
  2. A watchdog fed by a signal that cannot observe the work it guards. The mirror image of rule 53's liveness-as-idleness trap.

Why it wasn't caught

Existing tests covered create-when-absent and update-when-present — the two sequential outcomes. Nothing exercised two callers interleaving, despite the only production call site being a Promise.all fan-out. And no test existed at the route, so all 31 route tests passed while the endpoint 500'd in production.

Process fix included in this PR

  • .claude/rules/68-external-api-check-then-act.md (new) — its §6 immediately caught the second live instance fixed here.
  • .claude/rules/53 §5c (extended, not forked — same family) — "which step is the longest, and does it emit the signal the timer listens for?", plus the two traps found while fixing it.

Post-mortem file

tasks/active/2026-09-05-fix-app-route-dns-upsert-race.md

Specialist Review Evidence

  • All local reviewers completed and findings addressed before merge
Reviewer Status Outcome
cloudflare-specialist ADDRESSED Verified the CF error-code taxonomy against 5 independent sources and reproduced the discriminating-test claim. MEDIUM: v.optional(v.number()) rejects code: null/stringified, silently degrading error messages for 5 untouched callers → fixed 7db98bfb. 2 LOW (subrequest/latency budget) verified benign, documented.
architecture-reviewer ADDRESSED 2 HIGH: (1) createNodeBackendDNSRecord sibling with a worse failure mode → fixed 0ee9dd5e. (2) vm-agent duplicate apply spawns, with an unused dedup primitive already present → fixed 7625486e. MEDIUMs: rule wording overclaimed "a mutex cannot help"; retry bound was the only unnamed limit in apps/api/src → both fixed.
test-engineer ADDRESSED 2 HIGH, both empirically demonstrated: (1) no test enforced the !existing guard — the suite stayed green when widened; (2) no route-level test — pre-fix code passed all 31. Both fixed 0ee9dd5e. MEDIUM: the fan-out test was scripted, not a real race → replaced with a shared-store same-hostname race.
go-specialist ADDRESSED HIGH: livenessWriter retained the head of compose output, contradicting its own doc comment — compose prints the failure on its last lines, so the cap discarded exactly the diagnostic being preserved → fixed 9e43ad74 with tail retention + 2 content-asserting tests. Verified no deadlock across the 4 mutexes, no goroutine leak, correct defer semantics, and (against Go 1.26.6 stdlib source) that cmd.Wait() rules out the custom-writer race. 2 LOW (dead test code, overstated panic comment) fixed.

Known limitations (deliberate, stated)

  • The DNS tolerance is retained even though the dedup guard now prevents the overlapGET /deploy-release is retry-tolerant by design, and already-deployed agents keep retrying until replaced (rule 54). Defence in depth: the guard removes the cause, the tolerance survives it.
  • Still unexplained: the two original fetch_started events were 1 second apart, which does not match the 60s HEARTBEAT_INTERVAL. The 2:1 ratio above is what the goroutine-per-heartbeat bug predicts, but the 1-second spacing suggests a second trigger that was not identified. The dedup guard closes the window regardless, since it keys on the job id rather than the caller.
  • Not fixed here: deployment_volumes.status is written once from the provider's transient attach response and never re-polled, so an attached volume reads creating forever. Cosmetic — the heartbeat gate keys on attached_server_id, not status — but it misled this investigation and will mislead the next. Filed as an idea; unrelated to this chain and needs its own provider-polling design.

Agent Preflight (Required)

  • Preflight completed before code changes

Classification

  • external-api-change
  • cross-component-change
  • business-logic-change
  • public-surface-change
  • docs-sync-change
  • security-sensitive-change
  • ui-change
  • infra-change

External References

Context7 was not available in this environment; official primary documentation and independent reference implementations were used instead, as .claude/rules/05 permits.

Cloudflare's numeric DNS error codes are not published in its API reference, so they were corroborated against five independent production consumers rather than a single source:

A live call to the Cloudflare API confirmed code is emitted as a bare JSON integer (never quoted), which is what the typeof === 'number' narrowing relies on.

Go os/exec (official source, 1.26.6) read directly to confirm Cmd.Wait() blocks on <-c.goroutineErr until every I/O-copier goroutine has finished — the guarantee the livenessWriter design depends on for safe buffer reads after cmd.Run() returns: https://github.com/golang/go/blob/master/src/os/exec/exec.go

Codebase Impact Analysis

  • apps/api/src/services/dns.tsupsertAppRouteDNSRecord, createNodeBackendDNSRecord, readCloudflareErrorDetail
  • packages/vm-agent/internal/server/health.go, vm_jobs.go (claimJob), server.go (inFlightJobs)
  • packages/vm-agent/internal/deploy/compose.go (livenessWriter), engine.go, engine_config.go
  • Call sites unchanged: deploy-release-callback.ts:306,328, node-lifecycle.ts:446, nodes.ts:373
  • Worst-case CF subrequests per hostname 2 → 4, only on a race; latency ceiling 60s → 120s, well inside the caller's 15-min idle budget.
  • No schema, migration, binding, or env-var changes.

Documentation & Specs

N/A: internal service behavior only — no public API, env var, or setup step changed. Process documentation added as .claude/rules/68-external-api-check-then-act.md and .claude/rules/53 §5c.

Constitution & Risk Check

Principle XI (No Hardcoded Values) — checked, and it changed the code. The retry bound was initially a bare attempt >= 1; review flagged it as the only unnamed retry limit in apps/api/src against 15+ named siblings in env.ts, so it is now DNS_UPSERT_RACE_MAX_RETRIES. It is deliberately NOT env-exposed: it bounds an internal recovery loop against a constraint that resolves in a single round trip, not a caller-facing policy. composeOutputRetentionBytes is likewise a named constant. The vm-agent idle timeout it interacts with (DEPLOY_APPLY_IDLE_TIMEOUT / DefaultDeployApplyIdleTimeout) was already env-configurable and is unchanged. CF_DNS_DUPLICATE_RECORD_CODES is a curated static table of provider error codes — data, not configuration.

Principle XIII (Fail Fast) — the tolerance is deliberately narrow so failing fast is preserved where it matters: only 81057/81058, only on the create path, bounded to one retry. 81053 (cross-type collision), auth, and quota errors all still throw, each with a dedicated control test.

Key risks and tradeoffs:

  1. Tolerating a conflict could mask a real problem. Mitigated by scoping to two codes on one path, and by controls proving neighbouring codes still surface. The residual risk is a future CF code that means "exists" but is not 81057/81058 — that would surface as an error, not silence, which is the safe direction.
  2. The dedup guard could wedge a job id and stop a node applying. Mitigated by releasing via defer, an idempotent release, and a test proving a retry of the same seq after completion is allowed. Worst case degrades to the pre-existing behaviour (an apply is skipped and re-advertised on the next heartbeat).
  3. The liveness signal could mask a genuinely hung compose. This is the real tradeoff: a compose that emits output forever but makes no progress would now never trip the watchdog. Accepted because the alternative — the status quo — kills every legitimately slow pull, which was actively breaking production; and because a silent hang still trips the timer (proven by the silent-command control test).
  4. Subrequest/latency growth. Worst case 2 → 4 CF subrequests and 60s → 120s per hostname, only on a race, well inside the caller's 15-minute idle budget.
  5. Not verified on real infrastructure. The largest residual risk, stated plainly in Staging Verification above: the vm-agent changes need a fresh node per .claude/rules/27 and cannot be exercised from here.

🤖 Generated with Claude Code

@defang-sam defang-sam Bot added the needs-human-review Agent could not complete all review gates — human must approve before merge label Sep 5, 2026
@defangdevs

Copy link
Copy Markdown

Triaged the failing CI run (33973251752) after the webhook notification. Four jobs failed; breaking down each:

1. Preflight Evidence — fixed. The PR body's Agent Preflight block was missing both the closing <!-- AGENT_PREFLIGHT_END --> marker and the entire ### Constitution & Risk Check section required by .github/pull_request_template.md. I added the missing section (Principles II/XIII/XI, plus the known tradeoff already described in "Known limitation" above) and the closing marker directly to the PR body — should go green on the next push/rerun.

2. Durable Object Workers — appears pre-existing/unrelated. tests/workers/project-data-tool-payload-archive.test.ts > retrieves archived tool payloads through the MCP tool got a 401 instead of 200. This diff touches only dns.ts and its own tests — nothing in the MCP tool-payload/R2-archival or auth path. No history to compare against on this branch (first CI run), but given the diff's scope this looks like the same class of flake the PR already documented for job 3 below.

3. Test — pre-existing, per your own validation notes. tests/unit/services/project-data-snapshot-recovery-wake.test.ts > allows a stopped ProjectData session to wake only with an authorized restorable claim (expected true, got false). The PR body already states this exact failure is pre-existing and unrelated, verified by stashing the diff and re-running — matches what CI hit here.

4. Secret Scan — unresolved, needs a human/reviewed-baseline call. pnpm quality:gitleaks:current reports 1 new finding not in scripts/quality/gitleaks-reviewed-baseline.json. I did not attempt to identify the specific string — scripts/quality/run-gitleaks.ts deliberately withholds finding details everywhere (report only exists in a discarded temp dir) precisely so a real secret can't leak through logs, and I'm not going to work around that from outside. I scanned this diff by eye for anything secret-shaped (the touched files are dns.ts, two test files, a new rules doc, and a new task post-mortem) and found nothing beyond the pre-existing CF_API_TOKEN: 'cf-token' test fixture, which is unchanged in this diff — so if that's the trigger it'd be a baseline/digest issue, not new. Whoever holds gitleaks-baseline review access needs to run the scan directly and either confirm it's a true positive (in which case this needs to stop before merge) or add its reviewed digest to the baseline.

Also flagging: the PR body itself already marks Staging Verification as not performed and explicitly defers the merge decision to a human — so items 2–4 aside, this PR isn't self-mergeable regardless of CI state.

I only edited the PR body (Preflight section) — no code changes, no merge action taken.

raphaeltm and others added 3 commits September 5, 2026 16:02
A deployment stalled with no app-route DNS record and no TLS certificate. The
control plane had logged exactly one error in the whole window: a 500 from
GET /api/nodes/:id/deploy-release, thrown by upsertAppRouteDNSRecord out of a
Promise.all, with Cloudflare's "An identical record already exists."

That endpoint upserts every app-route DNS record before returning the node's
release payload, so the throw meant the node never received its release and the
deployment stalled upstream of anything cert-related. The missing certificate
was three layers downstream of the actual failure.

upsertAppRouteDNSRecord is a check-then-act with an await in the gap. Its only
production call site fans it out over every route via Promise.all, and
overlapping release fetches run the whole handler concurrently. Two callers both
observe "no record", both POST, and Cloudflare rejects the loser with 81058.

Evidence this is a race, not a stale record:
  - the lookup is an exact ?type=A&name= match, so "not found, then rejected as
    duplicate" can only mean the record appeared between the two calls;
  - 81058 means same name AND type AND content (a cross-type collision returns
    81053), so identical content = same node IP = two identical concurrent POSTs;
  - deployment_release_events shows the handler running concurrently: two
    fetch_started 1s apart and seq=4 written twice by two invocations;
  - deleteAppRouteDNSRecord was already documented as tolerant of "a record
    already removed by a concurrent caller" — delete had been hardened for
    concurrency, create never was.

Fix: on the create path, treat the duplicate-record codes as a lost race —
re-resolve once and update in place. Scoped to 81057/81058 (81053 is a genuine
misconfiguration retrying cannot fix and still throws), to the create path only
(!existing), and bounded by DNS_UPSERT_RACE_MAX_RETRIES.

Also fixes the same class in createNodeBackendDNSRecord, in the same file, whose
failure mode was worse: a blind POST with no lookup at all. Two paths create that
record (services/nodes.ts provisioning and the node-lifecycle heartbeat
backfill); on conflict the caller stamps nodes.error_message and leaves
backend_dns_record_id NULL, so every later heartbeat retried the same losing POST
forever and node deletion — which deletes by that id — orphaned the real record.
It now resolves the winner so the id is persisted, converging the IP first since
81057 does not guarantee matching content.

readCloudflareErrorDetail reads {code, message} in one pass because a Response
body can only be consumed once; readCloudflareError delegates to it so the five
other call sites are unchanged. `code` is parsed as unknown and narrowed with
typeof at use — valibot's optional() only bypasses a missing key, so
v.optional(v.number()) would fail the whole entry on `code: null` and silently
replace Cloudflare's real message with a generic fallback for those five callers.

Process fix: .claude/rules/68-external-api-check-then-act.md. Its requirement 6
(check sibling operations in the same module) is what surfaced the
createNodeBackendDNSRecord instance.

Tests (dns-app-routes 14 -> 23, deploy-release-callback 31 -> 32) cover both
tolerated codes, the update-path guard, 81053 and auth controls, the null and
stringified code regressions, the single-retry bound, a same-hostname race
against a shared fake CF store, the Promise.all fan-out, five sibling tests, and
a route-level vertical slice. Each guard verified discriminating once and
reverted; notably, on pre-fix dns.ts the new route test fails while the other 31
route tests all pass — the existing suite could not observe the production 500.

Co-Authored-By: Claude <noreply@anthropic.com>
… pulls

Two further defects in the same incident: the cause of the DNS race above, and
the failure that was killing deployments outright.

1. Duplicate apply spawns

health.go spawned a new runDetachedDeploymentApply goroutine on every heartbeat
for every pending release, with no in-flight check. observed.AppliedSeq only
advances after a fully successful apply, so any release slower than one heartbeat
interval accumulated another concurrent apply per tick — each re-running the
whole control-plane fetch: re-decrypting secrets, re-minting a registry
credential, regenerating presigned artifact URLs, re-signing the payload, and
racing on app-route DNS creation.

Live evidence, env pr18-preview-3, one node, 13:53-15:19:
    16x deployment.apply.fetch_started
     8x deployment.apply.started
     8x deployment.apply.compose_up_started
     0x  compose_up_completed
An exact 2:1 fetch/apply ratio. The duplicate's Apply() is rejected by the
engine's applyMu.TryLock(), which is why applies are half the fetches — but the
fetch has already run, and the fetch is what upserts DNS. 16 chances to lose the
race on one environment in 85 minutes.

claimJob(jobID) is an atomic claim over a shared in-flight set, applied to the
apply path and the route-config path, which had the same defect. Released via
defer, idempotent, and safe on a nil map so existing test fixtures still work.

2. Compose SIGKILLed mid-pull by the apply idle watchdog

The apply is bounded by a 15-minute IDLE timer reset only by ApplyProgressEvents
— the same events that become deployment_release_events rows. `docker compose up`
emits exactly one (compose_up_started) and then nothing until it returns, so a
legitimately slow image pull was indistinguishable from a hung apply. That is the
8-starts/0-completions column above, cycling every ~14 minutes, and it is what
produced `compose up: ... signal: killed (stderr: db Pulling ...)`.

Compose streams pull/extract progress to stderr, so that output is now the
liveness signal: runCompose wraps stderr in a livenessWriter that pokes the
watchdog per write without persisting an event, mirroring newIdleProgressReader
which already does exactly this for artifact downloads in the same package.

The buffer retains the TAIL, not the head. Compose prints megabytes of progress
and then the actual failure (`manifest unknown`, `no such image`, `no space left
on device`) on its last lines, so head-retention would discard precisely the
diagnostic the buffer exists to preserve. Compaction is amortized — the buffer
may reach 2x the cap before trimming — so a multi-megabyte pull costs O(total)
copying rather than O(total x limit).

3. The diagnostic that hid it

health.go overwrote the accurate "deployment apply stalled: no progress for
15m0s" with the child's `signal: killed`, which is a consequence of our own
cancel. The stall is now primary with the child result as context, so a
self-inflicted timeout is no longer indistinguishable from an OOM kill.

Process fix: .claude/rules/53 gains requirement 5c — the mirror image of its
existing liveness-as-idleness trap. There a liveness column answered an idleness
question; here a progress feed could not observe the work it guarded. Same
family, so it extends rule 53 rather than becoming a new file.

Tests, all under -race: liveness fires from real child output with the right
env/seq; silent-command and out-of-apply controls emit nothing; stderr survives
wrapping; the signal continues past the retention cap; tail retention asserted on
content both as a unit and end-to-end through a child emitting ~200 KiB before
failing; setActiveApplySeq restores on nest; concurrent seq access is race-free.
Dedup: duplicate skipped, different seq allowed, claim released on completion,
route path guarded, exclusive under 64 goroutines, idempotent release, nil map.

Each guard verified discriminating once and reverted: disabling the dedup guard
fails exactly the duplicate test with both controls green; a plain stderr buffer
fails exactly the liveness test; head-retention fails exactly the two tail tests.

vm-agent: build, vet, gofmt clean on all touched files; 23/23 packages pass;
deploy and server pass -race.

Co-Authored-By: Claude <noreply@anthropic.com>
Full incident record: the three defects, the live production evidence (2:1
fetch/apply ratio, 8 compose starts with zero completions), the discriminating-
mutation table for every guard, the two bug classes and their process fixes, and
the limitations left standing — the unexplained 1-second spacing of the original
duplicate fetches, and the deployment_volumes.status staleness filed separately.

The log excerpt is formatted one field per line: with `source=api` adjacent to
the node ULID, gitleaks' generic-api-key rule matched the high-entropy
identifier. It is a public node id that already appears in URLs and logs, not a
credential, so the right resolution is to not trip the scanner rather than to
self-approve a reviewed-baseline entry (which requires a security owner and an
expiry).

Co-Authored-By: Claude <noreply@anthropic.com>
@defang-sam
defang-sam Bot force-pushed the sam/use-sam-mcp-tools-wkkamr branch from bb9a3de to a128f72 Compare September 5, 2026 16:08
@defangdevs

Copy link
Copy Markdown

Follow-up triage on the new CI run (33976248875) after the three commits pushed since the last check (7625486e, 9e43ad74, bb9a3ded).

Preflight Evidence — likely a stale-snapshot artifact, not a real content gap. scripts/quality/check-preflight-evidence.ts reads pull_request.body from $GITHUB_EVENT_PATH (the webhook payload captured when this run was triggered), not a live re-fetch of the PR. The current live PR body already satisfies both checks it's complaining about: External References contains "Context7" (Context7 was not available in this environment...) and "official docs" (Cloudflare DNS API (official docs): https://...), plus five https:// source URLs. My read: the body edit that added the closing AGENT_PREFLIGHT_END marker/Constitution section (previous triage comment) — or a later bot edit — landed in a way this run's event snapshot didn't capture. Re-running this job against the same run won't help (it reuses the same event payload); it should go green on the next push that re-triggers pull_request: synchronize.

Specialist Review Evidence — expected, not a bug. The PR now carries the needs-human-review label, and check-specialist-review-evidence.ts fails hard on that label by design ("A human must review and remove this label before merging"). This is the gate doing its job, consistent with the PR body's own Staging Verification section already marking itself not-mergeable pending a human call.

Durable Object Workers and Test — unchanged from the last run, same two failures already covered in the previous triage comment: the R2 tool-payload-archive 401 and the pre-existing wakeSessionForSnapshotRecovery flake the PR author already verified is unrelated (stash-and-rerun). Neither touches this diff's files.

Secret Scan — still unresolved, same finding as before with no new information (details are withheld by policy). Still needs whoever holds gitleaks-reviewed-baseline.json review access to run the scan directly and either confirm true-positive or add the reviewed digest.

No code or PR-body changes made this round — the actionable blockers (needs-human-review label, unperformed Staging Verification, the gitleaks finding) all resolve to the same conclusion as before: this needs a human decision before merge, not further automated iteration.

raphaeltm and others added 2 commits September 5, 2026 16:16
…napshot

attachEnvironmentVolumes wrote `attached.status` straight from the provider's
attachVolume response. Hetzner commonly still reports `creating` at that instant,
and nothing ever re-polls the row — the only other writer is the detach path. So
an attached, mounted, fully working volume read `creating` forever.

Observed in production on volume 01M1RWP6VA… (pgdata) while its own release
events showed volume_mount_completed and volume_mounts_verified — the mount
demonstrably worked. Nothing reads the column (every `.status` branch in the
consumers is on node.status or environment.status, and the heartbeat gate
deploymentVolumesReadyForNode keys on attached_server_id), so this is not a
functional bug. It is worse in a specific way: it is the single most obvious
"here is your stuck deployment" signal in the table, and it is false. It
misdirected this investigation, and would misdirect the next one.

Now persists the settled SAM-side fact — the attach call returned and we hold a
server id, so `attached` is true and stable. No migration and no constraint
widening: the column is plain TEXT with no CHECK (0069_deployment_volumes.sql:14)
and `attached` is already in the provider VolumeStatus union. The `failed`
semantics on the creation path are untouched.

Rule 57 at the storage layer: a remote-owned value written once and never
reconciled. Polling was rejected as disproportionate — the fact SAM needs is
knowable locally the moment attach returns.

Test asserts the settled status survives a provider still reporting `creating`;
verified discriminating (restoring the pass-through fails exactly it plus the
existing attach assertion). API unit suite 8053/8054, 0 collection errors.

Co-Authored-By: Claude <noreply@anthropic.com>
Both were reported as "pre-existing failures" in earlier CI runs on this branch.
They are not flakes and were never going to self-resolve — each hardcodes a
historical date and seeds fixture data relative to it, while the PRODUCTION code
it exercises checks that data against REAL wall-clock time with no injectable
clock. Both were guaranteed to start failing on a specific calendar date
regardless of any code change, and did.

1. project-data-snapshot-recovery-wake.test.ts

   NOW = 2026-08-26T21:10:00Z; seedSnapshot() sets expires_at = NOW + 7 days =
   2026-09-02. wakeSessionForSnapshotRecovery -> hasAuthorizedRestorableSnapshot-
   WakeClaim calls the latter WITHOUT passing `now`, so it defaults to
   `new Date()` -- real time. From 2026-09-02 onward `expires_at > now` is false,
   the claim row never matches, and the test's `.resolves.toBe(true)` fails.

   Fix: freeze the clock (vi.useFakeTimers + setSystemTime(NOW) in beforeEach,
   vi.useRealTimers in afterEach) so the production default resolves to the
   fixture's NOW instead of real time. All 7 tests in the file still pass.

2. project-data-tool-payload-archive.test.ts > retrieves archived tool payloads
   through the MCP tool

   The file's FIXED_NOW (2026-08-26) is used correctly everywhere else --
   threaded explicitly through runArchiveCleanup's `now`/`nowMs` for the
   archival business logic. But this one test also stamped an MCP token's
   createdAt with that same FIXED_NOW. validateMcpToken has NO injectable clock
   (`const now = Date.now()`, hardcoded by design -- it's an auth boundary) and
   rejects tokens older than DEFAULT_MCP_TOKEN_MAX_LIFETIME_SECONDS (24h). Once
   real time passed 2026-08-27 the token looked ~10 days old and every call 401'd.

   Fix: stamp the token with the real current time (`new Date().toISOString()`)
   instead of the archival fixture's frozen date -- the two clocks are
   independent and were wrongly conflated.

Both fixes verified discriminating (reverting each reproduces its original
failure). Full suites after: unit 8054/8054 (was 8053/8054), workers 761/761
locally (was 760/761). Test-only changes -- no production code touched.

Filed and resolved: idea 01M1S79XZMF4Q9H53MQTCEQM0M (originally mis-filed as
"pre-existing failures", corrected with the actual root cause).

Co-Authored-By: Claude <noreply@anthropic.com>
@defangdevs

Copy link
Copy Markdown

Follow-up triage on the latest CI run (33982812640, commit f9697cdc).

Good news: everything flagged as unresolved in the previous two triage comments is now green — Secret Scan, Durable Object Workers, and Test all pass on this run, and Preflight Evidence also passes (confirming that was indeed the stale-snapshot issue, not a real content gap).

Only Specialist Review Evidence still fails, for the same reason already noted: the PR carries the needs-human-review label, and that check fails hard on the label by design. Same conclusion as before — this PR is otherwise CI-clean and just needs a human to review, then remove the label (or decide the label should stay and staging verification still needs to happen per the PR body).

No code or PR-body changes made. Not touching the label or merging — that decision belongs to a human reviewer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-human-review Agent could not complete all review gates — human must approve before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants