---
phase: 49-security-correctness-hardening
plan: 01
type: execute
wave: 1
depends_on: []
files_modified:
  - src/catalog.rs
  - src/expand/types.rs
  - src/ddl/alter.rs
  - src/ddl/drop.rs
  - src/ddl/get_ddl.rs
  - src/ddl/list.rs
  - src/ddl/show_facts.rs
  - src/ddl/show_dims.rs
  - src/ddl/show_metrics.rs
  - src/ddl/show_columns.rs
  - src/ddl/show_dims_for_metric.rs
  - src/ddl/describe.rs
  - src/ddl/define.rs
  - src/query/table_function.rs
  - src/query/explain.rs
  - src/lib.rs
autonomous: true
requirements:
  - SEC-02
  - SEC-04

must_haves:
  truths:
    - "A poisoned RwLock in the catalog returns a descriptive error instead of panicking"
    - "A poisoned Mutex in query streaming returns a descriptive error instead of panicking"
    - "Out-of-bounds row_idx in test helper triggers a debug assertion rather than undefined behavior"
    - "All existing tests continue to pass (no behavioral change for non-poisoned paths)"
  artifacts:
    - path: "src/catalog.rs"
      provides: "Lock acquisition via map_err instead of .unwrap()"
      contains: "map_err"
    - path: "src/expand/types.rs"
      provides: "CatalogPoisoned error variant"
      contains: "CatalogPoisoned"
    - path: "src/lib.rs"
      provides: "Bounds assertions in read_typed_value"
      contains: "debug_assert!"
  key_links:
    - from: "src/catalog.rs"
      to: "all VTab bind() methods"
      via: "catalog_insert/delete/upsert/rename return Result on poisoned lock"
      pattern: "map_err.*poisoned"
    - from: "src/query/table_function.rs"
      to: "src/expand/types.rs"
      via: "ExpandError or QueryError variant for poisoned lock"
      pattern: "CatalogPoisoned|LockPoisoned"
---

<objective>
Replace all panic-on-lock-poison patterns (.unwrap() / .expect() on RwLock/Mutex acquisitions) with graceful error returns across the entire codebase, and add bounds checks to test helper unsafe pointer arithmetic.

Purpose: Eliminate the two main categories of preventable panics: lock poisoning (which causes cascading panics across threads) and out-of-bounds pointer access in test helpers.

Output: All lock acquisitions return errors via Result instead of panicking. Test helper has debug assertions guarding pointer arithmetic.
</objective>

<execution_context>
@$HOME/.claude/get-shit-done/workflows/execute-plan.md
@$HOME/.claude/get-shit-done/templates/summary.md
</execution_context>

<context>
@.planning/PROJECT.md
@.planning/ROADMAP.md
@.planning/STATE.md
@src/catalog.rs
@src/expand/types.rs
@src/query/table_function.rs
@src/ddl/alter.rs
@src/ddl/get_ddl.rs
@src/lib.rs

<interfaces>
<!-- Key types the executor needs -->

From src/catalog.rs:
```rust
pub type CatalogState = Arc<RwLock<HashMap<String, String>>>;

pub fn catalog_insert(state: &CatalogState, name: &str, json: &str) -> Result<(), Box<dyn std::error::Error>>;
pub fn catalog_delete(state: &CatalogState, name: &str) -> Result<(), Box<dyn std::error::Error>>;
pub fn catalog_upsert(state: &CatalogState, name: &str, json: &str) -> Result<(), Box<dyn std::error::Error>>;
pub fn catalog_delete_if_exists(state: &CatalogState, name: &str);
pub fn catalog_rename(state: &CatalogState, old_name: &str, new_name: &str) -> Result<(), Box<dyn std::error::Error>>;
```

From src/expand/types.rs:
```rust
pub enum ExpandError {
    EmptyRequest { view_name: String },
    UnknownDimension { ... },
    // ... other variants
    WindowMetricRequiredDimension { ... },
}
```
</interfaces>
</context>

<tasks>

<task type="auto" tdd="true">
  <name>Task 1: Replace lock .unwrap()/.expect() with error returns across all modules</name>
  <files>src/catalog.rs, src/expand/types.rs, src/ddl/alter.rs, src/ddl/drop.rs, src/ddl/get_ddl.rs, src/ddl/list.rs, src/ddl/show_facts.rs, src/ddl/show_dims.rs, src/ddl/show_metrics.rs, src/ddl/show_columns.rs, src/ddl/show_dims_for_metric.rs, src/ddl/describe.rs, src/ddl/define.rs, src/query/table_function.rs, src/query/explain.rs</files>
  <read_first>src/catalog.rs, src/expand/types.rs, src/query/table_function.rs, src/query/explain.rs, src/ddl/alter.rs, src/ddl/drop.rs, src/ddl/get_ddl.rs, src/ddl/list.rs, src/ddl/show_facts.rs, src/ddl/show_dims.rs, src/ddl/show_metrics.rs, src/ddl/show_columns.rs, src/ddl/show_dims_for_metric.rs, src/ddl/describe.rs, src/ddl/define.rs</read_first>
  <behavior>
    - Test 1: catalog_insert on a poisoned RwLock returns Err containing "poisoned" (not panic)
    - Test 2: catalog_delete on a poisoned RwLock returns Err containing "poisoned" (not panic)
    - Test 3: catalog_upsert on a poisoned RwLock returns Err containing "poisoned" (not panic)
    - Test 4: catalog_delete_if_exists on a poisoned RwLock does not panic (returns nothing, but does not crash)
    - Test 5: catalog_rename on a poisoned RwLock returns Err containing "poisoned" (not panic)
    - Test 6: All existing cargo test tests pass unchanged
  </behavior>
  <action>
**Step 1: Add error variant to ExpandError** (src/expand/types.rs)

Add a new variant to the `ExpandError` enum:
```rust
/// The catalog RwLock is poisoned (a previous thread panicked while holding the lock).
CatalogPoisoned { view_name: String },
```

Add its Display implementation in the match arm:
```rust
Self::CatalogPoisoned { view_name } => {
    write!(
        f,
        "semantic view '{view_name}': internal error -- catalog lock is poisoned \
         (a previous operation panicked). Restart DuckDB to recover."
    )
}
```

**Step 2: Replace lock .unwrap()/.expect() in catalog.rs**

In `catalog_insert` (line 103): Replace `state.write().unwrap()` with:
```rust
let mut guard = state.write().map_err(|_| {
    Box::<dyn std::error::Error>::from("catalog lock poisoned")
})?;
```

Apply the same pattern to:
- `catalog_delete` (line 121): `state.write().unwrap()` -> `state.write().map_err(...)?`
- `catalog_upsert` (line 141-144): `state.write().unwrap()` -> `state.write().map_err(...)?`
- `catalog_delete_if_exists` (line 152): This function currently returns nothing. Change signature to return `Result<(), Box<dyn std::error::Error>>` is not feasible without changing callers. Instead, use `if let Ok(mut guard) = state.write() { guard.remove(name); }` — silently skip on poisoned lock (the delete-if-exists semantic is already best-effort).
- `catalog_rename` (line 164): `state.write().unwrap()` -> `state.write().map_err(...)?`

**Step 3: Replace lock .unwrap()/.expect() in VTab bind() methods**

All these VTab bind methods return `Result<Self::BindData, Box<dyn std::error::Error>>`, so we can use `?` directly.

Pattern to apply everywhere:
- Replace: `state.catalog.read().unwrap()` or `.expect("catalog RwLock poisoned")`
- With: `state.catalog.read().map_err(|_| Box::<dyn std::error::Error>::from("catalog lock poisoned"))?`

For VTabs that read catalog via raw pointer (`unsafe { (*state_ptr).read().expect(...) }`):
- Replace: `unsafe { (*state_ptr).read().expect("catalog RwLock poisoned") }`
- With: `unsafe { (*state_ptr).read().map_err(|_| Box::<dyn std::error::Error>::from("catalog lock poisoned"))? }`

Apply to each file:
- **src/ddl/alter.rs**: Lines 91, 106, 219 — three `.unwrap()` calls on `state.catalog.read()`
- **src/ddl/drop.rs**: Line 81 — `state.catalog.read().unwrap()`
- **src/ddl/get_ddl.rs**: Line 45 — `state.read().unwrap()`
- **src/ddl/list.rs**: Lines 76, 204 — `(*state_ptr).read().expect("catalog RwLock poisoned")`
- **src/ddl/show_facts.rs**: Lines 154, 201 — `(*state_ptr).read().expect(...)`
- **src/ddl/show_dims.rs**: Lines 154, 201 — `(*state_ptr).read().expect(...)`
- **src/ddl/show_metrics.rs**: Lines 156, 203 — `(*state_ptr).read().expect(...)`
- **src/ddl/show_columns.rs**: Line 146 — `(*state_ptr).read().expect(...)`
- **src/ddl/show_dims_for_metric.rs**: Line 183 — `(*state_ptr).read().expect(...)`
- **src/ddl/describe.rs**: Find the `.expect("catalog RwLock poisoned")` call and replace
- **src/ddl/define.rs**: Find any catalog read lock in resolve_pk_from_catalog and replace
- **src/query/table_function.rs**: Line 431 — `state.catalog.read().expect("catalog RwLock poisoned")`; Line 657 — `init_data.inner.lock().unwrap()` (Mutex)
- **src/query/explain.rs**: Line 147 — `state.catalog.read().expect("catalog RwLock poisoned")`

For the Mutex in table_function.rs line 657:
```rust
let mut guard = init_data.inner.lock().map_err(|_| {
    Box::<dyn std::error::Error>::from("query state lock poisoned")
})?;
```

**Step 4: Write unit tests for poisoned lock behavior in catalog.rs**

Add these tests to `catalog.rs` mod tests:
```rust
#[test]
fn catalog_insert_poisoned_lock_returns_error() {
    let state: CatalogState = Arc::new(RwLock::new(HashMap::new()));
    // Poison the lock by panicking in a write guard
    let state_clone = state.clone();
    let _ = std::thread::spawn(move || {
        let _guard = state_clone.write().unwrap();
        panic!("intentional poison");
    }).join();
    // Lock is now poisoned
    let result = catalog_insert(&state, "test", r#"{"tables":[],"joins":[],"dimensions":[],"metrics":[]}"#);
    assert!(result.is_err());
    let err_msg = result.unwrap_err().to_string();
    assert!(err_msg.contains("poisoned"), "Expected poisoned error, got: {err_msg}");
}

#[test]
fn catalog_delete_poisoned_lock_returns_error() {
    let state: CatalogState = Arc::new(RwLock::new(HashMap::new()));
    let state_clone = state.clone();
    let _ = std::thread::spawn(move || {
        let _guard = state_clone.write().unwrap();
        panic!("intentional poison");
    }).join();
    let result = catalog_delete(&state, "test");
    assert!(result.is_err());
    assert!(result.unwrap_err().to_string().contains("poisoned"));
}

#[test]
fn catalog_rename_poisoned_lock_returns_error() {
    let state: CatalogState = Arc::new(RwLock::new(HashMap::new()));
    let state_clone = state.clone();
    let _ = std::thread::spawn(move || {
        let _guard = state_clone.write().unwrap();
        panic!("intentional poison");
    }).join();
    let result = catalog_rename(&state, "old", "new");
    assert!(result.is_err());
    assert!(result.unwrap_err().to_string().contains("poisoned"));
}
```

**Important:** Do NOT use `into_inner()` recovery on poisoned locks. The user decision is explicit: return error on poisoned lock.
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo test 2>&1 | tail -5</automated>
  </verify>
  <acceptance_criteria>
    - grep -c '\.unwrap()' src/catalog.rs returns 0 (no .unwrap() on lock acquisitions — only on non-lock operations like JSON parsing which already have proper error paths)
    - grep -rn '\.expect.*poisoned' src/ddl/ src/query/ returns 0 matches
    - grep -rn '\.unwrap()' src/catalog.rs does not match any line containing '.write()' or '.read()'
    - grep 'CatalogPoisoned' src/expand/types.rs returns at least 1 match
    - grep 'map_err.*poisoned' src/catalog.rs returns at least 4 matches
    - cargo test -- catalog_insert_poisoned passes
    - cargo test -- catalog_delete_poisoned passes
    - cargo test -- catalog_rename_poisoned passes
    - cargo test passes with 0 failures
  </acceptance_criteria>
  <done>All RwLock/Mutex .unwrap()/.expect() calls in catalog.rs and all VTab bind/init/func methods replaced with .map_err() error returns. Poisoned lock tests pass. All existing tests unchanged.</done>
</task>

<task type="auto">
  <name>Task 2: Add bounds checks to test helper unsafe pointer arithmetic</name>
  <files>src/lib.rs</files>
  <read_first>src/lib.rs</read_first>
  <action>
In `src/lib.rs`, the `read_typed_value` function (starts around line 83, under `#[cfg(test)]`) performs raw pointer arithmetic like `data_ptr.cast::<i32>().add(row_idx)` without verifying that `row_idx` is within bounds of the data chunk.

Add bounds-checking assertions at the top of `read_typed_value`, immediately after the function signature opens (before the `use ffi::...` block):

```rust
pub unsafe fn read_typed_value(
    chunk: ffi::duckdb_data_chunk,
    col_idx: usize,
    row_idx: usize,
    type_id: u32,
    logical_type: ffi::duckdb_logical_type,
) -> TestValue {
    // SEC-04: Guard against out-of-range row indices in test helpers.
    let row_count = ffi::duckdb_data_chunk_get_size(chunk) as usize;
    debug_assert!(
        row_idx < row_count,
        "read_typed_value: row_idx {row_idx} out of bounds (chunk has {row_count} rows)"
    );
    let col_count = ffi::duckdb_data_chunk_get_column_count(chunk) as usize;
    debug_assert!(
        col_idx < col_count,
        "read_typed_value: col_idx {col_idx} out of bounds (chunk has {col_count} columns)"
    );
    // ... rest of function unchanged
```

Also add a bounds check inside the LIST child iteration loop (around line 165), before the child element access:

```rust
for i in 0..length {
    let child_row = offset + i;
    debug_assert!(
        child_row < ffi::duckdb_list_vector_get_size(child_vec) as usize
            || true, // list vector size API may not exist; skip runtime check if unavailable
        "read_typed_value: list child_row {child_row} out of bounds"
    );
    // ... existing code
```

Note: If `duckdb_list_vector_get_size` does not exist in the FFI bindings, omit the LIST child bounds check — only the top-level row_idx and col_idx checks are critical. Verify the available FFI API by searching for `duckdb_data_chunk_get_size` and `duckdb_data_chunk_get_column_count` in libduckdb_sys.

The `debug_assert!` macro is used (not `assert!`) because:
1. These are `#[cfg(test)]` only functions — they run in test builds where debug_assert is active
2. In release mode (extension binary), this code is not compiled at all (`#[cfg(test)]`)
  </action>
  <verify>
    <automated>cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && cargo test 2>&1 | tail -5</automated>
  </verify>
  <acceptance_criteria>
    - grep 'debug_assert!' src/lib.rs returns at least 2 matches (row_idx and col_idx checks)
    - grep 'row_idx < row_count' src/lib.rs returns 1 match
    - grep 'col_idx < col_count' src/lib.rs returns 1 match
    - cargo test passes with 0 failures
  </acceptance_criteria>
  <done>Test helper read_typed_value has debug_assert bounds checks on row_idx and col_idx. All existing tests pass (bounds are respected in current test code).</done>
</task>

</tasks>

<threat_model>
## Trust Boundaries

| Boundary | Description |
|----------|-------------|
| Catalog lock acquisition | Any thread can poison the lock; all subsequent acquisitions must handle gracefully |
| Test helper FFI | Raw pointer arithmetic on DuckDB data chunks; out-of-bounds = undefined behavior |

## STRIDE Threat Register

| Threat ID | Category | Component | Disposition | Mitigation Plan |
|-----------|----------|-----------|-------------|-----------------|
| T-49-01 | D (Denial of Service) | catalog.rs lock acquisition | mitigate | Replace .unwrap() with .map_err() returning descriptive error string |
| T-49-02 | D (Denial of Service) | query/table_function.rs Mutex lock | mitigate | Replace .unwrap() with .map_err() on Mutex::lock() |
| T-49-03 | T (Tampering) | lib.rs test helper pointer arithmetic | mitigate | Add debug_assert! bounds checks on row_idx and col_idx |
</threat_model>

<verification>
```bash
# All tests pass (full quality gate per CLAUDE.md)
cd /Users/paul/Documents/Dev/Personal/duckdb-semantic-views && just test-all

# No .unwrap()/.expect() on lock acquisitions remain
grep -rn '\.write()\.unwrap()\|\.read()\.unwrap()\|\.lock()\.unwrap()\|\.expect.*poisoned' src/catalog.rs src/ddl/ src/query/ | grep -v '#\[cfg(test)\]' | grep -v 'mod tests'

# Bounds checks present
grep -c 'debug_assert!' src/lib.rs
```
</verification>

<success_criteria>
- Zero .unwrap() or .expect() calls on RwLock/Mutex acquisitions in production code paths
- All lock poisoning scenarios return descriptive errors (not panics)
- Test helper has bounds assertions
- just test-all passes (including sqllogictest)
- New poisoned-lock tests pass
</success_criteria>

<output>
After completion, create `.planning/phases/49-security-correctness-hardening/49-01-SUMMARY.md`
</output>
