Returning a deferred marker traded one wrong signal for a worse one. A
redelivery reuses the original task id — Context.as_execution_options carries
task_id into the retry — so the duplicate's return marked the very id the
client polls as SUCCESS. /api/task_status reports celery's state verbatim and
the UI maps SUCCESS to "done", so the GraphRAG enable modal would announce a
finished build, rendered from a payload with no counts, while the run holding
the lease was still extracting.
Raise Ignore instead: celery records no state for the duplicate, so the task
id keeps whatever the holder sets and the poller keeps waiting. The autoretry
wrapper re-raises Ignore ahead of autoretry_for, so the wider
autoretry_for=(Exception,) on these tasks cannot turn it back into a retry.
Three findings from review, all on code this branch introduced.
apply_chunk was not replay-safe. commit() can report connection loss *after*
Postgres committed, and the reconnect retry then replays the write: _upsert_node
bumps doc_freq a second time and _add_edge inserts another row, since
graph_edges has no uniqueness constraint for a logical edge. The chunk's
graph_ingest_progress row is now written in the same transaction as the rows it
describes, and a replay that finds it already "done" returns (0, 0) without
touching the graph. Extraction drops its separate mark_chunk("done"): the
checkpoint and the graph can no longer disagree.
count_nodes swallows every query failure and answers 0, so extraction's
"fall back to the write count" handler could never run — a failed count after a
successful build reported an empty graph. count_nodes grows a strict mode that
re-raises; retrieval keeps the swallow, which is what routes a source to
ClassicRAG.
A zero edge weight was read as a full-strength link: `or 1.0` rewrote an
explicit 0 before the <= 0 filter. Only missing and null weights default now.
The same coercion sat in _ppr_scores, where it would have kept the ranker's
rule unreachable from the product path, so it is fixed there too.
Review feedback on _write_with_reconnect: an explicit return inside the loop
plus an implicit fall-through past it reads as a path that returns None. The
retry is exactly two attempts, so say so — first attempt, reconnect on
connection loss, second and final attempt — and every path now returns or
raises.
Also narrows the transitions hint to the (node, weight) tuples it holds, and
logs at debug when the power iteration hits its iteration cap instead of
returning the last iterate with no trace.
A task that runs longer than the broker's visibility timeout is redelivered
while its first run is still going. The idempotency lease correctly stops the
duplicate from doing the work, but the duplicate then re-queued itself once
per LEASE_TTL until celery ran out of retries and raised
MaxRetriesExceededError — so a perfectly healthy long task (a large graph
extraction is the one that found this) reported a task failure, with a
traceback, while the real run was still making progress next to it.
Catch the exhaustion and return a "deferred" result instead. A normal
deferral still re-queues: only the give-up path changes, and the lease
holder's dedup row is left untouched so its own completion still records.
Three ways a chunk disappeared from a graph with no way to tell:
A build checks out one connection and then spends minutes per chunk waiting
on the model, so the connection idles long enough for the server or a pooler
to drop it. The pool only validates a connection when it hands one out, and
this one was handed out at the start of the build, so the next write raised
"the connection is lost", the chunk was marked failed, and the build carried
on a chunk short. apply_chunk and mark_chunk now reconnect and retry once;
every statement they run is an idempotent upsert, so a replay cannot
double-write. Only connection loss retries — a bad statement still surfaces.
An unparseable model response marked the chunk failed and logged nothing at
all, so failed_chunks was the only evidence and it named no chunk. Both
failure modes now log the chunk id.
The summary's node count summed per-chunk upserts, so an entity appearing in
ten chunks counted ten times: it reported writes, not graph size. It now
reports the distinct node count, falling back to the write count only if the
count query fails.
_build_extraction_llm passed settings.LLM_PROVIDER with the resolved
extraction model id. Those two disagree in any deployment that leaves the
provider at its default: the model id comes from GRAPHRAG_EXTRACTION_MODEL
or LLM_NAME, while the provider stays "docsgpt" — the hosted public
endpoint, which does not serve it. The request is rejected, the shared
fallback answers instead, and the graph gets built by a different model
than the one configured, with nothing in the summary saying so.
Resolve the provider from the model registry (owner-scoped, so per-user
BYOM ids resolve too), fall back to settings.LLM_PROVIDER only when the
model is unknown, and take the API key for the provider actually dispatched
to rather than the generic settings.API_KEY. The effective provider is
logged, since a silent swap was the whole failure mode.
networkx.pagerank delegates to a scipy implementation, and scipy is not a
DocsGPT dependency — it only arrives transitively through the optional
docling extra. In a default install every graph retrieval raised
ModuleNotFoundError inside _ppr_scores, hit the per-source except, and
degraded to ClassicRAG: the graph was built and paid for, then never used,
with one ERROR line per source per query as the only signal.
Rank with a local power iteration over the same row-normalized transition
matrix: undirected edges normalized per endpoint, dangling nodes
redistributed along the restart vector, and the restart vector normalized
across the nodes the subgraph actually holds so seed mass cannot leak.
Parity with networkx is asserted while scipy happens to be installed in the
test env, and the retrieval path is exercised with the import blocked.
_redis_urls raises before any check runs and cli.main prints what it raises, so
every one of its five messages interpolated the URL it was given — password and
all — straight to stderr. They render it through _endpoint now.
_endpoint itself then echoed the whole path, so an over-long database number
came back at full length: 5132 characters of error for one bad setting. It caps
the path it renders, which protects every caller, since that string is written
into terminals, CI logs and error messages rather than reused as a URL.
Review follow-up. SCIM_ENABLED=true with no SCIM_TOKEN was only caught per
request, as a 503 from the SCIM routes. The auth group's model validator
now rejects that combination when Settings loads, the same way it rejects
AUTH_TYPE=oidc without its required settings, so the misconfiguration is
reported once at startup rather than on the first provisioning call. The
route-level guard stays as defence in depth.
CodeQL flags "host" in message as incomplete URL sanitisation. I removed that
shape from two assertions and introduced a third in the same commit; this takes
it out of the provider test and the remaining Redis one, comparing with what
_endpoint produced instead, which is the contract those tests actually mean.
- doctor printed OPENAI_BASE_URL raw, the third place a credential-bearing URL
reached the terminal; it goes through _endpoint like the others.
- dev.run left its output readers unjoined, so a child's last lines could be
lost on exit. The threads are kept and joined during teardown.
- logs read each file and then reopened it to follow, so anything written in
between appeared in neither. One handle now serves both.
- dev took --port straight from argparse: 0 would have served on an ephemeral
port while printing 0, and oversized values reach socket.bind. It goes
through _port_number first.
- doctor --redis-url overrode only the broker, so a stale result backend or
cache was still pinged and the flag looked broken. All three endpoints now
come from the URL given.
The docs site failed to build: a bare "<= 1" in the prose of the
generated page is parsed by MDX as the start of a JSX tag ("Unexpected
character '=' before name"). Constraints are rendered as code spans now,
where MDX leaves them alone, and a test rejects any bare <, { or } outside
a code span so a future description cannot reintroduce the failure.
Verified with a local next build of the docs site.
CodeQL flags `"host:port" in message` as incomplete URL sanitisation. It is a
message rather than a URL being authorised, so it is not a vulnerability, but
the check was red and comparing against the sanitiser's own output asserts the
real contract. The signal handler now says why it swallows: the child is
already gone, and shutdown must not fail on what it is cleaning up.
env.py sets no version_table_schema, so the table follows search_path. Pinning
public. in doctor's queries made them agree with each other and disagree with
alembic: on an install using another schema it reported no schema at all and
sent the user to migrate an already-migrated database. Both queries resolve the
table the same way alembic does now.
Both preflights passed for `dev --mock-llm --port 8090`: the port was free, and
then the mock and the API were each handed it. One child could not bind, and the
API was pointed at that port as its model server while trying to listen on it.
The mock starts first and the API and worker are pointed at it, but only the
API port was preflighted. A busy 8090 therefore showed up as a child exiting
once the rest were running, or as the API talking to whatever else was on that
port. --ui is deliberately left alone: vite.config.ts sets no strictPort, so
Vite moves to the next free port rather than failing.
urlsplit accepts redis://host:notaport/0; only parts.port raises, and it raises
on access rather than at split time, so the ValueError fell outside the try.
_check_redis catches the client error and then formats it through _endpoint, so
doctor ended with a traceback from inside its own error path.
Running outside a checkout and starting on a port something else holds are both
refusals a developer will meet, and neither had a test. The second also asserts
that nothing is spawned when the port is taken, which is the part that matters:
the guard runs before any child process exists.
Sanitising the URL in the message was not enough: the client's own error text
went into the detail too, and both psycopg and redis-py quote the URL they were
given. The reason is kept, the endpoint is kept, and the URL, username and
password are taken out of it.
The Redis test raised a generic error, so it asserted the password was absent
without ever exercising the path that leaked. Both tests now raise what the
clients actually raise.
The Redis check put the whole URL in its failure message. A managed Redis URL
carries user:password@host, so a failed ping printed the password to the
terminal and into any log or issue the output was pasted into. It names
scheme://host:port/db now, and falls back to naming no URL when the value
cannot be parsed at all.
The Postgres check asked to_regclass about public.alembic_version and then read
version_num through search_path, so another schema could answer with a
different revision, or the query could fail, and doctor would send you to run
migrations against a database that is already fine.
Review follow-up. EMBEDDINGS_POOLING became Optional[Literal["cls", "mean"]],
but the group-base rule that maps "", "None" and whitespace to None only
matched Optional[str], so EMBEDDINGS_POOLING= in a .env file would have
failed validation. The rule now also covers an Optional Literal whose
choices are all strings, which lets the AUTH_TYPE validator drop its own
copy of that handling.
_port_number was written so a hand-edited .env could not reach int() raw, and
then doctor did exactly that: a nonnumeric port ended the command with a
traceback rather than the message, in the one command whose job is to explain a
broken setup.
The checks were mocked wholesale, so the branching that produces each diagnosis
had never run: a database with no schema yet, one behind this version, one that
refuses the connection, and which of the three Redis URLs failed. Each of those
is the sentence a developer reads when something is wrong, so each is pinned.
_migration_head is tested against the packaged alembic.ini itself: it needs no
database, and it is the path resolution that breaks silently when files move.
Review follow-up. The per-group secret validators normalised a hand-picked
list of API keys, which left other optional credentials and overrides
(OPEN_ROUTER_API_KEY, S3 and Daytona keys, ELASTIC_PASSWORD, the OIDC
trio, connector client ids, MICROSOFT_AUTHORITY, MCP_OAUTH_REDIRECT_URI)
holding the literal "None" or "" a .env file spells "unset" with, so
truthiness checks and fallbacks downstream saw a value. One rule on the
group base replaces those lists: every Optional[str] field maps "", "None"
and whitespace to None and strips real values. Plain str fields are left
alone. The OIDC required-settings check therefore also rejects those
spellings.
EMBEDDINGS_POOLING is Literal["cls", "mean"] with case-insensitive
parsing; its consumer silently ignored anything else.
Bounds added where the consumer rejects or misbehaves on the value:
SCHEDULE_RUN_OUTPUT_RETENTION_DAYS and MESSAGE_EVENTS_RETENTION_DAYS (the
cleanup repositories raise on <= 0), EMBEDDINGS_DELEGATE_TIMEOUT, the
remote-device idle/pairing/invocation TTLs and CELERY_VISIBILITY_TIMEOUT
(> 0), REMOTE_DEVICE_CMD_QUEUE_TTL_SECONDS (> 605, the documented drain
deadline), GRAPHRAG_MAX_CHUNKS_FOR_EXTRACTION (>= 0; negative would slice
the pending list from the end).
The generated reference now renders generic type arguments
(dict[str, int] rather than dict).
`docsgpt up --native` installs services meant to outlive the shell. Development
wants the opposite, and until now it meant three terminals from the guide:
uvicorn, celery, and vite.
`docsgpt dev` runs this checkout's API and worker as children of one terminal,
both restarting when a file is saved, their output interleaved and labelled, and
Ctrl-C stopping them together. `--ui` adds the Vite dev server, `--mock-llm`
runs the bundled mock model so no API key is needed, and `--no-worker` leaves
the worker to your editor's debugger. Celery has no reloader of its own, so the
worker is wrapped in watchfiles when it is installed, and runs plain when it is
not.
Alongside it, the commands a dev loop keeps reaching for:
- `docsgpt doctor` checks what usually breaks a new setup: PostgreSQL answering
and its schema matching this version, Redis answering, a model provider being
configured, and the port being free.
- `docsgpt restart [api|worker]` bounces services without rewriting settings or
rerunning migrations, which `down` plus `up` did.
- `docsgpt logs -f` follows a native install instead of telling you to run
`tail -f` yourself.
- `docsgpt env set` applies itself to a running native install rather than
asking you to run `docsgpt up` again to change one value.
Two bugs found on the way, both older than this change:
- `docsgpt api --reload` watched the working directory, which in a checkout is
178,425 files: .venv, node_modules, and the indexes/ and inputs/ the app
writes to while ingesting, so the server restarted itself mid-request. It
watches the package now — 1,217 files.
- The VS Code "Flask Debugger" ran `flask run`, which serves only the WSGI app:
/mcp, the SSE streams and artifact downloads 404 under it. The guide warned
about this in prose while the debug config did it anyway. It runs uvicorn on
the ASGI app now, like production.
About 85 call sites read a setting as getattr(settings, "NAME", fallback),
each carrying its own copy of the default. Every one of those names is a
field with a default on the model, so the fallback could never apply to
the real settings object; it only masked drift. Two had drifted:
- OPENAI_PROMPT_CACHE_KEY defaults to True on the model but the reader
fell back to False, and two test stubs relied on that.
- SharePoint's MICROSOFT_AUTHORITY fallback to
https://login.microsoftonline.com/<tenant> never fired, because the
attribute always exists (as None), so MSAL got authority=None. The
connector now derives the tenant authority when the setting is unset,
as its test always assumed.
Four places read EMBEDDINGS_KEY straight from os.environ, skipping the
"None"/"" normalisation the model applies; they read the setting now.
Test stubs that replaced a module's settings with a SimpleNamespace list
every setting the code under test reads.
SAGEMAKER_REGION, SAGEMAKER_ACCESS_KEY and SAGEMAKER_SECRET_KEY survive
only as a fallback for the S3_* credentials. They carry
Field(deprecated=...) now, so any read emits a DeprecationWarning naming
the replacement and the generated reference shows the notice. The S3
store is the one sanctioned reader; it silences that warning locally
because it already logs its own operator-facing one when the fallback
is actually used.
DEFAULT_MAX_HISTORY was referenced nowhere. RETRIEVERS_ENABLED was read by
no code at all, while two docs pages described it as an enforced
allow-list; both the setting and those claims are removed.
The "AUTH_TYPE=oidc requires OIDC_ISSUER, OIDC_CLIENT_ID and
OIDC_FRONTEND_URL" check lived in app.py, so it only ran when the Flask
app was imported; a worker or script with the same misconfiguration
started fine. It is now a model validator on the auth group and runs
wherever Settings is loaded, with the same message.
DEPLOYMENT_TYPE, which app.py read straight from the environment to
decide whether a missing JWT_SECRET_KEY is fatal, is a documented
setting on the server group now, so it shows up in the reference like
every other variable the app reads.
Enum-like settings whose allowed values were only listed in a comment are
now Literal types, so a typo fails at startup with a message naming the
allowed values instead of falling through to a default with a warning
(or, for VECTOR_STORE, failing on first use):
AUTH_TYPE, VECTOR_STORE, STORAGE_TYPE, URL_STRATEGY, OCR_BACKEND,
OCR_ENGINE, SANDBOX_BACKEND, DOC_PARSER_ENGINE, TTS_PROVIDER, STT_PROVIDER
Each keeps a before-validator that strips and lower-cases the value, since
the registries that consume them already lower-cased at the use site, and
AUTH_TYPE maps the "None"/"none"/"" spellings a .env file carries to None
(it was the string "None" before, which only worked because nothing
compared against it). An empty TTS/STT provider still means "off".
LLM_PROVIDER stays a plain str because providers are plugin-extensible.
Containers are typed (dict[str, int], list[str], dict[str, Any]) instead
of bare dict/list, six fields that were Optional with a non-None default
are plain, and integer settings whose description already states a range
carry it as a constraint (ge=0 for "0 disables", ge=1 for counts that
cannot be zero, 0 < threshold <= 1).
The hand-maintained settings page documented 95 of 258 settings and
.env-template 42, and both drifted as fields were added. The field
descriptions now live on the model, so the reference is rendered from it:
python -m docsgpt.core.settings.reference --write
writes docs/content/Deploying/Settings-Reference.mdx, one section per
settings group with each field's type, default, constraints, aliases and
description. --check reports a stale page, and tests/core/test_settings.py
fails when the checked-in page no longer matches the definitions, so a
new setting cannot land undocumented.
test_settings.py also pins the composition contract: every group field is
a flat Settings attribute, no field is defined twice, every field has a
description, and the secret-normalising validator of every group is
applied (the case that a shared method name would silently drop).
The App Configuration page points at the reference instead of at
settings.py, and the reference is listed in the Deploying navigation.
docsgpt/core/settings.py had grown to 258 fields in one 600-line class,
touched by about two commits a week, with related settings scattered
(GitHub ingest caps inside the embeddings block, API keys in four places,
the OpenAI Responses knobs 100 lines from the other OpenAI fields).
It is now a package: one module per domain (auth, llm, embeddings,
retrieval, vectorstores, database, workers, ingestion, ocr, storage,
connectors, server, events, agents, guardrails, scheduler, sandbox,
speech), each a SettingsGroup owning its fields and validators, composed
by multiple inheritance into the same flat Settings class. Every
attribute name, type, default, alias and constraint is unchanged, so
settings.NAME reads, .env files and test monkeypatches all keep working;
the import path docsgpt.core.settings is the package. Settings.normalize_api_key
is kept as a classmethod for callers that reuse it.
The comment above or beside each field became its Field(description=...),
so the definitions are visible to tooling; the next commit generates the
docs reference from them.
Pitfall recorded for future groups: pydantic collects validators by
method name across the MRO, so two groups naming a validator the same
would silently keep only one. Each group's validator has a unique name.
parts.port returns 0 rather than raising, since 0 is inside the range it checks,
so the URL reached .env and the worker and cache had nothing to connect to.
urlsplit accepts an authority such as localhost:notaport or localhost:65536 and
urlunsplit rebuilds it verbatim; only parts.port raises, and nothing read it. The
unusable value reached .env, where the worker picked it up and failed to start
its broker, while `up` reported success because it waits only on the API health
endpoint.
The ASCII-digit check accepts any length, but since 3.11 Python refuses to
convert a digit string past its conversion limit, so a long one raised
ValueError straight through the CLI instead of the message every other
unusable URL gets.
The exemption asked whether any service of the install was running, so moving an
install onto a different port that something else held would pass the check and
then fail to bind, with the health poll answered by whatever owned that port —
the false success the check exists to prevent.
install.json carries the API port now, and a busy port is allowed only when it
is that port and the API service is running.
The Docker-stack check ran after logs/ was created, .env written and the
migrations applied, so a conflict left a migrated database and partial files
behind with no install.json — exactly the directory that down and uninstall
then refuse. It runs immediately after the port is resolved now.
Alongside it, `up --native` refuses a port it cannot bind. Neither service
manager confirms that the API bound, and /api/health carries no installation
identity, so a second install on the same port would have been answered by the
first and reported success while its own API was dead. An install re-running on
its own port is the exception, since its services are what hold it.
From the outside-diff findings on #2800:
- Service names are derived from the install directory. A service manager has
one namespace per user, so two installs in different --dir directories wrote
over each other's units and down, status and uninstall acted on whichever was
written last. The default install keeps the readable names; another directory
gets a digest suffix.
- `up --native` refuses when a Docker stack in another directory publishes the
same port: its API would answer the health check while these services failed
to bind. The check degrades quietly when Docker is absent, which is exactly
the machine a native install targets.
- A Redis database path is required to be ASCII digits: str.isdigit() is true
for characters int() then refuses.
- DOCSGPT_PORT from a hand-edited .env is validated before conversion, and the
error names where the bad value came from.
- Percent signs are doubled in systemd values, arguments and log paths, since
systemd expands specifiers in all of them.
urlsplit raises ValueError on input such as redis://[::1 , which nothing
converted, so a typo left native setup with a traceback rather than the message
every other unusable URL gets.
The three URLs were built by string surgery, so anything after the database
number was mangled rather than kept: rediss://host:6380/0?ssl_cert_reqs=required
came out as .../0?ssl_cert_reqs=required/0, and a URL carrying a query but no
database had /0 appended after the query. TLS and managed Redis endpoints
usually carry exactly those parameters.
The URL is split properly now, the three databases go in the path, and scheme,
credentials, host, query and fragment are preserved. A URL that cannot be
numbered this way — one that is not redis:// or rediss://, or that has
something other than a number where the database goes — is refused with a
message instead of being turned into something that merely looks like a URL.
Quoting cannot carry a newline into a unit file or a plist: the line ends and
whatever follows becomes another directive. Every value bound for a service
file — the working directory, the log path, environment names and values, and
the command arguments — is checked before any of it is rendered, on both
launchd and systemd.
`up --native` took --expose, --domain and --docling and did nothing with them.
Asking for network exposure and silently getting a loopback-only install, or
asking for docling and getting an install without it, is worse than being told.
Each now says what to do instead: a reverse proxy or the Docker stack for
exposure, and the docling extra for the parser engine. --expose local and
--no-docling already describe native mode, so they stay silent.
Both accepted a native install and then drove `docker compose` in a directory
with no compose file, so the user got "no configuration file provided" rather
than an explanation. They now refuse with what to do instead, and restore
refuses before it reads the archive or stops anything.
`open` built its address from stack.url, which honours DOCSGPT_BIND, while the
native units always bind 127.0.0.1: with a LAN bind it handed the browser an
address nothing was listening on. status had the same mismatch and was fixed
with it; both now go through one helper so they cannot drift apart again.
From the outside-diff findings on #2800:
- install.json is written before the services are installed and started. A
service that fails to start used to leave units behind in a directory that
status, down and uninstall no longer recognised as a native install, so
nothing could clean them up.
- systemd stop and removal propagate failures: `down` reporting success while
the unit still runs, or `uninstall` dropping the unit file and the record
while systemd still runs the service, is worse than an error. Removing a unit
that is already gone stays harmless.
- WorkingDirectory and each Environment value are quoted and escaped for
systemd. `--dir` takes a free-form path, and one with a space in it is not
hypothetical: this checkout lives in one.
- Native status checks and prints http://localhost:<port>, which is what the
units bind. With a LAN DOCSGPT_BIND it used to poll an address nothing
listened on and call a healthy install dead.
`upgrade` exec'd the bare name `docsgpt`, so after `python -m docsgpt upgrade`
in a virtualenv without the console script on PATH, os.execv failed with a
traceback. It now uses the same launcher the service units get, which is why
that helper is no longer named for native mode.
Alex