This addressed a deprecation warning on nightly (will be in Rust 1.38).
Use parking_lot instead, which in theory is faster (although I doubt
it's significant here).
Now the test actually has a recording and garbage with matching files.
This caught a few problems in the upgrade procedure:
* it didn't work with foreign keys enabled because the new recording
table was set up after the new camera table, and the old recording
table was destroyed after the old camera table. And now I enable
foreign keys all the time. Reorder the procedure to fix.
* the pathname manipulation in the v2 to v3 procedure was incorrect
since my introduction of nix because I gave it a &[u8] with the
trailing nul, where I should have used CStr::from_bytes_with_nul.
* it wasn't removing garbage files. It'd be most natural to do this
in the v2 to v3 upgrade (with the rename) but I historically removed
the table when upgrading to v2. I can't redefine the schema now, so
do it unnaturally.
I'm considering also renaming all uuid-like files on upgrade to v4/v5
to clean up this mess automatically for installations that have
already done this upgrade.
The immediate motivation is that Cargo.lock referred to a commit version
in a PR branch of my nix fork that no longer exists. (I didn't know, but
it makes sense, that "git push -f" not only forcibly updates the branch
to refer to a new commit but also gets rid of orphaned commits.) Use a
moonfire branch that I'll keep stable until I'm ready to move on.
I also updated parking_lot and rusqlite to new major versions (nothing
in the interface that I care about has changed) and did a full cargo
update.
This is nicer in a few ways:
* I can use openat so there's no possibility of any kind of a race
involving scanning a different directory than the one used in
other ways (locking, metadata file, adding/removing sample files)
* filename() doesn't need to allocate memory, so it's a bit more
efficient
* dogfooding - I wrote nix::dir.
Add a new schema version 5; now 4 means the directory meta may or may
not be upgraded.
Fixes#65: now it's possible to open the directory even if it lies on a
completely full disk.
Newer SQLite library versions (such as what you get when using
--features=bundled) actually enforce foreign keys. Unfortunately there's
no way to drop foreign key constraints, so you have to transitively
recreate all the tables with foreign key constraints on the table you're
recreating.
My dad's "GW-GW4089IP" cameras use separate ports for the main and sub
streams:
rtsp://192.168.1.110:5050/H264?channel=0&subtype=0&unicast=true&proto=Onvif
rtsp://192.168.1.110:5049/H264?channel=0&subtype=1&unicast=true&proto=Onvif
Previously I could get one of the streams to work by including :5050 or
:5049 in the host field of the camera. But not both. Now make the
camera's host field reflect the ONVIF port (which is also non-standard
on these cameras, :85). It's not directly used yet but probably will be
sooner or later. Make each stream know its full URL.
(I also considered the names "capabilities" and "scopes", but I think
"permissions" is the most widely understood.)
This is increasingly necessary as the web API becomes more capable.
Among other things, it allows:
* non-administrator users who can view but not access camera passwords
or change any state
* workers that update signal state based on cameras' built-in motion
detection or a security system's events but don't need to view videos
* control over what can be done without authenticating
Currently session permissions are just copied from user permissions, but
you can also imagine admin sessions vs not, as a checkbox when signing
in. This would match the standard Unix workflow of using a
non-administrative session most of the time.
Relevant to my current signals work (#28) and to the addition of an
administrative API (#35, including #66).
This is a definite work in progress. In particular,
* there's no src/web.rs support yet so it can't be used,
* the code is surprisingly complex, and there's almost no tests so far.
I want to at least get complete branch coverage.
* I may still go back to time_sec rather than time_90k to save RAM and
flash.
I simplified the approach a bit from the earlier goal in design/api.md.
In particular, there's no longer the separate concept of "observation"
vs "prediction". Now the predictions are just observations that extend a
bit beyond now. They may be flushed prematurely and I'll try living with
that to avoid making things even more complex.
This is mostly untested and useless by itself, but it's a starting
point. In particular:
* there's no way to set up signals or add/remove/update events yet
except by manual changes to the database.
* if you associate a signal with a camera then remove the camera,
hitting /api/ will error out.
My installation recently somehow ended up with a recording with a
duration of 503793844 90,000ths of a second, way over the maximum of 5
minutes. (Looks like the machine was pretty unresponsive at the time
and/or having network problems.)
When this happens, the system really spirals. Every flush afterward (12
per minute with my installation) fails with a CHECK constraint failure
on the recording table. It never gives up on that recording. /var/log
fills pretty quickly as this failure is extremely verbose (a stack
trace, and a line for each byte of video_index). Eventually the sample
file dirs fill up too as it continues writing video samples while GC is
stuck. The video samples are useless anyway; given that they're not
referenced in the database, they'll be deleted on next startup.
This ensures the offending recording is never added to the database, so
we don't get the same persistent problem. Instead, writing to the
recording will fail. The stream will drop and be retried. If the
underlying condition that caused a too-long recording (many
non-key-frames, or the camera returning a crazy duration, or the
monotonic clock jumping forward extremely, or something) has gone away,
the system should recover.
This is so far completely untested, for use by a new UI prototype.
It creates a new URL endpoint which sends one video/mp4 media segment
per key frame, with the dependent frames included. This means there will
be about one key frame interval of latency (typically about a second).
This seems hard to avoid, as mentioned in issue #59.
I went with the third idea in 1ce52e3: have the tests run each iteration
of the syncer explicitly. These are messy tests that know tons of
internal details, but I think they're less confusing and racy than if I
had the syncer running in a separate thread.
Now each syncer has a binary heap of the times it plans to do a flush.
When one of those times arrives, it rechecks if there's something to do.
Seems more straightforward than rechecking each stream's first
uncommitted recording, especially with the logic to retry failed flushes
every minute.
Also improved the info! log for each flush to see the actual recordings
being flushed for better debuggability.
No new tests right now. :-( They're tricky to write. One problem is that
it's hard to get the timing right: a different flush has to happen
after Syncer::save's database operations and before Syncer::run calls
SimulatedClocks::recv_timeout with an empty channel[*], advancing the
time. I've thought of a few ways of doing this:
* adding a new SyncerCommand to run something, but it's messy (have
to add it from the mock of one of the actions done by the save),
and Box<dyn FnOnce() + 'static> not working (see
rust-lang/rust#28796) makes it especially annoying.
* replacing SimulatedClocks with something more like MockClocks.
Lots of boilerplate. Maybe I need to find a good general-purpose
Rust mock library. (mockers sounds good but I want something that
works on stable Rust.)
* bypassing the Syncer::run loop, instead manually running iterations
from the test.
Maybe the last way is the best for now. I'm likely to try it soon.
[*] actually, it's calling Receiver::recv_timeout directly;
Clocks::recv_timeout is dead code now? oops.
* remove intermediate bool from adjust_day.
* rewrite LockedDatabase::list_aggregate_recordings.
I started by collapsing the flush into the first part of the if, in a
similar way to adjust_day. But then I refactored more and ended up
with a structure that probably would have been allowed with the old
lexical borrow checker. I think it's more readable, and it does 1
btree operation per row where before it did 2 or 3.
This is mostly just "cargo fix --edition" + Cargo.toml changes.
There's one fix for upgrading to NLL in db/writer.rs:
Writer::previously_opened wouldn't build with NLL because of a
double-borrow the previous borrow checker somehow didn't catch.
Restructure to avoid it.
I'll put elective NLL changes in a following commit.
I initially chose SameSite=Lax because I thought if a user followed a
link to the landing page, the landing page's ajax requests wouldn't send
the cookie. But I just did an experiment, and that's not true. Only the
initial page load (of a .html file) lacks the cookie. All of its
resources and ajax requests send the cookie. I'm not sure about
document.cookie accesses, but my cookie is HttpOnly anyway, so it's
irrelevant. So no reason to be lax.
I just ran a "cargo test" on this after a round of tweaks, not
"cargo test --all", so I missed compile errors in the db crate,
and a Javascript lint config error. travis-ci caught these.
Some caveats:
* it doesn't record the peer IP yet, which makes it harder to verify
sessions are valid. This is a little annoying to do in hyper now
(see hyperium/hyper#1410). The direct peer might not be what we want
right now anyway because there's no TLS support yet (see #27). In
the meantime, the sane way to expose Moonfire NVR to the Internet is
via a proxy server, and recording the proxy's IP is not useful.
Maybe better to interpret a RFC 7239 Forwarded header (and/or
the older X-Forwarded-{For,Proto} headers).
* it doesn't ever use Secure (https-only) cookies, for a similar reason.
It's not safe to use even with a tls proxy until this is fixed.
* there's no "moonfire-nvr config" support for inspecting/invalidating
sessions yet.
* in debug builds, logging in is crazy slow. See libpasta/libpasta#9.
Some notes:
* I removed the Javascript "no-use-before-defined" lint, as some of
the functions form a cycle.
* Fixed#20 along the way. I needed to add support for properly
returning non-OK HTTP statuses to signal unauthorized and such.
* I removed the Access-Control-Allow-Origin header support, which was
at odds with the "SameSite=lax" in the cookie header. The "yarn
start" method for running a local proxy server accomplishes the same
thing as the Access-Control-Allow-Origin support in a more secure
manner.
This is a minor code size reduction - instead of being monomorphized
into four variants (according to "cargo llvm-lines"), it's now
monomorphized into two. The stripped release binary on macOS is about
8kB smaller (0.15%). Not a huge improvement but better than nothing.
Benchmarks seem unchanged (though they have a lot of variance).
There was a race condition here because it wasn't waiting for the db
flush to complete. This made write_path_retries sometimes not reflect
the consequence of the flush, causing an assertion failure. I assume it
was also responsible for gc_path_retries timeouts under travis-ci.
Scott Lamb