Skip to content

fix: datetime round-trip in _insert_cb + clearer Rust callback error (closes #36)#37

Merged
ashish-jabble merged 4 commits into
mainfrom
issue-36
May 26, 2026
Merged

fix: datetime round-trip in _insert_cb + clearer Rust callback error (closes #36)#37
ashish-jabble merged 4 commits into
mainfrom
issue-36

Conversation

@ashish-jabble
Copy link
Copy Markdown
Collaborator

@ashish-jabble ashish-jabble commented May 26, 2026

Summary

Closes #36. Two scoped commits:

  1. fix(python): parse timestamp/shipped_at back to datetime in _insert_cb — the actual bug. Row crosses Python → JSON → Rust queue → Python _insert_cb on its way through the drainer. After json.loads, timestamp / shipped_at are ISO strings; both PostgresStore.insert and SQLiteStore.insert call _utc_iso(row.timestamp) which reads dt.tzinfo and crashes with AttributeError on a str. The bare except Exception: return 1 then swallows the cause, the drainer dead-letters the row after retry-exhaustion, and the only on-disk record of the audit event is the stdout NDJSON.

    Fix reconstructs the two datetime fields from their ISO strings before constructing AuditRow. Also replaces the bare except with logger.exception so any future drainer-side failure surfaces in the fasten logger instead of being silently masked.

  2. fix(core): report CallbackFailed instead of InvalidTableName for non-zero callback rc — the diagnostic-misdirection issue. When the host-language _insert_cb returned non-zero, fasten_core was wrapping it as Error::InvalidTableName(format!("insert callback rc={rc}")). The drainer's audit_drain_failed log then rendered:

    invalid table name "insert callback rc=1":
      must match ^[A-Za-z_][A-Za-z0-9_]*(\\.[A-Za-z_][A-Za-z0-9_]*)?$
    

    — an SQL-identifier validation message run against a callback return code, which sent me ~30 min down a "what table name is invalid?" rabbit hole before realising. New Error::CallbackFailed(i32) variant carries the rc unambiguously: insert callback returned non-zero rc=1. Maps to FastenErrorCode::ErrBackend (the callback's destination is a store backend; same failure category).

Tests

  • Python — new regression in python/tests/test_engine_init.py exercises the queue-mode drainer end-to-end and asserts the store receives a datetime, not a str. Fails on main, passes on this branch. Full suite: 229 of 229 lib tests pass (the 17 store_compat parametrised cases skip without a configured Postgres DSN — unrelated).

  • Rust — 2 new tests in fasten-core/src/ffi/mod.rs::tests:

    • callback_store_failure_reports_callback_failed_variant — non-zero rc produces CallbackFailed(7), human-readable form is "insert callback returned non-zero rc=7".
    • callback_store_success_returns_ok — rc=0 returns Ok(()).

    cargo test --release --features all passes: 12 drainer_conformance + 52 lib + 2 new = 66, no regressions.

  • End-to-end — verified in a downstream adopter (fromSpec factory pinned at this branch via git URL). emit → flush → SELECT FROM fasten.audit_log returns the row instead of dead-lettering.

Compatibility

Error::InvalidTableName is unchanged — validate.rs, store/sqlite.rs:417 assertion, store/postgres.rs:493 assertion, rust/src/engine.rs:104, and the Go / Swift binding tests all continue to use it for the cases where it's actually correct (table-name regex rejection). The drainer_conformance tests' BrokenStore / FlakyStore still construct InvalidTableName("store_down") as a stand-in store-failure shape — left alone since those tests assert drainer behaviour, not the variant kind, and changing them would obscure the narrow scope of this fix.

#36)

Row crosses Python -> JSON -> Rust queue -> Python _insert_cb on its
way through the drainer. After json.loads, timestamp/shipped_at are
ISO strings; both PostgresStore and SQLiteStore call _utc_iso on them
which reads dt.tzinfo and crashes with AttributeError on a str. The
bare except Exception: return 1 then swallows the cause, the drainer
dead-letters the row after retry-exhaustion, and the only on-disk
record of the audit event is the stdout NDJSON.

Reconstruct the two datetime fields from their ISO strings before
constructing AuditRow. Replace the bare except with logger.exception
so any future drainer-side failure surfaces in the 'fasten' logger
instead of being silently masked.

Tested:
- New regression in tests/test_engine_init.py exercises the queue-mode
  drainer end-to-end and asserts the store receives a datetime, not
  a str. Fails on main, passes on this branch.
- 229 of the existing python/tests pass on this branch (17 store_compat
  parametrized Postgres cases skip without FASTEN_AUDIT_DSN).
- E2E verified against a real Postgres in a downstream adopter app:
  emit -> flush -> SELECT FROM fasten.audit_log returns the row.

Signed-off-by: ashish-jabble <ashishjabble.cs@hotmail.com>
…zero callback rc (#36)

When a host-language insert callback (Python's _insert_cb, the JS / Go
/ Swift equivalents) returns a non-zero rc, the FFI shim was wrapping
that rc into Error::InvalidTableName(format!("insert callback rc={rc}")).
The drainer's audit_drain_failed log line then renders the wrong
variant's message, so adopters see:

  invalid table name "insert callback rc=1":
    must match ^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)?$

— an SQL-identifier validation message run against the callback rc.
That misled diagnosis of the actual issue (Python-side store failure)
in #36 for ~30 min before the bare except in _insert_cb was unswallowed.

New variant Error::CallbackFailed(i32) carries the rc unambiguously:

  insert callback returned non-zero rc=1

Maps to FastenErrorCode::ErrBackend (the host callback's destination
is a store backend; the failure category is the same).

Existing tests that use Error::InvalidTableName as a stand-in store
failure in drainer_conformance.rs are unchanged — they assert drainer
behaviour, not the variant kind, and changing them would obscure the
narrow scope of this fix.

Tests in fasten-core/src/ffi/mod.rs cover both branches: rc=0 → Ok,
rc!=0 → CallbackFailed with the carried code.

Signed-off-by: ashish-jabble <ashishjabble.cs@hotmail.com>
praveen-garg
praveen-garg previously approved these changes May 26, 2026
Comment thread python/fasten/engine.py
fasten emits timestamps via datetime.isoformat() (attrs.py:to_dict)
which always produces '+00:00' format — never 'Z'. The shim only
covered hosts that hand-craft Z-suffixed timestamps, which is
outside fastens own contract. Drop it; fromisoformat handles
'+HH:MM' natively on every supported Python (3.10+).

Signed-off-by: ashish-jabble <ashishjabble.cs@hotmail.com>
This reverts commit 6259654.

Signed-off-by: ashish-jabble <ashishjabble.cs@hotmail.com>
Copy link
Copy Markdown
Contributor

@praveen-garg praveen-garg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@ashish-jabble ashish-jabble merged commit d8be08d into main May 26, 2026
3 checks passed
@ashish-jabble ashish-jabble deleted the issue-36 branch May 26, 2026 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

audit-drain ProgrammingError: 'str' has no attribute 'tzinfo' — datetime not reconstructed across Python ↔ Rust JSON boundary

2 participants