diff --git a/backend/app/api/routes/compile.py b/backend/app/api/routes/compile.py index 7328478d..29a6cffe 100644 --- a/backend/app/api/routes/compile.py +++ b/backend/app/api/routes/compile.py @@ -44,14 +44,44 @@ _COMPILE_SEMAPHORE = asyncio.Semaphore(2) _TARGET_LOCKS: dict[str, asyncio.Lock] = {} +def _build_identity(board_fqbn: str) -> str: + """The identity of the persistent BUILD DIRECTORY this FQBN compiles in. + + The lock below has to be keyed on the shared resource, and that resource is + the build dir — which `_prepare_persistent_project_dir` keys on + (idf_target, variant), NOT on the FQBN. Several FQBNs map to one variant: + arduino-esp32's boards.txt gives both `esp32` and `esp32cam` + `build.variant=esp32`, so they land in the SAME directory. + + Keying the lock on the FQBN therefore left the two of them free to run at + once inside one build dir. Measured: compiling `esp32:esp32:esp32` and + `esp32:esp32:esp32cam` concurrently returned BYTE-IDENTICAL firmware (same + sha256, 285504 bytes) and the esp32cam request got the other sketch's + binary — its serial printed the other example's output. In the app that + looked like an example running someone else's program: two gallery tabs + open, and the second one shows the first one's log and does nothing. + + Falls back to the raw FQBN if the compiler cannot resolve the pair, which + is the previous behaviour and never less strict than it needs to be for a + board whose variant we could not read. + """ + try: + target = espidf_compiler._idf_target(board_fqbn) + variant = espidf_compiler._arduino_variant(board_fqbn, target) + return f"{target}::{variant}" + except Exception: # noqa: BLE001 - identity is best-effort; FQBN is a safe fallback + return board_fqbn + + def _target_lock(board_fqbn: str) -> asyncio.Lock: - """Lazy-initialised per-target lock so concurrent compiles to the same - board serialise. Different boards still run in parallel up to the - semaphore cap.""" - lock = _TARGET_LOCKS.get(board_fqbn) + """Lazy-initialised lock so concurrent compiles that SHARE A BUILD DIR + serialise. Boards with genuinely different build dirs still run in + parallel up to the semaphore cap.""" + key = _build_identity(board_fqbn) + lock = _TARGET_LOCKS.get(key) if lock is None: lock = asyncio.Lock() - _TARGET_LOCKS[board_fqbn] = lock + _TARGET_LOCKS[key] = lock return lock diff --git a/frontend/src/__tests__/__snapshots__/examples-netlist-snapshot.test.ts.snap b/frontend/src/__tests__/__snapshots__/examples-netlist-snapshot.test.ts.snap index f24bd7e0..ebbbfef1 100644 --- a/frontend/src/__tests__/__snapshots__/examples-netlist-snapshot.test.ts.snap +++ b/frontend/src/__tests__/__snapshots__/examples-netlist-snapshot.test.ts.snap @@ -1449,9 +1449,13 @@ D_led1 led1_sense_mid 0 LED_RED exports[`netlist snapshot — circuits (41 examples) > battery-voltage-monitor 1`] = ` "* Velxio circuit -R_r2 n0 n1 10000 +V_bat n0 bat_int DC 9 +R_bat_esr bat_int n2 1.5 +R_r1 n0 n1 20000 +R_r2 n1 n2 10000 R_autopull_n0 n0 0 100Meg R_autopull_n1 n1 0 100Meg +R_autopull_n2 n2 0 100Meg .op .end" `; diff --git a/frontend/src/data/examples-circuits.ts b/frontend/src/data/examples-circuits.ts index 67ed040d..7ad39cc0 100644 --- a/frontend/src/data/examples-circuits.ts +++ b/frontend/src/data/examples-circuits.ts @@ -1424,13 +1424,22 @@ void loop() { }`, components: [ UNO, + // The battery the description promises. Without it the divider's top end + // hung in the air, so A0 sat at 0 V and the sketch printed + // "Battery: 0.0 V / WARNING: Battery low!" forever — the one thing this + // example exists to show never happened. + { type: 'wokwi-battery-9v', id: 'bat', x: 250, y: 120, properties: {} }, { type: 'wokwi-resistor', id: 'r1', x: 350, y: 80, properties: { value: '20000' } }, { type: 'wokwi-resistor', id: 'r2', x: 350, y: 200, properties: { value: '10000' } }, ], wires: [ + w('w0', ['bat', '+'], ['r1', '1'], '#ff0000'), w('w1', ['r1', '2'], ['r2', '1']), w('w2', ['r2', '2'], ['arduino-uno', 'GND'], '#000000'), w('w3', ['r1', '2'], ['arduino-uno', 'A0'], '#ffaa00'), + // Battery negative shares the Arduino's ground — without a common + // reference the ADC has nothing to measure against. + w('w4', ['bat', '−'], ['arduino-uno', 'GND'], '#000000'), ], }, diff --git a/test/backend/unit/test_compile_build_lock.py b/test/backend/unit/test_compile_build_lock.py new file mode 100644 index 00000000..26c55c36 --- /dev/null +++ b/test/backend/unit/test_compile_build_lock.py @@ -0,0 +1,80 @@ +""" +The compile lock must protect the BUILD DIRECTORY, not the FQBN. + +`_prepare_persistent_project_dir` keys the persistent build dir on +(idf_target, variant). arduino-esp32's boards.txt gives several boards the same +variant — `esp32` and `esp32cam` are both `build.variant=esp32` — so those FQBNs +share one directory. The lock used to be keyed on the FQBN, which left them free +to build in it at the same time. + +That is not a theoretical race. Measured against staging: compiling +`esp32:esp32:esp32` and `esp32:esp32:esp32cam` concurrently returned +BYTE-IDENTICAL firmware (same sha256, 285504 bytes) and the esp32cam caller got +the other sketch's binary — verified by embedding a unique marker string in each +sketch and finding the wrong one in the returned image. In the product that +surfaced as a gallery example running someone else's program: open two ESP32 +examples in two tabs, and the first tab shows the second's serial log and does +nothing itself. + +Run from the repo root: + python -m pytest test/backend/unit/test_compile_build_lock.py -v +""" + +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).parent.parent.parent.parent / 'backend')) + +from app.api.routes import compile as compile_module + + +class BuildIdentityTests(unittest.TestCase): + def test_boards_sharing_a_variant_share_one_lock(self): + """esp32 and esp32cam build in the same dir, so they must serialise.""" + plain = compile_module._target_lock('esp32:esp32:esp32') + cam = compile_module._target_lock('esp32:esp32:esp32cam') + self.assertIs( + plain, + cam, + 'esp32 and esp32cam share one persistent build dir (both are ' + 'build.variant=esp32); handing them different locks lets them ' + 'corrupt each other and swap binaries.', + ) + + def test_the_same_board_still_serialises(self): + self.assertIs( + compile_module._target_lock('esp32:esp32:esp32'), + compile_module._target_lock('esp32:esp32:esp32'), + ) + + def test_different_chips_still_run_in_parallel(self): + """The lock must not become a global one — that would halve throughput.""" + xtensa = compile_module._target_lock('esp32:esp32:esp32') + riscv = compile_module._target_lock('esp32:esp32:esp32c3') + self.assertIsNot( + xtensa, + riscv, + 'different IDF targets have separate build dirs and should still ' + 'compile concurrently up to the semaphore cap.', + ) + + def test_unknown_boards_share_the_default_pair(self): + """An unrecognised FQBN resolves to the default (esp32, esp32) pair. + + That is not a hole: those compiles genuinely land in the same build dir, + because the compiler derives the dir from the same two functions. Sharing + a lock here is the safe direction — serialising more than necessary only + costs throughput, while serialising less corrupts a build. + """ + a = compile_module._build_identity('definitely:not:a:board') + b = compile_module._build_identity('also:not:a:board') + self.assertEqual(a, b) + self.assertIs( + compile_module._target_lock('definitely:not:a:board'), + compile_module._target_lock('also:not:a:board'), + ) + + +if __name__ == '__main__': + unittest.main()