Skip to content

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

Merged
psyb0t merged 3 commits into
psyb0t:masterfrom
Marinski:feat/compile-endpoint-upstream
Sep 30, 2026
Merged

psyb0t merged 3 commits into
psyb0t:masterfrom
Marinski:feat/compile-endpoint-upstream

Conversation

@Marinski

@Marinski Marinski commented Aug 19, 2026 •

Copy link
Copy Markdown
Contributor

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

psyb0t commented Aug 25, 2026

Copy link
Copy Markdown
Owner

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
Marinski force-pushed the feat/compile-endpoint-upstream branch from 6956f86 to cd2755a Compare August 27, 2026 06:24
@Marinski

Copy link
Copy Markdown
Contributor Author

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

psyb0t commented Sep 12, 2026

Copy link
Copy Markdown
Owner

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
Marinski force-pushed the feat/compile-endpoint-upstream branch from 252a1b5 to 838af07 Compare September 12, 2026 06:43
@Marinski

Copy link
Copy Markdown
Contributor Author

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
Marinski force-pushed the feat/compile-endpoint-upstream branch from 838af07 to a564eb2 Compare September 12, 2026 07:05
@Marinski

Copy link
Copy Markdown
Contributor Author

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.

@psyb0t

psyb0t commented Sep 29, 2026

Copy link
Copy Markdown
Owner

I reviewed head a564eb2. The filename handling, the argument-list subprocess calls and the separate compile token are well done, and tests/test_compile.py passes. I can't approve it yet. The points below are from reading the code paths on this head, and the config ones I also reproduced with focused tests in the project's test image.

1. The compile-only token can block the trading API

waitress runs 32 threads per terminal (mt5api/main.py:33), shared by every route. A compile request waiting on _COMPILE_LOCK holds one of them for up to COMPILE_TIMEOUT + 30s, 90 seconds by default (compile.py:934). A caller holding only the compile token can send 32 concurrent compiles and tie up every thread, so orders, positions and /ping wait behind them. That undoes the point of a token that "cannot trade". Please bound the waiting compiles, for example a small semaphore that answers 429 beyond one or two waiters, so compile traffic can never take the whole pool.

2. The cross-process lock can end up with two owners

In _reap_if_stale, a reaper renames the lock file away before checking whether it moved the dead lock it judged or a new live one. If it moved a live lock, the path stays empty until it restores it (compile.py:240). A third process can create its own lock in that gap with O_EXCL. The restore then sees the path exists and gives up (compile.py:245), leaving the first owner's lock stranded as .dead. The first owner never learns it lost the lock, so both processes run MetaEditor at once. Please close the gap, for example by never renaming a lock whose token you have not already confirmed as the stale one.

3. One failed read ends the heartbeat for the rest of the hold

_beat returns for good when _read_lock_token() does not match the owner token (compile.py:153), and that includes None from a single failed read. The comment below it says a transient Windows sharing violation or antivirus scan must not end liveness, but that handling only covers os.utime. After one bad read, the lock goes stale 60 seconds later while the compile is still running, and the warm-up holds it for up to COMPILE_TIMEOUT + 120s. Please treat None as "retry next beat" and stop only on a positive read of a different token.

4. include_hash is cached forever in the default setup

With compile_local_cache unset, which is the default, _refresh_mirrored_includes returns immediately (compile.py:563), so nothing ever clears the cached digest (compile.py:458). The include tree is the terminal's live MQL5 folder, so after anyone edits a header, every compile in that process keeps reporting the old hash. With a shared local cache, a refresh done by another process does not clear this process's cached value either. The docs promise the hash describes the tree the compiler actually read. Please compute it per compile, or invalidate it on the tree's mtime.

5. Config parsing (reproduced)

  • A malformed COMPILE_TIMEOUT, for example abc, raises ValueError from parse_duration_to_seconds while config.py is imported (config.py:297). The whole API then fails to start, trading included, over one optional compile setting. Your own comment on the byte limits says a typo there must not stop trading.
  • A bare number is read as hours: COMPILE_TIMEOUT=10 becomes 36000 seconds and is clamped to 60.
  • COMPILE_MAX_SOURCE_BYTES or COMPILE_MAX_EX5_BYTES of 0 or below becomes 1024 (config.py:319), which refuses almost every real EA.

None of these log anything. Please fall back to the default with a warning on bad values, and parse the timeout as seconds.

6. ea_version goes into the log unbounded

ea_version is taken from the body as-is (compile.py:918) and written with %s into the plain-text log (compile.py:984). A newline in it writes a forged log line, and with the body cap at about 4x the source cap it can put several MB into the log per request. Please restrict it to a short, simple string.

7. The shared include cache is written outside the cross-process lock

_run_compile calls _local_toolchain(), which can copy and prune the shared mirror (compile.py:960), before it takes the cross-process lock (compile.py:991). With compile_local_cache pointing at a cache shared across processes, another process's MetaEditor can read headers while they are being rewritten.

8. The warm-up claim is never released

_claim_warmup (compile.py:744) creates the claim file and nothing removes it, so it only expires after WARMUP_CLAIM_TTL_SECONDS (3600, compile.py:366). With the default reboot_interval: 30 minutes, the claim from one boot is still fresh on the next, so the warm-up is skipped every other boot.

9. Behavior changes that are not in the changelog

  • scripts/config_helper.py:263 and :292 set proxy_read_timeout 300s in the per-VM location block, so every route through nginx now waits up to 300 seconds, not only /compile.
  • Every API process now schedules a MetaEditor warm-up at boot.
  • The cross-process lock, the include digests and COMPILE_INCLUDE_DIGESTS are not mentioned either.

10. No tests for the source content itself

The tests cover a hostile filename, but nothing checks what the MQL5 source can reach. #include and #resource directives in caller-supplied source are resolved by MetaEditor, not by this handler. Please add tests on a real MetaEditor showing that source cannot pull in or embed files from outside the temp directory and the configured include tree, and document the result in the threat model.

The branch also conflicts with master now, so it needs a rebase. Please fix these, then post the focused regression output.

Marinski and others added 2 commits September 30, 2026 01:29
MetaEditor is the only thing that can produce an .ex5 and it only runs on
Windows, which this stack already has. Anything that generates or patches
EA source elsewhere -- CI, a code generator, a web app, an agent -- can
now get a binary back over HTTP instead of putting a human on an RDP
session.

Source text only: no caller-supplied path anywhere. `filename` is reduced
to a bare stem and re-suffixed, so "../../terminal64" cannot escape the
per-request temp directory; /log: and /inc: are computed here, never
taken from the caller. The handler cannot trade, cannot restart a
terminal, and never touches the MT5 SDK. A second compile-only credential
(`compile_api_token`) lets a caller that only needs to build hold a token
that cannot also place orders. Source and artifact are both size-capped
and exception detail is kept out of responses.

Serialization is the substance of this change. MetaEditor is
single-instance per installation directory, and a threading.Lock only
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 their own empty lock ran
MetaEditor fully concurrently. So real compiles and the warm-up compile
alike take an O_CREAT|O_EXCL lock file in the toolchain directory.

Getting that lock 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 a second concurrent MetaEditor.
  * 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, deleting the new owner's lock. Tokens do
    not help: the damage is done during acquisition, before any release.
    The sweep therefore claims the dead file by renaming it to a name
    only the caller knows (atomic on POSIX and Windows alike), verifies
    the file it moved really is the one 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.
    The heartbeat logs and retries transient utime failures rather than
    silently giving up on liveness for the rest of the hold.

Tests: tests/test_compile.py covers the response contract, the path and
token hardening, the size caps, and the lock. Three regressions pin the
races specifically -- 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, and a live holder must not be reaped while it works -- and
each is verified to fail when its own fix is removed. The sweep race is
forced deterministically rather than left to luck, because a plain
multi-process race reproduces it only sometimes. 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. tests/compile_lock_worker.py exists so that
target module is importable in a fresh interpreter without pulling in
mt5api before the MetaTrader5 stub is installed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- At most two compiles wait behind the running one; the rest get an
  immediate 429 with Retry-After, so the compile-only token cannot occupy
  every waitress thread.
- The cross-process lock is an OS byte-range lock (msvcrt on Windows, flock
  elsewhere) on a file that is never deleted. The OS releases it when the
  holder exits or is killed, so the heartbeat, stale window and rename-based
  reaper are gone, and with them the two-owner race and the heartbeat that
  one failed read ended.
- The mirror is refreshed inside the cross-process lock, which now lives in
  the local cache when one is configured.
- include_hash and include_files are keyed on a stat signature of the tree,
  so an edit in the terminal's MQL5 folder or a refresh by another process
  shows up on the next compile.
- compile_timeout: a bare number is seconds; invalid or <= 0 falls back to
  30s and above 60s is lowered, both with a warning. The byte caps use the
  same positive-integer fallback as the other limits. Nothing raises at
  import.
- ea_version must be 1-64 characters of A-Z a-z 0-9 . _ + -.
- The warm-up claim records the boot it was made in, so every boot warms
  once regardless of reboot_interval.
- nginx gives only the /compile location a 180s timeout; other routes keep
  the default.
- #include, #resource and #property icon paths that are absolute, UNC,
  walk up with .., or name a device are refused with a 400 before
  MetaEditor runs. Measured on MetaEditor build 5836: #include read any
  file by absolute path or .. walk and quoted its tokens back in the log,
  and #property icon read .. walks. tests/real_compile/ checks this against
  a deployed endpoint.
- POST /compile is in the MCP unifier's endpoint catalog.
- CHANGELOG entries rewritten, including the warm-up, lock, digests and
  nginx changes.
@Marinski
Marinski force-pushed the feat/compile-endpoint-upstream branch from a564eb2 to be851ac Compare September 29, 2026 23:02
@Marinski

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Rebased onto master (3749da0) and all ten points are fixed in one commit on top: be851ac. The original commit is unchanged apart from the conflict resolution in CHANGELOG.md and server.py. In server.py, /compile skips the new generic _refuse_oversized_body(): it already refuses from Content-Length (411/413) in its own {ok, log} shape, and the generic 4 MiB cap would have refused a legal source that JSON escaping took past it.

1. Compile traffic can no longer take the thread pool

A BoundedSemaphore admits the running compile plus MAX_WAITING_COMPILES = 2 waiters, taken without blocking. Anything beyond gets an immediate 429 with Retry-After: 5 and never holds a waitress thread. The slot is released on every exit path, including exceptions.

2 and 3. The cross-process lock is now an OS lock

I replaced the file-existence lock rather than patching the reaper. .compile-inflight.lock is opened O_RDWR|O_CREAT and byte 0 is locked with msvcrt.locking(LK_NBLCK) on Windows and flock(LOCK_EX|LOCK_NB) elsewhere. The file is never deleted. The OS drops the lock when the holder's handle closes, including on kill, so the heartbeat, the stale window, the token and the rename-based reaper are all gone. Nothing judges a lock stale any more, so there is no rename gap and no heartbeat to stop. Contention (EACCES/EAGAIN/EDEADLOCK) is silent; any other error is logged as "unusable".

On real Windows, I ran the new module in our Windows guest (Python 3.12.7), with 4 processes started together. The first took the lock and was killed with os._exit(9) while holding it:

Lock file on holders after the kill overlaps kill → next holder warnings
local disk C:\ 3 0 0.297 s 0
SMB share (\\host.lan\Data) 3 0 0.067 s 0

The Linux multi-process tests cover the same two cases (4 spawned interpreters, and one killed mid-hold).

4. include_hash follows the tree

The digest cache is keyed on (root, signature). The signature is (relative path, size, mtime_ns) for every file in the tree, recomputed on each compile: a few hundred stats, not a re-read. An edit in the terminal's live MQL5 folder with no local cache, or a refresh of a shared mirror by another process, produces a new hash on the next compile. include_files uses the same key.

5. Config parsing

  • compile_timeout: a bare number is seconds (10 is 10s), and units still work (45s, 1m). A zero, negative or unparseable value falls back to 30s with a warning. A value above the 60s ceiling is lowered to 60 with a warning. Nothing raises at import.
  • compile_max_source_bytes / compile_max_ex5_bytes now use the same _positive_int_setting as the other limits: 0 or below, or a non-integer, gives the default plus a warning instead of 1024.

6. ea_version

It must be 1–64 characters of A-Z a-z 0-9 . _ + -; anything else is a 400 before anything runs.

7. The mirror is written under the cross-process lock

_run_compile and the warm-up now take the cross-process lock first and only then call _local_toolchain(). So copying, pruning, compiling and hashing all happen under one hold. The lock file sits in compile_local_cache when one is set, so every process sharing the mirror contends on it. Without a cache it sits beside MetaEditor64.exe as before. The old deadline extension for mirror time is gone, since the mirror now runs inside the lock.

8. Warm-up claim

The claim file now holds psutil.boot_time(). A claim from another boot (±60s for Windows' uptime-derived boot time) is replaced, so each boot warms exactly once whatever reboot_interval is. A claim in the old timestamp format counts as another boot's.

9. nginx and the changelog

config_helper.py no longer adds 300s to every terminal location. Each prefix gets an extra exact-match location = <prefix>compile with a 180s read/send timeout: the handler's worst case at the 60s ceiling is 60 + 30 waiting, plus 60 compiling. Every other route keeps nginx's default. The changelog is rewritten into Unreleased. Under Added: the endpoint, the source-path check, the cross-process lock and 429, the token, the settings (with their fallbacks), include_hash/include_files/compile_include_digests and the local cache. Under Changed: the boot warm-up in every API process and the nginx timeout.

While re-running make test-integration, the new endpoints parity test caught that POST /compile was missing from the MCP unifier's _ROUTE_CATALOG. It is added.

10. What the source can reach, on a real MetaEditor

You were right to ask. I measured it against MetaEditor 5.00 build 5836 on our deployment, before any check, with C:\Windows\win.ini as the target:

Directive absolute path .. walk \\server\share
#include "…" read read not found
#include <…> read read —
#resource refused (error 313) refused (error 313) refused
#property icon not found read (error 342: invalid icon format, or embedded if it is a real .ico) —

Forward slashes, indentation and a comment before the # made no difference. A read that doesn't compile still leaks: #include "C:\Windows\win.ini" came back as C:\Windows\win.ini(1,3) : error 175: 'for' - expressions are not allowed on a global scope. That is a file's tokens in log, and an include of the API's own config.yaml would do the same. #import is not a compile-time read: an import of a file that does not exist compiles.

So the handler now refuses, with a 400 before MetaEditor runs, any #include, #resource or #property icon whose path:

  • has a drive letter or :;
  • starts with \\ or //;
  • has a .. segment, or any segment of only dots and spaces;
  • names a device (CON, NUL, COM1…);
  • or is not a literal "path" / <path>.

An #include path may not start with \ or / either. For #resource and #property icon, a leading \ means the MQL5 tree and stays allowed. The scan runs on the source with comments blanked, and it matches directives more loosely than MetaEditor does (any case, spaces after #).

tests/real_compile/test_compile_reach.py is the real-MetaEditor suite: 16 escapes that must be 400, and 4 in-tree directives that must compile. Like tests/real/, it is excluded from the offline run and pointed at a deployment with MT5_COMPILE_URL / MT5_COMPILE_TOKEN. Results on build 5836:

  • Before (the previous head, deployed): 16 failed, 4 passed. MetaEditor ran on all 16 escapes and read the target in 9 of them.
  • After (this head, run inside the same Windows guest against a separate metaquotes/base install, via Flask's test client rather than through nginx): all 16 escapes answered 400 with MetaEditor never launched. All 4 in-tree cases compiled: #include <Trade\Trade.mqh>, <Trade/Trade.mqh>, #resource "\Include\Trade\Trade.mqh", and #import "kernel32.dll".

The threat model in docs/compiling.md has a new "What the source can reach" section with this table and the rule.

Focused regression output

test_compile.py::test_waiting_compiles_are_bounded_and_the_rest_get_429 PASSED
test_compile.py::test_a_refused_or_failed_compile_hands_its_slot_back PASSED
test_compile.py::test_cross_process_lock_is_exclusive PASSED
test_compile.py::test_an_old_lock_file_is_not_a_held_lock PASSED
test_compile.py::test_a_long_hold_is_never_taken_over PASSED
test_compile.py::test_separate_processes_never_hold_the_compile_lock_at_the_same_time PASSED
test_compile.py::test_a_holder_killed_mid_compile_releases_the_lock_at_once PASSED
test_compile.py::test_cross_process_lock_logs_unexpected_errors_not_just_contention PASSED
test_compile.py::test_contention_itself_is_not_logged PASSED
test_compile.py::test_include_hash_follows_an_edit_without_a_local_cache PASSED
test_compile.py::test_include_hash_follows_a_refresh_made_by_another_process PASSED
test_compile.py::test_compile_timeout_reads_bare_numbers_as_seconds PASSED (5 cases)
test_compile.py::test_an_invalid_compile_timeout_falls_back_to_the_default_with_a_warning PASSED (5 cases)
test_compile.py::test_a_compile_timeout_above_the_ceiling_is_lowered_with_a_warning PASSED
test_compile.py::test_an_invalid_compile_byte_cap_falls_back_to_the_default_with_a_warning PASSED (6 cases)
test_compile.py::test_ea_version_must_be_a_short_simple_token PASSED (6 cases)
test_compile.py::test_a_simple_ea_version_is_logged PASSED
test_compile.py::test_the_mirror_is_refreshed_inside_the_cross_process_lock PASSED
test_compile.py::test_the_warmup_refreshes_the_mirror_inside_the_cross_process_lock PASSED
test_compile.py::test_the_lock_covers_the_local_cache_when_one_is_configured PASSED
test_compile.py::test_a_claim_from_an_earlier_boot_does_not_block_this_boots_warmup PASSED
test_compile.py::test_boot_time_jitter_does_not_look_like_a_reboot PASSED
test_compile.py::test_a_claim_in_the_old_format_is_replaced PASSED
test_config_generation.py::test_only_the_compile_route_waits_longer_than_nginx_default PASSED
test_compile.py::test_source_cannot_name_a_file_outside_the_sandbox PASSED (27 cases)
test_compile.py::test_ordinary_directives_still_compile PASSED (14 cases)
test_compile.py::test_the_refusal_names_the_directive_and_path PASSED
84 passed, 84 deselected in 7.44s

Full gates on be851ac, run locally:

  • make lint: all categories passed.
  • make verify-binaries: OK (1 known-unsigned, 0 problems).
  • make test-unit: 854 passed, 3 skipped; coverage 82.71%.
  • make test-integration: 25 passed, including nginx accepting the new location = blocks.
  • make test-go: ok.

Resolve the CHANGELOG conflict: the compile entries move to Unreleased and the v4.14.0 section stays as released.
@psyb0t

psyb0t commented Sep 30, 2026

Copy link
Copy Markdown
Owner

Thanks for the thorough follow-up, especially measuring what the source could reach on a real MetaEditor. The config, lock, admission and warm-up fixes all check out on my side, and make test-unit passes on the merged tree. I merged master into the branch to resolve the changelog conflict.

I'm merging this and following up on master with one hardening change to the directive scanner: the scanner and MetaEditor don't always agree on where a line or a directive starts (line-ending variants, line continuations, comments inside a directive), so I'll normalize the source before scanning and write exactly the text that was scanned. It would help if you could run the real-MetaEditor reach suite again once that lands.

@psyb0t
psyb0t merged commit 3915b24 into psyb0t:master Sep 30, 2026
3 checks passed
psyb0t added a commit that referenced this pull request Sep 30, 2026
The directive scan now splits lines the way the preprocessor does: CR and CRLF become LF before the scan and in the file MetaEditor compiles, Unicode line separators break lines, backslash-newline continuations are joined, form feed and vertical tab count as spaces, and a lone '#' left by a comment spanning a line break is refused. Follow-up to #16.
Marinski added a commit to Marinski/mt5-httpapi that referenced this pull request Oct 2, 2026
- At most two compiles wait behind the running one; the rest get an
  immediate 429 with Retry-After, so the compile-only token cannot occupy
  every waitress thread.
- The cross-process lock is an OS byte-range lock (msvcrt on Windows, flock
  elsewhere) on a file that is never deleted. The OS releases it when the
  holder exits or is killed, so the heartbeat, stale window and rename-based
  reaper are gone, and with them the two-owner race and the heartbeat that
  one failed read ended.
- The mirror is refreshed inside the cross-process lock, which now lives in
  the local cache when one is configured.
- include_hash and include_files are keyed on a stat signature of the tree,
  so an edit in the terminal's MQL5 folder or a refresh by another process
  shows up on the next compile.
- compile_timeout: a bare number is seconds; invalid or <= 0 falls back to
  30s and above 60s is lowered, both with a warning. The byte caps use the
  same positive-integer fallback as the other limits. Nothing raises at
  import.
- ea_version must be 1-64 characters of A-Z a-z 0-9 . _ + -.
- The warm-up claim records the boot it was made in, so every boot warms
  once regardless of reboot_interval.
- nginx gives only the /compile location a 180s timeout; other routes keep
  the default.
- #include, #resource and #property icon paths that are absolute, UNC,
  walk up with .., or name a device are refused with a 400 before
  MetaEditor runs. Measured on MetaEditor build 5836: #include read any
  file by absolute path or .. walk and quoted its tokens back in the log,
  and #property icon read .. walks. tests/real_compile/ checks this against
  a deployed endpoint.
- POST /compile is in the MCP unifier's endpoint catalog.
- CHANGELOG entries rewritten, including the warm-up, lock, digests and
  nginx changes.
Marinski pushed a commit to Marinski/mt5-httpapi that referenced this pull request Oct 2, 2026
…ream

feat(compile): POST /compile — MQL5 source in, .ex5 out
Marinski pushed a commit to Marinski/mt5-httpapi that referenced this pull request Oct 2, 2026
The directive scan now splits lines the way the preprocessor does: CR and CRLF become LF before the scan and in the file MetaEditor compiles, Unicode line separators break lines, backslash-newline continuations are joined, form feed and vertical tab count as spaces, and a lone '#' left by a comment spanning a line break is refused. Follow-up to psyb0t#16.
Marinski pushed a commit to Marinski/mt5-httpapi that referenced this pull request Oct 2, 2026
Takes the reworked POST /compile handler and its test suite from the PR head,
which already carries the source/artifact size caps and the no-exception-echo
fix that the integrated branch had applied locally (cd2755a). server.py keeps
the chartctl routes and the request-body cap gate around the compile-token
branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants