The Bug That Crashes Your Import Is the Lucky One

Migrating a 50,000-message Slack workspace to Zulip? Around message 31,000, the import dies with KeyError: 'ts'. Annoying, but that's the lucky outcome. The unlucky one: "ts": "NaN", where nothing dies, nothing warns, and your company's message history quietly comes out in the wrong order.

Zulip's Slack importer used float(message["ts"]) unguarded, both as a sort key and as date_sent. One message with a missing or malformed ts aborted the entire import; a non-finite value like "NaN" didn't even raise, silently breaking the sort. The fix (zulip/zulip#39813) skips such messages with a warning and requires ts to parse to a finite float via math.isfinite. The regression test fails with KeyError: 'ts' on the old code.

The Three Failure Modes

The code lives in zerver/data_import/slack.py. get_messages_iterator() streams every message, sorting each day's messages by timestamp:

yield from sorted(messages_for_one_day, key=get_timestamp_from_message)

where the sort key was simply:

def get_timestamp_from_message(message: ZerverFieldsT) -> float:
    return float(message["ts"])

That one line has three distinct failure modes:

  1. ts is missing: KeyError: 'ts' straight out of sorted(...). The whole import aborts because of one message.

  2. ts is garbage: "not-a-number" raises ValueError. Same total abort.

  3. ts is "NaN": The nasty one. float("NaN") parses fine, so nothing raises. But NaN is incomparable (NaN < x and x < NaN are both False), violating the total ordering Timsort assumes. sorted() silently returns an inconsistent order:

sorted(["1434139102.000002", "NaN", "1434139101.000001"], key=float)
# => ['1434139102.000002', 'NaN', '1434139101.000001']   # not sorted, no error

No exception, no warning. Just a migrated archive with scrambled chronology. Crashing is the lucky case; this is the case that costs you a re-migration three weeks later when someone notices the history reads wrong.

The Fix: Skip with Warning

The PR adds a predicate plus a guard, following the skip-with-warning pattern already in the file:

def message_has_valid_timestamp(message: ZerverFieldsT) -> bool:
    """Whether `ts` is present and parses to a finite float.
    ..."""
    try:
        return math.isfinite(float(message["ts"]))
    except (KeyError, TypeError, ValueError):
        return False

Then in the iterator:

if not message_has_valid_timestamp(message):
    logging.warning(
        "Skipping Slack message with invalid ts %r in %s",
        message.get("ts"), message_dir,
    )
    continue

The detail that matters: math.isfinite, not a bare try/except around float(). A naive "does it parse" check would fix the two loud failure modes and wave the silent one straight through.

Testing the Fix

The test had to fail first. test_get_messages_iterator_skips_invalid_timestamps creates a temp directory with one day of Slack export containing five messages: two valid (deliberately in reverse chronological order), one with no ts, one with "not-a-number", one with "NaN". Rolling the source file back to upstream/main and running the test produces KeyError: 'ts' — exactly the predicted crash. Restoring the fix, the test asserts that exactly the two valid messages survive, in correct order, with exactly three warnings logged.

Full-suite validation: ./tools/test-backend zerver.tests.test_slack_importer (56/56 passing), ./tools/lint and ./tools/run-mypy clean, and test-backend --coverage showing zero uncovered lines in zerver/data_import/slack.py after the change, including the new guard path.

The Tradeoff: Skip vs. Synthesize

One decision was explicitly left open for reviewers: skipping a message with a broken ts versus synthesizing a fallback timestamp. Skipping loses the message but keeps the archive honest; a synthetic timestamp keeps the message but fabricates chronology. The author went with skip-plus-warning because it matches the file's existing conventions, and flagged the tradeoff in the PR instead of pretending it doesn't exist.

Sentry Demo: Before and After

The author instrumented the demo with sentry-sdk (traces_sample_rate=1.0, environment bugsmash-demo) and ran the exact same poisoned export twice: once with zerver/data_import/slack.py rolled back to upstream/main, once with the fix. Same branch, same data, one file different.

Before the fix: The import span dies with KeyError: 'ts', attached right on the trace, status internal_error. Drilling into the issue gives the full stacktrace, pointing exactly where the PR points: get_messages_iterator -> get_timestamp_from_message. Sentry's Seer root-cause analysis correctly identifies the poisoned message from frame locals: {"channel_name": "general", "text": "no ts field"}.

One honest note: Seer's suggested one-liner, float(message.get("ts", 0)), is the fallback-timestamp option the author explicitly declined. It fixes the missing-ts mode, but leaves the ValueError mode alive, waves "NaN" straight through into the sort, and stamps real messages with a 1970 date_sent. Its closing advice — log a warning and investigate upstream data quality — is exactly what the shipped fix does.

After the fix: Same export, same transaction: zero issues, and the three skipped messages surface as three warning-level logs on the trace instead of one fatal.

That pair of traces is the whole fix, told by monitoring: the same bad input, downgraded from an unhandled exception that kills a migration to three structured warnings on a completed run. (Both runs report release c0b5d8c because the red run rolled back only the module file, not the branch HEAD.)

The Uncomfortable Takeaway

"Does it crash" is a terrible proxy for "is it correct". The failure modes that skip the crash are the ones that survive into production. For a message that is otherwise fine but has a broken timestamp, would you skip it or synthesize a fallback date_sent? The author picked skip. Convince them otherwise in the comments.

Next Steps

If you're building importers or data pipelines, audit every unguarded float() cast. Look for non-finite values, not just missing keys. Use math.isfinite when parsing user-supplied or external data. And write regression tests that fail on the old code — otherwise you're testing the fix, not the bug.