eae2ee08f11bab9b0da80f0eb79f9151d7e34ac7
6 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
MechaCat02
|
251d7fe3dd |
test(e2e): give each dispatcher test its own database
`dispatcher_e2e` failed about one run in three, on a different test each time, and never under `--test-threads=1`. Root cause, not timing: Each test calls `build_app`, which spawns a REAL dispatcher, and they all shared one database — so they shared one `outbox`. `OutboxRepo::claim_due` is deliberately not app-scoped (in production one dispatcher serves the whole instance, so claiming any due row is correct). Test A's dispatcher would therefore claim test B's row, test A would finish, its server would be dropped mid-dispatch, and the claim was stranded. Test B polled for its handler's side effect until it timed out. The harness was modelling something production never does: N independent instances sharing one database. So the fix belongs in the harness. Weakening the dispatcher (app-scoping `claim_due`) would be wrong, and shortening `PICLOUD_OUTBOX_CLAIM_TIMEOUT_SEC` to fit a 10s test window would make production abandon the live claims of scripts that may legitimately run 300s. `tests/common` now hands each test its own database. Replaying 73 migrations per test is too slow, so the first caller builds a migrated TEMPLATE database once and every test clones it (`CREATE DATABASE ... TEMPLATE` is a file copy), guarded by a cluster-wide advisory lock because Postgres will not clone a template that has an open connection. Names are derived from (suite, test), so a test reclaims its own database on entry: a crashed run leaves nothing to clean up. Only the five local `pool_or_skip` wrappers change — all 33 call sites are untouched, and the skip-when-`DATABASE_URL`-is-unset contract is preserved. (`#[sqlx::test]` already does this and is what `api.rs`/`authz.rs` use, but it requires `DATABASE_URL` and would force `#[ignore]`, silently dropping these tests from the default `cargo test --workspace` gate.) This also stops the e2e tests leaving stranded claims behind in the shared dev database, which is the likely source of the occasional lone journey failure. Before: dispatcher_e2e red ~1 run in 3. After: workspace green twice over, journeys 157/157. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
MechaCat02
|
80a0d31cd2 |
fix(files): make file writes transactional and close a quota bypass
Audit #6 and #8 for the last of the three stores, plus a bypass found on the way. **#6.** create/update/delete wrote the metadata row, then emitted best-effort, so an outbox failure left a committed file whose trigger never fired. `atomic_write::FilesWriter` commits the metadata row and the fan-out together. Files are the one store where the ordering is subtle, because the BYTES live on disk and cannot join a transaction: * create/update — blob first, then commit metadata + fan-out. A rollback unlinks the blob. (A crash at that exact point still orphans it; that hazard predates this change — the repo already wrote the blob and then inserted the row in a separate, failable statement — and the orphan is inert, referenced by nothing.) * delete — commit the metadata removal + fan-out FIRST, then unlink. The reverse order would destroy the bytes of a row that a rollback keeps, leaving a file that can never be read. **#8.** `GroupFilesService::create` read `total_bytes` on one connection and wrote on another, so concurrent uploads each saw the same pre-write total and together overshot the ceiling. This is the worst instance of the race in the codebase: the ceiling is DISK (10 GiB by default) and one file may be 100 MB, so a racing fleet overshoots by gigabytes. `PostgresGroupFilesWriter` takes the per-group advisory lock (on its own `files` key) across the check and the write. **The bypass.** `GroupFilesService::update` checked NO quota at all — so a 1-byte file could be updated to a 100 MB one without the ceiling ever being consulted, repeatedly, for unbounded disk. It now checks the projected total (the replaced file's bytes subtracted in SQL, so a same-size-or-smaller update near the cap still goes through). Also drive-by: `queue_e2e` asserted the ack the instant the marker appeared, but the marker is written DURING the handler and the ack happens after it returns — a zero-tolerance race. It polls now. (This does not fix the suite's flakiness, which reproduces on the pre-pass commit too.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
MechaCat02
|
05ed9b00bb |
fix(stage-6): dashboard hardening + audit Lows cherry-pick
Closes 4 dashboard hardening findings and 5 of the Lows from the audit. Dashboard hardening: - Subtabs no longer re-fetch the app via api.apps.get on every page load. users/files/dead-letters drop the fetch outright (the variable was set but never read); queues + queues/[name] now consume the layout's AppContext via getContext for the page title. The layout's reloadApp() owns the historical-slug redirect — subtab-local redirect blocks are removed so there's no race. - The global :global(details > summary::before) chevron is now scoped to details.chevron. The script editor's "Advanced sandbox" details and the inbound-email-shape help-text both opt in; the script exec-list logs no longer inherit a spurious chevron. - deriveTab now matches the path segment by anchored ===, so a future /apps/<slug>/queues-archived route wouldn't activate the queues tab. Lows cherry-pick: - ExecError gains Serialize/Deserialize derives + a snake_case tag so RemoteExecutorClient (cluster mode v1.3+) can round-trip the variant. - triggers_api rejects queue triggers whose visibility_timeout_secs is below the dispatcher's per-message executor budget; with no minimum the reclaim task races the handler and the queue silently double-delivers. Existing test using 5s updated to 30s. - New migration 0040: execution_logs.script_id cascade switched from ON DELETE CASCADE to ON DELETE SET NULL so deleting a script no longer wipes the forensic history that motivated the delete. - New migration 0041: dead_letters composite index on (app_id, created_at DESC) so the "list all" dashboard view stops falling back to seqscan + sort when unresolved=false. - Schema snapshot re-blessed. Deferred to v1.2: the ExecRequest principal serde(skip) marker (documented in-place; the cluster-mode PR will introduce the wire-safe snapshot at that point) and the `pic --help` mention of `picloud admin reset-password` (one-line follow-up). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
MechaCat02
|
649246213e |
fix(stage-3): queue DL fan-out + emit-failure visibility
Closes the queue DL fan-out gap (audit Medium) and surfaces the KV/docs/files non-transactional emit gap to operators. - handle_queue_failure previously called queue.dead_letter to write the DL row but never invoked fan_out_dead_letter. The comment claimed "the outbox arm fires registered dead_letter handlers off the new row" — but queue DL rows are written via a separate path that bypasses the outbox entirely, so handlers filtered on source="queue" sat idle forever. Refactor fan_out_dead_letter to take a DeadLetterFanOutCtx struct so both the outbox arm and the queue arm can call it; the queue arm constructs a TriggerEvent::Queue from the claimed message and passes it through. - New integration test queue_dead_letter_fans_out_to_dead_letter_handler registers a dead_letter trigger filtered on "queue", forces queue exhaustion, asserts the handler fires with the correctly-shaped event. - KV/docs/files services committed the data write then ran events.emit as a separate operation, logging-and-swallowing on failure. Bump the six call sites from tracing::warn to tracing::error with an event_emit_failure=true marker so operators can grep them. The full single-tx repo refactor (extending ServiceEventEmitter with emit_in_tx + tx-aware *_repo methods) is documented in kv_service.rs as a v1.2 follow-up — it's a meaningful redesign that deserves its own pass (pubsub_service::fan_out_publish is the reference shape). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
MechaCat02
|
c3baa87415 |
chore(v1.1.9): clippy clean — workspace -D warnings
Surfaced + fixed during the F3 attestation:
- executor-core/sdk/queue.rs: drop redundant .map(|()| ()) call sites;
enqueue_blocking discards QueueMessageId via .map(|_id| ()) (intentional)
- executor-core/sdk/retry.rs: hoist use std::collections::hash_map::DefaultHasher
+ use std::hash::Hasher to the top of the file (clippy::items_after_statements);
replace `as i64` casts with i64::try_from + clear comment about jitter
bound (clippy::cast_possible_wrap)
- executor-core/sdk/invoke.rs: move _LIMITS_IS_COPY const before #[cfg(test)]
mod tests (clippy::items_after_test_module)
- manager-core/dispatcher.rs: dispatch_one_queue gains #[allow(too_many_lines)]
(it's the queue tick's whole logic — split makes it less readable than
the lint); .map(...).unwrap_or(default) → .map_or(default, ...)
- picloud/lib.rs: Limits { trigger_depth_max, ..Limits::default() } via
struct-update instead of let-mut-assign (clippy::field_reassign_with_default)
- tests: r#"..."# → r"..." where there are no `"` inside (clippy::needless_raw_string_hashes);
combine InvokeTarget::Name | InvokeTarget::Path arms with identical bodies
in sdk_invoke.rs (clippy::match_same_arms)
cargo fmt --all -- --check 2>&1 | tail -3: no output (exit 0)
cargo clippy --workspace --all-targets --all-features -- -D warnings:
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.41s
Workspace lib smoke: 432 passing (306 manager-core + 74 executor-core + 34 shared + 18 orchestrator-core).
DB-gated E2E (against docker compose postgres on localhost:15432):
queue_e2e: 4 ok / invoke_e2e: 4 ok / retry_e2e: 3 ok / migration_queue_messages: 4 ok / schema_snapshot: 1 ok
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
MechaCat02
|
328d7d55e6 |
test(v1.1.9): queue_e2e + invoke_e2e + retry_e2e + migration test
Four new test binaries, DB-gated via the established pool_or_skip()
pattern (returns Some(pool) when DATABASE_URL is set; prints a skip
notice and returns None otherwise — so plain `cargo test` stays green
locally while CI runs them).
crates/picloud/tests/queue_e2e.rs (4 tests):
- queue_receive_acks_on_success: enqueue → consumer fires → KV
marker observable → queue row deleted (ack worked)
- queue_receive_dead_letters_after_max_attempts: throwing handler
retries 3× via the dispatcher's compute_backoff path → dead_letters
row written, queue row deleted
- queue_one_consumer_per_queue_rejected: second trigger create for the
same (app_id, queue_name) returns 4xx with the documented "already
has a consumer trigger" message (advisory-lock guard works)
- queue_visibility_timeout_reclaim: insert message with stale claim
(claimed_at = NOW() - 60s, visibility_timeout = 5s); either the
dispatcher re-claims after reclaim runs, or the claim clears
crates/picloud/tests/invoke_e2e.rs (4 tests):
- invoke_by_name_same_app_returns_value: callee returns body.x+1;
caller invokes by name and writes the result to KV (42 round-trips)
- invoke_cross_app_rejects: callee in app_B, caller in app_A; error
string contains "different app" or "CrossApp"
- invoke_depth_limit_exceeds_cleanly: recursive script throws with
"depth" in the message at the bound (Limits::trigger_depth_max=8)
- invoke_async_enqueues_outbox_row: invoke_async returns execution_id,
dispatcher fires the OutboxSourceKind::Invoke arm, callee marker
appears
crates/picloud/tests/retry_e2e.rs (3 tests):
- retry_run_eventually_succeeds_inside_http_handler: counter mutates
across 3 retries through a real HTTP route, returns 3
- retry_run_surfaces_last_error_after_max_attempts: max_attempts=2,
always throws — caller's try/catch surfaces "boom"
- retry_on_codes_filters_unmatched_errors: counter == 1 after a
throw NOT in the codes list (immediate surface, no retry)
crates/manager-core/tests/migration_queue_messages.rs (4 tests):
- queue_messages_table_exists_with_expected_columns: shape +
nullability of every column
- queue_trigger_details_table_exists: shape
- queue_widens_trigger_kind_and_outbox_source_kind: triggers.kind
CHECK admits 'queue'; outbox.source_kind admits 'invoke'
- queue_messages_dispatch_index_is_partial: idx_queue_messages_dispatch
has WHERE claim_token IS NULL (guards against future migrations
accidentally dropping the partial)
All 15 tests pass clean skip when DATABASE_URL is unset. Workspace unit
suite remains at 432 passing (306 manager-core + 74 executor-core + 34
shared + 18 orchestrator-core).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|