chore: save latest plans
This commit is contained in:
parent
1d4789be46
commit
4f7967513f
190
.plans/cloud-login-interactive-shell-history-fix.md
Normal file
190
.plans/cloud-login-interactive-shell-history-fix.md
Normal file
@ -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/<id>`.
|
||||
- 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/<id>` 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.
|
||||
129
.plans/cloud-login-session-crash-fix-plan.md
Normal file
129
.plans/cloud-login-session-crash-fix-plan.md
Normal file
@ -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).
|
||||
215
.plans/cloud-login-session-crash-merged-fix-plan-final.md
Normal file
215
.plans/cloud-login-session-crash-merged-fix-plan-final.md
Normal file
@ -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/<id>`
|
||||
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.
|
||||
190
.plans/cloud-login-session-crash-merged-fix-plan.md
Normal file
190
.plans/cloud-login-session-crash-merged-fix-plan.md
Normal file
@ -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/<id>`.
|
||||
|
||||
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.
|
||||
Loading…
Reference in New Issue
Block a user