sqlite 'cannot start tx within tx'

[noble plover]

my branch isn't rebased on main for the latest sqlite tweaks, but this is running from CI, and I'm not sure if it runs against a merged HEAD with our current setup

[wise kestrel]

I didn't think it did a merge, but not 100% sure

[noble plover]

trying to figure that out still

it's this job, i believe: https://github.com/dagger/dagger/actions/runs/19452980194/job/55661370054?pr=11434

i think it did - the build ran with a7762546b7a0c85b0e768117aca4d177aeec5fa5 which is:

  HEAD is now at a776254 Merge bbd092a8c6b8f2e7ddf2b231d074ad1f92b5c228 into 3807ba08e53f9506a02bbfc564225ff16a571b63

it's on this line:

    tx, err := ps.client.db.Begin()
    if err != nil {
        return fmt.Errorf("export spans %+v: begin tx: %w", spanNames(spans), err)
    }
    defer tx.Rollback()

engine/server/telemetry.go:407

maybe these DBs aren't safe for concurrent transactions...? i find myself asking how does anyone use sqlite again

[wise kestrel]

is there are a strong need for a transaction there? and in the other log inserts that use a transaction

[noble plover]

i remember going back and forth on this, lemme go spelunking in commit history, vaguely remember removing all the transactions to try to fix SQLITE_BUSY but it didn't help so i brought them back.

gut instinct is we don't really need them though

https://github.com/dagger/dagger/pull/8861 (removed them)
=>
https://github.com/dagger/dagger/pull/8950 (brought them back after finding the "right" fix)

This didn't help the user facing begin tx: database is locked at all - they just started to see context deadline exceeded instead.
...but that context deadline exceeded may have been fixed by the common root cause and unrelated to removing transactions

so, i'd guess we can just remove them

https://tenor.com/bJ8zA.gif

[wise kestrel]

Okay yeah may as well rm it, though still confused why this would even happen

[noble plover]

yeah i'm a little surprised. Is there only one connection across everything, and it enters into 'transaction mode'? Feels like a leaky *sql.DB abstraction or something, what's the point of transactions if you can't do them concurrently lol

i'll put up a PR to remove them

is it interesting that metrics already don't use a transaction?

[wise kestrel]

they are just a single insert, so it would have been redundant i think

[noble plover]

ah true

https://github.com/dagger/dagger/pull/11439

[wise kestrel]

gpt-5.1 thinking said it sounds like a modernc concurrency bug after giving it all the relevant code 😄 don't know if I believe it but at least we're not crazy in being confused