fix(labs): stop silently dropping WASM DesignLedger saves (#1985)

[aadityansha06]

Summary

Fixes a silent data-loss bug where DesignLedger.save() in WASM/Pyodide used a fire-and-forget asyncio.create_task(...) for persistence, so IndexedDB failures were swallowed and students could lose progress across all 34 browser lab notebooks with no indication anything went wrong.

Area

• Book (textbook content, figures, exercises)
• StaffML (interview questions, challenges)
• Kits (hardware labs)
• Infrastructure (CI/CD, scripts, config)
• TinyTorch (modules, tests, milestones)
• MLSysim (labs simulation engine — mlsysim/mlsysim/labs/state.py, not in the list above but the actual area this PR touches)

Changes

• save_async() now raises on IndexedDB failure instead of swallowing it into a print()
• save() attaches a done-callback to the background WASM save task and records failures on last_save_error / exposes in-flight status via save_pending, instead of the exception vanishing into an unobserved task
• Added asave() — an async variant of save() for callers that can await, guaranteeing persistence or raising
• Added flush() — awaits any in-flight background save, re-raising its exception if it failed
• Added regression tests in mlsysim/tests/test_state.py covering the native save/load roundtrip, a successful WASM save clearing last_save_error, a failing WASM save being captured (not silently dropped), and asave() propagating exceptions directly

Testing

• Rendered the book locally (quarto render)
• Ran tests (pytest tests/)
• Ran tito module test NN for affected module(s)
• Manual verification (describe below)

Verified locally and via CI:

• mlsysim/tests/ full suite passes (pytest tests/ -v --tb=short), including 4 new tests in test_state.py
• labs/tests/test_static.py and test_protocol.py pass unaffected, since save()'s public signature (track, step, design, chapter) is unchanged
• Ran the mlsysim-validate-dev.yml workflow via workflow_dispatch against this branch — Run Tests and Build Docs Site both passed. (Check Links showed a failure, but that's an unrelated pre-existing config issue in the link-check job — fail_on_broken: false is already set, but the underlying lychee-action still reports job failure when its glob resolves to zero files. Not something this diff touches.)
  Added labs/tests/test_wasm_persistence.py — a permanent real-Pyodide + real-IndexedDB regression test (see comment below for the full story). Verified red→green: fails correctly against the pre-fix code, passes against the fix.

Environment (local testing):

• OS: Fedora (Linux)
• Python: 3.14 (venv), also verified importable/compilable under 3.12 (CI's pinned version)
• Browser: Google Chrome (installed via Google's official RPM repo) + Playwright-managed headless Chromium for the WASM smoke test
• CI: GitHub Actions, ubuntu-latest runner, via workflow_dispatch on this branch

Related Issues

Fixes #1985

---

By submitting this PR, you agree to release your contribution under the project's license.

[aadityansha06]

Hi @Shashank-Tripathi-07, this is ready for your review. Fixes the silent WASM save failure from #1985 by having save() observe the background task's outcome (via a done-callback) instead of losing it, and save_async() now raises instead of just printing on failure. Added asave()/flush() for async callers, plus regression tests covering both success and failure paths.

Ran the full test suite plus the actual CI workflow against this branch — core checks pass. Still finishing the browser-level Pyodide smoke test to fully match the original repro method, will update here once done. Let me know if you'd like anything changed before the @profvjreddi gives his final verdict to merge this pr.

[Shashank-Tripathi-07]

Hey @aadityansha06 — really appreciate the thoroughness here (the done-callback approach, last_save_error/save_pending, asave(), flush() are all sensible API additions, and the regression tests are clean). Before this lands I did the thing your PR description says you're still working on — a real browser-level smoke test against real Pyodide + headless Chromium, using the exact reproduction methodology from #1985 — and I think it's worth sharing before you go further, because the result surprised me.

The exception/observability path works

Forcing indexedDB.open() to throw (simulating disabled storage / quota / private browsing) now correctly surfaces:

[DesignLedger] SAVE FAILED - progress was NOT persisted: InvalidStateError: Storage is disabled

and last_save_error gets set. That's a real, verified fix for the "exception vanishes into an unobserved task" failure mode. Good.

The success path still silently loses data

This is the part I'd hold on. I ran the actual (unmodified) state.py from this branch against real Pyodide v0.28.3 in headless Chromium — not a mock, the real save_async() / run_js() / IndexedDB code — and every real-method call I tried reports success while the data is not actually in IndexedDB:

• ledger.save(...), waited 3s, read IndexedDB directly → lost
• ledger.save(...) + await ledger.flush() → save_pending=False, last_save_error=None (reports success) → still lost
• await ledger.asave(...) (the fully-awaited API, no background task at all) → lost
• Isolated, minimal case: await ledger.save_async() called directly as a single bound-method call, nothing else → lost, 0/5 across five separate trials

The confusing part, and why I dug further before posting: if I take the exact same IndexedDB JS and run it as a plain top-level script (not through the DesignLedger class, just inlined directly in the same test), it persists reliably (5/5). So it's not that the JS/IndexedDB logic itself is wrong — something about going through the class's bound async def save_async(self) method specifically is where it breaks, even with zero background-task indirection. I was not able to pin down the exact Pyodide-internal mechanism (my best guess is some interaction between transaction auto-commit timing and the extra WASM↔JS boundary crossings introduced by nested coroutine calls, but that's a guess, not a finding).

I think this means the regression tests in test_state.py aren't catching the actual bug, since all four of them monkeypatch save_async itself (fake_save_async / failing_save_async), so none of them exercise the real run_js/IndexedDB code path at all — they verify the new bookkeeping logic (callbacks, last_save_error, flush(), asave() exception propagation) works correctly given whatever save_async() reports, but not whether save_async() itself reliably tells the truth in a real browser.

What I'd suggest before merging

1. The labs/tests/browser_smoke.py real-Pyodide test your PR description mentions as pending — I think that's exactly the right instinct, and it's what would have caught this. Worth prioritizing before merge rather than as a follow-up.
2. If you want, I can share my repro scripts (Playwright + real Pyodide, no marimo/Quarto build needed, just the unmodified state.py) so you can reproduce this directly and dig into the transaction-commit timing — happy to hand those over.
3. Given the issue title is specifically about students silently losing data, I'd want the success path verified end-to-end in real Pyodide before this closes #1985 — right now it closes the loud failure mode but the silent one (the one the issue is actually about) still reproduced for me every time I tried it against real IndexedDB.

Not trying to be discouraging — the API design here is the right shape, and the exception-path fix is real progress. It's specifically the "does store.put() reporting success actually mean the data is durably written, when called from inside this class" question that needs to be nailed down before this can close the issue.

[aadityansha06]

Update for @Shashank-Tripathi-07 - found the actual root cause, and it's not what either of us guessed.

Root cause: Python name mangling, not a Pyodide FFI quirk.

save_async() writes to globalThis.__mlsys_temp_state from inside the DesignLedger class body. Python silently mangles any __name identifier written lexically inside a class — that line actually compiles to globalThis._DesignLedger__mlsys_temp_state = state_json. The embedded JS string, being a plain string Python never parses, still reads the literal globalThis.__mlsys_temp_state — a different, never-set variable. store.put(undefined, ...) doesn't throw, tx.oncomplete still fires, save_async() still reports success — it just persists undefined every time. That's why it was 0/5, deterministic, not flaky, and why my first guess (resolving on tx.oncomplete instead of putReq.onsuccess) didn't fix it — it was already correct and irrelevant to this bug.

The smoking gun: I tried an alternate implementation using from js import __mlsysSaveLedger (also inside the class), and Pyodide's own ImportError handed me the answer directly: cannot import name '_DesignLedger__mlsysSaveLedger' from 'js'.

Fix: renamed __mlsys_temp_state → _mlsys_temp_state (single leading underscore — Python doesn't mangle that). Two-line change.

New regression test: added labs/tests/test_wasm_persistence.py — runs the real save_async() against real Pyodide + real IndexedDB in headless Chromium, reading the write back through a separate connection, exactly like your repro. I verified it red→green: it fails with trial(s) [0,1,2,3,4] of 5 against the pre-fix code (proving it actually catches this class of bug, not just decoration), and passes 5/5 against the fix. mlsysim/tests/test_state.py's mocked tests can't catch this since they replace save_async() entirely — this is the missing coverage layer you flagged.

Happy to also wire this into labs-validate-dev.yml's wasm-smoke-test job as a follow-up if useful, so it runs in CI going forward and not just locally. Let me know if you want that in this PR or a fast-follow.

Thanks for catching this-genuinely would have shipped a silent-data-loss bug disguised as a fix for a silent-data-loss bug without this review.

[Shashank-Tripathi-07]

@aadityansha06 following up on the CI wiring for test_wasm_persistence.py you offered to add. I can't leave this as inline suggestions since this PR doesn't currently touch labs-validate-dev.yml (GitHub only supports suggestion-blocks on lines that are part of the PR's diff), so here's each change broken out separately with the reasoning, if you could apply these to .github/workflows/labs-validate-dev.yml and push, that'd close the last gap before this is ready.

1. Pin PLAYWRIGHT_BROWSERS_PATH on the existing "Install Playwright + Chromium" step

labs/tests/conftest.py redirects $HOME at import time for test isolation. That happens after Playwright installs its browsers to the default, HOME-relative cache path, but before the new pytest step (change 3, below) runs. Without pinning this to a fixed, workspace-relative location, the new step won't be able to find the Chromium binary that was already installed here.

      - name: 🎭 Install Playwright + Chromium
        env:
          # Fixed, HOME-independent location. labs/tests/conftest.py
          # redirects $HOME for test isolation (so lab tests don't touch a
          # real ~/.mlsys), which would otherwise make Playwright look for
          # its browser cache in the wrong place once pytest runs. See the
          # WASM persistence regression test step below.
          PLAYWRIGHT_BROWSERS_PATH: ${{ github.workspace }}/.pw-browsers
        run: |
          pip install playwright
          python3 -m playwright install --with-deps chromium

2. Same pin on "Browser smoke test (real Chromium + Pyodide)"

Same reason, this step also launches a real browser and needs to resolve the same install location.

      - name: 🌐 Browser smoke test (real Chromium + Pyodide)
        env:
          PLAYWRIGHT_BROWSERS_PATH: ${{ github.workspace }}/.pw-browsers
        run: |
          python3 labs/tests/browser_smoke.py --labs-dir /tmp/wasm-smoke

3. Add the new persistence test as its own step, right after the one above

This is the actual wiring, it runs your new real-Pyodide + real-IndexedDB regression test as part of this job, so the fix for the silent-save bug is covered in CI going forward instead of only locally. Worth having this in CI specifically because the mocked mlsysim/tests/test_state.py tests can't catch this class of bug, they replace save_async() entirely rather than exercising the real IndexedDB path.

      # =====================================================================
      # WASM/IndexedDB persistence regression test (#1985 / PR #1988)
      # =====================================================================
      # DesignLedger.save_async() previously reported success while
      # silently failing to persist to IndexedDB, due to a Python
      # name-mangling bug (globalThis.__mlsys_temp_state written inside the
      # class body was rewritten to globalThis._DesignLedger__mlsys_temp_state,
      # desyncing it from the plain name the embedded JS read). Mocked unit
      # tests (mlsysim/tests/test_state.py) can't catch this class of bug
      # since they replace save_async() entirely. This runs the real
      # save_async() against real Pyodide + real IndexedDB in headless
      # Chromium and reads the write back through a separate connection.
      - name: 🧪 WASM persistence regression test (real IndexedDB)
        env:
          PLAYWRIGHT_BROWSERS_PATH: ${{ github.workspace }}/.pw-browsers
        run: |
          python3 -m pytest labs/tests/test_wasm_persistence.py -v

Once this is pushed, you can tag me and I'll do another review for a final go for the Pull Request before we send it to professor. Make sure to run a CI run on your github fork (sync it to the latest version of dev) so that you can verify the PR getting a CI green from your side. Then prof. vijay will look into it, whenever he gets time and get it merged.

Don't lose hope on this, even on your first try, you figured out a lot of stuff on your own and you made real good changes to the project, that's what we say engineering is all about !

[aadityansha06]

@Shashank-Tripathi-07 The CI wiring is pushed and green on my fork against the latest dev. The fixes are verified, and this is ready to merge.

Recap of the full change:

1. Fixed the original fire-and-forget asyncio.create_task() bug ([Bug][Labs] DesignLedger.save() silently fails in WASM/Pyodide, causing data loss across all browser labs #1985) so WASM save failures are explicitly observed and surfaced instead of silently swallowed.

2. Fixed the deeper Python name-mangling issue in save_async() caught during review. It now correctly persists to IndexedDB instead of writing undefined when called as a bound DesignLedger method.

3. Added labs/tests/test_wasm_persistence.py — a real Pyodide + real IndexedDB regression test, fully verified red→green against the pre-fix and post-fix code.

4. Wired the regression test into labs-validate-dev.yml's wasm-smoke-test job. Implemented the exact setup requested: PLAYWRIGHT_BROWSERS_PATH is pinned on both install and test steps, and --override-ini="addopts=" is kept on the pytest invocation to successfully bypass the root pyproject.toml's coverage requirements.

5. Verified mlsysim/tests/test_state.py's mocked bookkeeping tests still pass, keeping the full mlsysim/tests/ suite green.

Let me know if you need anything else to approve this, otherwise it's good to go.

[Shashank-Tripathi-07]

Hi @aadityansha06 ,

I've done the complete review, a few final surgical changes as required. This PR now looks complete and ready from my end and it's a go from my end. @profvjreddi assigning you as a review now to take a look at the final PR. Also, the backstory is quite cool on this PR that helps us to understand a few core things for the project.

---

Let's get this merged.

[github-actions]

Thanks @aadityansha06! 🎉

I added @aadityansha06 to labs, mlsysim for: bug, code, test.

The contributor tables are now handled directly by this workflow; no follow-up command is needed.