fix(backtest): symbol-suffix remap, safe cache priming, bounded log tails - #18
fix(backtest): symbol-suffix remap, safe cache priming, bounded log tails#18Marinski wants to merge 1 commit into
Conversation
|
I reproduced a stale-cache failure in the current head.
That makes the stale cache authoritative in Please give the cache a finite configured maximum age, have I also ran |
3e8c680 to
6e8e203
Compare
…detail to callers From review: /compile accepted an unbounded body, wrote the source to disk, read the whole .ex5 back into memory, and base64-inflated it into the response - so one authenticated request could consume unbounded disk, memory, worker time and bandwidth. And the catch-all handler echoed the exception class and message to the caller, which for an OSError is an internal path. Two documented per-request caps, both enforced before the resource they bound is spent: - COMPILE_MAX_SOURCE_BYTES (default 2 MB): an oversized body is refused with 413 straight from its declared Content-Length, before parsing; the decoded source is then checked against the cap itself, before anything reaches disk. - COMPILE_MAX_EX5_BYTES (default 16 MB): the artifact is size-checked on disk, before it would be read or encoded. A refusal carries no binary at all, and is a 500, not a 422 - the caller's source compiled fine; the server is declining to return the result. The log names the knob. Both settings clamp rather than raise on bad values, matching the other numeric settings in config.py - it is imported by the whole API, so a typo in an optional endpoint's tuning must not stop trading. Unexpected errors now return a bare 'internal error'; the traceback goes to the server log only. The two remaining detail leaks on the 500 path (MetaEditor's absolute path, the OSError from launching it) are genericized the same way. Six new tests: the 413 fires before the compiler ever runs and before the work dir exists, the declared-length refusal happens before parsing (proven with a non-JSON payload - a 400 would mean the parser read it), the artifact refusal carries no ex5_base64, within-cap requests are unaffected both ways, and the 500 body contains neither the exception class nor its message nor a path. The existing kaboom test permitted the leak by asserting only that log was a string; the new one closes that hole. All six fail against the previous handler. Also evicted scripts/prune-terminal-logs.sh from this branch: the squash had swept it in from unrelated local work. It is PR psyb0t#18's file (byte- identical to that branch's copy, referenced by nothing here), and psyb0t#18's own review round has since fixed a selection bug in it - keeping a stale copy in this PR would both collide with psyb0t#18 on merge and reintroduce the bug that fix removes.
6e8e203 to
14394f7
Compare
|
Both fixed in 1. The cache is no longer authoritative foreverYou named the design flaw precisely:
The integration-level test you asked for runs through the real INI-builder path ( 2. The root terminal log is pruned now
New behavioral suite ( Merge order#15 → #16 → #18 → #10, this third. #16 previously carried a stale copy of Also rebased onto current master (post-#17); the one conflict was the Full suite green, lint clean. |
|
Consolidated: #19 is folded into this one and closed. It changes the same file as item 1's caller and is the same kind of terminal-log robustness, so two PRs over One commit on top of what you last saw: It also fixes something I found in there:
Your two findings from 2026-08-25 remain fixed as described in |
|
I rechecked the current head locally. The two earlier findings are fixed: cache staleness is enforced inside One blocking issue remains in the suffix-cache workflow. The config says a backtest-mode terminal should be primed with
So the documented cache-priming request triggers the exact state backtest mode is intended to avoid. Please redesign the cache lifecycle so a backtest terminal never needs an SDK request to populate it. For example, populate a broker/account cache from a live terminal or provide an explicit safe cache import. Add an HTTP-level regression test for a Separate gate issue: I cannot approve this revision yet. |
Two defects surfaced during a deep manual QA pass on the prior commit (553bb99, the mode:backtest symbol-cache-priming fix for PR psyb0t#18): - POST /symbols/import crashed with an unhandled AttributeError (raw 500) on a syntactically-valid-but-non-object JSON body (a bare array or scalar), since `request.get_json(silent=True) or {}` passes such values straight through to a `.get()` call. Now validated explicitly and rejected with a clean 400. - Symbol names from this endpoint are operator/copy-paste input, unlike the SDK-sourced names GET /symbols persists, so stray whitespace could silently survive into the cache and defeat the exact-match lookup in backtest.handler._normalize_symbol. Now stripped before storage. Also gitignore .deep-qa/, the local audit-trail scaffolding used for that QA pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Both fixed, 1. The unsafe cache-priming pathYou named it exactly:
HTTP-level regression test ( I didn't stop at the unit suite for this one. I also ran the actual That same pass turned up two more real bugs in the new endpoint, now also fixed in
2. The lint gate
I reproduced your exact symptom on the old script (uninstalled the module inside the built image, ran it: prints "is not recognized" twice, still reports "all lint categories passed", exit 0) and confirmed the new script fails loudly under the identical scenario (exit 1, named error).
|
|
Thanks for fixing the SDK initialization, stale-cache, root-log matching, and PSScriptAnalyzer issues. I re-reviewed current head 1. The documented backtest priming flow is still broken
Replace that configuration guidance with the safe import workflow, including a concrete POST body and the live-terminal source option. Add a behavioral test for the documented backtest workflow, not a source-text assertion. 2. The backtest refusal still waits on the global MT5 lock
I reproduced this against the real Flask route on this head: I held Split the endpoint so the backtest guard runs before 3. The terminal log pruner is not deployed or runnable by usersThe new script is only referenced by its own test, There is no service or supervisor that invokes Either wire it into the generated Compose deployment with the terminal-root mount and documented configurable environment, then add a rendered-Compose integration test, or document it as a deliberately manual command with a usable invocation. Do not claim automated pruning until one of those is true. Merge and test statusGitHub currently reports this PR as I ran Please fix the three blockers, rebase, and post the focused regression output. |
88c6fbd to
36b24b6
Compare
|
All three fixed, rebased onto current master, and squashed to a single commit so the branch carries no intermediate states. 1. The documented backtest priming flow. Tested behaviorally, not by asserting on the file's text: 2. The refusal waiting on the lock. Confirmed and fixed. 3. The pruner. Your premise changed under me during the rebase, so I went a different way than either option you offered — flagging it explicitly since it wasn't what you asked for. v4.13.1 already added terminal-journal pruning into That capability is the part that mattered. Your pass deletes whole files whose Three notes on it, all of which cost me a round of self-review to find:
I verified the script's behavior in a real Also dropped: my PSScriptAnalyzer commit. v4.13.0 landed your own fix, which covers the same gap plus a pinned-version check — I took yours and dropped mine entirely rather than carry a conflicting variant. One thing I found on the way: On the integration gate: it is green here — 23 passed, including all four Wickworks lifecycle cases. Environment: docker 29.2.1, compose v5.1.0, runc 1.3.4, cgroup v2, overlayfs. Your runc/netns failure does not reproduce on this host, and I do not think it can come from this branch: those tests generate their own throwaway compose referencing only |
|
Thanks, the latest revision addresses the earlier issues around refusing backtest
The rest of the focused branch checks passed, including the existing test and lint workflows. These two points are the remaining blockers. |
36b24b6 to
8631193
Compare
|
Both fixed, head is now 1.
413 for the body, 400 for count and length. Nothing reaches the cache when a request is refused. The ordering is the part worth checking, so it is pinned by a test that sends an over-cap One decision worth surfacing: 2. The MCP catalog test. You were right that it was an implementation-source assertion. Deleted, along with its helpers. Replaced with two black-box tests in Comparison choices, all commented in place: equality in both directions, because a missing entry hides a real route from every agent and a surplus entry sends them at a 404 — a subset check would pass an empty catalog. Werkzeug's converter prefix is stripped from the Flask side ( Verified load-bearing two different ways: removing Known and deliberately not done here: |
…ails
Several related fixes to how a mode:backtest terminal resolves symbols
and bounds its own logs, plus an operator script for broker clock drift.
symbol_suffix was appended to symbols the broker carries bare.
_normalize_symbol appended it to every [Tester].Symbol that did not
already end with it, but brokers rarely suffix their whole book:
Eightcap Global carries 56 suffixed FX pairs (EURUSD.i) alongside 785
bare metals, indices and crypto (XAUUSD, BTCUSD, ASX200), so every
non-FX backtest there asked the tester for a symbol that does not exist
and came back empty. The suffix is now skipped when the broker's symbol
list has the bare name and lacks the suffixed one; brokers carrying both
forms (BlackBull lists AUDUSD and AUDUSDp) still get it.
That check needs a symbol list, which cannot be obtained at INI-build
time -- a backtest terminal never attaches the SDK and
Bases/<server>/symbols/*.dat is encrypted. GET /symbols (unfiltered
only) persists what it saw to <terminal>/mt5api-symbols.json and the INI
builder reads it back, trusted for symbol_cache_max_age (default 7d).
With no cache the previous append-always behaviour is unchanged.
Priming it no longer wedges the terminal. GET /symbols is @with_mt5, so
on a mode:backtest terminal it fell through ensure_initialized() into a
full mt5.initialize(), spawning terminal64.exe and holding the tester's
single-instance data-dir lock for that terminal's life -- every backtest
afterward exited clean with an empty report. It is now refused with 409
there, before any SDK call, and before the MT5 lock: the MODE check runs
undecorated and delegates the live path to a @with_mt5
_list_symbols_live, so a terminal already stuck behind an SDK request
answers immediately instead of 503-ing a minute later.
POST /symbols/import is the safe replacement, and it is bounded. It
previously accepted anything: a single 2,097,153-byte symbol name came
back 200 and was persisted into the cache file the INI builder parses on
every run, inside a fixed-disk Windows VM. Three per-request caps now
apply, all configurable and all clamped rather than raising, since
config.py is imported by the whole API and a typo in one endpoint's
tuning value must not stop trading:
* SYMBOL_IMPORT_MAX_BODY_BYTES (2 MiB) is checked against the declared
Content-Length BEFORE request.get_json() runs. Parsing first would
already have paid the memory cost the cap exists to prevent, so the
ordering is the point: it is covered by a test that sends an
over-cap length with a body that is not valid JSON, which only a
gate placed before the parser can answer 413. A body with no
declared length but a transfer encoding cannot be bounded up front
and is refused 411; a request with no body keeps its historical 400.
* SYMBOL_IMPORT_MAX_SYMBOLS (20000, ~20x the widest book in this
fleet) is counted on the raw array before dedup, because bounding
the strip/dedupe/sort pass is the job.
* SYMBOL_IMPORT_MAX_SYMBOL_LENGTH (64, double MT5's own 31-character
limit) is measured on the NORMALIZED name, so padding can neither
fail a legal name nor smuggle an illegal one.
Every request body is bounded, not just that one endpoint's. Capping the
endpoint that was reported left six others parsing whatever arrived --
POST /orders, PUT /orders/<id>, PUT and DELETE /positions/<id>,
POST /symbols/<symbol>/rates/ta, POST /backtest/build-ini and
/backtest/build-set -- and a per-endpoint check only bounds the endpoint
someone remembered, so the next route added is the next hole. The cap now
lives once in the before_request hook every route already passes through:
MAX_REQUEST_BODY_BYTES (4 MiB) for anything that is not a file upload,
MAX_UPLOAD_BODY_BYTES (25 MiB) for multipart, since POST /backtest carries
a compiled .ex5 plus its .set and .ini. The upload cap matches the
client_max_body_size nginx already enforces in front of the API, so
reaching the port directly answers the same as coming through the proxy.
An endpoint with a tighter cap of its own still reports that one, because
it is checked inside the handler.
Terminal journals are bounded by size, not just age. The retention
window only deletes journals once they are RETAIN_DAYS old, which cannot
reach the real hazard: one high-frequency backtest writes tens of
gigabytes into TODAY's journal and the disk fills a week before that
file is eligible. rotate-logs.sh now truncates any in-window journal
over MAX_LOG_BYTES once it has been idle for IDLE_MINUTES, in place
(terminal64.exe holds it open) and preserving the mtime, so a
just-emptied journal cannot outrank the one a running job is writing in
_tail_dir_log's newest-by-mtime selection. Sizing uses stat, not
`wc -c`: busybox wc reads the whole file, ~18s per 3 GB journal in the
alpine image this runs in.
Backtest log tails are bounded to the final 256 KB. Reading a
multi-gigabyte UTF-16 terminal log in full to show its tail took 45-65s
under the GIL, starving /ping and the container healthcheck with it.
The unified `endpoints` MCP tool is now tested through the protocol.
It was asserted by AST-parsing _ROUTE_CATALOG out of the unifier's
source -- an implementation-source check that stays green while tool
registration, transport or serialization breaks. It is replaced by
black-box tests that boot the shipped image, call the public tool over
MCP, and compare what it returns against the real Flask url_map.
POST /symbols/import was missing from that catalog, so the 409 above was
telling agents to call a route the unified endpoint never advertised.
Also: scripts/measure-broker-offsets.py reports each terminal's measured
broker clock offset, since config.yaml's utc_offset is static and
DST-unaware.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8631193 to
46aa2f5
Compare
|
Swept the rest rather than leaving them for a later round — head is now Capping only the endpoint you reported left six others parsing whatever arrived: The cap now lives once, in the
Both refuse with 413 from the declared
Honest note on that file: 15 of its 18 cases go red when I disable the guard. The other three assert that a cap does not fire (a body exactly at the limit, a multipart body above the JSON cap) or that the endpoint's own cap still wins — none of which can be made red by removing the guard. They pin off-by-one against a future tightening; I am not counting them as proof of the fix. I also checked the guard does not refuse anything legitimate, which is the real risk of a global gate: all seven JSON routes accept a normal body, a realistic 2 MB multipart backtest submission passes, and both caps fire at exactly +1. Deliberately not covered, so it does not look like it is: |
Three related fixes to how a
mode: backtestterminal resolves symbols and bounds its own logs, plus one operator script. Rebased on current master and squashed to a single commit.1.
symbol_suffixwas appended to symbols the broker carries bare_normalize_symbolappendedsymbol_suffixto every[Tester].Symbolthat did not already end with it. Brokers rarely suffix their whole book: Eightcap Global carries 56 suffixed FX pairs (EURUSD.i) alongside 785 bare metals, indices and crypto (XAUUSD,BTCUSD,ASX200). Every non-FX backtest there asked the tester for a symbol that does not exist and came back empty.The suffix is now skipped when the broker's symbol list has the bare name and lacks the suffixed one. Brokers that carry both forms — BlackBull lists
AUDUSDandAUDUSDp— still get the suffix, sosymbol_suffix: pkeeps meaning "use the prime variant".That check needs a symbol list, and there is no way to get one at INI-build time: a
mode: backtestterminal never attaches the SDK, andBases/<server>/symbols/*.datis encrypted. SoGET /symbols(unfiltered only) persists what it saw to<terminal>/mt5api-symbols.json, and the INI builder reads that back.With no cache present the previous append-always behaviour is used unchanged, so a terminal that has never been primed cannot regress.
The cache is trusted for
symbol_cache_max_age(default 7d — a top-level config key, orSYMBOL_CACHE_MAX_AGEin the environment). Past that, or with a missing/malformedupdatedstamp, it counts as absent and the append-always fallback applies, so a broker moving a symbol between bare and suffixed cannot be papered over by a years-old list.Covered by
tests/test_symbol_suffix_remap.py: bare-only, suffixed-only, both-forms, stale cache, corrupt cache, and the no-cache fallback.2. Priming that cache used to wedge the terminal
GET /symbolsis@with_mt5, so on amode: backtestterminal — which never attaches the SDK at startup — it fell throughensure_initialized()into a fullmt5.initialize(). That spawnsterminal64.exeand holds the tester's single-instance data-dir lock for the rest of that terminal's life; every backtest submitted afterward exits clean with an empty report. The documented way to prime the cache was the thing that broke the terminal.It is now refused with 409 there, before any SDK call — and before the MT5 lock. The
MODEcheck runs undecorated and delegates the live path to a@with_mt5_list_symbols_live, so a terminal already stuck behind an in-flight SDK request answers the refusal immediately instead of 503-ing after the full acquire timeout. A regression holds_mt5_lockfrom another thread and asserts the prompt 409; its counterpart asserts the live listing still waits, so the split can't later be "fixed" by dropping the decorator outright.POST /symbols/importis the safe replacement: it writes a caller-supplied list straight to the cache and never callsmt5.*or takes the lock. Source the list from a live terminal on the same broker/account, or the broker's own docs.config/config.yaml.exampleanddocs/market-data.mddocument this flow for backtest terminals. The example is covered behaviorally rather than by a source-text assertion: the test walks the documented steps against the real Flask app and asserts the outcome the comment promises — that the INI builder stops appending the suffix to a symbol the broker carries bare.POST /symbols/importis also registered in the MCP unifier's_ROUTE_CATALOG, which is a hand-maintained mirror of the Flask route table and the only route discovery an agent on the unified endpoint gets — the 409 tells agents to call a route the catalog did not list. A new parity test intests/test_mcp_tool_parity.pypins that catalog against the realurl_mapso the next added route cannot drift out of it.3. Every request body is bounded
POST /symbols/importabove takes a caller-supplied list, and it originally took it unbounded: onesymbolsitem of 2,097,153 bytes was accepted with a200and persisted into<terminal>/mt5api-symbols.json, a file the backtest INI builder parses on every run inside a fixed-disk Windows VM. Three per-request caps now apply, all configurable, all clamped rather than raising (config.pyis imported by the whole API, so a typo in one endpoint's tuning value must not stop trading):symbol_import_max_body_bytesContent-Lengthbeforerequest.get_json()symbol_import_max_symbolssymbol_import_max_symbol_lengthThe ordering is the part worth checking, so it is pinned by a test that sends an over-cap
Content-Lengthwith a body that is not valid JSON — only a gate placed before the parser can answer413; one placed after answers400.Capping that one endpoint left six others parsing whatever arrived, though —
POST /orders,PUT /orders/<id>,PUTandDELETE /positions/<id>,POST /symbols/<symbol>/rates/ta,POST /backtest/build-iniand/backtest/build-set— and a per-endpoint check only bounds the endpoint someone remembered, so the next route added is the next hole. The cap therefore also lives once, in thebefore_requesthook every route already passes through:max_request_body_bytes(4 MiB) for anything that is not a file upload,max_upload_body_bytes(25 MiB) for multipart, sincePOST /backtestcarries a compiled.ex5plus its.setand.ini. The upload cap matches theclient_max_body_sizenginx already enforces in front of the API, so reaching the port directly answers the same as coming through the proxy. It runs after the auth check, so limits cannot be probed anonymously, and an endpoint with a tighter cap of its own still reports that one.Deliberately not covered, so it does not look like it is:
/mcpis mounted throughDispatcherMiddlewareoutside Flask's router, sobefore_requestnever runs for it. Separate transport, separate framing — happy to bound it as its own change.4. Terminal journals are bounded by size, not just age
The retention window added in v4.13.1 deletes dated journals once they are
RETAIN_DAYSold, which cannot reach the actual hazard: a high-frequency grid strategy logs every order placement, modification and cancellation, so one backtest writes tens of gigabytes into today's journal — the disk fills a week before that file is even eligible for the age pass.rotate-logs.shnow truncates any in-window journal overMAX_LOG_BYTES(default 2 GiB) once it has been idle forIDLE_MINUTES(default 30), so a running backtest never loses its own diagnostics. Truncated in place, not deleted:terminal64.exeholds the journal open, so unlinking the inode would leave it writing to a deleted file with the space unreclaimed until it exited. Both knobs sit on the existinglog-rotatorservice in both Compose files and are validated at startup likeRETAIN_DAYS.Two details that are easy to get wrong, both covered by tests:
stat, notwc -c— busyboxwcreads the whole file to count bytes, ~18s per 3 GB journal in thealpine:3.20image this actually runs in, repeated everyINTERVALon precisely the files the cap exists for. The test image ships GNU coreutils, wherewc -cis already O(1), so that cost is invisible to the suite._tail_dir_logpicks the newest.login a directory by mtime, so bumping a just-emptied journal to now would make it outrank the one a running job is writing, andGET /backtest/<id>/tailwould answer with nothing until that job's next write — reintroducing, by another route, the stale-wrong-log failure item 5 fixes.The docs are explicit about what this does not do:
IDLE_MINUTESdeliberately exempts a journal its own backtest is still writing, so the cap reclaims space after the run goes quiet rather than bounding a runaway mid-run. Such a journal is logged asover cap but still active, left aloneon every pass rather than silently skipped.5. Backtest log tails are bounded to the final 256 KB
GET /backtest/<id>/tailread a whole multi-gigabyte UTF-16 terminal log on every call: whole-file bytes, plus a decodedstrcopy, plus asplitlines()list, all under the GIL. Measured at 45-65s per call, which starves every other request in the process —/pingand the container healthcheck included — so a perfectly healthy terminal looks wedged from the outside.Tailing is now
O(tail): seek to the final window, aligned to a 2-byte boundary so UTF-16 code units stay intact, and drop the partial first line. Encoding is sniffed from the first two bytes, covering both MT5's BOM-marked UTF-16 logs and the BOM-less ones, withrun.logdecoding as UTF-8.run.loggets the same treatment: it is usually sparse, but "usually" is not a bound and this endpoint is polled once a minute per running job.While in there,
_tail_dir_logpicked the newest log alphabetically, sometaeditor.logsorted after every<date>.logand a stale compile log was returned instead of the run being polled. It now picks by modification time and excludesmetaeditor.log, which is the rule_tail_terminal_logalready applied. 15 tests intests/test_backtest_log_tail.py.6. One operator script
scripts/measure-broker-offsets.py—config.yaml's per-terminalutc_offsetis a static number subtracted from every broker timestamp, and nothing in the stack is DST-aware, so a value that is right in August is an hour wrong after the autumn rollover. This reports the measured offset per terminal.It carries a prominent warning: hitting an SDK route on a
mode: backtestterminal launchesterminal64.exe, andPOST /terminal/shutdownonly detaches the SDK client — so the terminal is left holding MT5's single-instance lock, and the next backtest there returns an emptyBars=0 Ticks=0 Symbols=0report. It must be followed bydocker compose down && ./run.sh.make test-unit: 670 passed, 3 skipped, 80.60% coverage.make test-integration: 25 passed.make lintclean.make verify-binaries: OK.