diff --git a/ToDo.txt b/ToDo.txt index 22f9c42..2ba3f8f 100644 --- a/ToDo.txt +++ b/ToDo.txt @@ -17,16 +17,95 @@ DONE - Cap valEnergy magnitude at 60000 eV (positive incidence mode and the value in runSimulationSequence() and CreateInpFile(). - Warn when a shared configuration fixed the RanSeed: every scan point then uses the same generator, which can produce statistically misleading results. -- Cap client-controlled numberProj magnitude: an unbounded projectile count (e.g. - 1e9) does not terminate within the 300 s watchdog and can flood scan points - (empirically verified: 1e9 did not finish in 15 s). GUI ProjNumberLimit() is - currently dead code (defined at TrimSPlib.js:3516, never wired to an event). +DONE (advisory, not a hard cap) - Warn on the web when numberProj exceeds 25000 + per point. Measured cost model on a 3.2 GHz core, single-threaded engine: + runtime is linear in numberProj and linear in energy, and cost per projectile + is dominated by the number of nuclear collisions along the track, so the + expensive axis is the LIGHT projectiles, not the heavy ones. Projectile -> + Si, Maxwellian 60 keV, ZBL potential, ms/projectile: muon 3.04, H 3.30, + He 4.06 (worst), Li-8 3.91, Be-11 3.64, B-12 3.19, Mg-31 1.34, Ar 0.83. + Checked and found NOT to affect runtime: layer count (1..60 layers is flat, + the species lookup is a binary search), depth increment dz (0.1 A == 20 A), + and grazing incidence (ALPHA 89 deg is cheaper - the ion skims out). + NH=25000 at the worst case measured 98.6/99.5/100.1 s on the 1.3.2 binary and + 101.5/100.7/101.7 s on a fresh 1.4.0 build, i.e. ~3x margin under the 300 s + TRIMSP_CMD_TIMEOUT. Deliberately warning-only: the value is advisory, the run + still starts, and the user keeps the final say. Web-only, because the + standalone app has no watchdog. TRIMSP_WEB_PROJ_ADVISORY in TrimSPlib.js; + uses the max of the sweep when scanning numberProj. The dead GUI + ProjNumberLimit() (TrimSPlib.js:3516) was left alone - it clamps at 5000, + which is below the 50000-100000 used by the publication test configs, so + wiring it up as-is would have clamped the regression suite. - Persisting the effective per-point seed: the seed already lives in each archived *.inp (all three deck slots), so reproducibility does not require the *.seq header change; extending *.seq would shift the 999(A20,1x) layer slots and break getSequenceColumnInfo(). Surfacing it in run details instead is deferred - not sure it is needed. +Out-of-bounds recoil write in the sputtering path (parIRL >= 1) +============================================================= + +FIXED - the engine now refuses the divergent input instead of corrupting memory. + + Symptom, as originally reported: setting parIRL (the recoil/sputter output + level) to 1 crashed the engine with SIGSEGV, reproducibly 5/5 seeds, and end + to end through singleTrimSP.cgi as exitCode 139 / ok:false. parIRL=0 was never + affected. + + Root cause was NOT the KO dimension. The first diagnosis blamed + KO(600,MAXNL5,2) at trimspNL.F:114 (debug: "Index '601' of dimension 1 of + array 'ko' above upper bound of 600"), on the grounds that the other + recoil-list arrays are 2000 wide. Enlarging KO to 2000 does not help: with + KO(2000) the run still dies, because the cascade overruns all of them. + + The actual cause is a non-converging cascade driven by parERC = 0. The + primary knock-on test is `IF(DEN(IV).LE.ERC) GO TO 6`, so with a zero recoil + cutoff every atom that receives any energy at all is respawned as a recoil on + every cascade generation. The branching ratio exceeds one, so NREC1 and NREC2 + grow geometrically instead of draining: + + NREC2 = 81, 153, 269, 455, 773, 1317, ... (parERC=0, parIRL=1) + + and the list-2/list-1 loop at label 83 never terminates. Instrumented runs + show the block is never even entered when parIRL=0. parERC defaults to 0.0 + in the GUI (TrimSP.html:293) and tests/Si_1layer/Si_1layer_E19000.inp carries + 0.0, so the failing combination is the default one - no unusual user action + is required to hit it. + + Evidence that a real cutoff converges: forcing parERC to 0.02, 0.05 or 0.2 + makes parIRL=1 finish normally (exit 0), and the peak cascade depth is only + 103, well inside KO's 600. KO was therefore left at 600, and the resize was + reverted (it cost 7.7 MiB of .lbss and fixed nothing). + + Changes in fortran/trimspNL.F: + - After readGuiInput, reject parIRL>=1 with parERC<=0 using a clear message + and STOP 1, before any transport work. + - The `IF(NREC2.GT.2000)` guard now stops with a non-zero status instead of + falling through into the out-of-bounds write loop, so any future + non-convergent cascade fails cleanly rather than corrupting memory. + + The GUI now catches this before the engine is ever invoked: + - handleRecoilModeChange (wired to the parIRL widget) fills parERC with + TRIMSP_MIN_RECOIL_CUTOFF = 0.02 when recoil calculation is switched on with + a cutoff of 0, and says so in a transient warning. + - A pre-run check in runSimulationSequence refuses the run if the user then + insists on parIRL=1 with parERC=0, reports it in the status bar plus a + transient warning, and focuses the parERC field. The handler is deliberately + not wired to parERC, so retyping the cutoff is what "insisting" means and is + not silently overridden. This also covers shared-configuration links, which + assign widget values directly and so bypass the change handler. + - The parIRL tooltip documents the requirement. + + Verified: parERC=0/parIRL=1 -> exit 1 with the new message; parERC=0.02 with + parIRL=0 or 1 -> exit 0; parERC=0 with parIRL=0 -> exit 0 and byte-identical + .rge (and an identical .out apart from timestamps/elapsed/filenames) versus + the pre-fix binary. tests/cgi-tests.sh 18/18, plus + tests/recoil-cutoff-test.js (12 cases over the shipped cutoff code) for the + GUI behaviour above. + + Minimal repro: tests/Si_1layer/Si_1layer_E19000.inp with the last field of + line 2 changed from 0 to 1 (parERC is field 10 of line 1, still 0.0). + Deferred follow-ups from the C-list CGI hardening ================================================= diff --git a/TrimSP.html b/TrimSP.html index 7fc8566..7ff11db 100644 --- a/TrimSP.html +++ b/TrimSP.html @@ -395,12 +395,14 @@ No recoils are generated (i.e., no sputtering effects); used to speed up the calculation if only projectile ranges are of interest. -
  • Calculate collision recoils.
    • +
  • Calculate collision recoils. Requires a + parERC recoil cutoff above zero; 0.02 is filled in + automatically.
    • - + diff --git a/TrimSPlib.js b/TrimSPlib.js index c698610..5fea487 100644 --- a/TrimSPlib.js +++ b/TrimSPlib.js @@ -3804,6 +3804,53 @@ var TRIMSP_ENERGY_CAP_EV = 60000; // silently truncated. Restrict the seed to |v| <= 2^31-1. var TRIMSP_SEED_MAX_ABS = 2147483647; +// Advisory projectile count for the web backend. This is not a hard limit: the +// engine is single threaded and its cost per projectile is dominated by the +// number of nuclear collisions along the track, so the same count can differ by +// more than 5x across the projectile menu. Measured worst case is helium into a +// light heavy-ish target at the 60 keV energy cap with the ZBL potential, at +// about 4.0 ms per projectile on a 3.2 GHz core, which puts 25000 points at +// roughly 100 s. The web backend watchdog (TRIMSP_CMD_TIMEOUT, 300 s) then +// still has about a 3x margin, so this count normally completes while slower or +// more heavily loaded deployments keep the watchdog as a backstop. The warning +// is advisory only; the user keeps the final say. +var TRIMSP_WEB_PROJ_ADVISORY = 25000; + +// Recoil cutoff substituted when the user turns on recoil calculation with a +// cutoff of zero. A zero cutoff makes the transport respawn every recoiling +// atom on every cascade generation, so NREC1/NREC2 grow geometrically instead +// of draining and the run overruns the recoil arrays. The engine refuses that +// combination, so the GUI supplies a usable value instead of letting the run +// fail after the user presses Run. +var TRIMSP_MIN_RECOIL_CUTOFF = 0.02; + +// True when the recoil calculation is enabled without a usable recoil cutoff. +// The parIRL and parERC widgets are plain number inputs and a shared +// configuration assigns their values directly, so this can be reached without +// going through the change handler below. +function recoilCutoffIsUnusable() { + let irl = Number(document.getElementById("parIRL").value); + let cutoff = Number(document.getElementById("parERC").value); + return Number.isFinite(irl) && irl >= 1 + && (!Number.isFinite(cutoff) || cutoff <= 0); +} + +// Called when the parIRL widget changes. Turning recoil calculation on with no +// cutoff is filled in for the user here, because that is a value they almost +// never intend to type. Deliberately not wired to parERC: retyping the cutoff +// afterwards is how a user insists on the combination, and the pre-run check +// then refuses the run instead of silently overriding them. +function handleRecoilModeChange() { + if (!recoilCutoffIsUnusable()) { + return; + } + let cutoffField = document.getElementById("parERC"); + cutoffField.value = TRIMSP_MIN_RECOIL_CUTOFF; + showTransientWarning( + "Recoil calculation needs a cutoff, so parERC was set to " + + TRIMSP_MIN_RECOIL_CUTOFF + ".", cutoffField, 6000); +} + // Returns an empty string when valid, otherwise a message that asks the user // to change the offending value. function validateEnergyValue(raw) { @@ -4056,6 +4103,46 @@ async function runSimulationSequence() { } } + // Advisory only, and web only: the standalone app runs the engine locally + // with no watchdog, so a long run is the user's own choice there. A number + // of projectiles that scales badly with the collision count can still exceed + // the backend watchdog, so warn once per run without blocking it. For a + // numberProj sweep the scan values are authoritative, otherwise the base + // value is what every point uses. + if (amIWeb()) { + const projValues = (ScanAttrib === "numberProj") + ? SValues + : [Number(All["numberProj"])]; + let worstProj = 0; + for (var projIndex = 0; projIndex < projValues.length; projIndex++) { + let projValue = Number(projValues[projIndex]); + if (Number.isFinite(projValue) && projValue > worstProj) { + worstProj = projValue; + } + } + if (worstProj > TRIMSP_WEB_PROJ_ADVISORY) { + showTransientWarning( + "Warning: " + worstProj + " projectiles per point may exceed the web " + + "time limit and fail. Runs above ~" + TRIMSP_WEB_PROJ_ADVISORY + + " are safest; the run will start anyway.", + document.getElementById("numberProj"), 8000); + } + } + + // Hard stop, unlike the projectile advisory above: with a zero recoil + // cutoff the cascade does not converge, so the engine exits with an error + // instead of producing results. Refuse the run here and say which field to + // change, rather than letting the backend fail with a transport message. + if (recoilCutoffIsUnusable()) { + let cutoffField = document.getElementById("parERC"); + let message = "parIRL=1 needs a parERC recoil cutoff above 0; " + + TRIMSP_MIN_RECOIL_CUTOFF + " is a common choice."; + updateStatusBar({ run: message }); + showTransientWarning("Not running: " + message, cutoffField, 8000); + cutoffField.focus(); + return(0); + } + // Freeze the initial GUI state and expand the scan into independent jobs. // Each job gets its own input file name and a deterministic seed offset so // the results are reproducible even when runs finish out of order. diff --git a/fortran/trimspNL.F b/fortran/trimspNL.F index 7e8369f..320315e 100644 --- a/fortran/trimspNL.F +++ b/fortran/trimspNL.F @@ -111,6 +111,12 @@ C & ,ISPIS(MAXNL5),ISPOP(MAXNL5),ISPOS(MAXNL5) INTEGER*4 ITSP(MAXNL5),ISPALT(MAXNL) & ,ISPIPT(MAXNL5),ISPIST(MAXNL5),ISPOPT(MAXNL5),ISPOST(MAXNL5) +C KO flags each recoil as a primary knock-on or a cascade product. Its +C first index is a recoil-list index, and 600 is deep enough for every +C cutoff that yields a converging cascade (measured peak is 103 with +C ERC=0.02). See the ERC/IRL check after readGuiInput below: with a zero +C recoil cutoff the cascade does not converge at all, and it overruns +C these 600 entries as well as the 2000-deep recoil lists. INTEGER*4 KO(600,MAXNL5,2) INTEGER*4 MEAB(MAXD2,22),MAGB(62,22),MEAGB(MAXD2,36,22) & ,MEABL(75,21),MEPB(MAXD2,MAXD2) @@ -569,6 +575,24 @@ C GUI file layout is isolated from the transport code. & ED,BE,CH1,CH2,CH3,CH4,CH5) +C A zero recoil cutoff makes the cascade diverge. The primary knock-on +C test below is IF(DEN(IV).LE.ERC) GO TO 6, so with ERC=0 every atom that +C receives any energy at all is respawned as a recoil on every cascade +C generation. The branching ratio then exceeds one, NREC1 and NREC2 grow +C geometrically instead of draining, and the run overruns the recoil +C arrays. Measured on tests/Si_1layer/Si_1layer_E19000.inp with +C parIRL=1: NREC2 passes 2000 within a single projectile, whereas a +C cutoff of 0.02 keeps the peak at 103. parERC defaults to 0.0 in the +C GUI, so this combination is reachable without the user doing anything +C unusual. Refuse it here, before any transport work, instead of +C corrupting memory later on. + if (irl.ge.1 .and. erc.le.0.d0) then + write(*,*) 'ERROR : parIRL=1 requires a positive parERC cutoff.' + write(*,*) ' parERC=0 makes the recoil cascade diverge' + write(*,*) ' and overrun the recoil arrays. Set parERC to' + write(*,*) ' a positive value (0.02 is a common choice).' + STOP 1 + endif C======================================================================= C OPEN OUTPUT FILES AND START CLOCK C======================================================================= @@ -1401,11 +1425,18 @@ C MAXA=MAX0(MAXA,NREC2) NALL=NALL+NREC2 NREC1=0 +C Guard against a cascade that does not fit the recoil lists. A +C non-converging cascade reaches this test with NREC2 far above any array +C bound, so the old fall-through into the processing loop below overran +C every recoil array and killed the run with SIGSEGV. A cascade that does +C not fit is not recoverable, so stop with a non-zero status rather than +C let the CGI report a truncated run as a success. IF(NREC2.GT.2000) GO TO 8885 GO TO 8886 8885 WRITE(6,8887) 8887 FORMAT(1X,'ERROR : DIMENSION IN THE RECOIL LOOP ', & 'MUST BE INCREASED') + STOP 1 8886 CONTINUE C C PROCESS THE PARTICLES IN LIST 2 diff --git a/tests/recoil-cutoff-test.js b/tests/recoil-cutoff-test.js new file mode 100644 index 0000000..ff65c15 --- /dev/null +++ b/tests/recoil-cutoff-test.js @@ -0,0 +1,108 @@ +// Checks the GUI handling of the parIRL/parERC combination that used to crash +// the engine. A zero recoil cutoff makes the transport cascade diverge, so the +// GUI supplies a cutoff when recoils are switched on and refuses the run if the +// user insists on the zero cutoff. +// +// The real functions are lifted out of TrimSPlib.js and run against a minimal +// document stub, so this exercises shipped code rather than a copy of it. +// Run with: node tests/recoil-cutoff-test.js + +const fs = require('fs'); +const path = require('path'); + +const libPath = path.join(__dirname, '..', 'TrimSPlib.js'); +const src = fs.readFileSync(libPath, 'utf8'); + +function grab(re) { + const m = src.match(re); + if (!m) { + throw new Error('pattern not found in ' + libPath + ': ' + re); + } + return m[0]; +} + +const fields = {}; +let shown = []; +let statusMsg = null; + +global.document = { + getElementById(id) { + if (!(id in fields)) { + fields[id] = { value: '', focus() { this.focused = true; } }; + } + return fields[id]; + } +}; +global.showTransientWarning = (message) => { shown.push(message); }; +global.updateStatusBar = (options) => { + if (options && options.run) { statusMsg = options.run; } +}; + +eval([ + grab(/var TRIMSP_MIN_RECOIL_CUTOFF = [\d.]+;/), + grab(/function recoilCutoffIsUnusable\(\) \{[\s\S]*?\n\}/), + grab(/function handleRecoilModeChange\(\) \{[\s\S]*?\n\}/), +].join('\n\n')); + +// The pre-run refusal, lifted verbatim out of runSimulationSequence. It keeps +// that function's return convention, where 0 means "aborted before the engine +// was invoked" and 1 means the run may proceed. +eval('function runGuardVerdict() {\n' + + grab(/ if \(recoilCutoffIsUnusable\(\)\) \{[\s\S]*?\n \}\n/) + + ' return 1;\n}\n'); + +function reset(irl, erc) { + for (const key of Object.keys(fields)) { delete fields[key]; } + const makeField = (value) => ({ + value: value, + focus() { this.focused = true; }, + }); + fields.parIRL = makeField(String(irl)); + fields.parERC = makeField(String(erc)); + shown = []; + statusMsg = null; +} + +const cases = [ + // description, IRL, ERC, touchIRL, fills, refuses + ['default GUI state', 0, '0.0', false, false, false], + ['parIRL 0->1, parERC=0 default', 1, '0.0', true, true, false], + ['user insists, edits parERC=0', 1, '0', false, false, true], + ['user sets parERC back to 0', 1, '0.0', false, false, true], + ['valid parIRL=1 with cutoff', 1, '0.02', false, false, false], + ['parIRL 0->1, cutoff already set', 1, '0.02', true, false, false], + ['valid parIRL=1, large cutoff', 1, '15', false, false, false], + ['parIRL=0 with parERC=0', 0, '0.0', false, false, false], + ['parIRL 0->1, negative parERC', 1, '-1', true, true, false], + ['parIRL 0->1, blank parERC', 1, '', true, true, false], + ['parIRL 0->1, non-numeric parERC', 1, 'abc', true, true, false], + ['non-numeric parIRL treated as off', 'x', '0.0', true, false, false], +]; + +let passed = 0; +let failed = 0; + +for (const [name, irl, erc, touchIRL, expectFilled, expectRefused] of cases) { + reset(irl, erc); + const before = fields.parERC.value; + // The handler is wired to the parIRL widget only, so it runs when the user + // changes parIRL and not when they go back and retype the cutoff themselves. + if (touchIRL) { handleRecoilModeChange(); } + const filled = fields.parERC.value !== before; + const refused = runGuardVerdict() === 0; + const refusedOk = (refused === expectRefused); + // A refused run must actually say something in both places. + const explained = !refused + || (shown.length > 0 && typeof statusMsg === 'string' && statusMsg.length > 0); + + const ok = (filled === expectFilled) && refusedOk && explained; + if (ok) { passed++; } else { failed++; } + console.log((ok ? 'PASS ' : 'FAIL ') + name.padEnd(34) + + 'parERC=' + String(fields.parERC.value).padEnd(6) + + (filled ? 'filled in' : 'left alone').padEnd(13) + + (refused ? 'refused' : 'runs') + + (shown.length ? ' | "' + shown[shown.length - 1] + '"' : '')); +} + +console.log('\n' + passed + ' passed, ' + failed + ' failed'); +process.exit(failed ? 1 : 0);