More adversarial-testing fixes: race conditions, config schema validation, symlink cycles#163
Merged
zc277584121 merged 21 commits intoJul 6, 2026
Conversation
… --json Reset SIGPIPE to SIG_DFL at the start of main() on Unix so piping mfs output into head/grep/less exits quietly instead of panicking with a backtrace when the pipe closes early. Also wire the global --json flag through to `mfs serve logs`, which was silently ignoring it and always printing plain text; it now prints a JSON array of log lines like other --json commands do.
_PIPELINE_OKINDS was missing text_blob even though the producer dispatch table already mapped it to TextChunksProducer, so csv/json and other text_blob objects were classified indexable but silently never chunked or embedded — they landed with zero chunks and search_status=not_indexed with no error.
Probe ignored the global --json flag and always printed the terse type/ok/detail line, even though --help advertised the option and List already handled it correctly. Now probe prints the raw JSON envelope when --json is set, which also surfaces the target field so callers can tell which connector identity was actually probed.
The --help for `mfs connector update` claimed it was an alias for `mfs add <uri>`, but it only accepts --config/--json while add also supports --force-index, --since, --upload, --force-upload, and -y. Describe what update actually does instead of the false alias claim.
The s3 connector's read() accepted a range argument but never used it, so cat --range against an s3 object silently fell back to fetching and returning the whole thing. Stream the object body line by line and slice to [start, end), matching the file connector's approach.
Status ignored --json entirely and always dumped the raw JSON envelope, even though --help advertises --json as a distinct option and connector list already handles the split correctly. Now the default view prints one line per connector plus a job-count summary, matching connector list's formatting; --json is unchanged.
…connector Losing side of a first-registration race hit the UNIQUE(namespace_id, root_uri) constraint and the raw IntegrityError leaked past the endpoint as a bare 500 with SQL text in it. Catch it where sync_already_running already does the same thing and return a 409 connector_already_registered instead.
probe is meant for scriptable health checks, but it always exited 0 regardless of the result, so `mfs connector probe ... && echo healthy` reported healthy even on a genuine connection failure.
Two concurrent cancel calls on the same running job could both read the job as cancellable before either wrote, so both got cancelled:true back. Move the terminal-state check into the UPDATE itself and use its rowcount to decide the winner, matching the same guarded-write pattern already used for claiming queued jobs.
Concurrent `serve stop` calls both printed "stopped" even when one hit an already-dead pid, and concurrent `serve start` calls could both claim success even though the loser's process died from an address-already-in-use bind conflict, leaving a dead pid in the pidfile if it wrote last. `stop` now checks kill's actual exit status and says "already stopped" when it didn't signal a live process. `start` gives its own child a short grace window after the port test to reveal a losing bind attempt before trusting the probe and recording its pid.
…ing success Sending SIGTERM only proves the signal was delivered, not that the target died -- a process wedged in blocking (non-yielding) work can hold it pending indefinitely. Stop now waits up to 10s for the pid to actually go away (mirroring Restart's existing wait loop) before removing the pidfile and reporting success; on timeout it exits non-zero with an honest message and leaves the pidfile in place so status/start still see the still-alive process.
When the output directory for `mfs export` doesn't exist, the error was a bare "No such file or directory (os error 2)" with no hint about which path failed or that it was the local write step.
The example showed a {"pk":{"id":12}} wrapper that no connector
actually produces; real locators are flat, e.g. {"id":12} or
{"_id":"..."}.
The web connector doesn't parse or enforce robots.txt at all, but the Pitfalls section claimed it was "respected by default". Correct it to say what actually happens: every reachable page within max_pages gets crawled regardless of robots.txt.
Add and update printed a plain "job: <id>" line even with --json set, unlike probe and other subcommands that respect the flag. Now they print the raw JSON envelope when --json is passed.
Cancel printed a plain "cancelled: <bool>" line even with --json set, same bug as add/update had. Now it prints the raw JSON envelope when --json is passed.
serve status printed "running"/"not running" but always exited 0, unlike the systemctl is-active convention a status check is expected to follow (the whole point is being scriptable: `if mfs serve status; then ...`). job cancel's exit-0-on-no-op is left as is -- an earlier decision already judged a no-op cancel (target already reached a terminal state) as an honest, non-error outcome, and revisiting that wasn't part of this fix. Verified live: status against a genuinely running server still exits 0; against a scratch MFS_HOME with nothing tracked and no listener, prints "not running" and exits 1; against a scratch MFS_HOME with something listening on the probed port but no matching pidfile, prints the existing distinguishing message and also exits 1. cargo test (20/20) and cargo fmt --check both clean.
mfs connector add is a config-only subset (target + --config), not an alias -- it has none of mfs add's -y/--force-index/--since/upload flags, cost-estimate confirm flow, or local-path/upload decision logic. Corrected the --help text to describe the actual surface instead of a claim that fails at the clap layer the moment a user tries -y on it (confirmed: "error: unexpected argument '-y' found"). No behavior change -- mfs connector update's own doc comment never made the alias claim, so it needed no correction.
CONFIG_SCHEMA existed as a ConnectorPlugin class attribute (only FilePlugin declared one) but nothing ever read it -- there was no schema-level type/field validation anywhere in the config path. Three concrete failure shapes this let through: a numeric field given as a quoted string caused an internal TypeError deep in a `>` comparison while the job's top-level status still said "succeeded"; a bool where a string was expected surfaced a raw "'bool' object has no attribute 'decode'" from inside asyncpg/urlparse; an unrecognized field was silently accepted with zero warning, so a typo'd field name silently fell back to its default. Added ConnectorConfigSchema (connectors/base.py), a pydantic model base with extra="forbid" plus the cross-cutting fields every connector config may carry (credential_ref, its legacy `_credential_ref` alias, [[objects]] overrides) declared once. Defined real schemas for postgres/mysql/mongo/s3/web (the connector types with local, reproducible test coverage) and wired ConnectorFactory.validate_config into add(), probe(), and estimate() -- before any connection is attempted, matching each method's existing error contract: add/estimate surface a clean 400 config_invalid, probe returns ok=false like any other connect failure. FileConfig (pre-existing, a plain @DataClass) is deliberately left alone -- it was never actually enforced before, and retrofitting it risked its own regression for an unrelated field shape. A quoted number is coerced by pydantic's default lax mode (fixing case 1 without ever calling it invalid -- "100" unambiguously means 100); a genuinely wrong type or an unknown field is rejected with a single clean, named error line instead of a raw exception. Covered by 8 new ConnectorFactory unit tests (valid config passes, numeric-string coercion, wrong-type rejection, unknown-field rejection, credential_ref/legacy-alias/objects allowed, a schema-less connector is unaffected, non-dict/unknown-ctype are no-ops). Full suite (343 tests), ruff format --check, and ruff check all clean. Verified live against the real server: probe/add both reject with the new clean error instead of a raw 500; a numeric-string field is silently coerced, not rejected; all 7 real registered connectors are unaffected.
mfs tree's directory walk followed symlinks with no cycle detection at all -- a genuine symlink cycle (dirA/link_to_b -> dirB, dirB/link_to_a -> dirA) recursed indefinitely: -L 10 duplicated both files at every alternating level, -L 100000 hung the full length of a 20s timeout with zero output. The ingest/sync path was already safe (it doesn't follow symlinks into a cycle at all); only tree's walk was exposed. FilePlugin.list() now reports a symlink-resolved identity (dev:ino, survives symlink resolution where the path string itself doesn't) for every directory entry, threaded through Engine.ls() and the new LsEntry.real_id field. The CLI's tree/tree_json walkers track real_ids already seen across the whole traversal and stop descending into a repeat, printing it once (marked as a cycle in human output, tagged `"cycle": true` in JSON) rather than following it further. Connector types that don't supply a real_id are simply never flagged, same as before this fix. Covered by a new expand_tree_entries unit test (a two-node cycle stops after one repeat, doesn't recurse past it). Full suites clean: 343 Python tests, cargo test (21, incl. the new one), ruff format/check, cargo fmt --check. Verified live against a real reproduced symlink cycle: `-L 100000` now completes in ~0.02s (both human and --json output) instead of hanging to a 20s timeout; a real, non-adversarial file connector's tree/ls output is unaffected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up batch from continued adversarial/stress-testing (see #159 for the first round), covering job/server lifecycle races, connector correctness, and CLI consistency.
Concurrency / lifecycle races:
job cancelis now guarded against a concurrent double-cancel race.servecalls (start/restart) no longer lie about the outcome when they race each other.serve stopnow confirms the process actually exited before claiming success, instead of trusting the signal alone.addcalls racing to register the same new connector now get a clean error instead of a confusing failure.Config correctness (highest-impact fix in this batch):
add/update/probetime, before any connection is attempted. Previously there was no schema-level type/field checking anywhere: a numeric field given as a quoted string caused an internalTypeErrorwhile the job's status still said "succeeded"; a wrong-typed field surfaced a raw, unhandled exception from deep inside a driver library; an unrecognized field was silently accepted with zero warning. Added real schemas for postgres/mysql/mongo/s3/web and wired validation into the existingCONFIG_SCHEMAhook (previously declared but never enforced).Correctness fixes:
mfs treeno longer hangs on afile://connector containing a symlink cycle — directory entries now carry a symlink-resolved identity so the CLI's tree walker can detect and stop at a repeat instead of recursing indefinitely.--rangeinstead of always returning the full object.text_blobobjects are now correctly routed through the indexing pipeline.CLI consistency (
--json/ exit codes / help text):connector probe,connector add/update, andjob cancelnow honor--json(previously silently ignored on all three).connector probenow exits 1 when it reportsok=false;serve statusnow exits 1 when it can't confirm a running server — both previously always exited 0 regardless of outcome.serve logs --jsonnow emits real JSON instead of plain text, and handles SIGPIPE gracefully.connector update's misleading--helptext andconnector add's false "alias: mfs add" claim (it's a config-only subset, not a true alias).robots.txtby default — it doesn't currently parse or enforce it at all.cat --help's--locatorexample corrected;export's write-failure errors now include the destination path.--json) output formfs status.Test plan
cd server/python && uv run --extra dev pytest— 374 passed, 9 skipped (pre-existing@pytest.mark.live)cd server/python && uv run --extra dev ruff format --check src/ tests/andruff check src/ tests/— cleancd cli && cargo test— 21 passedcd cli && cargo fmt --all -- --check— cleancd cli && cargo build --release— builds cleantreefix, real registered postgres/mysql/mongo/s3/web connectors for the config-validation fix, live restart/race reproductions for the lifecycle fixes — documented per-commit