feat(compile): POST /compile — MQL5 source in, .ex5 out

[Marinski]

Adds POST /compile — MQL5 source text in, compiled .ex5 out — so a caller can build an EA without a Windows box, a MetaEditor install, or file access to the host.

Anyone developing an EA against this API currently has to compile out-of-band and copy the binary in by hand. This closes that gap, and pairs naturally with POST /backtest — compile, then test, over the same API.

Happy to change any of the contract below; it's the shape that fell out of our use, not a proposal I'm attached to. Everything here has been running on a real workload, and the measurements quoted are from that rather than from a bench.

Contract

POST /compile
{"source": "<.mq5 text>", "filename": "MyEA.mq5", "ea_version": "1.0.0"}

source is required. filename is cosmetic. ea_version is recorded in the log line so a compile can be correlated with a build.

Status	Body
200	{"ok": true, "ex5_base64": "...", "log": "...", "warnings": 0, "include_hash": "sha256:..."}
422	{"ok": false, "log": "<MetaEditor diagnostics>", "errors": 3}
400 / 401 / 413 / 500 / 504	{"ok": false, "log": "..."}

Two invariants the tests pin, because clients end up depending on them:

• Every response is JSON, including auth failures and timeouts — so a non-JSON body unambiguously means a broken host. This is why the /compile auth branch returns jsonify(...), 401 rather than abort(401), which would render Flask's HTML page.
• ok: true always carries a non-empty ex5_base64. The handler re-reads and verifies the artifact before claiming success.

Source text only. No caller-controlled paths, no compiler flags, no include uploads — filename is reduced to a bare stem, so ../../evil and C:\x\y.mq5 both become evil/y. Everything runs in a per-request temp dir that is removed on every exit path including timeout and crash.

Notable implementation details

Warnings are not failures. MetaEditor exits non-zero on warnings, so exit code alone would reject a perfectly good build. The handler parses the log's Result: N errors, M warnings line and treats errors, not exit status, as the verdict. A build with warnings returns 200 and a binary.

The log is UTF-16LE with a BOM. Decoding it as UTF-8 yields either mojibake or an exception depending on content. Decoded explicitly, with a latin-1 fallback so a malformed log degrades to unreadable rather than 500ing the request.

A missing #include is a 422, not a 500. It's a defect in the submitted source, and callers should not retry it.

Compiles are serialized across processes, not just threads — see Serializing MetaEditor below, which is where most of the review went. Concurrent callers wait; a caller that waits past its deadline gets a JSON 504 rather than a hung connection.

The include tree is the sharp edge

Most of the non-obvious work here is about one failure: a compile that succeeds against the wrong library. It returns ok: true with a valid binary, and nothing downstream can tell it apart from a correct build. Three mechanisms guard it, and each exists because the simpler version was wrong in practice:

The mirror is incremental, not a re-copy. compile_local_cache mirrors MetaEditor + Config + MQL5 to local disk, which is the difference between 29s and 1.3s when the terminals sit on a host-shared mount (MetaEditor64.exe is ~105MB and the page cache does not save you). The first version re-copied the whole tree on every process start — invisible with a handful of includes, and 103 seconds once the stock MQL5 Include tree (~260 files) was in place, landing directly in front of the first caller after every restart. Now only missing or changed files are copied, compared on size and whole-second mtime.

The mirror prunes. Copy-only left it one-way: a header deleted from the source stayed in the mirror and kept resolving, so #include <Gone.mqh> still compiled against a file nobody maintains. Pruning is guarded on a non-empty source walk — if the mount is unreachable the walk yields nothing, and pruning against that would delete the entire mirror over a transient failure.

The include tree is re-validated while running (INCLUDE_REFRESH_SECONDS, 60s). Resolving it once per process meant an edited .mqh was invisible until the next restart while compiles kept reporting success.

include_hash makes the remaining risk observable. It identifies the library a binary was built against — sha256 over relative paths and contents under the /inc: root. Computed from the tree the compiler actually read, never from the source it was mirrored from: if the mirror were stale, hashing the source would assert the build used a library it did not, which is worse than reporting nothing. A test pins that direction specifically. It caught a real divergence on its first run against a live host, which is how the pruning bug above was found.

Startup warm-up

Even with the mirror warm, the first compile after a restart pays MetaEditor's cold load — 30–55s on a busy host against ~2–3s warm, recurring on any host that restarts VMs automatically. With a local cache configured, the server compiles a throwaway EA in the background instead.

The gates matter more than the compile: delayed 180s, because the VM launches every terminal at boot and a MetaEditor run added to that contention slows the guest exactly when its health probe is most marginal; claimed once per host via O_CREAT|O_EXCL in the shared cache, because every API process exposes /compile and shares that directory, so ungated this starts one MetaEditor per terminal (twenty, on the host this was built for); and it takes the compile lock non-blocking, so a caller never queues behind a warm-up. The claim expires after an hour so a process killed mid-warm-up cannot disable warm-up permanently. Every failure is swallowed and logged — an optimisation must not be able to take the process down.

Concurrency guidance, corrected

docs/compiling.md originally said MetaEditor compiles take well under a second and left callers to their own concurrency. Both were measured warm on an idle host and neither survived a loaded one, so the docs now say plainly: compile one at a time.

Since the lock serializes them anyway, concurrency buys no throughput while stacking waits onto a fixed deadline. Measured, same EA, same host:

	total	outcome
5 concurrent	92s	one 504, waits of 24/47/72/90/92s
5 sequential	24.7s	all 200

Sustained parallel compiles also saturate the guest CPU hard enough to fail a short-timeout health probe, so a supervisor restarts a VM that was merely busy — turning a slow batch into an outage. That is how this was found; the probe-side fix is in #15.

scripts/config_helper.py is touched for the same reason: docs/compiling.md tells operators to raise nginx's 60s proxy_read_timeout, but the generator in this repo still emitted the default, so the advice only helped people running a hand-rolled proxy. Under a queue of compiles that produces an nginx HTML error page — breaking the JSON invariant above, and pre-empting the API's own JSON 504.

Serializing MetaEditor

MetaEditor is single-instance per installation directory, so two concurrent invocations against one install corrupt each other's output. The first version used a threading.Lock, which orders calls inside one process — while every mt5api process on a VM exposes /compile and, by default, resolves to the same install. Verified live: two separate OS processes, each holding its own empty lock, ran MetaEditor fully concurrently with no serialization at all.

So the lock is a file (O_CREAT|O_EXCL) in the toolchain directory, covering real compiles and the warm-up compile alike. Getting it right took three passes, because two different races hide in "expire a dead lock and take it over":

• Release side. Each holder writes a unique token; release reads it back and unlinks only while the token is still its own. Without that, a holder that finished late deletes whoever owns the lock now — admitting the second concurrent MetaEditor the lock exists to prevent.
• Reap side. Waiters poll, so the instant a lock expires they all judge it stale together, and a loser's os.remove can land after a winner recreated the file. Tokens do not help: the damage is done during acquisition, before anyone releases. The sweep claims the dead file by renaming it to a name only the caller knows (atomic on POSIX and Windows alike), verifies what it moved really is what it judged, and puts it back untouched if a winner recreated it in between.
• Staleness itself. A live holder refreshes its lock's mtime from a heartbeat thread while its subprocess runs, so the window is a count of missed beats (60s against a 5s beat) rather than COMPILE_TIMEOUT_SECONDS + 120. When it was the latter — the same value as the warm-up budget — a slow but live holder could be declared stale mid-compile, which is what opened the release race.

One more, found by auditing the above rather than by review: an abandoned lock that exists but cannot be read used to wedge the endpoint permanently, because the sweep bailed out whenever it could not identify the holder. stat and unlink need no read permission, so such a lock is now reaped if it is past the window, with a warning logged.

Four regressions pin these, each verified to fail when its own fix is removed: a late holder must not delete the lock that replaced its own; a stale sweep must not delete the lock that replaced the dead one; a live holder must not be reaped while it works; an unreadable abandoned lock must still be recoverable. The sweep race is forced deterministically rather than left to luck — a plain multi-process race reproduces it only sometimes, which I found out by writing that test first and watching it pass against the bug.

Mutual exclusion is also proven across real processes: spawn-started interpreters race for the lock on a barrier, from both an empty directory and a pre-staled lock, recording when they entered and left the critical section and asserting no two intervals overlap. And because exclusion now depends on the heartbeat, that is measured too: a holder keeps the lock for 75s — past the 60s stale window, covering the warm-up's longest possible hold — while three separate processes poll aggressively to reap it. Nothing steals it.

Auth

/compile accepts the normal api_token, so nothing changes for existing users.

It additionally accepts an optional compile_api_token, accepted only on this path — every other route falls through to the unchanged check against api_token and rejects it. The motivation: a build service that compiles untrusted source shouldn't hold a credential that can also place orders, close positions, or restart a terminal. Leave it unset and the feature is inert.

Config

All optional, env var or config.yaml, env wins:

Setting	Default	Purpose
compile_api_token	unset	Compile-only credential
compile_terminal_dir	terminals/metaquotes/base	Which terminal's toolchain to use
compile_include_dir	terminal's MQL5	/inc: root
compile_work_dir	temp	Scratch dir
compile_timeout	30s	Per-compile deadline, 60s ceiling
compile_local_cache	unset	Local toolchain mirror (see above)

Tests

94 tests in tests/test_compile.py; 658 passing overall, 3 skipped, with make test-integration at 25 passed and lint clean. Beyond the contract, the ones worth pointing at are the pairs that pin a decision in both directions: a warm mirror copies nothing on the next process start and an edited .mqh is still picked up; a deleted header is pruned and an unreachable source does not wipe the mirror; the hash follows the compiled tree and not the source; exactly one process out of twenty wins the warm-up claim and an abandoned claim expires.

Docs in docs/compiling.md, linked from README.md and docs/rest-api.md, plus a CHANGELOG.md entry and config/config.yaml.example block.

Not included

No compile queue or async job handle — the synchronous lock was sufficient at this volume, and a job API felt like a bigger decision than this PR should make. No PID-liveness check in the stale sweep either: it would make liveness authoritative instead of inferred from a heartbeat, but it needs Windows-specific process probing and the heartbeat measures out fine. No caller-supplied .mqh uploads: the include dir stays server-managed, since accepting arbitrary include trees from a caller reopens the path-safety surface this deliberately closes. No per-file include digests alongside include_hash — one tree hash answered the question that prompted it.

[psyb0t]

I tested the current endpoint implementation. The happy-path tests pass, but two boundary failures remain.

First, /compile has no source or output-size limit. It accepts the complete JSON body, writes source to disk, reads the generated .ex5 fully into memory, and Base64-encodes the full result into the HTTP response. There is no Content-Length guard, source-byte cap, output-byte cap, or tests for oversized input/output. A single authenticated request can therefore consume unbounded disk, memory, worker time, and response bandwidth.

Second, the broad exception handler returns the exception class and message directly to the caller. This leaks internal paths and implementation details whenever a compiler or filesystem error occurs. The existing RuntimeError("kaboom") test asserts only that log is a string, so it currently permits this leak.

Please add explicit, documented source and artifact limits, reject oversized requests before writing them, reject oversized artifacts before Base64 encoding, and cover both limits. Keep detailed exception context in server logs, but return a generic failure message to the caller.

[Marinski]

Both fixed in cd2755a.

1. Per-request size caps, enforced before the resource they bound is spent

Two documented settings, defaulting sane and clamped (not raised) on bad values — config.py is imported by the whole API, and a typo in an optional endpoint's tuning must not stop trading. Same call as the other numeric settings there, and the opposite of the watchdog's refuse-to-start, for the reason each file states.

• COMPILE_MAX_SOURCE_BYTES (default 2 MB). An oversized body is refused with 413 straight from its declared Content-Length, before parsing — the test proves the ordering by posting an oversized payload that is not even JSON: a 400 would mean the parser read it. The decoded source is then checked against the cap itself, before anything reaches disk; the refusal names the limit and the knob.
• COMPILE_MAX_EX5_BYTES (default 16 MB). The artifact is size-checked on disk, before it would be read into memory or base64-inflated into the response. A refusal carries no ex5_base64 at all — not a truncated one — and is a 500, not a 422: the caller's source compiled fine; the server is declining to return the result, and the log says which setting to raise.

Documented in docs/compiling.md (request table, new 413 section, config table) and the changelog.

2. The 500 body is generic now

{"ok": false, "log": "internal error"} — the traceback goes to the server log and only there. You were right that the existing RuntimeError("kaboom") test permitted the leak by asserting only that log was a string; the new test plants an internal path inside the exception message and asserts the response contains neither the class, the message, nor the path. The two other detail leaks on the 500 path went with it: the missing-MetaEditor message no longer echoes the absolute path, and a launch OSError (whose message is a path) is no longer forwarded.

Six new tests, all failing against the previous handler: the 413 fires before the compiler runs and before the work dir exists, the declared-length refusal precedes parsing, the artifact refusal carries no binary, within-cap requests are unaffected both ways, and the leak test above.

Also: evicted a stray file

scripts/prune-terminal-logs.sh had been swept into this branch's squash from unrelated local work. It is #18's file (byte-identical to that branch's copy, referenced by nothing here), and #18's review round has since fixed a selection bug in it — keeping a stale copy here would collide on merge and reintroduce the bug. Removed; this PR is compile-only again.

Merge order

#15 → #16 → #18 → #10. This second: #18 and #10 both build near this branch's mt5api/config.py / config_helper.py / changelog additions. I will rebase each successor promptly as its predecessor lands.

Full suite green (72 compile tests, 483 total offline), lint clean.

[psyb0t]

Thanks for taking the cross-process serialization further. I cannot approve this revision yet.

The new lock still has an ownership race that can reintroduce concurrent MetaEditor runs:

1. _CROSS_PROCESS_LOCK_STALE_SECONDS and the warmup budget are both COMPILE_TIMEOUT_SECONDS + 120. A slow but live holder can therefore be treated as stale while it is still in its critical section or subprocess cleanup.
2. A second process removes that path and creates its replacement lock.
3. The original holder eventually enters finally and _release_cross_process_lock() blindly removes the path. It has no owner token or other conditional ownership check, so it removes the second holder's lock. A third compiler can then enter concurrently with the second.

I reproduced that exact sequence against this head: acquire the first lock, make it stale, acquire a replacement, release the original owner, then acquire a third lock successfully.

Please make the lock ownership-aware. For example, write a unique owner token and unlink only when the token still belongs to the caller, keep a live holder fresh while the subprocess runs, and keep the stale interval safely beyond the maximum hold plus cleanup time. Add a real multi-process regression test, with synchronized separate processes, that proves no second compiler enters while the first is live. The current helper-level test is not a cross-process proof.

This PR is also currently conflicting with main, so please rebase and rerun the checks after fixing the race.

[Marinski]

You were right, and the sequence you reproduced is exactly what happened. Fixed, rebased onto master, and squashed to a single commit so the racy revisions are not in the history.

The release-side race you described. Each holder now writes a uuid4().hex token into the lock file, and release() reads it back and unlinks only while the token is still its own. A holder that finished late finds a foreign token and leaves the file alone, so it can no longer delete the lock that replaced its own.

The stale window. It is no longer derived from COMPILE_TIMEOUT_SECONDS. A live holder refreshes its lock's mtime from a heartbeat thread while its subprocess runs, so the window is a count of missed beats (60s against a 5s beat) rather than a guess about how long a compile may legitimately take. That removes step 1 of your sequence — a slow but live holder is no longer mistaken for a dead one. The heartbeat logs and retries a transient utime failure instead of silently giving up on liveness for the rest of the hold.

A second race in the same area, which your finding led me to. Tokens alone are not enough. Waiters poll, so the instant a lock does expire they all judge it stale in the same moment — and a loser's os.remove can land after a winner has already reaped and recreated the file, deleting the new owner's lock. Ownership tokens cannot catch that one: the damage happens during acquisition, before anyone releases. The sweep now claims the dead file by renaming it to a name only the caller knows (atomic on POSIX and Windows alike), verifies that what it moved really is the file it judged stale, and puts it back untouched if a winner recreated it in between.

On the tests, and a correction worth stating plainly. I first wrote the multi-process race exactly as you asked — real spawn-started interpreters, barrier-synchronised, recording their critical-section intervals and asserting no overlap. It passes. But when I reverted the reap fix to check it was actually load-bearing, it still passed — six processes doing reap-then-create simply do not hit the bad interleaving reliably. A test that cannot fail proves nothing, so the sweep race is now pinned by a deterministic regression that holds the window open explicitly (the loser is parked between judging and acting while a winner takes ownership).

Each of the three regressions is verified to fail when its own fix is removed:

• late holder must not delete the lock that replaced its own → fails without the token check
• stale sweep must not delete the lock that replaced the dead one → fails with a bare os.remove
• live holder must not be reaped while it works → fails without the heartbeat

The cross-process tests stay, now covering both an empty directory and a pre-staled lock. They are the mutual-exclusion proof across real processes; the deterministic one is what actually guards the race.

Rebased on current master (only the CHANGELOG and .gitignore conflicted).

make test-unit
  657 passed, 3 skipped, 80.84% coverage

make test-integration
  23 passed

make lint
  all categories clean

[Marinski]

Follow-up — head moved to a564eb2. A deeper audit of the lock found one more defect, which I had introduced in the revision you just read, so it is better you hear it from me than find it.

An abandoned lock that cannot be READ wedged the endpoint forever. The stale sweep began by reading the owner token and bailed out when there was none — which conflates "file is absent" with "file exists but is unreadable". The previous implementation used bare getmtime + os.remove, and neither needs read permission, so it recovered from this. Mine did not:

stat works: mtime age = 660s  (stale window 60s)
_read_lock_token -> None
after reap: stale unreadable lock still exists = True
can any compile acquire? False

Every future compile from every process sharing that install returns 504, permanently — the exact outcome the stale window exists to prevent. An existing-but-unreadable lock now falls through to the age check and is reaped if abandoned, with a warning logged. Regression test test_an_unreadable_abandoned_lock_is_still_reaped, verified to fail without the fix.

Worth noting how it hid: every other lock test reads a lock file it can read, so the whole suite, the review pass, and your own reproduction all exercised the readable path only.

Two smaller things from the same audit:

• Staleness compares wall-clock time.time() against mtime, so a forward clock step larger than the stale window would mark every live lock abandoned at once. ntpd slews rather than steps after initial sync, so I have documented the exposure in-code rather than defended against it. Say the word if you would rather it were defended.
• Your point that mutual exclusion now depends on the heartbeat is fair, so I tested it against the real runtime rather than arguing: one holder keeps the lock for 75s — deliberately longer than the 60s stale window, covering the warm-up's COMPILE_TIMEOUT_SECONDS + 120 hold — while three separate processes poll aggressively to reap it. The holder still owned it at the end and nothing stole it.

make test-unit: 658 passed, 3 skipped. make lint clean. make test-integration: 23 passed.