diff --git a/echo/AGENTS.md b/echo/AGENTS.md index ee3c2e706..784b76da7 100644 --- a/echo/AGENTS.md +++ b/echo/AGENTS.md @@ -116,6 +116,9 @@ See [docs/branching_and_releases.md](docs/branching_and_releases.md) for the ful - **Releases**: tagged from `main` every ~2 weeks → auto-deploys to production - **Hotfixes**: branch off release tag → fix → new release → cherry-pick back into `main` - Always check for Directus data migrations before deploying. See [docs/database_migrations.md](docs/database_migrations.md) +- **Config removals are release-gated.** GitOps `main` syncs to production in minutes, but production code only advances on a release tag. Remove the field from `settings.py`, ship the tag, then remove the key from gitops. See [docs/incidents/gitops-env-removal-release-gate.md](docs/incidents/gitops-env-removal-release-gate.md) +- Never `kubectl scale` / `kubectl patch` production. Argo self-heal reverts it. See [docs/incidents/argo-self-heal-reverts-manual-changes.md](docs/incidents/argo-self-heal-reverts-manual-changes.md) +- Adding a service to the Helm chart? Add its image to the `ci.yml` build matrix in the same change. See [docs/incidents/agent-missing-from-build-matrix.md](docs/incidents/agent-missing-from-build-matrix.md) ## Architecture @@ -155,6 +158,7 @@ Long-running progress streams via Server-Sent Events backed by Redis pub/sub (re - `SkipRetryOnUnrecoverableError` middleware skips retries for `TypeError`, `SyntaxError`, `AttributeError`, `ImportError`, `NotImplementedError` - To invoke async code from a Dramatiq actor: `run_async_in_new_loop` from `dembrane.async_helpers`. Never `asyncio.run` (clashes with nested loops) - Wrap blocking I/O in async endpoints with `run_in_thread_pool` from `dembrane.async_helpers` (Directus, service-layer, S3, token counting). Don't wrap already-async calls (e.g. `rag.aquery`) +- `run_async_in_new_loop` is a misnomer: it submits to ONE long-lived loop on a real OS thread, it does not make a loop. Never create/close a loop per call. Why these rules exist: [docs/incidents/dramatiq-actor-event-loops.md](docs/incidents/dramatiq-actor-event-loops.md) ### LLM Model Groups @@ -184,6 +188,8 @@ Which group powers which feature is non-obvious, so don't downgrade silently. 3. Pull the schema: `cd directus && bash sync.sh -u http://directus:8055 -e admin@dembrane.com -p admin pull` 4. Commit the snapshot JSON under `directus/sync/snapshot/`. That is the source of truth; the one-shot migration script does not need to be committed +Narrow exception: a field's `schema.is_indexed` must be `true` in the snapshot whenever the column is indexed on production, even though a `pull` from a local database writes `false`. Getting this wrong aborts the entire `schema/apply` on production and nowhere else. See [docs/incidents/directus-sync-is-indexed.md](docs/incidents/directus-sync-is-indexed.md) + ### Python DirectusClient - `create_item` / `update_item` return `{"data": {...}}`. **MUST** unwrap with `["data"]` diff --git a/echo/docs/branching_and_releases.md b/echo/docs/branching_and_releases.md index 62651777b..94218f3c7 100644 --- a/echo/docs/branching_and_releases.md +++ b/echo/docs/branching_and_releases.md @@ -39,7 +39,7 @@ Until we can fully synchronize the Directus schema push and server deployment, p 2. **Pre-release checks**: - Check for new env vars (`settings.py` fields, `config.ts` exports) - Run Directus data migrations if needed (see [database_migrations.md](database_migrations.md)) - - Update deployment env vars in the GitOps repo if needed + - Update deployment env vars in the GitOps repo if needed. **Adding** a key is safe any time; **removing** one is release-gated, because GitOps `main` reaches production immediately while the code does not. See [incidents/gitops-env-removal-release-gate.md](incidents/gitops-env-removal-release-gate.md) 3. **Tag and release** — create a GitHub release from a commit on `main` 4. The release triggers **auto-deployment to production**: - Backend: new image tags are picked up by the GitOps repo (`dembrane/echo-gitops`, Argo CD auto-sync) diff --git a/echo/docs/incidents/README.md b/echo/docs/incidents/README.md new file mode 100644 index 000000000..75c2c2417 --- /dev/null +++ b/echo/docs/incidents/README.md @@ -0,0 +1,37 @@ +# Production incidents + +Short write-ups of production failures whose cause was not obvious from reading the +code. Each page leads with the rule, then explains the mechanism precisely, what the +fix was, and how to avoid a repeat. + +These are engineering notes, not reports. No severities, no timelines, no blame. Where +timing matters, use `git log` / `git blame` on the commits cited rather than trusting a +date written here. + +## When to add a page here + +Add one when a failure meets both tests: + +- the cause would surprise a competent reader of the code, and +- the preventing rule is not already stated somewhere a person would look + +If the rule already lives in an `AGENTS.md`, link to that rule from here and link back +from the rule. Do not restate it in two places. + +## Index + +| Incident | Rule in one line | +| --- | --- | +| [gitops-env-removal-release-gate.md](gitops-env-removal-release-gate.md) | Never remove config from gitops before the code that reads it has shipped to a production release tag. | +| [argo-self-heal-reverts-manual-changes.md](argo-self-heal-reverts-manual-changes.md) | Never `kubectl scale` or `kubectl patch` production. Argo reverts it. Change capacity in gitops values. | +| [directus-sync-is-indexed.md](directus-sync-is-indexed.md) | A snapshot field with `is_indexed: false` over a live column indexed under a different name aborts the whole `schema/apply` on production. | +| [agent-missing-from-build-matrix.md](agent-missing-from-build-matrix.md) | Every deployment referencing an image tag needs a build-matrix entry that produces that tag. Nothing else notices when one does not. | +| [dramatiq-actor-event-loops.md](dramatiq-actor-event-loops.md) | Never create and close an event loop per call inside a worker. Go through `run_async_in_new_loop`, which owns one long-lived loop. | +| [validate-execution-not-construction.md](validate-execution-not-construction.md) | A dependency migration is validated by running the real path end to end, never by confirming imports and constructors work. | + +## Related + +- [../branching_and_releases.md](../branching_and_releases.md): environments, release tags, gitops +- [../database_migrations.md](../database_migrations.md): Directus schema and data migrations +- `../../AGENTS.md`: the cross-cutting rules these incidents produced +- `../../echo-gitops/AGENTS.md`: infrastructure notes, human-notes zone at the bottom diff --git a/echo/docs/incidents/agent-missing-from-build-matrix.md b/echo/docs/incidents/agent-missing-from-build-matrix.md new file mode 100644 index 000000000..c9e3c12f9 --- /dev/null +++ b/echo/docs/incidents/agent-missing-from-build-matrix.md @@ -0,0 +1,91 @@ +# echo-agent was never built for production + +**Rule: every deployment that references an image tag needs a matching entry in the +`ci.yml` build matrix that actually produces that tag. Nothing in the pipeline notices +when one is missing, and an internal ClusterIP service can stay broken indefinitely +without a single external symptom.** + +## What happened + +The `echo-agent` deployment in the `echo-prod` namespace sat in `ImagePullBackOff` for +weeks with no ready pods and therefore no service endpoints. The agentic chat backend +was unavailable in production for the entire period. + +## Mechanism + +Two workflows build images, and they tag differently: + +- `.github/workflows/deploy-testing.yml` built `dbr-echo-agent`, tagging it with a + short 7-character SHA plus `testing` +- `.github/workflows/ci.yml`, job `ci-build-and-push-servers`, built only + `dbr-echo-directus` and `dbr-echo-server`, tagged with the full 40-character + `github.sha` + +Production and Echo Next both deploy by full commit SHA, so their agent deployments +requested `dbr-echo-agent:`. That tag was never produced by anything, because +the only workflow that built the agent used the short form on a different branch. The +registry's newest agent tags were `testing` and a short SHA, both stale. + +Nothing failed loudly: + +- CI was green. It builds the images in its matrix and has no knowledge of what the + Helm chart references. +- Argo reported the application synced. The manifests matched git exactly; the + container simply could not pull. +- The service is `ClusterIP` on port 8001 and is not on any ingress. Production ingress + exposes only `directus.dembrane.com` and `api.dembrane.com`, and the dashboard and + portal front ends are on Vercel. So every external health check stayed green. + +The gap between "declared in the chart" and "produced by CI" had no owner, and the one +component affected happened to be invisible from outside the cluster. + +## Fix + +`dbr-echo-agent` was added to the `ci.yml` matrix in +`feat(agentic): foundation slices — agent image on main, v2 authz, design doc` +(d9e245cf, PR #745). It is present today, with the reason recorded inline so the entry +does not get pruned as redundant: + +```yaml +# Agent service. Also built by deploy-testing.yml for the testing +# env; without this entry no main-SHA agent tag ever exists, so the +# echo-next and prod agent deployments ImagePullBackOff. +- name: dbr-echo-agent + # Repo-root context: the image bakes docs/ as the agent's + # read-only knowledge corpus. + context: . + dockerfile: echo/agent/Dockerfile + tag: dbr-echo-agent +``` + +The build context is the repository root, not `echo/agent/`, because the image bakes +the docs corpus in as the agent's read-only knowledge base. + +Production only picks this up on a release tag, so the deployment stays broken until a +release containing d9e245cf goes out. + +## Preventing a repeat + +- When adding a service to `helm/echo/`, add its image to the `ci.yml` matrix in the + same change. The chart reference and the build entry are one unit. +- When adding an image to `deploy-testing.yml`, check whether `ci.yml` needs it too. + The two workflows serve different environments and neither implies the other. +- For an internal service with no ingress, add a check that fails visibly. Green + external probes prove nothing about a `ClusterIP` workload. + +A quick audit at any time: + +```bash +# image repositories the chart references +grep -rn "repository:" echo-gitops/helm/echo/values.yaml +# images CI actually builds +grep -n "tag: dbr-" .github/workflows/ci.yml +# what exists in the registry +doctl registry repository list-tags dbr-echo-agent +``` + +## See also + +- [gitops-env-removal-release-gate.md](gitops-env-removal-release-gate.md): the same + code-versus-config skew, from the other direction +- [../agentic-chat-design.md](../agentic-chat-design.md) diff --git a/echo/docs/incidents/argo-self-heal-reverts-manual-changes.md b/echo/docs/incidents/argo-self-heal-reverts-manual-changes.md new file mode 100644 index 000000000..edd931255 --- /dev/null +++ b/echo/docs/incidents/argo-self-heal-reverts-manual-changes.md @@ -0,0 +1,67 @@ +# Manual kubectl changes to production get reverted + +**Rule: never `kubectl scale` or `kubectl patch` a production workload. Argo CD +self-heal reverts it to the git-declared state within minutes. Capacity changes go +through `helm/echo/values-prod.yaml` in `echo-gitops`.** + +## What happened + +During a transcription backlog, `kubectl scale deploy echo-worker-cpu --replicas=10` +was run against the production cluster. Argo reset the deployment to its declared +replica count on the next sync. Nothing about the backlog changed, and the time spent +looked like progress while producing none. + +## Mechanism + +`argo/echo-prod.yaml` declares: + +```yaml +syncPolicy: + automated: + prune: true + selfHeal: true +``` + +`selfHeal` means Argo continuously compares live cluster state against the rendered +manifests from gitops `main` and reconciles any difference it finds, whatever produced +it. A hand-edited replica count or resource limit is drift, so it is undone. `prune` +means a live object with no manifest behind it is deleted outright. + +The behaviour differs by resource: + +- deployments without an HPA (Neo4j, for example) have their `spec.replicas` reverted + hard +- deployments with an HPA are managed by the autoscaler within the bounds declared in + gitops, so a manual scale is either overwritten by Argo or immediately re-evaluated + by the HPA, and in both cases the bounds in git win + +## What to do instead + +Change the declaration and let Argo apply it: + +1. Edit replicas, resources, or HPA bounds in `helm/echo/values-prod.yaml` (or the + relevant template under `helm/echo/templates/`). +2. Commit to gitops `main`, push, and watch the sync. + +Note the ceiling before assuming a replica bump helps: the production node pool sits +at its Terraform limit, so raising replicas past available capacity yields `Pending` +pods with `FailedScheduling: Insufficient cpu/memory`. Real headroom comes from +Terraform (a larger pool) or from reclaiming capacity by removing an unused workload +through gitops. + +## What is still fine by hand + +Self-heal only reconciles declared object state, so diagnosis is unaffected: + +- read-only `kubectl get` / `describe` / `logs` +- `kubectl exec` into a running pod, including a one-off enqueue or a backfill script +- `kubectl port-forward` for Loki or Grafana + +Deleting a pod is also fine when the intent is a restart: the replica set recreates it +and the declared state never changed. + +## See also + +- [gitops-env-removal-release-gate.md](gitops-env-removal-release-gate.md): the other + half of the same fact, that gitops `main` is production +- `echo-gitops/AGENTS.md`, recipe "Tune worker CPU scaling" diff --git a/echo/docs/incidents/directus-sync-is-indexed.md b/echo/docs/incidents/directus-sync-is-indexed.md new file mode 100644 index 000000000..c7ea062cf --- /dev/null +++ b/echo/docs/incidents/directus-sync-is-indexed.md @@ -0,0 +1,89 @@ +# directus-sync push fails on production with a DROP INDEX for an index that does not exist + +**Rule: if a column is indexed on production, the snapshot field file must say +`is_indexed: true`. A `false` there against a live index named by any other convention +makes Directus emit a `DROP INDEX` for a name that was never created, and the entire +`schema/apply` aborts.** + +## What happened + +`cd echo/directus && bash sync.sh ... push` against production returned a 500: + +``` +drop index "conversation_chunk_timestamp_index" - index "conversation_chunk_timestamp_index" does not exist +``` + +Nothing applied. The same push succeeded against local and testing. + +## Mechanism + +`sync.sh push` ends in `POST /schema/apply`. Directus builds the diff by introspecting +the live database and comparing it against the committed snapshot under +`echo/directus/sync/snapshot/`. + +Directus introspection reports `is_indexed: true` for a column if *any* index covers +it, regardless of that index's name. Production carries hand-applied performance +indexes using an `idx_*` naming convention (for `conversation_chunk.timestamp`, indexes +such as `idx_chunk_conv_timestamp` and `idx_chunk_recent_non_upload`). Those indexes +are not represented anywhere in the repository, so a `pull` from a database that lacks +them writes `is_indexed: false` into the snapshot. + +That leaves the diff as `is_indexed: true -> false` on production, which Directus +translates into "drop the index on this column". The knex layer does not look up which +index actually exists. It generates the name from its own convention, +`{table}_{field}_index`, so it emits +`DROP INDEX "conversation_chunk_timestamp_index"`. No such index exists. Postgres +errors, and because `schema/apply` runs in a single transaction the whole push rolls +back, including every unrelated collection and field change in the same snapshot. + +Local and testing never hit this: they have no `idx_*` indexes, so the column is +genuinely unindexed, snapshot `false` matches live `false`, and no index statement is +generated at all. The failure is production-only by construction. + +## Fix + +Set `is_indexed: true` in the snapshot field file. For +`conversation_chunk.timestamp` this is +`echo/directus/sync/snapshot/fields/conversation_chunk/timestamp.json`, done in +`fix(directus): mark conversation_chunk.timestamp as indexed in schema snapshot` +(4635762b, PR #724): + +```json +"schema": { + ... + "is_indexed": true, +``` + +Source `true` matches live `true`, the diff disappears, and no index DDL is generated. +The file is committed with `is_indexed: true` today. + +This is one of the narrow cases where editing snapshot JSON by hand is correct. The +standing rule in `AGENTS.md` is never to hand-write these files, because they should be +produced by `sync.sh pull` after running an idempotent migration script. That rule +still holds for structure. It cannot hold here, since a `pull` from any environment +without the production perf indexes reintroduces the `false`. + +## What not to do + +Creating `conversation_chunk_timestamp_index` on production so that the `DROP` has a +target does clear the immediate 500, but the push then drops it while the `idx_*` +indexes keep the column indexed. The next `pull`/`push` cycle regenerates the same +mismatch, so the failure returns on every sync. + +## Applying it to the next field + +When a push fails on production with `drop index ... does not exist`: + +1. Read the table and column out of the index name in the error. +2. Confirm the column really is indexed on production + (`\d ` or a query against `pg_indexes`), under whatever name. +3. Set `is_indexed: true` on that field's snapshot file and push again. + +Directus system tables are not affected. Only `directus_users` is present in the +snapshot; `directus_activity`, `directus_revisions` and `directus_presets` are not, so +their indexes never take part in `schema/apply`. + +## See also + +- `../../AGENTS.md`, "Directus Rules (Critical)" +- [../database_migrations.md](../database_migrations.md) diff --git a/echo/docs/incidents/dramatiq-actor-event-loops.md b/echo/docs/incidents/dramatiq-actor-event-loops.md new file mode 100644 index 000000000..aea4e8a35 --- /dev/null +++ b/echo/docs/incidents/dramatiq-actor-event-loops.md @@ -0,0 +1,98 @@ +# Event-loop corruption in Dramatiq actors + +**Rule: never create and close an asyncio event loop inside a worker, and never call +`asyncio.run` from an actor. Go through `run_async_in_new_loop` in +`dembrane.async_helpers`, which owns one long-lived loop on a real OS thread. For +concurrency without async, use `gevent.pool.Pool` plus `dramatiq.group()`, on the +`network` queue only.** + +The standing rules are in [`../../AGENTS.md`](../../AGENTS.md), "Dramatiq & Async +Rules". This page exists to record why they are worded the way they are, because the +short version ("no asyncio in actors") reads as superstition until you know what broke. + +## What happened + +`task_summarize_conversation` was failing at close to 100% in production: conversations +reached `is_finished` and `is_all_chunks_transcribed` but their `summary` stayed +`NULL`. `task_merge_conversation_chunks` failed alongside it. Two different exceptions: + +- merge raised `RuntimeError: Event loop is closed` +- summarize raised `sniffio.AsyncLibraryNotFoundError` + +## Mechanism + +`run_async_in_new_loop` originally did what its name says: it created a fresh asyncio +loop per call, ran the coroutine, and closed the loop. Two consequences, one per +symptom. + +**Closed loops orphan pooled clients.** The process-global async httpx client +(`async_directus`) binds its connection pool to whichever loop first used it. Once that +loop is closed, the pool's transports are dead, and the next call through the same +client raises `Event loop is closed`. + +**Ephemeral loops under gevent corrupt sniffio's detection.** asyncio's notion of "the +running loop" is thread-local, and under `dramatiq-gevent` all greenlets in a worker +share one OS thread. Greenlets interleaving across short-lived loops left that +thread-local state inconsistent, so `sniffio` could not identify which async library +was running and raised `AsyncLibraryNotFoundError` inside library code that had no +reason to care. + +## Fix + +`fix(workers): repair summary/merge (asyncio-in-gevent), scheduler Redis drops, NUL +transcript writes` (10b9d25b, PR #712) replaced the per-call loop with a single +loop that runs for the lifetime of the process on a dedicated real OS thread; work is +submitted with `asyncio.run_coroutine_threadsafe`. Because the loop never closes, httpx +pools correctly and sniffio always sees a genuinely running loop, so `nest_asyncio` is +not needed on that path. The thread must be a real OS thread, obtained via +`gevent.monkey.get_original`, so its selector blocks only that thread and never the +gevent hub; gevent (>= 25) yields cooperatively while a greenlet waits on the +cross-thread `Future`. See `_ensure_background_loop` and `_run_async_once` in +`server/dembrane/async_helpers.py`. + +`fix(workers): don't take nest_asyncio fallback under gevent` (12650b0a, PR #718) +closed a follow-on trap that the first fix introduced. PR #712 added a fallback: if +`asyncio.get_running_loop()` reports a loop, run nested on it via `nest_asyncio`, which +is right for the FastAPI server. Under `dramatiq-gevent` it misfires for exactly the +reason above, because one greenlet sees a loop that a different greenlet is driving, +drives it too, and the actor hangs until `TimeLimitExceeded`. The fallback is now gated +on `not _is_gevent_patched()`. The general lesson: `get_running_loop()` is not a +reliable answer to "am I inside async code" under gevent. + +Later work (acf32619 PR #816, da0e7f16 PR #825) added self-healing: an +`AsyncLibraryNotFoundError` crossing `run_async_in_new_loop` resets the async clients +and the background loop and retries once, provided the caller passed a zero-argument +coroutine *factory* rather than a coroutine object. Coroutine objects are consumed by +their first await and cannot be retried. Prefer passing a factory. + +## What the rules mean now + +The function is still called `run_async_in_new_loop`, which is now a misnomer: it runs +on the shared loop, not a new one. Do not read the name as permission to make your own. + +- **"No `asyncio` in Dramatiq actors"** means no bare `asyncio.run`, no + `new_event_loop()`, no per-call loop lifecycle. It does not mean async code is banned + from workers; `run_async_in_new_loop` is the supported route and is widely used. +- **`gevent.pool.Pool` is safe on the `network` queue only.** That queue runs under + `dramatiq-gevent`; the CPU queue runs standard dramatiq with no monkey-patching, so a + gevent pool there blocks a real thread instead of yielding. `report_generation.py` + fetches transcripts through a pool and its actor is declared + `queue_name="network"` for precisely this reason. +- **`gevent.sleep()`, not `time.sleep()`, in network-queue actors.** Monkey-patched + `time.sleep` usually yields, but relying on the patch being in place makes the actor + silently blocking wherever it is not. +- **Never reuse a loop-bound client across loops.** This is what turned a small loop + bug into a total outage rather than a slow path. + +## Validating a change here + +gevent behaviour depends on monkey-patching happening before imports, so it cannot be +asserted from a normal test process. Run gevent paths in a subprocess that patches +first. Deploy to Echo Next and watch a real queue drain before cutting a release: PR +#718's hang was caught that way and not by any unit test. + +## See also + +- [`../../AGENTS.md`](../../AGENTS.md), "Dramatiq & Async Rules" +- [`../../server/AGENTS.md`](../../server/AGENTS.md), "Background task design" +- [validate-execution-not-construction.md](validate-execution-not-construction.md) diff --git a/echo/docs/incidents/gitops-env-removal-release-gate.md b/echo/docs/incidents/gitops-env-removal-release-gate.md new file mode 100644 index 000000000..20573b483 --- /dev/null +++ b/echo/docs/incidents/gitops-env-removal-release-gate.md @@ -0,0 +1,96 @@ +# Removing an env var from gitops before the code ships + +**Rule: config removals are release-gated. Delete the field from `settings.py`, ship +that code on a `v*.*.*` release tag, and only then remove the key from +`echo-gitops`. Leaving a stale key in gitops costs nothing.** + +## What happened + +On 2026-07-02 two feature-flag fields were deleted from `server/dembrane/settings.py` +on `main` (`refactor(chat): remove auto-select in favor of agentic chat`, 8d543c27). +The matching env keys were then removed from `helm/echo/values*.yaml` in the gitops +repo (`chore(env): drop removed feature flags (auto-select, DISABLE_REDACTION)`, +gitops 2da4daa / PR #30). + +Production was still running release `v2.0.5` (1e0362e1), which predates the settings +change and still declared both fields. Argo synced the gitops change to production +within minutes. Every worker, the scheduler, and the new API replica set failed +pydantic settings validation at boot and crash-looped. Transcription stopped. The API +kept serving only because the previously running pods were not restarted. + +Recovery was a straight revert of the gitops commit (gitops 066ff21 / PR #32); Argo +self-heal re-synced and the pods came back. + +## Mechanism + +Two deploy channels move at different speeds and they are easy to conflate: + +| Channel | Trigger | Reaches production | +| --- | --- | --- | +| Application code | GitHub release tag on `main` | every ~2 weeks | +| Deployment config (`echo-gitops` `main`) | merge to `main` | within minutes, all clusters | + +`argo/echo-prod.yaml` sets `targetRevision: main` with `syncPolicy.automated` +(`prune: true`, `selfHeal: true`), and so do the dev and testing apps. There is no +per-environment branch. A config change merged to gitops `main` therefore lands on +production against whatever release tag is currently deployed, which may be weeks +behind `main`. + +When a required (no-default) field is removed from the environment, pydantic-settings +does not fall back to a default: it raises. In this case Helm still templated the env +name with an empty value, so the failure surfaced as a `bool_parsing` error on `''` +rather than a missing-field error. Either way the process cannot start, and because +workers and the scheduler restart aggressively, the whole background pipeline goes +down while the older API pods make it look partially healthy. + +## Why the opposite direction is safe + +Every settings class in `server/dembrane/settings.py` is configured with +`extra="ignore"`: + +```python +model_config = SettingsConfigDict(env_file=".env", extra="ignore", case_sensitive=False) +``` + +An env var present in the environment but no longer declared as a field is silently +ignored. So the asymmetry is total: + +- config present, code does not read it: harmless +- config absent, code requires it: crash loop + +That makes "leave it in gitops until after the release" a free action, and "remove it +early" the only risky one. + +## The rule in practice + +1. Remove the field from `settings.py` and merge to `main`. Echo Next picks it up. +2. Cut the release tag. Confirm production is running it. +3. Only then remove the key from `helm/echo/values*.yaml` and + `helm/echo/templates/_helpers.tpl` in gitops. + +Removing from gitops straight away is fine in exactly two cases: the key only exists +in `values.yaml` / `values-testing.yaml` (dev and testing track `main` anyway), or the +field is optional-with-a-default in the image production is currently running. + +When unsure, read the deployed code rather than guessing: + +```bash +git show :echo/server/dembrane/settings.py | grep -n +``` + +The same gate applies to renames, which are a removal plus an addition, and to any +required key consumed by the Directus or agent deployments. + +## Current state + +The revert is still in place. `DISABLE_REDACTION` and +`FEATURE_FLAGS__ENABLE_CHAT_AUTO_SELECT` are set in `helm/echo/values.yaml`, +`values-testing.yaml` and `values-prod.yaml`, and templated in `_helpers.tpl`, even +though neither field exists in `settings.py` on `main` any more. That is correct and +deliberate: production still runs `v2.0.5`, which requires them. They can be dropped +from gitops once a release containing 8d543c27 is live. + +## See also + +- [../branching_and_releases.md](../branching_and_releases.md), "Release Process" +- [argo-self-heal-reverts-manual-changes.md](argo-self-heal-reverts-manual-changes.md) diff --git a/echo/docs/incidents/validate-execution-not-construction.md b/echo/docs/incidents/validate-execution-not-construction.md new file mode 100644 index 000000000..da73f2412 --- /dev/null +++ b/echo/docs/incidents/validate-execution-not-construction.md @@ -0,0 +1,62 @@ +# A dependency migration validated by construction, not execution + +**Rule: validate a dependency or SDK migration by running the real path end to end and +asserting on the output. Imports resolving and objects constructing prove nothing. If +the change is a protocol or serving-interface change, the only honest test is a real +request.** + +## What happened + +The agent's `copilotkit` dependency was upgraded from 0.1.78 to 0.1.94 in +`feat(agent): upgrade copilotkit to 0.1.94 (AG-UI migration)` (79afc324, PR #754). + +Validation consisted of probing the new API surface and confirming that +`LangGraphAGUIAgent` imported and constructed cleanly. On that basis the migration was +judged small and shipped. It failed at runtime with +`AttributeError: no attribute 'execute'`, because the serving and execution interface +had changed, not the constructor. Two broken deploys to Echo Next followed before the +upgrade was reverted in +`revert: copilotkit 0.1.94 AG-UI upgrade (#754) — incompatible protocol` (28558302, +PR #756). `echo/agent/uv.lock` still resolves `copilotkit` to 0.1.78; the +`pyproject.toml` constraint is only `>=0.1.77`, so the lockfile is what holds the line. +Do not regenerate it casually. + +The same pattern had already appeared once that day, on the agent image: it imported +fine locally, and the container path bug and dependency-lock problems only showed up on +a real boot and a real request. + +## Mechanism + +Construction and execution are separate surfaces, and a major version bump can leave +one intact while replacing the other. Constructing an object exercises `__init__` +signatures and import graphs. It does not exercise the methods a server calls, the +event shapes a client parses, or the wire protocol between them. 0.1.94 was an AG-UI +protocol migration spanning several layers, so the parts that changed were exactly the +parts a construction check cannot reach. + +The check was not weak by accident. It was reassuring, quick, and produced a green +result, which is how it substituted for the harder test. + +## The rule in practice + +Before merging a dependency bump or a protocol change: + +1. Run the real entrypoint. Build the graph against a stub model and stream an actual + request through the endpoint. +2. Assert on the output: event types and ordering, status codes, the response body. + Not "no exception raised". +3. For anything that ships in a container, build and boot the container and send it a + request. Dockerfile, lockfile, and path problems are invisible to a host-side + import. +4. Keep the check. Turn it into a regression test so the next bump inherits it rather + than repeating the reasoning. + +The useful question for sizing a migration is not "did the API surface change" but +"which layers does this touch". A change across serving, protocol, client, and event +schema is not a version bump regardless of how the version number reads. + +## See also + +- [dramatiq-actor-event-loops.md](dramatiq-actor-event-loops.md): the same lesson from + the worker side, where the hang in PR #718 was caught by a real deploy and by no test +- [../agentic-chat-design.md](../agentic-chat-design.md) diff --git a/echo/server/AGENTS.md b/echo/server/AGENTS.md index 64d232996..8d4694ef4 100644 --- a/echo/server/AGENTS.md +++ b/echo/server/AGENTS.md @@ -34,6 +34,8 @@ When fixing or extending Dramatiq flows: - L3 `task_catch_up_unsummarized_conversations` (~5 min) → `is_all_chunks_transcribed=True AND summary=null` → summarize 4. **TEXT and AUDIO conversations share the same state machine**; both must converge to the same flags +The event-loop rules for actors live in @../AGENTS.md, "Dramatiq & Async Rules". The production failures behind them (closed-loop httpx pools, sniffio under gevent, the `nest_asyncio` fallback misfiring across greenlets) are written up in [../docs/incidents/dramatiq-actor-event-loops.md](../docs/incidents/dramatiq-actor-event-loops.md). Read it before touching `dembrane/async_helpers.py`. + ## Worker tuning - CPU Dramatiq worker uses **1 thread per process** to cap memory (FFmpeg can be hungry). Scale via processes/replicas, not threads