diff --git a/.plans/cloud-login-verify-password-plan-grilled-by-claude.md b/.plans/archived/cloud-login-verify-password-plan-grilled-by-claude.md similarity index 100% rename from .plans/cloud-login-verify-password-plan-grilled-by-claude.md rename to .plans/archived/cloud-login-verify-password-plan-grilled-by-claude.md diff --git a/.plans/cloud-login-verify-password-plan-grilled-by-gpt.md b/.plans/archived/cloud-login-verify-password-plan-grilled-by-gpt.md similarity index 100% rename from .plans/cloud-login-verify-password-plan-grilled-by-gpt.md rename to .plans/archived/cloud-login-verify-password-plan-grilled-by-gpt.md diff --git a/.plans/cloud-login-verify-password-plan-initial.md b/.plans/archived/cloud-login-verify-password-plan-initial.md similarity index 100% rename from .plans/cloud-login-verify-password-plan-initial.md rename to .plans/archived/cloud-login-verify-password-plan-initial.md diff --git a/.plans/cloud-login-interactive-shell-history-fix.md b/.plans/cloud-login-interactive-shell-history-fix.md new file mode 100644 index 0000000..ebb227f --- /dev/null +++ b/.plans/cloud-login-interactive-shell-history-fix.md @@ -0,0 +1,190 @@ +# Plan: fix interactive `cloud login` retry killing the shell + +## Context + +Interactive `cloud login` now verifies a candidate password by asking the cloud daemon to reconnect +with that candidate, then waiting for either `trapos_cloud_connected` or +`trapos_cloud_unauthorized`. + +Observed bug: + +- Run `cloud login` with no password argument. +- Enter a wrong password. +- Instead of printing `invalid password, try again` and prompting again, the command exits back to + the CraftOS shell. +- The shell's in-memory history is gone, so up-arrow no longer recalls the previous `cloud login` + command. + +The interactive branch in `packages/trapos-cloud/programs/cloud.lua` already looks logically +correct: on an `unauthorized` verdict it calls `restore()`, prints the retry message, and continues +the loop. Existing unit coverage also models wrong-then-right retry successfully. That points to the +live boot/runtime topology rather than the branch itself. + +## Root Cause Hypothesis + +The short-lived verifier event loop inside `cloud login` can accidentally stop the boot event loop. + +Relevant flow: + +- `packages/trapos-cloud/programs/cloud.lua` creates a local event loop inside `verifyPassword()`. +- When the daemon emits `trapos_cloud_unauthorized`, the verifier calls `el.stopLoop()`. +- `packages/trapos-core/apis/eventloop.lua` implements `stopLoop()` by globally queueing a synthetic + event named `@libeventloop/END_OF_LOOP/`. +- Event loop IDs currently come from a module-local counter. +- If `/apis/eventloop.lua` is loaded as independent module instances, both the boot event loop and + the local verifier loop can use ID `1`. +- The verifier's synthetic stop event can therefore match the boot loop's stop event name. +- `packages/trapos-boot/startup/boot.lua` runs the shell and boot event loop with + `parallel.waitForAny(shellFn, eventLoopFn)`. +- If the boot event loop exits, `parallel.waitForAny` returns and the shell coroutine is killed. +- Killing/restarting the shell explains losing shell history. + +The stale hello `messageId` issue found during exploration is real hardening work, but it does not +directly explain shell history loss. It can cause wrong verdict attribution, not shell teardown. + +## Goals + +- Wrong passwords in interactive `cloud login` re-prompt without leaving the command. +- Stopping a short-lived local event loop must not stop `_G.bootEventLoop`. +- Preserve CraftOS shell history after wrong-password retry. +- Keep the fix minimal and local to the event loop semantics where possible. +- Add regression tests that reproduce the event-loop collision class. + +## Non-Goals + +- Do not replace `cloud login` verification with a program-owned websocket. +- Do not add a standalone Lua test harness. +- Do not redesign `startup/boot.lua` or shell lifecycle unless the event-loop fix proves insufficient. +- Do not add persisted shell history as a workaround for shell teardown. + +## Implementation + +### 1. Make event loop IDs process-global + +File: `packages/trapos-core/apis/eventloop.lua` + +Replace the module-local `next_eventloop_id` allocation with a `_G`-backed counter so independently +loaded copies of the module cannot reuse the same synthetic end event name. + +Sketch: + +```lua +local COUNTER_KEY = '__trapos_next_eventloop_id'; + +local function nextEventLoopId() + local id = tonumber(_G[COUNTER_KEY]) or 1; + _G[COUNTER_KEY] = id + 1; + return id; +end +``` + +Then use: + +```lua +local eventloop_id = nextEventLoopId(); +``` + +This keeps the existing `@libeventloop/END_OF_LOOP/` design but removes collisions across module +instances. + +### 2. Avoid queued stop events when stopping from inside the same loop + +File: `packages/trapos-core/apis/eventloop.lua` + +Add a local `stopRequested = false` flag. + +When `stopLoop()` is called while the loop is actively dispatching its own handler or timeout, set +`stopRequested = true` instead of queueing a global event. After dispatching the current event, break +the loop if `stopRequested` is set. + +Keep queueing the synthetic end event when `stopLoop()` is called from another coroutine, because the +loop may be blocked inside `os.pullEventRaw()` and needs to be woken. + +This is a second layer of protection: the verifier's `finish()` calls `stopLoop()` from inside its own +event handler, so no global end event should be needed for that path at all. + +### 3. Add an event-loop collision regression test + +File: `packages/trapos-core/tests/eventloop.lua` + +Add a test that simulates independent module loads: + +- Load `/apis/eventloop.lua` twice via `loadfile()` or an equivalent isolated environment. +- Create one boot-like loop from the first module and one verifier-like loop from the second module. +- Run both under `parallel`. +- Stop the verifier loop from one of its handlers. +- Assert the boot-like loop still handles a later probe event. +- Stop the boot-like loop explicitly at the end so the test cannot hang. + +The regression should fail against the current module-local ID allocation and pass after the fix. + +### 4. Add or extend cloud program coverage + +File: `packages/trapos-cloud/tests/cloud-program.lua` + +Keep the existing wrong-then-right interactive test, because it verifies the program branch behavior. + +Add a small test for prompt-time terminate handling if practical: + +- Fake `read('*')` raises `Terminated`. +- Assert `cloud login` queues `login-restore` when needed and re-raises `Terminated`. + +This is not the primary wrong-password bug, but it closes an adjacent interactive cancellation gap. + +### 5. Optional cloud hardening: match hello responses by `messageId` + +File: `packages/trapos-cloud/apis/libcloud.lua` + +Store the active hello `messageId`, pass `frame.messageId` through `api.onMessage()` to `onHello`, +and ignore hello responses that do not match the currently active hello attempt. + +This prevents stale hello responses from earlier reconnects being attributed to the current +`cloud login` token. It should be treated as a separate hardening change if the event-loop fix is +intended to stay small. + +## Package Versions + +If only `eventloop.lua` changes: + +- Bump `trapos-core` in `packages/trapos-core/ccpm.json`. +- Mirror the same version in `packages/index.json`. + +If cloud program or cloud daemon behavior also changes: + +- Bump `trapos-cloud` in `packages/trapos-cloud/ccpm.json`. +- Mirror the same version in `packages/index.json`. + +Only bump the full `trapos` meta-package if the repository's release convention for this change +requires it. + +## Verification + +Targeted iteration: + +```sh +just trapos-exec 'shell.run("/programs/runtest.lua", "--pretty", "/tests/eventloop.lua", "/tests/cloud-program.lua", "/tests/cloud.lua")' +``` + +Required after Lua/package edits: + +```sh +just check +just test --pretty +``` + +Manual validation on a running TrapOS computer: + +- Run `cloud login --force`. +- Enter a known wrong password. +- Confirm it prints `invalid password, try again (ctrl+t to cancel)` and prompts again. +- Press up-arrow after eventually cancelling or completing, and confirm shell history still contains + `cloud login --force`. + +## Risks + +- Event loop stop semantics are shared by servers and programs, so regression tests should cover both + same-loop stop and cross-coroutine stop. +- If CraftOS-PC loads `require('/apis/eventloop')` as a singleton in more cases than expected, the ID + collision may be intermittent; the `_G` counter still makes the behavior deterministic and safer. +- The optional hello `messageId` hardening will require updating existing cloud tests that currently + send hello responses without message IDs. diff --git a/.plans/cloud-login-session-crash-fix-plan.md b/.plans/cloud-login-session-crash-fix-plan.md new file mode 100644 index 0000000..fab882a --- /dev/null +++ b/.plans/cloud-login-session-crash-fix-plan.md @@ -0,0 +1,129 @@ +# Fix: interactive `cloud login` tears down the whole trapos session on a wrong password + +## Context + +`cloud login` (interactive) was just rewritten to verify the password before saving +(commit `1d4789b`). When testing it at the **local in-game terminal**, typing a wrong +password produces two bugs: + +1. It prints `invalid password, try again (ctrl+t to cancel)` and then **quits to the + shell instead of re-prompting**. +2. The CraftOS shell **command history is gone** (up-arrow no longer recalls `cloud login`). + +### Why this happens (root cause — confirmed by code, not guessed) + +The interactive loop itself is correct: verified three ways (logic trace, a faithful +real-`eventloop`+daemon simulation, and a real-CC probe on `8:trap2`) that a wrong +password returns `'unauthorized'`, runs the re-prompt branch (which is why the user sees +the "invalid password" line), and loops back to `readPassword()`. So the program is *not* +the thing that exits — **the whole trapos session is being torn down underneath it.** + +The teardown chain, all confirmed in the code: + +- A wrong password triggers a rapid reconnect storm on the daemon: `login-verify` + (close live socket → open with candidate → rejected) immediately followed by + `login-restore` (close → open with persisted secret), plus any `reconnectDelay` retries. +- `connect()` opens the socket with `httpLike.websocketAsync(url)` **un-`pcall`'d** + (`packages/trapos-cloud/apis/libcloud.lua:256`). On rapid open/close this can throw + (CC's per-computer websocket limit / "already open" races). +- The shared boot event loop dispatches handlers **un-`pcall`'d** + (`packages/trapos-core/apis/eventloop.lua:294` — `handler(table.unpack(packed))`). +- The boot loop runs under `parallel.waitForAny(shellFn, eventLoopFn)` with no isolation + (`packages/trapos-boot/startup/boot.lua:80`). A throw in `eventLoopFn` propagates out of + `waitForAny`, the `startup` script dies, and CraftOS drops to its **default shell with a + fresh history** — i.e. "quit to shell" + "history lost". It fires right after the + "invalid password" line because the throw happens while the daemon processes the + `login-restore` reconnect. + +The connection bouncing is harmless on a local terminal; the **session crash** is the bug. + +## Fix + +Two small, complementary changes. The eventloop change is the real fix (it makes the +shared boot loop fault-isolated so no server hiccup can ever kill the session); the +`connect()` change removes the specific throw and keeps the daemon retrying correctly. + +### 1. (Recommended, systemic) Isolate handler errors in the boot event loop + +`packages/trapos-core/apis/eventloop.lua`, in `runLoop` (~line 293): + +Wrap the handler call so a throwing handler can never escape `runLoop` (and thus never +crash `parallel.waitForAny` / the trapos session). Keep the existing `api.STOP` +unregister behaviour for the success case; on error, report and continue. + +```lua +-- before: +local result_handler = handler(table.unpack(packed)) +if result_handler == api.STOP then + api.unregister(eventName, handler) +end +-- after: +local ok, result_handler = pcall(handler, table.unpack(packed)) +if not ok then + -- one bad handler must not tear down the shared boot loop / session + printError('eventloop handler error ('..eventName..'): '..tostring(result_handler)) +elseif result_handler == api.STOP then + api.unregister(eventName, handler) +end +``` + +(Use whatever error sink fits the file's style — `printError` if available, else a no-op / +existing logger. The point is: catch, don't propagate.) + +### 2. (Cause-specific) Make the daemon's socket-open crash-safe + +`packages/trapos-cloud/apis/libcloud.lua`, `connect()` (~line 246-257): + +`pcall` the `websocketAsync` call and, on failure, fall back to `scheduleReconnect()` so a +throw becomes a normal retry instead of a fatal error. `scheduleReconnect` is defined +*after* `connect`, so add a forward declaration `local scheduleReconnect` before `connect` +and drop the `local` on its later definition. + +```lua +local function connect() + if clearReconnectTimeout then clearReconnectTimeout(); clearReconnectTimeout = nil; end + secret = secretOverride or getSecret(); + state = 'connecting'; + if not reconnecting then log('info', 'connecting', { url = url }); end + local ok = pcall(function() httpLike.websocketAsync(url); end) + if not ok then + log('warn', 'websocket open failed', { url = url }); + scheduleReconnect(); + end +end +``` + +After these, the wrong-password flow is: verify → `unauthorized` → print "invalid +password" → `login-restore` (now crash-safe) → `readPassword()` re-prompts. Both symptoms +are fixed: the session survives and the prompt re-appears. + +### Note (not the cause, but flagged by the grilling plans) + +The interactive `terminate` branch still does `error('Terminated', 0)` +(`packages/trapos-cloud/programs/cloud.lua:182`). With the eventloop hardened this no +longer risks the session, but a clean `print('login cancelled'); return;` for the Ctrl+T +cancel is a reasonable follow-up (optional, out of scope for the crash fix). + +## Tests + +- **`packages/trapos-core/tests/eventloop.lua`** — add: a handler that throws does not + propagate out of `runLoop`; sibling handlers and subsequent events still fire. +- **`packages/trapos-cloud/tests/cloud.lua`** — add (uses the existing + `fakes.fakeHttp()` / `ctx.el.fire` harness, ~line 186-205): configure + `http.websocketAsync` to `error()`, fire `trapos_cloud_reconnect`, assert no error + propagates and the daemon schedules a retry rather than dying. +- Re-run the existing cloud suites (`tests/cloud.lua`, `tests/cloud-program.lua`) — the + fake-eventloop program tests are unaffected. + +## Verification (end to end) + +On a disposable/test CC computer (do **not** use the shared `8`/`10` machines — driving a +wrong-password verify bounces their live gateway connection): + +1. Deploy the updated `trapos-core` + `trapos-cloud`, set `cloud.url`, reboot so the daemon + runs. +2. `cloud login --force`, type a **wrong** password, Enter. + - Expect: `invalid password, try again (ctrl+t to cancel)` **and the prompt returns** — + the shell/session stays alive and command history is intact. +3. Type the correct password → `logged in`. +4. Sanity: `cloud status` still works (daemon recovered after the verify bounce). diff --git a/.plans/cloud-login-session-crash-merged-fix-plan-final.md b/.plans/cloud-login-session-crash-merged-fix-plan-final.md new file mode 100644 index 0000000..5c7c7d4 --- /dev/null +++ b/.plans/cloud-login-session-crash-merged-fix-plan-final.md @@ -0,0 +1,215 @@ +# Final Plan: fix interactive `cloud login` session teardown + +## Goal + +Fix the user-visible bug where interactive `cloud login` with a wrong password prints the retry +message, then drops back to a fresh CraftOS shell with lost in-memory command history instead of +prompting again. + +One end-to-end fix, split into small independently testable changes. Before building all of it, we +**disambiguate the actual root cause** (see Step 0) so we know which fix is curing the bug and which +are hardening — the prior merged plan shipped three fixes across two competing theories without ever +confirming which one mattered. + +## Confirmed code facts (verified against source) + +These are grounded in the current tree, not hypotheses: + +- `eventloop.lua:19` — `next_eventloop_id` is **module-local**. `eventloop.lua:174/186` — + `END_OF_LOOP` is keyed to that id, and `stopLoop()` stops a loop by queueing that global event. + Boot loop is created at `boot.lua:48`; the `cloud login` verifier loop at `cloud.lua:117`. +- `eventloop.lua:294` — handlers are dispatched **un-`pcall`'d**. The boot loop runs this under + `parallel.waitForAny(shellFn, eventLoopFn)` (`boot.lua:63,80`), so a throwing handler ends the + startup session and drops to a fresh shell. +- `libcloud.lua:256` — `connect()` calls `httpLike.websocketAsync(url)` **un-`pcall`'d**. The daemon + runs on `_G.bootEventLoop` (`servers/cloud.lua:45`), and `trapos_cloud_reconnect` → + `reconnectNow` → `connect()` (`libcloud.lua:415→425`). So a throw here escapes into the boot loop + and out through `waitForAny`. + +## Two candidate root causes predict *different* symptoms + +This is the key insight that drives Step 0. + +- **A. Event-loop ID collision (clean exit).** Only possible if `require('/apis/eventloop')` hands + the cloud program a *separate module instance* from boot's. If so, both loops can take id `1`; the + verifier's `stopLoop()` queues `END_OF_LOOP/1`, the boot loop also matches it and exits **cleanly** + → `waitForAny` returns → `startup` ends with **no error printed** → fresh shell. If `require` is a + singleton (normal CraftOS module cache), boot=id1 / verifier=id2+, names never collide, and this is + a non-cause. +- **B. Thrown handler (crash exit).** A throw in a boot-loop handler (e.g. websocket open) propagates + through `waitForAny`; bios prints a `startup:80:`-style **traceback** before the fresh shell. + +The original report — "invalid password, then dropped to shell", no traceback mentioned — leans +toward **A (silent clean exit)**. Step 0 confirms which it is so the after-test is meaningful. + +## Step 0: Disambiguate and reproduce the failure FIRST + +On a disposable TrapOS computer (never the shared `8`/`10` machines — a wrong-password verify bounces +their live gateway connection): + +1. Deploy the *current* (unfixed) `trapos-core` + `trapos-cloud`, set `cloud.url`, reboot so the + daemon runs. +2. `cloud login --force`, enter a **wrong** password. +3. Record exactly what happens: + - Was a Lua **traceback** printed before the shell returned? → cause is **B** (a throw); fix #3 is + load-bearing, fix #1 is unrelated hardening. + - Did it drop to the shell **silently** (only the "invalid password" line, then a fresh prompt)? + → cause is **A** (collision); fix #1 is load-bearing. +4. Confirm the runtime instance question directly. Probe whether the cloud program actually gets a + separate eventloop module instance from boot: + + ```sh + # from a shell.run program context, compare against boot's loader identity / next id + just trapos-exec 'print(tostring(require("/apis/eventloop")))' + ``` + + Compare the loader identity (and observed allocated ids) seen from boot vs. from a `shell.run` + program. If identical → singleton → collision (A) is impossible and the bug must be B. + +Do not skip this. If the failure does not reproduce on demand it may be instance-identity dependent +and therefore intermittent; a post-fix "it re-prompts once" result would prove nothing. + +## Bugs / Fix Areas + +### 1. Event-loop stop can leak across loops + +`packages/trapos-core/apis/eventloop.lua`. + +**Primary fix — same-loop `stopRequested` (cleaner, more local).** The verifier's `finish()` always +calls `stopLoop()` from *inside its own handler*. So when `stopLoop()` is invoked while the loop is +actively dispatching its own handler/timeout, set an internal `stopRequested = true` and break after +the current dispatch — never queue a global event at all. This makes the verifier→boot leak +impossible regardless of ids. + +**Belt-and-suspenders — process-global id counter.** Keep queueing the synthetic `END_OF_LOOP/` +for *external* (cross-coroutine) stop calls, where the loop may be blocked in `os.pullEventRaw()` and +must be woken. To make those ids collision-proof across independently loaded module instances, +allocate the id from a `_G`-backed counter: + +```lua +local COUNTER_KEY = '__trapos_next_eventloop_id'; +local function nextEventLoopId() + local id = tonumber(_G[COUNTER_KEY]) or 1; + _G[COUNTER_KEY] = id + 1; + return id; +end +``` + +Preserve existing contracts: `api.STOP` unregister on successful handler return, duplicate-register +error, and "stopping an already stopped loop" error. + +### 2. Boot event-loop handler errors can tear down the session + +`packages/trapos-core/apis/eventloop.lua` + `packages/trapos-boot/startup/boot.lua`. + +- Add protected handler dispatch as an explicit, opt-in event-loop option, e.g. + `createEventLoop({ onError = function(eventName, err) ... end })`. +- Use protected dispatch for `_G.bootEventLoop` in `boot.lua`; the error handler reports via + `printError` (else `print`) and the loop continues after a handler error. +- Keep dispatch **unprotected by default** for short-lived program-local loops so dev/test failures + still fail loudly unless explicitly isolated. +- Interaction to verify: after a handler throws under protected dispatch, the `api.STOP` unregister + (`:295`) and the `END_OF_LOOP`/`terminate` break (`:303`, which sits *outside* the handler loop) + must still run. + +### 3. Cloud daemon websocket open can throw during reconnect + +`packages/trapos-cloud/apis/libcloud.lua`. + +- Forward-declare `scheduleReconnect` so `connect()` can call it. +- Wrap `httpLike.websocketAsync(url)` in `pcall`. +- On failure, log `websocket open failed` and call `scheduleReconnect()` instead of throwing. +- Avoid duplicate noisy logs while already in reconnect-loop mode. + +## Out of scope (state the reasoning so it isn't re-litigated) + +- **`error('Terminated', 0)` at `cloud.lua:182/205` (ctrl+t path).** Raised in the *cloud program* + (shellFn side), which `shell.run` already `pcall`s — it never reaches `waitForAny` and is not + fatal to the session. Protected boot dispatch (#2) does not and need not cover it. Leave as-is. +- **Stale hello `messageId` matching.** Useful cloud-login hardening, but it causes wrong-verdict + attribution, not session teardown. Follow-up only if Step 0 / manual testing still shows wrong + verdicts after the main fixes. +- **Persisted shell history.** Not a workaround for session teardown. Do not add. + +## Implementation Steps + +### Step 1: Regression coverage + +`packages/trapos-core/tests/eventloop.lua`: + +- **Same-loop stop:** a handler calls `stopLoop()` on its own loop; assert the loop stops after the + current dispatch and that no global `END_OF_LOOP` event is required (i.e. a *second* sibling loop + sharing the id is unaffected). This is the test that actually corresponds to the real failure + mode — frame it that way, not as the artificial double-`loadfile`. +- **Cross-instance id:** simulate independent module loads (`loadfile`), create a boot-like and a + verifier-like loop, stop the verifier from inside its handler, assert the boot-like loop still + handles a later probe event. (Belt-and-suspenders for the `_G` counter.) Explicitly stop the + boot-like loop at the end so the test cannot hang. +- **Protected dispatch:** one handler throws, a sibling/later handler still runs, and the configured + `onError` sink receives the error. + +`packages/trapos-cloud/tests/cloud.lua`: + +- `http.websocketAsync` throws during `startSession()`/reconnect; assert the error does not + propagate and a reconnect timer is scheduled. + +Keep existing `cloud-program.lua` wrong-then-right interactive tests. + +### Step 2: Fix `eventloop.lua` + +- Add same-loop `stopRequested` break (primary). +- Replace module-local id allocation with the `_G` counter (secondary). +- Add opt-in protected dispatch (`onError`) for handlers (and timeout callbacks if practical). +- Preserve `api.STOP` unregister on success and the duplicate-register / already-stopped contracts. + +### Step 3: Use protected boot event loop + +`packages/trapos-boot/startup/boot.lua` — construct `_G.bootEventLoop` with an `onError` that reports +via `printError`/`print` and continues. + +### Step 4: Harden cloud reconnect + +`packages/trapos-cloud/apis/libcloud.lua` — forward-declare `scheduleReconnect`, `pcall` the +`websocketAsync` open, log + reschedule on failure, no duplicate noise while reconnecting. + +### Step 5: Package versions + +Bump owning packages and mirror in `packages/index.json`: + +- `trapos-core` (`apis/eventloop.lua`) +- `trapos-boot` (`startup/boot.lua`) +- `trapos-cloud` (`apis/libcloud.lua`) + +Do not bump the `trapos` meta-package unless release convention requires it. + +## Verification + +Targeted iteration: + +```sh +just trapos-exec 'shell.run("/programs/runtest.lua", "--pretty", "/tests/eventloop.lua", "/tests/cloud.lua", "/tests/cloud-program.lua")' +``` + +Required after Lua/package edits: + +```sh +just check +just test --pretty +``` + +Manual validation on the **same disposable computer used in Step 0** (so before/after is comparable): + +1. Deploy updated `trapos-core`, `trapos-boot`, `trapos-cloud`; reboot. +2. `cloud login --force`, enter a wrong password → retry message prints **and the prompt returns**. +3. Cancel or complete login, then up-arrow → shell history still contains the command. +4. Enter the correct password → `logged in`. +5. `cloud status` → daemon recovered after the verify bounce. + +Tie it back to Step 0: the symptom recorded there (silent vs. traceback) must be gone, and the fix +that addresses *that* cause must be the one demonstrated, not just "all three are in and it passes." + +## Recommended split (three reviewable chunks) + +1. Event-loop same-loop stop + cross-instance id safety. +2. Boot-loop protected dispatch. +3. Cloud reconnect crash-safety. diff --git a/.plans/cloud-login-session-crash-merged-fix-plan.md b/.plans/cloud-login-session-crash-merged-fix-plan.md new file mode 100644 index 0000000..2efc7f3 --- /dev/null +++ b/.plans/cloud-login-session-crash-merged-fix-plan.md @@ -0,0 +1,190 @@ +# Merged Plan: fix interactive `cloud login` session teardown + +## Goal + +Fix the user-visible bug where interactive `cloud login` with a wrong password prints the retry +message, then drops back to a fresh CraftOS shell with lost in-memory command history instead of +prompting again. + +This should be handled as one end-to-end fix plan, split into small independently testable changes. + +## Current Assessment + +The interactive `cloud login` branch itself is already covered and appears logically correct: + +- wrong password returns an unauthorized verdict; +- the program queues `login-restore`; +- it prints `invalid password, try again (ctrl+t to cancel)`; +- it loops back to `readPassword()`. + +The observed shell/history loss points to the surrounding runtime being torn down, not to the login +loop simply returning. + +There are two plausible and concrete runtime failure mechanisms in the current code. Either one can +explain the same user-visible symptom, and both are worth fixing because both affect boot-session +stability. + +## Bugs / Fix Areas + +### 1. Event-loop stop event collision + +`packages/trapos-core/apis/eventloop.lua` allocates loop IDs from a module-local counter and stops a +running loop by queueing a synthetic global event named `@libeventloop/END_OF_LOOP/`. + +If `/apis/eventloop.lua` is loaded as independent module instances, two loops can both use ID `1`. +The short-lived verifier loop created by `cloud login` can then queue an end event that also matches +the boot event loop. Since `startup/boot.lua` runs shell + boot loop under `parallel.waitForAny`, a +boot-loop exit kills the shell coroutine and loses shell history. + +Fix: + +- Allocate event loop IDs from a `_G`-backed process-global counter. +- Avoid queueing a global synthetic stop event when `stopLoop()` is called from inside the same + event loop's handler or timeout. Use an internal `stopRequested` flag and break after the current + dispatch. +- Keep queued synthetic stop events for external stop calls, where the loop may need to wake from + `os.pullEventRaw()`. + +### 2. Boot event-loop handler errors can tear down the session + +`eventloop.lua` currently dispatches handlers directly. A thrown handler escapes `runLoop()`. The +boot event loop is run directly inside `parallel.waitForAny(shellFn, eventLoopFn)`, so an unhandled +server/daemon error can terminate the TrapOS startup session and drop the user into a fresh shell. + +Fix: + +- Add protected handler dispatch as an explicit event-loop option, for example + `createEventLoop({ onError = function(eventName, err) ... end })`. +- Use protected dispatch for `_G.bootEventLoop` in `packages/trapos-boot/startup/boot.lua`. +- Keep the default unprotected for short-lived program-local loops so development/test failures still + fail loudly unless explicitly isolated. + +### 3. Cloud daemon websocket open can throw during reconnect + +`packages/trapos-cloud/apis/libcloud.lua` calls `httpLike.websocketAsync(url)` without `pcall`. +During wrong-password verification the daemon rapidly reconnects with the candidate secret, receives +an unauthorized response, restores the persisted secret, and reconnects again. If `websocketAsync` +throws during that churn, the error can currently escape through the boot event loop. + +Fix: + +- Forward-declare `scheduleReconnect` so `connect()` can call it. +- Wrap `httpLike.websocketAsync(url)` in `pcall`. +- On failure, log a warning and schedule a normal reconnect instead of throwing. + +## Out Of Scope For This Fix + +### Stale hello `messageId` matching + +Matching hello responses by the active hello `messageId` is useful cloud-login hardening. It can +prevent stale hello responses from an older reconnect from being attributed to the current candidate. + +However, this does not directly explain shell/session teardown. Treat it as a follow-up unless the +main fixes still leave wrong verdict attribution in manual or probe testing. + +### Persisted shell history + +Do not add persisted shell history as a workaround. The correct fix is to stop tearing down the +TrapOS session. + +## Implementation Steps + +### Step 1: Add regression coverage + +`packages/trapos-core/tests/eventloop.lua`: + +- Add a test that simulates independent module loads of `/apis/eventloop.lua`. +- Create one boot-like loop and one verifier-like loop from separate module instances. +- Stop the verifier-like loop from inside its handler. +- Assert the boot-like loop still handles a later probe event. +- Explicitly stop the boot-like loop at the end so the test cannot hang. + +`packages/trapos-core/tests/eventloop.lua`: + +- Add a protected-dispatch test where one handler throws but a sibling or later handler/event still + runs. +- Assert the configured error sink receives the error. + +`packages/trapos-cloud/tests/cloud.lua`: + +- Add a daemon test where `http.websocketAsync` throws during `startSession()` or a reconnect. +- Assert the error does not propagate. +- Assert a reconnect timer is scheduled. + +Keep existing `packages/trapos-cloud/tests/cloud-program.lua` retry tests. They already prove the +program-level wrong-then-right interactive branch. + +### Step 2: Fix `eventloop.lua` + +Change `packages/trapos-core/apis/eventloop.lua`: + +- Replace module-local event-loop ID allocation with a `_G` counter. +- Add same-loop `stopRequested` handling. +- Add optional protected dispatch support for handlers and timeout callbacks if practical. At minimum, + protect regular event handlers used by boot servers. +- Preserve existing `api.STOP` unregister behavior on successful handler return. +- Preserve existing error contracts for duplicate registration and stopping an already stopped loop. + +### Step 3: Use protected boot event loop + +Change `packages/trapos-boot/startup/boot.lua`: + +- Construct `_G.bootEventLoop` with an error handler. +- Error handler should report the failure using `printError` if available, otherwise `print`. +- The boot loop should continue after a server handler error. + +### Step 4: Harden cloud reconnect + +Change `packages/trapos-cloud/apis/libcloud.lua`: + +- Forward-declare `scheduleReconnect` before `connect()`. +- Wrap `httpLike.websocketAsync(url)` with `pcall`. +- On failure, log `websocket open failed` and call `scheduleReconnect()`. +- Avoid duplicate noisy logs while already in reconnect-loop mode. + +### Step 5: Package versions + +Bump owning package versions and mirror them in `packages/index.json`: + +- `trapos-core` for `apis/eventloop.lua`. +- `trapos-boot` for `startup/boot.lua`. +- `trapos-cloud` for `apis/libcloud.lua`. + +Do not bump the full `trapos` meta-package unless repository release convention requires it for this +change set. + +## Verification + +Targeted iteration: + +```sh +just trapos-exec 'shell.run("/programs/runtest.lua", "--pretty", "/tests/eventloop.lua", "/tests/cloud.lua", "/tests/cloud-program.lua")' +``` + +Required after Lua/package edits: + +```sh +just check +just test --pretty +``` + +Manual validation on a disposable TrapOS computer: + +1. Deploy updated `trapos-core`, `trapos-boot`, and `trapos-cloud`. +2. Reboot so the cloud daemon and boot loop use the updated code. +3. Run `cloud login --force`. +4. Enter a known wrong password. +5. Confirm the retry message is printed and the password prompt returns. +6. Cancel or complete the login, then use up-arrow to confirm shell history still contains the command. +7. Enter the correct password and confirm `logged in`. +8. Run `cloud status` to confirm the daemon recovered normally. + +## Recommended Split + +Keep this as one merged fix plan, but implement it in three reviewable chunks: + +1. Event-loop stop collision and same-loop stop semantics. +2. Boot-loop protected dispatch. +3. Cloud reconnect crash-safety. + +This split makes each failure mode testable while still fixing the single user-facing bug end to end.